Merge pull request #12475 from asselitx/hpcc-19831

HPCC-19831 Store DESDL definitions as blob to preserve element order

Reviewed-By: Anthony Fishbeck <anthony.fishbeck@lexisnexis.com>
Reviewed-By: Richard Chapman <rchapman@hpccsystems.com>

esp/services/esdl_svc_engine/esdl_store.cpp

@@ -33,6 +33,8 @@
 static const char* ESDL_DEFS_ROOT_PATH="/ESDL/Definitions/";
 static const char* ESDL_DEF_PATH="/ESDL/Definitions/Definition";
 static const char* ESDL_DEF_ENTRY="Definition";
+static const char* ESDL_DEF_CONTENT_PTREE="esxdl";
+static const char* ESDL_DEF_CONTENT_STR="content";
 static const char* ESDL_BINDINGS_ROOT_PATH="/ESDL/Bindings/";
 static const char* ESDL_BINDING_PATH="/ESDL/Bindings/Binding";
 static const char* ESDL_BINDING_ENTRY="Binding";
@@ -41,6 +43,75 @@ static const char* ESDL_METHODS_ENTRY="Methods";
 
 extern bool trimXPathToParentSDSElement(const char *element, const char * xpath, StringBuffer & parentNodeXPath);
 
+class CEsdlDefinitionInfo : implements IEsdlDefinitionInfo, public CInterface
+{
+    friend class CEsdlSDSStore;
+
+private:
+    typedef MapStringTo<StringArray*> MapStringToStringArray;
+
+    MapStringToStringArray m_serviceMethodMap;
+    StringArray m_services;
+    Owned<IProperties> m_metadata = createProperties();
+
+public:
+    IMPLEMENT_IINTERFACE;
+
+    ~CEsdlDefinitionInfo()
+    {
+        HashIterator iter(m_serviceMethodMap);
+        ForEach(iter)
+        {
+            StringArray* arr = *m_serviceMethodMap.mapToValue(&iter.query());
+            delete arr;
+        }
+    }
+
+    virtual const IProperties& getMetadata() override
+    {
+        return *m_metadata;
+    }
+
+    virtual const StringArray& getServices() override
+    {
+        return m_services;
+    }
+
+    virtual const StringArray* queryMethods(const char* service) override
+    {
+        StringArray** methods = m_serviceMethodMap.getValue(service);
+        if( methods )
+            return *(methods);
+        else
+            return nullptr;
+    }
+
+private:
+    void setMetadataValue(const char* name, const char* value)
+    {
+        m_metadata->setProp(name, value);
+    }
+
+    void ensureServiceExists(const char* service)
+    {
+        if( m_services.contains(service) == false )
+            m_services.append(service);
+        if( m_serviceMethodMap.find(service) == NULL )
+            m_serviceMethodMap.setValue(service, new StringArray);
+    }
+
+    void ensureServiceMethod(const char* service, const char* method)
+    {
+        if( m_serviceMethodMap.find(service) == NULL )
+        {
+            ensureServiceExists(service);
+        }
+
+        StringArray* methods = *m_serviceMethodMap.getValue(service);
+        methods->append(method);
+    }
+};
+
 class CEsdlSDSStore : implements IEsdlStore, public CInterface
 {
 private:
@@ -86,50 +157,104 @@ public:
         throw MakeStringException(-1, "ESDS store is not attached to dali");
     }
 
-    virtual IPropertyTree* fetchDefinition(const char* definitionId) override
+
+    unsigned fetchLatestSeqForDefinitionName(const char* definitionName)
+    {
+        unsigned latestSeq = 1;
+        Owned<IRemoteConnection> conn = checkQuerySDS().connect(ESDL_DEFS_ROOT_PATH, myProcessSession(), RTM_LOCK_READ, SDS_LOCK_TIMEOUT_DESDL);
+        if (!conn)
+            throw MakeStringException(-1, "Unable to connect to ESDL Service definition information in dali '%s'", ESDL_DEFS_ROOT_PATH);
+
+        IPropertyTree * esdlDefinitions = conn->queryRoot();
+
+        if (esdlDefinitions)
+        {
+            VStringBuffer xpath("%s[@name='%s']", ESDL_DEF_ENTRY, definitionName);
+            Owned<IPropertyTreeIterator> iter = esdlDefinitions->getElements(xpath.str());
+            ForEach(*iter)
+            {
+                IPropertyTree &item = iter->query();
+                unsigned thisSeq = item.getPropInt("@seq");
+                if (thisSeq > latestSeq)
+                    latestSeq = thisSeq;
+            }
+        } else
+            throw MakeStringException(-1, "Unable to fetch ESDL definition '%s' from dali", definitionName);
+
+        return latestSeq;
+    }
+
+    void readServiceMethodInfo(CEsdlDefinitionInfo* defInfo, IPropertyTree* defTree)
+    {
+        Owned<IPropertyTreeIterator> serviceiter = defTree->getElements("EsdlService");
+        ForEach(*serviceiter)
+        {
+            IPropertyTree &curservice = serviceiter->query();
+            const char* serviceName = curservice.queryProp("@name");
+
+            Owned<IPropertyTreeIterator> methoditer = curservice.getElements("EsdlMethod");
+            ForEach(*methoditer)
+            {
+                IPropertyTree &item = methoditer->query();
+                defInfo->ensureServiceMethod(serviceName, item.queryProp("@name"));
+            }
+        }
+    }
+
+    virtual IEsdlDefinitionInfo* fetchDefinitionInfo(const char* definitionId) override
     {
+        Owned<CEsdlDefinitionInfo> defInfo= new CEsdlDefinitionInfo();
+
         if (!definitionId || !*definitionId)
             throw MakeStringException(-1, "Unable to fetch ESDL Service definition information, definition id is not available");
 
+        StringBuffer targetId(definitionId);
+
         if (!strchr (definitionId, '.')) //no name.ver delimiter, find latest version of name
         {
-            Owned<IRemoteConnection> conn = checkQuerySDS().connect(ESDL_DEFS_ROOT_PATH, myProcessSession(), RTM_LOCK_READ, SDS_LOCK_TIMEOUT_DESDL);
-            if (!conn)
-                throw MakeStringException(-1, "Unable to connect to ESDL Service definition information in dali '%s'", ESDL_DEFS_ROOT_PATH);
+            unsigned latestSeq = fetchLatestSeqForDefinitionName(definitionId);
+            targetId.appendf(".%u", latestSeq);
+        }
 
-            IPropertyTree * esdlDefinitions = conn->queryRoot();
+        //There shouldn't be multiple entries here, but if so, we'll use the first one
+        VStringBuffer xpath("%s[@id='%s'][1]", ESDL_DEF_PATH, targetId.str());
+        Owned<IRemoteConnection> conn = checkQuerySDS().connect(xpath.str(), myProcessSession(), RTM_LOCK_READ, SDS_LOCK_TIMEOUT_DESDL);
+        if (!conn)
+            throw MakeStringException(-1, "Unable to connect to ESDL Service definition information in dali '%s'", xpath.str());
 
-            if (esdlDefinitions)
-            {
-                VStringBuffer xpath("%s[@name='%s']", ESDL_DEF_ENTRY, definitionId);
-                Owned<IPropertyTreeIterator> iter = esdlDefinitions->getElements(xpath.str());
+        IPropertyTree* defn = conn->queryRoot();
 
-                unsigned latestSeq = 1;
-                ForEach(*iter)
-                {
-                    IPropertyTree &item = iter->query();
-                    unsigned thisSeq = item.getPropInt("@seq");
-                    if (thisSeq > latestSeq)
-                        latestSeq = thisSeq;
-                }
-                xpath.setf("%s[@id='%s.%d'][1]", ESDL_DEF_ENTRY, definitionId, latestSeq);
-                DBGLOG("ESDL Binding: Fetching ESDL Definition '%s.%d' from Dali", definitionId, latestSeq);
-                return esdlDefinitions->getPropTree(xpath);
-            }
-            else
-                throw MakeStringException(-1, "Unable to fetch ESDL definition '%s' from dali", definitionId);
+        // Read the metadata - creation date, id, name, etc
+        Owned<IAttributeIterator> it = defn->getAttributes();
+        for (it->first(); it->isValid(); it->next())
+        {
+            const char* name = it->queryName();
+            const char* val = it->queryValue();
+            defInfo->setMetadataValue(name, val);
+        }
+
+        // Read the services and methods
+
+        // First get PTree for the definition
+        // In the future we may need to use an implementation that doesn't rely on
+        // PTree so we can keep the list of Services and Methods in lexical order
+        if( defn->hasProp(ESDL_DEF_CONTENT_STR) )
+        {
+            const char* defXML = defn->queryProp(ESDL_DEF_CONTENT_STR);
+            Owned<IPropertyTree> tmpTree= createPTreeFromXMLString(defXML, ipt_caseInsensitive|ipt_ordered);
+            readServiceMethodInfo(defInfo, tmpTree);
         }
         else
         {
-            //There shouldn't be multiple entries here, but if so, we'll use the first one
-            VStringBuffer xpath("%s[@id='%s'][1]", ESDL_DEF_PATH, definitionId);
-            Owned<IRemoteConnection> conn = checkQuerySDS().connect(xpath.str(), myProcessSession(), RTM_LOCK_READ, SDS_LOCK_TIMEOUT_DESDL);
-            if (!conn)
-             throw MakeStringException(-1, "Unable to connect to ESDL Service definition information in dali '%s'", xpath.str());
+            IPropertyTree* esxdlTree = defn->queryBranch(ESDL_DEF_CONTENT_PTREE);
+            if( esxdlTree != nullptr )
+                readServiceMethodInfo(defInfo, esxdlTree);
+            else
+                throw MakeStringException(-1, "Unable to retrieve ESDL Service definition %s interface", definitionId);
 
-             return createPTreeFromIPT(conn->queryRoot());
         }
-        return nullptr;
+
+        return defInfo.getLink();
     }
 
     virtual void fetchDefinitionXML(const char* definitionId, StringBuffer& esxdl) override
@@ -140,12 +265,18 @@ public:
         DBGLOG("ESDL Binding: Fetching ESDL Definition from Dali: %s ", definitionId);
 
         //There shouldn't be multiple entries here, but if so, we'll use the first one
-        VStringBuffer xpath("%s[@id='%s'][1]/esxdl", ESDL_DEF_PATH, definitionId);
+        VStringBuffer xpath("%s[@id='%s'][1]", ESDL_DEF_PATH, definitionId);
         Owned<IRemoteConnection> conn = checkQuerySDS().connect(xpath.str(), myProcessSession(), RTM_LOCK_READ, SDS_LOCK_TIMEOUT_DESDL);
         if (!conn)
            throw MakeStringException(-1, "Unable to connect to ESDL Service definition information in dali '%s'", xpath.str());
 
-        toXML(conn->queryRoot(), esxdl, 0, 0);
+        IPropertyTree* defn = conn->queryRoot();
+        if( defn->hasProp(ESDL_DEF_CONTENT_STR))
+            esxdl.set(defn->queryProp(ESDL_DEF_CONTENT_STR));
+        else if( defn->hasProp(ESDL_DEF_CONTENT_PTREE))
+            toXML(defn->queryPropTree(ESDL_DEF_CONTENT_PTREE), esxdl, 0, 0);
+        else
+            throw MakeStringException(-1, "Unable to fetch ESDL Service definition contents for %s", definitionId);
     }
 
     virtual void fetchLatestDefinitionXML(const char* definitionName, StringBuffer& esxdl) override
@@ -160,23 +291,17 @@ public:
            throw MakeStringException(-1, "Unable to connect to ESDL Service definition information in dali '%s'", ESDL_DEFS_ROOT_PATH);
 
         IPropertyTree * esdlDefinitions = conn->queryRoot();
+        unsigned latestSeq = fetchLatestSeqForDefinitionName(definitionName);
 
-        VStringBuffer xpath("%s[@name='%s']", ESDL_DEF_ENTRY, definitionName);
-        Owned<IPropertyTreeIterator> iter = esdlDefinitions->getElements(xpath.str());
-
-        unsigned latestSeq = 1;
-        ForEach(*iter)
-        {
-            IPropertyTree &item = iter->query();
-            unsigned thisSeq = item.getPropInt("@seq");
-            if (thisSeq > latestSeq)
-                latestSeq = thisSeq;
-        }
-
-        xpath.setf("%s[@id='%s.%d'][1]/esxdl", ESDL_DEF_ENTRY, definitionName, latestSeq);
+        VStringBuffer xpath("%s[@id='%s.%u'][1]", ESDL_DEF_ENTRY, definitionName, latestSeq);
         Owned<IPropertyTree> deftree = esdlDefinitions->getPropTree(xpath);
         if (deftree)
-            toXML(deftree, esxdl, 0,0);
+            if( deftree->hasProp(ESDL_DEF_CONTENT_STR))
+                esxdl.set(deftree->queryProp(ESDL_DEF_CONTENT_STR));
+            else if( deftree->hasProp(ESDL_DEF_CONTENT_PTREE))
+                toXML(deftree->queryPropTree(ESDL_DEF_CONTENT_PTREE), esxdl, 0, 0);
+            else
+                throw MakeStringException(-1, "Unable to fetch ESDL Service definition contents for %s", definitionName);
         else
             throw MakeStringException(-1, "Unable to fetch ESDL Service definition from dali: '%s'", definitionName);
     }
@@ -239,7 +364,7 @@ public:
         return nullptr;
     }
 
-    virtual bool addDefinition(const char* definitionName, IPropertyTree* definitionInfo, StringBuffer& newId, unsigned& newSeq, const char* userid, bool deleteprev, StringBuffer & message) override
+    virtual bool addDefinition(const char* definitionName, const char* xmldef, StringBuffer& newId, unsigned& newSeq, const char* userid, bool deleteprev, StringBuffer & message) override
     {
         Owned<IRemoteConnection> conn = checkQuerySDS().connect(ESDL_DEFS_ROOT_PATH, myProcessSession(), RTM_LOCK_WRITE, SDS_LOCK_TIMEOUT_DESDL);
         if (!conn)
@@ -300,6 +425,7 @@ public:
         CDateTime dt;
         dt.setNow();
         StringBuffer str;
+        Owned<IPropertyTree> definitionInfo = createPTree("Definition");
 
         newId.set(lcName).append(".").append(newSeq);
         definitionInfo->setProp("@name", lcName);
@@ -321,6 +447,9 @@ public:
         else
             definitionInfo->setProp("@created",dt.getString(str).str());
 
+        // ESDL_DEF_CONTENT_STR element contains this Definition is stored as serialized XML,
+        // not a pTree, to preserve element order
+        definitionInfo->addProp(ESDL_DEF_CONTENT_STR, xmldef);
         definitionRegistry->addPropTree(ESDL_DEF_ENTRY, LINK(definitionInfo));
         return true;
     }
@@ -346,38 +475,32 @@ public:
         if (!definitionId || !*definitionId)
             return false;
 
-        StringBuffer lcdefid (definitionId);
-        lcdefid.toLowerCase();
-        VStringBuffer xpath("%s[@id='%s']/esxdl", ESDL_DEF_PATH, lcdefid.str());
-        Owned<IRemoteConnection> globalLock = checkQuerySDS().connect(xpath.str(), myProcessSession(), RTM_LOCK_READ, SDS_LOCK_TIMEOUT_DESDL);
+        Owned<IEsdlDefinitionInfo> defInfo = fetchDefinitionInfo(definitionId);
+        const StringArray& services = defInfo->getServices();
 
-        if (globalLock)
+        // If there is only one service in the definition and an esdlServiceName was
+        // not passed in then search for the method in the single service.
+        // Otherwise search the service named by esdlServiceName for the method.
+        if( services.ordinality() == 1 && !esdlServiceName.length() )
         {
-            IPropertyTree * esxdl = globalLock->queryRoot();
-            Owned<IPropertyTreeIterator> it = esxdl->getElements("EsdlService");
-            int servicesCount = esxdl->getCount("EsdlService");
-
-            ForEach(*it)
+            const char* serviceName = services.item(0);
+            const StringArray* methods = defInfo->queryMethods(serviceName);
+            if( methods && methods->find(methodName) != NotFound )
             {
-                IPropertyTree* pCurrService = &it->query();
-                if ((servicesCount == 1 && !esdlServiceName.length()) || stricmp(pCurrService->queryProp("@name"), esdlServiceName.str())==0)
-                {
-                    Owned<IPropertyTreeIterator> it2 = pCurrService->getElements("EsdlMethod");
-                    ForEach(*it2)
-                    {
-                        IPropertyTree* pChildNode = &it2->query();
-                        if (stricmp(pChildNode->queryProp("@name"), methodName)==0)
-                        {
-                            if (!esdlServiceName.length())
-                                esdlServiceName.set(pCurrService->queryProp("@name"));
-                            return true;
-                        }
-                    }
-                }
+                esdlServiceName.set(serviceName);
+                return true;
+            }
+        } else if ( esdlServiceName.length() > 0 ){
+            const StringArray* methods = defInfo->queryMethods(esdlServiceName.str());
+            if( methods && methods->find(methodName) != NotFound )
+            {
+                return true;
             }
         }
+
         return false;
     }
+
     virtual int configureMethod(const char* bindingId, const char* methodName, IPropertyTree* configTree, bool overwrite, StringBuffer& message) override
     {
         if (!bindingId || !*bindingId)
@@ -1102,11 +1225,9 @@ private:
             return false;
         StringBuffer lcdefid (definitionId);
         lcdefid.toLowerCase();
-        Owned<IPTree> deftree = fetchDefinition(lcdefid.str());
-        if(!deftree)
-            return false;
-        VStringBuffer xpath("esxdl/EsdlService[@name='%s']", serviceName);
-        return deftree->hasProp(xpath.str());
+        Owned<IEsdlDefinitionInfo> defInfo = fetchDefinitionInfo(lcdefid.str());
+        const StringArray& services = defInfo->getServices();
+        return services.contains(serviceName);
     }
 
     int getIdFromProcBindingDef(const char* espProcName, const char* espBindingName, const char* definitionId, StringBuffer& bindingId, StringBuffer& message)

esp/services/esdl_svc_engine/esdl_store.hpp

@@ -28,16 +28,23 @@
  #define esdl_engine_decl DECL_IMPORT
 #endif
 
+interface IEsdlDefinitionInfo : public IInterface
+{
+    virtual const IProperties& getMetadata() = 0;
+    virtual const StringArray& getServices() = 0;
+    virtual const StringArray* queryMethods(const char* service) = 0;
+};
+
 interface IEsdlStore : public IInterface
 {
-    virtual IPropertyTree* fetchDefinition(const char* definitionId) = 0;
+    virtual IEsdlDefinitionInfo* fetchDefinitionInfo(const char* definitionId) = 0;
     virtual void fetchDefinitionXML(const char* definitionId, StringBuffer& esxdl) = 0;
     virtual void fetchLatestDefinitionXML(const char* definitionName, StringBuffer& esxdl) = 0;
     virtual IPropertyTree* fetchBinding(const char* espProcess, const char* espStaticBinding) = 0;
     virtual IPropertyTree* fetchBinding(const char* bindingId) = 0;
     virtual bool definitionExists(const char* definitionId) = 0;
     virtual bool isMethodDefined(const char* definitionId, StringBuffer & esdlServiceName, const char* methodName) = 0;
-    virtual bool addDefinition(const char* definitionName, IPropertyTree* definitionInfo, StringBuffer &newId, unsigned &newSeq, const char* userid, bool deleteprev, StringBuffer & message) = 0;
+    virtual bool addDefinition(const char* definitionName, const char* xmldef, StringBuffer &newId, unsigned &newSeq, const char* userid, bool deleteprev, StringBuffer & message) = 0;
     virtual int configureMethod(const char* bindingId, const char* methodName, IPropertyTree* configTree, bool overwrite, StringBuffer& message) = 0;
     virtual int configureMethod(const char* espProcName, const char* espBindingName, const char* definitionId, const char* methodName, IPropertyTree* configTree, bool overwrite, StringBuffer& message) = 0;
     virtual int configureLogTransform(const char* bindingId, const char* logTransformName, IPropertyTree* configTree, bool overwrite, StringBuffer& message) = 0;

esp/services/ws_esdlconfig/ws_esdlconfigservice.cpp

@@ -190,6 +190,89 @@ void CWsESDLConfigEx::init(IPropertyTree *cfg, const char *process, const char *
     m_esdlStore.setown(createEsdlCentralStore());
 }
 
+// Build two independent lists. First an array of IEspMethodConfig elements that lists
+// all methods defined across all services in the definition. Second is a StringArray
+// listing all the services defined in the definition.
+void CWsESDLConfigEx::buildServiceMethodsResponse(IEsdlDefinitionInfo* defInfo, IArrayOf<IEspMethodConfig>& methodList, StringArray& serviceList, const char* svc)
+{
+    const StringArray& services = defInfo->getServices();
+    ForEachItemIn(i, services)
+    {
+        const char* serviceName = services.item(i);
+
+        // If a service name is passed in, then only return info
+        // for that single service
+        if( isEmptyString(svc) || strcasecmp(svc, serviceName)==0 )
+        {
+            serviceList.append(serviceName);
+            const StringArray* methods = defInfo->queryMethods(serviceName);
+
+            if( methods )
+            {
+                ForEachItemIn(j, *methods)
+                {
+                    Owned<IEspMethodConfig> methodconfig = createMethodConfig("","");
+                    methodconfig->setName(methods->item(j));
+                    methodList.append(*methodconfig.getClear());
+                }
+            }
+        }
+    }
+}
+
+// Builds an array of IEspESDLService elements, one for earch service in the
+// definition. Each element contains an array of of IEspMethodConfig elements,
+// one for each method defined in that service.
+void CWsESDLConfigEx::buildServiceWithMethodsResponse(IEsdlDefinitionInfo* defInfo, IArrayOf<IEspESDLService>& serviceList, const char* svc)
+{
+    const StringArray& services = defInfo->getServices();
+    ForEachItemIn(i, services)
+    {
+        const char* serviceName = services.item(i);
+
+        // If a service name is passed in, then only return info
+        // for that single service
+        if( isEmptyString(svc) || strcasecmp(svc, serviceName)==0 )
+        {
+            Owned<IEspESDLService> esdlservice = createESDLService();
+            esdlservice->setName(serviceName);
+
+            IArrayOf<IEspMethodConfig> respmethodsarray;
+            const StringArray* methods = defInfo->queryMethods(serviceName);
+
+            if( methods )
+            {
+                ForEachItemIn(j, *methods)
+                {
+                    Owned<IEspMethodConfig> methodconfig = createMethodConfig("","");
+                    methodconfig->setName(methods->item(j));
+                    respmethodsarray.append(*methodconfig.getClear());
+                }
+            }
+            esdlservice->setMethods(respmethodsarray);
+            serviceList.append(*esdlservice.getClear());
+        }
+    }
+}
+
+// Here we construct the <Definition> root element from the definition metadata
+// and make it root around the provided definition XML
+void CWsESDLConfigEx::wrapWithDefinitionElement(IEsdlDefinitionInfo* defInfo, StringBuffer& def)
+{
+    StringBuffer definitionElement("<Definition");
+    const IProperties& metadata = defInfo->getMetadata();
+    Owned<IPropertyIterator> iter = metadata.getIterator();
+    ForEach(*iter)
+    {
+        const char* name = iter->getPropKey();
+        const char* val = metadata.queryProp(name);
+        // skip the leading @ in the property name
+        definitionElement.appendf(" %s=\"%s\"", &name[1], val);
+    }
+    definitionElement.appendf(">%s</Definition>", def.str());
+    def.swapWith(definitionElement);
+}
+
 bool CWsESDLConfigEx::onPublishESDLDefinition(IEspContext &context, IEspPublishESDLDefinitionRequest & req, IEspPublishESDLDefinitionResponse & resp)
 {
     try
@@ -218,21 +301,17 @@ bool CWsESDLConfigEx::onPublishESDLDefinition(IEspContext &context, IEspPublishE
         if (!inxmldef || !*inxmldef)
             throw MakeStringException(-1, "Service definition (XML ESDL) is missing");
 
-        //much easier than creating a temp tree later on, just to add a root tag...
-        StringBuffer xmldefinition;
-        xmldefinition.appendf("<Definition>%s</Definition>", inxmldef);
-
-        Owned<IPropertyTree> serviceXMLTree = createPTreeFromXMLString(xmldefinition,ipt_caseInsensitive);
-
+        Owned<IPropertyTree> serviceXMLTree = createPTreeFromXMLString(inxmldef, ipt_caseInsensitive|ipt_ordered);
 #ifdef _DEBUG
-        StringBuffer xml;
-        toXML(serviceXMLTree, xml, 0,0);
-        fprintf(stderr, "incoming ESDL def: %s", xml.str());
+        fprintf(stderr, "incoming ESDL def: %s", inxmldef);
 #endif
 
+        // No Service name provided on input, so search the definition
+        // tree for any Services defined - at least one is required
+        // Use a concatenation of defined service names
         if (service.length() == 0)
         {
-            Owned<IPropertyTreeIterator> iter = serviceXMLTree->getElements("esxdl/EsdlService");
+            Owned<IPropertyTreeIterator> iter = serviceXMLTree->getElements("EsdlService");
             StringArray servicenames;
             ForEach (*iter)
             {
@@ -254,8 +333,9 @@ bool CWsESDLConfigEx::onPublishESDLDefinition(IEspContext &context, IEspPublishE
         }
         else
         {
+            // A service name was provided- ensure it's present in the definition
             StringBuffer serviceXpath;
-            serviceXpath.appendf("esxdl/EsdlService[@name=\"%s\"]", service.str());
+            serviceXpath.appendf("EsdlService[@name=\"%s\"]", service.str());
 
             if (!serviceXMLTree->hasProp(serviceXpath))
                 throw MakeStringException(-1, "Service \"%s\" definition not found in ESDL provided", service.str());
@@ -268,7 +348,7 @@ bool CWsESDLConfigEx::onPublishESDLDefinition(IEspContext &context, IEspPublishE
         unsigned newseq = 0;
         StringBuffer msg;
 
-        if (m_esdlStore->addDefinition(service.str(), serviceXMLTree.get(), newqueryid, newseq, user, deletePrevious, msg))
+        if (m_esdlStore->addDefinition(service.str(), inxmldef, newqueryid, newseq, user, deletePrevious, msg))
         {
             if (newseq)
                 resp.setEsdlVersion(newseq);
@@ -292,10 +372,9 @@ bool CWsESDLConfigEx::onPublishESDLDefinition(IEspContext &context, IEspPublishE
 
                 try
                 {
-                    Owned<IPropertyTree> definitionTree;
-                    definitionTree.set(m_esdlStore->fetchDefinition(newqueryid.toLowerCase()));
-                    if(definitionTree)
-                        toXML(definitionTree, definitionxml);
+                    Owned<IEsdlDefinitionInfo> defInfo;
+                    defInfo.setown(m_esdlStore->fetchDefinitionInfo(newqueryid.toLowerCase()));
+                    m_esdlStore->fetchDefinitionXML(newqueryid.toLowerCase(), definitionxml);
 
                     msg.appendf("\nSuccessfully fetched ESDL Defintion: %s from Dali.", newqueryid.str());
 
@@ -306,74 +385,40 @@ bool CWsESDLConfigEx::onPublishESDLDefinition(IEspContext &context, IEspPublishE
                     }
                     else
                     {
+                        wrapWithDefinitionElement(defInfo, definitionxml);
+
                         if (ver >= 1.4)
                             resp.updateDefinition().setInterface(definitionxml.str());
                         else
                             resp.setXMLDefinition(definitionxml.str());
 
-                        if (definitionTree)
+                        try
                         {
-                            try
+                            if (ver >= 1.4)
                             {
-                                if (ver >= 1.4)
-                                {
-                                    IEspPublishHistory& defhistory = resp.updateDefinition().updateHistory();
-                                    addPublishHistory(definitionTree, defhistory);
-
-                                    IArrayOf<IEspESDLService> respservicesresp;
-                                    Owned<IPropertyTreeIterator> serviceiter = definitionTree->getElements("esxdl/EsdlService");
-                                    ForEach(*serviceiter)
-                                    {
-                                        Owned<IEspESDLService> esdlservice = createESDLService("","");
-                                        IPropertyTree &curservice = serviceiter->query();
-                                        esdlservice->setName(curservice.queryProp("@name"));
-
-                                        Owned<IPropertyTreeIterator> methoditer = curservice.getElements("EsdlMethod");
-                                        IArrayOf<IEspMethodConfig> respmethodsarray;
-                                        ForEach(*methoditer)
-                                        {
-                                            Owned<IEspMethodConfig> methodconfig = createMethodConfig("","");
-                                            IPropertyTree &item = methoditer->query();
-                                            methodconfig->setName(item.queryProp("@name"));
-                                            respmethodsarray.append(*methodconfig.getClear());
-                                        }
-                                        esdlservice->setMethods(respmethodsarray);
-                                        respservicesresp.append(*esdlservice.getClear());
-                                    }
-
-                                    resp.updateDefinition().setServices(respservicesresp);
-                                }
-                                else
-                                {
-                                    StringArray esdlServices;
-                                    Owned<IPropertyTreeIterator> serviceiter = definitionTree->getElements("Exsdl/EsdlService");
-                                    ForEach(*serviceiter)
-                                    {
-                                        IPropertyTree &curservice = serviceiter->query();
-                                        esdlServices.append(curservice.queryProp("@name"));
+                                IEspPublishHistory& defhistory = resp.updateDefinition().updateHistory();
+                                addPublishHistoryFromMetadata(defInfo->getMetadata(), defhistory);
+                                IArrayOf<IEspESDLService> respservicesresp;
 
-                                        Owned<IPropertyTreeIterator> iter = curservice.getElements("EsdlMethod");
-                                        IArrayOf<IEspMethodConfig> list;
-                                        ForEach(*iter)
-                                        {
-                                            Owned<IEspMethodConfig> methodconfig = createMethodConfig("","");
-                                            IPropertyTree &item = iter->query();
-                                            methodconfig->setName(item.queryProp("@name"));
-                                            list.append(*methodconfig.getClear());
-                                        }
-                                        resp.setMethods(list);
-                                    }
-                                    resp.setESDLServices(esdlServices);
-                                }
+                                buildServiceWithMethodsResponse(defInfo, respservicesresp);
+                                resp.updateDefinition().setServices(respservicesresp);
                             }
-                            catch (...)
+                            else
                             {
-                                msg.append("\nEncountered error while parsing fetching available methods");
+                                // Note previous implementation quirk- only the methods for the
+                                // last-processed Service were listed in the response
+                                StringArray esdlServices;
+                                IArrayOf<IEspMethodConfig> respmethodsarray;
+
+                                buildServiceMethodsResponse(defInfo, respmethodsarray, esdlServices);
+                                resp.setMethods(respmethodsarray);
+                                resp.setESDLServices(esdlServices);
                             }
-
                         }
-                        else
-                            msg.append("\nCould not fetch available methods");
+                        catch (...)
+                        {
+                            msg.append("\nEncountered error while fetching available methods");
+                        }
                     }
                 }
                 catch(IException* e)
@@ -1310,6 +1355,14 @@ bool CWsESDLConfigEx::onGetESDLBinding(IEspContext &context, IEspGetESDLBindingR
     return true;
 }
 
+void CWsESDLConfigEx::addPublishHistoryFromMetadata(const IProperties &publishedMetadata, IEspPublishHistory & history)
+{
+    history.setLastEditBy(publishedMetadata.queryProp("@lastEditedBy"));
+    history.setCreatedTime(publishedMetadata.queryProp("@created"));
+    history.setPublishBy(publishedMetadata.queryProp("@publishedBy"));
+    history.setLastEditTime(publishedMetadata.queryProp("@lastEdit"));
+}
+
 void CWsESDLConfigEx::addPublishHistory(IPropertyTree * publishedEntryTree, IEspPublishHistory & history)
 {
     if (publishedEntryTree)
@@ -1487,7 +1540,7 @@ bool CWsESDLConfigEx::onGetESDLDefinition(IEspContext &context, IEspGetESDLDefin
     StringBuffer message;
     int respcode = 0;
 
-    Owned<IPropertyTree> definitionTree;
+    Owned<IEsdlDefinitionInfo> defInfo;
     try
     {
         if (ver >= 1.3)
@@ -1507,22 +1560,16 @@ bool CWsESDLConfigEx::onGetESDLDefinition(IEspContext &context, IEspGetESDLDefin
         else
             resp.setId(id.str());
 
-        definitionTree.set(m_esdlStore->fetchDefinition(id.toLowerCase()));
+        m_esdlStore->fetchDefinitionXML(id.toLowerCase(), definition);
+        defInfo.setown(m_esdlStore->fetchDefinitionInfo(id.toLowerCase()));
 
-        if (definitionTree)
+        if (definition.length() == 0 )
         {
-            toXML(definitionTree, definition, 0,0);
-
-            if (definition.length() == 0 )
-            {
-                respcode = -1;
-                message.append("\nDefinition appears to be empty!");
-            }
-            else
-                message.setf("Successfully fetched ESDL Defintion: %s from Dali.", id.str());
+            respcode = -1;
+            message.appendf("\nDefinition %s is empty!", id.str());
         }
         else
-            throw MakeStringException(-1, "Could not fetch ESDL definition '%s' from Dali.", id.str());
+            message.setf("Successfully fetched ESDL Defintion: %s from Dali.", id.str());
     }
     catch(IException* e)
     {
@@ -1542,12 +1589,16 @@ bool CWsESDLConfigEx::onGetESDLDefinition(IEspContext &context, IEspGetESDLDefin
 
     if (ver >= 1.2)
     {
+
+
+        wrapWithDefinitionElement(defInfo, definition);
+
         if(ver >= 1.4)
         {
             resp.updateDefinition().setInterface(definition.str());
 
             IEspPublishHistory& defhistory = resp.updateDefinition().updateHistory();
-            addPublishHistory(definitionTree, defhistory);
+            addPublishHistoryFromMetadata(defInfo->getMetadata(), defhistory);
         }
         else
             resp.setXMLDefinition(definition.str());
@@ -1558,80 +1609,37 @@ bool CWsESDLConfigEx::onGetESDLDefinition(IEspContext &context, IEspGetESDLDefin
             {
                 try
                 {
-                    VStringBuffer xpath("esxdl/EsdlService");
-                    if (serviceName && *serviceName)
-                        xpath.appendf("[@name='%s']", serviceName);
-
                     if ( ver >= 1.4)
                     {
-                        Owned<IPropertyTreeIterator> services = definitionTree->getElements(xpath.str());
                         IArrayOf<IEspESDLService> servicesarray;
-                        ForEach(*services)
-                        {
-                            IPropertyTree &curservice = services->query();
-                            Owned<IEspESDLService> esdlservice = createESDLService("","");
-                            esdlservice->setName(curservice.queryProp("@name"));
-
-                            Owned<IPropertyTreeIterator> methods = curservice.getElements("EsdlMethod");
-                            IArrayOf<IEspMethodConfig> methodsarray;
-                            ForEach(*methods)
-                            {
-                                Owned<IEspMethodConfig> methodconfig = createMethodConfig("","");
-                                IPropertyTree &method = methods->query();
-                                methodconfig->setName(method.queryProp("@name"));
-                                methodsarray.append(*methodconfig.getClear());
-                            }
-                            esdlservice->setMethods(methodsarray);
-                            servicesarray.append(*esdlservice.getClear());
-                        }
+                        buildServiceWithMethodsResponse(defInfo, servicesarray, serviceName);
                         resp.updateDefinition().setServices(servicesarray);
                     }
                     else
                     {
-                        xpath.append("/EsdlMethod");
-                        Owned<IPropertyTreeIterator> iter = definitionTree->getElements(xpath.str());
+                        StringArray esdlServices;
                         IArrayOf<IEspMethodConfig> list;
-                        ForEach(*iter)
-                        {
-                            Owned<IEspMethodConfig> methodconfig = createMethodConfig("","");
-                            IPropertyTree &item = iter->query();
-                            methodconfig->setName(item.queryProp("@name"));
-                            list.append(*methodconfig.getClear());
-                        }
+
+                        buildServiceMethodsResponse(defInfo, list, esdlServices, serviceName);
                         resp.setMethods(list);
-                    }
-                }
-                catch (...)
-                {
-                    message.append("\nEncountered error while parsing fetching available methods");
-                }
-            }
 
-            if (definitionTree)
-            {
-                try
-                {
-                    StringArray esdlServices;
-                    StringBuffer xpath;
-                    if (serviceName && *serviceName)
-                        xpath.appendf("EsdlService[@name='%s']", serviceName);
-                    else
-                        xpath.set("EsdlService");
-                    Owned<IPropertyTreeIterator> serviceiter = definitionTree->getElements(xpath.str());
-                    ForEach(*serviceiter)
-                    {
-                        IPropertyTree &item = serviceiter->query();
-                        esdlServices.append(item.queryProp("@name"));
+                        // Historically this function has not returned a list of Services
+                        // although the onPublishEsdlDefinition response has, and they both
+                        // use the same structures. I've updated them both to behave the same
+                        resp.setESDLServices(esdlServices);
                     }
-                    resp.setESDLServices(esdlServices);
                 }
                 catch (...)
                 {
-                    message.append("\nEncountered error while parsing fetching EsdlServices");
+                    message.append("\nEncountered error while fetching available methods");
                 }
             }
-            else
-                message.append("\nCould not fetch ESDLServices");
+            // Previous code here removed as it was inconsistent or incorrect.
+            //   - It would overwrite how setESDLServices was called for client version >= 1.4.
+            //   - It would set the incorrect response type for version < 1.4, thought it might
+            //     have validated against the schema by luck.
+            // Instead services & methods response is handled above and behavior is now consistent across
+            // onGetESDLDefinition and onPublishESDLDefinition, since they use the same interface
         }
         else
             message.append("\nCould not fetch ESDL services definition details");

esp/services/ws_esdlconfig/ws_esdlconfigservice.hpp

@@ -39,6 +39,10 @@ private:
     Owned<IEsdlStore> m_esdlStore;
     IPropertyTree * getEspProcessRegistry(const char * espprocname, const char * espbingingport, const char * servicename);
     int getBindingXML(const char * bindingId, StringBuffer & bindingXml, StringBuffer & msg);
+    void buildServiceMethodsResponse(IEsdlDefinitionInfo* defInfo, IArrayOf<IEspMethodConfig>& methodList, StringArray& serviceList, const char* svc = nullptr);
+    void buildServiceWithMethodsResponse(IEsdlDefinitionInfo* defInfo, IArrayOf<IEspESDLService>& serviceList, const char* svc = nullptr);
+    void wrapWithDefinitionElement(IEsdlDefinitionInfo* defInfo, StringBuffer& def);
+
 public:
     IMPLEMENT_IINTERFACE;
     virtual ~CWsESDLConfigEx(){};
@@ -58,6 +62,7 @@ public:
 
     bool addESDLBindingContentsHistory(IPropertyTree * publishedEntryTree, IEspESDLBindingContents& esdlbindingcontents);
     void addPublishHistory(IPropertyTree * publishedEntryTree, IEspPublishHistory &resp);
+    void addPublishHistoryFromMetadata(const IProperties &publishedMetadata, IEspPublishHistory &resp);
 
     bool attachServiceToDali() override
     {