Improve Faraday::Error to get consistent response information

[qsona]

Hi, I faced this problem #956 on my development using faraday. I need to make patch in our private codebase but I also have a motivation to make an improvement to the upstream codebase. I'm glad if I can hear your opinions.

Description

I'm glad if this pull request can achieve these goals:

• Make improvements for faraday v1 (current version) as far as not breaking backward compatibility
  • In this PR, I added some instance methods returning response information to Faraday::Error.
• Make gradual consensus about how to fix that inconsistency problem in faraday v2

Current Problem

I think it is described well in #956 , but I'd like to clarify it here.

The Faraday::Error instance wraps response object. This object is desired to be Faraday::Response instance, but currently this is just a hash including status/body/headers keys. This makes an inconsistency which is a bit annoying to treat for Faraday users.

One more problem (which is my motivation to solve it) is, the body in the response hash is always a raw one, not one rewritten by the response middlewares. For example, I configured as below

(Edited. First I specified the response middlewares in the wrong order. Now it's corrected)

conn = Faraday.new(...) do |conn|
  conn.response :raise_error
  conn.response :json, :content_type => /\bjson$/
end

and send a request. Let me assume that the response status code is 500, and body is JSON data like { "error": "msg"} with the Content-Type: application/json header.

In this case, an error of Faraday::ServerError instance will be raised by the raise_error middleware. I expect that error.response[:body] is a hash { "error" => "msg"} because json middleware exists before the raise_error middleware, but actually it is just a String '{ "error": "msg"}' .

Improvement for v1 codebase

Currently, Faraday::Error#response returns a hash which is not an instance of Faraday::Response. This cannot be changed in v1 without breaking the backward compatibility.

As @technoweenie mentioned here , we can extend the initializers to receive an actual Faraday::Response object, and add some methods to try to get the data from that object. (note: we need to pay attention for that it cannot be got if the error is created in userland)

In this PR, I added 3 methods: response_status, response_body, response_headers.

How it should be in faraday v2 (allowing breaking changes)?

It's not mandatory to discuss this in this PR, but I think it's better to have gradual consensus for the sake of the better change considering the seamless migration.

In my understanding, Faraday::Error instance should wrap the Faraday::Response instance. The current response data (hash) will be no longer necessary because it is a subset of Faraday::Response instance.

One point we can discuss is the signature of initializer. Current one is below. (exc is expected to error message or response hash)

def initialize(exc, response = nil)
end

As mentioned here , the response might be desired to an named argument. So I'd like to suggest the signature basically like below:

# response is a Faraday::Response instance
def initialize(exc = nil, response:)
  @response = response
  # ...
end
attr_reader :response

I'm glad if I can hear your opinion.

Todos

• Documentation (if needed. I think it's not necessary until v2 will be released)

[qsona]

Let me assume that in faraday v2 this signature should be changed to def initialize(exc = nil, response:) (using keyword arg, as I mentioned in the PR description). Then, first I thought to change the signature at this time to such like

def initialize(exc, deperecated_response = nil, response: nil)

but I couldn't to do that because the old response object is hash and it conflicts with the keyword argument.

[qsona]

Can I ignore either Rubocop LineLength check or codeclimate Method on_complete has 32 lines of code (exceeds 25 allowed). ?

[iMacTia]

@qsona thanks for fixing Rubocop line-length cop, I've told CodeClimate not to complain 😄!
I'll review the PR now, thanks for the detailed summary!

[iMacTia]

@qsona ok, so first of all, THANKS for taking the time to tackle this and describing the issue in such details.
It took me some time to get the full picture, but I now have a better idea of what's going on and I believe there's been a small misunderstanding. Allow me to clarify

The issue with the response body parsing is not a code issue

First, your issue with the body not being parsed:

this case, an error of Faraday::ServerError instance will be raised by the raise_error middleware. I expect that error.response[:body] is a hash { "error" => "msg"} because json middleware exists before the raise_error middleware, but actually it is just a String '{ "error": "msg"}' .

I'm afraid you might have misunderstood the middleware order logic. The body is not parsed in your example because the raise_error middleware is added AFTER the json one.
As you can see in this website page, the middleware is traversed top-to-bottom while building the request, but the other way around when processing the response.
This means that if you swap the middleware in your example, you'll get what you want.

Does this solve #956? Yes, but...

Now, moving to the issue raised in #956, that relates to the fact that errors raised by the raise_error middleware contain a hash response, while the Faraday::RetriableResponse exception contains an actual Faraday::Response object.
The issue with that is that if you rescue them together, it gets hard to access the right properties (as shown in the example).
Your PR above partially solves this, so I welcome the work you've done, but I believe it does it in over-complicated way.
I understand why you want to pass the Faraday::Response object to the other exceptions, but even doing so, this object is not available from outside, and the helper methods you defined can be defined without it. For example, response_status can be defined as

def response_status
  @response[:status]
end

How to proceed

So basically what I'm saying is that, although I understand where you're coming from, I can't really come to justify the introduction of response and deprecated_response.
Here is my suggestion on how to proceed:

1. Revert lib/faraday/response/raise_error.rb file completely
2. Revert Faraday::Error#initialize to the previous state
3. Update response_status, response_body and response_headers implementation to use @response as in my example above.
4. Leave all the new tests as you've written them

If my understanding is correct, with the changes above all tests should still pass, so #956 would be fixed.

[qsona]

The body is not parsed in your example because the raise_error middleware is added AFTER the json one.

First of all, I'm very sorry about this - actually I mistakenly wrote the PR description, but I specified in the correct order in my private source code. I've fixed the description.

[qsona]

@iMacTia I appreciate for your kind advice.

I agree with the plan you suggested. I mixed up a suggestion for future Faraday::Error/raise_error spec (which was my main motivation) and changes for current code, and actually I was also a bit confusing. I'd like to separate these two concerns.

About future spec, let me create another issue; meanwhile, I uploaded my private code . This is basically what I want, and so I want to discuss this topic (I don't want to remain such a monkey patch in our private code forever).

About changes for current code (v1), I'd like to fix in this PR based on your advice. Please give me a few days!

[iMacTia]

Thanks again @qsona for all the work in collecting details and providing a first solution (with tests!) 🙌
I've collected your code and my comments into #1229 so this can now be closed

[qsona]

Oh, I'm really sorry that I completely forgot to work for this. I just noticed by seeing your release note. Thank you very much for your great works.

[iMacTia]

No worries at all @qsona! On the contrary, thanks for getting the ball rolling 🙏