Skip to content
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

Feature Suggestion: Make RaiseError middleware configurable to not raise error on certain status codes (e.g. 404) #1587

Closed
clemens opened this issue Sep 6, 2024 · 1 comment · Fixed by #1590

Comments

@clemens
Copy link
Contributor

clemens commented Sep 6, 2024

The RaiseError would IMO be significantly be more useful if it allows for somewhat flexible configuration of which status codes may not be treated errors. The most prominent example is a 404 status code, which might be entirely expected, such as in cases where you need to implement upsert behavior in an API that doesn't offer PUT semantics:

existing_object = client.get('/foo/1').body

if existing_object
  client.patch('/foo', data)
else
  client.post('/foo', data)
end

If I generally want to raise errors for 4xx/5xx status codes and thus enable the RaiseError middleware, in order to implement such a case, I'd have to clumsily wrap it:

existing_object = begin
  client.get('/foo/1').body
rescue Faraday::ResourceNotFound
end

# ...

I could imagine an API like f.response :raise_error, allowed_statuses: [404] that consequently just doesn't raise errors for the status codes in that list.

I'm happy to provide a PR with the feature implemented. But before I invest time, I just wanted to know whether you'd be willing to accept such a change and if yes, if you have any strong opinions on what the option should be called (allowed_statuses? whitelisted_statuses? dont_raise_on? etc.).

@clemens clemens changed the title RaiseError middleware doesn't allow for configuration of status codes that trigger an exception Feature Suggestion: Make RaiseError middleware configurable to not raise error on certain status codes (e.g. 404) Sep 6, 2024
@iMacTia
Copy link
Member

iMacTia commented Sep 10, 2024

Hi @clemens, thank you for opening this issue and asking before jumping onto an implementation.

I can totally see the value in adding such a feature, as it would mean you can control if/when to raise when configuring the connection, rather than having to rescue the right exception every time you use it.

I still think rescuing the errors allows for more granular control and is overall a better pattern because the Faraday connection should be a 1:1 mapping to an entire API, while this sort of behaviour (i.e. "allow these 4xx or 5xx statuses") seems more of an exception you want to make on an endpoint-by-endpoint case.
So the rationale is that you'd normally wrap the API communication behind some sort of "Client" class that internally manages a Faraday connection, and exposes methods to call API endpoints. Hence these methods are the perfect place to rescue these exception and customise how each endpoint behaves.

That said, I have no strong opinions against making this easier to configure at the connection level.
So if you think the scenario above doesn't really fit your use case, I'd be happy to review a PR that ticks all the boxes:

  • Add allowed_statuses (name looks good 👍) to the list of options for the raise_error middleware
  • Test coverage for the new option
  • Updated docs (see raising-errors.md)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants