Skip to content

Commit

Permalink
Fix respect required attribute (#990)
Browse files Browse the repository at this point in the history
Prior to this PR the `required` attribute was not correctly respected in
all cases. This was due to the fact that the `required` attribute had the
same weight with the default rule that is checked from the option and some
other serde attributes. Even this works in most cases but lacks control
over explicitly defining `required` to false when type is not
optional.

This PR fixes this by making clear separation between the default rule
and the manual `required` attribute with preference on the manually
defined `required` attribute.

Fixes #882
  • Loading branch information
juhaku authored Aug 12, 2024
1 parent 55544ef commit 5e780f1
Show file tree
Hide file tree
Showing 5 changed files with 192 additions and 17 deletions.
19 changes: 10 additions & 9 deletions utoipa-gen/src/component/into_params.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,8 @@ use crate::{

use super::{
features::{
impl_into_inner, impl_merge, parse_features, pop_feature, pop_feature_as_inner, Feature,
FeaturesExt, IntoInner, Merge, ToTokensExt,
impl_into_inner, impl_merge, parse_features, pop_feature, Feature, FeaturesExt, IntoInner,
Merge, ToTokensExt,
},
serde::{self, SerdeContainer, SerdeValue},
ComponentSchema, TypeTree,
Expand Down Expand Up @@ -432,14 +432,15 @@ impl ToTokensDiagnostics for Param<'_> {
.map_try(|value_type| value_type.as_type_tree())?
.unwrap_or(type_tree);

let required = pop_feature_as_inner!(param_features => Feature::Required(_v))
.as_ref()
.map(super::features::Required::is_true)
.unwrap_or(false);
let required: Option<features::Required> =
pop_feature!(param_features => Feature::Required(_)).into_inner();
let component_required = !component.is_option()
&& super::is_required(field_serde_params, self.serde_container);

let non_required = (component.is_option() && !required)
|| !component::is_required(field_serde_params, self.serde_container);
let required: Required = (!non_required).into();
let required = match (required, component_required) {
(Some(required_feature), _) => Into::<Required>::into(required_feature.is_true()),
(None, component_required) => Into::<Required>::into(component_required),
};

tokens.extend(quote! {
.required(#required)
Expand Down
13 changes: 7 additions & 6 deletions utoipa-gen/src/component/schema.rs
Original file line number Diff line number Diff line change
Expand Up @@ -477,13 +477,14 @@ impl ToTokensDiagnostics for NamedStructSchema<'_> {
object_tokens.extend(quote! {
.property(#name, #field_schema)
});
let component_required =
!is_option && super::is_required(field_rules, &container_rules);
let required = match (required, component_required) {
(Some(required), _) => required.is_true(),
(None, component_required) => component_required,
};

if (!is_option && super::is_required(field_rules, &container_rules))
|| required
.as_ref()
.map(super::features::Required::is_true)
.unwrap_or(false)
{
if required {
object_tokens.extend(quote! {
.required(#name)
})
Expand Down
105 changes: 104 additions & 1 deletion utoipa-gen/tests/path_parameter_derive_test.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use assert_json_diff::assert_json_eq;
use serde_json::json;
use utoipa::OpenApi;
use utoipa::{OpenApi, Path};

mod common;

Expand Down Expand Up @@ -345,3 +345,106 @@ fn derive_path_params_with_parameter_type_args() {
])
);
}

macro_rules! into_params {
( $( #[$me:meta] )* $key:ident $name:ident $( $tt:tt )*) => {
{
#[derive(utoipa::IntoParams)]
$(#[$me])*
$key $name $( $tt )*

#[utoipa::path(get, path = "/handler", params($name))]
#[allow(unused)]
fn handler() {}

let value = serde_json::to_value(&__path_handler::path_item())
.expect("path item should serialize to json");
value.pointer("/get/parameters").expect("should have get/handler").clone()
}
};
}

#[test]
fn derive_into_params_required_custom_query_parameter_required() {
#[allow(unused)]
struct Param<T>(T);

let value = into_params! {
#[into_params(parameter_in = Query)]
#[allow(unused)]
struct TasksFilterQuery {
/// Maximum number of results to return.
#[param(required = false, value_type = u32, example = 12)]
pub limit: Param<u32>,
/// Maximum number of results to return.
#[param(required = true, value_type = u32, example = 12)]
pub limit_explisit_required: Param<u32>,
/// Maximum number of results to return.
#[param(value_type = Option<u32>, example = 12)]
pub not_required: Param<u32>,
/// Maximum number of results to return.
#[param(required = true, value_type = Option<u32>, example = 12)]
pub option_required: Param<u32>,
}
};

assert_json_eq!(
value,
json!([
{
"description": "Maximum number of results to return.",
"example": 12,
"in": "query",
"name": "limit",
"required": false,
"schema": {
"format": "int32",
"minimum": 0,
"type": "integer"
}
},
{
"description": "Maximum number of results to return.",
"example": 12,
"in": "query",
"name": "limit_explisit_required",
"required": true,
"schema": {
"format": "int32",
"minimum": 0,
"type": "integer"
}
},
{
"description": "Maximum number of results to return.",
"example": 12,
"in": "query",
"name": "not_required",
"required": false,
"schema": {
"format": "int32",
"minimum": 0,
"type": [
"integer",
"null"
]
}
},
{
"description": "Maximum number of results to return.",
"example": 12,
"in": "query",
"name": "option_required",
"required": true,
"schema": {
"format": "int32",
"minimum": 0,
"type": [
"integer",
"null"
]
}
}
])
);
}
71 changes: 71 additions & 0 deletions utoipa-gen/tests/schema_derive_test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5497,3 +5497,74 @@ fn content_media_type_named_field() {
})
)
}

#[test]
fn derive_schema_required_custom_type_required() {
#[allow(unused)]
struct Param<T>(T);

let value = api_doc! {
#[allow(unused)]
struct Params {
/// Maximum number of results to return.
#[schema(required = false, value_type = u32, example = 12)]
limit: Param<u32>,
/// Maximum number of results to return.
#[schema(required = true, value_type = u32, example = 12)]
limit_explisit_required: Param<u32>,
/// Maximum number of results to return.
#[schema(value_type = Option<u32>, example = 12)]
not_required: Param<u32>,
/// Maximum number of results to return.
#[schema(required = true, value_type = Option<u32>, example = 12)]
option_required: Param<u32>,
}
};

assert_json_eq!(
value,
json!({
"properties": {
"limit": {
"description": "Maximum number of results to return.",
"example": 12,
"format": "int32",
"minimum": 0,
"type": "integer"
},
"limit_explisit_required": {
"description": "Maximum number of results to return.",
"example": 12,
"format": "int32",
"minimum": 0,
"type": "integer"
},
"not_required": {
"description": "Maximum number of results to return.",
"example": 12,
"format": "int32",
"minimum": 0,
"type": [
"integer",
"null"
]
},
"option_required": {
"description": "Maximum number of results to return.",
"example": 12,
"format": "int32",
"minimum": 0,
"type": [
"integer",
"null"
]
}
},
"type": "object",
"required": [
"limit_explisit_required",
"option_required"
]
})
);
}
1 change: 0 additions & 1 deletion utoipa/src/openapi.rs
Original file line number Diff line number Diff line change
Expand Up @@ -657,7 +657,6 @@ mod tests {
);
let serialized = serde_json::to_string_pretty(&openapi)?;

dbg!(&raw_json);
assert_eq!(
serialized, raw_json,
"expected serialized json to match raw: \nserialized: \n{serialized} \nraw: \n{raw_json}"
Expand Down

0 comments on commit 5e780f1

Please sign in to comment.