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

remove JsonInheritanceAttribute in abstract class #1081

Merged
merged 1 commit into from Nov 22, 2019

Conversation

@Goxiaoy
Copy link
Contributor

Goxiaoy commented Oct 15, 2019

abstract class can not be instantiate

(cherry picked from commit ae7e1a9)
@RicoSuter

This comment has been minimized.

Copy link
Owner

RicoSuter commented Oct 15, 2019

Are you sure we can just remove it? I think this is needed so that the deserializer knows the possible implementations of the (abstract) base class.

@Goxiaoy

This comment has been minimized.

Copy link
Contributor Author

Goxiaoy commented Oct 15, 2019

I think so.
Look at the function ReadJson in generated class JsonInheritanceConverter

var discriminator = Newtonsoft.Json.Linq.Extensions.Value<string>(jObject.GetValue(_discriminator));
var subtype = GetObjectSubtype(objectType, discriminator);

var objectContract = serializer.ContractResolver.ResolveContract(subtype) as Newtonsoft.Json.Serialization.JsonObjectContract;
if (objectContract == null || System.Linq.Enumerable.All(objectContract.Properties, p => p.PropertyName != _discriminator))
{
    jObject.Remove(_discriminator);
}

try
{
    _isReading = true;
    return serializer.Deserialize(jObject.CreateReader(), subtype);
}
finally
{
    _isReading = false;
}

When subtype is an abstract type, serializer.Deserialize(jObject.CreateReader(), subtype); will raise an error because abstract class can not be instantiate.

@RicoSuter RicoSuter merged commit 8df1da6 into RicoSuter:master Nov 22, 2019
1 check passed
1 check passed
NJsonSchema - CI Build #20191015.1 succeeded
Details
@RicoSuter

This comment has been minimized.

Copy link
Owner

RicoSuter commented Nov 22, 2019

Ok yes, makes sense.

@RicoSuter

This comment has been minimized.

Copy link
Owner

RicoSuter commented Nov 22, 2019

Thanks for the PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.