Fix some cases of unsatisfiable schemas when flattening enums (#325)

Addresses #164 and #165

schemars/src/_private.rs

@@ -1,4 +1,5 @@
 use crate::_alloc_prelude::*;
+use crate::transform::transform_immediate_subschemas;
 use crate::{JsonSchema, Schema, SchemaGenerator};
 use serde::Serialize;
 use serde_json::{json, map::Entry, Map, Value};
@@ -16,9 +17,26 @@ pub fn json_schema_for_flatten<T: ?Sized + JsonSchema>(
         }
     }
 
+    // Always allow aditional/unevaluated properties, because the outer struct determines
+    // whether it denies unknown fields.
+    allow_unknown_properties(&mut schema);
+
     schema
 }
 
+fn allow_unknown_properties(schema: &mut Schema) {
+    if let Some(obj) = schema.as_object_mut() {
+        if obj.get("additionalProperties").and_then(Value::as_bool) == Some(false) {
+            obj.remove("additionalProperties");
+        }
+        if obj.get("unevaluatedProperties").and_then(Value::as_bool) == Some(false) {
+            obj.remove("unevaluatedProperties");
+        }
+
+        transform_immediate_subschemas(&mut allow_unknown_properties, schema);
+    }
+}
+
 /// Hack to simulate specialization:
 /// `MaybeSerializeWrapper(x).maybe_to_value()` will resolve to either
 /// - The inherent method `MaybeSerializeWrapper::maybe_to_value(...)` if x is `Serialize`
@@ -182,16 +200,9 @@ pub fn apply_inner_validation(schema: &mut Schema, f: fn(&mut Schema) -> ()) {
 pub fn flatten(schema: &mut Schema, other: Schema) {
     fn flatten_property(obj1: &mut Map<String, Value>, key: String, value2: Value) {
         match obj1.entry(key) {
-            Entry::Vacant(vacant) => match vacant.key().as_str() {
+            Entry::Vacant(vacant) => {
-                "additionalProperties" | "unevaluatedProperties" => {
+                vacant.insert(value2);
-                    if value2 != Value::Bool(false) {
+            }
-                        vacant.insert(value2);
-                    }
-                }
-                _ => {
-                    vacant.insert(value2);
-                }
-            },
             Entry::Occupied(occupied) => {
                 match occupied.key().as_str() {
                     "required" | "allOf" => {
@@ -208,13 +219,6 @@ pub fn flatten(schema: &mut Schema, other: Schema) {
                             }
                         }
                     }
-                    "additionalProperties" | "unevaluatedProperties" => {
-                        // Even if an outer type has `deny_unknown_fields`, unknown fields
-                        // may be accepted by the flattened type
-                        if occupied.get() == &Value::Bool(false) {
-                            *occupied.into_mut() = value2;
-                        }
-                    }
                     "oneOf" | "anyOf" => {
                         // `OccupiedEntry` currently has no `.remove_entry()` method :(
                         let key = occupied.key().clone();
@@ -239,16 +243,49 @@ pub fn flatten(schema: &mut Schema, other: Schema) {
     match other.try_to_object() {
         Err(false) => {}
         Err(true) => {
-            schema
+            if let Some(obj) = schema.as_object_mut() {
-                .ensure_object()
+                if !obj.contains_key("additionalProperties")
-                .insert("additionalProperties".to_owned(), true.into());
+                    && !obj.contains_key("unevaluatedProperties")
+                {
+                    let key = if contains_immediate_subschema(obj) {
+                        "unevaluatedProperties"
+                    } else {
+                        "additionalProperties"
+                    };
+                    obj.insert(key.to_owned(), true.into());
+                }
+            }
         }
-        Ok(obj2) => {
+        Ok(mut obj2) => {
             let obj1 = schema.ensure_object();
 
+            // For complex merges, replace `additionalProperties` with `unevaluatedProperties`
+            // which usually "works out better".
+            normalise_additional_unevaluated_properties(obj1, &obj2);
+            normalise_additional_unevaluated_properties(&mut obj2, obj1);
+
             for (key, value2) in obj2 {
                 flatten_property(obj1, key, value2);
             }
         }
     }
 }
+
+fn normalise_additional_unevaluated_properties(
+    schema_obj1: &mut Map<String, Value>,
+    schema_obj2: &Map<String, Value>,
+) {
+    if schema_obj1.contains_key("additionalProperties")
+        && (schema_obj2.contains_key("unevaluatedProperties")
+            || contains_immediate_subschema(schema_obj2))
+    {
+        let ap = schema_obj1.remove("additionalProperties");
+        schema_obj1.insert("unevaluatedProperties".to_owned(), ap.into());
+    }
+}
+
+fn contains_immediate_subschema(schema_obj: &Map<String, Value>) -> bool {
+    ["if", "then", "else", "allOf", "anyOf", "oneOf", "$ref"]
+        .into_iter()
+        .any(|k| schema_obj.contains_key(k))
+}

schemars/src/generate.rs

@@ -71,7 +71,11 @@ impl SchemaSettings {
             option_add_null_type: true,
             definitions_path: "/definitions".to_owned(),
             meta_schema: Some("http://json-schema.org/draft-07/schema#".to_owned()),
-            transforms: vec![Box::new(RemoveRefSiblings), Box::new(ReplacePrefixItems)],
+            transforms: vec![
+                Box::new(ReplaceUnevaluatedProperties),
+                Box::new(RemoveRefSiblings),
+                Box::new(ReplacePrefixItems),
+            ],
             inline_subschemas: false,
         }
     }
@@ -111,6 +115,7 @@ impl SchemaSettings {
                     .to_owned(),
             ),
             transforms: vec![
+                Box::new(ReplaceUnevaluatedProperties),
                 Box::new(RemoveRefSiblings),
                 Box::new(ReplaceBoolSchemas {
                     skip_additional_properties: true,

schemars/src/json_schema_impls/maps.rs

@@ -1,6 +1,5 @@
 use crate::_alloc_prelude::*;
-use crate::SchemaGenerator;
+use crate::{json_schema, JsonSchema, Schema, SchemaGenerator};
-use crate::{json_schema, JsonSchema, Schema};
 use alloc::borrow::Cow;
 
 macro_rules! map_impl {

schemars/src/transform.rs

@@ -114,7 +114,8 @@ assert_eq!(
 */
 use crate::Schema;
 use crate::_alloc_prelude::*;
-use serde_json::{json, Value};
+use alloc::collections::BTreeSet;
+use serde_json::{json, Map, Value};
 
 /// Trait used to modify a constructed schema and optionally its subschemas.
 ///
@@ -144,58 +145,83 @@ where
 
 /// Applies the given [`Transform`] to all direct subschemas of the [`Schema`].
 pub fn transform_subschemas<T: Transform + ?Sized>(t: &mut T, schema: &mut Schema) {
-    if let Some(obj) = schema.as_object_mut() {
+    for (key, value) in schema.as_object_mut().into_iter().flatten() {
-        for (key, value) in obj {
+        // This is intentionally written to work with multiple JSON Schema versions, so that
-            // This is intentionally written to work with multiple JSON Schema versions, so that
+        // users can add their own transforms on the end of e.g. `SchemaSettings::draft07()` and
-            // users can add their own transforms on the end of e.g. `SchemaSettings::draft07()` and
+        // they will still apply to all subschemas "as expected".
-            // they will still apply to all subschemas "as expected".
+        // This is why this match statement contains both `additionalProperties` (which was
-            // This is why this match statement contains both `additionalProperties` (which was
+        // dropped in draft 2020-12) and `prefixItems` (which was added in draft 2020-12).
-            // dropped in draft 2020-12) and `prefixItems` (which was added in draft 2020-12).
+        match key.as_str() {
-            match key.as_str() {
+            "not"
-                "not"
+            | "if"
-                | "if"
+            | "then"
-                | "then"
+            | "else"
-                | "else"
+            | "contains"
-                | "contains"
+            | "additionalProperties"
-                | "additionalProperties"
+            | "propertyNames"
-                | "propertyNames"
+            | "additionalItems" => {
-                | "additionalItems" => {
+                if let Ok(subschema) = value.try_into() {
-                    if let Ok(subschema) = value.try_into() {
+                    t.transform(subschema);
-                        t.transform(subschema);
-                    }
                 }
-                "allOf" | "anyOf" | "oneOf" | "prefixItems" => {
-                    if let Some(array) = value.as_array_mut() {
-                        for value in array {
-                            if let Ok(subschema) = value.try_into() {
-                                t.transform(subschema);
-                            }
-                        }
-                    }
-                }
-                // Support `items` array even though this is not allowed in draft 2020-12 (see above comment)
-                "items" => {
-                    if let Some(array) = value.as_array_mut() {
-                        for value in array {
-                            if let Ok(subschema) = value.try_into() {
-                                t.transform(subschema);
-                            }
-                        }
-                    } else if let Ok(subschema) = value.try_into() {
-                        t.transform(subschema);
-                    }
-                }
-                "properties" | "patternProperties" | "$defs" | "definitions" => {
-                    if let Some(obj) = value.as_object_mut() {
-                        for value in obj.values_mut() {
-                            if let Ok(subschema) = value.try_into() {
-                                t.transform(subschema);
-                            }
-                        }
-                    }
-                }
-                _ => {}
             }
+            "allOf" | "anyOf" | "oneOf" | "prefixItems" => {
+                if let Some(array) = value.as_array_mut() {
+                    for value in array {
+                        if let Ok(subschema) = value.try_into() {
+                            t.transform(subschema);
+                        }
+                    }
+                }
+            }
+            // Support `items` array even though this is not allowed in draft 2020-12 (see above comment)
+            "items" => {
+                if let Some(array) = value.as_array_mut() {
+                    for value in array {
+                        if let Ok(subschema) = value.try_into() {
+                            t.transform(subschema);
+                        }
+                    }
+                } else if let Ok(subschema) = value.try_into() {
+                    t.transform(subschema);
+                }
+            }
+            "properties" | "patternProperties" | "$defs" | "definitions" => {
+                if let Some(obj) = value.as_object_mut() {
+                    for value in obj.values_mut() {
+                        if let Ok(subschema) = value.try_into() {
+                            t.transform(subschema);
+                        }
+                    }
+                }
+            }
+            _ => {}
+        }
+    }
+}
+
+// Similar to `transform_subschemas`, but only transforms subschemas that apply to the top-level
+// object, e.g. "oneOf" but not "properties".
+pub(crate) fn transform_immediate_subschemas<T: Transform + ?Sized>(
+    t: &mut T,
+    schema: &mut Schema,
+) {
+    for (key, value) in schema.as_object_mut().into_iter().flatten() {
+        match key.as_str() {
+            "if" | "then" | "else" => {
+                if let Ok(subschema) = value.try_into() {
+                    t.transform(subschema);
+                }
+            }
+            "allOf" | "anyOf" | "oneOf" => {
+                if let Some(array) = value.as_array_mut() {
+                    for value in array {
+                        if let Ok(subschema) = value.try_into() {
+                            t.transform(subschema);
+                        }
+                    }
+                }
+            }
+            _ => {}
         }
     }
 }
@@ -369,3 +395,61 @@ impl Transform for ReplacePrefixItems {
         }
     }
 }
+
+#[derive(Debug, Clone)]
+pub struct ReplaceUnevaluatedProperties;
+
+impl Transform for ReplaceUnevaluatedProperties {
+    fn transform(&mut self, schema: &mut Schema) {
+        transform_subschemas(self, schema);
+
+        if let Some(obj) = schema.as_object_mut() {
+            if let Some(up) = obj.remove("unevaluatedProperties") {
+                obj.insert("additionalProperties".to_owned(), up);
+            } else {
+                return;
+            }
+        } else {
+            return;
+        }
+
+        let mut gather_property_names = GatherPropertyNames::default();
+        gather_property_names.transform(schema);
+        let property_names = gather_property_names.0;
+
+        if property_names.is_empty() {
+            return;
+        }
+
+        if let Some(properties) = schema
+            .ensure_object()
+            .entry("properties")
+            .or_insert(Map::new().into())
+            .as_object_mut()
+        {
+            for name in property_names {
+                properties.entry(name).or_insert(true.into());
+            }
+        }
+    }
+}
+
+// Helper for getting property names for all *immediate* subschemas
+#[derive(Default)]
+struct GatherPropertyNames(BTreeSet<String>);
+
+impl Transform for GatherPropertyNames {
+    fn transform(&mut self, schema: &mut Schema) {
+        self.0.extend(
+            schema
+                .as_object()
+                .iter()
+                .filter_map(|o| o.get("properties"))
+                .filter_map(Value::as_object)
+                .flat_map(Map::keys)
+                .cloned(),
+        );
+
+        transform_immediate_subschemas(self, schema);
+    }
+}

schemars/tests/enum_flatten.rs

@@ -1,10 +1,9 @@
 mod util;
-use schemars::JsonSchema;
+use schemars::{generate::SchemaSettings, JsonSchema};
 use util::*;
 
 #[allow(dead_code)]
 #[derive(JsonSchema)]
-#[schemars(rename = "Flat")]
 struct Flat {
     f: f32,
     #[schemars(flatten)]
@@ -58,3 +57,33 @@ enum Enum5 {
 fn test_flat_schema() -> TestResult {
     test_default_generated_schema::<Flat>("enum_flatten")
 }
+
+#[allow(dead_code)]
+#[derive(JsonSchema)]
+#[schemars(deny_unknown_fields)]
+struct FlatDenyUnknownFields {
+    f: f32,
+    #[schemars(flatten)]
+    e1: Enum1,
+    #[schemars(flatten)]
+    e2: Enum2,
+    #[schemars(flatten)]
+    e3: Enum3,
+    #[schemars(flatten)]
+    e4: Enum4,
+    #[schemars(flatten)]
+    e5: Enum5,
+}
+
+#[test]
+fn test_flat_schema_duf() -> TestResult {
+    test_default_generated_schema::<FlatDenyUnknownFields>("enum_flatten_duf")
+}
+
+#[test]
+fn test_flat_schema_duf_draft07() -> TestResult {
+    test_generated_schema::<FlatDenyUnknownFields>(
+        "enum_flatten_duf_draft07",
+        SchemaSettings::draft07(),
+    )
+}

schemars/tests/expected/enum_flatten.json

@@ -23,8 +23,7 @@
           },
           "required": [
             "B"
-          ],
+          ]
-          "additionalProperties": false
         },
         {
           "type": "object",
@@ -35,8 +34,7 @@
           },
           "required": [
             "S"
-          ],
+          ]
-          "additionalProperties": false
         }
       ]
     },
@@ -53,8 +51,7 @@
           },
           "required": [
             "U"
-          ],
+          ]
-          "additionalProperties": false
         },
         {
           "type": "object",
@@ -66,8 +63,7 @@
           },
           "required": [
             "F"
-          ],
+          ]
-          "additionalProperties": false
         }
       ]
     },
@@ -82,8 +78,7 @@
           },
           "required": [
             "B2"
-          ],
+          ]
-          "additionalProperties": false
         },
         {
           "type": "object",
@@ -94,8 +89,7 @@
           },
           "required": [
             "S2"
-          ],
+          ]
-          "additionalProperties": false
         }
       ]
     },
@@ -112,8 +106,7 @@
           },
           "required": [
             "U2"
-          ],
+          ]
-          "additionalProperties": false
         },
         {
           "type": "object",
@@ -125,8 +118,7 @@
           },
           "required": [
             "F2"
-          ],
+          ]
-          "additionalProperties": false
         }
       ]
     }
@@ -141,8 +133,7 @@
       },
       "required": [
         "B3"
-      ],
+      ]
-      "additionalProperties": false
     },
     {
       "type": "object",
@@ -153,8 +144,7 @@
       },
       "required": [
         "S3"
-      ],
+      ]
-      "additionalProperties": false
     }
   ]
 }

schemars/tests/expected/enum_flatten_duf.json

@@ -0,0 +1,151 @@
+{
+  "$schema": "https://json-schema.org/draft/2020-12/schema",
+  "title": "FlatDenyUnknownFields",
+  "type": "object",
+  "properties": {
+    "f": {
+      "type": "number",
+      "format": "float"
+    }
+  },
+  "required": [
+    "f"
+  ],
+  "unevaluatedProperties": false,
+  "allOf": [
+    {
+      "oneOf": [
+        {
+          "type": "object",
+          "properties": {
+            "B": {
+              "type": "boolean"
+            }
+          },
+          "required": [
+            "B"
+          ]
+        },
+        {
+          "type": "object",
+          "properties": {
+            "S": {
+              "type": "string"
+            }
+          },
+          "required": [
+            "S"
+          ]
+        }
+      ]
+    },
+    {
+      "oneOf": [
+        {
+          "type": "object",
+          "properties": {
+            "U": {
+              "type": "integer",
+              "format": "uint32",
+              "minimum": 0
+            }
+          },
+          "required": [
+            "U"
+          ]
+        },
+        {
+          "type": "object",
+          "properties": {
+            "F": {
+              "type": "number",
+              "format": "double"
+            }
+          },
+          "required": [
+            "F"
+          ]
+        }
+      ]
+    },
+    {
+      "oneOf": [
+        {
+          "type": "object",
+          "properties": {
+            "B2": {
+              "type": "boolean"
+            }
+          },
+          "required": [
+            "B2"
+          ]
+        },
+        {
+          "type": "object",
+          "properties": {
+            "S2": {
+              "type": "string"
+            }
+          },
+          "required": [
+            "S2"
+          ]
+        }
+      ]
+    },
+    {
+      "oneOf": [
+        {
+          "type": "object",
+          "properties": {
+            "U2": {
+              "type": "integer",
+              "format": "uint32",
+              "minimum": 0
+            }
+          },
+          "required": [
+            "U2"
+          ]
+        },
+        {
+          "type": "object",
+          "properties": {
+            "F2": {
+              "type": "number",
+              "format": "double"
+            }
+          },
+          "required": [
+            "F2"
+          ]
+        }
+      ]
+    }
+  ],
+  "oneOf": [
+    {
+      "type": "object",
+      "properties": {
+        "B3": {
+          "type": "boolean"
+        }
+      },
+      "required": [
+        "B3"
+      ]
+    },
+    {
+      "type": "object",
+      "properties": {
+        "S3": {
+          "type": "string"
+        }
+      },
+      "required": [
+        "S3"
+      ]
+    }
+  ]
+}

schemars/tests/expected/enum_flatten_duf_draft07.json

@@ -0,0 +1,161 @@
+{
+  "$schema": "http://json-schema.org/draft-07/schema#",
+  "title": "FlatDenyUnknownFields",
+  "type": "object",
+  "properties": {
+    "f": {
+      "type": "number",
+      "format": "float"
+    },
+    "B": true,
+    "B2": true,
+    "B3": true,
+    "F": true,
+    "F2": true,
+    "S": true,
+    "S2": true,
+    "S3": true,
+    "U": true,
+    "U2": true
+  },
+  "required": [
+    "f"
+  ],
+  "allOf": [
+    {
+      "oneOf": [
+        {
+          "type": "object",
+          "properties": {
+            "B": {
+              "type": "boolean"
+            }
+          },
+          "required": [
+            "B"
+          ]
+        },
+        {
+          "type": "object",
+          "properties": {
+            "S": {
+              "type": "string"
+            }
+          },
+          "required": [
+            "S"
+          ]
+        }
+      ]
+    },
+    {
+      "oneOf": [
+        {
+          "type": "object",
+          "properties": {
+            "U": {
+              "type": "integer",
+              "format": "uint32",
+              "minimum": 0
+            }
+          },
+          "required": [
+            "U"
+          ]
+        },
+        {
+          "type": "object",
+          "properties": {
+            "F": {
+              "type": "number",
+              "format": "double"
+            }
+          },
+          "required": [
+            "F"
+          ]
+        }
+      ]
+    },
+    {
+      "oneOf": [
+        {
+          "type": "object",
+          "properties": {
+            "B2": {
+              "type": "boolean"
+            }
+          },
+          "required": [
+            "B2"
+          ]
+        },
+        {
+          "type": "object",
+          "properties": {
+            "S2": {
+              "type": "string"
+            }
+          },
+          "required": [
+            "S2"
+          ]
+        }
+      ]
+    },
+    {
+      "oneOf": [
+        {
+          "type": "object",
+          "properties": {
+            "U2": {
+              "type": "integer",
+              "format": "uint32",
+              "minimum": 0
+            }
+          },
+          "required": [
+            "U2"
+          ]
+        },
+        {
+          "type": "object",
+          "properties": {
+            "F2": {
+              "type": "number",
+              "format": "double"
+            }
+          },
+          "required": [
+            "F2"
+          ]
+        }
+      ]
+    }
+  ],
+  "oneOf": [
+    {
+      "type": "object",
+      "properties": {
+        "B3": {
+          "type": "boolean"
+        }
+      },
+      "required": [
+        "B3"
+      ]
+    },
+    {
+      "type": "object",
+      "properties": {
+        "S3": {
+          "type": "string"
+        }
+      },
+      "required": [
+        "S3"
+      ]
+    }
+  ],
+  "additionalProperties": false
+}

schemars/tests/flatten.rs

@@ -76,24 +76,6 @@ struct FlattenMap {
     value: BTreeMap<String, Value>,
 }
 
-#[allow(dead_code)]
-#[derive(JsonSchema)]
-#[schemars(rename = "FlattenValue", deny_unknown_fields)]
-struct FlattenValueDenyUnknownFields {
-    flag: bool,
-    #[serde(flatten)]
-    value: Value,
-}
-
-#[allow(dead_code)]
-#[derive(JsonSchema)]
-#[schemars(rename = "FlattenValue", deny_unknown_fields)]
-struct FlattenMapDenyUnknownFields {
-    flag: bool,
-    #[serde(flatten)]
-    value: BTreeMap<String, Value>,
-}
-
 #[test]
 fn test_flattened_value() -> TestResult {
     test_default_generated_schema::<FlattenValue>("flattened_value")
@@ -105,18 +87,6 @@ fn test_flattened_map() -> TestResult {
     test_default_generated_schema::<FlattenMap>("flattened_value")
 }
 
-#[test]
-fn test_flattened_value_deny_unknown_fields() -> TestResult {
-    // intentionally using the same file as test_flattened_value, as the schema should be identical
-    test_default_generated_schema::<FlattenValueDenyUnknownFields>("flattened_value")
-}
-
-#[test]
-fn test_flattened_map_deny_unknown_fields() -> TestResult {
-    // intentionally using the same file as test_flattened_value, as the schema should be identical
-    test_default_generated_schema::<FlattenMapDenyUnknownFields>("flattened_value")
-}
-
 #[derive(JsonSchema)]
 pub struct OuterAllowUnknownFields {
     pub outer_field: bool,