dbatools icon indicating copy to clipboard operation
dbatools copied to clipboard

[RFC] Remove the use of Select-DefaultView

Open wsmelton opened this issue 4 years ago • 13 comments

I know this will be difficult to get acceptance for but here goes...

Summary of new feature

Replace the use of Select-DefaultView (internal function used for controlling the "default" output of commands) with proper (I'd say best practice) use of format and view ps1xml files.

Proposed technical details (if applicable)

In and of itself if you have not authored modules before the use of <module>.format.ps1xml and <module.types.ps1xml are a bit more cumbersome to use than Select-DefaultView. However, because we send the object in each command to another function (the internal one for the view) it adds unnecessary overhead that PowerShell will handle for us more efficiently.

An example of this can be seen with Get-DbaDatabase, with the internal function used on Codespaces environment with SQL Server on Linux it takes 278ms to return the object output:

image

Adjusting to use a the dbatools.format.ps1xml drops it to 247ms:

image

One additional thing we inherit with using a format file is that when multiple instances are queried the output will keep the table view output. We do not inherit this with our internal function.

image

wsmelton avatar Jun 06 '21 02:06 wsmelton

I think this is a good idea and a good way for me to learn new powershell stuff. So if you need someone to code or someone to review changes - just say hello and I join the team. It might be worth looking at the various default properties of the commands in advance, perhaps to change some of them to match others.

andreasjordan avatar Jun 06 '21 10:06 andreasjordan

I'm open to it but we need the ps1xml files to support aliases and I'm not sure if it does 👍🏼

potatoqualitee avatar Jun 23 '21 21:06 potatoqualitee

  • Format.ps1xml controls table view
  • Types.ps1xml controls aliasing properties

Example:

https://github.com/thycotic-ps/thycotic.secretserver/blob/ced9e007c1fbcc1eccd973c270e3c2ae1247ea14/src/Thycotic.SecretServer.Types.ps1xml#L3-L15

Which we already use a Types file as that is what lets you add Script methods as well.

wsmelton avatar Jun 23 '21 21:06 wsmelton

Actually, thinking about it more...

we use a lot of generic pscustomobjects with select-defaultview and that won't be able to be modified without adding a type to the object and that'd probably undo all the time saved.

Another huge problem I see is that sometimes we want to see more from a database object in one command than another. Having a universal type wouldn't be good there unless we can say "if the caller is This command, then use that format".

I think that's one of the reasons I liked Select-DefaultView so much is that it wasn't as strict as what we see in the SqlServer module and it's not as destructive as Select-Object

potatoqualitee avatar Jun 23 '21 23:06 potatoqualitee

we use a lot of generic pscustomobjects with select-defaultview and that won't be able to be modified without adding a type to the object and that'd probably undo all the time saved.

Easy peasy, adding PSTypeName property to the PSCustomObject allows you to reference it in a format or types file (primary use of it). See more.

wsmelton avatar Jun 24 '21 01:06 wsmelton

😱 crazy easy!! how cool. how do we handle when different types need different outputs? Just skip those types?

potatoqualitee avatar Jun 24 '21 08:06 potatoqualitee

If you need a hand full of commands to test this - I would suggest the Remove-DbaDb* commands, as they need a global rewrite to align the output anyway. And for a command that returns an SMO, maybe Get-DbaEndpoint is a good candidate, as it is not so important but good to test with.

andreasjordan avatar Jun 24 '21 09:06 andreasjordan

@wsmelton have you implemented some of this already? Where are we now? Can I help with something?

andreasjordan avatar Jun 11 '23 12:06 andreasjordan

I can see if I can get an initial PR does for one command and figure out how to handle it. This is something we can piece meal one type/class at a time.

Once I get one type completed, we should be able to go off that PR and just open issues for each type we want to format. We don't necessarily have to remove it all outright I think in case there are one-off scenarios that the format file just doesn't behave like we want.

wsmelton avatar Jun 11 '23 16:06 wsmelton

This reminds me @potatoqualitee we should start signing our dbatools.Types.ps1xml file along with all the other things we sign since it does contain script blocks. We do not contain any destructive code but it leaves a whole where it could be added since that is where we define the $server.Query() method used around the module.

wsmelton avatar Jul 02 '23 17:07 wsmelton

VSCode says: "501 results in 268 files". A lot of work...

And have a look at Invoke-DbMirrorValidation:

if ((Test-Bound -ParameterName Witness)) {
    $results | Select-DefaultView -Property Primary, Mirror, Witness, Database, RecoveryModel, MirroringStatus, State, EndPoints, DatabaseExistsOnMirror, OnlineWitness, DatabaseExistsOnWitness, EditionMatch, AccessibleShare, DestinationDbStatus, WitnessDbStatus, ValidationPassed
} else {
    $results | Select-DefaultView -Property Primary, Mirror, Database, RecoveryModel, MirroringStatus, State, EndPoints, DatabaseExistsOnMirror, EditionMatch, AccessibleShare, DestinationDbStatus, ValidationPassed
}

andreasjordan avatar Jul 03 '23 16:07 andreasjordan

Hence the discussion with an RFC issue, because it is a massive change.

wsmelton avatar Jul 03 '23 20:07 wsmelton

So maybe start with those Get- commands that return SMO objects. Like Get-DbaAgentJob.

I could start with a PR for all the Get-DbaAgent* commands. Or you start with a small PR so that I have a template.

We should then add .OUTPUTS documentation to those commands to have the type name in the documentation so that we find the corresponding section in dbatools.Format.ps1xml more easily in the future.

andreasjordan avatar Jul 04 '23 06:07 andreasjordan