NullReference when JsonExtensionData dictionary have a custom JsonConverter

[jozkee]

Repro:

private class ClassWithExtensionData
{
    [JsonExtensionData]
    public Dictionary<string, object> Overflow { get; set; }
}

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)
    {
        // Do something.
    }
}

[Fact]
public static void QuickTest()
{
    var root = new ClassWithExtensionData();
    root.Overflow = new Dictionary<string, object>();
    root.Overflow.Add("test", "TestValue");

    var opts = new JsonSerializerOptions();
    opts.Converters.Add(new DictionaryOverflowConverter());

    string json = JsonSerializer.Serialize(root, opts);
}

This only happens in master.

@steveharter @layomia

[layomia]

Note: custom converters for extension properties can only be used on serialization. Deserialization is handled by the serializer.

[ahsonkhan]

can only be used on serialization. Deserialization is handled by the serializer.

That's a bit strange. Is there a reason for that and should we (can we?) fix it?

[jozkee]

@ahsonkhan Overflow properties does not come on a particular order when reading the JSON and the Read method must return a Dictionary<string, JsonElement> or Dictionary<string, object> while overflow properties are parsed to JsonElement entries in such dictionary. That is why we don't use it on deserialization.

[layomia]

can only be used on serialization. Deserialization is handled by the serializer.

That's a bit strange. Is there a reason for that and should we (can we?) fix it?

@ahsonkhan it's because we may not have all the extra JSON in one go - we'd have to parse and wrap all the extension properties in one JSON object, or pass the entire payload to the converter to be filtered.

FWIW Newtonsoft does not honor custom converters for both reading and writing extension data: https://dotnetfiddle.net/qwMrPS. Writing seems doable, so I think the action for this issue is to fix the NRE and make sure custom converters are honored on serialization.

@steveharter, it seems that the assumption in

runtime/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonConverterOfT.cs

Line 283 in 0bf721a

Debug.Assert(this is JsonDictionaryConverter<T>);

is that only internal converters are used for serialization. Any thoughts?

[RezaJooyandeh]

I think for writing it makes sense to support custom serialization.