Skip to content

Add Serialize/Deserialize for ErrorStatusCode #1346

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

hawkw
Copy link
Member

@hawkw hawkw commented 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. 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.

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
@hawkw hawkw requested a review from ahl June 11, 2025 17:41
Copy link
Collaborator

@ahl ahl left a comment

Choose a reason for hiding this comment

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

Looks good. What do you see as the tradeoff with the custom json schemas? What you've done is definitely more precise, but I'm not sure if that's also going to make it harder for e.g. generators to work with. I could see it either way, but just wanted to offer that schema precision isn't always the most convenient (i.e. if you had not considered "just" making it an integer). Thanks for doing this!

@hawkw
Copy link
Member Author

hawkw commented Jun 18, 2025

What do you see as the tradeoff with the custom json schemas? What you've done is definitely more precise, but I'm not sure if that's also going to make it harder for e.g. generators to work with.

Ah, hmm, that's a good point — I presume that generators might see this schema and produce some newtype with additional validation (depending on the language)? Since this is almost always going to be a response type rather than a request parameter, I dunno if advertising the range of accepted values to client code generators is actually all that useful, since it doesn't get us automated validation that something would be acceptable to the server. I think I'm sold on the idea it's more convenient for it to just be an integer.

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.

2 participants