Implement new SerializationFeature.IGNORE_FAILURE_TO_ORDER_MAP_ENTRIES_BY_INCOMPARABLE_KEYS

[JooHyukKim]

resolves #4773

[cowtowncoder]

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.

[cowtowncoder]

Ok, so: I generally prefer avoiding exceptions in cases like this, if at all possible: relying on catching specific exception, and worse, with specific message, is brittle.

And in this case, we should have key type available via keyType. So we should have an idea whether type implements Comparable. Except for java.lang.Object... and I guess, come to think of that, it is not guaranteed that we get full type, it being possible to have Maps where declared key type is not Comparable, but actual Map contains all Comparable keys.

So, I'll propose different approach here: how about we base decision on the first key value iterated with map.keySet()? So when we are to sort a non-empty Map, we'll check the first key and if it does not implement Comparable (something like if (!Comparable.isInstance(key))) we'll either:

1. Throw proper JsonMappingException (probably, InvalidDefinitionException via DeserializationContext.reportBadDefinition(keyType, ...) if failing enabled
2. Skip sorting otherwise

Empty Map case can be skipped as no sorting needed. (theoretically we could skip Map-of-1 case too, but that might just be confusing when one expects failure?)

I think this would work pretty well -- not guaranteed to work for all cases always if keys are of mixed types. But... we can resolve that problem if and when we get there (that is, if it's an actual and not theoretical problem).

WDYT?

[JooHyukKim]

I really like your suggestion!
Your Current approach feels more brittle, now that someone else tells me 😅.

What you described was actually my first version of implementation, but decided to let go and take current approach because of the case you mentioned --keys with mixed type.
Also, what you suggest around solving problem when we face one approach sounds reasonable.
Will try to come up with new version.

Good review as always, thanks! @cowtowncoder

[JooHyukKim]

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 ! 🙏🏼

[cowtowncoder]

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.

[JooHyukKim]

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.

[cowtowncoder]

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.

[JooHyukKim]

Applied review, made changes to JavaDoc and ready for review again.
No rush tho @cowtowncoder, thanks!

[cowtowncoder]

Looks good, approved!

[cowtowncoder]

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

[JooHyukKim]

Ughh how did I miss that 😂 thank you!

[JooHyukKim]

Looks good, approved!

Alright, now I will move on to do some consequent tasks to be made, like...

• updating J-STEP 2 wiki, and
• Modifying master branch (within a day)

Will keep these updates on the original issue #4773, not on this PR.

[cowtowncoder]

@JooHyukKim I merged this PR to master, but did not change defaults. So if you want to do that (along with Javadoc change, that'd be good).