Repository navigation
fix(compilers/openapi): warn on allowReserved outside query - #725
fuad-daoud wants to merge 6 commits into
Conversation
OpenAPI 3.0/3.1 apply allowReserved to in: query alone; 3.2 widened it to every location. A path, header or cookie declaration in a 3.0/3.1 document lowered as declared and said nothing, so the IR of a spec using the keyword where the dialect does not was indistinguishable from a legal one. This adds the second rule under the existing openapi/invalid-location-keyword code, warning at the keyword's own pointer, presence rather than truth, and still lowering the value as declared (invariant 2). - lowering.Ctx.AllowReservedIsQueryOnly reads load.SupportedMinor the way ExclusiveBoundIsBoolean beside it does; operation's archtest allowlist excludes load, so the dialect question cannot be answered from there. - allowReservedLocationDiag reports path/header/cookie in 3.0/3.1 and stays out of querystring, whose single #408 report is unchanged. - conformance fixture param-allowreserved-locations.yaml + golden; the param-styles assertion now pins the 3.1 query declaration as silent.
fuad-daoud
changed the title
Resolve the conflicts with main and bring the doc comments this branch adds under the 100-word cap main now enforces. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GCzyGPq9DQ7RLfagZm2JB5
| // AllowReservedIsQueryOnly reports whether this document's dialect confines | ||
| // allowReserved to in: query. OpenAPI 3.0 and 3.1 apply the keyword at the | ||
| // query location alone, so a path, header or cookie declaration is one the | ||
| // dialect does not use; 3.2 widens it to every location. |
There was a problem hiding this comment.
This premise looks wrong for 3.2. The 3.2 text for allowReserved says "This field only applies to in and style values that automatically percent-encode", so 3.2 did not widen the keyword to every location. in: header never percent-encodes ("URI percent-encoding MUST NOT be applied"), and neither does in: cookie with style: cookie; path, query and default-style (form) cookies do.
Leaving 3.2 silent is fine as scope, since #628 covers 3.0 and 3.1. But the claim that 3.2 "applies the keyword at every location" should be reworded wherever it appears, and the 3.2 in: header gap recorded where the next reader will reach it. The 3.2 block of TestParams_AllowReservedOutsideQueryIsSilentElsewhere currently asserts that silence as intended behaviour.
There was a problem hiding this comment.
The same claim is in these places, so rewording needs to cover all of them: lowering.go:358; lowering_test.go:204, 231 and 237; params.go:103; params_test.go:411, 450 and 467; the OpenAPI 3.x row of the §14 table in docs/ir-design.md ("while 3.2 applies the keyword at every location and is silent"); the first commit's message; and the PR summary.
The commit message matters most: with the repo's COMMIT_MESSAGES squash setting it is pasted into the commit on main. (params_test.go:400 only says the declared value is kept at each tested location, and is fine as written.)
There was a problem hiding this comment.
Agreed. 3.2 limits allowReserved to in/style values that percent-encode, so header and style: cookie are excluded. dcc489f removes the "every location" and "location-agnostic" wording from every site you listed (lowering.go, lowering_test.go, params.go, params_test.go and the ir-design §14 row). The doc comment on allowReservedLocationDiag and the §14 clause now record that 3.2's rule isn't reported, and an issue will track it. The 3.2 rows are still silent, but the test comment now calls that a scoping choice, not a rule. dbf33cf can't be rewritten, so its message will be replaced by an explicit squash body at merge.
| return nil | ||
| } | ||
| switch in { | ||
| case soa.ParameterInPath, soa.ParameterInHeader, soa.ParameterInCookie: |
There was a problem hiding this comment.
The exclusion of in: querystring here is not pinned by any test. Adding soa.ParameterInQueryString to this case list leaves go test ./compilers/openapi/... ./cmd/... green, and a 3.1 document with in: querystring plus allowReserved: true then reports two warnings at .../parameters/0/allowReserved (the #408 one and this one).
The existing querystring tests all use 3.2, where AllowReservedIsQueryOnly is false and this function returns before the switch, so none of them reach the case list. A small 3.1 case asserting exactly one invalid-location-keyword at that pointer would pin the claim the PR description and the comment above make.
There was a problem hiding this comment.
Reproduced: the mutation left everything green. TestParams_AllowReservedLocationRule now has a 3.1.0 in: querystring row. It requires exactly one location-keyword report at /paths/~1p/get/parameters/0/allowReserved across both codes: the #408 invalid-location-keyword with its message. With querystring added to the case list, that row fails.
| default: | ||
| return nil | ||
| } | ||
| return []ir.Diagnostic{c.DiagAt(ir.SeverityWarning, diag.InvalidLocationKeyword, |
There was a problem hiding this comment.
For a parameter that arrives through an external $ref (allow-external-refs=true) two things differ from the description, both reproduced with a two-file spec:
- The dialect is read from the root document (
c.Doc), so a 3.1 root pulling a parameter from a 3.2 document warns "before OpenAPI 3.2", and a 3.2 root pulling one from a 3.0.3 document stays silent. ObjectAtkeeps the use-site pointer for a reference that leaves the document, so the warning lands at<operation>/parameters/<i>/allowReserved. That entry is a bare$refwith no such key; the keyword is in the other file.
The querystring rule beside this has the same pointer behaviour, so this may be acceptable, but "reported at the keyword's own coordinate" is not true for this case and is worth stating.
There was a problem hiding this comment.
I reproduced both directions with a two-file spec and allow-external-refs=true: a 3.1 root referencing a parameter in a 3.2 file warns "before OpenAPI 3.2" at the use-site .../parameters/0/allowReserved, and a 3.2 root referencing one in a 3.0.3 file stays silent.
This is the same shape as the existing in: querystring rule, which also reports at the use-site pointer when the parameter arrives through an external $ref, and as ExclusiveBoundIsBoolean, which also reads the root document's version. So I would not change the behaviour in this PR. What the PR should do is state the limitation: "reported at the keyword's own coordinate" in the PR body, and the doc comment on allowReservedLocationDiag, are not true for a parameter that comes from another file.
There was a problem hiding this comment.
Reproduced both directions with the CLI. Behaviour is unchanged, as you suggested, since it matches the querystring rule and the exclusive-bound check. The limitation is now stated in the allowReservedLocationDiag doc comment, in ir-design §14 and in the PR body.
| // which matches ExclusiveBoundIsBoolean's choice of the unrecognized-version | ||
| // answer and keeps the reader from inventing a rule for a document it cannot | ||
| // place. In production load refuses an unsupported version before any lowering | ||
| // runs; this is what lets a zero context answer rather than panic. |
There was a problem hiding this comment.
The last sentence does not follow. load refusing an unsupported version is not what lets a zero context answer without panicking; that is GetOpenAPI being nil-safe on a nil *soa.OpenAPI, which is what the ExclusiveBoundIsBoolean comment above says.
There was a problem hiding this comment.
Two more reasons the sentence is off, both from compiling specs with the built CLI. openapi: 3.3.0 and 4.0.0 never reach load: the engine stops them with engine/no-compiler-for-format. And a malformed two-part version such as openapi: "3.1" draws load's validation-supported-version errors, yet lowering still runs and this rule still fires. So "load refuses an unsupported version before any lowering runs" does not hold as written.
ExclusiveBoundIsBoolean makes no such claim. The PR body's "unreachable in production because load refuses unsupported versions" needs the same trim.
There was a problem hiding this comment.
Agreed, and the follow-up holds. 3.3.0 stops at engine/no-compiler-for-format, and a two-part "3.1" draws load's validation errors but still lowers as 3.1, so the rule fires on it (TestDialectMinor_FollowsTheDocument pins "3.1"→"3.1"). The comment now credits the nil-safe GetOpenAPI only.
| // The bundled parser enforces this for style at that location but not for | ||
| // these two (GitHub #408), so the compiler reports the gap itself. The value | ||
| // still lowers as declared: dropping stated content is an emitter's call | ||
| // The bundled parser enforces this for style at querystring but not for these |
| // OpenAPI 3.2 binds the whole query string from the parameter's content there | ||
| // and states its serialization through the media type alone, so neither | ||
| // keyword has anything left to qualify. | ||
| // InvalidLocationKeyword reports a serialization keyword at a parameter |
There was a problem hiding this comment.
Both rules now report under one code, so a consumer that wants to treat the 3.2 in: querystring violations (a MUST NOT) differently from the before-3.2 dialect mismatch (a keyword the dialect simply has no use for) has to match on the message text. Worth confirming one code is the intent, or giving the second rule its own code.
There was a problem hiding this comment.
Split (eed9bd9). The pre-3.2 rule now reports openapi/inapplicable-location-keyword, and invalid-location-keyword stays with the 3.2 querystring MUST NOT. A consumer can tell them apart by code, not message text. The table test asserts each row's code, and putting the rule back on the old code reddens it, the per-declaration test and the conformance case.
| // answer and keeps the reader from inventing a rule for a document it cannot | ||
| // place. In production load refuses an unsupported version before any lowering | ||
| // runs; this is what lets a zero context answer rather than panic. | ||
| func (c Ctx) AllowReservedIsQueryOnly() bool { |
There was a problem hiding this comment.
This body is the same as ExclusiveBoundIsBoolean above apart from the comparison. lowering.Ctx is at 18 of the 20 methods TestMethodsPerType_StayUnderTheCap allows, and every per-keyword dialect question adds one. A single accessor for the minor version on Ctx (callers compare it) would answer both questions and the next one without growing the type.
There was a problem hiding this comment.
Done. Ctx now has a single DialectMinor() string, and callers compare it. It replaces both ExclusiveBoundIsBoolean and AllowReservedIsQueryOnly, so Ctx loses two methods and gains one. I swapped the comparison in each schema.go/params.go caller and checked that every caller's existing tests go red.
| // TestParams_AllowReservedOutsideQueryIsSilentElsewhere is the control for the | ||
| // rule above: the 3.0/3.1 warning tracks the keyword *and* its location, so a | ||
| // query declaration, an absent keyword and every 3.2 location all stay silent. | ||
| func TestParams_AllowReservedOutsideQueryIsSilentElsewhere(t *testing.T) { |
There was a problem hiding this comment.
This packs three independent scenarios into one body, with doc, diags = reassigned between them, so a failing require in the first hides the other two. A table over version, location, keyword present and expected warning would be flat, match the testing convention, and let the 3.2 header/cookie rows be changed in one place if the 3.2 question on AllowReservedIsQueryOnly is resolved.
There was a problem hiding this comment.
Whichever shape the test takes, the PR body's "reports once" claim for a $ref'd component parameter has no test. Compiling by hand: a 3.1 cookie parameter declaring allowReserved: true in components/parameters, referenced from several operations, gives one warning at #/components/parameters/Shared/allowReserved; a path-item-level parameter reports once at /paths/~1a/parameters/0/allowReserved; parameters under callbacks and webhooks each report at their own pointer. All of that behaves sensibly, but only the plain operation-level case is pinned. Rows for a $ref'd parameter, a path-level parameter, a callback and a webhook would pin the rest.
There was a problem hiding this comment.
Done. TestParams_AllowReservedLocationRule is one flat table over version × location × declared value × expected code and report, covering both the reported and the silent cases.
On the follow-up: added TestParams_AllowReservedOutsideQueryReportsOncePerDeclaration. Its spec covers a component $ref'd from two operations, a path-item parameter shared by two operations, a callback parameter and a webhook parameter. It requires exactly one inapplicable-location-keyword report at each declaration's own pointer, and four in total.
|
Notes on the PR description (the inline comments cover the code):
|
OpenAPI 3.2 did not widen allowReserved to every location: it applies the keyword only to in/style values that percent-encode, which excludes in: header and in: cookie with style: cookie. The rule here stays scoped to 3.0 and 3.1, so 3.2 is silent, but the code, tests and ir-design §14 now say why rather than claiming 3.2 allows it everywhere, and they record that 3.2's own rule goes unreported. They also record that a parameter reached through an external $ref is judged by the root document's version and reported at its use-site pointer, as the querystring rule beside it already is. lowering.Ctx answers the dialect with DialectMinor, which callers compare, in place of one predicate per keyword. It replaces ExclusiveBoundIsBoolean and AllowReservedIsQueryOnly, so the next dialect question does not grow a type near its method cap. The operation tests become one flat table over version, location, declared value and expected report. A 3.1 in: querystring row pins its single #408 report, so adding querystring to the rule's location set now reddens. A second test pins one report per declaration for a $ref'd component, a path-item parameter, a callback and a webhook.
The fixture added on this branch predates the idSpaces and group IDs main introduced with IR 0.7.0; the merge brought those in without touching the golden. Only those fields change.
allowReserved at path, header or cookie in a 3.0 or 3.1 document now reports openapi/inapplicable-location-keyword instead of sharing openapi/invalid-location-keyword with the 3.2 querystring rule. The querystring case breaks a MUST NOT; this one is a keyword its dialect simply ignores. A consumer that weighs the two differently can now tell them apart by code rather than by message text.
|
Thanks. All three notes are addressed in the updated PR description:
The first commit's message still carries the old 3.2 claim. The branch won't be rewritten, so I'll set the squash body by hand at merge rather than paste the commit messages. |
2 participants
Summary
allowReservedis a query-only keyword in OpenAPI 3.0 and 3.1: both dialects apply it toin: queryalone. 3.2 replaced that rule with a different one. It applies the keyword only toin/stylevalues that percent-encode, which leaves outin: headerandin: cookiewithstyle: cookie. This PR covers 3.0 and 3.1 only (#628). The compiler already kept every declared value onir.HTTPParamBinding.AllowReservedand said nothing there, so a 3.0/3.1 document declaringallowReservedatpath,headerorcookieproduced IR indistinguishable from a legal one.Rule. A new diagnostic code, reported at the keyword's own coordinate:
openapi/inapplicable-location-keyword<parameters>/<i>/allowReservedparameter field allowReserved only applies to in=query before OpenAPI 3.2; lowered as declared3.0or3.1, the keyword is present (presence, not truth: a declaredallowReserved: falseis reported too, matching the querystring check and theallowEmptyValuediscipline), andinispath,headerorcookieA parameter reached through an external
$ref(allow-external-refs=true) is the exception to "the keyword's own coordinate". It is judged by the root document's version and reported at its use-site pointer (<operation>/parameters/<i>/allowReserved), where the entry is a bare$ref. Thein: querystringrule beside it and the exclusive-bound dialect check already behave the same way. The doc comment onallowReservedLocationDiagand the ir-design §14 clause state this.Where it lives.
lowering.Ctx.DialectMinor()returns the document's minor version ("3.0"/"3.1"/"3.2", or""when unrecognized), and callers compare it. It replacesExclusiveBoundIsBoolean, whose two callers now compare== "3.0", so a new dialect question no longer adds a method toCtx(at 18 of its 20-method cap).operation's archtest allowlist excludesload, which is why the version is read throughCtx. A context with no document answers""without panicking, becauseGetOpenAPIis nil-safe.allowReservedLocationDiagsits besidequerystringKeywordDiagsand is called fromlowerParameterbeside the two existing diagnostic calls.in: querystringis deliberately outside the location set. The parser admits that location in any version, so including it would put two location-keyword warnings (openapi: explode and allowReserved at in: querystring are kept in silence #408's and this one) at one pointer for a 3.1 document and change openapi: explode and allowReserved at in: querystring are kept in silence #408's behaviour.diag.InapplicableLocationKeywordis new and joins the code list indiag_test.go.diag.InvalidLocationKeyword's GoDoc now describes the 3.2 querystring rule alone, and thedocs/ir-design.md§14 OpenAPI census clause names both. The before-3.2 rule reports its own code because, unlike the querystring case, it breaks no MUST, so a consumer can weigh it separately.Not changed. The value still lands in
ir.HTTPParamBinding.AllowReservedat every location and version: dropping declared content is an emitter's call, not a compiler's (invariant 2). 3.2 stays silent at every location, including thein: headerandstyle: cookiedeclarations its percent-encoding rule leaves out (see Deliberately not done). A 3.1/3.0in: querydeclaration stays silent, and the 3.2in: querystringreports from #408 are untouched. Duplicates are already impossible, sincecompile.Diags.Appenddedups by full diagnostic identity, so one$ref'd component parameter mounted at several operations reports once.Test plan
go test ./compilers/openapi/internal/lowering/:TestDialectMinor_FollowsTheDocument(3.0.x→3.0, 3.1.x→3.1, 3.2.0→3.2, a two-part"3.1"→3.1, 4.0.0/""→"") andTestDialectMinor_TheZeroContextNamesNoDialect.go test ./compilers/openapi/internal/operation/:TestParams_AllowReservedLocationRuleis one flat table over version × location × declared value × expected code and report. It covers 3.0.3/3.1.0 path, header and cookie (one declaringfalse), the query, undeclared and 3.2 controls, and a 3.1in: querystringrow that pins exactly one report across both codes, the openapi: explode and allowReserved at in: querystring are kept in silence #408 one.TestParams_AllowReservedOutsideQueryReportsOncePerDeclarationpins one report per declaration at its own pointer. Its spec has a$ref'd component mounted at two operations, a path-item-level parameter shared by two operations, a callback operation's parameter and a webhook's.go test ./compilers/openapi -run TestConformance: newparam-allowreserved-locations.yamlfixture (3.1.0: path true, header true, cookie false, query true as the version-independent control) with its golden. It expects exactly threeinapplicable-location-keywordwarnings, in source order, at the three keyword pointers, and"format": "openapi@3.1". No existing golden moved andunwitnessed.golden.txtis untouched. The fixture's golden was regenerated after main's IR 0.7.0 merge. OnlyirVersion,idSpacesand the default group'sidchanged (ce3e81a).assertParamStylesnow uses itsdiagsargument to pin the 3.1 query declaration as silent. That is the version-independent half of the rule, on a spec that already declaredallowReservedat a query parameter.in: querystringto the case list reddens the querystring row.invalid-location-keywordreddens the table, the per-declaration test and the conformance case.-update).go run ./cmd/morphic-harness testdata/conformance/openapi/param-allowreserved-locations.yaml→ok(irverify, round-trip, determinism, order-invariance).make gategreen on eed9bd9, including the 100 % statement gate.Deliberately not done
allowReservedonly toin/stylevalues that percent-encode, which excludesin: headerandin: cookiewithstyle: cookie. Style combinations 3.2 calls unusable (e.g.deepObjectorformwithallowReserved) are not reported either. openapi: allowReserved on a path or header parameter is kept without a warning #628 scopes the rule to 3.0/3.1 locations; openapi: allowReserved where OpenAPI 3.2 does not apply it is kept without a warning #819 tracks the 3.2 rule.in: querystringin a 3.0/3.1 document keeps its existing single openapi: explode and allowReserved at in: querystring are kept in silence #408 warning; that location's version legality is not this issue.components/parametersentries are not lowered today, so an unreferenced one declaringallowReservedat a path stays unreported, exactly as every other per-lowering rule (e.g.reservedHeaderParamDiag).Closes #628.