fix(compilers/openapi): drop a non-object security requirement - #327
Merged
Conversation
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
A
securitylist entry the document does not write as an object —null, a scalar, asequence — was lowered to
ir.AuthRequirement{}, the encodingir-design.md§9 reservesfor a deliberately empty option: "no auth is one acceptable choice". The two spellings
produced byte-identical IR, so a malformed entry silently granted a permission the
document never wrote. Beside a real requirement,
security: [{key: []}, null]compiled toan OR-list reading as optionally authenticated; on its own,
security: [null]compiledto
[{}], i.e. explicitly public.The entry is now dropped whole, on the terms #41 established for an option naming an
undeclared scheme, and the enclosing collapse handles the sole-entry case for free: an
originally non-empty list left empty by dropping becomes
nil— "inherit the enclosingdefault" — rather than
[], which §9 reserves for a deliberate "explicitly public"declaration.
One guard serves both call sites of the lowering, the document-level
securitylist andan operation's own override. Both are tested, because a document-level fixture alone would
pass just as well on a fix wired into the service walk that an operation's override never
reaches.
Telling the two apart
nulland{}both unmarshal to a requirement holding no members, so nothing about therequirement itself separates them. The node it was read from does: the marshaller records
a root node for a value it could read as a mapping and none for anything else, so
{}carries one whether written inline or reached through an alias, and no other spelling
does. That is a fact about the library rather than about the document, so the tests
holding it compile documents rather than build requirements — including an alias to
{},which is what stops the check being rewritten as a test of the node's kind.
No new diagnostic, deliberately
The drop draws no report from the compiler. That follows the rule this package already
states for the same shape in
securitySchemes: an entry written as something other thanan object already draws the loader's type-mismatch, which names both the entry
(
openapi.security.1) and what was wrong with it, so a second report would send thereader to the same position to learn less. An unresolved scheme name is reported here,
because nothing else places it — that asymmetry is the existing rule rather than a new
one. Every malformed row asserts the loader's report is present, so the drop cannot
quietly become silent if that report stops arriving.
The contrast the issue notes is left standing: the loader's report is sited at a
line:colrather than at a/security/<index>pointer like the requirement diagnosticsbeside it. That is not a gap at this entry — a structured validation error is sited at
line:col by
load.validationProvenance, the same shapescan.cyclicDiaguses, and thisentry's type-mismatch is one of those. So the two reports differ because they come from
different channels, not because this one was missed. Worth knowing when reading them side
by side; not something one entry could settle.
Test plan
go test ./compilers/openapi/internal/auth/...— three new tests:null,~, a string, a number, a sequence,an alias to a scalar), plus
{}and an alias to{}as controls that must survivenulldropped,{}keptnilguard written as a node-kind check instead, the alias-to-
{}control goes red.gofmt,go vet,golangci-lint,go build, and the 100% coveragegate.
Closes #284