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

Converter called twice results in ClassCastException #1189

Closed
carrino opened this issue Apr 7, 2016 · 9 comments
Closed

Converter called twice results in ClassCastException #1189

carrino opened this issue Apr 7, 2016 · 9 comments
Milestone

Comments

@carrino
Copy link

carrino commented Apr 7, 2016

I have a minimal repro below. testConverter fails with a class cast. The converter is called once on JsonNode and returns a SubClass. Then it is called again with that returned SubClass and fails.

testConverter2 passes.

import com.fasterxml.jackson.annotation.JsonSubTypes;
import com.fasterxml.jackson.annotation.JsonTypeInfo;
import com.fasterxml.jackson.annotation.JsonTypeInfo.As;
import com.fasterxml.jackson.annotation.JsonTypeInfo.Id;
import com.fasterxml.jackson.annotation.JsonTypeName;
import com.fasterxml.jackson.core.JsonProcessingException;
import com.fasterxml.jackson.databind.JsonNode;
import com.fasterxml.jackson.databind.ObjectMapper;
import com.fasterxml.jackson.databind.annotation.JsonDeserialize;
import com.fasterxml.jackson.databind.annotation.JsonSerialize;
import com.fasterxml.jackson.databind.node.JsonNodeFactory;
import com.fasterxml.jackson.databind.node.ObjectNode;
import com.fasterxml.jackson.databind.util.StdConverter;
import java.util.Collections;
import org.junit.Assert;
import org.junit.Test;

public class JacksonConverterTest {
    @JsonSubTypes({
        @JsonSubTypes.Type(SubClass.class),
    })
    @JsonTypeInfo(use = Id.NAME, include = As.EXISTING_PROPERTY, property = "type", visible = false)
    interface Base {
        String getType();
    }

    @JsonTypeName("sub")
    @JsonSerialize(as = ImmutableSubClass.class)
    @JsonDeserialize(as = ImmutableSubClass.class)
    interface SubClass extends Base {
        @Override
        default String getType() {
            return "sub";
        }

        default boolean getVisible() {
            return true;
        }

    }

    static class ImmutableSubClass implements SubClass {
        public boolean visible = true;

        @Override
        public int hashCode() {
            return visible ? 0 : 1;
        }

        @Override
        public boolean equals(Object obj) {
            if (this == obj)
                return true;
            if (obj == null)
                return false;
            if (getClass() != obj.getClass())
                return false;
            ImmutableSubClass other = (ImmutableSubClass) obj;
            if (visible != other.visible)
                return false;
            return true;
        }
    }

    @Test
    public void testConverter() throws Exception {
        ObjectMapper mapper = new ObjectMapper();
        ObjectNode json = new ObjectNode(JsonNodeFactory.instance);
        json.put("hidden", false);
        json.put("type", "sub");

        mapper.setMixIns(Collections.singletonMap(SubClass.class, SubClassMixin.class));

        SubClass value = mapper.treeToValue(json, SubClass.class);
        Assert.assertEquals(new ImmutableSubClass(), value);
    }

    @Test
    public void testConverter2() throws Exception {
        ObjectMapper mapper = new ObjectMapper();
        ObjectNode json = new ObjectNode(JsonNodeFactory.instance);
        json.put("hidden", false);
        json.put("type", "sub");

        mapper.setMixIns(Collections.singletonMap(SubClass.class, SubClassMixin.class));

        Base value = mapper.treeToValue(json, Base.class);
        Assert.assertEquals(new ImmutableSubClass(), value);
    }

    @JsonDeserialize(converter = SubClassConverter.class)
    interface SubClassMixin {}

    static class SubClassConverter extends StdConverter<JsonNode, SubClass> {
        @Override
        public SubClass convert(JsonNode value) {
            // Modify the old Json in some way that the new library will accept.
            ObjectNode node = (ObjectNode) value;
            boolean hidden = node.remove("hidden").asBoolean(false);
            node.put("visible", !hidden);
            node.put("type", "sub");
            try {
                return new ObjectMapper().treeToValue(node, SubClass.class);
            } catch (JsonProcessingException e) {
                throw new RuntimeException(e);
            }
        }
    }
}
@cowtowncoder
Copy link
Member

Could you please include stack trace for exception, as well as Jackson version you are using.

@carrino
Copy link
Author

carrino commented Apr 7, 2016

Yes, good point. I am on jackson 2.6.5

java.lang.ClassCastException: JacksonConverterTest$ImmutableSubClass cannot be cast to com.fasterxml.jackson.databind.JsonNode
    at JacksonConverterTest$SubClassConverter.convert(JacksonConverterTest.java:1)
    at com.fasterxml.jackson.databind.deser.std.StdDelegatingDeserializer.convertValue(StdDelegatingDeserializer.java:230)
    at com.fasterxml.jackson.databind.deser.std.StdDelegatingDeserializer.deserializeWithType(StdDelegatingDeserializer.java:179)
    at com.fasterxml.jackson.databind.deser.impl.TypeWrappedDeserializer.deserialize(TypeWrappedDeserializer.java:42)
    at com.fasterxml.jackson.databind.ObjectMapper._readValue(ObjectMapper.java:3708)
    at com.fasterxml.jackson.databind.ObjectMapper.readValue(ObjectMapper.java:2005)
    at com.fasterxml.jackson.databind.ObjectMapper.treeToValue(ObjectMapper.java:2476)
    at JacksonConverterTest.testConverter(JacksonConverterTest.java:75)

@cowtowncoder
Copy link
Member

Ok, I suspect that combination of polymorphic type handling, and forced subtype, combined with converter does not play well together. In fact I am not sure if, specifically, polymorphic subtype handling and converter can even be made to work together.

@cowtowncoder
Copy link
Member

Follow-up comment on above: I am not saying this won't work, just that it is not entirely surprising there may be issues. But I wonder if there was a way to simplify test to tease out the failure.

@carrino
Copy link
Author

carrino commented Apr 8, 2016

The funny thing is that testConvert2 actually loads it polymorphically and
that is the one that passes, which leads me to think that it could work.
I'm hoping we can just remove the double convert in the testConvert case.

@cowtowncoder
Copy link
Member

@carrino agreed.

@cowtowncoder
Copy link
Member

Ok. In general case, I don't think combination of Converter and polymorphic handling can be made to work reasonably: combination of type handling and actual deserialization are closely coupled, and I can not think of where conversion should occur to work for generic case. Conceptually conversion should probably work between initial type detection and resolution and actual deserialization but all in all I don't really see how pieces would naturally fit together.

What I can change is the existing handling in case where both are used, in a way that ClassCastException is avoided, and what effectively happens is that Converter is used and any polymorphic handling is ignored.
Handling will not be very natural for some of type id inclusion mechanisms because type id will be exposed to Converter, but doing anything else would require additions to TypeDeserializer. And this to support case that may well be unsupportable.

Anyway; making the change should allow use of Converters instead of automatic CCE, so it should be better than current behavior.

@cowtowncoder cowtowncoder added this to the 2.7.4 milestone Apr 13, 2016
@carrino
Copy link
Author

carrino commented Apr 13, 2016

I'm not intimately familiar with the inner workings but I was hoping the
the destination type could be resolved then we'd see it has a converter and
then that would resolve in 2 steps.

Other option is we don't need to call the converter if the type is already
the dest type. I'll try out this change soon
On Apr 12, 2016 7:53 PM, "Tatu Saloranta" [email protected] wrote:

Closed #1189 #1189
via c12fecd
c12fecd
.


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#1189 (comment)

@cowtowncoder
Copy link
Member

@carrino If the polymorphic type was related to type that was given to Converter, yes, it could work.
But because it is related to eventual target type that converter is responsible for producing (Base) -- and not to intermediate type (JsonNode), Jackson can not use type id for anything, nor pass it, even if it could extract it.

There may be cases where types are chosen in such a way that things would work, but as far as I can see this is not such case; and I don't like solutions that would sometimes work, other times not, depending on exact situation.

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

2 participants