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.

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

Lookup definitions in child element first #958

Merged
merged 2 commits into from
Oct 30, 2017

Conversation

lfantone
Copy link
Contributor

@lfantone lfantone commented Aug 8, 2017

I have this case where the xmlns definition is added as a xmlns:{prefix} attribute to the element and the wsdl library wasn't able to find the Schema Object because It only looks into the global definitions.
Example of a not working xml schema:

  <xs:extension xmlns:q1="http://schemas.org/Request" base="q1:Info">
    <xs:sequence>
      <xs:element name="id" nillable="true" type="xs:string"/>
    </xs:sequence>
  </xs:extension>

This PR solves the issue describe above by looking the prefix in the child xmlns with a fallback to the this.definitions.xmlns.

I'm not sure if this this needs an specific unit test to being added, if so please let me know I'll try to add it.

@coveralls
Copy link

coveralls commented Aug 8, 2017

Coverage Status

Coverage remained the same at 93.293% when pulling e1f90de on lfantone:fix/child-xmlns into a337681 on vpulim:master.

@herom
Copy link
Contributor

herom commented Aug 10, 2017

@lfantone thanks a lot for your contribution 👍

please add a request/repsonse sample test, as required in our Guidelines on Submitting a Pull Request to make sure your fix won't get overwritten accidentially 🎲

@jsdevel
Copy link
Collaborator

jsdevel commented Aug 10, 2017

@lfantone this fix is very much appreciated! Echoing what @herom said, we hope you can add some request/response samples as this fix is important enough to demand some.

@crash7
Copy link

crash7 commented Oct 27, 2017

@jsdevel @herom Hi! I'm trying to create the request/response test for this, any hints? Thanks!

@jsdevel
Copy link
Collaborator

jsdevel commented Oct 27, 2017

@jsdevel @herom Hi! I'm trying to create the request/response test for this, any hints? Thanks!
I'd just follow existing examples.

@coveralls
Copy link

coveralls commented Oct 30, 2017

Coverage Status

Coverage increased (+0.05%) to 93.648% when pulling 201e4cf on lfantone:fix/child-xmlns into 4487d09 on vpulim:master.

@crash7
Copy link

crash7 commented Oct 30, 2017

@jsdevel @herom let me know if that request/response test is enough! thanks!

@jsdevel
Copy link
Collaborator

jsdevel commented Oct 30, 2017

Awesome job @crash7 ! I love seeing all that green and no red! That's what we're after!

@jsdevel jsdevel merged commit a142aee into vpulim:master Oct 30, 2017
@crash7
Copy link

crash7 commented Oct 30, 2017

@jsdevel excellent 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants