From 46084d724a15dc263c416bd6d3319bbefbbf3c51 Mon Sep 17 00:00:00 2001 From: "Adam H. Leventhal" Date: Wed, 23 Mar 2022 11:52:39 -0700 Subject: [PATCH 1/6] WIP --- dropshot/src/handler.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/dropshot/src/handler.rs b/dropshot/src/handler.rs index abb010e8e..ce0cd0a4c 100644 --- a/dropshot/src/handler.rs +++ b/dropshot/src/handler.rs @@ -1172,6 +1172,10 @@ fn make_subschema_for( gen.subschema_for::() } +trait ContentResponse { + const CONTENT_TYPE: &'static str; +} + /** * `HttpResponseCreated` wraps an object of any serializable type. * It denotes an HTTP 201 "Created" response whose body is generated by From 4424249dedc918a345103e7cebadbb4e89184446 Mon Sep 17 00:00:00 2001 From: "Adam H. Leventhal" Date: Thu, 24 Mar 2022 15:14:31 -0700 Subject: [PATCH 2/6] more WIPPY --- dropshot/src/handler.rs | 168 ++++++++++++++++++++++----------- dropshot/src/lib.rs | 1 + dropshot/tests/test_openapi.rs | 14 ++- 3 files changed, 129 insertions(+), 54 deletions(-) diff --git a/dropshot/src/handler.rs b/dropshot/src/handler.rs index ce0cd0a4c..31752f0c1 100644 --- a/dropshot/src/handler.rs +++ b/dropshot/src/handler.rs @@ -49,6 +49,7 @@ use crate::pagination::PaginationParams; use crate::pagination::PAGINATION_PARAM_SENTINEL; use crate::router::VariableSet; use crate::to_map::to_map; +use crate::CONTENT_TYPE_OCTET_STREAM; use async_trait::async_trait; use bytes::Bytes; @@ -1107,6 +1108,16 @@ impl HttpResponse for Response { } } +pub struct BodyWrapper(pub Body); + +/** + * An "empty" type used to represent responses that have no associated data + * payload. This isn't intended for general use, but must be pub since it's + * used as the Body type for certain responses. + */ +#[doc(hidden)] +pub struct Empty; + /* * Specific Response Types * @@ -1115,6 +1126,70 @@ impl HttpResponse for Response { * kind of HTTP response body they produce. */ +pub trait HttpForContent { + fn to_response(self, builder: http::response::Builder) + -> HttpHandlerResult; + + // TODO some method that produces something that can indirectly produce an + // Option<(String, MediaType)> + + fn content_metadata() -> Option; +} + +impl HttpForContent for BodyWrapper { + fn to_response( + self, + builder: http::response::Builder, + ) -> HttpHandlerResult { + Ok(builder + .header(http::header::CONTENT_TYPE, CONTENT_TYPE_JSON) + .body(self.0)?) + } + + fn content_metadata() -> Option { + None + } +} + +impl HttpForContent for Empty { + fn to_response( + self, + builder: http::response::Builder, + ) -> HttpHandlerResult { + Ok(builder.body(Body::empty())?) + } + + fn content_metadata() -> Option { + Some(ApiSchemaGenerator::Gen { + name: Self::schema_name, + schema: make_subschema_for::, + }) + } +} + +impl HttpForContent for T +where + T: JsonSchema + Serialize + Send + Sync + 'static, +{ + fn to_response( + self, + builder: http::response::Builder, + ) -> HttpHandlerResult { + let serialized = serde_json::to_string(&self) + .map_err(|e| HttpError::for_internal_error(e.to_string()))?; + Ok(builder + .header(http::header::CONTENT_TYPE, CONTENT_TYPE_JSON) + .body(serialized.into())?) + } + + fn content_metadata() -> Option { + Some(ApiSchemaGenerator::Gen { + name: Self::schema_name, + schema: make_subschema_for::, + }) + } +} + /** * The `HttpTypedResponse` trait is used for all of the specific response types * that we provide. We use it in particular to encode the success status code @@ -1123,7 +1198,7 @@ impl HttpResponse for Response { pub trait HttpTypedResponse: Into + Send + Sync + 'static { - type Body: JsonSchema + Serialize; + type Body: HttpForContent; const STATUS_CODE: StatusCode; const DESCRIPTION: &'static str; @@ -1133,13 +1208,8 @@ pub trait HttpTypedResponse: * and the STATUS_CODE specified by the implementing type. This is a default * trait method to allow callers to avoid redundant type specification. */ - fn for_object(body_object: &Self::Body) -> HttpHandlerResult { - let serialized = serde_json::to_string(&body_object) - .map_err(|e| HttpError::for_internal_error(e.to_string()))?; - Ok(Response::builder() - .status(Self::STATUS_CODE) - .header(http::header::CONTENT_TYPE, CONTENT_TYPE_JSON) - .body(serialized.into())?) + fn for_object(body: Self::Body) -> HttpHandlerResult { + body.to_response(Response::builder().status(Self::STATUS_CODE)) } } @@ -1155,10 +1225,7 @@ where } fn metadata() -> ApiEndpointResponse { ApiEndpointResponse { - schema: Some(ApiSchemaGenerator::Gen { - name: T::Body::schema_name, - schema: make_subschema_for::, - }), + schema: T::Body::content_metadata(), success: Some(T::STATUS_CODE), description: Some(T::DESCRIPTION.to_string()), ..Default::default() @@ -1172,10 +1239,29 @@ fn make_subschema_for( gen.subschema_for::() } -trait ContentResponse { +trait StatusResponse { + const STATUS_CODE: StatusCode; + const DESCRIPTION: &'static str; + type Content: ContentResponse; +} + +pub trait ContentResponse { const CONTENT_TYPE: &'static str; } +impl ContentResponse for T +where + T: JsonSchema, +{ + const CONTENT_TYPE: &'static str = CONTENT_TYPE_JSON; +} + +struct MyBody(Body); + +impl ContentResponse for MyBody { + const CONTENT_TYPE: &'static str = CONTENT_TYPE_OCTET_STREAM; +} + /** * `HttpResponseCreated` wraps an object of any serializable type. * It denotes an HTTP 201 "Created" response whose body is generated by @@ -1201,7 +1287,7 @@ impl { fn from(response: HttpResponseCreated) -> HttpHandlerResult { /* TODO-correctness (or polish?): add Location header */ - HttpResponseCreated::for_object(&response.0) + HttpResponseCreated::for_object(response.0) } } @@ -1224,7 +1310,7 @@ impl From> for HttpHandlerResult { fn from(response: HttpResponseAccepted) -> HttpHandlerResult { - HttpResponseAccepted::for_object(&response.0) + HttpResponseAccepted::for_object(response.0) } } @@ -1233,32 +1319,22 @@ impl * denotes an HTTP 200 "OK" response whose body is generated by serializing the * object. */ -pub struct HttpResponseOk( - pub T, -); -impl HttpTypedResponse +pub struct HttpResponseOk(pub T); +impl HttpTypedResponse for HttpResponseOk { type Body = T; const STATUS_CODE: StatusCode = StatusCode::OK; const DESCRIPTION: &'static str = "successful operation"; } -impl From> +impl From> for HttpHandlerResult { fn from(response: HttpResponseOk) -> HttpHandlerResult { - HttpResponseOk::for_object(&response.0) + HttpResponseOk::for_object(response.0) } } -/** - * An "empty" type used to represent responses that have no associated data - * payload. This isn't intended for general use, but must be pub since it's - * used as the Body type for certain responses. - */ -#[doc(hidden)] -pub struct Empty; - impl JsonSchema for Empty { fn schema_name() -> String { "Empty".to_string() @@ -1275,14 +1351,14 @@ impl JsonSchema for Empty { } } -impl Serialize for Empty { - fn serialize(&self, _: S) -> Result - where - S: serde::Serializer, - { - panic!("Empty::serialize() should never be called"); - } -} +// impl Serialize for Empty { +// fn serialize(&self, _: S) -> Result +// where +// S: serde::Serializer, +// { +// panic!("Empty::serialize() should never be called"); +// } +// } /** * `HttpResponseDeleted` represents an HTTP 204 "No Content" response, intended @@ -1297,9 +1373,7 @@ impl HttpTypedResponse for HttpResponseDeleted { } impl From for HttpHandlerResult { fn from(_: HttpResponseDeleted) -> HttpHandlerResult { - Ok(Response::builder() - .status(HttpResponseDeleted::STATUS_CODE) - .body(Body::empty())?) + HttpResponseDeleted::for_object(Empty) } } @@ -1317,9 +1391,7 @@ impl HttpTypedResponse for HttpResponseUpdatedNoContent { } impl From for HttpHandlerResult { fn from(_: HttpResponseUpdatedNoContent) -> HttpHandlerResult { - Ok(Response::builder() - .status(HttpResponseUpdatedNoContent::STATUS_CODE) - .body(Body::empty())?) + HttpResponseUpdatedNoContent::for_object(Empty) } } @@ -1500,8 +1572,6 @@ fn schema_extract_description( #[cfg(test)] mod test { - use crate::handler::HttpHandlerResult; - use crate::HttpResponseOk; use crate::{ api_description::ApiEndpointParameterMetadata, ApiEndpointParameter, ApiEndpointParameterLocation, PaginationParams, @@ -1510,7 +1580,6 @@ mod test { use serde::{Deserialize, Serialize}; use super::get_metadata; - use super::Empty; use super::ExtractorMetadata; #[derive(Deserialize, Serialize, JsonSchema)] @@ -1617,11 +1686,4 @@ mod test { compare(params, true, expected); } - - #[test] - #[should_panic] - fn test_empty_serialized() { - let response = HttpResponseOk::(Empty); - let _ = HttpHandlerResult::from(response); - } } diff --git a/dropshot/src/lib.rs b/dropshot/src/lib.rs index 8b34eb41e..20446557c 100644 --- a/dropshot/src/lib.rs +++ b/dropshot/src/lib.rs @@ -630,6 +630,7 @@ pub use config::ConfigDropshot; pub use config::ConfigTls; pub use error::HttpError; pub use error::HttpErrorResponseBody; +pub use handler::BodyWrapper; pub use handler::Extractor; pub use handler::ExtractorMetadata; pub use handler::HttpResponse; diff --git a/dropshot/tests/test_openapi.rs b/dropshot/tests/test_openapi.rs index ac0e12708..d1a205282 100644 --- a/dropshot/tests/test_openapi.rs +++ b/dropshot/tests/test_openapi.rs @@ -1,7 +1,7 @@ // Copyright 2022 Oxide Computer Company use dropshot::{ - endpoint, ApiDescription, HttpError, HttpResponseAccepted, + endpoint, ApiDescription, BodyWrapper, HttpError, HttpResponseAccepted, HttpResponseCreated, HttpResponseDeleted, HttpResponseHeaders, HttpResponseOk, HttpResponseUpdatedNoContent, PaginationParams, Path, Query, RequestContext, ResultsPage, TagConfig, TagDetails, TypedBody, @@ -337,6 +337,17 @@ async fn handler17( unimplemented!(); } +#[endpoint { + method = GET, + path = "/playing/a/bit/nicer", + tags = ["it"], +}] +async fn handler18( + _rqctx: Arc>, +) -> Result, HttpError> { + unimplemented!(); +} + fn make_api( maybe_tag_config: Option, ) -> Result, String> { @@ -363,6 +374,7 @@ fn make_api( api.register(handler15)?; api.register(handler16)?; api.register(handler17)?; + api.register(handler18)?; Ok(api) } From 30fccd6bfc1ba99282939911cecc6ce016cfbf35 Mon Sep 17 00:00:00 2001 From: "Adam H. Leventhal" Date: Mon, 28 Mar 2022 09:18:15 -0700 Subject: [PATCH 3/6] more --- dropshot/src/api_description.rs | 73 +++++++-------- dropshot/src/handler.rs | 113 +++++++++-------------- dropshot/tests/fail/bad_endpoint7.stderr | 5 +- dropshot/tests/test_openapi.json | 24 +++++ dropshot/tests/test_openapi.rs | 3 +- dropshot/tests/test_openapi_fuller.json | 24 +++++ 6 files changed, 129 insertions(+), 113 deletions(-) diff --git a/dropshot/src/api_description.rs b/dropshot/src/api_description.rs index bafe1442f..90b1056a7 100644 --- a/dropshot/src/api_description.rs +++ b/dropshot/src/api_description.rs @@ -60,7 +60,7 @@ impl<'a, Context: ServerContext> ApiEndpoint { ResponseType: HttpResponse + Send + Sync + 'static, { let func_parameters = FuncParams::metadata(); - let response = ResponseType::metadata(); + let response = ResponseType::response_metadata(); ApiEndpoint { operation_id, handler: HttpRouteHandler::new(handler), @@ -670,7 +670,7 @@ impl ApiDescription { ); } - if let Some(schema) = &endpoint.response.schema { + let response = if let Some(schema) = &endpoint.response.schema { let (name, js) = match schema { ApiSchemaGenerator::Gen { name, schema } => { (Some(name()), schema(&mut generator)) @@ -745,36 +745,7 @@ impl ApiDescription { headers, ..Default::default() }; - - match &endpoint.response.success { - None => { - // Without knowing the specific status code we use the - // 2xx range. - operation.responses.responses.insert( - openapiv3::StatusCode::Range(2), - openapiv3::ReferenceOr::Item(response), - ); - } - Some(code) => { - operation.responses.responses.insert( - openapiv3::StatusCode::Code(code.as_u16()), - openapiv3::ReferenceOr::Item(response), - ); - } - } - - // 4xx and 5xx responses all use the same error information - let err_ref = openapiv3::ReferenceOr::ref_( - "#/components/responses/Error", - ); - operation - .responses - .responses - .insert(openapiv3::StatusCode::Range(4), err_ref.clone()); - operation - .responses - .responses - .insert(openapiv3::StatusCode::Range(5), err_ref); + response } else { // If no schema was specified, the response is hand-rolled. In // this case we'll fall back to the default response type which @@ -795,15 +766,37 @@ impl ApiDescription { ..Default::default() }, ); + openapiv3::Response { + // TODO: perhaps we should require even free-form + // responses to have a description since it's required + // by OpenAPI. + description: "".to_string(), + content, + ..Default::default() + } + }; + + if let Some(code) = &endpoint.response.success { + operation.responses.responses.insert( + openapiv3::StatusCode::Code(code.as_u16()), + openapiv3::ReferenceOr::Item(response), + ); + + // 4xx and 5xx responses all use the same error information + let err_ref = openapiv3::ReferenceOr::ref_( + "#/components/responses/Error", + ); + operation + .responses + .responses + .insert(openapiv3::StatusCode::Range(4), err_ref.clone()); + operation + .responses + .responses + .insert(openapiv3::StatusCode::Range(5), err_ref); + } else { operation.responses.default = - Some(openapiv3::ReferenceOr::Item(openapiv3::Response { - // TODO: perhaps we should require even free-form - // responses to have a description since it's required - // by OpenAPI. - description: "".to_string(), - content, - ..Default::default() - })); + Some(openapiv3::ReferenceOr::Item(response)) } // Drop in the operation. diff --git a/dropshot/src/handler.rs b/dropshot/src/handler.rs index 31752f0c1..cd321464e 100644 --- a/dropshot/src/handler.rs +++ b/dropshot/src/handler.rs @@ -37,6 +37,7 @@ use super::error::HttpError; use super::http_util::http_extract_path_params; use super::http_util::http_read_body; use super::http_util::CONTENT_TYPE_JSON; +use super::http_util::CONTENT_TYPE_OCTET_STREAM; use super::server::DropshotState; use super::server::ServerContext; use crate::api_description::ApiEndpointBodyContentType; @@ -49,7 +50,6 @@ use crate::pagination::PaginationParams; use crate::pagination::PAGINATION_PARAM_SENTINEL; use crate::router::VariableSet; use crate::to_map::to_map; -use crate::CONTENT_TYPE_OCTET_STREAM; use async_trait::async_trait; use bytes::Bytes; @@ -1092,7 +1092,7 @@ pub trait HttpResponse { * Extract status code and structure metadata for the non-error response. * Type information for errors is handled generically across all endpoints. */ - fn metadata() -> ApiEndpointResponse; + fn response_metadata() -> ApiEndpointResponse; } /** @@ -1103,13 +1103,21 @@ impl HttpResponse for Response { fn to_result(self) -> HttpHandlerResult { Ok(self) } - fn metadata() -> ApiEndpointResponse { + fn response_metadata() -> ApiEndpointResponse { ApiEndpointResponse::default() } } +/// Wraps a [hyper::Body] so that it can be used with coded response types such +/// as [HttpResponseOk]. pub struct BodyWrapper(pub Body); +impl From for BodyWrapper { + fn from(body: Body) -> Self { + Self(body) + } +} + /** * An "empty" type used to represent responses that have no associated data * payload. This isn't intended for general use, but must be pub since it's @@ -1118,6 +1126,22 @@ pub struct BodyWrapper(pub Body); #[doc(hidden)] pub struct Empty; +impl JsonSchema for Empty { + fn schema_name() -> String { + "Empty".to_string() + } + + fn json_schema( + _: &mut schemars::gen::SchemaGenerator, + ) -> schemars::schema::Schema { + schemars::schema::Schema::Bool(false) + } + + fn is_referenceable() -> bool { + false + } +} + /* * Specific Response Types * @@ -1130,9 +1154,6 @@ pub trait HttpForContent { fn to_response(self, builder: http::response::Builder) -> HttpHandlerResult; - // TODO some method that produces something that can indirectly produce an - // Option<(String, MediaType)> - fn content_metadata() -> Option; } @@ -1142,7 +1163,7 @@ impl HttpForContent for BodyWrapper { builder: http::response::Builder, ) -> HttpHandlerResult { Ok(builder - .header(http::header::CONTENT_TYPE, CONTENT_TYPE_JSON) + .header(http::header::CONTENT_TYPE, CONTENT_TYPE_OCTET_STREAM) .body(self.0)?) } @@ -1223,7 +1244,7 @@ where fn to_result(self) -> HttpHandlerResult { self.into() } - fn metadata() -> ApiEndpointResponse { + fn response_metadata() -> ApiEndpointResponse { ApiEndpointResponse { schema: T::Body::content_metadata(), success: Some(T::STATUS_CODE), @@ -1239,29 +1260,6 @@ fn make_subschema_for( gen.subschema_for::() } -trait StatusResponse { - const STATUS_CODE: StatusCode; - const DESCRIPTION: &'static str; - type Content: ContentResponse; -} - -pub trait ContentResponse { - const CONTENT_TYPE: &'static str; -} - -impl ContentResponse for T -where - T: JsonSchema, -{ - const CONTENT_TYPE: &'static str = CONTENT_TYPE_JSON; -} - -struct MyBody(Body); - -impl ContentResponse for MyBody { - const CONTENT_TYPE: &'static str = CONTENT_TYPE_OCTET_STREAM; -} - /** * `HttpResponseCreated` wraps an object of any serializable type. * It denotes an HTTP 201 "Created" response whose body is generated by @@ -1272,18 +1270,18 @@ impl ContentResponse for MyBody { * could restrict this to an ApiObject::View (by having T: ApiObject and the * field having type T::View). */ -pub struct HttpResponseCreated< - T: JsonSchema + Serialize + Send + Sync + 'static, ->(pub T); -impl HttpTypedResponse +pub struct HttpResponseCreated( + pub T, +); +impl HttpTypedResponse for HttpResponseCreated { type Body = T; const STATUS_CODE: StatusCode = StatusCode::CREATED; const DESCRIPTION: &'static str = "successful creation"; } -impl - From> for HttpHandlerResult +impl From> + for HttpHandlerResult { fn from(response: HttpResponseCreated) -> HttpHandlerResult { /* TODO-correctness (or polish?): add Location header */ @@ -1296,18 +1294,18 @@ impl * serializable type. It denotes an HTTP 202 "Accepted" response whose body is * generated by serializing the object. */ -pub struct HttpResponseAccepted< - T: JsonSchema + Serialize + Send + Sync + 'static, ->(pub T); -impl HttpTypedResponse +pub struct HttpResponseAccepted( + pub T, +); +impl HttpTypedResponse for HttpResponseAccepted { type Body = T; const STATUS_CODE: StatusCode = StatusCode::ACCEPTED; const DESCRIPTION: &'static str = "successfully enqueued operation"; } -impl - From> for HttpHandlerResult +impl From> + for HttpHandlerResult { fn from(response: HttpResponseAccepted) -> HttpHandlerResult { HttpResponseAccepted::for_object(response.0) @@ -1335,31 +1333,6 @@ impl From> } } -impl JsonSchema for Empty { - fn schema_name() -> String { - "Empty".to_string() - } - - fn json_schema( - _: &mut schemars::gen::SchemaGenerator, - ) -> schemars::schema::Schema { - schemars::schema::Schema::Bool(false) - } - - fn is_referenceable() -> bool { - false - } -} - -// impl Serialize for Empty { -// fn serialize(&self, _: S) -> Result -// where -// S: serde::Serializer, -// { -// panic!("Empty::serialize() should never be called"); -// } -// } - /** * `HttpResponseDeleted` represents an HTTP 204 "No Content" response, intended * for use when an API operation has successfully deleted an object. @@ -1474,8 +1447,8 @@ impl< Ok(result) } - fn metadata() -> ApiEndpointResponse { - let mut metadata = T::metadata(); + fn response_metadata() -> ApiEndpointResponse { + let mut metadata = T::response_metadata(); let mut generator = schemars::gen::SchemaGenerator::new( schemars::gen::SchemaSettings::openapi3(), diff --git a/dropshot/tests/fail/bad_endpoint7.stderr b/dropshot/tests/fail/bad_endpoint7.stderr index 747959217..85c773f3c 100644 --- a/dropshot/tests/fail/bad_endpoint7.stderr +++ b/dropshot/tests/fail/bad_endpoint7.stderr @@ -4,8 +4,9 @@ error[E0277]: the trait bound `Ret: serde::ser::Serialize` is not satisfied 25 | ) -> Result, HttpError> { | ^^^^^^^^^^^^^^^^^^^ the trait `serde::ser::Serialize` is not implemented for `Ret` | + = note: required because of the requirements on the impl of `dropshot::handler::HttpForContent` for `Ret` note: required by a bound in `HttpResponseOk` --> src/handler.rs | - | pub struct HttpResponseOk( - | ^^^^^^^^^ required by this bound in `HttpResponseOk` + | pub struct HttpResponseOk(pub T); + | ^^^^^^^^^^^^^^ required by this bound in `HttpResponseOk` diff --git a/dropshot/tests/test_openapi.json b/dropshot/tests/test_openapi.json index 7b5563d36..f54ee74cb 100644 --- a/dropshot/tests/test_openapi.json +++ b/dropshot/tests/test_openapi.json @@ -261,6 +261,30 @@ "x-dropshot-pagination": true } }, + "/playing/a/bit/nicer": { + "get": { + "tags": [ + "it" + ], + "operationId": "handler18", + "responses": { + "200": { + "description": "", + "content": { + "*/*": { + "schema": {} + } + } + }, + "4XX": { + "$ref": "#/components/responses/Error" + }, + "5XX": { + "$ref": "#/components/responses/Error" + } + } + } + }, "/test/camera": { "post": { "tags": [ diff --git a/dropshot/tests/test_openapi.rs b/dropshot/tests/test_openapi.rs index d1a205282..0116a8dd9 100644 --- a/dropshot/tests/test_openapi.rs +++ b/dropshot/tests/test_openapi.rs @@ -345,7 +345,8 @@ async fn handler17( async fn handler18( _rqctx: Arc>, ) -> Result, HttpError> { - unimplemented!(); + let (_, body) = Body::channel(); + Ok(HttpResponseOk(body.into())) } fn make_api( diff --git a/dropshot/tests/test_openapi_fuller.json b/dropshot/tests/test_openapi_fuller.json index 4e9bd4988..266123f00 100644 --- a/dropshot/tests/test_openapi_fuller.json +++ b/dropshot/tests/test_openapi_fuller.json @@ -269,6 +269,30 @@ "x-dropshot-pagination": true } }, + "/playing/a/bit/nicer": { + "get": { + "tags": [ + "it" + ], + "operationId": "handler18", + "responses": { + "200": { + "description": "", + "content": { + "*/*": { + "schema": {} + } + } + }, + "4XX": { + "$ref": "#/components/responses/Error" + }, + "5XX": { + "$ref": "#/components/responses/Error" + } + } + } + }, "/test/camera": { "post": { "tags": [ From b78c5b2b66c393aa60e45879b931a3f91751dc3a Mon Sep 17 00:00:00 2001 From: "Adam H. Leventhal" Date: Mon, 28 Mar 2022 09:28:25 -0700 Subject: [PATCH 4/6] add comments and commentary --- dropshot/src/handler.rs | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/dropshot/src/handler.rs b/dropshot/src/handler.rs index cd321464e..03b17fb79 100644 --- a/dropshot/src/handler.rs +++ b/dropshot/src/handler.rs @@ -1150,10 +1150,20 @@ impl JsonSchema for Empty { * kind of HTTP response body they produce. */ +// TODO this is a terrible name +/// Adapter trait that allows both concrete types that implement [JsonSchema] +/// and our [BodyWrapper] type to add their content to a response builder +/// object. pub trait HttpForContent { fn to_response(self, builder: http::response::Builder) -> HttpHandlerResult; + // TODO the return type here could be something more elegant that is able + // to produce the map of mime type -> openapiv3::MediaType that's needed in + // in api_description. One could imagine, for example, that this could + // allow dropshot consumers in the future to have endpoints that respond + // with multiple, explicitly enumerated mime types. + // TODO the ApiSchemaGenerator type is particularly inelegant. fn content_metadata() -> Option; } @@ -1219,6 +1229,8 @@ where pub trait HttpTypedResponse: Into + Send + Sync + 'static { + // TODO with this change, the this isn't particularly "Typed" but rather + // we merely know that there's a single associated [StatusCode] type Body: HttpForContent; const STATUS_CODE: StatusCode; const DESCRIPTION: &'static str; From aa74de53fb5f605c0a629a9d7dc0232cf708b84f Mon Sep 17 00:00:00 2001 From: "Adam H. Leventhal" Date: Thu, 7 Apr 2022 11:57:35 -0700 Subject: [PATCH 5/6] ready for review --- dropshot/src/handler.rs | 83 +++++++++-------------- dropshot/src/lib.rs | 4 +- dropshot/tests/fail/bad_endpoint12.stderr | 4 +- dropshot/tests/fail/bad_endpoint7.stderr | 6 +- dropshot/tests/test_openapi.rs | 4 +- 5 files changed, 42 insertions(+), 59 deletions(-) diff --git a/dropshot/src/handler.rs b/dropshot/src/handler.rs index 03b17fb79..783b24518 100644 --- a/dropshot/src/handler.rs +++ b/dropshot/src/handler.rs @@ -1110,9 +1110,9 @@ impl HttpResponse for Response { /// Wraps a [hyper::Body] so that it can be used with coded response types such /// as [HttpResponseOk]. -pub struct BodyWrapper(pub Body); +pub struct FreeformBody(pub Body); -impl From for BodyWrapper { +impl From for FreeformBody { fn from(body: Body) -> Self { Self(body) } @@ -1126,22 +1126,6 @@ impl From for BodyWrapper { #[doc(hidden)] pub struct Empty; -impl JsonSchema for Empty { - fn schema_name() -> String { - "Empty".to_string() - } - - fn json_schema( - _: &mut schemars::gen::SchemaGenerator, - ) -> schemars::schema::Schema { - schemars::schema::Schema::Bool(false) - } - - fn is_referenceable() -> bool { - false - } -} - /* * Specific Response Types * @@ -1150,11 +1134,10 @@ impl JsonSchema for Empty { * kind of HTTP response body they produce. */ -// TODO this is a terrible name /// Adapter trait that allows both concrete types that implement [JsonSchema] -/// and our [BodyWrapper] type to add their content to a response builder +/// and the [FreeformBody] type to add their content to a response builder /// object. -pub trait HttpForContent { +pub trait HttpResponseContent { fn to_response(self, builder: http::response::Builder) -> HttpHandlerResult; @@ -1167,7 +1150,7 @@ pub trait HttpForContent { fn content_metadata() -> Option; } -impl HttpForContent for BodyWrapper { +impl HttpResponseContent for FreeformBody { fn to_response( self, builder: http::response::Builder, @@ -1182,7 +1165,7 @@ impl HttpForContent for BodyWrapper { } } -impl HttpForContent for Empty { +impl HttpResponseContent for Empty { fn to_response( self, builder: http::response::Builder, @@ -1191,14 +1174,14 @@ impl HttpForContent for Empty { } fn content_metadata() -> Option { - Some(ApiSchemaGenerator::Gen { - name: Self::schema_name, - schema: make_subschema_for::, + Some(ApiSchemaGenerator::Static { + schema: Box::new(schemars::schema::Schema::Bool(false)), + dependencies: indexmap::IndexMap::default(), }) } } -impl HttpForContent for T +impl HttpResponseContent for T where T: JsonSchema + Serialize + Send + Sync + 'static, { @@ -1222,16 +1205,14 @@ where } /** - * The `HttpTypedResponse` trait is used for all of the specific response types + * The `HttpCodedResponse` trait is used for all of the specific response types * that we provide. We use it in particular to encode the success status code * and the type information of the return value. */ -pub trait HttpTypedResponse: +pub trait HttpCodedResponse: Into + Send + Sync + 'static { - // TODO with this change, the this isn't particularly "Typed" but rather - // we merely know that there's a single associated [StatusCode] - type Body: HttpForContent; + type Body: HttpResponseContent; const STATUS_CODE: StatusCode; const DESCRIPTION: &'static str; @@ -1251,7 +1232,7 @@ pub trait HttpTypedResponse: */ impl HttpResponse for T where - T: HttpTypedResponse, + T: HttpCodedResponse, { fn to_result(self) -> HttpHandlerResult { self.into() @@ -1282,18 +1263,18 @@ fn make_subschema_for( * could restrict this to an ApiObject::View (by having T: ApiObject and the * field having type T::View). */ -pub struct HttpResponseCreated( +pub struct HttpResponseCreated( pub T, ); -impl HttpTypedResponse +impl HttpCodedResponse for HttpResponseCreated { type Body = T; const STATUS_CODE: StatusCode = StatusCode::CREATED; const DESCRIPTION: &'static str = "successful creation"; } -impl From> - for HttpHandlerResult +impl + From> for HttpHandlerResult { fn from(response: HttpResponseCreated) -> HttpHandlerResult { /* TODO-correctness (or polish?): add Location header */ @@ -1306,18 +1287,18 @@ impl From> * serializable type. It denotes an HTTP 202 "Accepted" response whose body is * generated by serializing the object. */ -pub struct HttpResponseAccepted( +pub struct HttpResponseAccepted( pub T, ); -impl HttpTypedResponse +impl HttpCodedResponse for HttpResponseAccepted { type Body = T; const STATUS_CODE: StatusCode = StatusCode::ACCEPTED; const DESCRIPTION: &'static str = "successfully enqueued operation"; } -impl From> - for HttpHandlerResult +impl + From> for HttpHandlerResult { fn from(response: HttpResponseAccepted) -> HttpHandlerResult { HttpResponseAccepted::for_object(response.0) @@ -1329,15 +1310,17 @@ impl From> * denotes an HTTP 200 "OK" response whose body is generated by serializing the * object. */ -pub struct HttpResponseOk(pub T); -impl HttpTypedResponse +pub struct HttpResponseOk( + pub T, +); +impl HttpCodedResponse for HttpResponseOk { type Body = T; const STATUS_CODE: StatusCode = StatusCode::OK; const DESCRIPTION: &'static str = "successful operation"; } -impl From> +impl From> for HttpHandlerResult { fn from(response: HttpResponseOk) -> HttpHandlerResult { @@ -1351,7 +1334,7 @@ impl From> */ pub struct HttpResponseDeleted(); -impl HttpTypedResponse for HttpResponseDeleted { +impl HttpCodedResponse for HttpResponseDeleted { type Body = Empty; const STATUS_CODE: StatusCode = StatusCode::NO_CONTENT; const DESCRIPTION: &'static str = "successful deletion"; @@ -1369,7 +1352,7 @@ impl From for HttpHandlerResult { */ pub struct HttpResponseUpdatedNoContent(); -impl HttpTypedResponse for HttpResponseUpdatedNoContent { +impl HttpCodedResponse for HttpResponseUpdatedNoContent { type Body = Empty; const STATUS_CODE: StatusCode = StatusCode::NO_CONTENT; const DESCRIPTION: &'static str = "resource updated"; @@ -1394,14 +1377,14 @@ pub struct NoHeaders {} * conflicts. */ pub struct HttpResponseHeaders< - T: HttpTypedResponse, + T: HttpCodedResponse, H: JsonSchema + Serialize + Send + Sync + 'static = NoHeaders, > { body: T, structured_headers: H, other_headers: HeaderMap, } -impl HttpResponseHeaders { +impl HttpResponseHeaders { pub fn new_unnamed(body: T) -> Self { Self { body, @@ -1411,7 +1394,7 @@ impl HttpResponseHeaders { } } impl< - T: HttpTypedResponse, + T: HttpCodedResponse, H: JsonSchema + Serialize + Send + Sync + 'static, > HttpResponseHeaders { @@ -1428,7 +1411,7 @@ impl< } } impl< - T: HttpTypedResponse, + T: HttpCodedResponse, H: JsonSchema + Serialize + Send + Sync + 'static, > HttpResponse for HttpResponseHeaders { diff --git a/dropshot/src/lib.rs b/dropshot/src/lib.rs index 20446557c..22364f2d1 100644 --- a/dropshot/src/lib.rs +++ b/dropshot/src/lib.rs @@ -630,9 +630,10 @@ pub use config::ConfigDropshot; pub use config::ConfigTls; pub use error::HttpError; pub use error::HttpErrorResponseBody; -pub use handler::BodyWrapper; pub use handler::Extractor; pub use handler::ExtractorMetadata; +pub use handler::FreeformBody; +pub use handler::HttpCodedResponse; pub use handler::HttpResponse; pub use handler::HttpResponseAccepted; pub use handler::HttpResponseCreated; @@ -640,7 +641,6 @@ pub use handler::HttpResponseDeleted; pub use handler::HttpResponseHeaders; pub use handler::HttpResponseOk; pub use handler::HttpResponseUpdatedNoContent; -pub use handler::HttpTypedResponse; pub use handler::NoHeaders; pub use handler::Path; pub use handler::Query; diff --git a/dropshot/tests/fail/bad_endpoint12.stderr b/dropshot/tests/fail/bad_endpoint12.stderr index 512d99458..cd66c8efd 100644 --- a/dropshot/tests/fail/bad_endpoint12.stderr +++ b/dropshot/tests/fail/bad_endpoint12.stderr @@ -1,8 +1,8 @@ -error[E0277]: the trait bound `String: HttpTypedResponse` is not satisfied +error[E0277]: the trait bound `String: HttpCodedResponse` is not satisfied --> tests/fail/bad_endpoint12.rs:16:6 | 16 | ) -> Result { - | ^^^^^^ the trait `HttpTypedResponse` is not implemented for `String` + | ^^^^^^ the trait `HttpCodedResponse` is not implemented for `String` | = note: required because of the requirements on the impl of `HttpResponse` for `String` note: required because of the requirements on the impl of `ResultTrait` for `Result` diff --git a/dropshot/tests/fail/bad_endpoint7.stderr b/dropshot/tests/fail/bad_endpoint7.stderr index 85c773f3c..aab15074e 100644 --- a/dropshot/tests/fail/bad_endpoint7.stderr +++ b/dropshot/tests/fail/bad_endpoint7.stderr @@ -4,9 +4,9 @@ error[E0277]: the trait bound `Ret: serde::ser::Serialize` is not satisfied 25 | ) -> Result, HttpError> { | ^^^^^^^^^^^^^^^^^^^ the trait `serde::ser::Serialize` is not implemented for `Ret` | - = note: required because of the requirements on the impl of `dropshot::handler::HttpForContent` for `Ret` + = note: required because of the requirements on the impl of `dropshot::handler::HttpResponseContent` for `Ret` note: required by a bound in `HttpResponseOk` --> src/handler.rs | - | pub struct HttpResponseOk(pub T); - | ^^^^^^^^^^^^^^ required by this bound in `HttpResponseOk` + | pub struct HttpResponseOk( + | ^^^^^^^^^^^^^^^^^^^ required by this bound in `HttpResponseOk` diff --git a/dropshot/tests/test_openapi.rs b/dropshot/tests/test_openapi.rs index 0116a8dd9..0b6908f7f 100644 --- a/dropshot/tests/test_openapi.rs +++ b/dropshot/tests/test_openapi.rs @@ -1,7 +1,7 @@ // Copyright 2022 Oxide Computer Company use dropshot::{ - endpoint, ApiDescription, BodyWrapper, HttpError, HttpResponseAccepted, + endpoint, ApiDescription, FreeformBody, HttpError, HttpResponseAccepted, HttpResponseCreated, HttpResponseDeleted, HttpResponseHeaders, HttpResponseOk, HttpResponseUpdatedNoContent, PaginationParams, Path, Query, RequestContext, ResultsPage, TagConfig, TagDetails, TypedBody, @@ -344,7 +344,7 @@ async fn handler17( }] async fn handler18( _rqctx: Arc>, -) -> Result, HttpError> { +) -> Result, HttpError> { let (_, body) = Body::channel(); Ok(HttpResponseOk(body.into())) } From b60a0d84f5a4e7cd105ed011055a97fcce8515f1 Mon Sep 17 00:00:00 2001 From: "Adam H. Leventhal" Date: Thu, 7 Apr 2022 12:07:51 -0700 Subject: [PATCH 6/6] changelog --- CHANGELOG.adoc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.adoc b/CHANGELOG.adoc index 35af1d966..1b040322f 100644 --- a/CHANGELOG.adoc +++ b/CHANGELOG.adoc @@ -23,12 +23,13 @@ https://github.com/oxidecomputer/dropshot/compare/v0.6.0\...HEAD[Full list of co === Other notable changes * https://github.com/oxidecomputer/dropshot/pull/198[#198] Responses that used `()` (the unit type) as their `Body` type parameter previously (and inaccurately) were represented in OpenAPI as an empty `responseBody`. They are now more accurately represented as a body whose value is `null` (4 bytes). We encourage those use cases to instead use either `HttpResponseUpdatedNoContent` or `HttpResponseDeleted` both of which have empty response bodies. If there are other situations where you would like a response type with no body, please file an issue. -* https://github.com/oxidecomputer/dropshot/pull/252[#252] Endpoints specified with the `#[endpoint ..]` attribute macro now use the first line of a doc comment as the OpenAPI `summary` and subsequent lines as the `description`. Previously all lines were used as the `description`. +* https://github.com/oxidecomputer/dropshot/pull/252[#252] Endpoints specified with the `##[endpoint ..]` attribute macro now use the first line of a doc comment as the OpenAPI `summary` and subsequent lines as the `description`. Previously all lines were used as the `description`. * https://github.com/oxidecomputer/dropshot/pull/260[#260] Pulls in a newer serde that changes error messages around parsing NonZeroU32. * https://github.com/oxidecomputer/dropshot/pull/283[#283] Add support for response headers with the `HttpResponseHeaders` type. Headers may either be defined by a struct type parameter (in which case they appear in the OpenAPI output) or *ad-hoc* added via `HttpResponseHeaders::headers_mut()`. * https://github.com/oxidecomputer/dropshot/pull/286[#286] OpenAPI output includes descriptions of 4xx and 5xx error responses. * https://github.com/oxidecomputer/dropshot/pull/296[#296] `ApiDescription` includes a `tag_config` method to specify both predefined tags with descriptions and links as well as a tag policy to ensure that endpoints, for example, only use predefined tags or have at least one tag. * https://github.com/oxidecomputer/dropshot/pull/317[#317] Allow use of usdt probes with stable Rust. Dropshot consumers can build with USDT probes enabled on stable compilers >= 1.59 (except on MacOS). +* https://github.com/oxidecomputer/dropshot/pull/310[#310] Freeform (and streaming) response bodies may be specified with specific HTTP response codes e.g. by having an endpoint return `Result, HttpError>`. == 0.6.0 (released 2021-11-18)