From 4a058a927ebcea6a1c5f9daccac1394aa688e139 Mon Sep 17 00:00:00 2001 From: James Lal Date: Tue, 8 Sep 2026 07:02:38 -0600 Subject: [PATCH 1/3] fix: keep arbitrary keys on an unconstrained object union member (#70) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit All members of a `type: [...]` union share one `SchemaDetails`, so the `object` member may carry no object shape at all. It was still routed through `add_allocated_object_schema`, which projected it as a closed, empty struct: `{"payload": {"key": "value"}}` deserialized fine and then serialized back as `{"payload": {}}`, silently dropping every key. The single-type path already guards this with `should_use_dynamic_json`; the typed-multi path did not. Split the object-shape half of `is_dynamic_object_pattern` into `object_shape_is_unconstrained` so the union member can ask the same question — `schema_type()` reports only the first non-null member of a `type: [...]` list, so that caller cannot reuse the type check — and give an unconstrained member a `BTreeMap` carrier. Unlike `serde_json::Value` the map matches only objects, so the untagged enum keeps routing arrays and scalars to their own members. Members the spec does shape (`properties`, `required`, `additionalProperties`, `minProperties`/`maxProperties`, and the rest) keep their generated structs. The regression test compiles the generated code in a scratch crate and round-trips every member of the issue's union. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_012mBAVbYp2iUSL8VwpyDf19 --- CHANGELOG.md | 12 + src/analysis.rs | 59 ++++- .../unconstrained_object_union_member_test.rs | 217 ++++++++++++++++++ 3 files changed, 280 insertions(+), 8 deletions(-) create mode 100644 tests/unconstrained_object_union_member_test.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index 6fd9516..ae5c379 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,18 @@ when correcting output that was wrong or incomplete on the wire. ## [Unreleased] +### Fixed + +- The `object` member of a `type: [...]` union keeps arbitrary keys. All members + of such a union share one schema, so the object member may carry no object + shape at all; it was projected as a closed, empty struct, which deserialized + any object and then serialized it back as `{}`. An unconstrained member now + generates a `BTreeMap` carrier, which still + matches only objects, so the other members keep their own variants. Members + the spec does shape (`properties`, `required`, `additionalProperties`, + `minProperties`/`maxProperties`, and the rest) keep their generated structs + (#70). + ## [0.15.0] - 2026-08-28 ### Breaking changes diff --git a/src/analysis.rs b/src/analysis.rs index 25617d3..3ecf3b1 100644 --- a/src/analysis.rs +++ b/src/analysis.rs @@ -1759,6 +1759,16 @@ impl SchemaAnalyzer { } } + /// As [`Self::untyped_value`], for an object whose members are entirely + /// unconstrained. Unlike [`Self::untyped_value`] this only ever matches a + /// JSON object, which is what makes it usable as an untagged union member. + fn untyped_value_map(&self, _context: impl Into, reason: UntypedReason) -> SchemaType { + SchemaType::Untyped { + shape: UntypedShape::ValueMap, + reason, + } + } + /// The schema currently being analyzed, for finding context. fn untyped_context(&self, detail: &str) -> String { match (&self.current_schema_name, detail) { @@ -3668,6 +3678,36 @@ impl SchemaAnalyzer { "typed-multi-object", schema, ); + // The shared `SchemaDetails` may say nothing at all about an + // object's members (`type: [string, object]` and nothing else). + // Projecting that as a struct yields a closed, empty one, which + // deserializes any object and then serializes it back as `{}`. + // A map carrier keeps the keys, and unlike `serde_json::Value` + // it still matches only objects, so the untagged enum keeps + // routing arrays and scalars to their own members. + if Self::object_shape_is_unconstrained(schema.details()) { + let schema_type = self.untyped_value_map( + self.untyped_context(&object_type_name), + UntypedReason::OpaqueObject, + ); + self.resolved_cache.insert( + object_type_name.clone(), + AnalyzedSchema { + name: object_type_name.clone(), + original: serde_json::to_value(schema).unwrap_or(Value::Null), + schema_type, + dependencies: HashSet::new(), + nullable: false, + description: Some("Object variant in union".to_string()), + default: None, + }, + ); + dependencies.insert(object_type_name.clone()); + return Ok(SchemaRef { + target: object_type_name, + nullable: false, + }); + } let object_type = self.add_allocated_object_schema( object_type_name.clone(), schema, @@ -7375,13 +7415,22 @@ impl SchemaAnalyzer { return false; } - let details = schema.details(); + Self::object_shape_is_unconstrained(schema.details()) + } + /// Whether the schema's object-shape keywords leave an object's members + /// entirely unconstrained, ignoring the `type` keyword itself. + /// + /// Split out of [`Self::is_dynamic_object_pattern`] so the `object` member + /// of a `type: […]` union can ask the same question: `schema_type()` + /// reports only the first non-null member there, so the type check in that + /// caller cannot be reused. + fn object_shape_is_unconstrained(details: &crate::openapi::SchemaDetails) -> bool { // An explicit additionalProperties policy is structural even when no // named properties exist. `true`/a schema needs a map carrier, while // `false` is a closed empty object (GitHub's `empty-object`) and must // not become `serde_json::Value`, which would match every oneOf branch. - if self.has_explicit_additional_properties(schema) { + if details.additional_properties.is_some() { return false; } @@ -7416,12 +7465,6 @@ impl SchemaAnalyzer { false } - /// Check whether the object declares any explicit additional-properties policy. - fn has_explicit_additional_properties(&self, schema: &Schema) -> bool { - let details = schema.details(); - details.additional_properties.is_some() - } - /// Analyze OpenAPI operations to extract request/response schemas fn analyze_operations(&mut self, analysis: &mut SchemaAnalysis) -> Result<()> { let spec: crate::openapi::OpenApiSpec = parse_spec_document(&self.openapi_spec)?; diff --git a/tests/unconstrained_object_union_member_test.rs b/tests/unconstrained_object_union_member_test.rs new file mode 100644 index 0000000..feba90c --- /dev/null +++ b/tests/unconstrained_object_union_member_test.rs @@ -0,0 +1,217 @@ +//! gh#70: the `object` member of a `type: [...]` union shares one +//! `SchemaDetails` with every other member, so it may carry no object shape at +//! all. Projecting that as a struct produced a closed, empty one: arbitrary +//! objects deserialized fine and then serialized back as `{}`, silently +//! dropping every key. The unconstrained member must use a map carrier, while +//! a member the spec *does* shape keeps its generated struct. + +use openapi_to_rust::{CodeGenerator, GeneratorConfig, SchemaAnalyzer}; +use serde_json::{Value, json}; +use std::process::Command; + +/// The reproduction from gh#70 plus the two shapes that must not change: +/// an object member with declared properties, and one closed by +/// `additionalProperties: false`. +fn union_spec() -> Value { + json!({ + "openapi": "3.1.0", + "info": { "title": "object union", "version": "1.0.0" }, + "paths": {}, + "components": { "schemas": { + "Unconstrained": { + "type": "object", + "required": ["payload"], + "properties": { "payload": { + "type": ["string", "number", "boolean", "object", "array", "null"], + "items": {} + } } + }, + "Shaped": { + "type": "object", + "required": ["payload"], + "properties": { "payload": { + "type": ["string", "object"], + "properties": { "id": { "type": "string" } } + } } + }, + "Closed": { + "type": "object", + "required": ["payload"], + "properties": { "payload": { + "type": ["string", "object"], + "additionalProperties": false + } } + } + } } + }) +} + +fn generate(spec: Value, output_dir: std::path::PathBuf) -> String { + let mut analyzer = SchemaAnalyzer::new(spec).expect("parse object union spec"); + let mut analysis = analyzer.analyze().expect("analyze object union spec"); + let generator = CodeGenerator::new(GeneratorConfig { + output_dir, + module_name: "object_union".into(), + enable_async_client: false, + enable_sse_client: false, + tracing_enabled: false, + ..Default::default() + }); + generator + .generate(&mut analysis) + .expect("generate object union models") +} + +#[test] +fn unconstrained_object_member_uses_a_map_carrier() { + let temp = tempfile::TempDir::new().expect("temporary output directory"); + let generated = generate(union_spec(), temp.path().join("generated")); + + // rustfmt wraps the alias across lines, so compare without whitespace. + let dense = generated + .chars() + .filter(|c| !c.is_whitespace()) + .collect::(); + assert!( + dense.contains( + "pubtypeUnconstrainedPayloadObject=std::collections::BTreeMap"), + "declared properties must still produce a struct:\n{generated}" + ); + assert!( + generated.contains("struct ClosedPayloadObject"), + "`additionalProperties: false` is a closed empty object, not a map:\n{generated}" + ); + assert!( + !generated.contains("pub type ClosedPayloadObject"), + "`additionalProperties: false` must not gain a map carrier:\n{generated}" + ); +} + +#[test] +fn generated_object_union_round_trips_arbitrary_keys() { + let temp = tempfile::TempDir::new().expect("temporary scratch crate"); + let output_dir = temp.path().join("src/generated"); + let mut analyzer = SchemaAnalyzer::new(union_spec()).expect("parse object union spec"); + let mut analysis = analyzer.analyze().expect("analyze object union spec"); + let generator = CodeGenerator::new(GeneratorConfig { + output_dir, + module_name: "object_union".into(), + enable_async_client: false, + enable_sse_client: false, + tracing_enabled: false, + ..Default::default() + }); + let result = generator + .generate_all(&mut analysis) + .expect("generate object union models"); + generator + .write_files(&result) + .expect("write object union models"); + + let dependency_fragment = + std::fs::read_to_string(temp.path().join("src/generated/REQUIRED_DEPS.toml")) + .expect("generated dependency fragment"); + std::fs::write( + temp.path().join("Cargo.toml"), + format!( + r#"[package] +name = "unconstrained-object-union-smoke" +version = "0.0.0" +edition = "2024" +publish = false + +{dependency_fragment} +"# + ), + ) + .expect("write scratch manifest"); + std::fs::write( + temp.path().join("src/lib.rs"), + r#"pub mod generated; + +#[cfg(test)] +mod tests { + use super::generated; + use serde_json::{Value, json}; + + fn round_trip(input: Value) { + let hydrated: generated::Unconstrained = + serde_json::from_value(input.clone()).expect("hydrate valid JSON"); + assert_eq!(serde_json::to_value(hydrated).unwrap(), input); + } + + #[test] + fn every_member_of_the_union_preserves_its_value() { + // gh#70: the object payload used to come back out as `{}`. + round_trip(json!({"payload": {"key": "value"}})); + round_trip(json!({"payload": {"nested": {"a": [1, 2]}, "b": null}})); + round_trip(json!({"payload": {}})); + round_trip(json!({"payload": "text"})); + round_trip(json!({"payload": 1.5})); + round_trip(json!({"payload": true})); + round_trip(json!({"payload": ["a", 1]})); + round_trip(json!({"payload": []})); + } + + #[test] + fn the_object_member_is_the_one_that_matches_an_object() { + let hydrated: generated::Unconstrained = + serde_json::from_value(json!({"payload": {"key": "value"}})).expect("hydrate object"); + assert!(matches!( + hydrated.payload, + Some(generated::UnconstrainedPayload::UnconstrainedPayloadObject(_)) + )); + } +} +"#, + ) + .expect("write scratch tests"); + + let output = Command::new("cargo") + .args(["test", "--quiet", "--offline"]) + .current_dir(temp.path()) + .env( + "CARGO_TARGET_DIR", + std::path::Path::new(env!("CARGO_MANIFEST_DIR")) + .join("target/unconstrained-object-union-smoke"), + ) + .output() + .expect("run generated object union round-trip tests"); + assert!( + output.status.success(), + "generated object union round-trip tests failed:\nstdout:\n{}\nstderr:\n{}", + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr) + ); +} From de1da0ef2c33a737bdf68f16b2354a0c30100c44 Mon Sep 17 00:00:00 2001 From: James Lal Date: Tue, 8 Sep 2026 07:34:38 -0600 Subject: [PATCH 2/3] fix: keep an opaque object union branch from claiming other branches MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `serde_json::Value` matches every JSON shape, so an untagged union branch typed that way claims values belonging to a later branch: with `anyOf: [{type: object}, {type: string}]` a JSON string deserialized into the object branch and never reached `String(String)`. Verified both ways against the compiled generated code — `Some(WVariant(String("hello")))` before, `Some(String("hello"))` after. A branch that *declares* `type: object` is not "any JSON", so narrow it to `BTreeMap`: equally lossless, and it matches only objects. Applied at `add_inline_union_branch_schema`, the single choke point every union-branch path funnels through. Only `UntypedReason::OpaqueObject` is narrowed. A branch that declares no type at all (`{}`, `true`, `{nullable: true}`) really does admit any JSON and keeps `serde_json::Value` — pinned as the negative case in the new test, because that distinction is the whole reason the reason code exists. `odata_nullable_reference_union_keeps_its_literal_object_branch` pinned the old spelling. Its intent — don't collapse the union, keep a dynamic branch — is unchanged; only the carrier moves. The explicit `null` that `{type: object, nullable: true}` admits is still carried by the field's own `Option`, verified before the assertion was touched. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_012mBAVbYp2iUSL8VwpyDf19 --- CHANGELOG.md | 8 ++ src/analysis.rs | 25 +++- tests/opaque_object_union_branch_test.rs | 160 +++++++++++++++++++++++ tests/recoverable_typing_test.rs | 8 +- 4 files changed, 199 insertions(+), 2 deletions(-) create mode 100644 tests/opaque_object_union_branch_test.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index ae5c379..45c62e4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,14 @@ when correcting output that was wrong or incomplete on the wire. ### Fixed +- A union branch that declares `type: object` no longer claims values that + belong to a later branch. `serde_json::Value` matches every JSON shape, so + with `anyOf: [{type: object}, {type: string}]` a JSON string deserialized + into the object branch and never reached `String(String)`. Such a branch now + carries a `BTreeMap`, which is equally lossless + and matches only objects. A branch that declares no type at all (`{}`, + `true`, `{nullable: true}`) genuinely admits any JSON and still generates + `serde_json::Value`. - The `object` member of a `type: [...]` union keeps arbitrary keys. All members of such a union share one schema, so the object member may carry no object shape at all; it was projected as a closed, empty struct, which deserialized diff --git a/src/analysis.rs b/src/analysis.rs index 3ecf3b1..1844c90 100644 --- a/src/analysis.rs +++ b/src/analysis.rs @@ -5374,7 +5374,30 @@ impl SchemaAnalyzer { discriminator, schema, ); - self.add_allocated_inline_schema(allocated_name, schema, dependencies) + let branch_name = self.add_allocated_inline_schema(allocated_name, schema, dependencies)?; + self.narrow_opaque_object_branch(&branch_name); + Ok(branch_name) + } + + /// `serde_json::Value` matches every JSON shape, so an untagged union + /// branch typed that way claims values that belong to a later branch: + /// with `anyOf: [{type: object}, {type: string}]` a JSON string + /// deserializes into the object branch. A branch that *declares* + /// `type: object` is not "any JSON" — narrow it to a map, which is + /// equally lossless and matches only objects. + /// + /// Only [`UntypedReason::OpaqueObject`] is narrowed. A branch that + /// declares no type at all (`{}`, `true`, `{nullable: true}`) really does + /// admit any JSON and must keep `serde_json::Value`. + fn narrow_opaque_object_branch(&mut self, branch_name: &str) { + if let Some(cached) = self.resolved_cache.get_mut(branch_name) + && let SchemaType::Untyped { + shape: shape @ UntypedShape::Value, + reason: UntypedReason::OpaqueObject, + } = &mut cached.schema_type + { + *shape = UntypedShape::ValueMap; + } } fn add_allocated_inline_schema( diff --git a/tests/opaque_object_union_branch_test.rs b/tests/opaque_object_union_branch_test.rs new file mode 100644 index 0000000..9afb16e --- /dev/null +++ b/tests/opaque_object_union_branch_test.rs @@ -0,0 +1,160 @@ +//! A union branch typed `serde_json::Value` matches every JSON shape, so it +//! claims values that belong to a later branch: with +//! `anyOf: [{type: object}, {type: string}]` a JSON string deserializes into +//! the object branch and never reaches `String(String)`. A branch that +//! declares `type: object` gets a map carrier instead — equally lossless, and +//! it matches only objects. +//! +//! The negative case is the point of the split: a branch that declares no type +//! at all really does admit any JSON and must keep `serde_json::Value`. + +use openapi_to_rust::{CodeGenerator, GeneratorConfig, SchemaAnalyzer}; +use serde_json::{Value, json}; +use std::process::Command; + +fn spec() -> Value { + json!({ + "openapi": "3.1.0", + "info": { "title": "opaque object branch", "version": "1.0.0" }, + "paths": {}, + "components": { "schemas": { + // The object branch is declared first, so it is the one serde tries + // first and the one that used to swallow the string. + "ObjectFirst": { + "type": "object", + "properties": { "p": { "anyOf": [ + { "type": "object" }, + { "type": "string" } + ] } } + }, + // No `type` at all: this branch is genuinely "any JSON". + "AnyBranch": { + "type": "object", + "properties": { "p": { "anyOf": [ + { "type": "string" }, + { } + ] } } + } + } } + }) +} + +fn generate(spec: Value, output_dir: std::path::PathBuf) -> String { + let mut analyzer = SchemaAnalyzer::new(spec).expect("parse branch spec"); + let mut analysis = analyzer.analyze().expect("analyze branch spec"); + CodeGenerator::new(GeneratorConfig { + output_dir, + module_name: "opaque_object_branch".into(), + enable_async_client: false, + enable_sse_client: false, + tracing_enabled: false, + ..Default::default() + }) + .generate(&mut analysis) + .expect("generate branch models") +} + +#[test] +fn an_object_branch_uses_a_map_and_an_untyped_branch_stays_a_value() { + let temp = tempfile::TempDir::new().expect("temporary output directory"); + let generated = generate(spec(), temp.path().join("generated")); + let dense = generated + .chars() + .filter(|c| !c.is_whitespace()) + .collect::(); + + assert!( + dense.contains("=std::collections::BTreeMap", "pub enum MemberUser", - "pub type MemberVariant2 = serde_json::Value", + "pub type MemberVariant2 = std::collections::BTreeMap<", ], ); } From 2105961b10913f2919333d75ed0bfdb1986ebab3 Mon Sep 17 00:00:00 2001 From: James Lal Date: Tue, 8 Sep 2026 08:06:37 -0600 Subject: [PATCH 3/3] fix: reject undeclared keys on a closed struct in union-branch position MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `#[serde(untagged)]` takes the first branch that deserializes, so a branch accepting more than its schema allows claims values belonging to a later one. A struct stating `additionalProperties: false` whose fields are all optional matched *any* object, won the branch ahead of the one that actually accepts the value, and dropped the extra keys — `{"user": {"free": "shape"}}` round-tripped as `{"user": {}}` even though the union's open branch was right there. Fixing it first required splitting a conflated model. `ObjectAdditionalProperties::Forbidden` meant both "the document stated `additionalProperties: false`" and "the keyword was omitted, so the generator projects a closed struct". Those are different facts, and across the 57-spec corpus they are 7,157 and 59,287 object schemas respectively — keying strictness on `Forbidden` would have made 59k JSON-Schema-*open* objects reject keys the spec permits. Split into `Denied` (stated) and `Closed` (projected); the 16 read sites that only care whether a catch-all field exists now say so via `is_open()`. `#[serde(deny_unknown_fields)]` is then emitted only when all three hold: the document stated it (`Denied`), the struct is reachable as an untagged union branch, and there is no flattened variant (serde rejects `deny_unknown_fields` alongside `flatten` at compile time). A closed struct outside any union has no branch to lose and stays tolerant, so generated clients keep working when a server adds a field. Measured, not assumed: 83 of 19,369 structs across six large corpus specs gain the attribute (0.4%; anthropic 41/450, stripe 0/4,984), zero of 19,019 scanned struct blocks pair it with `flatten`, and anthropic's generated module compiles. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_012mBAVbYp2iUSL8VwpyDf19 --- CHANGELOG.md | 10 + src/analysis.rs | 90 ++++++-- src/client_generator.rs | 4 +- src/generator.rs | 53 +++-- src/server/codegen.rs | 4 +- tests/closed_union_branch_strictness_test.rs | 218 ++++++++++++++++++ ..._additional_properties_constraints_test.rs | 2 +- tests/http_error_test.rs | 1 + tests/operation_generation_test.rs | 1 + 9 files changed, 344 insertions(+), 39 deletions(-) create mode 100644 tests/closed_union_branch_strictness_test.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index 45c62e4..d0c963b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,16 @@ when correcting output that was wrong or incomplete on the wire. ### Fixed +- A struct whose schema states `additionalProperties: false` rejects undeclared + keys when it is a branch of an untagged union. `#[serde(untagged)]` takes the + first branch that deserializes, so a closed struct whose fields are all + optional matched *any* object, won the branch ahead of the one that actually + accepts the value, and dropped the extra keys — `{"free": "shape"}` came back + as `{}`. Only a stated `additionalProperties: false` in union-branch position + is tightened: an omitted keyword leaves the object open in JSON Schema, and a + closed struct outside any union has no branch to lose, so both stay tolerant + of keys they do not declare and generated clients keep working when a server + adds a field. Across six large corpus specs this affects 83 of 19,369 structs. - A union branch that declares `type: object` no longer claims values that belong to a later branch. `serde_json::Value` matches every JSON shape, so with `anyOf: [{type: object}, {type: string}]` a JSON string deserialized diff --git a/src/analysis.rs b/src/analysis.rs index 1844c90..4f3bba0 100644 --- a/src/analysis.rs +++ b/src/analysis.rs @@ -3,7 +3,7 @@ use crate::type_mapping::TypeMapper; use crate::{GeneratorError, Result}; use serde::Deserialize; use serde_json::Value; -use std::collections::{BTreeMap, HashSet}; +use std::collections::{BTreeMap, BTreeSet, HashSet}; use std::path::Path; /// Q2.6 — pull `x-enum-varnames` / `x-enum-descriptions` arrays off @@ -101,6 +101,48 @@ pub struct SchemaAnalysis { /// This is deliberately independent of `schemas`, which model pruning may /// mutate before server artifacts are emitted. pub validation_context: ValidationContext, + /// Schemas reachable as a branch of an untagged union. + /// + /// `#[serde(untagged)]` tries branches in order and takes the first that + /// deserializes, so a branch that accepts more than its schema allows + /// claims values belonging to a later branch. A struct in this set whose + /// document states `additionalProperties: false` must therefore reject + /// undeclared keys; elsewhere leniency costs nothing and keeps generated + /// clients working when a server adds a field. + /// + /// Populated at the end of [`SchemaAnalyzer::analyze`], after inline + /// branch schemas exist. Side-channel keyed by analyzed-schema name so no + /// object constructor has to carry it. + pub untagged_union_branches: BTreeSet, +} + +/// Every schema reachable as a branch of a [`SchemaType::Union`], which the +/// generator always renders `#[serde(untagged)]`. +/// +/// A branch target may name a type alias rather than the struct itself +/// (`pub type Foo = Bar`), so [`SchemaType::Reference`] hops are followed — +/// the struct at the end is the one serde actually tries. +fn collect_untagged_union_branches(analysis: &SchemaAnalysis) -> BTreeSet { + let mut branches = BTreeSet::new(); + for schema in analysis.schemas.values() { + let SchemaType::Union { variants, .. } = &schema.schema_type else { + continue; + }; + for variant in variants { + let mut target = variant.target.clone(); + // A malformed document can point an alias at itself; the visited + // set keeps that from spinning. + let mut seen = BTreeSet::new(); + while seen.insert(target.clone()) { + branches.insert(target.clone()); + match analysis.schemas.get(&target).map(|s| &s.schema_type) { + Some(SchemaType::Reference { target: next }) => target = next.clone(), + _ => break, + } + } + } + } + branches } impl SchemaType { @@ -345,7 +387,7 @@ fn collect_untyped( findings, depth + 1, ), - ObjectAdditionalProperties::Forbidden => {} + ObjectAdditionalProperties::Denied | ObjectAdditionalProperties::Closed => {} } } SchemaType::Array { item_type } => { @@ -676,11 +718,17 @@ pub enum SchemaType { /// value-type schema instead of degrading to `serde_json::Value`. #[derive(Debug, Clone)] pub enum ObjectAdditionalProperties { - /// No catch-all field is emitted. This is exact for - /// `additionalProperties: false`; for an omitted keyword it is the - /// generator's historical closed-model projection and is used only while - /// no required unknown member forces an open carrier. - Forbidden, + /// `additionalProperties: false`: the document itself forbids extra keys. + /// No catch-all field is emitted, and because the rule is stated rather + /// than assumed the generated struct may also *reject* extra keys — which + /// it must, to stay distinguishable as an untagged union branch. + Denied, + /// The keyword is omitted. JSON Schema leaves such an object open; the + /// generator projects it closed (no catch-all field) while no required + /// unknown member forces an open carrier. Unlike [`Self::Denied`] this is + /// the generator's projection, not a rule the document stated, so the + /// struct stays tolerant of keys it does not declare. + Closed, /// `additionalProperties: true` — extra keys captured as /// `BTreeMap`. Untyped, @@ -693,7 +741,14 @@ impl ObjectAdditionalProperties { /// True when extra keys are accepted (regardless of typing). /// Used by callers that only care whether the field exists. pub fn is_open(&self) -> bool { - !matches!(self, Self::Forbidden) + !matches!(self, Self::Denied | Self::Closed) + } + + /// True when the *document* forbids extra keys, as opposed to the + /// generator merely projecting a closed struct. Only this case may + /// tighten the generated struct's own deserialization. + pub fn denies_unknown_keys(&self) -> bool { + matches!(self, Self::Denied) } } @@ -2079,6 +2134,7 @@ impl SchemaAnalyzer { used_type_features: crate::type_mapping::UsedFeatures::default(), enum_extensions: BTreeMap::new(), validation_context, + untagged_union_branches: BTreeSet::new(), }; // First pass: detect patterns @@ -2183,6 +2239,8 @@ impl SchemaAnalyzer { } } + analysis.untagged_union_branches = collect_untagged_union_branches(&analysis); + for schema in analysis.schemas.values_mut() { normalize_untyped(&mut schema.schema_type, 0); } @@ -3475,7 +3533,7 @@ impl SchemaAnalyzer { false, ), Some(crate::openapi::AdditionalProperties::Boolean(false)) => { - (ObjectAdditionalProperties::Forbidden, None, true) + (ObjectAdditionalProperties::Denied, None, true) } Some(crate::openapi::AdditionalProperties::Schema(value_schema)) if typed_enabled => @@ -3524,7 +3582,7 @@ impl SchemaAnalyzer { false, ), None => ( - ObjectAdditionalProperties::Forbidden, + ObjectAdditionalProperties::Closed, Some(untyped_required_property()), false, ), @@ -3619,7 +3677,7 @@ impl SchemaAnalyzer { properties.insert(name, required_additional_property.clone()); } - if matches!(additional_properties, ObjectAdditionalProperties::Forbidden) { + if !additional_properties.is_open() { *additional_properties = ObjectAdditionalProperties::Untyped; } Ok(()) @@ -4360,7 +4418,7 @@ impl SchemaAnalyzer { { ObjectAdditionalProperties::Untyped } else { - ObjectAdditionalProperties::Forbidden + ObjectAdditionalProperties::Closed }; self.finalize_required_object_members( &mut merged_properties, @@ -8666,9 +8724,7 @@ impl SchemaAnalyzer { else { return None; }; - if properties.is_empty() - || !matches!(additional_properties, ObjectAdditionalProperties::Forbidden) - { + if properties.is_empty() || additional_properties.is_open() { return None; } let mut projected = Vec::with_capacity(properties.len()); @@ -8764,9 +8820,7 @@ impl SchemaAnalyzer { else { return None; }; - if properties.is_empty() - || !matches!(additional_properties, ObjectAdditionalProperties::Forbidden) - { + if properties.is_empty() || additional_properties.is_open() { return None; } properties diff --git a/src/client_generator.rs b/src/client_generator.rs index 2115af9..20a1d04 100644 --- a/src/client_generator.rs +++ b/src/client_generator.rs @@ -1003,7 +1003,7 @@ impl CodeGenerator { operation: &OperationInfo, analysis: &SchemaAnalysis, ) -> Option { - use crate::analysis::{ObjectAdditionalProperties, RequestBodyContent, SchemaType}; + use crate::analysis::{RequestBodyContent, SchemaType}; let request_body = operation.request_body.as_ref()?; let (body_name, body_ident) = match request_body { @@ -1080,7 +1080,7 @@ impl CodeGenerator { if required_fields.is_empty() { RequiredBodyConstruction::Default } else if emitted.iter().any(|field| !field.is_required) - || !matches!(additional_properties, ObjectAdditionalProperties::Forbidden) + || additional_properties.is_open() { RequiredBodyConstruction::New(required_fields) } else { diff --git a/src/generator.rs b/src/generator.rs index 72b1f49..e8f89b0 100644 --- a/src/generator.rs +++ b/src/generator.rs @@ -2296,7 +2296,8 @@ impl CodeGenerator { // surfaces the actual schema-declared type, e.g. // BTreeMap. `Forbidden` emits no field. match additional_properties { - crate::analysis::ObjectAdditionalProperties::Forbidden => {} + crate::analysis::ObjectAdditionalProperties::Denied + | crate::analysis::ObjectAdditionalProperties::Closed => {} crate::analysis::ObjectAdditionalProperties::Untyped => { let serde_flatten = if variant.is_none() { quote! { #[serde(flatten)] } @@ -2385,6 +2386,30 @@ impl CodeGenerator { }, }; + // `#[serde(untagged)]` takes the first branch that deserializes, so a + // branch accepting more than its schema allows claims values that + // belong to a later one: with `anyOf: [Closed, {type: object}]` and a + // `Closed` whose fields are all optional, `{"other": 1}` matches + // `Closed` — which the document forbids — and the key is dropped + // instead of reaching the open branch. + // + // Only a *stated* `additionalProperties: false` earns this. An omitted + // keyword leaves the object open in JSON Schema (the generator merely + // projects it closed), and tightening those would make every generated + // client fail the first time a server adds a field. + // + // Serde rejects `deny_unknown_fields` alongside `flatten` at compile + // time; `Denied` emits no catch-all field, and a flattened variant is + // excluded here as well. + let denies_unknown_fields = variant.is_none() + && additional_properties.denies_unknown_keys() + && analysis.untagged_union_branches.contains(&schema.name); + let deny_unknown_fields = if denies_unknown_fields { + quote! { #[serde(deny_unknown_fields)] } + } else { + TokenStream::new() + }; + // `#[serde(flatten)]` removes keys already consumed by sibling fields // before it invokes the flattened value's deserializer. That is wrong // for a sibling-property + oneOf schema when both halves intentionally @@ -2445,7 +2470,8 @@ impl CodeGenerator { .collect(); match additional_properties { - crate::analysis::ObjectAdditionalProperties::Forbidden => {} + crate::analysis::ObjectAdditionalProperties::Denied + | crate::analysis::ObjectAdditionalProperties::Closed => {} crate::analysis::ObjectAdditionalProperties::Untyped => { helper_fields.push(quote! { #[serde(flatten)] @@ -2554,10 +2580,8 @@ impl CodeGenerator { && (emitted_properties .iter() .any(|property| !property.is_required) - || !matches!( - additional_properties, - crate::analysis::ObjectAdditionalProperties::Forbidden - )) { + || additional_properties.is_open()) + { self.generate_request_model_builder( schema, &emitted_properties, @@ -2572,6 +2596,7 @@ impl CodeGenerator { Ok(quote! { #doc_comment #derives + #deny_unknown_fields pub struct #struct_name { #(#fields)* } @@ -2597,10 +2622,7 @@ impl CodeGenerator { sorted_properties.sort_by_key(|(name, _)| name.as_str()); let mut used_field_idents = std::collections::HashSet::new(); - if !matches!( - additional_properties, - crate::analysis::ObjectAdditionalProperties::Forbidden - ) { + if additional_properties.is_open() { used_field_idents.insert("additional_properties".to_string()); } @@ -2708,7 +2730,8 @@ impl CodeGenerator { .collect(); let additional_initializer = match additional_properties { - crate::analysis::ObjectAdditionalProperties::Forbidden => TokenStream::new(), + crate::analysis::ObjectAdditionalProperties::Denied + | crate::analysis::ObjectAdditionalProperties::Closed => TokenStream::new(), crate::analysis::ObjectAdditionalProperties::Untyped | crate::analysis::ObjectAdditionalProperties::Typed { .. } => quote! { additional_properties: ::std::collections::BTreeMap::new(), @@ -2717,10 +2740,7 @@ impl CodeGenerator { let mut used_builder_methods = std::collections::HashSet::from(["new".to_string(), "build".to_string()]); - if !matches!( - additional_properties, - crate::analysis::ObjectAdditionalProperties::Forbidden - ) { + if additional_properties.is_open() { used_builder_methods.insert("additional_properties".to_string()); } let optional_setters: Vec = properties @@ -2812,7 +2832,8 @@ impl CodeGenerator { .collect(); let additional_setter = match additional_properties { - crate::analysis::ObjectAdditionalProperties::Forbidden => TokenStream::new(), + crate::analysis::ObjectAdditionalProperties::Denied + | crate::analysis::ObjectAdditionalProperties::Closed => TokenStream::new(), crate::analysis::ObjectAdditionalProperties::Untyped => quote! { /// Replace the request's additional properties. #[must_use] diff --git a/src/server/codegen.rs b/src/server/codegen.rs index 4c47f3f..e7e33ef 100644 --- a/src/server/codegen.rs +++ b/src/server/codegen.rs @@ -692,7 +692,7 @@ impl<'a> ServerCodegen<'a> { reason: "only flat object schemas are supported".to_string(), }); }; - if !matches!(additional_properties, ObjectAdditionalProperties::Forbidden) + if additional_properties.is_open() || properties.values().any(|property| { !self.query_property_is_scalar( &property.schema_type, @@ -998,7 +998,7 @@ impl<'a> ServerCodegen<'a> { ))); } }; - if !matches!(additional_properties, ObjectAdditionalProperties::Forbidden) { + if additional_properties.is_open() { return Err(error( "styled object parameters with additionalProperties have an ambiguous wire namespace" .to_string(), diff --git a/tests/closed_union_branch_strictness_test.rs b/tests/closed_union_branch_strictness_test.rs new file mode 100644 index 0000000..bbe5bd4 --- /dev/null +++ b/tests/closed_union_branch_strictness_test.rs @@ -0,0 +1,218 @@ +//! `#[serde(untagged)]` takes the first branch that deserializes, so a branch +//! that accepts more than its schema allows claims values belonging to a later +//! branch. A struct whose document states `additionalProperties: false` but +//! whose fields are all optional matches *any* object, wins the branch, and +//! drops the keys — `{"free": "shape"}` came back as `{}`. +//! +//! The negatives are the whole design. An omitted `additionalProperties` +//! leaves the object open in JSON Schema, and a closed struct outside any +//! union has no branch to lose, so neither is tightened: strict parsing there +//! would only make generated clients fail when a server adds a field. + +use openapi_to_rust::{CodeGenerator, GeneratorConfig, SchemaAnalyzer}; +use serde_json::{Value, json}; +use std::process::Command; + +fn spec() -> Value { + json!({ + "openapi": "3.1.0", + "info": { "title": "closed branch", "version": "1.0.0" }, + "paths": {}, + "components": { "schemas": { + // States `additionalProperties: false` and is a union branch: the + // one shape that must reject undeclared keys. + "ClosedBranch": { + "type": "object", "additionalProperties": false, + "properties": { "id": { "type": "string" } } + }, + // States nothing about extra keys, and is a union branch. JSON + // Schema leaves it open, so it must stay tolerant. + "OpenBranch": { + "type": "object", + "properties": { "name": { "type": "string" } } + }, + // Closed, but never a union branch: nothing to lose, stays + // tolerant so an added server field does not break the client. + "ClosedStandalone": { + "type": "object", "additionalProperties": false, + "properties": { "id": { "type": "string" } } + }, + "Holder": { + "type": "object", + "properties": { + "closed": { "anyOf": [ + { "$ref": "#/components/schemas/ClosedBranch" }, + { "type": "object" } + ] }, + "open": { "anyOf": [ + { "$ref": "#/components/schemas/OpenBranch" }, + { "type": "string" } + ] }, + "standalone": { "$ref": "#/components/schemas/ClosedStandalone" } + } + } + } } + }) +} + +fn generate(output_dir: std::path::PathBuf) -> String { + let mut analyzer = SchemaAnalyzer::new(spec()).expect("parse closed branch spec"); + let mut analysis = analyzer.analyze().expect("analyze closed branch spec"); + CodeGenerator::new(GeneratorConfig { + output_dir, + module_name: "closed_branch".into(), + enable_async_client: false, + enable_sse_client: false, + tracing_enabled: false, + ..Default::default() + }) + .generate(&mut analysis) + .expect("generate closed branch models") +} + +/// The attribute lands on exactly one of the three structs. +#[test] +fn only_a_stated_closed_union_branch_denies_unknown_fields() { + let temp = tempfile::TempDir::new().expect("temporary output directory"); + let generated = generate(temp.path().join("generated")); + + let denied = |name: &str| { + let anchor = format!("pub struct {name} "); + let at = generated + .find(&anchor) + .unwrap_or_else(|| panic!("`{name}` must be generated:\n{generated}")); + generated[..at] + .rsplit("#[derive") + .next() + .unwrap_or_default() + .contains("deny_unknown_fields") + }; + + assert!( + denied("ClosedBranch"), + "a stated `additionalProperties: false` in union-branch position must \ + reject undeclared keys:\n{generated}" + ); + assert!( + !denied("OpenBranch"), + "an omitted `additionalProperties` leaves the object open in JSON \ + Schema and must stay tolerant:\n{generated}" + ); + assert!( + !denied("ClosedStandalone"), + "a closed struct outside any union has no branch to lose and must stay \ + tolerant:\n{generated}" + ); + assert!( + !denied("Holder"), + "the enclosing object declares nothing about extra keys:\n{generated}" + ); +} + +#[test] +fn generated_unions_route_unknown_keys_to_the_branch_that_accepts_them() { + let temp = tempfile::TempDir::new().expect("temporary scratch crate"); + let mut analyzer = SchemaAnalyzer::new(spec()).expect("parse closed branch spec"); + let mut analysis = analyzer.analyze().expect("analyze closed branch spec"); + let generator = CodeGenerator::new(GeneratorConfig { + output_dir: temp.path().join("src/generated"), + module_name: "closed_branch".into(), + enable_async_client: false, + enable_sse_client: false, + tracing_enabled: false, + ..Default::default() + }); + let result = generator + .generate_all(&mut analysis) + .expect("generate closed branch models"); + generator.write_files(&result).expect("write models"); + + let dependency_fragment = + std::fs::read_to_string(temp.path().join("src/generated/REQUIRED_DEPS.toml")) + .expect("generated dependency fragment"); + std::fs::write( + temp.path().join("Cargo.toml"), + format!( + r#"[package] +name = "closed-branch-smoke" +version = "0.0.0" +edition = "2024" +publish = false + +{dependency_fragment} +"# + ), + ) + .expect("write scratch manifest"); + std::fs::write( + temp.path().join("src/lib.rs"), + r#"pub mod generated; + +#[cfg(test)] +mod tests { + use super::generated; + use serde_json::json; + + #[test] + fn an_undeclared_key_reaches_the_open_branch_and_survives() { + let input = json!({"closed": {"free": "shape"}}); + let hydrated: generated::Holder = + serde_json::from_value(input.clone()).expect("hydrate undeclared key"); + assert_eq!( + serde_json::to_value(hydrated).unwrap(), + input, + "the key must reach the open branch instead of being dropped by the closed one" + ); + } + + #[test] + fn the_closed_branch_still_wins_the_shape_it_declares() { + let hydrated: generated::Holder = + serde_json::from_value(json!({"closed": {"id": "x"}})).expect("hydrate declared shape"); + assert!(matches!( + hydrated.closed, + Some(generated::HolderClosed::ClosedBranch(_)) + )); + } + + #[test] + fn a_tolerant_branch_still_absorbs_extra_keys_as_before() { + // `OpenBranch` omits `additionalProperties`, so it keeps matching an + // object carrying keys it does not declare. + let hydrated: generated::Holder = + serde_json::from_value(json!({"open": {"name": "n", "extra": 1}})) + .expect("hydrate open branch"); + assert!(matches!( + hydrated.open, + Some(generated::HolderOpen::OpenBranch(_)) + )); + } + + #[test] + fn a_closed_standalone_struct_still_tolerates_a_new_server_field() { + let hydrated: generated::ClosedStandalone = + serde_json::from_value(json!({"id": "x", "addedLater": true})) + .expect("a closed struct outside a union must not hard-fail"); + assert_eq!(hydrated.id.as_deref(), Some("x")); + } +} +"#, + ) + .expect("write scratch tests"); + + let output = Command::new("cargo") + .args(["test", "--quiet", "--offline"]) + .current_dir(temp.path()) + .env( + "CARGO_TARGET_DIR", + std::path::Path::new(env!("CARGO_MANIFEST_DIR")).join("target/closed-branch-smoke"), + ) + .output() + .expect("run generated branch tests"); + assert!( + output.status.success(), + "generated branch routing tests failed:\nstdout:\n{}\nstderr:\n{}", + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr) + ); +} diff --git a/tests/dynamic_additional_properties_constraints_test.rs b/tests/dynamic_additional_properties_constraints_test.rs index 762b759..f8104e4 100644 --- a/tests/dynamic_additional_properties_constraints_test.rs +++ b/tests/dynamic_additional_properties_constraints_test.rs @@ -92,7 +92,7 @@ fn constraints_and_examples_promote_omitted_additional_properties_to_a_carrier() } assert!(matches!( additional_properties(&analysis, "Closed"), - ObjectAdditionalProperties::Forbidden + ObjectAdditionalProperties::Denied )); assert!(matches!( additional_properties(&analysis, "Typed"), diff --git a/tests/http_error_test.rs b/tests/http_error_test.rs index 1af2fb3..ad2f3e5 100644 --- a/tests/http_error_test.rs +++ b/tests/http_error_test.rs @@ -368,6 +368,7 @@ fn test_generated_error_code() { used_type_features: Default::default(), enum_extensions: BTreeMap::new(), validation_context: Default::default(), + untagged_union_branches: Default::default(), }; // Generate HTTP client code which includes error types diff --git a/tests/operation_generation_test.rs b/tests/operation_generation_test.rs index f69280b..9c3b96c 100644 --- a/tests/operation_generation_test.rs +++ b/tests/operation_generation_test.rs @@ -34,6 +34,7 @@ fn create_test_analysis_with_operations(operations: Vec) -> Schem used_type_features: Default::default(), enum_extensions: BTreeMap::new(), validation_context: Default::default(), + untagged_union_branches: Default::default(), } }