readTree explodes when reading json with type information

[electricmonk]

When reading json that has type information, readTree explodes on ClassCastException upon trying to assert that JsonNode is a supertype of the type in the type information. This is, of course, wrong.

As far as I can tell, the bug is in AsPropertyTypeDeserializer.deserializeTypedFromObject:

            if (_typePropertyName.equals(name)) { // gotcha!
                return _deserializeTypedForId(jp, ctxt, tb);
            }

Follows is a test that recreates the bug:

import com.fasterxml.jackson.annotation.JsonTypeInfo;
import com.fasterxml.jackson.databind.JsonNode;
import com.fasterxml.jackson.databind.ObjectMapper;
import com.fasterxml.jackson.databind.node.TextNode;
import org.junit.Before;
import org.junit.Test;

import static org.junit.Assert.assertEquals;

/**
 * @author shaiyallin
 * @since 10/2/12
 */
public class DefaultTypingReadTreeBugTest {

    ObjectMapper mapper = new ObjectMapper();

    @Test
    public void valueAsStringWithoutDefaultTyping() throws Exception {

        Foo foo = new Foo("baz");
        String json = mapper.writeValueAsString(foo);

        JsonNode jsonNode = mapper.readTree(json);
        assertEquals(jsonNode.get("bar").textValue(), foo.getBar());
    }

    @Test
    public void valueAsStringWithDefaultTyping() throws Exception {
        mapper.enableDefaultTyping(ObjectMapper.DefaultTyping.NON_FINAL, JsonTypeInfo.As.PROPERTY);

        Foo foo = new Foo("baz");
        String json = mapper.writeValueAsString(foo);

        JsonNode jsonNode = mapper.readTree(json);
        assertEquals(jsonNode.get("bar").textValue(), foo.getBar());
    }

    @Test
    public void readTreeWithDefaultTyping() throws Exception {

        mapper.enableDefaultTyping(ObjectMapper.DefaultTyping.NON_FINAL, JsonTypeInfo.As.PROPERTY);
        String json = "{\"@class\":\"DefaultTypingReadTreeBugTest$Foo\",\"bar\":\"baz\"}";
        JsonNode jsonNode = mapper.readTree(json);
        assertEquals(jsonNode.get("bar").textValue(), "baz");
    }

    @Test
    public void valueToTreeWithoutDefaultTyping() throws Exception {

        Foo foo = new Foo("baz");
        JsonNode jsonNode = mapper.valueToTree(foo);
        assertEquals(jsonNode.get("bar").textValue(), foo.getBar());
    }

    @Test
    public void valueToTreeWithDefaultTyping() throws Exception {
        mapper.enableDefaultTyping(ObjectMapper.DefaultTyping.NON_FINAL, JsonTypeInfo.As.PROPERTY);

        Foo foo = new Foo("baz");
        JsonNode jsonNode = mapper.valueToTree(foo);
        assertEquals(jsonNode.get("bar").textValue(), foo.getBar());
    }


    public static class Foo {
        private String bar;

        public Foo() {
        }

        public Foo(String bar) {
            this.bar = bar;
        }

        public String getBar() {
            return bar;
        }

        public void setBar(String bar) {
            this.bar = bar;
        }
    }
}

[cowtowncoder]

Will have a look. Trees and polymorphic types are somewhat incompatible (trees expose physical JSON structure; and type information is metadata that is hidden when dealing with POJOs), but exception is probably not the right thing to do regardless.

[electricmonk]

Well, as someone who's not familiar with Jackson's inner workings, I don't see any incompatibility. IMO JsonNode should simply treat the type info property as just another property. If that's doable, it would be great.

[cowtowncoder]

Yes, when reading JSON with type info, it should be exposed as-is, not handled.

But there are other cases where things are less clear; specifically when a POJO property would have type of JsonNode. In that case type information handling must be robust enough to survive mix'n matching.

[cowtowncoder]

Ok, I add these as failing unit tests; fix will need to wait until 2.1 is released this week.

In the meantime, it is safe to say that "default typing" will not work well with conversion between trees and JSON Values, for same reason as default typing (or type info embedding) will not work with plain "convertValue()".
But I hope that we may be able to fix tree to/from value cases, such that type tree handlers will ignore type information (not add any when serializing; not expect any when deserializing).

[electricmonk]

I'd prefer it if the tree model will just treat the type property as another node in the tree. This way, transforming from serialized form to tree will work and then transforming from tree to an object graph will also work.

[cowtowncoder]

Agreed, but the problem is that ObjectMapper treats JsonNode just like any other object
And when "default typing" is enabled, this means that type information is both included in serialization and excepted in deserialization.
This is where implementation details of 'convertValue()' pop in -- since it is implemented as serialize-deserialize pair, default typing generally messes it up. Worse, since default typing setting affects construction of actual serializers (and maybe deserializers), its effects are not easy to undo, and specifically it can not be changed on per-call basis.

So I have to think about proper way to deal with this. Conceptually I fully agree in that it'd be best to just expose physical JSON Tree details, including type information.

One work around for now is to use two ObjectMappers, and instead of 'convertValue' (or its variants, 'valueToTree' and 'treeToValue' which are just convenience wrappers) do conversions explicitly by serializing using one, deserialize using the other.

[bostjanl]

Any news regarding this issue? We have exactly the same problem.

[cowtowncoder]

No news. I can have a look at tests to see if I can think of anything new here.

[cowtowncoder]

@bostjanl Can you elaborate on what is your specific usage, and which Jackson version you are using?

[bostjanl]

Using the latest released version - 2.3.1.
We're trying to convert JSON which was generated with default typing to JsonNode.

[cowtowncoder]

Ok. Are you using convertValue() or valueToTree()? Asking because there were changes in 2.3 to try to handle convertValue() more gracefully wrt some aspects (wrapper name I think).

[cowtowncoder]

Added unit tests (failing) for this -- have an idea of how this should be handled. Basically, if root type is JsonNode (or, more generally maybe, TreeNode`), default typing should be disabled. That is easily said, but we'll see how easy or difficult it is to do...

[cowtowncoder]

I think there are two ways to tackle this; in both cases need to prevent use of type info for TreeNode even if default typing would otherwise apply:

1. Change semantics of default typing options, to exclude TreeNode type and subtypes.
2. Change behavior of root serializer/deserializer fetching to not look for TypeSerializer/TypeDeserializer.

Both should work, need to think of which one would be preferable.

[bostjanl]

That was fast :-)
Tested with latest commit and it works great for us, thank you.

[cowtowncoder]

Ok good. Sometimes problem is not as difficult to fix as I initially fear. :)

[cowtowncoder]

As an added bonus, I suspect #353 got fixed due to this as well; tests pass for 2.4 (master).