Closed brendo closed 1 year ago
Thank you for opening this @brendo, it does look like a bug indeed! I'm not sure if this was introduced just recently, but in any case test coverage does not highlight the issue.
The stacktrace above was not really helpful, but I managed to reproduce this locally and get a more interesting one:
res = Faraday::Response.new(status: 502)
res[:status]
# => warning: Faraday::Response#[] at
# /.rvm/gems/ruby-3.2.2/gems/faraday-2.7.6/lib/faraday/response.rb:30 forwarding to private method NilClass#[]
# /.rvm/rubies/ruby-3.2.2/lib/ruby/3.2.0/forwardable.rb:238:in `[]': undefined method `[]' for nil:NilClass (NoMethodError)
And that's becasue Faraday::Response#[]
will forward to #headers
.
Now, I suspect this was originally OK for :status
, and the reason it doesn't work for you is that you're not setting headers, so your last snippet should solve the issue for you.
However, looking at the following methods in Faraday::Error
, #response_headers
and #response_body
, it's clear that the intention there was to access Env
properties from the response, which is not what happens using #[]
.
The confusion arise from the fact that Faraday::Error#response
can be either a Faraday::Response
or a Hash
.
I previously attempted to address this by introducing helper methods in #1229, but it's clear the current helpers implementation doesn't work as expected.
I'll get those fixed 👍
Thank you so much! We were going a bit crazy and after logging this issue also ran into similar issues with response_body
. Is it correct that there are currently a few ways to fetch status (and body/headers)? Are any more correct than others? We were assuming error.response_status
was the intended API.
error.response_status
error.response.status
error.response.env.status
Absolutely, you should use those helpers because they abstract the underlying structure which again can be either a hash or a Faraday::Response
.
This means that the following:
will only work if error.response
is a Faraday::Response
, which unfortunately is not always the case 😅
@iMacTia why is error.response sometimes a hash? Why not always return a proper Response
object?
It's mostly due to historical reasons, hence why the helpers have been put in place to mitigate the issue, but some people's code might rely on using error.response
directly and expect either one object or the other.
Also, existing middleware raise errors passing both, so the only way to address this would be a breaking change
Basic Info
Issue description
I'll start off by saying I'm not sure this is an error in production, but we've encountered it when attempting to mock/test our application's handling of odd responses. In particular, we're currently testing that when a parsing error occurs that the HTTP status code will be what the server returned.
Steps to reproduce
I slightly modified the existing test in
json_spec
to demonstrate the "bug":This test will fail:
It appears that
response_status
will delegate toresponse[:status]
and this will attempt to find the status using the headers of the stubbed response. In this test, the headers arenil
, so it blows up.I'm curious, should it be
response.status
instead, or should we stubbing our responses as: