Skip to content

SqlRole: Get/Test always complete and fix parameter checks #2459

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
dan-hughes wants to merge 2 commits into
base: main
Choose a base branch
from
+24 −24
Open

SqlRole: Get/Test always complete and fix parameter checks #2459

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d920dfc
Fix if statements and prevent resource throwing on get and test
dan-hughes Apr 23, 2026
d98eedc
Update changelog
dan-hughes Apr 23, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
- SqlDatabase
- 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.
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Apr 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

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.

Suggested change
- `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.

- Fix if statements checking the parameters are present. ([issue #1702](https://github.com/dsccommunity/SqlServerDsc/issues/1702))

### Changed

Expand Down
25 changes: 11 additions & 14 deletions source/DSCResources/DSC_SqlRole/DSC_SqlRole.psm1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,15 +50,14 @@ function Get-TargetResource
$ServerRoleName
)

$sqlServerObject = Connect-SQL -ServerName $ServerName -InstanceName $InstanceName -ErrorAction 'Stop'
$sqlServerObject = Connect-SQL -ServerName $ServerName -InstanceName $InstanceName -ErrorAction 'SilentlyContinue'
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Apr 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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.

$ensure = 'Absent'
$membersInRole = $null

if ($sqlServerObject)
{
Write-Verbose -Message (
$script:localizedData.GetProperties `
-f $ServerRoleName
$script:localizedData.GetProperties -f $ServerRoleName
)

if ($sqlServerRoleObject = $sqlServerObject.Roles[$ServerRoleName])
Expand All @@ -70,10 +69,8 @@ function Get-TargetResource
}
catch
{
$errorMessage = $script:localizedData.EnumMemberNamesServerRoleGetError `
-f $ServerName, $InstanceName, $ServerRoleName

New-InvalidOperationException -Message $errorMessage -ErrorRecord $_
Write-Verbose -Message ($script:localizedData.EnumMemberNamesServerRoleGetError `
-f $ServerName, $InstanceName, $ServerRoleName)
}
}
}
Expand Down Expand Up @@ -237,7 +234,7 @@ function Set-TargetResource

$correctedParameters = Get-CorrectedMemberParameters @originalParameters

if ($Members)
if ($PSBoundParameters.ContainsKey('Members'))
{
$memberNamesInRoleObject = $sqlServerObject.Roles[$ServerRoleName].EnumMemberNames()

Expand All @@ -263,7 +260,7 @@ function Set-TargetResource
}
else
{
if ($MembersToInclude)
if ($PSBoundParameters.ContainsKey('MembersToInclude'))
{
$memberNamesInRoleObject = $sqlServerObject.Roles[$ServerRoleName].EnumMemberNames()

Expand All @@ -278,7 +275,7 @@ function Set-TargetResource
}
}

if ($MembersToExclude)
if ($PSBoundParameters.ContainsKey('MembersToExclude'))
{
$memberNamesInRoleObject = $sqlServerObject.Roles[$ServerRoleName].EnumMemberNames()

Expand Down Expand Up @@ -428,9 +425,9 @@ function Test-TargetResource
}
else
{
if ($Members)
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))
Comment on lines +428 to +430
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Apr 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 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 -50

Repository: 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.psm1

Repository: dsccommunity/SqlServerDsc

Length of output: 99

🏁 Script executed:

sed -n '31,150p' source/DSCResources/DSC_SqlRole/DSC_SqlRole.psm1

Repository: dsccommunity/SqlServerDsc

Length of output: 3181

🏁 Script executed:

grep -n "function Test-TargetResource" source/DSCResources/DSC_SqlRole/DSC_SqlRole.psm1

Repository: dsccommunity/SqlServerDsc

Length of output: 101

🏁 Script executed:

sed -n '325,410p' source/DSCResources/DSC_SqlRole/DSC_SqlRole.psm1

Repository: dsccommunity/SqlServerDsc

Length of output: 2396

🏁 Script executed:

grep -n "function Get-CorrectedMemberParameters" source/DSCResources/DSC_SqlRole/DSC_SqlRole.psm1

Repository: dsccommunity/SqlServerDsc

Length of output: 111

🏁 Script executed:

sed -n '650,750p' source/DSCResources/DSC_SqlRole/DSC_SqlRole.psm1

Repository: 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.psm1

Repository: dsccommunity/SqlServerDsc

Length of output: 442

🏁 Script executed:

# Check if there are unit tests for this scenario
find tests -name "*SqlRole*" -type f

Repository: 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 -100

Repository: dsccommunity/SqlServerDsc

Length of output: 51

🏁 Script executed:

# Check for test cases involving Members parameter
rg "Members" tests/Unit/DSC_SqlRole.Tests.ps1 -c

Repository: dsccommunity/SqlServerDsc

Length of output: 72

🏁 Script executed:

grep -n "Members" tests/Unit/DSC_SqlRole.Tests.ps1 | head -30

Repository: 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).

{
Write-Verbose -Message (
$script:localizedData.DesiredMembersNotPresent `
Expand All @@ -442,7 +439,7 @@ function Test-TargetResource
}
else
{
if ($MembersToInclude)
if ($PSBoundParameters.ContainsKey('MembersToInclude'))
{
foreach ($memberToInclude in $correctedParameters.MembersToInclude)
{
Expand All @@ -458,7 +455,7 @@ function Test-TargetResource
}
}

if ($MembersToExclude)
if ($PSBoundParameters.ContainsKey('MembersToExclude'))
{
foreach ($memberToExclude in $correctedParameters.MembersToExclude)
{
Expand Down
20 changes: 10 additions & 10 deletions tests/Unit/DSC_SqlRole.Tests.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -311,24 +311,24 @@ Describe "DSC_SqlRole\Get-TargetResource" -Tag 'Get' {
$mockInvalidOperationForEnumMethod = $true
}

It 'Should throw the correct error' {
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
Comment on lines +314 to +328
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Apr 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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.

}
}

It 'Should call the mock function Connect-SQL' {
Should -Invoke -CommandName Connect-SQL -Exactly -Times 1 -Scope Context
Should -Invoke -CommandName Connect-SQL -Exactly -Times 1 -Scope It
}
}
}
Expand Down
Loading