[TCGC] api-version parameter should not be onClient===true if the server is non-versioned

[haolingdong-msft]

Case:
https://github.com/Azure/cadl-ranch/blob/main/packages/cadl-ranch-specs/http/server/versions/not-versioned/main.tsp#L33

This is a non-versioned client, and I think the api-version should not be on client, but TCGC mark it as onClient.
TCGC output:

Seems a bug in TCGC?

[iscai-msft]

Our current behavior is that we will always elevate api-version onto the client. It does make sense that versioning corresponds to setting api versioning on clients, but I do think we should talk about this in scrum on weds. Adding that to the agenda list

[haolingdong-msft]

Sure, let's have discussion how the generated sdk should look like for non-versioned clients, I think we should not add api-version to the client parameter, otnerwise it is not non-versioned client.
For Java, if it is a non-versioned client, we will not add api-version parameter to the client. How about other languages? What is your generated code look like for this cadl-ranch case: https://github.com/Azure/cadl-ranch/blob/main/packages/cadl-ranch-specs/http/server/versions/not-versioned/main.tsp#L33
/cc @iscai-msft @tadelesh @archerzz @chunyu3 @qiaozha @MaryGao

[archerzz]

C# doesn't check onClient. C# emitter will pass the api-version method parameter to the generator. And generator will check if there is already a client parameter api-version. If so, the method parameter will be suppressed: https://github.com/Azure/autorest.csharp/blob/bb1c49bfd05512775ed080679152e5cad1f64211/src/AutoRest.CSharp/Common/Generation/Writers/RequestWriterHelpers.cs#L291-L301

What's onClient for? Does it indicate that the query parameter already exists on client level?

[haolingdong-msft]

Hi @archerzz , from my understanding, onClient is used to decide whether the parameter should be on client or not. And emitter is expected to use this value to decide whether to put the parameter on client. This is also related with Izzy's latest pr as well, which adds client parameters to the client initialization object. Those parameters will also have onClient==true on SdkHttpOperation.

[haolingdong-msft]

Summary on 9/5 meeting:

1. TCGC will mark onClient as false for api-version in non-versioned clients.
2. (TBD) what should we set for isApiVersion flag on the non-versioned client's api-version parameter?

[archerzz]

Another proposal: just delete onClient, because where you get SdkParameter should already tell you.

• check SdkClientType.initialization:

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

  Line 101 in 85d0f98

  initialization: SdkInitializationType;

  which contains an array of SdkParameter:

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

  Lines 381 to 383 in 85d0f98

  	export interface SdkInitializationType extends SdkModelType {
  	properties: SdkParameter[];
  	}

• any SdkParameter from SdkClientType.initialization.properties should be onClient: true
• any SdkParawmeter from SdkServiceMethodBase.parameters should be onClient: false:

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

  Line 601 in 85d0f98

  parameters: SdkMethodParameter[];

• so, we could remove onClient property, because:
  • If you think an SdkParameter should be onClient: true, just put it in SdkClientType.initialization.properties
  • If you think an SdkParameter should be onClient: false, just put it in any place other than SdkClientType.initialization.properties (e.g. SdkServiceMethodBase.parameters)

[haolingdong-msft]

Another proposal: just delete onClient, because where you get SdkParameter should already tell you.

Hi @mingzhe, Personally I think these logics you mentioned on calculating onClient value should be better to put to TCGC, and emitters only need to rely on onClient value. Add @tadelesh and @iscai-msft for more comments as well.

[iscai-msft]

@archerzz that is a good point, where we might no longer need onClient. I'm going to open another issue about this, and see what can be done. Thanks!

[chunyu3]

I think we should keep onClient flag. When we do @clientInitialization to bump a parameter from operation to client. .NET need to know if the client parameter is exactly the client parameter or it is a parameter which is bumped from operation. Current we use onClient flag in the parameter of an operation to identify that this operation parameter is bumped to client.

[haolingdong-msft]

Hi @iscai-msft, I would agree with @chunyu3 , I think checking onClient is the most straightforward and easiest way, though one can also argue that emitter could check whether the parameter is in SdkClientType.initialization.properties or not to determine if it's onClient. Both ways are workable, personally I would prefer the easier way which is to use onClient, and I think most language emitters are doing so already or planning to do so. Removing onClient would cause breaking changes to those emitters. To me, we(TCGC) should avoid breaking changs unless we have sufficient reasons.