Parsing objects with specified type which does not inherit default type.

[masyamandev]

Tests which originally fails:
testBaseWithDefaultAsImpl()
testDeserializationListOfImpl()

Comment in StdTypeResolverBuilder in lines 127-129 is not clear enough. If there are some use cases I don't know, you can point me and I'll try to write tests and fix.

[cowtowncoder]

If the default implementation is not a subtype of base type, should this not rather throw an exception?
Otherwise it will simply hide incorrect usage.
(I realize that ideally method should allow throwing of JsonMappingException, or just IOException, so for now that'd have to be something like IllegalArgumentException that will get converted up the call stack).

[masyamandev]

In this baseType is a target type of deserialization, AFAIK.
Let's say we have abstract class Animal, implementations are Dog, Cat etc and UnknownAnimal as default implementation.
In some file (e.g. cats.json) I have json with Cat and no other animals. In this case it is suitable to parse it as Cat. In current implementation code like this will throw an exception because UnknownAnimal is not subclass of Cat (it's subclass of Animal):
Cat cat = new ObjectMapper().readValue(json, Cat.class);
Ugly workaround is to use class cast:
Cat cat = (Cat) new ObjectMapper().readValue(json, Animal.class);
And it's even worse if I need to parse a list of cats:
List cat = (List) new ObjectMapper().readValue(json, new TypeReference<List>() {});
Instead of:
List cat = new ObjectMapper().readValue(json, new TypeReference<List>() {});
As for me parsing specific subtype is absolutely correct behavior if input data is correct and has only objects of specific type. And yes, it should throw an exception if input json contains other type like Dog or some UnknownAnimal. But it may not throw an exception if all input data is correct and can be parsed as or cast to target type.

BTW, this functionality worked in version 2.3.5, but was broken in some later versions, I've checked 2.7 and 2.9.

Update: I've read all your comments and understand more clearly your first comment. It make sense to throw an exception if UnknownAnimal is not a subclass of Animal. Comment regarding caching is extremely important, tests are required here. I'll try to find a solution when I have time. Thank you.

[cowtowncoder]

@masyamandev Sounds good. I am hoping to work a bit on this too -- I think your patch helped a lot, showing that fix could be quite simple, fundamentally. I hadn't had time to dig deeper into this, although I am aware that this is a relatively common problem.

[masyamandev]

I have added a couple of tests which checks DeserializerCache by deserializing different variations of objects and types using single ObjectMapper. It seems that it works fine.

Regarding throwing an Exception when default implementation class does not extends base class (UnknownAnimal is not a subclass of Animal). Do you think it's a critical issue?
There are too many cases when Exception can be thrown in runtime when parsing objects. For instance:

• new ObjectMapper().readValue("{"type":"dog"}", Cat.class);
• new ObjectMapper().readValue("{"type":"unknownAnimal"}", Cat.class);
• new ObjectMapper().readValue("{"type":"cat"}", SphinxCat.class); // SphinxCat extends Cat
• new ObjectMapper().readValue("{"type":"unknownAnimal"}", Animal.class); // In case if UnknownAnimal does not extend Animal

In proposed implementation Exception will not be thrown for cases when UnknownAnimal does not extend Animal:

• new ObjectMapper().readValue("{"type":"cat"}", Animal.class)
• new ObjectMapper().readValue("{"type":"cat"}", Cat.class)

But will be thrown when we try to parse "{"type":"unknownAnimal"}.
Unfortunately I can't find simple solution where to add checking root type and default implementation compatibility.

[masyamandev]

BTW, I've found another PR which fixes exactly the same issue.
#1656
So I'm not the only one who needs this.
That PR contains code for checking root type. But it has slightly smaller amount of tests.
I can implement checking root type the same way as in PR 1656 into my PR if it's acceptable solution.

[cowtowncoder]

@masyamandev I think what would be great would be to start with unit tests: ones that already pass can be added wherever they make sense; and ones failing now (but to be fixed) under failing. I think we agree on definition on what should work, and perhaps an what shouldn't too.

But on latter, just to make sure, I'd want to check the case where annotation defined incompatible type. Something like:

public class Zoo {
   @JsonTypeInfo(.... defaultImpl = NotAnimal.class)
   public Animal animal;
}

I am not as worried about what root type is being requested, or whether incoming type id is compatible, just because those cases are already covered (... as far as I know).
I really just want to prevent obviously incompatible case.

Now: ideally this would be caught by annotation processing logic, but unfortunately this is not currently (2.x) possible, as annotations are first flattened throughout type hierarchy, and it is not possible to know what class actually had annotations.
If this was available, we could probably simply skip the checks.

[cowtowncoder]

First of all, thank you for contributing this patch!
I think there are issues reported that this PR would address. It would be great to have references to those; I'll see if I can locate them and add links.

Now: I think something along these lines might work for solving the problem of base type potentially varying between calls, but only in case of root value. Root value is special because whereas base type can vary for root value (it is what caller explicitly specifies), it can not vary for references via property.

My main/only concern here is two-fold. First concern (which I added as a comment first) is that for explicit case of mismatching default implementation it would make sense to indicate the problem via exception. But on the other hand I think in other cases this might not make sense.
The real problem, I think, is that assignment compatibility MUST be preserved regarding base type of class or property being annotated -- but MUST NOT be considered problematic in case where this type is sort of overridden for root value.
And the problem wrt PR is simply that code does not have enough information on determining what is the base type it should consider for purposes of checking.

Second concern I have has to do with caching: it is important that if defaultImpl is nulled due to mismatch (since target type is subtype of base type, essentially), this does not affect case(s) where defaultImpl should be retained. Since caching is on per-mapper basis, I think it'd be enough to simple test different base type cases with same mapper (in same test method perhaps).
If things work as expected in both cases (work in case where defaultImpl is retained; exception when it's nulled due to mismatch) that is fine. I just don't want some user to find this out over multi-threaded usage; they are hard to debug. :)

[cowtowncoder]

One more thing: place that is highly relevant wrt root-value caching, and place that I think is where alternative fix could be applied is on findRootValueDeserializer() of DeserializationContext (line 474 in 2.9).
In this case, TypeDeserializer is constructed as necessary, as per target type, so I think caching should work correctly, and not conflict.

But I wonder if it would be possible to somehow indicate that this call is different... hmmh.
I think that although ideally no API changes were done, in this case it would be possible to add one new method in DeserializerFactory: findRootTypeDeserializer, and this could cascade changes needed (if any) to affect change in handling logic.
And the reason why I think API addition would be ok for 2.9 is simply because the only caller here would be ObjectReader (and ObjectMapper) from databind itself.

I'll have to think about this a bit, but I think something like this could help avoid problems coming from two distinct cases (root value with explicit target base type; non-root value where base type is fixed by type declaration and cannot vary).

[cowtowncoder]

Ok, I was unable to find issue I was thinking of: there are some related ones for defaultImpl, but not quite the same (like #644, #436). Of closed issues, however, #747 is sort of the same thing.
Anyway it is ok to have a PR only if no issues exist; I just thought there was an open issue too.

[masyamandev]

I've updated tests and ported defaultImpl compatibility check from PR 1656. Please check.

[masyamandev]

No comments for 10 days. Is there any progress in reviewing this pull request?

[cowtowncoder]

I am heavily overloaded at this point; no progress. Still hoping to get this in 2.9.4, hence active tag.

[rakeshhny]

Can you please use specific imports?

[rakeshhny]

Can you please use specific imports?

[masyamandev]

Do you mean replace

import com.fasterxml.jackson.databind.*;

to

import com.fasterxml.jackson.databind.DeserializationConfig;
import com.fasterxml.jackson.databind.JavaType;
import com.fasterxml.jackson.databind.SerializationConfig;

and other imports like this?
It was original imports. I can change them if it's really needed. I just need to clarify if I understand you correctly.

[cowtowncoder]

No, you do no have to do that in general. Often times IDEs automatically use wildcard for some number (like 5), but it does not greatly matter. There is nothing inherently wrong in wildcards, except for rare cases of class names from different packages overlap (in which case explicit reference has to be used).

[rakeshhny]

In our project we follow some guidelines and in which having specific imports is one of them. This is why I have asked to use specific imports.

Wild card imports is a feature provided by the language and should be used whenever required.

I think it is about the standard practices we use in a project. If here we want to use wildcards for imports greater than or equal to number - n, then I am also fine with this.

I agree to @cowtowncoder that it does not matters a lot.

[cowtowncoder]

@rakeshhny My surprise has more to do with your suggestion -- if you are working with submitter on source side, fine, but in general I don't want others to suggest their own preferences here. If I feel strongly about style I can do that myself.
But in general I am proponent of "when in Rome", and it should be possible to get a feel of style by observing existing codebase.

So that's all. I know some developers are firmly against wildcarding; I am not. Conversely Jackson projects accept use of wild carding to reduce amount of boilerplate imports.

[cowtowncoder]

Ok, hmmmh. Looking at tests, trying to trim them down a bit, to focus on core problem.
I think combination of ignoring properties and visible type id is hairy enough that I'll probably just remove that aspect altogether.

[cowtowncoder]

Thank you everyone for your help. I will be fixing this with approach shown here, except with less checking: for 2.x I think I will essentially allow ignoring of incompatible default impl altogether, and for 3.x hope to improve handling to properly check the specific case (similar to what shown here but without having to by-pass AnnotationIntrospector and having to resolve all intermediate classes).