Improve validation for EnabledSoftDelete and SoftDeleteRetentionDay params #28619
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
This PR improves parameter validation for the EnableSoftDelete and SoftDeleteRetentionDays parameters in Azure SQL Server cmdlets. The changes enhance parameter consistency, validation logic, and error handling for soft delete functionality.
Key Changes
- Enhanced validation logic for soft delete parameters with explicit null-value handling
- Improved error messages and added range validation for retention days (1-35)
- Updated parameter types and test cleanup procedures for better consistency
Reviewed Changes
Copilot reviewed 6 out of 11 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
SetAzureSqlServer.cs |
Changed EnableSoftDelete to nullable bool and enhanced validation logic with explicit three-way branching |
NewAzureSqlServer.cs |
Applied same parameter type change and validation improvements as SetAzureSqlServer |
RestoreAzureSqlServer.cs |
Fixed error message to reference Location instead of ResourceGroupName for deleted server searches |
Resources.resx |
Added new error messages for range validation and zero retention scenarios |
Set-AzSqlServer.md |
Improved documentation clarity for soft delete retention behavior |
ServerCrudTests.ps1 |
Updated test cleanup to use explicit EnableSoftDelete $False instead of setting retention days to 0 |
Files not reviewed (1)
- src/Sql/Sql/Properties/Resources.Designer.cs: Language not supported
rambabu-yalla
force-pushed
the
dev/ramyal/fix_server-restore
branch
from
e526311 to
ee1b8a8
Compare
rambabu-yalla
force-pushed
the
dev/ramyal/fix_server-restore
branch
from
ee1b8a8 to
2514bc6
Compare
rambabu-yalla
force-pushed
the
dev/ramyal/fix_server-restore
branch
from
2514bc6 to
c041d24
Compare
| updateData[0].KeyId = this.KeyId ?? updateData[0].KeyId; | ||
| updateData[0].FederatedClientId = this.FederatedClientId ?? updateData[0].FederatedClientId; | ||
| if (this.EnableSoftDelete) | ||
| if (this.EnableSoftDelete == true) |
There was a problem hiding this comment.
Use direct boolean comparison instead of explicit comparison to true. This can be simplified to if (this.EnableSoftDelete == true) or better yet if (EnableSoftDelete.GetValueOrDefault()).
| if (this.EnableSoftDelete == true) | |
| if (this.EnableSoftDelete.GetValueOrDefault()) |
Copilot uses AI. Check for mistakes.
| // If enabling soft-delete retention, use the explicitly provided value or default to 7 days if none provided. | ||
| updateData[0].SoftDeleteRetentionDays = this.SoftDeleteRetentionDays ?? 7; | ||
| } | ||
| else if (this.EnableSoftDelete == false) |
There was a problem hiding this comment.
Use direct boolean comparison instead of explicit comparison to false. This can be simplified to if (EnableSoftDelete.GetValueOrDefault(true) == false) or use a more explicit nullable check pattern.
| else if (this.EnableSoftDelete == false) | |
| else if (this.EnableSoftDelete.GetValueOrDefault(true) == false) |
Copilot uses AI. Check for mistakes.
| if (this.EnableSoftDelete == true) | ||
| { | ||
| // If enabling soft-delete retention, use the explicitly provided value or default to 7 days if none provided. | ||
| softDeleteRetentionDays = this.SoftDeleteRetentionDays ?? 7; | ||
| } | ||
| else if (this.EnableSoftDelete == false) |
There was a problem hiding this comment.
Same as in SetAzureSqlServer.cs - use direct boolean comparison instead of explicit comparison to true/false for nullable boolean values.
| if (this.EnableSoftDelete == true) | |
| { | |
| // If enabling soft-delete retention, use the explicitly provided value or default to 7 days if none provided. | |
| softDeleteRetentionDays = this.SoftDeleteRetentionDays ?? 7; | |
| } | |
| else if (this.EnableSoftDelete == false) | |
| if (this.EnableSoftDelete) | |
| { | |
| // If enabling soft-delete retention, use the explicitly provided value or default to 7 days if none provided. | |
| softDeleteRetentionDays = this.SoftDeleteRetentionDays ?? 7; | |
| } | |
| else if (!this.EnableSoftDelete) |
Copilot uses AI. Check for mistakes.
| // If enabling soft-delete retention, use the explicitly provided value or default to 7 days if none provided. | ||
| softDeleteRetentionDays = this.SoftDeleteRetentionDays ?? 7; | ||
| } | ||
| else if (this.EnableSoftDelete == false) |
There was a problem hiding this comment.
Same as in SetAzureSqlServer.cs - use direct boolean comparison instead of explicit comparison to true/false for nullable boolean values.
| else if (this.EnableSoftDelete == false) | |
| else if (this.EnableSoftDelete is false) |
Copilot uses AI. Check for mistakes.
|
/azp run azure-powershell - security-tools |
|
Azure Pipelines successfully started running 1 pipeline(s). |
… params
Description
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.