Skip to content

Add EscapeHandling parameter in ConvertTo-Json cmdlet #7775

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

Jump to bottom
Merged
iSazonov merged 5 commits into from
Oct 2, 2018
Merged

Add EscapeHandling parameter in ConvertTo-Json cmdlet #7775

iSazonov merged 5 commits into from
Oct 2, 2018

Conversation

@iSazonov
Copy link
Collaborator

@iSazonov iSazonov commented Sep 13, 2018 •
edited
Loading

PR Summary

Fix #7693.

Add EscapeHandling parameter in ConvertTo-Json cmdlet to allow escape the HTML (<, >, &, ', ") and control characters (e.g. newline).

PR Checklist

@iSazonov iSazonov added the Documentation Needed in this repo Documentation is needed in this repo label Sep 13, 2018
@iSazonov iSazonov self-assigned this Sep 13, 2018
@iSazonov
Copy link
Collaborator Author

iSazonov commented Sep 13, 2018

/cc @PlagueHO @mkht @markekraus

@PlagueHO
Copy link

PlagueHO commented Sep 13, 2018

Great solution - thanks @iSazonov - really appreciate it. 🏆

@iSazonov iSazonov force-pushed the convertto-json-escapehandling branch 2 times, most recently from d2e6719 to 1af310d Compare September 13, 2018 09:39
@iSazonov
Copy link
Collaborator Author

iSazonov commented Sep 13, 2018

@TravisEz13 Should CI Travis be removed?

@TravisEz13
Copy link
Member

TravisEz13 commented Sep 13, 2018

@iSazonov Yes, it is now replaced with the PowerShell-CI-linux and PowerShell-CI-macos

SteveL-MSFT
SteveL-MSFT requested changes Sep 14, 2018
View reviewed changes
test/powershell/Modules/Microsoft.PowerShell.Utility/ConvertTo-Json.Tests.ps1 Outdated
Copy link
Member

@SteveL-MSFT SteveL-MSFT Sep 14, 2018

Choose a reason for hiding this comment

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

Turn this into -TestCases? Benefit is if first assert fails, the whole thing doesn't fail.

@iSazonov iSazonov force-pushed the convertto-json-escapehandling branch from 3ff21c6 to 4cf4378 Compare September 17, 2018 11:24
SteveL-MSFT
SteveL-MSFT requested changes Sep 17, 2018
View reviewed changes
test/powershell/Modules/Microsoft.PowerShell.Utility/ConvertTo-Json.Tests.ps1 Outdated
Copy link
Member

@SteveL-MSFT SteveL-MSFT Sep 17, 2018

Choose a reason for hiding this comment

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

Is there a reason for this change from using the verbose channel to know that the cmdlet has started to a sleep? On a particularly slow container/VM, it might take longer than 300ms.

Copy link
Collaborator Author

@iSazonov iSazonov Sep 18, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

We removed the verbose message in #7487 and currently the test take 10 seconds until timeout.
We could consider adding debug message instead of the verbose message.

Copy link
Member

@SteveL-MSFT SteveL-MSFT Sep 18, 2018

Choose a reason for hiding this comment

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

@iSazonov ok, got it. Seems like the right thing to do would be to have some test hook so that we can make this predictable as anything with small timeouts are more likely to fail in CI especially when it's under load.

Copy link
Collaborator Author

@iSazonov iSazonov Sep 19, 2018

Choose a reason for hiding this comment

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

I added verbose in BeginProcessing(). (Previously it was in ProcessRecord())

SteveL-MSFT
SteveL-MSFT requested changes Sep 19, 2018
View reviewed changes
test/powershell/Modules/Microsoft.PowerShell.Utility/ConvertTo-Json.Tests.ps1 Outdated
Copy link
Member

@SteveL-MSFT SteveL-MSFT Sep 19, 2018

Choose a reason for hiding this comment

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

Per comment on line 33, Stop() is synchronous and in the chance it blocks, Pester is blocked. Perhaps we can change the sleep on line 34 to Wait-UntilTrue where State changes?

Copy link
Collaborator Author

@iSazonov iSazonov Sep 19, 2018

Choose a reason for hiding this comment

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

I think we could discuss this in #7819 and perhaps remove the tests at all.

Copy link
Member

@SteveL-MSFT SteveL-MSFT Sep 19, 2018

Choose a reason for hiding this comment

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

If you can revert changes to this test in this PR, I think we can move forward with this PR and resolve this test in the other PR.

Copy link
Collaborator Author

@iSazonov iSazonov Sep 20, 2018

Choose a reason for hiding this comment

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

Reverted in the PR and moved to #7819.

@iSazonov iSazonov force-pushed the convertto-json-escapehandling branch from 24ffde6 to 7ef9f10 Compare September 20, 2018 07:52
@iSazonov iSazonov force-pushed the convertto-json-escapehandling branch from 6df405a to 7ef9f10 Compare September 20, 2018 08:13
@iSazonov
Copy link
Collaborator Author

iSazonov commented Sep 20, 2018

Reopen to restart PowerShell-CI-Linux.

@iSazonov iSazonov closed this Sep 20, 2018
@iSazonov iSazonov reopened this Sep 20, 2018
@iSazonov iSazonov mentioned this pull request Sep 20, 2018
Closed
8 tasks
SteveL-MSFT
SteveL-MSFT reviewed Sep 21, 2018
View reviewed changes
test/powershell/Modules/Microsoft.PowerShell.Utility/ConvertTo-Json.Tests.ps1 Outdated
$obj = [PSCustomObject]@{P1 = ''; P2 = ''; P3 = ''; P4 = ''; P5 = ''; P6 = ''}
$obj.P1 = $obj.P2 = $obj.P3 = $obj.P4 = $obj.P5 = $obj.P6 = $obj
1..100 | Foreach-Object { $obj } | ConvertTo-Json -Depth 10 -Verbose
1..100 | Foreach-Object { $obj } | ConvertTo-Json -Depth 10
Copy link
Member

@SteveL-MSFT SteveL-MSFT Sep 21, 2018

Choose a reason for hiding this comment

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

Surprised this test passed if you remove -verbose as line 35 is waiting on verbose output?

Copy link
Collaborator Author

@iSazonov iSazonov Sep 23, 2018

Choose a reason for hiding this comment

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

The waiting take 10 seconds until timeout.

Copy link
Member

@SteveL-MSFT SteveL-MSFT Sep 24, 2018

Choose a reason for hiding this comment

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

You are removing -Verbose because another PR removed the verbose message (which you added back with the test hook in the other PR), right? If so, I suppose this is ok, but it seems this change doesn't have any impact to this test so wondering why have this change in this PR at all?

Copy link
Collaborator Author

@iSazonov iSazonov Sep 25, 2018

Choose a reason for hiding this comment

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

Reverted the change.

SteveL-MSFT
SteveL-MSFT requested changes Sep 24, 2018
View reviewed changes
test/powershell/Modules/Microsoft.PowerShell.Utility/ConvertTo-Json.Tests.ps1 Outdated
$obj = [PSCustomObject]@{P1 = ''; P2 = ''; P3 = ''; P4 = ''; P5 = ''; P6 = ''}
$obj.P1 = $obj.P2 = $obj.P3 = $obj.P4 = $obj.P5 = $obj.P6 = $obj
1..100 | Foreach-Object { $obj } | ConvertTo-Json -Depth 10 -Verbose
1..100 | Foreach-Object { $obj } | ConvertTo-Json -Depth 10
Copy link
Member

@SteveL-MSFT SteveL-MSFT Sep 24, 2018

Choose a reason for hiding this comment

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

You are removing -Verbose because another PR removed the verbose message (which you added back with the test hook in the other PR), right? If so, I suppose this is ok, but it seems this change doesn't have any impact to this test so wondering why have this change in this PR at all?

@iSazonov
Copy link
Collaborator Author

iSazonov commented Sep 26, 2018

@daxian-dbw @dantraMSFT Could you please do (fast) review before we merge?

markekraus
markekraus approved these changes Oct 2, 2018
View reviewed changes
Copy link
Contributor

@markekraus markekraus left a comment

Choose a reason for hiding this comment

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

LGTM

@iSazonov iSazonov merged commit d862d40 into PowerShell:master Oct 2, 2018
@iSazonov iSazonov deleted the convertto-json-escapehandling branch October 2, 2018 05:54
@joeyaiello joeyaiello mentioned this pull request Oct 15, 2018
Closed
@joeyaiello joeyaiello removed Documentation Needed in this repo Documentation is needed in this repo labels Oct 15, 2018
adityapatwardhan pushed a commit to adityapatwardhan/PowerShell that referenced this pull request Apr 9, 2019
@iSazonov
Add EscapeHandling parameter in ConvertTo-Json cmdlet (PowerShell#7775)
a9a258e 
Add EscapeHandling parameter in ConvertTo-Json cmdlet to allow escape the HTML (<, >, &, ', ") and control characters (e.g. newline).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@TravisEz13 TravisEz13 TravisEz13 approved these changes

@SteveL-MSFT SteveL-MSFT SteveL-MSFT approved these changes

@dantraMSFT dantraMSFT Awaiting requested review from dantraMSFT

@JamesWTruher JamesWTruher Awaiting requested review from JamesWTruher

@PaulHigin PaulHigin Awaiting requested review from PaulHigin

@BrucePay BrucePay Awaiting requested review from BrucePay

@daxian-dbw daxian-dbw Awaiting requested review from daxian-dbw

+1 more reviewer

@markekraus markekraus markekraus approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@iSazonov iSazonov

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

ConvertTo-Json string escaped handling differs between PS and PSCore

6 participants

@iSazonov @PlagueHO @TravisEz13 @markekraus @SteveL-MSFT @joeyaiello