Add DynamicJson type #12843

pakrym · 2020-06-17T17:16:45Z

Adds an initial implementation, unit tests and perf tests.

The implementation is known to be terrible but I'd like to get something out and start getting feedback.

tg-msft

Just adding some quick initial thoughts - I'll do a full review later tonight.

tg-msft · 2020-06-18T21:15:41Z

sdk/core/Azure.Core.Experimental/api/Azure.Core.Experimental.netstandard2.0.cs

+        public System.Collections.Generic.IEnumerable<Azure.Core.DynamicJson> EnumerateArray() { throw null; }
+        public System.Collections.Generic.IEnumerable<System.Collections.Generic.KeyValuePair<string, Azure.Core.DynamicJson>> EnumerateObject() { throw null; }
+        public int GetArrayLength() { throw null; }
+        public bool GetBoolean() { throw null; }


Does this convert the current value to a boolean? As might be a better prefix.

I was purposefully trying to mirror JsonElement: https://docs.microsoft.com/en-us/dotnet/api/system.text.json.jsonelement?view=netcore-3.1.

Yeah, I wasn't really expecting that. When I see this API, I wonder why we don't just add a couple of extension methods to JsonElement like Serialize<T>, AsDynamic, etc. I was thinking this type would be much smaller and focused exclusively on objects to avoid all these other accessors.

This type can't be focused just on objects, services like digitaltwins support taking arbitrary JSON so the root can be an object or an array maybe even a primitive.

And maybe we're trying to solve too much here? Maybe we should tell folks if you want to use primitives or arrays with DigitalTwins that you need to work with raw JSON elements. I don't think I'd want to use this type as it exists right now for Search because it's way too general and I think would confuse what's already a tricky part of the Search API.

tg-msft · 2020-06-18T21:16:29Z

sdk/core/Azure.Core.Experimental/api/Azure.Core.Experimental.netstandard2.0.cs

@@ -24,6 +24,52 @@ namespace Azure.Core
        public override string ToString() { throw null; }
        public string ToString(System.Text.Encoding encoding) { throw null; }
    }
+    public partial class DynamicJson : System.Dynamic.IDynamicMetaObjectProvider


We should also implement IDictionary sooner than later because I think it'll help force some of the naming choices below.

Do you feel strongly about having IDictionary and why?

It's such a well known, easy to use thing. This is getting to be a pretty hefty API at this point and if I'm trying to get a quick job done, seeing IDictionary may let me shortcut all the rest of it. All of https://docs.microsoft.com/en-us/dotnet/api/system.collections.generic.idictionary-2?view=netcore-3.1#extension-methods as well.

Problem with implementing IDictionary<string, DJ> is that I would also need to implement IList if the current value is an array. Then we get 2 IEnumerable interfaces on the same object and that's a bit weird.

You're jamming an object and an array and primitives into the same type. It's already weird. 😄

sdk/core/Azure.Core.Experimental/api/Azure.Core.Experimental.netstandard2.0.cs

tg-msft · 2020-06-18T21:27:02Z

sdk/core/Azure.Core.Experimental/api/Azure.Core.Experimental.netstandard2.0.cs

+        public long GetLong() { throw null; }
+        public Azure.Core.DynamicJson GetProperty(string name) { throw null; }
+        public string? GetString() { throw null; }
+        public static Azure.Core.DynamicJson Object() { throw null; }


From the signature, I don't really know what this API does. Should this just be Create()?

Yeah. Any idea how to name the one that creates an array?

Create vs CreateArray?

tg-msft · 2020-06-18T21:27:56Z

sdk/core/Azure.Core.Experimental/api/Azure.Core.Experimental.netstandard2.0.cs

+        public static Azure.Core.DynamicJson Serialize<T>(T value, Azure.Core.ObjectSerializer serializer) { throw null; }
+        public static Azure.Core.DynamicJson Serialize<T>(T value, System.Text.Json.JsonSerializerOptions? options = null) { throw null; }
+        System.Dynamic.DynamicMetaObject System.Dynamic.IDynamicMetaObjectProvider.GetMetaObject(System.Linq.Expressions.Expression parameter) { throw null; }
+        public System.Text.Json.JsonElement ToJsonElement() { throw null; }


Would Serialize<object>() do the same?

Not sure I understand. Serialize is a static method that takes an object and turns it into DJ.

Yes, sorry, should have said Deserialize. It feels like this is a short cut for calling Serialize<object>(this).

tg-msft · 2020-06-18T21:28:11Z

sdk/core/Azure.Core.Experimental/api/Azure.Core.Experimental.netstandard2.0.cs

+        System.Dynamic.DynamicMetaObject System.Dynamic.IDynamicMetaObjectProvider.GetMetaObject(System.Linq.Expressions.Expression parameter) { throw null; }
+        public System.Text.Json.JsonElement ToJsonElement() { throw null; }
+        public override string ToString() { throw null; }
+        public void WriteTo(System.Text.Json.Utf8JsonWriter writer) { }


Does this need to be public?

Not feeling strong about it. I was trying to have a lot of interop with S.T.J.

sdk/core/Azure.Core.Experimental/api/Azure.Core.Experimental.netstandard2.0.cs

tg-msft · 2020-06-18T21:29:55Z

sdk/core/Azure.Core.Experimental/api/Azure.Core.Experimental.netstandard2.0.cs

+        public string? GetString() { throw null; }
+        public static Azure.Core.DynamicJson Object() { throw null; }
+        public static Azure.Core.DynamicJson Object(System.Collections.Generic.IEnumerable<System.Collections.Generic.KeyValuePair<string, Azure.Core.DynamicJson>> values) { throw null; }
+        public static explicit operator bool (Azure.Core.DynamicJson json) { throw null; }


I would love a virtual I could override with domain specific conversion logic. We'd probably need that for Search.

That would also require us to have a factory func so the child DynamicJson objects we create are of the right type.

Yes, and I'd like a virtual one of those as well please. SearchDocument turns nested objects into more SearchDocuments

How do you get an object nested into array via SearchDocument?

We do that when parsing JSON. But if you just set a property, we leave your object type and let the JSON serializer deal with it when turning it back into JSON.

tg-msft · 2020-06-19T16:59:36Z

sdk/core/Azure.Core.Experimental/tests/DynamicJsonTest.cs

+        public void DynamicCanConvertToString() => Assert.AreEqual("string", JsonAsType<string>("\"string\""));
+
+        [Test]
+        public void DynamicCanConvertToInt() => Assert.AreEqual(5, JsonAsType<int>("5"));


Can you add a test making it explicit whether "\"5\"" converts to int? That's been a source of unpleasantness in SearchDocument.

FYI, this upcoming feature should help with that:
dotnet/runtime#30255

tg-msft · 2020-06-19T17:03:57Z

sdk/core/Azure.Core.Experimental/src/DynamicJson.cs

+namespace Azure.Core
+{
+    /// <summary>
+    ///


Maybe Represents a JSON value returned from or provided to service operations. It can be used as either a dynamic object or via its accessors. for now?

Oh, you didn't do any of these yet... 😄 It's probably okay for Experimental but it'd be good to fix them up as soon as folks are happy with the broad shape of the API.

pakrym · 2020-06-22T18:54:15Z

I'll address issues in a follow up PR.

ahsonkhan · 2020-07-14T23:36:28Z

sdk/core/Azure.Core.Experimental/src/DynamicJson.cs

+            private long _long;
+            private bool _hasLong;
+            private double _double;
+            private bool _hasDouble;


Why didn't you make these fields nullable?

ahsonkhan · 2020-07-14T23:48:37Z

sdk/core/Azure.Core.Experimental/src/DynamicJson.cs

+        public static DynamicJson Serialize<T>(T value, JsonSerializerOptions? options = null)
+        {
+            var serialized = JsonSerializer.Serialize<T>(value, options);
+            return new DynamicJson(JsonDocument.Parse(serialized).RootElement);


JsonDocument is IDisposable.

Who is responsible for disposing it? If you don't, APIs like these can accidentally exhaust the underlying array pool.

For your specific case, consider cloning the element to detach it from the document's lifetime, and dispose the document.
https://docs.microsoft.com/en-us/dotnet/api/system.text.json.jsonelement.clone?view=netcore-3.1
Something like this:

using doc = JsonDocument.Parse(serialized); return new DynamicJson(doc.RootElement.Clone());

pakrym added 14 commits May 22, 2020 13:43

Add DynamicJson type

b6fc0a1

meh

4b0ea31

1

89ebb3f

Progress

848ac93

Progress

1d5f123

Merge remote-tracking branch 'azure/master' into pakrym/dynamicjson

4cfa31e

..

f133095

More fixes

05eb10d

More

99507ce

More

fab111c

Morte

b2b1ba0

Merge branch 'master' into pakrym/dynamicjson

65349a2

newtonsoft

b6f4983

Export API

639d5f9

pakrym requested a review from tg-msft June 17, 2020 17:47

Test

5fbc4ce

pakrym marked this pull request as ready for review June 17, 2020 18:01

pakrym requested review from chidozieononiwu, danieljurek, KrzysztofCwalina, mitchdenny and weshaggard as code owners June 17, 2020 18:01

pakrym added 4 commits June 18, 2020 10:06

Add number format checks.

6d8f927

Tests

dbca343

More tests

8616cc6

Export api

18c1dbd

tg-msft reviewed Jun 18, 2020

View reviewed changes

FB

ee8b195

tg-msft approved these changes Jun 19, 2020

View reviewed changes

pakrym merged commit 1286a9d into Azure:master Jun 22, 2020

pakrym deleted the pakrym/dynamicjson branch June 22, 2020 18:54

prmathur-microsoft pushed a commit that referenced this pull request Jul 8, 2020

Add DynamicJson type (#12843)

114c7ce

ahsonkhan reviewed Jul 14, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add DynamicJson type #12843

Add DynamicJson type #12843

pakrym commented Jun 17, 2020 •

edited

Loading

tg-msft left a comment

tg-msft Jun 18, 2020

pakrym Jun 18, 2020

tg-msft Jun 19, 2020

pakrym Jun 19, 2020

tg-msft Jun 19, 2020

tg-msft Jun 18, 2020

pakrym Jun 18, 2020

tg-msft Jun 19, 2020

pakrym Jun 19, 2020 •

edited

Loading

tg-msft Jun 19, 2020

tg-msft Jun 18, 2020

pakrym Jun 18, 2020

tg-msft Jun 19, 2020

tg-msft Jun 18, 2020

pakrym Jun 18, 2020

tg-msft Jun 19, 2020

tg-msft Jun 18, 2020

pakrym Jun 18, 2020

tg-msft Jun 18, 2020

pakrym Jun 19, 2020

tg-msft Jun 19, 2020

pakrym Jun 19, 2020

tg-msft Jun 19, 2020

tg-msft Jun 19, 2020

ahsonkhan Jul 14, 2020

tg-msft Jun 19, 2020

tg-msft Jun 19, 2020

pakrym commented Jun 22, 2020

ahsonkhan Jul 14, 2020

ahsonkhan Jul 14, 2020

Add DynamicJson type #12843

Add DynamicJson type #12843

Conversation

pakrym commented Jun 17, 2020 • edited Loading

tg-msft left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pakrym Jun 19, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pakrym commented Jun 22, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pakrym commented Jun 17, 2020 •

edited

Loading

pakrym Jun 19, 2020 •

edited

Loading