Skip to content

fix(generator): leave union-declared required names to the variant #103

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
lightsofapollo merged 2 commits into from
Oct 12, 2026
Merged

fix(generator): leave union-declared required names to the variant #103

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
16a14c5
fix(generator): leave union-declared required names to the variant
gilesw Oct 12, 2026
c987af6
docs: changelog entry for union-declared required names
gilesw Oct 12, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
78 changes: 75 additions & 3 deletions src/analysis.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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<HashSet<String>> {
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<SchemaRef>,
candidate: SchemaRef,
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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,
Expand Down
136 changes: 136 additions & 0 deletions tests/allof_object_union_preservation_test.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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,
..
Expand Down Expand Up @@ -272,6 +377,35 @@ where
assert_eq!(serde_json::to_value(hydrated_again).unwrap(), output);
}

fn round_trip_records<T>()
where
T: serde::de::DeserializeOwned + serde::Serialize,
{
round_trip::<T>(serde_json::json!({
"id": "r1",
"name": "www.example.com",
"type": "A",
"content": "198.51.100.4",
"ttl": 1.0
}));
round_trip::<T>(serde_json::json!({
"id": "r2",
"name": "example.com",
"type": "MX",
"content": "mail.example.com",
"priority": 10.0,
"ttl": 3600.0
}));
round_trip::<T>(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::<generated::RuleFeature>(serde_json::json!({
"name": "card",
Expand Down Expand Up @@ -303,6 +437,8 @@ fn main() {
"root_value": "root",
"child_value": 7
}));
round_trip_records::<generated::RecordResponse>();
round_trip_records::<generated::SiblingRecordResponse>();
round_trip::<generated::GitpodOutputSpec>(serde_json::json!({
"key": "coverage",
"command": "collect-coverage",
Expand Down
4 changes: 2 additions & 2 deletions tests/corpus-manifest.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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