fix(compilers/openapi): read $id past an empty pointer segment - #362
Open
OmarAlJarrah wants to merge 1 commit into
Open
fix(compilers/openapi): read $id past an empty pointer segment#362OmarAlJarrah wants to merge 1 commit into
OmarAlJarrah wants to merge 1 commit into
Conversation
declaresResourceIDAbove walks a JSON Pointer from the document root looking for the $id that starts a schema resource of its own, and skipped every empty segment on the way. Only the leading one is an artifact of splitting on '/': every later one is a real RFC 6901 reference token naming the key "", which is how a component schema named "" is addressed (/components/schemas/). A pointer ending in one therefore stopped the walk at the parent map and never read the position's own $id. dynamicChainVerdict then let a $dynamicRef expansion cross a schema resource boundary it should have degraded at, which is the worse of the two directions to err in: a missed boundary mints a reference the IR cannot express, where a false one only costs an expansion that would have been safe. Split the pointer through pointerTokens, which drops the leading empty segment and nothing else, so every remaining token takes a step of its own. The empty pointer and any string without a leading '/' yield no tokens at all, leaving the walk reading the document root as before.
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
declaresResourceIDAbove(compilers/openapi/internal/schema/schema.go) walks a JSON Pointer downfrom the document root looking for the
$idthat starts a schema resource of its own, and skippedevery empty segment on the way. Only the leading empty segment is an artifact of splitting a
pointer on
/; every later one is a real RFC 6901 reference token naming the key"", which isexactly how a component schema named
""is addressed —/components/schemas/.So a pointer ending in an empty token stopped the walk at the
components/schemasmap and neverread the position's own
$id.dynamicChainVerdictthen found no boundary and let a$dynamicRefexpansion cross into a schema resource it should have degraded at. That is the worse of the two
directions to err in, as the function's own comment records: a false boundary only costs an
expansion that would have been safe, while a missed one mints a reference the IR cannot express.
The fix splits the pointer through a new
pointerTokens, which drops the leading empty segment andnothing else, so every remaining token — empty or not — takes a step of its own. The empty pointer
and any string without a leading
/yield no tokens at all, so the walk still reads only thedocument root for those, as it did before.
Scope: the same laundering in
nodeview.PointerPathhas a different consequence (a pointer walkthat can deadlock) and needs a different fix, since that walk descends before it reads. It is left
alone here and handled on its own branch. Sweeping the rest of the compiler for the mechanism found
no other instance:
firstPathSegment(internal/operation) splits a URL path template, where "thefirst non-empty segment" is the intent, and
branchPointerHint(internal/schema/compose.go) readsthe last two segments without dropping any.
Test plan
gofmt,go vet,golangci-lint run(0 issues),go build ./...and./scripts/check-coverage.sh(4945/4945 statements) all pass.
Probe — the same spec compiled end to end, before and after. A schema named
""declaring both$idand a$dynamicRef, with the matching$dynamicAnchoron a sibling component:Before, the reference was expanded straight across the
$idboundary:After, the boundary is seen and the reference is kept verbatim instead:
The full diagnostic list was read on both runs; the only other entry either time is the pre-existing
info diagnostic that keeps
$iditself underUnmodeled.The tests were watched failing. Two were added: a direct case in
TestDynamicRef_IrreducibleIsKeptAndSaysWhyfor the end-to-end verdict, andTestDeclaresResourceIDAbove_EmptySegmentIsATokenNotAnArtifactfor the walk itself (a trailing emptytoken, an interior one, a sibling under no
$id, and the empty pointer). Restoring the old line andre-running gives:
The two cases that pass under both lines are there on purpose: a sibling component and the empty
pointer pin that the fix widened the walk by exactly one token class and no more.
Closes #302