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

Polymorphic deserialization regression in 2.8.1 #1311

Closed
pkwarren opened this issue Jul 23, 2016 · 11 comments
Closed

Polymorphic deserialization regression in 2.8.1 #1311

pkwarren opened this issue Jul 23, 2016 · 11 comments

Comments

@pkwarren
Copy link

Version: 2.8.1

When attempting to update DropWizard to 2.8.1, I encountered a regression in polymorphic deserialization. Here's a sample testcase to illustrate the problem.

import com.fasterxml.jackson.annotation.JsonTypeInfo;
import com.fasterxml.jackson.annotation.JsonTypeName;
import com.fasterxml.jackson.databind.ObjectMapper;

import java.io.IOException;

public class Test {

    @JsonTypeInfo(property = "type", use = JsonTypeInfo.Id.NAME, defaultImpl = DefaultFactory.class)
    public interface Factory {

    }

    @JsonTypeName("default")
    public static class DefaultFactory implements Factory {

    }

    @JsonTypeName("simple")
    public static class SimpleFactory implements Factory {

    }

    public static void main(String[] args) throws IOException {
        final ObjectMapper mapper = new ObjectMapper();
        mapper.registerSubtypes(DefaultFactory.class, SimpleFactory.class);
        final SimpleFactory factory = mapper.readValue("{\"type\":\"simple\"}", SimpleFactory.class);
    }
}

Expected results: No error.

Actual results: Exception:

Exception in thread "main" java.lang.IllegalArgumentException: Class Test$DefaultFactory not subtype of [simple type, class Test$SimpleFactory]
    at com.fasterxml.jackson.databind.type.TypeFactory.constructSpecializedType(TypeFactory.java:359)
    at com.fasterxml.jackson.databind.jsontype.impl.StdTypeResolverBuilder.buildTypeDeserializer(StdTypeResolverBuilder.java:118)
    at com.fasterxml.jackson.databind.deser.BasicDeserializerFactory.findTypeDeserializer(BasicDeserializerFactory.java:1363)
    at com.fasterxml.jackson.databind.DeserializationContext.findRootValueDeserializer(DeserializationContext.java:481)
    at com.fasterxml.jackson.databind.ObjectMapper._findRootDeserializer(ObjectMapper.java:3890)
    at com.fasterxml.jackson.databind.ObjectMapper._readMapAndClose(ObjectMapper.java:3785)
    at com.fasterxml.jackson.databind.ObjectMapper.readValue(ObjectMapper.java:2833)
    at Test.main(Test.java:27)
@cowtowncoder
Copy link
Member

Interesting. I just saw the test failure in DW test suite, and was wondering whether there might be type hierarchy issue that was only now uncovered. But the test code seems solid enough suggesting an actual problem with validation Jackson does.

Thank you for reporting this and providing a clear test case for reproduction.

@cowtowncoder
Copy link
Member

Also seems to specifically require discordant combination of expected type, and default implementation type, which somewhat explains why no test case covered this.

@cowtowncoder
Copy link
Member

Hmmh. And... this may be tricky to actually fix. There is a real problem due to example code supplying SimpleFactory as the target type, instead of more straight-forward Factory. Problem being that DefaultFactory (default implementation to use if no type specified) is indeed NOT a subtype of alleged target type of SimpleFactory. So in some sense, code doing checking is not exactly at fault -- in fact, if not type was found, a ClassCastException is likely to occur when assignment is attempted.

But at the same time it would seem like there should be a better way to handle this.

Since I think this failure came from DropWizard test suite, I can have a look at test to see if type handling there makes sense.

@cowtowncoder
Copy link
Member

More that I look at DropWizard case, more confused I am. Test itself does nothing wrong, it just relies on definition of Configuration, ServerFactory. So where does the SimpleServerFactory come from, to "replace" base type ServerFactory?
In test code above it's much simpler, not much mystery there. But maybe it is due to intermediate step, buffering, from YAML to TokenBuffer, binding from tree.

@cowtowncoder
Copy link
Member

Ah. Found the culprit, it is a test artifact. Will do a PR for DropWizard, regardless of whether there is more work to be done on Jackson side.

As to Jackson, it might make sense to walk the inheritance chain to see if a compatible supertype might be found "close enough"... however, heuristics may or may not work. And fundamentally doing this would trade one type of failure to another one, so hard to say a priori if it's worth it.

@azuritus
Copy link

azuritus commented Aug 9, 2016

Hi, we are experiencing almost same issue with indirect use of concrete subtype of an abstract type. From our point of view this is valid use case and this issue blocks us from upgrading to Jackson 2.8.

Jackson 2.8.1, with 2.7.x work just fine.

Reduced test case:

import java.io.IOException;

import com.fasterxml.jackson.annotation.JsonTypeInfo;
import com.fasterxml.jackson.databind.ObjectMapper;

public class Jackson1311 {

    @JsonTypeInfo(use = JsonTypeInfo.Id.CLASS, defaultImpl = B.class)
    public abstract static class A {
    }

    public static class B extends A {
    }

    public static class C extends A {
    }

    public static class X {
        public C c;
    }

    public static void main(String[] args) throws IOException {
        ObjectMapper mapper = new ObjectMapper();
        mapper.readValue("{}", X.class);
    }
}

Exception thrown:

Exception in thread "main" com.fasterxml.jackson.databind.JsonMappingException: Class Jackson1311$B not subtype of [simple type, class Jackson1311$C]
 at [Source: {}; line: 1, column: 1]
    at com.fasterxml.jackson.databind.JsonMappingException.from(JsonMappingException.java:296)
    at com.fasterxml.jackson.databind.deser.DeserializerCache._createAndCache2(DeserializerCache.java:269)
    at com.fasterxml.jackson.databind.deser.DeserializerCache._createAndCacheValueDeserializer(DeserializerCache.java:244)
    at com.fasterxml.jackson.databind.deser.DeserializerCache.findValueDeserializer(DeserializerCache.java:142)
    at com.fasterxml.jackson.databind.DeserializationContext.findRootValueDeserializer(DeserializationContext.java:475)
    at com.fasterxml.jackson.databind.ObjectMapper._findRootDeserializer(ObjectMapper.java:3890)
    at com.fasterxml.jackson.databind.ObjectMapper._readMapAndClose(ObjectMapper.java:3785)
    at com.fasterxml.jackson.databind.ObjectMapper.readValue(ObjectMapper.java:2833)
    at Jackson1311.main(Jackson1311.java:24)
    at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
    at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
    at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
    at java.lang.reflect.Method.invoke(Method.java:498)
    at com.intellij.rt.execution.application.AppMain.main(AppMain.java:147)
Caused by: java.lang.IllegalArgumentException: Class Jackson1311$B not subtype of [simple type, class Jackson1311$C]
    at com.fasterxml.jackson.databind.type.TypeFactory.constructSpecializedType(TypeFactory.java:359)
    at com.fasterxml.jackson.databind.jsontype.impl.StdTypeResolverBuilder.buildTypeDeserializer(StdTypeResolverBuilder.java:118)
    at com.fasterxml.jackson.databind.deser.BasicDeserializerFactory.findTypeDeserializer(BasicDeserializerFactory.java:1363)
    at com.fasterxml.jackson.databind.deser.BasicDeserializerFactory.findPropertyTypeDeserializer(BasicDeserializerFactory.java:1498)
    at com.fasterxml.jackson.databind.deser.BasicDeserializerFactory.resolveMemberAndTypeAnnotations(BasicDeserializerFactory.java:1847)
    at com.fasterxml.jackson.databind.deser.BeanDeserializerFactory.constructSettableProperty(BeanDeserializerFactory.java:735)
    at com.fasterxml.jackson.databind.deser.BeanDeserializerFactory.addBeanProps(BeanDeserializerFactory.java:520)
    at com.fasterxml.jackson.databind.deser.BeanDeserializerFactory.buildBeanDeserializer(BeanDeserializerFactory.java:226)
    at com.fasterxml.jackson.databind.deser.BeanDeserializerFactory.createBeanDeserializer(BeanDeserializerFactory.java:141)
    at com.fasterxml.jackson.databind.deser.DeserializerCache._createDeserializer2(DeserializerCache.java:406)
    at com.fasterxml.jackson.databind.deser.DeserializerCache._createDeserializer(DeserializerCache.java:352)
    at com.fasterxml.jackson.databind.deser.DeserializerCache._createAndCache2(DeserializerCache.java:264)
    ... 12 more

@cowtowncoder
Copy link
Member

@azuritus I am sorry but strictly speaking this is not legal or safe. Wouldn't it physically fail if you had JSON like:

{
    "c" : {  }
}

because value for C-valued property can not be set to an instance of B? Running your test with 2.7 gives me

com.fasterxml.jackson.databind.JsonMappingException: Problem deserializing property 'c' (expected type: [simple type, class com.fasterxml.jackson.failing.Polymorphic1311Test$C]; actual type: com.fasterxml.jackson.failing.Polymorphic1311Test$B), problem: Can not set com.fasterxml.jackson.failing.Polymorphic1311Test$C field com.fasterxml.jackson.failing.Polymorphic1311Test$X.c to com.fasterxml.jackson.failing.Polymorphic1311Test$B
 at [Source: {"c":{ }}; line: 1, column: 8] (through reference chain: com.fasterxml.jackson.failing.X["c"])
    at com.fasterxml.jackson.databind.JsonMappingException.from(JsonMappingException.java:262)
    at 

Now, if you do happen to have legal default implementation class that extends C, you could override @JsonTypeInfo for C (or any other intermediate base class), and keep hierarchy legal.
But as things are, I don't think your example is legal use case, in that actualy usage with defalt implementation would not work. It just would fail at a later point.

@cshannon
Copy link

cshannon commented Aug 16, 2016

@cowtowncoder ,

I have another example of where this is broken. Here is a very simple test case that works fine with Jackson 2.7.5 but is broken with Jackson 2.8.1.

package jackson.test;

import org.junit.Test;
import com.fasterxml.jackson.databind.ObjectMapper;

public class BrokenJackson {

    @Test
    public void test() throws Exception {
        ObjectMapper objectMapper = new ObjectMapper();
        final JacksonChild childObject = new JacksonChild();

        String value = objectMapper.writeValueAsString(childObject);
        objectMapper.readValue(value, JacksonChild.class);
    }
}
package jackson.test;

import com.fasterxml.jackson.annotation.JsonIgnoreProperties;
import com.fasterxml.jackson.annotation.JsonInclude;

@JsonIgnoreProperties(ignoreUnknown = true)
@JsonInclude(JsonInclude.Include.NON_EMPTY)
public class JacksonChild extends JacksonParent {

}
package jackson.test;

import com.fasterxml.jackson.annotation.JsonIgnoreProperties;
import com.fasterxml.jackson.annotation.JsonInclude;
import com.fasterxml.jackson.annotation.JsonSubTypes;
import com.fasterxml.jackson.annotation.JsonTypeInfo;

@JsonTypeInfo(use = JsonTypeInfo.Id.NAME,
    include = JsonTypeInfo.As.PROPERTY, property = "type",
    defaultImpl = JacksonParent.class)
@JsonSubTypes({ @JsonSubTypes.Type(value = JacksonChild.class, name = "child") })
@JsonIgnoreProperties(ignoreUnknown = true)
@JsonInclude(JsonInclude.Include.NON_EMPTY)
public class JacksonParent {

}

If you run the test case with these 3 classes, the resulting stacktrace is:

java.lang.IllegalArgumentException: Class jackson.test.JacksonParent not subtype of [simple type, class jackson.test.JacksonChild]
    at com.fasterxml.jackson.databind.type.TypeFactory.constructSpecializedType(TypeFactory.java:359)
    at com.fasterxml.jackson.databind.jsontype.impl.StdTypeResolverBuilder.buildTypeDeserializer(StdTypeResolverBuilder.java:118)
    at com.fasterxml.jackson.databind.deser.BasicDeserializerFactory.findTypeDeserializer(BasicDeserializerFactory.java:1363)
    at com.fasterxml.jackson.databind.DeserializationContext.findRootValueDeserializer(DeserializationContext.java:481)
    at com.fasterxml.jackson.databind.ObjectMapper._findRootDeserializer(ObjectMapper.java:3890)
    at com.fasterxml.jackson.databind.ObjectMapper._readMapAndClose(ObjectMapper.java:3785)
    at com.fasterxml.jackson.databind.ObjectMapper.readValue(ObjectMapper.java:2833)
    at jackson.test.BrokenJackson.test(BrokenJackson.java:16)
.....

@cowtowncoder
Copy link
Member

@cshannon that looks like possible bug indeed. Given that there may be multiple distinct issues and/or root causes, would it be possible to create a separate GH issue, with sample code you gave? It is otherwise difficult to track down fate of specific reported pieces.
Thanks!

@cshannon
Copy link

@cowtowncoder , no problem, I created a new issue here with the same code example : #1339

@cowtowncoder
Copy link
Member

As per earlier comment, I think the original case is failing as designed, and the second case is a new issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants