Skip to content

fix(generator): resolve allOf members that restate the same union - #104

Open
gilesw wants to merge 2 commits into
gpu-cli:mainfrom
gilesw:fix/allof-multiple-union-members
Open

gilesw wants to merge 2 commits into
gpu-cli:mainfrom
gilesw:fix/allof-multiple-union-members

Conversation

@gilesw

@gilesw gilesw commented Oct 12, 2026

Copy link
Copy Markdown
Contributor

Summary

An allOf with more than one union member fails analysis with "intersects multiple union members", which is right when the unions differ. The current Cloudflare spec, though, has an allOf whose two unions are the same union written twice, and that error stops the whole spec from generating:

Error: Invalid schema: allOf object `SharedQueryPostRequestParameters` intersects multiple union members (`workers-observability_filter_node` and `SharedQueryPostRequestParametersAllOfVariant2`), which cannot be represented by one flattened variant

Five Workers Observability request bodies type their filter items as:

allOf:
  - $ref: '#/components/schemas/workers-observability_filter_node'   # anyOf: [group, $ref leaf]
  - anyOf: [group (with descriptions), $ref leaf]
    description: "Supports nested groups via kind: 'group'. ..."

The inline anyOf repeats filter_node's branches word for word, apart from added descriptions.

This PR drops an allOf member whose union equals an earlier member's union, comparing assertions only:

  • annotation keywords (title, description, example(s), readOnly, writeOnly, deprecated, externalDocs, xml, $comment, x-*) are stripped recursively
  • member names under properties, patternProperties, $defs and similar maps are kept, so a property named description still counts
  • enum, const, default, required and discriminator values are compared as data
  • type: object on the referenced union is ignored, because the inline restatement omits it

The dedupe runs before the existing early returns, so a restatement next to a single $ref becomes an alias of that schema. Unions that differ are untouched and still report the diagnostic. multiple_union_intersections_are_reported_instead_of_silently_dropped is unchanged and passes.

Generated compatibility

  • Generated model or method signatures: unchanged for the pinned corpus; tests/corpus-manifest.txt is unchanged. For specs that previously failed this way, generation now succeeds. In the current Cloudflare spec the filter items become Vec<Box<WorkersObservabilityFilterNode>>, an enum of Group and WorkersObservabilityFilterLeaf.
  • Query/path/header/body wire behavior: none changed.
  • Generated runtime dependencies or features: none.
  • Configuration defaults or migrations: none.
  • Remaining unsupported OpenAPI shapes: an allOf that intersects genuinely different unions is still rejected.

Validation

  • Added FilterNode/RestatedFilter to tests/allof_object_union_preservation_test.rs, a recursive group/leaf union restated inline with extra descriptions, as in Cloudflare's schema. Analysis must alias RestatedFilter to FilterNode, and the compiled scratch crate round-trips a nested group and a leaf. Without the change, all three analysis-based tests in the file fail with the error above. The file needs #![recursion_limit = "256"] for the larger json! fixture.
  • The current Cloudflare spec (cloudflare/api-schemas main, 3,671 operations) now generates: 1.1M lines in about 8 s. A cut-down spec with all 36 Workers Observability operations (about 28k lines) passes cargo check. I couldn't finish a check of the full 1.1M-line output locally because the disk filled up.
  • Across the current Cloudflare spec, I listed every allOf with more than one union member. There are seven: the five restatements this PR resolves, and two whose anyOfs only constrain required and are already skipped (email-sending_EmailBuilder, magic_app_config_add_single_request).
  • cargo fmt --check
  • cargo clippy --all-features -- -D warnings
  • cargo nextest run --all-features --no-fail-fast: 715 of 727 passed in the parallel run. The other 12 are scratch-crate compile tests that failed while the local disk was full. All 12 passed on rerun, 10 of them run one at a time.
  • cargo test --doc --all-features
  • scripts/corpus-manifest.sh: no changes
  • Changelog entry under Unreleased

Notes for reviewers

  • Smallest review path: without_restated_unions and union_signature in src/analysis.rs, the two lines at the top of analyze_allof_composition that call them, and strip_schema_annotations next to schema_is_annotation_only.
  • Follow-up from fix(generator): leave union-declared required names to the variant #103, from the same Cloudflare CLI evaluation. Together they let the current Cloudflare spec generate in full, and its DNS record responses decode into typed variants.

An allOf whose members are the same union, once annotations are
ignored, failed with "intersects multiple union members". The current
Cloudflare spec types its Workers Observability query filters as
allOf: [$ref filter_node, anyOf: [group, leaf]], where the inline anyOf
repeats filter_node's branches with added descriptions. That error
stopped the whole spec from generating.

Repeated union members are now dropped before composition, comparing
assertions only: annotation keywords are stripped recursively, member
names under properties are kept, and `type: object` on the named union
is ignored. A restatement next to a single $ref becomes an alias of
that schema. Unions that differ still report the diagnostic.

The pinned corpus has no such schema, so its output is unchanged.
@vercel

vercel Bot commented Oct 12, 2026

Copy link
Copy Markdown

@gilesw is attempting to deploy a commit to the lbl-rd Team on Vercel.

A member of the Team first needs to authorize it.

@lightsofapollo lightsofapollo left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found two cases where the deduplication treats unions with different assertions as equivalent. Both repro tests pass against main's analyzer and fail against this PR; the four existing tests in allof_object_union_preservation_test still pass. I'd fix these before merging.

Comment thread src/analysis.rs
"enum" | "const" | "default" | "required" | "discriminator" => {}
_ => match child {
Value::Array(items) => items.iter_mut().for_each(strip_schema_annotations),
Value::Object(_) => strip_schema_annotations(child),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fallback also recurses into dependentRequired, whose keys are property names rather than schema keywords. For example, two otherwise identical union branches with dependentRequired: {"description": ["kind"]} and dependentRequired: {"description": ["value"]} both become dependentRequired: {} after stripping. The second union then gets dropped even though {"description": "x", "kind": "y"} satisfies the first branch and fails the second.

I reproduced this with two anyOf members in an allOf: main reports the multiple-union diagnostic, but this PR succeeds and retains only the first union. Could we preserve dependentRequired as data, as we already do for required, and add a regression test? More generally, restricting recursion to schema-valued keywords would avoid interpreting other data maps as schemas.

Comment thread src/analysis.rs
strip_schema_annotations(&mut value);
let object = value.as_object_mut()?;
if object.get("type").and_then(Value::as_str) == Some("object") {
object.remove("type");

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing type: object is safe for the Cloudflare example because its branches are objects, but it isn't redundant for every union. Consider allOf: [{anyOf: [{type: object}, {type: string}]}, {type: object, anyOf: [{type: object}, {type: string}]}]. The second member excludes strings, yet these signatures compare equal and it gets dropped. The resulting analyzed union still includes String.

I checked this with a regression test that first verifies the two members accept different values using jsonschema::is_valid. Main rejects the intersection; this PR silently collapses it. Could we only ignore the object type when the union branches provably imply it, or retain the distinction otherwise? A mixed object/scalar union would make a useful negative test here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just chiming in here.. So my plan was to use your work to make a flarectl cli built in rust as Cloudflare has seemingly dropped their support. The Cloudflare api could be a good test case of your work as it seems to be a monster. One warning though. Processing it used all the memory on my 32gb macbook! Good luck and thanks for the project :)

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants