Repository navigation
Conversation
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.
|
@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
left a comment
There was a problem hiding this comment.
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.
| "enum" | "const" | "default" | "required" | "discriminator" => {} | ||
| _ => match child { | ||
| Value::Array(items) => items.iter_mut().for_each(strip_schema_annotations), | ||
| Value::Object(_) => strip_schema_annotations(child), |
There was a problem hiding this comment.
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.
| 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"); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 :)
Summary
An
allOfwith 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 anallOfwhose two unions are the same union written twice, and that error stops the whole spec from generating:Five Workers Observability request bodies type their filter items as:
The inline
anyOfrepeatsfilter_node's branches word for word, apart from addeddescriptions.This PR drops an
allOfmember whose union equals an earlier member's union, comparing assertions only:title,description,example(s),readOnly,writeOnly,deprecated,externalDocs,xml,$comment,x-*) are stripped recursivelyproperties,patternProperties,$defsand similar maps are kept, so a property nameddescriptionstill countsenum,const,default,requiredanddiscriminatorvalues are compared as datatype: objecton the referenced union is ignored, because the inline restatement omits itThe dedupe runs before the existing early returns, so a restatement next to a single
$refbecomes an alias of that schema. Unions that differ are untouched and still report the diagnostic.multiple_union_intersections_are_reported_instead_of_silently_droppedis unchanged and passes.Generated compatibility
tests/corpus-manifest.txtis unchanged. For specs that previously failed this way, generation now succeeds. In the current Cloudflare spec the filter items becomeVec<Box<WorkersObservabilityFilterNode>>, an enum ofGroupandWorkersObservabilityFilterLeaf.allOfthat intersects genuinely different unions is still rejected.Validation
FilterNode/RestatedFiltertotests/allof_object_union_preservation_test.rs, a recursive group/leaf union restated inline with extra descriptions, as in Cloudflare's schema. Analysis must aliasRestatedFiltertoFilterNode, 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 largerjson!fixture.cloudflare/api-schemasmain, 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) passescargo check. I couldn't finish a check of the full 1.1M-line output locally because the disk filled up.allOfwith more than one union member. There are seven: the five restatements this PR resolves, and two whoseanyOfs only constrainrequiredand are already skipped (email-sending_EmailBuilder,magic_app_config_add_single_request).cargo fmt --checkcargo clippy --all-features -- -D warningscargo 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-featuresscripts/corpus-manifest.sh: no changesNotes for reviewers
without_restated_unionsandunion_signatureinsrc/analysis.rs, the two lines at the top ofanalyze_allof_compositionthat call them, andstrip_schema_annotationsnext toschema_is_annotation_only.