From 03526383dad70228c54a506e3392d096d6e5bce6 Mon Sep 17 00:00:00 2001 From: iscai-msft <43154838+iscai-msft@users.noreply.github.com> Date: Fri, 30 Aug 2024 14:02:54 -0400 Subject: [PATCH] [tcgc] unify behavior of client params not on method signature (#1451) Co-authored-by: iscai-msft Co-authored-by: Chenjie Shi --- ...hod_params_no_client-2024-7-29-16-14-12.md | 7 + .../src/http.ts | 60 +++---- .../src/package.ts | 15 +- .../test/package.test.ts | 148 +++++++++++------- .../test/packages/parameters.test.ts | 20 ++- .../test/packages/spread.test.ts | 11 +- 6 files changed, 155 insertions(+), 106 deletions(-) create mode 100644 .chronus/changes/method_params_no_client-2024-7-29-16-14-12.md diff --git a/.chronus/changes/method_params_no_client-2024-7-29-16-14-12.md b/.chronus/changes/method_params_no_client-2024-7-29-16-14-12.md new file mode 100644 index 0000000000..4a595fe10e --- /dev/null +++ b/.chronus/changes/method_params_no_client-2024-7-29-16-14-12.md @@ -0,0 +1,7 @@ +--- +changeKind: breaking +packages: + - "@azure-tools/typespec-client-generator-core" +--- + +Have no client parameters appear on method signatures diff --git a/packages/typespec-client-generator-core/src/http.ts b/packages/typespec-client-generator-core/src/http.ts index 5f68e69efc..2f7a149ca5 100644 --- a/packages/typespec-client-generator-core/src/http.ts +++ b/packages/typespec-client-generator-core/src/http.ts @@ -498,49 +498,33 @@ export function getCorrespondingMethodParams( if (serviceParam.isApiVersionParam) { const existingApiVersion = context.__namespaceToApiVersionParameter.get(operationLocation); if (!existingApiVersion) { - const apiVersionParam = methodParameters.find((x) => x.name.includes("apiVersion")); - if (!apiVersionParam) { - diagnostics.add( - createDiagnostic({ - code: "no-corresponding-method-param", - target: serviceParam.__raw!, - format: { - paramName: "apiVersion", - methodName: operation.name, - }, - }) - ); - return diagnostics.wrap([]); - } - const apiVersionParamUpdated: SdkParameter = { - ...apiVersionParam, - name: "apiVersion", - isGeneratedName: apiVersionParam.name !== "apiVersion", - optional: false, - clientDefaultValue: - context.__namespaceToApiVersionClientDefaultValue.get(operationLocation), - }; - context.__namespaceToApiVersionParameter.set(operationLocation, apiVersionParamUpdated); + diagnostics.add( + createDiagnostic({ + code: "no-corresponding-method-param", + target: serviceParam.__raw!, + format: { + paramName: "apiVersion", + methodName: operation.name, + }, + }) + ); + return diagnostics.wrap([]); } return diagnostics.wrap([context.__namespaceToApiVersionParameter.get(operationLocation)!]); } if (isSubscriptionId(context, serviceParam)) { if (!context.__subscriptionIdParameter) { - const subscriptionIdParam = methodParameters.find((x) => isSubscriptionId(context, x)); - if (!subscriptionIdParam) { - diagnostics.add( - createDiagnostic({ - code: "no-corresponding-method-param", - target: serviceParam.__raw!, - format: { - paramName: "subscriptionId", - methodName: operation.name, - }, - }) - ); - return diagnostics.wrap([]); - } - context.__subscriptionIdParameter = subscriptionIdParam; + diagnostics.add( + createDiagnostic({ + code: "no-corresponding-method-param", + target: serviceParam.__raw!, + format: { + paramName: "subscriptionId", + methodName: operation.name, + }, + }) + ); + return diagnostics.wrap([]); } return diagnostics.wrap([context.__subscriptionIdParameter]); } diff --git a/packages/typespec-client-generator-core/src/package.ts b/packages/typespec-client-generator-core/src/package.ts index b81be72999..1e19bbe886 100644 --- a/packages/typespec-client-generator-core/src/package.ts +++ b/packages/typespec-client-generator-core/src/package.ts @@ -62,6 +62,7 @@ import { getLocationOfOperation, getTypeDecorators, isNeverOrVoidType, + isSubscriptionId, updateWithApiVersionInformation, } from "./internal-utils.js"; import { createDiagnostic } from "./lib.js"; @@ -236,7 +237,19 @@ function getSdkBasicServiceMethod for (const param of params) { if (isNeverOrVoidType(param.type)) continue; - methodParameters.push(diagnostics.pipe(getSdkMethodParameter(context, param, operation))); + const sdkMethodParam = diagnostics.pipe(getSdkMethodParameter(context, param, operation)); + if (sdkMethodParam.onClient) { + const operationLocation = getLocationOfOperation(operation); + if (sdkMethodParam.isApiVersionParam) { + if (!context.__namespaceToApiVersionParameter.has(operationLocation)) { + context.__namespaceToApiVersionParameter.set(operationLocation, sdkMethodParam); + } + } else if (isSubscriptionId(context, param)) { + context.__subscriptionIdParameter = sdkMethodParam; + } + } else { + methodParameters.push(sdkMethodParam); + } } const serviceOperation = diagnostics.pipe( diff --git a/packages/typespec-client-generator-core/test/package.test.ts b/packages/typespec-client-generator-core/test/package.test.ts index 50229ea0d4..34c7ed6609 100644 --- a/packages/typespec-client-generator-core/test/package.test.ts +++ b/packages/typespec-client-generator-core/test/package.test.ts @@ -846,9 +846,7 @@ describe("typespec-client-generator-core: package", () => { strictEqual(sdkPackage.clients[0].methods.length, 1); const withApiVersion = sdkPackage.clients[0].methods[0]; strictEqual(withApiVersion.kind, "basic"); - strictEqual(withApiVersion.parameters.length, 1); - strictEqual(withApiVersion.operation.parameters[0].name, "apiVersion"); - strictEqual(withApiVersion.operation.parameters[0].isApiVersionParam, true); + strictEqual(withApiVersion.parameters.length, 0); strictEqual(withApiVersion.operation.parameters.length, 1); const apiVersionParam = withApiVersion.operation.parameters[0]; @@ -858,6 +856,11 @@ describe("typespec-client-generator-core: package", () => { strictEqual(apiVersionParam.onClient, true); strictEqual(apiVersionParam.type.kind, "string"); strictEqual(apiVersionParam.clientDefaultValue, undefined); + strictEqual(apiVersionParam.correspondingMethodParams.length, 1); + strictEqual( + apiVersionParam.correspondingMethodParams[0], + client.initialization.properties.find((x) => x.isApiVersionParam) + ); }); it("service with default api version, method without api version param", async () => { @@ -927,10 +930,8 @@ describe("typespec-client-generator-core: package", () => { withApiVersion.crossLanguageDefintionId, "Server.Versions.Versioned.withQueryApiVersion" ); - strictEqual(withApiVersion.parameters.length, 1); + strictEqual(withApiVersion.parameters.length, 0); strictEqual(withApiVersion.operation.parameters.length, 1); - strictEqual(withApiVersion.parameters[0].isApiVersionParam, true); - strictEqual(withApiVersion.parameters[0].name, "apiVersion"); const apiVersionParam = withApiVersion.operation.parameters[0]; strictEqual(apiVersionParam.kind, "query"); @@ -939,6 +940,11 @@ describe("typespec-client-generator-core: package", () => { strictEqual(apiVersionParam.onClient, true); strictEqual(apiVersionParam.type.kind, "string"); strictEqual(apiVersionParam.clientDefaultValue, "2022-12-01-preview"); + strictEqual(apiVersionParam.correspondingMethodParams.length, 1); + strictEqual( + apiVersionParam.correspondingMethodParams[0], + client.initialization.properties.find((x) => x.isApiVersionParam) + ); }); it("service with default api version, method with path api version param", async () => { @@ -978,9 +984,7 @@ describe("typespec-client-generator-core: package", () => { withApiVersion.crossLanguageDefintionId, "Server.Versions.Versioned.withPathApiVersion" ); - strictEqual(withApiVersion.parameters.length, 1); - strictEqual(withApiVersion.parameters[0].isApiVersionParam, true); - strictEqual(withApiVersion.parameters[0].name, "apiVersion"); + strictEqual(withApiVersion.parameters.length, 0); strictEqual(withApiVersion.operation.parameters.length, 1); const apiVersionParam = withApiVersion.operation.parameters[0]; @@ -992,6 +996,11 @@ describe("typespec-client-generator-core: package", () => { strictEqual(apiVersionParam.onClient, true); strictEqual(apiVersionParam.type.kind, "string"); strictEqual(apiVersionParam.clientDefaultValue, "2022-12-01-preview"); + strictEqual(apiVersionParam.correspondingMethodParams.length, 1); + strictEqual( + apiVersionParam.correspondingMethodParams[0], + client.initialization.properties.find((x) => x.isApiVersionParam) + ); }); }); @@ -1718,7 +1727,7 @@ describe("typespec-client-generator-core: package", () => { strictEqual(parentClient.name, "WidgetManagerClient"); strictEqual(method.name, "getWidget"); strictEqual(method.kind, "basic"); - strictEqual(method.parameters.length, 8); + strictEqual(method.parameters.length, 7); // TODO: what should we do with eTags and client request id? const methodWidgetName = method.parameters.find((p) => p.name === "widgetName"); @@ -1746,7 +1755,10 @@ describe("typespec-client-generator-core: package", () => { strictEqual(queryParam.name, "apiVersion"); strictEqual(queryParam.serializedName, "api-version"); strictEqual(queryParam.onClient, true); - strictEqual(queryParam.correspondingMethodParams.length, 1); + strictEqual( + queryParam.correspondingMethodParams[0], + parentClient.initialization.properties.find((x) => x.isApiVersionParam) + ); ok(parentClient.initialization); strictEqual( queryParam.correspondingMethodParams[0], @@ -1814,7 +1826,7 @@ describe("typespec-client-generator-core: package", () => { getStatus.crossLanguageDefintionId, "Contoso.WidgetManager.Widgets.getWidgetOperationStatus" ); - strictEqual(getStatus.parameters.length, 4); + strictEqual(getStatus.parameters.length, 3); const methodWidgetName = getStatus.parameters.find((p) => p.name === "widgetName"); ok(methodWidgetName); @@ -1886,48 +1898,41 @@ describe("typespec-client-generator-core: package", () => { const createOrUpdate = client.methods.find((x) => x.name === "createOrUpdateWidget"); ok(createOrUpdate); strictEqual(createOrUpdate.kind, "lro"); - strictEqual(createOrUpdate.parameters.length, 12); + strictEqual(createOrUpdate.parameters.length, 11); strictEqual( createOrUpdate.crossLanguageDefintionId, "Contoso.WidgetManager.Widgets.createOrUpdateWidget" ); - deepStrictEqual( - createOrUpdate.parameters.map((x) => x.name), - [ - "apiVersion", - "widgetName", - "contentType", - "resource", - "repeatabilityRequestId", - "repeatabilityFirstSent", - "ifMatch", - "ifNoneMatch", - "ifUnmodifiedSince", - "ifModifiedSince", - "clientRequestId", - "accept", - ] - ); + deepStrictEqual(createOrUpdate.parameters.map((x) => x.name).sort(), [ + "accept", + "clientRequestId", + "contentType", + "ifMatch", + "ifModifiedSince", + "ifNoneMatch", + "ifUnmodifiedSince", + "repeatabilityFirstSent", + "repeatabilityRequestId", + "resource", + "widgetName", + ]); const serviceOperation = createOrUpdate.operation; strictEqual(serviceOperation.verb, "patch"); const headerParams = serviceOperation.parameters.filter( (x): x is SdkHeaderParameter => x.kind === "header" ); - deepStrictEqual( - headerParams.map((x) => x.name), - [ - "contentType", - "repeatabilityRequestId", - "repeatabilityFirstSent", - "ifMatch", - "ifNoneMatch", - "ifUnmodifiedSince", - "ifModifiedSince", - "clientRequestId", - "accept", - ] - ); + deepStrictEqual(headerParams.map((x) => x.name).sort(), [ + "accept", + "clientRequestId", + "contentType", + "ifMatch", + "ifModifiedSince", + "ifNoneMatch", + "ifUnmodifiedSince", + "repeatabilityFirstSent", + "repeatabilityRequestId", + ]); strictEqual(headerParams.length, 9); const pathParam = serviceOperation.parameters.find((x) => x.kind === "path"); ok(pathParam); @@ -2020,7 +2025,13 @@ describe("typespec-client-generator-core: package", () => { const widgetClient = sdkPackage.clients[0].methods.find((x) => x.kind === "clientaccessor") ?.response as SdkClientType; ok(widgetClient); + strictEqual(widgetClient.initialization.properties.length, 3); + const apiVersionClientParam = widgetClient.initialization.properties.find( + (x) => x.isApiVersionParam + ); + ok(apiVersionClientParam); + strictEqual(widgetClient.initialization.access, "internal"); strictEqual(widgetClient.methods.length, 1); const listManufacturers = widgetClient.methods[0]; @@ -2031,11 +2042,11 @@ describe("typespec-client-generator-core: package", () => { "Contoso.WidgetManager.Widgets.listManufacturers" ); strictEqual(listManufacturers.kind, "paging"); - strictEqual(listManufacturers.parameters.length, 3); - deepStrictEqual( - listManufacturers.parameters.map((x) => x.name), - ["apiVersion", "clientRequestId", "accept"] - ); + strictEqual(listManufacturers.parameters.length, 2); + deepStrictEqual(listManufacturers.parameters.map((x) => x.name).sort(), [ + "accept", + "clientRequestId", + ]); const methodResponse = listManufacturers.response.type; ok(methodResponse); strictEqual(methodResponse.kind, "array"); @@ -2052,19 +2063,17 @@ describe("typespec-client-generator-core: package", () => { strictEqual(apiVersion.name, "apiVersion"); strictEqual(apiVersion.serializedName, "api-version"); strictEqual(apiVersion.onClient, true); + strictEqual(apiVersion.correspondingMethodParams[0], apiVersionClientParam); const clientRequestId = operation.parameters.find((x) => x.name === "clientRequestId"); ok(clientRequestId); strictEqual(clientRequestId.kind, "header"); - deepStrictEqual( - clientRequestId.correspondingMethodParams[0], - listManufacturers.parameters[1] - ); + strictEqual(clientRequestId.correspondingMethodParams[0], listManufacturers.parameters[0]); const accept = operation.parameters.find((x) => x.name === "accept"); ok(accept); strictEqual(accept.kind, "header"); - deepStrictEqual(accept.correspondingMethodParams[0], listManufacturers.parameters[2]); + strictEqual(accept.correspondingMethodParams[0], listManufacturers.parameters[1]); strictEqual(operation.responses.size, 1); const response200 = operation.responses.get(200); @@ -2151,7 +2160,20 @@ describe("typespec-client-generator-core: package", () => { const sdkPackage = runner.context.sdkPackage; const client = sdkPackage.clients[0].methods.find((x) => x.kind === "clientaccessor") ?.response as SdkClientType; - strictEqual(client.methods[0].parameters[0].clientDefaultValue, "v2"); + const apiVersionClientParam = client.initialization.properties.find( + (x) => x.isApiVersionParam + ); + ok(apiVersionClientParam); + strictEqual(apiVersionClientParam.clientDefaultValue, "v2"); + + const method = client.methods[0]; + strictEqual(method.parameters.length, 0); + strictEqual(method.kind, "basic"); + + const apiVersionOpParam = method.operation.parameters.find((x) => x.isApiVersionParam); + ok(apiVersionOpParam); + strictEqual(apiVersionOpParam.clientDefaultValue, "v2"); + strictEqual(apiVersionOpParam.correspondingMethodParams[0], apiVersionClientParam); }); it("default api version for operation is", async () => { @@ -2176,7 +2198,21 @@ describe("typespec-client-generator-core: package", () => { `); const sdkPackage = runner.context.sdkPackage; - strictEqual(sdkPackage.clients[0].methods[0].parameters[0].clientDefaultValue, "v2"); + const client = sdkPackage.clients[0]; + const method = client.methods[0]; + strictEqual(method.kind, "basic"); + strictEqual(method.parameters.length, 0); // api-version will be on the client + strictEqual(method.operation.parameters.length, 1); + const apiVersionParam = method.operation.parameters[0]; + strictEqual(apiVersionParam.isApiVersionParam, true); + strictEqual(apiVersionParam.clientDefaultValue, "v2"); + strictEqual(apiVersionParam.correspondingMethodParams.length, 1); + const clientApiVersionParam = client.initialization.properties.find( + (x) => x.isApiVersionParam + ); + ok(clientApiVersionParam); + strictEqual(apiVersionParam.correspondingMethodParams[0], clientApiVersionParam); + strictEqual(clientApiVersionParam.clientDefaultValue, "v2"); }); it("add method", async () => { await runner.compileWithVersionedService(` diff --git a/packages/typespec-client-generator-core/test/packages/parameters.test.ts b/packages/typespec-client-generator-core/test/packages/parameters.test.ts index 0b05c494d4..f03f465b75 100644 --- a/packages/typespec-client-generator-core/test/packages/parameters.test.ts +++ b/packages/typespec-client-generator-core/test/packages/parameters.test.ts @@ -709,11 +709,21 @@ describe("typespec-client-generator-core: parameters", () => { const sdkPackage = runnerWithCore.context.sdkPackage; const method = getServiceMethodOfClient(sdkPackage); - strictEqual(method.parameters.length, 4); - deepStrictEqual( - method.parameters.map((x) => x.name), - ["apiVersion", "prompt", "contentType", "accept"] - ); + strictEqual(method.parameters.length, 3); + deepStrictEqual(method.parameters.map((x) => x.name).sort(), [ + "accept", + "contentType", + "prompt", + ]); + strictEqual(method.operation.parameters.length, 3); + deepStrictEqual(method.operation.parameters.map((x) => x.name).sort(), [ + "accept", + "apiVersion", + "contentType", + ]); + strictEqual(method.operation.bodyParam?.type.kind, "model"); + strictEqual(method.operation.bodyParam.type.properties.length, 1); + strictEqual(method.operation.bodyParam.type.properties[0].name, "prompt"); }); it("never void parameter or response", async () => { diff --git a/packages/typespec-client-generator-core/test/packages/spread.test.ts b/packages/typespec-client-generator-core/test/packages/spread.test.ts index 48cf36ca08..effb96b32f 100644 --- a/packages/typespec-client-generator-core/test/packages/spread.test.ts +++ b/packages/typespec-client-generator-core/test/packages/spread.test.ts @@ -253,7 +253,7 @@ describe("typespec-client-generator-core: spread", () => { const createOrReplace = client.methods[1]; strictEqual(createOrReplace.kind, "basic"); strictEqual(createOrReplace.name, "createOrReplaceDataConnection"); - strictEqual(createOrReplace.parameters.length, 6); + strictEqual(createOrReplace.parameters.length, 5); ok( createOrReplace.parameters.find( (x) => x.name === "dataConnectionName" && x.type.kind === "string" @@ -267,7 +267,6 @@ describe("typespec-client-generator-core: spread", () => { ); ok(createOrReplace.parameters.find((x) => x.name === "contentType")); ok(createOrReplace.parameters.find((x) => x.name === "accept")); - ok(createOrReplace.parameters.find((x) => x.isApiVersionParam && x.onClient)); const opParams = createOrReplace.operation.parameters; strictEqual(opParams.length, 4); @@ -280,13 +279,13 @@ describe("typespec-client-generator-core: spread", () => { createOrReplace.operation.bodyParam?.type.name, "CreateOrReplaceDataConnectionRequest" ); - deepStrictEqual( + strictEqual( createOrReplace.operation.bodyParam.correspondingMethodParams[0], - createOrReplace.parameters[2] + createOrReplace.parameters[1] ); - deepStrictEqual( + strictEqual( createOrReplace.operation.bodyParam.correspondingMethodParams[1], - createOrReplace.parameters[3] + createOrReplace.parameters[2] ); strictEqual(createOrReplace.operation.responses.size, 1); const response200 = createOrReplace.operation.responses.get(200);