RicoSuter / NJsonSchema

JSON Schema reader, generator and validator for .NET
http://NJsonSchema.org
MIT License
1.4k stars 535 forks source link

JsonInheritanceConverter adds discriminator without checking for its pre-existence #1085

Open alexbutler-iress-zz opened 5 years ago

alexbutler-iress-zz commented 5 years ago

When trying to use inheritance where the discriminator already exists on the object definition, the templated inheritance converter doesn't check for it. Simple one-line if check would fix this I believe.

public override void WriteJson(Newtonsoft.Json.JsonWriter writer, object value, Newtonsoft.Json.JsonSerializer serializer)
{
  try
  {
    _isWriting = true;

    var jObject = Newtonsoft.Json.Linq.JObject.FromObject(value, serializer);
    if(!jObject.ContainsKey(_discriminator)) // Add this gate
      jObject.AddFirst(new Newtonsoft.Json.Linq.JProperty(_discriminator, GetSubtypeDiscriminator(value.GetType()))); // gate this line
    writer.WriteToken(jObject.CreateReader());
  }
  finally
  {
    _isWriting = false;
  }
}

Please note, the above is currently untested.

alexbutler-iress-zz commented 5 years ago

Although, the ContainsKey is case sensitive, and I'm not entirely sure if JObject.AddFirst is case-sensitive. So this maybe a factor in considering the solution.

RicoSuter commented 5 years ago

I’d say it’s a “bug” if the discriminator is already set. What is the scenario here?

alexbutler-iress-zz commented 5 years ago

We have address objects, UKAddress, OverseasAddress and more, they all extend Address. They are differentiated by a property called AddressType, which we intend to be the discriminator.

The way the contract generation has happen, and the code-gen has happened on the other side, we get an Address object with no properties and several specialisations of address, each with an AddressType, and annoyingly each with their own version of the enumeration for AddressType.