Skip to content
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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DYN-7278: Fix dictionary preview by adding support for more types #15750

Merged
merged 9 commits into from
Jan 14, 2025
Merged

DYN-7278: Fix dictionary preview by adding support for more types #15750

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
ec9ef14
fix dictionary preview by adding support for more types
aparajit-pratap Jan 10, 2025
ab6bad8
revert assemblyinfo
aparajit-pratap Jan 10, 2025
5fb2728
process json object for preview
aparajit-pratap Jan 10, 2025
4569a81
cleanup
aparajit-pratap Jan 10, 2025
432af98
add tests for byte and uint previews
aparajit-pratap Jan 14, 2025
688332e
add test for JSON preview
aparajit-pratap Jan 14, 2025
836f3f9
remove bad test
aparajit-pratap Jan 14, 2025
daa15f2
rename nodes
aparajit-pratap Jan 14, 2025
28b4f3d
review comments
aparajit-pratap Jan 14, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 37 additions & 2 deletions src/DynamoCoreWpf/Interfaces/IWatchHandler.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,11 @@
using System;

using System;
using System.Collections;
using System.Collections.Generic;
using System.Globalization;
using System.Linq;
using Dynamo.Configuration;
using Newtonsoft.Json.Linq;

using Dynamo.Extensions;
using Dynamo.ViewModels;
using Dynamo.Wpf.Properties;
Expand Down Expand Up @@ -83,7 +85,18 @@ private WatchViewModel ProcessThing(object value, ProtoCore.RuntimeCore runtimeC

return node;
}
if (value is JObject obj)
{
var dict = ConvertJObjectToDictionary(obj);
var node = new WatchViewModel(dict.Keys.Any() ? WatchViewModel.DICTIONARY : WatchViewModel.EMPTY_DICTIONARY, tag, RequestSelectGeometry, true);

foreach (var e in dict.Keys.Zip(dict.Values, (key, val) => new { key, val }))
{
node.Children.Add(ProcessThing(e.val, runtimeCore, tag + ":" + e.key, showRawData, callback));
}

return node;
}
if (!(value is string) && value is IEnumerable)
{
var list = (value as IEnumerable).Cast<dynamic>().ToList();
Expand Down Expand Up @@ -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)
{
Copy link
Member

@mjkkirschner mjkkirschner Jan 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a couple thoughts.

  1. 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

TypeNameHandling = TypeNameHandling.None,

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?)

  1. 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)

  2. I have not thought about it much - but wouldn't it be nice if packages could provide their own watch handlers?

  3. 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?

Copy link
Contributor Author

@aparajit-pratap aparajit-pratap Jan 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. These are NewtonSoft.Json JObject types 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. The JObject seems 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.
  2. 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.
  3. 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.
  4. I'm not sure I understand. What exactly are you referring to by object handler and overloads?
    I think I see what you mean. No particular reason for doing it this way. I can add an overload for ProcessThing to handle JObject and maybe another for Dictionary, if that's what you're alluding to.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

var dictionary = new Dictionary<string, object>();

foreach (var property in jObject.Properties())
{
if (property.Value is JObject nestedObject)
{
dictionary[property.Name] = ConvertJObjectToDictionary(nestedObject);
}
else if (property.Value is JArray nestedArray)
{
dictionary[property.Name] = nestedArray.ToObject<List<object>>();
}
else
{
dictionary[property.Name] = property.Value.ToObject<object>();
}
}
return dictionary;
}

private WatchViewModel ProcessThing(double value, ProtoCore.RuntimeCore runtimeCore, string tag, bool showRawData, WatchHandlerCallback callback)
{
return new WatchViewModel(value, tag, RequestSelectGeometry);
Expand Down
14 changes: 14 additions & 0 deletions src/DynamoCoreWpf/ViewModels/Preview/WatchViewModel.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
aparajit-pratap marked this conversation as resolved.
Show resolved Hide resolved
return Encoding.UTF8.GetString(byteArray);
return ObjectToLabelString(obj);
case TypeCode.Byte:
return ((byte)obj).ToString(CultureInfo.InvariantCulture);
case TypeCode.UInt32:
return ((uint)obj).ToString(CultureInfo.InvariantCulture);
case TypeCode.UInt64:
return ((ulong)obj).ToString(CultureInfo.InvariantCulture);
default:
return (string)obj;
};
Expand Down Expand Up @@ -313,6 +321,12 @@ private string GetDisplayType(object obj)
return nameof(TypeCode.Object);
case TypeCode.String:
return nameof(TypeCode.String);
case TypeCode.Byte:
return nameof(TypeCode.Byte);
case TypeCode.UInt32:
return nameof(TypeCode.UInt32);
case TypeCode.UInt64:
return nameof(TypeCode.UInt64);
Copy link
Contributor Author

@aparajit-pratap aparajit-pratap Jan 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

case TypeCode.Empty:
return String.Empty;
default:
Expand Down
Loading