-
Notifications
You must be signed in to change notification settings - Fork 76
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
hawkw
wants to merge
40
commits into
main
Choose a base branch
from
eliza/custom-error-httpresponse-result
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 30 commits
Commits
Show all changes
40 commits
Select commit
Hold shift + click to select a range
af226ea
[WIP] custom error responses using `HttpResponse`
hawkw 4d7c3e4
use a new trait, but HttpResponseContent
hawkw 5bd7e3d
hmmm maybe this is good actually
hawkw 2eff88a
wip schema generation
hawkw f2c7f5f
use schemars existing deduplication
hawkw 8da1c05
use a refined type for error status
hawkw 86b3afb
just have `HttpError` be a normal `HttpResponseError`
hawkw 1f611bf
just rely on `schemars` to disambiguate colliding names
hawkw 90e7247
start documenting stuff
hawkw 06a1af3
TRYBUILD=overwrite
hawkw 6de6de3
docs etc
hawkw cc4a2b9
remove unneeded `JsonSchema` impl for `HttpError`
hawkw c513c46
theory of operation comment in error module
hawkw cfc582b
actually, we can completely insulate the user from `HandlerError`
hawkw 3d0575c
EXPECTORATE=overwrite
hawkw 6b4b6d4
fix wsrong doctest
hawkw 5f374b8
Merge branch 'main' into eliza/custom-error-httpresponse-result
hawkw 53ed323
rustfmt (NEVER use the github web merge editor)
hawkw ab798a9
update to track merged changes
hawkw e87ad82
EXPECTORATE=overwrite
hawkw 6c9c824
Apply docs suggestions from @ahl
hawkw 10a4a99
remove local envrc
hawkw b9f194c
update copyright dates
hawkw 576ba5f
reticulating comments
hawkw 8a4d52f
reticulating comments
hawkw f9642d1
nicer error for missing `HttpResponse` impls
hawkw 8f6d70e
fix trait-based stub API not knowing about error schemas
hawkw ccbbbe2
EXPECTORATE=overwrite
hawkw 46b4df1
whoops i forgot to add changes to endpoint tests
hawkw 00bcea7
convert `HttpError`s into endpoint's error type
hawkw a6c3472
add a note about `HttpError`
hawkw 4c93e2e
reticulating implementation comments
hawkw a0e71bf
update docs, improve examples
hawkw 9a15443
fix missing request ID header with custom errors
hawkw 9c8d898
add tests and test utils for custom errors
hawkw 2b7cdea
remove unrelated change
hawkw a3ee555
Update dropshot/src/handler.rs
hawkw 9d99131
just panic
hawkw 5239d17
Update error.rs
hawkw a3497ea
Update error.rs
hawkw File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,149 @@ | ||
// Copyright 2024 Oxide Computer Company | ||
|
||
//! An example demonstrating how to return user-defined error types from | ||
//! endpoint handlers. | ||
|
||
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; | ||
use dropshot::RequestContext; | ||
use dropshot::ServerBuilder; | ||
use schemars::JsonSchema; | ||
use serde::Deserialize; | ||
use serde::Serialize; | ||
|
||
#[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 | ||
/// `HttpResponseContent` may be used as an error type for a | ||
/// return value from an endpoint handler. | ||
impl HttpResponseError for ThingyError { | ||
// Note that this method returns a `dropshot::ErrorStatusCode`, rather than | ||
// an `http::StatusCode`. This type is a refinement of `http::StatusCode` | ||
// that can only be constructed from status codes in 4xx (client error) or | ||
// 5xx (server error) ranges. | ||
fn status_code(&self) -> dropshot::ErrorStatusCode { | ||
match self { | ||
ThingyError::NoThingies => { | ||
// The `dropshot::ErrorStatusCode` type provides constants for | ||
// all well-known 4xx and 5xx status codes, such as 503 Service | ||
// Unavailable. | ||
dropshot::ErrorStatusCode::SERVICE_UNAVAILABLE | ||
} | ||
ThingyError::InvalidThingy { .. } => { | ||
// Alternatively, an `ErrorStatusCode` can be constructed from a | ||
// u16, but the `ErrorStatusCode::from_u16` constructor | ||
// validates that the status code is a 4xx or 5xx. | ||
// | ||
// This allows using extended status codes, while still | ||
// validating that they are errors. | ||
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, | ||
} | ||
} | ||
} | ||
|
||
/// Just some kind of thingy returned by the API. This doesn't actually matter. | ||
#[derive(Deserialize, Serialize, JsonSchema)] | ||
struct Thingy { | ||
magic_number: u64, | ||
} | ||
|
||
#[derive(Deserialize, JsonSchema)] | ||
struct ThingyPathParams { | ||
name: String, | ||
} | ||
|
||
/// Fetch the thingy with the provided name. | ||
#[endpoint { | ||
method = GET, | ||
path = "/thingy/{name}", | ||
}] | ||
async fn get_thingy( | ||
_rqctx: RequestContext<()>, | ||
path_params: Path<ThingyPathParams>, | ||
) -> Result<HttpResponseOk<Thingy>, ThingyError> { | ||
let ThingyPathParams { name } = path_params.into_inner(); | ||
Err(ThingyError::InvalidThingy { name }) | ||
} | ||
|
||
#[endpoint { | ||
method = GET, | ||
path = "/nothing", | ||
}] | ||
async fn get_nothing( | ||
_rqctx: RequestContext<()>, | ||
) -> Result<HttpResponseOk<Thingy>, ThingyError> { | ||
Err(ThingyError::NoThingies) | ||
} | ||
|
||
/// An example of an endpoint which returns a `Result<_, HttpError>`. | ||
#[endpoint { | ||
method = GET, | ||
path = "/something", | ||
}] | ||
async fn get_something( | ||
_rqctx: RequestContext<()>, | ||
) -> Result<HttpResponseOk<Thingy>, dropshot::HttpError> { | ||
Ok(HttpResponseOk(Thingy { magic_number: 42 })) | ||
} | ||
|
||
#[tokio::main] | ||
async fn main() -> Result<(), String> { | ||
// See dropshot/examples/basic.rs for more details on most of these pieces. | ||
let config_logging = | ||
ConfigLogging::StderrTerminal { level: ConfigLoggingLevel::Info }; | ||
let log = config_logging | ||
.to_logger("example-custom-error") | ||
.map_err(|error| format!("failed to create logger: {}", error))?; | ||
|
||
let mut api = ApiDescription::new(); | ||
api.register(get_thingy).unwrap(); | ||
api.register(get_nothing).unwrap(); | ||
api.register(get_something).unwrap(); | ||
|
||
api.openapi("Custom Error Example", semver::Version::new(0, 0, 0)) | ||
.write(&mut std::io::stdout()) | ||
.map_err(|e| e.to_string())?; | ||
|
||
let server = ServerBuilder::new(api, (), log) | ||
.start() | ||
.map_err(|error| format!("failed to create server: {}", error))?; | ||
|
||
server.await | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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...There was a problem hiding this comment.
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 likeError2
or whatever, but (AFAICT) we only get that when we actually generate the schema and it gives us back a reference (intocomponents.schemas
). We could then try to parse that reference and get the name back out to then use it to generate acomponents.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?There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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()
; asfar 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?