HPCC-29381 Improve XSD generation for ESP services #17349

kenrowland · 2023-05-18T14:13:15Z

Replaced explicit XSD and WSDL generation in genreated code by using existing library function.

Signed-Off-By: Kenneth Rowland [email protected]

Type of change:

This change is a bug fix (non-breaking change which fixes an issue).
This change is a new feature (non-breaking change which adds functionality).
This change improves the code (refactor or other change that does not change the functionality)
This change fixes warnings (the fix does not alter the functionality or the generated code)
This change is a breaking change (fix or feature that will cause existing behavior to change).
This change alters the query API (existing queries will have to be recompiled)

Checklist:

Smoketest:

Send notifications about my Pull Request position in Smoketest queue.
Test my draft Pull Request.

Testing:

github-actions · 2023-05-18T14:39:39Z

https://track.hpccsystems.com/browse/HPCC-29381
Jira updated

asselitx

The only other thing is a kind of overall suggestion- Tony can weigh in too if it is appropriate or not. It may be good to add a kind of unittest or maybe regression test to have a known baseline showing that the esdl and hidl generated wsdls are matching, then we can keep track in future updates if anything breaks it.

asselitx · 2023-05-19T22:07:46Z

esp/bindings/http/platform/httpbinding.cpp

    response->setContent(verxml.str());
    response->setContentType(HTTP_TYPE_APPLICATION_XML_UTF8);
    response->setStatus(HTTP_STATUS_OK);
    response->send();
    return 0;
 }

+void getServiceSchema(const char *serviceQName, const char *methodQName, double version, const char *nstr,
+                      const StringBuffer &xmlFilename,  Owned<IProperties> &params, EsdlXslTypeId EsdlXslType, StringBuffer &schema)
+{


This may be premature optimization, but it could make sense to consider creating an IEsdlDefinitionHelper as a member of the EspHttpBinding and initializing it with the xslt and parameters at an appropriate time (initialization or service loading). Then we can avoid the overhead of compiling the transform each time the wsdl is requested. However, since the ECLWatch services aren't customer-facing transactional services with latency SLAs, it may not be required. I'm not sure where the pro/con balance is for doing it this way, but may depend on how feasible it is to hook into HttpBinding in a similar way as was done for EsdlBindingImpl.

This may be premature optimization, but it could make sense to consider creating an IEsdlDefinitionHelper as a member of the EspHttpBinding and initializing it with the xslt and parameters at an appropriate time (initialization or service loading). Then we can avoid the overhead of compiling the transform each time the wsdl is requested. However, since the ECLWatch services aren't customer-facing transactional services with latency SLAs, it may not be required. I'm not sure where the pro/con balance is for doing it this way, but may depend on how feasible it is to hook into HttpBinding in a similar way as was done for EsdlBindingImpl.

I think saving off the compiled esp_service_xml2xsd.xslt after the first use and then reusing it would be a good optimization. These wsdls and xsds are used in quite a few ways. I'd be fine with having a new jira opened for that if you don't want to do it in this PR.

Optimization was something I wanted to do after all uses of the getXsdDefinition method (generated code) are replaced. Then all options/settings are known.

Sounds good. Can you open a JIRA about only compiling the xslt once?

Opened https://track.hpccsystems.com/browse/HPCC-29598 to address it

asselitx · 2023-05-23T21:29:15Z

esp/xslt/esp_service_xml2xsd.xslt

There are a few differences I'm seeing using this transform compared to hidl that I think will likely require some changes.

Comparison of the PR esdl-generated wsdl for WsWorkunits vs. hidl-generated yields some differences:

WSDL Namespace

In the PR, the default namespace for wsdl is declared on the each parent element that needs it, starting with the <definitions> element. Also the same namespace is declared with the wsdl prefix on the <definitions> element and the <xsl:stylesheet> element.

This results in:

An extra xmlns:wsdl declaration in the output <definitions> element (possibly not a big deal)

Extra xmlns="" declarations on enum <item> elements, such as those in the LogSelectColumnMode xsd:simpleType. Strictly speaking, these elements probably should be in the xsd namespace, but they aren't in the hidl-generated output.

These discrepancies can be cleared up by:

Placing the only xmlns="http://schemas.xmlsoap.org/wsdl/" declaration in the <xsl:stylesheet> element.

Declaring xmlns:wsdl="http://schemas.xmlsoap.org/wsdl/" in the stylesheet element.

Adding exclude-result-prefixes="wsdl" attribute to the stylesheet element.

Numeric Default Values

It appears that where the scm file defines a default numeric value of zero, the esdl wsdl is generating a default="…" attribute but the hidl wsdl is not. Reference <xsd:element name="WUSubmit"> or <xsd:element name="WUAnalyseHotspot">.

Strangely, you can also see in WsAnalyseHotspot that hidl does output default="1" for the ThresholdPercent double, while esdl outputs default="1.0". And also both agree when the default value is -1 such as WUQueryResponse.LastPage.

Optional

It appears that the esdl wsdl is not generating the optional elements. See that <xsd:element name="WUAnalyseHotspot"> is missing RootScope and PropertyOptions. I think this is because you're passing a non-null IProperties in to the call to getDependencies(). Passing a non-null pointer there means that you intend to enforce the 'optional' attributes from the scm definitions. If you do that you'd need to pass in explicitly those optional values that are in effect for the request. However since it is empty, that means you're enforcing the optional flags but there are none in effect.

This is confusing I apologize and in retrospect I'd probably make that getDependencies() interface more clear.

Element Types

It looks like hidl and esdl aren't agreeing on the types output for some cases:

hidl type esdl type example

xsd:base64Binary xsd:binary WUDeployWorkunit.Object

xsd:long xsd:int64 WUFile.SizeLimit

xsd:dateTime xsd:string ECLWorkunitLW.DateTimeScheduled

@asselitx for the optional stuff. Are you saying that simply requesting the xsd should include all the optionals?

It's been a long time since I've tried that, but seems like optionals would dissapear if not requests, but ultimately of course we just want to match current behavior

This was just getting the wsdl from the link in a browser at the URL like http://127.0.0.1:8010/WsWorkunits

But I think you're right @afishbeck my original suggestion is not the correct solution across the board- I was referencing the behavior of the esdl xsd tool command.

We will for sure need to pass through the actual "optionals" included in the URL request, but at least in this case it appears that by default hidl is showing us those elements marked optional even when its not explicitly included in the URL. I didn't check to see if there was a special case for just that value or if it is exposing all.

Further, I see that this use of a bare [optional] attribute differs from what I've seen used ( across the board I believe) in scapps, where it always has a string label like [optional("foobar")]. I don't know if this "bare" usage is a special case or how it has been treated historically.

@asselitx I don't think the bare optionals are correct. Looks like they were added when someone thought it was just a way of marking an entity as "not required". @kenrowland you may want to remove those but in any case if they are never filtered out that is fine.

I'm not sure exactly, but the esdl tool by default I think may be trying to give us the superset. ESP shouldn't expose an optional tag unless it's requested by the caller.

If you want you can ask Shamser about the bare optionals. I think he added them but thought it just meant the opposite of required.

@afishbeck @asselitx I am a little confused on whether the optionals should be included or not. As stated by Terrance, removal of the empty opts parameter and using nullptr instead results in the optional request members being included in the wsdl output. I am not aware of any way externally to optionally include them or not.

Ultimately if we are striving to match what the current hidl generated code outputs, then it needs to be included. The hidl generated code makes no distinction between optional and non-optional and simply outputs all request members.

We should test and support real optionals, but ignore the invalid ones just like it seems to do now.

A real optional looks like this:
[optional("myoptionname")] string a;

that string "a" should not show up in the xsd if you do this:
https://esp:8010/wsabc/mymethod?xsd

but should show up if you do this:
https://esp:8010/wsabc/mymethod?xsd&myoptionname

[optional] string a;
Should do nothing, and the "optional" should probably be removed.

Please experiment by adding a real optional to one of the fields and trying the old and new xsd mechanisms.

@kenrowland I think my approval was a bit premature. Modifying an ecm file with a correctly implemented [optional("foobar")] attribute has the effect that now it is never output even when &foobar is included in the URL. I'll need to dig in a little more to understand exactly why the [optional] decorations appeared to affect the output in your previous revision.

Should be addressed now.

afishbeck · 2023-05-23T21:55:38Z

The only other thing is a kind of overall suggestion- Tony can weigh in too if it is appropriate or not. It may be good to add a kind of unittest or maybe regression test to have a known baseline showing that the esdl and hidl generated wsdls are matching, then we can keep track in future updates if anything breaks it.

But the hidl generated ones are going away. We could store them off as a unit test, but we'd have to find a way to make it manageable because the actual content of the interfaces can change.

asselitx · 2023-05-24T15:18:42Z

The only other thing is a kind of overall suggestion- Tony can weigh in too if it is appropriate or not. It may be good to add a kind of unittest or maybe regression test to have a known baseline showing that the esdl and hidl generated wsdls are matching, then we can keep track in future updates if anything breaks it.

But the hidl generated ones are going away. We could store them off as a unit test, but we'd have to find a way to make it manageable because the actual content of the interfaces can change.

Ah yes of course. Probably then it doesn't really make sense in this case.

afishbeck · 2023-05-26T13:35:14Z

@kenrowland In regards to the change in types.

xsd:base64Binary vs xsd:binary
xsd:long vs xsd:int64
xsd:dateTime vs xsd:string

There is a chance it would break clients.

A good test would be to have Rodrigo see what changes that made to the Java classes he generates from the xsd/wsdl.

I'm not sure what java types are used for what schema types, but for a made up example, if what was generated as a java LocalDateTime is now generated as a string, then code that uses the generated code might break.

@rpastrana might have some thoughts. But I would consider adding some flags to the xslt or wherever needed to allow you to specify which mapping you want.

afishbeck · 2023-05-26T13:37:03Z

or since you are using an isolated xslt it may be even easier than that. I forget where the mappings happen.

kenrowland · 2023-05-26T13:42:56Z

@rpastrana Please comment on whether a date time typed method request member being typed as an xsd:string in the generated WSDL for an ESP service method is an issue. Note that this would be a change from the current generated WSDL

@afishbeck The source type for how the new WSDL generation is done is the service XML generated as part of the build. The XML has date time fields typed as a string. The esdl-xml command used to generate the XML as part of the build would need to change so it honors the the original date time type. I should note that running the esdl command against the source ecm file also results in a "string" type in the generated WSDL. For example, if you run

esdl wsdl .../src/esp/scm/ws_workunits.ecm WsWorkunits > test.out

And inspect the output, you'll see

<xsd:element minOccurs="0" name="DateTimeScheduled" type="xsd:string"/>

The DateTimeScheduled field is declared in the ecm as a date time type.

afishbeck · 2023-05-26T13:47:15Z

@rpastrana Please comment on whether a date time typed method request member being typed as an xsd:string in the generated WSDL for an ESP service method is an issue. Note that this would be a change from the current generated WSDL

@afishbeck The source type for how the new WSDL generation is done is the service XML generated as part of the build. The XML has date time fields typed as a string. The esdl-xml command used to generate the XML as part of the build would need to change so it honors the the original date time type. I should note that running the esdl command against the source ecm file also results in a "string" type in the generated WSDL. For example, if you run

esdl wsdl .../src/esp/scm/ws_workunits.ecm WsWorkunits > test.out

And inspect the output, you'll see

<xsd:element minOccurs="0" name="DateTimeScheduled" type="xsd:string"/>

The DateTimeScheduled field is declared in the ecm as a date time type.

Agreed, the fact that the generated xml would have to change would make fixing it trickier.

rpastrana · 2023-05-26T15:02:07Z

@kenrowland In regards to the change in types.

xsd:base64Binary vs xsd:binary xsd:long vs xsd:int64 xsd:dateTime vs xsd:string

There is a chance it would break clients.

A good test would be to have Rodrigo see what changes that made to the Java classes he generates from the xsd/wsdl.

I'm not sure what java types are used for what schema types, but for a made up example, if what was generated as a java LocalDateTime is now generated as a string, then code that uses the generated code might break.

@rpastrana might have some thoughts. But I would consider adding some flags to the xslt or wherever needed to allow you to specify which mapping you want.

Thanks for the heads-up @afishbeck, yes changing existing field types will break the hpcc4j client (in general all clients based on the existing WSDLs would be affected).

afishbeck · 2023-05-26T18:32:18Z

@kenrowland I would suggest adding an option on the code that generates the xml files so that when an element is generated that would differ between esdl and hidl versions of the xsd an extra attribute would be output. When it doesn't differ it should be left off.

This should be used for all of the xml files generated by our build process.

For example:

<xsd:element minOccurs="0" name="DateTimeScheduled" type="xsd:string" hidl_type="xsd:dateTime" />

Then the esdl xsd will continue to use xsd:string, but the hidl replacement xsd can use hidl_type when it is present.

You could open another JIRA about adding an option to the "esdl xsd" and "esdl wsdl" cli commands to cause them to output the hidl version when it's wanted.

kenrowland · 2023-05-30T13:35:59Z

@afishbeck @asselitx I am going to divert and work on the generated service XML in order to get the types correct. I'll use the strategy mentioned by Tony where a hidl type is output.

afishbeck · 2023-06-15T19:54:16Z

I think the title of the PR / Commit should be changed. The XSD isn't improving it's staying the same, this is really about providing a runtime alternative to HIDL generated XSD code.

asselitx

Comparison against hidl-based wsdls with a variety of different URL parameters showed no differences.

esp/scm/additional.cmake

afishbeck · 2023-06-15T19:56:04Z

esp/esdlscriptlib/esdl_script.hpp

@@ -0,0 +1,239 @@
+/*##############################################################################


I think if you delete the files from their previous locations it will show this as a relocation rather than a new file. In any case we never want the file in two places people (myself especially) will get confused.

Deleted duplicate files.

afishbeck · 2023-06-15T19:59:45Z

esp/xslt/esp_service_xml2xsd.xslt

+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+##############################################################################
+#    HPCC SYSTEMS software Copyright (C) 2012 HPCC Systems®.


If this is a new file (not just relocated) then the date should be 2023.

afishbeck · 2023-06-15T20:51:58Z

tools/hidl/hidlcomp.cpp

@@ -5371,6 +5384,9 @@ void EspServInfo::write_esp_binding()
    }
    outs("\tDBGLOG(\"Client version: %g\", context.getClientVersion());\n");

+    // getXSDDefinition(StringBuffer &content, const char *packageName, const char *serviceName, double version, const char *method);


Is this the commented out code you said you were removing?

This is the original generated code that calls the hardcoded code in the service. I left if for reference until it fully removed.

Still think this should just be removed, but I assume this will be removed as part of one of your next PRs. Can be merged as is.

afishbeck · 2023-06-15T20:53:38Z

tools/hidl/hidlcomp.cpp

+    //method ==> getXmlFilename for xsd and wsdl transformations
+    outf("\nint C%sSoapBinding::getXmlFilename(StringBuffer &filename)\n", name_);
+    outs("{\n");
+    outf("\tfilename.append(\"%s\");\n", packagename);


You could add the .xml here, producing one line:

outf("\tfilename.append("%s.xml");\n", packagename);

Reduced to one line

asselitx · 2023-06-15T21:18:28Z

esp/bindings/http/platform/httpbinding.cpp

+    xlstFile.append("xslt").append(PATHSEPSTR).append("esp_service_xml2xsd.xslt");
+
+    defHelper->loadTransform(xlstFile, params, EsdlXslType);
+    defHelper->toWSDL(*structs, schema, EsdlXslType, version, nullptr, nstr, optFlags);


Instead of passing nullptr for the opts parameter, we'll need to pull the URL parameters from the request and pass those instead. Then will be the issue of how to deal with ecm elements tagged [optional]. Either we can remove that from the ecm if it would have no negative effect, or we could figure out if there is some other value to insert (for this code path) in the IProperties* opts parameter that the transform would recognize as the name for that 'optional'. My hunch is maybe an empty string, but I'll need to check the generated xml format of esdl.

Passed the context request parameters to the function and used accordingly.

asselitx

Looks good. Tested modified WsWorkunits interface with an optional("foobar") element that correctly toggled in the wsdl depending on the presence of the &foobar URL parameter.

afishbeck · 2023-07-07T00:26:58Z

esp/bindings/http/platform/httpbinding.cpp

+    Owned<IEsdlDefinition> esdlDef;
+    Owned<IEsdlDefinitionHelper> defHelper;
+
+    esdlDef.set(createEsdlDefinition(nullptr, nullptr));


These look like they would leak to me. Have you tested with valgrind? To hand ownership to "Owned" you should use setown().
But the easiest thing would be just to do:

Owned<IEsdlDefinition> esdlDef = createEsdlDefinition(nullptr, nullptr); Owned<IEsdlDefinitionHelper> defHelper = createEsdlDefinitionHelper();

Changed as indicated.

Hard to tell looking at Owned and Shared if it would leak or not.

I ran valgrind and nothing jumped out regarding a leak in that area. I ran after the above change you suggested.

afishbeck

@kenrowland some comments.

afishbeck · 2023-07-07T00:30:10Z

esp/bindings/http/platform/httpbinding.cpp

+
+    std::string xmlFilePath(getCFD());
+    xmlFilePath.append("esdl_files").append(PATHSEPSTR).append(xmlFilename.str());
+    esdlDef->addDefinitionsFromFile(xmlFilePath.c_str());


We should keep in mind for the future that we could cache some esdlDefs loaded from the xml files if accessing these is perceived to be slow.

HPCC-29598 is open to optimize this process. Once all use of the current XSD/WSDL generation are removed (there is at least one more Jira to do that), optimization will be done.

afishbeck · 2023-07-07T00:46:34Z

esp/esdlscriptlib/esdl_xpath_extensions_libxml.cpp

+    See the License for the specific language governing permissions and
+    limitations under the License.
+############################################################################## */
+


It looks like this file now exists in two places? It should be removed from the old location and only this version should now be used.

Removed the extra copy

afishbeck · 2023-07-07T00:47:08Z

esp/esdlscriptlib/esdl_xpath_extensions_unsupported.cpp

+    See the License for the specific language governing permissions and
+    limitations under the License.
+############################################################################## */
+


Same as above, file should only exist in one place.

Removed the extra copy.

afishbeck

Approved.

afishbeck · 2023-07-10T20:49:45Z

tools/hidl/hidlcomp.cpp

@@ -5371,6 +5384,9 @@ void EspServInfo::write_esp_binding()
    }
    outs("\tDBGLOG(\"Client version: %g\", context.getClientVersion());\n");

+    // getXSDDefinition(StringBuffer &content, const char *packageName, const char *serviceName, double version, const char *method);


Still think this should just be removed, but I assume this will be removed as part of one of your next PRs. Can be merged as is.

Replaced explicit XSD and WSDL generation in genreated code by using existing library function. Signed-Off-By: Kenneth Rowland [email protected]

kenrowland · 2023-07-10T21:00:04Z

@ghalliday Please merge

GordonSmith · 2023-07-13T13:53:22Z

esp/esdlscriptlib/esdl_script.hpp

Looks like this change is breaking the build

kenrowland requested review from afishbeck and asselitx May 18, 2023 14:14

kenrowland force-pushed the HPCC-29381 branch from a688682 to 213ea5a Compare May 19, 2023 15:01

asselitx requested changes May 23, 2023

View reviewed changes

kenrowland force-pushed the HPCC-29381 branch from 213ea5a to 88f95d8 Compare June 14, 2023 15:03

kenrowland requested a review from asselitx June 14, 2023 15:04

kenrowland force-pushed the HPCC-29381 branch from 88f95d8 to 82bc706 Compare June 15, 2023 15:54

asselitx approved these changes Jun 15, 2023

View reviewed changes

esp/scm/additional.cmake Show resolved Hide resolved

afishbeck reviewed Jun 15, 2023

View reviewed changes

asselitx requested changes Jun 15, 2023

View reviewed changes

kenrowland requested review from afishbeck and asselitx June 16, 2023 02:52

asselitx approved these changes Jul 3, 2023

View reviewed changes

Michael-Gardner approved these changes Jul 5, 2023

View reviewed changes

afishbeck reviewed Jul 7, 2023

View reviewed changes

afishbeck requested changes Jul 7, 2023

View reviewed changes

kenrowland requested a review from afishbeck July 7, 2023 15:26

afishbeck approved these changes Jul 10, 2023

View reviewed changes

HPCC-29381 Improve XSD generation for ESP services

cd987c5

Replaced explicit XSD and WSDL generation in genreated code by using existing library function. Signed-Off-By: Kenneth Rowland [email protected]

kenrowland force-pushed the HPCC-29381 branch from a6ab09d to cd987c5 Compare July 10, 2023 20:57

GordonSmith reviewed Jul 13, 2023

View reviewed changes

esp/esdlscriptlib/esdl_script.hpp

Copy link

Member

GordonSmith Jul 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this change is breaking the build

Add dependency so unit tests will build

8a7a098

kenrowland requested a review from GordonSmith July 13, 2023 16:11

GordonSmith approved these changes Jul 13, 2023

View reviewed changes

GordonSmith merged commit 75bc83c into hpcc-systems:master Jul 13, 2023
49 checks passed

hidl type	esdl type	example
xsd:base64Binary	xsd:binary	WUDeployWorkunit.Object
xsd:long	xsd:int64	WUFile.SizeLimit
xsd:dateTime	xsd:string	ECLWorkunitLW.DateTimeScheduled

		@@ -0,0 +1,239 @@
		/*##############################################################################

HPCC-29381 Improve XSD generation for ESP services #17349

HPCC-29381 Improve XSD generation for ESP services #17349

Conversation

kenrowland commented May 18, 2023 • edited Loading

Type of change:

Checklist:

Smoketest:

Testing:

github-actions bot commented May 18, 2023

asselitx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WSDL Namespace

Numeric Default Values

Optional

Element Types

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

afishbeck commented May 23, 2023

asselitx commented May 24, 2023

afishbeck commented May 26, 2023

afishbeck commented May 26, 2023

kenrowland commented May 26, 2023 • edited Loading

afishbeck commented May 26, 2023

rpastrana commented May 26, 2023

afishbeck commented May 26, 2023

kenrowland commented May 30, 2023

afishbeck commented Jun 15, 2023

asselitx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asselitx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

afishbeck left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

afishbeck left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kenrowland commented Jul 10, 2023

Choose a reason for hiding this comment

kenrowland commented May 18, 2023 •

edited

Loading

kenrowland commented May 26, 2023 •

edited

Loading