Faraday::ParsingError.response_status doesn't return expected value when stubbing response

[brendo]

Basic Info

• Faraday Version: 2.7.6
• Ruby Version: 3.2.2

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":

  it 'includes the response on the ParsingError instance' do
    process('{') { |env| env[:response] = Faraday::Response.new(status: 502) }
    raise 'Parsing should have failed.'
  rescue Faraday::ParsingError => e
    expect(e.response_status).to eq(502)
    expect(e.response).to be_a(Faraday::Response)
  end

This test will fail:

  1) Faraday::Response::Json includes the response on the ParsingError instance
     Failure/Error: @response[:status] if @response
     
     NoMethodError:
       undefined method `[]' for nil:NilClass
     # ./lib/faraday/error.rb:32:in `response_status'
     # ./spec/faraday/response/json_spec.rb:82:in `rescue in block (2 levels) in <top (required)>'
     # ./spec/faraday/response/json_spec.rb:79:in `block (2 levels) in <top (required)>'
     # /Users/brendanabbott/.gem/ruby/3.2.2/gems/webmock-3.18.1/lib/webmock/rspec.rb:37:in `block (2 levels) in <top (required)>'
     # ------------------
     # --- Caused by: ---
     # JSON::ParserError:
     #   unexpected token at '{'
     #   ./lib/faraday/response/json.rb:30:in `parse'

It appears that response_status will delegate to response[:status] and this will attempt to find the status using the headers of the stubbed response. In this test, the headers are nil, so it blows up.

I'm curious, should it be response.status instead, or should we stubbing our responses as:

Faraday::Response.new(status: 502, response_headers: { status: 502 })

[iMacTia]

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.

What's going on?

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 👍

[brendo]

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

[iMacTia]

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:

• error.response.status
• error.response.env.status

will only work if error.response is a Faraday::Response, which unfortunately is not always the case 😅

[semaperepelitsa]

@iMacTia why is error.response sometimes a hash? Why not always return a proper Response object?

[iMacTia]

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