Skip to content

Conversation

@SteveL-MSFT
Copy link
Member

@SteveL-MSFT SteveL-MSFT commented Sep 25, 2020

PR Summary

When using ConvertTo-Json, the default -Depth value is 2 to avoid recursion for objects like Services on Windows. Users who do not set a deep enough depth may be losing data they may not be aware of as they expected full fidelity of the object to be serialized to JSON. The change agreed by @PowerShell/powershell-committee is to emit a warning message once the depth is exceeded. The warning is only emitted once per object serialized, but note that arrays are treated as a single object and not unrolled.

The SecureString tests were updated because it seems a credscan change identified a test "secret" in the script and caused CI to fail. After fixing that, I needed to have scriptanalyzer not fail on using plaintext "secret".

PR Context

Fix #8393

PR Checklist

@iSazonov
Copy link
Collaborator

@SteveL-MSFT If we will move to new .Net JSON API in next milestone (7.2 LTS!) does it make sense to change anything today in JSON cmdlets?

@SteveL-MSFT
Copy link
Member Author

@iSazonov this change doesn't impact functionality and addresses the concern that users aren't aware they aren't getting a full fidelity serialization of their object. I think it's ok to take this for 7.1 even if we plan to move to .NET JSON API in 7.2.

@SteveL-MSFT SteveL-MSFT added this to the 7.1-Consider milestone Sep 25, 2020
@iSazonov iSazonov merged commit 9bf512f into PowerShell:master Sep 28, 2020
@iSazonov iSazonov added the CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log label Sep 28, 2020
@SteveL-MSFT SteveL-MSFT deleted the json-depth branch September 28, 2020 16:29
@heilkn
Copy link

heilkn commented Sep 28, 2020

Thank you!

@ghost
Copy link

ghost commented Oct 21, 2020

🎉v7.1.0-rc.2 has been released which incorporates this pull request.:tada:

Handy links:

@ghost
Copy link

ghost commented Nov 17, 2020

🎉v7.2.0-preview.1 has been released which incorporates this pull request.:tada:

Handy links:

@adityapatwardhan
Copy link
Member

Not taking this to 7.0.x as it adds a new warning and is a change in behavior.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

BackPort-7.1.x-Done Backport to 7.1.x completed CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Consider removing the default -Depth value from ConvertTo-Json

5 participants