lostisland / faraday

Simple, but flexible HTTP client library, with support for multiple backends.
https://lostisland.github.io/faraday
MIT License
5.72k stars 972 forks source link

regression with 2.5.1: ArgumentError: unknown keyword #1444

Closed rocket-turtle closed 2 years ago

rocket-turtle commented 2 years ago

Basic Info

Issue description

With the new Version I get an ruby deprecation and my test is failing:

../faraday-2.5.1/lib/faraday/adapter/test.rb:287: warning: Using the last argument as keyword parameters is deprecated; maybe ** should be added to the call
../faraday-2.5.1/lib/faraday/adapter.rb:62: warning: The called method `save_response' is defined here
    returns a predefined response (FAILED - 1)

Failures:

  1) Test::Client#disabled_api_connection returns a predefined response
     Failure/Error: response = connection.get('')

     ArgumentError:
       unknown keyword: :"Content-Type"
     # ../faraday-2.5.1/lib/faraday/adapter.rb:62:in `save_response'
     # ../faraday-2.5.1/lib/faraday/adapter/test.rb:287:in `call'
     # ../faraday-2.5.1/lib/faraday/rack_builder.rb:153:in `build_response'
     # ../faraday-2.5.1/lib/faraday/connection.rb:445:in `run_request'
     # ../faraday-2.5.1/lib/faraday/connection.rb:200:in `get'
     # ../spec/models/test/client_spec.rb:323:in `block (3 levels) in <top (required)>'

I think the Problem is that https://github.com/lostisland/faraday/commit/6799f5852d31dae32699949790633c68282ab71d added an keyword parameter to save_response and now the headers from lib/faraday/adapter.rb:62 are splitted into the arguments and cause the Problem.

Steps to reproduce

I do not know why the tests for the Adapter do not show the same error. I can investigate further if you can not reproduce the error.

iMacTia commented 2 years ago

Thanks for reporting this @rocket-turtle, I'm taking a look 👍

iMacTia commented 2 years ago

For starters, I can now tell why this was not captured by tests:

# this works as expected
save_response(env, 200, 'test', {'test' => 'test'})

# this raises an unknown keyword: :test error
save_response(env, 200, 'test', {test: 'test'})

It turns out, Ruby 2.7 will attempt to use the last hash argument as kwargs only if the keys are symbols

iMacTia commented 2 years ago

I've merged a fix and will soon release this as v2.5.2 👍

rocket-turtle commented 2 years ago

Thank you for the fast fix.

It turns out, Ruby 2.7 will attempt to use the last hash argument as kwargs only if the keys are symbols

That is interesting. Lets hope no other Adapter uses save_response with a Symbole Hash

iMacTia commented 2 years ago

I actually checked all the known adapters (at least those documented in awesome-faraday), and since they all support the reason_phrase, they should all be unaffected 👍 (as reason_phrase will act as a separator between headers and kwargs)