-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Improve handling of @JsonTypeInfo
, defaultImpl
to anchor definition
#1358
Comments
As part of the upgrade, jackson has upgraded to 2.8 They over-eagerly prevent our polymorphism (FasterXML/jackson-databind#1358) so we have to use custom deserialisers for the interfaces Solo: @hugh-emerson
from #1778: @JsonTypeInfo(use = JsonTypeInfo.Id.NAME, include = JsonTypeInfo.As.EXISTING_PROPERTY, visible = true, property = "type", defaultImpl = DefaultEvent.class)
@JsonSubTypes({ @Type(DefaultEvent.class), @Type(value = SpecificEvent.class, name = "specific") })
public interface Event {
String getId();
String getType();
}
@Data (lombok)
class DefaultEvent implements Event {
private String id;
private String type;
}
@Data (lombok)
class SpecificEvent implements Event {
private String id;
private String type;
}
@Test
public void test() throws JsonParseException, JsonMappingException, IOException {
ObjectMapper mapper = new ObjectMapper();
Event event = mapper.readValue("{\"type\":\"specific\"}", Event.class);
// prints out SpecificEvent, this is perfect for cases where dynamic / subtyping is necessary
System.out.println(event.getClass());
// throws exception: java.lang.IllegalArgumentException: Class DefaultEvent not subtype of [simple type, class SpecificEvent]
// this is undesired since there are also cases where the specific type is needed but even though I am requesting the SpecificEvent to be resolved directly it attempts to resolve to the defaultImpl instead. Also there is no change to the "type" being submitted.
event = mapper.readValue("{\"type\":\"specific\"}", SpecificEvent.class);
System.out.println(event.getClass());
} IMHO, what I believe is desirable behavior in the particular case I showed above is that when the type field matches the desired type value, then absolutely, it should return my "SpecificEvent" class back. Without this, and using defaultImpl, it is a bug IMHO. However, there may also be a feature request in the mix here: For cases where the type field is unmatched to the type value, but a specific type class is requested, then (perhaps) as an option, have a way to ensure that the particular (specific) requested type is fulfilled. That is, rather than using the defaultImpl. For this use case I would propose this be the default. I could understand why it may not be since type is unmatched. What I would say about this, though, is that when you are referring to simple 1-level class hierarchy that has annotations set on just the parent, then this is a clear separation of use case IMHO. In other words, only resolve the FasterJackson subtypes to the parent when the FasterJackson settings are placed on just the parent. This keeps the intention clear IMHO. This way when you specifically request a Java Subtype you can always get that Java-subtype back. But if you request the parent, then you have the subtype mapping resolve. Also in a simple use case like this, resolving FasterJackson subtypes does not make sense when the Java subtypes will never correlate. At least by default. Perhaps by option, for those folks that think they need an error, should get an error. Though at this point I can understand that this may be a FasterJackson "feature" so take that with a grain of salt. An option to resolve the correct type would be sufficient IMHO though, again, not what I think most folks would actually want and need. IMHO, these 2 approaches, would address 80%+ of use cases requested with the other related issues noted around subtyping problems. |
Any news on this issue? With actively using |
@michalmela As general practice, I add notes on issues if I work on them. So no news unfortunately. |
Had deleted my comment while trying to verify that. The test for #1565 is actually a little different: you have However, that same PR introduces a test for #1861 -> 09e5ba1#diff-7d961794a75a46f308447cd997960153R54 which is equivalent to the example outlined in this issue. Not sure about semantic differences either, but this fixes the practical issues we found while upgrading from |
So: currently it is possible to declare a valid
@JsonTypeInfo
in basetype, wheredefaultImpl
specifies a subtype that is valid within context of declaration; but use it in a way that triggers an exception for potential conflict.This happens when user requests deserialization into a subtype of original type, but one that is not castable to the type of
defaultImpl
. For example, something like:where deserialization using:
succeeds, but attempts to do:
will fail, because
DefaultImpl
is not assignable fromConcrete
.While code is not exactly wrong in that there is potential problem -- if default implementation ends up required to be used, there would be a cast exception -- it is not necessarily something that will happen, and in fact user may be able to either guarantee it will not, or, even if not it may be better to indicate problem only when it does occur as opposed to merely possibly happening.
The technical problem is most likely due to inheritance of
@JsonTypeInfo
annotation, so that code has no way of knowing where exactly declaration is bound. If binding was known, base type comparison could useBase
regardless of expected type, and we could indicate failure only when it actually happens.It is not 100% certain that this problem can be resolved, but it would be good to try to do so.
The text was updated successfully, but these errors were encountered: