Faraday::Error does not provide original request URL or params

[mattbrictson]

Basic Info

• Faraday Version: 1.3.0
• Ruby Version: 3.0.0

Issue description

When the raise_error middleware is used, the resulting Faraday::Error object provides no access to the original request URL that triggered the error. This makes it hard to troubleshoot errors and attribute an error to a problematic URL.

Steps to reproduce

require "faraday"

faraday = Faraday.new("https://example.com/") do |config|
  config.response :raise_error
end

begin
  faraday.get("wut", "q" => "hello")
rescue Faraday::Error => e
  puts e.response[:request].inspect
end

Output:

{:method=>:get, :url_path=>"/wut", :params=>nil, :headers=>{"User-Agent"=>"Faraday v1.3.0"}, :body=>nil}

Note that :params is nil, even though I passed "q" => "hello" as params in the .get request. Also there is a :url_path but no full URL that includes the query string.

[iMacTia]

Great catch @mattbrictson, thanks for reporting this!
The request info was recently added, but it seems like some things are not working as expected.
I'd very much welcome a PR to add the url_host and to fix the lack of params (looking at the PR, that seems to work when params are provided in the request url, so something is probably wrong with when they're provided as a separate hash)

[mattbrictson]

@iMacTia I'm wondering if the url itself was omitted from the request info on purpose? Adding a separate url_host will partially fix the problem, but what about url_protocol?

Also I suspect params is problematic because its meaning is ambiguous. If I make a POST request that contains both query string params in the URL and form encoded params in the body, which should appear in params in the error? I suspect the original intention of the code is that it will be body params, not the query string params.

So then we would also need a url_query_params key?

I guess what I am getting at is, would you accept a PR that just adds a url key to the error request data? 🙂

[iMacTia]

@iMacTia I'm wondering if the url itself was omitted from the request info on purpose? Adding a separate url_host will partially fix the problem, but what about url_protocol?

Good point, I guess url_base (protocol+host) could be a good tradeoff then. I'm not really sure why it was omitted in the first place, but to be fair, since you provide that at the connection level, it's probably not as useful as the rest of the path that changes on every request.

Also I suspect params is problematic because its meaning is ambiguous. If I make a POST request that contains both query string params in the URL and form encoded params in the body, which should appear in params in the error? I suspect the original intention of the code is that it will be body params, not the query string params.

Correct, for POST and other requests that would be the body. In Faraday we usually refer to the query-string as params.

I guess what I am getting at is, would you accept a PR that just adds a url key to the error request data?

Absolutely! Either adding the url_base or a full url sounds good to me. The only thing I'd ask is not to change existing fields (so don't remove the url_path please) for backwards-compatibility and add extra tests to cover the new ones 👍

[mvastola]

I have a similar issue whose fix ideally overlaps somewhat with this one:
When a request (with or without raise_error) results in a timeout, it is impossible (as far as I know) to retrieve the details of the original request (at least in a way that's compatible with all other HTTP errors).

There should be a way to tell Faraday to never raise an error resulting from a connection (i.e. this should cover all the different timeout errors, socket errors, SSL errors) and instead, in those cases provide the error in the object returned by Connection#get, et al. Right now I'm developing unit tests for a place we use faraday and I'm realizing there's no easy way to recover information about the request that threw an error if a ruby exception was thrown.

[iMacTia]

You're right @mvastola, I don't think you can retrieve that at the moment.
Looking at TimeoutError it seems like it does support providing a response (which in turn can contain the request), but most of the adapters don't really pass that info, either because they don't have it or because they're just old.

I don't see a straightforward path to support that to be fair, as that would require changing not only the implementation of the error classes but each adapter as well.
I can see this being added to v2.0 so that adapters will be "forced" to comply with the new definition.
So what we could do, is have a discussion on how a better API would look like for error classes in order to better manage this cases, then add that to the v2.0 roadmap 👍

I agree this is related in part with this issues, but we can at least add extra information to Faraday::Error in a backwards-compatible way to solve it, so I'd discuss your point separately

[mvastola]

@iMacTia would you like me to open a new issue?

[iMacTia]

Either that or a GitHub discussion if you prefer 😄

[mvastola]

But also, as for a fix, couldn't you just add an option to wrap Faraday's invocation of the adapter in a begin..rescue block and if something is raised, treat it similarly to any other non-erroring request (i.e. return a blank body, a nil status, and add in a way to access the error?) And if :raise_error is used, re-throw the error wrapped in a Faraday::Error

[iMacTia]

Because of the middleware stack structure, it's not that trivial. Faraday does not directly call the adapter, but injects it at the bottom of the middleware stack when the connection is created.

What you could do as a user though, if this use-case is important for you, is to create your own custom middleware that wraps the call to @app.call into a begin ... rescue block.
I personally don't see the need to add a tacky solution like this one in a rush, and would much rather prefer to solve this properly in v2

[mvastola]

Ah, I get it. I guess you can add something that would always be the closest middleware to the adapter. Writing custom middleware we have to maintain would not be preferable for us. It would seem like a better fit for faraday-middleware if it were to be done.