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

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

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
 

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) {
+#[derive(Default)]
+struct AllowUnknownProperties {
+    did_modify: bool,
+}
+
+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(&mut allow_unknown_properties, schema);
+        transform_immediate_subschemas(self, schema);
+    }
 }
 
 /// Hack to simulate specialization:

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!({
+        .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!({
+        .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!({
+        .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,

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",
+        "tag"
-        "foo",
-        "bar"
       ]
     },
     {
@@ -95,5 +87,23 @@
         "tag"
       ]
     }
+  ],
+  "$defs": {
+    "Struct": {
+      "type": "object",
+      "properties": {
+        "foo": {
+          "type": "integer",
+          "format": "int32"
+        },
+        "bar": {
+          "type": "boolean"
+        }
+      },
+      "required": [
+        "foo",
+        "bar"
       ]
+    }
+  }
 }

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"
+      ]
     }
   }
 }

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"
+      ]
     }
   }
 }

schemars/tests/integration/snapshots/schemars/tests/integration/enums_deny_unknown_fields.rs~internally_tagged_enum.json

@@ -33,22 +33,36 @@
     {
       "type": "object",
       "properties": {
-        "foo": {
-          "type": "integer",
-          "format": "int32"
-        },
-        "bar": {
-          "type": "boolean"
-        },
         "tag": {
           "type": "string",
           "const": "StructNewType"
         }
       },
+      "$ref": "#/$defs/Struct",
+      "required": [
+        "tag"
+      ]
+    },
+    {
+      "type": "object",
+      "properties": {
+        "baz": {
+          "type": "integer",
+          "format": "int32"
+        },
+        "foobar": {
+          "type": "boolean"
+        },
+        "tag": {
+          "type": "string",
+          "const": "StructDenyUnknownFieldsNewType"
+        }
+      },
+      "additionalProperties": false,
       "required": [
         "tag",
-        "foo",
+        "baz",
-        "bar"
+        "foobar"
       ]
     },
     {
@@ -73,5 +87,23 @@
         "bar"
       ]
     }
+  ],
+  "$defs": {
+    "Struct": {
+      "type": "object",
+      "properties": {
+        "foo": {
+          "type": "integer",
+          "format": "int32"
+        },
+        "bar": {
+          "type": "boolean"
+        }
+      },
+      "required": [
+        "foo",
+        "bar"
       ]
+    }
+  }
 }

schemars/tests/integration/snapshots/schemars/tests/integration/enums_deny_unknown_fields.rs~untagged_enum.json

@@ -14,6 +14,9 @@
     {
       "$ref": "#/$defs/Struct"
     },
+    {
+      "$ref": "#/$defs/StructDenyUnknownFields"
+    },
     {
       "type": "object",
       "properties": {
@@ -48,6 +51,23 @@
         "foo",
         "bar"
       ]
+    },
+    "StructDenyUnknownFields": {
+      "type": "object",
+      "properties": {
+        "baz": {
+          "type": "integer",
+          "format": "int32"
+        },
+        "foobar": {
+          "type": "boolean"
+        }
+      },
+      "additionalProperties": false,
+      "required": [
+        "baz",
+        "foobar"
+      ]
     }
   }
 }

schemars_derive/src/schema_exprs.rs

@@ -112,23 +112,24 @@ pub fn expr_for_repr(cont: &Container) -> Result<SchemaExpr, syn::Error> {
     Ok(schema_expr)
 }
 
-fn expr_for_field(field: &Field, allow_ref: bool) -> SchemaExpr {
+fn expr_for_field(field: &Field, is_internal_tagged_enum_newtype: bool) -> SchemaExpr {
     let (ty, type_def) = type_for_field_schema(field);
     let span = field.original.span();
 
-    let mut schema_expr = SchemaExpr::from(if field.attrs.validation.required {
+    let schema_expr = if field.attrs.validation.required {
         quote_spanned! {span=>
             <#ty as schemars::JsonSchema>::_schemars_private_non_optional_json_schema(#GENERATOR)
         }
-    } else if allow_ref {
+    } else if is_internal_tagged_enum_newtype {
         quote_spanned! {span=>
-            #GENERATOR.subschema_for::<#ty>()
+            schemars::_private::json_schema_for_internally_tagged_enum_newtype_variant::<#ty>(#GENERATOR)
         }
     } else {
         quote_spanned! {span=>
-            <#ty as schemars::JsonSchema>::json_schema(#GENERATOR)
+            #GENERATOR.subschema_for::<#ty>()
         }
-    });
+    };
+    let mut schema_expr = SchemaExpr::from(schema_expr);
 
     schema_expr.definitions.extend(type_def);
     field.add_mutators(&mut schema_expr.mutators);
@@ -407,7 +408,7 @@ fn expr_for_untagged_enum_variant(variant: &Variant, deny_unknown_fields: bool)
 
     match variant.style {
         Style::Unit => expr_for_unit_struct(),
-        Style::Newtype => expr_for_field(&variant.fields[0], true),
+        Style::Newtype => expr_for_field(&variant.fields[0], false),
         Style::Tuple => expr_for_tuple_struct(&variant.fields),
         Style::Struct => expr_for_struct(&variant.fields, &SerdeDefault::None, deny_unknown_fields),
     }
@@ -430,7 +431,7 @@ fn expr_for_internal_tagged_enum_variant(
 
     match variant.style {
         Style::Unit => expr_for_unit_struct(),
-        Style::Newtype => expr_for_field(&variant.fields[0], false),
+        Style::Newtype => expr_for_field(&variant.fields[0], true),
         Style::Tuple => expr_for_tuple_struct(&variant.fields),
         Style::Struct => expr_for_struct(&variant.fields, &SerdeDefault::None, deny_unknown_fields),
     }
@@ -444,14 +445,14 @@ fn expr_for_unit_struct() -> SchemaExpr {
 }
 
 fn expr_for_newtype_struct(field: &Field) -> SchemaExpr {
-    expr_for_field(field, true)
+    expr_for_field(field, false)
 }
 
 fn expr_for_tuple_struct(fields: &[Field]) -> SchemaExpr {
     let fields: Vec<_> = fields
         .iter()
         .map(|f| {
-            let field_expr = expr_for_field(f, true);
+            let field_expr = expr_for_field(f, false);
             f.with_contract_check(quote! {
                 prefix_items.push((#field_expr).to_value());
             })