Improve handling of @JsonTypeInfo, defaultImpl to anchor definition

[cowtowncoder]

So: currently it is possible to declare a valid @JsonTypeInfo in basetype, where defaultImpl 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:

@JsonTypeInfo(.... defaultImpl=DefaultImpl.class)
public class Base { }

public class Concrete extends Base { }

public class DefaultImpl extends Base { }

where deserialization using:

Base result = mapper.readValue("{ }", Base.class);

succeeds, but attempts to do:

Concrete result = mapper.readValue("{ }", Concrete.class);

will fail, because DefaultImpl is not assignable from Concrete.

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 use Base 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.

[michelbetancourt]

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.

[michalmela]

Any news on this issue? With actively using defaultImpl in our code-base this blocks our company (and, as I suspect, not only us) from upgrading past Jackson 2.6.x (since, given the discussion at #1083 and #1084 , there was some refactoring in 2.7.x which broke #1083 , which was then fixed in #1084 but then it effectively broke the behaviour shown in this issue).

[cowtowncoder]

@michalmela As general practice, I add notes on issues if I work on them. So no news unfortunately.

[cowtowncoder]

@gm2211 yes I think you are right. So this would be dup of #1565 (or vice versa, but that one is now mentioned in release notes...).

I mean, in practice at least. I don't know if there are semantic differences in considering how defaultImpl binds, getting ignored (or not).

[gm2211]

Had deleted my comment while trying to verify that.

The test for #1565 is actually a little different: you have A(defaultImpl = B.class), B extends A, C extends B and testing that you can deserialize to C.

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 2.6.5 to 2.9.5