fix(compilers/openapi): stop stamping querystring with a style - #364
Open
OmarAlJarrah wants to merge 2 commits into
Open
fix(compilers/openapi): stop stamping querystring with a style#364OmarAlJarrah wants to merge 2 commits into
OmarAlJarrah wants to merge 2 commits into
Conversation
defaultParamStyle put in: querystring in the same arm as query and cookie, so a 3.2 parameter binding the whole query string came out of the compiler carrying style: "form" and explode: true. Neither keyword is legal at that location: 3.2 binds the query string from the parameter's content and forbids style there, and the bundled parser refuses the declaration outright. The IR therefore recorded a serialization fact the source cannot state, and an emitter reading style: form would serialize the parameter as a form-exploded field instead of as the media type ContentType already names. The location now has no default style, and a parameter that resolves to none takes no explode either — explode qualifies a style, and there is none to qualify. A style declared at that location anyway still lowers as declared, beside the parser's error diagnostic: the compiler stops inventing a style there, it does not start erasing one. param-querystring joins the conformance corpus with the contrast the golden needs — a querystring binding beside an ordinary query parameter, which keeps the defaults its own location does admit.
Suppressing the invented querystring style took a declared explode with it: resolveStyleExplode returned early whenever the resolved style was empty, so `in: querystring` with `explode: false` and no style beside it reached the IR with no explode at all, and nothing said so. That is the same silent drop the invented style was, in the other direction, and it contradicts the rule the style half already follows -- 3.2 forbids both keywords at that location and the bundled parser refuses neither, so both reach the compiler, and dropping content a document states is an emitter's call rather than a compiler's. A declared explode now lowers as declared wherever it is written; only the default is suppressed where there is no style to qualify. The existing test could not see this: it declares style and explode together, so the early return it would have tripped was never reached.
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
defaultParamStyleputin: querystringin the same arm asqueryandcookie, so anOpenAPI 3.2 parameter that binds the whole query string came out of the compiler carrying
style: "form"andexplode: true. Neither keyword is legal there: 3.2 binds the querystring from the parameter's
contentand forbidsstyleat that location, and the bundledparser refuses the declaration outright. The IR was therefore recording a serialization fact
the source cannot state, and an emitter reading
style: formwould serialize the parameteras a form-exploded field rather than as the media type
contentTypealready names.The location now resolves to no style, and a parameter that resolves to none takes no
explode either — explode qualifies a style, and there is none to qualify.
resolveStyleExplode'sdoc comment named only two of the three cases in its table; it now states the third.
A
styledeclared at that location anyway still lowers as declared, beside the errordiagnostic the parser already raises for it: the compiler stops inventing a style there, it
does not start erasing one. That is the same posture the reserved-header parameter takes —
dropping declared content is an emitter's call rather than a compiler's.
Before and after, compiling a parameter declared as
Both compiles report no diagnostics at all, so nothing flagged the invented style before.
Query parameters in the same document are untouched: they keep
form/true.Out of scope, and stated here because the change makes it reachable: 3.2 also forbids
explodeandallowReservedatin: querystring, and the parser validates neither. Adocument that declares
explodethere without a style now loses it with the style it wasqualifying, and a declared
allowReservedstill lowers onto the binding as it did before.Test plan
testdata/conformance/openapi/param-querystring.yamljoins the conformance corpus with thecontrast the golden needs: a querystring binding, and an ordinary query parameter in its own
operation (3.2 forbids the two sharing one) that keeps the defaults its own location does
admit.
assertParamQuerystringreads both, so the golden shows the difference is thelocation rather than the presence of
content.TestParams_QueryStringLocationpreviously asserted only the location, from a fixture thatdeclared
schema— which 3.2 forbids there too — and discarded its diagnostics. It nowcompiles a valid content-bound parameter, requires no error diagnostics, and pins the whole
binding: location, media type, no style, no explode.
TestParams_QueryStringDeclaredStyleIsKeptAndReportedpins the other half: a declared stylesurvives and the validation error is reported.
Both assertions bite. Restoring the old arm reddens them:
The golden moves with them: compiling the new corpus spec with the old arm restored differs
from the committed golden by exactly the two keys above. Deleting a property from the fixture
reddens the golden comparison too, so the snapshot is reading the spec rather than agreeing
with itself.
Gate:
gofmtclean,go vet ./...clean,golangci-lint runreports 0 issues,go build ./...succeeds,
./scripts/check-coverage.shpasses at 100% (4945 statements).Closes #334
Follow-up in this branch: a declared
explodeis keptSelf-review caught a regression the first commit introduced. Suppressing the
invented style took a declared
explodewith it, becauseresolveStyleExplodereturned early whenever the resolved style was empty:That was the same silent drop the invented style was, in the other direction,
and it contradicted the rule the style half already states: 3.2 forbids both
keywords at this location, the bundled parser refuses neither, so both reach the
compiler — and dropping content a document states is an emitter's call rather
than a compiler's. A declared
explodenow lowers as declared wherever it iswritten; only the default is suppressed where there is no style to qualify.
TestParams_QueryStringDeclaredStyleIsKeptAndReportedcould not catch this: itdeclares style and explode together, so the early return was never reached.
TestParams_QueryStringDeclaredExplodeAloneIsKeptcovers the gap, and restoringthe exact pre-fix body of
resolveStyleExplodefails that test and only thattest.