Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not override hash method on Savon::Response #950

Closed
BroiSatse opened this issue Feb 16, 2021 · 3 comments
Closed

Do not override hash method on Savon::Response #950

BroiSatse opened this issue Feb 16, 2021 · 3 comments
Assignees

Comments

@BroiSatse
Copy link

Bug report

Method Savon::Response#hash is returning a result of Nori parse. hash is a special Ruby method and should almost never be overriden (unless for matching override of == method in which case it has to return a number). This can lead to a number of unexpected issues, one of which is explained below.

Steps to reproduce current behavior:

Having following module stub:

module MonitorStub
  def self.call(response:, other_arg:)
  end
end

and following rspec test stub:

  allow(MonitorStub).to receive(:call)
  MonitorStub.call(response: savon_response_object, other_arg: 1)
  
  expect(MonitorStub).to have_received(:call).with(
    response: savon_response_object,
    other_arg: 2
  )

The test is expected to fail due to other_arg mismatch, however rspec is unable to generate the failure message as it tries to count call by arguments. It does it by all_call_args.group_by(&:itself).map {|args, calls| [args, calls.count]}. and since savon_response_object.hash returns instance of Hash rather than number, it cannot be part of hash key, leading to:

*** TypeError Exception: no implicit conversion of Hash into Integer

Expected behavior:

I'd expect to see correct matcher failure message.
The only step required to fix this and potentially number of other issues is to rename Savon::Response#hash to nori_hash or parsed_response.

System information:

  • ruby version: 2.7.2p137
  • savon version: 2.12.1
@olleolleolle
Copy link
Contributor

I agree with this, a rename would be prudent.

@stale
Copy link

stale bot commented Jan 8, 2022

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 stale bot added the stale label Jan 8, 2022
@pcai pcai removed the stale label Oct 17, 2022
@pcai pcai self-assigned this Oct 17, 2022
@pcai
Copy link
Member

pcai commented Oct 19, 2022

resolved in #985

@pcai pcai closed this as completed Oct 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

3 participants