From 420ae68fecf4cd1759f9557413a4217fc45fd741 Mon Sep 17 00:00:00 2001 From: joohyukkim Date: Sun, 3 Nov 2024 21:48:55 +0900 Subject: [PATCH 1/8] feature : Add implementation for SerializationFeature.IGNORE_FAILURE_TO_ORDER_MAP_ENTRIES_BY_KEYS --- .../databind/SerializationFeature.java | 11 +++++ .../databind/ser/std/MapSerializer.java | 24 +++++++++- ...iesByKeysSerializationFeature4773Test.java | 44 +++++++++++++++---- 3 files changed, 69 insertions(+), 10 deletions(-) rename src/test/java/com/fasterxml/jackson/databind/{tofix => deser}/OrderMapEntriesByKeysSerializationFeature4773Test.java (59%) diff --git a/src/main/java/com/fasterxml/jackson/databind/SerializationFeature.java b/src/main/java/com/fasterxml/jackson/databind/SerializationFeature.java index 98ca736c78..c4cd87d465 100644 --- a/src/main/java/com/fasterxml/jackson/databind/SerializationFeature.java +++ b/src/main/java/com/fasterxml/jackson/databind/SerializationFeature.java @@ -434,6 +434,17 @@ public enum SerializationFeature implements ConfigFeature */ ORDER_MAP_ENTRIES_BY_KEYS(false), + /** + * Feature that determines whether to try to ignore failure to order map entries by keys. + * This feature will apply only when serialization is to order its serialize output, configured + * through annotation or enabling {@link #ORDER_MAP_ENTRIES_BY_KEYS}. + *

+ * Feature is disabled by default and will default true in Jackson 3 and later. + * + * @since 2.19 + */ + IGNORE_FAILURE_TO_ORDER_MAP_ENTRIES_BY_KEYS(false), + /* /****************************************************** /* Other diff --git a/src/main/java/com/fasterxml/jackson/databind/ser/std/MapSerializer.java b/src/main/java/com/fasterxml/jackson/databind/ser/std/MapSerializer.java index e5147cb829..a87128ae65 100644 --- a/src/main/java/com/fasterxml/jackson/databind/ser/std/MapSerializer.java +++ b/src/main/java/com/fasterxml/jackson/databind/ser/std/MapSerializer.java @@ -750,7 +750,7 @@ public void serializeWithType(Map value, JsonGenerator gen, SerializerProvi public void serializeWithoutTypeInfo(Map value, JsonGenerator gen, SerializerProvider provider) throws IOException { if (!value.isEmpty()) { if (_sortKeys || provider.isEnabled(SerializationFeature.ORDER_MAP_ENTRIES_BY_KEYS)) { - value = _orderEntries(value, gen, provider); + value = _tryOrderingEntries(value, gen, provider); } PropertyFilter pf; if ((_filterId != null) && (pf = findPropertyFilter(provider, _filterId, value)) != null) { @@ -1158,6 +1158,28 @@ protected final JsonSerializer _findAndAddDynamic(PropertySerializerMap return result.serializer; } + // Since 2.19 + protected Map _tryOrderingEntries(Map input, JsonGenerator gen, + SerializerProvider provider) + throws IOException + { + try { + return _orderEntries(input, gen, provider); + } catch (ClassCastException cce) { + // [databind#4773] Since 2.19, `SerializationFeature.ORDER_MAP_ENTRIES_BY_KEYS` should not + // apply to Maps with incomparable keys. + // Due to dynamic nature of Map, we cannot be able to check key type in advance, so + // we may catch ClassCastException and handle it gracefully. + if (cce.getMessage() != null + && cce.getMessage().contains("cannot be cast to class java.lang.Comparable") + && provider.isEnabled(SerializationFeature.IGNORE_FAILURE_TO_ORDER_MAP_ENTRIES_BY_KEYS) + ) { + return input; + } + throw cce; + } + } + protected Map _orderEntries(Map input, JsonGenerator gen, SerializerProvider provider) throws IOException diff --git a/src/test/java/com/fasterxml/jackson/databind/tofix/OrderMapEntriesByKeysSerializationFeature4773Test.java b/src/test/java/com/fasterxml/jackson/databind/deser/OrderMapEntriesByKeysSerializationFeature4773Test.java similarity index 59% rename from src/test/java/com/fasterxml/jackson/databind/tofix/OrderMapEntriesByKeysSerializationFeature4773Test.java rename to src/test/java/com/fasterxml/jackson/databind/deser/OrderMapEntriesByKeysSerializationFeature4773Test.java index fc114f0b6e..19501bfd97 100644 --- a/src/test/java/com/fasterxml/jackson/databind/tofix/OrderMapEntriesByKeysSerializationFeature4773Test.java +++ b/src/test/java/com/fasterxml/jackson/databind/deser/OrderMapEntriesByKeysSerializationFeature4773Test.java @@ -1,4 +1,4 @@ -package com.fasterxml.jackson.databind.tofix; +package com.fasterxml.jackson.databind.deser; import org.junit.jupiter.api.Test; @@ -6,13 +6,12 @@ import java.util.HashMap; import java.util.Map; +import com.fasterxml.jackson.databind.JsonMappingException; import com.fasterxml.jackson.databind.ObjectMapper; import com.fasterxml.jackson.databind.SerializationFeature; import com.fasterxml.jackson.databind.testutil.DatabindTestUtil; -import com.fasterxml.jackson.databind.testutil.failure.JacksonTestFailureExpected; -import static org.junit.jupiter.api.Assertions.assertEquals; -import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.junit.jupiter.api.Assertions.*; // [databind#4773] Test to verify `SerializationFeature.ORDER_MAP_ENTRIES_BY_KEYS` behavior // when serializing `Map` instances with un-comparable keys. @@ -32,7 +31,6 @@ public static class ObjectContainer4773 { .configure(SerializationFeature.ORDER_MAP_ENTRIES_BY_KEYS, true) .build(); - @JacksonTestFailureExpected @Test void testSerializationWithIncomparableKeys() throws Exception @@ -44,11 +42,18 @@ void testSerializationWithIncomparableKeys() // When : Throws exception // com.fasterxml.jackson.databind.JsonMappingException: class java.util.Currency cannot be cast to class java.lang.Comparable - String jsonResult = objectMapper.writeValueAsString(entity); + try { + objectMapper.writer() + .without(SerializationFeature.IGNORE_FAILURE_TO_ORDER_MAP_ENTRIES_BY_KEYS) + .writeValueAsString(entity); + fail("Should not pass"); + } catch (JsonMappingException e) { - // Then : Order should not matter, just plain old serialize - assertTrue(jsonResult.contains("GBP")); - assertTrue(jsonResult.contains("AUD")); + // Then + assertInstanceOf(ClassCastException.class, e.getCause()); + assertTrue(e.getMessage() + .contains("class java.util.Currency cannot be cast to class java.lang.Comparable")); + } } @Test @@ -75,4 +80,25 @@ void testSerializationWithGenericObjectKeys() "'5':'N_TEXT'}}"), jsonResult); } + @Test + void testSerializationWithBothComparableAndIncomparableKeys() + throws Exception + { + // Given : Mixed keys with incomparable `Currency` and comparable `Integer` + ObjectContainer4773 entity = new ObjectContainer4773(); + entity.exampleMap.put(1, "AUD_TEXT"); + entity.exampleMap.put(Currency.getInstance("GBP"), "GBP_TEXT"); + entity.exampleMap.put(2, "KRW_TEXT"); + + // When + String jsonResult = objectMapper.writer() + .with(SerializationFeature.IGNORE_FAILURE_TO_ORDER_MAP_ENTRIES_BY_KEYS) + .writeValueAsString(entity); + + // Then + assertTrue(jsonResult.contains("\"1\":\"AUD_TEXT\"")); + assertTrue(jsonResult.contains("\"2\":\"KRW_TEXT\"")); + assertTrue(jsonResult.contains("\"GBP\":\"GBP_TEXT\"")); + } + } From 1faca12b63debe4215fb30c8bf779ff9c0a1a662 Mon Sep 17 00:00:00 2001 From: joohyukkim Date: Sun, 3 Nov 2024 22:07:56 +0900 Subject: [PATCH 2/8] chore : Clean up docs and naming etc... --- .../jackson/databind/SerializationFeature.java | 10 +++++++--- .../jackson/databind/ser/std/MapSerializer.java | 2 +- ...erMapEntriesByKeysSerializationFeature4773Test.java | 4 ++-- 3 files changed, 10 insertions(+), 6 deletions(-) diff --git a/src/main/java/com/fasterxml/jackson/databind/SerializationFeature.java b/src/main/java/com/fasterxml/jackson/databind/SerializationFeature.java index c4cd87d465..c2065e30f0 100644 --- a/src/main/java/com/fasterxml/jackson/databind/SerializationFeature.java +++ b/src/main/java/com/fasterxml/jackson/databind/SerializationFeature.java @@ -435,15 +435,19 @@ public enum SerializationFeature implements ConfigFeature ORDER_MAP_ENTRIES_BY_KEYS(false), /** - * Feature that determines whether to try to ignore failure to order map entries by keys. - * This feature will apply only when serialization is to order its serialize output, configured + * Feature that determines whether to try to ignore failure to order map entries by incomparable keys. + * If enabled, will not throw an exception when ordering fails due to keys with incomparable key type + * and instead just return the original map. + * If disabled, will simply fail by throwing an exception. + *

+ * Note that this feature will apply only when configured to order map entries by keys, either * through annotation or enabling {@link #ORDER_MAP_ENTRIES_BY_KEYS}. *

* Feature is disabled by default and will default true in Jackson 3 and later. * * @since 2.19 */ - IGNORE_FAILURE_TO_ORDER_MAP_ENTRIES_BY_KEYS(false), + IGNORE_FAILURE_TO_ORDER_MAP_ENTRIES_BY_INCOMPARABLE_KEYS(false), /* /****************************************************** diff --git a/src/main/java/com/fasterxml/jackson/databind/ser/std/MapSerializer.java b/src/main/java/com/fasterxml/jackson/databind/ser/std/MapSerializer.java index a87128ae65..945ec622b6 100644 --- a/src/main/java/com/fasterxml/jackson/databind/ser/std/MapSerializer.java +++ b/src/main/java/com/fasterxml/jackson/databind/ser/std/MapSerializer.java @@ -1172,7 +1172,7 @@ protected final JsonSerializer _findAndAddDynamic(PropertySerializerMap // we may catch ClassCastException and handle it gracefully. if (cce.getMessage() != null && cce.getMessage().contains("cannot be cast to class java.lang.Comparable") - && provider.isEnabled(SerializationFeature.IGNORE_FAILURE_TO_ORDER_MAP_ENTRIES_BY_KEYS) + && provider.isEnabled(SerializationFeature.IGNORE_FAILURE_TO_ORDER_MAP_ENTRIES_BY_INCOMPARABLE_KEYS) ) { return input; } diff --git a/src/test/java/com/fasterxml/jackson/databind/deser/OrderMapEntriesByKeysSerializationFeature4773Test.java b/src/test/java/com/fasterxml/jackson/databind/deser/OrderMapEntriesByKeysSerializationFeature4773Test.java index 19501bfd97..0103faa2f0 100644 --- a/src/test/java/com/fasterxml/jackson/databind/deser/OrderMapEntriesByKeysSerializationFeature4773Test.java +++ b/src/test/java/com/fasterxml/jackson/databind/deser/OrderMapEntriesByKeysSerializationFeature4773Test.java @@ -44,7 +44,7 @@ void testSerializationWithIncomparableKeys() // com.fasterxml.jackson.databind.JsonMappingException: class java.util.Currency cannot be cast to class java.lang.Comparable try { objectMapper.writer() - .without(SerializationFeature.IGNORE_FAILURE_TO_ORDER_MAP_ENTRIES_BY_KEYS) + .without(SerializationFeature.IGNORE_FAILURE_TO_ORDER_MAP_ENTRIES_BY_INCOMPARABLE_KEYS) .writeValueAsString(entity); fail("Should not pass"); } catch (JsonMappingException e) { @@ -92,7 +92,7 @@ void testSerializationWithBothComparableAndIncomparableKeys() // When String jsonResult = objectMapper.writer() - .with(SerializationFeature.IGNORE_FAILURE_TO_ORDER_MAP_ENTRIES_BY_KEYS) + .with(SerializationFeature.IGNORE_FAILURE_TO_ORDER_MAP_ENTRIES_BY_INCOMPARABLE_KEYS) .writeValueAsString(entity); // Then From 5f3c1d13ea6f81a08c513a313000f310afb5af67 Mon Sep 17 00:00:00 2001 From: joohyukkim Date: Mon, 4 Nov 2024 22:20:29 +0900 Subject: [PATCH 3/8] Change feature name to `FAIL_ON_ORDER_MAP_BY_INCOMPARABLE_KEY` --- .../com/fasterxml/jackson/databind/SerializationFeature.java | 2 +- .../com/fasterxml/jackson/databind/ser/std/MapSerializer.java | 2 +- .../OrderMapEntriesByKeysSerializationFeature4773Test.java | 4 ++-- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/main/java/com/fasterxml/jackson/databind/SerializationFeature.java b/src/main/java/com/fasterxml/jackson/databind/SerializationFeature.java index c2065e30f0..79be052a2e 100644 --- a/src/main/java/com/fasterxml/jackson/databind/SerializationFeature.java +++ b/src/main/java/com/fasterxml/jackson/databind/SerializationFeature.java @@ -447,7 +447,7 @@ public enum SerializationFeature implements ConfigFeature * * @since 2.19 */ - IGNORE_FAILURE_TO_ORDER_MAP_ENTRIES_BY_INCOMPARABLE_KEYS(false), + FAIL_ON_ORDER_MAP_BY_INCOMPARABLE_KEY(true), /* /****************************************************** diff --git a/src/main/java/com/fasterxml/jackson/databind/ser/std/MapSerializer.java b/src/main/java/com/fasterxml/jackson/databind/ser/std/MapSerializer.java index 945ec622b6..d572287943 100644 --- a/src/main/java/com/fasterxml/jackson/databind/ser/std/MapSerializer.java +++ b/src/main/java/com/fasterxml/jackson/databind/ser/std/MapSerializer.java @@ -1172,7 +1172,7 @@ protected final JsonSerializer _findAndAddDynamic(PropertySerializerMap // we may catch ClassCastException and handle it gracefully. if (cce.getMessage() != null && cce.getMessage().contains("cannot be cast to class java.lang.Comparable") - && provider.isEnabled(SerializationFeature.IGNORE_FAILURE_TO_ORDER_MAP_ENTRIES_BY_INCOMPARABLE_KEYS) + && !provider.isEnabled(SerializationFeature.FAIL_ON_ORDER_MAP_BY_INCOMPARABLE_KEY) ) { return input; } diff --git a/src/test/java/com/fasterxml/jackson/databind/deser/OrderMapEntriesByKeysSerializationFeature4773Test.java b/src/test/java/com/fasterxml/jackson/databind/deser/OrderMapEntriesByKeysSerializationFeature4773Test.java index 0103faa2f0..36ec958b9a 100644 --- a/src/test/java/com/fasterxml/jackson/databind/deser/OrderMapEntriesByKeysSerializationFeature4773Test.java +++ b/src/test/java/com/fasterxml/jackson/databind/deser/OrderMapEntriesByKeysSerializationFeature4773Test.java @@ -44,7 +44,7 @@ void testSerializationWithIncomparableKeys() // com.fasterxml.jackson.databind.JsonMappingException: class java.util.Currency cannot be cast to class java.lang.Comparable try { objectMapper.writer() - .without(SerializationFeature.IGNORE_FAILURE_TO_ORDER_MAP_ENTRIES_BY_INCOMPARABLE_KEYS) + .without(SerializationFeature.FAIL_ON_ORDER_MAP_BY_INCOMPARABLE_KEY) .writeValueAsString(entity); fail("Should not pass"); } catch (JsonMappingException e) { @@ -92,7 +92,7 @@ void testSerializationWithBothComparableAndIncomparableKeys() // When String jsonResult = objectMapper.writer() - .with(SerializationFeature.IGNORE_FAILURE_TO_ORDER_MAP_ENTRIES_BY_INCOMPARABLE_KEYS) + .with(SerializationFeature.FAIL_ON_ORDER_MAP_BY_INCOMPARABLE_KEY) .writeValueAsString(entity); // Then From 7b8aa7117b0656025d5a4fb7a3cd144383b40792 Mon Sep 17 00:00:00 2001 From: joohyukkim Date: Mon, 4 Nov 2024 23:37:14 +0900 Subject: [PATCH 4/8] Implement eager comparable type check as suggested and also catch error --- .../databind/ser/std/MapSerializer.java | 81 +++++++++++-------- ...iesByKeysSerializationFeature4773Test.java | 18 ++--- 2 files changed, 56 insertions(+), 43 deletions(-) diff --git a/src/main/java/com/fasterxml/jackson/databind/ser/std/MapSerializer.java b/src/main/java/com/fasterxml/jackson/databind/ser/std/MapSerializer.java index d572287943..236ac5c738 100644 --- a/src/main/java/com/fasterxml/jackson/databind/ser/std/MapSerializer.java +++ b/src/main/java/com/fasterxml/jackson/databind/ser/std/MapSerializer.java @@ -11,6 +11,7 @@ import com.fasterxml.jackson.core.type.WritableTypeId; import com.fasterxml.jackson.databind.*; import com.fasterxml.jackson.databind.annotation.JacksonStdImpl; +import com.fasterxml.jackson.databind.exc.InvalidDefinitionException; import com.fasterxml.jackson.databind.introspect.AnnotatedMember; import com.fasterxml.jackson.databind.jsonFormatVisitors.JsonFormatVisitorWrapper; import com.fasterxml.jackson.databind.jsonFormatVisitors.JsonMapFormatVisitor; @@ -750,7 +751,7 @@ public void serializeWithType(Map value, JsonGenerator gen, SerializerProvi public void serializeWithoutTypeInfo(Map value, JsonGenerator gen, SerializerProvider provider) throws IOException { if (!value.isEmpty()) { if (_sortKeys || provider.isEnabled(SerializationFeature.ORDER_MAP_ENTRIES_BY_KEYS)) { - value = _tryOrderingEntries(value, gen, provider); + value = _orderEntries(value, gen, provider); } PropertyFilter pf; if ((_filterId != null) && (pf = findPropertyFilter(provider, _filterId, value)) != null) { @@ -1158,53 +1159,65 @@ protected final JsonSerializer _findAndAddDynamic(PropertySerializerMap return result.serializer; } - // Since 2.19 - protected Map _tryOrderingEntries(Map input, JsonGenerator gen, + protected Map _orderEntries(Map input, JsonGenerator gen, SerializerProvider provider) throws IOException { + // minor optimization: may already be sorted? + if (input instanceof SortedMap) { + return input; + } + // or is it empty? then no need to sort either + if (input == null || input.isEmpty()) { + return input; + } + // [databind#4773] Since 2.19: We should not try sorting Maps with uncomparable keys + // And first key is a good enough sample for now. + Object firstKey = input.keySet().iterator().next(); + if (firstKey != null && !Comparable.class.isInstance(firstKey)) { + // We cannot sort incomparable keys, should we fail or just skip sorting? + if (!provider.isEnabled(SerializationFeature.FAIL_ON_ORDER_MAP_BY_INCOMPARABLE_KEY)) { + return input; + } else { + provider.reportBadDefinition(firstKey.getClass(), + String.format("Cannot order Map entries by key of incomparable type %s, consider disabling " + + "SerializationFeature.FAIL_ON_ORDER_MAP_BY_INCOMPARABLE_KEY to simply skip sorting", + ClassUtil.classNameOf(firstKey))); + } + } try { - return _orderEntries(input, gen, provider); + // [databind#1411]: TreeMap does not like null key... (although note that + // check above should prevent this code from being called in that case) + // [databind#153]: but, apparently, some custom Maps do manage hit this + // problem. + if (_hasNullKey(input)) { + TreeMap result = new TreeMap(); + for (Map.Entry entry : input.entrySet()) { + Object key = entry.getKey(); + if (key == null) { + _writeNullKeyedEntry(gen, provider, entry.getValue()); + continue; + } + result.put(key, entry.getValue()); + } + return result; + } + return new TreeMap(input); } catch (ClassCastException cce) { // [databind#4773] Since 2.19, `SerializationFeature.ORDER_MAP_ENTRIES_BY_KEYS` should not // apply to Maps with incomparable keys. - // Due to dynamic nature of Map, we cannot be able to check key type in advance, so - // we may catch ClassCastException and handle it gracefully. if (cce.getMessage() != null && cce.getMessage().contains("cannot be cast to class java.lang.Comparable") && !provider.isEnabled(SerializationFeature.FAIL_ON_ORDER_MAP_BY_INCOMPARABLE_KEY) ) { return input; } - throw cce; - } - } - - protected Map _orderEntries(Map input, JsonGenerator gen, - SerializerProvider provider) - throws IOException - { - // minor optimization: may already be sorted? - if (input instanceof SortedMap) { - return input; - } - // [databind#1411]: TreeMap does not like null key... (although note that - // check above should prevent this code from being called in that case) - // [databind#153]: but, apparently, some custom Maps do manage hit this - // problem. - if (_hasNullKey(input)) { - TreeMap result = new TreeMap(); - for (Map.Entry entry : input.entrySet()) { - Object key = entry.getKey(); - if (key == null) { - _writeNullKeyedEntry(gen, provider, entry.getValue()); - continue; - } - result.put(key, entry.getValue()); - } - return result; + return provider.reportBadDefinition(firstKey.getClass(), + String.format("Cannot order Map entries by key of incomparable type %s, consider disabling " + + "SerializationFeature.FAIL_ON_ORDER_MAP_BY_INCOMPARABLE_KEY to simply skip sorting. " + + "Underlying exception message (%s): %s", + ClassUtil.classNameOf(firstKey), cce.getClass().getName(), cce.getMessage())); } - return new TreeMap(input); } /** diff --git a/src/test/java/com/fasterxml/jackson/databind/deser/OrderMapEntriesByKeysSerializationFeature4773Test.java b/src/test/java/com/fasterxml/jackson/databind/deser/OrderMapEntriesByKeysSerializationFeature4773Test.java index 36ec958b9a..48b53cfd23 100644 --- a/src/test/java/com/fasterxml/jackson/databind/deser/OrderMapEntriesByKeysSerializationFeature4773Test.java +++ b/src/test/java/com/fasterxml/jackson/databind/deser/OrderMapEntriesByKeysSerializationFeature4773Test.java @@ -9,6 +9,7 @@ import com.fasterxml.jackson.databind.JsonMappingException; import com.fasterxml.jackson.databind.ObjectMapper; import com.fasterxml.jackson.databind.SerializationFeature; +import com.fasterxml.jackson.databind.exc.InvalidDefinitionException; import com.fasterxml.jackson.databind.testutil.DatabindTestUtil; import static org.junit.jupiter.api.Assertions.*; @@ -32,7 +33,7 @@ public static class ObjectContainer4773 { .build(); @Test - void testSerializationWithIncomparableKeys() + void testSerializationFailureWhenEnabledWithIncomparableKeys() throws Exception { // Given @@ -44,15 +45,14 @@ void testSerializationWithIncomparableKeys() // com.fasterxml.jackson.databind.JsonMappingException: class java.util.Currency cannot be cast to class java.lang.Comparable try { objectMapper.writer() - .without(SerializationFeature.FAIL_ON_ORDER_MAP_BY_INCOMPARABLE_KEY) + .with(SerializationFeature.FAIL_ON_ORDER_MAP_BY_INCOMPARABLE_KEY) .writeValueAsString(entity); fail("Should not pass"); - } catch (JsonMappingException e) { - + } catch (InvalidDefinitionException e) { // Then - assertInstanceOf(ClassCastException.class, e.getCause()); - assertTrue(e.getMessage() - .contains("class java.util.Currency cannot be cast to class java.lang.Comparable")); + verifyException(e, + "Cannot order Map entries by key of incomparable type", + "consider disabling SerializationFeature.FAIL_ON_ORDER_MAP_BY_INCOMPARABLE_KEY to simply skip sorting"); } } @@ -81,7 +81,7 @@ void testSerializationWithGenericObjectKeys() } @Test - void testSerializationWithBothComparableAndIncomparableKeys() + void testSerializationSuccessWhenDisabledWithMixedTypes() throws Exception { // Given : Mixed keys with incomparable `Currency` and comparable `Integer` @@ -92,7 +92,7 @@ void testSerializationWithBothComparableAndIncomparableKeys() // When String jsonResult = objectMapper.writer() - .with(SerializationFeature.FAIL_ON_ORDER_MAP_BY_INCOMPARABLE_KEY) + .without(SerializationFeature.FAIL_ON_ORDER_MAP_BY_INCOMPARABLE_KEY) .writeValueAsString(entity); // Then From a5017e6d8a08274e44926f03843813383a73c23b Mon Sep 17 00:00:00 2001 From: joohyukkim Date: Mon, 4 Nov 2024 23:49:43 +0900 Subject: [PATCH 5/8] Fix error failure --- .../fasterxml/jackson/databind/ser/std/MapSerializer.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/main/java/com/fasterxml/jackson/databind/ser/std/MapSerializer.java b/src/main/java/com/fasterxml/jackson/databind/ser/std/MapSerializer.java index 236ac5c738..4311f45934 100644 --- a/src/main/java/com/fasterxml/jackson/databind/ser/std/MapSerializer.java +++ b/src/main/java/com/fasterxml/jackson/databind/ser/std/MapSerializer.java @@ -1206,9 +1206,9 @@ protected Map _orderEntries(Map input, JsonGenerator gen, } catch (ClassCastException cce) { // [databind#4773] Since 2.19, `SerializationFeature.ORDER_MAP_ENTRIES_BY_KEYS` should not // apply to Maps with incomparable keys. - if (cce.getMessage() != null - && cce.getMessage().contains("cannot be cast to class java.lang.Comparable") - && !provider.isEnabled(SerializationFeature.FAIL_ON_ORDER_MAP_BY_INCOMPARABLE_KEY) + if (!provider.isEnabled(SerializationFeature.FAIL_ON_ORDER_MAP_BY_INCOMPARABLE_KEY) + && cce.getMessage() != null + && cce.getMessage().contains("java.lang.Comparable") ) { return input; } From 2d1e5f483e0b20e65c19b5f59058ad58e1ad39f4 Mon Sep 17 00:00:00 2001 From: joohyukkim Date: Tue, 5 Nov 2024 07:23:03 +0900 Subject: [PATCH 6/8] Apply review --- .../databind/SerializationFeature.java | 2 +- .../databind/ser/std/MapSerializer.java | 55 +++++++------------ ...iesByKeysSerializationFeature4773Test.java | 28 ++++------ .../ser/jdk/MapSerializationTest.java | 3 +- 4 files changed, 35 insertions(+), 53 deletions(-) diff --git a/src/main/java/com/fasterxml/jackson/databind/SerializationFeature.java b/src/main/java/com/fasterxml/jackson/databind/SerializationFeature.java index 79be052a2e..d3fe841fba 100644 --- a/src/main/java/com/fasterxml/jackson/databind/SerializationFeature.java +++ b/src/main/java/com/fasterxml/jackson/databind/SerializationFeature.java @@ -443,7 +443,7 @@ public enum SerializationFeature implements ConfigFeature * Note that this feature will apply only when configured to order map entries by keys, either * through annotation or enabling {@link #ORDER_MAP_ENTRIES_BY_KEYS}. *

- * Feature is disabled by default and will default true in Jackson 3 and later. + * Feature is enabled by default and will default false in Jackson 3 and later. * * @since 2.19 */ diff --git a/src/main/java/com/fasterxml/jackson/databind/ser/std/MapSerializer.java b/src/main/java/com/fasterxml/jackson/databind/ser/std/MapSerializer.java index 4311f45934..c5c5019924 100644 --- a/src/main/java/com/fasterxml/jackson/databind/ser/std/MapSerializer.java +++ b/src/main/java/com/fasterxml/jackson/databind/ser/std/MapSerializer.java @@ -11,7 +11,6 @@ import com.fasterxml.jackson.core.type.WritableTypeId; import com.fasterxml.jackson.databind.*; import com.fasterxml.jackson.databind.annotation.JacksonStdImpl; -import com.fasterxml.jackson.databind.exc.InvalidDefinitionException; import com.fasterxml.jackson.databind.introspect.AnnotatedMember; import com.fasterxml.jackson.databind.jsonFormatVisitors.JsonFormatVisitorWrapper; import com.fasterxml.jackson.databind.jsonFormatVisitors.JsonMapFormatVisitor; @@ -1168,56 +1167,42 @@ protected Map _orderEntries(Map input, JsonGenerator gen, return input; } // or is it empty? then no need to sort either - if (input == null || input.isEmpty()) { + if (input.isEmpty()) { return input; } // [databind#4773] Since 2.19: We should not try sorting Maps with uncomparable keys // And first key is a good enough sample for now. Object firstKey = input.keySet().iterator().next(); - if (firstKey != null && !Comparable.class.isInstance(firstKey)) { + if (!Comparable.class.isInstance(firstKey)) { // We cannot sort incomparable keys, should we fail or just skip sorting? if (!provider.isEnabled(SerializationFeature.FAIL_ON_ORDER_MAP_BY_INCOMPARABLE_KEY)) { return input; } else { - provider.reportBadDefinition(firstKey.getClass(), + Class clazz = firstKey == null ? Object.class : firstKey.getClass(); + provider.reportBadDefinition(clazz, String.format("Cannot order Map entries by key of incomparable type %s, consider disabling " + - "SerializationFeature.FAIL_ON_ORDER_MAP_BY_INCOMPARABLE_KEY to simply skip sorting", + "`SerializationFeature.FAIL_ON_ORDER_MAP_BY_INCOMPARABLE_KEY` to simply skip sorting", ClassUtil.classNameOf(firstKey))); } } - try { - // [databind#1411]: TreeMap does not like null key... (although note that - // check above should prevent this code from being called in that case) - // [databind#153]: but, apparently, some custom Maps do manage hit this - // problem. - if (_hasNullKey(input)) { - TreeMap result = new TreeMap(); - for (Map.Entry entry : input.entrySet()) { - Object key = entry.getKey(); - if (key == null) { - _writeNullKeyedEntry(gen, provider, entry.getValue()); - continue; - } - result.put(key, entry.getValue()); + + // [databind#1411]: TreeMap does not like null key... (although note that + // check above should prevent this code from being called in that case) + // [databind#153]: but, apparently, some custom Maps do manage hit this + // problem. + if (_hasNullKey(input)) { + TreeMap result = new TreeMap(); + for (Map.Entry entry : input.entrySet()) { + Object key = entry.getKey(); + if (key == null) { + _writeNullKeyedEntry(gen, provider, entry.getValue()); + continue; } - return result; - } - return new TreeMap(input); - } catch (ClassCastException cce) { - // [databind#4773] Since 2.19, `SerializationFeature.ORDER_MAP_ENTRIES_BY_KEYS` should not - // apply to Maps with incomparable keys. - if (!provider.isEnabled(SerializationFeature.FAIL_ON_ORDER_MAP_BY_INCOMPARABLE_KEY) - && cce.getMessage() != null - && cce.getMessage().contains("java.lang.Comparable") - ) { - return input; + result.put(key, entry.getValue()); } - return provider.reportBadDefinition(firstKey.getClass(), - String.format("Cannot order Map entries by key of incomparable type %s, consider disabling " + - "SerializationFeature.FAIL_ON_ORDER_MAP_BY_INCOMPARABLE_KEY to simply skip sorting. " + - "Underlying exception message (%s): %s", - ClassUtil.classNameOf(firstKey), cce.getClass().getName(), cce.getMessage())); + return result; } + return new TreeMap(input); } /** diff --git a/src/test/java/com/fasterxml/jackson/databind/deser/OrderMapEntriesByKeysSerializationFeature4773Test.java b/src/test/java/com/fasterxml/jackson/databind/deser/OrderMapEntriesByKeysSerializationFeature4773Test.java index 48b53cfd23..9976a4e6a3 100644 --- a/src/test/java/com/fasterxml/jackson/databind/deser/OrderMapEntriesByKeysSerializationFeature4773Test.java +++ b/src/test/java/com/fasterxml/jackson/databind/deser/OrderMapEntriesByKeysSerializationFeature4773Test.java @@ -6,7 +6,6 @@ import java.util.HashMap; import java.util.Map; -import com.fasterxml.jackson.databind.JsonMappingException; import com.fasterxml.jackson.databind.ObjectMapper; import com.fasterxml.jackson.databind.SerializationFeature; import com.fasterxml.jackson.databind.exc.InvalidDefinitionException; @@ -50,9 +49,7 @@ void testSerializationFailureWhenEnabledWithIncomparableKeys() fail("Should not pass"); } catch (InvalidDefinitionException e) { // Then - verifyException(e, - "Cannot order Map entries by key of incomparable type", - "consider disabling SerializationFeature.FAIL_ON_ORDER_MAP_BY_INCOMPARABLE_KEY to simply skip sorting"); + verifyException(e, "Cannot order Map entries by key of incomparable type"); } } @@ -81,24 +78,23 @@ void testSerializationWithGenericObjectKeys() } @Test - void testSerializationSuccessWhenDisabledWithMixedTypes() + void testSerWithNullType() throws Exception { // Given : Mixed keys with incomparable `Currency` and comparable `Integer` ObjectContainer4773 entity = new ObjectContainer4773(); - entity.exampleMap.put(1, "AUD_TEXT"); - entity.exampleMap.put(Currency.getInstance("GBP"), "GBP_TEXT"); - entity.exampleMap.put(2, "KRW_TEXT"); + entity.exampleMap.put(null, "AUD_TEXT"); - // When - String jsonResult = objectMapper.writer() - .without(SerializationFeature.FAIL_ON_ORDER_MAP_BY_INCOMPARABLE_KEY) + // When : Throws exception + try { + objectMapper.writer() + .with(SerializationFeature.FAIL_ON_ORDER_MAP_BY_INCOMPARABLE_KEY) .writeValueAsString(entity); - - // Then - assertTrue(jsonResult.contains("\"1\":\"AUD_TEXT\"")); - assertTrue(jsonResult.contains("\"2\":\"KRW_TEXT\"")); - assertTrue(jsonResult.contains("\"GBP\":\"GBP_TEXT\"")); + fail("Should not pass"); + } catch (InvalidDefinitionException e) { + // Then + verifyException(e, "Cannot order Map entries by key of incomparable type [null]"); + } } } diff --git a/src/test/java/com/fasterxml/jackson/databind/ser/jdk/MapSerializationTest.java b/src/test/java/com/fasterxml/jackson/databind/ser/jdk/MapSerializationTest.java index d52198eb9f..6ceef81128 100644 --- a/src/test/java/com/fasterxml/jackson/databind/ser/jdk/MapSerializationTest.java +++ b/src/test/java/com/fasterxml/jackson/databind/ser/jdk/MapSerializationTest.java @@ -164,7 +164,8 @@ public void testOrderByKey() throws IOException @Test public void testOrderByWithNulls() throws IOException { - ObjectWriter sortingW = MAPPER.writer(SerializationFeature.ORDER_MAP_ENTRIES_BY_KEYS); + ObjectWriter sortingW = MAPPER.writer(SerializationFeature.ORDER_MAP_ENTRIES_BY_KEYS) + .without(SerializationFeature.FAIL_ON_ORDER_MAP_BY_INCOMPARABLE_KEY); // 16-Oct-2016, tatu: but mind the null key, if any Map mapWithNullKey = new LinkedHashMap(); mapWithNullKey.put(null, 1); From 7b95e2d30330baee9faf7180f85dda86c9bff145 Mon Sep 17 00:00:00 2001 From: joohyukkim Date: Tue, 5 Nov 2024 07:30:42 +0900 Subject: [PATCH 7/8] Doc modify java doc some more --- .../jackson/databind/SerializationFeature.java | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/main/java/com/fasterxml/jackson/databind/SerializationFeature.java b/src/main/java/com/fasterxml/jackson/databind/SerializationFeature.java index d3fe841fba..a16e5cbeff 100644 --- a/src/main/java/com/fasterxml/jackson/databind/SerializationFeature.java +++ b/src/main/java/com/fasterxml/jackson/databind/SerializationFeature.java @@ -435,10 +435,12 @@ public enum SerializationFeature implements ConfigFeature ORDER_MAP_ENTRIES_BY_KEYS(false), /** - * Feature that determines whether to try to ignore failure to order map entries by incomparable keys. - * If enabled, will not throw an exception when ordering fails due to keys with incomparable key type - * and instead just return the original map. - * If disabled, will simply fail by throwing an exception. + * Feature that determines whether to intentionally fail when the mapper attempts to + * order map entries with incomparable keys by accessing the first key of the map. + * So depending on the Map implementation, this may not be the same key every time. + *

+ * If enabled, will simply fail by throwing an exception. + * If disabled, will not throw an exception and instead simply return the original map. *

* Note that this feature will apply only when configured to order map entries by keys, either * through annotation or enabling {@link #ORDER_MAP_ENTRIES_BY_KEYS}. From 6ff6bab741f9fd5a298b910f02def3d9c556e60c Mon Sep 17 00:00:00 2001 From: Tatu Saloranta Date: Mon, 4 Nov 2024 15:10:49 -0800 Subject: [PATCH 8/8] Update release notes --- release-notes/VERSION-2.x | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/release-notes/VERSION-2.x b/release-notes/VERSION-2.x index d42e8723f6..a59fb2cc6a 100644 --- a/release-notes/VERSION-2.x +++ b/release-notes/VERSION-2.x @@ -20,6 +20,10 @@ Project: jackson-databind #4676: Support other enum naming strategies than camelCase (requested by @hajdamak) (contributed by Lars B) +#4773: `SerializationFeature.ORDER_MAP_ENTRIES_BY_KEYS` should not apply to Maps + with uncomparable keys + (requested by @nathanukey) + (fix by Joo-Hyuk K) 2.18.1 (28-Oct-2024)