Skip to content

Commit

Permalink
Basically revert pull request #15
Browse files Browse the repository at this point in the history
@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.
  • Loading branch information
tjarratt committed Dec 9, 2013
1 parent 3942790 commit d079227
Show file tree
Hide file tree
Showing 7 changed files with 18 additions and 18 deletions.
4 changes: 2 additions & 2 deletions lib/wasabi/parser.rb
Original file line number Diff line number Diff line change
Expand Up @@ -269,9 +269,9 @@ def input_output_for(operation, input_output)

# Fall back to the name of the binding operation
if message_type
[message_ns_id, message_type]
[message_ns_id, operation_name]
else
[port_message_ns_id, port_message_type]
[port_message_ns_id, operation_name]
end
else
[nil, operation_name]
Expand Down
2 changes: 1 addition & 1 deletion spec/wasabi/document/authentication_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@

its(:operations) do
should == {
:authenticate => { :input => "authenticate", :output => "authenticateResponse", :action => "authenticate", :namespace_identifier => "tns" }
:authenticate => { :input => "authenticate", :output => "authenticate", :action => "authenticate", :namespace_identifier => "tns" }
}
end

Expand Down
2 changes: 1 addition & 1 deletion spec/wasabi/document/multiple_namespaces_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
it { should have(1).operations }

its(:operations) do
should == { :save => { :input => "Save", :output=>"SaveResponse", :action => "http://example.com/actions.Save", :namespace_identifier => "actions", :parameters => { :article => { :name => "article", :type => "Article" } } } }
should == { :save => { :input => "Save", :output=>"Save", :action => "http://example.com/actions.Save", :namespace_identifier => "actions", :parameters => { :article => { :name => "article", :type => "Article" } } } }
end

its(:type_namespaces) do
Expand Down
6 changes: 3 additions & 3 deletions spec/wasabi/document/namespaced_actions_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,9 @@

its(:operations) do
should include(
{ :delete_client => { :input => "Client.Delete", :output => "Client.DeleteResponse", :action => "http://api.example.com/api/Client.Delete", :namespace_identifier => "tns" } },
{ :get_clients => { :input => "User.GetClients", :output => "User.GetClientsResponse", :action => "http://api.example.com/api/User.GetClients", :namespace_identifier => "tns" } },
{ :get_api_key => { :input => "User.GetApiKey", :output => "User.GetApiKeyResponse", :action => "http://api.example.com/api/User.GetApiKey", :namespace_identifier => "tns" } }
{ :delete_client => { :input => "DeleteClient", :output => "DeleteClient", :action => "http://api.example.com/api/Client.Delete", :namespace_identifier => "tns" } },
{ :get_clients => { :input => "GetClients", :output => "GetClients", :action => "http://api.example.com/api/User.GetClients", :namespace_identifier => "tns" } },
{ :get_api_key => { :input => "GetApiKey", :output => "GetApiKey", :action => "http://api.example.com/api/User.GetApiKey", :namespace_identifier => "tns" } }
)
end

Expand Down
6 changes: 3 additions & 3 deletions spec/wasabi/document/no_namespace_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,9 @@

its(:operations) do
should include(
{ :get_user_login_by_id => { :input => "GetUserLoginById", :output => "GetUserLoginByIdResponse", :action => "/api/api/GetUserLoginById", :namespace_identifier => "typens" } },
{ :get_all_contacts => { :input => "GetAllContacts", :output =>"GetAllContactsResponse", :action => "/api/api/GetAllContacts", :namespace_identifier => "typens" } },
{ :search_user => { :input => "SearchUser", :output =>"SearchUserResponse", :action => "/api/api/SearchUser", :namespace_identifier => nil } }
{ :get_user_login_by_id => { :input => "GetUserLoginById", :output => "GetUserLoginById", :action => "/api/api/GetUserLoginById", :namespace_identifier => "typens" } },
{ :get_all_contacts => { :input => "GetAllContacts", :output =>"GetAllContacts", :action => "/api/api/GetAllContacts", :namespace_identifier => "typens" } },
{ :search_user => { :input => "SearchUser", :output =>"SearchUser", :action => "/api/api/SearchUser", :namespace_identifier => nil } }
)
end

Expand Down
2 changes: 1 addition & 1 deletion spec/wasabi/document/savon295_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@

its(:operations) do
should include(
{ :sendsms => { :input => "sendsmsRequest", :output => "sendsmsResponse", :action => "sendsms", :namespace_identifier => "tns" } }
{ :sendsms => { :input => "sendsms", :output => "sendsms", :action => "sendsms", :namespace_identifier => "tns" } }
)
end

Expand Down
14 changes: 7 additions & 7 deletions spec/wasabi/parser/no_message_parts_spec.rb
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
require "spec_helper"
require 'spec_helper'

describe Wasabi::Parser do
context "with: no_message_parts.wsdl" do
context 'with: no_message_parts.wsdl' do

subject do
parser = Wasabi::Parser.new Nokogiri::XML(xml)
Expand All @@ -11,16 +11,16 @@

let(:xml) { fixture(:no_message_parts).read }

it "falls back to using the message type in the port element" do
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.
subject.operations[:save][:input].should == "SaveSoapIn"
subject.operations[:save][:input].should == 'Save'

# Operation's output has part element in the message, so using part element's type.
subject.operations[:save][:output].should == "SaveResponse"
subject.operations[:save][:output].should == 'Save'
end

it "falls back to using the namespace ID in the port element" do
subject.operations[:save][:namespace_identifier].should == "actions"
it 'falls back to using the namespace ID in the port element' do
subject.operations[:save][:namespace_identifier].should == 'actions'
end
end
end

4 comments on commit d079227

@cklatt
Copy link

@cklatt cklatt commented on d079227 Dec 10, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this change introduces a bug that improperly returns the operation_name in all cases when calling input_output_for
The following section of code is incorrect:

Fall back to the name of the binding operation

    if message_type
      [message_ns_id, operation_name]
    else
      [port_message_ns_id, operation_name]
    end
  else
    [nil, operation_name]

I believe it should be:

Fall back to the name of the binding operation

    if message_type
      [message_ns_id, message_type]
    else
      [port_message_ns_id, port_message_type]
    end
  else
    [nil, operation_name]

@tjarratt
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey, @cklatt, thanks for the feedback. That's basically how this code was prior to this commit. I'd like to discuss this more, ideally in a public way here on github, because I'm not entirely sure this was the absolute correct change to make.

Some context may help explain this from the point of view of Savon users. A year ago, @leoping issued pull request #15 where this changed, and it was merged in. Five months ago, this PR was merged in and closed and shortly afterwards a new version of Savon was released, and people started to complain about their requests erroneously having the string Request appended to the end of various message tags. From that point of view, it is clear that this change broke some functionality in Savon and that making this change in a minor version was probably not the correct thing to do.

A short list of issues filed in Savon as a direct result of PR #15 should be

Now, I suspect we want something in between. Clearly sometimes we should use message_type or port_message_type, but other times we need to fall back to the operation name. From what I can tell, the current logic is not sufficient. Further evidence for this comes from @davidsantoso who filed savonrb/savon#530 today. I verified that these changes actually broke his code, so it's clear that in some cases we do need to use the message type and not the operation name.

Could you explain how you came to your conclusion, and maybe help clarify where this bug is? From the testing I've done, it's pretty clear that just reverting this to how it was a few days ago is not going to fix everything for everyone, so there must be a more subtle change needed. Do you use Wasabi on its own, or as part of another gem? How did you discover this was an issue?

Thanks!

@leoping
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @tjarratt, thanks for investigating and fixing this issue. It seems that this commit did not completely revert the original PR. If you look at this line that got removed in the original PR: https://github.com/savonrb/wasabi/pull/15/files#diff-7c3adc3042c46c5df567b221e60c3442L169

This would be the correct code to revert back to:

if message_type
      [message_ns_id, message_type]
    else
      [port_message_ns_id, operation_name]
    end
  else
    [nil, operation_name]

Thanks.

@leoping
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @tjarratt, I've made #39 for the issue mentioned by @cklatt above.

I'm no wsdl expert, but I think a more thorough way to fix this is to have different fallback logic for operation input vs. output. For operation input, the fallback should be the original logic in input_for before #15, which is also what I reverted the logic to in #39.

For operation output however, it should be:

if message_type
      [message_ns_id, message_type]
    else
      [port_message_ns_id, port_message_type]  # fallback to port_message_type instead of operation_name if message_type can't be found.
    end
 else
    [nil, operation_name]  # fallback to operation_name if port_message_type can't be found.

Look at this XML from a wsdl file that we're using for operation 'renewConversation' output 'impl:renewConversationResponse':

<wsdl:portType name="CobrandLogin">
    <wsdl:operation name="renewConversation">
        <wsdl:input message="impl:renewConversationRequest" name="renewConversationRequest" />
        <wsdl:output message="impl:renewConversationResponse" name="renewConversationResponse" />
    </wsdl:operation>
</wsdl:portType>

<wsdl:message name="renewConversationResponse">
    <wsdl:part element="impl:renewConversationResponse" name="parameters" />
</wsdl:message>

<element name="renewConversationResponse">
    <complexType>
        <sequence>
            <element name="renewConversationReturn" type="tns2:CobrandContext" nillable="true" minOccurs="0" />
        </sequence>
    </complexType>
</element>

In this example:

  • message_type is set to 'renewConversationResponse'
  • port_message_type is also set to 'renewConversationResponse'
  • operation_name is set to 'renewConversation'

Clearly, in this case port_message_type is a good candidate to fallback to if message_type is not found.

What do you think?

Thanks.

Please sign in to comment.