Add failing spec for savon issue #562

[davidsantoso]

Here's a failing spec for that addresses savon issue #562 which follows up on some regressions that seemed to have been reverted in PR #39. Hopefully this kicks off a bit of discussion on how best to move forward with the logic behind properly setting the message tag.

In the case of savon issue #562, the message element may not work for setting the message tag. This section of the parser in particular may need to be refactored a bit to accommodate this WSDL and others like it.

Here's the snippet from the WSDL:

<wsdl:message name="writeCaseEformData">
      <wsdl:part element="flt:FLEformFields" name="WriteCaseEform"/>
</wsdl:message>

<wsdl:operation name="writeCaseEformData">
       <wsdl:input message="fls:writeCaseEformData"/>
       <wsdl:output message="fls:writeCaseEformDataResponse"/>
       <wsdl:fault message="fls:serviceException" name="serviceException"/>
</wsdl:operation>

[tjarratt]

AWESOME. Failing tests are the best. I frequently fall into the trap of believing that writing code that implements new features is the best way to spend my time, but I truly believe that writing good tests and identifying areas that need to be improved is the real heroic part of maintaining open source software. For that, I must thank you @davidsantoso.

In retrospect, it's ... really weird to see this test because once you look at the WSDL it's pretty clear that the current implementation is 100% wrong in this case. Wow.

[davidsantoso]

Glad I can help! Hopefully this shines a bit more light on the issue so we can find a solution that's flexible enough for different WSDL structures.

The current code seems to work for most people, myself included, so, I'm wondering if generally speaking, the message element is what the message tag should be, but in savon issue #562, the WSDL element is just defined differently with the element referencing a complexType further up the document.

<xs:complexType name="FWTCaseEformData">
    <xs:sequence>
        <xs:element name="CaseEformInstance" type="FWTCaseEformInstance"/>
        <xs:element name="EformData" type="FWTEformFieldList"/>
    </xs:sequence>
</xs:complexType>

<xs:element name="FLEformFields" type="FWTCaseEformData"/>

So in this case, the message name is actually what we should use. Or maybe even the message part name?

<wsdl:message name="writeCaseEformData">
      <wsdl:part element="flt:FLEformFields" name="WriteCaseEform"/>
</wsdl:message>

<wsdl:operation name="writeCaseEformData">
       <wsdl:input message="fls:writeCaseEformData"/>
       <wsdl:output message="fls:writeCaseEformDataResponse"/>
       <wsdl:fault message="fls:serviceException" name="serviceException"/>
</wsdl:operation>

On the other hand, with the WSDL I'm using, the name of the message isn't what I want (and I think this may explain the various issues filed where people's message tags were getting 'Request' appended to them, like #520.) but the message part element is correct.

So, all that goes to say, seems like WSDLs have a fair amount of variation. I'd say, the message part name seems to cover the bases, but in the #562 issue WSDL, it's WriteCaseEform and not writeCaseEform and I imagine the capitalization makes a difference.

Thoughts?

[coveralls]

Coverage remained the same when pulling d23ea142550c3d05c47fd9bca5d7596567b9ab0a on davidsantoso:issue-562 into cd191deedf71928f5b2741e76c8ee314d89ff4b9 on savonrb:master.

[tjarratt]

Thanks @davidsantoso for taking the time to write this PR. I'm still not entirely sure which part of the WSDL to refer to as the canonical name. Somewhere in the history of Savon and Wasabi there is an issue where someone refers to the part of the SOAP spec that lays out the rules for how this is supposed to work, but I can't find it at the moment which is frustrating me to no end.

I think that rather than trying to guess at the correct behavior (as I've done in the past, unfortunately), I'm going to take the time to read the spec and identify what the correct behavior is.

[davidsantoso]

Yeah, I figured there were probably rules as to how it should work. I've been meaning to look into it a bit further myself so I'll see if I can track it down. Agreed, probably shouldn't be guessing!

[tjarratt]

Okay, it took me a few hours, but I finally found the section of the W3 spec concerning WSDLs that describes the only correct way to determine the qualified name for an operation.

It seems the correct way is much simpler than what Wasabi is doing today. All one does is find the appropriate tag in the definitions with the same operation name and look at its name property. According to the spec, that element MUST exist.

All in all, fairly exhausting and very abstract stuff. I'm going to take a shot at merging this and fixing it all in one go tonight. Totally would not have been possible without your help, @davidsantoso. Of note, @paracycle has also been very helpful in helping me understand when this regression came to be.

[tjarratt]

Ping @leoping and @cklatt to get their thoughts. We had discussed this briefly back in December and I'd like to get their thoughts (since they've been directly impacted by these changes).

The change I'm proposing right now is to make the operation name take precedence over anything else. This seems to be most correct according to the spec, although my understanding is that most of these hacks and fallbacks in Wasabi are to help accommodate various SOAP services that are not compliant.

[tjarratt]

I'd like to leave this issue open for discussion but I can't actually reopen it. That sucks.

[paracycle]

I'd propose the following approach: The library should, by default, adhere to standards, as you say, and parse the WSDL in that manner. If there are consumers affected by this (ie people working against broken implementations), then the library should add behaviour changes via some optional parameter(s).

This way:

1. Maintainers write the smallest amount of code up front and obtain standards compliance.
2. Most of the consumers of the library are happy.
3. Extra code will only be written if and when there are edge cases.
4. Since edge case code will be protected by a flag/option, maintainers will have a smaller chance of breaking the compliant path.

[davidsantoso]

+1 for @paracycle's proposal. Standards compliance seems like a good bar for the lowest common denominator among WSDL implementations. Unfortunately, 18c4c6a does break with the WSDL I'm working with (I've fixed the spec in PR #47 to show) so I'd say that providing a way to work with broken/bad WSDL implementations would be nice!

[davidsantoso]

@tjarratt just wondering if you had any thoughts on working with broken WSDL implementations? I imagine that in my case with a bad WSDL where the operation name is not what the message tag should be, I could use the message_tag local option?

[tjarratt]

@davidsantoso I think that's the only case where it makes sense to use :message_tag. I've been thinking about this off and on for a few days and I'm beginning to wonder about whether it makes sense to implement some way for users of Savon and Wasabi to specify a convenient way to override this behavior.

In your case, you never want to use the message tag, but you DO want to use the first <wsdl:part> element inside the message.

  <wsdl:message name="GetLeadRequest"> // don't want to use this
    <wsdl:part name="paramsGetLead" element="tns:paramsGetLead"/> // but this would be fine
  </wsdl:message>

Having to manually specify the message tag each time sounds like a huge pain. I wonder if keeping the default behavior standards compliant and allowing users to tell Savon / Wasabi that they would prefer it to pick up the message tag from a different source makes sense.

I'm envisioning something like this

require 'savon'
client = Savon.client do
  wsdl "http://example.org/wsdl"
  read_message_tag_from :port_message_part

  # alternatively
  read_message_tag_from :operation_name

  # or maybe
  read_message_tag_from :input_tag
end

[davidsantoso]

That works for me. It's a bit of an unfortunate situation overall since it's the WSDL not being standards compliant.

[tjarratt]

The more I think about this ... I wonder if the marketo WSDL actually is compliant and there actually is a bug in Savon. @rubiii seemed to think that there were some hard to solve bugs in Savon (especially around Soap v1 and Soap v2 w.r.t. method tags) and that Sekken (neé Savon v3) was a fresh start at solving this.

Might be interesting to throw as many integration tests into Sekken for the "desired" behavior and see how many are passing right now. Just comparing a few operations between Sekken and SoapUI would probably be enough to gauge the scope of this work.

[davidsantoso]

Sounds good to me. If it's ok, I'll submit some PR's to Sekken to migrate over (and add) some integration tests for this and see where we land.