Skip to content

Allow handlers to return user-defined error types#1180

Merged
hawkw merged 61 commits into
mainfrom
eliza/custom-error-httpresponse-result
Dec 4, 2024
Merged

Allow handlers to return user-defined error types#1180
hawkw merged 61 commits into
mainfrom
eliza/custom-error-httpresponse-result

Conversation

@hawkw

@hawkw hawkw commented Nov 20, 2024

Copy link
Copy Markdown
Member

Currently, all endpoint handler functions must return a
Result<T, HttpError>, where T implements HttpResponse. This is
unfortunate, as it limits how error return values are represented in
the API. There isn't presently a mechanism for an endpoint to return
a structured error value of its own which is part of the OpenAPI spec
for that endpoint. This is discussed at length in issue #39.

This branch relaxes this requirement, and instead allows endpoint
handler functions to return Result<T, E>, where E is any type that
implements a new HttpResponseError trait.

The HttpResponseError trait defines how to produce an error response
for an error value. This is implemented by dropshot's HttpError
type, but it may also be implemented by user errors. Types implementing
this trait must implement HttpResponseContent, to determine how to
generate the response body and define its schema, and they must also
implement a method HttpResponseError::status_code to provide the
status code to use for the error response. This is somewhat different
from the existing HttpCodedResponse trait, which allows successful
responses to indicate at compile time that they will always have a
particular status code, such as 201 Created. Errors are a bit different:
we would like to be able to return any number of different error status
codes, but would still like to ensure that they represent errors, in
order to correctly generate an OpenAPI document where the error schemas
are returned only for error responses (see this comment for
details). As discussed here, we ensure this by providing new
ErrorStatusCode and ClientErrorStatusCode types, which are newtypes
around http::StatusCode that only contain a 4xx or 5xx status (in the
case of ErrorStatusCode), or only contain a 4xx (in the case of
ClientErrorStatusCode). These types may be fallibly converted from an
http::StatusCode at runtime, but we also provide constants for
well-known 4xx and 5xx statuses, which can be used infallibly. The
HttpResponseError::status_code method returns an ErrorStatusCode
rather than a http::StatusCode, allowing us to ensure that error types
always have error statuses and generate a correct OpenAPI document.

Additionally, while adding ErrorStatusCodes, I've gone ahead and
changed the dropshot::HttpError type to also use it, and changed the
HttpError::for_client_error and HttpError::for_status constructors
to take a ClientErrorStatusCode. Although this is a breaking change,
it resolves a long-standing issue with these APIs: currently, they
assert that the provided status code is a 4xx internally, which is often
surprising to the user. Thus, this PR fixes #693.

Fixes #39
Fixes #693
Fixes #801

This branch is a second attempt at the change originally proposed in PR
#1164, so this closes #1164. This design is substantially simpler, and
only addresses the ability for handler functions to return
user-defined types. Other changes made in #1164, such as a way to
specify a global handler for dropshot-generated errors, and adding
headers to HttpError responses, can be addressed separately. For now,
all extractors and internal errors still produce dropshot::HttpErrors.
A subsequent change will implement a mechanism for providing alternate
presentation for such errors (such as an HTML 404 page).

@hawkw hawkw marked this pull request as ready for review November 21, 2024 00:04
@hawkw hawkw requested review from ahl and davepacheco November 21, 2024 00:05
Comment thread .envrc Outdated
@hawkw

hawkw commented Nov 21, 2024

Copy link
Copy Markdown
Member Author

it occurs to me that HttpResponseHeaders might want to forward an implementation of HttpResponseError when the wrapped type implements that. I'll add one.

@ahl ahl left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

thanks for doing this

Comment thread dropshot/examples/custom-error.rs Outdated
Comment thread dropshot/src/api_description.rs Outdated
Comment thread dropshot/src/error.rs Outdated
Comment thread dropshot/src/error.rs Outdated
Comment thread dropshot/src/error.rs Outdated
Comment thread dropshot/src/router.rs Outdated
Comment thread dropshot/src/server.rs
@@ -943,9 +946,9 @@ async fn http_request_handle<C: ServerContext>(
),

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

should we be firing a USDT probe here?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

We weren't previously, but yeah, we probably should. I can add it in this PR if that makes sense?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Probably doesn't... (unless you've done it already). We can file an issue

Comment thread dropshot/tests/fail/bad_endpoint10.stderr Outdated
Comment thread dropshot/tests/fail/bad_endpoint12.stderr Outdated
Comment thread dropshot_endpoint/src/endpoint.rs
hawkw added a commit to oxidecomputer/omicron that referenced this pull request Dec 3, 2024
Comment thread CHANGELOG.adoc
+
For details on how to implement `HttpResponseError` for user-defined types, see
the trait documentation, or
https://github.com/oxidecomputer/dropshot/blob/main/dropshot/examples/custom-error.rs[`examples/custom-error.rs`].

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

this link will be broken until this PR has merged --- it still felt like the nicest way to reference the example in the changelog, IMO.

@hawkw

hawkw commented Dec 4, 2024

Copy link
Copy Markdown
Member Author

Okay, @davepacheco, I've updated Omicron to use this branch in oxidecomputer/omicron#7196, and uncovered an issue with the generated OpenAPI docs, which I've resolved (see #1180). I've added changelog entries describing the breaking changes and the migration instructions; let me know what you think of https://github.com/oxidecomputer/dropshot/blob/75cbc09542cf439412c4c13ee2b799f1dff1f28a/CHANGELOG.adoc#unreleased-changes-release-date-tbd

@hawkw hawkw requested a review from davepacheco December 4, 2024 17:54

@davepacheco davepacheco left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for the fixups and comments. The CHANGELOG notes look terrific! (I haven't re-reviewed all the diffs here.)

@hawkw hawkw merged commit 14db823 into main Dec 4, 2024
@hawkw hawkw deleted the eliza/custom-error-httpresponse-result branch December 4, 2024 18:16
hawkw added a commit that referenced this pull request Jun 11, 2025
PR #1180 added `ErrorStatusCode` and `ClientErrorStatusCode` types that
represent HTTP status codes that are validated to be in the 400-599 and
400-499 ranges, respectively. These are itnended to be returned by
user-defined error types in their `HttpResponseError` implementations.

User-defined errors are typically implemented as a type that implements
`Serialize` and `JsonSchema` along with `HttpResponseError`, which in
turn requires that the type have a `From<dropshot::HttpError>`
conversion, used in the case of extractor and dropshot-internal errors.
In order for the user-defined error's `HttpResponseError` implementation
to have the same status as the `dropshot::HttpError` from which it was
constructed, it must contain that status code within the error type so
that it may return it. However, `ErrorStatusCode` does not implement
`Serialize`, `Deserialize`, or `JsonSchema`, so holding it in the
user-defined error type breaks deriving those traits.

One solution is to use `#[serde(skip)]` for the `ErrorStatusCode` field,
which I showed in the [`custom-error.rs` example][1]. When the
user-defined error type is only used in the server, this is sufficient,
and it will simply omit the status when serializing. However, this
prevents the user-defined error type from implementing `Deserialize`,
since the status code is never serialized. This means that generated
client code cannot use the same type for the error response value as the
server code, which is desirable in some cases (e.g. to use the same
`fmt::Display` implementation on both sides, etc). Also, in some cases,
it may be useful to include a status code in the serialized body, such
as when embedding an HTTP error returned by an external service which
may not actually be the same as the response's status code (e.g. I might
return a 500 or a 503 when a request to an upstream service returns a
4xx error).

Therefore, this commit adds `Serialize`, `Deserialize`, and `JsonSchema`
implementations for the `ErrorStatusCode` and `ClientErrorStatusCode`
types. These implementations serialize and deserialize these types as a
`u16`, and we generate a JSON Schema that represents them as integers
with appropriate minimum and maximum value validation.

[1]:
    https://github.com/oxidecomputer/dropshot/blob/c028c6751771d89457c44286e26b465ed14ef25f/dropshot/examples/custom-error.rs#L63-L69
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.

Add ability to set error response headers HttpError::for_status asserts when passed a 500-level error could use customisation of error presentation

3 participants