JsonArray and transformations API implementation by kasiabulat · Pull Request #40469 · dotnet/corefx

kasiabulat · 2019-08-20T22:26:19Z

I implemented JsonArray and transformations API for JsonNode. In order to do this, I needed to adjust other types as well. I also made some minor fixes and included some feedback from discussions. I still new to add much more tests and documentation to some of the methods, but I wanted to create this PR today to discuss new issues that came up during tomorrow meeting. I can split it in to couple of PRs later on, if the number of changes it too big there.

New open questions (I added them to the open questions section in specification as well):

Where do we want to enable user to set handling properties manner? Do we want to support it as JsonElement does not? (and currently JsonNode.Parse implementation uses JsonDocument.Parse)
Do we want to support transforming JsonObject into JSON string? Should ToString behave this way or do we want an additional method - e.g. GetJsonString (it might be confusing as we have a type JsonString) or GetJsonRepresenation?
AsJsonElement function on JsonNode currently does not work for null node. Do we want to support null case somehow?
Do we want separate JsonElement implementations for JsonNode and JsonDocument or can the implementation be mixed as it is now (with a lot of ifs in each method)?
Do we want separate JsonElement.ArrayEnumerator and JsonElement.ObjectEnumerator implementations for JsonNode and JsonDocument? Do we want separate methods in JsonElement returning strongly typed different implementations?
Is it OK that JsonElement.GetRawText does not return the raw text for JsonNode parent?
Should we support JsonElement.WriteTo for JsonNode parent?
Private property JsonElement.TokenType is currently throwing an InvalidCast exception for JsonNode parent in debugger. Is it OK?
Do we want both Clone and DeepCopy methods for JsonNode?
Are we OK with needing to cast JsonArray to JsonNode in order to add it to JsonObject? (there's an ambiguous call between JsonArray and IEnumerable<JsonNode> right now)
Do we want IsImmutable property for JsonElement?
Is DateTime for JsonString handled correctly?

addresses: #39922
cc: @joperezr @bartonjs @ericstj @ahsonkhan @terrajobst @JamesNK @stephentoub

bartonjs

I reviewed the ref and src changes. Test changes to follow.

kasiabulat · 2019-08-23T17:37:12Z

I addressed review comments, made fixes and added documentation to JsonNode. I will add API (and specification) changes in new PR in case we want to come back to this version. I will change JsonElement.ObjectEnumerator not to use JsonObjectEnumerator, add more tests and improve existing ones in PR with API changes.

kasiabulat · 2019-08-23T20:44:14Z

Do you think I can merge this PR? @ahsonkhan @bartonjs @joperezr

bartonjs · 2019-08-23T22:16:05Z

+            }
+
+            var jsonNode = (JsonNode)_parent;
+            return jsonNode.ToString();


This should probably be ToJson(), unless that's being done at a later time (changes whether or not string values can be read, for one)

Yes, I don't have ToJson yet, but I'll add it later. I was wondering if we want this method supported for JsonNode at all if we actually aren't going to have "the original input data backing this value".

bartonjs · 2019-08-24T01:29:20Z

+        ///   <see langword="true"/> if the value is successfully found in a JSON array,
+        ///   <see langword="false"/> otherwise.
+        /// </returns>
+        public bool Contains(JsonNode value) => _list.Contains(value);


Given that this method (and IndexOf) are doing reference equality, we should delete Contains. IndexOf, and LastIndexOf until we can come up with a stronger definition.

This is made worse by the implicit conversions, because array.Add(true), array.Contains(true) => false.

ahsonkhan · 2019-08-26T21:48:01Z

+                }
+                else
+                {
+                    var jsonArray = (JsonArray)target._parent;


What happens if I do the following:

JsonElement element = default; foreach(var temp in element.EnumerateArray()) { }

It will throw InvalidOperationException (the instance is invalid).

ahsonkhan · 2019-08-26T21:49:24Z

+                        return jsonArray[_curIdx].AsJsonElement();
+                    }
+
+                    var document = (JsonDocument)_target._parent;


It would be good to measure the perf regression here, when enumerating a JSON array.

ahsonkhan · 2019-08-26T22:01:13Z

+            {
+                get
+                {
+                    if (!_target.IsImmutable)


nit: Can we follow the existing pattern and use the if (target._parent is JsonDocument document) check rather than _target.IsImmutable, and do that up-front?

if (_curIdx < 0) { return default; } if (target._parent is JsonDocument document) { return new JsonProperty(new JsonElement(document, _curIdx)); } Debug.Assert(!_target.IsImmutable); //...

I will be rewriting this enumerator when I will be adding API, but I will keep it in mind.

ahsonkhan · 2019-08-26T22:04:07Z

+
+                var jsonNode = (JsonNode)_parent;
+
+                if (jsonNode == null)


This will not be required once we have a JsonNull type, correct?

ahsonkhan · 2019-08-26T22:08:10Z

+
+                var jsonNode = (JsonNode)_parent;
+
+                if (jsonNode is JsonArray jsonArray)


Can this be simplified as follows (and remove jsonNode):

if (_parent is JsonArray jsonArray) { ... }

ahsonkhan · 2019-08-26T22:10:12Z

+
+            var jsonNode = (JsonNode)_parent;
+
+            if (jsonNode is JsonObject jsonObject)


Same here/elsewhere. Seems like casting to JsonNode is unnecessary/redundant. Cast directly to JsonObject.

I need to cast it to JsonNode in order to call jsonNode.ValueKind and throw appropriate exception.

ahsonkhan

LGTM

ahsonkhan · 2019-08-27T01:09:26Z

+                    JsonArray jsonArray = new JsonArray();
+                    foreach (JsonElement element in jsonElement.EnumerateArray())
+                    {
+                        jsonArray.Add(DeepCopy(element));


Is it OK to have these unbounded recursive calls based on user input? What happens if I pass a payload containing 1_000 nested deep json array as a JsonElement to DeepCopy?

Right, I can add depth check.

This currently stack overflows:

var builder = new StringBuilder(); for (int i = 0; i < 2_000; i++) { builder.Append("["); } for (int i = 0; i < 2_000; i++) { builder.Append("]"); } var options = new JsonDocumentOptions { MaxDepth = 5_000 }; using (JsonDocument dom = JsonDocument.Parse(builder.ToString(), options)) { JsonNode node = JsonNode.DeepCopy(dom.RootElement); }

Ok, I can rewrite it to use my own stack instead of recursion.

ahsonkhan · 2019-08-27T01:11:31Z

        /// </summary>
        /// <param name="value">The value to represent as a JSON string.</param>
-        public JsonString(Guid value) => Value = value.ToString();
+        public JsonString(Guid value) => Value = value.ToString("D");


Should this be CultureInfo.InvariantCulture as well?

ahsonkhan · 2019-08-27T01:14:11Z

+        }
+
+        [Fact]
+        public static void TestClean()


nit: TestClear

Suggested change

public static void TestClean()

public static void TestClear()

ahsonkhan · 2019-08-27T01:15:44Z

+        ///         {
+        ///             "software developers" :
+        ///             {
+        ///                 "full time employees" : /JsonObject of 3 employees fromk database/ 


nit: Same below

Suggested change

/// "full time employees" : /JsonObject of 3 employees fromk database/

/// "full time employees" : /JsonObject of 3 employees from database/

ahsonkhan · 2019-08-27T01:17:08Z

+        public static IEnumerable<object[]> DateTimeData =>
+           new List<object[]>
+           {
+                       new object[] { new DateTime(DateTime.MinValue.Ticks, DateTimeKind.Utc) },


nit: spacing. Why is this indented 2 levels deep?

I will fix it.

ahsonkhan · 2019-08-27T01:17:35Z

    <Compile Include="JsonEncodedTextTests.cs" />
    <Compile Include="JsonGuidTestData.cs" />
+    <Compile Include="JsonNodeToJsonElementTests.cs" />
+    <Compile Include="JsonNodeCloneTests.cs" />


nit: Move these to the other ItemGroup.

ahsonkhan · 2019-08-27T01:19:01Z

  <ItemGroup>
+    <Compile Include="JsonArrayTests.cs" />
    <Compile Include="JsonBooleanTests.cs" />
+    <Compile Include="JsonNodeTestData.cs" />


nit: Keep the similar filename structure as before, i.e. rename the file:
JsonNodeTests.TestData.cs

I wanted to reuse this class in both JsonArray and JsonObject tests.

ahsonkhan · 2019-08-27T01:22:00Z

+
+namespace System.Text.Json.Tests
+{
+    public static class JsonNodeToJsonElementTests


nit: To me, these are all JsonNode tests. Why split it into separate classes: JsonNodeCloneTests, JsonNodeParseTests, JsonNodeToJsonElementTests.

Instead, make it a partial class JsonNodeTests and rename the files:
JsonNodeTests.Clone.cs
JsonNodeTests.Parse.cs
JsonNodeTests.ToJsonElement.cs

Comment: #40469 (comment), but right, I can make it one class.

ahsonkhan · 2019-08-27T01:29:50Z


        /// <summary>
-        ///  Initializes a new instance of the <see cref="JsonString"/> with a string representation of the <see cref="DateTime"/> structure.
+        ///  Initializes a new instance of the <see cref="JsonString"/> with an ISO 8601 representation of the <see cref="DateTime"/> structure.


Does it matter what representation we store the DateTime in as, as long as it round-trips correctly when user tries to get the DateTime back? If so, maybe omit the ISO representations.

Also, is it accurate to state it's an ISO 8601 representation when we use "s"? I suppose "s" is a subset of the ISO support we have for other APIs, so it seems fine, but ISO allowed for a lot more representations than just "s".

https://github.com/dotnet/docs/blob/master/docs/standard/datetime/system-text-json-support.md#support-for-parsing

cc @layomia

* JsonArray added * Transformations API added * Review Comments included Commit migrated from dotnet/corefx@ad4fdd9

kasiabulat added 15 commits August 15, 2019 07:29

JsonArray added

260f676

JsonArray scenario-tests added

70263cb

work on tests

b9f6d19

DeepCopy methods implemented

41d798c

Simple parse added

e8efb68

Work on JsonElement changing

a6ca8b7

Work on JsonElement changing

3fabfe0

JsonElement and JsonString adjusted to transformation API

161404d

work on enumerators

2e387d1

work on enumerators

e52c48c

enumerators fixed

7795b87

nulls fixes

835392e

csproj fixes

a0f774c

open questions added to specification

c2b0b5a

open questions added to specification

6f15b4c

kasiabulat requested review from JamesNK, ahsonkhan, bartonjs, joperezr and stephentoub August 20, 2019 22:26

Resolve merge conflicts with upstream/master

6612312

joperezr reviewed Aug 20, 2019

View reviewed changes

Comment thread src/System.Text.Json/src/System/Text/Json/Node/JsonArray.cs

joperezr reviewed Aug 20, 2019

View reviewed changes

Comment thread src/System.Text.Json/src/System/Text/Json/Node/JsonArray.cs Outdated

joperezr reviewed Aug 20, 2019

View reviewed changes

Comment thread src/System.Text.Json/src/System/Text/Json/Node/JsonArray.cs Outdated

JamesNK reviewed Aug 20, 2019

View reviewed changes

Comment thread src/System.Text.Json/ref/System.Text.Json.cs

Comment thread src/System.Text.Json/ref/System.Text.Json.cs

Comment thread src/System.Text.Json/src/System/Text/Json/Node/JsonArrayEnumerator.cs

ahsonkhan added the area-System.Text.Json label Aug 20, 2019

ahsonkhan added this to the 5.0 milestone Aug 20, 2019

joperezr reviewed Aug 21, 2019

View reviewed changes

Comment thread src/System.Text.Json/src/System/Text/Json/Node/JsonArray.cs Outdated

minor fixes, code coverage improved

c63d553

bartonjs reviewed Aug 21, 2019

View reviewed changes

kasiabulat added 10 commits August 22, 2019 14:23

work on review comments

3b11f1e

work on review comments

eea2a96

build fixed

d63c48b

work on fixing tests

6131766

object enumerator fixed

ed78505

array enumerator fixed

7798af1

all test failures fixed

6d4b869

minor tests modifications, documentation added to JsonNode

2cb68c9

minor fix

2c63db6

implementation related questions removed

b5af593

netfx fix

0a5c15f

bartonjs reviewed Aug 24, 2019

View reviewed changes

review comments included

126d03e

bartonjs approved these changes Aug 26, 2019

View reviewed changes

ahsonkhan reviewed Aug 26, 2019

View reviewed changes

ahsonkhan approved these changes Aug 26, 2019

View reviewed changes

kasiabulat merged commit ad4fdd9 into dotnet:master Aug 26, 2019

ahsonkhan reviewed Aug 27, 2019

View reviewed changes

kasiabulat deleted the kasiabulat/transformations branch August 27, 2019 14:24

picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022

JsonArray and transformations API implementation (dotnet/corefx#40469)

c3afc7f

* JsonArray added * Transformations API added * Review Comments included Commit migrated from dotnet/corefx@ad4fdd9


		var jsonNode = (JsonNode)_parent;

		if (jsonNode is JsonArray jsonArray)


		var jsonNode = (JsonNode)_parent;

		if (jsonNode is JsonObject jsonObject)

	public static void TestClean()
	public static void TestClear()

	/// "full time employees" : /JsonObject of 3 employees fromk database/
	/// "full time employees" : /JsonObject of 3 employees from database/

Conversation

kasiabulat commented Aug 20, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bartonjs left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kasiabulat commented Aug 23, 2019

Uh oh!

kasiabulat commented Aug 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ahsonkhan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ahsonkhan Aug 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

kasiabulat commented Aug 23, 2019 •

edited

Loading

ahsonkhan Aug 27, 2019 •

edited

Loading