Possibly wrong behavior of @JsonMerge

[nlisker]

Search before asking

• I searched in the issues and found nothing similar.

Describe the bug

From #4756:

Test case:

import java.util.ArrayList;
import java.util.List;

import com.fasterxml.jackson.annotation.JsonMerge;
import com.fasterxml.jackson.annotation.JsonProperty;
import com.fasterxml.jackson.databind.json.JsonMapper;

// dummy interface to "confuse" type resolution
interface MyList<T> extends List<T> {}

class MergeList {

	// dummy implementation of MyList that Jackson shouldn't care about with a black-box creator
	private static class MyArrayList<T> extends ArrayList<T> implements MyList<T> {}
	static <T> MyList<T> create() {
		return new MyArrayList<T>();
	}

	@JsonMerge
	@JsonProperty
	MyList<String> values = create();

	{
		values.add("a");
	}

	public static void main(String[] args) throws Exception {
		JsonMapper MAPPER = JsonMapper.builder().build();
		MergeList mergeList = new MergeList();
		var string = MAPPER.writeValueAsString(mergeList);
		System.out.println(string);
		MergeList w = MAPPER.readValue(("{\"values\":[\"x\"]}"), MergeList.class);
		System.out.println(w.values);
	}
}

Throws

Exception in thread "main" com.fasterxml.jackson.databind.exc.InvalidDefinitionException: Cannot find a deserializer for non-concrete Collection type [collection type; class example.MyList, contains [simple type, class java.lang.String]]
 at [Source: REDACTED (`StreamReadFeature.INCLUDE_SOURCE_IN_LOCATION` disabled); line: 1, column: 1]
	at com.fasterxml.jackson.databind.exc.InvalidDefinitionException.from(InvalidDefinitionException.java:67)
	at com.fasterxml.jackson.databind.DeserializationContext.reportBadDefinition(DeserializationContext.java:1888)
	at com.fasterxml.jackson.databind.deser.DeserializerCache._createAndCache2(DeserializerCache.java:321)
	at com.fasterxml.jackson.databind.deser.DeserializerCache._createAndCacheValueDeserializer(DeserializerCache.java:284)
	at com.fasterxml.jackson.databind.deser.DeserializerCache.findValueDeserializer(DeserializerCache.java:174)
	at com.fasterxml.jackson.databind.DeserializationContext.findNonContextualValueDeserializer(DeserializationContext.java:659)
	at com.fasterxml.jackson.databind.deser.BeanDeserializerBase.resolve(BeanDeserializerBase.java:552)
	at com.fasterxml.jackson.databind.deser.DeserializerCache._createAndCache2(DeserializerCache.java:347)
	at com.fasterxml.jackson.databind.deser.DeserializerCache._createAndCacheValueDeserializer(DeserializerCache.java:284)
	at com.fasterxml.jackson.databind.deser.DeserializerCache.findValueDeserializer(DeserializerCache.java:174)
	at com.fasterxml.jackson.databind.DeserializationContext.findRootValueDeserializer(DeserializationContext.java:669)
	at com.fasterxml.jackson.databind.ObjectMapper._findRootDeserializer(ObjectMapper.java:5048)
	at com.fasterxml.jackson.databind.ObjectMapper._readMapAndClose(ObjectMapper.java:4918)
	at com.fasterxml.jackson.databind.ObjectMapper.readValue(ObjectMapper.java:3860)
	at com.fasterxml.jackson.databind.ObjectMapper.readValue(ObjectMapper.java:3828)
	at example.MergeList.main(MergeList.java:33)

If I understood @JsonMerge correctly, it should have resolved correctly:

merging meaning that the current value is first accessed (with a getter or field) and then modified with incoming data. If merging is not used assignment happens without considering current state.
...
For Collection and Arrays, merging is done by appending incoming data into contents of existing Collection/array

But the error is that Jackson cannot find a deserializer for MyList. It doesn't need to because it can access the current value (with a getter or @JsonProperty as in this example) and modify it. Without @JsonMerge, the current state of the values field is not considered and so I expect exactly this kind of failure.
Perhaps I misunderstood @JsonMerge, but the docs are quite clear I think.

A workaround is possible via a custom setter:

private void setMyList(List<Integer> ints) { ints.forEach(MyList::add); }

But this "pollutes" the class and also requires opens declarations in module-info to be able to read the private setter.

Version Information

2.17 and 2.18

[JooHyukKim]

// dummy interface to "confuse" type resolution

Why would you try to confuse type resolution?

Below seems enough for reprouction.

class MergeListTest {
    private static class MyArrayList<T> extends ArrayList<T> {
        static int constructorCount = 0;
        public MyArrayList() {
            if (constructorCount > 0) {
                throw new Error("Should only be called once");
            }
            ++constructorCount;
        }
    }

    public static class MergeList {
        @JsonMerge
        @JsonProperty
        List<String> values = create();

    }

    public static List<String> create() {
        List<String> list = new MyArrayList<>();
        list.add("a");
        return list;
    }

    public static void main(String[] args) throws Exception {
        JsonMapper MAPPER = JsonMapper.builder().build();
        MergeList mergeList = new MergeList();

        // Fails with exception here
        MergeList w = MAPPER.readValue(("{\"values\":[\"x\"]}"), MergeList.class);
    }
}

[JooHyukKim]

I don't know honestly. The documentation does not mention anything about instantiation. The part of JavaDoc shared...

For Collection and Arrays, merging is done by appending incoming data into contents of existing Collection/array

... also could be understood as "incoming collection into existing collection", though the expressions may be ambiguous?
Just because we read and understood JavaDoc this way and implementation works the other way shouldn't mean it's bug --that would be too harsh. Let's wait for @cowtowncoder. We weren't there to help write documentations.

Additionally, here is blog written about, when @JsonMerge first came out. You may find some more hint on what original intention would be.

[cowtowncoder]

I think this case should work so it seems like a legit bug. There are quite a few @JsonMerge tests under com.fasterxml.jackson.databind.deser.merge, including CollectionMergeTest. But I suspect none tests custom List for which deserializer can't be created -- so you may have uncovered an edge case @nlisker .

Checking in simplified/minimal reproduction under src/test/java/.../tofix would make sense.

[JooHyukKim]

Okay, going through the reproduction over and over, got me wondering... How would Mapper would update existing list if there is no shared connection between the original object and deserialization? The list field is not and cannot even be static 🤔

So I went on to read a Baeldung blog written about @JsonMerge usage. And using that proper usage, test passes.

We should be doing like ...

 MAPPER.readerForUpdating(Object).readValue(("{\"values\":[\"x\"]}"), MergeList.class);`

... this, where as original reproduction was only using....

`MAPPER.readValue(("{\"values\":[\"x\"]}"), MergeList.class);` 

I wrote a test PR #4784 to demonstrate that, @nlisker so PTAL.

[cowtowncoder]

@JooHyukKim There is initialization block:

    public static class MergeList {
        @JsonMerge
        @JsonProperty
        List<String> values = create();

    }

which initializes value when MergeList is created by ObjectMapper (via regular BeanDeserializer constructed for MergeList).

So I don't think "updatingReader" should be needed here.

Looking at CollectionMergeTest it seems there is exact test:

    @Test
    public void testCollectionMerging() throws Exception
    {
        CollectionWrapper w = MAPPER.readValue(a2q("{'bag':['b']}"), CollectionWrapper.class);
        assertEquals(2, w.bag.size());
        assertTrue(w.bag.contains("a"));
        assertTrue(w.bag.contains("b"));
    }

    static class CollectionWrapper {
        @JsonMerge
        public Collection<String> bag = new TreeSet<String>();
        {
            bag.add("a");
        }
    }

with one difference being that Collection is of known JDK type, not custom one.

So I wonder if/suspect that custom subtype is significant here.

[nlisker]

I never used readerForUpdating. My use case is rather straightforward. I have a data object that holds various data types, and one of them is an interface MyList<T> extends List<T> which is initialized on construction, as can be done with JDK's List<T>. When I deserialize, because List is a known JDK collection, Jackson knows what to do under the hood:

class MergeList {

	interface MyList<T> extends List<T> {}
	private static class MyArrayList<T> extends ArrayList<T> implements MyList<T> {}
	static <T> MyList<T> create() {
		return new MyArrayList<T>();
	}

	@JsonMerge
	@JsonProperty
	List<String> values = create(); // known collection

	public static void main(String[] args) throws Exception {
		JsonMapper MAPPER = JsonMapper.builder().build();
		MergeList mergeList = new MergeList();
		var string = MAPPER.writeValueAsString(mergeList);
		MergeList w = MAPPER.readValue(string, MergeList.class);
	}
}

completes successfully. Changing List to MyList for values throws the deserialization exception, even though Jackson can check that MyList is a List. This is the perplexing part. If Jackson looks for known types by x.getClass() == List.class then this discrepancy would happen, but using Class::isInstance or Class::isAssignableFrom should expand the default handling to any subclass/interface. I assume there's also code for calling add and put on these known collection for the process of merging, they would be called the same way also for subtyped user-made collections.

So to answer @JooHyukKim:

How would Mapper would update existing list if there is no shared connection between the original object and deserialization?

However is works already with List.

[cowtowncoder]

@nlisker I think the problem really is that construction of deserializer for custom type fails due to it being abstract type -- and we do need a JsonDeserializer to actually do merge (it's part of deserialize() that is given existing value to modify).
To fix this, we'd need to figure out how to postpone failure due to lack of Creator (it's abstract type...).
In theory this is doable, of course, but in practice many things are done eagerly and are difficult to change to do on-demand (and more complex, less efficient). The thing is: code that looks up JsonDeserializer does not (and maybe cannot) know that ValueInstantiator is not needed -- it simply does not have that information.

In the meantime, you might be able to work around the issue if you can use concrete type instead of MyList. Or possible having static factory method annotated with @JsonCreator on it (it need not to anything, won't be called on merge case).
I mention this because actual fix may be difficult to figure out.

So basically: List/Collection are mapped to defaults (some well-known types, like ArrayList), and they all are concrete, have 0-args constructor, so it's known how to construct instances (to create ValueInstantiator). As such, deserializer can be created.
This is not true for MyList.

[cowtowncoder]

@JooHyukKim I think your MyArrayList work because it is concrete type with no-args Constructor.

But I am not 100% sure if (and if so, why) mapper.readerForUpdating() might work around the isssue -- there would still be need for deserializer in that case.

[cowtowncoder]

@nlisker Oh. Another thing that would work is a custom JsonDeserializer for MyList (registered via Module or @JsonDeserialize(using = ...) annotation). That'd need to implement merging by overloading secondary deserialize() method (one taking existing value). Not sure how easy it'd be use CollectionDeserializer (or one of base classes).

[nlisker]

There's already a workaround with a custom setter that I wrote in the initial post.

I don't understand why a deserializer is needed for an instance that is already created. As can be seen in the setter workaround, Jackson already knows to create a List as an input to the setter (or Map if I have a custom map). From there it's a matter of abiding by the contract of List. What am I missing?

[cowtowncoder]

@nlisker JsonDeserializer is needed because it has code that actually handles merging (add to Collection). That's a hard constraint and not something that can really be changed.

But whether deserializer must be able to create an instance is not strictly needed for case like this. As things are, no deserializer can be constructed for abstract collection type it seems. So question is whether this can be changed or not.