Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Be able to support custom inheritance discriminator field #1331

Closed
manuc66 opened this issue May 31, 2017 · 23 comments
Closed

Be able to support custom inheritance discriminator field #1331

manuc66 opened this issue May 31, 2017 · 23 comments

Comments

@manuc66
Copy link

manuc66 commented May 31, 2017

Source JSON

{
  "pets": [
    {
      "className": "Cat",
      "color": "white",
      "breed": "Aidi"
    },
    {
      "className": "Dog",
      "color": "white",
      "declawed": false
    }
  ]
}

destination types

class Root {
    List<Annimal> pets { get; set;}   
}

class Annimal {
    string ClassName { get; set;} // not mandatory
    string Color { get; set;}
}

class Dog : Annimal {
    string breed { get; set;}
}

class Cat : Annimal {
    bool declawed { get; set;}
}

Expected behavior

Be able to serialize the given json with few elegant configuration properties. Concretely if we were able to configure by some way the JsonTypeReflector.TypePropertyName per inheritance hierarchy it might help us.

This would be very helpful to support the

Actual behavior

Actually we are forced to implement some sub-optimal/complex solutions, see: https://stackoverflow.com/questions/9490345/json-net-change-type-field-to-another-name

@MihaMarkic
Copy link

Heck, out there are people using all sort of field names. I'm really curious why is this a constant with no options to override.

@JamesNK
Copy link
Owner

JamesNK commented Jun 11, 2017

Because you have to draw a line at what point you stop allow customization. I have decided to draw the line here.

@JamesNK JamesNK closed this as completed Jun 11, 2017
@MihaMarkic
Copy link

@JamesNK Ok, but know that this isn't a purely visual preference. There are a lot of different field names used for that purpose and it really hampers json.net in such case when field isn't named $type.
I don't understand why do you insist on a const. Make it a property with a default value, what's the big deal?

@manuc66
Copy link
Author

manuc66 commented Jun 13, 2017

@JamesNK I seems to understand the tradeoff problematic you're dealing with, note that this customization line has been crossed in other popular language libraries such as the Jackson (Java) (example here : https://gist.github.com/christophercurrie/8939489 and is by example leveraged in the Swagger-Codegen). That does not off course means you have to embrace it too but I think the request is not either unreasonable.

@manuc66
Copy link
Author

manuc66 commented Jun 19, 2017

A workaround implementation can be found here: https://github.com/manuc66/JsonSubTypes

@MihaMarkic
Copy link

@manuc66 Nice.

@sungam3r
Copy link

I ran into the same problem. It is very problematic to use JSON.NET for deserialization GraphQL responses with interfaces fields because GraphQL uses __typename field discriminator.

In my opinion the choice of a constant for a discriminator is a short-sighted decision. These steps simply prohibit / complicate any adaptation of the library to different environments. Instead of a simple, effective setup, the library itself has to resort to writing unnecessary converters, which complicate the process and reduce productivity.

@sliekens
Copy link

sliekens commented May 24, 2019

I solved this problem with a custom JsonConverter. My solution borrows from CustomCreationConverter but instead of making you extend the converter, my converter is sealed and you customize its behaviors via parameters.

Source code for DiscriminatedJsonConverter can be found in my Gist:
https://gist.github.com/StevenLiekens/82ddcf1823ee91cf6d5edfcdb1f6a591

Example usage:

[JsonConverter(typeof(DiscriminatedJsonConverter), typeof(AnimalDiscriminatorOptions))]
public class Animal
{
    private string ClassName { get; set; } // not mandatory
    private string Color { get; set; }
}

// MUST have a parameterless ctor so it can be instantiated with Activator.CreateInstance
public class AnimalDiscriminatorOptions : DiscriminatorOptions
{
    public override Type BaseType => typeof(Animal);

    public override string DiscriminatorFieldName => "className";

    // true if you want to set the value of ClassName, false to skip it
    public override bool SerializeDiscriminator => false;

    public override IEnumerable<(string TypeName, Type Type)> GetDiscriminatedTypes()
    {
        yield return ("Cat", typeof(Cat));
        yield return ("Dog", typeof(Dog));
    }
}

@sfmskywalker
Copy link

sfmskywalker commented Nov 17, 2019

Another issue with the choice of the $type value is that it conflicts when storing a model to MongoDB, since its a MongoDB reserved name.

Although it's possible to write a custom converter as seen in this thread, it sure would be nice to be able to change the name value without having to write a custom converter.

I do understand @JamesNK 's point that you need to draw a line somewhere, but perhaps that line can be reconsidered? :)

@ethanshea-ms
Copy link

Not allowing the type discriminator also prevents from properly serializing/deserializing OData formatted documents, which use $odata.type.

@atrauzzi
Copy link

atrauzzi commented Oct 11, 2020

@JamesNK - The number of times this flexibility is requested makes your choice to "draw the line" a little odd.

Today, working with another ecosystem and I can't just force your convention upon it. Yet somehow, I need to be compatible. Wat du? 😆

I understand in software when people need to draw a line, but this is usually done in response to adding novel functionality. Not in imposing limitations, which you must admit makes drawing the line a bit disingenuous.

Please reconsider.

@HappyNomad
Copy link

@atrauzzi I used Linq-to-Json to overcome that limitation in my mobile client app.

using ( var bsonReader = new BsonDataReader( await response.Content.ReadAsStreamAsync(), false, DateTimeKind.Utc ) ) {
    var doc = new JsonSerializer().Deserialize<JObject>( bsonReader );
    if ( doc == null ) return default;
    foreach ( var obj in doc.DescendantsAndSelf().OfType<JObject>() ) {
        var property = obj.Properties().SingleOrDefault( p => p.Name == "_id" );
        string id = Hex.ToString( (byte[])property?.Value );
        property?.Replace( new JProperty( "$id", id ) );

        property = obj.Properties().SingleOrDefault( p => p.Name == "_t" );
        if ( id != null )
            property.AddAfterSelf( new JProperty( "id", id ) );
        property?.Replace( new JProperty( "$type", (string)property.Value ) );
    }
    foreach ( var reference in doc.Descendants().OfType<JValue>().Where( v => v.Type == JTokenType.Bytes ).ToList() )
        reference.Replace( new JObject( new JProperty( "$ref", Hex.ToString( (byte[])reference ) ) ) );
    RepositionDefinitions( doc );
    object result = doc.ToObject<object>( JsonSerializer.Create( jSettings ) );
    cancelToken.ThrowIfCancellationRequested();
    return result;
}

Now I'm adding a web client and will remove the above code. I was trying to be efficient by serving BSON straight from MongoDB but, since the browser is slow with BSON, I'll instead serve it JSON. Because of this, I may as well deserialize the BSON into POCOs before reserializing it for transport to the client. What will I use to reserialize? JSON.NET of course. So I'll no longer need that client-side preprocessing.

Basically, I'm giving up on my custom toolchain for transporting data from server to client. I generally like to innovate but I've concluded that this isn't an area in which to do it. It's not worth the aggravation since it'll make communicating with new systems more difficult, and it'll be more work than you expected.

@julealgon
Copy link

Extremely surprised to find out this hasn't been implemented in Newtonsoft.Json yet. We now have to interface with a REST API that returns a few hierarchies and uses a discriminator field called _type, which is incompatible with the standard mechanism.

@Myndale
Copy link

Myndale commented Jun 19, 2022

@JamesNK please, I urge you to re-visit and reconsider your position on this. It's such a basic, fundamental capability which has been requested by so many people over the years. If everyone who used NJ did so for both serialization and deserialization across all projects then it wouldn't be such a big deal, but that’s simply not the reality of modern software development. It's absolutely critical that the impedance match between core sub-systems like NJ and all the other building blocks we use is as high as possible. Forcing your users to resort to dodgy hacks like string-replace on the json itself can only serve to undermine the security and stability of your product, as well as any applications that rely upon it.

@tdjastrzebski
Copy link

tdjastrzebski commented Aug 17, 2022

@JamesNK, I found at least 20 publications and countless discussions on ways around this limitation placed, I presume, in attempt to impose own industry standard. It is simply sad, such a waste of time and effort - on both sides.

@marchy
Copy link

marchy commented May 24, 2023

@JamesNK with all due respect, you are clearly off in your decision on this.

Please rectify this problem. It's beyond annoying and you trying to impose your own conventions on third-party systems just plain doesn't work.

@sungam3r
Copy link

I am sure that these problems, like many others, have never been resolved. We should be honest to ourselves - JSON.NET project is dead in a sense of evolution. STJ is a replacement. JSON.NET will work for many-many years, good library (and one of the most known in NET ecosystem), but it's golden age is over. Over 600 issues and 75 PRs. It is frozen in the current state.

@marchy
Copy link

marchy commented May 26, 2023

@sungam3r don't disagree, unfortunately System.Text.JSON still has deal-breakers that prevent us from migrating – such as opt-ing in properties into serialization (dotnet/runtime#30180) – which apparently the STJ team doesn't even plan on taking on, thus we have no migration path into it.

Stuck between a rock and a hard place.

@sliekens
Copy link

sliekens commented May 26, 2023

A bit off-topic, but I can recommend using JObject and a bit of custom logic to decide which type to use. Then you can use JObject.ToObject<T>() where T is the type decided by your code.

If performance is a concern, you can do something similar with System.Text.Json.JsonDocument which has better performance characteristics than JObject.

@sungam3r
Copy link

@sungam3r don't disagree, unfortunately System.Text.JSON still has deal-breakers that prevent us from migrating – such as opt-ing in properties into serialization (dotnet/runtime#30180) – which apparently the STJ team doesn't even plan on taking on, thus we have no migration path into it.

Stuck between a rock and a hard place.

Of course STJ does not allow you to do whatever you want. I told about evolution. STJ moves forward. NSJ is dead. That is what I'm talking about. I see how issues number grows every year. I see a lot of abandoned PRs without any feedback from repo owner. I think this is a question not only of technical development but also MS policy. This project is a completed one staying with minimal support for the most critical bugs and it makes no sense for MS to invest in it anymore.

@julealgon
Copy link

I am sure that these problems, like many others, have never been resolved. We should be honest to ourselves - JSON.NET project is dead in a sense of evolution. STJ is a replacement. JSON.NET will work for many-many years, good library (and one of the most known in NET ecosystem), but it's golden age is over. Over 600 issues and 75 PRs. It is frozen in the current state.

@sungam3r if this is really the case (and I'm not necessarily saying it isn't...) it should be made official somewhere.

Right now there is no expectation that this library is just in "support mode" or anything of the sort as, AFAIK, no statements have been made on that front at all.

Even Microsoft themselves recommend using Newtonsoft.Json in some of their documentation when one needs features that are not present in STJ.

Not supporting it further, while there is still no feature-parity between it and STJ, would be a big mistake IMHO.

@JamesNK can you clarify on this please?

@sungam3r
Copy link

it should be made official somewhere
no statements have been made on that front at all.

Exactly. I asked about that somewhere else in this repo.

@smetlov
Copy link

smetlov commented Aug 28, 2024

Imagine refactoring the entire system for 5 days straight, assuming that little feature is obviously there, so you can just go ahead and turn it on at the end, but then get surprised that hard... And even the custom converter posted above is 500 lines of code manually iterating through the JSON jeez.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests