diff --git a/compilers/openapi/conformance_test.go b/compilers/openapi/conformance_test.go index 17ae7d9..56e3f38 100644 --- a/compilers/openapi/conformance_test.go +++ b/compilers/openapi/conformance_test.go @@ -180,6 +180,7 @@ func conformanceCases() []conformanceCase { {"tags-grouping", assertTagsGrouping}, {"http-binding", assertHTTPBinding}, {"param-styles", assertParamStyles}, + {"param-querystring", assertParamQuerystring}, {"param-xml-residue", assertParamXMLResidue}, {"param-ref-inheritance", assertParamRefInheritance}, {"header-content-schema", assertHeaderContentSchema}, @@ -1452,6 +1453,35 @@ func assertAllowEmptyValueKept(t *testing.T, op ir.Operation) { assert.JSONEq(t, `true`, string(entry.Value)) } +// assertParamQuerystring pins the 3.2 querystring location, where the whole +// query string binds from the parameter's content: the binding carries that +// media type and neither style nor explode, the two keywords the location +// forbids (GitHub #334). The ordinary query parameter beside it keeps the +// defaults its own location does admit, so what separates them is the location +// rather than the presence of content. +func assertParamQuerystring(t *testing.T, doc *ir.Document, _ []ir.Diagnostic) { + report, ok := opByName(doc, "runReport") + require.True(t, ok) + require.Len(t, report.Bindings.HTTP, 1) + require.Len(t, report.Bindings.HTTP[0].ParamBindings, 1) + qs := report.Bindings.HTTP[0].ParamBindings[0] + assert.Equal(t, ir.HTTPLocationQuerystring, qs.Location) + assert.Equal(t, "application/x-www-form-urlencoded", qs.ContentType, + "the media type is the whole of a querystring binding's declared serialization") + assert.Empty(t, qs.Style, "style is not a legal keyword at in: querystring") + assert.Nil(t, qs.Explode, "and explode qualifies a style there is none of") + + summary, ok := opByName(doc, "summarize") + require.True(t, ok) + require.Len(t, summary.Bindings.HTTP, 1) + require.Len(t, summary.Bindings.HTTP[0].ParamBindings, 1) + q := summary.Bindings.HTTP[0].ParamBindings[0] + assert.Equal(t, ir.HTTPLocationQuery, q.Location) + assert.Equal(t, "form", q.Style, "a query parameter still takes its own location's default") + require.NotNil(t, q.Explode) + assert.True(t, *q.Explode) +} + // assertParamRefInheritance pins ir-design §14 at a parameter whose schema is a // $ref: docs, deprecation and default come from the referent when the use site is // silent, and from the use site when it is not. Constraints inherit at neither diff --git a/compilers/openapi/internal/operation/params.go b/compilers/openapi/internal/operation/params.go index e0875ee..24a0366 100644 --- a/compilers/openapi/internal/operation/params.go +++ b/compilers/openapi/internal/operation/params.go @@ -307,25 +307,40 @@ func preserveAllowEmptyValue(c lowering.Ctx, param *ir.Parameter, p *soa.Paramet // resolveStyleExplode materializes a parameter's resolved serialization style // and explode flag: an explicit value wins, else the OpenAPI per-location -// default (query/cookie → form/true, path/header → simple/false). The result is -// declared facts, not policy. +// default (query/cookie → form/true, path/header → simple/false, querystring → +// neither). The result is declared facts, not policy. func resolveStyleExplode(p *soa.Parameter, in soa.ParameterIn) (string, *bool) { style := defaultParamStyle(in) if p.Style != nil { style = string(*p.Style) } - explode := style == string(soa.SerializationStyleForm) + // A declared explode lowers as declared wherever it is written, for the same + // reason a declared style does: 3.2 forbids both at in: querystring, but + // dropping content the document states is an emitter's call, not a + // compiler's, and this position has an IR field to hold it. if p.Explode != nil { - explode = *p.Explode + explode := *p.Explode + return style, &explode + } + // Absent one, explode qualifies a style, so a position resolving to no style + // gets no default either — a querystring binding's ContentType is the whole + // of its declared serialization (GitHub #334). + if style == "" { + return "", nil } + explode := style == string(soa.SerializationStyleForm) return style, &explode } // defaultParamStyle returns the OpenAPI default serialization style for a -// parameter location. +// parameter location, and "" for one that has none: 3.2 binds in: querystring +// from the parameter's content and forbids style there, so that location has no +// default to fall back on. func defaultParamStyle(in soa.ParameterIn) string { switch in { - case soa.ParameterInQuery, soa.ParameterInCookie, soa.ParameterInQueryString: + case soa.ParameterInQueryString: + return "" + case soa.ParameterInQuery, soa.ParameterInCookie: return string(soa.SerializationStyleForm) default: return string(soa.SerializationStyleSimple) diff --git a/compilers/openapi/internal/operation/params_test.go b/compilers/openapi/internal/operation/params_test.go index ab7feed..0df0aed 100644 --- a/compilers/openapi/internal/operation/params_test.go +++ b/compilers/openapi/internal/operation/params_test.go @@ -3,6 +3,7 @@ package operation_test import ( "testing" + "github.com/speakeasy-api/openapi/validation" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -180,19 +181,103 @@ func TestParams_AllLocationsAndStyles(t *testing.T) { assert.True(t, hasDiag(diags, diag.NumericPrecision), "malformed param constraint warns") } +// TestParams_QueryStringLocation pins the whole of a querystring binding's +// declared serialization: the 3.2 location, and the media type from content +// carrying it alone. Style and explode are not legal keywords there, so the +// binding takes neither rather than the query defaults (GitHub #334). func TestParams_QueryStringLocation(t *testing.T) { t.Parallel() spec := pathsSpecVer("3.2.0", ` /q: get: operationId: q parameters: - - {name: qs, in: querystring, schema: {type: string}} + - name: qs + in: querystring + content: + application/x-www-form-urlencoded: + schema: {type: object, properties: {page: {type: string}}} responses: "200": {description: ok} `) - doc, _ := parseFull(t, spec) + doc, diags := parseFull(t, spec) + requireNoErrorDiags(t, diags) op := findOp(t, doc, "q") - assert.Equal(t, ir.HTTPLocationQuerystring, op.Bindings.HTTP[0].ParamBindings[0].Location) + require.Len(t, op.Bindings.HTTP, 1) + require.Len(t, op.Bindings.HTTP[0].ParamBindings, 1) + + qs := op.Bindings.HTTP[0].ParamBindings[0] + assert.Equal(t, ir.HTTPLocationQuerystring, qs.Location) + assert.Equal(t, "application/x-www-form-urlencoded", qs.ContentType) + assert.Empty(t, qs.Style, "3.2 binds the query string from content and forbids style there") + assert.Nil(t, qs.Explode, "and explode with it: it qualifies a style, and there is none") +} + +// TestParams_QueryStringDeclaredStyleIsKeptAndReported pins the other half of +// the same rule: the compiler stopped inventing a style at that location, it did +// not start erasing one. A document declaring style there is invalid and the +// parser says so; what it declared still lowers, because dropping declared +// content is an emitter's call rather than a compiler's. +func TestParams_QueryStringDeclaredStyleIsKeptAndReported(t *testing.T) { + t.Parallel() + spec := pathsSpecVer("3.2.0", ` /q: + get: + operationId: q + parameters: + - name: qs + in: querystring + style: form + explode: false + content: + application/x-www-form-urlencoded: + schema: {type: object} + responses: + "200": {description: ok} +`) + doc, diags := parseFull(t, spec) + assertHasCode(t, diags, diag.Validation+"/"+string(validation.RuleValidationAllowedValues), ir.SeverityError) + op := findOp(t, doc, "q") + require.Len(t, op.Bindings.HTTP, 1) + require.Len(t, op.Bindings.HTTP[0].ParamBindings, 1) + + qs := op.Bindings.HTTP[0].ParamBindings[0] + assert.Equal(t, "form", qs.Style, "the declared style lowers as declared") + require.NotNil(t, qs.Explode) + assert.False(t, *qs.Explode, "and so does the explode qualifying it") +} + +// TestParams_QueryStringDeclaredExplodeAloneIsKept covers the half of that rule +// the case above cannot see, because it declares both keywords: an explode +// written without a style beside it. +// +// Suppressing the invented style must not take a declared explode with it. 3.2 +// forbids explode at this location as it forbids style, and the bundled parser +// refuses neither — so this reaches the compiler, and erasing it would be the +// same silent drop the invented style was, in the other direction. +func TestParams_QueryStringDeclaredExplodeAloneIsKept(t *testing.T) { + t.Parallel() + spec := pathsSpecVer("3.2.0", ` /q: + get: + operationId: q + parameters: + - name: qs + in: querystring + explode: false + content: + application/x-www-form-urlencoded: + schema: {type: object} + responses: + "200": {description: ok} +`) + doc, diags := parseFull(t, spec) + requireNoErrorDiags(t, diags) + op := findOp(t, doc, "q") + require.Len(t, op.Bindings.HTTP, 1) + require.Len(t, op.Bindings.HTTP[0].ParamBindings, 1) + + qs := op.Bindings.HTTP[0].ParamBindings[0] + assert.Empty(t, qs.Style, "no style is invented at this location") + require.NotNil(t, qs.Explode, "but the declared explode is not dropped with it") + assert.False(t, *qs.Explode) } const componentParamRefSpec = `openapi: 3.1.0 diff --git a/testdata/conformance/openapi/param-querystring.golden.json b/testdata/conformance/openapi/param-querystring.golden.json new file mode 100644 index 0000000..f6fcd78 --- /dev/null +++ b/testdata/conformance/openapi/param-querystring.golden.json @@ -0,0 +1,281 @@ +{ + "irVersion": "0.3.0", + "name": "ParamQuerystring", + "version": "1.0.0", + "docs": {}, + "services": [ + { + "id": "s/openapi/0", + "name": { + "source": "ParamQuerystring", + "canonical": "param_querystring" + }, + "docs": {}, + "groups": [ + { + "name": { + "hint": "default" + }, + "docs": {}, + "operations": [ + { + "id": "op/openapi/paths/~1reports/get", + "name": { + "source": "runReport", + "canonical": "run_report" + }, + "docs": {}, + "params": [ + { + "name": { + "source": "criteria", + "canonical": "criteria" + }, + "type": { + "target": "t/anon/paths/~1reports/get/parameters/0/content/application~1x-www-form-urlencoded/schema", + "nullable": false + }, + "required": false, + "docs": {} + } + ], + "responses": [ + { + "name": { + "hint": "200" + }, + "conditions": { + "statusCodes": [ + { + "from": 200, + "to": 200 + } + ] + }, + "docs": { + "description": "ok" + } + } + ], + "oneWay": false, + "idempotency": {}, + "auth": null, + "bindings": { + "http": [ + { + "method": "GET", + "uriTemplate": "/reports", + "sharedRoute": false, + "paramBindings": [ + { + "param": "criteria", + "location": "querystring", + "wireName": "criteria", + "allowReserved": false, + "contentType": "application/x-www-form-urlencoded" + } + ], + "checksumRequired": false, + "isWebhook": false + } + ] + }, + "provenance": { + "source": 0, + "pointer": "/paths/~1reports/get" + } + }, + { + "id": "op/openapi/paths/~1reports~1summary/get", + "name": { + "source": "summarize", + "canonical": "summarize" + }, + "docs": {}, + "params": [ + { + "name": { + "source": "status", + "canonical": "status" + }, + "type": { + "target": "t/prim/string", + "nullable": false + }, + "required": false, + "docs": {} + } + ], + "responses": [ + { + "name": { + "hint": "200" + }, + "conditions": { + "statusCodes": [ + { + "from": 200, + "to": 200 + } + ] + }, + "docs": { + "description": "ok" + } + } + ], + "oneWay": false, + "idempotency": {}, + "auth": null, + "bindings": { + "http": [ + { + "method": "GET", + "uriTemplate": "/reports/summary", + "sharedRoute": false, + "paramBindings": [ + { + "param": "status", + "location": "query", + "wireName": "status", + "style": "form", + "explode": true, + "allowReserved": false + } + ], + "checksumRequired": false, + "isWebhook": false + } + ] + }, + "provenance": { + "source": 0, + "pointer": "/paths/~1reports~1summary/get" + } + } + ] + } + ], + "auth": null, + "provenance": { + "source": 0 + } + } + ], + "types": { + "t/anon/paths/~1reports/get/parameters/0/content/application~1x-www-form-urlencoded/schema": { + "kind": "model", + "id": "t/anon/paths/~1reports/get/parameters/0/content/application~1x-www-form-urlencoded/schema", + "name": { + "hint": "criteria" + }, + "anonymous": true, + "docs": {}, + "sensitive": false, + "provenance": { + "source": 0, + "pointer": "/paths/~1reports/get/parameters/0/content/application~1x-www-form-urlencoded/schema" + }, + "properties": [ + { + "id": "p/openapi/paths/~1reports/get/parameters/0/content/application~1x-www-form-urlencoded/schema/properties/since", + "name": { + "source": "since", + "canonical": "since" + }, + "wireName": "since", + "type": { + "target": "t/prim/date", + "nullable": false + }, + "required": false, + "clientOptional": false, + "defaultAdded": false, + "visibility": { + "none": false + }, + "flatten": false, + "eventHeader": false, + "eventPayload": false, + "secret": false, + "docs": {}, + "provenance": { + "source": 0, + "pointer": "/paths/~1reports/get/parameters/0/content/application~1x-www-form-urlencoded/schema/properties/since" + } + }, + { + "id": "p/openapi/paths/~1reports/get/parameters/0/content/application~1x-www-form-urlencoded/schema/properties/status", + "name": { + "source": "status", + "canonical": "status" + }, + "wireName": "status", + "type": { + "target": "t/prim/string", + "nullable": false + }, + "required": false, + "clientOptional": false, + "defaultAdded": false, + "visibility": { + "none": false + }, + "flatten": false, + "eventHeader": false, + "eventPayload": false, + "secret": false, + "docs": {}, + "provenance": { + "source": 0, + "pointer": "/paths/~1reports/get/parameters/0/content/application~1x-www-form-urlencoded/schema/properties/status" + } + } + ], + "abstract": false, + "positional": false, + "inputOnly": false + }, + "t/prim/date": { + "kind": "primitive", + "id": "t/prim/date", + "name": {}, + "anonymous": false, + "docs": {}, + "sensitive": false, + "provenance": { + "source": 0 + }, + "prim": "date" + }, + "t/prim/string": { + "kind": "primitive", + "id": "t/prim/string", + "name": {}, + "anonymous": false, + "docs": {}, + "sensitive": false, + "provenance": { + "source": 0 + }, + "prim": "string" + } + }, + "servers": [ + { + "name": { + "hint": "server" + }, + "urlTemplate": "/", + "description": {}, + "auth": null + } + ], + "sources": [ + { + "format": "openapi@3.2", + "path": "param-querystring.yaml", + "hash": "f0cf6d786e63396444a439407e38ad19f5dbcb65928593e9d8818c5baf01b463" + } + ] +} diff --git a/testdata/conformance/openapi/param-querystring.yaml b/testdata/conformance/openapi/param-querystring.yaml new file mode 100644 index 0000000..0026b95 --- /dev/null +++ b/testdata/conformance/openapi/param-querystring.yaml @@ -0,0 +1,30 @@ +openapi: 3.2.0 +info: {title: ParamQuerystring, version: "1.0.0"} +paths: + # 3.2 binds the whole query string from the parameter's content, so style and + # explode are not legal keywords here and the binding carries neither. + /reports: + get: + operationId: runReport + parameters: + - name: criteria + in: querystring + content: + application/x-www-form-urlencoded: + schema: + type: object + properties: + since: {type: string, format: date} + status: {type: string} + responses: + "200": {description: ok} + # The contrast, in its own operation because a querystring parameter may not + # share one with an in: query parameter: a location that does admit the + # defaults, so the golden shows the difference is the location. + /reports/summary: + get: + operationId: summarize + parameters: + - {name: status, in: query, schema: {type: string}} + responses: + "200": {description: ok}