Skip to content

Ensure -PipelineVariable is set for all output from script cmdlets #12766

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

Merged
rjmholt merged 2 commits into from
Dec 10, 2020
Merged

Ensure -PipelineVariable is set for all output from script cmdlets #12766

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
bbbb4ee
:bug: Fix PipelineVariable not being set correctly
vexx32 May 22, 2020
7b0a58b
:white_check_mark: Add test for pipelinevariable
vexx32 May 23, 2020
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
11 changes: 11 additions & 0 deletions src/System.Management.Automation/engine/MshCommandRuntime.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3744,6 +3744,17 @@ internal void SetVariableListsInPipe()

Comment thread
vexx32 marked this conversation as resolved.
Outdated
Comment thread
vexx32 marked this conversation as resolved.
Outdated
Comment thread
vexx32 marked this conversation as resolved.
Outdated
if (this.PipelineVariable != null)
{
// _state can be null if the current script block is dynamicparam, etc.
if (_state != null)
{
// Create the pipeline variable
_state.PSVariable.Set(_pipelineVarReference);

// Get the reference again in case we re-used one from the
// same scope.
_pipelineVarReference = _state.PSVariable.Get(this.PipelineVariable);
}

this.OutputPipe.SetPipelineVariable(_pipelineVarReference);
}
}
Expand Down
56 changes: 56 additions & 0 deletions test/powershell/engine/ParameterBinding/ParameterBinding.Tests.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -256,6 +256,62 @@ Describe "Parameter Binding Tests" -Tags "CI" {
DynamicParamTest -PipelineVariable bar | ForEach-Object { $bar } | Should -Be "hi"
}

Context "PipelineVariable Behaviour" {

BeforeAll {
function Write-PipelineVariable {
[CmdletBinding()]
[OutputType([int])]
param(
[Parameter(ValueFromPipeline)]
$a
)
begin { 1 }
process { 2 }
end { 3 }
}

$testScripts = @(
Copy link
Copy Markdown
Collaborator

@JamesWTruher JamesWTruher Jun 3, 2020

Choose a reason for hiding this comment

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

i'm a little concerned that there isn't enough validation. Can you think of additional edges here that we should be poking?

Copy link
Copy Markdown
Collaborator Author

@vexx32 vexx32 Jun 3, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

There are probably numerous odd cases where this might crop up, but I can't think of any that are really different from the test case here. At the very least, this does cover the test case that we know is currently broken.

I've not seen any other cases crop up myself, nor had someone drop in the community channels with an issue that came down to this. I suspect that that's more due to -PipelineVariable being a bit more of a niche concept from the start, though.

Without knowing for sure about other broken cases, the best I can do is test a few other cases with multiple script cmdlets and various mixes of script / compiled cmdlets to see if the logic breaks down anywhere.

The only real unknown where I don't know what to actually expect is what is meant to happen if multiple cmdlets in the same pipeline declare the same pipeline variable. I'm not sure the expected behaviour there is even recorded anywhere. Best guess - the nearest command to the variable's usage will have its output in there at any given point...

If anyone else has cases they know to be broken / likely to break, I'd appreciate having those as well.

@{
CmdletType = 'Script Cmdlet'
Script = {
1..3 |
Write-PipelineVariable -PipelineVariable pipe |
Select-Object -Property @(
@{ Name = "PipelineVariableSet"; Expression = { $null -ne $pipe ? $true : $false } }
@{ Name = "PipelineVariable"; Expression = { $pipe } }
)
}
}
@{
CmdletType = 'Compiled Cmdlet'
Script = {
1..3 |
Write-PipelineVariable |
ForEach-Object { $_ } -PipelineVariable pipe |
Select-Object -Property @(
@{ Name = "PipelineVariableSet"; Expression = { $null -ne $pipe ? $true : $false } }
@{ Name = "PipelineVariable"; Expression = { $pipe } }
)
}
}
)
}

AfterAll {
Remove-Item -Path 'function:Write-PipelineVariable'
}

It 'should set the pipeline variable every time for a <CmdletType>' -TestCases $testScripts {
param($Script, $CmdletType)

$result = & $Script
$result.Count | Should -Be 5
$result.PipelineVariableSet | Should -Not -Contain $false
$result.PipelineVariable | Should -Be 1, 2, 2, 2, 3
}
}

Context "Use automatic variables as default value for parameters" {
BeforeAll {
## Explicit use of 'CmdletBinding' make it a script cmdlet
Expand Down