Allow handlers to return user-defined error types #1180

hawkw · 2024-11-20T22:46:29Z

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).

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

.envrc

hawkw · 2024-11-21T00:34:17Z

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

thanks for doing this

dropshot/examples/custom-error.rs

ahl · 2024-11-21T00:27:09Z

dropshot/src/api_description.rs

@@ -919,27 +925,59 @@ impl<Context: ServerContext> ApiDescription<Context> {
                }
            };

+            // If the endpoint defines an error type, emit that for


Alternatively, we could have added to components.responses as before and then referenced that. I can see the inline approach you've taken as potentially simpler, though it does bloat up the json output...

So, I'd like to put them in components.responses, too. The reason I didn't is that it might be a bit annoying to determine the name for each response schema. schemars internally disambiguates colliding schema names by turning subsequent ones into like Error2 or whatever, but (AFAICT) we only get that when we actually generate the schema and it gives us back a reference (into components.schemas). We could then try to parse that reference and get the name back out to then use it to generate a components.responses entry for that response, which seems possible, I just thought it seemed annoying enough that I didn't really want to bother with it. Do you think it's worth doing?

What if we name the response based on <T as JsonSchema>::schema_name()? Might that work?

Do I think it's worth doing? I think it's worth trying. It might make the code worse, but it might make the output simpler. At a minimum it will make the diffs against current json simpler. These together--I think--at least warrant giving it a shot.

I don't believe that deduplication is applied to JsonSchema::schema_name(); as
far as I can tell, it only happens once a schema has already been generated,
because that's when the generator can check if the name already exists in the
set of schemas that have been generated so far?

dropshot/src/error.rs

dropshot/src/router.rs

ahl · 2024-11-21T17:42:49Z

dropshot/src/server.rs

@@ -943,9 +946,9 @@ async fn http_request_handle<C: ServerContext>(
                        ),


should we be firing a USDT probe here?

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

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

dropshot/tests/fail/bad_endpoint10.stderr

dropshot/tests/fail/bad_endpoint12.stderr

dropshot_endpoint/src/endpoint.rs

Co-authored-by: Adam Leventhal <[email protected]>

dropshot/tests/fail/bad_endpoint10.stderr

dropshot/tests/fail/bad_endpoint12.stderr

dropshot/src/handler.rs

dropshot/tests/fail/bad_trait_endpoint12.stderr

See #1180 (comment) This does mean we can no longer have infallible endpoint handlers, but I never actually wanted that in the first place --- it's just a side benefit.

ahl

I like this direction

dropshot/tests/fail/bad_endpoint10.stderr

ahl

nice work

dropshot/src/handler.rs

dropshot/tests/fail/bad_endpoint10.stderr

ahl · 2024-11-25T21:37:45Z

dropshot/tests/test_openapi_custom_error_types.json

+            "content": {
+              "application/json": {
+                "schema": {
+                  "$ref": "#/components/schemas/EnumError"


Co-authored-by: Adam Leventhal <[email protected]>

davepacheco

Very nice! Thanks again for both taking this on and sticking with it through all the design iteration. I don't have anything major here but a few things to fix up, plus: for all breaking changes I generally ask:

We should update CHANGELOG.adoc. it should have super easy-to-follow instructions there for updating across this change. There should be plenty of examples -- usually we try to say how to know you're affected and exactly what code changes to make, with an example or two.
I think it's worth doing a test-update of Omicron to a dropshot with this change. We're going to have to do it anyway and doing it first sometimes smokes out cases where the breaking change was worse than we thought to move past or stuff like that.

I did not review the bad-endpoint error output, the OpenAPI spec changes, or the dropshot_endpoint changes FWIW.

dropshot/src/error.rs

davepacheco · 2024-11-27T22:25:01Z

dropshot/src/error.rs

@@ -245,7 +266,7 @@ impl HttpError {
    /// Found").
    pub fn for_status(


It's great that the type now reflects this constraint and I think I'd still rename it to something that conveys that this is only for client errors (e.g., for_client_error_with_status or something). That'll be another breaking change we should document but I think it's worth it -- we want to make sure all callers of this aren't actually going to panic.

yeah, good call --- while we're breaking this interface, we may as well make the name clearer, too!

davepacheco · 2024-11-27T22:27:25Z

dropshot/src/error.rs

@@ -316,6 +337,649 @@ impl Error for HttpError {
    }
 }

+/// An HTTP 4xx (client error) or 5xx (server error) status code.


nit, feel free to ignore: I feel like this is all big enough and separable enough to put into a separate file / module.

davepacheco · 2024-11-27T22:28:17Z

dropshot/src/error.rs

+/// [`ErrorStatusCode::INTERNAL_SERVER_ERROR`], and so on, including those in
+/// the IANA HTTP Status Code Registry](
+/// https://www.iana.org/assignments/http-status-codes/http-status-codes.xhtml).
+/// Using these constants avoids the fallible conversion from an [`http::StatusCode`].


nit: a bunch of these comment lines are longer than 80 columns. Some are just long links but several others could be fixed. (IIRC you can have rustfmt fix them if you put unstable = true and format_comments = true or something like that into rustfmt.toml.)

davepacheco · 2024-11-27T22:43:45Z

dropshot/src/test_util.rs

+    /// we can use to invoke this endpoint from the client.  This essentially
+    /// appends the path to a base URL constructed from the server's IP address
+    /// and port.
+    pub fn url(&self, path: &str) -> Uri {


Are all these just copy/paste of the corresponding methods in ClientTestContext?

Yup, except that the error types are changed. Unfortunately that felt like the simplest way to do this without changing the ClientTestContext interface in ways that I felt made it unpleasant for the user; we could do something like turning these into default methods on a trait which both ClientTestContext and TypedErrorClientTestContext would implement, or something, but then the user has to import that trait...

davepacheco · 2024-11-27T22:45:27Z

dropshot/tests/integration-tests/versions.rs

@@ -467,7 +467,7 @@ fn test_versions_openapi_same_names() {
    );

    assert_eq!(func_mods_v1, func_overrides_v1);
-    assert_eq!(func_mods_v1, traits_v1);
+    assert_eq!(func_mods_v1, traits_v1,);


This seems fine but spurious.

davepacheco · 2024-11-27T22:52:18Z

dropshot/tests/integration-tests/custom_errors.rs

@@ -0,0 +1,304 @@
+// Copyright 2024 Oxide Computer Company
+
+//! Test cases for user-defined error types.


What about a smoke test that exercises a custom error type using the trait-based interface?

dropshot/src/handler.rs

davepacheco · 2024-11-27T22:54:46Z

dropshot/src/handler.rs

+/// If a handler function's return value is a [`Result`], the error type must
+/// implement this trait, so that a response can be generated when the handler


I think this is always true now

Co-authored-by: David Pacheco <[email protected]>

hawkw added 16 commits November 13, 2024 11:22

[WIP] custom error responses using HttpResponse

af226ea

use a new trait, but HttpResponseContent

4d7c3e4

hmmm maybe this is good actually

5bd7e3d

wip schema generation

2eff88a

use schemars existing deduplication

f2c7f5f

just have HttpError be a normal HttpResponseError

86b3afb

just rely on schemars to disambiguate colliding names

1f611bf

start documenting stuff

90e7247

TRYBUILD=overwrite

06a1af3

docs etc

6de6de3

remove unneeded JsonSchema impl for HttpError

cc4a2b9

theory of operation comment in error module

c513c46

actually, we can completely insulate the user from HandlerError

cfc582b

EXPECTORATE=overwrite

3d0575c

fix wsrong doctest

6b4b6d4

hawkw marked this pull request as ready for review November 21, 2024 00:04

Merge branch 'main' into eliza/custom-error-httpresponse-result

5f374b8

hawkw requested review from ahl and davepacheco November 21, 2024 00:05

hawkw added 2 commits November 20, 2024 16:11

rustfmt (NEVER use the github web merge editor)

53ed323

update to track merged changes

ab798a9

ahl reviewed Nov 21, 2024

View reviewed changes

.envrc Outdated Show resolved Hide resolved

EXPECTORATE=overwrite

e87ad82

ahl reviewed Nov 21, 2024

View reviewed changes

hawkw and others added 4 commits November 21, 2024 10:47

Apply docs suggestions from @ahl

6c9c824

Co-authored-by: Adam Leventhal <[email protected]>

remove local envrc

10a4a99

update copyright dates

b9f194c

reticulating comments

576ba5f

hawkw added 4 commits November 21, 2024 13:50

reticulating comments

8a4d52f

nicer error for missing HttpResponse impls

f9642d1

fix trait-based stub API not knowing about error schemas

8f6d70e

EXPECTORATE=overwrite

ccbbbe2

hawkw requested a review from ahl November 21, 2024 22:59

whoops i forgot to add changes to endpoint tests

46b4df1

ahl reviewed Nov 22, 2024

View reviewed changes

convert HttpErrors into endpoint's error type

00bcea7

See #1180 (comment) This does mean we can no longer have infallible endpoint handlers, but I never actually wanted that in the first place --- it's just a side benefit.

ahl reviewed Nov 22, 2024

View reviewed changes

dropshot/tests/fail/bad_endpoint10.stderr Show resolved Hide resolved

hawkw added 5 commits November 22, 2024 14:59

add a note about HttpError

a6c3472

reticulating implementation comments

4c93e2e

update docs, improve examples

a0e71bf

fix missing request ID header with custom errors

9a15443

add tests and test utils for custom errors

9c8d898

hawkw requested a review from ahl November 25, 2024 20:32

remove unrelated change

2b7cdea

ahl approved these changes Nov 25, 2024

View reviewed changes

hawkw and others added 2 commits November 25, 2024 14:20

Update dropshot/src/handler.rs

a3ee555

Co-authored-by: Adam Leventhal <[email protected]>

just panic

9d99131

davepacheco reviewed Nov 27, 2024

View reviewed changes

hawkw and others added 2 commits November 27, 2024 16:17

Update error.rs

5239d17

Co-authored-by: David Pacheco <[email protected]>

Update error.rs

a3497ea

Co-authored-by: David Pacheco <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow handlers to return user-defined error types #1180

Allow handlers to return user-defined error types #1180

hawkw commented Nov 20, 2024 •

edited

Loading

hawkw commented Nov 21, 2024

ahl left a comment

ahl Nov 21, 2024

hawkw Nov 21, 2024

ahl Nov 22, 2024

hawkw Nov 22, 2024

ahl Nov 21, 2024

hawkw Nov 21, 2024

ahl Nov 22, 2024

ahl left a comment

ahl left a comment

ahl Nov 25, 2024

davepacheco left a comment

davepacheco Nov 27, 2024

hawkw Nov 27, 2024

davepacheco Nov 27, 2024

davepacheco Nov 27, 2024

davepacheco Nov 27, 2024

hawkw Nov 27, 2024

davepacheco Nov 27, 2024

davepacheco Nov 27, 2024

davepacheco Nov 27, 2024

		@@ -943,9 +946,9 @@ async fn http_request_handle<C: ServerContext>(
		),

		@@ -245,7 +266,7 @@ impl HttpError {
		/// Found").
		pub fn for_status(

		@@ -0,0 +1,304 @@
		// Copyright 2024 Oxide Computer Company

		//! Test cases for user-defined error types.

		/// If a handler function's return value is a [`Result`], the error type must
		/// implement this trait, so that a response can be generated when the handler

Allow handlers to return user-defined error types #1180

Are you sure you want to change the base?

Allow handlers to return user-defined error types #1180

Conversation

hawkw commented Nov 20, 2024 • edited Loading

hawkw commented Nov 21, 2024

ahl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahl left a comment

Choose a reason for hiding this comment

ahl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davepacheco left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hawkw commented Nov 20, 2024 •

edited

Loading