Skip to content

fix(compilers/openapi): read $id past an empty pointer segment - #362

Open
OmarAlJarrah wants to merge 1 commit into
mainfrom
fix/openapi-empty-segment-id-walk
Open

fix(compilers/openapi): read $id past an empty pointer segment#362
OmarAlJarrah wants to merge 1 commit into
mainfrom
fix/openapi-empty-segment-id-walk

Conversation

@OmarAlJarrah

Copy link
Copy Markdown
Member

Summary

declaresResourceIDAbove (compilers/openapi/internal/schema/schema.go) walks a JSON Pointer down
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 empty segment is an artifact of splitting a
pointer on /; every later one is a real RFC 6901 reference token naming the key "", which is
exactly how a component schema named "" is addressed — /components/schemas/.

So a pointer ending in an empty token stopped the walk at the components/schemas map and never
read the position's own $id. dynamicChainVerdict then found no boundary and let a $dynamicRef
expansion 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 and
nothing 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 the
document root for those, as it did before.

Scope: the same laundering in nodeview.PointerPath has a different consequence (a pointer walk
that 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 "the
first non-empty segment" is the intent, and branchPointerHint (internal/schema/compose.go) reads
the 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
$id and a $dynamicRef, with the matching $dynamicAnchor on a sibling component:

components:
  schemas:
    "": {$id: 'https://example.com/empty', $dynamicRef: '#m'}
    M: {$dynamicAnchor: m, type: string}

Before, the reference was expanded straight across the $id boundary:

TYPE t/anon/components/schemas/ = {"kind":"scalar", ... ,"base":{"target":"t/openapi/components/schemas/M"}}
DIAG [info] openapi/dynamic-ref-expanded at "/components/schemas//$dynamicRef":
  $dynamicRef expanded to "t/openapi/components/schemas/M", the one matching $dynamicAnchor in this document

After, the boundary is seen and the reference is kept verbatim instead:

TYPE t/anon/components/schemas/ = {"kind":"scalar", ... ,"unmodeled":{"openapi:$dynamicRef":
  {"reason":"degraded_lowering","value":"#m","provenance":{"pointer":"/components/schemas//$dynamicRef"}}, ...},
  "base":{"target":"t/prim/any"}}
DIAG [info] openapi/degraded-construct at "/components/schemas//$dynamicRef":
  $dynamicRef was not expanded because an $id at or above "/components/schemas/" starts a schema
  resource of its own, and the IR resolves no resource base URIs; it is kept verbatim under Unmodeled

The full diagnostic list was read on both runs; the only other entry either time is the pre-existing
info diagnostic that keeps $id itself under Unmodeled.

The tests were watched failing. Two were added: a direct case in
TestDynamicRef_IrreducibleIsKeptAndSaysWhy for the end-to-end verdict, and
TestDeclaresResourceIDAbove_EmptySegmentIsATokenNotAnArtifact for the walk itself (a trailing empty
token, an interior one, a sibling under no $id, and the empty pointer). Restoring the old line and
re-running gives:

--- FAIL: TestDeclaresResourceIDAbove_EmptySegmentIsATokenNotAnArtifact (0.01s)
    --- FAIL: .../the_position_is_named_by_a_trailing_empty_token (0.00s)
        	Error:      	Not equal:
        	            	expected: true
        	            	actual  : false
    --- FAIL: .../an_interior_empty_token_still_descends (0.00s)
        	Error:      	Not equal:
        	            	expected: true
        	            	actual  : false
--- FAIL: TestDynamicRef_IrreducibleIsKeptAndSaysWhy (0.00s)
    --- FAIL: .../a_schema_named_""_is_still_in_a_resource_of_its_own (0.01s)
        	Error:      	Should be true
        	Messages:   	an irreducible $dynamicRef is kept verbatim, not dropped

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

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.
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: $id resource-boundary walk skips empty pointer segments

1 participant