feat(server): Move to structured ApiError #263
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Replaces anyhow::Error with structured error types to enable proper HTTP status codes and clearer error boundaries between the service and API layers. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Enhances ApiError to return structured JSON responses containing the error message and full cause chain, improving debuggability for API consumers. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Consolidates ApiError and ApiErrorResponse into endpoints/common.rs where they logically belong as endpoint utilities. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Changes the FromRequestParts implementation to return ApiError instead of bare StatusCode, ensuring auth failures are logged with full error details and cause chains, and return structured JSON responses. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Changes: - Rename BadRequest → Client for malformed request errors - Rename Service → Server for internal server errors - Change Server variant to contain message and boxed error source - Implement From<ServiceError> manually (no #[from]) to allow future flexibility - Add logging for Server errors in IntoResponse - Update all usage sites to use new variant names Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
lcian
commented
Jan 12, 2026
| @@ -1,42 +1,90 @@ | |||
| //! | |||
| //! This is mostly adapted from <https://github.com/tokio-rs/axum/blob/main/examples/anyhow-error-response/src/main.rs> | |||
| //! Common types and utilities for API endpoints. | |||
Member
Author
There was a problem hiding this comment.
This is now mostly adapted from Relay's ApiErrorResponse: https://github.com/getsentry/relay/blob/88f4c53e98e407025168f01276e09b2681373c98/relay-server/src/utils/api.rs#L24
lcian
commented
Jan 12, 2026
| ) -> ApiResult<Response> { | ||
| let mut metadata = | ||
| Metadata::from_headers(&headers, "").context("extracting metadata from headers")?; | ||
| let mut metadata = Metadata::from_headers(&headers, "").map_err(ServiceError::from)?; |
Member
Author
There was a problem hiding this comment.
Here it's kinda weird that we're creating a ServiceError from the server.
However ServiceError already conveniently has a variant that can be obtained directly from a objectstore_types::Error as we need to deal with these metadata conversions there too, so I thought we might as well use that one, instead of creating a brand new ApiError.
Properly distinguishes between authentication and authorization failures: - 401 UNAUTHORIZED: Authentication failed (ValidationFailure, VerificationFailure) - 403 FORBIDDEN: Authorization failed (NotPermitted - valid token but insufficient permissions) - 400 BAD_REQUEST: Malformed auth request (BadRequest) - 500 INTERNAL_SERVER_ERROR: Auth system errors (InternalError) Fixes Python test that expects 403 for insufficient permissions. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
lcian
commented
Jan 12, 2026
| } | ||
|
|
||
| ApiError::Server(_) => { | ||
| tracing::error!(error = &self as &dyn Error, "error handling request"); |
Member
Author
There was a problem hiding this comment.
This whole refactor will end up creating new issue groups in Sentry
matt-codecov
approved these changes
Jan 12, 2026
Comment on lines
+25
to
+32
| /// Server errors, indicating that something went wrong when receiving or executing a request. | ||
| #[error("server error: {0}")] | ||
| Server(#[source] Box<dyn Error + Send + Sync>), | ||
| } | ||
|
|
||
| impl From<ServiceError> for ApiError { | ||
| fn from(err: ServiceError) -> Self { | ||
| ApiError::Server(Box::new(err)) |
Contributor
There was a problem hiding this comment.
why not #[from] ServiceError?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Builds on top of #262 to achieve structured errors all the way to the API layer.
matchit and return the correct status codesAuthError::InitFailurevariant, as this can only be raised when the service starts up. For these kind of errors that would prevent the service from initializing, and are not returned to end users, it's fine to use Anyhows IMO.Some parts of the API still return either a status code or a pair of status code and description, and then convert it with
into_response. I wonder if we should change all of those occurrences to always returnApiErroras well, to achieve consistency in our response bodies.