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.
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
Using Select-Object -ExpandProperty Updates Source PSCustomObjects #21308
Comments
This was first discussed in 2018 in #7768 and change was actually green-lit and marked as up-for-grabs, but no one has stepped up to implement, and the microsoft-github-policy-service bot closed it due to inactivity (something that I think should never happen for green-lit changes). Though, what change, precisely, was green-lit is a bit murky. The preceding discussion is very long, but perhaps starting here helps. |
@mklement0, thanks for bringing that one to my attention. Didn't find it on my first pass. As for what was greenlit, I wouldn't consider it murky. They said "We believe the original intent is to not modify the original object [and] we should just fix the original behavior so that a new object is emitted." i.e., Select-Object should never modify the source object but should instead emit a new one. I'll take a look and see if I can come up with anything that fixes this. |
Weird thing is that I'm seeing code comments indicating that this was already addressed. Did some more looking, and I think the issue might lie here: PowerShell/src/Microsoft.PowerShell.Commands.Utility/commands/utility/Select-Object.cs Lines 548 to 553 in ad2bf78
PowerShell/src/System.Management.Automation/engine/MshObject.cs Lines 1049 to 1054 in ad2bf78
Given the comments in Select-Object, I don't think this was expected by the programmer and it is likely why this issue only affects PSObjects and their derivatives. Other objects like Hash Tables are not affected. |
Always copy the source object instead of referencing it. PowerShell#21308
Nice sleuthing. The immediate fix to the problem at hand is probably something like (a custom object has no base object): PSObject expandedObject =
r.Result is PSObject && r.Result.BaseObject is PSCustomObject ?
r.Result.Copy() : PSObject.AsPSObject(r.Result, true); However, this - intended - per-
// Note the `false` argument.
PSObject expandedObject =
r.Result is PSObject && r.Result.BaseObject is PSCustomObject ?
r.Result.Copy() : PSObject.AsPSObject(r.Result, false); |
@mklement0 I don't think that solution would work as this issue affects PSObject in general, not just the PSCustomObject. $obj = [PSObject]@{"name"="admin1";"children"=[PSObject]@{"name"="admin2"}}
$obj | Select-Object @{N="country";E={$_.name}} -ExpandProperty children
$obj.children.country
# Results in "admin1" when it should be $null. Moreover, the Copy method actually preserves ETS properties through the use of ResurrectionTables, so we won't be loosing things.
PowerShell/src/System.Management.Automation/engine/MshObject.cs Lines 1610 to 1630 in ad2bf78
Testing confirms this. $v = [datetime]::now; $v | Add-Member myProp myPropValue
$v2 = $v.PSObject.Copy()
$v2.myProp
# results in "myPropValue"
$v3 = [PSCustomObject]@{prop = $v}
$v4 = $v3.PSObject.Copy()
$v4.prop.myProp
# results in "myPropValue" |
My interest in delving into the depths of this is limited, but let me offer the following observations:
|
@mklement0, thanks for going a bit deeper into this with me. I appreciate all these details, they helped me figure out how I could implement it. It was a huge pain getting this working as I haven't been able to build PowerShell locally, even in Dev Containers. But it finally builds in CI, and it passes the new Pester tests for #21308 and #7937. PR: #21310 Describe "Select-Object with ExpandProperty" -Tags "CI" {
# Issue #7937
It "Select-Object with ExpandProperty preserves ETS instance members" {
$obj = [DateTime]::Now
$obj | Add-Member myProp myPropValue
$results = [PSCustomObject] @{ prop = $obj } | Select-Object -ExpandProperty prop
$results.myProp | Should -BeExactly "myPropValue"
}
# Issue #21308
It "Select-Object with ExpandProperty and Calculated Property does not modify source object" {
$obj = [PSCustomObject]@{"name"="admin1";"children"=[PSCustomObject]@{"name"="admin2"}}
$obj | Select-Object -Property @{N="country";E={$_.name}} -ExpandProperty children | Out-Null
$obj.children.country | Should -BeNullOrEmpty
}
} |
Glad to hear it, @BinaryWizard904, and thanks for tackling a fix (I see that it isn't a PR yet). It looks like what you've done covers the committee decision, and I now realize, after re-reading the original source-code comments, that the intent indeed appears to be not to use the resurrection tables for the ETS members to be attached for any
(All of these behaviors predate your attempted fix.) $obj = [DateTime]::Now
# Add ETS property via resurrection tables.
$obj | Add-Member ResurrectionProp 1
# Via Select-Object -ExpandProperty combined with -Property,
# try to add - hopefully *another* - ETS property.
# However, because this creates an incidental [psobject] wrapper that
# has ONLY the -Property-specified properties attached, the original,
# resurrection-table property is now *eclipsed*.
$results = [pscustomobject] @{ PSObjectInstanceProp = 2; prop = $obj } |
Select-Object -ExpandProperty prop -Property PSObjectInstanceProp
[pscustomobject] @{
ResurrectionProp = $results.ResurrectionProp
PSObjectInstanceProp = $results.PSObjectInstanceProp
} Output, showing that the resurrection table-based property is now eclipsed:
|
@mklement0, knew I forgot something. The PR is: #21310 That said, if you now don't think fixing #7937 is in the cards, I'll need to rewrite things a bit. |
@BinaryWizard904: Fundamentally, I think it's important to explicitly spell out all the ramifications of the change in the PR description. As for what I think the behavior should be, following the discussions so far:
On a meta note:
|
@mklement0 I do not believe we need to differentiate While it is true the the logic in the Anyways, with your words in mind I have updated the PR description. After further thought I believe we can still resolve both this and #7937 as resolving the latter isn't breaking so much as additive. And if not, well I think I've made clear all that needs changing to remove the fix for #7937 without preventing a fix for this issue is to flip a boolean argument and remove or invert the Pester test. I don't think it was ever really feasible not to allow users to combine |
In part due to my hesitancy to dig into the source code, my previous comment was meant to spell out all cases that need to be considered conceptually. It's great if the existing code allows translating that into fewer cases in the implementation. However, your PR currently doesn't cover all these cases, from what I can tell. The following, extended version of your first Pester test tries to cover all cases I've described: Update: see below Describe 'Select-Object with ExpandProperty' -Tags 'CI' {
# Issue #7937
It 'Select-Object with ExpandProperty preserves ETS instance members' {
# Use a .NET reference type, because the copy semantics of value types
# could hide some behaviors.
$obj = [regex] 'foo'
# Add a property via the resurrection tables.
$obj | Add-Member resurrectTableProp 1
# Now embed the object inside another object and extract it via -ExpandProperty,
# while also decorating it via another property using -Property.
$results =
[PSCustomObject] @{ psobjectWrapperProp = 2; prop = $obj } |
Select-Object -ExpandProperty prop -Property psobjectWrapperProp
# Make sure the resurrection-table member is still present.
$results.resurrectTableProp | Should -BeExactly 1
# Make sure the directly [psobject]-attached member is also present.
$results.psobjectWrapperProp | Should -BeExactly 2
# Make sure that the original object doesn't unexpectedly see the
# directly [psobject]-attached member.
$obj.psobjectWrapperProp | Should -BeExactly $null
# --
# Inverse test:
# Make sure that the resurrection-table member is still that
# (didn't unexpectedly become a directly [psobject]-attached member.)
$results.psobject.BaseObject.resurrectTableProp | Should -BeExactly 1
# Make sure the directly [psobject]-attached member didn't unexpectedly
# become a resurrection-table member.
$results.psobject.BaseObject.psobjectWrapperProp | Should -BeExactly $null
}
# Issue #21308
It 'Select-Object with ExpandProperty and Calculated Property does not modify source object' {
$obj = [PSCustomObject]@{'name' = 'admin1'; 'children' = [PSCustomObject]@{'name' = 'admin2' } }
$obj | Select-Object -Property @{N = 'country'; E = { $_.name } } -ExpandProperty children | Out-Null
$obj.children.country | Should -BeNullOrEmpty
}
} That is, unless I'm missing something, it looks like the Failure:
Similarly, the effectively equivalent later As for the merits of combining |
P.S.: The above test was written under the assumption that a composite view of ETS members is possible, in other words that a given However, I suspect that's not actually supported - hence the suggestion in my conceptual description to copy resurrection table-based members to the wrapper before attaching the |
Indeed, such a composite view (a single,
Here are the updated Pester tests: Describe 'Select-Object with ExpandProperty' -Tags 'CI' {
# Issue #7937
It 'Select-Object with ExpandProperty preserves ETS instance members' {
# Use a .NET reference type, because the copy semantics of value types
# could hide some behaviors.
$obj = [regex] 'foo'
# Add a property via the resurrection tables.
$obj | Add-Member resurrectTableProp 1
# Now embed the object inside another object and extract it via -ExpandProperty,
# while also decorating it via another property using -Property.
$results =
[PSCustomObject] @{ psobjectWrapperProp = 2; prop = $obj } |
Select-Object -ExpandProperty prop -Property psobjectWrapperProp
# Make sure the resurrection-table member is still present, due to having
# been copied to the [psobject] wrapper.
$results.resurrectTableProp | Should -BeExactly 1
# Make sure the directly [psobject] wrapper-attached member, via -Property,
# is also present.
$results.psobjectWrapperProp | Should -BeExactly 2
# Make sure that the original object doesn't unexpectedly see the
# -Property-specified property, i.e. that it wasn't mistakenly added to the
# the resurrection tables instead of being directly attached to the [psobject] wrapper.
$obj.psobjectWrapperProp | Should -BeExactly $null
}
# Issue #21308
It 'Select-Object with ExpandProperty and Calculated Property does not modify source object' {
$obj = [PSCustomObject]@{'name' = 'admin1'; 'children' = [PSCustomObject]@{'name' = 'admin2' } }
$obj | Select-Object -Property @{N = 'country'; E = { $_.name } } -ExpandProperty children | Out-Null
$obj.children.country | Should -BeNullOrEmpty
}
} |
I've reverted the PR to a draft WIP while I get this figured out. You're right about the new tests failing. I will see what I can do to fix it. |
So I've figured out how to get it to pass every test except That said, I have tried a number of things but have been unable to so much as detect ETS members, much less copy them. Unless you have an idea on implementation, I don't think I'll be able to fix it. We will just have to be happy with unchanged behavior. I think I'm also going to need to open a new branch and pull as the existing ones have gotten...crowded. I still haven't been able to get PowerShell to build myself due to
|
Make sure you fetch all the remote tags, it is needed in the build process and failing to do so causes |
@jborean93 In a dev container, I ran PS /workspaces/PowerShell> git fetch --tags --recurse-submodules=yes
PS /workspaces/PowerShell> Import-Module ./build.psm1
PS /workspaces/PowerShell> Start-PSBuild
WARNING: Could not find 'dotnet', appending /root/.dotnet to PATH.
VERBOSE: In Find-DotNet
VERBOSE: Executing: dotnet --list-sdks
VERBOSE: Find-DotNet: dotnetCLIInstalledVersion = 9.0.100-preview.1.24101.2; chosenDotNetVersion = 9.0.100-preview.1.24101.2
VERBOSE: Using configuration 'Debug'
VERBOSE: Using framework 'net9.0'
VERBOSE: Using runtime 'linux-x64'
VERBOSE: Top project directory is /workspaces/PowerShell/src/powershell-unix
VERBOSE: Building without shim
Run dotnet publish /property:GenerateFullPaths=true /property:ErrorOnDuplicatePublishOutputFiles=false --self-contained /property:IsWindows=false --configuration Debug --framework net9.0 --runtime linux-x64 /property:SDKToUse=Microsoft.NET.Sdk from /workspaces/PowerShell/src/powershell-unix
MSBuild version 17.10.0-preview-24101-01+07fd5d51f for .NET
/workspaces/PowerShell/PowerShell.Common.props(18,5): error MSB3073: The command "git describe --abbrev=60 --long" exited with code 128. [/workspaces/PowerShell/src/Microsoft.PowerShell.Security/Microsoft.PowerShell.Security.csproj]
/workspaces/PowerShell/PowerShell.Common.props(18,5): error MSB3073: The command "git describe --abbrev=60 --long" exited with code 128. [/workspaces/PowerShell/src/Microsoft.PowerShell.SDK/Microsoft.PowerShell.SDK.csproj]
/workspaces/PowerShell/PowerShell.Common.props(18,5): error MSB3073: The command "git describe --abbrev=60 --long" exited with code 128. [/workspaces/PowerShell/src/Microsoft.PowerShell.Commands.Utility/Microsoft.PowerShell.Commands.Utility.csproj]
/workspaces/PowerShell/PowerShell.Common.props(18,5): error MSB3073: The command "git describe --abbrev=60 --long" exited with code 128. [/workspaces/PowerShell/src/Microsoft.PowerShell.ConsoleHost/Microsoft.PowerShell.ConsoleHost.csproj]
/workspaces/PowerShell/PowerShell.Common.props(18,5): error MSB3073: The command "git describe --abbrev=60 --long" exited with code 128. [/workspaces/PowerShell/src/Microsoft.PowerShell.Commands.Management/Microsoft.PowerShell.Commands.Management.csproj]
/workspaces/PowerShell/PowerShell.Common.props(18,5): error MSB3073: The command "git describe --abbrev=60 --long" exited with code 128. [/workspaces/PowerShell/src/System.Management.Automation/System.Management.Automation.csproj]
/workspaces/PowerShell/PowerShell.Common.props(18,5): error MSB3073: The command "git describe --abbrev=60 --long" exited with code 128. [/workspaces/PowerShell/src/powershell-unix/powershell-unix.csproj]
Restore failed with errors in 4.4s
Exception: /workspaces/PowerShell/tools/buildCommon/startNativeExecution.ps1:40:17
Line |
40 | throw $errorMessage
| ~~~~~~~~~~~~~~~~~~~
| Execution of { dotnet $Arguments } by build.psm1: line 561 failed with exit code 1 |
Always copy the source object instead of referencing it. This ensures that the source object is never implicitly updated by the Select-Object command. PowerShell#21308
Alright, I've created a new PR #21328 based on a squashed version of the previous work. There is now one commit, and it passes all related tests. |
You need to ensure you are fetching the tags from this repo and not your fork. For example # podman run --rm -it fedora:39
rpm -Uvh https://packages.microsoft.com/config/rhel/9/packages-microsoft-prod.rpm
dnf install -y git powershell
# Substitute this with your fork
git clone https://github.com/jborean93/PowerShell.git
cd PowerShell
# Make sure you use PowerShell/PowerShell, you will see
# multiple tags retrieved when you run it.
git fetch --tags https://github.com/PowerShell/PowerShell.git
# Now the build should work
pwsh -Command 'Import-Module ./build.psm1; Start-PSBootstrap; Start-PSBuild'
src/powershell-unix/bin/Debug/net9.0/linux-x64/pwsh |
Prerequisites
PR: #21328
Steps to reproduce
In PowerShell v7.4.1, when you use
Select-Object
to select a custom column from a nested PSCustomObject object and then use-ExpandProperty
to retrieve the contents of a child object, the custom column will be appended to the selected child object in the source object. This is unexpected asSelect-Object
documentation states that using both-Property
and-ExpandProperty
should only, "[attempt] to add each selected property as a NoteProperty to every outputted object".There is nothing in the documentation about Select-Object altering the source object as well. Moreover, while technically a breaking change, fixing this was green-lit in #7768 (comment) by the @PowerShell/powershell-committee, but improperly implemented using
PSObject.AsPSObject()
.Example:
Expected behavior
Actual behavior
Error details
No response
Environment data
Visuals
No response
The text was updated successfully, but these errors were encountered: