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

util/errors: Extract CustomApiError struct #7812

Merged
merged 5 commits into from
Dec 28, 2023

Conversation

Turbo87
Copy link
Member

@Turbo87 Turbo87 commented Dec 27, 2023

Instead of having a dedicated struct per HTTP status code (and then some more), we can use a single struct and save the status code inside the struct, next to the error message. This allows us to throw away a significant chunk of verbose code while the API behaves exactly the same as before. Admittedly this removes the ability for e.is::<NotFound>() checks, but we were not and probably shouldn't be using those anyway.

Note that a few structs were not converted to using this "generic" struct yet:

  • InsecurelyGeneratedTokenRevoked because it is used in an e.is::<_>() check
  • ReadOnlyMode because it is used in an e.is::<_>() check
  • TooManyRequests because it is setting a custom header, which CustomApiError does not currently allow

Related:

@Turbo87 Turbo87 requested a review from a team December 27, 2023 10:17
@Turbo87 Turbo87 added C-internal 🔧 Category: Nonessential work that would make the codebase more consistent or clear A-backend ⚙️ labels Dec 27, 2023
Copy link
Contributor

@LawnGnome LawnGnome left a comment

Choose a reason for hiding this comment

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

LGTM.

Losing the ability to assert on specific errors is a little sad, but I assume we could add a way for tests to introspect CustomApiError if we really needed that for any of the errors that have been converted. It's probably not worth keeping the boilerplate that's getting removed here.

@Turbo87
Copy link
Member Author

Turbo87 commented Dec 28, 2023

Losing the ability to assert on specific errors is a little sad

if there are cases where the introspection makes sense then I'm not against having custom error structs for that, but in 99% of our code base we don't need it :)

@Turbo87 Turbo87 merged commit ae54fdb into rust-lang:main Dec 28, 2023
6 checks passed
@Turbo87 Turbo87 deleted the custom-api-error branch December 28, 2023 09:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-backend ⚙️ C-internal 🔧 Category: Nonessential work that would make the codebase more consistent or clear
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants