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

Fix min/maxOccurs parsing and handling #1100

Merged
merged 1 commit into from
Jan 13, 2020

Conversation

kouak
Copy link
Contributor

@kouak kouak commented Dec 9, 2019

Consider this XSD :

                <xs:element name="DummyList" minOccurs="1" maxOccurs="10">
                    <xs:complexType>
                        <xs:sequence>
                            <xs:element name="DummyFirst" type="xs:string" />    
                            <xs:element name="DummySecond" type="xs:string" />
                        </xs:sequence>
                    </xs:complexType>
                </xs:element>

We should expect node-soap to deserialize DummyList as an array regardless of the number of elements in the document.

Consider this response :

<Parent>
    <DummyList>
        <DummyFirst>foo</DummyFirst>
        <DummySecond>foo</DummySecond>
    </DummyList>
</Parent>

Before this PR, this would be transformed to :

{
  "DummyList": {
    "DummyFirst": "foo",
    "DummySecond": "foo"
  }
}

After this PR :

{
  "DummyList": [
    {
      "DummyFirst": "foo",
      "DummySecond": "foo"
    }
  ],
}

We noticed this regression upgrading our app from node-soap 0.26.0 to 0.30.0. We pinpointed its introduction between 0.26 and 0.27, possibly during the typescript rewrite.

Also the former typings were wrong: AFAIK, sax parses minOccurs and maxOccurs attributes to strings making this code not perform correctly unless maxOccurs was set to "unbounded".

@coveralls
Copy link

coveralls commented Dec 9, 2019

Coverage Status

Coverage decreased (-0.5%) to 94.538% when pulling 23271a2 on kouak:fix-min-max-occurs into af93a6f on vpulim:master.

@kouak kouak force-pushed the fix-min-max-occurs branch from 6f44fbe to 5a964f5 Compare December 9, 2019 16:12
@jsdevel
Copy link
Collaborator

jsdevel commented Dec 9, 2019

please fix the build

@kouak
Copy link
Contributor Author

kouak commented Dec 9, 2019

Coveralls seems to be the issue for the failed builds : https://travis-ci.org/vpulim/node-soap/jobs/622742785#L1622-L1625

Not sure how I can fix that though.

@kouak kouak force-pushed the fix-min-max-occurs branch from 5a964f5 to a2c0736 Compare December 10, 2019 10:54
@kouak
Copy link
Contributor Author

kouak commented Dec 13, 2019

I ran the pipeline again by force pushing to the source branch.

Travis CI is all good now. The previous error was probably just a fluke.

@jsdevel
Copy link
Collaborator

jsdevel commented Jan 8, 2020

please revert package-lock.json. also, i can't tell what was changed in the src file because the entire file was formatted. please revert space changes.

@kouak kouak force-pushed the fix-min-max-occurs branch from a2c0736 to 1ac10d3 Compare January 10, 2020 13:38
@kouak kouak force-pushed the fix-min-max-occurs branch from 1ac10d3 to 23271a2 Compare January 10, 2020 13:41
@kouak
Copy link
Contributor Author

kouak commented Jan 10, 2020

My bad, the PR should be reviewable now.

@jsdevel
Copy link
Collaborator

jsdevel commented Jan 10, 2020

thanks @kouak ! looks like coverage decreased. can you cover the other cases that you're adding?

@kouak
Copy link
Contributor Author

kouak commented Jan 10, 2020

Yeah I had a look into that but I'm at lost here ...

If I read the following image correctly, coveralls reports a decreased coverage in src/ and src/wsdl/ but an increase in both src/wsdl/index.ts and src/wsdl/element.ts ?

Capture d’écran de 2020-01-10 21-45-43

Also, it looks like all the added lines are covered if I read this report correctly.

Capture d’écran de 2020-01-10 21-46-37

@jsdevel jsdevel merged commit ead275d into vpulim:master Jan 13, 2020
@jsdevel
Copy link
Collaborator

jsdevel commented Jan 13, 2020

thanks @kouak !

@jsdevel
Copy link
Collaborator

jsdevel commented Jan 13, 2020

@kouak could it be that we now have dead code elsewhere?

@kouak kouak deleted the fix-min-max-occurs branch January 15, 2020 13:26
Danail-Irinkov pushed a commit to Danail-Irinkov/node-soap that referenced this pull request Jan 19, 2020
@kouak
Copy link
Contributor Author

kouak commented Feb 4, 2020

@jsdevel: As far as I'm aware I'd say no.

Could it be possible to prepare a new release including this fix ? That would greatly help our project. Thanks !

@jsdevel
Copy link
Collaborator

jsdevel commented Feb 10, 2020

soon @kouak

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

Successfully merging this pull request may close these issues.

3 participants