openapi: resolve or drop reference targets instead of emitting dangling IR IDs - #22
Merged
Conversation
Several reference-like constructs lowered their target into an IR type or
auth ID without checking that the target actually interns, leaving the IR
with IDs that point at nothing:
- discriminator mappings and defaultMapping (a bare name, a $ref, or a deep
pointer) synthesized a TypeID whether or not the schema existed;
- a $ref to a component sub-schema (e.g. .../Foo/properties/bar, a raw
validation-only sub-schema, or a scalar under additionalProperties)
derived a pointer ID that nothing was interned under;
- a $ref whose document part names the compiled file itself was treated as
external and never lowered;
- a security requirement naming a scheme absent from
components.securitySchemes wrote a dangling AuthID.
Reference resolution now follows one rule, matching how schema-position
$refs were already handled: if a target resolves to an internal same-file
schema, intern it (hoisting the sub-schema under its pointer-derived ID
when needed) so the reference is valid; otherwise emit an
openapi/unresolved-ref diagnostic and drop the offending entry. Same-file
document references resolve internally; genuinely external targets are
dropped, since Milestone 1 does not intern cross-file schemas. A schema
name spelled as a pointer segment ("A/B" -> "A~1B") now unescapes so it
resolves to the schema it names.
A dangling-reference oracle asserts referential closure across the twelve
reproducers and the full conformance corpus plus petstore. It walks every
ID-bearing site of a produced document, not only the ones this fix touched:
each TypeRef, discriminator, and value ref in the type registry, and the
whole service/operation tree — parameters, request and response payloads,
content items and file contents, part and response headers, error cases,
streaming, pagination, long-running, resource members, service renames, and
every scheme use — so a dangling reference reintroduced anywhere is caught.
Closes #14
Add unit tests for the interning-lookup and drop paths in reference lowering that the existing suite left uncovered: internedID's byPointer and type-registry hits, resolveSchemaRef reusing an already-hoisted sub-schema versus dropping an unresolved deep $ref, hoistSubSchema guarding a nil resolved body and returning a node its pointer already owns, and discriminatorDefault dropping an unresolved defaultMapping with a diagnostic. Restores compilers/openapi statement coverage to 100%.
The reference-lowering unification still left two paths that could write a type ID with no interned node behind it, plus one same-file heuristic that could misfire: - A discriminator mapping to a bare component name always resolved to the component's namedTypeID, but a component whose name is the empty string is interned anonymously (componentSchemaName rejects an empty trailing segment), so the mapping pointed at nothing. Derive the target through typeIDForPointer so it matches the ID the component was interned under. - resolveComponentRef built the resolved ID from the raw $ref pointer text, so a reference that escaped its pointer non-canonically (a bare '~' for a component named "A~B", interned under "A~0B") produced an ID that did not match the interned node. Rebuild the ID from the component's canonical name so any equivalent spelling resolves to the same node. - sameFile compared a $ref's document part to the source purely by basename, which would treat a genuine cross-directory reference (dir2/m.yaml from dir1/m.yaml) as a self-reference. Restrict the basename shortcut to a bare filename and match a directory-qualified document part in full. The referential-closure test now delegates to irverify.Verify, the shared structural checker, instead of a bespoke IR walker, so it tracks new ID-bearing fields automatically. Reproducers f31 and f32 are added, and the spec-check harness allowlist is reconciled with the new fixtures.
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
Several reference-like constructs were lowered into IR IDs without checking that the target resolved, and without emitting a diagnostic — leaving the IR with a type or auth ID that pointed at nothing. This unifies every reference-lowering path on one rule: never write a type/auth ID into the IR unless the target interns, mirroring how normal schema-position
$refs already emitopenapi/unresolved-ref.mapping(oneOf/anyOf unions and allOf model bases): a mapping value is resolved only to an already-interned schema — a bare name (including one containing/), a$refto a declared component, or an already-interned node. Unresolvable or external targets are dropped with anopenapi/unresolved-refdiagnostic; an all-dropped mapping collapses to nil for a clean round-trip.$refs (e.g.#/components/schemas/Foo/properties/bar, anot/validation-only branch, a map-value sub-schema): the referenced sub-schema is now interned/hoisted under its pointer-derived ID so the ref resolves, instead of minting an un-backed ID.$refwhose document part is the compiled file's own name is treated as internal and resolved; a document part that carries its own directory is matched in full rather than by basename, so a cross-directory reference is never mistaken for a self-reference; genuinely external cross-file targets are dropped with a diagnostic (Milestone 1 does not intern cross-file).components.securitySchemesis dropped with a diagnostic rather than writing a danglingAuthID.typeIDForPointerover the canonical, RFC 6901-escaped name), so it resolves regardless of how the reference happens to be spelled. This covers a schema literally namedA/B(escapedA~1B), a name containing~referenced with a non-canonical escape (A~Bfor a component interned underA~0B), and the degenerate empty-named ("") component — which is interned anonymously and would otherwise dangle behind anamedTypeID.Test plan
irverify.Verify— the shared IR structural checker — over 14 minimal reproducers undertestdata/dangling/openapi/plus the entire conformance corpus and the petstore golden: zero dangling references. Each reproducer is also pinned to its intended outcome (interned, or dropped with anopenapi/unresolved-refdiagnostic).TestConformancegoldens and the petstore snapshot are unchanged (no golden churn).go test ./...,gofmt -l .,go vet ./...,golangci-lint runall clean, and the 100% statement-coverage gate holds.Closes #14.