`MapDeserializer` can not merge `Map`s with polymorphic values

[rgreig]

I have found what appears to be a defect in MapDeserializer when trying to use JSON merging, with a Map where the values are polymorphic types. So for example, consider a Map<String, FooBase> where FooBase is an abstract class (annotated with JsonTypoInfo and JsonSubTypes etc). When trying to merge another structure into that map, it fails when trying to merge an existing value - i.e. where there is an existing mapping from key "SomeKey" to a concrete subclass of FooBase, it fails with exception:

com.fasterxml.jackson.databind.exc.InvalidDefinitionException: Cannot construct instance of `FooBase` (no Creators, like default construct, exist): abstract types either need to be mapped to concrete types, have custom deserializer, or contain additional type information

Having looked at the code I believe the issue is in MapDeserializer, specifically in method _readAndUpdateStringKeyMap here:

Object old = result.get(key);
Object value;

if (old != null) {
    value = valueDes.deserialize(p, ctxt, old);
} else if (typeDeser == null) {
    value = valueDes.deserialize(p, ctxt);
} else {
   value = valueDes.deserializeWithType(p, ctxt, typeDeser);
}

Note in the above code, if there is an existing value it calls deserialize on the valueDes instance which is just an abstract deserializer rather than leveraging the type (in typeDeser). I don't know much more than that yet so I was hoping that someone could offer some guidance about whether this is easily fixed or going to be more involved. I'd be happy to work on a pull request with some guidance from people more familiar with the codebase.

Here is a fully self-contained example that illustrates the issue:

public class JacksonBugTest {

    private ObjectMapper mapper = new ObjectMapper();

    @JsonTypeInfo(use = JsonTypeInfo.Id.NAME, include = JsonTypeInfo.As.PROPERTY, property = "discriminator", visible = true)
    @JsonSubTypes({@JsonSubTypes.Type(value = SomeClassA.class, name = "FirstConcreteImpl")})
    @JsonInclude(JsonInclude.Include.NON_NULL)
    public static abstract class SomeBaseClass {
        private String name;

        @JsonCreator
        public SomeBaseClass(@JsonProperty("name") String name) {
            this.name = name;
        }

        public String getName() {
            return name;
        }

        public void setName(String name) {
            this.name = name;
        }
    }

    @JsonTypeName("FirstConcreteImpl")
    public static class SomeClassA extends SomeBaseClass {
        private Integer a;
        private Integer b;

        @JsonCreator
        public SomeClassA(@JsonProperty("name") String name, @JsonProperty("a") Integer a, @JsonProperty("b") Integer b) {
            super(name);
            this.a = a;
            this.b = b;
        }

        public Integer getA() {
            return a;
        }

        public void setA(Integer a) {
            this.a = a;
        }

        public Integer getB() {
            return b;
        }

        public void setB(Integer b) {
            this.b = b;
        }
    }

    public static class SomeOtherClass {

        private String someprop;

        @JsonCreator
        public SomeOtherClass(@JsonProperty("someprop") String someprop) {
            this.someprop = someprop;
        }

        @JsonMerge
        private Map<String, SomeBaseClass> data = new LinkedHashMap<>();

        public void addValue(String key, SomeBaseClass value) {
            data.put(key, value);
        }

        public Map<String, SomeBaseClass> getData() {
            return data;
        }

        public void setData(
                Map<String, SomeBaseClass> data) {
            this.data = data;
        }
    }

    @Test
    public void test1() throws Exception {
        // first let's just get some valid JSON
        SomeOtherClass test = new SomeOtherClass("house");
        test.addValue("SOMEKEY", new SomeClassA("fred", 1, null));
        String serializedValue = mapper.writeValueAsString(test);
        System.out.println("Serialized value: " + serializedValue);

        // now create a reader specifically for merging
        ObjectReader reader = mapper.readerForUpdating(test);

        SomeOtherClass toBeMerged = new SomeOtherClass("house");
        toBeMerged.addValue("SOMEKEY", new SomeClassA("jim", null, 2));
        String jsonForMerging = mapper.writeValueAsString(toBeMerged);
        System.out.println("JSON to be merged: " + jsonForMerging);
        // now try to do the merge and it blows up
        SomeOtherClass mergedResult = reader.readValue(jsonForMerging);
        System.out.println("Serialized value: " + mapper.writeValueAsString(mergedResult));
    }
}

[cowtowncoder]

Thank you for reporting this. I hope to look into this in near future, but there's bit of backlog of items so apologies in advance for this probably taking a while.

One note on abstract type deserializer: that may be ok as such deserializer should be passed a TypeDeserializer to extract necessary type id, and locate actual subtype deserializer.
But handling of that call sequence is sometimes non-obvious wrt which specific deserializer it is to be called on. And it is also true that since merging is relatively new feature there may be some rough edges regarding coordination of things.

[rgreig]

Thank you for a quick reply.

I started looking at this in the case of a polymorphic type not in a map and found this in the code (which I think was your comment):

 // 20-Oct-2016, tatu: Also tricky -- for now, report an error
        if (_valueTypeDeserializer != null) {
            ctxt.reportBadDefinition(getType(),
                    String.format("Cannot merge polymorphic property '%s'",
                            getName()));
//            return _valueDeserializer.deserializeWithType(p, ctxt, _valueTypeDeserializer);
        }

So I am guessing there is something more structurally difficult about doing this in general with the way the deserializers are currently written?

[cowtowncoder]

Ok, finally found time to revisit this. So, yes, merging polymorphic values is tricky because extra polymorphic type information must be handled, and it is not clear how you would merge, say, instances of different types (new type might have properties that old instance does not have).
But even if ignoring that problem (assuming it's fine, come what may), there is the technical problem that TypeDeserializer would still need to at least "unwrap" information to get to actual properties (remove "type id" and wrapper array/object, if any) to pass to old instance.

Now: for containers it probably would make most sense to either:

1. Unwrap type id, ignore polymorphic type, pass contents to deserializer of existing value -- alas, beyond problem of unwrapping (which might be doable but difficult wrt safely changing code), we don't really have access to that old value deserializer -- in fact, instance might not even have been deserialized by jackson for all we know. But at this point it is just value, without type info
2. Ignore merge for values and just do "shallow" merge in case of Maps (or more generally containers)

Of these, I don't think (1) can made to work. (2) might be possible, and I think I'll try that next.
If I can solve it for Map, separate work would be needed for arrays, Collections; and then there's the question of whether behavior should be configurable in that user could choose to fail in this case (since deep merge is impossible, just shallow), or opt for shallow merge.

[cowtowncoder]

Ok things look promising: test had one problem (should not have visible = true), but I think I can make this work after all.

[cowtowncoder]

So, with 2.10 things will now work (assuming visible = true is removed from the test: that is unrelated to polymorphic handling), in a way that merging works for Maps, but entries within can not be merged -- new value will be read from input and will replace old value.
This behavior can be changed by disabling MapperFeature.IGNORE_MERGE_FOR_UNMERGEABLE in which case it would result in exception, but with bit more information than previously.

If attempts to provide some form of risky merging for polymorphic values are still desired, a new issue should be created (with reference to this issue for back story). But I hope this improvement is still useful for at least some cases.