Repository: oxidecomputer/typify (rust)

Issue:
Assertion failure in convert_all_of
The following openapi fails in progenitor 0.4.0 (typify 0.0.14) and `main`

```yaml
---
openapi: 3.0.3
info:
  title: progenitor-0.4.0-breakage
  version: 0.4.0
paths: {}
components:
  schemas:
    Action:
      type: string
      enum:
        - foo
    Notification:
      allOf:
        - $ref: "#/components/schemas/Action"
        - type: object
          properties:
            action:
              $ref: "#/components/schemas/NotificationAction"
    NotificationAction:
      type: string
      enum:
        - bar
```
The error is in typify
```
typify-impl/src/convert.rs:1140:9:
  assertion `left != right` failed
    left: Bool(false)
   right: Bool(false)
```

In `convert_all_of`, this is failing when `type_name` is `Required("Notification")`

Pull request:
unsatisfiable `allOf` construction panics
If the merged result of an `allOf` is unsatisfiable (i.e. false) we should produce a Never-like type.

Closes #443 

Patch:
diff --git a/typify-impl/src/convert.rs b/typify-impl/src/convert.rs
index 1e76429..a4220b7 100644
--- a/typify-impl/src/convert.rs
+++ b/typify-impl/src/convert.rs
@@ -1093,22 +1093,34 @@ impl TypeSpace {
         }
 
         // In the general case, we merge all schemas in the array. The merged
-        // schema will reflect all definitions and constraints. For example, it
-        // will have the union of all properties for an object, recursively
-        // merging properties defined in multiple schemas; it will have the
-        // union of all required object properties; and it will enforce the
-        // greater of all numeric constraints (e.g. the greater of all
-        // specified minimum values).
+        // schema will reflect all definitions and constraints, the
+        // intersection of all schemas. For example, it will have the union of
+        // all properties for an object, recursively merging properties defined
+        // in multiple schemas; it will have the union of all required object
+        // properties; and it will enforce the greater of all numeric
+        // constraints (e.g. the greater of all specified minimum values).
         //
         // Sometimes merging types will produce a result for which no data is
         // valid, the schema is unsatisfiable. Consider this trivial case:
         //
-        // "allOf": [
+        // {
+        //   "allOf": [
         //     { "type": "integer", "minimum": 5 },
         //     { "type": "integer", "maximum": 3 }
-        // ]
+        //   ]
+        // }
+        //
+        // No number is >= 5 *and* <= 3! Good luck with that schema! Similarly
+        // for this case:
         //
-        // No number is >= 5 *and* <= 3! Good luck with that schema!
+        // {
+        //   "allOf": [
+        //     { "type": "string" },
+        //     { "type": "object" }
+        //   ]
+        // }
+        //
+        // A value cannot be both a string and an object!
         //
         // Note that we will effectively "embed" any referenced types. Consider
         // a construction like this:
@@ -1119,10 +1131,10 @@ impl TypeSpace {
         // ]
         //
         // The resulting merged schema will include all properties of
-        // "SuperClass" as well as "another_prop" (which we assume to not have
-        // been present in the original). This is suboptimal in that we would
-        // like the generated types to reflect some association with the
-        // original sub-type.
+        // "SuperClass" as well as "another_prop" (which we assume for this
+        // example to not have been present in the original). This is
+        // suboptimal in that we would like the generated types to reflect some
+        // association with the original sub-type.
         //
         // TODO
         // In cases where we have a named type, we would like to provide
@@ -1137,13 +1149,16 @@ impl TypeSpace {
         // the more expansive numeric type.
 
         let merged_schema = merge_all(subschemas, &self.definitions);
-        assert_ne!(merged_schema, Schema::Bool(false));
-        let mut merged_schema = merged_schema.into_object();
-        assert!(merged_schema.metadata.is_none());
-        merged_schema.metadata = metadata.clone();
+        if let Schema::Bool(false) = &merged_schema {
+            self.convert_never(type_name)
+        } else {
+            let mut merged_schema = merged_schema.into_object();
+            assert!(merged_schema.metadata.is_none());
+            merged_schema.metadata = metadata.clone();
 
-        let (type_entry, _) = self.convert_schema(type_name, &merged_schema.into())?;
-        Ok((type_entry, &None))
+            let (type_entry, _) = self.convert_schema(type_name, &merged_schema.into())?;
+            Ok((type_entry, &None))
+        }
     }
 
     fn convert_any_of<'a>(
diff --git a/typify-impl/src/merge.rs b/typify-impl/src/merge.rs
index bc49210..9f83fd4 100644
--- a/typify-impl/src/merge.rs
+++ b/typify-impl/src/merge.rs
@@ -876,46 +876,65 @@ fn merge_so_object(
     match (a, b) {
         (None, other) | (other, None) => Ok(other.cloned().map(Box::new)),
         (Some(aa), Some(bb)) => {
-            let a_props = aa.properties.iter().filter_map(|(name, a_schema)| {
-                let resolved_schema = if let Some(b_schema) = bb.properties.get(name) {
-                    try_merge_schema(a_schema, b_schema, defs)
-                } else {
-                    filter_prop(name, a_schema, bb)
-                };
-
-                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(())),
-
-                    // We can ignore incompatible, non-required fields.
-                    Err(()) => None,
-
-                    // Compatible schema; proceed.
-                    Ok(schema) => Some(Ok((name.clone(), schema))),
-                }
-            });
-
-            let b_props = bb.properties.iter().filter_map(|(name, b_schema)| {
-                if aa.properties.contains_key(name) {
-                    // We handled the intersection above.
-                    None
-                } else {
-                    match filter_prop(name, b_schema, aa) {
+            enum AOrB<'a> {
+                A(&'a Schema),
+                B(&'a Schema),
+                Both(&'a Schema, &'a Schema),
+            }
+
+            let properties = aa
+                .properties
+                .iter()
+                // First characterize properties as being in a, b, or both.
+                .map(|(name, a_schema)| {
+                    if let Some(b_schema) = bb.properties.get(name) {
+                        (name, AOrB::Both(a_schema, b_schema))
+                    } else {
+                        (name, AOrB::A(a_schema))
+                    }
+                })
+                .chain(bb.properties.iter().filter_map(|(name, b_schema)| {
+                    if aa.properties.contains_key(name) {
+                        None
+                    } else {
+                        Some((name, AOrB::B(b_schema)))
+                    }
+                }))
+                // Then resolve properties against the other full schema or
+                // against the schemas for the properties if it appears in
+                // both.
+                .filter_map(|(name, ab_schema)| {
+                    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)
+                        }
+                    };
+                    match resolved_schema {
                         // If a required field is incompatible with the
                         // other schema, this object is unsatisfiable.
-                        Err(()) if bb.required.contains(name) => Some(Err(())),
+                        Err(()) if aa.required.contains(name) => Some(Err(())),
 
                         // We can ignore incompatible, non-required fields.
+                        //
+                        // TODO ... however we may need to note that the given
+                        // property is explicitly disallowed. We could do that
+                        // either by *leaving* the property in with an
+                        // unsatisfiable type (i.e. the schema `false`) or by
+                        // adding something to `propertyNames` (which would be
+                        // significantly more complicated).
+                        //
+                        // Note that if `additionalProperties` already disallows
+                        // all unnamed properties we could simply drop this
+                        // property.
                         Err(()) => None,
 
                         // Compatible schema; proceed.
                         Ok(schema) => Some(Ok((name.clone(), schema))),
                     }
-                }
-            });
-
-            let properties = a_props.chain(b_props).collect::<Result<_, ()>>()?;
+                })
+                .collect::<Result<_, ()>>()?;
 
             let required = aa.required.union(&bb.required).cloned().collect();
             let additional_properties = merge_additional_properties(
@@ -925,12 +944,21 @@ fn merge_so_object(
             )
             .map(Box::new);
 
+            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);
+
+            if let (Some(min), Some(max)) = (min_properties, max_properties) {
+                if min > max {
+                    return Err(());
+                }
+            }
+
             let object_validation = ObjectValidation {
                 required,
                 properties,
                 additional_properties,
-                max_properties: Default::default(),     // TODO
-                min_properties: Default::default(),     // TODO
+                max_properties,
+                min_properties,
                 pattern_properties: Default::default(), // TODO
                 property_names: Default::default(),     // TODO
             };


Test patch:
diff --git a/typify/tests/schemas/merged-schemas.json b/typify/tests/schemas/merged-schemas.json
index 2a2d573..94e48d1 100644
--- a/typify/tests/schemas/merged-schemas.json
+++ b/typify/tests/schemas/merged-schemas.json
@@ -208,6 +208,50 @@
           }
         }
       ]
+    },
+    "unsatisfiable-1": {
+      "allOf": [
+        {
+          "type": "string",
+          "enum": [
+            "foo"
+          ]
+        },
+        {
+          "type": "object",
+          "properties": {
+            "bar": {}
+          }
+        }
+      ]
+    },
+    "unsatisfiable-2": {
+      "allOf": [
+        {
+          "type": "object",
+          "properties": {
+            "action": {
+              "type": "string",
+              "enum": [
+                "foo"
+              ]
+            }
+          },
+          "additionalProperties": false
+        },
+        {
+          "type": "object",
+          "properties": {
+            "action": {
+              "type": "string",
+              "enum": [
+                "bar"
+              ]
+            }
+          },
+          "additionalProperties": false
+        }
+      ]
     }
   }
 }
\ No newline at end of file
diff --git a/typify/tests/schemas/merged-schemas.rs b/typify/tests/schemas/merged-schemas.rs
index 29ceec3..88c06a8 100644
--- a/typify/tests/schemas/merged-schemas.rs
+++ b/typify/tests/schemas/merged-schemas.rs
@@ -191,6 +191,22 @@ impl From<&TrimFat> for TrimFat {
         value.clone()
     }
 }
+#[derive(Clone, Copy, Debug, Deserialize, Eq, Hash, Ord, PartialEq, PartialOrd, Serialize)]
+#[serde(deny_unknown_fields)]
+pub enum Unsatisfiable1 {}
+impl From<&Unsatisfiable1> for Unsatisfiable1 {
+    fn from(value: &Unsatisfiable1) -> Self {
+        value.clone()
+    }
+}
+#[derive(Clone, Debug, Deserialize, Serialize)]
+#[serde(deny_unknown_fields)]
+pub struct Unsatisfiable2 {}
+impl From<&Unsatisfiable2> for Unsatisfiable2 {
+    fn from(value: &Unsatisfiable2) -> Self {
+        value.clone()
+    }
+}
 #[derive(Clone, Debug, Deserialize, Serialize)]
 #[serde(untagged)]
 pub enum WeirdEnum {