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

Mitigate gaps for common types between swagger and typespec #2335

Closed
Tracked by #2320
MaryGao opened this issue Mar 6, 2024 · 10 comments
Closed
Tracked by #2320

Mitigate gaps for common types between swagger and typespec #2335

MaryGao opened this issue Mar 6, 2024 · 10 comments
Assignees
Labels
HRLC Mgmt This issue is related to a management-plane library. p0 priority 0

Comments

@MaryGao
Copy link
Member

MaryGao commented Mar 6, 2024

No description provided.

@MaryGao MaryGao added priority-0 Mgmt This issue is related to a management-plane library. HRLC labels Mar 6, 2024
@kazrael2119
Copy link
Contributor

kazrael2119 commented Mar 11, 2024

  1. model name changed:
    TrackedResource->TrackedResourceBase
    Resource->ArmResource
    ProxyResource->ProxyResourceBase
    same issue with resource model changes of generated SDK code based on TSP compared with swagger typespec-azure#104

  2. model name chagned:
    ManagedServiceIdentity->ManagedIdentityProperties
    ManagedServiceIdentityType->ManagedIdentityType
    same issue with [Bug] ManagedServiceIdentityType enum name is different between TSP and Swagger typespec-azure#278

  3. remove parameter name under ArmResource
    in swagger , it has name https://github.com/Azure/azure-rest-api-specs/blob/main/specification/common-types/resource-management/v5/types.json#L20
    but in tsp, it doesn't have it https://github.com/Azure/typespec-azure/blob/50e384ce1b0597035b819c9370b1cc8545dafbbe/packages/typespec-azure-resource-manager/lib/arm.foundations.tsp#L23
    same issue with Discrepancy of generation from tsp and swagger for resource name property typespec-azure#378

  4. removed model ErrorResponse

  5. type of createdByType in SystemData under swagger common model has modelAsStringhttps://github.com/Azure/azure-rest-api-specs/blob/main/specification/common-types/resource-management/v5/types.json#L502-L524
    in tsp , it only a string https://github.com/Azure/typespec-azure/blob/main/packages/typespec-azure-resource-manager/lib/arm.foundations.tsp#L313-L325

  6. in swagger, parameter id, type from Resource are optional and readonly https://github.com/Azure/azure-rest-api-specs/blob/main/specification/common-types/resource-management/v5/types.json#L14-L29
    in tsp, id, type from ArmResource are required but readonly https://github.com/Azure/typespec-azure/blob/main/packages/typespec-azure-resource-manager/lib/arm.foundations.tsp#L23-L32

@MaryGao
Copy link
Member Author

MaryGao commented Mar 11, 2024

For 1, 2, 3 we have issues tracked.

For 4 this is introduced by design because we move ErrorResponse to core.

For 5 both are modeled as extensible enum but modular name is createdByType but hlc is CreatedByType(normailized).

@MaryGao
Copy link
Member Author

MaryGao commented Mar 11, 2024

For 6 in swagger we could specify the required or optional in required section so the subtypes could change the parent's optionality. but in typespec we can't do that. we would always inheritant this from parent's definition. We may need the tsp team's help to resolve this and issue is tracked here.

For example the DataProduct type didn't specify any required properties in swagger. So all properties are optional.

In typespec the id and type are required by default so the type DataProduct would have them as required https://github.com/Azure/azure-sdk-for-js/pull/28812/files#r1519701904.

@MaryGao
Copy link
Member Author

MaryGao commented Mar 12, 2024

but in typespec we can't do that

One workaround is to override the definition in sub types. tracked here: Azure/typespec-azure#401

@MaryGao
Copy link
Member Author

MaryGao commented Mar 13, 2024

Add HoldOn label cause we need tsp team to resolve common types' breakings.

@MaryGao MaryGao changed the title Analyze gaps for common models between swagger and typespec Analyze gaps for common types between swagger and typespec Mar 13, 2024
@qiaozha qiaozha added p0 priority 0 and removed priority-0 labels Apr 4, 2024
@MaryGao MaryGao changed the title Analyze gaps for common types between swagger and typespec Mitigate gaps for common types between swagger and typespec May 8, 2024
@qiaozha
Copy link
Member

qiaozha commented May 8, 2024

need to upgrade the typespec compiler version and verify it.

@qiaozha qiaozha removed the HoldOn label May 15, 2024
@MaryGao
Copy link
Member Author

MaryGao commented May 20, 2024

Updated as May 20,

  1. fixed
  2. not verify
  3. fixed
  4. fixed
  5. breaking is introduced by extensible enum design change and tracked here: [JS] Redesign Extensible Enum generation #2420
  6. fixed
  7. [New] @kazrael2119 could you verify this also? [TCGC] [ARM] Expected client code on definition model UserAssignedIdentities is Record<UserAssignedIdentity>;  typespec-azure#825

@kazrael2119
Copy link
Contributor

kazrael2119 commented May 20, 2024

Updated as May 20,

  1. fixed
  2. not verify
  3. fixed
  4. fixed
  5. breaking is introduced by extensible enum design change and tracked here: [JS] Redesign Extensible Enum generation #2420
  6. fixed
  7. [New] @kazrael2119 could you verify this also? [TCGC] [ARM] Expected client code on definition model UserAssignedIdentities is Record<UserAssignedIdentity>;  typespec-azure#825

2 fixed

7 not a breaking in JS
image

@MaryGao
Copy link
Member Author

MaryGao commented May 21, 2024

Close this one considering there is no breaking introduced by common types.

@MaryGao MaryGao closed this as completed May 21, 2024
@MaryGao
Copy link
Member Author

MaryGao commented May 21, 2024

the item 7 is tracked here #2535

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
HRLC Mgmt This issue is related to a management-plane library. p0 priority 0
Projects
None yet
Development

No branches or pull requests

3 participants