Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

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

Closed
XiaofeiCao opened this issue Apr 11, 2024 · 5 comments
Closed
Assignees
Labels
Mgmt This issue is related to a management-plane library.

Comments

@XiaofeiCao
Copy link
Contributor

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.

@XiaofeiCao XiaofeiCao added the Mgmt This issue is related to a management-plane library. label Apr 11, 2024
@XiaofeiCao XiaofeiCao self-assigned this Apr 11, 2024
@weidongxu-microsoft
Copy link
Member

weidongxu-microsoft commented Apr 12, 2024

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
Copy link
Contributor Author

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
Copy link
Member

weidongxu-microsoft commented Apr 15, 2024

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

@weidongxu-microsoft
Copy link
Member

weidongxu-microsoft commented May 16, 2024

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
Copy link
Contributor Author

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.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Mgmt This issue is related to a management-plane library.
Projects
None yet
Development

No branches or pull requests

2 participants