Fix for #6855 - Measure-Object should handle scriptblock properties. #6934

BrucePay · 2018-05-25T01:10:41Z

PR Summary

Fixed issue #6855 by renaming the class that does all the work from MshExpression to PSPropertyExpression, making the class public and then in MeasureObjectCommand, lifting it up to the parameter level. Previously the implementation exposed the -Property as string then wrapped it internally as a PSPropertyExpression. Now the parameter type is PSPropertyExpression directly allowing for both wildcard strings and scriptblocks. Note that this is technically a breaking change (the public type of the parameter has changed) but this was reviewed by the committee and approved. (The vast majority of changes in this PR are due to the renaming. The code changes are relatively small.)

PSPropertyExpression now lives in a public namespace where it can be used by cmdlet and script authors to easily add the same type of functionality to their commands.

I also modified PSPropertyExpression to treat hashtables as objects so that

@{prop = 3} | measure-object prop

and

@{prop = 3} | measure-object {$_.prop}

will work the identically. (Previously the example using just the property name would just fail.)

Breaking Change Summary

This PR brings in a breaking change in an edge case:

This could be a breaking change when it comes to the ConstrainedLanguage mode.
@{prop = 3} | measure-object Count works in ConstrainedLanguage mode today, but with this change, the conversion from Hashtable to PSCustomObject will throw exception.

It has been reviewed by the committee and agreed to accept this breaking change.
Please see the discussion at #6934 (comment) for more information.

PR Checklist

iSazonov · 2018-05-25T06:10:58Z

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

If we use pipeline make sense use some values:

$result = "a,b,c", "d,e" | Measure-Object -Word {$_ -split ','} $result.Words | Should -Be 5

iSazonov · 2018-05-25T06:22:17Z

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

Maybe move the function and tests in new Context block?

Moved into a separate Describe.

iSazonov · 2018-05-25T06:23:37Z

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

Please add empty new line.

iSazonov · 2018-05-25T06:25:24Z

We should get PowerShell Committee conclusion for new public API.

BrucePay · 2018-05-29T03:20:47Z

@iSazonov We reviewed this in the last committee meeting and approved the name. I thought @SteveL-MSFT had updated the bug to reflect this but I don't see it.

iSazonov · 2018-05-29T03:26:47Z

@BrucePay Thanks! I am extecting that the PR will marked by the label.
Seems we lost all MSFT team in recent weeks. I wonder.

BrucePay · 2018-06-01T00:11:39Z

Oops - I had the wrong issue number in the comment so you might have looked at the wrong issue. #6855 is, in fact, marked "Committee Reviewed".

iSazonov

LGTM with one minor comment.

iSazonov · 2018-06-01T06:44:35Z

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

Could you please use C# 7 syntax?

if (PSObject.Base(target) as Hashtable targetAsHash)

BrucePay · 2018-06-01T19:37:43Z

@SteveL-MSFT Says we're ignoring CodeFactor issues for now.

SteveL-MSFT · 2018-06-01T21:09:37Z

@iSazonov FYI - Most of my team has been involved with helping port existing Windows PowerShell modules owned by other teams to work with PSCore6 so you may find us not as active. Hopefully we'll be back to fully engaged by end of June (or mostly engaged).

iSazonov · 2018-06-02T10:15:11Z

I'm glad that this process began. I do not see this leading to corrections in PowerShell Core - it convinces me that PowerShell Core is very good! 😄

daxian-dbw

/cc @PaulHigin Now that PSPropertyExpression is publically exposed and it has the ability to run scripts, Paul you may want to check if this will introduce any security concerns to the ConstrainedLanguage mode.

daxian-dbw · 2018-06-05T22:39:01Z

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

This could be a breaking change when it comes to the ConstrainedLanguage mode.
@{prop = 3} | measure-object Count works in ConstrainedLanguage mode today, but with this change, the conversion from Hashtable to PSCustomObject will throw exception.

Hmm - true - good catch. It's pretty obscure and currently the only useful scenario for piping hashtables into measure-object (other than instance-counting). Still, we should probably get a committee review on this.

daxian-dbw · 2018-06-05T22:42:31Z

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

Can you please change the name to WrapHashtableAsPSCustomObject? C# method naming conversion suggests using verbs or verb phrases to name methods.

The point of the rule is that the names of methods should be imperative which this is. There are other examples of (type 1) conditional clauses in .NET such as ThrowIfCancellationRequested on System.Threading.CancellationToken.

OK. #Closed

daxian-dbw · 2018-06-05T22:44:01Z

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

If we want to handle hashtable specially, shall we expand that to IDictionary? LanguagePrimitives.ConvertPSObjectToType is able to convert IDictionary to PSObject.

The goal is to match the behaviour of [PSCustomObject] @{a=1; b=2}. Currently casting Dictionary<T,T> to PSCustomObject doesn't result in a custom object so I don't think we should introduce that behaviour here.

Understood. Good to know that we are matching the behavior of [PSCustomObject] @{a=1; b=2}.

daxian-dbw

Waiting on the committee's review.

PaulHigin · 2018-06-14T17:45:03Z

@daxian-dbw @BrucePay Sorry for the late review. To answer Dongbo's question, from a security point of view I think this is Ok. MshExpression (now PSPropertyExpression) takes a ScriptBlock in its constructor which it will invoke. But as long as the passed in ScriptBlock is created in a constrained environment it will be marked accordingly and the script block will be invoked with CL mode constraints.

A constructor also takes a string "expression", but as near as I can tell it is never evaluated as script and is used to look up properties. Is this correct?

However, we should include a CL mode test to ensure the expression scriptblock is run in CL mode.

daxian-dbw · 2018-06-14T19:08:55Z

A constructor also takes a string "expression", but as near as I can tell it is never evaluated as script and is used to look up properties. Is this correct?

@PaulHigin Yes, that's correct. It's for property lookup.

daxian-dbw · 2018-06-14T19:12:09Z

src/System.Management.Automation/engine/parser/TypeResolver.cs

@PaulHigin I would like you to take another look at this change. PSPropertyExpression is added to CoreTypes, and that means users can call its methods in ConstrainedLanguage mode. Do you have any concerns about it?

Thanks, I didn't see that. If this type is going to be white listed then it needs to go through a separate/full security review. I see that some internal methods are now public, and these along with constructors need to be reviewed for vulnerabilities. Please set up a meeting for this.

No - it's only intended to be a type accelerator. I added it in the wrong place. I'll move it to the TypeAccelerators() method where it should be.

Yeah, I agree it should be moved :)

PaulHigin · 2018-06-14T19:49:05Z

src/System.Management.Automation/engine/parser/TypeResolver.cs

Thanks, I didn't see that. If this type is going to be white listed then it needs to go through a separate/full security review. I see that some internal methods are now public, and these along with constructors need to be reviewed for vulnerabilities. Please set up a meeting for this.

SteveL-MSFT · 2018-06-14T20:23:23Z

@PowerShell/powershell-committee reviewed this and is fine with this breaking change which is likely only an edge case, but the positive impact for most users is beneficial

…operties. Fixed by renaming MshExpression to PSPropertyExpression and making it public. Then in MeasureObjectCommand, lifting it up to the parameter level. Previously the implementation exposed the Property as string and wrapped it internally as a PSPropertyExpression. Now the parameter type is PSPropertyExpression directly allowing for both wildcard strings and scriptblocks. PSPropertyExpression now lives in a public namespace where it can be used by cmdlet and script authors to easily add the same type of functionality to their commands. I also modified PSPropertyExpression to handle hashtables properly as objects so @{prop = 3} | measure-object prop and @{prop = 3} | measure-object {$_.prop} will work the same. (Previously the example using just the property name would fail.)

… matching

Added missing type accelerator test, cleaned up tests.

daxian-dbw · 2018-06-18T18:53:57Z

@PaulHigin New commits have been pushed. Can you please update your review? Thanks

PaulHigin

LGTM

daxian-dbw · 2018-06-19T00:01:24Z

@BrucePay I just noticed that we didn't run feature tests for this PR CI. Can you please push another commit with [feature] in the commit message? That will trigger the feature test run. You can also update the current head commit by running git commit --amend in your local brucepay_MeasureObject branch, and then forcing push to the remote by git push --force-with-lease <your fork repo> brucepay_MeasureObject. Thanks!

daxian-dbw · 2018-06-19T16:47:08Z

@BrucePay I have submitted a dummy commit to trigger the feature test run for this PR.

Prior to this change, the documentation for `Group-Object`, `Select-Object`, and `Sort-Object` did not specifically address using those cmdlets against **hashtable** input. In PowerShell 6 an improvement (see PowerShell/PowerShell#6934) enabled users to reference the values of **hashtable** keys in these commands without having to specify calculated properties. This change: - Updates the documentation for `Group-Object`, `Select-Object`, and `Sort-Object` in versions 7.0 and above to show how users can group, select, and sort **hashtable** objects by their key values - Updates the documentation for `Group-Object`, `Select-Object`, and `Sort-Object` in Windows PowerShell 5.1 to show how users can group, select, and sort **hashtable** objects with calculated properties - Cleans up Vale linting warnings across those files - Fixes AB#20552 - Resolves MicrosoftDocs#9311

Prior to this change, the documentation for `Group-Object`, `Select-Object`, and `Sort-Object` did not specifically address using those cmdlets against **hashtable** input. In PowerShell 6 an improvement (see PowerShell/PowerShell#6934) enabled users to reference the values of **hashtable** keys in these commands without having to specify calculated properties. This change: - Updates the documentation for `Group-Object`, `Select-Object`, and `Sort-Object` in versions 7.0 and above to show how users can group, select, and sort **hashtable** objects by their key values - Updates the documentation for `Group-Object`, `Select-Object`, and `Sort-Object` in Windows PowerShell 5.1 to show how users can group, select, and sort **hashtable** objects with calculated properties - Cleans up Vale linting warnings across those files - Fixes AB#20552 - Resolves #9311

BrucePay requested review from JamesWTruher, PaulHigin, dantraMSFT, daxian-dbw and vors as code owners May 25, 2018 01:10

BrucePay assigned daxian-dbw May 25, 2018

iSazonov reviewed May 25, 2018

View reviewed changes

BrucePay requested review from TravisEz13 and joeyaiello as code owners June 1, 2018 00:41

iSazonov reviewed Jun 1, 2018

View reviewed changes

iSazonov added the Documentation Needed in this repo Documentation is needed in this repo label Jun 1, 2018

iSazonov approved these changes Jun 1, 2018

View reviewed changes

daxian-dbw requested changes Jun 5, 2018

View reviewed changes

BrucePay added the Review - Committee The PR/Issue needs a review from the PowerShell Committee label Jun 8, 2018

daxian-dbw approved these changes Jun 13, 2018

View reviewed changes

daxian-dbw reviewed Jun 14, 2018

View reviewed changes

PaulHigin suggested changes Jun 14, 2018

View reviewed changes

SteveL-MSFT added Committee-Reviewed PS-Committee has reviewed this and made a decision and removed Review - Committee The PR/Issue needs a review from the PowerShell Committee labels Jun 14, 2018

SteveL-MSFT added the Breaking-Change breaking change that may affect users label Jun 14, 2018

BrucePay requested review from Francisco-Gamino, adityapatwardhan, anmenaga and mirichmo as code owners June 15, 2018 22:55

Bruce Payette added 6 commits June 15, 2018 17:10

merge conflict

78e9618

Added missing type accelerator test, cleaned up tests

5c94dc3

modified IfHashtableWrapAsPSCustomObject function to use C# 7 pattern…

662406e

… matching

Moved PSPropertyExpression shortcut to type accelerator list.

0a9525c

Added missing type accelerator test, cleaned up tests.

merge conflict

a7386a9

BrucePay force-pushed the brucepay_MeasureObject branch from 6ba5c16 to a7386a9 Compare June 16, 2018 00:13

PaulHigin approved these changes Jun 18, 2018

View reviewed changes

[Feature] Dummy commit to trigger daily test run

999a31c

daxian-dbw merged commit 3a4c410 into PowerShell:master Jun 20, 2018

iSazonov mentioned this pull request Aug 31, 2018

Join-String cmdlet for creating text from pipeline input #7660

Merged

11 tasks

joeyaiello mentioned this pull request Oct 15, 2018

Docs needed for 'Fix for #6855 - Measure-Object should handle scriptblock properties.' MicrosoftDocs/PowerShell-Docs#3034

Closed

joeyaiello removed Documentation Needed in this repo Documentation is needed in this repo labels Oct 15, 2018

KyleMit mentioned this pull request Oct 15, 2022

Document that Select-Object, Sort-Object and Group-Object handle HashTable Properties in PowerShell 6+ MicrosoftDocs/PowerShell-Docs#9311

Closed

3 tasks

michaeltlombardi mentioned this pull request Oct 17, 2022

(GH-9311) Document *-Object hashtable handling MicrosoftDocs/PowerShell-Docs#9318

Merged

4 tasks

Fix for #6855 - Measure-Object should handle scriptblock properties. #6934

Fix for #6855 - Measure-Object should handle scriptblock properties. #6934

Uh oh!

Conversation

BrucePay commented May 25, 2018 • edited by daxian-dbw Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

Breaking Change Summary

PR Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iSazonov commented May 25, 2018

Uh oh!

BrucePay commented May 29, 2018

Uh oh!

iSazonov commented May 29, 2018

Uh oh!

BrucePay commented Jun 1, 2018

Uh oh!

iSazonov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BrucePay commented Jun 1, 2018

Uh oh!

SteveL-MSFT commented Jun 1, 2018

Uh oh!

iSazonov commented Jun 2, 2018

Uh oh!

daxian-dbw left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daxian-dbw left a comment

Choose a reason for hiding this comment

Uh oh!

PaulHigin commented Jun 14, 2018

Uh oh!

daxian-dbw commented Jun 14, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SteveL-MSFT commented Jun 14, 2018

Uh oh!

daxian-dbw commented Jun 18, 2018

BrucePay commented May 25, 2018 •

edited by daxian-dbw

Loading