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.

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

JsonNode feedback #55827

Closed
NinoFloris opened this issue Jul 16, 2021 · 6 comments
Closed

JsonNode feedback #55827

NinoFloris opened this issue Jul 16, 2021 · 6 comments

Comments

@NinoFloris
Copy link
Contributor

Some feedback after using the new JsonNode apis for some time:

  • For F# use JsonNode is missing a convenience method to safely 'dot into' a deep path. F# does not have a null conditional operator to conveniently intersperse between indexer calls. I've had to add something like the following code to make querying workable.
    type JsonNode with
        member node.GetNode(index: int) = Option.ofObj node.[index]
        member node.GetNode(segment: string) = Option.ofObj node.[segment]
        member node.GetNode([<ParamArray>]pathSegments: obj[]) =
            let mutable current, i = node, 0
            while not (isNull current) && i < pathSegments.Length do
                match pathSegments.[i] with
                | :? int as index -> current <- current.[index]
                | :? string as segment -> current <- current.[segment]
                | _ -> failwith "Unknown path segment type, either int or string is supported."
                i <- i + 1
            Option.ofObj current

It could make sense to provide this in the box but mainly I wanted to note this usability issue for F#.

  • JsonArray is missing a convenience method to get an IEnumerable or array of * GetValue * results out. I'm assuming the alternative pattern is node.AsArray().Select(x => x.GetValue<int>()). A direct method on JsonArray would be useful for discoverability.

  • I'm missing an easy way to access (if available) PropertyName on JsonNode. I can parse it back out via GetPath or track it on the side during folds but it might be helpful to have an easier way for it?

  • For dynamic data or when converting JsonNode to another tree representation I'm missing any and all useful information on JsonValue. What kind might it be? Which TryGetValue calls are probably going to succeed? Is the expectation here to just probe TryGetValue against likely types? I get that you want to support arbitrary values to support whatever serialization conversions users may want but as a result handling json data with a conventional shape (primitives/JsonElement) is slightly crazy. Do I serialize JsonValue instances to json and deserialize to a JsonElement? IMO this severly limits the usability to just use it as a mutable version of JsonDocument.

  • I really am missing DeepClone, DeepEquals, and Merge. I've written our own versions now but I'm probably not alone. (though again I understand with JsonValue being what it is it would be hard to provide useful and consistent semantics out of the box)

Beyond JsonNode I think a lot of us are still hoping for a 'one-shot' querying method on JsonDocument in the box. I'd be happy with anything given JsonPath was shot down here #31068 (comment). A lot of our code that queries json doesn't need to have a mutable model passed in. Having even the simplest workable querying options on JsonDocument/JsonElement should really come in the box and I'm sad to see any chances for it slip into 7.0.

@dotnet-issue-labeler dotnet-issue-labeler bot added area-System.Text.Json untriaged New issue has not been triaged by the area owner labels Jul 16, 2021
@ghost
Copy link

ghost commented Jul 16, 2021

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

Issue Details

Some feedback after using the new JsonNode apis for some time:

  • For F# use JsonNode is missing a convenience method to safely 'dot into' a deep path. F# does not have a null conditional operator to conveniently intersperse between indexer calls. I've had to add something like the following code to make querying workable.
    type JsonNode with
        member node.GetNode(index: int) = Option.ofObj node.[index]
        member node.GetNode(segment: string) = Option.ofObj node.[segment]
        member node.GetNode([<ParamArray>]pathSegments: obj[]) =
            let mutable current, i = node, 0
            while not (isNull current) && i < pathSegments.Length do
                match pathSegments.[i] with
                | :? int as index -> current <- current.[index]
                | :? string as segment -> current <- current.[segment]
                | _ -> failwith "Unknown path segment type, either int or string is supported."
                i <- i + 1
            Option.ofObj current

It could make sense to provide this in the box but mainly I wanted to note this usability issue for F#.

  • JsonArray is missing a convenience method to get an IEnumerable or array of * GetValue * results out. I'm assuming the alternative pattern is node.AsArray().Select(x => x.GetValue<int>()). A direct method on JsonArray would be useful for discoverability.

  • I'm missing an easy way to access (if available) PropertyName on JsonNode. I can parse it back out via GetPath or track it on the side during folds but it might be helpful to have an easier way for it?

  • For dynamic data or when converting JsonNode to another tree representation I'm missing any and all useful information on JsonValue. What kind might it be? Which TryGetValue calls are probably going to succeed? Is the expectation here to just probe TryGetValue against likely types? I get that you want to support arbitrary values to support whatever serialization conversions users may want but as a result handling json data with a conventional shape (primitives/JsonElement) is slightly crazy. Do I serialize JsonValue instances to json and deserialize to a JsonElement? IMO this severly limits the usability to just use it as a mutable version of JsonDocument.

  • I really am missing DeepClone, DeepEquals, and Merge. I've written our own versions now but I'm probably not alone. (though again I understand with JsonValue being what it is it would be hard to provide useful and consistent semantics out of the box)

Beyond JsonNode I think a lot of us are still hoping for a 'one-shot' querying method on JsonDocument in the box. I'd be happy with anything given JsonPath was shot down here #31068 (comment). A lot of our code that queries json doesn't need to have a mutable model passed in. Having even the simplest workable querying options on JsonDocument/JsonElement should really come in the box and I'm sad to see any chances for it slip into 7.0.

Author: NinoFloris
Assignees: -
Labels:

area-System.Text.Json, untriaged

Milestone: -

@danmoseley
Copy link
Member

Cc @steveharter also

@eiriktsarpalis eiriktsarpalis added needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration and removed untriaged New issue has not been triaged by the area owner labels Jul 23, 2021
@eiriktsarpalis eiriktsarpalis added this to the 6.0.0 milestone Jul 23, 2021
@steveharter
Copy link
Member

steveharter commented Jul 29, 2021

@NinoFloris thanks for the feedback as it helps determine priority and semantics. Most of the suggestions were previously considered, but scoped out for the initial feature set.

For now, I've added sample extensions below to demonstrate the easier ones including:

  • JsonArray.GetValues<T>() : returns a strongly-typed enumerator for T (for homogeneous arrays).
  • GetAccessorFromPath() : returns the "JsonPath" syntax for the parent accessor which is either the property name, the array index with surrounding brackets, or $ for a root node.
  • GetValueKind(): returns the JsonValueKind for a given node.
  • DeepClone() and DeepCopy().

For GetValueKind() you can also call the existing jsonValue.GetValue<object>() to get the underlying value out and do any type-specific logic that way. In "read-mode" (after a JsonNode.Parse()) the underlying value will always be a JsonElement while in "edit-mode" the underlying type can be anything (JsonElement, object, collection, number, etc) depending on what was assigned; the extension handles both read- and edit-mode. I'm also updating the doc to make this more clear.

Click for samples.
using System;
using System.Collections.Generic;
using System.Diagnostics;
using System.Linq;
using System.Text.Json;
using System.Text.Json.Nodes;
using SystemTextJsonExtensions;

namespace ConsoleApp
{
    class Program
    {
        static void Main(string[] args)
        {
            Sample_GetValues();
            Sample_GetAccessorFromPath();
            Sample_GetValueType();
            Sample_DeepClone_DeepEquals();
        }

        static void Sample_GetValues()
        {
            JsonArray jArray = JsonNode.Parse("[1,2]").AsArray();

            int count = 0;
            foreach (int value in jArray.GetValues<int>())
            {
                count += value;
            }
            Debug.Assert(3 == count);

            double countDouble = 0;
            foreach (double value in jArray.GetValues<double>())
            {
                countDouble += value;
            }
            Debug.Assert(3 == countDouble);
        }

        static void Sample_GetAccessorFromPath()
        {
            string propertyAccessor;

            propertyAccessor = JsonNode.Parse("{\"Prop1\":42}")["Prop1"].GetAccessorFromPath();
            Debug.Assert("Prop1" == propertyAccessor);

            propertyAccessor = JsonNode.Parse("{\"Prop1\":{\"Prop2\":42}}")["Prop1"]["Prop2"].GetAccessorFromPath();
            Debug.Assert("Prop2" == propertyAccessor);

            propertyAccessor = JsonNode.Parse("{\"Prop1\":[11,22,33]}")["Prop1"][1].GetAccessorFromPath();
            Debug.Assert("[1]" == propertyAccessor);

            propertyAccessor = JsonNode.Parse("{\"Prop1\":42}").GetAccessorFromPath();
            Debug.Assert("$" == propertyAccessor);

            propertyAccessor = JsonNode.Parse("{}").GetAccessorFromPath();
            Debug.Assert("$" == propertyAccessor);

            propertyAccessor = JsonNode.Parse("[]").GetAccessorFromPath();
            Debug.Assert("$" == propertyAccessor);
        }

        static void Sample_GetValueType()
        {
            JsonValueKind valueKind;

            valueKind = JsonNode.Parse("{}").GetValueKind();
            Debug.Assert(JsonValueKind.Object == valueKind);

            valueKind = JsonNode.Parse("[]").GetValueKind();
            Debug.Assert(JsonValueKind.Array == valueKind);

            valueKind = JsonNode.Parse("null").GetValueKind();

            valueKind = JsonNode.Parse("\"Hello\"").GetValueKind();
            Debug.Assert(JsonValueKind.String == valueKind);

            valueKind = JsonNode.Parse("{\"Prop1\":42}")["Prop1"].GetValueKind();
            Debug.Assert(JsonValueKind.Number == valueKind);

            valueKind = JsonNode.Parse("[1]").GetValueKind();
            Debug.Assert(JsonValueKind.Array == valueKind);

            var anon = new
            {
                String1 = "",
                String2 = DateTime.Now,
                String3 = Guid.NewGuid(),
                Null = (string)null,
                Array = new int[2],
                Object1 = new Dictionary<string, int> { },
                Number1 = (sbyte)1,
                Number2 = (double)3.14,
                Number3 = JsonCommentHandling.Disallow // an Enum
            };

            string json = JsonSerializer.Serialize<object>(anon);
            JsonNode node = JsonNode.Parse(json);

            valueKind = node["String1"].GetValueKind();
            Debug.Assert(JsonValueKind.String == valueKind);

            valueKind = node["String2"].GetValueKind();
            Debug.Assert(JsonValueKind.String == valueKind);

            valueKind = node["String3"].GetValueKind();
            Debug.Assert(JsonValueKind.String == valueKind);

            valueKind = node["Null"].GetValueKind();
            Debug.Assert(JsonValueKind.Null == valueKind);

            valueKind = node["Object1"].GetValueKind();
            Debug.Assert(JsonValueKind.Object == valueKind);

            valueKind = node["Number1"].GetValueKind();
            Debug.Assert(JsonValueKind.Number == valueKind);

            valueKind = node["Number2"].GetValueKind();
            Debug.Assert(JsonValueKind.Number == valueKind);

            valueKind = node["Number3"].GetValueKind();
            Debug.Assert(JsonValueKind.Number == valueKind);
        }

        static void Sample_DeepClone_DeepEquals()
        {
            JsonNode node = JsonNode.Parse("{\"Prop1\":{\"Prop2\":42,\"ArrayProp\":[1]}}");
            JsonNode nodeOther = node.DeepClone();

            int i1 = (int)node["Prop1"]["ArrayProp"][0];
            int i2 = (int)nodeOther["Prop1"]["ArrayProp"][0];

            Debug.Assert(i1 == i2);
            Debug.Assert(i1 == 1);

            string json = node.ToJsonString();
            string jsonClone = nodeOther.ToJsonString();

            Debug.Assert(true == (json == jsonClone));
            Debug.Assert(true == node.DeepEquals(nodeOther));
            Debug.Assert(true == nodeOther.DeepEquals(node));
        }
    }
}

namespace SystemTextJsonExtensions
{
    public static class JsonNodeExtensions
    {
        public static IEnumerable<T> GetValues<T>(this JsonArray jArray)
        {
            return jArray.Select(v => v.GetValue<T>());
        }

        public static string GetAccessorFromPath(this JsonNode node)
        {
            string path = node.GetPath();

            int propertyIndex = path.LastIndexOf('.');
            int arrayIndex = path.LastIndexOf('[');

            if (propertyIndex > arrayIndex)
            {
                return path.Substring(propertyIndex + 1);
            }

            if (arrayIndex > 0)
            {
                return path.Substring(arrayIndex);
            }

            return "$";
        }

        public static JsonValueKind GetValueKind(this JsonNode node, JsonSerializerOptions options = null)
        {
            JsonValueKind valueKind;

            if (node is null)
            {
                valueKind = JsonValueKind.Null;
            }
            else if (node is JsonObject)
            {
                valueKind = JsonValueKind.Object;
            }
            else if (node is JsonArray)
            {
                valueKind = JsonValueKind.Array;
            }
            else 
            {
                JsonValue jValue = (JsonValue)node;

                if (jValue.TryGetValue(out JsonElement element))
                {
                    // Typically this will occur in read mode after a Parse(), so just use the JsonElement.
                    valueKind = element.ValueKind;
                }
                else
                {
                    object obj = jValue.GetValue<object>();


                    if (obj is string)
                    {
                        valueKind = JsonValueKind.String;
                    }
                    else if (IsKnownNumberType(obj.GetType()))
                    {
                        valueKind = JsonValueKind.Number;
                    }
                    else
                    {
                        // Slow, but accurate.
                        string json = jValue.ToJsonString();
                        valueKind = JsonSerializer.Deserialize<JsonElement>(json, options).ValueKind;
                    }
                }
            }

            return valueKind;

            static bool IsKnownNumberType(Type type)
            {
                return type == typeof(sbyte) ||
                    type == typeof(byte) ||
                    type == typeof(short) ||
                    type == typeof(ushort) ||
                    type == typeof(int) ||
                    type == typeof(uint) ||
                    type == typeof(long) ||
                    type == typeof(ulong) ||
                    type == typeof(float) ||
                    type == typeof(double) ||
                    type == typeof(decimal);
            }
        }

        public static JsonNode DeepClone(this JsonNode node, JsonSerializerOptions options = null)
        {
            if (node is null)
            {
                return null;
            }

            string json = node.ToJsonString(options);

            JsonNodeOptions nodeOptions = default;
            if (options != null)
            {
                nodeOptions = new JsonNodeOptions() { PropertyNameCaseInsensitive = options.PropertyNameCaseInsensitive };
            }


            return JsonNode.Parse(json, nodeOptions);
        }

        public static bool DeepEquals(this JsonNode node, JsonNode other, JsonSerializerOptions options = null)
        {
            string json = node.ToJsonString(options);
            string jsonOther = other.ToJsonString(options);

            return json == jsonOther;
        }
    }
}

@steveharter
Copy link
Member

Moving to 7.0; 6.0 is pretty much locked down for new APIs, especially if there are workarounds.

See also the API issue here #56592.

A larger feature may be necessary for JsonPath in general, including querying, that will address the F# issue and others including those from #31068

@eiriktsarpalis
Copy link
Member

Closing in favor of #56592.

@Gonkers
Copy link

Gonkers commented Jan 14, 2022

  • For dynamic data or when converting JsonNode to another tree representation I'm missing any and all useful information on JsonValue. What kind might it be? Which TryGetValue calls are probably going to succeed? Is the expectation here to just probe TryGetValue against likely types? I get that you want to support arbitrary values to support whatever serialization conversions users may want but as a result handling json data with a conventional shape (primitives/JsonElement) is slightly crazy. Do I serialize JsonValue instances to json and deserialize to a JsonElement? IMO this severly limits the usability to just use it as a mutable version of JsonDocument.

Oh, my goodness, thank you @NinoFloris for suggesting this back in July to get it on the radar! I just discovered JsonNode and now that I've been using it, not being able to get the JSON data type is driving me batty.

@ghost ghost locked as resolved and limited conversation to collaborators Feb 13, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants