search

Azure / autorest.java

Extension for AutoRest (https://github.com/Azure/autorest) that generates Java code
MIT License
32 stars 80 forks source link

tsp, TCGC common layer, adopt model types #2698

Closed haolingdong-msft closed 1 month ago

haolingdong-msft commented 2 months ago

API view with diff comparing this PR's generated code for typespec-tests with main branch: https://apiview.dev/Assemblies/Review/e0cf256f814e456bac0f0d751bd4aa9f/d08e9493669445c4bf9916c090262bd5?diffRevisionId=b988ab5926314dc0a7d226750d035115&doc=False&diffOnly=True

Note this: Currently TCGC treated enum as fixed enum, and this can cause breaking changes in mgmt plane sdk. typespec-azure will fix the tsp definition in 0.42. image

Fix https://github.com/Azure/autorest.java/issues/2564

Will create issues for below follow up items and create seperate PR to support:

weidongxu-microsoft commented 2 months ago

Chenjie mentioned for the new additionalProperties tests, it would need dev version of TCGC.

Also is there some code we can now delete in code-model-builder tsp? It seems have more lines than before (TCGC supposed to be able to reduce the code).

haolingdong-msft commented 2 months ago

I will use latest dev version to regen and test. I am also wip on cleaning up the codes, there are codes can be deleted.

weidongxu-microsoft commented 2 months ago

Just a notice.

There appears to be a bug on DifferentSpreadStringDerived, which should be a subclass of DifferentSpreadStringRecord, but not so in generated code. https://github.com/Azure/cadl-ranch/blob/main/packages/cadl-ranch-specs/http/type/property/additional-properties/main.tsp#L389-L394

(would take a look on whether the cause is in typescript code or Java code -- emm, both https://github.com/Azure/autorest.java/pull/2703)

If TCGC have change on that, this should be good.

weidongxu-microsoft commented 2 months ago

You will need to d a minor version bump, before release this change.

haolingdong-msft commented 1 month ago

Our build passes at version "@azure-tools/typespec-client-generator-core": "0.42.0", but encouter generation error at version "@azure-tools/typespec-client-generator-core": "0.42.1".

Seems TCGC has issue on version 0.42.1 which causes the build failure: https://dev.azure.com/azure-sdk/internal/_build/results?buildId=3777204&view=logs&j=ca395085-040a-526b-2ce8-bdc85f692774&t=50b9345f-2a7f-5980-cb0f-1a8f9fb09dd0&l=4528

Fixed the TCGC version to 0.42.0 currently to unblock Java CI.

@tadelesh @iscai-msft

tadelesh commented 1 month ago

Our build passes at version "@azure-tools/typespec-client-generator-core": "0.42.0", but encouter generation error at version "@azure-tools/typespec-client-generator-core": "0.42.1".

Seems TCGC has issue on version 0.42.1 which causes the build failure: https://dev.azure.com/azure-sdk/internal/_build/results?buildId=3777204&view=logs&j=ca395085-040a-526b-2ce8-bdc85f692774&t=50b9345f-2a7f-5980-cb0f-1a8f9fb09dd0&l=4528

Fixed the TCGC version to 0.42.0 currently to unblock Java CI.

@tadelesh @iscai-msft

0.42.1 should be a wrong hotfix release. we are trying to release another 0.42.2.

haolingdong-msft commented 1 month ago

Remaining issues on this PR:

  1. TCGC 0.42.2 released, I will integrate with the latest version to see if there is any behavior change.
  2. Mgmt case error after upgrade to 0.42: https://github.com/Azure/autorest.java/pull/2698#discussion_r1596260260, Xiaofei is working on this. But we have pending discussion issue on TCGC(see item 3)

Three pending discussion issues:

  1. Type of model property/enum member when it is defined as constant number: https://github.com/microsoft/typespec/issues/3314
  2. when should standalone models be generated: https://github.com/Azure/typespec-azure/issues/823
  3. ARM has definition: model UserAssignedIdentities is Record<UserAssignedIdentity>; But for model A is Record<> case, TCGC will return model A with additional property, this will be potential breaking changes in Mgmt plane, as current swagger generated code will use Map directly, instead of model with additonal properties. Should TCGC handle the UserAssignedIdentities specially or should ARM change the definition? https://github.com/Azure/typespec-azure/issues/825

/cc @iscai-msft @tadelesh @lmazuel

XiaofeiCao commented 1 month ago
  1. Mgmt case error after switching to 0.42: tsp, TCGC common layer, adopt model types #2698 (comment), Xiaofei is working on this.

Offline synced with Haoling. The extra generated UserAssignedIdentities class is due to that we switched to using TCGC's result, and in 0.42, tsp-arm introduced a new model UserAssignedIdentities is Record<UserAssignedIdentity>, this will cause TCGC to generate a model A if model A is Record<>.

Though, tsp-arm themselves discourage use of model is Record: https://azure.github.io/typespec-azure/docs/libraries/azure-resource-manager/rules/no-record#-incorrect-1

@weidongxu-microsoft We probably need a special handling for UserAssignedIdentity, or ARM, in case of breaking changes to existing mgmt libs.

Raised an issue here: https://github.com/Azure/typespec-azure/issues/824