From a2928e5f144db66c8a156015a501455d9b9063e2 Mon Sep 17 00:00:00 2001 From: Adam Leventhal Date: Wed, 1 Dec 2021 11:10:22 -0800 Subject: [PATCH] Fix conflation of () which serializes to the JSON value `null` and a return that produces no content (#198) * Fix conflation of () which serializes to the JSON value `null` and a return that actually produces no content. For the latter, we introduce, a new type `Empty` which has a custom JsonSchema implementation that evaluates to the empty schema (i.e. nothing can match it). --- CHANGELOG.adoc | 4 ++ dropshot/src/api_description.rs | 85 +++++++++++++++++++++---- dropshot/src/from_map.rs | 8 +++ dropshot/src/handler.rs | 48 +++++++++++++- dropshot/src/pagination.rs | 2 + dropshot/tests/test_openapi.json | 63 +++++++++++++++--- dropshot/tests/test_openapi.rs | 19 ++++-- dropshot/tests/test_openapi_fuller.json | 63 +++++++++++++++--- 8 files changed, 253 insertions(+), 39 deletions(-) diff --git a/CHANGELOG.adoc b/CHANGELOG.adoc index 2b3c6b70..02ad87aa 100644 --- a/CHANGELOG.adoc +++ b/CHANGELOG.adoc @@ -20,6 +20,10 @@ https://github.com/oxidecomputer/dropshot/compare/v0.6.0\...HEAD[Full list of co * https://github.com/oxidecomputer/dropshot/pull/197[#197] Endpoints using wildcard path params (i.e. those using the `/foo/{bar:.*}` syntax) previously could be included in OpenAPI output albeit in a form that was invalid. Specifying a wildcard path **without** also specifying `unpublished = true` is now a **compile-time error**. * https://github.com/oxidecomputer/dropshot/pull/204[#204] Rust 1.58.0-nightly introduced a new feature `asm_sym` which the `usdt` crate requires on macOS. As of this change 1.58.0-nightly or later is required to build with the `usdt-probes` feature on macOS. +=== 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. + == 0.6.0 (released 2021-11-18) https://github.com/oxidecomputer/dropshot/compare/v0.5.1\...v0.6.0[Full list of commits] diff --git a/dropshot/src/api_description.rs b/dropshot/src/api_description.rs index 9b334c6e..85f338bf 100644 --- a/dropshot/src/api_description.rs +++ b/dropshot/src/api_description.rs @@ -55,13 +55,14 @@ impl<'a, Context: ServerContext> ApiEndpoint { ResponseType: HttpResponse + Send + Sync + 'static, { let func_parameters = FuncParams::metadata(); + let response = ResponseType::metadata(); ApiEndpoint { operation_id, handler: HttpRouteHandler::new(handler), method, path: path.to_string(), parameters: func_parameters.parameters, - response: ResponseType::metadata(), + response, description: None, tags: vec![], paginated: func_parameters.paginated, @@ -600,7 +601,8 @@ impl ApiDescription { } }; let mut content = indexmap::IndexMap::new(); - if !is_null(&js) { + if !is_empty(&js) { + println!("not empty"); content.insert( CONTENT_TYPE_JSON.to_string(), openapiv3::MediaType { @@ -674,18 +676,60 @@ impl ApiDescription { } /** - * Returns true iff the schema represents the null type i.e. the rust type `()`. + * Returns true iff the schema represents the void schema that matches no data. */ -fn is_null(schema: &schemars::schema::Schema) -> bool { +fn is_empty(schema: &schemars::schema::Schema) -> bool { + if let schemars::schema::Schema::Bool(false) = schema { + return true; + } if let schemars::schema::Schema::Object(schemars::schema::SchemaObject { - instance_type: Some(schemars::schema::SingleOrVec::Single(it)), - .. + metadata: _, + instance_type: None, + format: None, + enum_values: None, + const_value: None, + subschemas: Some(subschemas), + number: None, + string: None, + array: None, + object: None, + reference: None, + extensions: _, }) = schema { - if let schemars::schema::InstanceType::Null = it.as_ref() { - return true; + if let schemars::schema::SubschemaValidation { + all_of: None, + any_of: None, + one_of: None, + not: Some(not), + if_schema: None, + then_schema: None, + else_schema: None, + } = subschemas.as_ref() + { + match not.as_ref() { + schemars::schema::Schema::Bool(true) => return true, + schemars::schema::Schema::Object( + schemars::schema::SchemaObject { + metadata: _, + instance_type: None, + format: None, + enum_values: None, + const_value: None, + subschemas: None, + number: None, + string: None, + array: None, + object: None, + reference: None, + extensions: _, + }, + ) => return true, + _ => {} + } } } + false } @@ -744,7 +788,14 @@ fn j2oas_schema_object( }; let kind = match (ty, &obj.subschemas) { - (Some(schemars::schema::InstanceType::Null), None) => todo!(), + (Some(schemars::schema::InstanceType::Null), None) => { + openapiv3::SchemaKind::Type(openapiv3::Type::String( + openapiv3::StringType { + enumeration: vec![None], + ..Default::default() + }, + )) + } (Some(schemars::schema::InstanceType::Boolean), None) => { openapiv3::SchemaKind::Type(openapiv3::Type::Boolean {}) } @@ -801,25 +852,33 @@ fn j2oas_schema_object( fn j2oas_subschemas( subschemas: &schemars::schema::SubschemaValidation, ) -> openapiv3::SchemaKind { - match (&subschemas.all_of, &subschemas.any_of, &subschemas.one_of) { - (Some(all_of), None, None) => openapiv3::SchemaKind::AllOf { + match ( + &subschemas.all_of, + &subschemas.any_of, + &subschemas.one_of, + &subschemas.not, + ) { + (Some(all_of), None, None, None) => openapiv3::SchemaKind::AllOf { all_of: all_of .iter() .map(|schema| j2oas_schema(None, schema)) .collect::>(), }, - (None, Some(any_of), None) => openapiv3::SchemaKind::AnyOf { + (None, Some(any_of), None, None) => openapiv3::SchemaKind::AnyOf { any_of: any_of .iter() .map(|schema| j2oas_schema(None, schema)) .collect::>(), }, - (None, None, Some(one_of)) => openapiv3::SchemaKind::OneOf { + (None, None, Some(one_of), None) => openapiv3::SchemaKind::OneOf { one_of: one_of .iter() .map(|schema| j2oas_schema(None, schema)) .collect::>(), }, + (None, None, None, Some(not)) => openapiv3::SchemaKind::Not { + not: Box::new(j2oas_schema(None, not)), + }, _ => panic!("invalid subschema {:#?}", subschemas), } } diff --git a/dropshot/src/from_map.rs b/dropshot/src/from_map.rs index 6b574914..8cba81e6 100644 --- a/dropshot/src/from_map.rs +++ b/dropshot/src/from_map.rs @@ -475,6 +475,8 @@ mod test { #[test] fn test_deep() { + #![allow(dead_code)] + #[derive(Deserialize, Debug)] struct A { b: B, @@ -494,6 +496,8 @@ mod test { } #[test] fn test_missing_data1() { + #![allow(dead_code)] + #[derive(Deserialize, Debug)] struct A { aaa: String, @@ -513,6 +517,8 @@ mod test { } #[test] fn test_missing_data2() { + #![allow(dead_code)] + #[derive(Deserialize, Debug)] struct A { aaa: String, @@ -576,6 +582,8 @@ mod test { } #[test] fn wherefore_art_thou_a_valid_sequence_when_in_fact_you_are_a_lone_value() { + #![allow(dead_code)] + #[derive(Deserialize, Debug)] struct A { b: Vec, diff --git a/dropshot/src/handler.rs b/dropshot/src/handler.rs index 511ae44e..831d446d 100644 --- a/dropshot/src/handler.rs +++ b/dropshot/src/handler.rs @@ -1231,6 +1231,39 @@ impl From> } } +/** + * 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"); + } +} + /** * `HttpResponseDeleted` represents an HTTP 204 "No Content" response, intended * for use when an API operation has successfully deleted an object. @@ -1238,7 +1271,7 @@ impl From> pub struct HttpResponseDeleted(); impl HttpTypedResponse for HttpResponseDeleted { - type Body = (); + type Body = Empty; const STATUS_CODE: StatusCode = StatusCode::NO_CONTENT; const DESCRIPTION: &'static str = "successful deletion"; } @@ -1256,8 +1289,9 @@ impl From for HttpHandlerResult { * has nothing to return. */ pub struct HttpResponseUpdatedNoContent(); + impl HttpTypedResponse for HttpResponseUpdatedNoContent { - type Body = (); + type Body = Empty; const STATUS_CODE: StatusCode = StatusCode::NO_CONTENT; const DESCRIPTION: &'static str = "resource updated"; } @@ -1271,6 +1305,8 @@ impl From for HttpHandlerResult { #[cfg(test)] mod test { + use crate::handler::HttpHandlerResult; + use crate::HttpResponseOk; use crate::{ api_description::ApiEndpointParameterMetadata, ApiEndpointParameter, ApiEndpointParameterLocation, PaginationParams, @@ -1279,6 +1315,7 @@ mod test { use serde::{Deserialize, Serialize}; use super::get_metadata; + use super::Empty; use super::ExtractorMetadata; #[derive(Deserialize, Serialize, JsonSchema)] @@ -1385,4 +1422,11 @@ 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/pagination.rs b/dropshot/src/pagination.rs index bfc751cc..03a92573 100644 --- a/dropshot/src/pagination.rs +++ b/dropshot/src/pagination.rs @@ -644,6 +644,7 @@ mod test { } #[derive(Debug, Deserialize)] + #[allow(dead_code)] struct MyOptionalScanParams { the_field: Option, only_good: Option, @@ -799,6 +800,7 @@ mod test { * precedence (and it's not clear how else this could work). */ #[derive(Debug, Deserialize)] + #[allow(dead_code)] struct SketchyScanParams { page_token: String, } diff --git a/dropshot/tests/test_openapi.json b/dropshot/tests/test_openapi.json index ac87cc9f..10707536 100644 --- a/dropshot/tests/test_openapi.json +++ b/dropshot/tests/test_openapi.json @@ -20,8 +20,8 @@ "required": true }, "responses": { - "200": { - "description": "successful operation" + "204": { + "description": "resource updated" } } } @@ -74,8 +74,8 @@ "required": true }, "responses": { - "200": { - "description": "successful operation" + "204": { + "description": "resource updated" } } } @@ -94,8 +94,8 @@ "required": true }, "responses": { - "200": { - "description": "successful operation" + "204": { + "description": "resource updated" } } } @@ -114,8 +114,8 @@ "required": true }, "responses": { - "200": { - "description": "successful operation" + "204": { + "description": "resource updated" } } } @@ -243,7 +243,18 @@ "operationId": "handler1", "responses": { "200": { - "description": "successful operation" + "description": "successful operation", + "content": { + "application/json": { + "schema": { + "title": "Null", + "type": "string", + "enum": [ + null + ] + } + } + } } } } @@ -299,7 +310,18 @@ }, "responses": { "202": { - "description": "successfully enqueued operation" + "description": "successfully enqueued operation", + "content": { + "application/json": { + "schema": { + "title": "Null", + "type": "string", + "enum": [ + null + ] + } + } + } } } } @@ -334,6 +356,27 @@ } } } + }, + "/unit_please": { + "get": { + "operationId": "handler15", + "responses": { + "200": { + "description": "successful operation", + "content": { + "application/json": { + "schema": { + "title": "Null", + "type": "string", + "enum": [ + null + ] + } + } + } + } + } + } } }, "components": { diff --git a/dropshot/tests/test_openapi.rs b/dropshot/tests/test_openapi.rs index bd995af6..bc21ec18 100644 --- a/dropshot/tests/test_openapi.rs +++ b/dropshot/tests/test_openapi.rs @@ -133,7 +133,7 @@ async fn handler6( async fn handler7( _rqctx: Arc>, _dump: UntypedBody, -) -> Result, HttpError> { +) -> Result { unimplemented!(); } @@ -195,7 +195,7 @@ struct NeverDuplicatedBodyNextLevel { async fn handler10( _rqctx: Arc>, _b: TypedBody, -) -> Result, HttpError> { +) -> Result { unimplemented!(); } @@ -206,7 +206,7 @@ async fn handler10( async fn handler11( _rqctx: Arc>, _b: TypedBody, -) -> Result, HttpError> { +) -> Result { unimplemented!(); } @@ -234,7 +234,7 @@ struct NeverDuplicatedNext { async fn handler12( _rqctx: Arc>, _b: TypedBody, -) -> Result, HttpError> { +) -> Result { unimplemented!(); } @@ -266,6 +266,16 @@ async fn handler14( unimplemented!(); } +#[endpoint { + method = GET, + path = "/unit_please", +}] +async fn handler15( + _rqctx: Arc>, +) -> Result, HttpError> { + unimplemented!(); +} + fn make_api() -> Result, String> { let mut api = ApiDescription::new(); api.register(handler1)?; @@ -282,6 +292,7 @@ fn make_api() -> Result, String> { api.register(handler12)?; api.register(handler13)?; api.register(handler14)?; + api.register(handler15)?; Ok(api) } diff --git a/dropshot/tests/test_openapi_fuller.json b/dropshot/tests/test_openapi_fuller.json index d27d1054..344a8bca 100644 --- a/dropshot/tests/test_openapi_fuller.json +++ b/dropshot/tests/test_openapi_fuller.json @@ -28,8 +28,8 @@ "required": true }, "responses": { - "200": { - "description": "successful operation" + "204": { + "description": "resource updated" } } } @@ -82,8 +82,8 @@ "required": true }, "responses": { - "200": { - "description": "successful operation" + "204": { + "description": "resource updated" } } } @@ -102,8 +102,8 @@ "required": true }, "responses": { - "200": { - "description": "successful operation" + "204": { + "description": "resource updated" } } } @@ -122,8 +122,8 @@ "required": true }, "responses": { - "200": { - "description": "successful operation" + "204": { + "description": "resource updated" } } } @@ -251,7 +251,18 @@ "operationId": "handler1", "responses": { "200": { - "description": "successful operation" + "description": "successful operation", + "content": { + "application/json": { + "schema": { + "title": "Null", + "type": "string", + "enum": [ + null + ] + } + } + } } } } @@ -307,7 +318,18 @@ }, "responses": { "202": { - "description": "successfully enqueued operation" + "description": "successfully enqueued operation", + "content": { + "application/json": { + "schema": { + "title": "Null", + "type": "string", + "enum": [ + null + ] + } + } + } } } } @@ -342,6 +364,27 @@ } } } + }, + "/unit_please": { + "get": { + "operationId": "handler15", + "responses": { + "200": { + "description": "successful operation", + "content": { + "application/json": { + "schema": { + "title": "Null", + "type": "string", + "enum": [ + null + ] + } + } + } + } + } + } } }, "components": {