From 36fc768c87b9bf78515949f225c0c9a6fe03459f Mon Sep 17 00:00:00 2001 From: joohyukkim Date: Wed, 6 Nov 2024 09:13:47 +0900 Subject: [PATCH 01/15] test : Add test to verify JsonMerge behavior with custom merge --- ...JsonMergeWithCustomCollection4783Test.java | 68 +++++++++++++++++++ 1 file changed, 68 insertions(+) create mode 100644 src/test/java/com/fasterxml/jackson/databind/tofix/JsonMergeWithCustomCollection4783Test.java diff --git a/src/test/java/com/fasterxml/jackson/databind/tofix/JsonMergeWithCustomCollection4783Test.java b/src/test/java/com/fasterxml/jackson/databind/tofix/JsonMergeWithCustomCollection4783Test.java new file mode 100644 index 0000000000..9f54a12054 --- /dev/null +++ b/src/test/java/com/fasterxml/jackson/databind/tofix/JsonMergeWithCustomCollection4783Test.java @@ -0,0 +1,68 @@ +package com.fasterxml.jackson.databind.tofix; + +import java.util.ArrayList; +import java.util.List; + +import org.junit.jupiter.api.Test; + +import com.fasterxml.jackson.annotation.JsonMerge; +import com.fasterxml.jackson.databind.ObjectMapper; +import com.fasterxml.jackson.databind.ObjectReader; +import com.fasterxml.jackson.databind.json.JsonMapper; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertTrue; + +// [databind#4783] Test to verify that JsonMerge also works for custom list +public class JsonMergeWithCustomCollection4783Test { + + static class CustomList4783 extends ArrayList { + static int constructorCount = 0; + + public CustomList4783() { + if (constructorCount > 0) { + throw new Error("Should only be called once"); + } + ++constructorCount; + } + + public static void reset() { + constructorCount = 0; + } + } + + static class MergeList4783 { + @JsonMerge + public List values; + + public MergeList4783() { + this.values = new CustomList4783<>(); + this.values.add("a"); + } + } + + private final ObjectMapper MAPPER = JsonMapper.builder().build(); + + @Test + void testSimpleMergingWithCustomCollection() throws Exception { + _verifyMergeList( + MAPPER.readerForUpdating(new MergeList4783()) + ); + CustomList4783.reset(); + + _verifyMergeList( + MAPPER.readerFor(MergeList4783.class) + .withValueToUpdate(new MergeList4783()) + ); + CustomList4783.reset(); + } + + private void _verifyMergeList(ObjectReader reader) throws Exception { + MergeList4783 result = reader.readValue("{\"values\":[\"x\"]}", MergeList4783.class); + + assertEquals(2, result.values.size()); + assertTrue(result.values.contains("x")); + assertTrue(result.values.contains("a")); + assertTrue(CustomList4783.constructorCount == 1); + } +} From 6289ba397edc377c8944ec0f7e1b4695a3db0a4c Mon Sep 17 00:00:00 2001 From: joohyukkim Date: Wed, 6 Nov 2024 18:15:18 +0900 Subject: [PATCH 02/15] Modify test --- ...JsonMergeWithCustomCollection4783Test.java | 65 +++++++++---------- 1 file changed, 32 insertions(+), 33 deletions(-) diff --git a/src/test/java/com/fasterxml/jackson/databind/tofix/JsonMergeWithCustomCollection4783Test.java b/src/test/java/com/fasterxml/jackson/databind/tofix/JsonMergeWithCustomCollection4783Test.java index 9f54a12054..ed19eb0a61 100644 --- a/src/test/java/com/fasterxml/jackson/databind/tofix/JsonMergeWithCustomCollection4783Test.java +++ b/src/test/java/com/fasterxml/jackson/databind/tofix/JsonMergeWithCustomCollection4783Test.java @@ -6,63 +6,62 @@ import org.junit.jupiter.api.Test; import com.fasterxml.jackson.annotation.JsonMerge; +import com.fasterxml.jackson.annotation.JsonProperty; import com.fasterxml.jackson.databind.ObjectMapper; -import com.fasterxml.jackson.databind.ObjectReader; import com.fasterxml.jackson.databind.json.JsonMapper; +import com.fasterxml.jackson.databind.testutil.failure.JacksonTestFailureExpected; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertTrue; // [databind#4783] Test to verify that JsonMerge also works for custom list -public class JsonMergeWithCustomCollection4783Test { +public class JsonMergeWithCustomCollection4783Test +{ - static class CustomList4783 extends ArrayList { - static int constructorCount = 0; - - public CustomList4783() { - if (constructorCount > 0) { - throw new Error("Should only be called once"); - } - ++constructorCount; - } + private static class MyArrayListJDK extends ArrayList implements List { } + static class MergeListJDK { + @JsonMerge + @JsonProperty + public List values = create(); + { values.add("a");} + } - public static void reset() { - constructorCount = 0; - } + static List create() { + return new MyArrayListJDK(); } - static class MergeList4783 { + interface MyListCustom extends List { } + private static class MyArrayListCustom extends ArrayList implements MyListCustom { } + static class MergeListCustom { @JsonMerge - public List values; + @JsonProperty + public MyListCustom values = createCustom(); + { values.add("a"); } + } - public MergeList4783() { - this.values = new CustomList4783<>(); - this.values.add("a"); - } + static MyListCustom createCustom() { + return new MyArrayListCustom(); } private final ObjectMapper MAPPER = JsonMapper.builder().build(); @Test - void testSimpleMergingWithCustomCollection() throws Exception { - _verifyMergeList( - MAPPER.readerForUpdating(new MergeList4783()) - ); - CustomList4783.reset(); + void testJDKMapperReading() throws Exception { + MergeListJDK result = MAPPER.readValue("{\"values\":[\"x\"]}", MergeListJDK.class); - _verifyMergeList( - MAPPER.readerFor(MergeList4783.class) - .withValueToUpdate(new MergeList4783()) - ); - CustomList4783.reset(); + assertEquals(2, result.values.size()); + assertTrue(result.values.contains("x")); + assertTrue(result.values.contains("a")); } - private void _verifyMergeList(ObjectReader reader) throws Exception { - MergeList4783 result = reader.readValue("{\"values\":[\"x\"]}", MergeList4783.class); + @JacksonTestFailureExpected + @Test + void testCustomMapperReading() throws Exception { + MergeListCustom result = MAPPER.readValue("{\"values\":[\"x\"]}", MergeListCustom.class); assertEquals(2, result.values.size()); assertTrue(result.values.contains("x")); assertTrue(result.values.contains("a")); - assertTrue(CustomList4783.constructorCount == 1); } + } From 8667a7e61e7cb7986dcafde86e3dfc850584a7dc Mon Sep 17 00:00:00 2001 From: joohyukkim Date: Fri, 8 Nov 2024 22:45:17 +0900 Subject: [PATCH 03/15] Possible implementation? --- .../deser/BasicDeserializerFactory.java | 22 +++++- ...JsonMergeWithCustomCollection4783Test.java | 73 +++++++++++++++++-- 2 files changed, 87 insertions(+), 8 deletions(-) diff --git a/src/main/java/com/fasterxml/jackson/databind/deser/BasicDeserializerFactory.java b/src/main/java/com/fasterxml/jackson/databind/deser/BasicDeserializerFactory.java index 8c262cf406..ca5d6483dd 100644 --- a/src/main/java/com/fasterxml/jackson/databind/deser/BasicDeserializerFactory.java +++ b/src/main/java/com/fasterxml/jackson/databind/deser/BasicDeserializerFactory.java @@ -827,9 +827,16 @@ public JsonDeserializer createCollectionDeserializer(DeserializationContext c if (implType == null) { // [databind#292]: Actually, may be fine, but only if polymorphich deser enabled if (type.getTypeHandler() == null) { - throw new IllegalArgumentException("Cannot find a deserializer for non-concrete Collection type "+type); + // [databind#4783] Since 2.19, Allowe @JsonMerge with Custom Collection + // classes that extend JDK Collection class can handle deserialization + if (_canMapToSuperInterfaceCollectionType(type, config)) { + // do nothing + } else { + throw new IllegalArgumentException("Cannot find a deserializer for non-concrete Collection type "+type); + } + } else { + deser = AbstractDeserializer.constructForNonPOJO(beanDesc); } - deser = AbstractDeserializer.constructForNonPOJO(beanDesc); } else { type = implType; // But if so, also need to re-check creators... @@ -867,6 +874,17 @@ public JsonDeserializer createCollectionDeserializer(DeserializationContext c return deser; } + protected boolean _canMapToSuperInterfaceCollectionType(CollectionType type, DeserializationConfig config) + { + for (JavaType superType : type.getInterfaces()) { + Class collectionClass = ContainerDefaultMappings.findCollectionFallback(superType); + if (collectionClass != null) { + return true; + } + } + return false; + } + protected CollectionType _mapAbstractCollectionType(JavaType type, DeserializationConfig config) { final Class collectionClass = ContainerDefaultMappings.findCollectionFallback(type); diff --git a/src/test/java/com/fasterxml/jackson/databind/tofix/JsonMergeWithCustomCollection4783Test.java b/src/test/java/com/fasterxml/jackson/databind/tofix/JsonMergeWithCustomCollection4783Test.java index ed19eb0a61..af23398bd9 100644 --- a/src/test/java/com/fasterxml/jackson/databind/tofix/JsonMergeWithCustomCollection4783Test.java +++ b/src/test/java/com/fasterxml/jackson/databind/tofix/JsonMergeWithCustomCollection4783Test.java @@ -9,7 +9,6 @@ import com.fasterxml.jackson.annotation.JsonProperty; import com.fasterxml.jackson.databind.ObjectMapper; import com.fasterxml.jackson.databind.json.JsonMapper; -import com.fasterxml.jackson.databind.testutil.failure.JacksonTestFailureExpected; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertTrue; @@ -19,6 +18,7 @@ public class JsonMergeWithCustomCollection4783Test { private static class MyArrayListJDK extends ArrayList implements List { } + static class MergeListJDK { @JsonMerge @JsonProperty @@ -31,18 +31,64 @@ static List create() { } interface MyListCustom extends List { } + private static class MyArrayListCustom extends ArrayList implements MyListCustom { } - static class MergeListCustom { + + static class MergeCustomStringList { @JsonMerge @JsonProperty - public MyListCustom values = createCustom(); + public MyListCustom values = createCustomStringList(); { values.add("a"); } } - static MyListCustom createCustom() { + static MyListCustom createCustomStringList() { return new MyArrayListCustom(); } + interface MyCustomLongList extends List { } + + private static class MyArrayCustomLongList extends ArrayList implements MyCustomLongList { } + + static class MergeMyCustomLongList { + @JsonMerge + @JsonProperty + public MyCustomLongList values = createCustomLongList(); + { values.add("a"); } + } + + static MyCustomLongList createCustomLongList() { + return new MyArrayCustomLongList(); + } + + interface MyCustomPojoList extends List { } + + private static class MyArrayCustomPojoList extends ArrayList implements MyCustomPojoList { } + + static class MergeMyCustomPojoList { + @JsonMerge + @JsonProperty + public MyCustomPojoList values = createCustomPojoList(); + } + + private static MyCustomPojoList createCustomPojoList() { + MyCustomPojoList list = new MyArrayCustomPojoList(); + list.add(CustomPojo.create("a", 1)); + list.add(CustomPojo.create("b", 2)); + return list; + } + + public static class CustomPojo { + public String name; + public int age; + + public static CustomPojo create(String name, int age) { + CustomPojo pojo = new CustomPojo(); + pojo.name = name; + pojo.age = age; + return pojo; + } + } + private final ObjectMapper MAPPER = JsonMapper.builder().build(); @Test @@ -54,14 +100,29 @@ void testJDKMapperReading() throws Exception { assertTrue(result.values.contains("a")); } - @JacksonTestFailureExpected @Test void testCustomMapperReading() throws Exception { - MergeListCustom result = MAPPER.readValue("{\"values\":[\"x\"]}", MergeListCustom.class); + MergeCustomStringList result = MAPPER.readValue("{\"values\":[\"x\"]}", MergeCustomStringList.class); assertEquals(2, result.values.size()); assertTrue(result.values.contains("x")); assertTrue(result.values.contains("a")); } + @Test + void testCustomMapperReadingLongArrayList() throws Exception { + MergeMyCustomLongList result = MAPPER.readValue("{\"values\":[\"x\"]}", MergeMyCustomLongList.class); + + assertEquals(2, result.values.size()); + assertTrue(result.values.contains("x")); + assertTrue(result.values.contains("a")); + } + + @Test + void testCustomMapperReadingPojoArrayList() throws Exception { + MergeMyCustomPojoList result = MAPPER.readValue("{\"values\":[{\"name\":\"c\",\"age\":3}]}", MergeMyCustomPojoList.class); + + assertEquals(3, result.values.size()); + } + } From 85c9b911c13869e48cfe9552abc501dcd0450e98 Mon Sep 17 00:00:00 2001 From: joohyukkim Date: Fri, 8 Nov 2024 22:47:34 +0900 Subject: [PATCH 04/15] chore : rename method and comment --- .../jackson/databind/deser/BasicDeserializerFactory.java | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/main/java/com/fasterxml/jackson/databind/deser/BasicDeserializerFactory.java b/src/main/java/com/fasterxml/jackson/databind/deser/BasicDeserializerFactory.java index ca5d6483dd..3fc8ca6143 100644 --- a/src/main/java/com/fasterxml/jackson/databind/deser/BasicDeserializerFactory.java +++ b/src/main/java/com/fasterxml/jackson/databind/deser/BasicDeserializerFactory.java @@ -827,9 +827,9 @@ public JsonDeserializer createCollectionDeserializer(DeserializationContext c if (implType == null) { // [databind#292]: Actually, may be fine, but only if polymorphich deser enabled if (type.getTypeHandler() == null) { - // [databind#4783] Since 2.19, Allowe @JsonMerge with Custom Collection - // classes that extend JDK Collection class can handle deserialization - if (_canMapToSuperInterfaceCollectionType(type, config)) { + // [databind#4783] Since 2.19, Allow `@JsonMerge` with Custom Collection extension. + // Let's not throw exceptino so instantiation can happen downstream + if (_canMapSuperInterfaceAsAbstractCollectionType(type, config)) { // do nothing } else { throw new IllegalArgumentException("Cannot find a deserializer for non-concrete Collection type "+type); @@ -874,7 +874,7 @@ public JsonDeserializer createCollectionDeserializer(DeserializationContext c return deser; } - protected boolean _canMapToSuperInterfaceCollectionType(CollectionType type, DeserializationConfig config) + protected boolean _canMapSuperInterfaceAsAbstractCollectionType(CollectionType type, DeserializationConfig config) { for (JavaType superType : type.getInterfaces()) { Class collectionClass = ContainerDefaultMappings.findCollectionFallback(superType); From 53c6455f458b107e1611ef536ed00a3e2afb0996 Mon Sep 17 00:00:00 2001 From: joohyukkim Date: Sat, 9 Nov 2024 10:30:11 +0900 Subject: [PATCH 05/15] test : clean up unnecessary parameterization --- ...JsonMergeWithCustomCollection4783Test.java | 49 +++++-------------- 1 file changed, 13 insertions(+), 36 deletions(-) diff --git a/src/test/java/com/fasterxml/jackson/databind/tofix/JsonMergeWithCustomCollection4783Test.java b/src/test/java/com/fasterxml/jackson/databind/tofix/JsonMergeWithCustomCollection4783Test.java index af23398bd9..5b5605ecc6 100644 --- a/src/test/java/com/fasterxml/jackson/databind/tofix/JsonMergeWithCustomCollection4783Test.java +++ b/src/test/java/com/fasterxml/jackson/databind/tofix/JsonMergeWithCustomCollection4783Test.java @@ -17,19 +17,15 @@ public class JsonMergeWithCustomCollection4783Test { - private static class MyArrayListJDK extends ArrayList implements List { } + private static class MyArrayListJDK extends ArrayList { } static class MergeListJDK { @JsonMerge @JsonProperty - public List values = create(); + public List values = new MyArrayListJDK<>(); { values.add("a");} } - static List create() { - return new MyArrayListJDK(); - } - interface MyListCustom extends List { } private static class MyArrayListCustom extends ArrayList implements MyListCustom { } @@ -37,44 +33,25 @@ private static class MyArrayListCustom extends ArrayList implements MyList static class MergeCustomStringList { @JsonMerge @JsonProperty - public MyListCustom values = createCustomStringList(); + public MyListCustom values = new MyArrayListCustom<>(); { values.add("a"); } } - static MyListCustom createCustomStringList() { - return new MyArrayListCustom(); - } - - interface MyCustomLongList extends List { } - - private static class MyArrayCustomLongList extends ArrayList implements MyCustomLongList { } - static class MergeMyCustomLongList { @JsonMerge @JsonProperty - public MyCustomLongList values = createCustomLongList(); - { values.add("a"); } - } - - static MyCustomLongList createCustomLongList() { - return new MyArrayCustomLongList(); + public MyListCustom values = new MyArrayListCustom<>(); + { values.add(1L); } } - interface MyCustomPojoList extends List { } - - private static class MyArrayCustomPojoList extends ArrayList implements MyCustomPojoList { } - static class MergeMyCustomPojoList { @JsonMerge @JsonProperty - public MyCustomPojoList values = createCustomPojoList(); - } - - private static MyCustomPojoList createCustomPojoList() { - MyCustomPojoList list = new MyArrayCustomPojoList(); - list.add(CustomPojo.create("a", 1)); - list.add(CustomPojo.create("b", 2)); - return list; + public MyListCustom values = new MyArrayListCustom<>(); + { + values.add(CustomPojo.create("a", 1)); + values.add(CustomPojo.create("b", 2)); + } } public static class CustomPojo { @@ -111,11 +88,11 @@ void testCustomMapperReading() throws Exception { @Test void testCustomMapperReadingLongArrayList() throws Exception { - MergeMyCustomLongList result = MAPPER.readValue("{\"values\":[\"x\"]}", MergeMyCustomLongList.class); + MergeMyCustomLongList result = MAPPER.readValue("{\"values\":[7]}", MergeMyCustomLongList.class); assertEquals(2, result.values.size()); - assertTrue(result.values.contains("x")); - assertTrue(result.values.contains("a")); + assertTrue(result.values.contains(1L)); + assertTrue(result.values.contains(7L)); } @Test From 434e41830aad6372c252cf9f91ed176f8b258b39 Mon Sep 17 00:00:00 2001 From: joohyukkim Date: Sat, 9 Nov 2024 10:47:40 +0900 Subject: [PATCH 06/15] feature : Straight up create AbstractDeserializer.constructForNonPOJO(beanDesc); --- .../databind/deser/BasicDeserializerFactory.java | 13 +------------ 1 file changed, 1 insertion(+), 12 deletions(-) diff --git a/src/main/java/com/fasterxml/jackson/databind/deser/BasicDeserializerFactory.java b/src/main/java/com/fasterxml/jackson/databind/deser/BasicDeserializerFactory.java index 3fc8ca6143..f3ed5680b8 100644 --- a/src/main/java/com/fasterxml/jackson/databind/deser/BasicDeserializerFactory.java +++ b/src/main/java/com/fasterxml/jackson/databind/deser/BasicDeserializerFactory.java @@ -825,18 +825,7 @@ public JsonDeserializer createCollectionDeserializer(DeserializationContext c if (type.isInterface() || type.isAbstract()) { CollectionType implType = _mapAbstractCollectionType(type, config); if (implType == null) { - // [databind#292]: Actually, may be fine, but only if polymorphich deser enabled - if (type.getTypeHandler() == null) { - // [databind#4783] Since 2.19, Allow `@JsonMerge` with Custom Collection extension. - // Let's not throw exceptino so instantiation can happen downstream - if (_canMapSuperInterfaceAsAbstractCollectionType(type, config)) { - // do nothing - } else { - throw new IllegalArgumentException("Cannot find a deserializer for non-concrete Collection type "+type); - } - } else { - deser = AbstractDeserializer.constructForNonPOJO(beanDesc); - } + deser = AbstractDeserializer.constructForNonPOJO(beanDesc); } else { type = implType; // But if so, also need to re-check creators... From 897d27ce1a3a7d5874582c192382034f34d923c5 Mon Sep 17 00:00:00 2001 From: Tatu Saloranta Date: Sat, 9 Nov 2024 14:18:52 -0800 Subject: [PATCH 07/15] Minor clean up --- ...JsonMergeWithCustomCollection4783Test.java | 21 +++++++++++-------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/src/test/java/com/fasterxml/jackson/databind/tofix/JsonMergeWithCustomCollection4783Test.java b/src/test/java/com/fasterxml/jackson/databind/tofix/JsonMergeWithCustomCollection4783Test.java index 5b5605ecc6..ceefe18445 100644 --- a/src/test/java/com/fasterxml/jackson/databind/tofix/JsonMergeWithCustomCollection4783Test.java +++ b/src/test/java/com/fasterxml/jackson/databind/tofix/JsonMergeWithCustomCollection4783Test.java @@ -8,16 +8,16 @@ import com.fasterxml.jackson.annotation.JsonMerge; import com.fasterxml.jackson.annotation.JsonProperty; import com.fasterxml.jackson.databind.ObjectMapper; -import com.fasterxml.jackson.databind.json.JsonMapper; +import com.fasterxml.jackson.databind.testutil.DatabindTestUtil; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertTrue; // [databind#4783] Test to verify that JsonMerge also works for custom list -public class JsonMergeWithCustomCollection4783Test +@SuppressWarnings("serial") +public class JsonMergeWithCustomCollection4783Test extends DatabindTestUtil { - - private static class MyArrayListJDK extends ArrayList { } + static class MyArrayListJDK extends ArrayList { } static class MergeListJDK { @JsonMerge @@ -28,7 +28,7 @@ static class MergeListJDK { interface MyListCustom extends List { } - private static class MyArrayListCustom extends ArrayList implements MyListCustom { } + static class MyArrayListCustom extends ArrayList implements MyListCustom { } static class MergeCustomStringList { @JsonMerge @@ -66,7 +66,7 @@ public static CustomPojo create(String name, int age) { } } - private final ObjectMapper MAPPER = JsonMapper.builder().build(); + private final ObjectMapper MAPPER = newJsonMapper(); @Test void testJDKMapperReading() throws Exception { @@ -79,7 +79,8 @@ void testJDKMapperReading() throws Exception { @Test void testCustomMapperReading() throws Exception { - MergeCustomStringList result = MAPPER.readValue("{\"values\":[\"x\"]}", MergeCustomStringList.class); + MergeCustomStringList result = MAPPER.readValue("{\"values\":[\"x\"]}", + MergeCustomStringList.class); assertEquals(2, result.values.size()); assertTrue(result.values.contains("x")); @@ -88,7 +89,8 @@ void testCustomMapperReading() throws Exception { @Test void testCustomMapperReadingLongArrayList() throws Exception { - MergeMyCustomLongList result = MAPPER.readValue("{\"values\":[7]}", MergeMyCustomLongList.class); + MergeMyCustomLongList result = MAPPER.readValue("{\"values\":[7]}", + MergeMyCustomLongList.class); assertEquals(2, result.values.size()); assertTrue(result.values.contains(1L)); @@ -97,7 +99,8 @@ void testCustomMapperReadingLongArrayList() throws Exception { @Test void testCustomMapperReadingPojoArrayList() throws Exception { - MergeMyCustomPojoList result = MAPPER.readValue("{\"values\":[{\"name\":\"c\",\"age\":3}]}", MergeMyCustomPojoList.class); + MergeMyCustomPojoList result = MAPPER.readValue("{\"values\":[{\"name\":\"c\",\"age\":3}]}", + MergeMyCustomPojoList.class); assertEquals(3, result.values.size()); } From d372a7266e6004fed979488287955c272d6f7b21 Mon Sep 17 00:00:00 2001 From: Tatu Saloranta Date: Sat, 9 Nov 2024 14:25:40 -0800 Subject: [PATCH 08/15] Remove use of `AbstractDeserializer` --- .../databind/deser/BasicDeserializerFactory.java | 15 +-------------- 1 file changed, 1 insertion(+), 14 deletions(-) diff --git a/src/main/java/com/fasterxml/jackson/databind/deser/BasicDeserializerFactory.java b/src/main/java/com/fasterxml/jackson/databind/deser/BasicDeserializerFactory.java index f3ed5680b8..5750c36514 100644 --- a/src/main/java/com/fasterxml/jackson/databind/deser/BasicDeserializerFactory.java +++ b/src/main/java/com/fasterxml/jackson/databind/deser/BasicDeserializerFactory.java @@ -824,9 +824,7 @@ public JsonDeserializer createCollectionDeserializer(DeserializationContext c if (deser == null) { if (type.isInterface() || type.isAbstract()) { CollectionType implType = _mapAbstractCollectionType(type, config); - if (implType == null) { - deser = AbstractDeserializer.constructForNonPOJO(beanDesc); - } else { + if (implType != null) { type = implType; // But if so, also need to re-check creators... beanDesc = config.introspectForCreation(type); @@ -863,17 +861,6 @@ public JsonDeserializer createCollectionDeserializer(DeserializationContext c return deser; } - protected boolean _canMapSuperInterfaceAsAbstractCollectionType(CollectionType type, DeserializationConfig config) - { - for (JavaType superType : type.getInterfaces()) { - Class collectionClass = ContainerDefaultMappings.findCollectionFallback(superType); - if (collectionClass != null) { - return true; - } - } - return false; - } - protected CollectionType _mapAbstractCollectionType(JavaType type, DeserializationConfig config) { final Class collectionClass = ContainerDefaultMappings.findCollectionFallback(type); From bf7ec4e0c7303d97b33861e55d8fc7f32b083ac9 Mon Sep 17 00:00:00 2001 From: Tatu Saloranta Date: Sat, 9 Nov 2024 14:32:00 -0800 Subject: [PATCH 09/15] Add test for error handling too --- ...JsonMergeWithCustomCollection4783Test.java | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/src/test/java/com/fasterxml/jackson/databind/tofix/JsonMergeWithCustomCollection4783Test.java b/src/test/java/com/fasterxml/jackson/databind/tofix/JsonMergeWithCustomCollection4783Test.java index ceefe18445..a8e4f94eda 100644 --- a/src/test/java/com/fasterxml/jackson/databind/tofix/JsonMergeWithCustomCollection4783Test.java +++ b/src/test/java/com/fasterxml/jackson/databind/tofix/JsonMergeWithCustomCollection4783Test.java @@ -8,10 +8,12 @@ import com.fasterxml.jackson.annotation.JsonMerge; import com.fasterxml.jackson.annotation.JsonProperty; import com.fasterxml.jackson.databind.ObjectMapper; +import com.fasterxml.jackson.databind.exc.InvalidDefinitionException; import com.fasterxml.jackson.databind.testutil.DatabindTestUtil; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.junit.jupiter.api.Assertions.fail; // [databind#4783] Test to verify that JsonMerge also works for custom list @SuppressWarnings("serial") @@ -54,6 +56,11 @@ static class MergeMyCustomPojoList { } } + // And then non-merging case too + static class NonMergeCustomStringList { + public MyListCustom values; + } + public static class CustomPojo { public String name; public int age; @@ -105,4 +112,16 @@ void testCustomMapperReadingPojoArrayList() throws Exception { assertEquals(3, result.values.size()); } + // Failure-handling testing important too + @Test + void testNonMergeAbstractListFail() throws Exception { + try { + MAPPER.readValue("{\"values\":[\"x\"]}", NonMergeCustomStringList.class); + fail("Should not pass"); + } catch (InvalidDefinitionException e) { + verifyException(e, String.format( + "Cannot construct instance of `%s` (no Creators", + MyListCustom.class.getName())); + } + } } From 3c848db9f3f29425692e10d3e77fbd0827dd980d Mon Sep 17 00:00:00 2001 From: Tatu Saloranta Date: Sat, 9 Nov 2024 14:33:46 -0800 Subject: [PATCH 10/15] renaming --- .../merge/CustomCollectionMerge783Test.java} | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) rename src/test/java/com/fasterxml/jackson/databind/{tofix/JsonMergeWithCustomCollection4783Test.java => deser/merge/CustomCollectionMerge783Test.java} (97%) diff --git a/src/test/java/com/fasterxml/jackson/databind/tofix/JsonMergeWithCustomCollection4783Test.java b/src/test/java/com/fasterxml/jackson/databind/deser/merge/CustomCollectionMerge783Test.java similarity index 97% rename from src/test/java/com/fasterxml/jackson/databind/tofix/JsonMergeWithCustomCollection4783Test.java rename to src/test/java/com/fasterxml/jackson/databind/deser/merge/CustomCollectionMerge783Test.java index a8e4f94eda..6715702db4 100644 --- a/src/test/java/com/fasterxml/jackson/databind/tofix/JsonMergeWithCustomCollection4783Test.java +++ b/src/test/java/com/fasterxml/jackson/databind/deser/merge/CustomCollectionMerge783Test.java @@ -1,4 +1,4 @@ -package com.fasterxml.jackson.databind.tofix; +package com.fasterxml.jackson.databind.deser.merge; import java.util.ArrayList; import java.util.List; @@ -17,7 +17,7 @@ // [databind#4783] Test to verify that JsonMerge also works for custom list @SuppressWarnings("serial") -public class JsonMergeWithCustomCollection4783Test extends DatabindTestUtil +public class CustomCollectionMerge783Test extends DatabindTestUtil { static class MyArrayListJDK extends ArrayList { } From 02748f41bc2f5ca532a22828ef9ce56f33442947 Mon Sep 17 00:00:00 2001 From: Tatu Saloranta Date: Sat, 9 Nov 2024 14:50:27 -0800 Subject: [PATCH 11/15] Change abstract Map handling similar to abstract Collection (defer fail) --- .../jackson/databind/deser/BasicDeserializerFactory.java | 6 ------ 1 file changed, 6 deletions(-) diff --git a/src/main/java/com/fasterxml/jackson/databind/deser/BasicDeserializerFactory.java b/src/main/java/com/fasterxml/jackson/databind/deser/BasicDeserializerFactory.java index 5750c36514..70bf2d49c5 100644 --- a/src/main/java/com/fasterxml/jackson/databind/deser/BasicDeserializerFactory.java +++ b/src/main/java/com/fasterxml/jackson/databind/deser/BasicDeserializerFactory.java @@ -972,12 +972,6 @@ public JsonDeserializer createMapDeserializer(DeserializationContext ctxt, mapClass = type.getRawClass(); // But if so, also need to re-check creators... beanDesc = config.introspectForCreation(type); - } else { - // [databind#292]: Actually, may be fine, but only if polymorphic deser enabled - if (type.getTypeHandler() == null) { - throw new IllegalArgumentException("Cannot find a deserializer for non-concrete Map type "+type); - } - deser = AbstractDeserializer.constructForNonPOJO(beanDesc); } } else { // 10-Jan-2017, tatu: `java.util.Collections` types need help: From 1f1f84c08a2e8e7778f7e3d7410736f6763fb447 Mon Sep 17 00:00:00 2001 From: joohyukkim Date: Sun, 10 Nov 2024 12:15:45 +0900 Subject: [PATCH 12/15] Add release note --- release-notes/VERSION-2.x | 3 +++ 1 file changed, 3 insertions(+) diff --git a/release-notes/VERSION-2.x b/release-notes/VERSION-2.x index cb3726c68d..c2f761d321 100644 --- a/release-notes/VERSION-2.x +++ b/release-notes/VERSION-2.x @@ -13,6 +13,9 @@ Project: jackson-databind #4788: `EnumFeature.WRITE_ENUMS_TO_LOWERCASE` overrides `@JsonProperty` values (reported by Mike M) (fix by Joo-Hyuk K) + #4783 Possibly wrong behavior of @JsonMerge + (reported by @nlisker) + (fix by Joo-Hyuk K) 2.18.1 (28-Oct-2024) From b0860b3e9efcd55308220ecaf899241150b10260 Mon Sep 17 00:00:00 2001 From: joohyukkim Date: Sun, 10 Nov 2024 12:17:02 +0900 Subject: [PATCH 13/15] Update release note --- release-notes/VERSION-2.x | 3 +++ 1 file changed, 3 insertions(+) diff --git a/release-notes/VERSION-2.x b/release-notes/VERSION-2.x index 8099ad4279..5ad842e46e 100644 --- a/release-notes/VERSION-2.x +++ b/release-notes/VERSION-2.x @@ -16,6 +16,9 @@ Project: jackson-databind #4790: Fix `@JsonAnySetter` issue with "setter" method (related to #4639) (reported by @bsa01) (fix by Joo-Hyuk K) + #4783 Possibly wrong behavior of @JsonMerge + (reported by @nlisker) + (fix by Joo-Hyuk K) 2.18.1 (28-Oct-2024) From 85101758e830afd37a823caaed5764fe1d86e09d Mon Sep 17 00:00:00 2001 From: joohyukkim Date: Sun, 10 Nov 2024 12:18:48 +0900 Subject: [PATCH 14/15] Fix test name typo --- ...onMerge783Test.java => CustomCollectionMerge4783Test.java} | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) rename src/test/java/com/fasterxml/jackson/databind/deser/merge/{CustomCollectionMerge783Test.java => CustomCollectionMerge4783Test.java} (98%) diff --git a/src/test/java/com/fasterxml/jackson/databind/deser/merge/CustomCollectionMerge783Test.java b/src/test/java/com/fasterxml/jackson/databind/deser/merge/CustomCollectionMerge4783Test.java similarity index 98% rename from src/test/java/com/fasterxml/jackson/databind/deser/merge/CustomCollectionMerge783Test.java rename to src/test/java/com/fasterxml/jackson/databind/deser/merge/CustomCollectionMerge4783Test.java index 6715702db4..6fe4a5c290 100644 --- a/src/test/java/com/fasterxml/jackson/databind/deser/merge/CustomCollectionMerge783Test.java +++ b/src/test/java/com/fasterxml/jackson/databind/deser/merge/CustomCollectionMerge4783Test.java @@ -17,7 +17,8 @@ // [databind#4783] Test to verify that JsonMerge also works for custom list @SuppressWarnings("serial") -public class CustomCollectionMerge783Test extends DatabindTestUtil +public class CustomCollectionMerge4783Test + extends DatabindTestUtil { static class MyArrayListJDK extends ArrayList { } @@ -124,4 +125,5 @@ void testNonMergeAbstractListFail() throws Exception { MyListCustom.class.getName())); } } + } From 7f985f0d5aee5cd60fcaa55ac1b85072671e0f53 Mon Sep 17 00:00:00 2001 From: Tatu Saloranta Date: Mon, 11 Nov 2024 19:44:50 -0800 Subject: [PATCH 15/15] Remove one unnecessary change (wrt Map defaulting), add a test case --- .../deser/BasicDeserializerFactory.java | 12 ++++++--- .../merge/CustomCollectionMerge4783Test.java | 27 ++++++++++++++++--- 2 files changed, 32 insertions(+), 7 deletions(-) diff --git a/src/main/java/com/fasterxml/jackson/databind/deser/BasicDeserializerFactory.java b/src/main/java/com/fasterxml/jackson/databind/deser/BasicDeserializerFactory.java index 70bf2d49c5..93fae8e49d 100644 --- a/src/main/java/com/fasterxml/jackson/databind/deser/BasicDeserializerFactory.java +++ b/src/main/java/com/fasterxml/jackson/databind/deser/BasicDeserializerFactory.java @@ -966,12 +966,18 @@ public JsonDeserializer createMapDeserializer(DeserializationContext ctxt, */ if (deser == null) { if (type.isInterface() || type.isAbstract()) { - MapType fallback = _mapAbstractMapType(type, config); - if (fallback != null) { - type = (MapType) fallback; + MapType implType = _mapAbstractMapType(type, config); + if (implType != null) { + type = (MapType) implType; mapClass = type.getRawClass(); // But if so, also need to re-check creators... beanDesc = config.introspectForCreation(type); + } else { + // [databind#292]: Actually, may be fine, but only if polymorphic deser enabled + if (type.getTypeHandler() == null) { + throw new IllegalArgumentException("Cannot find a deserializer for non-concrete Map type "+type); + } + deser = AbstractDeserializer.constructForNonPOJO(beanDesc); } } else { // 10-Jan-2017, tatu: `java.util.Collections` types need help: diff --git a/src/test/java/com/fasterxml/jackson/databind/deser/merge/CustomCollectionMerge4783Test.java b/src/test/java/com/fasterxml/jackson/databind/deser/merge/CustomCollectionMerge4783Test.java index 6fe4a5c290..2d3f84ba7f 100644 --- a/src/test/java/com/fasterxml/jackson/databind/deser/merge/CustomCollectionMerge4783Test.java +++ b/src/test/java/com/fasterxml/jackson/databind/deser/merge/CustomCollectionMerge4783Test.java @@ -18,7 +18,7 @@ // [databind#4783] Test to verify that JsonMerge also works for custom list @SuppressWarnings("serial") public class CustomCollectionMerge4783Test - extends DatabindTestUtil + extends DatabindTestUtil { static class MyArrayListJDK extends ArrayList { } @@ -33,6 +33,11 @@ interface MyListCustom extends List { } static class MyArrayListCustom extends ArrayList implements MyListCustom { } + static abstract class MyAbstractStringList extends ArrayList { + MyAbstractStringList() { super(); } + MyAbstractStringList(int i) { super(); } + } + static class MergeCustomStringList { @JsonMerge @JsonProperty @@ -61,7 +66,7 @@ static class MergeMyCustomPojoList { static class NonMergeCustomStringList { public MyListCustom values; } - + public static class CustomPojo { public String name; public int age; @@ -113,9 +118,11 @@ void testCustomMapperReadingPojoArrayList() throws Exception { assertEquals(3, result.values.size()); } - // Failure-handling testing important too + // // // And then failure cases + + // Fail can't construct Collection interface unless there's maaping @Test - void testNonMergeAbstractListFail() throws Exception { + void failNonMergeInterfaceList() throws Exception { try { MAPPER.readValue("{\"values\":[\"x\"]}", NonMergeCustomStringList.class); fail("Should not pass"); @@ -126,4 +133,16 @@ void testNonMergeAbstractListFail() throws Exception { } } + // Fail can't construct abstract types + @Test + void failNonMergeAbstractList() throws Exception { + try { + MAPPER.readValue("[]", MyAbstractStringList.class); + fail("Should not pass"); + } catch (InvalidDefinitionException e) { + verifyException(e, String.format( + "Cannot construct instance of `%s` (no Creators", + MyAbstractStringList.class.getName())); + } + } }