ExcludeSystemUser correct logic in Get-DbaDbUser

pollusb commented 4 days ago

Type of Change

[x] Bug fix (non-breaking change, fixes # )
[ ] New feature (non-breaking change, adds functionality, fixes # )
[ ] Breaking change (affects multiple commands or functionality, fixes # )
[ ] Ran manual Pester test and has passed (.\tests\manual.pester.ps1)
[ ] Adding code coverage to existing functionality
[ ] Pester test is included
[ ] If new file reference added for test, has is been added to github.com/dataplat/appveyor-lab ?
[ ] Unit test is included
[ ] Documentation
[ ] Build system

Purpose

Correct a logical error in Get-DbaDbUser

Approach

Commands to test

# Actual logic:
PS> (Get-DbaDbUser @splat).Count
15
PS> (Get-DbaDbUser @splat -ExcludeSystemUser).Count 
11
PS> (Get-DbaDbUser @splat -ExcludeSystemUser:$false).Count 
11 # logical error here
PS> (Get-DbaDbUser @splat -ExcludeSystemUser:$true).Count 
11

# Revised logic:
PS> (Get-DbaDbUser @splat).Count
15
PS> (Get-DbaDbUser @splat -ExcludeSystemUser).Count 
11
PS> (Get-DbaDbUser @splat -ExcludeSystemUser:$false).Count 
15
PS> (Get-DbaDbUser @splat -ExcludeSystemUser:$true).Count 
11

Screenshots

Learning

niphlod commented 4 days ago

LGTM, @andreasjordan ?

pollusb commented 3 days ago

Good catch. Maybe we have the same problem in other commands as well.

We do. Testing the presence of a switch parameter does not provide the right logic. Here's a list of a similar pattern:

PS C:\Users\me\Code\Repo\dbatools.fork.git\public> dir *.ps1|sls 'Test-Bound -ParameterName Exclude'

Get-DbaDbCheckConstraint.ps1:105:                    if ( (Test-Bound -ParameterName ExcludeSystemTable) -and
$tbl.IsSystemObject ) {
Get-DbaDbForeignKey.ps1:105:                    if ( (Test-Bound -ParameterName ExcludeSystemTable) -and
$tbl.IsSystemObject ) {
Get-DbaDbServiceBrokerQueue.ps1:92:                if ( (Test-Bound -ParameterName ExcludeSystemQueue) -and
$queue.IsSystemObject ) {
Get-DbaDbServiceBrokerService.ps1:92:                if ( (Test-Bound -ParameterName ExcludeSystemService) -and
$service.IsSystemObject ) {
Get-DbaDbStoredProcedure.ps1:185:                if ( (Test-Bound -ParameterName ExcludeSystemSp) -and
$proc.IsSystemObject ) {
Get-DbaDbUdf.ps1:122:                if (Test-Bound -ParameterName ExcludeSystemUdf) {
Get-DbaDbUser.ps1:124:                if (Test-Bound -ParameterName ExcludeSystemUser) {
Get-DbaDbView.ps1:170:            if (Test-Bound -ParameterName ExcludeSystemView) {
Get-DbaInstanceAudit.ps1:76:            if (Test-Bound -ParameterName ExcludeAudit) {
Set-DbaMaxDop.ps1:153:                    } elseif ((Test-Bound -Not -ParameterName AllDatabases) -and (Test-Bound
-ParameterName ExcludeDatabase)) {

I would probably send PR in the same ways for these. I mean with a testing scenario. Would that be OK?

dataplat / dbatools

ExcludeSystemUser correct logic in Get-DbaDbUser #9415