[tcgc] new spread behavior #886

tadelesh · 2024-05-21T10:23:23Z

resolve: #772

several cases related with spread:

op test(a: string, b: string, c: string): void;

model Test {
  prop: string
}
op test(...Test): void;

op test(@body body: {prop: string}): void;

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:

The parameters of SdkServiceMethod are spread.
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.
Emitters could rely on the above info to generate clients' code.

For emitters that have not adopted getAllOperations, emitters need to do:

Use getHttpOperationWithCache to prevent different body type resolving for spread cases.

Refer this code:

typespec-azure/packages/typespec-client-generator-core/src/types.ts

Lines 1311 to 1325 in 5d77ba3

    
           // 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)

to change emitter's logic of handling operations with spread parameters.

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.

azure-sdk · 2024-05-21T10:25:55Z

All changed packages have been documented.

✅ @azure-tools/typespec-client-generator-core

Show changes

`@azure-tools/typespec-client-generator-core` - breaking ✏️

always spread models and aliases with ...

azure-sdk · 2024-05-21T10:42:24Z

You can try these changes at https://cadlplayground.z22.web.core.windows.net/typespec-azure/prs/886/

Check the website changes at https://tspwebsitepr.z22.web.core.windows.net/typespec-azure/prs/886/

…ew_spread

…into new_spread

packages/typespec-client-generator-core/src/types.ts

packages/typespec-client-generator-core/src/http.ts

iscai-msft

thanks @tadelesh

packages/typespec-client-generator-core/test/package.test.ts

haolingdong-msft · 2024-07-01T08:24:50Z

Could we add some guidance on how should emitter adopt this?

tadelesh · 2024-07-01T09:23:27Z

Could we add some guidance on how should emitter adopt this?

added in description.

haolingdong-msft · 2024-07-02T02:25:02Z

Overall looks good to me. Just to confirm: so emitter will use body param's access and usage of SdkServiceOperation
to determine whether it's spread body or not, if it is spread body, then read from SdkServiceOperation.body.correspondingMethodParams to get the spread parameter list? So seems emitters don't need to interact with SdkServiceMethod?

weidongxu-microsoft · 2024-07-02T03:17:54Z

Tested on Java emitter, should be good now.

MaryGao · 2024-07-03T03:06:42Z

packages/typespec-client-generator-core/src/package.ts

@@ -64,7 +64,6 @@ import {
  getCrossLanguageDefinitionId,
  getCrossLanguagePackageId,
  getDefaultApiVersion,
-  getEffectivePayloadType,


Deco getEffectivePayloadType?

I think java won't use this. So, agree on deprecation.

it is still used in response.

MaryGao · 2024-07-03T03:16:37Z

packages/typespec-client-generator-core/test/package.test.ts

      deepStrictEqual(op.bodyParam.correspondingMethodParams, [documentMethodParam]);
    });
+
+    it("anonymous model with @body should not be spread", async () => {


Do we have a case for alias? I believe it is the same as spread model?

alias Test = { prop: string }; op test(...Test): void;

How about this one? Also spread case, right?

model Test { @header foo: string; prop: string }; op test(...Test): void;

same as spread model, i think it has been covered by another case.

What I take is

if there is @body or @bodyRoot anywhere, it is explicit body, and it is not spread

otherwise, it is implicit body, and it is spread

model or alias no longer matter.

MaryGao · 2024-07-03T03:24:21Z

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,


Maybe not relevant to this pr, just want to clarify UsageFlags, if a model is used in

patch operation for input

another operation for spread input

another operation for output

How many medels we will get from TCGC?

two. one original model with path|output, one anonymous model with spread.

MaryGao · 2024-07-03T03:26:04Z

packages/typespec-client-generator-core/src/types.ts

+  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";


If a model is only used for spread, does that mean the TCGC would not return that model in getAllModels by default?

it will return it but with access internal

remove get effective model part

6e428a4

iscai-msft and others added 8 commits May 21, 2024 17:53

Merge branch 'main' of https://github.com/Azure/typespec-azure into n…

bce5411

…ew_spread

change some of the tests

895f5fd

change logic for spread

b560333

Merge branch 'main' of https://github.com/Azure/typespec-azure into n…

d24a5ab

…ew_spread

Merge branch 'new_spread' of https://github.com/Azure/typespec-azure …

5d62ace

…into new_spread

format

3e924a3

add changeset

098457f

Merge branch 'main' into new_spread

2cd77d2

iscai-msft mentioned this pull request May 29, 2024

[tcgc] default .access to "public" #925

Merged

tadelesh added 8 commits May 29, 2024 22:33

Merge remote-tracking branch 'origin/main' into new_spread

4d4333a

Merge remote-tracking branch 'origin/main' into new_spread

8d0f327

change spread logic and fix tests

1d1b4d6

update test

af7a50e

Merge remote-tracking branch 'origin/main' into new_spread

585d5a4

Merge remote-tracking branch 'origin/main' into new_spread

e0f32bf

Merge remote-tracking branch 'origin/main' into new_spread

ce45bac

refine logic and fix test

967f134

tadelesh marked this pull request as ready for review June 13, 2024 10:01

tadelesh requested review from lmazuel, m-nash, iscai-msft, srnagar, joheredi, bterlson, markcowl, allenjzhang and timotheeguerin as code owners June 13, 2024 10:01

refine logic

139e3b9

tadelesh commented Jun 25, 2024

View reviewed changes

packages/typespec-client-generator-core/src/types.ts Outdated Show resolved Hide resolved

weidongxu-microsoft reviewed Jun 25, 2024

View reviewed changes

packages/typespec-client-generator-core/src/http.ts Show resolved Hide resolved

packages/typespec-client-generator-core/src/http.ts Show resolved Hide resolved

tadelesh added 2 commits June 25, 2024 17:30

add comment

32a9449

refine logic

de92375

iscai-msft approved these changes Jun 25, 2024

View reviewed changes

tadelesh added 4 commits June 26, 2024 16:36

Merge branch 'main' into new_spread

d695968

Merge branch 'main' into new_spread

fcd036f

refine test

0a2e7a6

Merge branch 'main' into new_spread

5d77ba3

qiaozha reviewed Jul 1, 2024

View reviewed changes

packages/typespec-client-generator-core/test/package.test.ts Show resolved Hide resolved

packages/typespec-client-generator-core/test/package.test.ts Show resolved Hide resolved

Merge branch 'main' into new_spread

c69bb3c

haolingdong-msft approved these changes Jul 2, 2024

View reviewed changes

weidongxu-microsoft approved these changes Jul 2, 2024

View reviewed changes

tadelesh added this pull request to the merge queue Jul 2, 2024

Merged via the queue into main with commit 56a4f7a Jul 2, 2024
21 checks passed

tadelesh deleted the new_spread branch July 2, 2024 07:47

MaryGao reviewed Jul 3, 2024

View reviewed changes

qiaozha mentioned this pull request Jul 11, 2024

New spread support Azure/autorest.typescript#2653

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[tcgc] new spread behavior #886

[tcgc] new spread behavior #886

tadelesh commented May 21, 2024 •

edited

Loading

azure-sdk commented May 21, 2024 •

edited

Loading

`@azure-tools/typespec-client-generator-core` - breaking ✏️

azure-sdk commented May 21, 2024

iscai-msft left a comment

haolingdong-msft commented Jul 1, 2024

tadelesh commented Jul 1, 2024

haolingdong-msft commented Jul 2, 2024

weidongxu-microsoft commented Jul 2, 2024

MaryGao Jul 3, 2024

weidongxu-microsoft Jul 3, 2024

tadelesh Jul 3, 2024

MaryGao Jul 3, 2024

MaryGao Jul 3, 2024

tadelesh Jul 3, 2024

weidongxu-microsoft Jul 3, 2024 •

edited

Loading

MaryGao Jul 3, 2024

tadelesh Jul 3, 2024 •

edited

Loading

MaryGao Jul 3, 2024

tadelesh Jul 3, 2024

	// 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)

[tcgc] new spread behavior #886

[tcgc] new spread behavior #886

Conversation

tadelesh commented May 21, 2024 • edited Loading

azure-sdk commented May 21, 2024 • edited Loading

@azure-tools/typespec-client-generator-core - breaking ✏️

azure-sdk commented May 21, 2024

iscai-msft left a comment

Choose a reason for hiding this comment

haolingdong-msft commented Jul 1, 2024

tadelesh commented Jul 1, 2024

haolingdong-msft commented Jul 2, 2024

weidongxu-microsoft commented Jul 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weidongxu-microsoft Jul 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tadelesh Jul 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tadelesh commented May 21, 2024 •

edited

Loading

azure-sdk commented May 21, 2024 •

edited

Loading

`@azure-tools/typespec-client-generator-core` - breaking ✏️

weidongxu-microsoft Jul 3, 2024 •

edited

Loading

tadelesh Jul 3, 2024 •

edited

Loading