diff --git a/CHANGELOG.md b/CHANGELOG.md index cb10eeb..63cd447 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 diff --git a/src/analysis.rs b/src/analysis.rs index 3509e0b..a0a460c 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 395ec94..6ce2704 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 49f5af8..19888d5 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