Repository: oxidecomputer/typify (rust)

Issue:
Regression after typify 0.0.12
This openapi worked in progenitor 0.3.0 (which used typify 0.0.12), but no longer works in progenitor 0.4.0 (which is using typify 0.0.14)

```yaml
openapi: 3.0.3
info:
  title: progenitor-0.4.0-regression
  version: 0.4.0
paths: {}
components:
  schemas:
    Action:
      type: string
      enum:
        - foo
    BaseAction:
      type: object
      properties:
        action:
          allOf:
            - $ref: "#/components/schemas/Action"
    Notification:
      allOf:
        - $ref: "#/components/schemas/BaseAction"
        - type: object
          properties:
            action:
              $ref: "#/components/schemas/NotificationAction"
    NotificationAction:
      type: string
      enum:
        - bar
```

The error is
```
typify-impl/src/convert.rs:662:36:
  not yet implemented: invalid (or unexpected) schema:
  {
    "enum": [
      "bar"
    ],
    "$ref": "#/components/schemas/Action"
  }
```

Pull request:
fix handling of references within allOf subschemas
Closes #444 

Patch:
diff --git a/typify-impl/src/merge.rs b/typify-impl/src/merge.rs
index 9f83fd4..4c7a08d 100644
--- a/typify-impl/src/merge.rs
+++ b/typify-impl/src/merge.rs
@@ -138,6 +138,12 @@ fn merge_schema_object(
     b: &SchemaObject,
     defs: &BTreeMap<RefKey, Schema>,
 ) -> Result<SchemaObject, ()> {
+    debug!(
+        "merging {}\n{}",
+        serde_json::to_string_pretty(a).unwrap(),
+        serde_json::to_string_pretty(b).unwrap(),
+    );
+
     assert!(a.reference.is_none());
     assert!(b.reference.is_none());
 
@@ -149,14 +155,21 @@ fn merge_schema_object(
     let array = merge_so_array(a.array.as_deref(), b.array.as_deref(), defs)?;
     let object = merge_so_object(a.object.as_deref(), b.object.as_deref(), defs)?;
 
+    let enum_values = merge_so_enum_values(
+        a.enum_values.as_ref(),
+        a.const_value.as_ref(),
+        b.enum_values.as_ref(),
+        b.const_value.as_ref(),
+    )?;
+
     // We could clean up this schema to eliminate data irrelevant to the
     // instance type, but logic in the conversion path should already handle
     // that.
-    let merged_schema = SchemaObject {
+    let mut merged_schema = SchemaObject {
         metadata: None,
         instance_type,
         format,
-        enum_values: None,
+        enum_values,
         const_value: None,
         subschemas: None,
         number,
@@ -174,45 +187,37 @@ fn merge_schema_object(
     // two schemas and then do the appropriate merge with subschemas (i.e.
     // potentially twice). This is effectively an `allOf` between the merged
     // "body" schema and the component subschemas.
-    let merged_schema = try_merge_with_subschemas(merged_schema, a.subschemas.as_deref(), defs)?;
-    let merged_schema = try_merge_with_subschemas(merged_schema, b.subschemas.as_deref(), defs)?;
+    merged_schema = try_merge_with_subschemas(merged_schema, a.subschemas.as_deref(), defs)?;
+    merged_schema = try_merge_with_subschemas(merged_schema, b.subschemas.as_deref(), defs)?;
 
     assert_ne!(merged_schema, Schema::Bool(false).into_object());
 
-    let enum_values = merge_so_enum_values(
-        a.enum_values.as_ref(),
-        a.const_value.as_ref(),
-        b.enum_values.as_ref(),
-        b.const_value.as_ref(),
-    )?;
+    // Now that we've finalized the schemas, we take a pass through the
+    // enumerated values (if there are any) to weed out any that might be
+    // invalid.
+    if let Some(enum_values) = merged_schema.enum_values.take() {
+        let wrapped_schema = Schema::Object(merged_schema);
+        let enum_values = Some(
+            enum_values
+                .into_iter()
+                .filter(|value| schema_value_validate(&wrapped_schema, value, defs).is_ok())
+                .collect(),
+        );
+        let Schema::Object(new_merged_schema) = wrapped_schema else {
+            unreachable!()
+        };
+        merged_schema = new_merged_schema;
+        merged_schema.enum_values = enum_values;
+    }
 
     debug!(
-        "merging {}\n{}\n|\nv\n{}",
+        "merged {}\n{}\n|\nv\n{}",
         serde_json::to_string_pretty(a).unwrap(),
         serde_json::to_string_pretty(b).unwrap(),
         serde_json::to_string_pretty(&merged_schema).unwrap(),
     );
 
-    match enum_values {
-        None => Ok(merged_schema),
-        Some(enum_values) => {
-            let enum_values = enum_values
-                .into_iter()
-                .filter(|value| {
-                    schema_value_validate(&Schema::Object(merged_schema.clone()), value, defs)
-                        .is_ok()
-                })
-                .collect::<Vec<_>>();
-            if enum_values.is_empty() {
-                Err(())
-            } else {
-                Ok(SchemaObject {
-                    enum_values: Some(enum_values),
-                    ..merged_schema
-                })
-            }
-        }
-    }
+    Ok(merged_schema)
 }
 
 fn merge_so_enum_values(
@@ -876,6 +881,12 @@ fn merge_so_object(
     match (a, b) {
         (None, other) | (other, None) => Ok(other.cloned().map(Box::new)),
         (Some(aa), Some(bb)) => {
+            let additional_properties = merge_additional_properties(
+                aa.additional_properties.as_deref(),
+                bb.additional_properties.as_deref(),
+                defs,
+            );
+
             enum AOrB<'a> {
                 A(&'a Schema),
                 B(&'a Schema),
@@ -916,33 +927,35 @@ fn merge_so_object(
                         // other schema, this object is unsatisfiable.
                         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).
+                        // 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.
                         //
-                        // Note that if `additionalProperties` already disallows
-                        // all unnamed properties we could simply drop this
-                        // property.
-                        Err(()) => None,
+                        // 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(()) => {
+                            if let Some(Schema::Bool(false)) = additional_properties {
+                                None
+                            } else {
+                                Some(Ok((name.clone(), Schema::Bool(false))))
+                            }
+                        }
 
                         // Compatible schema; proceed.
                         Ok(schema) => Some(Ok((name.clone(), schema))),
                     }
                 })
-                .collect::<Result<_, ()>>()?;
+                .collect::<Result<_, _>>()?;
 
+            let additional_properties = additional_properties.map(Box::new);
             let required = aa.required.union(&bb.required).cloned().collect();
-            let additional_properties = merge_additional_properties(
-                aa.additional_properties.as_deref(),
-                bb.additional_properties.as_deref(),
-                defs,
-            )
-            .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);
diff --git a/typify-impl/src/validate.rs b/typify-impl/src/validate.rs
index 9d51365..ea61f7a 100644
--- a/typify-impl/src/validate.rs
+++ b/typify-impl/src/validate.rs
@@ -24,8 +24,23 @@ fn schema_object_value_validate(
     value: &Value,
     _defs: &BTreeMap<RefKey, Schema>,
 ) -> Result<(), String> {
-    assert!(object.enum_values.is_none());
-    assert!(object.const_value.is_none());
+    if let Some(const_value) = &object.const_value {
+        if value != const_value {
+            return Err(format!(
+                "{} does not match the const value {}",
+                value, const_value,
+            ));
+        }
+    }
+    if let Some(enum_values) = &object.enum_values {
+        if !enum_values.contains(value) {
+            return Err(format!(
+                "{} does not match the enum values {}",
+                value,
+                serde_json::to_string(enum_values).unwrap(),
+            ));
+        }
+    }
 
     let SchemaObject { instance_type, .. } = object;
 


Test patch:
diff --git a/typify/tests/schemas/merged-schemas.json b/typify/tests/schemas/merged-schemas.json
index 94e48d1..2a8a67e 100644
--- a/typify/tests/schemas/merged-schemas.json
+++ b/typify/tests/schemas/merged-schemas.json
@@ -252,6 +252,45 @@
           "additionalProperties": false
         }
       ]
+    },
+    "unsatisfiable-3": {
+      "allOf": [
+        {
+          "$ref": "#/definitions/unsatisfiable-3-a"
+        },
+        {
+          "type": "object",
+          "properties": {
+            "action": {
+              "$ref": "#/definitions/unsatisfiable-3-b"
+            }
+          }
+        }
+      ]
+    },
+    "unsatisfiable-3-a": {
+      "type": "object",
+      "properties": {
+        "action": {
+          "allOf": [
+            {
+              "$ref": "#/definitions/unsatisfiable-3-c"
+            }
+          ]
+        }
+      }
+    },
+    "unsatisfiable-3-b": {
+      "type": "string",
+      "enum": [
+        "bar"
+      ]
+    },
+    "unsatisfiable-3-c": {
+      "type": "string",
+      "enum": [
+        "foo"
+      ]
     }
   }
 }
\ No newline at end of file
diff --git a/typify/tests/schemas/merged-schemas.rs b/typify/tests/schemas/merged-schemas.rs
index 88c06a8..3326541 100644
--- a/typify/tests/schemas/merged-schemas.rs
+++ b/typify/tests/schemas/merged-schemas.rs
@@ -208,6 +208,122 @@ impl From<&Unsatisfiable2> for Unsatisfiable2 {
     }
 }
 #[derive(Clone, Debug, Deserialize, Serialize)]
+pub struct Unsatisfiable3 {
+    #[serde(default, skip_serializing_if = "Option::is_none")]
+    pub action: Option<Unsatisfiable3Action>,
+}
+impl From<&Unsatisfiable3> for Unsatisfiable3 {
+    fn from(value: &Unsatisfiable3) -> Self {
+        value.clone()
+    }
+}
+#[derive(Clone, Debug, Deserialize, Serialize)]
+pub struct Unsatisfiable3A {
+    #[serde(default, skip_serializing_if = "Option::is_none")]
+    pub action: Option<Unsatisfiable3C>,
+}
+impl From<&Unsatisfiable3A> for Unsatisfiable3A {
+    fn from(value: &Unsatisfiable3A) -> Self {
+        value.clone()
+    }
+}
+#[derive(Clone, Copy, Debug, Deserialize, Eq, Hash, Ord, PartialEq, PartialOrd, Serialize)]
+#[serde(deny_unknown_fields)]
+pub enum Unsatisfiable3Action {}
+impl From<&Unsatisfiable3Action> for Unsatisfiable3Action {
+    fn from(value: &Unsatisfiable3Action) -> Self {
+        value.clone()
+    }
+}
+#[derive(Clone, Copy, Debug, Deserialize, Eq, Hash, Ord, PartialEq, PartialOrd, Serialize)]
+pub enum Unsatisfiable3B {
+    #[serde(rename = "bar")]
+    Bar,
+}
+impl From<&Unsatisfiable3B> for Unsatisfiable3B {
+    fn from(value: &Unsatisfiable3B) -> Self {
+        value.clone()
+    }
+}
+impl ToString for Unsatisfiable3B {
+    fn to_string(&self) -> String {
+        match *self {
+            Self::Bar => "bar".to_string(),
+        }
+    }
+}
+impl std::str::FromStr for Unsatisfiable3B {
+    type Err = &'static str;
+    fn from_str(value: &str) -> Result<Self, &'static str> {
+        match value {
+            "bar" => Ok(Self::Bar),
+            _ => Err("invalid value"),
+        }
+    }
+}
+impl std::convert::TryFrom<&str> for Unsatisfiable3B {
+    type Error = &'static str;
+    fn try_from(value: &str) -> Result<Self, &'static str> {
+        value.parse()
+    }
+}
+impl std::convert::TryFrom<&String> for Unsatisfiable3B {
+    type Error = &'static str;
+    fn try_from(value: &String) -> Result<Self, &'static str> {
+        value.parse()
+    }
+}
+impl std::convert::TryFrom<String> for Unsatisfiable3B {
+    type Error = &'static str;
+    fn try_from(value: String) -> Result<Self, &'static str> {
+        value.parse()
+    }
+}
+#[derive(Clone, Copy, Debug, Deserialize, Eq, Hash, Ord, PartialEq, PartialOrd, Serialize)]
+pub enum Unsatisfiable3C {
+    #[serde(rename = "foo")]
+    Foo,
+}
+impl From<&Unsatisfiable3C> for Unsatisfiable3C {
+    fn from(value: &Unsatisfiable3C) -> Self {
+        value.clone()
+    }
+}
+impl ToString for Unsatisfiable3C {
+    fn to_string(&self) -> String {
+        match *self {
+            Self::Foo => "foo".to_string(),
+        }
+    }
+}
+impl std::str::FromStr for Unsatisfiable3C {
+    type Err = &'static str;
+    fn from_str(value: &str) -> Result<Self, &'static str> {
+        match value {
+            "foo" => Ok(Self::Foo),
+            _ => Err("invalid value"),
+        }
+    }
+}
+impl std::convert::TryFrom<&str> for Unsatisfiable3C {
+    type Error = &'static str;
+    fn try_from(value: &str) -> Result<Self, &'static str> {
+        value.parse()
+    }
+}
+impl std::convert::TryFrom<&String> for Unsatisfiable3C {
+    type Error = &'static str;
+    fn try_from(value: &String) -> Result<Self, &'static str> {
+        value.parse()
+    }
+}
+impl std::convert::TryFrom<String> for Unsatisfiable3C {
+    type Error = &'static str;
+    fn try_from(value: String) -> Result<Self, &'static str> {
+        value.parse()
+    }
+}
+#[derive(Clone, Debug, Deserialize, Serialize)]
 #[serde(untagged)]
 pub enum WeirdEnum {
     Variant0 {