[Az.RecoveryServices.Backup] Fix AFS Restore Command Bug #28302
Conversation
| Thanks for your contribution! The pull request validation has started. Please revisit this comment for updated status. |
There was a problem hiding this comment.
Pull Request Overview
Fixes a bug in the Azure File Share restore functionality where the restore command fails when the source storage account has been deleted, by implementing fallback logic to retrieve the sourceResourceId from the protected item when the primary method fails.
- Adds exception handling around storage account resource retrieval with fallback to protected item
- Implements validation to ensure sourceResourceId is available before proceeding with restore
- Updates changelog to document the fix
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| ChangeLog.md | Documents the bug fix for Azure File Share restore command |
| AzureFilesPsBackupProvider.cs | Implements fallback logic to handle deleted source storage accounts during restore operations |
| // Validate that we have a source resource ID | ||
| if (storageAccountResource == null || string.IsNullOrEmpty(storageAccountResource.Id)) | ||
| { | ||
| throw new ArgumentException(string.Format(Resources.AzureFileShareNotFound, |
There was a problem hiding this comment.
The error message uses 'AzureFileShareNotFound' resource which may not accurately describe the actual problem - missing source resource ID rather than a missing file share. Consider using a more specific error message that indicates the source storage account resource ID could not be retrieved.
Copilot uses AI. Check for mistakes.
| // Create a minimal resource object with the source resource ID | ||
| if (!string.IsNullOrEmpty(sourceResourceId)) | ||
| { | ||
| storageAccountResource = new GenericResource |
There was a problem hiding this comment.
Creating a new GenericResource with minimal properties could lead to issues if other parts of the code expect additional properties to be populated. Consider documenting this limitation or ensuring all required properties are set.
Copilot uses AI. Check for mistakes.
| GenericResource storageAccountResource = ServiceClientAdapter.GetStorageAccountResource(recoveryPoint.ContainerName.Split(';')[2]); | ||
| GenericResource storageAccountResource = null; | ||
| string sourceResourceId = null; | ||
| string storageAccountLocation = vaultLocation; // Default to vault location |
There was a problem hiding this comment.
The assumption that the storage account location should default to vault location may not always be correct. Consider adding a comment explaining why this is a reasonable fallback or implement logic to determine the actual storage account location.
| string storageAccountLocation = vaultLocation; // Default to vault location | |
| // Default to vault location as a fallback. This assumption is based on the expectation | |
| // that in most cases, the storage account and the vault are in the same location. | |
| // However, this may not always be true. If the storage account location cannot be retrieved, | |
| // we log a warning and proceed with the fallback. Ensure that this fallback is safe for your scenario. | |
| string storageAccountLocation = vaultLocation; |
Copilot uses AI. Check for mistakes.
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
| throw new ArgumentException(string.Format(Resources.AzureFileShareNotFound, | ||
| recoveryPoint.ContainerName)); | ||
| } | ||
|
|
There was a problem hiding this comment.
can we add a test case for this. also have we thoroughly tested this change?
| { | ||
| storageAccountResource = new GenericResource | ||
| { | ||
| Id = sourceResourceId, |
There was a problem hiding this comment.
initiliazing this way might not work. please confirm
|
Hi @IannGeorges please also sync main branch in next commit to keep the changelog updated. |
Description
Implementing fix to handle cases where the source storage account is deleted & required sourceResourceId property is missing for restore request payload.
Tested restore where AFS source storage account was deleted before changes were made to reproduce bug.
Tested restore where AFS source storage account was deleted after changes were made to validate fix.
Mandatory Checklist
Please choose the target release of Azure PowerShell. (⚠️ Target release is a different concept from API readiness. Please click below links for details.)
Check this box to confirm: I have read the Submitting Changes section of
CONTRIBUTING.mdand reviewed the following information:ChangeLog.mdfile(s) appropriatelysrc/{{SERVICE}}/{{SERVICE}}/ChangeLog.md.## Upcoming Releaseheader in the past tense.ChangeLog.mdif no new release is required, such as fixing test case only.