diff --git a/CHANGELOG.md b/CHANGELOG.md index 55072e1..b934459 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -30,6 +30,16 @@ when correcting output that was wrong or incomplete on the wire. 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. +- An `allOf` whose members repeat the same union, differing only in + annotations such as `description`, resolves to that union instead of failing + with "intersects multiple union members". The current Cloudflare spec + restates its Workers Observability filter union inline next to a `$ref` to + it, which stopped the whole spec from generating. Members that are different + unions still fail with that diagnostic. Assertion data such as + `dependentRequired` is preserved, and `type: object` is ignored only when + every union branch explicitly requires an object. Object types, fields and + required names declared beside `allOf` survive when deduplication leaves a + single member. ## [0.22.0] - 2026-10-06 diff --git a/src/analysis.rs b/src/analysis.rs index a0a460c..83adfa5 100644 --- a/src/analysis.rs +++ b/src/analysis.rs @@ -1415,6 +1415,63 @@ fn schema_is_annotation_only(schema: &crate::openapi::Schema) -> bool { }) } +fn is_annotation_keyword(key: &str) -> bool { + matches!( + key, + "title" + | "description" + | "deprecated" + | "readOnly" + | "writeOnly" + | "examples" + | "example" + | "externalDocs" + | "xml" + | "$comment" + ) || key.starts_with("x-") +} + +/// Remove annotation keywords from a schema and its subschemas, leaving the +/// assertions. Member names under `properties` and similar maps are kept. +fn strip_schema_annotations(value: &mut Value) { + let Some(object) = value.as_object_mut() else { + return; + }; + object.retain(|key, _| !is_annotation_keyword(key)); + for (key, child) in object.iter_mut() { + match key.as_str() { + "properties" | "patternProperties" | "$defs" | "definitions" | "dependentSchemas" => { + if let Some(members) = child.as_object_mut() { + members.values_mut().for_each(strip_schema_annotations); + } + } + "allOf" | "anyOf" | "oneOf" | "prefixItems" => { + if let Some(items) = child.as_array_mut() { + items.iter_mut().for_each(strip_schema_annotations); + } + } + "items" => match child { + Value::Array(items) => items.iter_mut().for_each(strip_schema_annotations), + _ => strip_schema_annotations(child), + }, + "additionalProperties" + | "additionalItems" + | "unevaluatedProperties" + | "unevaluatedItems" + | "contains" + | "propertyNames" + | "not" + | "if" + | "then" + | "else" + | "contentSchema" => strip_schema_annotations(child), + // Other keywords contain data, not schemas (notably dependentRequired). + // Preserve unknown keywords conservatively as well. + _ => {} + } + } +} + /// Load an extension file and parse it into the JSON representation used by /// the analyzer. YAML extensions follow the same conversion policy as YAML /// OpenAPI documents; every other extension is parsed as JSON. @@ -4278,6 +4335,24 @@ impl SchemaAnalyzer { all_of_schemas: &[Schema], dependencies: &mut HashSet, ) -> Result { + let without_restated = self.without_restated_unions(all_of_schemas); + let all_of_schemas = without_restated.as_deref().unwrap_or(all_of_schemas); + + // After deduplication, only an owner with no assertions beside allOf + // can collapse to a bare member. Otherwise keep the merge path, retaining + // the owner's object shape and fields. + let preserve_owner_shape = without_restated.is_some() + && !serde_json::to_value(owner_schema) + .ok() + .and_then(|value| value.as_object().cloned()) + .is_some_and(|owner| { + owner.keys().all(|key| { + key == "allOf" + || is_annotation_keyword(key) + || matches!(key.as_str(), "default" | "nullable") + }) + }); + // A scalar/array carrier intersected only with annotation-only // siblings keeps its wire type. This is deliberately narrower than // "pick the first non-object": multiple carriers or assertion-bearing @@ -4298,7 +4373,8 @@ impl SchemaAnalyzer { let only_reference_and_annotations = all_of_schemas .iter() .all(|schema| schema.reference().is_some() || schema_is_annotation_only(schema)); - if referenced_targets.len() == 1 && only_reference_and_annotations { + if !preserve_owner_shape && referenced_targets.len() == 1 && only_reference_and_annotations + { let target = referenced_targets[0]; dependencies.insert(target.to_string()); return Ok(SchemaType::Reference { @@ -4309,7 +4385,8 @@ impl SchemaAnalyzer { // A single member composes with nothing: `allOf: [{type: string}]` is // that string. Specs write it to hang a description off a scalar or a // `$ref`, and merging it as an object would lose the type entirely. - if let [only] = all_of_schemas + if !preserve_owner_shape + && let [only] = all_of_schemas && !matches!( only.schema_type(), Some(OpenApiSchemaType::Object) | Some(OpenApiSchemaType::Null) @@ -4646,6 +4723,80 @@ impl SchemaAnalyzer { } } + /// Drop `allOf` members that repeat an earlier member's union, compared + /// without annotations. Returns `None` when nothing repeats. + fn without_restated_unions(&self, members: &[Schema]) -> Option> { + let mut seen = Vec::new(); + let mut kept = Vec::with_capacity(members.len()); + for member in members { + if let Some(signature) = self.union_signature(member) { + if seen.contains(&signature) { + continue; + } + seen.push(signature); + } + kept.push(member.clone()); + } + (kept.len() < members.len()).then_some(kept) + } + + /// The assertions of a member that is only a union, following one `$ref`. + /// `type: object` is ignored only when every branch already requires an + /// object, so a named object union can match an inline restatement. + fn union_signature(&self, member: &Schema) -> Option { + let schema = match member.reference() { + Some(reference) => self.schemas.get(self.extract_schema_name(reference)?)?, + None => member, + }; + let mut value = serde_json::to_value(schema).ok()?; + strip_schema_annotations(&mut value); + let object = value.as_object_mut()?; + if object.get("type").and_then(Value::as_str) == Some("object") + && ["anyOf", "oneOf"].iter().any(|keyword| { + object + .get(*keyword) + .and_then(Value::as_array) + .is_some_and(|branches| { + !branches.is_empty() + && branches + .iter() + .all(|branch| self.union_branch_requires_object(branch)) + }) + }) + { + object.remove("type"); + } + let is_union = object.contains_key("anyOf") || object.contains_key("oneOf"); + let only_union = object + .keys() + .all(|key| matches!(key.as_str(), "anyOf" | "oneOf" | "discriminator")); + (is_union && only_union).then_some(value) + } + + /// A conservative proof: require an explicit object type, optionally + /// following one reference. Do not infer a type from object-only keywords + /// such as properties/required, which also accept scalars in JSON Schema. + fn union_branch_requires_object(&self, branch: &Value) -> bool { + let resolved; + let branch = if let Some(reference) = branch.get("$ref").and_then(Value::as_str) { + let Some(schema) = self + .extract_schema_name(reference) + .and_then(|name| self.schemas.get(name)) + else { + return false; + }; + let Ok(value) = serde_json::to_value(schema) else { + return false; + }; + resolved = value; + &resolved + } else { + branch + }; + branch.get("type").and_then(Value::as_str) == Some("object") + && branch.get("nullable").and_then(Value::as_bool) != Some(true) + } + /// 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> { diff --git a/tests/allof_object_union_preservation_test.rs b/tests/allof_object_union_preservation_test.rs index 6ce2704..0a958c7 100644 --- a/tests/allof_object_union_preservation_test.rs +++ b/tests/allof_object_union_preservation_test.rs @@ -1,9 +1,11 @@ +#![recursion_limit = "256"] + use openapi_to_rust::analysis::SchemaType; use openapi_to_rust::{CodeGenerator, GeneratorConfig, SchemaAnalyzer}; use serde_json::{Value, json}; fn object_union_allof_spec() -> Value { - json!({ + let mut spec = json!({ "openapi": "3.1.0", "info": { "title": "object union allOf", "version": "1" }, "paths": {}, @@ -188,6 +190,61 @@ fn object_union_allof_spec() -> Value { { "$ref": "#/components/schemas/RecordWithData" } ] }, + "FilterLeaf": { + "type": "object", + "required": ["key", "operation"], + "properties": { + "key": { "type": "string" }, + "operation": { "type": "string" }, + "value": { "type": "string" } + } + }, + "FilterNode": { + "description": "A leaf filter or a nested group.", + "type": "object", + "anyOf": [ + { + "type": "object", + "required": ["kind", "filters"], + "properties": { + "kind": { "type": "string", "enum": ["group"] }, + "filters": { + "type": "array", + "items": { "$ref": "#/components/schemas/FilterNode" } + } + } + }, + { "$ref": "#/components/schemas/FilterLeaf" } + ] + }, + "RestatedFilter": { + "allOf": [ + { "$ref": "#/components/schemas/FilterNode" }, + { + "description": "Supports nested groups.", + "anyOf": [ + { + "description": "A group of child filters.", + "type": "object", + "required": ["kind", "filters"], + "properties": { + "kind": { + "description": "Marks a group.", + "type": "string", + "enum": ["group"] + }, + "filters": { + "description": "Child filters.", + "type": "array", + "items": { "$ref": "#/components/schemas/FilterNode" } + } + } + }, + { "$ref": "#/components/schemas/FilterLeaf" } + ] + } + ] + }, "GitpodOutputSpec": { "type": "object", "additionalProperties": false, @@ -231,7 +288,47 @@ fn object_union_allof_spec() -> Value { } } } - }) + }); + let schemas = spec["components"]["schemas"] + .as_object_mut() + .expect("schema map"); + let reference = json!({ "$ref": "#/components/schemas/FilterNode" }); + let inline = schemas["FilterNode"].clone(); + for (name, members) in [ + ("RepeatedFilterWithOwner", json!([reference, reference])), + ("InlineRestatedFilterWithOwner", json!([inline, reference])), + ] { + schemas.insert( + name.into(), + json!({ + "type": "object", + "properties": { "owner_field": { "type": "string" } }, + "required": ["owner_field"], + "allOf": members + }), + ); + } + schemas.insert( + "MixedFilter".into(), + json!({"anyOf": [ + {"type": "string"}, + {"type": "object", "properties": {"key": {"type": "string"}}, "required": ["key"]} + ]}), + ); + for (name, mut owner) in [ + ("RepeatedMixedFilterObject", json!({"type": "object"})), + ( + "RepeatedMixedFilterNotString", + json!({"not": {"type": "string"}}), + ), + ] { + owner["allOf"] = json!([ + {"$ref": "#/components/schemas/MixedFilter"}, + {"$ref": "#/components/schemas/MixedFilter"} + ]); + schemas.insert(name.into(), owner); + } + spec } fn analyze() -> openapi_to_rust::SchemaAnalysis { @@ -445,6 +542,41 @@ fn main() { "boolean": {} })); round_trip::(serde_json::json!({})); + round_trip::(serde_json::json!({ + "kind": "group", + "filters": [ + { "key": "service", "operation": "eq", "value": "api" }, + { "kind": "group", "filters": [] } + ] + })); + round_trip::(serde_json::json!({ + "key": "status", + "operation": "exists" + })); + round_trip::(serde_json::json!({ + "owner_field": "request-1", + "key": "status", + "operation": "exists" + })); + round_trip::(serde_json::json!({ + "owner_field": "request-2", + "kind": "group", + "filters": [{ "key": "status", "operation": "exists" }] + })); + assert!(serde_json::from_value::( + serde_json::json!({ "key": "status", "operation": "exists" }) + ).is_err()); + assert!(serde_json::from_value::( + serde_json::json!({ "kind": "group", "filters": [] }) + ).is_err()); + round_trip::(serde_json::json!({ "key": "status" })); + assert!(serde_json::from_value::( + serde_json::json!("scalar") + ).is_err()); + round_trip::(serde_json::json!({ "key": "status" })); + assert!(serde_json::from_value::( + serde_json::json!("scalar") + ).is_err()); } "##, ) @@ -468,6 +600,127 @@ fn main() { ); } +#[test] +fn allof_restating_a_union_up_to_annotations_resolves_to_that_union() { + let analysis = analyze(); + let SchemaType::Reference { target } = &analysis.schemas["RestatedFilter"].schema_type else { + panic!( + "RestatedFilter should alias FilterNode, got {:?}", + analysis.schemas["RestatedFilter"].schema_type + ); + }; + assert_eq!(target, "FilterNode"); +} + +#[test] +fn restated_unions_retain_owner_properties_and_required_names() { + for keyword in ["anyOf", "oneOf"] { + let union = json!({keyword: [ + {"type": "object", "properties": {"a": {"type": "string"}}, "required": ["a"]}, + {"type": "object", "properties": {"b": {"type": "integer"}}, "required": ["b"]} + ]}); + let reference = json!({"$ref": "#/components/schemas/Union"}); + for members in [ + json!([reference, reference]), + json!([reference, union]), + json!([union, reference]), + json!([union, union]), + ] { + for (declares_property, requires_property) in + [(true, true), (true, false), (false, true)] + { + let mut owner = json!({"allOf": members}); + if declares_property { + owner["properties"] = json!({"owner_field": {"type": "string"}}); + } + if requires_property { + owner["required"] = json!(["owner_field"]); + } + let spec = json!({ + "openapi": "3.1.0", + "info": {"title": "restated union owner", "version": "1"}, + "paths": {}, + "components": {"schemas": {"Union": union, "Owner": owner}} + }); + let analysis = SchemaAnalyzer::new(spec).unwrap().analyze().unwrap(); + let SchemaType::Object { + properties, + required, + variant, + .. + } = &analysis.schemas["Owner"].schema_type + else { + panic!( + "owner fields were dropped for {owner}: {:?}", + analysis.schemas["Owner"].schema_type + ); + }; + assert!(variant.is_some(), "owner must retain its union for {owner}"); + assert_eq!( + required.contains("owner_field"), + requires_property, + "{owner}" + ); + let property = &properties["owner_field"]; + if declares_property { + assert!( + matches!( + &property.schema_type, + SchemaType::Primitive { rust_type, .. } if rust_type == "String" + ), + "declared owner field must keep its type for {owner}" + ); + } else { + assert!(property.synthesized_required, "{owner}"); + } + } + } + } +} + +#[test] +fn restated_unions_preserve_owner_assertions_for_mixed_branches() { + for keyword in ["anyOf", "oneOf"] { + let union = json!({keyword: [ + {"type": "object", "properties": {"key": {"type": "string"}}, "required": ["key"]}, + {"type": "string"} + ]}); + assert!(jsonschema::is_valid(&union, &json!("scalar"))); + for mut owner in [ + json!({"type": "object"}), + json!({"not": {"type": "string"}}), + ] { + owner["allOf"] = json!([union, union]); + assert!(!jsonschema::is_valid(&owner, &json!("scalar"))); + owner["allOf"] = json!([ + {"$ref": "#/components/schemas/Union"}, + {"$ref": "#/components/schemas/Union"} + ]); + let spec = json!({ + "openapi": "3.1.0", + "info": {"title": "restated union owner type", "version": "1"}, + "paths": {}, + "components": {"schemas": { + "Union": union, + "Owner": owner + }} + }); + let analysis = SchemaAnalyzer::new(spec).unwrap().analyze().unwrap(); + assert!( + matches!( + &analysis.schemas["Owner"].schema_type, + SchemaType::Object { + variant: Some(_), + .. + } + ), + "owner must still require an object: {:?}", + analysis.schemas["Owner"].schema_type + ); + } + } +} + #[test] fn multiple_union_intersections_are_reported_instead_of_silently_dropped() { let spec = json!({ @@ -497,3 +750,56 @@ fn multiple_union_intersections_are_reported_instead_of_silently_dropped() { "unexpected error: {error}" ); } + +fn assert_distinct_unions_are_rejected(first: Value, second: Value, witness: Value) { + assert!(jsonschema::is_valid(&first, &witness)); + assert!(!jsonschema::is_valid(&second, &witness)); + for members in [json!([first, second]), json!([second, first])] { + let spec = json!({ + "openapi": "3.1.0", + "info": { "title": "distinct unions", "version": "1" }, + "paths": {}, + "components": { "schemas": { "Intersection": { "allOf": members } } } + }); + let error = SchemaAnalyzer::new(spec) + .unwrap() + .analyze() + .expect_err("different union assertions must not be deduplicated"); + assert!( + error + .to_string() + .contains("intersects multiple union members"), + "unexpected error: {error}" + ); + } +} + +#[test] +fn union_restatements_preserve_dependent_required_data() { + for keyword in ["anyOf", "oneOf"] { + let first = json!({keyword: [ + {"type": "object", "dependentRequired": {"description": ["kind"]}}, + {"type": "string"} + ]}); + let second = json!({keyword: [ + {"type": "object", "dependentRequired": {"description": ["value"]}}, + {"type": "string"} + ]}); + assert_distinct_unions_are_rejected( + first, + second, + json!({"description": "x", "kind": "y"}), + ); + } +} + +#[test] +fn union_restatements_preserve_object_type_for_mixed_branches() { + for keyword in ["anyOf", "oneOf"] { + let first = json!({keyword: [{"type": "object"}, {"type": "string"}]}); + let second = json!({"type": "object", keyword: [ + {"type": "object"}, {"type": "string"} + ]}); + assert_distinct_unions_are_rejected(first, second, json!("scalar")); + } +}