System.Text.Json custom converter for Dictionary<string, object> collides with JsonExtensionData

[florianbader]

Description

Implementing a custom converter for a Dictionary<string, object> is used to serialize JsonExtensionData properties but there is no way for the converter to know if it is serializing an extension property or a dictionary property. Because of that invalid JSON is generated.

Reproduction Steps

See fiddle: https://dotnetfiddle.net/VIrtyu

Expected behavior

The custom converter should have a way to check if the current write operation value is json extension data.
{"RegularData":{"World":"Hello"},"Hello":"World"}

Actual behavior

The custom converter does not know if it json extension data and therefore writes an object instead of weaving the properties into the parent.
{"RegularData":{"World":"Hello"},{"Hello":"World"}}

Regression?

No response

Known Workarounds

No response

Configuration

.NET 5, System.Text.Json 5.0.2

Other information

Might be related to #32903

[jeffhandley]

Tagging @dotnet/area-system-text-json since the bot didn't.

[eiriktsarpalis]

Can reproduce, minimal repro:

using System;
using System.Collections.Generic;
using System.Text.Json;
using System.Text.Json.Serialization;

var value = new MyClass { ExtData = { ["Hello"] = "World" } };
var settings = new JsonSerializerOptions { Converters = { new CustomDictionaryConverter() } };
string json = JsonSerializer.Serialize(value, settings);
Console.WriteLine(json); // {42}

public class MyClass
{
    [JsonExtensionData]
    public Dictionary<string, object> ExtData { get; set; } = new Dictionary<string, object>();
}

public class CustomDictionaryConverter : JsonConverter<Dictionary<string, object?>>
{
    public override Dictionary<string, object?> Read(ref Utf8JsonReader reader, Type? typeToConvert, JsonSerializerOptions options)
        => throw new NotSupportedException();

    public override void Write(Utf8JsonWriter writer, Dictionary<string, object?> values, JsonSerializerOptions options)
        => writer.WriteNumberValue(42);
}

This is a bug, I think we need to make JsonExtensionData properties ignore custom converters, since it is bound to generate invalid JSON.

[nextdracool]

As shown in #60806 this also happens when using JsonObject. As a workaround you can define a converter, that writes the JsonObject's properties directly without writing the start and end of an object. You can then successfully serialize the object, but deserialization causes an exception: System.InvalidOperationException: 'A custom converter for JsonObject is not allowed on an extension property.'.

Given that it is apparently not allowed to add a custom converter to the JsonObject extension property during deserialization it would make sense to ignore present custom converters during serialization as well. Any explicitly added custom converter should probably throw the same exception when serializing as it does when deserializng.

[eiriktsarpalis]

Minimal reproduction using JsonObject without a custom converter:

using System;
using System.Text.Json;
using System.Text.Json.Nodes;
using System.Text.Json.Serialization;

Person value = new() { Extensions = new() { ["Key"] = 42 } };
Console.WriteLine(JsonSerializer.Serialize(value));

public class Person
{
    [JsonExtensionData]
    public JsonObject? Extensions { get; set; }
}

[eiriktsarpalis]

Upon closer inspection, it appears that this is by-design behavior. Registering custom converters for extension data properties is a supported scenario, but converters need to be written in a way that accounts for the fact that the types are being used as extension data:

runtime/src/libraries/System.Text.Json/tests/Common/ExtensionDataTests.cs

Lines 366 to 403 in cd75ae1

	public class DictionaryOverflowConverter : JsonConverter<Dictionary<string, object>>
	{
	public override Dictionary<string, object> Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)
	{
	throw new NotImplementedException();
	}
	public override void Write(Utf8JsonWriter writer, Dictionary<string, object> value, JsonSerializerOptions options)
	{
	writer.WriteString("MyCustomOverflowWrite", "OverflowValueWrite");
	}
	}
	public class JsonElementOverflowConverter : JsonConverter<Dictionary<string, JsonElement>>
	{
	public override Dictionary<string, JsonElement> Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)
	{
	throw new NotImplementedException();
	}
	public override void Write(Utf8JsonWriter writer, Dictionary<string, JsonElement> value, JsonSerializerOptions options)
	{
	writer.WriteString("MyCustomOverflowWrite", "OverflowValueWrite");
	}
	}
	public class JsonObjectOverflowConverter : JsonConverter<JsonObject>
	{
	public override JsonObject Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)
	{
	throw new NotImplementedException();
	}
	public override void Write(Utf8JsonWriter writer, JsonObject value, JsonSerializerOptions options)
	{
	writer.WriteString("MyCustomOverflowWrite", "OverflowValueWrite");
	}
	}

Writing converters in this way violates the invariants of JsonConverter, and almost certainly renders the types non-serializable for any usage beyond extension data. Conversely, using a custom JsonConverter that honors the serialization invariants for the type will result in invalid JSON being produced by the serializer (this is because JsonSerializerOptions is skipping validation for Utf8JsonWriter by default).

My recommendation would be to introduce a breaking change here, I don't see real value in customizing extension data serialization and the current behavior sets up users for emitting invalid JSON, even though they might have never intended to customize extension data serialization.

@krwq @steveharter thoughts?

[krwq]

@eiriktsarpalis I agree extension data properties should not use global converters (whatever is set on options or type) but it should still respect any JsonConverter attribute on the property and CustomConverter on JsonPropertyInfo

[eiriktsarpalis]

it should still respect any JsonConverter attribute on the property and CustomConverter on JsonPropertyInfo

The problem is that it cannot do that unless the converter has been written in way such that properties are emitted without any curly braces (see example above). It's a pit of failure that breaks the composition model for converters (hence the invalid JSON being reported in this issue). I think we should consider making a breaking change in the future.

[amis92]

Just wanted to say, using JsonNode for JsonExtensionData underlying type is very convenient in terms of usage, and works today for deserialization (it's not documented, but there's a unit test supporting it). Also arrived here from #60806 . Making it work for serialization correctly would be appreciated. It's also very problematic because, as mentioned, fixing it is virtually impossible (adding converter to make serialization work disables deserialization).

Supporting unit test:

runtime/src/libraries/System.Text.Json/tests/Common/ExtensionDataTests.cs

Lines 786 to 795 in b6115b0

	[Fact]
	public async Task DeserializeIntoJsonObjectProperty()
	{
	string json = @"{""MyDict"":{""Property1"":1}}";
	ClassWithExtensionPropertyAsJsonObject obj =
	await Serializer.DeserializeWrapper<ClassWithExtensionPropertyAsJsonObject>(json);
	Assert.Equal(1, obj.MyOverflow.Count);
	Assert.Equal(1, obj.MyOverflow["MyDict"]["Property1"].GetValue<int>());
	}

[layomia]

Just wanted to say, using JsonNode for JsonExtensionData underlying type is very convenient in terms of usage, and works today for deserialization (it's not documented, but there's a unit test supporting it). Also arrived here from #60806 . Making it work for serialization correctly would be appreciated. It's also very problematic because, as mentioned, fixing it is virtually impossible (adding converter to make serialization work disables deserialization).

Supporting unit test:

runtime/src/libraries/System.Text.Json/tests/Common/ExtensionDataTests.cs

Lines 786 to 795 in b6115b0

	[Fact]
	public async Task DeserializeIntoJsonObjectProperty()
	{
	string json = @"{""MyDict"":{""Property1"":1}}";
	ClassWithExtensionPropertyAsJsonObject obj =
	await Serializer.DeserializeWrapper<ClassWithExtensionPropertyAsJsonObject>(json);
	Assert.Equal(1, obj.MyOverflow.Count);
	Assert.Equal(1, obj.MyOverflow["MyDict"]["Property1"].GetValue<int>());
	}

@amis92 this strikes me as a separate issue. Can you please open a new one with this info, also describing the issues you're having with serialization?

[springy76]

Just having written code on net7 which overwrote some json files on which the reading app now crashes because they are invalid now due to this problem - now I remember having tried the same on net6 and got weird results, too; by the time thought I was using something in beta stage.

Ok, what exactly tells me here that JsonObject cannot be used for serialization but only deserialization?

// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

namespace System.Text.Json.Serialization
{
    /// <summary>
    /// When placed on a property or field of type <see cref="System.Text.Json.Nodes.JsonObject"/> or
    /// <see cref="System.Collections.Generic.IDictionary{TKey, TValue}"/>, any properties that do not have a
    /// matching property or field are added during deserialization and written during serialization.
    /// </summary>
    /// <remarks>
    /// When using <see cref="System.Collections.Generic.IDictionary{TKey, TValue}"/>, the TKey value must be <see cref="string"/>
    /// and TValue must be <see cref="JsonElement"/> or <see cref="object"/>.
    ///
    /// During deserializing with a <see cref="System.Collections.Generic.IDictionary{TKey, TValue}"/> extension property with TValue as
    /// <see cref="object"/>, the type of object created will either be a <see cref="System.Text.Json.Nodes.JsonNode"/> or a
    /// <see cref="JsonElement"/> depending on the value of <see cref="System.Text.Json.JsonSerializerOptions.UnknownTypeHandling"/>.
    ///
    /// If a <see cref="JsonElement"/> is created, a "null" JSON value is treated as a JsonElement with <see cref="JsonElement.ValueKind"/>
    /// set to <see cref="JsonValueKind.Null"/>, otherwise a "null" JSON value is treated as a <c>null</c> object reference.
    ///
    /// During serializing, the name of the extension data member is not included in the JSON;
    /// the data contained within the extension data is serialized as properties of the JSON object.
    ///
    /// If there is more than one extension member on a type, or the member is not of the correct type,
    /// an <see cref="InvalidOperationException"/> is thrown during the first serialization or deserialization of that type.
    /// </remarks>
    [AttributeUsage(AttributeTargets.Property | AttributeTargets.Field, AllowMultiple = false)]
    public sealed class JsonExtensionDataAttribute : JsonAttribute
    {
    }
}

Doesn't look like "not documented" to me.

[layomia]

Ok, what exactly tells me here that JsonObject cannot be used for serialization but only deserialization?

It appears to be supported for serialization. What do you expect for the following?

using System;
using System.Text.Json;
using System.Text.Json.Nodes;
using System.Text.Json.Serialization;
					
public class Program
{
    public static void Main()
    {
	JsonObject overflow = new JsonObject() { ["Hello"] = "World" };
	ClassWithExtensionPropertyAsJsonObject obj = new() { MyOverflow = overflow  };
        string json = JsonSerializer.Serialize(overflow);
        Console.WriteLine(json);
    }
	
    public class ClassWithExtensionPropertyAsJsonObject
    {
	[JsonExtensionData]
        public JsonObject MyOverflow { get; set; }
    }
}

I expect {"Hello":"World"}.

[springy76]

I expect a Json serializer to generate json - and not something which looks like json but cannot be parsed by any json parser out in the world. But the latter is what happens.

[amis92]

Just wanted to say, using JsonNode for JsonExtensionData underlying type is very convenient in terms of usage, and works today for deserialization (it's not documented, but there's a unit test supporting it). Also arrived here from #60806 . Making it work for serialization correctly would be appreciated. It's also very problematic because, as mentioned, fixing it is virtually impossible (adding converter to make serialization work disables deserialization).
Supporting unit test:

runtime/src/libraries/System.Text.Json/tests/Common/ExtensionDataTests.cs

Lines 786 to 795 in b6115b0

	[Fact]
	public async Task DeserializeIntoJsonObjectProperty()
	{
	string json = @"{""MyDict"":{""Property1"":1}}";
	ClassWithExtensionPropertyAsJsonObject obj =
	await Serializer.DeserializeWrapper<ClassWithExtensionPropertyAsJsonObject>(json);
	Assert.Equal(1, obj.MyOverflow.Count);
	Assert.Equal(1, obj.MyOverflow["MyDict"]["Property1"].GetValue<int>());
	}

@amis92 this strikes me as a separate issue. Can you please open a new one with this info, also describing the issues you're having with serialization?

@layomia I believe it's the same issue. This one was opened specifically for custom converters for [JsonExtensionData] Dictionary<string, object>, but quickly was widened to include JsonExtensionData on JsonObject (as it is also caused by a converter, although built-in - original issue #60806 was closed in favor of this one), and I believe the same issue is with JsonNode (issue being built-in converter used for extension data property).

[layomia]

In general I think a separate, internal converter should be used to handle extension data. We could support a custom converter applied with [JsonConverter] attribute. Taking a breaking change early in the release to get more sensible semantics might be worth it.

[fknshwow]

Has there been any progress on this issue? Or even a work around for the time being?

This has been open for a while, I hope there is still appetite to get this resolved.

[eiriktsarpalis]

@lukecusolito it's been parked for a while since upon closer inspection this is by design, see #60560 (comment). I still think it needs to be fixed, but that would need to be flagged as a breaking change and we should be providing a workaround for users that do depend on the current behavior. It certainly won't be changed for .NET 8.

[wzchua]

The documentation should be updated to not say you can use JsonObject

[fknshwow]

@eiriktsarpalis Look a good workaround would be fantastic if one can be provided. The system I am building is dependent on on dynamic data, this is the only piece of the puzzle that does not work.

The current workaround I have in place is to serialize my JsonObject into a string and then deserialize to a Dictionary<string, JsonElement> then assigning to a JsonExtensionData property on my response class. It sucks, but it is 'temporary' until I come up with something better.

The problem is from what I can see, I doesn't look like there is anyway to convert a JsonObject into either a JsonDocument or a JsonElement without first converting to and from a string due to their immutable nature.

I am going to explore a JsonConverter next, if you have a good workaround I would be greatly appreciative. I will post what I have if I can get it to work.

If support for this is dropped, then there needs to be an efficient way to convert to a JsonDocument or JsonElement, maybe some way to parse from a JsonObject

[fknshwow]

Okay, I have 2 potential workarounds for anyone interested. Keeping in mind that my requirements only need me to write to an API response object so my samples are only going one way.

Option 1 - Custom Converter
This is a very basic implementation, I think there could be some modifications to consider null values. It basically loops through the root elements of the JsonObject and writes their property name and value using the JsonNode.WriteTo() method. Note that the JsonExtensionData attribute is still required on the response class

public class CustomJsonObjectOverflowConverter : JsonConverter<JsonObject>
{
    public override JsonObject Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)
    {
        throw new NotImplementedException();
    }

    public override void Write(Utf8JsonWriter writer, JsonObject value, JsonSerializerOptions options)
    {
        foreach (var item in value)
        {
            writer.WritePropertyName(item.Key);

            item.Value.WriteTo(writer, options);
        }
    }
}

public class JsonObjectResponse
{
    // JsonExtensionData attribute is still required. If not supplied the property name ("Value") still gets written to the response body
    [JsonExtensionData]
    [JsonConverter(typeof(CustomJsonObjectOverflowConverter))]
    public JsonObject Value { get; set; }

    ...
}

Option 2 - Convert type
This will take a JsonObject and convert it to a Dictionary<string, JsonElement>. This is clean but means the data will be written twice, once in the constructor when we deserialize to a dictionary and again when the API writes the response. From what I understand, JsonNode.Deserialize() internally works much like custom converter from option 1, it too will write using a Utf8JsonWriter and does not need to go to and from a string

public class JsonObjectResponse
{
    public JsonObjectResponse(JsonObject jsonObject)
    {
        Value = jsonObject.Deserialize<Dictionary<string, JsonElement>>();
    }
    
    [JsonExtensionData]
    public Dictionary<string, JsonElement> Value { get; set; }
    
    ...
}

I am interested in your thoughts on these workarounds. I feel option 2 is safer as it uses first party mechanisms but double handling the data might have an impact on performance

[bachratyg]

Option 1 - Custom Converter

Unfortunately this does not work for deserialization. JsonConverter.ReadElementAndSetProperty throws and should be overriden just like in JsonObjectConverter, but it's internal.

[JanEggers]

same issue here with ProblemDetails retuned by asp.net