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

De-/Serialization for sub-types without "type" property #13

Closed
sir-boformer opened this issue Sep 27, 2017 · 8 comments
Closed

De-/Serialization for sub-types without "type" property #13

sir-boformer opened this issue Sep 27, 2017 · 8 comments
Assignees
Milestone

Comments

@sir-boformer
Copy link
Contributor

I would like to avoid having an extra "type" property in my sub-types.

Motivation: It is cleaner to avoid the extra property (avoid duplicate information)

[JsonConverter(typeof(JsonSubtypes), "type")]
[JsonSubtypes.KnownSubType(typeof(Dog), AnimalType.Dog)]
[JsonSubtypes.KnownSubType(typeof(Cat), AnimalType.Cat)]
public abstract class Animal
{
    public int Age { get; set; }
}

public class Dog : Animal
{
    public bool CanBark { get; set; } = true;
}

public class Cat : Animal
{
    public int Lives { get; set; } = 7;
}

public enum AnimalType
{
    Dog = 1,
    Cat = 2
}

Deserializing is working perfectly:

[{"canBark":false,"type":1,"age":3},{"lives":6,"type":2,"age":11}]
JsonConvert.DeserializeObject<Animal[]>(json, settings); 
// outputs correct array of [Dog, Cat]

Serialization not so much:

JsonConvert.SerializeObject(new Dog {Age = 3, CanBark = false}, settings);
{"canBark":false,"age":3}

It seems like it would be necessary to annotate all sub-types with [JsonConverter], and it would require reflection to get the parent type that contains the type mapping.

@manuc66
Copy link
Owner

manuc66 commented Oct 16, 2017

Hi @sir-boformer ,

Thanks for giving your improvement idea !

It seems to me that having to specify [JsonConverter] on all sub-types versus having a type field in the base type is not something that makes the code more readable nor concise from my point of view.
The advantage of having this field is that you see in your code that this value would be serialized and you can customize it with the [JsonProperty] attribute.

Note that this type field also helps on the deserializing side when no subtype match have been found, then it's possible to know which was the type.

What do you think ?

@boformer
Copy link

I don't get why it helps on the deserializing side. There would still be the JSON type property which can be logged when something goes wrong.

The [JsonConverter] attribute caused me a lot of headaches because it does not really replace a proper converter registration (when you try to deserialize a list of Dog objects with sub-types of Dog, it does not work if the attribute is defined in Animal).

Actually I created this issue because the GSON Java library supports sub-classes without extra "type" fields. Here is how the converter registration works in GSON:

// adding all different container classes with their flag
final RuntimeTypeAdapterFactory<AbstractContainer> typeFactory = RuntimeTypeAdapterFactory  
        .of(Animal.class, "type") // Here you specify which is the parent class and what field particularizes the child class.
        .registerSubtype(Dog.class, "dog") // if the flag equals the class name, you can skip the second parameter. This is only necessary, when the "type" field does not equal the class name.
        .registerSubtype(Cat.class, "cat");

// add the polymorphic specialization
final Gson gson = new GsonBuilder().registerTypeAdapterFactory(typeFactory).create();

Basically all that is needed is a way to configure JsonSubTypes without attributes, and the serialization logic that adds the property to JSON (serialization would only be enabled when configured without attributes).

After all I can live with the extra property. It was just a suggestion ;)

@manuc66 manuc66 assigned manuc66 and unassigned sir-boformer Oct 18, 2017
manuc66 pushed a commit that referenced this issue Oct 18, 2017
@manuc66
Copy link
Owner

manuc66 commented Oct 18, 2017

Base on the proposal of @boformer I might have a solution draft...

Could you tell me if it might meet your needs ?

P.S.: More in depth testing needs to be done and naming does not sound really good.

@sir-boformer
Copy link
Contributor Author

That's actually brilliant, nice work! I think it just needs a shorter name 😄

@sir-boformer
Copy link
Contributor Author

Maybe just JsonSubTypesBuilder and JsonSubTypesWithRegistry?

@manuc66
Copy link
Owner

manuc66 commented Jan 19, 2018

I've added the failing test in the branch that is related to #16

@manuc66
Copy link
Owner

manuc66 commented Jan 28, 2018

Released in v1.3.0 with #28

@sir-boformer
Copy link
Contributor Author

Awesome! Thank you!

@manuc66 manuc66 closed this as completed Jan 28, 2018
@manuc66 manuc66 added this to the 1.3.0 milestone Apr 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants