Skip to content

Include HTTP method and URL in Faraday::Error messages for improved exception log transparency #1628

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

Merged

Conversation

nielsbuus
Copy link
Contributor

@nielsbuus nielsbuus commented Jun 26, 2025

Description

Created in continuation of #1627

Rubocop violations

I am new to the Faraday code base, so please forgive me if my assumptions or style is wrong. I noticed there is a .rubocop.yml that enforces a maximum line length of 120 characters. I have two violations because the new exception messages are longer. How to deal with that?

Type checking

During the work on exc_msg_and_response in error.rb, I found myself shifting away from the ducktyping style towards a stricter more explicitly type-oriented approach? I like it (that shifting did happen deliberately on my part), but how do the maintainers of Faraday feel about it? Specs don't break, but could there be code hiding somewhere - perhaps in other gems - that relies on the weaker duck-typed approach?

Peculiar URL's generated by the Faraday Test adapter (I think)

I noticed when going through the specs, that the full URL would look something like http:/not-found which I have never seen before. Is this deliberate? Is it a valid URL? It doesn't have a hostname and there seems to be missing a slash after the scheme. I would it expect the generated test url to be something like http://example.com/not-found

The include_request middleware option can limit the functionality

Method and URL cannot be included when this Middleware option for RaiseError is set. I guess if somebody uses that option, they would probably have a reason and might also prefer the existing log messages that only reveal the status code. Any thoughts on this?

Todos

List any remaining work that needs to be done, i.e:

  • Tests - I think it is covered by updating the existing examples.
  • Documentation - maybe? I'm not sure if new documentation should be added. Perhaps mention this change in the changelog at release time, but I think that's on the maintainer to do.
  • 2 rubocop violations - how to deal with that?

Copy link
Member

@iMacTia iMacTia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for opening the PR and for the thorough description, it really helped assessing the work and interpreting what was being done.

I've addressed your main concerns in comments, but basically I'm happy with all the changes. The only thing you'll need to address is the Rubocop error, but I've left a comment with a suggestion on how to do that.

As for your point about the weird http:/ URLs, that surprised me too!
And I don't think this is actually an issue with the test adapter, it seems to be at the connection level, since I was able to reproduce it with the default adapter too:

conn = Faraday.new do |b|
   b.response :logger
end
conn.get('bad-request')
# => I, [2025-06-27T13:56:41.862155 #59699]  INFO -- : request: GET http:/bad-request
# => I, [2025-06-27T13:56:41.862228 #59699]  INFO -- : request: User-Agent: "Faraday v2.12.2"

I wouldn't worry too much as this only happens when no URL is provided when initialising the connection, but I do agree it's weird.
We don't need to address it in scope of this MR though, so don't worry about it 🙂


[nil, exc.to_s, response]
exception_message = if request.nil?
"the server responded with status #{http_status} - method and url are not available due to include_request: false on Faraday::Response::RaiseError middleware"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can overcome the Rubocop error by using Ruby's line continuation:

"the server responded with status #{http_status} - method and url are " \
'not available due to include_request: false on Faraday::Response::RaiseError middleware'

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, I can see why the limitation with the include_request: false option. Since that is NOT the default and the error message clearly explains what is going on, I'd say this is fine 👍

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just went on a bit of a wild goose chase because of this error message because the moneybird gem raises an exception descended from Faraday::Error but passes in a hash without request information. This lead to the "due to include_request: false" text appearing in my logs, even though I don't set that option anywhere.

Should the moneybird just not do that, or is there some way to make sure the error actually comes from Faraday::Response::RaiseError before emitting that message?

For reference: https://github.com/maartenvanvliet/moneybird/blob/master/lib/moneybird/middleware/error_handling.rb

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mvz

I'm not the architect of Faraday and I can see that this change I submitted last month has caused regressions for some people. 😢

I did not assume that anyone was subclassing the Faraday errors or using their own middleware to raise them. If you want to raise subclassed errors, I think you should include the request property in the hash you generate in your response_values method. Alternatively, maybe subclass from StandardError or RuntimeError instead of Faraday::Error.

Perhaps you don't need your own implementation of this middleware, considering that Faraday ships with the RaiseError middleware. However, this would break your consumers error handling if they are rescuing Moneybird specific exceptions, as those would now become Faraday errors.

Maybe @iMacTia can provide a better perspective.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @nielsbuus. I'll open a ticket in the moneybird gem repo as well and see what their thoughts are.

Comment on lines +82 to +85
if exc.is_a?(Exception)
[exc, exc.message, response]
elsif exc.is_a?(Hash)
http_status = exc.fetch(:status)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is perfectly fine, duck typing was OK back in the day, but I think people expect more of this nowadays with the advent of Sorbet and RBS

@nielsbuus
Copy link
Contributor Author

@iMacTia Thank you for the feedback. I have pushed an extra commit to address the linting issue. Do you need anything else to merge?

Copy link
Member

@iMacTia iMacTia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for addressing that, it looks great now 🚀 !

@iMacTia iMacTia merged commit ad8fe1e into lostisland:main Jul 4, 2025
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants