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

Implement TCGC's SdkPackage types #2665

Closed
28 of 30 tasks
Tracked by #594
iscai-msft opened this issue Apr 4, 2024 · 11 comments
Closed
28 of 30 tasks
Tracked by #594

Implement TCGC's SdkPackage types #2665

iscai-msft opened this issue Apr 4, 2024 · 11 comments
Assignees
Labels

Comments

@iscai-msft
Copy link
Contributor

iscai-msft commented Apr 4, 2024

Language emitters should work to move entirely to TCGC's new ecosystem. The entry point for the ecosystem is accessing .sdkPackage on the SdkContext returned from tcgc

https://azure.github.io/typespec-azure/docs/howtos/DataPlane%20Generation%20-%20DPG/07tcgcTypes

Tasks

TCGC dependencies:

TCGC dependency issues

  1. bug lib:tcgc
    haolingdong-msft
  2. feature lib:tcgc
    iscai-msft
  3. lib:tcgc
    tadelesh
  4. feature lib:tcgc
    tadelesh
  5. feature lib:tcgc
    iscai-msft
  6. feature lib:tcgc
    iscai-msft
  7. feature lib:tcgc
    iscai-msft
  8. bug lib:tcgc
    haolingdong-msft
  9. feature lib:tcgc
    tadelesh
  10. lib:tcgc
  11. lib:tcgc
    haolingdong-msft
  12. lib:tcgc
    iscai-msft
  13. bug needs-area

TCGC dependency issues - continue.

  1. feature lib:tcgc
    iscai-msft
@haolingdong-msft
Copy link
Member

haolingdong-msft commented May 27, 2024

Hi @iscai-msft @lmazuel, Java may need to start this work at June as we are working on below remaining items related with TCGC model types in May. Please let me know if there is any concern. Thank you.

#2728 (tried from emitter side, now depends on TCGC to remove undefined value)
#2729 (tried from emitter side, depends on two TCGC issues)
#2732 (done)
#2730 (will adopt after TCGC released)
#2731 (done)

@haolingdong-msft
Copy link
Member

haolingdong-msft commented Jul 4, 2024

First API view to show the API diff. Though there are 500+ file diffs, there are just very few public API change. As most file diffs are due to descipription change and adding "accept" and "content-type" header.
https://apiview.dev/Assemblies/Review/e0cf256f814e456bac0f0d751bd4aa9f/81e4187324194e33899d9cbb375aeb4a?diffRevisionId=46902be02c1b4f0cbe9f89e2a5137a05&diffOnly=true

There are not much public API diff. Existing API diffs are caused by below cases:

  1. client structure case: The latest revision does not generate for cadl-ranch client structure case (https://github.com/Azure/cadl-ranch/tree/main/packages/cadl-ranch-specs/http/client/structure), as it has compile error, and depends on TCGC issue: feat(tcgc): provide origin of SdkClientType typespec-azure#1060
  2. ARM case's endpoint cannot be set: this is pending on TypeSpec team's fix. issue: [Feature Request] Get ARM Server with hostParameters typespec-azure#217
  3. api version is added to builder when there are multiple servers - still wip on this.

There are other issues not showing on public API:

  1. parameter description change, e.g. endpoint parameter
  2. "content-type" added to page get operation.
    image
    update: checked the TCGC output and our code, we may need to specially handle the get next page operation to remove the "content-type" added by TCGC for the paging operation.
  3. if it has request body, content type will always be added. So the code is like below:
    image

I am also checking and will update the progress in this comment.

@haolingdong-msft
Copy link
Member

haolingdong-msft commented Jul 15, 2024

7.15 API view:
https://apiview.dev/Assemblies/Review/e0cf256f814e456bac0f0d751bd4aa9f/99297657ec5a4c28a177ad9d3a984044?diffRevisionId=b792a3ef67e044379ff8407f58c99dd1&diffOnly=True

Public API diffs summary:

  1. client structure case: The latest revision does not generate for cadl-ranch client structure case (https://github.com/Azure/cadl-ranch/tree/main/packages/cadl-ranch-specs/http/client/structure), as it has compile error, and depends on TCGC issue: feat(tcgc): provide origin of SdkClientType typespec-azure#1060
  2. ARM case's endpoint cannot be set: pending on TCGC PR to be merged and new release. [Feature Request] Get ARM Server with hostParameters typespec-azure#217
  3. api version is added to builder when there are multiple servers - still wip on this.
  4. operation group's client name change, caused by TCGC not consider @clientName to overwrite client name. [TCGC][Bug] client's name in sdkpackage is not overwritten by decorator @clientName typespec-azure#1164

Other generated code diff can be found in this pr: #2861

  1. content-type and accept headers case change: to Content-Type and Accept.
  2. content-type header will always be added when there is request body.
  3. accept header will always be added when there is response body.
  4. Our logic of adding accept header is removed, now I only rely on TCGC's added header.
  5. endpoint parameter description change
  6. For body optional case, we will not let user to add the content-type header, as it is added by TCGC.
  7. Description is added to javadoc for operations in Impl.

@haolingdong-msft
Copy link
Member

Update for Meeting 8/1:

  1. Finished the two follow up items from last meeting:
    • return type's response: Try to add logic in codegen to dislay response's desc, but have some issues remaining for handling ok response, file an issue first: dpg, add logics to get response's description when calculating return type's javadoc #2878. Finish the logic to merge summary and description on return type to unblock sdkpackage adoption
    • optional body case: remove content type added by TCGC for optional body case

  2. Issues fixed:
    • Override ARM endpoint param: work around from emitter side and regened, passed test, will adopt TCGC after Izzy made the change we agreed on this morning
    • Found the root cause for multiple server case diff, fixed logics and wip regen to test

  3. Integrage with latest TCGC changes:
    • Integrate with TCGC's filtered API version logics: finished and passed tests
    - Merge with multipart changes, found some unexpected diffs, checking now

@haolingdong-msft
Copy link
Member

haolingdong-msft commented Aug 7, 2024

Update on 8/7:
latest API view: com.azure.tools:azure-typespec-tests - apiview.dev

There are only two issues remaining.

  1. multipart (this depends on TCGC bug fix to be released) [Multipart][TCGC] Support @multipartBody for bodyParam of SdkHttpOperation typespec-azure#1281
  2. parameter order changes(maybe due to we adopt TCGC parameter spread feature, I am checking now)

@haolingdong-msft
Copy link
Member

haolingdong-msft commented Aug 12, 2024

Update on 8/12:
latest API view: https://apiview.dev/Assemblies/Review/e0cf256f814e456bac0f0d751bd4aa9f/e0f4e0c3e4704de89332b6229b14ad64?diffRevisionId=fbdae726b3a04e839587766478ca88eb&diffOnly=true

The API diff is all due to below:

image

Diff in impl:

  • TCGC has not supported allowReserved for path parameter yet, so I comment out the corresponding part of logic in emitter and add TODO fc35d0f.
  • A strange thing is the order of properties changes during regen, and it only happens to ARM generated code I'm checking the reason. 643105b

@weidongxu-microsoft
Copy link
Member

weidongxu-microsoft commented Aug 13, 2024

Order change on property may due to a PR by @XiaofeiCao -- could you take a look?

kind of like this https://github.com/Azure/azure-sdk-for-java/pull/41497/files

@XiaofeiCao
Copy link
Contributor

Order change on property may due to a PR by @XiaofeiCao -- could you take a look?

kind of like this https://github.com/Azure/azure-sdk-for-java/pull/41497/files

Yes, it is. I've replied in chat:

ARM diff is expected. It's a side-effect of a bug fix:
fix, mgmt shadow properties from Resource/ProxyResource by XiaofeiCao · Pull Request #2905 · Azure/autorest.java (github.com)

generated code diffs are reordering of the shadowed properties(a side effect of the fix logic). Now properties from subclass will come before the ones from parent class.

@haolingdong-msft
Copy link
Member

Java released emitter version 0.20.0 with sdkpackage adoption changes in:
https://www.npmjs.com/package/@azure-tools/typespec-java/v/0.20.0
I have also verified the diffs in SDK repo and the refreshed code in SDK repo.
Azure/azure-sdk-for-java#41666

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants