diff --git a/.chronus/changes/uptake-uri-templates-2024-6-26-21-7-49.md b/.chronus/changes/uptake-uri-templates-2024-6-26-21-7-49.md new file mode 100644 index 0000000000..693b68556f --- /dev/null +++ b/.chronus/changes/uptake-uri-templates-2024-6-26-21-7-49.md @@ -0,0 +1,8 @@ +--- +# Change versionKind to one of: internal, fix, dependencies, feature, deprecation, breaking +changeKind: internal +packages: + - "@azure-tools/typespec-azure-core" +--- + +Call correct HTTP library API diff --git a/.chronus/changes/uptake-uri-templates-2024-7-6-9-38-18.md b/.chronus/changes/uptake-uri-templates-2024-7-6-9-38-18.md new file mode 100644 index 0000000000..40b3c23d94 --- /dev/null +++ b/.chronus/changes/uptake-uri-templates-2024-7-6-9-38-18.md @@ -0,0 +1,6 @@ +--- +changeKind: internal +packages: + - "@azure-tools/typespec-client-generator-core" +--- + diff --git a/.chronus/changes/uptake-uri-templates-2024-7-6-9-38-7.md b/.chronus/changes/uptake-uri-templates-2024-7-6-9-38-7.md new file mode 100644 index 0000000000..2a4e348bd1 --- /dev/null +++ b/.chronus/changes/uptake-uri-templates-2024-7-6-9-38-7.md @@ -0,0 +1,7 @@ +--- +changeKind: feature +packages: + - "@azure-tools/typespec-autorest" +--- + +Add support for URI templates in routes diff --git a/core b/core index e1ad7c8b46..e492ff7d7b 160000 --- a/core +++ b/core @@ -1 +1 @@ -Subproject commit e1ad7c8b466e05087d8a15fb97538680483aa0a7 +Subproject commit e492ff7d7ba4aac89aac73e594d547d267d05b02 diff --git a/docs/libraries/azure-core/rules/no-query-explode.md b/docs/libraries/azure-core/rules/no-query-explode.md new file mode 100644 index 0000000000..f431555ab6 --- /dev/null +++ b/docs/libraries/azure-core/rules/no-query-explode.md @@ -0,0 +1,35 @@ +--- +title: "no-query-explode" +--- + +```text title="Full name" +@azure-tools/typespec-azure-core/no-query-explode +``` + +Azure services favor serializing query parameter of array type using the simple style(Where each values is joined by a `,`) +Using the `*` uri template modifier or `explode: true` property specify that each individual value of the array should be sent as a separate query parameter. + +#### ❌ Incorrect + +```tsp +op list( + @query(#{ explode: true }) + select: string[], +): Pet[]; +``` + +#### ✅ Correct + +```tsp +op list( + @query + select: string[], +): Pet[]; +``` + +```tsp +op list( + @query(#{ explode: false }) + select: string[], +): Pet[]; +``` diff --git a/docs/release-notes/release-2023-04-11.md b/docs/release-notes/release-2023-04-11.md index 3d89633778..c99495549f 100644 --- a/docs/release-notes/release-2023-04-11.md +++ b/docs/release-notes/release-2023-04-11.md @@ -59,9 +59,7 @@ op list(@collectionFormat("multi") @query select: string[]): Pet[]; // Now op list( - @query({ - format: "multi", - }) + @query(#{ explode: true }) select: string[], ): Pet[]; ``` diff --git a/packages/e2e-tests/cadl-ranch-specs/package.json b/packages/e2e-tests/cadl-ranch-specs/package.json index 06e31b8344..d1f6fa6db3 100644 --- a/packages/e2e-tests/cadl-ranch-specs/package.json +++ b/packages/e2e-tests/cadl-ranch-specs/package.json @@ -1,7 +1,7 @@ { "name": "@azure-tools/typespec-e2e-cadl-ranch-specs", "dependencies": { - "@azure-tools/cadl-ranch-specs": "0.34.10" + "@azure-tools/cadl-ranch-specs": "0.35.0" }, "type": "module", "private": true diff --git a/packages/samples/specs/misc/appconfig/keyvalues.tsp b/packages/samples/specs/misc/appconfig/keyvalues.tsp index a6d4595bae..d03c04651c 100644 --- a/packages/samples/specs/misc/appconfig/keyvalues.tsp +++ b/packages/samples/specs/misc/appconfig/keyvalues.tsp @@ -44,9 +44,7 @@ namespace KeyValuesResource { After: plainDate, @doc("Used to select what fields are present in the returned resource(s).") - @query({ - format: "multi", - }) + @query(#{ explode: true }) $Select?: KeyField[], ): { ...Response<200>; @@ -70,9 +68,7 @@ namespace KeyValuesResource { After: plainDate, @doc("Used to select what fields are present in the returned resource(s).") - @query({ - format: "multi", - }) + @query(#{ explode: true }) $Select?: KeyField[], ): { ...Response<200>; @@ -88,9 +84,7 @@ namespace KeyValuesResource { ...KeyWithFilters, @doc("Used to select what fields are present in the returned resource(s).") - @query({ - format: "multi", - }) + @query(#{ explode: true }) $Select?: KeyField[], ): { ...Response<200>; diff --git a/packages/samples/specs/misc/appconfig/labels.tsp b/packages/samples/specs/misc/appconfig/labels.tsp index 60f934c00f..a8546d1386 100644 --- a/packages/samples/specs/misc/appconfig/labels.tsp +++ b/packages/samples/specs/misc/appconfig/labels.tsp @@ -34,9 +34,7 @@ namespace LabelsResource { @query name?: string, @query after?: string, - @query({ - format: "multi", - }) + @query(#{ explode: true }) $Select?: LabelField[], ): { ...Response<200>; diff --git a/packages/samples/specs/misc/appconfig/revisions.tsp b/packages/samples/specs/misc/appconfig/revisions.tsp index 75a2696d29..bfcff59472 100644 --- a/packages/samples/specs/misc/appconfig/revisions.tsp +++ b/packages/samples/specs/misc/appconfig/revisions.tsp @@ -14,9 +14,7 @@ namespace RevisionsResource { ...AcceptDatetimeHeader, @doc("Used to select what fields are present in the returned resource(s).") - @query({ - format: "multi", - }) + @query(#{ explode: true }) $Select?: KeyField[], @doc("A filter used to match labels") diff --git a/packages/samples/test/output/azure/core/data-plane/api-path-parameter/@typespec/openapi3/openapi.2022-08-31.yaml b/packages/samples/test/output/azure/core/data-plane/api-path-parameter/@typespec/openapi3/openapi.2022-08-31.yaml index 2d260b3707..7b9d53b04d 100644 --- a/packages/samples/test/output/azure/core/data-plane/api-path-parameter/@typespec/openapi3/openapi.2022-08-31.yaml +++ b/packages/samples/test/output/azure/core/data-plane/api-path-parameter/@typespec/openapi3/openapi.2022-08-31.yaml @@ -725,7 +725,6 @@ components: type: array items: type: string - style: form explode: true Azure.Core.SkipQueryParameter: name: skip diff --git a/packages/samples/test/output/azure/core/data-plane/custom-error-type/@typespec/openapi3/openapi.2022-08-31.yaml b/packages/samples/test/output/azure/core/data-plane/custom-error-type/@typespec/openapi3/openapi.2022-08-31.yaml index 508d595759..5b221b5c78 100644 --- a/packages/samples/test/output/azure/core/data-plane/custom-error-type/@typespec/openapi3/openapi.2022-08-31.yaml +++ b/packages/samples/test/output/azure/core/data-plane/custom-error-type/@typespec/openapi3/openapi.2022-08-31.yaml @@ -375,7 +375,6 @@ components: type: array items: type: string - style: form explode: true Azure.Core.SkipQueryParameter: name: skip diff --git a/packages/samples/test/output/azure/core/data-plane/trait-versioning/@typespec/openapi3/openapi.2022-08-31.yaml b/packages/samples/test/output/azure/core/data-plane/trait-versioning/@typespec/openapi3/openapi.2022-08-31.yaml index cee3ed7f6d..a62eaa5237 100644 --- a/packages/samples/test/output/azure/core/data-plane/trait-versioning/@typespec/openapi3/openapi.2022-08-31.yaml +++ b/packages/samples/test/output/azure/core/data-plane/trait-versioning/@typespec/openapi3/openapi.2022-08-31.yaml @@ -642,7 +642,6 @@ components: type: array items: type: string - style: form explode: true Azure.Core.SkipQueryParameter: name: skip diff --git a/packages/samples/test/output/azure/core/data-plane/trait-versioning/@typespec/openapi3/openapi.2023-02-07.yaml b/packages/samples/test/output/azure/core/data-plane/trait-versioning/@typespec/openapi3/openapi.2023-02-07.yaml index 80daf12544..24fd671ccf 100644 --- a/packages/samples/test/output/azure/core/data-plane/trait-versioning/@typespec/openapi3/openapi.2023-02-07.yaml +++ b/packages/samples/test/output/azure/core/data-plane/trait-versioning/@typespec/openapi3/openapi.2023-02-07.yaml @@ -726,7 +726,6 @@ components: type: array items: type: string - style: form explode: true Azure.Core.SkipQueryParameter: name: skip diff --git a/packages/samples/test/output/azure/core/data-plane/widget-manager/@typespec/openapi3/openapi.2022-08-31.yaml b/packages/samples/test/output/azure/core/data-plane/widget-manager/@typespec/openapi3/openapi.2022-08-31.yaml index dd53aaff77..d7d8262283 100644 --- a/packages/samples/test/output/azure/core/data-plane/widget-manager/@typespec/openapi3/openapi.2022-08-31.yaml +++ b/packages/samples/test/output/azure/core/data-plane/widget-manager/@typespec/openapi3/openapi.2022-08-31.yaml @@ -1349,7 +1349,6 @@ components: type: array items: type: string - style: form explode: true Azure.Core.SkipQueryParameter: name: skip diff --git a/packages/samples/test/output/azure/core/misc/appconfig/@typespec/openapi3/openapi.yaml b/packages/samples/test/output/azure/core/misc/appconfig/@typespec/openapi3/openapi.yaml index 60e41db0dc..b9ba87e303 100644 --- a/packages/samples/test/output/azure/core/misc/appconfig/@typespec/openapi3/openapi.yaml +++ b/packages/samples/test/output/azure/core/misc/appconfig/@typespec/openapi3/openapi.yaml @@ -101,7 +101,6 @@ paths: type: array items: $ref: '#/components/schemas/KeyField' - style: form explode: true responses: '200': @@ -147,7 +146,6 @@ paths: type: array items: $ref: '#/components/schemas/KeyField' - style: form explode: true responses: '200': @@ -182,7 +180,6 @@ paths: type: array items: $ref: '#/components/schemas/KeyField' - style: form explode: true responses: '200': @@ -464,7 +461,6 @@ paths: type: array items: $ref: '#/components/schemas/LabelField' - style: form explode: true responses: '200': @@ -570,7 +566,6 @@ paths: type: array items: $ref: '#/components/schemas/KeyField' - style: form explode: true - name: label in: query diff --git a/packages/typespec-autorest/src/openapi.ts b/packages/typespec-autorest/src/openapi.ts index 12fe3a8dce..470b3da3e7 100644 --- a/packages/typespec-autorest/src/openapi.ts +++ b/packages/typespec-autorest/src/openapi.ts @@ -95,7 +95,10 @@ import { HttpOperation, HttpOperationBody, HttpOperationMultipartBody, + HttpOperationParameter, HttpOperationParameters, + HttpOperationPathParameter, + HttpOperationQueryParameter, HttpOperationResponse, HttpStatusCodeRange, HttpStatusCodesEntry, @@ -106,7 +109,6 @@ import { getAllHttpServices, getAuthentication, getHeaderFieldOptions, - getQueryParamOptions, getServers, getStatusCodeDescription, getVisibilitySuffix, @@ -141,7 +143,6 @@ import { OpenAPI2Operation, OpenAPI2Parameter, OpenAPI2ParameterBase, - OpenAPI2ParameterType, OpenAPI2PathItem, OpenAPI2PathParameter, OpenAPI2QueryParameter, @@ -426,10 +427,20 @@ export async function getOpenAPIForService( } const parameters: OpenAPI2PathParameter[] = []; for (const prop of server.parameters.values()) { - const param = getOpenAPI2Parameter(prop, "path", { - visibility: Visibility.Read, - ignoreMetadataAnnotations: false, - }); + const param = getOpenAPI2Parameter( + { + param: prop, + type: "path", + name: prop.name, + explode: false, + style: "simple", + allowReserved: false, + }, + { + visibility: Visibility.Read, + ignoreMetadataAnnotations: false, + } + ); if ( prop.type.kind === "Scalar" && ignoreDiagnostics( @@ -853,11 +864,17 @@ export async function getOpenAPIForService( } function getResponseHeader(prop: ModelProperty): OpenAPI2HeaderDefinition { - const header: any = {}; - populateParameter(header, prop, "header", { + const header: any = getOpenAPI2HeaderParameter(prop, { visibility: Visibility.Read, ignoreMetadataAnnotations: false, }); + Object.assign( + header, + applyIntrinsicDecorators(prop, { + type: (header as any).type, + format: (header as any).format, + }) + ); delete header.in; delete header.name; delete header.required; @@ -1050,11 +1067,8 @@ export async function getOpenAPIForService( if (httpOpParam.type === "header" && isContentTypeHeader(program, httpOpParam.param)) { continue; } - emitParameter( - httpOpParam.param, - httpOpParam.type, - { visibility, ignoreMetadataAnnotations: false }, - httpOpParam.name + emitParameter(httpOpParam.param, () => + getOpenAPI2Parameter(httpOpParam, { visibility, ignoreMetadataAnnotations: false }) ); } @@ -1104,17 +1118,14 @@ export async function getOpenAPIForService( compilerAssert(bodyModelType.kind === "Model", "Body should always be a Model."); if (bodyModelType) { for (const param of bodyModelType.properties.values()) { - emitParameter(param, "formData", schemaContext, getJsonName(param)); + emitParameter(param, () => + getOpenAPI2FormDataParameter(param, schemaContext, getJsonName(param)) + ); } } } else if (body.property) { - emitParameter( - body.property, - "body", - { visibility, ignoreMetadataAnnotations: false }, - getJsonName(body.property), - schema - ); + const prop = body.property; + emitParameter(prop, () => getOpenAPI2BodyParameter(prop, getJsonName(prop), schema)); } else { currentEndpoint.parameters.push({ name: "body", @@ -1166,23 +1177,17 @@ export async function getOpenAPIForService( return undefined; } - function emitParameter( - param: ModelProperty, - kind: OpenAPI2ParameterType, - schemaContext: SchemaContext, - name?: string, - typeOverride?: any - ) { - if (isNeverType(param.type)) { + function emitParameter(prop: ModelProperty, resolve: () => OpenAPI2Parameter) { + if (isNeverType(prop.type)) { return; } - const ph = getParamPlaceholder(param); + const ph = getParamPlaceholder(prop); currentEndpoint.parameters.push(ph); // If the parameter already has a $ref, don't bother populating it if (!("$ref" in ph)) { - populateParameter(ph, param, kind, schemaContext, name, typeOverride); + Object.assign(ph, resolve()); } } @@ -1244,10 +1249,7 @@ export async function getOpenAPIForService( } } - function getOpenAPI2ParameterBase( - param: ModelProperty, - name: string | undefined - ): OpenAPI2ParameterBase { + function getOpenAPI2ParameterBase(param: ModelProperty, name?: string): OpenAPI2ParameterBase { const base: OpenAPI2ParameterBase = { name: name ?? param.name, required: !param.optional, @@ -1308,41 +1310,57 @@ export async function getOpenAPIForService( } } - function getOpenAPI2QueryParameter( - param: ModelProperty, - schemaContext: SchemaContext, - name?: string - ): OpenAPI2QueryParameter { - const base = getOpenAPI2ParameterBase(param, name); - let collectionFormat = getQueryParamOptions(program, param).format; + function getQueryCollectionFormat(param: HttpOperationQueryParameter): string | undefined { + if (param.explode) { + return "multi"; + } + let collectionFormat = param.format; if (collectionFormat && !["csv", "ssv", "tsv", "pipes", "multi"].includes(collectionFormat)) { collectionFormat = undefined; - reportDiagnostic(program, { code: "invalid-multi-collection-format", target: param }); + reportDiagnostic(program, { code: "invalid-multi-collection-format", target: param.param }); } + return collectionFormat; + } + function getOpenAPI2QueryParameter( + param: HttpOperationQueryParameter, + schemaContext: SchemaContext + ): OpenAPI2QueryParameter { + const base = getOpenAPI2ParameterBase(param.param, param.name); + const collectionFormat = getQueryCollectionFormat(param); + const schema = getSimpleParameterSchema(param.param, schemaContext, base.name); return { in: "query", - collectionFormat: collectionFormat as any, - default: param.defaultValue && getDefaultValue(param.defaultValue), + collectionFormat: + collectionFormat === "csv" && schema.items === undefined // If csv + ? undefined + : (collectionFormat as any), + default: param.param.defaultValue && getDefaultValue(param.param.defaultValue), ...base, - ...getSimpleParameterSchema(param, schemaContext, base.name), + ...schema, }; } function getOpenAPI2PathParameter( - param: ModelProperty, - schemaContext: SchemaContext, - name?: string + param: HttpOperationPathParameter, + schemaContext: SchemaContext ): OpenAPI2PathParameter { - const base = getOpenAPI2ParameterBase(param, name); + const base = getOpenAPI2ParameterBase(param.param, param.name); - return { + const result: OpenAPI2PathParameter = { in: "path", - default: param.defaultValue && getDefaultValue(param.defaultValue), + default: param.param.defaultValue && getDefaultValue(param.param.defaultValue), ...base, - ...getSimpleParameterSchema(param, schemaContext, base.name), + ...getSimpleParameterSchema(param.param, schemaContext, base.name), }; + + if (param.allowReserved) { + result["x-ms-skip-url-encoding"] = true; + } + + return result; } + function getOpenAPI2HeaderParameter( param: ModelProperty, schemaContext: SchemaContext, @@ -1363,58 +1381,40 @@ export async function getOpenAPIForService( }; } - function getOpenAPI2ParameterInternal( - param: ModelProperty, - kind: T, - schemaContext: SchemaContext, - name?: string, - bodySchema?: any - ): OpenAPI2Parameter & { in: T } { - switch (kind) { - case "body": - return getOpenAPI2BodyParameter(param, name, bodySchema) as any; - case "formData": - return getOpenAPI2FormDataParameter(param, schemaContext, name) as any; + function getOpenAPI2ParameterInternal( + param: HttpOperationParameter, + schemaContext: SchemaContext + ): OpenAPI2Parameter & { in: "query" | "path" | "header" } { + switch (param.type) { case "query": - return getOpenAPI2QueryParameter(param, schemaContext, name) as any; + return getOpenAPI2QueryParameter(param, schemaContext); case "path": - return getOpenAPI2PathParameter(param, schemaContext, name) as any; + return getOpenAPI2PathParameter(param, schemaContext); case "header": - return getOpenAPI2HeaderParameter(param, schemaContext, name) as any; + return getOpenAPI2HeaderParameter(param.param, schemaContext, param.name); default: - const _assertNever: never = kind; + const _assertNever: never = param; compilerAssert(false, "Unreachable"); } } function getOpenAPI2Parameter( - param: ModelProperty, - kind: T, - schemaContext: SchemaContext, - name?: string, - bodySchema?: any + param: HttpOperationParameter & { type: T }, + schemaContext: SchemaContext ): OpenAPI2Parameter & { in: T } { - const value = getOpenAPI2ParameterInternal(param, kind, schemaContext, name, bodySchema); + const value = getOpenAPI2ParameterInternal(param, schemaContext); // Apply decorators to a copy of the parameter definition. We use // Object.assign here because applyIntrinsicDecorators returns a new object // based on the target object and we need to apply its changes back to the // original parameter. Object.assign( value, - applyIntrinsicDecorators(param, { type: (value as any).type, format: (value as any).format }) + applyIntrinsicDecorators(param.param, { + type: (value as any).type, + format: (value as any).format, + }) ); - return value; - } - - function populateParameter( - ph: OpenAPI2Parameter, - param: ModelProperty, - kind: OpenAPI2ParameterType, - schemaContext: SchemaContext, - name?: string, - bodySchema?: any - ) { - Object.assign(ph, getOpenAPI2Parameter(param, kind, schemaContext, name, bodySchema)); + return value as any; } function emitParameters() { diff --git a/packages/typespec-autorest/test/parameters.test.ts b/packages/typespec-autorest/test/parameters.test.ts index 6865ed6bea..e140ff7380 100644 --- a/packages/typespec-autorest/test/parameters.test.ts +++ b/packages/typespec-autorest/test/parameters.test.ts @@ -1,17 +1,58 @@ import { expectDiagnostics } from "@typespec/compiler/testing"; import { deepStrictEqual, ok, strictEqual } from "assert"; import { describe, expect, it } from "vitest"; -import { OpenAPI2HeaderParameter, OpenAPI2QueryParameter } from "../src/openapi2-document.js"; +import { + OpenAPI2HeaderParameter, + OpenAPI2PathParameter, + OpenAPI2QueryParameter, +} from "../src/openapi2-document.js"; import { diagnoseOpenApiFor, ignoreUseStandardOps, openApiFor } from "./test-host.js"; -describe("typespec-autorest: parameters", () => { +describe("path parameters", () => { + async function getPathParam(code: string, name = "myParam"): Promise { + const res = await openApiFor(code); + return res.paths[`/{${name}}`].get.parameters[0]; + } + + it("figure out the route parameter from the name of the param", async () => { + const res = await openApiFor(`op test(@path myParam: string): void;`); + expect(res.paths).toHaveProperty("/{myParam}"); + }); + + it("uses explicit name provided from @path", async () => { + const res = await openApiFor(`op test(@path("my-custom-path") myParam: string): void;`); + expect(res.paths).toHaveProperty("/{my-custom-path}"); + }); + + describe("setting reserved expansion attribute applies the x-ms-skip-url-encoding property", () => { + it("with option", async () => { + const param = await getPathParam( + `op test(@path(#{allowReserved: true}) myParam: string[]): void;` + ); + expect(param).toMatchObject({ + "x-ms-skip-url-encoding": true, + }); + }); + it("with uri template", async () => { + const param = await getPathParam(`@route("{+myParam}") op test(myParam: string[]): void;`); + expect(param).toMatchObject({ + "x-ms-skip-url-encoding": true, + }); + }); + }); +}); + +describe("query parameters", () => { + async function getQueryParam(code: string): Promise { + const res = await openApiFor(code); + const param = res.paths[`/`].get.parameters[0]; + strictEqual(param.in, "query"); + return param; + } + it("create a query param", async () => { - const res = await openApiFor( - ` - op test(@query arg1: string): void; - ` - ); - deepStrictEqual(res.paths["/"].get.parameters[0], { + const param = await getQueryParam(`op test(@query arg1: string): void;`); + deepStrictEqual(param, { in: "query", name: "arg1", required: true, @@ -19,9 +60,39 @@ describe("typespec-autorest: parameters", () => { }); }); - it("create a query param of array type", async () => { + it("create a query param with different name", async () => { + const param = await getQueryParam(`op test(@query("$select") select: string): void;`); + deepStrictEqual(param, { + in: "query", + name: "$select", + "x-ms-client-name": "select", + required: true, + type: "string", + }); + }); + + describe("setting parameter explode modifier set collectionFormat to multi", () => { + it("with option", async () => { + const param = await getQueryParam( + `op test(@query(#{explode: true}) myParam: string[]): void;` + ); + expect(param).toMatchObject({ + collectionFormat: "multi", + }); + }); + + it("with uri template", async () => { + const param = await getQueryParam(`@route("{?myParam*}") op test(myParam: string[]): void;`); + expect(param).toMatchObject({ + collectionFormat: "multi", + }); + }); + }); + + it("LEGACY specify the format", async () => { const res = await openApiFor( ` + #suppress "deprecated" "test" op test(@query({format: "multi"}) arg1: string[], @query({format: "csv"}) arg2: string[]): void; ` ); @@ -43,22 +114,6 @@ describe("typespec-autorest: parameters", () => { }); }); - it("create a header param of array type", async () => { - const res = await openApiFor( - ` - op test(@header({format: "csv"}) arg1: string[]): void; - ` - ); - deepStrictEqual(res.paths["/"].get.parameters[0], { - in: "header", - name: "arg1", - required: true, - type: "array", - items: { type: "string" }, - collectionFormat: "csv", - }); - }); - it("create a query param that is a model property", async () => { const res = await openApiFor( ` @@ -96,27 +151,23 @@ describe("typespec-autorest: parameters", () => { ); strictEqual(res.paths["/"].get.parameters[0].description, "my-doc"); }); +}); - it("@query/@header/@path names & @projectedName on body parameter are honored (LEGACY)", async () => { +describe("header parameters", () => { + it("create a header param of array type", async () => { const res = await openApiFor( ` - @route("/{x-ms-arg-3}") - op test( - @query("x-ms-arg-1") @doc("my-doc") arg1: string, - @header("x-ms-arg-2") @doc("my-doc") arg2: string, - @path("x-ms-arg-3") @doc("my-doc") arg3: string): void; - - @put - op test2( - #suppress "deprecated" "for testing" - @projectedName("json", "x-body") @body @doc("my-doc") arg: string): void; - + op test(@header({format: "csv"}) arg1: string[]): void; ` ); - strictEqual(res.paths["/{x-ms-arg-3}"].get.parameters[0].name, "x-ms-arg-1"); - strictEqual(res.paths["/{x-ms-arg-3}"].get.parameters[1].name, "x-ms-arg-2"); - strictEqual(res.paths["/{x-ms-arg-3}"].get.parameters[2].name, "x-ms-arg-3"); - strictEqual(res.paths["/"].put.parameters[0].name, "x-body"); + deepStrictEqual(res.paths["/"].get.parameters[0], { + in: "header", + name: "arg1", + required: true, + type: "array", + items: { type: "string" }, + collectionFormat: "csv", + }); }); it("errors on duplicate parameter keys", async () => { @@ -216,7 +267,9 @@ describe("typespec-autorest: parameters", () => { strictEqual(res.paths["/"].get.parameters[0].in, "query"); strictEqual(res.paths["/"].get.parameters[0].name, "top"); }); +}); +describe("body parameters", () => { it("omit request body if type is void", async () => { const res = await openApiFor(`op test(@body foo: void): void;`); deepStrictEqual(res.paths["/"].post.parameters, []); @@ -288,74 +341,84 @@ describe("typespec-autorest: parameters", () => { required: ["name"], }); }); +}); - describe("content type parameter", () => { - it("header named with 'Content-Type' gets resolved as content type for operation.", async () => { - const res = await openApiFor( - ` - op test( - @header("Content-Type") explicitContentType: "application/octet-stream", - @body foo: string - ): void; - ` - ); - deepStrictEqual(res.paths["/"].post.consumes, ["application/octet-stream"]); - }); +describe("content type parameter", () => { + it("header named with 'Content-Type' gets resolved as content type for operation.", async () => { + const res = await openApiFor( + ` + op test( + @header("Content-Type") explicitContentType: "application/octet-stream", + @body foo: string + ): void; + ` + ); + deepStrictEqual(res.paths["/"].post.consumes, ["application/octet-stream"]); + }); - it("header named contentType gets resolved as content type for operation.", async () => { - const res = await openApiFor( - ` - op test( - @header contentType: "application/octet-stream", - @body foo: string - ): void; - ` - ); - deepStrictEqual(res.paths["/"].post.consumes, ["application/octet-stream"]); - }); + it("header named contentType gets resolved as content type for operation.", async () => { + const res = await openApiFor( + ` + op test( + @header contentType: "application/octet-stream", + @body foo: string + ): void; + ` + ); + deepStrictEqual(res.paths["/"].post.consumes, ["application/octet-stream"]); + }); + + it("query named contentType doesn't get resolved as the content type parmaeter.", async () => { + const res = await openApiFor( + ` + op test( + @query contentType: "application/octet-stream", + @body foo: string + ): void; + ` + ); + strictEqual(res.paths["/"].post.consumes, undefined); + }); +}); - it("query named contentType doesn't get resolved as the content type parmaeter.", async () => { +describe("misc", () => { + describe("type can only be primitive items without $ref", () => { + async function testParameter( + decorator: string, + type: string + ): Promise { const res = await openApiFor( ` - op test( - @query contentType: "application/octet-stream", - @body foo: string - ): void; + union NamedUnion {"one", "two"} + enum Foo {one, two} + op test(${decorator} arg1: ${type}): void; ` ); - strictEqual(res.paths["/"].post.consumes, undefined); - }); - - describe("type can only be primitive items without $ref", () => { - async function testParameter( - decorator: string, - type: string - ): Promise { - const res = await openApiFor( - ` - union NamedUnion {"one", "two"} - enum Foo {one, two} - op test(${decorator} arg1: ${type}): void; - ` - ); - return res.paths["/"].get.parameters[0]; - } - - ["query", "header"].forEach((kind) => { - describe(kind, () => { - it("enum is kept inline", async () => { - deepStrictEqual(await testParameter(`@${kind}`, "Foo"), { - in: kind, - name: "arg1", - required: true, - type: "string", - enum: ["one", "two"], - "x-ms-enum": { modelAsString: false, name: "Foo" }, - }); + return res.paths["/"].get.parameters[0]; + } + + ["query", "header"].forEach((kind) => { + describe(kind, () => { + it("enum is kept inline", async () => { + deepStrictEqual(await testParameter(`@${kind}`, "Foo"), { + in: kind, + name: "arg1", + required: true, + type: "string", + enum: ["one", "two"], + "x-ms-enum": { modelAsString: false, name: "Foo" }, }); + }); - it("array of enum is kept inline", async () => { - deepStrictEqual(await testParameter(`@${kind}({format: "csv"})`, "Foo[]"), { + it("array of enum is kept inline", async () => { + deepStrictEqual( + await testParameter( + ` + #suppress "deprecated" "For tests" + @${kind}({format: "csv"})`, + "Foo[]" + ), + { in: kind, name: "arg1", required: true, @@ -366,21 +429,43 @@ describe("typespec-autorest: parameters", () => { enum: ["one", "two"], "x-ms-enum": { modelAsString: false, name: "Foo" }, }, - }); - }); + } + ); + }); - it("named union is kept inline", async () => { - deepStrictEqual(await testParameter(`@${kind}`, "NamedUnion"), { - in: kind, - name: "arg1", - required: true, - type: "string", - enum: ["one", "two"], - "x-ms-enum": { modelAsString: false, name: "NamedUnion" }, - }); + it("named union is kept inline", async () => { + deepStrictEqual(await testParameter(`@${kind}`, "NamedUnion"), { + in: kind, + name: "arg1", + required: true, + type: "string", + enum: ["one", "two"], + "x-ms-enum": { modelAsString: false, name: "NamedUnion" }, }); }); }); }); + + it("@query/@header/@path names & @projectedName on body parameter are honored (LEGACY)", async () => { + const res = await openApiFor( + ` + @route("/{x-ms-arg-3}") + op test( + @query("x-ms-arg-1") @doc("my-doc") arg1: string, + @header("x-ms-arg-2") @doc("my-doc") arg2: string, + @path("x-ms-arg-3") @doc("my-doc") arg3: string): void; + + @put + op test2( + #suppress "deprecated" "for testing" + @projectedName("json", "x-body") @body @doc("my-doc") arg: string): void; + + ` + ); + strictEqual(res.paths["/{x-ms-arg-3}"].get.parameters[0].name, "x-ms-arg-1"); + strictEqual(res.paths["/{x-ms-arg-3}"].get.parameters[1].name, "x-ms-arg-2"); + strictEqual(res.paths["/{x-ms-arg-3}"].get.parameters[2].name, "x-ms-arg-3"); + strictEqual(res.paths["/"].put.parameters[0].name, "x-body"); + }); }); }); diff --git a/packages/typespec-azure-core/lib/models.tsp b/packages/typespec-azure-core/lib/models.tsp index f34f2adbc5..308bf98d00 100644 --- a/packages/typespec-azure-core/lib/models.tsp +++ b/packages/typespec-azure-core/lib/models.tsp @@ -90,9 +90,7 @@ model FilterParameter { @doc("Provides the standard 'orderby' query parameter for list operations.") model OrderByQueryParameter { #suppress "@azure-tools/typespec-azure-core/prefer-csv-collection-format" "By design" - @query({ - format: "multi", - }) + @query(#{ explode: true }) @doc("Expressions that specify the order of returned results.") orderby?: string[]; } @@ -107,9 +105,7 @@ model FilterQueryParameter { @doc("Provides the standard 'select' query parameter for list operations.") model SelectQueryParameter { #suppress "@azure-tools/typespec-azure-core/prefer-csv-collection-format" "By design" - @query({ - format: "multi", - }) + @query(#{ explode: true }) @doc("Select the specified fields to be included in the response.") select?: string[]; } @@ -117,9 +113,7 @@ model SelectQueryParameter { @doc("Provides the standard 'expand' query parameter for list operations.") model ExpandQueryParameter { #suppress "@azure-tools/typespec-azure-core/prefer-csv-collection-format" "By design" - @query({ - format: "multi", - }) + @query(#{ explode: true }) @doc("Expand the indicated resources into the response.") expand?: string[]; } diff --git a/packages/typespec-azure-core/src/lro-info.ts b/packages/typespec-azure-core/src/lro-info.ts index 1997d23486..ce0868f0b3 100644 --- a/packages/typespec-azure-core/src/lro-info.ts +++ b/packages/typespec-azure-core/src/lro-info.ts @@ -10,13 +10,14 @@ import { compilerAssert, createDiagnosticCollector, getEffectiveModelType, + ignoreDiagnostics, isErrorType, isType, } from "@typespec/compiler"; import { HttpOperationResponse, getHeaderFieldName, - getOperationParameters, + getHttpOperation, getResponsesForOperation, isHeader, isMetadata, @@ -128,7 +129,7 @@ export function getLroOperationInfo( ): [LroOperationInfo | undefined, readonly Diagnostic[]] { const diagnostics = createDiagnosticCollector(); const targetResponses = diagnostics.pipe(getResponsesForOperation(program, targetOperation)); - const targetParameters = diagnostics.pipe(getOperationParameters(program, targetOperation)); + const targetParameters = ignoreDiagnostics(getHttpOperation(program, targetOperation)).parameters; const targetProperties = new Map(); const parameterMap = new Map(); const unmatchedParameters = new Set(targetParameters.parameters.flatMap((p) => p.name)); @@ -146,7 +147,7 @@ export function getLroOperationInfo( } } const sourceResponses = diagnostics.pipe(getResponsesForOperation(program, sourceOperation)); - const sourceParameters = diagnostics.pipe(getOperationParameters(program, sourceOperation)); + const sourceParameters = ignoreDiagnostics(getHttpOperation(program, sourceOperation)).parameters; const sourceBodyProperties = new Map(); if (sourceParameters.body && sourceParameters.body.type.kind === "Model") { for (const [sourceName, sourceProp] of getAllProperties(sourceParameters.body.type)) { diff --git a/packages/typespec-azure-core/src/rules/no-query-explode.ts b/packages/typespec-azure-core/src/rules/no-query-explode.ts new file mode 100644 index 0000000000..b4a6a299bd --- /dev/null +++ b/packages/typespec-azure-core/src/rules/no-query-explode.ts @@ -0,0 +1,26 @@ +import { createRule, ignoreDiagnostics, Operation } from "@typespec/compiler"; +import { getHttpOperation } from "@typespec/http"; + +export const noQueryExplodeRule = createRule({ + name: "no-query-explode", + description: "It is recommended to serialize query parameter without explode: true", + severity: "warning", + url: "https://azure.github.io/typespec-azure/docs/libraries/azure-core/rules/no-query-explode", + messages: { + default: `It is preferred to not use explode: true for query parameters`, + }, + create(context) { + return { + operation: (operation: Operation) => { + const httpOperation = ignoreDiagnostics(getHttpOperation(context.program, operation)); + for (const prop of httpOperation.parameters.properties.filter((x) => x.kind === "query")) { + if (prop.options.explode === true) { + context.reportDiagnostic({ + target: prop.property, + }); + } + } + }, + }; + }, +}); diff --git a/packages/typespec-azure-core/src/rules/prefer-csv-collection-format.ts b/packages/typespec-azure-core/src/rules/prefer-csv-collection-format.ts index 41264730b5..7c5af55c6c 100644 --- a/packages/typespec-azure-core/src/rules/prefer-csv-collection-format.ts +++ b/packages/typespec-azure-core/src/rules/prefer-csv-collection-format.ts @@ -1,5 +1,5 @@ import { ModelProperty, createRule } from "@typespec/compiler"; -import { getHeaderFieldOptions, getQueryParamOptions } from "@typespec/http"; +import { getHeaderFieldOptions } from "@typespec/http"; import { isExcludedCoreType } from "./utils.js"; export const preferCsvCollectionFormatRule = createRule({ @@ -15,11 +15,7 @@ export const preferCsvCollectionFormatRule = createRule({ if (isExcludedCoreType(context.program, property)) return; const headerOptions = getHeaderFieldOptions(context.program, property); - const queryOptions = getQueryParamOptions(context.program, property); - if ( - (headerOptions?.format !== undefined && headerOptions?.format !== "csv") || - (queryOptions?.format !== undefined && queryOptions?.format !== "csv") - ) { + if (headerOptions?.format !== undefined && headerOptions?.format !== "csv") { context.reportDiagnostic({ target: property, }); diff --git a/packages/typespec-azure-core/test/rules/no-query-explode.test.ts b/packages/typespec-azure-core/test/rules/no-query-explode.test.ts new file mode 100644 index 0000000000..8c191ea954 --- /dev/null +++ b/packages/typespec-azure-core/test/rules/no-query-explode.test.ts @@ -0,0 +1,37 @@ +import { + BasicTestRunner, + LinterRuleTester, + createLinterRuleTester, +} from "@typespec/compiler/testing"; +import { beforeEach, it } from "vitest"; +import { noQueryExplodeRule } from "../../src/rules/no-query-explode.js"; +import { createAzureCoreTestRunner } from "../test-host.js"; + +let runner: BasicTestRunner; +let tester: LinterRuleTester; + +beforeEach(async () => { + runner = await createAzureCoreTestRunner(); + tester = createLinterRuleTester(runner, noQueryExplodeRule, "@azure-tools/typespec-azure-core"); +}); + +it("emit warning if using * modifier", async () => { + await tester.expect(`@route("abc{?select*}") op foo(select: string[]): void;`).toEmitDiagnostics({ + code: "@azure-tools/typespec-azure-core/no-query-explode", + }); +}); + +it("emit warning if using explode: true", async () => { + await tester + .expect(`op foo(@query(#{explode: true}) select: string[]): void;`) + .toEmitDiagnostics({ + code: "@azure-tools/typespec-azure-core/no-query-explode", + }); +}); + +it("is ok if query param doesn't specify explode", async () => { + await tester.expect(`op foo(@query select: string[]): void;`).toBeValid(); +}); +it("is ok if query param specify explode: false", async () => { + await tester.expect(`op foo(@query(#{explode: false}) select: string[]): void;`).toBeValid(); +}); diff --git a/packages/typespec-azure-core/test/rules/prefer-csv-collection-format.test.ts b/packages/typespec-azure-core/test/rules/prefer-csv-collection-format.test.ts index 0703beae53..d22ee384ef 100644 --- a/packages/typespec-azure-core/test/rules/prefer-csv-collection-format.test.ts +++ b/packages/typespec-azure-core/test/rules/prefer-csv-collection-format.test.ts @@ -20,8 +20,7 @@ describe("typespec-azure-core: prefer-csv-collection-format rule", () => { ); }); - // Header doesn't allow anything but csv for now so can't be testing this - it.skip("emit warning if using another format for a header ", async () => { + it("emit warning if using another format for a header ", async () => { await tester .expect(`op foo(@header({format: "tsv"}) select: string[]): void;`) .toEmitDiagnostics({ @@ -29,19 +28,7 @@ describe("typespec-azure-core: prefer-csv-collection-format rule", () => { }); }); - it("emit warning if using another format for a query parameter", async () => { - await tester - .expect(`op foo(@query({format: "tsv"}) select: string[]): void;`) - .toEmitDiagnostics({ - code: "@azure-tools/typespec-azure-core/prefer-csv-collection-format", - }); - }); - it("is ok if header param doesn't need format", async () => { await tester.expect(`op foo(@header filter: string): void;`).toBeValid(); }); - - it("is ok if query param doesn't need format", async () => { - await tester.expect(`op foo(@query filter: string): void;`).toBeValid(); - }); }); diff --git a/packages/typespec-client-generator-core/src/http.ts b/packages/typespec-client-generator-core/src/http.ts index 81b3cb6d3a..476118455e 100644 --- a/packages/typespec-client-generator-core/src/http.ts +++ b/packages/typespec-client-generator-core/src/http.ts @@ -596,7 +596,11 @@ function getCollectionFormat( ? getHeaderFieldOptions(program, type) : undefined )?.format; - if (tspCollectionFormat === "form" || tspCollectionFormat === "simple") { + if ( + tspCollectionFormat === "form" || + tspCollectionFormat === "simple" || + tspCollectionFormat === "csv" + ) { return undefined; } return tspCollectionFormat; diff --git a/packages/typespec-client-generator-core/test/package.test.ts b/packages/typespec-client-generator-core/test/package.test.ts index 8513eef466..a42f827856 100644 --- a/packages/typespec-client-generator-core/test/package.test.ts +++ b/packages/typespec-client-generator-core/test/package.test.ts @@ -1237,7 +1237,8 @@ describe("typespec-client-generator-core: package", () => { await runner.compile(`@server("http://localhost:3000", "endpoint") @service({}) namespace My.Service; - + + #suppress "deprecated" "Legacy test" op myOp(@query({format: "multi"}) query: string): void; `); const sdkPackage = runner.context.sdkPackage;