From 63af0ceb73e177bc792a7b256b223c3072910313 Mon Sep 17 00:00:00 2001 From: Graham Esau Date: Sat, 11 Apr 2020 22:06:48 +0100 Subject: [PATCH] Fix skip_serializing_if/serialize_with handling Previously whenever a field with a default value has both `skip_serializing_if` and `with`/`serialize_with` attributes, the value would be converted to a type that performs the custom serialization before checking if it should be serialized. This would cause the wrong type to be given to the skip_serializing_if function, causing a compile error. Issue #26 --- schemars/src/gen.rs | 20 ++++++++++++---- schemars/src/json_schema_impls/core.rs | 5 +--- schemars/src/lib.rs | 5 +--- schemars/tests/default.rs | 11 ++++++++- schemars/tests/expected/default.json | 3 +++ schemars_derive/src/lib.rs | 19 +++++++++++++-- schemars_derive/src/metadata.rs | 33 ++++++-------------------- 7 files changed, 54 insertions(+), 42 deletions(-) diff --git a/schemars/src/gen.rs b/schemars/src/gen.rs index e0aa203..41e9641 100644 --- a/schemars/src/gen.rs +++ b/schemars/src/gen.rs @@ -343,12 +343,22 @@ impl SchemaGenerator { } } - pub(crate) fn apply_metadata(&self, schema: Schema, metadata: Metadata) -> Schema { - let mut schema_obj = schema.into(); + /// This function is only public for use by schemars_derive. + /// + /// It should not be considered part of the public API. + #[doc(hidden)] + pub fn apply_metadata(&self, schema: Schema, metadata: Option) -> Schema { + match metadata { + None => return schema, + Some(metadata) if metadata == Metadata::default() => return schema, + Some(metadata) => { + let mut schema_obj = schema.into(); - self.make_extensible(&mut schema_obj); - schema_obj.metadata = Some(Box::new(metadata)).merge(schema_obj.metadata); + self.make_extensible(&mut schema_obj); + schema_obj.metadata = Some(Box::new(metadata)).merge(schema_obj.metadata); - Schema::Object(schema_obj) + Schema::Object(schema_obj) + } + } } } diff --git a/schemars/src/json_schema_impls/core.rs b/schemars/src/json_schema_impls/core.rs index 8accdb1..84917c5 100644 --- a/schemars/src/json_schema_impls/core.rs +++ b/schemars/src/json_schema_impls/core.rs @@ -65,10 +65,7 @@ impl JsonSchema for Option { _required: bool, ) { let mut schema = gen.subschema_for::(); - - if let Some(metadata) = metadata { - schema = gen.apply_metadata(schema, metadata); - } + schema = gen.apply_metadata(schema, metadata); let object = parent.object(); object.properties.insert(name, schema); diff --git a/schemars/src/lib.rs b/schemars/src/lib.rs index d3204da..1315897 100644 --- a/schemars/src/lib.rs +++ b/schemars/src/lib.rs @@ -300,10 +300,7 @@ pub trait JsonSchema { required: bool, ) { let mut schema = gen.subschema_for::(); - - if let Some(metadata) = metadata { - schema = gen.apply_metadata(schema, metadata); - } + schema = gen.apply_metadata(schema, metadata); let object = parent.object(); if required { diff --git a/schemars/tests/default.rs b/schemars/tests/default.rs index aabdde6..d91585a 100644 --- a/schemars/tests/default.rs +++ b/schemars/tests/default.rs @@ -3,6 +3,10 @@ use schemars::JsonSchema; use serde::{Deserialize, Serialize}; use util::*; +fn is_default(value: &T) -> bool { + value == &T::default() +} + fn ten_and_true() -> MyStruct2 { MyStruct2 { my_int: 10, @@ -28,9 +32,14 @@ pub struct MyStruct { pub my_bool: bool, #[serde(serialize_with = "custom_serialize")] pub my_struct2: MyStruct2, + #[serde( + serialize_with = "custom_serialize", + skip_serializing_if = "is_default" + )] + pub my_struct2_default_skipped: MyStruct2, } -#[derive(Default, Deserialize, Serialize, JsonSchema, Debug)] +#[derive(Default, Deserialize, Serialize, JsonSchema, Debug, PartialEq)] #[serde(default = "ten_and_true")] pub struct MyStruct2 { #[serde(default = "six")] diff --git a/schemars/tests/expected/default.json b/schemars/tests/expected/default.json index 35d88f3..635cb63 100644 --- a/schemars/tests/expected/default.json +++ b/schemars/tests/expected/default.json @@ -19,6 +19,9 @@ "$ref": "#/definitions/MyStruct2" } ] + }, + "my_struct2_default_skipped": { + "$ref": "#/definitions/MyStruct2" } }, "definitions": { diff --git a/schemars_derive/src/lib.rs b/schemars_derive/src/lib.rs index c94a74b..7f51732 100644 --- a/schemars_derive/src/lib.rs +++ b/schemars_derive/src/lib.rs @@ -316,7 +316,6 @@ fn schema_for_struct(fields: &[Field], cattrs: Option<&serde_attr::Container>) - read_only: field.attrs.skip_deserializing(), write_only: field.attrs.skip_serializing(), default, - skip_default_if: field.attrs.skip_serializing_if().cloned(), ..SchemaMetadata::from_doc_attrs(&field.original.attrs) }; @@ -367,6 +366,22 @@ fn field_default_expr(field: &Field, container_has_default: bool) -> Option quote!(#path()), }; + let default_expr = match field.attrs.skip_serializing_if() { + Some(skip_if) => { + quote! { + { + let default = #default_expr; + if #skip_if(&default) { + None + } else { + Some(default) + } + } + } + } + None => quote!(Some(#default_expr)), + }; + Some(if let Some(ser_with) = field.attrs.serialize_with() { quote! { { @@ -382,7 +397,7 @@ fn field_default_expr(field: &Field, container_has_default: bool) -> Option, - pub skip_default_if: Option, } impl ToTokens for SchemaMetadata { @@ -41,19 +40,10 @@ impl SchemaMetadata { } pub fn apply_to_schema(&self, schema_expr: TokenStream) -> TokenStream { - let setters = self.make_setters(); - - if setters.is_empty() { - return schema_expr; - } - quote! { { - let mut schema = #schema_expr.into(); - gen.make_extensible(&mut schema); - let mut metadata = schema.metadata(); - #(#setters)* - schemars::schema::Schema::Object(schema) + let schema = #schema_expr; + gen.apply_metadata(schema, #self) } } } @@ -83,19 +73,10 @@ impl SchemaMetadata { }); } - match (&self.default, &self.skip_default_if) { - (Some(default), Some(skip_if)) => setters.push(quote! { - { - let default = #default; - if !#skip_if(&default) { - metadata.default = schemars::_serde_json::value::to_value(default).ok(); - } - } - }), - (Some(default), None) => setters.push(quote! { - metadata.default = schemars::_serde_json::value::to_value(#default).ok(); - }), - _ => {} + if let Some(default) = &self.default { + setters.push(quote! { + metadata.default = #default.and_then(|d| schemars::_serde_json::value::to_value(d).ok()); + }); } setters