DYN-6468 Add Guard for circular references that can impact Stringify.JSON and Remember node #14651
Conversation
| } | ||
| }; | ||
|
|
||
| StringBuilder sb = new StringBuilder(256); |
There was a problem hiding this comment.
why was this capacity chosen?
There was a problem hiding this comment.
I just copied what was in the internal implementation for Json.Net for JsonConvert.SerializeObject which looks like the new code but without the overridden JsonTextWriter
| }; | ||
|
|
||
| StringBuilder sb = new StringBuilder(256); | ||
| using (var writer = new StringWriter(sb, CultureInfo.InvariantCulture)) |
There was a problem hiding this comment.
does json.net usually use invariant culture to serialize strings?
There was a problem hiding this comment.
Apparently... Again just duplicated the implementation.
| private readonly int maxDepth = 15; | ||
| private int depth = 0; | ||
|
|
||
| public MaxDepthJsonTextWriter(TextWriter writer) : base(writer) { } |
There was a problem hiding this comment.
maybe allow setting the depth in the constructor?
There was a problem hiding this comment.
I had it that way but simplified it since we aren't using this anywhere else currently. Right now one place to set it.
|
regression test would be good. |
Purpose
In testing the Remember node we found that some classes of NET objects can cause Dynamo Crashes when Stringify.JSON tries to serialize the objects to JSON via json.net. The main example cases were some types of Revit objects. While it is expected that these objects should not serialize to json. They should fail with a warning on the node and not a hard crash.
Todo
Declarations
Check these if you believe they are true
*.resxfilesRelease Notes
(FILL ME IN) Brief description of the fix / enhancement. Mandatory section
Reviewers
@mjkkirschner
FYIs
@jnealb