System.Text.Json.Serialization: Converters are ignored for types derived from supported base types

[phizch]

cc: @layomia,@ahsonkhan, @scalablecory, @steveharter

Related:

• Add support for types derived from supported BCL collections Add support for types derived from supported BCL collections corefx#39001
• Json serializer support for IEnumerable deserialization extensibilty Serializer support for easier object and collection converters #1562
• Add support for generic interface-based collections in JsonSerializer Add support for generic interface-based collections in JsonSerializer corefx#36756
• Json serializer support for collections Json serializer support for collections #29180

Description

Pull dotnet/corefx#39001 implemented by @layomia added support for serialization of types that implements natively supported collections without needing to create converters for them.

Unfortunately the implementation treats all derived types as the base type, so it's not possible to create specialized collections with converters. E.g. class MyCollection : List<int> gets treated as List<int>.

Specialized collections is necessary if the json has multiple ways of declaring a list, e.g.

{
  "SpecialList" : 
  {
    "Count": 2,
    "Values" : [ 1, 2 ]
  },
  "NormalList" : [ 1, 2 ] 
}

public class A
{
    [JsonConverter(typeof(MyCollectionConverter))] // this is ignored and converter for List<int> is used instead
    public MyCollection SpecialList { get; set; }
    public List<int> NormalList { get; set }
}

[JsonConverter(typeof(MyCollectionConverter))] // this is ignored and converter for List<int> is used instead
public class MyCollection : List<int> { }

public class MyCollectionConverter : JsonConverter<MyCollection>
{
    ....
}
public class MyCollectionFactory : JsonConverterFactory
{
	public override bool CanConvert(Type typeToConvert)
    {
        // this only sees List<int>
    }
}
public static void RunTest()
{
    JsonSerializerOptions options = new JsonSerializerOptions
    {
        Converters = 
        { 
            new MyCollectionFactory(), 
            new  MyCollectionConverter() // this is ignored and converter for List<int> is used 
        }
    };

    _ = JsonSerializer.Deserialize<A>("{}", options);
}

Expected priorities for getting the converter:

// Priority 1: attempt to get converter from JsonConverterAttribute on property.
// Priority 2: Attempt to get custom converter added at runtime.
// Priority 3: Attempt to get converter from [JsonConverter] on the type being converted.
// Priority 4: Attempt to get built-in converter.

Current behavior

// Priority 0: if type is derived from supported type, use converter for base type
// Priority 1: attempt to get converter from JsonConverterAttribute on property.
// Priority 2: Attempt to get custom converter added at runtime.
// Priority 3: Attempt to get converter from [JsonConverter] on the type being converted.
// Priority 4: Attempt to get built-in converter.

From JsonClassInfo.AddProperty [source]From JsonClassInfo.AddProperty source:

    // Get implemented type, if applicable.
    // Will return the propertyType itself if it's a non-enumerable, string, or natively supported collection.
    Type implementedType = GetImplementedCollectionType(propertyType);
    if (implementedType != propertyType)
    {
        jsonInfo = CreateProperty(implementedType, implementedType, implementedType, null, typeof(object), options);
    }
    else
    {
        jsonInfo = CreateProperty(propertyType, propertyType, propertyType, propertyInfo, classType, options);
    }

In my example GetImplementedCollectionType(typeof(MyCollection)) will return List<int>.

I'm currently looking into it to see if I can find a fix.

--
Just found another bug: Since propertyInfo is ignored any JsonIgnoreAttribute will also be ignored.
e.g.

public class C
{
    public int Version { get; set; }
    
    [JsonIgnore] // this will be ignored
    public MyCollection { get; set; }
}

[layomia]

@phizch, thanks for the detailed description - I was able to repro this. This is something we should fix for 3.0, and I've opened dotnet/corefx#40411 to address it.

Just found another bug: Since propertyInfo is ignored any JsonIgnoreAttribute will also be ignored.

This has been fixed in dotnet/corefx#40401.

[ahsonkhan]

Re-opening until its fixed for 3.0.

[layomia]

Closed in 3.0 by dotnet/corefx#40432.

[Rauce]

This bug is marked as fixed, although I am seeing a very similar bug in which my JsonConverter is ignored if a property type is Dictionary<string, string>.
Here's my original Stack Overflow post, from which I was pointed here: https://stackoverflow.com/questions/58217491/deserialising-json-using-jsonserializer-deserializeasync-is-not-using-my-jsoncon/58217930

[phizch]

@Rauce
The original issue was about making a custom type that inherited a base type,
e.g class QueryStringDictionary : Dictionary<string,string> {} and having that deserialized from a Json object.

I only reported the issue, and didn't have anything to do with the PR. That said, I did some testing and I can confirm that the example you gave in the stackoverflow question is not working as it should.

It seems like it's ignoring the JsonConverterAttribute value, or maybe it's because it's trying to deserialize from a string, not a more complex type. In any case this should be reported in a new Issue,

A workaround is to use JsonSerializerOptions

class QueryStringToDictionaryJsonConverter : JsonConverter<Dictionary<string,string>>
{
    ...
}
class Test
{
    public Dictionary<string,string> Parameters { get; set; }
}
static void Main( string[] args )
{
    string json = "{ \"Parameters\": \"key1=value1&key2=value2\" }";
    var options = new JsonSerializerOptions
    {
        Converters = { new QueryStringToDictionaryJsonConverter() }
    };
    var result = JsonSerializer.Deserialize<Test>( json, options );  
}

A potential problem is that all Dictionary<string,string> will be handled by the converter.
To avoid that you can make a wrapper and provide a converter for that. e.g :

[JsonConverter( typeof( QueryStringDictionaryConverter ) )]
class QueryStringDictionary : Dictionary<string,string> { }

class QueryStringDictionaryConverter : JsonConverter<QueryStringDictionary>
{
    ... 
}

Hope this helps.

As an aside, it seems like at least one of my cases in this issue still doesn't work;

public class A
{
    [JsonConverter(typeof(MyCollectionConverter))] // no worky
    public MyCollection SpecialList { get; set; }
    public List<int> NormalList { get; set; }
}

It's not critical though, since the preferred way is to set the attribute on the MyCollection class.

[ahsonkhan]

@Rauce, can you please file a separate issue for this so we can track and fix the issue you are observing Thanks!

[ahsonkhan]

@layomia - can you take a look at this:

As an aside, it seems like at least one of my cases in this issue still doesn't work;

public class A
{
    [JsonConverter(typeof(MyCollectionConverter))] // no worky
    public MyCollection SpecialList { get; set; }
    public List<int> NormalList { get; set; }
}

It's not critical though, since the preferred way is to set the attribute on the MyCollection class.

[steveharter]

@layomia - can you take a look at this:
As an aside, it seems like at least one of my cases in this issue still doesn't work;

The JsonConverter attribute not working when applied to a collection is a bug that is expected to be fixed for 3.1