Repository: oxidecomputer/typify (rust)

Issue:
Panic due to missing/incorrect metadata after subschema merge
Processing this schema:
```json
{
    "title": "My root schema",
    "properties": {
        "foo": {
            "type": "integer"
        }
    },
    "allOf": [
        {
            "title": "My subschema",
            "properties": {
                "bar": {
                    "type": "integer"
                }
            }
        }
    ]
}
```
will panic with
```
The application panicked (crashed).
Message:  called `Option::unwrap()` on a `None` value
Location: typify-impl/src/type_entry.rs:289
```
due to metadata getting lost somewhere in the `// Subschemas with other stuff.` branch of  `TypeSpace::convert_schema_object`.

Reducing the schema to:
```json
{
    "title": "My root schema",
    "allOf": [
        {
            "title": "My subschema",
            "properties": {
                "bar": {
                    "type": "integer"
                }
            }
        }
    ]
}
```
will go into the `convert_all_of` branch and (in my opinion incorrectly) generate:
```rust
pub struct MySubschema {
    #[serde(default, skip_serializing_if = "Option::is_none")]
    pub bar: Option<i64>,
}
```
`anyOf` also shows the same behaviour.

For an example of such a schema in the wild, see (after resolving references):
* https://github.com/KhronosGroup/glTF/blob/main/specification/2.0/schema/glTFChildOfRootProperty.schema.json
* https://github.com/KhronosGroup/glTF/blob/main/specification/2.0/schema/glTFProperty.schema.json

Pull request:
improvements to schema merge logic
This fixes a couple of bugs. The first (#573) is pretty simple: when we merge schemas that contain both subschemas and other stuff, we drop all metadata; we should preserve the outer schema metadata for `title` and `description` in particular.

The next is that `convert_schema_object` calls `merge_with_subschemas` and expects it not to fail... but it can fail! In particular it will fail if the result of the merged schema is unsatisfiable (i.e. there is no value that would validate against the schema). In such a situation we now handle the error and produce a "never" schema (enum with no variants).


Closes #573, #572 



Patch:
diff --git a/typify-impl/src/convert.rs b/typify-impl/src/convert.rs
index a164604..1ad1666 100644
--- a/typify-impl/src/convert.rs
+++ b/typify-impl/src/convert.rs
@@ -2,7 +2,7 @@
 
 use std::collections::BTreeSet;
 
-use crate::merge::{merge_all, merge_with_subschemas};
+use crate::merge::{merge_all, try_merge_with_subschemas};
 use crate::type_entry::{
     EnumTagType, TypeEntry, TypeEntryDetails, TypeEntryEnum, TypeEntryNewtype, TypeEntryStruct,
     Variant, VariantDetails,
@@ -517,31 +517,42 @@ impl TypeSpace {
 
             // Subschemas with other stuff.
             SchemaObject {
-                subschemas: Some(_),
+                metadata,
+                subschemas: subschemas @ Some(_),
                 ..
             } => {
                 let without_subschemas = SchemaObject {
                     subschemas: None,
+                    metadata: None,
                     ..schema.clone()
                 };
                 debug!(
                     "pre merged schema {}",
                     serde_json::to_string_pretty(schema).unwrap(),
                 );
-                let merged_schema = merge_with_subschemas(
+                match try_merge_with_subschemas(
                     without_subschemas,
-                    schema.subschemas.as_deref(),
+                    subschemas.as_deref(),
                     &self.definitions,
-                );
+                ) {
+                    Ok(merged_schema) => {
+                        // Preserve metadata from the outer schema.
+                        let merged_schema = SchemaObject {
+                            metadata: metadata.clone(),
+                            ..merged_schema
+                        };
+                        debug!(
+                            "merged schema {}",
+                            serde_json::to_string_pretty(&merged_schema).unwrap(),
+                        );
 
-                debug!(
-                    "merged schema {}",
-                    serde_json::to_string_pretty(&merged_schema).unwrap(),
-                );
+                        let (type_entry, _) =
+                            self.convert_schema_object(type_name, original_schema, &merged_schema)?;
+                        Ok((type_entry, &None))
+                    }
 
-                let (type_entry, _) =
-                    self.convert_schema_object(type_name, original_schema, &merged_schema)?;
-                Ok((type_entry, &None))
+                    Err(_) => self.convert_never(type_name, original_schema),
+                }
             }
 
             // TODO let's not bother with const values at the moment. In the
@@ -1786,8 +1797,8 @@ impl TypeSpace {
             // convert the resulting type to be optional.
             let enum_values = enum_values
                 .iter()
-                .cloned()
                 .filter(|v| !v.is_null())
+                .cloned()
                 .collect::<Vec<_>>();
 
             let (type_entry, metadata) =
diff --git a/typify-impl/src/merge.rs b/typify-impl/src/merge.rs
index 73e5827..6ad751d 100644
--- a/typify-impl/src/merge.rs
+++ b/typify-impl/src/merge.rs
@@ -266,18 +266,10 @@ fn merge_so_enum_values(
     }
 }
 
-pub(crate) fn merge_with_subschemas(
-    schema_object: SchemaObject,
-    maybe_subschemas: Option<&SubschemaValidation>,
-    defs: &BTreeMap<RefKey, Schema>,
-) -> SchemaObject {
-    try_merge_with_subschemas(schema_object, maybe_subschemas, defs).unwrap()
-}
-
 /// Merge the schema with a subschema validation object. It's important that
 /// the return value reduces the complexity of the problem so avoid infinite
 /// recursion.
-fn try_merge_with_subschemas(
+pub(crate) fn try_merge_with_subschemas(
     mut schema_object: SchemaObject,
     maybe_subschemas: Option<&SubschemaValidation>,
     defs: &BTreeMap<RefKey, Schema>,


Test patch:
diff --git a/typify/tests/schemas/merged-schemas.json b/typify/tests/schemas/merged-schemas.json
index 0ed0e19..bbdb512 100644
--- a/typify/tests/schemas/merged-schemas.json
+++ b/typify/tests/schemas/merged-schemas.json
@@ -360,6 +360,63 @@
           "bar": "string"
         }
       }
+    },
+    "Unresolvable": {
+      "$comment": "subschemas all end up unresolvable",
+      "oneOf": [
+        {
+          "type": "object",
+          "properties": {
+            "x": {
+              "enum": [
+                "a"
+              ]
+            }
+          },
+          "required": [
+            "x"
+          ]
+        },
+        {
+          "type": "object",
+          "properties": {
+            "x": {
+              "enum": [
+                "b"
+              ]
+            }
+          },
+          "required": [
+            "x"
+          ]
+        }
+      ],
+      "type": "object",
+      "properties": {
+        "x": {
+          "enum": [
+            "c"
+          ]
+        }
+      },
+      "required": [
+        "x"
+      ]
+    },
+    "CommentedTypeMerged": {
+      "description": "if we don't see this, we dropped the metadata",
+      "type": "object",
+      "properties": {
+        "x": true
+      },
+      "allOf": [
+        {
+          "type": "object",
+          "properties": {
+            "y": true
+          }
+        }
+      ]
     }
   }
 }
\ No newline at end of file
diff --git a/typify/tests/schemas/merged-schemas.rs b/typify/tests/schemas/merged-schemas.rs
index b3608d9..ea090cf 100644
--- a/typify/tests/schemas/merged-schemas.rs
+++ b/typify/tests/schemas/merged-schemas.rs
@@ -79,6 +79,40 @@ impl From<&ButNotThat> for ButNotThat {
         value.clone()
     }
 }
+#[doc = "if we don't see this, we dropped the metadata"]
+#[doc = r""]
+#[doc = r" <details><summary>JSON schema</summary>"]
+#[doc = r""]
+#[doc = r" ```json"]
+#[doc = "{"]
+#[doc = "  \"description\": \"if we don't see this, we dropped the metadata\","]
+#[doc = "  \"type\": \"object\","]
+#[doc = "  \"allOf\": ["]
+#[doc = "    {"]
+#[doc = "      \"type\": \"object\","]
+#[doc = "      \"properties\": {"]
+#[doc = "        \"y\": true"]
+#[doc = "      }"]
+#[doc = "    }"]
+#[doc = "  ],"]
+#[doc = "  \"properties\": {"]
+#[doc = "    \"x\": true"]
+#[doc = "  }"]
+#[doc = "}"]
+#[doc = r" ```"]
+#[doc = r" </details>"]
+#[derive(Clone, Debug, Deserialize, Serialize)]
+pub struct CommentedTypeMerged {
+    #[serde(default, skip_serializing_if = "Option::is_none")]
+    pub x: Option<serde_json::Value>,
+    #[serde(default, skip_serializing_if = "Option::is_none")]
+    pub y: Option<serde_json::Value>,
+}
+impl From<&CommentedTypeMerged> for CommentedTypeMerged {
+    fn from(value: &CommentedTypeMerged) -> Self {
+        value.clone()
+    }
+}
 #[doc = "JsonResponseBase"]
 #[doc = r""]
 #[doc = r" <details><summary>JSON schema</summary>"]
@@ -571,6 +605,63 @@ impl From<&TrimFat> for TrimFat {
         value.clone()
     }
 }
+#[doc = "Unresolvable"]
+#[doc = r""]
+#[doc = r" <details><summary>JSON schema</summary>"]
+#[doc = r""]
+#[doc = r" ```json"]
+#[doc = "{"]
+#[doc = "  \"type\": \"object\","]
+#[doc = "  \"oneOf\": ["]
+#[doc = "    {"]
+#[doc = "      \"type\": \"object\","]
+#[doc = "      \"required\": ["]
+#[doc = "        \"x\""]
+#[doc = "      ],"]
+#[doc = "      \"properties\": {"]
+#[doc = "        \"x\": {"]
+#[doc = "          \"enum\": ["]
+#[doc = "            \"a\""]
+#[doc = "          ]"]
+#[doc = "        }"]
+#[doc = "      }"]
+#[doc = "    },"]
+#[doc = "    {"]
+#[doc = "      \"type\": \"object\","]
+#[doc = "      \"required\": ["]
+#[doc = "        \"x\""]
+#[doc = "      ],"]
+#[doc = "      \"properties\": {"]
+#[doc = "        \"x\": {"]
+#[doc = "          \"enum\": ["]
+#[doc = "            \"b\""]
+#[doc = "          ]"]
+#[doc = "        }"]
+#[doc = "      }"]
+#[doc = "    }"]
+#[doc = "  ],"]
+#[doc = "  \"required\": ["]
+#[doc = "    \"x\""]
+#[doc = "  ],"]
+#[doc = "  \"properties\": {"]
+#[doc = "    \"x\": {"]
+#[doc = "      \"enum\": ["]
+#[doc = "        \"c\""]
+#[doc = "      ]"]
+#[doc = "    }"]
+#[doc = "  },"]
+#[doc = "  \"$comment\": \"subschemas all end up unresolvable\""]
+#[doc = "}"]
+#[doc = r" ```"]
+#[doc = r" </details>"]
+#[derive(Clone, Copy, Debug, Deserialize, Eq, Hash, Ord, PartialEq, PartialOrd, Serialize)]
+#[serde(deny_unknown_fields)]
+pub enum Unresolvable {}
+impl From<&Unresolvable> for Unresolvable {
+    fn from(value: &Unresolvable) -> Self {
+        value.clone()
+    }
+}
 #[doc = "Unsatisfiable1"]
 #[doc = r""]
 #[doc = r" <details><summary>JSON schema</summary>"]