DYN-7278: Fix dictionary preview by adding support for more types #15750
Conversation
github-actions
bot
changed the title
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
See the ticket for this pull request: https://jira.autodesk.com/browse/DYN-7278
UI Smoke TestsTest: success. 11 passed, 0 failed. |
| @@ -122,6 +135,28 @@ private WatchViewModel ProcessThing(object value, ProtoCore.RuntimeCore runtimeC | |||
| return new WatchViewModel(value, tag, RequestSelectGeometry); | |||
| } | |||
|
|
|||
| private static Dictionary<string, object> ConvertJObjectToDictionary(JObject jObject) | |||
There was a problem hiding this comment.
a couple thoughts.
- If I am understanding correctly, this is essentially arbitrary deserialization of json using the
ToObject()calls. It's not clear to me exactly what types this allows deserialization of.
I think the default deserialization calls use a secure serializer (TypeNameHandling = None), but I am not sure, it might be good to make it explicit by using a serializer here with settings that encode that specifically - for example:
Dynamo/src/DynamoCore/Models/DynamoModel.cs
Line 2158 in 3d8a262
IMO this kind of thing becomes more important now that we have to support DaaS - though I guess maybe this code is never even present on the service? (wpf?)
-
Where is this type coming from? The Forma nodes? Personally I feel we should ditch newtonsoft everywhere and instead use the built in system.text.json - it's harder to screw up security using it (though not impossible)
-
I have not thought about it much - but wouldn't it be nice if packages could provide their own watch handlers?
-
why are both dictionary and this newtonsoft parsing done in the object handler instead of defining more specific overloads - was there a performance reason for this?
There was a problem hiding this comment.
- These are
NewtonSoft.Json JObjecttypes that are coming mainly from Forma elements represented as JSON. As of now, I'm not sure what other nodes exist that output such types that have preview issues but so far, I'm seeing this issue only with the Forma nodes. TheJObjectseems to have a structure similar to dictionaries or key-value pairs so it just seems natural to be able to convert them to a form such that they can be previewed such as dictionaries. - Yes, to types coming from Forma. I'll see if I can switch the deserialization to use System.Text.Json instead but I think it would mean changing the DynamoForma package to use STJ as well. If that's the case, that would be out of scope for this fix.
- Not sure given that I think this is the first time we've encountered a case such as this where nodes in a package are having preview issues and therefore such cases seem to be rare but still open to discuss.
I'm not sure I understand. What exactly are you referring to byobject handlerandoverloads?
I think I see what you mean. No particular reason for doing it this way. I can add an overload forProcessThingto handleJObjectand maybe another for Dictionary, if that's what you're alluding to.
There was a problem hiding this comment.
These changes seem to be the first step in trying to support NewtonSoft types as native dynamo types.
If JObjects are passed around between nodes, do you foresee any other nodes/functionality that might not work with this type?
If we want to favor more system.text.json instead of Newtonsoft, I assume we'll encounter similar system.text.json types (like JsonElement and JSonDocument). Should we try to support these at the Proto level in the Marshaler code?
There was a problem hiding this comment.
I kind of like the packages could provide their own watch handlers? idea. Some packages might bring their own magic types, and would be nice if they could specify the way Dynamo presents the data.
There was a problem hiding this comment.
@pinzart90 these changes are only affecting the node previews for these types at the UI level and are not related to marshaling types so in that sense IMO these changes are safer. In order to support System.Text.Json, yes, I believe we'll need to add support for it here as well although I don't think anything specific would need to be done in the marshaler.
| case TypeCode.UInt32: | ||
| return nameof(TypeCode.UInt32); | ||
| case TypeCode.UInt64: | ||
| return nameof(TypeCode.UInt64); |
There was a problem hiding this comment.
I don't believe the type returned from GetDisplayType is actually being used currently so it might just be better to remove this function for the time being.
aparajit-pratap
changed the title
Yes, sorry for not making that clear. I intend to do that investigation separately outside this PR's scope. Thanks. |
Purpose
Fixes for previewing dictionaries containing
UInt32,UInt64,Byte, andNewtonSoft.Json.Linq.JObjecttypes. The latter is needed to support previewing Forma nodes currently as those are the output types generated for some nodes.Declarations
Check these if you believe they are true
*.resxfilesRelease Notes
Node preview fixes for dictionaries containing certain types.
Reviewers
(FILL ME IN) Reviewer 1 (If possible, assign the Reviewer for the PR)
(FILL ME IN, optional) Any additional notes to reviewers or testers.
FYIs
(FILL ME IN, Optional) Names of anyone else you wish to be notified of