GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Refactor ConvertTo-Json to expose `JsonObject.ConvertToJson` as a public API by daxian-dbw · Pull Request #8682 · PowerShell/PowerShell

Conversation

@daxian-dbw

@daxian-dbw Dongbo Wang (daxian-dbw) commented Jan 18, 2019 •
edited by adityapatwardhan
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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

  • PR has a meaningful title
    • Use the present tense and imperative mood when describing your changes
  • Summarized changes
  • Change is not breaking
  • Make sure all .h, .cpp, .cs, .ps1 and .psm1 files have the correct copyright header
  • This PR is ready to merge and is not Work in Progress.
    • If the PR is work in progress, please add the prefix WIP: or [ WIP ] to the beginning of the title (the WIP bot will keep its status check at Pending while the prefix is present) and remove the prefix when the PR is ready.
  • User-facing changes
    • Not Applicable
    • OR
    • Documentation needed
      • Issue filed:
  • Testing - New and feature
    • N/A or can only be tested interactively
    • OR
    • Make sure you've added a new test if existing tests do not effectively test the code changed
      • Add [feature] to your commit messages if the change is significant or affects feature tests

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@daxian-dbw Dongbo Wang (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
@daxian-dbw
Dongbo Wang (daxian-dbw) requested review from Steve Lee (SteveL-MSFT), TylerLeonhardt and Ilya (iSazonov) January 18, 2019 07:46
@daxian-dbw
Dongbo Wang (daxian-dbw) requested review from James Truher (JamesWTruher) and Paul Higinbotham (PaulHigin) as code owners January 18, 2019 07:46
@daxian-dbw
[Feature] Refactor ConvertTo-Json to expose JsonObject.ConvertToJson …
7df2a22 
…as public API
@daxian-dbw
Dongbo Wang (daxian-dbw) force-pushed the jsonobj branch from 06de2f4 to 7df2a22 Compare January 18, 2019 07:47
@iSazonov
Ilya (iSazonov) requested a review from Mark Kraus (markekraus) January 18, 2019 07:55
@iSazonov

Ilya (iSazonov) commented Jan 18, 2019

Copy link
Copy Markdown
Collaborator

Dongbo Wang (@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?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@daxian-dbw

Dongbo Wang (daxian-dbw) commented Jan 18, 2019 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

Ilya (@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 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
iSazonov
Ilya (iSazonov) reviewed Jan 18, 2019
View reviewed changes
Comment thread src/Microsoft.PowerShell.Commands.Utility/commands/utility/WebCmdlet/ConvertToJsonCommand.cs Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread src/Microsoft.PowerShell.Commands.Utility/commands/utility/WebCmdlet/ConvertToJsonCommand.cs
targetCmdlet: this);

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

@iSazonov Ilya (iSazonov) Jan 18, 2019

Copy link
Copy Markdown
Collaborator

There was a problem hiding this comment.

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@daxian-dbw Dongbo Wang (daxian-dbw) Jan 18, 2019 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@iSazonov Ilya (iSazonov) Jan 18, 2019

Copy link
Copy Markdown
Collaborator

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@daxian-dbw Dongbo Wang (daxian-dbw) Jan 18, 2019

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@iSazonov Ilya (iSazonov) Jan 18, 2019 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Collaborator

There was a problem hiding this comment.

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 Mark Kraus (@markekraus) could comment. If he will confirm that the scenario is useful we'll open new issue.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@markekraus Mark Kraus (markekraus) Jan 18, 2019 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
daxian-dbw and iSazonov reacted with thumbs up emoji
All reactions
  • 👍 2 reactions

@iSazonov Ilya (iSazonov) Jan 19, 2019

Copy link
Copy Markdown
Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Mark Kraus (@markekraus) Thanks!

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@daxian-dbw Dongbo Wang (daxian-dbw) Jan 20, 2019

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread src/Microsoft.PowerShell.Commands.Utility/commands/utility/WebCmdlet/ConvertToJsonCommand.cs
return obj;

return dict;
cancellationSource.Cancel();

@iSazonov Ilya (iSazonov) Jan 18, 2019

Copy link
Copy Markdown
Collaborator

There was a problem hiding this comment.

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.)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@daxian-dbw Dongbo Wang (daxian-dbw) Jan 18, 2019 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

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 :)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@iSazonov Ilya (iSazonov) Jan 18, 2019

Copy link
Copy Markdown
Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@xtqqczze xtqqczze Jan 19, 2019 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@xtqqczze xtqqczze Jan 19, 2019

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

See also: #8593

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@daxian-dbw Dongbo Wang (daxian-dbw) Jan 21, 2019

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread src/Microsoft.PowerShell.Commands.Utility/commands/utility/WebCmdlet/ConvertToJsonCommand.cs Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread src/Microsoft.PowerShell.Commands.Utility/commands/utility/WebCmdlet/JsonObject.cs Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread src/Microsoft.PowerShell.Commands.Utility/commands/utility/WebCmdlet/JsonObject.cs Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread src/Microsoft.PowerShell.Commands.Utility/commands/utility/WebCmdlet/JsonObject.cs Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread src/Microsoft.PowerShell.Commands.Utility/commands/utility/WebCmdlet/JsonObject.cs Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread src/Microsoft.PowerShell.Commands.Utility/commands/utility/WebCmdlet/JsonObject.cs Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread src/Microsoft.PowerShell.Commands.Utility/commands/utility/WebCmdlet/JsonObject.cs Resolved

Uh oh!

There was an error while loading. Please reload this page.
Dongbo Wang (daxian-dbw) added 2 commits January 18, 2019 12:41
@daxian-dbw
[Feature] Address Ilya's feedback
e18198c
@daxian-dbw
[Feature] Change order of parameters and add xUnit tests for the API
d7264a1
@daxian-dbw
Dongbo Wang (daxian-dbw) requested review from Travis Plunk (TravisEz13), Aditya Patwardhan (adityapatwardhan) and Andrew (anmenaga) as code owners January 21, 2019 02:29
iSazonov
Ilya (iSazonov) approved these changes Jan 21, 2019
View reviewed changes
TravisEz13
Travis Plunk (TravisEz13) approved these changes Jan 22, 2019
View reviewed changes
TravisEz13
Travis Plunk (TravisEz13) reviewed Jan 22, 2019
View reviewed changes
Comment thread src/Microsoft.PowerShell.Commands.Utility/commands/utility/WebCmdlet/JsonObject.cs Resolved

Uh oh!

There was an error while loading. Please reload this page.
@daxian-dbw
Add comment per Travis' ask
50208a9
@TravisEz13 Travis Plunk (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
Travis Plunk (TravisEz13) merged commit c606b1c into PowerShell:master Jan 23, 2019
@daxian-dbw
Dongbo Wang (daxian-dbw) deleted the jsonobj branch January 23, 2019 19:41
Thatgfsj (Thatgfsj) pushed a commit to Thatgfsj-contribs/PowerShell that referenced this pull request Aug 6, 2026
@daxian-dbw @TravisEz13
Refactor ConvertTo-Json to expose JsonObject.ConvertToJson as a pub…
c749dcf 
…lic API (PowerShell#8682)

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](https://github.com/Azure/azure-functions-powershell-worker/blob/729710d2595b4b96abd4fa5a5547fe880e98bfa6/src/PowerShell/PowerShellManager.cs#L198-L205) 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.
Thatgfsj (Thatgfsj) pushed a commit to Thatgfsj-contribs/PowerShell that referenced this pull request Aug 6, 2026
@daxian-dbw @TravisEz13
Refactor ConvertTo-Json to expose JsonObject.ConvertToJson as a pub…
7a56b3f 
…lic API (PowerShell#8682)

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](https://github.com/Azure/azure-functions-powershell-worker/blob/729710d2595b4b96abd4fa5a5547fe880e98bfa6/src/PowerShell/PowerShellManager.cs#L198-L205) 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.

# Conflicts:
#	test/Test.Common.props
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@TravisEz13 Travis Plunk (TravisEz13) TravisEz13 approved these changes
@iSazonov Ilya (iSazonov) iSazonov approved these changes
@TylerLeonhardt TylerLeonhardt Awaiting requested review from TylerLeonhardt
@SteveL-MSFT Steve Lee (SteveL-MSFT) Awaiting requested review from SteveL-MSFT
@JamesWTruher James Truher (JamesWTruher) Awaiting requested review from JamesWTruher
@PaulHigin Paul Higinbotham (PaulHigin) Awaiting requested review from PaulHigin
@markekraus Mark Kraus (markekraus) Awaiting requested review from markekraus
@adityapatwardhan Aditya Patwardhan (adityapatwardhan) Awaiting requested review from adityapatwardhan
@anmenaga Andrew (anmenaga) Awaiting requested review from anmenaga
+1 more reviewer
@xtqqczze xtqqczze xtqqczze left review comments
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

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

Uh oh!

There was an error while loading. Please reload this page.

5 participants

@daxian-dbw @iSazonov @markekraus @TravisEz13 @xtqqczze
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.