Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[tcgc] revisit tcgc method and operation design #2295

Open
tadelesh opened this issue Mar 4, 2025 · 4 comments
Open

[tcgc] revisit tcgc method and operation design #2295

tadelesh opened this issue Mar 4, 2025 · 4 comments
Assignees
Labels
design:needed A design request has been raised that needs a proposal lib:tcgc Issues for @azure-tools/typespec-client-generator-core library

Comments

@tadelesh
Copy link
Member

tadelesh commented Mar 4, 2025

many languages have concern about the relationship of method and operation in tcgc. also, we have concern about how we should honor (only see raw typespec or include some of the protocol info) for tcgc method level signature. and, current tcgc's method has inconsistency of parameters and responses. parameters are convenience level, but responses are protocol level. we should at lease either consider to make it consistent or switch to a middle line that emitter is easy to use. let's use this issue to start discussion.

@tadelesh tadelesh self-assigned this Mar 4, 2025
@tadelesh tadelesh added lib:tcgc Issues for @azure-tools/typespec-client-generator-core library design:needed A design request has been raised that needs a proposal labels Mar 4, 2025
@tadelesh
Copy link
Member Author

First step let's see how we should deal with the responses.

For current TCGC implementation, the type property of both method level SdkMethodResponse and operation level SdkServiceResponse are the same and try to use user defined type as much as possible. SdkServiceResponse also contains the response headers property.

Then for situation that user does not specify the response wrapper, current implementation follows the rule of "generate method as TypeSpec method". Only for some cases, the HTTP payload type should be different from user defined type, current implementation seems not so correct. See cases.

Case1: user uses implicit body, but body model has no metadata property, and all properties has no non-read visibility (most common case), TCGC response type property is model Test, while TypeSpec HTTP response body type is model Test. For this case, everything is correct.

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

Case2: user uses explicit @body or @bodyRoot, whatever body model is defined, TCGC response type property is model Test, while TypeSpec HTTP response body type is model Test. For this case, method level type is correct. But for service level, the body payload should not contain the headerProp and password. TypeSpec HTTP lib and TCGC does not do the filtering. Shall we do the filtering? If so, the payload type will be a new anonymous model, which will make emitter to deal with the mapping, as well as generate the new model.

model Test {
  @header headerProp: string;
  @@visibility(Lifecycle.Create) password: string;
  prop: string;
}
op test(): {@body body: Test};

Case 3: user uses implicit body, and the body model has either @header or @visibility(Lifecycle.Create) on any of the properties, TCGC still tries to get back the original type, so TCGC response type property is model Test, while TypeSpec HTTP response body type is an anonymous model without such properties {prop: string}. This case is similar with case 2. The only difference is TypeSpec Http lib has done the filtering, and TCGC could use it directly. So, the question is same with case 2.

model Test {
  @header headerProp: string;
  @@visibility(Lifecycle.Create) password: string;
  prop: string;
}
op test(): Test;

For situation that user does specify the response wrapper with HTTP header or HTTP response status code, current implementation does not follow the rule of "generate method as TypeSpec method". The method level response type is always the HTTP payload body type. The original thinking is most languages does not export response envelop for customer. See cases.

Case 1: user uses all explicit definition for response. This always happen for template cases. TCGC response type property is model Test, while TypeSpec HTTP response body type is model Test, and TypeSpec raw operation return type is the original anonymous model {@statusCode: 200, @header headerProp: string, @body: Test}. For this case, shall we generate the response wrapper in TCGC as method response?

model Test {
  prop: string;
}
op test(): {@statusCode: 200, @header headerProp: string, @body: Test};

Case 2: user define the response with intersection or spread. This also always happen for template cases. For current TCGC implementation, it tries to get back the original user defined type. So, TCGC response type property is model Test, while TypeSpec HTTP response body type is an anonymous model {prop: string}, and TypeSpec raw operation return type is another anonymous model {@header headerProp: string, prop: string}. If we want to follow the TypeSpec type graph, then all the response type will be anonymous and it will break lots of current SDKs.

model Test {
  prop: string;
}
model ResponseHeader {
  @header headerProp: string;
}
op test(): {...ResponseHeader & Test}

@tadelesh
Copy link
Member Author

@Azure/dpg-devs @Azure/dpg-owners please share your thoughts.

@weidongxu-microsoft
Copy link
Member

weidongxu-microsoft commented Mar 26, 2025

Yeah, at current design, Java does not use the response envelop (combine body and header) as response model in API (the only exception could be HEAD http method, where http headers is the only data).

For case 2 of @visibility, I think Java would want a same round-trip model (similar to @visibility(read) on property).

For case 3, we shall see how language handles the response envelop (class, or not). I think TCGC can return different model in SDK method response and service method response, but the difficulty is the naming (we probably don't want both model takes same name, but we also want emitter able to get a proper name if they only choose one model).

@ArcturusZhang
Copy link
Member

ArcturusZhang commented Mar 27, 2025

Since Timothee has clarified, @query and @path does not work at all in response models, those properties would be exactly the same as a payload property, as if the decorator does not exist.
From .Net's perspective, the only issue I am seeing is when the response contains @header, and @visibility.
.Net is working on a "method is method" effort - which means we generate a method whose signature matches its op definition in typespec. Please note this is only working in progress and not finalized at all - the current generated code would not fully following this principle

Below are my own understandings, we do not have solid designs for these yet. But these are the results that I think reasonable following the principle above.

// case 1
public async Task<ClientResult<Test>> TestAsync();

// case 2
public partial class Test
{
	public string HeaderProp {get;} // this model is purely output therefore no setter will be generated
	public string Password {get;} // .Net did not discuss about visibility yet - I assume we just have it here.
	public string Prop {get;}
}
public partial class TestResponse
{
	public Test Body {get;}
}
public async Task<ClientResult<TestResponse>> TestAsync();

// case 3
// class Test is the same as case 2
public async Task<ClientResult<Test>> TestAsync();

The second part cases:

// case 1
public partial class Test
{
  public string Prop {get;}
}
public partial class TestResponse
{
	public string HeaderProp {get;}
	public Test Body {get;}
}
public async Task<ClientResult<TestResponse>> TestAsync();

// case 2
public partial class TestResponse
{
	public string Prop {get;}
	public string HeaderProp {get;}
}
public async Task<ClientResult<TestResponse>> TestAsync();

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
design:needed A design request has been raised that needs a proposal lib:tcgc Issues for @azure-tools/typespec-client-generator-core library
Projects
None yet
Development

No branches or pull requests

3 participants