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

Sdk package methods adoption #2943

Merged
merged 76 commits into from
Dec 26, 2024
Merged

Conversation

qiaozha
Copy link
Member

@qiaozha qiaozha commented Dec 9, 2024

Goals:

  1. Sdk Package method adoptions:
    fixes Adopt the getAllOperations in TCGC for modular #2322
    fixes Implement TCGC's SdkPackage types #2406
    fixes [Epic][DPG] Adopt to the TCGC code model converter #1893

  2. Handle apiVersion at client level.
    fixes Should we prompt apiVersion if the service is not versioning controled? #2867
    Partially fixes [Codegen/TypeSpec] Handle apiVersion as Path Parameter #2148 in Modular

  3. Support client level path parameter in general.
    fixes Support client-level parameter in general for modular #2618
    fixes Respect client level parameter in api client factory function #2809

  4. Support generate paging in unbranded as Azure style.
    fixes Unbranded CodeGen should generate paging operation with Azure Style #2932

Non Goals:

We should manage the operation reference with binder as what we have done for models, but since this could be done in a separate PR and we are in a tight timeline, prefer to not include it this time.
#2862

Summary of the changes:

1. Adopt to sdkPackage methods

  • remove Modular buildCodeModel logic
  • replace Modular's own operations/types with TCGC operations/methods/types and remove all the places that has references to those Modular types.
  • replace listOperationsInOperationGroups with method iteration logic.
  • introduce ModularClientOptions and ModularEmitterOptions for codegen's own generation options and filepath related logic.
  • remove previous client initialization work around for apiVersion related change.

2. Support client level path parameter in general.

You will see the path parameter has been lifted to the ClientContext properties, and inside operation send requestion request operation, we will set that value from context.

3. contentType/accept header parameter logic change,

  • for contentType with constantType in typespec, we will just respect tsp definition do no more things about it.
  • for tcgc guessed contentType no matter it's constant or non constant due to multiple resposne, we will set the value if it has but not expose that parameter either in operation signature or in operation options.
  • If typespec has specified contentType/accept as a non constant type, it will stay in the operation signature if required and optional parameter if optional.
  • if customer wants to modify contentType or accept headers, they could use options.requestOptions.headers to pass that to the client no matter what the type is.

4. apiVersion and its policy related change.

  • we will generate apiVersion as a Context property if it's required and has been lifted to the client level by tcgc.
  • we will add ClientApiVersion policy in the client level as long as it's a method level apiVersion
  • we will remove the operation level ClientApiVersion policy if this specific operation doesn't have apiVersion or the apiVersion is not a query parameter.
  • we will set the non query apiVersion such as path apiVersion or header apiVersion in each operation send request function.

A side note: we can't completely remove the ClientApiVersion policy as in paging nextLink and polling process, we are leveraging glass breaker to help us sending that url, removing the policy in global would cause problems for them.

5. Parameter order related change.

Previously, when we discuss about spread, we have found that we have no way to figure out the order between a normal parameter and body parameter spreaded parameters, but adopt to tcgc has resolved this problem, See this comment for more details #2943 (comment)

6. Bytes gets generated inconsistent as before.

This is because the logic that tcgc uses to infer whether the bytes should be translated as a binary bytes is inconsistent with what we use before, see Azure/typespec-azure#1999. For now, we just trust the encode tcgc returns to us.

7. replace core related dependencies in static helper.

As todo spec in the smoke test has paging operation, we have to generate paginate helper for it which has some core related dependencies, we have to replace those dependencies into @typespec/ts-http-runtime for unbranded.

Issues found in TCGC

Azure/typespec-azure#1999
Azure/typespec-azure#1994
Azure/typespec-azure#1993
Azure/typespec-azure#1985

@qiaozha qiaozha marked this pull request as ready for review December 20, 2024 11:29
Copy link
Member

@joheredi joheredi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is nothing that concerns me looking at the api surface changes. The only thing I would like you to get closure on before merging is the addition of accept as a parameter.

Copy link
Member

@MaryGao MaryGao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Quickly go through most codesnipet but I didn't realize the changes you mention for bytes part. I may take a review again for rest part but I expect no big comments.

@qiaozha qiaozha merged commit 3b5a635 into Azure:main Dec 26, 2024
15 checks passed
@qiaozha qiaozha deleted the sdkPackage-methods-adoption branch December 30, 2024 13:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants