[Storage] Fix #22679: az storage account file-service-properties update: Fix AttributeError: 'NoneType' object has no attribute 'smb'

[evelyn-ys]

Related command

az storage account file-service-properties update

Description

Fix #22679 , IcM 311511171
update_file_service_properties can't handle properly when instance.protocol_settings is None.

Previously get_file_service_properties would get response "protocolSettings":{"smb":{}} from server.
But recently server won't return "protocalSettings" property any more.

Then CLI would crash with AttributeError: 'NoneType' object has no attribute 'smb'

---

This checklist is used to make sure that common guidelines for a pull request are followed.

• The PR title and description has followed the guideline in Submitting Pull Requests.

• I adhere to the Command Guidelines.

• I adhere to the Error Handling Guidelines.

[yonzhan]

Storage

[jiasli]

I don't think this PR is a good idea.

If we implement this check, we should implement checks for all object properties, like this protocolSettings or shareDeleteRetentionPolicy, just in case they are suddenly missing from the API's response.

> az storage account file-service-properties show -n clitest23zhehg2ug7pzcmmt
{
  "cors": {
    "corsRules": []
  },
  "id": "/subscriptions/0b1f6471-1bf0-4dda-aec3-cb9272f09590/resourceGroups/clitest47xrljy3nijo3qd5vzsdilpqy5gmhc6vhrxdt4iznh6uaopskftgp4scam2w7drpot4l/providers/Microsoft.Storage/storageAccounts/clitest23zhehg2ug7pzcmmt/fileServices/default",
  "name": "default",
  "protocolSettings": null,
  "resourceGroup": "clitest47xrljy3nijo3qd5vzsdilpqy5gmhc6vhrxdt4iznh6uaopskftgp4scam2w7drpot4l",
  "shareDeleteRetentionPolicy": {
    "allowPermanentDelete": null,
    "days": 0,
    "enabled": false
  },
  "sku": {
    "name": "Standard_LRS",
    "tier": "Standard"
  },
  "type": "Microsoft.Storage/storageAccounts/fileServices"
}

Also, after API fixes this, the check for protocolSettings will be rendered useless.

[jiasli]

This PR reminds me of #17526 where managedByTenants suddenly became missing.

[evelyn-ys]

If we implement this check, we should implement checks for all object properties, like this protocolSettings or shareDeleteRetentionPolicy, just in case they are suddenly missing from the API's response.

If we want to update sub property of shareDeleteRetentionPolicy, then we should check if it's None. And this is what we already do:

azure-cli/src/azure-cli/azure/cli/command_modules/storage/operations/account.py

Lines 737 to 738 in 05c00f9

	if instance.share_delete_retention_policy is not None and instance.share_delete_retention_policy.enabled:
	instance.share_delete_retention_policy.days = delete_retention_days

Also, after API fixes this, the check for protocolSettings will be rendered useless.

Service won't "fix" API. They think return None is the fix. Here're words from service team:

The code change is a right behavior from SRP perspective. The call needs to exactly send the response which data plane sends and that's what the fix does

We should do essential checks to make sure our product doesn't break frequently. That's quality requirements for our own product. We can't rely on the quality of external input.

[jiasli]

We should do essential checks to make sure our product doesn't break frequently. That's quality requirements for our own product. We can't rely on the quality of external input.

Then we need to be prepared that any object property can be None all of a sudden. This is not external input, but Azure's own API. Keeping a consistent interface is both beneficial to us and external customers.

I approved the PR, but I keep my personal opinion.

[evelyn-ys]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 2 pipeline(s).

[evelyn-ys]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 2 pipeline(s).

[wangzelin007]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 2 pipeline(s).

[wangzelin007]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 2 pipeline(s).