Dbatools: Fix PSAvoidUsingEmptyCatchBlock

[joshcorr]

Summary of new feature

There are ~34 incidents of cmdlets breaking the PSAvoidUsingEmptyCatchBlock script analyzer rule, which could be impacting cmdlet use as well as our BOH. This rule indicates that a catch block should not be empty. In the case that it is empty it may suppress an error (which may be needed), and as a result will potentially make diagnosing why a cmdlet is not running difficult.
See PSAvoidUsingEmptyCatchBlock.md for more details

Proposed technical details (if applicable)

Fill Catchblock with Stop-Function or Write-Message unless it seems that the try block doesn't need to be there then it should be removed.

[joshcorr]

So after analyzing these cmdlets a little I found that 19 of them had truly blank catch blocks

ScriptName                          Line    Extent
----------                          ----    ------
Copy-DbaDbAssembly.ps1              107     catch { }
Copy-DbaDbAssembly.ps1              128     catch { }
Copy-DbaLinkedServer.ps1            112     catch { }
Find-DbaInstance.ps1                282     catch { }
Find-DbaInstance.ps1                288     catch { }
Find-DbaInstance.ps1                296     catch { }
Find-DbaInstance.ps1                478     catch { }
Find-DbaInstance.ps1                653     catch {...
Get-DbaCmsRegServer.ps1             178     catch { }
Get-DbaCmsRegServerGroup.ps1        117     catch { }
Get-DbaService.ps1                  154     catch { }
Get-DbaUserPermission.ps1           222     catch {}
Get-DbaUserPermission.ps1           228     catch { }
Get-DbaUserPermission.ps1           253     catch { }
Get-DbaUserPermission.ps1           278     catch { }
Get-DbaWindowsLog.ps1               179     catch { }
Import-DbaCsvToSql.ps1             1271     catch {...
Install-DbaMaintenanceSolution.ps1  375     catch {...
New-DbatoolsSupportPackage.ps1      120     catch { }

While the remaing 15 had some sort of comment stating the catch block did nothing.

ScriptName                            Line Extent
----------                            ---- ------
Copy-DbaAgentProxyAccount.ps1          132 catch {...
Copy-DbaDatabase.ps1                   355 catch {...
Copy-DbaSsisCatalog.ps1                413 catch {...
Copy-DbaSysDbUserObject.ps1            371 catch {...
Copy-DbaSysDbUserObject.ps1            376 catch {...
Disable-DbaForceNetworkEncryption.ps1   87 catch {...
Enable-DbaForceNetworkEncryption.ps1    87 catch {...
Find-DbaInstance.ps1                   307 catch {...
Get-DbaComputerCertificate.ps1          93 catch {...
Get-DbaComputerCertificate.ps1         101 catch {...
Get-DbaForceNetworkEncryption.ps1       89 catch {...
Get-DbaNetworkCertificate.ps1           97 catch {...
Install-DbaWatchUpdate.ps1              65 catch {...
Test-DbaConnection.ps1                 130 catch {...
Write-DbaDataTable.ps1                 451 catch {...

We had some discussion about this in slack, but wanted further clarification. How do we want to handle these linting errors, now and moving forward? Do we want to just ignore them like the PSUseSingularNouns rules? or do we want to write a verbose message if Stop-Function is not being called? or do we want to remove them completely?

I think removing the try/catch blocks would be the most distruptive right now to the cmdlets and the module. Ignoring them would be least distructive. Write-Message is the one I would go with as it would provide more information if we are trying to diagnose an issue in a cmdlet.

cc: @potatoqualitee

[niphlod]

if write-message is NEEDED I think it'd be already in there. If it's empty as a way to "pass on without hiccups" a $null = 1 #here to avoid empty catch can be as good as it can be.

[joshcorr]

Okay, I can proceed with that for now. I still think if we have a catch block we should have a message or a termination.

[niphlod]

not really. Think "to make this happen (behaviourally), I either need to do this or that or that. In the end, I know it'll work, and I don't need to inform the user about what path I took to make that happen, as long as it happens"

try { dumby but fast } catch { }
if (-not($thathappened)) try { less dumby but longer } catch { }
if (-not($thathappened)) try { third time is a charmer } catch { }
if (-not($thathappened)) try { longer but shady } catch { throw "ok i give up" }

it surely can be ported to

try {
    try { dumby but fast } catch {
        try { less dumby but longer } catch {
           try { third time is a charmer } catch {

          }
       }
    }
catch {}
try { longer but shady } catch { throw "ok i give up" }

but sometime scoping is a bitch and some other times SMO is with it's lazy property evaluation (i.e. see the empty catch on Write-DbaDataTable, which is needed for Azure)

Also, we're in the business of not nagging the user if not for very serious issues .... so okay, having an empty catch is generally considered unsafe but surely there are cases where it's still perfectly acceptable ^_^

[wsmelton]

Most places we have catch blocks that are empty, we have them there for a reason....mainly we don't care if that statement/command being tried errors because we catch it in other areas.

[wsmelton]

That is main reason I recall we never cared about this PSSA rule to begin with, because empty catch blocks are used. If they are empty, you need to validate that it should be which deserves an issue per command if it is not blatantly obviously why.

[potatoqualitee]

+1 to the above, some empty catches can't be avoided

[joshcorr]

I understand that having empty catch is often unavoidable and a design choice that has been made (I am willing to comply with that design choice). Thankfully the errors are being caught other places, because the cmdlet design has been thought through.

My concern in this issue is how do we standardize these PSSA rules. If we don't care about this rule then we should put an ignore in place with a justification, or the $null = 1 that was suggested (I only mentioned the Write-Message because of my own experience writing nested try/catches. It was handy to have a friendly verbose message to which catch block). Having some override for the PSSA check on these cmdlets will help us down the road.

I am thinking of new PRs in the future (i.e. post v1) when a new contributor (who maybe doesn't catch their errors somewhere else) tries to submit a PR and before any pester tests run, PSSA runs (maybe with custom rules for dbatools specific formats) and warns them on their syntax.

[niphlod]

yeah, this line of thinking is post 1.0, but I think down the line we'll go for $null = 1 #avoid an empty catch because:

• we do want to avoid empty catch if not "necessary"
• codeanalyzer exception works on the whole file instead of a single block

This way we can enforce good behaviour and still "leave the door open". That being said, this should probably go in the styleguide.