GeezerVison is showing github.com — Add configurable maximum depth in ConvertFrom-Json with `-Depth` by adamgauthier · Pull Request #8199 · PowerShell/PowerShell
Add configurable maximum depth in ConvertFrom-Json with -Depth#8199
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.
The reason will be displayed to describe this comment to others. Learn more.
Wondering if a custom type SerializerSettings to encapsulate default 1024 maxDepth and false returnHashtable here would be better than adding overloads.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
The reason will be displayed to describe this comment to others. Learn more.
I would consider the option to set -Depthless than0 for no maximum depth. 0 or $Null are often the result of a wrong calculation. Besides, 0 could technically still be a desired "depth".
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Besides, 0 could technically still be a desired "depth".
How so? Wouldn't that mean you want the deserialization to always fail? Is that really a use case? Also, the underlying Newtonsoft.Json does not allow 0 or less as MaxDepth, it throws at runtime.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
ConvertTo-Json : Cannot validate argument on parameter 'Depth'. The 0 argument is less than the minimum allowed range
of 1. Supply an argument that is greater than or equal to 1 and then try the command again.
At line:1 char:29
Anyways, wouldn't it be better to get either a completely flat output or an error in case of a (incorrect calculated/unassigned/mistyped variable) Depth of 0 or $Null?
I think a negative Depth would just be more knowingly chosen.
In other words, I do not really expect it to be a desired used case, but in the case of a scripting failure, I think a flat result or an error is more clear then a possible performance issue.
Just a thought, the decision is up to you...
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
The reason will be displayed to describe this comment to others. Learn more.
In the real world, I expect the default of 1024 to be sufficient, so I would be ok with not needing unlimited. -Depth 0 for only the top level makes sense and consistent with Get-ChildItem -Depth 0
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
The reason will be displayed to describe this comment to others. Learn more.
iRon7 after thinking it through, I agree a negative value might be better than 0.
Mark Kraus (@markekraus) I understand powershell needs to be secure by default but I think this is an opportunity to provide the option for people to just say "I know what I'm doing, get out of my way". Sure, you could have them type -Depth:([int]::MaxValue), but feels less convenient in my opinion and it would still have the overhead (however small) of the serializer repeatedly checking to make sure max depth isn't exceeded (not the case if you pass null).
What do you guys think of having -Depth -1 for unlimited and maybe throw an error on BeginProcessing() if -Depth is 0?
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
The reason will be displayed to describe this comment to others. Learn more.
It's not about security.. it's about absurdity. The only JSON objects that are that deep are contrived (made to be deep for fun or to specifically test depth limits) or broken (whatever produced the JSON had some issue). 1024 is already absurdly deep for a JSON object. We all ready catch outliers with the default setting. Between 1024 and Int32.MaxValue is a the realm of intrigue only. We don't need to have special case logic for that. We don't even really need infinite depth. What is the target audience for such a feature? Who really has objects that deep?
Just because we can, doesn't mean we should I still stand by my statement that 1 to int32.MaxValue is sufficient.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
adamgauthier and iSazonov reacted with thumbs up emoji
All reactions
👍2 reactions
}
/// <summary>
/// Convert a JSON string back to an object of type <see cref="System.Management.Automation.PSObject"/> or
/// <see cref="System.Collections.Hashtable"/> depending on parameter <paramref name="returnHashtable"/>.
/// </summary>
/// <param name="input">The JSON text to convert.</param>
/// <param name="returnHashtable">True if the result should be returned as a <see cref="System.Collections.Hashtable"/>
/// instead of a <see cref="System.Management.Automation.PSObject"/>.</param>
/// <param name="maxDepth">The max depth allowed when deserializing the json input. Set to null for no maximum.</param>
/// <param name="error">An error record if the conversion failed.</param>
/// <returns>A <see cref="System.Management.Automation.PSObject"/> or a <see cref="System.Collections.Hashtable"/>
/// if the <paramref name="returnHashtable"/> parameter is true.</returns>
[SuppressMessage("Microsoft.Naming", "CA1704:IdentifiersShouldBeSpelledCorrectly", Justification = "Preferring Json over JSON")]
public static object ConvertFromJson(string input, bool returnHashtable, int? maxDepth, out ErrorRecord error)
{
if (input == null)
{
Expand DownExpand Up
@@ -162,7 +179,7 @@ public static object ConvertFromJson(string input, bool returnHashtable, out Err
// This TypeNameHandling setting is required to be secure.
It 'Can convert an object with Newtonsoft.Json metadata properties with AsHashtable switch set to <AsHashtable>' -TestCase $testCasesWithAndWithoutAsHashtableSwitch {
It 'Can convert an object with Newtonsoft.Json metadata properties with AsHashtable switch set to <AsHashtable>' -TestCases $testCasesWithAndWithoutAsHashtableSwitch {
It 'Can convert an object of depth 1024 by default with AsHashtable switch set to <AsHashtable>' -TestCases $testCasesWithAndWithoutAsHashtableSwitch {
adamgauthier marked this conversation as resolved.
Resolved
Uh oh!
There was an error while loading. Please reload this page.
if ($AsHashtable)
{
$json | Should -BeOfType Hashtable
}
else
{
$json | Should -BeOfType PSCustomObject
}
}
It 'Fails to convert an object of depth higher than 1024 by default with AsHashtable switch set to <AsHashtable>' -TestCases $testCasesWithAndWithoutAsHashtableSwitch {
It 'Can convert an object with depth less than Depth param set to <Depth> and AsHashtable switch set to <AsHashtable>' -TestCases $testCasesJsonDepthWithAndWithoutAsHashtableSwitch {
(Count-ObjectDepth -InputObject $json) | Should -Be ($Depth - 1)
}
It 'Can convert an object with depth equal to Depth param set to <Depth> and AsHashtable switch set to <AsHashtable>' -TestCases $testCasesJsonDepthWithAndWithoutAsHashtableSwitch {
(Count-ObjectDepth -InputObject $json) | Should -Be $Depth
}
It 'Fails to convert an object with greater depth than Depth param set to <Depth> and AsHashtable switch set to <AsHashtable>' -TestCases $testCasesJsonDepthWithAndWithoutAsHashtableSwitch {
{ [Microsoft.PowerShell.Commands.JsonObject]::ConvertFromJson($str, $ReturnHashTable, [ref]$errRecord) } | Should -Throw -ErrorId "ArgumentException"
}
Context 'Empty key name' {
It 'Can Convert json with depth less than -Depth:<Depth> with -ReturnHashTable:<ReturnHashTable>' -TestCases $testCasesJsonDepthWithAndWithoutReturnHashTable {
It 'Can Convert json with depth equal to -Depth:<Depth> with -ReturnHashTable:<ReturnHashTable>' -TestCases $testCasesJsonDepthWithAndWithoutReturnHashTable {
It 'Throws ArgumentException for json with greater depth than -Depth:<Depth> with -ReturnHashTable:<ReturnHashTable>' -TestCases $testCasesJsonDepthWithAndWithoutReturnHashTable {
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 01402520 — 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.