Allow @JsonMerge with Custom List

[JooHyukKim]

To address #4783

Hopefully it's okay to add to 2.18

[JooHyukKim]

Feel free to close! Wrote a PR in case we might want to add this to test cases,

[cowtowncoder]

As per my comments elsewhere, I don't think mapper.readerForUpdating() should be needed in this case. Unless existing merge tests has something that work around the issue, different from case here.

[JooHyukKim]

You are right. @JsonMerge with custom Collection type does not work.
Modified test accordingly.

I will probably investigate how different between JDK/custom collection case differ, probably at below point to be specific.

[cowtowncoder]

Note _mapAbstractCollectionType simply maps things like Collection/List -> ArrayList, Set -> LinkedHashSet and so on, to have concrete type to deserialize.
Custom type wouldn't map. But we can't resolve it by mapping; rather would need to figure out how to postpone failure so that lack of deserializer would not trigger exception for this particular @Merge case. Not sure how, but that'd be the idea.

It might be tricky, come to think of it, as there's need for a deserializer to handle merging, I think. Construction of which should not fail unless... hmmh.

[JooHyukKim]

.

[cowtowncoder]

Actually... while I understand the intent here, I don't think this check buys as anything: we already know type implements Collection, which is mappable.

So how about this: looking at what AbstractDeserializer.constructForNonPOJO(beanDesc); maybe we should just always construct that -- so remove check for

if (type.getTypeHandler() == null) {

and just do it if we cannot map abstract type. And see how that works wrt our test suite and issue here?

[JooHyukKim]

I tried actually, it fails with below exception.
I checked what gets created inside AbstractDeserializer and almost nothing to actually deserialize anything.
The intent behind the // do nothing current approach was to create StringCollectionDeserializer / CollectionDeserializer below starting at line 862.

com.fasterxml.jackson.databind.exc.InvalidDefinitionException: Cannot construct instance of `com.fasterxml.jackson.databind.tofix.JsonMergeWithCustomCollection4783Test$MyListCustom` (no Creators, like default constructor, exist): abstract types either need to be mapped to concrete types, have custom deserializer, or contain additional type information
 at [Source: REDACTED (`StreamReadFeature.INCLUDE_SOURCE_IN_LOCATION` disabled); line: 1, column: 11] (through reference chain: com.fasterxml.jackson.databind.tofix.JsonMergeWithCustomCollection4783Test$MergeCustomStrin
	```

[JooHyukKim]

But wouldn't hurt showcasing here! Lemme just make the suggested change so we both can see

[JooHyukKim]

Yup, you can see checks are failing with the same exceptions.

[JooHyukKim]

Took a shot at solving this issue.
Added comment🔗 at the implementation.
PTAL when you have time? @cowtowncoder

PS: Looking at current implementation I am not sure if we have this merged to 2.18 (seems a bit risky?)

[cowtowncoder]

No need for implements List<T>, already does via ArrayList<T>.

[cowtowncoder]

I don't think helper method needed? Just create directly with new MyArrayListCustom<>()

[cowtowncoder]

Unless you want to add values in this method?

[cowtowncoder]

Not really needed as per above (but if it was, should parameterize with String to align with name.

[cowtowncoder]

Hmmmh. If its "Long" list, shouldn't it contain Long values?

[cowtowncoder]

Ok, so, I think we can simplify the fix by using AbstractDeserializer for all cases, and see how that works.

[cowtowncoder]

Took a shot at solving this issue. Added comment🔗 at the implementation. PTAL when you have time? @cowtowncoder

PS: Looking at current implementation I am not sure if we have this merged to 2.18 (seems a bit risky?)

I agree, bit risky for 2.18, based on experience. So let's target 2.19

EDIT: I notice pr is against 2.18. We can consider that if necessary.

[JooHyukKim]

EDIT: I notice pr is against 2.18. We can consider that if necessary.

I didn't think this was an urgent issue actually, so leaning toward 2.19.
The only reason PR is against 2.18 is that we can make a choice move to 2.19 not the other way :))

I was even thinking to close(to postpone) this one since you got so much going on now @cowtowncoder

[cowtowncoder]

@JooHyukKim Ok thanks. I think we are good -- let's try getting this done, I think it is a valid problem to solve. I'll see why the test failures occur...

[cowtowncoder]

Ahhh. Ok, so that'd be too easy -- things won't work because AbstractDeserializer does not override

public T deserialize(JsonParser p, DeserializationContext ctxt, T intoValue)

and it can't really, since we need method like one from CollectionDeserializer

@Override
public Collection<Object> deserialize(JsonParser p, DeserializationContext ctxt,
        Collection<Object> result)

for merging to actually work. So basically we need a proper CollectionDeserializer that... hmmh.

Let me try a simple change to see what happens if we simply remove the check for abstract altogether.

[cowtowncoder]

Well look at that -- tests actually pass. But probably need a test for non-merging case, to see how error handling works (when trying to deserialize abstract collection without merging)

[cowtowncoder]

@JooHyukKim Ok so it appears fix might be surprisingly simple: just basically remove the "is it abstract -> fail" check. Love when this happens. :)

[cowtowncoder]

Due to simplicity of the fix, I think 2.18 might be fine after all.

[JooHyukKim]

@JooHyukKim Ok so it appears fix might be surprisingly simple: just basically remove the "is it abstract -> fail" check. Love when this happens. :)

Interestinnggg :)))
Thank you for the improvement with additional test and all 👌🏼👌🏼
Fixed test class name typo, added release notes and such.

[cowtowncoder]

Minor tweak: remove change wrt Map defaulting: will do that for 2.19, to reduce risk for 2.18.2.