Add JsonNode feature #51025

steveharter · 2021-04-09T20:08:57Z

Implementation of writeable JSON DOM including support for C# dynamic.

Notes:

The ability to have extension properties by of type JsonObject is not implemented in the PR to help reduce review size. That support will come after this PR is in and will allow an extension property of the form:
[JsonExtensionData] public JsonObject ExtensionData {get; set;}
MetaDynamic.cs contains internal code to support C# dynamic. As a follow-up to this PR, additional tests may be added to increase coverage here and\or that code refactored to reduce some lower-level expression-based code.
Test coverage is nearly 100% except for MetaDynamic.cs as mentioned above.
The internal use of JsonObject's System.Collections.Generic.Dictionary<,> will be replaced with a dictionary that supports ordering semantics so inserts\removes are deterministic with list-based semantics; ordering is nondeterministic with Dictionary<,>.

dotnet-issue-labeler · 2021-04-09T20:09:03Z

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

ghost · 2021-04-09T20:09:06Z

Tagging subscribers to this area: @eiriktsarpalis, @layomia
See info in area-owners.md if you want to be subscribed.

Issue Details

Implementation of writeable JSON DOM including support for C# dynamic.

Fixes #47649

Notes:

The ability to have extension properties by of type JsonObject is not implemented in the PR to help reduce review size. That support will come after this PR is in and will allow an extension property of the form:
[JsonExtensionData] public JsonObject ExtensionData {get; set;}
MetaDynamic.cs contains internal code to support C# dynamic. As a follow-up to this PR, additional tests may be added to increase coverage here and\or that code refactored to reduce some lower-level expression-based code.
Test coverage is nearly 100% except for MetaDynamic.cs as mentioned above.

Author:	steveharter
Assignees:	steveharter
Labels:	`area-System.Text.Json`
Milestone:	6.0.0

src/libraries/System.Text.Json/src/System/Text/Json/Node/JsonObject.cs

danmoseley · 2021-04-11T22:36:50Z

🥳

tdykstra

LGTM, made some suggestions for what look like copy/paste errors.

src/libraries/System.Text.Json/src/System/Text/Json/Node/JsonArray.IList.cs

src/libraries/System.Text.Json/src/System/Text/Json/Node/JsonObject.IDictionary.cs

src/libraries/System.Text.Json/src/System/Text/Json/Node/JsonObject.cs

src/libraries/System.Text.Json/ref/System.Text.Json.InboxOnly.cs

src/libraries/System.Text.Json/src/System/Text/Json/Node/JsonArray.IList.cs

src/libraries/System.Text.Json/src/System/Text/Json/Node/JsonNode.cs

src/libraries/System.Text.Json/tests/JsonNode/Common.cs

layomia · 2021-04-13T15:15:53Z

src/libraries/System.Text.Json/tests/JsonNode/DynamicTests.cs

+            Assert.Equal("Hello", (string)obj.MyString);
+
+            // Verify other string-based types.
+            // Since this requires a custom converter, an exception is thrown.


Hmm isn't this why JsonStringEnumConverter was added above on L64? Is it not honored? Also, is the specific RuntimeBinderException known or can it vary?

Currently the semantics don't go through the serializer when calling GetValue(), so another step is needed to get the enum. We can revisit this based on feedback, but per discussion decided to start with more restrictive semantics.

In the future, would we need more overloads that take the options (to retrieve converters and such), or will the existing API surface suffice?

layomia · 2021-04-13T15:19:11Z

src/libraries/System.Text.Json/tests/JsonNode/DynamicTests.cs

+
+            // Numbers must specify the type through a cast or assignment.
+            Assert.IsAssignableFrom<JsonValue>(obj.MyInt);
+            Assert.ThrowsAny<Exception>(() => obj.MyInt == 42L);


What exception is thrown?

On a separate note - can I dynamically change the CLR type of a property on a dynamic object? e.g. could I say obj.MyInt = "Hello"?

Changed to RuntimeBinderException. We could catch and throw InvalidOperation in this case; I'll look at that for consistency sake with non-dynamic.

Yes you can change the value with obj.MyInt = "Hello" since that uses the indexer to create a new node, and then replace the previous node.

+1 to consistency with non-dynamic.

I'll address this after the PR. To support this will require changes to the lower-level dynamic interop in MetaDynamic.cs and adding more extensive testing for failure cases.

src/libraries/System.Text.Json/tests/JsonNode/DynamicTests.cs

src/libraries/System.Text.Json/src/System/Text/Json/Node/JsonArray.cs

src/libraries/System.Text.Json/tests/JsonNode/JsonArrayTests.cs

src/libraries/System.Text.Json/tests/JsonNode/JsonNodeOperatorTests.cs

src/libraries/System.Text.Json/tests/JsonNode/JsonNodeTests.cs

layomia · 2021-04-13T20:43:12Z

src/libraries/System.Text.Json/tests/JsonNode/JsonObjectTests.cs

+        public static void SetItem_Fail()
+        {
+            var jObject = new JsonObject();
+            Assert.Throws<ArgumentNullException>(() => jObject[null] = 42);


Is this test any different from NullPropertyNameFail above?

layomia · 2021-04-13T20:44:50Z

src/libraries/System.Text.Json/tests/JsonNode/JsonObjectTests.cs

+            JsonNode node1 = jObject["One"];
+            JsonNode node2 = jObject["Two"];


nit: these don't seem to be used

layomia · 2021-04-13T20:57:30Z

src/libraries/System.Text.Json/tests/JsonNode/JsonObjectTests.cs

+            // Dictionary is created when Add is called for the first time, so we need to be added first.
+            jArray = new JsonArray(options);
+            jObject = new JsonObject();
+            jObject.Add("MyProperty", 42);
+            jArray.Add(jObject);
+            jObject.Add("myproperty", 42); // no exception since options were not set in time.


Do you imagine people will run into issues with the somewhat subtle semantics here?

With collection initializers this can occur although I don't see an easy alternative to "force" the options to be specified upfront before "Add()".

Originally, I had a design where the JsonSerializerOptions were used instead of JsonNodeOptions since those options have a concept of case insensitivity. However, that dependency was removed because tying those options to DOM creation was somewhat confusing since basically only the case insensitivity flag was used and nothing else. However, using JsonSerializerOptions like originally proposed could work in this scenario when we implement the ability to change the global options (as is scheduled for v6 I believe).

So if we add the ability to change the global JsonSerializerOptions perhaps we also add the ability to change\specify a global JsonNodeOptions and that could be used to address this scenario.

I see - yeah we should document this explicitly.

when we implement the ability to change the global options (as is scheduled for v6 I believe).

I'm getting more and more scared of doing this due to side effects with the defaults being changed in a way that affects all assemblies in a compilation.

src/libraries/System.Text.Json/tests/JsonNode/JsonObjectTests.cs

src/libraries/System.Text.Json/src/System/Text/Json/Node/JsonValueOfT.cs

src/libraries/System.Text.Json/tests/JsonNode/JsonValueTests.cs

layomia · 2021-04-13T21:40:43Z

src/libraries/System.Text.Json/tests/JsonNode/JsonValueTests.cs

+
+            JsonNode node = JsonSerializer.Deserialize<JsonNode>("\"42\"", options);
+            Assert.IsAssignableFrom<JsonValue>(node);
+            Assert.Throws<InvalidOperationException>(() => node.GetValue<int>());


Is it possible to have this work in the future?

Yes if we decide we want to ease up on the GetValue() semantics and invoke the serializer. That would also require the JsonSerializerOptions to be passed in for things like quoted numbers.

We could also add a new method, like Json.NET's JNode.ToObject(), to deserialize.

src/libraries/System.Text.Json/tests/JsonNode/JsonValueTests.cs

layomia · 2021-04-13T22:06:33Z

src/libraries/System.Text.Json/tests/JsonNode/ParentPathRootTests.cs

+        {
+            JsonNode node = new JsonObject
+            {
+                ["[Child"] = 1


Nice. Are there other characters that are interesting enough to test?

Do you mean for the property name or for the value? The JsonNodeOperatorTests.cs has every value type possible (int, uint, string, char, etc)

src/libraries/System.Text.Json/tests/JsonNode/ParentPathRootTests.cs

src/libraries/System.Text.Json/src/System/Text/Json/Node/JsonNode.cs

layomia · 2021-04-13T22:42:53Z

src/libraries/System.Text.Json/src/System/Text/Json/Node/JsonNode.cs

+        {
+            if (Parent != null)
+            {
+                ThrowHelper.ThrowInvalidOperationException_NodeAlreadyHasParent();


Could it be valid in the future to want to replace/unbind from the parent?

Unbinding happens when an element is removed via Remove() or replaced via an indexer.

layomia · 2021-04-13T22:49:27Z

src/libraries/System.Text.Json/src/System/Text/Json/Node/JsonObject.Dynamic.cs

+
+namespace System.Text.Json.Node
+{
+    public partial class JsonObject


It seems dynamic semantics are only supported when using JsonObject. What happens when other node types are treated as dynamic i.e setting/getting properties is attempted?

The dynamic feature allows you to call members on JsonArray and JsonValue (e.g. jArray.Add()). If an invalid member is specifies, RuntimeBinderException is thrown.

src/libraries/System.Text.Json/src/System/Text/Json/Node/JsonObject.IDictionary.cs

src/libraries/System.Text.Json/src/System/Text/Json/Node/JsonObject.cs

layomia · 2021-04-13T23:12:08Z

src/libraries/System.Text.Json/src/System/Text/Json/Node/JsonValueOfT.cs

+
+            if (_value is JsonElement jsonElement)
+            {
+                return ConvertJsonElement<T>();


ConvertJsonElement and TryConvertJsonElement seem very similar. Could we instead call TryConvertJsonElement here, and throw the IOE if it returns false? Perhaps we could then delete ConvertJsonElement.

Perhaps. The reason is since we delegate to JsonElement to obtain the values in ConvertJsonElement we leverage the existing validation and exception logic in JsonElement. JsonElement in some cases throws FormatException and in other cases InvalidOperation which we would need to duplicate if we combine.

layomia · 2021-04-13T23:15:40Z

...ies/System.Text.Json/src/System/Text/Json/Serialization/Converters/Node/JsonNodeConverter.cs

+        public static JsonNodeConverter Default { get; } = new JsonNodeConverter();
+        public JsonArrayConverter ArrayConverter { get; } = new JsonArrayConverter();
+        public JsonObjectConverter ObjectConverter { get; } = new JsonObjectConverter();
+        public JsonValueConverter ValueConverter { get; } = new JsonValueConverter();
+        public ObjectConverter ElementConverter { get; } = new ObjectConverter();


Is it worth it to instantiate these lazily and save on allocs?

They are already lazy. The JsonNodeConverterFactory will only access these properties when given a JsonNode Type.

I believe these properties will be initialized by the constructor.

These particular properties will be initialized eagerly by the constructor:

https://sharplab.io/#v2:C4LglgNgNAJiDUAfAAgJgIwFgBQyDMABGgQMIEDeOB1RhA9gEYBWApgMbAEBiddFBAcxbAA3AQC+BALwEAdiwDuBRqw4AKAJQiqNfMubtOAIQCGAJ35DRE6XMX7VwTduw0CO6nuQAWAgFlNCg8JHHEgA

...ries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.Converters.cs

...ies/System.Text.Json/src/System/Text/Json/Serialization/Converters/Node/JsonNodeConverter.cs

...t.Json/tests/Serialization/CustomConverterTests/CustomConverterTests.Dynamic.Sample.Tests.cs

layomia

LGTM

src/libraries/System.Text.Json/src/System/Text/Json/Node/JsonNode.To.cs

src/libraries/System.Text.Json/src/System/Text/Json/Node/JsonNode.cs

src/libraries/System.Text.Json/src/System/Text/Json/Node/JsonObject.cs

src/libraries/System.Text.Json/src/System/Text/Json/Node/JsonValueOfT.cs

...ries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.Converters.cs

src/libraries/System.Text.Json/tests/JsonNode/JsonObjectTests.cs

steveharter added the area-System.Text.Json label Apr 9, 2021

steveharter added this to the 6.0.0 milestone Apr 9, 2021

steveharter requested review from tdykstra, eiriktsarpalis and layomia April 9, 2021 20:08

steveharter self-assigned this Apr 9, 2021

steveharter requested a review from Jozkee as a code owner April 9, 2021 20:08

dotnet-issue-labeler bot added the new-api-needs-documentation label Apr 9, 2021

campersau reviewed Apr 9, 2021

View reviewed changes

src/libraries/System.Text.Json/src/System/Text/Json/Node/JsonObject.cs Show resolved Hide resolved

This was referenced Apr 9, 2021

[tests] System.Text.Json.Tests segfault, for Libraries Test Run release coreclr OSX x64 Release #47805

Closed

System.Text.Json.Tests failing on browser #50433

Closed

Add JsonNode feature

63f26c7

steveharter force-pushed the JsonNode branch from f13f25d to 63f26c7 Compare April 12, 2021 19:26

steveharter added 3 commits April 12, 2021 14:29

Fix test failure on non-windows

f661e1b

Fix test issue non non-core

50a6d58

Fix test issue non non-core

1e5f46b

tdykstra reviewed Apr 12, 2021

View reviewed changes

layomia reviewed Apr 13, 2021

View reviewed changes

Feedback; additional tests

6644e84

layomia reviewed Apr 13, 2021

View reviewed changes

steveharter added 2 commits April 13, 2021 14:06

Test clarifications

5d2ae42

Enable dynamic on netstandard and netfx

3d1e7a7

layomia reviewed Apr 13, 2021

View reviewed changes

steveharter added 2 commits April 13, 2021 16:52

Ensure previous node is unparented on JObject indexer setter

9f228ac

Change string-to-char operator and address test feedback

8c796ff

layomia reviewed Apr 13, 2021

View reviewed changes

layomia approved these changes Apr 13, 2021

View reviewed changes