Skip to content

Refactor ConvertTo-Json to expose JsonObject.ConvertToJson as a public API #8682

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
TravisEz13 merged 4 commits into from
Jan 23, 2019
Merged

Refactor ConvertTo-Json to expose JsonObject.ConvertToJson as a public API #8682

TravisEz13 merged 4 commits into from
Jan 23, 2019

Conversation

@daxian-dbw
Copy link
Member

@daxian-dbw daxian-dbw commented Jan 18, 2019 •
edited by adityapatwardhan
Loading

Fixes #8123

PR Summary

We have the public API JsonObject.ConvertFromJson to convert from JSON string in the PowerShell context. It would be good to have a public API for conversion to JSON. This PR refactors the ConvertTo-Json cmdlet to move the core implementation to JsonObject.ConvertToJson, and make ConvertTo-Json call that public method.

This would help the Azure Function PowerShell worker. Currently, we depends on calling the cmdlet to convert object to JSON which is expensive. Once we have the public method JsonObject.ConvertToJson exposed, we can call the API directly to avoid a command invocation.

PR Context

PR Checklist

@daxian-dbw daxian-dbw added the CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log label Jan 18, 2019
@iSazonov iSazonov requested a review from markekraus January 18, 2019 07:55
@iSazonov
Copy link
Collaborator

iSazonov commented Jan 18, 2019

@daxian-dbw I see most of code is simply copy-pasted. It seems old code is not ideal - should we review line by line in depth or only new API?

@daxian-dbw
Copy link
Member Author

daxian-dbw commented Jan 18, 2019 •
edited
Loading

@iSazonov This PR is purely a change on how code is organized, in order to expose the convert-to-json functionality through a public API similar to the existing JsonObject.ConvertFromJson. Updating old code to make it better/faster is not a goal.

When reviewing the changes, I suggest to focus on the new APIs, how ConvertToJsonCommand is changed to depend on JsonObject, and how ConvertToJsonContext is used within JsonObject. Thanks!

iSazonov
iSazonov reviewed Jan 18, 2019
View reviewed changes
src/Microsoft.PowerShell.Commands.Utility/commands/utility/WebCmdlet/ConvertToJsonCommand.cs
targetCmdlet: this);

string output = JsonObject.ConvertToJson(objectToProcess, in context);
if (output != null)
Copy link
Collaborator

@iSazonov iSazonov Jan 18, 2019

Choose a reason for hiding this comment

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

Do we still need to check this?

Copy link
Member Author

@daxian-dbw daxian-dbw Jan 18, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I believe so. In case ctrl+c is signaled, JsonObject.ConvertToJson would return null, and I think we don't want to write that null to pipeline.
I know that when the pipeline is stopping, WriteObject will throw from MshCommandRuntime._WriteObjectSkipAllowCheck, so technically it might be OK to remove this check.
But I want to play safe here. What do you think?

For comparison, please take a look at the use of Stopping in the original code path where a StopException is thrown which results in return from EndProcessing.

Copy link
Collaborator

@iSazonov iSazonov Jan 18, 2019

Choose a reason for hiding this comment

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

I see that new code keep previous behavior and my thoughts was "is it useful to return null?" like $null | ConvertTo-Json | Should -Be $null

Copy link
Member Author

@daxian-dbw daxian-dbw Jan 18, 2019

Choose a reason for hiding this comment

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

Well, if we want to change that behavior, we should change the code here. JsonObject.ConvertToJson returns null only if StopException is thrown, and it has nothing to do with enabling $null | ConvertTo-Json.

Yeah, $null | ConvertTo-Json seems like a reasonable scenario, but the test should be $null | ConvertTo-Json | Should -Be "null". It's better to discuss this in a separate issue. Can you open an issue?

PS:30> [Newtonsoft.Json.JsonConvert]::SerializeObject($null) -eq "null"
True

Copy link
Collaborator

@iSazonov iSazonov Jan 18, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Can you open an issue?

I think @markekraus could comment. If he will confirm that the scenario is useful we'll open new issue.

Copy link
Contributor

@markekraus markekraus Jan 18, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

You never want ConvertTo-Json write $null to the pipeline. But looking at this, the only scenario that would do that is a stopping exception. I'm not sure if there is any edge case where JsonConvert.SerializeObejct() would return a null string object, nothing I have tried to create such a scenario works anyway. It may be safe to remove it, but, I think it's safer to leave it.

Copy link
Collaborator

@iSazonov iSazonov Jan 19, 2019

Choose a reason for hiding this comment

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

@markekraus Thanks!

@daxian-dbw I think we need a test for $null | ConvertTo-Json.

Copy link
Member Author

@daxian-dbw daxian-dbw Jan 20, 2019

Choose a reason for hiding this comment

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

$null | ConvertTo-Json is not supported today -- the passed-in $null is ignored. Whether or not we should change the current behavior to support it should be a separate issue.

src/Microsoft.PowerShell.Commands.Utility/commands/utility/WebCmdlet/ConvertToJsonCommand.cs
return obj;

return dict;
cancellationSource.Cancel();
Copy link
Collaborator

@iSazonov iSazonov Jan 18, 2019

Choose a reason for hiding this comment

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

Should we add tests for this?
(I'd not want.)

Copy link
Member Author

@daxian-dbw daxian-dbw Jan 18, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

It would be difficult to mimic 'ctrl+c' in the test. I have manually tested it though :)

Copy link
Collaborator

@iSazonov iSazonov Jan 18, 2019

Choose a reason for hiding this comment

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

@SteveL-MSFT added such tests previously and they looks tricky. I don't like this but should ask :-)

Copy link
Contributor

@xtqqczze xtqqczze Jan 19, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

There is already a test for StopProcessing in test\powershell\Modules\Microsoft.PowerShell.Utility\ConvertTo-Json.Tests.ps1. It is not currently working perfectly, see #8645.

Copy link
Contributor

@xtqqczze xtqqczze Jan 19, 2019

Choose a reason for hiding this comment

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

See also: #8593

Copy link
Member Author

@daxian-dbw daxian-dbw Jan 21, 2019

Choose a reason for hiding this comment

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

Thanks @xtqqczze , I will take a look at those two PRs.

@TravisEz13 TravisEz13 added Review - Committee The PR/Issue needs a review from the PowerShell Committee 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 Jan 23, 2019
@TravisEz13 TravisEz13 merged commit c606b1c into PowerShell:master Jan 23, 2019
@daxian-dbw daxian-dbw deleted the jsonobj branch January 23, 2019 19:41
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

@iSazonov iSazonov iSazonov approved these changes

@TylerLeonhardt TylerLeonhardt Awaiting requested review from TylerLeonhardt

@SteveL-MSFT SteveL-MSFT Awaiting requested review from SteveL-MSFT

@JamesWTruher JamesWTruher Awaiting requested review from JamesWTruher

@PaulHigin PaulHigin Awaiting requested review from PaulHigin

@markekraus markekraus Awaiting requested review from markekraus

@adityapatwardhan adityapatwardhan Awaiting requested review from adityapatwardhan

@anmenaga anmenaga Awaiting requested review from anmenaga

+1 more reviewer

@xtqqczze xtqqczze xtqqczze left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log Committee-Reviewed PS-Committee has reviewed this and made a decision

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Expose ConvertToJson as an API

5 participants

@daxian-dbw @iSazonov @markekraus @TravisEz13 @xtqqczze