diff --git a/CHANGELOG.adoc b/CHANGELOG.adoc index 35af1d96..1b040322 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) diff --git a/dropshot/src/api_description.rs b/dropshot/src/api_description.rs index bafe1442..90b1056a 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 abb010e8..783b2451 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; @@ -1091,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; } /** @@ -1102,11 +1103,29 @@ 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 FreeformBody(pub Body); + +impl From for FreeformBody { + 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 + * used as the Body type for certain responses. + */ +#[doc(hidden)] +pub struct Empty; + /* * Specific Response Types * @@ -1115,15 +1134,85 @@ impl HttpResponse for Response { * kind of HTTP response body they produce. */ +/// Adapter trait that allows both concrete types that implement [JsonSchema] +/// and the [FreeformBody] type to add their content to a response builder +/// object. +pub trait HttpResponseContent { + 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; +} + +impl HttpResponseContent for FreeformBody { + fn to_response( + self, + builder: http::response::Builder, + ) -> HttpHandlerResult { + Ok(builder + .header(http::header::CONTENT_TYPE, CONTENT_TYPE_OCTET_STREAM) + .body(self.0)?) + } + + fn content_metadata() -> Option { + None + } +} + +impl HttpResponseContent for Empty { + fn to_response( + self, + builder: http::response::Builder, + ) -> HttpHandlerResult { + Ok(builder.body(Body::empty())?) + } + + fn content_metadata() -> Option { + Some(ApiSchemaGenerator::Static { + schema: Box::new(schemars::schema::Schema::Bool(false)), + dependencies: indexmap::IndexMap::default(), + }) + } +} + +impl HttpResponseContent 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 + * 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 { - type Body: JsonSchema + Serialize; + type Body: HttpResponseContent; const STATUS_CODE: StatusCode; const DESCRIPTION: &'static str; @@ -1133,13 +1222,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)) } } @@ -1148,17 +1232,14 @@ pub trait HttpTypedResponse: */ impl HttpResponse for T where - T: HttpTypedResponse, + T: HttpCodedResponse, { fn to_result(self) -> HttpHandlerResult { self.into() } - fn metadata() -> ApiEndpointResponse { + fn response_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() @@ -1182,22 +1263,22 @@ 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< - T: JsonSchema + Serialize + Send + Sync + 'static, ->(pub T); -impl HttpTypedResponse +pub struct HttpResponseCreated( + pub T, +); +impl HttpCodedResponse for HttpResponseCreated { type Body = T; const STATUS_CODE: StatusCode = StatusCode::CREATED; const DESCRIPTION: &'static str = "successful creation"; } -impl +impl From> for HttpHandlerResult { fn from(response: HttpResponseCreated) -> HttpHandlerResult { /* TODO-correctness (or polish?): add Location header */ - HttpResponseCreated::for_object(&response.0) + HttpResponseCreated::for_object(response.0) } } @@ -1206,21 +1287,21 @@ 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 HttpCodedResponse for HttpResponseAccepted { type Body = T; const STATUS_CODE: StatusCode = StatusCode::ACCEPTED; const DESCRIPTION: &'static str = "successfully enqueued operation"; } -impl +impl From> for HttpHandlerResult { fn from(response: HttpResponseAccepted) -> HttpHandlerResult { - HttpResponseAccepted::for_object(&response.0) + HttpResponseAccepted::for_object(response.0) } } @@ -1229,54 +1310,21 @@ impl * denotes an HTTP 200 "OK" response whose body is generated by serializing the * object. */ -pub struct HttpResponseOk( +pub struct HttpResponseOk( pub T, ); -impl HttpTypedResponse +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 { - 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() - } - - 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"); + HttpResponseOk::for_object(response.0) } } @@ -1286,16 +1334,14 @@ impl Serialize for Empty { */ 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"; } impl From for HttpHandlerResult { fn from(_: HttpResponseDeleted) -> HttpHandlerResult { - Ok(Response::builder() - .status(HttpResponseDeleted::STATUS_CODE) - .body(Body::empty())?) + HttpResponseDeleted::for_object(Empty) } } @@ -1306,16 +1352,14 @@ 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"; } impl From for HttpHandlerResult { fn from(_: HttpResponseUpdatedNoContent) -> HttpHandlerResult { - Ok(Response::builder() - .status(HttpResponseUpdatedNoContent::STATUS_CODE) - .body(Body::empty())?) + HttpResponseUpdatedNoContent::for_object(Empty) } } @@ -1333,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, @@ -1350,7 +1394,7 @@ impl HttpResponseHeaders { } } impl< - T: HttpTypedResponse, + T: HttpCodedResponse, H: JsonSchema + Serialize + Send + Sync + 'static, > HttpResponseHeaders { @@ -1367,7 +1411,7 @@ impl< } } impl< - T: HttpTypedResponse, + T: HttpCodedResponse, H: JsonSchema + Serialize + Send + Sync + 'static, > HttpResponse for HttpResponseHeaders { @@ -1398,8 +1442,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(), @@ -1496,8 +1540,6 @@ fn schema_extract_description( #[cfg(test)] mod test { - use crate::handler::HttpHandlerResult; - use crate::HttpResponseOk; use crate::{ api_description::ApiEndpointParameterMetadata, ApiEndpointParameter, ApiEndpointParameterLocation, PaginationParams, @@ -1506,7 +1548,6 @@ mod test { use serde::{Deserialize, Serialize}; use super::get_metadata; - use super::Empty; use super::ExtractorMetadata; #[derive(Deserialize, Serialize, JsonSchema)] @@ -1613,11 +1654,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 8b34eb41..22364f2d 100644 --- a/dropshot/src/lib.rs +++ b/dropshot/src/lib.rs @@ -632,6 +632,8 @@ pub use error::HttpError; pub use error::HttpErrorResponseBody; 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; @@ -639,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 512d9945..cd66c8ef 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 74795921..aab15074 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::HttpResponseContent` for `Ret` note: required by a bound in `HttpResponseOk` --> src/handler.rs | - | pub struct HttpResponseOk( - | ^^^^^^^^^ required by this bound in `HttpResponseOk` + | pub struct HttpResponseOk( + | ^^^^^^^^^^^^^^^^^^^ required by this bound in `HttpResponseOk` diff --git a/dropshot/tests/test_openapi.json b/dropshot/tests/test_openapi.json index 7b5563d3..f54ee74c 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 ac0e1270..0b6908f7 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, FreeformBody, HttpError, HttpResponseAccepted, HttpResponseCreated, HttpResponseDeleted, HttpResponseHeaders, HttpResponseOk, HttpResponseUpdatedNoContent, PaginationParams, Path, Query, RequestContext, ResultsPage, TagConfig, TagDetails, TypedBody, @@ -337,6 +337,18 @@ async fn handler17( unimplemented!(); } +#[endpoint { + method = GET, + path = "/playing/a/bit/nicer", + tags = ["it"], +}] +async fn handler18( + _rqctx: Arc>, +) -> Result, HttpError> { + let (_, body) = Body::channel(); + Ok(HttpResponseOk(body.into())) +} + fn make_api( maybe_tag_config: Option, ) -> Result, String> { @@ -363,6 +375,7 @@ fn make_api( api.register(handler15)?; api.register(handler16)?; api.register(handler17)?; + api.register(handler18)?; Ok(api) } diff --git a/dropshot/tests/test_openapi_fuller.json b/dropshot/tests/test_openapi_fuller.json index 4e9bd498..266123f0 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": [