Operation output #15
Operation output #15
Conversation
…tion's input message type was not correct. Also added operation's output for each operation.
|
Hi, this is a co-worker of leoping. I looked at the Travis build and it seems to be passing on all platforms except jruby, which fails with a gem install error (nokogiri). Since we haven't changed any gem dependencies in our pull request, we are not sure why this is happening or how to fix it, but it seems to not be related to our changes. |
|
hm. that's a strange reason for failure. i'll investigate further when i have a chance. |
|
@rubiii - do you think that the operation output should be included in the return value of |
|
that said, i'm not sure that i agree with the implementation of the feature in this PR. |
|
@timabdulla well, basically the code was moved from i'm not sure whether this new piece of information can be used to improve savon, but it looks like a nice addition. |
|
Thanks @timabdulla and @rubiii for looking into this PR, and thanks @timabdulla for investigating the failed Travis build for jRuby. @timabdulla, could you please provide a bit more details on your comments about not agreeing to the implementation of the feature at your earliest convenience? We'll be glad to make any changes if necessary. Thanks! |
|
this should be fixed on master. sorry that this took me so long. i updated the changelog to reflect the changes on master. if you would like to test the new code before the release, please give it a try and let me know if it works for you. it's probably still weeks until 4.0.0, but the sooner i can get feedback, the better. thanks. |
|
Thanks @rubiii. We'll test the new code asap and let you know. |
|
@rubiii We cannot test Wasabi 4.0.0 because we also need a version of Savon that uses Wasabi 4.0.0. Is there something else that we can work with? |
ghost
assigned
|
hey @leoping, my plans for savon version 3 changed to not use wasabi, but to include the code for the new parser into savon. since savon version 2 will keep using wasabi, i merged your changes and i'm planning to release them with savon v2.3.0. hope you can test your app against wasabi |
|
Thanks @rubiii! We'll test our code with savon version2 branch and wasabi master and let you know. For Savon version 3, what do you mean by "include the code for new parser into savon"? Which new parser are you referring to? Thanks. |
|
@rubiii, nvm, i guess you mean a new parser similar to lib/wasabi/parser.rb right? |
|
right. savon master is what will become savon version 3. if you look at the code, you can see that it doesn't depend on wasabi any longer, but ships with all the code i've been working on for wasabi 4. |
|
I think part of this patch caused a regression in Savon. The first item in the list of the original PR message is not valid. The name of the operation (and thus the message_tag) should be read from operation[@name] and not from input[@message]. With this patch the behaviour of Savon is not inline with other tools (for example SoapUI). For reference, see my comments in savonrb/savon#520. Thus either this patch should be partially rolled back, or Savon should implement a different logic when deciding on the message tag name. |
|
I'm looking into this issue now. My current thoughts are that @paracycle's analysis is spot on, and that we'll need to revert this issue and figure out which part of this PR caused the regression in Savon. |
@paracycle was kind enough to track down these changes. I believe these changes may be valid for Output, but almost certainly not for Input. I had an opportunity to speak with @rubiii earlier about this and his thoughts were that it wasn't very surprising that this change broke a lot of real code. For one, it also changed specs. Secondly, there wasn't a real example of why this would need to be like this in the PR. Lastly, the end result of the code is overly complicated. I've just spent the last hour looking at the `input_output_for` method in Wasabi::Parser just to find out that this was pulling in a completely bogus request / response tag name in two thirds of the cases. My hopes are that this fix will fix the regression in Savon 2.3.x and we can stop worrying about this for Savon v3.
There's a lot of convoluted logic going on in Wasabi for determining the "correct name" for an operation. Around this time in 2013, a change in PR #15 that made the parser fall back to the port message's message attribute. I searched around in the W3 spec for where this is the correct behavior and all I could find was hints that the operation name should always be used. Unsure if and or when this will ever be determined correctly, but this seems to be a step in the right direction.
This pull request addresses two issues:
operation_nameinstead ofport_mesage_type). For example, see fixed specs where it was (incorrectly) identifying"sendsms"as the input name, instead of the correct"sendsmsRequest".We have tested our changes against the wasabi specs, as well as the specs of savon and savon-multipart, and they all pass (on a local test machine). We will look into the Travis build report ASAP.