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.
| @@ -272,7 +272,15 @@ private static string GetStringFromObject(object obj) | |||
| case TypeCode.DateTime: | |||
| return ((DateTime)obj).ToString(PreferenceSettings.DefaultDateFormat, CultureInfo.InvariantCulture); | |||
| case TypeCode.Object: | |||
| if (obj is byte[] byteArray) | |||
There was a problem hiding this comment.
can you add tests for the cases you are fixing?
There was a problem hiding this comment.
Yes, I was going to add tests, which is why I didn't call this out for review yet but thanks for the review all the same.
| 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 for function for the time being.
Purpose
(FILL ME IN) This section describes why this PR is here. Usually it would include a reference to the tracking task that it is part or all of the solution for.
Declarations
Check these if you believe they are true
*.resxfilesRelease Notes
(FILL ME IN) Brief description of the fix / enhancement. Use N/A to indicate that the changes in this pull request do not apply to Release Notes. Mandatory section
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