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

Faraday::Response - argument order causes unexpected behaviour #1498

Open
opensourceame opened this issue Apr 17, 2023 · 3 comments
Open

Faraday::Response - argument order causes unexpected behaviour #1498

opensourceame opened this issue Apr 17, 2023 · 3 comments
Assignees

Comments

@opensourceame
Copy link

opensourceame commented Apr 17, 2023

Basic Info

  • Faraday Version: 1.10.3 and 2.7.4
  • Ruby Version: 2.7.5

Issue description

When creating a Faraday::Response the body of the response is nullified if the status argument is specified after the body. This very unexpected behaviour.

Steps to reproduce

works as expected:

pry(main)> r = Faraday::Response.new(status: 200, body: 'test')
{
              :status => 200,
                :body => "test",
    :response_headers => nil
}

body is nullified:

pry(main)> r = Faraday::Response.new(body: 'test', status: 200)
{
              :status => 200,
                :body => nil,
    :response_headers => nil
}
@iMacTia
Copy link
Member

iMacTia commented Apr 18, 2023

Thank you for reporting this @opensourceame, I totally agree this is surprising!
Please take a look at this more detailed inspection of the two objects:

[19] pry(main)> Faraday::Response.new(status: 200, body: 'test')
=> #<Faraday::Response:0x0000000136195a78
 @env=
  #<struct Faraday::Env
   method=nil,
   request_body=nil,
   url=nil,
   request=nil,
   request_headers=nil,
   ssl=nil,
   parallel_manager=nil,
   params=nil,
   response=nil,
   response_headers=nil,
   status=200,
   reason_phrase=nil,
   response_body="test">,
 @on_complete_callbacks=[]>
[20] pry(main)>
[21] pry(main)> Faraday::Response.new(body: 'test', status: 200)
=> #<Faraday::Response:0x0000000116e26898
 @env=
  #<struct Faraday::Env
   method=nil,
   request_body="test",
   url=nil,
   request=nil,
   request_headers=nil,
   ssl=nil,
   parallel_manager=nil,
   params=nil,
   response=nil,
   response_headers=nil,
   status=200,
   reason_phrase=nil,
   response_body=nil>,
 @on_complete_callbacks=[]>

Why is this happening?

As you can see from above, body is not an actual field in Faraday::Response, but rather an alias for either request_body or response_body.
And this "dynamic aliasing" works based on status: if status is set, then we assume the response have been fetched and body refers to response_body. Otherwise, no response was fetched, so body still refers to request_body.
You can see that in how [] and []= are implemented for Env: https://github.com/lostisland/faraday/blob/main/lib/faraday/options/env.rb#L89-L105

When you generate the hashes in your examples, body always refer to response_body, because in both cases status is set.
However, the former example sets response_body, while the latter sets request_body, hence the latter example shows body: nil when you print it out.

Is this expected?

Although really convoluted, I'd say this is expected behaviour, because of the nature of the body alias.
Users of the library won't normally instantiate a Faraday::Response because the adapter will do that for them.
Adapters, OTOH, are encouraged to use the save_response method which takes care of this for them among other things.

The real surprise here is that Faraday::Response accepts body as an initializer parameter, I'd have thought that would raise an error (instead, we should use request_body or response_body explicitly).

I'm currently planning some work on those Structs which may solve this confusion, so I'll leave this issue open until then.

@opensourceame
Copy link
Author

opensourceame commented May 2, 2023

Thanks for your response @iMacTia. I'm creating the response manually to create a dummy response when I get a callback from a remote server, which is passed to Ruby code via a queue system. Is there a better way to do this than initialising a response object, as in my example?

Agreed that body should not be accepted as an init param. Looking forward to your improvements ;-)

@iMacTia
Copy link
Member

iMacTia commented May 3, 2023

Is there a better way to do this than initialising a response object, as in my example?

I don't have a good alternative to that to be honest, but if you use a response_body key instead of body, at least you shouldn't have this issue anymore.

@iMacTia iMacTia self-assigned this May 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants