Azure / autorest.java

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

[MPG] models under `xxUpdateProperties` are not generated with `Update` suffix #2688

Closed XiaofeiCao closed 2 months ago

XiaofeiCao commented 5 months ago

DeviceRegistry diff: https://apiview.dev/Assemblies/Review/90da387f53324bba89095f644611247c/e075fa8b740542859e8723c02d2166aa?diffRevisionId=3fce795412de47d78364048e06ee587f#com.azure.resourcemanager.deviceregistry.models.TransportAuthenticationUpdate

Swagger generates additional xxUpdate classes, e.g. TransportAuthenticationUpdate.

Screenshot 2024-04-11 at 16 14 28

, while tsp doesn't.

weidongxu-microsoft commented 5 months ago

seems typespec-autorest make all models in PATCH adding a Update suffix (include all model referred there) when generate swagger. I don't think SDK want all such duplications. I guess we are good without them, if this is greenfield.

Also see OperationListResult -> PagedOperation May check if we can move this class to implementation.models (the same processing of "Internal" -- maybe I can check this later) so name will not matter.

And some additional setters. Should we enable output-model-immutable option for tsp (again it would cause diff with Swagger, but if it start from tsp it won't be a problem -- it should reduce the API exposed from model class)? About options for greenfield, should we disable the name override? https://github.com/Azure/autorest.java/blob/main/typespec-extension/src/main/java/com/azure/autorest/fluent/TypeSpecFluentPlugin.java#L117-L132

Why this? image

XiaofeiCao commented 5 months ago

Seems like a bug in swagger file:

In common model, the property is int32:

@doc("Percent of the operation that is complete.")
@minValue(0)
@maxValue(100)
percentComplete?: int32;

https://github.com/Azure/typespec-azure/blob/main/packages/typespec-azure-resource-manager/lib/arm.foundations.tsp#L171


Though in swagger, it's number, and our codegen will treat it as float/decimal:

"percentComplete": {
  "description": "Percent of the operation that is complete.",
  "type": "number",
  "minimum": 0,
  "maximum": 100
},

https://github.com/Azure/azure-rest-api-specs/blob/main/specification/common-types/resource-management/v3/types.json#L437-L442

Guess in swagger, it should be "type": "integer"?

weidongxu-microsoft commented 5 months ago

Ok, then that is probably the reason TypeSpec validation fails.

weidongxu-microsoft commented 4 months ago

I don't think we should ever generate with "Updated" suffix.

If we GA with TypeSpec, we do not need to worry about that.

It could be problem if service is TypeSpec, but we GA it with Swagger (AKS fleet?).

XiaofeiCao commented 2 months ago

Default ArmResourcePatch* won't generate even the root xxUpdate now. For greenfield services, we are good. For backward compatibility with brownfield, ArmCustomPatch* along with @parameterVisibility will solve the split update models issue.