fix(compilers/openapi): keep unions/allOf beside a $ref - #461
Open
fuad-daoud wants to merge 2 commits into
Open
Conversation
A schema that writes oneOf/anyOf beside a $ref at the same level lost the union outright: it reached no field of the document, was not kept under Unmodeled, and nothing was reported. Under JSON Schema 2020-12 (and so OpenAPI 3.1), $ref is an ordinary keyword and its siblings conjoin with it, so the union narrows the referenced schema — dropping it silently widens what the SDK accepts. allOf beside a $ref at the same level had the identical gap. The union family's one keeper, preserveUnionSiblings, was reached only through the structural-body path (lowerBesideUnmodeledUnion); a declaration that peels a leading $ref never reached it. The $ref-site keyword census (unhomedKeywords) that already covers format/const/enum/ required/additionalProperties beside a $ref (#283, #348) never named oneOf/anyOf/allOf either. Split preserveUnionSiblings into a thin TypeID-addressed wrapper and preserveUnionSiblingsAt, which writes directly into an ir.Unmodeled map; refSiteRef and PreserveRefSiteKeywords now call the latter directly when a $ref site (or a carrier's $ref) co-declares oneOf/anyOf, reusing the structural-body path's own keeper rather than adding a second one. allOf beside a $ref joins the existing unhomed-keyword census through a new refSiteUnhomedKeywords helper, confined to the $ref site: a body position already homes allOf through family election (dispatchOf/ recordSkippedFamilies), a mechanism a $ref site never reaches, so adding allOf to the general censusKeywords list would misreport every ordinarily composed schema as degraded. Adds the union-beside-ref conformance case (Base/G/H plus an allOf-beside- $ref schema, at both a component position and a property carrier) and extends TestRefSiteKeywords_KeptAtEveryPosition/adds TestRefSiteKeywords_UnionKeptAtEveryPosition in compilers/openapi/internal/ schema, and records the OpenAPI lowering-table row in ir-design.md. Closes #406 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TJZJ8HaugGZTjzVueafLAT
Review of the previous fix (#406) found the sweep incomplete: fillAllOf's handling of an allOf branch spelled as a $ref is a third position with the identical gap, alongside the component/inline-subschema and carrier sites already fixed. `allOf: [{$ref: Base, oneOf: [...]}]` composed straight to Base with the oneOf read by nothing — no alias, no Unmodeled, no diagnostic. Fixed the same way as the other two sites: the branch now routes through refSiteUnhomedKeywords (allOf) and preserveUnionSiblings (oneOf/anyOf), reusing both keepers rather than adding a third. Also fixes a self-contradictory diagnostic: the shared message template asserted "co-declared with structural keywords" even at a $ref site, where there is no structural body. The prefix now lives only in the structural-body path's five reasons; a $ref site's own why is a complete, non-contradictory sentence. Minor cleanups from the same review: the Carrier position's diagnostic is now pinned symmetrically with the other three; the duplicated oneOf/anyOf-presence check across three call sites is one declaresUnion predicate; and the duplicated GoDoc between censusKeywords and refSiteUnhomedKeywords is now argued in one place. Extends the allof-ref-branch-siblings conformance case with the branch-site union/allOf rows and adds TestRefSiteKeywords_AllOfBranchKeepsUnion in compilers/openapi/internal/schema for coverage the top-level conformance test cannot provide (it runs in a different package). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TJZJ8HaugGZTjzVueafLAT
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
A schema that writes
oneOf/anyOfbeside a$refat the same level lost the union outright: it reached no field of the document, was not kept underUnmodeled, and nothing was reported (#406). Under JSON Schema 2020-12 — and so OpenAPI 3.1 —$refis an ordinary keyword whose siblings conjoin with it, so the union narrows the referenced schema; dropping it silently widens what the SDK accepts.allOfbeside a$refhad the identical gap.The union family's one keeper,
preserveUnionSiblings, was reached only through the structural-body path; a declaration that peels a leading$refnever got there, and the$ref-site keyword census that already coversformat/const/enum/required/additionalPropertiesbeside a$ref(#283, #348) never namedoneOf/anyOf/allOfeither.The keeper is split into a thin
TypeID-addressed wrapper andpreserveUnionSiblingsAt, which writes into anir.Unmodeledmap directly, and every$refsite now calls it: the component/alias position, the property carrier, and — the site the mechanism sweep turned up — a$refinside anallOfbranch (fillAllOf), which had the same gap and would have been left standing. All three write the same key form (openapi:oneOf/openapi:anyOf), reason (degraded_lowering), code and message, so a consumer sees one spelling of "a union the IR could not lower".allOfbeside a$refjoins the existing unhomed-keyword census throughrefSiteUnhomedKeywords, confined to the$refsite: a body position already homesallOfthrough family election, which a$refsite never reaches, so adding it to the generalcensusKeywordslist would misreport every ordinarily composed schema. The diagnostic message at a$refsite now states its own reason rather than the body path's ("co-declared with structural keywords…", which is false there).ir-design.md's OpenAPI row records the rule.Test plan
union-beside-ref(witnessesuntagged-unions):Base,G(oneOf),H(anyOf),I(allOf) and a property carrier; each pins the alias node,Base.Target, the kept value (JSONEq), the reason, and exactly one infodegraded-constructwith the pinned message at the position.allof-ref-branch-siblingsgainsUnionBranchandNestedAllOfBranchfor the allOf-branch site, asserted at the branch pointer;TestRefSiteKeywords_AllOfBranchKeepsUnioncoversanyOfthere too. A$ref+oneOfbranch under a distributed union was probed: reached once per variant, idempotent, one diagnostic.make gatepasses: lint 0 issues, coverage at 100%, fuzz, bench-smoke.Closes #406
🤖 Generated with Claude Code
https://claude.ai/code/session_01TJZJ8HaugGZTjzVueafLAT