From 16a14c544e01c273f4b76dbbcf40856787afc8b3 Mon Sep 17 00:00:00 2001 From: Giles Westwood <201443+gilesw@users.noreply.github.com> Date: Mon, 12 Oct 2026 10:43:35 +0100 Subject: [PATCH 1/2] fix(generator): leave union-declared required names to the variant An object that pairs shared fields with a union, through allOf or as sibling properties and anyOf/oneOf, synthesized a required serde_json::Value field for every required name absent from the shared properties, even when the union's branches declare it. Cloudflare's dns-records_dns-record-response lists `data` as required though only CAA, SRV and similar record types carry it, so A and MX records failed to deserialize with "missing field `data`", and name, content and ttl were untyped. Required names declared by any branch reachable through the union are now left to the variant, which types them and keeps each branch's own optionality. Names declared nowhere keep the existing placeholder. The corpus change is confined to Cloudflare's DNS record response types. --- src/analysis.rs | 78 +++++++++- tests/allof_object_union_preservation_test.rs | 136 ++++++++++++++++++ tests/corpus-manifest.txt | 4 +- 3 files changed, 213 insertions(+), 5 deletions(-) diff --git a/src/analysis.rs b/src/analysis.rs index 3509e0be..a0a460c3 100644 --- a/src/analysis.rs +++ b/src/analysis.rs @@ -4456,6 +4456,15 @@ impl SchemaAnalyzer { // placeholder depending on branch order. if !merged_properties.is_empty() || !merged_required.is_empty() || merged_variant.is_some() { + // A required name declared only by the union's branches belongs to + // the variant, which types it and decides per branch whether it + // must be present. + if let Some(variant) = &merged_variant { + let declared = self.union_declared_property_names(&variant.target)?; + merged_required.retain(|name| { + merged_properties.contains_key(name) || !declared.contains(name) + }); + } let mut additional_properties = if merged_properties .values() .any(|property| property.synthesized_required) @@ -4637,6 +4646,50 @@ impl SchemaAnalyzer { } } + /// Property names declared by any object reachable through the branches + /// of an analyzed union, including nested unions and flattened variants. + fn union_declared_property_names(&mut self, target: &str) -> Result> { + let mut names = HashSet::new(); + let mut visited = HashSet::new(); + let mut pending = vec![target.to_string()]; + while let Some(target) = pending.pop() { + if !visited.insert(target.clone()) { + continue; + } + let schema_type = if let Some(analyzed) = self.resolved_cache.get(&target) { + analyzed.schema_type.clone() + } else if self.schemas.contains_key(&target) { + self.analyze_schema(&target)?.schema_type + } else { + continue; + }; + match &schema_type { + SchemaType::Object { + properties, + variant, + .. + } => { + names.extend(properties.keys().cloned()); + pending.extend(variant.iter().map(|variant| variant.target.clone())); + } + SchemaType::Union { variants, .. } => { + pending.extend(variants.iter().map(|variant| variant.target.clone())); + } + SchemaType::DiscriminatedUnion { + discriminator_field, + variants, + .. + } => { + names.insert(discriminator_field.clone()); + pending.extend(variants.iter().map(|variant| variant.type_name.clone())); + } + SchemaType::Reference { target } => pending.push(target.clone()), + _ => {} + } + } + Ok(names) + } + fn merge_allof_variant( merged_variant: &mut Option, candidate: SchemaRef, @@ -6314,9 +6367,9 @@ impl SchemaAnalyzer { let base = self.analyze_object_schema(schema, dependencies)?; let SchemaType::Object { - properties, - required, - additional_properties, + mut properties, + mut required, + mut additional_properties, .. } = base else { @@ -6361,6 +6414,25 @@ impl SchemaAnalyzer { ); dependencies.insert(variant_name.clone()); + // A required name declared only by the branches belongs to the variant, + // not to a placeholder on the base. + let declared = self.union_declared_property_names(&variant_name)?; + properties.retain(|name, property| { + let owned_by_variant = property.synthesized_required && declared.contains(name); + if owned_by_variant { + required.remove(name); + } + !owned_by_variant + }); + if details.additional_properties.is_none() + && !Self::object_shape_needs_additional_property_carrier(details) + && !properties + .values() + .any(|property| property.synthesized_required) + { + additional_properties = ObjectAdditionalProperties::Closed; + } + Ok(Some(SchemaType::Object { properties, required, diff --git a/tests/allof_object_union_preservation_test.rs b/tests/allof_object_union_preservation_test.rs index 395ec945..6ce2704a 100644 --- a/tests/allof_object_union_preservation_test.rs +++ b/tests/allof_object_union_preservation_test.rs @@ -102,6 +102,92 @@ fn object_union_allof_spec() -> Value { } ] }, + "RecordSharedFields": { + "type": "object", + "properties": { + "name": { "type": "string" }, + "ttl": { "type": "number" } + } + }, + "ARecord": { + "allOf": [ + { "$ref": "#/components/schemas/RecordSharedFields" }, + { + "type": "object", + "properties": { + "content": { "type": "string" }, + "type": { "type": "string", "enum": ["A"] } + } + } + ] + }, + "MXRecord": { + "allOf": [ + { "$ref": "#/components/schemas/RecordSharedFields" }, + { + "type": "object", + "properties": { + "content": { "type": "string" }, + "priority": { "type": "number" }, + "type": { "type": "string", "enum": ["MX"] } + } + } + ] + }, + "CAARecord": { + "allOf": [ + { "$ref": "#/components/schemas/RecordSharedFields" }, + { + "type": "object", + "properties": { + "content": { "type": "string" }, + "data": { + "type": "object", + "properties": { + "flags": { "type": "number" }, + "tag": { "type": "string" } + } + }, + "type": { "type": "string", "enum": ["CAA"] } + } + } + ] + }, + "RecordWithoutData": { + "oneOf": [ + { "$ref": "#/components/schemas/ARecord" }, + { "$ref": "#/components/schemas/MXRecord" } + ] + }, + "RecordWithData": { + "oneOf": [{ "$ref": "#/components/schemas/CAARecord" }] + }, + "RecordResponse": { + "type": "object", + "required": ["name", "type", "content", "data", "ttl"], + "allOf": [ + { + "anyOf": [ + { "$ref": "#/components/schemas/RecordWithoutData" }, + { "$ref": "#/components/schemas/RecordWithData" } + ] + }, + { + "type": "object", + "required": ["id"], + "properties": { "id": { "type": "string" } } + } + ] + }, + "SiblingRecordResponse": { + "type": "object", + "required": ["name", "type", "content", "data", "ttl", "id"], + "properties": { "id": { "type": "string" } }, + "anyOf": [ + { "$ref": "#/components/schemas/RecordWithoutData" }, + { "$ref": "#/components/schemas/RecordWithData" } + ] + }, "GitpodOutputSpec": { "type": "object", "additionalProperties": false, @@ -198,6 +284,25 @@ fn allof_objects_retain_inline_referenced_and_root_sibling_shapes() { assert!(required.contains("root_value")); assert!(required.contains("child_value")); + for name in ["RecordResponse", "SiblingRecordResponse"] { + let SchemaType::Object { + properties: record, + variant, + .. + } = &analysis.schemas[name].schema_type + else { + panic!("{name} should be an object with a flattened union"); + }; + assert!(variant.is_some(), "{name} must retain its union member"); + assert!(record.contains_key("id")); + for field in ["name", "type", "content", "data", "ttl"] { + assert!( + !record.contains_key(field), + "{name}.{field} is declared by the union branches, not the shared fields" + ); + } + } + let SchemaType::Object { properties: output_spec, .. @@ -272,6 +377,35 @@ where assert_eq!(serde_json::to_value(hydrated_again).unwrap(), output); } +fn round_trip_records() +where + T: serde::de::DeserializeOwned + serde::Serialize, +{ + round_trip::(serde_json::json!({ + "id": "r1", + "name": "www.example.com", + "type": "A", + "content": "198.51.100.4", + "ttl": 1.0 + })); + round_trip::(serde_json::json!({ + "id": "r2", + "name": "example.com", + "type": "MX", + "content": "mail.example.com", + "priority": 10.0, + "ttl": 3600.0 + })); + round_trip::(serde_json::json!({ + "id": "r3", + "name": "example.com", + "type": "CAA", + "content": "0 issue \"letsencrypt.org\"", + "data": { "flags": 0.0, "tag": "issue" }, + "ttl": 1.0 + })); +} + fn main() { round_trip::(serde_json::json!({ "name": "card", @@ -303,6 +437,8 @@ fn main() { "root_value": "root", "child_value": 7 })); + round_trip_records::(); + round_trip_records::(); round_trip::(serde_json::json!({ "key": "coverage", "command": "collect-coverage", diff --git a/tests/corpus-manifest.txt b/tests/corpus-manifest.txt index 49f5af8c..19888d52 100644 --- a/tests/corpus-manifest.txt +++ b/tests/corpus-manifest.txt @@ -49,7 +49,7 @@ circleci/types.rs 384614 10072 6f27708 cloudflare/REQUIRED_DEPS.toml 787 19 09cccc57db93029b cloudflare/client.rs 14164566 369093 263c089be637fd2b cloudflare/mod.rs 446 17 ce0e733f646147da -cloudflare/types.rs 16794350 390250 a2c8ec538e83083e +cloudflare/types.rs 16786215 390040 49b29a93e8d06f10 coda/REQUIRED_DEPS.toml 672 16 9e666f39c7301c02 coda/client.rs 848417 21853 bfee52292599720a coda/mod.rs 434 17 4e8b99caec52b623 @@ -235,4 +235,4 @@ writer/client.rs 191302 5007 33061e5 writer/mod.rs 438 17 0435bf1552e493d1 writer/types.rs 198394 4888 800dd8afe5798530 # -# specs: 56 files: 224 bytes: 330621909 +# specs: 56 files: 224 bytes: 330613774 From c987af633dbe24a1e6bf9e70430ca648fa976771 Mon Sep 17 00:00:00 2001 From: Giles Westwood <201443+gilesw@users.noreply.github.com> Date: Mon, 12 Oct 2026 10:43:35 +0100 Subject: [PATCH 2/2] docs: changelog entry for union-declared required names --- CHANGELOG.md | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index cb10eeba..63cd447c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,16 @@ when correcting output that was wrong or incomplete on the wire. ## [Unreleased] +### Fixed + +- An object that pairs shared fields with a union, through `allOf` or as + sibling `properties` and `anyOf`/`oneOf`, no longer turns a `required` name + declared only by the union's branches into a required `serde_json::Value` + field on the shared struct. The branch keeps the typed field and its own + optionality. Cloudflare's DNS record responses list `data` as required + although only some record types carry it, so A and MX records failed to + deserialize, and `name`, `content` and `ttl` were untyped. + ## [0.22.0] - 2026-10-06 ### Added