Add configurable maximum depth in ConvertFrom-Json with -Depth - #8199

@PowerShell/powershell-committee reviewed this, the design should be that there is a -Depth parameter (which aligns with ConvertTo-Json) which defaults to 1024 and the user can change as needed. This is to preserve existing behavior.

Steve Lee (@SteveL-MSFT) with all due respect, may I ask to reconsider this decision? The -Depth parameter in ConvertTo-Json does not have the same behavior as the -MaxDepth I implemented here:

-Depth in ConvertTo-Json will quietly suppress conversion after the depth level specified (2 by default), for example:

PS C:\> ConvertTo-Json (ConvertFrom-Json '{"1":{"2":{"3":{"4":{"5":1}}}}}')
{
  "1": {
    "2": {
      "3": "@{4=}"
    }
  }
}

-MaxDepth as implemented here strictly fails the conversion if the depth of the json exceeds the value, the same way ConvertFrom-Json currently fails if the depth is higher than 1024 and ConvertTo-Json currently fails if the depth is higher than 100. With this in mind, -MaxDepth could also be implemented for ConvertTo-Json and is not equivalent to its -Depth parameter.

Furthermore, I believe the current -Depth parameter of ConvertTo-Json is not intuitive and has caused a lot of confusion for users in the past (there are lots of threads about it on StackOverflow, for example). What json serializer do you expect to stop serializing at third depth level by default? This leads to the user having to know beforehand what depth his input has in order for conversion to work properly (an unrealistic thing to expect). I don't think we should model new parameters aligning with it.

I also think putting 1024 as the default is a bad idea. This would make it so there is no way to say "I don't want a maximum depth" (which is the most common scenario). It would also lead to the same problem ConvertTo-Json currently has: we expect the user to know beforehand what is the depth of his input.

I know it would be a breaking change to have no maximum be the new default, but is it really a common scenario that someone is relying on the default behavior failing with input of depth higher than 1024? I think handling higher depth with this new parameter is the perfect opportunity to change the default behavior to something more intuitive: the cmdlets should handle whatever is thrown at them, if the user expects the input to cause a DoS in their system (which I assume was the reason for this default max depth), then they should specify a maximum depth optionally.

So we should discuss:

• Depth vs MaxDepth
• Defaults
• Nullable type for the parameter

My thoughts:

• Depth name looks good to be consistent with ConvertTo-Json.
• Current default is 2 in ConvertTo-Json. What is history why we use the value? Seems for ConvertFrom-Json the default should be larger. And defaults for both cmdlets should be the same for better UX: $source | ConvertFrom-Json | ConvertTo-Json is expected to return $source.
• I believe nullable type has no sense in scripts.

/cc Mark Kraus (@markekraus) Michael Klement (@mklement0) What do you think?

Adam Gauthier (@louistio) to be clear, the decision regarding -Depth is simply the name of the parameter not the semantics. At the time of discussion, we were not aware of the limitation of "maxdepth" would fail the entire operation rather than serialize up to 1024. This (to me) seems broken, but would be a breaking change. In light of that information, it's worth discussing again the the committee to see if there should be two parameters (which would be confusing to the user) or accept a breaking change at the limit.

It would also be useful if you can provide examples where the current limit of 1024 is insufficient.

Steve Lee (@SteveL-MSFT) Thank you! To be clear the naming is not a big concern to me, it's more about the bad user experience of having a default that I described later in my post. 😄

@PowerShell/powershell-committee had a very bikeshed conversation about this, and we understand the parallel concepts argument with this rough visualization:

ConvertTo-Json          -Depth                               (MaxDepth is internal as 100)
                   (produces partial)
ConvertFrom-Json        [-Depth]                             (MaxDepth is internal as 1024 and settable)
   (doesn't make sense because we don't have types)          (could be called -MaxDepth, could be called -Depth for PS consistency)

Does anyone else actually care?

Two cents:

• The MaxDepth internal value in ConvertTo-Json must have a low default because PowerShell must be secure by default -- omitting a parameter shouldn't open up a DoS attack, but setting it to a high (or negative - to turn off) value => you're on your own
• MaxDepth makes sense as a settable parameter that a user can specify to avoid errors with large objects
• Exposing that, it should be symmetric with anything on ConvertFrom-Json where symmetry makes sense

Mark Kraus (@markekraus) Michael Klement (@mklement0) Have you any thoughts about consistency of Depth parameters?

I agree with Rob Holt (@rjmholt)

Joey Aiello (@joeyaiello) Steve Lee (@SteveL-MSFT) Seems we can make final conclusion and continue the PR.

In terms of naming, -Depth is currently consistently being used to denote a maximum recursion depth.

Regrettably, exceeding that maximum depth doesn't cause an error in ConvertTo-Json, but results in near-useless .psobject.ToString() serialization of property values that exceed the depth.
In combination with the low default -Depth value of 2, that makes for frequent user frustration, as Adam Gauthier (@louistio) notes - the current ConvertTo-Json behavior frequently amounts to quiet de-facto failure.

In a utopian world, unencumbered by backward-compatibility concerns:

• Having a high-enough default that accommodates most use cases, yet prevents DoS attacks, as Rob Holt (@rjmholt) suggests, makes sense.

• Exceeding that default must fail.

• You should be able to opt out of the default fail-if-exceeded limit with an explicit -MaxDepth value, again with failure if exceeded; to avoid the awkwardness of -MaxDepth ([int]::MaxValue) or -MaxDepth -1, an additional -NoMaxDepth switch could be considered, if really needed.

• You should be able to intentionally cut off recursion with -Depth, similar to how Get-ChildItem uses the parameter: while there may be additional levels, you choose to ignore them.

Rob Holt (@rjmholt) Fair point about powershell being secure by default.

Michael Klement (@mklement0) I agree with everything you're saying, I think you basically put into words what I was struggling to convey. I like the idea of another switch for no -NoMaxDepth, more elegant than another invalid value!

I would like to implement an equivalent to -MaxDepth for ConvertTo-Json too, to allow users to change the current default 100 value. If we're naming this parameter -Depth, it would conflict with the current ConvertTo-Json parameter, unless the team decides to break its backwards compatibility and make it explicitly fail from now on. In my opinion this is the best option because the current behavior of that parameter is a pain for powershell users for the reasons I explained above but I understand the backward compatibility concerns.

Adam Gauthier (@louistio) What would be the difference between -Depth and -MaxDepth on ConvertTo-Json

Mark Kraus (@markekraus) As mentioned above, the behavior difference resides in the cmdlet failing on input exceeding the maximum depth. -Depth silently suppresses exceeding depth (soft) and -MaxDepth fails the execution (hard).

Currently ConvertTo-Json has a default soft depth of 2 (configurable with -Depth) and default hard max depth of 100 (non configurable). ConvertFrom-Json has no concept of soft max depth and a default hard max depth of 1024 (non configurable - what this PR adds).

What I'm saying is I'd be fine with changing ConvertTo-Json's -Depth into a hard max depth but I know this is compatibility breaking.

I think it would be confusing to have both Depth and Max Depth. We should have one for the depth and one to adjust the behavior of that depth.

True, having both -MaxDepth and -Depth could be confusing; on further reflection, all we need is
-Depth:

-Depth can be the opt-in, I-know-that-many-levels-is-all-I-need-or-I-really-want-that-many-levels parameter.

(What is conceptually) -MaxDepth needn't be user-facing (but needs to be documented): it can a fixed, documented limit that is chosen high enough to (a) accommodate most use cases while (b) preventing DoS attacks - and exceeding that limit causes an error.

-Depth then does double duty:

• You use it for data whose depth falls below the documented, fixed max. depth, in case you intentionally want to cut it off at the specified depth.

• You also use it for data that you do want whose depth happens to exceed the internal (but documented) max. depth that you do want to serialize/deserialize in full nonetheless.

I personally then don't see a need for any other parameters - I think opting into levels beyond the fixed limit with an explicit number or even with -Depth ([int]::MaxValue) (without the need for an abstract -NoMaxDepth switch) strikes me as acceptable, because I presume this to be a rare use case.

In the absence of using -Depth in a given command:

• You either live happily ever after, given that most commands won't hit the fixed limit.

• You are alerted to a problem through the error that is reported if your data unexpectedly exceeds the fixed limit (in which case you'll need to use -Depth, if using that many levels is really your intent).

As for required changes and backward compatibility:

• Remove the default -Depth of 2 from ConvertTo-Json (from what I understand, it already has a fixed, internal limit of 100 beyond which it fails).

• Document the fixed limits.

• The only way in which existing code could be impacted is in that payloads generated with ConvertTo-Json could now increase, if users previously implied on the implicit cut-off at depth 2 - that strikes me as a Bucket 3: Unlikely Grey Area change.

There are 2 things: 1) The maximum depth you want to convert and 2) the behavior of that conversion. They both need to be configurable, IMO and just relying on a single param for both would be a pain.

There are a very large number of objects passed to ConvertTo-Json where only having a max depth that errors would make them impossible objects to convert. We need the ability to squash. This includes object types that have parent child mappings. There would no way to convert those without squashing.

Consider the Current behavior:

I would ultimately like to see it as this:

Ideally we would would want a -Depth param that sets the max depth and an -Squash ( or something). we still need default depths (2 for ConvertTo-Json and 1024 for ConverFrom-Json). it would be nice to add the ability to change the behavior of ConvertTo-Json, but that's a bit out of scope for this PR. It would also be nice to be able to squash with ConvertFrom-Json but that i also a bit out of scope for this PR (or maybe it is easier than I think?).

But, keeping in mind that we could add parameters to control the behavior later, for this PR all we need is to add -Depth to ConvertFrom-Json with a default of 1024, of type Int, with a ValidateRange of 1 to Int32.MaxValue. I don't see a pressing need to change the behavior of ConvertTo-Json in the same PR, but if it was I would recommend changing the internal max depth from 100 to Int.MaxValue (Keeping Depth at a default of 2). That way we are safe from DoS by default, but let the user hold their fate in their own hands should they wish to try an absurdly deep object.

A bit of flavor text: A JSON object over 1024 in depth is an outlier. Other than abused/broken APIs or contrived examples, I have rarely seen a legit JSON objet that is deeper than ~30. On the flip-side, infinitely deep objects are very commonly thrown at ConvertTo-Json. (Based on my experience, YMMV.)

I don't see a pressing need to change the behavior of ConvertTo-Json in the same PR, but if it was I would recommend

I think it is better to make both changes for Depth in the PR to get clean history and continue with rest ideas in follow PRs.

Thanks for the analysis, Mark Kraus (@markekraus), but I still think -Depth is enough, for both ConvertTo-Json and ConvertFrom-Json:

• In the absence of -Depth, exceeding the documented, hard-coded max. depth will result in an error.

• Use of -Depth never results in an error, and is used intentionally to:

  • truncate the object tree at the specified level (whether or not that level is below or above the hard-coded max. depth)
  • opt-into object trees that are deeper than the hard-coded max. depth

To put it differently: the hard-coded max. depth is then no longer a default value for -Depth, it is a built-in safety mechanism that can be overridden with -Depth, if needed (which, with reasonably high defaults, will rarely be necessary).

If you have a "runaway" tree of infinite depth, then the hard-coded max. will save you.
If you actually do need more depth, you can opt in with -Depth.

@PowerShell/powershell-committee appreciates the discussion and we still suggest that we have the single -Depth defaulting to 1024 on ConvertFrom-Json to preserve existing behavior and not have any changes to erroring out vs truncation as it seems that there is no practical reason to force symmetry between the two while incurring a breaking change.

Mark Kraus (@markekraus) Michael Klement (@mklement0) Steve Lee (@SteveL-MSFT) Please update your review.

Adam Gauthier (@louistio) Please fix StyleCop issues.

Ilya (@iSazonov) Hey! Sorry, I wasn't aware of the colon convention 😅 I applied your suggestions through GitHub's integrated tool, but there seems to be error when running the tests without the colons! I get errors like:

ConvertFrom-Json : The input object cannot be bound to any parameters for the command either because the command does not take pipeline input or the input and its properties do not match any of the parameters that take pipeline input.

I'm not super familiar as to why removing colons would cause this, could you point me into the right direction? Thanks!

Adam Gauthier (@louistio) Sorry, colons is needed for asHashtable but not for Depth.

Steve Lee (@SteveL-MSFT) Mark Kraus (@markekraus) Please update your review.

Steve Lee (@SteveL-MSFT) Please update your review.

Adam Gauthier (@louistio) Please update the PR description and we'll merge.

Ilya (@iSazonov) description updated, I believe this still requires documentation changes, not sure if that prevents from merging.

Adam Gauthier (@louistio) Please open new Issue in PowerShell-Docs repo and add a reference to the PR description.

Ilya (@iSazonov) done!

Adam Gauthier (@louistio) Thanks for great work!