search

Azure / autorest.java

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

mgmt+tsp, brownfield for vmware/avs #2803

Open weidongxu-microsoft opened 3 weeks ago

weidongxu-microsoft commented 3 weeks ago

vmware/avs already GAed with Swagger

service moved to TypeSpec on 2023-09-01 https://github.com/Azure/azure-rest-api-specs/pull/28023 (at the time of the PR, the breaking changes CI having problem and not picking up the breaks)

service should name the folder as suffix ".Management", see https://github.com/Azure/azure-rest-api-specs/blob/main/documentation/typespec-structure-guidelines.md#service-folder-structure

WIP

weidongxu-microsoft commented 3 weeks ago

REST API breaks

Need confirmation from service whether this is intended.

weidongxu-microsoft commented 3 weeks ago

API breaks

This is hard to keep non-breaking change in SDK.

weidongxu-microsoft commented 3 weeks ago

SDK models

Mostly addressable in "client.tsp" https://github.com/Azure/azure-rest-api-specs/pull/29308

Important: AddonProperties cannot be x-ms-client-flatten as it is polymorphic Should TCGC handle this (i.e., set flatten=false, whatever the @flattenProperty)?

weidongxu-microsoft commented 3 weeks ago

SDK operation groups

WIP on whether we need to modify the interface structure in "route.tsp" e.g. https://github.com/Azure/azure-rest-api-specs/pull/29336/commits/7105e2d048d317d58071527a48c0d3ef86884a39

(alternative is write all the APIs explicitly in client.tsp, which likely be hard to maintain)

weidongxu-microsoft commented 3 weeks ago

Potential bug

Java: seems missing Addon$Update.withProperties <-- not emitter bug, ARM ProxyResource has the @visibility("read", "create") https://github.com/Azure/typespec-azure/blob/main/packages/typespec-azure-resource-manager/lib/models.tsp#L54-L68

cataggar commented 3 weeks ago

service should name the folder as suffix ".Management", see https://github.com/Azure/azure-rest-api-specs/blob/main/documentation/typespec-structure-guidelines.md#service-folder-structure

I thought it was the RP name. Yes, you can change it to align with the guidance.

cataggar commented 3 weeks ago

We do not actually want the operation API in our spec or SDKs. I've never understood why it is required. Anyhow, in TypeSpec we are using the standard typespec-azure-core definition.

https://github.com/Azure/azure-rest-api-specs/blob/32f566eb38af9593cfc1ed183362471c50d67fb2/specification/vmware/Microsoft.AVS/routes.tsp#L15

cataggar commented 3 weeks ago

This was unintentional. It should be changed back to int64.

cataggar commented 3 weeks ago

This was an approved breaking change. model WorkloadNetwork is a @singleton and this is how it is modeled in TypeSpec.

cataggar commented 3 weeks ago

The properties are spread in. This is how it is done in TypeSpec.

model ManagementCluster {
  ...CommonClusterProperties;
}
model ClusterProperties {
  ...CommonClusterProperties;
}

I'm not sure of another way to do it in TypeSpec. For example, this does not work here: image

Not having the inheritance modeled is breaking, but seams acceptable.

cataggar commented 3 weeks ago

SDK operation groups

  • Lots of @operationId, which is not supported in SDK emitter (they are only for typespec-autorest emitter)

WIP on whether we need to modify the interface structure in "route.tsp" e.g. Azure/azure-rest-api-specs@7105e2d

(alternative is write all the APIs explicitly in client.tsp, which likely be hard to maintain)

I did a lot of work to make the operationIds match in the generated OpenAPI. Are there operationIds that do not match? Are you generating the SDK from the OpenAPI spec or directly from TypeSpec?

weidongxu-microsoft commented 3 weeks ago

SDK operation groups

  • Lots of @operationId, which is not supported in SDK emitter (they are only for typespec-autorest emitter)

WIP on whether we need to modify the interface structure in "route.tsp" e.g. Azure/azure-rest-api-specs@7105e2d (alternative is write all the APIs explicitly in client.tsp, which likely be hard to maintain)

I did a lot of work to make the operationIds match in the generated OpenAPI. Are there operationIds that do not match? Are you generating the SDK from the OpenAPI spec or directly from TypeSpec?

@operationId is from typespec-openapi lib https://typespec.io/docs/libraries/openapi/reference/decorators#@TypeSpec.OpenAPI.operationId

Java SDK would be generated directly from TypeSpec (and hence won't use the openapi lib or any decorator within). -- any SDK emitter that is ready to generate from TypeSpec would generate SDK from TypeSpec

The @operationId is required anyway, since MS Learn still generate REST API doc from Swagger. Just SDK emitter need a different way to solve the operation group. Would you mind we adjust the interface in route.tsp (we would also get a acknowledge from Alan/Mark before doing it)?

PS: we are making corrections to operation name (the part after _ in operationId in Swagger) as well. This is something we can do in client.tsp https://github.com/Azure/azure-rest-api-specs/blob/1418642cf8a57954c781c26c759fd4aef39a1287/specification/vmware/Microsoft.AVS/client.tsp#L44-L56

weidongxu-microsoft commented 3 weeks ago

We do not actually want the operation API in our spec or SDKs. I've never understood why it is required. Anyhow, in TypeSpec we are using the standard typespec-azure-core definition.

https://github.com/Azure/azure-rest-api-specs/blob/32f566eb38af9593cfc1ed183362471c50d67fb2/specification/vmware/Microsoft.AVS/routes.tsp#L15

But, unfortunately, it is already in Swagger for a very long time, and lots of SDK have been shipped with that.

We will discuss the break within SDK devs first, see if we accept it, or try to keep backward-compatible. Do you mind, if we modify the operation to make it same as your older Swagger, if we decide to keep backward-compatible?

weidongxu-microsoft commented 3 weeks ago

This was unintentional. It should be changed back to int64.

PR to change back to int64 https://github.com/Azure/azure-rest-api-specs/pull/29351 Please approve the PR if you are OK with it.

cataggar commented 3 weeks ago

We do not actually want the operation API in our spec or SDKs. I've never understood why it is required. Anyhow, in TypeSpec we are using the standard typespec-azure-core definition. https://github.com/Azure/azure-rest-api-specs/blob/32f566eb38af9593cfc1ed183362471c50d67fb2/specification/vmware/Microsoft.AVS/routes.tsp#L15

But, unfortunately, it is already in Swagger for a very long time, and lots of SDK have been shipped with that.

We will discuss the break within SDK devs first, see if we accept it, or try to keep backward-compatible. Do you mind, if we modify the operation to make it same as your older Swagger, if we decide to keep backward-compatible?

We are using the standard Azure.ResourceManager.Operations. We never wanted the API in our SDKs. We do not want to maintain a non-standard version of it.

cataggar commented 3 weeks ago

Hi @weidongxu-microsoft , an engineer working on the Azure CLI noticed that ManagementCluster used to require clusterSize, but no longer does. That was non intentional and should be changed back. Do you see the impact in this generated Java SDK?

https://github.com/Azure/azure-rest-api-specs/blob/1418642cf8a57954c781c26c759fd4aef39a1287/specification/vmware/resource-manager/Microsoft.AVS/stable/2023-03-01/vmware.json#L6106-L6110

    "ManagementCluster": {
      "type": "object",
      "description": "The properties of a management cluster",
      "required": [
        "clusterSize"
      ],
weidongxu-microsoft commented 3 weeks ago

Hi @weidongxu-microsoft , an engineer working on the Azure CLI noticed that ManagementCluster used to require clusterSize, but no longer does. That was non intentional and should be changed back. Do you see the impact in this generated Java SDK?

https://github.com/Azure/azure-rest-api-specs/blob/1418642cf8a57954c781c26c759fd4aef39a1287/specification/vmware/resource-manager/Microsoft.AVS/stable/2023-03-01/vmware.json#L6106-L6110

    "ManagementCluster": {
      "type": "object",
      "description": "The properties of a management cluster",
      "required": [
        "clusterSize"
      ],

As mentioned in the discription of this issue, the BreakingChanges CI is broken when your merge that migration PR. It really not able to report anything useful. (tooling team already got notified and fixing/fixed the problem)

This part of Swagger is pretty tricky. You can see that the clusterSize is defined in CommonClusterProperties, but the required=["clusterSize"] is defined in ManagementCluster. SDK codegen/emitter may not able to handle this case as what you expect (as most langugages model this allOf as superclass/subclass relationship, and subclass usually won't touch property/variable from its superclass).

I've looked at existing .NET code (2021-06-01? form the last commit), appears not a required as well. (it is int? with a default value) https://github.com/Azure/azure-sdk-for-net/blob/main/sdk/avs/Microsoft.Azure.Management.Avs/src/Generated/Models/ManagementCluster.cs#L39

weidongxu-microsoft commented 3 weeks ago

And we are still in the progress of trying to get all the potential breaks.

Here is another one, please let us know if we want to revert this int32 back to int64

cataggar commented 3 weeks ago

And we are still in the progress of trying to get all the potential breaks.

Here is another one, please let us know if we want to revert this int32 back to int64

Yes, also unintentional and should be changed back.

cataggar commented 3 weeks ago

This part of Swagger is pretty tricky. You can see that the clusterSize is defined in CommonClusterProperties, but the required=["clusterSize"] is defined in ManagementCluster. SDK codegen/emitter may not able to handle this case as what you expect (as most langugages model this allOf as superclass/subclass relationship, and subclass usually won't touch property/variable from its superclass).

I've looked at existing .NET code (2021-06-01? form the last commit), appears not a required as well. (it is int? with a default value) https://github.com/Azure/azure-sdk-for-net/blob/main/sdk/avs/Microsoft.Azure.Management.Avs/src/Generated/Models/ManagementCluster.cs#L39

You are right. Same with JavaScript here, it is clusterSize?: number;. I think it is best to leave it the way it is.

weidongxu-microsoft commented 2 weeks ago

Yes, also unintentional and should be changed back.

PR to fix https://github.com/Azure/azure-rest-api-specs/pull/29395

weidongxu-microsoft commented 2 weeks ago

I was told by Go that there was multiple places in PATCH, that previously request body is e.g. WorkloadNetworkSegment (same model as in PUT) being changed to WorkloadNetworkSegmentUpdate

This would cause some breaks to Go (and probably other languages as well).

Please let us know if this is intended. If yes, any reason?

raych1 commented 2 weeks ago

SDK models

  • x-ms-client-flatten
  • mismatch on model names / property names

Mostly addressable in "client.tsp" Azure/azure-rest-api-specs#29308

Important: AddonProperties cannot be x-ms-client-flatten as it is polymorphic Should TCGC handle this (i.e., set flatten=false, whatever the @flattenProperty)?

@weidongxu-microsoft , the following two properties need to be renamed back to current name in client.tsp.

`PrivateCloud.Identity`
`PrivateCloudUpdate.Identity`
weidongxu-microsoft commented 2 weeks ago

SDK models

  • x-ms-client-flatten
  • mismatch on model names / property names

Mostly addressable in "client.tsp" Azure/azure-rest-api-specs#29308 Important: AddonProperties cannot be x-ms-client-flatten as it is polymorphic Should TCGC handle this (i.e., set flatten=false, whatever the @flattenProperty)?

@weidongxu-microsoft , the following two properties need to be renamed back to current name in client.tsp.

`PrivateCloud.Identity`
`PrivateCloudUpdate.Identity`

In TypeSpec, it can be renamed in client.tsp https://github.com/Azure/azure-rest-api-specs/blob/1418642cf8a57954c781c26c759fd4aef39a1287/specification/vmware/Microsoft.AVS/client.tsp#L59-L64

For Swagger, it may had to be done via directive. https://github.com/Azure/azure-rest-api-specs/pull/29413

weidongxu-microsoft commented 2 weeks ago

This can be handled by using a customized ResourceList template, e.g. https://github.com/Azure/azure-rest-api-specs/pull/29408/files

weidongxu-microsoft commented 2 weeks ago

There is certain change to "x-ms-mutability", e.g. properties in Addon changed to "read", "create" -- meaning this proxy resource cannot be updated

The x-ms-mutability comes from ProxyResource in lib

Is it intended?

cataggar commented 2 weeks ago

There is certain change to "x-ms-mutability", e.g. properties in Addon changed to "read", "create" -- meaning this proxy resource cannot be updated

The x-ms-mutability comes from ProxyResource in lib

Is it intended?

It is not intended.

cataggar commented 2 weeks ago
  • rename of List responses from ##List to ##ListResult

This can be handled by using a customized ResourceList template, e.g. https://github.com/Azure/azure-rest-api-specs/pull/29408/files

I approved that draft. That looks like a good way to restore name compatibility.

cataggar commented 2 weeks ago

@weidongxu-microsoft , the following two properties need to be renamed back to current name in client.tsp.

`PrivateCloud.Identity`
`PrivateCloudUpdate.Identity`

In TypeSpec, it can be renamed in client.tsp https://github.com/Azure/azure-rest-api-specs/blob/1418642cf8a57954c781c26c759fd4aef39a1287/specification/vmware/Microsoft.AVS/client.tsp#L59-L64

For Swagger, it may had to be done via directive. Azure/azure-rest-api-specs#29413

We do wish that the generated SDKs use the standard identity types. We support system identity now, but will support user managed identity in the future.

cataggar commented 2 weeks ago

I was told by Go that there was multiple places in PATCH, that previously request body is e.g. WorkloadNetworkSegment (same model as in PUT) being changed to WorkloadNetworkSegmentUpdate

This would cause some breaks to Go (and probably other languages as well).

Please let us know if this is intended. If yes, any reason?

No, not intended.

weidongxu-microsoft commented 2 weeks ago

There is certain change to "x-ms-mutability", e.g. properties in Addon changed to "read", "create" -- meaning this proxy resource cannot be updated The x-ms-mutability comes from ProxyResource in lib Is it intended?

It is not intended.

The unreleased typespec-azure-resource-manager appears have removed this @visibility https://github.com/Azure/typespec-azure/blob/main/packages/typespec-azure-resource-manager/lib/models.tsp#L62-L66

typespec-next

We probably just wait for the new lib to be released.

weidongxu-microsoft commented 2 weeks ago

I was told by Go that there was multiple places in PATCH, that previously request body is e.g. WorkloadNetworkSegment (same model as in PUT) being changed to WorkloadNetworkSegmentUpdate This would cause some breaks to Go (and probably other languages as well). Please let us know if this is intended. If yes, any reason?

No, not intended.

PR to move it back https://github.com/Azure/azure-rest-api-specs/pull/29392

weidongxu-microsoft commented 2 weeks ago

We do wish that the generated SDKs use the standard identity types. We support system identity now, but will support user managed identity in the future.

ManagedServiceIdentity has one more property (userAssignedIdentities) compared to SystemAssignedServiceIdentity. Other properties appear to be exactly same.

Therefore, I think service should be able to evolve from SystemAssignedServiceIdentity to ManagedServiceIdentity.

Though for SDK, we probably would continue do this rename, so that user would see a same class/model (with new property userAssignedIdentities), before/after the evolution.

cataggar commented 2 weeks ago

Thanks @weidongxu-microsoft for the spec PRs. Here is a summary:

cataggar commented 2 weeks ago

The TypeSpec fix was merged two days ago. You can see it fixes our spec.

weidongxu-microsoft commented 1 week ago

All Swagger related PR should have been merged.

Allen upgraded spec repo to 0.57 as well (it includes the Swagger change via typespec-azure-resource-manager).

Swagger should be completed.

Remain issue to TypeSpec (on SDK emitter) is the operation group that is still in discussion https://gist.github.com/weidongxu-microsoft/fa420c35cf0611a62a8d155771dd3425