Handling of transitive allOf/refs is busted (for omicron)

src/schema.rs

@@ -733,28 +733,49 @@ impl Compare {
         Ok(ret)
     }
 
-    // NOTE: Single-element allOf schemas are flattened by `try_compare_flattened`
-    // before reaching this function. Multi-element allOf would require semantic
-    // merging for proper comparison, so we just do an equality check.
     fn compare_schema_type_all_of(
         &mut self,
         comparison: SchemaComparison,
         dry_run: bool,
         old_all_of: Contextual<'_, &Vec<ReferenceOr<Schema>>>,
         new_all_of: Contextual<'_, &Vec<ReferenceOr<Schema>>>,
     ) -> anyhow::Result<bool> {
-        if old_all_of.as_ref() != new_all_of.as_ref() {
-            self.schema_push_change(
+        let old_schemas = old_all_of.as_ref();
+        let new_schemas = new_all_of.as_ref();
+        if old_schemas.len() != new_schemas.len() {
+            return self.schema_push_change(
                 dry_run,
-                "unhandled, 'allOf' schema",
+                "allOf schema count changed",
                 &old_all_of,
                 &new_all_of,
                 comparison,
                 ChangeClass::Unhandled,
                 ChangeDetails::UnknownDifference,
+            );
+        }
+
+        if old_schemas.len() == 1 {
+            let old_single_schema = old_all_of.append_deref(old_all_of.first().unwrap(), "0");
+            let new_single_schema = new_all_of.append_deref(new_all_of.first().unwrap(), "0");
+
+            self.compare_schema_ref_helper(
+                dry_run,
+                comparison,
+                old_single_schema,
+                new_single_schema,
             )
-        } else {
+        } else if old_schemas == new_schemas {
             Ok(true)
+        } else {
+            self.schema_push_change(
+                dry_run,
+                "allOf with multiple schemas is unhandled",
+                &old_all_of,
+                &new_all_of,
+                comparison,
+                ChangeClass::Unhandled,
+                ChangeDetails::UnknownDifference,
+            )
         }
     }
 
@@ -813,6 +834,7 @@ impl SchemaKindTag {
 }
 
 /// Classification of a schema reference for flattening purposes.
+#[derive(Debug)]
 enum SchemaRefKind<'a> {
     /// A bare $ref.
     BareRef,

tests/cases/simple/base.json

@@ -345,6 +345,14 @@
               }
             ]
           },
+          "via_clone": {
+            "description": "Via clone of SubType using allOf.",
+            "allOf": [
+              {
+                "$ref": "#/components/schemas/SubTypeClone"
+              }
+            ]
+          },
           "not_a_number": {
             "not": {
               "type": "number"
@@ -363,6 +371,14 @@
           }
         }
       },
+      "SubTypeClone": {
+        "description": "...",
+        "allOf": [
+          {
+            "$ref": "#/components/schemas/SubType"
+          }
+        ]
+      },
       "Tree": {
         "type": "object",
         "properties": {
@@ -381,7 +397,9 @@
             "type": "string"
           }
         },
-        "required": ["name"]
+        "required": [
+          "name"
+        ]
       },
       "UpdateItem": {
         "type": "object",
@@ -401,7 +419,10 @@
             "type": "integer"
           }
         },
-        "required": ["message", "code"]
+        "required": [
+          "message",
+          "code"
+        ]
       },
       "ArrayWithConstraints": {
         "type": "array",

tests/cases/simple/output/allof-ref-to-identical-type.out

@@ -0,0 +1,27 @@
+--- allof-ref-to-identical-type.json
++++ patched
+@@
+       "SubTypeClone": {
+         "allOf": [
+           {
+-            "$ref": "#/components/schemas/SubType"
++            "$ref": "#/components/schemas/SubTypeV2"
+           }
+         ],
+         "description": "..."
+       },
++      "SubTypeV2": {
++        "properties": {
++          "value": {
++            "type": "string"
++          }
++        },
++        "type": "object"
++      },
+       "Tree": {
+         "properties": {
+           "children": {
+
+
+Result for patch:
+[]

tests/cases/simple/output/anyof-to-allof.out

@@ -13,8 +13,8 @@
 -            "description": "Via anyOf."
 +            "description": "Via allOf."
            },
-           "via_oneof": {
-             "description": "Via oneOf.",
+           "via_clone": {
+             "allOf": [
 
 
 Result for patch:

tests/cases/simple/output/anyof-to-oneof.out

@@ -14,8 +14,8 @@
 -            "description": "Via anyOf."
 +            ]
            },
-           "via_oneof": {
-             "description": "Via oneOf.",
+           "via_clone": {
+             "allOf": [
 
 
 Result for patch:

tests/cases/simple/output/anyof-to-ref.out

@@ -12,8 +12,8 @@
 -            "description": "Via anyOf."
 +            "$ref": "#/components/schemas/SubType"
            },
-           "via_oneof": {
-             "description": "Via oneOf.",
+           "via_clone": {
+             "allOf": [
 
 
 Result for patch:

tests/cases/simple/output/multi-allof-variant-change.out

@@ -14,7 +14,7 @@
 Result for patch:
 [
     Change {
-        message: "unhandled, 'allOf' schema",
+        message: "allOf with multiple schemas is unhandled",
         old_path: [
             "#/components/schemas/MultiAllOf/allOf",
             "#/paths/~1allof/get/responses/200/content/application~1json/schema/$ref",

tests/cases/simple/output/oneof-to-allof.out

@@ -1,7 +1,7 @@
 --- oneof-to-allof.json
 +++ patched
 @@
-             "description": "Via anyOf."
+             "description": "Via clone of SubType using allOf."
            },
            "via_oneof": {
 -            "description": "Via oneOf.",

tests/cases/simple/output/oneof-to-anyof.out

@@ -1,7 +1,7 @@
 --- oneof-to-anyof.json
 +++ patched
 @@
-             "description": "Via anyOf."
+             "description": "Via clone of SubType using allOf."
            },
            "via_oneof": {
 -            "description": "Via oneOf.",

tests/cases/simple/output/oneof-to-ref.out

@@ -1,7 +1,7 @@
 --- oneof-to-ref.json
 +++ patched
 @@
-             "description": "Via anyOf."
+             "description": "Via clone of SubType using allOf."
            },
            "via_oneof": {
 -            "description": "Via oneOf.",

tests/cases/simple/output/schema-kind-type-to-oneof.out

@@ -17,8 +17,8 @@
 -        "type": "object"
 +        ]
        },
-       "Tree": {
-         "properties": {
+       "SubTypeClone": {
+         "allOf": [
 
 
 Result for patch:

tests/cases/simple/patch/allof-ref-to-identical-type.json

@@ -0,0 +1,21 @@
+[
+  {
+    "op": "add",
+    "path": "/components/schemas/SubTypeV2",
+    "value": {
+      "type": "object",
+      "properties": {
+        "value": {
+          "type": "string"
+        }
+      }
+    }
+  },
+  {
+    "op": "replace",
+    "path": "/components/schemas/SubTypeClone/allOf/0",
+    "value": {
+      "$ref": "#/components/schemas/SubTypeV2"
+    }
+  }
+]

tests/test_changes.rs

@@ -24,7 +24,12 @@ fn test_change() {
             // Start by making sure that the base compares cleanly against
             // itself.
             let result = compare(&base_value, &base_value).expect("fatal failure in comparison");
-            assert!(result.is_empty());
+            if !result.is_empty() {
+                panic!(
+                    "base.json does not compare cleanly against itself: {:#?}",
+                    result
+                );
+            }
 
             // Iterate through every file in the "patch" subdirectory.
             let patch_dir = path.join("patch");