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

Implement new SerializationFeature.IGNORE_FAILURE_TO_ORDER_MAP_ENTRIES_BY_INCOMPARABLE_KEYS #4778

Merged
merged 8 commits into from
Nov 4, 2024
Original file line number Diff line number Diff line change
Expand Up @@ -434,6 +434,21 @@ 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.
* <p>
* 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}.
* <p>
* Feature is disabled by default and will default true in Jackson 3 and later.
JooHyukKim marked this conversation as resolved.
Show resolved Hide resolved
*
* @since 2.19
*/
IGNORE_FAILURE_TO_ORDER_MAP_ENTRIES_BY_INCOMPARABLE_KEYS(false),
JooHyukKim marked this conversation as resolved.
Show resolved Hide resolved

/*
/******************************************************
/* Other
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -1158,6 +1158,28 @@ protected final JsonSerializer<Object> _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) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately the try-catching only just got bigger :-(
Without try-catching like we do here or finding some other way, this feature will only be un-deterministic.

If we look at below test testSerializationSuccessWhenDisabledWithMixedTypes() case, you can see this feature is so easy to violate what the name FAIL_ON_ORDER_MAP_BY_INCOMPARABLE_KEY itself imposes.

I am also thinking without figuring out how to handle the case with incomparable key in the middle, we might be better off not push through this feature 🤔

Lemme know how you see @cowtowncoder ! 🙏🏼

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My initial reaction:

  1. I don't want any try-catch block; and I am ok with mixed-type case failing: it would be unsupported case.
  2. The idea is to improve handling, and to me uniform key types is what can be supported for Java.
  3. I don't have problem for having edge cases that are no better, as long as presumed common case is improved -- which I think it is.

So I don't think we really must solve all cases; especially when trying to do that leads to awkward and brittle reliance on JDK-thrown exceptions, which are basically side-effects of type casts.

But let me have a look at PR see if that changes my mind.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But let me have a look at PR see if that changes my mind.

You don't need to.
You are right on saying "what can be supported" or not. Mixed-type case would be a painful case to handle from maintenance point of view, and also actual usage wouldn't be much reasonable either.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Plus, if we change our mind, we can always add catch block -- or, even if we choose to, scan the whole keySet() to check type -- as a follow-up improvement.

// [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.
JooHyukKim marked this conversation as resolved.
Show resolved Hide resolved
if (cce.getMessage() != null
&& cce.getMessage().contains("cannot be cast to class java.lang.Comparable")
JooHyukKim marked this conversation as resolved.
Show resolved Hide resolved
&& provider.isEnabled(SerializationFeature.IGNORE_FAILURE_TO_ORDER_MAP_ENTRIES_BY_INCOMPARABLE_KEYS)
) {
return input;
}
throw cce;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually think that if we went this route and recognized failure, we should convert that into JsonMappingException (one of sub-types ideally) -- and not pass ClassCastException.

However: I have bigger concerns about catch-approach, will add different note.

}
}

protected Map<?,?> _orderEntries(Map<?,?> input, JsonGenerator gen,
SerializerProvider provider)
throws IOException
Expand Down
Original file line number Diff line number Diff line change
@@ -1,18 +1,17 @@
package com.fasterxml.jackson.databind.tofix;
package com.fasterxml.jackson.databind.deser;

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missed this one: this is serialization, not deserialization test. Will move.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ughh how did I miss that 😂 thank you!

import org.junit.jupiter.api.Test;

import java.util.Currency;
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.
Expand All @@ -32,7 +31,6 @@ public static class ObjectContainer4773 {
.configure(SerializationFeature.ORDER_MAP_ENTRIES_BY_KEYS, true)
.build();

@JacksonTestFailureExpected
@Test
void testSerializationWithIncomparableKeys()
throws Exception
Expand All @@ -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_INCOMPARABLE_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
Expand All @@ -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_INCOMPARABLE_KEYS)
.writeValueAsString(entity);

// Then
assertTrue(jsonResult.contains("\"1\":\"AUD_TEXT\""));
assertTrue(jsonResult.contains("\"2\":\"KRW_TEXT\""));
assertTrue(jsonResult.contains("\"GBP\":\"GBP_TEXT\""));
}

}
Loading