Skip to content
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

Allow handlers to return user-defined error types #1180

Open
wants to merge 40 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
40 commits
Select commit Hold shift + click to select a range
af226ea
[WIP] custom error responses using `HttpResponse`
hawkw Nov 13, 2024
4d7c3e4
use a new trait, but HttpResponseContent
hawkw Nov 13, 2024
5bd7e3d
hmmm maybe this is good actually
hawkw Nov 14, 2024
2eff88a
wip schema generation
hawkw Nov 18, 2024
f2c7f5f
use schemars existing deduplication
hawkw Nov 18, 2024
8da1c05
use a refined type for error status
hawkw Nov 20, 2024
86b3afb
just have `HttpError` be a normal `HttpResponseError`
hawkw Nov 20, 2024
1f611bf
just rely on `schemars` to disambiguate colliding names
hawkw Nov 20, 2024
90e7247
start documenting stuff
hawkw Nov 20, 2024
06a1af3
TRYBUILD=overwrite
hawkw Nov 20, 2024
6de6de3
docs etc
hawkw Nov 20, 2024
cc4a2b9
remove unneeded `JsonSchema` impl for `HttpError`
hawkw Nov 20, 2024
c513c46
theory of operation comment in error module
hawkw Nov 20, 2024
cfc582b
actually, we can completely insulate the user from `HandlerError`
hawkw Nov 20, 2024
3d0575c
EXPECTORATE=overwrite
hawkw Nov 20, 2024
6b4b6d4
fix wsrong doctest
hawkw Nov 20, 2024
5f374b8
Merge branch 'main' into eliza/custom-error-httpresponse-result
hawkw Nov 21, 2024
53ed323
rustfmt (NEVER use the github web merge editor)
hawkw Nov 21, 2024
ab798a9
update to track merged changes
hawkw Nov 21, 2024
e87ad82
EXPECTORATE=overwrite
hawkw Nov 21, 2024
6c9c824
Apply docs suggestions from @ahl
hawkw Nov 21, 2024
10a4a99
remove local envrc
hawkw Nov 21, 2024
b9f194c
update copyright dates
hawkw Nov 21, 2024
576ba5f
reticulating comments
hawkw Nov 21, 2024
8a4d52f
reticulating comments
hawkw Nov 21, 2024
f9642d1
nicer error for missing `HttpResponse` impls
hawkw Nov 21, 2024
8f6d70e
fix trait-based stub API not knowing about error schemas
hawkw Nov 21, 2024
ccbbbe2
EXPECTORATE=overwrite
hawkw Nov 21, 2024
46b4df1
whoops i forgot to add changes to endpoint tests
hawkw Nov 22, 2024
00bcea7
convert `HttpError`s into endpoint's error type
hawkw Nov 22, 2024
a6c3472
add a note about `HttpError`
hawkw Nov 22, 2024
4c93e2e
reticulating implementation comments
hawkw Nov 23, 2024
a0e71bf
update docs, improve examples
hawkw Nov 25, 2024
9a15443
fix missing request ID header with custom errors
hawkw Nov 25, 2024
9c8d898
add tests and test utils for custom errors
hawkw Nov 25, 2024
2b7cdea
remove unrelated change
hawkw Nov 25, 2024
a3ee555
Update dropshot/src/handler.rs
hawkw Nov 25, 2024
9d99131
just panic
hawkw Nov 25, 2024
5239d17
Update error.rs
hawkw Nov 28, 2024
a3497ea
Update error.rs
hawkw Nov 28, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 29 additions & 7 deletions dropshot/examples/custom-error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@ use dropshot::endpoint;
use dropshot::ApiDescription;
use dropshot::ConfigLogging;
use dropshot::ConfigLoggingLevel;
use dropshot::ErrorStatusCode;
use dropshot::HttpError;
use dropshot::HttpResponseError;
use dropshot::HttpResponseOk;
use dropshot::Path;
Expand All @@ -16,12 +18,22 @@ use schemars::JsonSchema;
use serde::Deserialize;
use serde::Serialize;

#[derive(Debug, thiserror::Error, Serialize, Deserialize, JsonSchema)]
#[derive(Debug, thiserror::Error, Serialize, JsonSchema)]
enum ThingyError {
#[allow(dead_code)]
#[error("no thingies are currently available")]
NoThingies,
#[error("invalid thingy: {:?}", .name)]
InvalidThingy { name: String },
#[error("{message}")]
Other {
message: String,
#[serde(skip)]
internal_message: String,
#[serde(skip)]
status: ErrorStatusCode,
error_code: Option<String>,
},
}

/// Any type implementing `dropshot::HttpResponseError` and
Expand Down Expand Up @@ -50,6 +62,18 @@ impl HttpResponseError for ThingyError {
dropshot::ErrorStatusCode::from_u16(442)
.expect("442 is a 4xx status code")
}
ThingyError::Other { status, .. } => *status,
}
}
}

impl From<HttpError> for ThingyError {
fn from(error: HttpError) -> Self {
ThingyError::Other {
message: error.external_message,
internal_message: error.internal_message,
status: error.status_code,
error_code: error.error_code,
}
}
}
Expand All @@ -60,9 +84,6 @@ struct Thingy {
magic_number: u64,
}

#[derive(Deserialize, Serialize, JsonSchema)]
struct NoThingy {}

#[derive(Deserialize, JsonSchema)]
struct ThingyPathParams {
name: String,
Expand All @@ -81,13 +102,14 @@ async fn get_thingy(
Err(ThingyError::InvalidThingy { name })
}

/// An example of an endpoint which does not return a `Result<_, _>`.
#[endpoint {
method = GET,
path = "/nothing",
}]
async fn get_nothing(_rqctx: RequestContext<()>) -> HttpResponseOk<NoThingy> {
HttpResponseOk(NoThingy {})
async fn get_nothing(
_rqctx: RequestContext<()>,
) -> Result<HttpResponseOk<Thingy>, ThingyError> {
Err(ThingyError::NoThingies)
}

/// An example of an endpoint which returns a `Result<_, HttpError>`.
Expand Down
40 changes: 32 additions & 8 deletions dropshot/src/api_description.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@
use crate::extractor::RequestExtractor;
use crate::handler::HttpHandlerFunc;
use crate::handler::HttpResponse;
use crate::handler::HttpResponseContent;
use crate::handler::HttpResponseError;
use crate::handler::HttpRouteHandler;
use crate::handler::RouteHandler;
use crate::handler::StubRouteHandler;
Expand Down Expand Up @@ -51,6 +53,7 @@ pub struct ApiEndpoint<Context: ServerContext> {
pub parameters: Vec<ApiEndpointParameter>,
pub body_content_type: ApiEndpointBodyContentType,
pub response: ApiEndpointResponse,
pub error: ApiEndpointErrorResponse,
pub summary: Option<String>,
pub description: Option<String>,
pub tags: Vec<String>,
Expand Down Expand Up @@ -79,6 +82,9 @@ impl<'a, Context: ServerContext> ApiEndpoint<Context> {
.expect("unsupported mime type");
let func_parameters = FuncParams::metadata(body_content_type.clone());
let response = ResponseType::response_metadata();
let error = ApiEndpointErrorResponse {
schema: <HandlerType::Error>::content_metadata(),
};
ApiEndpoint {
operation_id,
handler: HttpRouteHandler::new(handler),
Expand All @@ -87,6 +93,7 @@ impl<'a, Context: ServerContext> ApiEndpoint<Context> {
parameters: func_parameters.parameters,
body_content_type,
response,
error,
summary: None,
description: None,
tags: vec![],
Expand Down Expand Up @@ -162,7 +169,7 @@ impl<'a> ApiEndpoint<StubContext> {
/// );
/// api.register(endpoint).unwrap();
/// ```
pub fn new_for_types<FuncParams, ReturnType>(
pub fn new_for_types<FuncParams, ResultType>(
operation_id: String,
method: Method,
content_type: &'a str,
Expand All @@ -171,13 +178,16 @@ impl<'a> ApiEndpoint<StubContext> {
) -> Self
where
FuncParams: RequestExtractor + 'static,
ReturnType: HttpResponse + Send + Sync + 'static,
ResultType: HttpResultType,
{
let body_content_type =
ApiEndpointBodyContentType::from_mime_type(content_type)
.expect("unsupported mime type");
let func_parameters = FuncParams::metadata(body_content_type.clone());
let response = ReturnType::response_metadata();
let response = <ResultType::Response>::response_metadata();
let error = ApiEndpointErrorResponse {
schema: <ResultType::Error>::content_metadata(),
};
let handler = StubRouteHandler::new_with_name(&operation_id);
ApiEndpoint {
operation_id,
Expand All @@ -187,6 +197,7 @@ impl<'a> ApiEndpoint<StubContext> {
parameters: func_parameters.parameters,
body_content_type,
response,
error,
summary: None,
description: None,
tags: vec![],
Expand All @@ -198,6 +209,20 @@ impl<'a> ApiEndpoint<StubContext> {
}
}

pub trait HttpResultType {
type Response: HttpResponse + Send + Sync + 'static;
type Error: HttpResponseError + Send + Sync + 'static;
}

impl<T, E> HttpResultType for Result<T, E>
where
T: HttpResponse + Send + Sync + 'static,
E: HttpResponseError + Send + Sync + 'static,
{
type Response = T;
type Error = E;
}

/// ApiEndpointParameter represents the discrete path and query parameters for a
/// given API endpoint. These are typically derived from the members of stucts
/// used as parameters to handler functions.
Expand Down Expand Up @@ -317,14 +342,13 @@ pub struct ApiEndpointResponse {
pub schema: Option<ApiSchemaGenerator>,
pub headers: Vec<ApiEndpointHeader>,
pub success: Option<StatusCode>,
pub error: Option<ApiEndpointErrorResponse>,
pub description: Option<String>,
}

/// Metadata for an API endpoint's error response type.
#[derive(Debug)]
#[derive(Debug, Default)]
pub struct ApiEndpointErrorResponse {
pub(crate) schema: ApiSchemaGenerator,
pub(crate) schema: Option<ApiSchemaGenerator>,
}

/// Wrapper for both dynamically generated and pre-generated schemas.
Expand Down Expand Up @@ -915,8 +939,8 @@ impl<Context: ServerContext> ApiDescription<Context> {

// If the endpoint defines an error type, emit that for
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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?

// the 4xx and 5xx responses.
if let Some(ref error) = endpoint.response.error {
let error_schema = match error.schema {
if let Some(ref schema) = endpoint.error.schema {
let error_schema = match schema {
ApiSchemaGenerator::Gen { ref name, ref schema } => {
j2oas_schema(Some(&name()), &schema(&mut generator))
}
Expand Down
Loading