fix(compilers/openapi): find a subtype's tag past its parent - #368
Open
OmarAlJarrah wants to merge 1 commit into
Open
fix(compilers/openapi): find a subtype's tag past its parent#368OmarAlJarrah wants to merge 1 commit into
OmarAlJarrah wants to merge 1 commit into
Conversation
Model.DiscriminatorValue was stamped only when the subtype's own allOf $ref branch resolved to a schema declaring the discriminator. In a hierarchy more than two levels deep that branch names an intermediate schema, which declares no discriminator of its own, so the mapping key the discriminated ancestor spells for the subtype was dropped — silently, since the composition chain itself survived and nothing diagnosed the missing tag. subtypeDiscriminatorValue now asks every discriminated ancestor rather than the immediate base, returning the key of the nearest one whose mapping names this subtype and falling back to OpenAPI's implicit schema name when no mapping does. That is the rule ir-design.md §4.3 already states, applied along the chain instead of at one hop; a subtype with no discriminated ancestor at any depth still carries no value. The walk is level by level, so "nearest" is by distance rather than by which chain was walked first. It carries a visited set because a cyclic composition compiles without a diagnostic and so reaches it, plus an explicit depth cap per the bounded-everything rule. The corpus fixture witnesses the walk through the implicit-name spelling. The mapping-key spelling at depth is pinned in the compiler's own tests instead, because pass.Validate still refuses a transitive mapping target as a missing variant (#52) and every corpus spec has to clear that sweep; the fixture and the test both say so.
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
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
Model.DiscriminatorValuewas stamped only when a subtype's ownallOf$refbranchresolved to a schema declaring the discriminator. In a hierarchy more than two levels deep
that branch names an intermediate schema, which declares no discriminator of its own, so
subtypeDiscriminatorValuefound nothing and returned early — dropping the mapping key thediscriminated ancestor spells for that subtype, and never reaching the implicit-schema-name
fallback either. The composition chain itself survived, so the tag was the only thing lost,
and it was lost with no diagnostic.
subtypeDiscriminatorValuenow asks every discriminated ancestor rather than the immediatebase: the value is the mapping key of the nearest ancestor whose discriminator names this
subtype, falling back to OpenAPI's implicit schema name when no mapping does. That is the
rule
ir-design.md§4.3 already states, applied along the chain instead of at one hop. Asubtype with no discriminated ancestor at any depth still carries no value.
Bounding the walk. It is level by level (breadth-first), so "nearest" means fewest hops
rather than whichever chain happened to be walked first — which is what decides between two
ancestors whose mappings both name the same subtype. It carries a visited set keyed on the
resolved schema, because a cyclic composition is not refused upstream:
A: allOf [$ref B],B: allOf [$ref A]compiles clean today and reaches this code, so the set is load-bearingrather than defensive. On top of that is an explicit
maxDiscriminatorAncestorDepthcap, perthe bounded-everything rule.
Before, on the issue's reproduction:
After:
The full diagnostic list is empty in both runs — the drop was silent, which is why no
existing assertion caught it.
Test plan
New corpus spec
testdata/conformance/openapi/discriminator-transitive.yamlplusassertDiscriminatorTransitive, and four cases in the compiler's own suite. Every gate steppasses (
gofmt,go vet,golangci-lint→0 issues.,go build,check-coverage.sh→all 4959 statements covered).Golden movement is exactly the two new files; no existing golden moved, and
unwitnessed.golden.txtis unchanged. That is the finding, not an accident: no committedfixture reached a three-level hierarchy, which is how the bug survived the corpus.
Cases covered, each verified against a planted mutation rather than by reading:
TestAllOf_DiscriminatorValueFromDistantMappingTestAllOf_DiscriminatorValueFromNearestAncestorTestAllOf_DiscriminatorValueUndiscriminatedChainTestAllOf_DiscriminatorValueCyclicCompositionTestAllOf_DiscriminatorValueChainDeeperThanCapRestoring the one-hop reading reddens the corpus and the compiler suite:
The cyclic case is the one that cannot fail by assertion: with the visited set removed, the
walk fans out five ways per level for the length of the cap and the package times out after
90s instead of finishing in under a second.
Out of scope
The corpus fixture witnesses the walk through the implicit-name spelling. The mapping-key
spelling at depth — a root whose mapping names a grandchild, the issue's own reproduction —
is pinned in
compilers/openapi/internal/schemainstead, becausepass.Validatestillrefuses a transitive mapping target as a missing variant (#52, open), and
TestValidate_Corpusholds every corpus spec to producing no error diagnostic. Putting that spelling in the corpus
would mean either fixing #52 here or weakening that sweep, so it stays where it can be pinned
honestly; the fixture comment and the test's doc comment both record why. Worth noting for
#52's sake: this change is what first makes that false positive reachable from a real spec.
Closes #305