JsonConverter is called despite that CanConvert returns false, and despite that the target type isn't decorated with JsonConverterAttribute

Question

JsonConverter is called despite that CanConvert returns false, and despite that the target type isn't decorated with JsonConverterAttribute

Closed this issue 4 months ago · 6 comments

Source/destination types

[JsonConverter(typeof(QuestionConverter))]
public abstract class Question {
  public required string type { get; set; } = null!;
  public required string questionText { get; set; } = null!;
}

public class SingleLinePlainTextQuestion : Question {
  public SingleLinePlainTextQuestion() {
    type = "single-plain";
  }

  public string placeholder { get; set; } = null!;
}

public class RadioQuestion : Question {
  public RadioQuestion() {
    type = "radio";
  }

  public required ICollection<string> options { get; set; }
}

Source/destination JSON

[
  {
    "type": "single-plain",
    "questionText": "Single-line plain text",
    "placeholder": "Test placeholder"
  },
  {
    "type": "radio",
    "options": [
      "Option 0",
      "Option 1",
      "Option 2"
    ],
    "questionText": "Single selection"
  }
]

TL;DR: a JsonConverter is called despite that CanConvert returns false, and despite that the target type isn't decorated with JsonConverterAttribute.

I store survey questions (single-line plain text or single selection) like this. In the model, there are separate classes for both types of questions, and both classes inherit from an abstract base class Question. The derived classes declare properties specific to concrete question types. I strongly need to be able to deserialize the questions into this model with a simple JsonConvert.DeserializeObject<ICollection<Question>> call.

However, Json.NET isn't able to do that out of the box, so I had to create a custom JsonConverter:

public class QuestionConverter : JsonConverter {
  public override bool CanConvert(Type objectType)
  {
    return objectType == typeof(Question);
  }

  public override object ReadJson(JsonReader reader,
    Type objectType, object? existingValue, JsonSerializer serializer)
  {
    var obj = JObject.Load(reader);

    return (string?)obj["type"] switch
    {
      "single-plain" => obj.ToObject<SingleLinePlainTextQuestion>()!,
      "radio" => obj.ToObject<RadioQuestion>()!,
      _ => throw new ArgumentException("Unknown question type: " + (string?)obj["type"]),
    };
  }

  public override void WriteJson(JsonWriter writer, object? value, JsonSerializer serializer) {
    throw new NotImplementedException();
  }
}

I applied it to the Question type using JsonConverterAttribute.

Debugging shows that the ToObject calls reuse this converter, asking it to convert JSON to SingleLinePlainTextQuestion or RadioQuestion. (This can be seen by objectType passed to ReadJson.) This is IMHO a blatant bug: neither SingleLinePlainTextQuestion nor RadioQuestion are decorated with JsonConverterAttribute, and besides that, CanConvert clearly says that only Questions are allowed!

Expected behavior

ToObject deserializes the source JSON as SingleLinePlainTextQuestion or RadioQuestion (depending on the type value) and passes the result to converter, which, in turn, passes it to client code.

Actual behavior

ToObject calls reuse this converter again and again, which leads to infinite recursion and stack overflow.

Steps to reproduce

string json = "json";
var questions = JsonConvert.DeserializeObject<ICollection<Question>>(json);

Answer 1 · 2024-08-21T12:04:25.000Z

The workaround I'm currently using is this "converter":

public class NoConverter : JsonConverter {
  public override bool CanConvert(Type objectType) => false;
  public override bool CanRead => false;
  public override bool CanWrite => false;

  public override object ReadJson(JsonReader reader,
    Type objectType, object? existingValue, JsonSerializer serializer)
  {
    throw new NotImplementedException();  // will never be called
  }

  public override void WriteJson(JsonWriter writer, object? value, JsonSerializer serializer) {
    throw new NotImplementedException();  // will never be called
  }
}

I applied it to SingleLinePlainTextQuestion and RadioQuestion. This is no more than a workaround, though.

Answer 2 · 2024-08-21T16:29:22.000Z

Json.Net actually has a built in system for using a discriminator field called $type to determine which concrete type to instanciate.

Here is a test program that will use a custom binder to do that. It will serialize and deserialize the data. If you need the exact format shown, I've included a little transformer that will load the old style data.

using Newtonsoft.Json;
using Newtonsoft.Json.Linq;

namespace JamesNK_Newtonsoft.Json_issues_2978;

internal class Program
{
    static void Main(string[] args)
    {
        var qs = new List<Question>()
        {
            new SingleLinePlainTextQuestion()
            {
                QuestionText="Single-line plain text",
                Placeholder="Test placeholder"
            },
            new RadioQuestion()
            {
                QuestionText="Single selection",
                Options = new List<string>()
                {
                    "Option 0",
                    "Option 1",
                    "Option 2"
                }
            }
        };
        var json = JsonConvert.SerializeObject(qs, Formatting.Indented);

        Console.WriteLine(json);

        JsonSerializerSettings settings = new JsonSerializerSettings()
        {
            SerializationBinder = CustomBinder.Instance,
            TypeNameAssemblyFormatHandling = TypeNameAssemblyFormatHandling.Simple,
            TypeNameHandling = TypeNameHandling.All
        };
        var questions = JsonConvert.DeserializeObject<ICollection<Question>>(json, settings);

        foreach(var q in questions!)
        {
            Console.WriteLine(q.ToString());
        }

        // Fixup data in 'old' format
        using var oldJsonStream = System.IO.File.OpenText("test.json");
        using var jr = new JsonTextReader(oldJsonStream);
        var jarr = (JArray)JToken.ReadFrom(jr);
        foreach (var jo in jarr)
        {
            if (jo.First is JProperty prp && prp.Name=="type")
            {
                prp.Replace(new JProperty("$type", prp.Value));
            }
        }

        var oldJson = jarr.ToString();
        
        var oldQuestions = JsonConvert.DeserializeObject<ICollection<Question>>(oldJson, settings);

        foreach (var q in oldQuestions!)
        {
            Console.WriteLine(q.ToString());
        }
    }
}

The updated models are here

using Newtonsoft.Json;

namespace JamesNK_Newtonsoft.Json_issues_2978;

public abstract class Question
{
    [JsonProperty("$type")]
    public abstract string Type { get; }
    [JsonProperty("questionText")]
    public required string QuestionText { get; set; } = null!;
}

public class SingleLinePlainTextQuestion : Question
{
    public SingleLinePlainTextQuestion()
    {
    }
    public override string Type => "single-plain";
    [JsonProperty("placeholder")]
    public string Placeholder { get; set; } = null!;
}

public class RadioQuestion : Question
{
    public RadioQuestion()
    {
    }
    public override string Type => "radio";
    [JsonProperty("options")]
    public required ICollection<string> Options { get; set; }
}

And the binder is here

using Newtonsoft.Json.Serialization;

namespace JamesNK_Newtonsoft.Json_issues_2978;

internal class CustomBinder : Newtonsoft.Json.Serialization.DefaultSerializationBinder
{
    internal static readonly ISerializationBinder Instance = new CustomBinder();

    public override void BindToName(Type serializedType, out string? assemblyName, out string? typeName)
    {
        assemblyName = null;
        typeName = null;
        if (serializedType == typeof(RadioQuestion))
        {
            assemblyName = null;
            typeName = "radio";
            return;
        }
        if (serializedType == typeof(SingleLinePlainTextQuestion))
        {
            assemblyName = null;
            typeName = "single-plain";
            return;
        }
        base.BindToName(serializedType, out assemblyName, out typeName);
    }

    public override Type BindToType(string? assemblyName, string typeName)
    {
        if (assemblyName == null)
        {
            return typeName switch
            {
                "radio" => typeof(RadioQuestion),
                "single-plain" => typeof(SingleLinePlainTextQuestion),
                _ => base.BindToType(assemblyName, typeName)
            };
        }
        return base.BindToType(assemblyName, typeName);
    }
}

Hope this helps.

Answer 3 · 2024-08-21T16:51:08.000Z

See also a slightly updated and optimized version at https://github.com/CZEMacLeod/JamesNK_Newtonsoft.Json_issues_2978

Answer 4 · 2024-08-21T19:55:16.000Z

Thank you @CZEMacLeod! I'll try using this binder when I return from vacation.

If it's not too much trouble, can you please also tell me how does CanConvert work in converters? I thought that, if it returns false, the converter will not be called...

Answer 5 · 2024-08-21T21:49:17.000Z

Using the same model (without the attribute)...

using Newtonsoft.Json.Linq;
using Newtonsoft.Json;

namespace JamesNK_Newtonsoft.Json_issues_2978;

public class QuestionConverter : JsonConverter
{
    public override bool CanRead => true;
    public override bool CanWrite => true;

    public override bool CanConvert(Type objectType) => typeof(Question).IsAssignableFrom(objectType);

    public override object ReadJson(JsonReader reader,
      Type objectType, object? existingValue, JsonSerializer serializer)
    {
        var obj = JObject.Load(reader);

        return (string?)obj["type"] switch
        {
            "single-plain" => obj.ToObject<SingleLinePlainTextQuestion>()!,
            "radio" => obj.ToObject<RadioQuestion>()!,
            _ => throw new ArgumentException("Unknown question type: " + (string?)obj["type"]),
        };
    }

    public override void WriteJson(JsonWriter writer, object? value, JsonSerializer serializer)
    {
        if (value is null)
        {
            writer.WriteNull();
            return;
        }
        writer.WriteStartObject();
        writer.WritePropertyName("type");
        writer.WriteValue(value switch
        {
            SingleLinePlainTextQuestion _ => "single-plain",
            RadioQuestion _ => "radio",
            _ => throw new ArgumentException("Unknown question type: " + value.GetType().Name),
        });
        var jobj = JToken.FromObject(value);
        foreach (var item in jobj)
        {
            writer.WriteToken(item.CreateReader());
        }
        writer.WriteEndObject();
    }
}

        using var oldJsonStream = System.IO.File.OpenText("test.json");
        using var jr = new JsonTextReader(oldJsonStream);

        // Converters are queried for each item type in the tree...
        JsonSerializerSettings settings = new JsonSerializerSettings()
        {
            Converters =
            {
                new QuestionConverter()
            },
            Formatting = Formatting.Indented
        };
        var serializer = JsonSerializer.Create(settings);
        var oldQuestions = serializer.Deserialize<ICollection<Question>>(jr);

        foreach (var q in oldQuestions!)
        {
            Console.WriteLine(q.ToString());
        }

        var json = JsonConvert.SerializeObject(oldQuestions, settings);
        Console.WriteLine(json);

JsonConverter is mainly used when you want to manually control the serialization format. Say to a single string and back again. E.g. ISODate or UnixEpoch type conversions on DateTime or say Enum <=> String.
You can force one to always be called for an object (and anything inheriting from that object) like you originally tried - but in that case the CanConvert is ignored (as it is already chosen by the attribute). Otherwise you setup your serializer with all the converters (or add them to the global default), and then your new type of ser/deser will be used on any objects that match your criteria.
Part of the problem is cascading this when you want to use the same settings/serializer on child objects as you end up with recusive loops that stack overflow. There are some workarounds, but the solution to your problem of using an abstract type is the discriminator field and using the binder mechanism.

Answer 6 · 2024-08-22T17:25:33.000Z

So this is a feature — when a class is decorated with JsonConverterAttribute, CanConvert is ignored when working with that class and its subclasses.

Thank you again @CZEMacLeod. And apologies for reporting not-a-bug.