Improve the handling of conflicting properties while merging schemas (#549)

Author: ahl

typify-impl/src/merge.rs

@@ -1,4 +1,4 @@
-// Copyright 2023 Oxide Computer Company
+// Copyright 2024 Oxide Computer Company
 
 use std::{
     collections::{BTreeMap, BTreeSet},
@@ -25,17 +25,19 @@ fn try_merge_all(schemas: &[Schema], defs: &BTreeMap<RefKey, Schema>) -> Result<
         serde_json::to_string_pretty(schemas).unwrap(),
     );
 
-    match schemas {
+    let merged_schema = match schemas {
         [] => panic!("we should not be trying to merge an empty array of schemas"),
-        [only] => Ok(only.clone()),
+        [only] => only.clone(),
         [first, second, rest @ ..] => {
             let mut out = try_merge_schema(first, second, defs)?;
             for schema in rest {
                 out = try_merge_schema(&out, schema, defs)?;
             }
-            Ok(out)
+            out
         }
-    }
+    };
+
+    Ok(merged_schema)
 }
 
 /// Given two additionalItems schemas that might be None--which is equivalent
@@ -71,12 +73,16 @@ fn merge_additional_properties(
     }
 }
 
+fn merge_schema(a: &Schema, b: &Schema, defs: &BTreeMap<RefKey, Schema>) -> Schema {
+    try_merge_schema(a, b, defs).unwrap_or(Schema::Bool(false))
+}
+
 /// Merge two schemas returning the resulting schema. If the two schemas are
 /// incompatible (i.e. if there is no data that can satisfy them both
 /// simultaneously) then this returns Err.
 fn try_merge_schema(a: &Schema, b: &Schema, defs: &BTreeMap<RefKey, Schema>) -> Result<Schema, ()> {
     match (a, b) {
-        (Schema::Bool(false), _) | (_, Schema::Bool(false)) => Ok(Schema::Bool(false)),
+        (Schema::Bool(false), _) | (_, Schema::Bool(false)) => Err(()),
         (Schema::Bool(true), other) | (other, Schema::Bool(true)) => Ok(other.clone()),
 
         // If we have two references to the same schema, that's easy!
@@ -489,9 +495,11 @@ fn try_merge_schema_not(
                         if required.contains(not_required) {
                             return Err(());
                         }
-                        // We don't care if a property we're saying is not
-                        // required was not present in the properties map.
-                        let _ = properties.remove(not_required);
+                        // Set the property's schema to false i.e. that the
+                        // presence of any value would be invalid. We ignore
+                        // the return value as it doesn't matter if the
+                        // property was there previously or not.
+                        let _ = properties.insert(not_required.clone(), Schema::Bool(false));
                     }
                 }
             }
@@ -947,6 +955,11 @@ fn merge_so_object(
     match (a, b) {
         (None, other) | (other, None) => Ok(other.cloned().map(Box::new)),
         (Some(aa), Some(bb)) => {
+            let required = aa
+                .required
+                .union(&bb.required)
+                .cloned()
+                .collect::<BTreeSet<_>>();
             let additional_properties = merge_additional_properties(
                 aa.additional_properties.as_deref(),
                 bb.additional_properties.as_deref(),
@@ -984,29 +997,29 @@ fn merge_so_object(
                     let resolved_schema = match ab_schema {
                         AOrB::A(a_schema) => filter_prop(name, a_schema, bb),
                         AOrB::B(b_schema) => filter_prop(name, b_schema, aa),
-                        AOrB::Both(a_schema, b_schema) => {
-                            try_merge_schema(a_schema, b_schema, defs)
-                        }
+                        AOrB::Both(a_schema, b_schema) => merge_schema(a_schema, b_schema, defs),
                     };
                     match resolved_schema {
                         // If a required field is incompatible with the
                         // other schema, this object is unsatisfiable.
-                        Err(()) if aa.required.contains(name) => Some(Err(())),
+                        Schema::Bool(false) if required.contains(name) => Some(Err(())),
 
                         // For incompatible, non-required fields we need to
                         // exclude the property from any values. If
                         // `additionalProperties` is `false` (i.e. excludes all
                         // other properties) then we can simply omit the
-                        // property. Otherwise we include the optional property
-                        // but with the `false` schema that means that no value
-                        // will satisfy that property--the value would always
-                        // be None and any serialization that included the
-                        // named property would fail to deserialize.
+                        // property knowing that it (like all other unnamed
+                        // properties) will not be permitted. Otherwise we
+                        // include the optional property but with the `false`
+                        // schema that means that no value will satisfy that
+                        // property--the value would always be None and any
+                        // serialization that included the named property would
+                        // fail to deserialize.
                         //
                         // If we ever make use of `propertyNames`, it's
                         // conceivable that we might check it or modify it in
                         // this case, but that may be overly complex.
-                        Err(()) => {
+                        Schema::Bool(false) => {
                             if let Some(Schema::Bool(false)) = additional_properties {
                                 None
                             } else {
@@ -1015,25 +1028,11 @@ fn merge_so_object(
                         }
 
                         // Compatible schema; proceed.
-                        Ok(schema) => Some(Ok((name.clone(), schema))),
+                        schema => Some(Ok((name.clone(), schema))),
                     }
                 })
                 .collect::<Result<schemars::Map<_, _>, _>>()?;
 
-            let additional_properties = additional_properties.map(Box::new);
-            let required = aa.required.union(&bb.required).cloned().collect();
-
-            // So. We merged the properties and we merged the array of
-            // properties that are required. We use the absence of a property
-            // in the properties map to indicate that the property must *not*
-            // be present. This is imprecise, but allows us to make progress
-            // without a most significant conversion to a new representation.
-            for req_prop in &required {
-                if !properties.contains_key(req_prop) {
-                    return Err(());
-                }
-            }
-
             let max_properties = choose_value(aa.max_properties, bb.max_properties, Ord::min);
             let min_properties = choose_value(aa.min_properties, bb.min_properties, Ord::max);
 
@@ -1046,7 +1045,7 @@ fn merge_so_object(
             let object_validation = ObjectValidation {
                 required,
                 properties,
-                additional_properties,
+                additional_properties: additional_properties.map(Box::new),
                 max_properties,
                 min_properties,
                 pattern_properties: Default::default(), // TODO
@@ -1057,11 +1056,7 @@ fn merge_so_object(
     }
 }
 
-fn filter_prop(
-    name: &str,
-    prop_schema: &Schema,
-    object_schema: &ObjectValidation,
-) -> Result<Schema, ()> {
+fn filter_prop(name: &str, prop_schema: &Schema, object_schema: &ObjectValidation) -> Schema {
     // We're only considering properties we *know* do not appear in the other
     // object's schema.
     assert!(!object_schema.properties.contains_key(name));
@@ -1077,6 +1072,7 @@ fn filter_prop(
     assert!(object_schema.pattern_properties.is_empty());
 
     merge_additional(object_schema.additional_properties.as_deref(), prop_schema)
+        .unwrap_or(Schema::Bool(false))
 }
 
 fn merge_additional(additional: Option<&Schema>, prop_schema: &Schema) -> Result<Schema, ()> {

typify-impl/src/structs.rs

@@ -1,4 +1,4 @@
-// Copyright 2022 Oxide Computer Company
+// Copyright 2024 Oxide Computer Company
 
 use heck::ToSnakeCase;
 use proc_macro2::TokenStream;
@@ -27,16 +27,49 @@ impl TypeSpace {
         //assert!(validation.pattern_properties.is_empty());
         //assert!(validation.property_names.is_none());
 
+        // Gather up the properties that are required but for which we have no
+        // schema. In those cases any value will do.
+        let required_unspecified = validation.required.iter().filter_map(|prop_name| {
+            validation
+                .properties
+                .get(prop_name)
+                .is_none()
+                .then_some((prop_name, &Schema::Bool(true)))
+        });
+
         let mut properties = validation
             .properties
             .iter()
-            .map(|(name, ty)| {
-                // Generate a name we can use for the type of this property
-                // should there not be a one defined in the schema.
-                let sub_type_name = type_name
-                    .as_ref()
-                    .map(|base| format!("{}_{}", base, name.to_snake_case()));
-                self.struct_property(sub_type_name, &validation.required, name, ty)
+            .chain(required_unspecified)
+            .filter_map(|(prop_name, schema)| {
+                match schema {
+                    // TODO We use the schema `false` to indicate an
+                    // unsatisfiable schema. We take a shortcut here and simply
+                    // ignore these. This is wrong in two subtle and important
+                    // ways that we'll need to address at some point. First,
+                    // there are other schemas in some non-trivial,
+                    // non-canonical form that might indicate the same thing.
+                    // We should handle those in the same way. In addition,
+                    // ignoring them isn't really right. We need to actively
+                    // exclude them. Specifically this would look like a custom
+                    // serde::Deserialize implementation that failed in the
+                    // presence of these values.
+                    Schema::Bool(false) => None,
+                    _ => {
+                        // Generate a name we can use for the type of this
+                        // property should there not be one specified by the
+                        // schema itself (i.e. via the title field).
+                        let sub_type_name = type_name
+                            .as_ref()
+                            .map(|base| format!("{}_{}", base, prop_name.to_snake_case()));
+                        Some(self.struct_property(
+                            sub_type_name,
+                            &validation.required,
+                            prop_name,
+                            schema,
+                        ))
+                    }
+                }
             })
             .collect::<Result<Vec<_>>>()?;
 
@@ -107,7 +140,7 @@ impl TypeSpace {
         Ok((properties, deny_unknown_fields))
     }
 
-    pub(crate) fn struct_property(
+    fn struct_property(
         &mut self,
         type_name: Option<String>,
         required: &schemars::Set<String>,

typify-impl/tests/test_github.rs

@@ -59,7 +59,13 @@ fn test_vega() {
           }
     };
     let schema = serde_json::from_value(raw_schema).unwrap();
-    settings.with_conversion(schema, "MyEnum", [TypeSpaceImpl::FromStr].into_iter());
+    settings
+        .with_conversion(schema, "MyEnum", [TypeSpaceImpl::FromStr].into_iter())
+        // TODO ColorValue has resulted in an extremely expensive type; to
+        // resolve this we need to do a better job of canonicalizing the schema
+        // once rather than repeatedly doing quadratic expansions over it.
+        // Alternatively we can memoize conversions rather than repeating them.
+        .with_replacement("ColorValue", "ColorValue", [].into_iter());
 
     let mut type_space = TypeSpace::new(&settings);