Fix Issue in Select-Object where Hashtable members (e.g., `Keys`) cannot be used with -Property or -ExpandProperty #11097

vexx32 · 2019-11-18T05:39:32Z

PR Summary

Prior to this PR, a hashtable piped into Select-Object -[Expand]Property Keys where the user expected Keys to retrieve the property value from the hashtable rather than the dictionary entry would return no value (in the case of -ExpandProperty, an error would be emitted, stating that the property could not be found).

This PR fixes this issue, and examines hashtable members as well as their key/value entries. The logic is designed such that hashtable key/values are still prioritised over hashtable members, so if a user creates a hashtable such as @{ Keys = "None" } and then selects the Keys property via Select-Object, it will have the same result as using $ht.Keys already does: the returned value will always prioritise an actual key/value entry over the hashtable members.

Note that this means creating a hashtable with a key named Keys will prevent access to the Keys member on the hashtable in this manner. This new behaviour is consistent with existing behaviour with retrieval of hashtable keys, and also consistent with dot-property access behaviour with hashtables -- you can access $ht.Keys up until you add a key with the name Keys, at which point you will always get that key value back.

Tests have been added to cover these cases explicitly.

PR Context

Users should be able to utilise hashtables as actual hashtable objects as well as pseudo-PSObjects where appropriate. It is disingenuous for these commands to pretend hashtables don't have other members which may be desired in some circumstances.

This PR fixes #11094

PR Checklist

vexx32 · 2019-11-18T05:45:48Z

Note for reviewers -- the Select-Object test file hasn't been updated in a long time (still using tabs instead of spaces) so I included a format-only commit 6544c98 to sort that separately. You'll probably want to review that commit individually and then the following one for added tests.

No existing tests were modified in this PR apart from the formatting fixes. I only did a quick tabs-to-spaces and then auto-format commit, so if there are specific style issues you also want to fix while I'm there let me know, we can fix them either here or in a follow up PR as necessary. 🙂

vexx32 · 2019-11-18T05:49:57Z

Also, I think the Codacy suggestion to make IfHashtableWrapAsPSCustomObject() a static method can be ignored. 😄

src/System.Management.Automation/FormatAndOutput/common/Utilities/Mshexpression.cs

vexx32 · 2019-11-18T06:07:29Z

Hmm, interesting. Didn't expect to break that, but there you have it.

vexx32 · 2019-11-18T06:09:07Z

@PoshChan please remind me in 17 hours

iSazonov · 2019-11-18T08:52:38Z

@vexx32 The PR description is more complex than the fix :-)

PoshChan · 2019-11-18T23:09:17Z

@vexx32, this is the reminder you requested 17 hours ago

vexx32 · 2019-11-18T23:24:34Z

That was simpler than I expected! 🎉

vexx32 · 2019-11-19T00:06:14Z

@PoshChan please retry macos

PoshChan · 2019-11-19T00:06:53Z

@vexx32, successfully started retry of PowerShell-CI-macOS

ghost · 2020-05-27T02:02:26Z

This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days.
Mainainer, Please provide feedback and/or mark it as Waiting on Author

src/System.Management.Automation/FormatAndOutput/common/Utilities/Mshexpression.cs

test/powershell/Modules/Microsoft.PowerShell.Utility/Select-Object.Tests.ps1

- Fix an issue where Select-Object and similar could not resolve member names from hashtables due to them carelessly converting them. - Allows both hashtable keys/values and actual members to be referenced. Precedence is given to the hashtable keys first to avoid collisions.

- Add test for Count & Length members - Use base target object instead of wrapping hashtable

iSazonov · 2020-06-11T07:25:22Z

@vexx32 Please look CI failures.

- Still had tab stops in some places, replaced everything with spaces. - removed some accidentally duplicated code - 'Format Document' to ensure consistent indentation. Sorry about the diff, this file was a bit of a mess 🙏

test/powershell/Modules/Microsoft.PowerShell.Utility/Select-Object.Tests.ps1

daxian-dbw · 2020-06-12T23:52:12Z

@vexx32 The test failure in mac CI is a known issue. Can you please check on the Windows failure?

vexx32 · 2020-06-13T00:05:42Z

That's one I haven't seen before. Something to do with remoting? Not sure what to make of it 😅

Unhandled exception. System.NullReferenceException: Object reference not set to an instance of an object.
   at System.Management.Automation.Remoting.PrioritySendDataCollection.ReadOrRegisterCallback(OnDataAvailableCallback callback, DataPriorityType& priorityType) in D:\a\1\s\src\System.Management.Automation\engine\remoting\fanin\PriorityCollection.cs:line 240
   at System.Management.Automation.Remoting.Client.WSManClientCommandTransportManager.SendOneItem() in D:\a\1\s\src\System.Management.Automation\engine\remoting\fanin\WSManTransportManager.cs:line 3975

   at System.Management.Automation.Remoting.Client.WSManClientCommandTransportManager.OnRemoteCmdSendCompleted(IntPtr operationContext, Int32 flags, IntPtr error, IntPtr shellOperationHandle, IntPtr commandOperationHandle, IntPtr operationHandle, IntPtr data) in D:\a\1\s\src\System.Management.Automation\engine\remoting\fanin\WSManTransportManager.cs:line 3681
An error has occurred that was not properly handled. Additional information is shown below. The PowerShell process will exit.
Execution of {& $powershell $PSFlags -c $command} by build.psm1: line 1332 failed with exit code -1073741819
At D:\a\1\s\build.psm1:2191 char:17
+                 throw $errorMessage
+                 ~~~~~~~~~~~~~~~~~~~
+ CategoryInfo          : OperationStopped: (Execution of {& $po…it code -1073741819:String) [], RuntimeException
+ FullyQualifiedErrorId : Execution of {& $powershell $PSFlags -c $command} by build.psm1: line 1332 failed with exit code -1073741819
##[error]PowerShell exited with code '1'.

daxian-dbw · 2020-06-15T17:41:10Z

@PoshChan please retry windows

PoshChan · 2020-06-15T17:41:34Z

@daxian-dbw, did not find any matching pull request checks

iSazonov

~~Tests contain many style fixes - please move it in separate PR.~~

iSazonov · 2020-06-15T18:33:16Z

src/System.Management.Automation/FormatAndOutput/common/Utilities/Mshexpression.cs

+                {
+                    members = target.Members.Match(
+                        _stringValue,
+                        PSMemberTypes.Properties | PSMemberTypes.PropertySet | PSMemberTypes.Dynamic);


We evaluate this again. Maybe put this in a variable before line 172?

This is evaluated against the wrapped target object in the prior case.

I mean PSMemberTypes.Properties | PSMemberTypes.PropertySet | PSMemberTypes.Dynamic

src/System.Management.Automation/FormatAndOutput/common/Utilities/Mshexpression.cs

iSazonov · 2020-06-15T18:38:41Z

test/powershell/Modules/Microsoft.PowerShell.Utility/Select-Object.Tests.ps1

+        $result = $(Select-Object -InputObject $dirObject -Last $TestLength).Length
+        $expected = $dirObject.Length
+        { $dirObject | Select-Object } | Should -Not -Throw


Should -Not -Throw is not good test.
Why do we remove one test below? And why do we need $expected above? Why do we change the tests at all?

daxian-dbw · 2020-06-15T20:31:20Z

@iSazonov It was called out at #11097 (comment) that there is a format-only commit included.
I'm fine to keep that commit in this PR because this test file was seldom touched.

BTW @iSazonov, it's easier to review after ignoring the white space characters: https://github.com/PowerShell/PowerShell/pull/11097/files?w=1

daxian-dbw

I agree with @iSazonov, something is wrong in the changes of these couple of tests.

daxian-dbw · 2020-06-16T17:16:00Z

test/powershell/Modules/Microsoft.PowerShell.Utility/Select-Object.Tests.ps1

-	$result   = $(Select-Object -InputObject $dirObject -Last $TestLength).Length
-	$expected = $dirObject.Length
-
-	$result | Should -Be $expected


$result | Should -Be $expected shouldn't be removed here.

daxian-dbw · 2020-06-16T17:16:31Z

test/powershell/Modules/Microsoft.PowerShell.Utility/Select-Object.Tests.ps1

-	{ $dirObject | select } | Should -Not -Throw
+        $result = $(Select-Object -InputObject $dirObject -Last $TestLength).Length
+        $expected = $dirObject.Length
+        { $dirObject | Select-Object } | Should -Not -Throw


You need to keep the It "Should be able to use the alias" { here.

daxian-dbw · 2020-06-16T17:22:07Z

test/powershell/Modules/Microsoft.PowerShell.Utility/Select-Object.Tests.ps1


-	It "Select-Object with Property First Last Overlap should work"{


Why is this test case removed?

Weird, not sure, will add it back.

daxian-dbw · 2020-06-16T17:22:48Z

test/powershell/Modules/Microsoft.PowerShell.Utility/Select-Object.Tests.ps1

@@ -362,13 +340,48 @@ Describe "Select-Object with Property = '*'" -Tags "CI" {
        $obj.psobject.TypeNames.Count | Should -Be 3
        $obj.psobject.TypeNames[0] | Should -BeLike "Selected*"
        $obj.psobject.TypeNames[1] | Should -Not -BeLike "Selected*"
+        $p = Get-Process -Id $pid | Select-Object -Property Process* -ExcludeProperty ProcessorAffinity -ExpandProperty Modules
+        $p[0].psobject.Properties.Item("ProcessorAffinity") | Should -BeNullOrEmpty


The new code should be moved after $obj.psobject.TypeNames[2] | Should .. line below.

Looks like this got duplicated from above (L#328-329) somehow. Sorry about that, really not sure what on earth happened here. Will remove.

Some of the Select-Object tests got a bit mangled during format. This commit should restore them all back to proper order.

vexx32 · 2020-06-19T16:17:20Z

Apologies about the mangled tests, not sure how that happened. Should be all fixed up now. 💖 😊

ghost · 2020-06-25T19:07:17Z

🎉v7.1.0-preview.4 has been released which incorporates this pull request.:tada:

Handy links:

Release Notes

vexx32 added the CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log label Nov 18, 2019

PoshChan reviewed Nov 18, 2019

View reviewed changes

src/System.Management.Automation/FormatAndOutput/common/Utilities/Mshexpression.cs Show resolved Hide resolved

PoshChan reviewed Nov 18, 2019

View reviewed changes

src/System.Management.Automation/FormatAndOutput/common/Utilities/Mshexpression.cs Show resolved Hide resolved

PoshChan reviewed Nov 18, 2019

View reviewed changes

src/System.Management.Automation/FormatAndOutput/common/Utilities/Mshexpression.cs Show resolved Hide resolved

iSazonov changed the title ~~Select-Object - 🐛 Fix Issue where Hashtable members (e.g., Keys) cannot be used with -Property or -ExpandProperty~~ Fix Issue in Select-Object where Hashtable members (e.g., Keys) cannot be used with -Property or -ExpandProperty Nov 18, 2019

iSazonov added this to the 7.1.0-preview.1 milestone Nov 19, 2019

iSazonov assigned daxian-dbw Nov 19, 2019

vexx32 mentioned this pull request Nov 21, 2019

Q&A for PowerShell Community Call for Nov 21, 2019 PowerShell/PowerShell-RFC#235

Closed

daxian-dbw modified the milestones: 7.1.0-preview.1, 7.1.0-preview.2 Apr 1, 2020

TravisEz13 modified the milestones: 7.1.0-preview.2, 7.1.0-preview.3 Apr 24, 2020

TravisEz13 modified the milestones: 7.1.0-preview.3, 7.1.0-preview.4 May 14, 2020

ghost added the Review - Needed The PR is being reviewed label May 27, 2020

daxian-dbw requested changes Jun 10, 2020

View reviewed changes

ghost added the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Jun 10, 2020

ghost removed this from the 7.1.0-preview.4 milestone Jun 10, 2020

ghost removed the Review - Needed The PR is being reviewed label Jun 10, 2020

ghost removed the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Jun 11, 2020

♻️ Address @daxian-dbw review comments

74e066c

- Add test for Count & Length members - Use base target object instead of wrapping hashtable

vexx32 force-pushed the SelectObjectCommand-DictionaryProperties branch from 6de53fe to 74e066c Compare June 11, 2020 02:23

♻️ un-break test file

cfea794

- Still had tab stops in some places, replaced everything with spaces. - removed some accidentally duplicated code - 'Format Document' to ensure consistent indentation. Sorry about the diff, this file was a bit of a mess 🙏

vexx32 force-pushed the SelectObjectCommand-DictionaryProperties branch from 8beeb21 to cfea794 Compare June 11, 2020 15:59

daxian-dbw requested changes Jun 12, 2020

View reviewed changes

ghost added Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept and removed Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept labels Jun 12, 2020

✅ Resolve test issues

f7845eb

daxian-dbw approved these changes Jun 15, 2020

View reviewed changes

iSazonov reviewed Jun 15, 2020

View reviewed changes

♻️ Address review comments

3d83000

daxian-dbw requested changes Jun 16, 2020

View reviewed changes

ghost added Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept and removed Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept labels Jun 16, 2020

✅ Restore mangled tests

f0802de

Some of the Select-Object tests got a bit mangled during format. This commit should restore them all back to proper order.

iSazonov approved these changes Jun 19, 2020

View reviewed changes

daxian-dbw approved these changes Jun 19, 2020

View reviewed changes

daxian-dbw merged commit 02af121 into PowerShell:master Jun 19, 2020

daxian-dbw added this to the 7.1.0-preview.4 milestone Jun 19, 2020

vexx32 deleted the SelectObjectCommand-DictionaryProperties branch June 20, 2020 00:46

ghost mentioned this pull request Jun 25, 2020

Select-Object -ExpandProperty does not work with Hashtable members #11094

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Issue in Select-Object where Hashtable members (e.g., `Keys`) cannot be used with -Property or -ExpandProperty #11097

Fix Issue in Select-Object where Hashtable members (e.g., `Keys`) cannot be used with -Property or -ExpandProperty #11097

vexx32 commented Nov 18, 2019 •

edited

Loading

vexx32 commented Nov 18, 2019 •

edited

Loading

vexx32 commented Nov 18, 2019

vexx32 commented Nov 18, 2019 •

edited

Loading

vexx32 commented Nov 18, 2019

iSazonov commented Nov 18, 2019

PoshChan commented Nov 18, 2019

vexx32 commented Nov 18, 2019

vexx32 commented Nov 19, 2019

PoshChan commented Nov 19, 2019

ghost commented May 27, 2020

iSazonov commented Jun 11, 2020

daxian-dbw commented Jun 12, 2020

vexx32 commented Jun 13, 2020

daxian-dbw commented Jun 15, 2020

PoshChan commented Jun 15, 2020

iSazonov left a comment •

edited

Loading

iSazonov Jun 15, 2020

vexx32 Jun 16, 2020 •

edited

Loading

iSazonov Jun 16, 2020

iSazonov Jun 15, 2020

daxian-dbw commented Jun 15, 2020 •

edited

Loading

daxian-dbw left a comment

daxian-dbw Jun 16, 2020

daxian-dbw Jun 16, 2020

daxian-dbw Jun 16, 2020

vexx32 Jun 19, 2020

daxian-dbw Jun 16, 2020

vexx32 Jun 19, 2020

vexx32 commented Jun 19, 2020

ghost commented Jun 25, 2020


		It "Select-Object with Property First Last Overlap should work"{

Fix Issue in Select-Object where Hashtable members (e.g., Keys) cannot be used with -Property or -ExpandProperty #11097

Fix Issue in Select-Object where Hashtable members (e.g., Keys) cannot be used with -Property or -ExpandProperty #11097

Conversation

vexx32 commented Nov 18, 2019 • edited Loading

PR Summary

PR Context

PR Checklist

vexx32 commented Nov 18, 2019 • edited Loading

vexx32 commented Nov 18, 2019

vexx32 commented Nov 18, 2019 • edited Loading

vexx32 commented Nov 18, 2019

iSazonov commented Nov 18, 2019

PoshChan commented Nov 18, 2019

vexx32 commented Nov 18, 2019

vexx32 commented Nov 19, 2019

PoshChan commented Nov 19, 2019

ghost commented May 27, 2020

iSazonov commented Jun 11, 2020

daxian-dbw commented Jun 12, 2020

vexx32 commented Jun 13, 2020

daxian-dbw commented Jun 15, 2020

PoshChan commented Jun 15, 2020

iSazonov left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vexx32 Jun 16, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daxian-dbw commented Jun 15, 2020 • edited Loading

daxian-dbw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vexx32 commented Jun 19, 2020

ghost commented Jun 25, 2020

Fix Issue in Select-Object where Hashtable members (e.g., `Keys`) cannot be used with -Property or -ExpandProperty #11097

Fix Issue in Select-Object where Hashtable members (e.g., `Keys`) cannot be used with -Property or -ExpandProperty #11097

vexx32 commented Nov 18, 2019 •

edited

Loading

vexx32 commented Nov 18, 2019 •

edited

Loading

vexx32 commented Nov 18, 2019 •

edited

Loading

iSazonov left a comment •

edited

Loading

vexx32 Jun 16, 2020 •

edited

Loading

daxian-dbw commented Jun 15, 2020 •

edited

Loading