savonrb / savon

Heavy metal SOAP client
https://www.savonrb.com
MIT License
2.07k stars 616 forks source link

Savon 2: Mangling the XML response into a broken hash! #558

Closed apenney closed 6 years ago

apenney commented 10 years ago

Hi,

I've been working with Savon2 for a few weeks now and I keep coming across similar examples of this, but I've finally backed myself into a corner and can't work around it. I'm using savon to manage an F5 device and in this specific case when I try to get some information from it I get a bad hash back:

transport[wsdl].get(:get_agent_listen_address)

Which gives me the following blob of XML:

<E:Body>
<m:get_agent_listen_addressResponse
        xmlns:m="urn:iControl:Management/SNMPConfiguration">
<return
        s:type="A:Array"
        A:arrayType="iControl:Management.SNMPConfiguration.AgentListenAddressPort[2]">
<item>
<transport
        s:type="iControl:Management.SNMPConfiguration.TransportType">TRANSPORT_TCP6</transport>
<ipport
        s:type="iControl:Common.IPPortDefinition">
<address
        s:type="y:string"></address>
<port
        s:type="y:long">161</port>
</ipport>
</item>
<item>
<transport
        s:type="iControl:Management.SNMPConfiguration.TransportType">TRANSPORT_UDP6</transport>
<ipport
        s:type="iControl:Common.IPPortDefinition">
<address
        s:type="y:string"></address>
<port
        s:type="y:long">161</port>
</ipport>
</item>
</return>
</m:get_agent_listen_addressResponse>
</E:Body>

If you look for the

pair above you'll see what we actually have is:

<address
        s:type="y:string"></address>

When I run:

transport[wsdl].call(:get_agent_listen_address).body

That's turned into:

=> {:get_agent_listen_address_response=>
  {:return=>
    {:item=>
      [{:transport=>"TRANSPORT_TCP6",
        :ipport=>
         {:address=>{:"@s:type"=>"y:string"},
          :port=>"161",
          :"@s:type"=>"iControl:Common.IPPortDefinition"}},
       {:transport=>"TRANSPORT_UDP6",
        :ipport=>
         {:address=>{:"@s:type"=>"y:string"},
          :port=>"161",
          :"@s:type"=>"iControl:Common.IPPortDefinition"}}],
     :"@s:type"=>"A:Array",
     :"@a:array_type"=>
      "iControl:Management.SNMPConfiguration.AgentListenAddressPort[2]"},
   :"@xmlns:m"=>"urn:iControl:Management/SNMPConfiguration"}}

The issue being that instead of address=>'' I get :address=>{:"@s:type"=>"y:string" and other bad elements like ipport containing a key of :"@s:type".

I don't know if this is a genuine bug parsing the XML, some other weird local issue, or just something I can control with an appropriate setting but I'm stumped.

tjarratt commented 10 years ago

I'm wondering if this occurred as part of bumping some dependencies in the last few releases. While I haven't seen anything quite like this, I can imagine this being a bug in Nori where we are translating the XML to a hash incorrectly.

I was able to reproduce this by just pasting that XML into Nori.new.parse(...), so that's probably a good point to investigate what's going on. There are quite a few options in Nori that Savon passes in, and a few that I think Savon users can control. When I get some time this week, I'll try to follow up, but I hope this helps point you in the right direction @apenney.

Really appreciate the issue. This kind of annoying bug is really frustrating to work with on your own, so I'm feeling guilty that I can't just magic up a solution right now.

tjarratt commented 10 years ago

Okay, I dug into this a little more and what's going on is to be expected, from Nori's perspective. Tags with attributes are turned into nested hashes where the attribute is the key

To answer your question around how you might control this behavior, you can pass several options to Savon::Client.new that are passed along to Nori, including :strip_namespaces and convert_tags_to, which are useful for stripping namespaces from tags and passing your own custom tag converter to Nori, respectively.[1]

I suppose the question to ask is how would you expect this XML to be returned? The XML here seems a little pathological, but it doesn't seem completely unreasonable that Nori could parse it into a nicer hash.

[1]https://github.com/savonrb/savon/blob/master/lib/savon/response.rb#L101

apenney commented 10 years ago

Thanks for looking into this so fast! I'm currently passing in:

Savon.client(wsdl: wsdl_path, ssl_verify_mode: :none, basic_auth: [@username, @password], endpoint: url, namespace: namespace, convert_request_keys_to: :none, strip_namespaces: true, log: false)

I'll have to experiment with convert_tags_to, which I haven't been using. In terms of what I'm expecting is pretty much what you're suggesting, for it to completely ignore the namespaces in tags so that it would effectively render

into :address => ''. I'll play around with convert_tags_to and see what I can do there!

apenney commented 10 years ago

Oh, convert_tags_to is just mapped to convert_request_keys_to, which I'm already trying to disable. In that case I'm not sure where to go from here. :/

apenney commented 10 years ago

Having looked around Nori I think what I really need is a "ignore_tag_attributes" altogether option when parsing.

tjarratt commented 10 years ago

I'm in favor of that. Tag attributes tend to get in the way sometimes. If you'd like to issue a PR for that behavior, that would be awesome, but I'll add it to my backlog of things to investigate this weekend.

apenney commented 10 years ago

I started looking into a PR for this, but I'm struggling to work out where this belongs. I made a new option, wrote an api_spec test, and while exploring xml_utility_node I found:

if converter = options[:convert_attributes_to]

Would I be able to hijack this to do something like:

converter = lambda {|key, value| [] }

To completely destroy attributes and stop them being parsed? I'm not sure if that's the right route to go. It looks like nori has moved far enough beyond 2.3.0 that https://github.com/apenney/nori/commit/e133558043e4c1b2aa481bd9ade049af5491a267 probably contains 99% of what I need?

apenney commented 10 years ago

[5] pry(main)> Nori.new(:convert_attributes_to => lambda {|key, value| [] }).parse('active') => {"userResponse"=>{"accountStatus"=>"active"}}

That looks like it would do exactly what I want!

apenney commented 10 years ago

I've made #559 as a starter PR for adding the Savon part of the equation. I'm still working on getting it to test against Nori out of master (I'm terrible at ruby) so I'm sure there's probably issues with what I've done but I figured you can just tell me what's lacking.

tjarratt commented 10 years ago

Hey @apenney now that we've merged in #559, can we close this issue?

stale[bot] commented 6 years ago

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[bot] commented 6 years ago

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