Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix xml namespace/element/type handling #756

Merged
merged 1 commit into from
Oct 21, 2015
Merged

Fix xml namespace/element/type handling #756

merged 1 commit into from
Oct 21, 2015

Conversation

raymondfeng
Copy link
Contributor

/to @jsdevel

The PR improves the XML namespace handling for JSON to XML transformation.

  1. Introduce a NamespaceContext class in lib/nscontext.js to keep track of namespace prefix/uri mappings and declarations in the hierarchy.
  2. Rename the variables so that they are consistent with XML terminology to avoid confusions.
  3. With 1, if the same prefix/uri mapping is declared at ancestor elements, we don't repeat such xmlns:prefix=uri
  4. Fix some test cases that reference the wrong element names
  5. Fix/enhance the resolution to various schema objects, such as local element, global element, element ref, type ref. Better handing the local element qualification form.
  6. Fix the allowed child elements based on XML schema spec, for example, sequence can be a child element of sequence.

Please review.

@jsdevel
Copy link
Collaborator

jsdevel commented Oct 20, 2015

The changes look very promising. I have a couple of questions:

  1. Why rename DummyFilterString to DummyStringFilter?
  2. Why were request response samples (not related to DummyFilterString) changed?

@raymondfeng
Copy link
Contributor Author

  1. Why rename DummyFilterString to DummyStringFilter?

DummyStringFilter is the element name defined in the XSD.

  1. Why were request response samples (not related to DummyFilterString) changed?

It's related to item 3 to dedup the prefix/uri declarations at child levels.

jsdevel
jsdevel reviewed Oct 20, 2015
View reviewed changes
test/wsdl-test.js
@@ -89,6 +89,18 @@ wsdlStrictTests['should handle element ref'] = function(done) {
});
};

wsdlStrictTests['should handle type ref'] = function(done) {
var expectedMsg = require('./wsdl/typeref/request.xml.js');
var req_json = require('./wsdl/typeref/request.json');
Copy link
Collaborator

Choose a reason for hiding this comment

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

camelCase variable name

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure

@jsdevel
Copy link
Collaborator

jsdevel commented Oct 20, 2015

Fix the allowed child elements based on XML schema spec, for example, sequence can be a child element of sequence.

Have you provided tests for this?

@jsdevel
Copy link
Collaborator

jsdevel commented Oct 20, 2015

Fix some test cases that reference the wrong element names

Why weren't these failing before?

@raymondfeng
Copy link
Contributor Author

If the code fails to look up a schema object by name, it assumes that element is under the parent namespace. And it works by coincidence.

@raymondfeng
Copy link
Contributor Author

I fixed the camelCase and nesting sequence.

jsdevel
jsdevel reviewed Oct 20, 2015
View reviewed changes
lib/wsdl.js
This will attempt to fix circular dependencies with XSD files,
Given

file.wsdl
Copy link
Collaborator

Choose a reason for hiding this comment

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

I like this formatting, but see now that each line should start with *. Can you add that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, that's messed up from the merge.

@jsdevel
Copy link
Collaborator

jsdevel commented Oct 20, 2015

Overall it looks good to me. I'd just go through it and ensure that each change was intended (no auto indending etc.). I'd also like @herom to review this.

@raymondfeng
Copy link
Contributor Author

@jsdevel Thanks for the review. I'm still not happy about the JSON to XML object mapping as-is. The description phase already builds up the tree of elements as the template for XML structure. We should reuse that knowledge to greatly simplify the mapping code and improve performance.

jsdevel
jsdevel reviewed Oct 20, 2015
View reviewed changes
lib/client.js
@@ -6,9 +6,11 @@
"use strict";

function findKey(obj, val) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

can we extract this out into a utility file? I see that it's duplicated in lib/server.js as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure

@jsdevel
Copy link
Collaborator

jsdevel commented Oct 20, 2015

I'm still not happy about the JSON to XML object mapping as-is.

I hear you. NamespaceContext is going to be a great step in moving us more towards a robust soap library though, so I'm glad you've done this work!

The description phase already builds up the tree of elements as the template for XML structure. We should reuse that knowledge to greatly simplify the mapping code and improve performance.

Couldn't agree more.

jsdevel added a commit that referenced this pull request Oct 21, 2015
@jsdevel
Merge pull request #756 from strongloop-forks/feature/local-element
bba7b25 
Fix xml namespace/element/type handling
@jsdevel jsdevel merged commit bba7b25 into vpulim:master Oct 21, 2015
@jsdevel
Copy link
Collaborator

jsdevel commented Oct 21, 2015

Another great PR @raymondfeng ! Thank you!

diarmaidm pushed a commit to diarmaidm/node-soap that referenced this pull request Feb 3, 2016
@jsdevel
Merge pull request vpulim#756 from strongloop-forks/feature/local-ele…
ef04a8b 
…ment

Fix xml namespace/element/type handling
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@raymondfeng @jsdevel