Avoid inlining schemas in internally-tagged enum newtype variants (#355)

GREsau · web-flow · commit 95023c2ab0e1 · 2024-11-25T10:26:37.000Z
Schemas are still inlined in some cases, e.g. when the inner type has `deny_unknown_fields`, because then `$ref` would cause an unsatisfiable schema due to the variant tag not being allowed
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -10,6 +10,7 @@
 
 - MSRV is now 1.70
 - [The `example` attribute](https://graham.cool/schemars/deriving/attributes/#example) value is now an arbitrary expression, rather than a string literal identifying a function to call. To avoid silent behaviour changes, the expression must not be a string literal where the value can be parsed as a function path - e.g. `#[schemars(example = "foo")]` is now a compile error, but `#[schemars(example = foo())]` is allowed (as is `#[schemars(example = &"foo")]` if you want the the literal string value `"foo"` to be the example).
+- For newtype variants of internally-tagged enums, prefer referencing the inner type's schema via `$ref` instead of always inlining the schema (https://github.com/GREsau/schemars/pull/355)
 
 ### Fixed
 
diff --git a/schemars/src/_private/mod.rs b/schemars/src/_private/mod.rs
@@ -1,5 +1,5 @@
 use crate::_alloc_prelude::*;
-use crate::transform::transform_immediate_subschemas;
+use crate::transform::{transform_immediate_subschemas, Transform};
 use crate::{JsonSchema, Schema, SchemaGenerator};
 use serde::Serialize;
 use serde_json::{json, map::Entry, Map, Value};
@@ -12,6 +12,39 @@ pub extern crate serde_json;
 
 pub use rustdoc::get_title_and_description;
 
+pub fn json_schema_for_internally_tagged_enum_newtype_variant<T: ?Sized + JsonSchema>(
+    generator: &mut SchemaGenerator,
+) -> Schema {
+    let mut schema = T::json_schema(generator);
+
+    // Inline the newtype's inner schema if any of:
+    // - The type specifies that its schema should always be inlined
+    // - The generator settings specify that all schemas should be inlined
+    // - The inner type is a unit struct, which would cause an unsatisfiable schema due to mismatched `type`.
+    //    In this case, we replace its type with "object" in `apply_internal_enum_variant_tag`
+    // - The inner schema specified `"additionalProperties": false` or `"unevaluatedProperties": false`,
+    //    since that would disallow the variant tag. If additional/unevaluatedProperties is in the top-level
+    //    schema, then we can leave it there, because it will "see" the variant tag property. But if it is
+    //    nested e.g. in an `allOf`, then it must be removed, which is why we run `AllowUnknownProperties`
+    //    but only on immediate subschemas.
+
+    let mut transform = AllowUnknownProperties::default();
+    transform_immediate_subschemas(&mut transform, &mut schema);
+
+    if T::always_inline_schema()
+        || generator.settings().inline_subschemas
+        || schema.get("type").and_then(Value::as_str) == Some("null")
+        || schema.get("additionalProperties").and_then(Value::as_bool) == Some(false)
+        || schema.get("unevaluatedProperties").and_then(Value::as_bool) == Some(false)
+        || transform.did_modify
+    {
+        return schema;
+    }
+
+    // ...otherwise, we can freely refer to the schema via a `$ref`
+    generator.subschema_for::<T>()
+}
+
 // Helper for generating schemas for flattened `Option` fields.
 pub fn json_schema_for_flatten<T: ?Sized + JsonSchema>(
     generator: &mut SchemaGenerator,
@@ -25,20 +58,29 @@ pub fn json_schema_for_flatten<T: ?Sized + JsonSchema>(
 
     // Always allow aditional/unevaluated properties, because the outer struct determines
     // whether it denies unknown fields.
-    allow_unknown_properties(&mut schema);
+    AllowUnknownProperties::default().transform(&mut schema);
 
     schema
 }
 
-fn allow_unknown_properties(schema: &mut Schema) {
-    if schema.get("additionalProperties").and_then(Value::as_bool) == Some(false) {
-        schema.remove("additionalProperties");
-    }
-    if schema.get("unevaluatedProperties").and_then(Value::as_bool) == Some(false) {
-        schema.remove("unevaluatedProperties");
-    }
+#[derive(Default)]
+struct AllowUnknownProperties {
+    did_modify: bool,
+}
 
-    transform_immediate_subschemas(&mut allow_unknown_properties, schema);
+impl Transform for AllowUnknownProperties {
+    fn transform(&mut self, schema: &mut Schema) {
+        if schema.get("additionalProperties").and_then(Value::as_bool) == Some(false) {
+            schema.remove("additionalProperties");
+            self.did_modify = true;
+        }
+        if schema.get("unevaluatedProperties").and_then(Value::as_bool) == Some(false) {
+            schema.remove("unevaluatedProperties");
+            self.did_modify = true;
+        }
+
+        transform_immediate_subschemas(self, schema);
+    }
 }
 
 /// Hack to simulate specialization:
diff --git a/schemars/tests/integration/enums_deny_unknown_fields.rs b/schemars/tests/integration/enums_deny_unknown_fields.rs
@@ -15,6 +15,10 @@ macro_rules! fn_values {
                     foo: 123,
                     bar: true,
                 }),
+                Self::StructDenyUnknownFieldsNewType(StructDenyUnknownFields {
+                    baz: 123,
+                    foobar: true,
+                }),
                 Self::Struct {
                     foo: 123,
                     bar: true,
@@ -30,12 +34,20 @@ struct Struct {
     bar: bool,
 }
 
+#[derive(JsonSchema, Deserialize, Serialize, Default)]
+#[serde(deny_unknown_fields)]
+struct StructDenyUnknownFields {
+    baz: i32,
+    foobar: bool,
+}
+
 #[derive(JsonSchema, Deserialize, Serialize)]
 #[serde(deny_unknown_fields)]
 enum External {
     Unit,
     StringMap(BTreeMap<String, String>),
     StructNewType(Struct),
+    StructDenyUnknownFieldsNewType(StructDenyUnknownFields),
     Struct { foo: i32, bar: bool },
 }
 
@@ -49,6 +61,7 @@ enum Internal {
     Unit,
     StringMap(BTreeMap<String, String>),
     StructNewType(Struct),
+    StructDenyUnknownFieldsNewType(StructDenyUnknownFields),
     Struct { foo: i32, bar: bool },
 }
 
@@ -62,6 +75,7 @@ enum Adjacent {
     Unit,
     StringMap(BTreeMap<String, String>),
     StructNewType(Struct),
+    StructDenyUnknownFieldsNewType(StructDenyUnknownFields),
     Struct { foo: i32, bar: bool },
 }
 
@@ -75,6 +89,7 @@ enum Untagged {
     Unit,
     StringMap(BTreeMap<String, String>),
     StructNewType(Struct),
+    StructDenyUnknownFieldsNewType(StructDenyUnknownFields),
     Struct { foo: i32, bar: bool },
 }
 
@@ -88,13 +103,22 @@ fn externally_tagged_enum() {
         .assert_snapshot()
         .assert_allows_ser_roundtrip(External::values())
         .assert_matches_de_roundtrip(arbitrary_values())
-        .assert_rejects_de([json!({
-            "Struct": {
-                "foo": 123,
-                "bar": true,
-                "extra": null
-            }
-        })])
+        .assert_rejects_de([
+            json!({
+                "Struct": {
+                    "foo": 123,
+                    "bar": true,
+                    "extra": null
+                }
+            }),
+            json!({
+                "StructDenyUnknownFieldsNewType": {
+                    "baz": 123,
+                    "foobar": true,
+                    "extra": null
+                }
+            }),
+        ])
         .assert_allows_de_roundtrip([json!({
             "StructNewType": {
                 "foo": 123,
@@ -110,12 +134,20 @@ fn internally_tagged_enum() {
         .assert_snapshot()
         .assert_allows_ser_roundtrip(Internal::values())
         .assert_matches_de_roundtrip(arbitrary_values())
-        .assert_rejects_de([json!({
-            "tag": "Struct",
-            "foo": 123,
-            "bar": true,
-            "extra": null
-        })])
+        .assert_rejects_de([
+            json!({
+                "tag": "Struct",
+                "foo": 123,
+                "bar": true,
+                "extra": null
+            }),
+            json!({
+                "tag": "StructDenyUnknownFieldsNewType",
+                "baz": 123,
+                "foobar": true,
+                "extra": null
+            }),
+        ])
         .assert_allows_de_roundtrip([json!({
             "tag": "StructNewType",
             "foo": 123,
@@ -130,14 +162,24 @@ fn adjacently_tagged_enum() {
         .assert_snapshot()
         .assert_allows_ser_roundtrip(Adjacent::values())
         .assert_matches_de_roundtrip(arbitrary_values())
-        .assert_rejects_de([json!({
-            "tag": "Struct",
-            "content": {
-                "foo": 123,
-                "bar": true,
-                "extra": null
-            }
-        })])
+        .assert_rejects_de([
+            json!({
+                "tag": "Struct",
+                "content": {
+                    "foo": 123,
+                    "bar": true,
+                    "extra": null
+                }
+            }),
+            json!({
+                "tag": "StructDenyUnknownFieldsNewType",
+                "content": {
+                    "baz": 123,
+                    "foobar": true,
+                    "extra": null
+                }
+            }),
+        ])
         .assert_allows_de_roundtrip([json!({
             "tag": "StructNewType",
             "content": {
@@ -154,6 +196,11 @@ fn untagged_enum() {
         .assert_snapshot()
         .assert_allows_ser_roundtrip(Untagged::values())
         .assert_matches_de_roundtrip(arbitrary_values())
+        .assert_rejects_de([json!({
+            "baz": 123,
+            "foobar": true,
+            "extra": null
+        })])
         .assert_allows_de_roundtrip([json!({
             "foo": 123,
             "bar": true,
diff --git a/schemars/tests/integration/snapshots/schemars/tests/integration/enums.rs~internally_tagged_enum.json b/schemars/tests/integration/snapshots/schemars/tests/integration/enums.rs~internally_tagged_enum.json
@@ -44,22 +44,14 @@
     {
       "type": "object",
       "properties": {
-        "foo": {
-          "type": "integer",
-          "format": "int32"
-        },
-        "bar": {
-          "type": "boolean"
-        },
         "tag": {
           "type": "string",
           "const": "StructNewType"
         }
       },
+      "$ref": "#/$defs/Struct",
       "required": [
-        "tag",
-        "foo",
-        "bar"
+        "tag"
       ]
     },
     {
@@ -95,5 +87,23 @@
         "tag"
       ]
     }
-  ]
+  ],
+  "$defs": {
+    "Struct": {
+      "type": "object",
+      "properties": {
+        "foo": {
+          "type": "integer",
+          "format": "int32"
+        },
+        "bar": {
+          "type": "boolean"
+        }
+      },
+      "required": [
+        "foo",
+        "bar"
+      ]
+    }
+  }
 }
diff --git a/schemars/tests/integration/snapshots/schemars/tests/integration/enums_deny_unknown_fields.rs~adjacently_tagged_enum.json b/schemars/tests/integration/snapshots/schemars/tests/integration/enums_deny_unknown_fields.rs~adjacently_tagged_enum.json
@@ -52,6 +52,23 @@
       ],
       "additionalProperties": false
     },
+    {
+      "type": "object",
+      "properties": {
+        "tag": {
+          "type": "string",
+          "const": "StructDenyUnknownFieldsNewType"
+        },
+        "content": {
+          "$ref": "#/$defs/StructDenyUnknownFields"
+        }
+      },
+      "required": [
+        "tag",
+        "content"
+      ],
+      "additionalProperties": false
+    },
     {
       "type": "object",
       "properties": {
@@ -100,6 +117,23 @@
         "foo",
         "bar"
       ]
+    },
+    "StructDenyUnknownFields": {
+      "type": "object",
+      "properties": {
+        "baz": {
+          "type": "integer",
+          "format": "int32"
+        },
+        "foobar": {
+          "type": "boolean"
+        }
+      },
+      "additionalProperties": false,
+      "required": [
+        "baz",
+        "foobar"
+      ]
     }
   }
 }
diff --git a/schemars/tests/integration/snapshots/schemars/tests/integration/enums_deny_unknown_fields.rs~externally_tagged_enum.json b/schemars/tests/integration/snapshots/schemars/tests/integration/enums_deny_unknown_fields.rs~externally_tagged_enum.json
@@ -35,6 +35,18 @@
       ],
       "additionalProperties": false
     },
+    {
+      "type": "object",
+      "properties": {
+        "StructDenyUnknownFieldsNewType": {
+          "$ref": "#/$defs/StructDenyUnknownFields"
+        }
+      },
+      "required": [
+        "StructDenyUnknownFieldsNewType"
+      ],
+      "additionalProperties": false
+    },
     {
       "type": "object",
       "properties": {
@@ -78,6 +90,23 @@
         "foo",
         "bar"
       ]
+    },
+    "StructDenyUnknownFields": {
+      "type": "object",
+      "properties": {
+        "baz": {
+          "type": "integer",
+          "format": "int32"
+        },
+        "foobar": {
+          "type": "boolean"
+        }
+      },
+      "additionalProperties": false,
+      "required": [
+        "baz",
+        "foobar"
+      ]
     }
   }
 }
diff --git a/schemars/tests/integration/snapshots/schemars/tests/integration/enums_deny_unknown_fields.rs~internally_tagged_enum.json b/schemars/tests/integration/snapshots/schemars/tests/integration/enums_deny_unknown_fields.rs~internally_tagged_enum.json
diff --git a/schemars/tests/integration/snapshots/schemars/tests/integration/enums_deny_unknown_fields.rs~untagged_enum.json b/schemars/tests/integration/snapshots/schemars/tests/integration/enums_deny_unknown_fields.rs~untagged_enum.json
diff --git a/schemars_derive/src/schema_exprs.rs b/schemars_derive/src/schema_exprs.rs
