SqlRole: Get/Test always complete and fix parameter checks#2459
SqlRole: Get/Test always complete and fix parameter checks#2459dan-hughes wants to merge 2 commits intodsccommunity:mainfrom
Conversation
Co-authored-by: Copilot <copilot@github.com>
WalkthroughThe PR updates the SqlRole DSC resource to properly distinguish between unspecified parameters and empty parameters using Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
source/DSCResources/DSC_SqlRole/DSC_SqlRole.psm1 (2)
63-74:⚠️ Potential issue | 🟠 MajorKeep
Ensure = 'Present'when only member enumeration fails.Line 63 already proves the server role exists, but
$ensureis only set afterEnumMemberNames()succeeds. If enumeration throws, the role is reported asAbsent, which can makeTest-TargetResource -Ensure Absentpass incorrectly.🐛 Proposed fix
if ($sqlServerRoleObject = $sqlServerObject.Roles[$ServerRoleName]) { + $ensure = 'Present' + try { [System.String[]] $membersInRole = $sqlServerRoleObject.EnumMemberNames() - $ensure = 'Present' } catch { Write-Verbose -Message ($script:localizedData.EnumMemberNamesServerRoleGetError `🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/DSCResources/DSC_SqlRole/DSC_SqlRole.psm1` around lines 63 - 74, The role-exists check sets $sqlServerRoleObject but $ensure is only assigned 'Present' after EnumMemberNames() succeeds, so a member-enumeration failure causes the role to be treated as Absent; move or add the assignment so $ensure = 'Present' is set as soon as $sqlServerRoleObject is truthy (e.g., immediately after the if ($sqlServerRoleObject = $sqlServerObject.Roles[$ServerRoleName]) line or ensure the catch does not unset it), keeping the EnumMemberNames() call inside the try/catch and still logging errors from EnumMemberNames() without changing the role presence state.
228-237:⚠️ Potential issue | 🔴 CriticalPreserve
SAprotection whenMembersis explicitly empty forsysadmin.The new
ContainsKey('Members')path makesMembers = @()authoritative, butGet-CorrectedMemberParametersonly addsSAwhen$Membersis truthy. ForServerRoleName = 'sysadmin'andMembers = @(), Set can attempt to remove every current member, includingSA, despite the helper’s stated guard.Please pass parameter presence into the correction logic, or normalize
sysadminafter correction so explicit emptyMembersstill includesSA.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/DSCResources/DSC_SqlRole/DSC_SqlRole.psm1` around lines 228 - 237, The patch makes an explicit empty Members (@()) authoritative but Get-CorrectedMemberParameters only adds SA when $Members is truthy, risking removal of SA for ServerRoleName = 'sysadmin'; update the call/logic so the helper knows that Members was explicitly passed (e.g., include a flag in $originalParameters like HasMembers = $PSBoundParameters.ContainsKey('Members') or pass PSBoundParameters membership), or after receiving $correctedParameters normalize for sysadmin by ensuring 'SA' is present when the original call included Members (even if empty) to prevent removal; reference Get-CorrectedMemberParameters, $originalParameters, $correctedParameters, ServerRoleName, Members and the 'sysadmin'/'SA' case when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CHANGELOG.md`:
- Line 21: Update the first SqlRole bullet on Line 21 in CHANGELOG.md to include
the issue reference using the required format; locate the bullet text
"`Get-TargetResource` and `Test-TargetResource` always return successfully." and
append or replace with a link in the form [issue
#<issue_number>](https://github.com/<owner>/<repo>/issues/<issue_number>) so it
mirrors the linked entry on the subsequent SqlRole line.
In `@source/DSCResources/DSC_SqlRole/DSC_SqlRole.psm1`:
- Line 53: Connect-SQL is currently called with -ErrorAction 'SilentlyContinue'
so connection failures produce no $sqlServerObject and make
Get-TargetResource/Test-TargetResource think the role is absent; replace that
silent failure with a controlled try/catch around the Connect-SQL call (use
Connect-SQL -ServerName $ServerName -InstanceName $InstanceName without
SilentlyContinue), capture any exception into a $connectionFailed or
$connectionError variable and log the error, and then update
Get-TargetResource/Test-TargetResource to treat $connectionFailed as an
unknown/unverified state and return non-compliant (e.g., Test-TargetResource
returns $false) rather than treating the role as 'Absent' or throwing; refer to
Connect-SQL, $sqlServerObject, Get-TargetResource, and Test-TargetResource to
locate and implement these changes.
- Around line 428-430: Normalize null/empty member lists to empty arrays before
calling Compare-Object: when PSBoundParameters contains 'Members', wrap both
$getTargetResourceResult.Members and $correctedParameters.Members with an array
coercion (e.g., use @(...)) so neither side is $null; then call Compare-Object
on those normalized arrays (reference the symbols
$getTargetResourceResult.Members, correctedParameters.Members, Compare-Object
and Get-CorrectedMemberParameters).
In `@tests/Unit/DSC_SqlRole.Tests.ps1`:
- Around line 314-328: The test is asserting the role is Absent even though
Get-TargetResource simulates the role exists but EnumMemberNames() failed;
update the expectation so Ensure remains 'Present' to reflect that the resource
exists despite enumeration errors—modify the assertion in the
DSC_SqlRole.Tests.ps1 block testing Get-TargetResource to assert $result.Ensure
-Be 'Present' (references: Get-TargetResource, EnumMemberNames, Ensure) and keep
the other member-related assertions as-is.
---
Outside diff comments:
In `@source/DSCResources/DSC_SqlRole/DSC_SqlRole.psm1`:
- Around line 63-74: The role-exists check sets $sqlServerRoleObject but $ensure
is only assigned 'Present' after EnumMemberNames() succeeds, so a
member-enumeration failure causes the role to be treated as Absent; move or add
the assignment so $ensure = 'Present' is set as soon as $sqlServerRoleObject is
truthy (e.g., immediately after the if ($sqlServerRoleObject =
$sqlServerObject.Roles[$ServerRoleName]) line or ensure the catch does not unset
it), keeping the EnumMemberNames() call inside the try/catch and still logging
errors from EnumMemberNames() without changing the role presence state.
- Around line 228-237: The patch makes an explicit empty Members (@())
authoritative but Get-CorrectedMemberParameters only adds SA when $Members is
truthy, risking removal of SA for ServerRoleName = 'sysadmin'; update the
call/logic so the helper knows that Members was explicitly passed (e.g., include
a flag in $originalParameters like HasMembers =
$PSBoundParameters.ContainsKey('Members') or pass PSBoundParameters membership),
or after receiving $correctedParameters normalize for sysadmin by ensuring 'SA'
is present when the original call included Members (even if empty) to prevent
removal; reference Get-CorrectedMemberParameters, $originalParameters,
$correctedParameters, ServerRoleName, Members and the 'sysadmin'/'SA' case when
making the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 73077004-f5bf-427a-a822-7112c6b07776
📒 Files selected for processing (3)
CHANGELOG.mdsource/DSCResources/DSC_SqlRole/DSC_SqlRole.psm1tests/Unit/DSC_SqlRole.Tests.ps1
| - Fixed logic in `Set-SqlDscDatabaseProperty` so pipeline input is properly | ||
| handled in the cmdlet's `Process` block. ([issue #2456](https://github.com/dsccommunity/SqlServerDsc/issues/2456)) | ||
| - SqlRole | ||
| - `Get-TargetResource` and `Test-TargetResource` always return successfully. |
There was a problem hiding this comment.
Add an issue reference to the first SqlRole bullet.
Line 21 is missing an issue link while the related SqlRole fix entry on Line 22 includes one.
Suggested fix
- SqlRole
- - `Get-TargetResource` and `Test-TargetResource` always return successfully.
+ - `Get-TargetResource` and `Test-TargetResource` always return successfully.
+ ([issue `#1702`](https://github.com/dsccommunity/SqlServerDsc/issues/1702))
- Fix if statements checking the parameters are present. ([issue `#1702`](https://github.com/dsccommunity/SqlServerDsc/issues/1702))As per coding guidelines, "Reference issues using format issue #<issue_number> in CHANGELOG.md".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - `Get-TargetResource` and `Test-TargetResource` always return successfully. | |
| - `Get-TargetResource` and `Test-TargetResource` always return successfully. | |
| ([issue `#1702`](https://github.com/dsccommunity/SqlServerDsc/issues/1702)) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@CHANGELOG.md` at line 21, Update the first SqlRole bullet on Line 21 in
CHANGELOG.md to include the issue reference using the required format; locate
the bullet text "`Get-TargetResource` and `Test-TargetResource` always return
successfully." and append or replace with a link in the form [issue
#<issue_number>](https://github.com/<owner>/<repo>/issues/<issue_number>) so it
mirrors the linked entry on the subsequent SqlRole line.
| ) | ||
|
|
||
| $sqlServerObject = Connect-SQL -ServerName $ServerName -InstanceName $InstanceName -ErrorAction 'Stop' | ||
| $sqlServerObject = Connect-SQL -ServerName $ServerName -InstanceName $InstanceName -ErrorAction 'SilentlyContinue' |
There was a problem hiding this comment.
Do not let connection failures look like an absent role.
With -ErrorAction 'SilentlyContinue', a failed Connect-SQL returns no server object and Get-TargetResource falls through to Ensure = 'Absent'. Then Test-TargetResource -Ensure Absent can incorrectly return $true when the SQL instance was unreachable, not when the role was verified absent.
Please preserve a failure signal and have Test-TargetResource return non-compliant for unknown state while still avoiding a thrown exception.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@source/DSCResources/DSC_SqlRole/DSC_SqlRole.psm1` at line 53, Connect-SQL is
currently called with -ErrorAction 'SilentlyContinue' so connection failures
produce no $sqlServerObject and make Get-TargetResource/Test-TargetResource
think the role is absent; replace that silent failure with a controlled
try/catch around the Connect-SQL call (use Connect-SQL -ServerName $ServerName
-InstanceName $InstanceName without SilentlyContinue), capture any exception
into a $connectionFailed or $connectionError variable and log the error, and
then update Get-TargetResource/Test-TargetResource to treat $connectionFailed as
an unknown/unverified state and return non-compliant (e.g., Test-TargetResource
returns $false) rather than treating the role as 'Absent' or throwing; refer to
Connect-SQL, $sqlServerObject, Get-TargetResource, and Test-TargetResource to
locate and implement these changes.
| if ($PSBoundParameters.ContainsKey('Members')) | ||
| { | ||
| if ( $null -ne (Compare-Object -ReferenceObject $getTargetResourceResult.Members -DifferenceObject $correctedParameters.Members)) | ||
| if ($null -ne (Compare-Object -ReferenceObject $getTargetResourceResult.Members -DifferenceObject $correctedParameters.Members)) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify PowerShell Compare-Object behavior with empty/null desired member lists.
# Expected: These commands demonstrate why the resource should normalize/guard empty lists.
pwsh -NoProfile -Command "Compare-Object -ReferenceObject @('CONTOSO\User') -DifferenceObject @()"
pwsh -NoProfile -Command "Compare-Object -ReferenceObject @('CONTOSO\User') -DifferenceObject \$null"Repository: dsccommunity/SqlServerDsc
Length of output: 246
🏁 Script executed:
cat -n source/DSCResources/DSC_SqlRole/DSC_SqlRole.psm1 | head -450 | tail -50Repository: dsccommunity/SqlServerDsc
Length of output: 2282
🏁 Script executed:
# Find Get-TargetResource function and how Members is returned
grep -n "function Get-TargetResource" source/DSCResources/DSC_SqlRole/DSC_SqlRole.psm1Repository: dsccommunity/SqlServerDsc
Length of output: 99
🏁 Script executed:
sed -n '31,150p' source/DSCResources/DSC_SqlRole/DSC_SqlRole.psm1Repository: dsccommunity/SqlServerDsc
Length of output: 3181
🏁 Script executed:
grep -n "function Test-TargetResource" source/DSCResources/DSC_SqlRole/DSC_SqlRole.psm1Repository: dsccommunity/SqlServerDsc
Length of output: 101
🏁 Script executed:
sed -n '325,410p' source/DSCResources/DSC_SqlRole/DSC_SqlRole.psm1Repository: dsccommunity/SqlServerDsc
Length of output: 2396
🏁 Script executed:
grep -n "function Get-CorrectedMemberParameters" source/DSCResources/DSC_SqlRole/DSC_SqlRole.psm1Repository: dsccommunity/SqlServerDsc
Length of output: 111
🏁 Script executed:
sed -n '650,750p' source/DSCResources/DSC_SqlRole/DSC_SqlRole.psm1Repository: dsccommunity/SqlServerDsc
Length of output: 1029
🏁 Script executed:
# Verify the behavior of type casting $null to [System.String[]]
pwsh -NoProfile -Command '[System.String[]]$null | ForEach-Object { "Value: $_"; "Length: $($_.Length)" } ; if ($null -eq ([System.String[]]$null)) { "Result is null" } else { "Result is not null" }'Repository: dsccommunity/SqlServerDsc
Length of output: 101
🏁 Script executed:
# Verify Compare-Object with $null values specifically
pwsh -NoProfile -Command "
try {
Compare-Object -ReferenceObject @('CONTOSO\User') -DifferenceObject \$null
} catch {
Write-Host 'Error caught: ' + \$_.Exception.Message
}
"Repository: dsccommunity/SqlServerDsc
Length of output: 158
🏁 Script executed:
# Verify the actual scenario: what happens at line 430 when either side is $null
pwsh -NoProfile -Command "
# Simulate the scenario
\$getTargetResourceResult = @{ Members = \$null }
\$correctedParameters = @{ Members = \$null }
# Try the current code path
try {
\$result = \$null -ne (Compare-Object -ReferenceObject \$getTargetResourceResult.Members -DifferenceObject \$correctedParameters.Members)
Write-Host \"Result: \$result\"
} catch {
Write-Host \"Error: \$(\$_.Exception.Message)\"
}
# Now test with empty arrays instead
\$getTargetResourceResult.Members = @()
\$correctedParameters.Members = @()
\$result = \$null -ne (Compare-Object -ReferenceObject \$getTargetResourceResult.Members -DifferenceObject \$correctedParameters.Members)
Write-Host \"With empty arrays, result: \$result\"
"Repository: dsccommunity/SqlServerDsc
Length of output: 180
🏁 Script executed:
# Verify what happens with actual sysadmin role with no members
pwsh -NoProfile -Command "
# Simulating the actual code flow for sysadmin role with no members
\$Members = \$null
\$MembersToInclude = \$null
\$MembersToExclude = \$null
\$ServerRoleName = 'sysadmin'
# This simulates Get-CorrectedMemberParameters
\$correctedParameters = @{
Members = [System.String[]](\$Members)
MembersToInclude = [System.String[]](\$MembersToInclude)
MembersToExclude = [System.String[]](\$MembersToExclude)
}
Write-Host \"Members type: \$(\$correctedParameters.Members.GetType())\"
Write-Host \"Members is null: \$(\$null -eq \$correctedParameters.Members)\"
Write-Host \"Members value: '\$(\$correctedParameters.Members)'\"
"Repository: dsccommunity/SqlServerDsc
Length of output: 379
🏁 Script executed:
# Check if there are tests for this scenario and if the issue manifests
rg -A 5 "Compare-Object" source/DSCResources/DSC_SqlRole/DSC_SqlRole.psm1Repository: dsccommunity/SqlServerDsc
Length of output: 442
🏁 Script executed:
# Check if there are unit tests for this scenario
find tests -name "*SqlRole*" -type fRepository: dsccommunity/SqlServerDsc
Length of output: 214
🏁 Script executed:
# Search for test cases with Members parameter
rg -B 2 -A 5 "Members.*\`$null\|Members.*@\(\)" tests/Unit/DSC_SqlRole.Tests.ps1 | head -100Repository: dsccommunity/SqlServerDsc
Length of output: 51
🏁 Script executed:
# Check for test cases involving Members parameter
rg "Members" tests/Unit/DSC_SqlRole.Tests.ps1 -cRepository: dsccommunity/SqlServerDsc
Length of output: 72
🏁 Script executed:
grep -n "Members" tests/Unit/DSC_SqlRole.Tests.ps1 | head -30Repository: dsccommunity/SqlServerDsc
Length of output: 2821
Normalize empty/null member lists before Compare-Object.
When comparing members, $getTargetResourceResult.Members can be $null (when a role has no members), and Get-CorrectedMemberParameters returns Members = [System.String[]]$null which remains $null. This causes Compare-Object at line 430 to fail with a terminating error: "Cannot bind argument to parameter 'DifferenceObject' because it is null." Normalize both sides to empty arrays before the comparison.
Proposed fix
if ($PSBoundParameters.ContainsKey('Members'))
{
- if ($null -ne (Compare-Object -ReferenceObject $getTargetResourceResult.Members -DifferenceObject $correctedParameters.Members))
+ $currentMembers = if ($null -eq $getTargetResourceResult.Members)
+ {
+ @()
+ }
+ else
+ {
+ @($getTargetResourceResult.Members)
+ }
+
+ $desiredMembers = if ($null -eq $correctedParameters.Members)
+ {
+ @()
+ }
+ else
+ {
+ @($correctedParameters.Members)
+ }
+
+ $membersAreDifferent = $currentMembers.Count -ne $desiredMembers.Count
+
+ if (-not $membersAreDifferent -and $currentMembers.Count -gt 0)
+ {
+ $membersAreDifferent = $null -ne (
+ Compare-Object -ReferenceObject $currentMembers -DifferenceObject $desiredMembers
+ )
+ }
+
+ if ($membersAreDifferent)
{
Write-Verbose -Message (
$script:localizedData.DesiredMembersNotPresent `🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@source/DSCResources/DSC_SqlRole/DSC_SqlRole.psm1` around lines 428 - 430,
Normalize null/empty member lists to empty arrays before calling Compare-Object:
when PSBoundParameters contains 'Members', wrap both
$getTargetResourceResult.Members and $correctedParameters.Members with an array
coercion (e.g., use @(...)) so neither side is $null; then call Compare-Object
on those normalized arrays (reference the symbols
$getTargetResourceResult.Members, correctedParameters.Members, Compare-Object
and Get-CorrectedMemberParameters).
| It 'Should not throw an error and return the correct result' { | ||
| InModuleScope -ScriptBlock { | ||
| Set-StrictMode -Version 1.0 | ||
|
|
||
| $mockTestParameters.ServerRoleName = 'AdminSqlForBI' | ||
|
|
||
| $mockErrorMessage = $script:localizedData.EnumMemberNamesServerRoleGetError -f @( | ||
| 'localhost', | ||
| 'MSSQLSERVER', | ||
| 'AdminSqlForBI' | ||
| ) | ||
| $result = Get-TargetResource @mockTestParameters | ||
|
|
||
| { Get-TargetResource @mockTestParameters } | Should -Throw -ExpectedMessage ('*' + $mockErrorMessage + '*') | ||
| $result.ServerName | Should -Be $mockTestParameters.ServerName | ||
| $result.InstanceName | Should -Be $mockTestParameters.InstanceName | ||
| $result.ServerRoleName | Should -Be $mockTestParameters.ServerRoleName | ||
| $result.Ensure | Should -Be 'Absent' | ||
| $result.Members | Should -BeNullOrEmpty | ||
| $result.MembersToInclude | Should -BeNullOrEmpty | ||
| $result.MembersToExclude | Should -BeNullOrEmpty |
There was a problem hiding this comment.
Assert the role remains present when enumeration fails.
This test now encodes the false state from Get-TargetResource: the role exists, but EnumMemberNames() failed. The non-throwing behavior is good, but Ensure should remain 'Present'; otherwise the test allows Test-TargetResource -Ensure Absent to pass against an existing role.
🧪 Proposed test expectation update
- $result.Ensure | Should -Be 'Absent'
+ $result.Ensure | Should -Be 'Present'
$result.Members | Should -BeNullOrEmpty🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/Unit/DSC_SqlRole.Tests.ps1` around lines 314 - 328, The test is
asserting the role is Absent even though Get-TargetResource simulates the role
exists but EnumMemberNames() failed; update the expectation so Ensure remains
'Present' to reflect that the resource exists despite enumeration errors—modify
the assertion in the DSC_SqlRole.Tests.ps1 block testing Get-TargetResource to
assert $result.Ensure -Be 'Present' (references: Get-TargetResource,
EnumMemberNames, Ensure) and keep the other member-related assertions as-is.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2459 +/- ##
=====================================
- Coverage 94% 94% -1%
=====================================
Files 227 227
Lines 10803 10802 -1
=====================================
- Hits 10174 10173 -1
Misses 629 629
🚀 New features to boost your workflow:
|
1 participant
Pull Request (PR) description
Get/Test do not fail/error, allows Azure Machine Config and DSC3.0 compliance.
Check the PSBoundParameters in
ifstatementsThis Pull Request (PR) fixes the following issues
Task list
file CHANGELOG.md. Entry should say what was changed and how that
affects users (if applicable), and reference the issue being resolved
(if applicable).
This change is