Azure / azure-rest-api-specs

The source for REST API specifications for Microsoft Azure.
MIT License
2.69k stars 5.12k forks source link

Stop using x-ms-client-flatten #13578

Open jiasli opened 3 years ago

jiasli commented 3 years ago

Context

The inconsistent usage of x-ms-client-flatten across Resource Providers causes inconsistency between Azure CLI commands' return values.

For example, az group show (internally calls Resource Groups - Get) returns a ResourceGroup with properties:

> az group show -n test
{
  "id": "/subscriptions/0b1f6471-1bf0-4dda-aec3-cb9272f09590/resourceGroups/test",
  "location": "westus",
  "managedBy": null,
  "name": "test",
  "properties": {
    "provisioningState": "Succeeded"
  },
  "tags": null,
  "type": "Microsoft.Resources/resourceGroups"
}

Meanwhile, az storage account show (internally calls Storage Accounts - Get Properties) returns a StorageAccount whose properties has been flattened by Python SDK:

> az storage account show -n test
{
  "accessTier": "Hot",
  "allowBlobPublicAccess": null,
  "allowSharedKeyAccess": null,
  "azureFilesIdentityBasedAuthentication": null,
  ...
  "name": "test",
  "type": "Microsoft.Storage/storageAccounts"
}

The reason behind the flattening is because ResourceGroup's REST spec defines

https://github.com/Azure/azure-rest-api-specs/blob/e10fd5f5f0b3444e589cd816321fce7036e33554/specification/resources/resource-manager/Microsoft.Resources/stable/2020-10-01/resources.json#L5549-L5552

while StorageAccount's REST spec defines:

https://github.com/Azure/azure-rest-api-specs/blob/e10fd5f5f0b3444e589cd816321fce7036e33554/specification/storage/resource-manager/Microsoft.Storage/stable/2021-01-01/storage.json#L3083-L3087

Azure Python SDK honors x-ms-client-flatten and flattens properties accordingly.

Expected solution

All REST specs should stop specifying x-ms-client-flatten in order to provided consistent responses across all Resource Providers and all SDKs.

Originally posted by @Morriz in https://github.com/Azure/azure-cli/issues/17332

ghost commented 3 years ago

Thanks for the feedback! We are routing this to the appropriate team for follow-up. cc @xgithubtriage.

Issue Details
## Context The inconsistent usage of `x-ms-client-flatten` across Resource Providers causes inconsistency between Azure CLI commands' return values. For example, [`az group show`](https://docs.microsoft.com/en-us/cli/azure/group?view=azure-cli-latest#az_group_show) (internally calls [Resource Groups - Get](https://docs.microsoft.com/en-us/rest/api/resources/resourcegroups/get)) returns a [`ResourceGroup`](https://docs.microsoft.com/en-us/rest/api/resources/resourcegroups/get#resourcegroup) with `properties`: ``` > az group show -n test { "id": "/subscriptions/0b1f6471-1bf0-4dda-aec3-cb9272f09590/resourceGroups/test", "location": "westus", "managedBy": null, "name": "test", "properties": { "provisioningState": "Succeeded" }, "tags": null, "type": "Microsoft.Resources/resourceGroups" } ``` Meanwhile, [`az storage account show`](https://docs.microsoft.com/en-us/cli/azure/storage/account?view=azure-cli-latest#az_storage_account_show) (internally calls [Storage Accounts - Get Properties](https://docs.microsoft.com/en-us/rest/api/storagerp/storageaccounts/getproperties)) returns a [`StorageAccount`](https://docs.microsoft.com/en-us/rest/api/storagerp/storageaccounts/getproperties#storageaccount) whose `properties` has been flattened by Python SDK: ``` > az storage account show -n test { "accessTier": "Hot", "allowBlobPublicAccess": null, "allowSharedKeyAccess": null, "azureFilesIdentityBasedAuthentication": null, ... "name": "test", "type": "Microsoft.Storage/storageAccounts" } ``` The reason behind the **flattening** is because `ResourceGroup`'s REST spec defines https://github.com/Azure/azure-rest-api-specs/blob/e10fd5f5f0b3444e589cd816321fce7036e33554/specification/resources/resource-manager/Microsoft.Resources/stable/2020-10-01/resources.json#L5549-L5552 while `StorageAccount`'s REST spec defines: https://github.com/Azure/azure-rest-api-specs/blob/e10fd5f5f0b3444e589cd816321fce7036e33554/specification/storage/resource-manager/Microsoft.Storage/stable/2021-01-01/storage.json#L3083-L3087 Azure Python SDK honors `x-ms-client-flatten` and flattens `properties` accordingly. ## Expected solution All REST specs should stop specifying `x-ms-client-flatten` in order to provided consistent responses across all Resource Providers and all SDKs. _Originally posted by @Morriz in https://github.com/Azure/azure-cli/issues/17332_
Author: jiasli
Assignees: -
Labels: `Service Attention`, `Storage`, `needs-triage`
Milestone: -
leni-msft commented 3 years ago

@qiaozha any thoughts?

leni-msft commented 3 years ago

@blueww Could you take a look? is the behavior of showing storage account in CLI intended?

qiaozha commented 3 years ago

x-ms-client-flatten doesn't impact the original rest api response. it's purely a client side feature.

If we do remove x-ms-client-flatten for all the RP. That will cause problems like breaking changes and customer complains like SDK is difficult to use etc.

If CLI wants to get the same structure. it should parse the original response instead of using what python SDK returns. or make use of the _attribute_map key https://github.com/Azure/azure-sdk-for-python/blob/master/sdk/storage/azure-mgmt-storage/azure/mgmt/storage/v2021_01_01/models/_models_py3.py#L4611-L4637

blueww commented 3 years ago

@leni-msft

Agree with @qiaozha , remove the flag will be a breaking change for SDK and tools (like PSH). BTW, why you don't want to show the account properties flattened in CLI?

Morriz commented 3 years ago

If we do remove x-ms-client-flatten for all the RP. That will cause problems like breaking changes

Sure, that can be announced. It is a common issue in evolving software.

and customer complains like SDK is difficult to use etc.

I don't see any increased complexity coming from removing flattening (or adding flattening).

Alignment between cli and rest api will bring a lot of gains afterwards (complete api docs for a change, see https://github.com/Azure/azure-cli/issues/17332). It would be a missed opportunity to not align them imo, and issues will keep coming up (double docs maintenance, confusion, stagnation, M$ brand degradation).

Morriz commented 3 years ago

BTW, why you don't want to show the account properties flattened in CLI?

Doesn't flattening remove the context of these flattened properties?

qiaozha commented 3 years ago

and customer complains like SDK is difficult to use etc.

I don't see any increased complexity coming from removing flattening (or adding flattening).

with flattening customer just need to pass whichever parameter with simple value, without it, everything going to be one big object. Customers have to understand the object structure before they can build the correct parameter. We've already got complains on that.

Alignment between cli and rest api will bring a lot of gains afterwards (complete api docs for a change, see Azure/azure-cli#17332). It would be a missed opportunity to not align them imo, and issues will keep coming up (double docs maintenance, confusion, stagnation, M$ brand degradation).

besides azure cli, we have other SDKs, those SDK are already aligned in the current way. it doesn't make sense to break all the SDKs just because we want azure-cli align with rest api response. The Solution of how azure-cli can be align with rest api response has already been provided here, If CLI wants to get the same structure. it should parse the original response instead of using what python SDK returns. or make use of the _attribute_map key https://github.com/Azure/azure-sdk-for-python/blob/master/sdk/storage/azure-mgmt-storage/azure/mgmt/storage/v2021_01_01/models/_models_py3.py#L4611-L4637

jiasli commented 3 years ago

besides azure cli, we have other SDKs, those SDK are already aligned in the current way. it doesn't make sense to break all the SDKs just because we want azure-cli align with rest api response.

This means all SDKs are not aligned with the underlying REST APIs.

kichalla commented 3 years ago

Nice discussion. Could you please provide what should be the guidance for folks who are using ASP.NET Core + Swashbuckle to generate Swagger docs? Swashbuckle for ASP.NET Core uses Open API SDK to generate the Swagger docs and this SDK does not emit a sibling key-value like "x-ms-client-flatten" here in our case. I can imagine this being a general problem for anybody as many folks use ASP.NET Core + Swashbuckle combination.

Any guidance or docs on how to make "x-ms-client-flatten" still be generated in order to avoid breaking clients' experience as you have suggested? Are we supposed to use autorest "directives"?

cc @qiaozha @blueww @leni-msft

Related issues: