Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SerializationFeature.ORDER_MAP_ENTRIES_BY_KEYS should not apply to Maps with uncomparable keys #4773

Closed
1 task done
nathanukey opened this issue Oct 31, 2024 · 6 comments · Fixed by #4778
Closed
1 task done
Labels
2.19 Issues planned at 2.19 or later has-failing-test Indicates that there exists a test case (under `failing/`) to reproduce the issue
Milestone

Comments

@nathanukey
Copy link

Search before asking

  • I searched in the issues and found nothing similar.

Describe the bug

We have a global ObjectMapper configured in springboot used to convert objects to JSON for REST return values as well as sending objects over JMS and other places.

To have somewhat deterministic ordering of the JSON elements, we have turned on

objectMapper.configure(SerializationFeature.ORDER_MAP_ENTRIES_BY_KEYS, true);

This has been working fine for years, but now one of the other teams have implemented a map of exchange rates
Map<java.util.Currency, internal.dto.ExchangeRate> exchangeRates

When the system tries to convert this to JSON, it causes an exception:

Caused by: java.lang.ClassCastException: class java.util.Currency cannot be cast to class java.lang.Comparable (java.util.Currency and java.lang.Comparable are in module java.base of loader 'bootstrap')
	at java.base/java.util.TreeMap.compare(TreeMap.java:1569)
	at java.base/java.util.TreeMap.addEntryToEmptyMap(TreeMap.java:776)
	at java.base/java.util.TreeMap.put(TreeMap.java:785)
	at java.base/java.util.TreeMap.put(TreeMap.java:534)
	at java.base/java.util.AbstractMap.putAll(AbstractMap.java:281)
	at java.base/java.util.TreeMap.putAll(TreeMap.java:326)
	at java.base/java.util.TreeMap.<init>(TreeMap.java:187)
	at com.fasterxml.jackson.databind.ser.std.MapSerializer._orderEntries(MapSerializer.java:1182)
	at com.fasterxml.jackson.databind.ser.std.MapSerializer.serializeWithoutTypeInfo(MapSerializer.java:754)
	at com.fasterxml.jackson.databind.ser.std.MapSerializer.serialize(MapSerializer.java:720)
	at com.fasterxml.jackson.databind.ser.std.MapSerializer.serialize(MapSerializer.java:35)
	at com.fasterxml.jackson.databind.ser.BeanPropertyWriter.serializeAsField(BeanPropertyWriter.java:733)
	at com.fasterxml.jackson.databind.ser.std.BeanSerializerBase.serializeFields(BeanSerializerBase.java:774)
	... 32 common frames omitted

The solution seems to be to disable map sorting feature, but then we lose the huge value this has given everywhere else with deterministic ordering.

In this situation, would be preferable if the fallback was just not to sort a map that can't be sorted. Perhaps warrants a different feature flag, ORDER_COMPARABLE_MAP_ENTRIES_BY_KEYS?

Some way to specify global sorting comparators would be nice, but since there is always possibility a new Map would be created using keys that weren't sorted, defaulting to converting/sending message (even if not sorted) would still be preference.

That secondary ability to sort seems to already be raised here: #2162
But still feel this is separate issue/bug, ability to keep sorting feature without failing on unsortables.

Version Information

2.14

Reproduction

    private static class Entity {
        private Map<Currency, String> exampleMap = new HashMap<>();

        public Map<Currency, String> getExampleMap() {
            return exampleMap;
        }

        public void setExampleMap(Map<Currency, String> exampleMap) {
            this.exampleMap = exampleMap;
        }
    }

    @Test
    void testDeserializationCurrencyMap3() throws IOException {
        final Entity entity = new Entity();
        final Map<Currency, String> exampleMap = entity.getExampleMap();
        exampleMap.put(Currency.getInstance("GBP"), "GBP_TEXT");
        exampleMap.put(Currency.getInstance("AUD"), "AUD_TEXT");

        final ObjectMapper objectMapper = new ObjectMapper();
        objectMapper.configure(SerializationFeature.ORDER_MAP_ENTRIES_BY_KEYS, true);

        objectMapper.writeValueAsString(entity);
    }

Expected behavior

No response

Additional context

No response

@nathanukey nathanukey added the to-evaluate Issue that has been received but not yet evaluated label Oct 31, 2024
@cowtowncoder
Copy link
Member

Yes, this is an unfortunate issue and it does feel like not-sorting non-Comparable Maps might be the way to go. This is not necessarily fool-proof, still, because static information may be incomplete wrt actual type -- specifically, I think sorting needs to be left enabled for Map<Object, X> (or more likely, non-typed Map values) since chance is intent is to enable sorting.

But until there's a fix (which probably needs to go in 2.19 for being bit risky change, potentially), there's one thing you may be able to use disable sorting for such one thing:

@JsonFormat.Feature.WRITE_SORTED_MAP_ENTRIES

see f.ex test "src/test/java/com/fasterxml/jackson/databind/struct/FormatFeatureOrderedMapTest.java"

    static class SortedKeysMap {
        // to enable
        @JsonFormat(with = JsonFormat.Feature.WRITE_SORTED_MAP_ENTRIES)
        public Map<String,Integer> values;

        // to disable
        @JsonFormat(without = JsonFormat.Feature.WRITE_SORTED_MAP_ENTRIES)
        public Map<String,Integer> values = new LinkedHashMap<>();

it might also be possible to apply it via ConfigOverrides (@JsonFormat itself is) but since there is no test it perhaps isn't implemented (not 100% sure where annotation there'd be... probably for type of enclosing class).

Anyway, on per-field basis this override could help.

@JooHyukKim
Copy link
Member

Add a reproducible failing test via #4776. Thank you for reproduction @nathanukey!

This issue feels like general case where we make the behavior... (of simply serializing un-ordered)

  • configurable SerializationFeature in Jackson 2 , but
  • default in Jackson 3 (Optionally configurable)

WDYT, @cowtowncoder ?

cowtowncoder pushed a commit that referenced this issue Nov 3, 2024
@cowtowncoder cowtowncoder added 2.18 and removed to-evaluate Issue that has been received but not yet evaluated labels Nov 3, 2024
@cowtowncoder
Copy link
Member

@JooHyukKim Agreed: sounds like a plan -- with a feature, with defaults like you suggested.

@cowtowncoder cowtowncoder added has-failing-test Indicates that there exists a test case (under `failing/`) to reproduce the issue 2.19 Issues planned at 2.19 or later and removed 2.18 labels Nov 3, 2024
@cowtowncoder cowtowncoder added this to the 2.19.0 milestone Nov 4, 2024
@JooHyukKim
Copy link
Member

JooHyukKim commented Nov 5, 2024

Implemented in 2.19 version via #4778 , we just released 2.18.1, so not so sure about expected date.
Thank you anyways @nathanukey!

Further Actions to Take

  1. ✅ Update master accordingly Resolve #4773 Change FAIL_ON_ORDER_MAP_BY_INCOMPARABLE_KEY default and JavaDoc #4781
  2. JSTEP update (new addition)

@JooHyukKim
Copy link
Member

Done!
Added default change history in JSTEP 2

image

@cowtowncoder
Copy link
Member

@JooHyukKim Thank you for updating docs too -- I don't always remember to do that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.19 Issues planned at 2.19 or later has-failing-test Indicates that there exists a test case (under `failing/`) to reproduce the issue
Projects
None yet
3 participants