Allow handlers to return user-defined error types#1180
Conversation
Enforce that `HttpResponseError`s status codes are 4xx or 5xx using an `ErrorStatusCode` type which may only be those statuses. See: #39 (comment) While we're making breaking API changes, let's also have `HttpError::for_status` take a validated client-error-only type, rather than panicking surprisingly. This way, it's obvious to the user that the argument to this has to be a 4xx. Fixes #693
|
it occurs to me that |
| @@ -943,9 +946,9 @@ async fn http_request_handle<C: ServerContext>( | |||
| ), | |||
There was a problem hiding this comment.
should we be firing a USDT probe here?
There was a problem hiding this comment.
We weren't previously, but yeah, we probably should. I can add it in this PR if that makes sense?
There was a problem hiding this comment.
Probably doesn't... (unless you've done it already). We can file an issue
Co-authored-by: Adam Leventhal <ahl@oxide.computer>
| + | ||
| 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`]. |
There was a problem hiding this comment.
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.
|
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 |
davepacheco
left a comment
There was a problem hiding this comment.
Thanks for the fixups and comments. The CHANGELOG notes look terrific! (I haven't re-reviewed all the diffs here.)
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
Currently, all endpoint handler functions must return a
Result<T, HttpError>, whereTimplementsHttpResponse. This isunfortunate, 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>, whereEis any type thatimplements a new
HttpResponseErrortrait.The
HttpResponseErrortrait defines how to produce an error responsefor an error value. This is implemented by
dropshot'sHttpErrortype, but it may also be implemented by user errors. Types implementing
this trait must implement
HttpResponseContent, to determine how togenerate the response body and define its schema, and they must also
implement a method
HttpResponseError::status_codeto provide thestatus code to use for the error response. This is somewhat different
from the existing
HttpCodedResponsetrait, which allows successfulresponses 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
ErrorStatusCodeandClientErrorStatusCodetypes, which are newtypesaround
http::StatusCodethat only contain a 4xx or 5xx status (in thecase of
ErrorStatusCode), or only contain a 4xx (in the case ofClientErrorStatusCode). These types may be fallibly converted from anhttp::StatusCodeat runtime, but we also provide constants forwell-known 4xx and 5xx statuses, which can be used infallibly. The
HttpResponseError::status_codemethod returns anErrorStatusCoderather than a
http::StatusCode, allowing us to ensure that error typesalways have error statuses and generate a correct OpenAPI document.
Additionally, while adding
ErrorStatusCodes, I've gone ahead andchanged the
dropshot::HttpErrortype to also use it, and changed theHttpError::for_client_errorandHttpError::for_statusconstructorsto 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
HttpErrorresponses, 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).