From de536e4153c97940add4adb2c61d802248ce998d Mon Sep 17 00:00:00 2001 From: Tobias Bieniek Date: Thu, 19 Dec 2024 15:31:29 +0100 Subject: [PATCH] utopia-gen: Adjust params code to not set `nullable` on `Option` for `Query` params (#1248) --- utoipa-gen/CHANGELOG.md | 1 + utoipa-gen/src/component.rs | 45 +++++++-- .../src/component/features/attributes.rs | 6 ++ utoipa-gen/src/component/into_params.rs | 18 ++-- utoipa-gen/src/path/parameter.rs | 55 +++++++---- utoipa-gen/tests/path_derive.rs | 91 +++++++------------ utoipa-gen/tests/path_derive_axum_test.rs | 2 +- utoipa-gen/tests/path_derive_rocket.rs | 2 +- .../tests/path_parameter_derive_test.rs | 16 +--- 9 files changed, 132 insertions(+), 104 deletions(-) diff --git a/utoipa-gen/CHANGELOG.md b/utoipa-gen/CHANGELOG.md index bbcdac92..ca870952 100644 --- a/utoipa-gen/CHANGELOG.md +++ b/utoipa-gen/CHANGELOG.md @@ -13,6 +13,7 @@ ### Changed +* Adjust params code to not set `nullable` on `Option` for `Query` params (https://github.com/juhaku/utoipa/pull/1248) * Use `insta` for snapshot testing (https://github.com/juhaku/utoipa/pull/1247) * Make `parse_named_attributes` a method of `MediaTypeAttr` (https://github.com/juhaku/utoipa/pull/1236) * Use a re-exported `serde_json` dependency in macros instead of implicitly requiring it as dependency in end projects (https://github.com/juhaku/utoipa/pull/1243) diff --git a/utoipa-gen/src/component.rs b/utoipa-gen/src/component.rs index 45808ad9..90bce51d 100644 --- a/utoipa-gen/src/component.rs +++ b/utoipa-gen/src/component.rs @@ -694,6 +694,18 @@ pub struct ComponentSchemaProps<'c> { pub description: Option<&'c ComponentDescription<'c>>, } +impl ComponentSchemaProps<'_> { + fn set_nullable(&mut self) { + if !self + .features + .iter() + .any(|feature| matches!(feature, Feature::Nullable(_))) + { + self.features.push(Nullable::new().into()); + } + } +} + #[cfg_attr(feature = "debug", derive(Debug))] pub enum ComponentDescription<'c> { CommentAttributes(&'c CommentAttributes), @@ -750,11 +762,33 @@ pub struct ComponentSchema { } impl ComponentSchema { - pub fn new( + pub fn for_params( + mut schema_props: ComponentSchemaProps, + option_is_nullable: bool, + ) -> Result { + // Add nullable feature if not already exists. + // Option is always nullable, except when used in query parameters. + if schema_props.type_tree.is_option() && option_is_nullable { + schema_props.set_nullable() + } + + Self::new_inner(schema_props) + } + + pub fn new(mut schema_props: ComponentSchemaProps) -> Result { + // Add nullable feature if not already exists. Option is always nullable + if schema_props.type_tree.is_option() { + schema_props.set_nullable(); + } + + Self::new_inner(schema_props) + } + + fn new_inner( ComponentSchemaProps { container, type_tree, - mut features, + features, description, }: ComponentSchemaProps, ) -> Result { @@ -791,13 +825,6 @@ impl ComponentSchema { description, )?, Some(GenericType::Option) => { - // Add nullable feature if not already exists. Option is always nullable - if !features - .iter() - .any(|feature| matches!(feature, Feature::Nullable(_))) - { - features.push(Nullable::new().into()); - } let child = type_tree .children .as_ref() diff --git a/utoipa-gen/src/component/features/attributes.rs b/utoipa-gen/src/component/features/attributes.rs index 872b0083..e0306d0b 100644 --- a/utoipa-gen/src/component/features/attributes.rs +++ b/utoipa-gen/src/component/features/attributes.rs @@ -414,6 +414,12 @@ impl_feature! { pub struct ParameterIn(parameter::ParameterIn); } +impl ParameterIn { + pub fn is_query(&self) -> bool { + matches!(self.0, parameter::ParameterIn::Query) + } +} + impl Parse for ParameterIn { fn parse(input: syn::parse::ParseStream, _: Ident) -> syn::Result { parse_utils::parse_next(input, || input.parse::().map(Self)) diff --git a/utoipa-gen/src/component/into_params.rs b/utoipa-gen/src/component/into_params.rs index 7f3868db..3871cd80 100644 --- a/utoipa-gen/src/component/into_params.rs +++ b/utoipa-gen/src/component/into_params.rs @@ -407,12 +407,18 @@ impl Param { }); tokens.extend(param_features.to_token_stream()?); - let schema = ComponentSchema::new(component::ComponentSchemaProps { - type_tree: component, - features: schema_features, - description: None, - container: &Container { generics }, - })?; + let is_query = matches!(container_attributes.parameter_in, Some(Feature::ParameterIn(p)) if p.is_query()); + let option_is_nullable = !is_query; + + let schema = ComponentSchema::for_params( + component::ComponentSchemaProps { + type_tree: component, + features: schema_features, + description: None, + container: &Container { generics }, + }, + option_is_nullable, + )?; let schema_tokens = schema.to_token_stream(); tokens.extend(quote! { .schema(Some(#schema_tokens)).build() }); diff --git a/utoipa-gen/src/path/parameter.rs b/utoipa-gen/src/path/parameter.rs index 7bb3ede1..1f37a9e5 100644 --- a/utoipa-gen/src/path/parameter.rs +++ b/utoipa-gen/src/path/parameter.rs @@ -126,16 +126,21 @@ impl ToTokensDiagnostics for Parameter<'_> { ))] impl<'a> From> for Parameter<'a> { fn from(argument: crate::ext::ValueArgument<'a>) -> Self { + let parameter_in = if argument.argument_in == crate::ext::ArgumentIn::Path { + ParameterIn::Path + } else { + ParameterIn::Query + }; + + let option_is_nullable = parameter_in != ParameterIn::Query; + Self::Value(ValueParameter { name: argument.name.unwrap_or_else(|| Cow::Owned(String::new())), - parameter_in: if argument.argument_in == crate::ext::ArgumentIn::Path { - ParameterIn::Path - } else { - ParameterIn::Query - }, + parameter_in, parameter_schema: argument.type_tree.map(|type_tree| ParameterSchema { parameter_type: ParameterType::External(type_tree), features: Vec::new(), + option_is_nullable, }), ..Default::default() }) @@ -160,6 +165,7 @@ impl<'a> From> for Parameter<'a> { struct ParameterSchema<'p> { parameter_type: ParameterType<'p>, features: Vec, + option_is_nullable: bool, } impl ToTokensDiagnostics for ParameterSchema<'_> { @@ -178,14 +184,17 @@ impl ToTokensDiagnostics for ParameterSchema<'_> { let required: Required = (!type_tree.is_option()).into(); to_tokens( - ComponentSchema::new(component::ComponentSchemaProps { - type_tree, - features: self.features.clone(), - description: None, - container: &Container { - generics: &Generics::default(), + ComponentSchema::for_params( + component::ComponentSchemaProps { + type_tree, + features: self.features.clone(), + description: None, + container: &Container { + generics: &Generics::default(), + }, }, - })? + self.option_is_nullable, + )? .to_token_stream(), required, ); @@ -199,14 +208,17 @@ impl ToTokensDiagnostics for ParameterSchema<'_> { schema_features.push(Feature::Inline(inline_type.is_inline.into())); to_tokens( - ComponentSchema::new(component::ComponentSchemaProps { - type_tree: &type_tree, - features: schema_features, - description: None, - container: &Container { - generics: &Generics::default(), + ComponentSchema::for_params( + component::ComponentSchemaProps { + type_tree: &type_tree, + features: schema_features, + description: None, + container: &Container { + generics: &Generics::default(), + }, }, - })? + self.option_is_nullable, + )? .to_token_stream(), required, ); @@ -267,6 +279,7 @@ impl Parse for ValueParameter<'_> { }) })?), features: Vec::new(), + option_is_nullable: true, }); } } else { @@ -289,6 +302,10 @@ impl Parse for ValueParameter<'_> { parameter.features = (schema_features.clone(), parameter_features); if let Some(parameter_schema) = &mut parameter.parameter_schema { parameter_schema.features = schema_features; + + if parameter.parameter_in == ParameterIn::Query { + parameter_schema.option_is_nullable = false; + } } Ok(parameter) diff --git a/utoipa-gen/tests/path_derive.rs b/utoipa-gen/tests/path_derive.rs index ea5f9b22..490b7653 100644 --- a/utoipa-gen/tests/path_derive.rs +++ b/utoipa-gen/tests/path_derive.rs @@ -249,9 +249,7 @@ fn derive_path_with_extra_attributes_without_nested_module() { "parameters.[1].in" = r#""query""#, "Parameter 1 in" "parameters.[1].name" = r#""since""#, "Parameter 1 name" "parameters.[1].required" = r#"false"#, "Parameter 1 required" - "parameters.[1].schema.allOf.[0].format" = r#"null"#, "Parameter 1 schema format" - "parameters.[1].schema.allOf.[0].type" = r#"null"#, "Parameter 1 schema type" - "parameters.[1].schema.allOf.nullable" = r#"null"#, "Parameter 1 schema type" + "parameters.[1].schema.type" = r#""string""#, "Parameter 1 schema type" } } @@ -477,14 +475,7 @@ fn derive_path_with_parameter_schema() { "name": "since", "required": false, "schema": { - "oneOf": [ - { - "type": "null" - }, - { - "$ref": "#/components/schemas/Since" - } - ], + "$ref": "#/components/schemas/Since" } } ]) @@ -549,28 +540,21 @@ fn derive_path_with_parameter_inline_schema() { "name": "since", "required": false, "schema": { - "oneOf": [ - { - "type": "null" + "properties": { + "date": { + "description": "Some date", + "type": "string" }, - { - "properties": { - "date": { - "description": "Some date", - "type": "string" - }, - "time": { - "description": "Some time", - "type": "string" - } - }, - "required": [ - "date", - "time" - ], - "type": "object" + "time": { + "description": "Some time", + "type": "string" } + }, + "required": [ + "date", + "time" ], + "type": "object" } } ]) @@ -831,7 +815,7 @@ fn derive_required_path_params() { "name": "vec_default", "required": false, "schema": { - "type": ["array", "null"], + "type": "array", "items": { "type": "string" } @@ -842,7 +826,7 @@ fn derive_required_path_params() { "name": "string_default", "required": false, "schema": { - "type": ["string", "null"] + "type": "string" } }, { @@ -872,7 +856,7 @@ fn derive_required_path_params() { "items": { "type": "string" }, - "type": ["array", "null"], + "type": "array", }, }, { @@ -880,7 +864,7 @@ fn derive_required_path_params() { "name": "string_option", "required": false, "schema": { - "type": ["string", "null"] + "type": "string" } }, { @@ -938,7 +922,7 @@ fn derive_path_params_with_serde_and_custom_rename() { "name": "vecDefault", "required": false, "schema": { - "type": ["array", "null"], + "type": "array", "items": { "type": "string" } @@ -949,7 +933,7 @@ fn derive_path_params_with_serde_and_custom_rename() { "name": "STRING", "required": false, "schema": { - "type": ["string", "null"] + "type": "string" } }, { @@ -1001,7 +985,7 @@ fn derive_path_params_custom_rename_all() { "name": "vecDefault", "required": false, "schema": { - "type": ["array", "null"], + "type": "array", "items": { "type": "string" } @@ -1030,7 +1014,7 @@ fn derive_path_params_custom_rename_all_serde_will_override() { "name": "VEC_DEFAULT", "required": false, "schema": { - "type": ["array", "null"], + "type": "array", "items": { "type": "string" } @@ -1163,7 +1147,7 @@ fn derive_path_params_intoparams() { "name": "since", "required": false, "schema": { - "type": ["string", "null"] + "type": "string" }, "style": "form" }, @@ -1196,17 +1180,10 @@ fn derive_path_params_intoparams() { "name": "foo_inline_option", "required": false, "schema": { - "oneOf": [ - { - "type": "null", - }, - { - "default": "foo1", - "example": "foo1", - "enum": ["foo1", "foo2"], - "type": "string", - } - ], + "default": "foo1", + "example": "foo1", + "enum": ["foo1", "foo2"], + "type": "string", }, "style": "form" }, @@ -1343,7 +1320,7 @@ fn derive_path_params_into_params_with_value_type() { "name": "value3", "required": false, "schema": { - "type": ["string", "null"] + "type": "string" } }, { @@ -1351,7 +1328,7 @@ fn derive_path_params_into_params_with_value_type() { "name": "value4", "required": false, "schema": { - "type": ["object", "null"] + "type": "object" } }, { @@ -1776,7 +1753,7 @@ fn derive_into_params_required() { "name": "name2", "required": false, "schema": { - "type": ["string", "null"], + "type": "string", }, }, { @@ -1784,7 +1761,7 @@ fn derive_into_params_required() { "name": "name3", "required": true, "schema": { - "type": ["string", "null"], + "type": "string", }, }, ]) @@ -1830,7 +1807,7 @@ fn derive_into_params_with_serde_skip() { "name": "name2", "required": false, "schema": { - "type": ["string", "null"], + "type": "string", }, }, ]) @@ -1878,7 +1855,7 @@ fn derive_into_params_with_serde_skip_deserializing() { "name": "name2", "required": false, "schema": { - "type": ["string", "null"], + "type": "string", }, }, ]) @@ -1924,7 +1901,7 @@ fn derive_into_params_with_serde_skip_serializing() { "name": "name2", "required": false, "schema": { - "type": ["string", "null"], + "type": "string", }, }, ]) diff --git a/utoipa-gen/tests/path_derive_axum_test.rs b/utoipa-gen/tests/path_derive_axum_test.rs index 66048738..4272ee6d 100644 --- a/utoipa-gen/tests/path_derive_axum_test.rs +++ b/utoipa-gen/tests/path_derive_axum_test.rs @@ -729,7 +729,7 @@ fn derive_path_with_validation_attributes_axum() { }, { "schema": { - "type": ["string", "null"], + "type": "string", "minLength": 3, }, "required": false, diff --git a/utoipa-gen/tests/path_derive_rocket.rs b/utoipa-gen/tests/path_derive_rocket.rs index c823f42b..c3065e59 100644 --- a/utoipa-gen/tests/path_derive_rocket.rs +++ b/utoipa-gen/tests/path_derive_rocket.rs @@ -137,7 +137,7 @@ fn resolve_get_with_optional_query_args() { "items": { "type": "string", }, - "type": ["array", "null"], + "type": "array", } } ]) diff --git a/utoipa-gen/tests/path_parameter_derive_test.rs b/utoipa-gen/tests/path_parameter_derive_test.rs index 1f1e90f3..9019e720 100644 --- a/utoipa-gen/tests/path_parameter_derive_test.rs +++ b/utoipa-gen/tests/path_parameter_derive_test.rs @@ -194,7 +194,7 @@ fn derive_parameters_with_all_types() { "[2].description" = r#""Foo numbers list""#, "Parameter description" "[2].required" = r#"false"#, "Parameter required" "[2].deprecated" = r#"null"#, "Parameter deprecated" - "[2].schema.type" = r#"["array","null"]"#, "Parameter schema type" + "[2].schema.type" = r#""array""#, "Parameter schema type" "[2].schema.format" = r#"null"#, "Parameter schema format" "[2].schema.items.type" = r#""integer""#, "Parameter schema items type" "[2].schema.items.format" = r#""int64""#, "Parameter schema items format" @@ -286,7 +286,7 @@ fn derive_params_with_params_ext() { "[0].description" = r#""Foo value description""#, "Parameter description" "[0].required" = r#"false"#, "Parameter required" "[0].deprecated" = r#"true"#, "Parameter deprecated" - "[0].schema.type" = r#"["array","null"]"#, "Parameter schema type" + "[0].schema.type" = r#""array""#, "Parameter schema type" "[0].schema.items.type" = r#""string""#, "Parameter schema items type" "[0].style" = r#""form""#, "Parameter style" "[0].allowReserved" = r#"true"#, "Parameter allowReserved" @@ -330,7 +330,7 @@ fn derive_path_params_with_parameter_type_args() { "deprecated": true, "description": "Foo value description", "schema": { - "type": ["array", "null"], + "type": "array", "items": { "maxLength": 20, "pattern": r"\w", @@ -424,10 +424,7 @@ fn derive_into_params_required_custom_query_parameter_required() { "schema": { "format": "int32", "minimum": 0, - "type": [ - "integer", - "null" - ] + "type": "integer" } }, { @@ -439,10 +436,7 @@ fn derive_into_params_required_custom_query_parameter_required() { "schema": { "format": "int32", "minimum": 0, - "type": [ - "integer", - "null" - ] + "type": "integer" } } ])