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

Tag 2.11.2 fails to handle namespace assignment properly, when compared to 2.11.1 #830

Closed
skoona opened this issue Aug 24, 2017 · 11 comments

Comments

@skoona
Copy link

skoona commented Aug 24, 2017

A recent bundle install picked up release tag 2.11.2 of savon, and broke soap interaction that had been working for quite some time. Error: AgencyId has wrong namespace label in the second example.

Proper XML from 2.11.1
<env:Body> <agen:producerListing> <agen:request> <ins0:AgencyId>0034</ins0:AgencyId> </agen:request> </agen:producerListing> </env:Body>

Improper XML from 2.11.2, 'agen' instead of 'ins0'
<env:Body> <agen:producerListing> <agen:request> <agen:AgencyId>3101</agen:AgencyId> </agen:request> </agen:producerListing> </env:Body>

I've regressed my codeset back to 2.11.1 and applied the Gemfile.lock to keep deployments on track. I file this issue for your info and others.

James,

@svyatov
Copy link

svyatov commented Sep 4, 2017

I can confirm a similar issue with v2.11.2. Had to pinpoint the gem to v2.11.1.

@otherguy
Copy link
Contributor

otherguy commented Sep 6, 2017

Confirmed as well.

fphilipe referenced this issue Sep 21, 2017
* Fixes handling of content! and attributes!
* General cleanup.
@fphilipe
Copy link

fphilipe commented Sep 21, 2017

Confirmed. I've tracked it down I think. Let me try to explain it (sorry if this is not very clear, I only have limited knowledge of SOAP and WSDL).

Before, if a nested tag would have its own top level type definition, that would be used. E.g. when ["A", "B"] and ["B", "C"] are defined and the message has the structure A > B > C, the ["B", "C"] path would be used for the namespace lookup of C.

In fbe0d13 this was removed. Now the structure A > B > C would falsely try to look up the type of C at ["A", "B", "C"] which does not exist.

The bug was introduced by changing this line

to_hash(value, @types[newpath] ? [@types[newpath]] : newpath)

to the following line

to_hash(value, newpath)

Replacing the new line with the old one fixes my problem.

@pcai
Copy link
Member

pcai commented Sep 27, 2017

Hi all, is someone able to give me the outline of a repro / test case? I dont see a PR for this fix yet (it sounds like its a single line change) and I'm happy to make one myself, but I'd like to have coverage for this specific scenario if possible. Once this bug is fixed I will release a new version.

@pcai
Copy link
Member

pcai commented Sep 28, 2017

OK, I wrote up the following rspec, but clearly I don't understand this bug as it fails before and after the change suggested by @fphilipe :

      it "looks up used_namespaces in nested tags correctly" do
        used_namespaces = {
          %w(tns Bar Baz) => 'ns'
        }
        
        input_hash = {
          foo: {
            bar: {
              baz: {
                content!: false
              }
            }
          }
        }
        
        good_result = {
          foo: {
            bar: {
              "ns:Baz" => {
                content!: false
              }
            }
          }
        }

        message = described_class.new(types, used_namespaces, key_converter)
        resulting_hash = message.to_hash(input_hash, ['tns'])
        resulting_xml = Gyoku.xml(resulting_hash, key_converter: key_converter)

        expect(resulting_hash).to eq good_result
        expect(resulting_xml).to eq %(<Foo><Bar><ns:Baz>false</ns:Baz></Bar></Foo>)
      end

result:

Failures:

  1) Savon::QualifiedMessage#to_hash if a key ends with ! looks up used_namespaces in nested tags correctly
     Failure/Error: expect(resulting_hash).to eq good_result
       
       expected: {:foo=>{:bar=>{"ns:Baz"=>{:content!=>false}}}}
            got: {:foo=>{:bar=>{:baz=>{:content!=>false}}}}
       
       (compared using ==)

I don't actually use savon day to day, is someone able to offer a pointer? I suspect at the least that the first argument (types) to the QualifiedMessage constructor should be something other than {}

@aledustet
Copy link

@pcai i have a similar scenario to this. The real issue here is to have some other type definition that would change the namespace. In my case there is a couple of type definitions:

xmlns:tns="http://tempuri.org/"
xmlns:env="http://test.org/soap/envelope/" 
xmlns:ins0="http://test.org/ServiceTypes"

And the data is defined under another set of values. When the lookup is made the types that live on the ins0 type definition don't get matched since those are nested.

So based on this request:

action(message: { request: {'TypeOnIns0' => 'blah'} })

turns into

 <tns:request>
        <tns:TypeOnIns0>REGISTER</tns:TypeOnIns0>

instead of:

 <tns:request>
        <ins0:TypeOnIns0>REGISTER</ins0:TypeOnIns0>

Hope this makes some sense

@stale
Copy link

stale bot commented Mar 18, 2018

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix label Mar 18, 2018
@stale
Copy link

stale bot commented Mar 25, 2018

This issue is now closed due to inactivity. If you believe this needs further action, please re-open to discuss.

@violen
Copy link

violen commented Apr 23, 2019

Still unfixed in Savon 2.

Just to notice others who are using the "Stable" Savon Gem. (And the Maintainers here)

Use this line in your Gemfile gem 'savon', '~> 2.11', '<= 2.11.1'
In newer implementations the Namespace Lookup is broken and you will get errors when working with complex types in XML.

See previous Posts for explanation.

@ghost
Copy link

ghost commented Apr 30, 2019

This should be reopened! @violen is right, the bug is not fixed.

@sergioisidoro
Copy link

Yep, we're also having this problem in latest version, as specified in #830 (comment)

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

No branches or pull requests

8 participants