[tcgc] new spread behavior (#886)

resolve: #772 several cases related with spread: 1. ``` op test(a: string, b: string, c: string): void; ``` 2. ``` model Test { prop: string } op test(...Test): void; ``` 3. ``` op test(@Body body: {prop: string}): void; ``` 4. ``` model Test { prop: string } op test(@Body body: {...Test}): void; ``` tcgc will do http body spread for 1 and 2, not for 3 and 4. and for http body model, 1, 2 will have a model `TestRequest` with spread usage and internal access. **Adoption guideline for emitters:** For emitters that have already adopted `getAllOperations`, if an operation has spread parameters: 1. The parameters of `SdkServiceMethod` are spread. 2. The body parameter of `SdkServiceOperation` is an anonymous model with `Usage.Spread` usage and `Access.Internal` access. And the `correspondingMethodParams` of the body will be list of the parameters of `SdkServiceMethod`. 3. Emitters could rely on the above info to generate clients' code. For emitters that have not adopted `getAllOperations`, emitters need to do: 1. Use `getHttpOperationWithCache` to prevent different body type resolving for spread cases. 2. Refer this code: https://github.com/Azure/typespec-azure/blob/5d77ba3e93521735a84051b4037c6f55e0a8a00a/packages/typespec-client-generator-core/src/types.ts#L1311-L1325 to change emitter's logic of handling operations with spread parameters. 3. When call `getClientType` from the body type of operation with spread, the return model will be an anonymous model with `Usage.Spread` usage and `Access.Internal` access. DO NOT CALL `getEffectivePayloadType` for body type if it is from spread. --------- Co-authored-by: iscai-msft <[email protected]> Co-authored-by: iscai-msft <[email protected]>
Azure · Jul 2, 2024 · 56a4f7a · 56a4f7a
1 parent 88ba7bf
commit 56a4f7a
Show file tree

Hide file tree

Showing 12 changed files with 675 additions and 259 deletions.
diff --git a/.chronus/changes/new_spread-2024-4-23-14-11-6.md b/.chronus/changes/new_spread-2024-4-23-14-11-6.md
@@ -0,0 +1,7 @@
+---
+changeKind: breaking
+packages:
+  - "@azure-tools/typespec-client-generator-core"
+---
+
+always spread models and aliases with `...`
diff --git a/packages/typespec-client-generator-core/src/http.ts b/packages/typespec-client-generator-core/src/http.ts
@@ -127,17 +127,17 @@ function getSdkHttpParameters(
   const correspondingMethodParams: SdkModelPropertyType[] = [];
   if (tspBody && tspBody?.bodyKind !== "multipart") {
     // if there's a param on the body, we can just rely on getSdkHttpParameter
-    if (tspBody.parameter && !isNeverOrVoidType(tspBody.parameter.type)) {
+    if (tspBody.property && !isNeverOrVoidType(tspBody.property.type)) {
       const getParamResponse = diagnostics.pipe(
-        getSdkHttpParameter(context, tspBody.parameter, httpOperation.operation, "body")
+        getSdkHttpParameter(context, tspBody.property, httpOperation.operation, "body")
       );
       if (getParamResponse.kind !== "body") {
         diagnostics.add(
           createDiagnostic({
             code: "unexpected-http-param-type",
-            target: tspBody.parameter,
+            target: tspBody.property,
             format: {
-              paramName: tspBody.parameter.name,
+              paramName: tspBody.property.name,
               expectedType: "body",
               actualType: getParamResponse.kind,
             },
@@ -505,62 +505,27 @@ export function getCorrespondingMethodParams(
     }
     return diagnostics.wrap([context.__subscriptionIdParameter]);
   }
-  const correspondingMethodParameter = methodParameters.find((x) => x.name === serviceParam.name);
-  if (correspondingMethodParameter) {
-    return diagnostics.wrap([correspondingMethodParameter]);
-  }
-
-  const serviceParamType = serviceParam.type;
-  if (serviceParam.kind === "body" && serviceParamType.kind === "model") {
-    const serviceParamPropertyNames = Array.from(serviceParamType.properties.values())
-      .filter((x) => x.kind === "property")
-      .map((x) => x.name);
-    // Here we have a spread method parameter
 
-    // easy case is if the spread method parameter directly has the entire body as a property
-    const directBodyProperty = methodParameters
-      .map((x) => x.type)
-      .filter((x): x is SdkModelType => x.kind === "model")
-      .flatMap((x) => x.properties)
-      .find((x) => x.type === serviceParamType);
-    if (directBodyProperty) return diagnostics.wrap([directBodyProperty]);
-    let correspondingProperties: SdkModelPropertyType[] = methodParameters.filter((x) =>
-      serviceParamPropertyNames.includes(x.name)
-    );
-    for (const methodParam of methodParameters) {
-      const methodParamIterable =
-        methodParam.type.kind === "model" ? methodParam.type.properties : [methodParam];
-      correspondingProperties = correspondingProperties.concat(
-        methodParamIterable.filter(
-          (x) =>
-            serviceParamPropertyNames.includes(x.name) &&
-            !correspondingProperties.find((e) => e.name === x.name)
-        )
-      );
-    }
-    if (correspondingProperties.length === serviceParamPropertyNames.length)
-      return diagnostics.wrap(correspondingProperties);
-    diagnostics.add(
-      createDiagnostic({
-        code: "no-corresponding-method-param",
-        target: serviceParam.__raw!,
-        format: {
-          paramName: serviceParam.name,
-          methodName: operation.name,
-        },
-      })
-    );
-    return diagnostics.wrap([]);
+  // to see if the service parameter is a method parameter or a property of a method parameter
+  const directMapping = findMapping(methodParameters, serviceParam);
+  if (directMapping) {
+    return diagnostics.wrap([directMapping]);
   }
-  for (const methodParam of methodParameters) {
-    if (methodParam.type.kind === "model") {
-      for (const prop of methodParam.type.properties) {
-        if (prop.name === serviceParam.name) {
-          return diagnostics.wrap([prop]);
-        }
+
+  // to see if all the property of service parameter could be mapped to a method parameter or a property of a method parameter
+  if (serviceParam.kind === "body" && serviceParam.type.kind === "model") {
+    const retVal = [];
+    for (const serviceParamProp of serviceParam.type.properties) {
+      const propertyMapping = findMapping(methodParameters, serviceParamProp);
+      if (propertyMapping) {
+        retVal.push(propertyMapping);
       }
     }
+    if (retVal.length === serviceParam.type.properties.length) {
+      return diagnostics.wrap(retVal);
+    }
   }
+
   diagnostics.add(
     createDiagnostic({
       code: "no-corresponding-method-param",
@@ -574,6 +539,52 @@ export function getCorrespondingMethodParams(
   return diagnostics.wrap([]);
 }
 
+function findMapping(
+  methodParameters: SdkModelPropertyType[],
+  serviceParam: SdkHttpParameter | SdkModelPropertyType
+): SdkModelPropertyType | undefined {
+  const queue: SdkModelPropertyType[] = [...methodParameters];
+  const visited: Set<SdkModelType> = new Set();
+  while (queue.length > 0) {
+    const methodParam = queue.shift()!;
+    // http operation parameter/body parameter/property of body parameter could either from an operation parameter directly or from a property of an operation parameter
+    if (
+      methodParam.__raw &&
+      serviceParam.__raw &&
+      (methodParam.__raw === serviceParam.__raw ||
+        methodParam.__raw === serviceParam.__raw.sourceProperty)
+    ) {
+      return methodParam;
+    }
+    // this following two hard code mapping is for the case that TCGC help to add content type and accept header is not exist
+    if (
+      serviceParam.kind === "header" &&
+      serviceParam.serializedName === "Content-Type" &&
+      methodParam.name === "contentType"
+    ) {
+      return methodParam;
+    }
+    if (
+      serviceParam.kind === "header" &&
+      serviceParam.serializedName === "Accept" &&
+      methodParam.name === "accept"
+    ) {
+      return methodParam;
+    }
+    if (methodParam.type.kind === "model" && !visited.has(methodParam.type)) {
+      visited.add(methodParam.type);
+      let current: SdkModelType | undefined = methodParam.type;
+      while (current) {
+        for (const prop of methodParam.type.properties) {
+          queue.push(prop);
+        }
+        current = current.baseModel;
+      }
+    }
+  }
+  return undefined;
+}
+
 function getCollectionFormat(
   context: TCGCContext,
   type: ModelProperty

diff --git a/packages/typespec-client-generator-core/src/interfaces.ts b/packages/typespec-client-generator-core/src/interfaces.ts
@@ -598,4 +598,6 @@ export enum UsageFlags {
   JsonMergePatch = 1 << 4,
   // Input will also be set when MultipartFormData is set.
   MultipartFormData = 1 << 5,
+  // Used in spread.
+  Spread = 1 << 6,
 }
diff --git a/packages/typespec-client-generator-core/src/internal-utils.ts b/packages/typespec-client-generator-core/src/internal-utils.ts
@@ -392,6 +392,7 @@ export interface TCGCContext {
   modelsMap?: Map<Type, SdkModelType | SdkEnumType>;
   operationModelsMap?: Map<Operation, Map<Type, SdkModelType | SdkEnumType>>;
   generatedNames?: Map<Union | Model | TspLiteralType, string>;
+  spreadModels?: Map<Model, SdkModelType>;
   httpOperationCache?: Map<Operation, HttpOperation>;
   unionsMap?: Map<Union, SdkUnionType>;
   __namespaceToApiVersionParameter: Map<Interface | Namespace, SdkParameter>;

diff --git a/packages/typespec-client-generator-core/src/package.ts b/packages/typespec-client-generator-core/src/package.ts
@@ -64,7 +64,6 @@ import {
   getCrossLanguageDefinitionId,
   getCrossLanguagePackageId,
   getDefaultApiVersion,
-  getEffectivePayloadType,
   getHttpOperationWithCache,
   getLibraryName,
 } from "./public-utils.js";
@@ -217,7 +216,7 @@ function getSdkMethodResponse<
       crossLanguageDefinitionId: getCrossLanguageDefinitionId(context, operation),
       decorators: [],
     };
-  } else if (responseTypes) {
+  } else if (responseTypes.size === 1) {
     type = allResponseBodies[0];
   }
   if (nonBodyExists && type) {
@@ -249,38 +248,10 @@ function getSdkBasicServiceMethod<
     operation,
     getLocationOfOperation(operation)
   );
-  const httpOperation = getHttpOperationWithCache(context, operation);
-  const parameters = httpOperation.parameters;
-  // path/query/header parameters
-  for (const param of parameters.parameters) {
-    if (isNeverOrVoidType(param.param.type)) continue;
-    methodParameters.push(diagnostics.pipe(getSdkMethodParameter(context, param.param, operation)));
-  }
-  // body parameters
-  if (
-    parameters.body?.bodyKind !== "multipart" &&
-    parameters.body?.property &&
-    !isNeverOrVoidType(parameters.body.property.type)
-  ) {
-    methodParameters.push(
-      diagnostics.pipe(getSdkMethodParameter(context, parameters.body?.property, operation))
-    );
-  } else if (parameters.body && !isNeverOrVoidType(parameters.body.type)) {
-    if (parameters.body.type.kind === "Model") {
-      const type = getEffectivePayloadType(context, parameters.body.type);
-      // spread case
-      if (type.name === "") {
-        for (const prop of type.properties.values()) {
-          methodParameters.push(diagnostics.pipe(getSdkMethodParameter(context, prop, operation)));
-        }
-      } else {
-        methodParameters.push(diagnostics.pipe(getSdkMethodParameter(context, type, operation)));
-      }
-    } else {
-      methodParameters.push(
-        diagnostics.pipe(getSdkMethodParameter(context, parameters.body.type, operation))
-      );
-    }
+
+  for (const param of operation.parameters.properties.values()) {
+    if (isNeverOrVoidType(param.type)) continue;
+    methodParameters.push(diagnostics.pipe(getSdkMethodParameter(context, param, operation)));
   }
 
   const serviceOperation = diagnostics.pipe(

diff --git a/packages/typespec-client-generator-core/src/public-utils.ts b/packages/typespec-client-generator-core/src/public-utils.ts
@@ -18,15 +18,7 @@ import {
   listServices,
   resolveEncodedName,
 } from "@typespec/compiler";
-import {
-  HttpOperation,
-  getHeaderFieldName,
-  getHttpOperation,
-  getPathParamName,
-  getQueryParamName,
-  isMetadata,
-  isStatusCode,
-} from "@typespec/http";
+import { HttpOperation, getHttpOperation, isMetadata } from "@typespec/http";
 import { Version, getVersions } from "@typespec/versioning";
 import { pascalCase } from "change-case";
 import pluralize from "pluralize";
@@ -119,16 +111,7 @@ export function getEffectivePayloadType(context: TCGCContext, type: Model): Mode
     return type;
   }
 
-  function isSchemaProperty(property: ModelProperty) {
-    const program = context.program;
-    const headerInfo = getHeaderFieldName(program, property);
-    const queryInfo = getQueryParamName(program, property);
-    const pathInfo = getPathParamName(program, property);
-    const statusCodeinfo = isStatusCode(program, property);
-    return !(headerInfo || queryInfo || pathInfo || statusCodeinfo);
-  }
-
-  const effective = getEffectiveModelType(program, type, isSchemaProperty);
+  const effective = getEffectiveModelType(program, type, (t) => !isMetadata(context.program, t));
   if (effective.name) {
     return effective;
   }
@@ -404,9 +387,13 @@ function getContextPath(
     for (const response of httpOperation.responses) {
       for (const innerResponse of response.responses) {
         if (innerResponse.body?.type) {
+          const body =
+            innerResponse.body.type.kind === "Model"
+              ? getEffectivePayloadType(context, innerResponse.body.type)
+              : innerResponse.body.type;
           visited.clear();
           result = [{ name: root.name }];
-          if (dfsModelProperties(typeToFind, innerResponse.body.type, "Response")) {
+          if (dfsModelProperties(typeToFind, body, "Response")) {
             return result;
           }
         }
@@ -475,7 +462,6 @@ function getContextPath(
       const dictOrArrayItemType: Type = currentType.indexer.value;
       return dfsModelProperties(expectedType, dictOrArrayItemType, pluralize.singular(displayName));
     } else if (currentType.kind === "Model") {
-      currentType = getEffectivePayloadType(context, currentType);
       // handle model
       result.push({ name: displayName, type: currentType });
       for (const property of currentType.properties.values()) {

diff --git a/packages/typespec-client-generator-core/src/types.ts b/packages/typespec-client-generator-core/src/types.ts
@@ -547,7 +547,6 @@ export function getSdkModelWithDiagnostics(
   operation?: Operation
 ): [SdkModelType, readonly Diagnostic[]] {
   const diagnostics = createDiagnosticCollector();
-  type = getEffectivePayloadType(context, type);
   let sdkType = context.modelsMap?.get(type) as SdkModelType | undefined;
 
   if (sdkType) {
@@ -1309,8 +1308,26 @@ function updateTypesFromOperation(
       getClientTypeWithDiagnostics(context, httpBody.type, operation)
     );
     if (generateConvenient) {
-      // spread body model should be none usage
-      if (sdkType.kind !== "model" || !sdkType.isGeneratedName) {
+      // Special logic for spread body model:
+      // If body is from spread, then it should be an anonymous model.
+      // Also all model properties should be
+      // either equal to one of operation parameters (for case spread from model without property with metadata decorator)
+      // or its source property equal to one of operation parameters (for case spread from model with property with metadata decorator)
+      if (
+        httpBody.type.kind === "Model" &&
+        httpBody.type.name === "" &&
+        [...httpBody.type.properties.keys()].every(
+          (k) =>
+            operation.parameters.properties.has(k) &&
+            (operation.parameters.properties.get(k) ===
+              (httpBody.type as Model).properties.get(k) ||
+              operation.parameters.properties.get(k) ===
+                (httpBody.type as Model).properties.get(k)?.sourceProperty)
+        ) &&
+        !context.spreadModels?.has(httpBody.type)
+      ) {
+        context.spreadModels?.set(httpBody.type as Model, sdkType as SdkModelType);
+      } else {
         updateUsageOfModel(context, UsageFlags.Input, sdkType);
       }
       if (httpBody.contentTypes.includes("application/merge-patch+json")) {
@@ -1326,9 +1343,11 @@ function updateTypesFromOperation(
   for (const response of httpOperation.responses) {
     for (const innerResponse of response.responses) {
       if (innerResponse.body?.type && !isNeverOrVoidType(innerResponse.body.type)) {
-        const sdkType = diagnostics.pipe(
-          getClientTypeWithDiagnostics(context, innerResponse.body.type, operation)
-        );
+        const body =
+          innerResponse.body.type.kind === "Model"
+            ? getEffectivePayloadType(context, innerResponse.body.type)
+            : innerResponse.body.type;
+        const sdkType = diagnostics.pipe(getClientTypeWithDiagnostics(context, body, operation));
         if (generateConvenient) {
           updateUsageOfModel(context, UsageFlags.Output, sdkType);
         }
@@ -1403,6 +1422,18 @@ function updateAccessOfModel(context: TCGCContext): void {
   }
 }
 
+function updateSpreadModelUsageAndAccess(context: TCGCContext): void {
+  for (const sdkType of context.spreadModels?.values() ?? []) {
+    updateUsageOfModel(context, UsageFlags.Spread, sdkType);
+  }
+  for (const sdkType of context.modelsMap?.values() ?? []) {
+    // if a type only has spread usage, then it could be internal
+    if (sdkType.usage === UsageFlags.Spread) {
+      sdkType.access = "internal";
+    }
+  }
+}
+
 interface GetAllModelsOptions {
   input?: boolean;
   output?: boolean;
@@ -1484,6 +1515,9 @@ export function getAllModelsWithDiagnostics(
   if (context.operationModelsMap === undefined) {
     context.operationModelsMap = new Map<Operation, Map<Type, SdkModelType | SdkEnumType>>();
   }
+  if (context.spreadModels === undefined) {
+    context.spreadModels = new Map<Model, SdkModelType>();
+  }
   for (const client of listClients(context)) {
     for (const operation of listOperationsInOperationGroup(context, client)) {
       // operations on a client
@@ -1542,6 +1576,8 @@ export function getAllModelsWithDiagnostics(
   }
   // update access
   updateAccessOfModel(context);
+  // update spread model
+  updateSpreadModelUsageAndAccess(context);
   // filter out models
   filterOutModels(context);
   let filter = 0;