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

Java codegen, use TCGC enum name for MPG #4315

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

XiaofeiCao
Copy link
Member

@XiaofeiCao XiaofeiCao commented Sep 2, 2024

@microsoft-github-policy-service microsoft-github-policy-service bot added the emitter:client:java Issue for the Java client emitter: @typespec/http-client-java label Sep 2, 2024
@azure-sdk
Copy link
Collaborator

No changes needing a change description found.

@azure-sdk
Copy link
Collaborator

You can try these changes here

🛝 Playground 🌐 Website 📚 Next docs

@haolingdong-msft
Copy link
Member

is there any test to cover this?

@XiaofeiCao
Copy link
Member Author

is there any test to cover this?

Yeah, I'm adding that. I'll skip Swagger fluent-test for now since currently we don't have that in microsoft/typespec.

@weidongxu-microsoft
Copy link
Contributor

The repo is not yet up-to-date, so local test would do, at present.

Comment on lines 14 to +15
/**
* Static value None for ManagedServiceIdentityType.
* No managed identity.
Copy link
Member Author

Choose a reason for hiding this comment

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

Result of using TCGC enum description.

Comment on lines 31 to +32
*/
public static final ManagedServiceIdentityType SYSTEM_ASSIGNED_USER_ASSIGNED
public static final ManagedServiceIdentityType SYSTEM_AND_USER_ASSIGNED_V3
Copy link
Member Author

@XiaofeiCao XiaofeiCao Sep 3, 2024

Choose a reason for hiding this comment

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

Result of using TCGC enum name:
https://github.com/Azure/typespec-azure/blob/c76a9c190ec5753627e56365dce8dd0ca53486f3/packages/typespec-azure-resource-manager/lib/common-types/managed-identity.tsp#L75

union ManagedServiceIdentityType {
  /** No managed identity. */
  None: "None",

  /** System assigned managed identity. */
  SystemAssigned: "SystemAssigned",

  /** User assigned managed identity. */
  UserAssigned: "UserAssigned",

  /** System and user assigned managed identity. */
  @renamedFrom(Versions.v3, "SystemAndUserAssigned")
  @removed(Versions.v4)
  SystemAndUserAssignedV3: "SystemAssigned,UserAssigned",

  /** System and user assigned managed identity. */
  @added(Versions.v4)
  @renamedFrom(Versions.v4, "SystemAndUserAssigned")
  @removed(Versions.v5)
  SystemAndUserAssignedV4: "SystemAssigned, UserAssigned",

  /** System and user assigned managed identity. */
  @added(Versions.v5)
  SystemAndUserAssigned: "SystemAssigned,UserAssigned",

  string,
}

Copy link
Member Author

@XiaofeiCao XiaofeiCao Sep 3, 2024

Choose a reason for hiding this comment

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

Not sure it's a good enum name...
@renamedFrom(Versions.v4, "SystemAndUserAssigned") should mean its should've been SystemAndUserAssigned, not sure if it's a TCGC bug.

Copy link
Contributor

@weidongxu-microsoft weidongxu-microsoft Sep 3, 2024

Choose a reason for hiding this comment

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

I can see the reason, but this is pretty unexpected.

On @renamedFrom(Versions.v3, "SystemAndUserAssigned"), did typespec actually wish name "SystemAndUserAssigned" be used (but projection seems not handling this?)?
E.g., how typespec-autorest handles this?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's referencing common types...

"identity": {
  "$ref": "../../../../common-types/v3/managedidentity.json#/definitions/ManagedServiceIdentity",
  "description": "Managed identity."
}

Even if emit-common-types-schema: for-visibility-changes, generated type will still reference that from common types:

"type": {
  "$ref": "../../../../common-types/v3/managedidentity.json#/definitions/ManagedServiceIdentityType",
}

In common types, it's defined as:

 "ManagedServiceIdentityType": {
  "description": "Type of managed service identity (where both SystemAssigned and UserAssigned types are allowed).",
  "enum": [
    "None",
    "SystemAssigned",
    "UserAssigned",
    "SystemAssigned,UserAssigned"
  ],
  "type": "string",
  "x-ms-enum": {
    "name": "ManagedServiceIdentityType",
    "modelAsString": true
  }
}

I guess this will differ with m4 generated name.

Copy link
Member Author

@XiaofeiCao XiaofeiCao Sep 3, 2024

Choose a reason for hiding this comment

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

Synced with Chenjie, the tsp definition seems not correct. I'll create an issue for TypeSpec team.

Copy link
Member Author

Choose a reason for hiding this comment

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

If tsp:

union DogKind {
  string,

  @doc("Species golden")
  Golden: "golden_dog",

  @doc("Species golden")
  @added(Versions.v2023_11_01)
  @removed(Versions.v2023_12_01_preview)
  @renamedFrom(Versions.v2023_11_01, "Bull")
  Bull1: "bull_dog",

  @doc("Species golden")
  @added(Versions.v2023_12_01_preview)
  @renamedFrom(Versions.v2023_12_01_preview, "Bull")
  Bull2: "bull_dog",
}

When generating for Versions.v2023_12_01_preview, Swagger would be:

"DogKind": {
  "type": "string",
  "description": "extensible enum type for discriminator",
  "enum": [
    "golden_dog",
    "bull_dog"
  ],
  "x-ms-enum": {
    "name": "DogKind",
    "modelAsString": true,
    "values": [
      {
        "name": "Golden",
        "value": "golden_dog",
        "description": "Species golden"
      },
      {
        "name": "Bull2",
        "value": "bull_dog",
        "description": "Species golden"
      }
    ]
  }
}

Same issue.

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

They've fixed it: https://github.com/Azure/typespec-azure/pull/1551/files

Now Swagger and TypeSpec matches each other.
Swagger: https://github.com/Azure/azure-rest-api-specs/blob/a3b67e1e3a9554951860b9f9d2e1723036a5e032/specification/common-types/resource-management/v5/managedidentity.json#L42

"ManagedServiceIdentityType": {
  "description": "Type of managed service identity (where both SystemAssigned and UserAssigned types are allowed).",
  "enum": [
    "None",
    "SystemAssigned",
    "UserAssigned",
    "SystemAssigned,UserAssigned"
  ],
  "type": "string",
  "x-ms-enum": {
    "name": "ManagedServiceIdentityType",
    "modelAsString": true
  }
}

Since no release this month uses identity(mongocluster, fabric, computeschedule), and ExpandableEnum relies on some of this PR's implementation, I'll merge this PR.

@XiaofeiCao XiaofeiCao marked this pull request as ready for review September 3, 2024 05:34
github-merge-queue bot pushed a commit that referenced this pull request Sep 5, 2024
- Part of #4315, though before
TypeSpec ARM fix, we'll probably will not merge that PR
sarangan12 pushed a commit to sarangan12/typespec that referenced this pull request Sep 16, 2024
…4334)

- Part of microsoft#4315, though before
TypeSpec ARM fix, we'll probably will not merge that PR
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
emitter:client:java Issue for the Java client emitter: @typespec/http-client-java
Projects
None yet
Development

Successfully merging this pull request may close these issues.

tsp, honor name for Enum in TypeSpec
4 participants