Parse Input/Output When Operation Style is Document #114
Conversation
Co-authored-by: Matt Polito <[email protected]> Co-authored-by: Tony Yunker <[email protected]>
|
Any feedback on this one? |
|
Thanks for your patience. I'm going through this today to try and understand the change |
| @@ -247,8 +248,7 @@ def input_output_for(operation, input_output) | |||
| port_type_operation = @port_type_operations[binding_type][operation_name] | |||
| end | |||
|
|
|||
| port_type_input_output = port_type_operation && | |||
| port_type_operation.element_children.find { |node| node.name == input_output } | |||
| port_type_input_output = port_type_operation&.element_children&.find { |node| node.name == input_output } | |||
There was a problem hiding this comment.
just noting this: the lonely operator was introduced in ruby 2.3x but this gem still declares compatibility with >= 1.9.2
in practice, this is most likely an oversight, as savon projects have a de facto policy of covering only supported rubies (1.9x isnt even covered in the test matrix)
| @@ -15,7 +15,7 @@ | |||
|
|
|||
| it 'falls back to using the message type in the port element' do | |||
| # Operation's input has no part element in the message, so using the message type. | |||
| expect(subject.operations[:save][:input]).to eq('Save') | |||
| expect(subject.operations[:save][:input]).to eq("SaveSoapIn") | |||
There was a problem hiding this comment.
so if i understand correctly, this value is coming from here? https://github.com/savonrb/wasabi/blob/master/spec/fixtures/no_message_parts.wsdl#L51
How did this work before? it would fall back to here? https://github.com/savonrb/wasabi/pull/114/files#diff-42bb513f8ac3b44919b550f9374bdadf8ebc8130dd0f1776571d6aba51e07c2dR292
|
OK This seems good to me, sorry for the silly questions as I am no longer involved in an org that uses savon/wasabi, but my main concern is -- Is there any universe where the current behavior (without this PR) would be considered correct? In context these changes appear good, but I am somewhat apprehensive about merging this in, cutting a bugfix release, and then getting a complaint that it breaks someone's working code. If there's a risk of that, then perhaps a major version bump is in order? Note that I have updated main branch to clarify we are 2.7+ so the use of lonely operator should not be an issue |
|
Thanks for taking a look. We were hoping you might have some insight into if it was a sane change. It works for us and obviously we made it so all tests pass. However we can only verify it against the wsdl we're currently using. |
|
OK so in summary we aren't sure if this is safe but this fixes a problem with a well known provider, I assume this affects others we don't know about too. I will merge this and cut a release and we'll deal with any side effects as they come up. I will also do a major version bump alongside this for the change in supported ruby versions and to ensure this gets extra scrutiny downstream. Thanks for your contribution. |
|
This seems like an appropriate move. Thanks for looking over it. If you need help maintaining the savon repos, let me know. I'd like to help where you need it. |
|
@mattpolito i would like to take you up on that, couldn't find your contact info and twitter dms seem broken 👎🏻 , can you email me? |
|
@pcai I reached out via email. |
Fixes a regression introduced in #114
Problem:
When parsing operations of the Workday API we were getting incorrect input values. The operation here is not declared with a
soapActionso it never made it into the logic to parse inputs/outputs.Note:
Please review changes to specs. As far as we can tell the specs were not accurately describing parsing of inputs/outputs, hence the changes to reflect 'correct' parsing.
However, we fully understand that someone wrote the specs with those assertions in mind. So there is an expectation that they should work that way.
Please let us know if these assumptions are incorrect so we can try and adjust.