Duplicate object keys shouldn't result in an ArgumentException

[gregsdennis]

Description

Currently, when a parsed JSON object has duplicate keys, an ArguementException is thrown from the underlying dictionary implementation. I would expect this to be thrown if I were manually attempting to add an existing key, but not from parsing.

Reproduction Steps

var json = JsonNode.Parse("[{\"foo\":1, \"bar\":2, \"foo\":3}]") as JsonArray;

var item = json[0]["bar"];

Expected behavior

Maybe throw JsonException?

Actual behavior

ArgumentException is thrown. Stack trace:

System.ArgumentException : An item with the same key has already been added. Key: {0} (Parameter 'foo')
   at System.Text.Json.ThrowHelper.ThrowArgumentException_DuplicateKey(String propertyName)
   at System.Text.Json.JsonPropertyDictionary`1.AddValue(String propertyName, T value)
   at System.Text.Json.JsonPropertyDictionary`1.Add(String propertyName, T value)
   at System.Text.Json.Nodes.JsonObject.InitializeIfRequired()
   at System.Text.Json.Nodes.JsonObject.System.Collections.Generic.IDictionary<System.String,System.Text.Json.Nodes.JsonNode>.TryGetValue(String propertyName, JsonNode& jsonNode)
   at System.Text.Json.Nodes.JsonObject.TryGetPropertyValue(String propertyName, JsonNode& jsonNode)
   at System.Text.Json.Nodes.JsonObject.GetItem(String propertyName)
   at System.Text.Json.Nodes.JsonNode.get_Item(String propertyName)

Regression?

No response

Known Workarounds

No response

Configuration

Seen in all released versions (.Net 6 and previous)

Other information

No response

[ghost]

Tagging subscribers to this area: @dotnet/area-system-text-json, @gregsdennis
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Description

Currently, when a parsed JSON object has duplicate keys, an ArguementException is thrown from the underlying dictionary implementation. I would expect this to be thrown if I were manually attempting to add an existing key, but not from parsing.

Reproduction Steps

var json = JsonNode.Parse("[{\"foo\":1, \"bar\":2, \"foo\":3}]") as JsonArray;

var item = json[0]["bar"];

Expected behavior

Maybe throw JsonException?

Actual behavior

ArgumentException is thrown. Stack trace:

System.ArgumentException : An item with the same key has already been added. Key: {0} (Parameter 'foo')
   at System.Text.Json.ThrowHelper.ThrowArgumentException_DuplicateKey(String propertyName)
   at System.Text.Json.JsonPropertyDictionary`1.AddValue(String propertyName, T value)
   at System.Text.Json.JsonPropertyDictionary`1.Add(String propertyName, T value)
   at System.Text.Json.Nodes.JsonObject.InitializeIfRequired()
   at System.Text.Json.Nodes.JsonObject.System.Collections.Generic.IDictionary<System.String,System.Text.Json.Nodes.JsonNode>.TryGetValue(String propertyName, JsonNode& jsonNode)
   at System.Text.Json.Nodes.JsonObject.TryGetPropertyValue(String propertyName, JsonNode& jsonNode)
   at System.Text.Json.Nodes.JsonObject.GetItem(String propertyName)
   at System.Text.Json.Nodes.JsonNode.get_Item(String propertyName)

Regression?

No response

Known Workarounds

No response

Configuration

Seen in all released versions (.Net 6 and previous)

Other information

No response

Author:	gregsdennis
Assignees:	-
Labels:	area-System.Text.Json
Milestone:	-

[krwq]

since this is easy enough let's try to fix this in 7.0

[eiriktsarpalis]

I'm removing the easy label since changing this would require decoupling the implementation from JsonPropertyDictionary. Note that the same type is shared with the property cache in JsonTypeInfo, so we can't really address the issue here without subtly breaking member visibility in contract resolution logic. I think we should try to replace JsonPropertyDictionary with bespoke implementations in the corresponding scenaria, but that is nontrivial.

[AigioL]

There are some third-party webapi that return json with duplicate keys, which is allowed in the Newtonsoft.Json.

Now using the following code reflection modification fixes the problem, but will the November .NET 8 RTM fix it?

        const string json0 =
"""
{
  "test1": "Hello World",
  "test1": "2",
  "test1": "#",
  "test1": "saf",
  "test1": "😋",
  "test2": 22,
  "test3": false
}
""";
        var obj = System.Text.Json.JsonSerializer.Deserialize<System.Text.Json.Nodes.JsonObject>(json0);
        TestContext.WriteLine(obj);
        var value = obj.GetValue(() =>
        {
            var value = obj?["test1"]?.ToString();
            return value;
        });
        Assert.That(value, Is.EqualTo("😋"));

public static partial class JsonObjectExtensions
{
    [RequiresUnreferencedCode("JSON serialization and deserialization might require types that cannot be statically analyzed. Use the overload that takes a JsonTypeInfo or JsonSerializerContext, or make sure all of the required types are preserved."]
    [RequiresDynamicCode("JSON serialization and deserialization might require types that cannot be statically analyzed and might need runtime code generation. Use System.Text.Json source generation for native AOT applications.")]
    public static T? GetValue<T>(this System.Text.Json.Nodes.JsonObject? obj, Func<T?> func, bool checknull = true) where T : notnull
    {
        if (checknull && obj == null)
            return default;
        try
        {
            return func();
        }
        catch (ArgumentException)
        {
            /* 
        System.ArgumentException : An item with the same key has already been added. Key: test1 (Parameter 'propertyName')

      StackTrace:
        ThrowHelper.ThrowArgumentException_DuplicateKey(String paramName, String propertyName)
        JsonObject.InitializeIfRequired()
        JsonNode.get_Item(String propertyName)
             */
            InternalReflection.InitializeDictionary(obj);
            return func();
        }
    }

    [RequiresUnreferencedCode("JSON serialization and deserialization might require types that cannot be statically analyzed. Use the overload that takes a JsonTypeInfo or JsonSerializerContext, or make sure all of the required types are preserved.")]
    [RequiresDynamicCode("JSON serialization and deserialization might require types that cannot be statically analyzed and might need runtime code generation. Use System.Text.Json source generation for native AOT applications."]
    static partial class InternalReflection
    {
        static readonly Lazy<MethodInfo> _GetUnderlyingRepresentation = new(() =>
        {
            var typeOfJObject = typeof(System.Text.Json.Nodes.JsonObject);
            var methodGetUnderlyingRepresentation = typeOfJObject.GetMethod("GetUnderlyingRepresentation", BindingFlags.Instance | BindingFlags.NonPublic);
            object?[] parameters = [null, null];
            return methodGetUnderlyingRepresentation.ThrowIsNull();
        });

        static void GetUnderlyingRepresentation(System.Text.Json.Nodes.JsonObject? obj, out object? dictionary, out JsonElement? jsonElement)
        {
            object?[] parameters = [null, null];
            _GetUnderlyingRepresentation.Value.Invoke(obj, parameters);
            dictionary = parameters[0];
            jsonElement = (JsonElement?)parameters[1];
        }

        static readonly Lazy<Type> _CreateJsonPropertyDictionaryJsonNode = new(() =>
        {
            var typeOfJObject = typeof(System.Text.Json.Nodes.JsonObject);
            var typeOfJsonPropertyDictionary = typeOfJObject.Assembly.GetType("System.Text.Json.JsonPropertyDictionary`1", true);
            var typeOfJsonPropertyDictionaryMakeGeneric = typeOfJsonPropertyDictionary.ThrowIsNull().MakeGenericType(typeof(JsonNode));
            return typeOfJsonPropertyDictionaryMakeGeneric.ThrowIsNull();
        });

        static object? CreateJsonPropertyDictionaryJsonNode(bool caseInsensitive) => Activator.CreateInstance(_CreateJsonPropertyDictionaryJsonNode.Value, [caseInsensitive]);

        /// <summary>
        /// https://github.com/dotnet/runtime/blob/v8.0.0-rc.2.23479.6/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Node/JsonNodeConverter.cs#L59
        /// </summary>
        static readonly Lazy<MethodInfo> _JsonNodeConverterCreate = new(() =>
        {
            var typeOfJObject = typeof(System.Text.Json.Nodes.JsonObject);
            var typeOfJsonNodeConverter = typeOfJObject.Assembly.GetType("System.Text.Json.Serialization.Converters.JsonNodeConverter", true);
            var methodCreateJsonNode = typeOfJsonNodeConverter.ThrowIsNull().GetMethod("Create", BindingFlags.Static | BindingFlags.Public);
            return methodCreateJsonNode.ThrowIsNull();
        });

        static JsonNode? JsonNodeConverterCreate(JsonElement element, JsonNodeOptions? options) => (JsonNode?)_JsonNodeConverterCreate.Value.Invoke(null, [element, options]);

        static readonly Lazy<PropertyInfo> _SetParent = new(() =>
        {
            var propertyParent = typeof(JsonNode).GetProperty(nameof(JsonNode.Parent));
            return propertyParent.ThrowIsNull();
        });

        static void SetPropertyParent(JsonNode jsonNode, JsonNode parent) => _SetParent.Value.SetValue(jsonNode, parent);

        static readonly Lazy<FieldInfo> _SetFieldDictionary = new(() =>
        {
            var typeOfJObject = typeof(System.Text.Json.Nodes.JsonObject);
            var fieldDictionary = typeOfJObject.GetField("_dictionary", BindingFlags.Instance | BindingFlags.NonPublic);
            return fieldDictionary.ThrowIsNull();
        });

        static void SetFieldDictionary(System.Text.Json.Nodes.JsonObject obj, object? value) => _SetFieldDictionary.Value.SetValue(obj, value);

        static readonly Lazy<FieldInfo> _SetFieldJsonElement = new(() =>
        {
            var typeOfJObject = typeof(System.Text.Json.Nodes.JsonObject);
            var fieldDictionary = typeOfJObject.GetField("_jsonElement", BindingFlags.Instance | BindingFlags.NonPublic);
            return fieldDictionary.ThrowIsNull();
        });

        static void SetFieldJsonElement(System.Text.Json.Nodes.JsonObject obj, object? value) => _SetFieldJsonElement.Value.SetValue(obj, value);

        static readonly Lazy<MethodInfo> _JsonPropertyDictionaryJsonNodeTryAddValue = new(() =>
        {
            var typeOfJObject = typeof(System.Text.Json.Nodes.JsonObject);
            var typeOfJsonPropertyDictionary = typeOfJObject.Assembly.GetType("System.Text.Json.JsonPropertyDictionary`1", true);
            var typeOfJsonPropertyDictionaryMakeGeneric = typeOfJsonPropertyDictionary.ThrowIsNull().MakeGenericType(typeof(JsonNode));
            var methodTryAddValue = typeOfJsonPropertyDictionaryMakeGeneric.GetMethod("TryAddValue", BindingFlags.Instance | BindingFlags.NonPublic);
            return methodTryAddValue.ThrowIsNull();
        });

        static void JsonPropertyDictionaryJsonNodeTryAddValue(object? dictionary, string? propertyName, JsonNode? value) => _JsonPropertyDictionaryJsonNodeTryAddValue.Value.Invoke(dictionary, [propertyName, value]);

        public static void InitializeDictionary(System.Text.Json.Nodes.JsonObject? obj)
        {
            if (obj == null) return;
            // https://github.com/dotnet/runtime/blob/v8.0.0-rc.2.23479.6/src/libraries/System.Text.Json/src/System/Text/Json/Nodes/JsonObject.IDictionary.cs#L196-L225
            GetUnderlyingRepresentation(obj, out var dictionary, out var jsonElement);
            if (dictionary == null)
            {
                bool caseInsensitive = obj.Options.HasValue && obj.Options.Value.PropertyNameCaseInsensitive;
                dictionary = CreateJsonPropertyDictionaryJsonNode(caseInsensitive);
                if (jsonElement.HasValue)
                {
                    IEnumerable<System.Text.Json.JsonProperty> items = jsonElement.Value.EnumerateObject();
                    foreach (var item in items.Reverse()) // Loop in reverse order to take the value of the last one when repeated.
                    {
                        var jsonNode = JsonNodeConverterCreate(item.Value, obj.Options);
                        if (jsonNode != null)
                        {
                            SetPropertyParent(jsonNode, obj);
                        }
                        JsonPropertyDictionaryJsonNodeTryAddValue(dictionary, item.Name, jsonNode);
                    }
                }
                SetFieldDictionary(obj, dictionary);
                Interlocked.MemoryBarrier();
                SetFieldJsonElement(obj, null);
            }
        }
    }
}

[eiriktsarpalis]

will the November .NET 8 RTM fix it?

Unfortunately, no. Development for .NET 8 concluded back in August and any work happening since targets .NET 9.

[BSS05]

Hi @gregsdennis,
Is this fixed? I still get this issue in latest version. I have no idea to fix it.

[gregsdennis]

From Eirik's comment above, I would guess not.

[asklar]

I just ran into this. In my case, I get the json from some web api. Merely calling Count on the JsonObject, or iterating through its child properties, results in the ArgumentException...
I tried using PropertyNamingPolicy in the deserialization options, but I don't see that getting called (maybe it's only for serialization?).

Is there a workaround to either allow multiple entries with the same name, allow the caller to rename duplicate entries, or at least ignore re-definitions or something?

As it is, the current behavior presents a problem to adopt System.Text.Json.

[eiriktsarpalis]

We would be happy to accept a PR that addresses the issue, since we can't prioritise a fix for the moment.