tsp, tcgc, adopt sdkpackage

[haolingdong-msft]

Link #2665

WIP, have not cleaned up code in emitter yet, will let you know once it's done. But would like to create PR to track generated code diff. I'm checking the diffs generated impl files. Especially on accept and content-type headers, as well as descriptions.

Latest API view and diff summary: #2665 (comment)

-------- update on 8/7----------------
api view and diff summary: #2665 (comment)

I have also done a round of code clean up. If you have time, please feel free to take a look in the meanwhile I'm fixing the two diffs.
--------update on 8/9--------------------
Note that after adopting new SdkclientType design, those previously constant endpoint would be overridable, so there are diffs generated: 955620e
---------update on 8/12---------------------
new api-view: https://apiview.dev/Assemblies/Review/e0cf256f814e456bac0f0d751bd4aa9f/e0f4e0c3e4704de89332b6229b14ad64?diffRevisionId=fbdae726b3a04e839587766478ca88eb&diffOnly=true

Diff summary in the issue: #2665 (comment)

-----------8/13 update------
latest api view: https://apiview.dev/Assemblies/Review/e0cf256f814e456bac0f0d751bd4aa9f/0e83c8742b5942379d330f0e8f40b667?diffRevisionId=af2d36f4bf4243a39d2cab13ace75d25&diffOnly=true

public API diff:

• allow override endpoint for constant tsp endpoint definition
• parameter order, I will sync to sdk repo to see if there is existing sdk impacted, if not, I will keep the parameter order change.

impl diff:

• allowReserved on path not supoprted by TCGC.

--------------------update on 8/19----------------------
API view:
https://apiview.dev/Assemblies/Review/e0cf256f814e456bac0f0d751bd4aa9f/b6c1bc44f2a64d90bd19587aab003913?diffRevisionId=1eed99f40d2d46f0a66d7184e26c5d86&diffOnly=true

API view diff

1. endpoint always overridable - expected
2. parameter order change for spread parameter case. - will regen on sdk repo to see the impact
   Reason: There is limitation on parameters list of SdkServiceMethod that it cannot map to corresponding SdkHttpOperation's parameters/bodyType which causes we can't use the SdkServiceMethod.parameters. TCGC is considering adding a mapping for us to map SdkServiceMethod's parameters to SdkHttpOperation's parameters and bodyType. But before it is supported, we are now using SdkServiceMethod.SdkHttpOperation.
   For spread parameters, we will process the parameters and bodyType under SdkHttpOperation. So the order is first query/header/path parameters, then body parameter.

Notable changes comparing to last version:

• update spread part code: tsp, tcgc, adopt sdkpackage #2861 (comment)
• add test case for HttpStatusCodeRange: b7ff02c
• support allowReserved in path: a423afa

[weidongxu-microsoft]

Then what's this line for?

    if (opParameter.kind !== "property") {

In TCGC, for both spread and non-spread, SdkHttpOperation.parameters would only contain query/header/path parameters.

[haolingdong-msft]

Then what's this line for?

    if (opParameter.kind !== "property") {

In TCGC, for both spread and non-spread, SdkHttpOperation.parameters would only contain query/header/path parameters.

it is property of the body model.
Because body parameter and query/header/path parameters are processed seperately, so I guess there is no need to check existing parameter for body parameter. Please correct me if I understand wrongly.

autorest.java/typespec-extension/src/code-model-builder.ts

Lines 1360 to 1373 in 0026417

	// header/query/path params
	for (const opParameter of parameters) {
	this.addParameterOrBodyPropertyToCodeModelRequest(opParameter, op, request, schema, parameter);
	}
	// body param
	if (bodyParameter) {
	if (bodyParameter.type.kind === "model") {
	for (const bodyProperty of bodyParameter.type.properties) {
	if (bodyProperty.kind === "property") {
	this.addParameterOrBodyPropertyToCodeModelRequest(bodyProperty, op, request, schema, parameter);
	}
	}
	}
	}

[weidongxu-microsoft]

In TCGC, for both spread and non-spread, SdkHttpOperation.parameters would only contain query/header/path parameters.

it is property of the body model. Because body parameter and query/header/path parameters are processed seperately, so I guess there is no need to check existing parameter for body parameter. Please correct me if I understand wrongly.

autorest.java/typespec-extension/src/code-model-builder.ts

Lines 1360 to 1373 in 0026417

	// header/query/path params
	for (const opParameter of parameters) {
	this.addParameterOrBodyPropertyToCodeModelRequest(opParameter, op, request, schema, parameter);
	}
	// body param
	if (bodyParameter) {
	if (bodyParameter.type.kind === "model") {
	for (const bodyProperty of bodyParameter.type.properties) {
	if (bodyProperty.kind === "property") {
	this.addParameterOrBodyPropertyToCodeModelRequest(bodyProperty, op, request, schema, parameter);
	}
	}
	}
	}

Yes, I think it is not needed in your code. And that is the reason I am verifying whether TCGC gives you the "property" type parameter.

Also, from this code, the order of parameters of "spread" op is not what TCGC gives you. It is by your code, that non-property parameter is processed, then the property of the bodyParam is processed.

The order in TCGC on this would be in its SdkServiceMethod, and I assume you didn't use that type.

I likely going to refactor and use SdkServiceMethod later (probably during option bag impl). But please check that your order here is same as order in SdkServiceMethod (just take a look on param in it, for the cadl-ranch case that we see the change in apivew -- spreadParameterWithInnerAlias I think). I would want to avoid the case that we change it today, then had to change it again after migrating to SdkServiceMethod.

[haolingdong-msft]

In TCGC, for both spread and non-spread, SdkHttpOperation.parameters would only contain query/header/path parameters.

it is property of the body model. Because body parameter and query/header/path parameters are processed seperately, so I guess there is no need to check existing parameter for body parameter. Please correct me if I understand wrongly.

autorest.java/typespec-extension/src/code-model-builder.ts

Lines 1360 to 1373 in 0026417

	// header/query/path params
	for (const opParameter of parameters) {
	this.addParameterOrBodyPropertyToCodeModelRequest(opParameter, op, request, schema, parameter);
	}
	// body param
	if (bodyParameter) {
	if (bodyParameter.type.kind === "model") {
	for (const bodyProperty of bodyParameter.type.properties) {
	if (bodyProperty.kind === "property") {
	this.addParameterOrBodyPropertyToCodeModelRequest(bodyProperty, op, request, schema, parameter);
	}
	}
	}
	}

Yes, I think it is not needed in your code. And that is the reason I am verifying whether TCGC gives you the "property" type parameter

Also, from this code, the order of parameters of "spread" op is not what TCGC gives you. It is by your code, that non-property parameter is processed, then the property of the bodyParam is processed.

The order in TCGC on this would be in its SdkServiceMethod, and I assume you didn't use that type.

I likely going to refactor and use SdkServiceMethod later (probably during option bag impl). But please check that your order here is same as order in SdkServiceMethod (just take a look on param in it, for the cadl-ranch case that we see the change in apivew -- spreadParameterWithInnerAlias I think). I would want to avoid the case that we change it today, then had to change it again after migrating to SdkServiceMethod.

No, it is not same as SdkServiceMethod's parameters, as SdkServiceMethod returns body and non-body parameters together in TSP compiler's order.
But there is limitation on parameters list of SdkServiceMethod that it cannot map to corresponding SdkHttpOperation's parameters/bodyType which causes we can't use the SdkServiceMethod.parameters. TCGC is considering adding a mapping for us to map SdkServiceMethod's parameters to SdkHttpOperation's parameters and bodyType. But before it is supported, we are now using SdkServiceMethod.SdkHttpOperation.
For spread parameters, we will process the parameters and bodyType under SdkHttpOperation. So the order is first query/header/path parameters, then body parameter.
I'm afraid the order would change later if we use SdkServiceMethod.

[weidongxu-microsoft]

Mostly LGTM

• please check the "if" branch, seems to see some extra __raw check that seems not used in the code block
• there is some new collectionFormat from TCGC tsp, tcgc, adopt sdkpackage #2861 (comment)

---

You can use 0.20.0 for next version

[srnagar]

Yeah, we should use isStable to not duplicate the logic of checking for -preview string and also should be case insensitive.