Skip to content

Conversation

@vexx32
Copy link
Collaborator

@vexx32 vexx32 commented May 17, 2019

PR Summary

Adds [ValidateProperty()] attribute for duck-typing parameters, allowing function and cmdlet authors to accept arbitrary PSObject input provided the object(s) provides at least the specified named properties.

PR Context

Resolves #7835. See issue for context.

PR Checklist

@vexx32 vexx32 requested a review from daxian-dbw as a code owner May 17, 2019 04:21
@PoshChan
Copy link
Collaborator

@vexx32, your last commit had 3 failures in PowerShell-CI-windows
Type accelerators.BuiltIn Accelerators.Error occurred in Context block

Unable to find type [System.Management.Automation.ValidateProperty].
At D:\a\1\s\test\powershell\Language\Parser\TypeAccelerator.Tests.ps1:303 char:35

Tests for parameter binding.Default value conversion tests.ValidateScript can use custom ErrorMessage

Expected strings to be the same, but they were different.
Expected length: 80
Actual length:   82
Strings differ at index 60.
Expected: '...led '$_ -g...'
But was:  '...led ' $_ -...'
at <ScriptBlock>, D:\a\1\s\test\powershell\Language\Scripting\ParameterBinding.Tests.ps1: line 391
391:             $err.Exception.Message | Should -BeExactly "Cannot validate argument on parameter 'p'. Item '2' failed '`$_ -gt 2' validation"

Tests for parameter binding.Default value conversion tests.ValidateProperty can use custom ErrorMessage

Expected strings to be the same, but they were different.
Expected length: 79
Actual length:   111
Strings differ at index 60.
Expected: '...led 'a,b' ...'
But was:  '...led '[Syst...'
at <ScriptBlock>, D:\a\1\s\test\powershell\Language\Scripting\ParameterBinding.Tests.ps1: line 413
413:             $err.Exception.Message | Should -BeExactly "Cannot validate argument on parameter 'p'. Item '2' failed 'a,b' property check"

📝 Fix typo
🔧 add in type names to tests
@PoshChan
Copy link
Collaborator

@vexx32, your last commit had 2 failures in PowerShell-CI-macos
Type accelerators.BuiltIn Accelerators.Should have all the type accelerators

Expected 99, but got 100.
at <ScriptBlock>, /Users/vsts/agent/2.150.3/work/1/s/test/powershell/Language/Parser/TypeAccelerator.Tests.ps1: line 445
445:             $TypeAccelerators.Count | Should -Be $totalAccelerators

Tests for parameter binding.Default value conversion tests.ValidateScript can use custom ErrorMessage

Expected strings to be the same, but they were different.
Expected length: 80
Actual length:   82
Strings differ at index 60.
Expected: '...led '$_ -g...'
But was:  '...led ' $_ -...'
at <ScriptBlock>, /Users/vsts/agent/2.150.3/work/1/s/test/powershell/Language/Scripting/ParameterBinding.Tests.ps1: line 391
391:             $err.Exception.Message | Should -BeExactly "Cannot validate argument on parameter 'p'. Item '2' failed '`$_ -gt 2' validation"

@PoshChan
Copy link
Collaborator

@vexx32, your last commit had 2 failures in PowerShell-CI-windows
Type accelerators.BuiltIn Accelerators.Should have all the type accelerators

Expected 104, but got 105.
at <ScriptBlock>, D:\a\1\s\test\powershell\Language\Parser\TypeAccelerator.Tests.ps1: line 445
445:             $TypeAccelerators.Count | Should -Be $totalAccelerators

Tests for parameter binding.Default value conversion tests.ValidateScript can use custom ErrorMessage

Expected strings to be the same, but they were different.
Expected length: 80
Actual length:   82
Strings differ at index 60.
Expected: '...led '$_ -g...'
But was:  '...led ' $_ -...'
at <ScriptBlock>, D:\a\1\s\test\powershell\Language\Scripting\ParameterBinding.Tests.ps1: line 391
391:             $err.Exception.Message | Should -BeExactly "Cannot validate argument on parameter 'p'. Item '2' failed '`$_ -gt 2' validation"

@PoshChan
Copy link
Collaborator

@vexx32, your last commit had 2 failures in PowerShell-CI-linux
Type accelerators.BuiltIn Accelerators.Should have all the type accelerators

Expected 99, but got 100.
at <ScriptBlock>, /home/vsts/work/1/s/test/powershell/Language/Parser/TypeAccelerator.Tests.ps1: line 445
445:             $TypeAccelerators.Count | Should -Be $totalAccelerators

Tests for parameter binding.Default value conversion tests.ValidateScript can use custom ErrorMessage

Expected strings to be the same, but they were different.
Expected length: 80
Actual length:   82
Strings differ at index 60.
Expected: '...led '$_ -g...'
But was:  '...led ' $_ -...'
at <ScriptBlock>, /home/vsts/work/1/s/test/powershell/Language/Scripting/ParameterBinding.Tests.ps1: line 391
391:             $err.Exception.Message | Should -BeExactly "Cannot validate argument on parameter 'p'. Item '2' failed '`$_ -gt 2' validation"

@iSazonov
Copy link
Collaborator

@vexx32 Please move all formatting in new PR.

@vexx32
Copy link
Collaborator Author

vexx32 commented May 17, 2019

Wasn't intentional, VS Code got snarky, it seems. 🙂

I'll do a quick pass in a separate PR for just formatting to help with that.

@KirkMunro
Copy link
Contributor

Calling out specific feedback on the related issue, here.

@vexx32
Copy link
Collaborator Author

vexx32 commented May 19, 2019

Cheers, @KirkMunro!

@iSazonov did a full style pass for Attributes.cs in #9625. I'll move onto the other one VS Code was nitpicking about in a bit.

@TravisEz13
Copy link
Member

@vexx32 Please rebase and resolve failures.

@TravisEz13 TravisEz13 added the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Jun 21, 2019
@KirkMunro
Copy link
Contributor

@vexx32 Coming back to this one (since it just jumped into view again in my email watch list for this repo), and forgive me if I already asked this, but part of me wonders "why?" for this PR. We can already specify a custom type name in an attribute (e.g. [PSTypeName('My.Custom.Type.Name')]), and in my mind that's a better way to identify intent for a PSObject that is passed in, rather than look for specific parameters, especially since error handling will tell them what they should be passing in.

For example:

PS C:\> function Get-Something {
>>     [CmdletBinding()]
>>     [OutputType('That.Is.Something')]
>>     param()
>>     [pscustomobject]@{
>>         PSTypeName = 'That.Is.Something'
>>         One = 'Fish'
>>         Two = 'Fish'
>>     }
>>     [pscustomobject]@{
>>         PSTypeName = 'That.Is.Something.Else'
>>         Red = 'Fish'
>>         Blue = 'Fish'
>>     }
>> }
PS C:\> $one,$two = Get-Something
PS C:\> function Test-Something {
>>     [CmdletBinding()]
>>     [OutputType([bool])]
>>     param(
>>         [Parameter(Mandatory, Position=0)]
>>         [ValidateNotNull()]
>>         [PSTypeName('That.Is.Something')]
>>         $Something
>>     )
>>     $true
>> }
PS C:\> Test-Something -Something $one
True
PS C:\> Test-Something -Something $two
Test-Something : Cannot bind argument to parameter 'Something', because PSTypeNames of the argument
do not match the PSTypeName required by the parameter: That.Is.Something.
At line:1 char:27
+ Test-Something -Something $two
+                           ~~~~
    + CategoryInfo          : InvalidArgument: (:) [Test-Something], ParameterBindingArgumentTransformationException
    + FullyQualifiedErrorId : MismatchedPSTypeName,Test-Something

@vexx32
Copy link
Collaborator Author

vexx32 commented Jun 21, 2019

Have you heard of duck typing? Objects with similar property names may often have similar design intents despite coming from different sources. This kind of system allows folks to design a command with a particular input in mind and then to just make it work.

Though, coming back to this now, it occurs to me that you kind of already can do this, at least with ValueFromPipelineByPropertyName, but it's a bit tricky to work with in some instances. This would just make it a bit easier in cases where you want to consolidate all the otherwise messy parameters into a single input object.

I'll rebase this soon, I have a few things to get to.

@ghost ghost removed the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Jun 21, 2019
@vexx32
Copy link
Collaborator Author

vexx32 commented Jun 21, 2019

@PoshChan please remind me in 9 days.

@KirkMunro
Copy link
Contributor

I know duck typing very well, yes, but I still think the community should learn to use PSTypeName much more, and yeah, when I read this I also thought about ValueFromPipelineByPropertyName as well. With respect to that, I'm curious what you're inferring to by this:

but it's a bit tricky to work with in some instances

@vexx32
Copy link
Collaborator Author

vexx32 commented Jun 21, 2019

I mean, using ValueFromPipelineByPropertyName needs 1 parameter per property. That can get pretty darn messy when you want to validate say 10 or so properties, especially if you need to take other parameters that aren't meant to come from the object itself.

Results in some unfortunately messy design patterns... :/

@KirkMunro
Copy link
Contributor

On the flipside to that, if you're really checking 10 or so properties I'd argue you should be using a custom PSTypeName and checking for that type name instead. 👀

@vexx32
Copy link
Collaborator Author

vexx32 commented Jun 21, 2019

That's true, but there's no real guarantee that the object has been constructed properly in that instance, so that too only feels like a half-solution to the problem.

@KirkMunro
Copy link
Contributor

If you're accepting a custom object as input, you should be receiving that custom object as output from somewhere else. Is that a fair statement? That's the only real way to get anything close to a guarantee. Otherwise what about the types of the values on the members in those objects...those could cause all sorts of issues in a command.

No matter what, when you're working with custom objects, there is potential for an issue somewhere, but isn't that a design issue for the command author to think about? I don't really think someone shouldn't be writing commands that take a custom object as pipeline input with the expectation that the scripter will create the custom object, in which case the command they get that object from can give it to them with PSTypeName already set. For individual property validation, we have ValueFromPipelineByPropertyName which not only will only have a value if the property was there on the object piped into the command, but it also validates type, checks against null, or other validators that may be important.

Something that might help me find this less questionable is a specific, non-contrived example where the issue you're trying to solve here is actually a problem in PowerShell today that would best be solved by this new attribute. Do we have one of those in a PowerShell issue somewhere?

@adityapatwardhan
Copy link
Member

@daxian-dbw Ping..

@adityapatwardhan
Copy link
Member

@vexx32 can you resolve the merge conflict

@adityapatwardhan adityapatwardhan added the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Oct 15, 2019
@ghost ghost added the Stale label Oct 15, 2019
@ghost
Copy link

ghost commented Oct 15, 2019

This pull request has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 30 days. It will be closed if no further activity occurs within 10 days of this comment.

@ghost ghost closed this Oct 25, 2019
@vexx32 vexx32 deleted the Feature-ValidatePropertyAttribute branch June 13, 2020 22:08
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Stale Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Suggestion: Duck-typing ValidateProperty attribute

6 participants