Skip to content

fix(compilers/openapi): find a subtype's tag past its parent - #368

Open
OmarAlJarrah wants to merge 1 commit into
mainfrom
fix/openapi-transitive-discriminator
Open

fix(compilers/openapi): find a subtype's tag past its parent#368
OmarAlJarrah wants to merge 1 commit into
mainfrom
fix/openapi-transitive-discriminator

Conversation

@OmarAlJarrah

Copy link
Copy Markdown
Member

Summary

Model.DiscriminatorValue was stamped only when a 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
subtypeDiscriminatorValue found nothing and returned early — dropping the mapping key the
discriminated 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.

subtypeDiscriminatorValue now asks every discriminated ancestor rather than the immediate
base: 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. A
subtype 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-bearing
rather than defensive. On top of that is an explicit maxDiscriminatorAncestorDepth cap, per
the bounded-everything rule.

Before, on the issue's reproduction:

$ morphic compile repro.yaml -skip-validate | jq -c '...'
{"id":"Dog","base":"Pet","dv":"dog"}
{"id":"Pet","base":"","dv":""}
{"id":"Puppy","base":"Dog","dv":""}      <- named "puppy" by Pet's mapping

After:

{"id":"Dog","base":"Pet","dv":"dog"}
{"id":"Pet","base":"","dv":""}
{"id":"Puppy","base":"Dog","dv":"puppy"}

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.yaml plus
assertDiscriminatorTransitive, and four cases in the compiler's own suite. Every gate step
passes (gofmt, go vet, golangci-lint0 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.txt is unchanged. That is the finding, not an accident: no committed
fixture 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:

Case Where Mutation that reddens it
Mapping names the subtype two levels up TestAllOf_DiscriminatorValueFromDistantMapping one-hop reading restored
Mapping omits it — implicit schema name at depth 2 and 3 corpus fixture + golden one-hop reading restored
Nearest of two discriminated ancestors wins TestAllOf_DiscriminatorValueFromNearestAncestor walk made depth-first
No discriminated ancestor at any depth stays empty TestAllOf_DiscriminatorValueUndiscriminatedChain empty-ancestor guard dropped
Cyclic composition terminates TestAllOf_DiscriminatorValueCyclicComposition visited set removed
The depth cap binds TestAllOf_DiscriminatorValueChainDeeperThanCap straddles the cap in both directions

Restoring the one-hop reading reddens the corpus and the compiler suite:

--- FAIL: TestConformance/discriminator-transitive
        expected: "Puppy"   actual: ""    Puppy answers to the tag its ancestor spells for it
        expected: "Whelp"   actual: ""    Whelp answers to the tag its ancestor spells for it
        capability assertion failed; not comparing or rewriting the golden
--- FAIL: TestAllOf_DiscriminatorValueFromDistantMapping
        expected: "puppy"   actual: ""    the key Pet's mapping spells for Puppy survives the intermediate
--- FAIL: TestAllOf_DiscriminatorValueChainDeeperThanCap/within_the_cap
        expected: "S254"    actual: ""    S254 is 255 links below the discriminated root

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/schema instead, because pass.Validate still
refuses a transitive mapping target as a missing variant (#52, open), and TestValidate_Corpus
holds 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

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.
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.

openapi: a subtype tagged by an ancestor's discriminator mapping loses its wire tag value

1 participant