From 98ad16288b848e6abb0be5d30a22a663957c36f7 Mon Sep 17 00:00:00 2001 From: "Adam H. Leventhal" Date: Thu, 7 Oct 2021 23:21:05 -0700 Subject: [PATCH] Internally tagged enums don't honor deny_unknown_fields as precisely as they might. flatten doesn't act quite as intended with regard to additional_properties --- schemars/src/flatten.rs | 29 ++++++++++- schemars/tests/enum.rs | 13 +++++ schemars/tests/enum_deny_unknown_fields.rs | 13 +++++ .../tests/expected/enum-internal-duf.json | 12 +++-- schemars/tests/expected/enum-internal.json | 3 -- .../expected/enum-simple-internal-duf.json | 51 +++++++++++++++++++ .../tests/expected/enum-simple-internal.json | 48 +++++++++++++++++ schemars_derive/src/schema_exprs.rs | 18 +++++++ 8 files changed, 178 insertions(+), 9 deletions(-) create mode 100644 schemars/tests/expected/enum-simple-internal-duf.json create mode 100644 schemars/tests/expected/enum-simple-internal.json diff --git a/schemars/src/flatten.rs b/schemars/src/flatten.rs index 646f614..35a734f 100644 --- a/schemars/src/flatten.rs +++ b/schemars/src/flatten.rs @@ -38,6 +38,31 @@ macro_rules! impl_merge { }; } +// For ObjectValidation::additional_properties. +impl Merge for Option> { + fn merge(self, other: Self) -> Self { + match (self.map(|x| *x), other.map(|x| *x)) { + // Perfer permissive schemas. + (Some(Schema::Bool(true)), _) => Some(Box::new(true.into())), + (_, Some(Schema::Bool(true))) => Some(Box::new(true.into())), + (None, _) => None, + (_, None) => None, + + // Merge if we have two non-trivial schemas. + (Some(Schema::Object(s1)), Some(Schema::Object(s2))) => { + Some(Box::new(Schema::Object(s1.merge(s2)))) + } + + // Perfer the more permissive schema. + (Some(s1 @ Schema::Object(_)), Some(Schema::Bool(false))) => Some(Box::new(s1)), + (Some(Schema::Bool(false)), Some(s2 @ Schema::Object(_))) => Some(Box::new(s2)), + + // Default to the null schema. + (Some(Schema::Bool(false)), Some(Schema::Bool(false))) => Some(Box::new(false.into())), + } + } +} + impl_merge!(SchemaObject { merge: extensions instance_type enum_values metadata subschemas number string array object, @@ -76,8 +101,8 @@ impl_merge!(ArrayValidation { }); impl_merge!(ObjectValidation { - merge: required properties pattern_properties, - or: max_properties min_properties additional_properties property_names, + merge: required properties pattern_properties additional_properties, + or: max_properties min_properties property_names, }); impl Merge for Option { diff --git a/schemars/tests/enum.rs b/schemars/tests/enum.rs index ea34d06..48e0d34 100644 --- a/schemars/tests/enum.rs +++ b/schemars/tests/enum.rs @@ -99,3 +99,16 @@ pub enum Adjacent { fn enum_adjacent_tagged() -> TestResult { test_default_generated_schema::("enum-adjacent-tagged") } + +#[derive(Debug, JsonSchema)] +#[schemars(tag = "typeProperty")] +pub enum SimpleInternal { + A, + B, + C, +} + +#[test] +fn enum_simple_internal_tag() -> TestResult { + test_default_generated_schema::("enum-simple-internal") +} diff --git a/schemars/tests/enum_deny_unknown_fields.rs b/schemars/tests/enum_deny_unknown_fields.rs index bf0f400..8720b72 100644 --- a/schemars/tests/enum_deny_unknown_fields.rs +++ b/schemars/tests/enum_deny_unknown_fields.rs @@ -104,3 +104,16 @@ pub enum Adjacent { fn enum_adjacent_tagged() -> TestResult { test_default_generated_schema::("enum-adjacent-tagged-duf") } + +#[derive(Debug, JsonSchema)] +#[schemars(tag = "typeProperty", deny_unknown_fields)] +pub enum SimpleInternal { + A, + B, + C, +} + +#[test] +fn enum_simple_internal_tag() -> TestResult { + test_default_generated_schema::("enum-simple-internal-duf") +} diff --git a/schemars/tests/expected/enum-internal-duf.json b/schemars/tests/expected/enum-internal-duf.json index deb3966..fc36644 100644 --- a/schemars/tests/expected/enum-internal-duf.json +++ b/schemars/tests/expected/enum-internal-duf.json @@ -14,7 +14,8 @@ "UnitOne" ] } - } + }, + "additionalProperties": false }, { "type": "object", @@ -45,7 +46,8 @@ "UnitStructNewType" ] } - } + }, + "additionalProperties": false }, { "type": "object", @@ -106,7 +108,8 @@ "UnitTwo" ] } - } + }, + "additionalProperties": false }, { "type": [ @@ -124,7 +127,8 @@ "WithInt" ] } - } + }, + "additionalProperties": false } ] } \ No newline at end of file diff --git a/schemars/tests/expected/enum-internal.json b/schemars/tests/expected/enum-internal.json index b43b779..37739b0 100644 --- a/schemars/tests/expected/enum-internal.json +++ b/schemars/tests/expected/enum-internal.json @@ -28,9 +28,6 @@ "StringMap" ] } - }, - "additionalProperties": { - "type": "string" } }, { diff --git a/schemars/tests/expected/enum-simple-internal-duf.json b/schemars/tests/expected/enum-simple-internal-duf.json new file mode 100644 index 0000000..833f7b7 --- /dev/null +++ b/schemars/tests/expected/enum-simple-internal-duf.json @@ -0,0 +1,51 @@ +{ + "$schema": "http://json-schema.org/draft-07/schema#", + "title": "SimpleInternal", + "oneOf": [ + { + "type": "object", + "required": [ + "typeProperty" + ], + "properties": { + "typeProperty": { + "type": "string", + "enum": [ + "A" + ] + } + }, + "additionalProperties": false + }, + { + "type": "object", + "required": [ + "typeProperty" + ], + "properties": { + "typeProperty": { + "type": "string", + "enum": [ + "B" + ] + } + }, + "additionalProperties": false + }, + { + "type": "object", + "required": [ + "typeProperty" + ], + "properties": { + "typeProperty": { + "type": "string", + "enum": [ + "C" + ] + } + }, + "additionalProperties": false + } + ] +} \ No newline at end of file diff --git a/schemars/tests/expected/enum-simple-internal.json b/schemars/tests/expected/enum-simple-internal.json new file mode 100644 index 0000000..50cd62c --- /dev/null +++ b/schemars/tests/expected/enum-simple-internal.json @@ -0,0 +1,48 @@ +{ + "$schema": "http://json-schema.org/draft-07/schema#", + "title": "SimpleInternal", + "oneOf": [ + { + "type": "object", + "required": [ + "typeProperty" + ], + "properties": { + "typeProperty": { + "type": "string", + "enum": [ + "A" + ] + } + } + }, + { + "type": "object", + "required": [ + "typeProperty" + ], + "properties": { + "typeProperty": { + "type": "string", + "enum": [ + "B" + ] + } + } + }, + { + "type": "object", + "required": [ + "typeProperty" + ], + "properties": { + "typeProperty": { + "type": "string", + "enum": [ + "C" + ] + } + } + } + ] +} \ No newline at end of file diff --git a/schemars_derive/src/schema_exprs.rs b/schemars_derive/src/schema_exprs.rs index 4c463dc..c5b1672 100644 --- a/schemars_derive/src/schema_exprs.rs +++ b/schemars_derive/src/schema_exprs.rs @@ -183,6 +183,12 @@ fn expr_for_external_tagged_enum<'a>( required.insert(#name.to_owned()); required }, + // Externally tagged variants must prohibit additional + // properties irrespective of the disposition of + // `deny_unknown_fields`. If additional properties were allowed + // one could easily construct an object that validated against + // multiple variants since here it's the properties rather than + // the values of a property that distingish between variants. additional_properties: Some(Box::new(false.into())), ..Default::default() })), @@ -206,6 +212,13 @@ fn expr_for_internal_tagged_enum<'a>( ) -> TokenStream { let mut unique_names = HashSet::new(); let mut count = 0; + let set_additional_properties = if deny_unknown_fields { + quote! { + additional_properties: Some(Box::new(false.into())), + } + } else { + TokenStream::new() + }; let variant_schemas = variants .map(|variant| { unique_names.insert(variant.name()); @@ -230,6 +243,9 @@ fn expr_for_internal_tagged_enum<'a>( required.insert(#tag_name.to_owned()); required }, + // As we're creating a "wrapper" object, we can honor the + // disposition of deny_unknown_fields. + #set_additional_properties ..Default::default() })), }); @@ -328,6 +344,8 @@ fn expr_for_adjacent_tagged_enum<'a>( #add_content_to_required required }, + // As we're creating a "wrapper" object, we can honor the + // disposition of deny_unknown_fields. #set_additional_properties ..Default::default() })),