Azure / typespec-azure

About TypeSpec Azure Libraries
https://azure.github.io/typespec-azure/
MIT License
14 stars 40 forks source link

When all model properties are optional, ArmPatchResourceSync/Async will not emit a new Update model #1586

Open zminot opened 1 month ago

zminot commented 1 month ago

If all of the properties of a model are optional (?) and using ArmResourcePatchSync/Async, the emitter will not define and emit a new Update model to represent the PATCH operation. This results in what I think is improper spec (or, at least, azure-openapi-validator complains about it).

Playground Link

In the above playground, the patch expects the EmployeeUpdate which contains EmployeeProperties and the ManagedServiceIdentityUpdate. However, the properties of EmployeeProperties are all create/read-only properties. Removing the nullability of one of the properties--say, age--will instead prompt the emittance of EmployeeUpdate containing EmployeePropertiesUpdate, which in turn is defined with no properties (as nothing from EmployeeProperties should be able to be updated.

Not sure if this improper definition by us, though. We're running into RPC-Patch-V1-02 and RPC-Patch-V1-10 because of this.

mikeharder commented 1 day ago

Related, why does our own sample use ArmCustomPatchSync instead of ArmResourcePatchSync? Shouldn't our sample use the highest-level built-in types?

  update is ArmCustomPatchSync<
    Employee,
    Azure.ResourceManager.Foundations.ResourceUpdateModel<
      Employee,
      EmployeeProperties
    >
  >;

https://github.com/Azure/azure-rest-api-specs/blob/98d74b2db60e46ceb7e3b75755e51519cd500485/specification/contosowidgetmanager/Contoso.Management/employee.tsp#L59-L65

markcowl commented 1 day ago

Part of this is by design, and part is a bug

markcowl commented 1 day ago