Monday 12 June 2017

Stack Overflow With Custom JsonConverter

[There is a Gist on GitHub that contains a minimal working example and summary of this post.]

We recently needed to change our data model so that what was originally a list of one type, became a list of objects of different types with a common base, i.e. our JSON deserialization now needed to deal with polymorphic types.

Naturally we googled the problem to see what support, if any, Newtonsoft’s JSON.Net had. Although it has some built-in support, like many built-in solutions it stores fully qualified type names which we didn’t want in our JSON, we just wanted simple technology-agnostic type names like “cat” or “dog” that we would be happy to map manually somewhere in our code. We didn’t want to write all the deserialization logic manually, but was happy to give the library a leg-up with the mapping of types.

JsonConverter

Our searching quickly led to the following question on Stack Overflow: “Deserializing polymorphic json classes without type information using json.net”. The lack of type information mentioned in the question meant the exact .Net type (i.e. name, assembly, version, etc.), and so the answer describes how to do it where you can infer the resulting type from one or more attributes in the data itself. In our case it was a field unsurprisingly called “type” that held a simplified name as described earlier.

The crux of the solution involves creating a JsonConverter and implementing the two methods CanConvert and ReadJson. If we follow that Stack Overflow post’s top answer we end up with an implementation something like this:

public class CustomJsonConverter : JsonConverter
{
  public override bool CanConvert(Type objectType)
  {
    return typeof(BaseType).
                       IsAssignableFrom(objectType);
  }

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

    if (item.Value<string>(“type”) == “Derived”)
    {
      return item.ToObject<DerivedType>();
    }
    else
    . . .
  }
}

This all made perfect sense and even agreed with a couple of other blog posts on the topic we unearthed. However when we plugged it in we ended up with an infinite loop in the ReadJson method that resulted in a StackOverflowException. Doing some more googling and checking the Newtonsoft JSON.Net documentation didn’t point out our “obvious” mistake and so we resorted to the time honoured technique of fumbling around with the code to see if we could get this (seemingly promising) solution working.

A Blind Alley

One avenue that appeared to fix the problem was manually adding the JsonConverter to the list of Converters in the JsonSerializerSettings object instead of using the [JsonConverter] attribute on the base class. We went back and forth with some unit tests to prove that this was indeed the solution and even committed this fix to our codebase.

However I was never really satisfied with this outcome and so decided to write this incident up. I started to work through the simplest possible example to illustrate the behaviour but when I came to repro it I found that neither approach worked – attribute or serializer settings - I always got into an infinite loop.

Hence I questioned our original diagnosis and continued to see if there was a more satisfactory answer.

ToObject vs Populate

I went back and re-read the various hits we got with those additional keywords (recursion, infinite loop and stack overflow) to see if we’d missed something along the way. The two main candidates were “Polymorphic JSON Deserialization failing using Json.Net” and “Custom inheritance JsonConverter fails when JsonConverterAttribute is used”. Neither of these explicitly references the answer we initially found and what might be wrong with it – they give a different answer to a slightly different question.

However in these answers they suggest de-serializing the object in a different way, instead of using ToObject<DerivedType>() to do all the heavy lifting, they suggest creating the uninitialized object yourself and then using Populate() to fill in the details, like this:

{
  JObject item = JObject.Load(reader);

  if (item.Value<string>(“type”) == “Derived”)
  {
    var @object = new DerivedType();
    serializer.Populate(item.CreateReader(), @object);
    return @object;
  }
  else
    . . .
}

Plugging this approach into my minimal example worked, and for both the converter techniques too: attribute and serializer settings.

Unanswered Questions

So I’ve found another technique that works, which is great, but I still lack closure around the whole affair. For example, how come the answer in the the original Stack Overflow question “Deserializing polymorphic json classes” didn’t work for us? That answer has plenty of up-votes and so should be considered pretty reliable. Has there been a change to Newtonsoft’s JSON.Net library that has somehow caused this answer to now break for others? Is there a new bug that we’ve literally only just discovered (we’re using v10)? Why don’t the JSON.Net docs warn against this if it really is an issue, or are we looking in the wrong part of the docs?

As described right at the beginning I’ve published a Gist with my minimal example and added a comment to the Stack Overflow answer with that link so that anyone else on the same journey has some other pieces of the jigsaw to work with. Perhaps over time my comment will also acquire up-votes to help indicate that it’s not so cut-and-dried. Or maybe someone who knows the right answer will spot it and point out where we went wrong.

Ultimately though this is probably a case of not seeing the wood for the trees. It’s so easy when you’re trying to solve one problem to get lost in the accidental complexity and not take a step back. Answers on Stack Overflow generally carry a large degree of gravitas, but they should not be assumed to be infallible. All documentation can go out of date even if there are (seemingly) many eyes watching over it.

When your mind-set is one that always assumes the bugs are of your own making, unless the evidence is overwhelming, then those times when you might actually not be entirely at fault seem to feel all the more embarrassing when you realise the answer was probably there all along but you discounted it too early because your train of thought was elsewhere.

6 comments:

  1. I'm following the same thinking that you had and I think I've come to understand what is happening.
    Adding a custom JsonConverter is essentially a short circuit within Newtonsoft.Json. The mistake we're making is to consider our CustomJsonConverter as an overload of existing functionality when really it is a hook into the pipeline.
    I attempted to call base.ReadJson within my customer converter and it created the loop for me.
    So unless we can remove the converter at runtime from that pipeline or create a new one without it, we'll encounter this loop.
    This is essentially what the "deserializing polymorphic classes" question did by not applying the custom converter to sub types (and not using it as an attribute so that it isn't automatically applied). The converter is applied to the base class and then decides which child class to run through the pipeline. Those child classes will not have the Jsonconverter attribute and so will not encounter the loop. It works but in my mind is brittle.
    My solution was to use generics, create an instance of the class and use populate.
    public class CustomConverter : Jsonconverter where T: new()
    public override object ReadJson(...)
    {
    JObject jObject = JObject.Load(reader);
    var instance = new T();
    serializer.Populate(jObject.CreateReader(), instance);
    return instance;
    }

    ReplyDelete
    Replies
    1. Essentially the deserialization engine has many converters and the base type should really be an interface.

      Removing the converter at runtime is basically out of the question, so my generic solution is at least satisfactory for me

      Delete
  2. Brilliant, thank you, best solution to avoid the stack overflow I could find :)

    ReplyDelete
  3. I think the problem is that the JsonConverter attribute is inheritable. When Newtonsoft tries to deserialize the subtype, it sees the same converter attribute. But it doesn't call "CanConvert" to see if the inherited converter applies to the subtype. Pretty clearly a bug in the library IMHO.

    ReplyDelete
  4. Thanks Chris! You saved quite a bit of head scratching.

    ReplyDelete
  5. I am strugging with the same problem, and Peter Rudeman is definitely correct. The problem is that is applied the custom converter when trying to deserialize the implementing (sub)type as well, because it is inherited.

    We had used the "new T()" solution and serializer.Populate, but then suddenly someone introduced a [JsonConstructor] property, and we didn't understand at all why the attribute "wasn't working". Except it was. But we just called the empty one too, in our converter. This is not an easy one. Trying to find a good solution without re-implementing much of the logic here:

    https://github.com/JamesNK/Newtonsoft.Json/blob/master/Src/Newtonsoft.Json/Serialization/JsonSerializerInternalReader.cs#L1934

    ReplyDelete