From 4a058a927ebcea6a1c5f9daccac1394aa688e139 Mon Sep 17 00:00:00 2001 From: James Lal Date: Tue, 8 Sep 2026 07:02:38 -0600 Subject: [PATCH] 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) + ); +}