MapSerializer._orderEntries should check for null keys.

[huxi]

    protected Map<?,?> _orderEntries(Map<?,?> input)
    {
        // minor optimization: may already be sorted?
        if (input instanceof SortedMap<?,?>) {
            return input;
        }
        return new TreeMap<Object,Object>(input);
    }

should be changed to

    protected Map<?,?> _orderEntries(Map<?,?> input)
    {
        // minor optimization: may already be sorted?
        if (input instanceof SortedMap<?,?>) {
            return input;
        }
        // prevent NPE in TreeMap
        if (input.containsKey(null)) {
            return input;
        }
        return new TreeMap<Object,Object>(input);
    }

TreeMap will otherwise throw a NullPointerException.

This would mean that a Map is potentially serialized unsorted as a fallback even if ORDER_MAP_ENTRIES_BY_KEYS is active but I guess that's still better than exploding with an exception.

[huxi]

This is about jackson-databind 2.8.3.
Sorry, forgot to mention the version.

[cowtowncoder]

Interesting... thank you for reporting this, will do.

[huxi]

@cowtowncoder
a1d0251 is not a good fix.

First of all, you aren't returning the result map... but I just saw you fixed that in 50855c3 so forget this point.

Secondly, the value stored for null is lost. This is really bad. Data integrity should be way more important than the sorting of the keys.

I've given this some more thoughts and I now think that the right way of fixing this issue would look like this:

    protected Map<?,?> _orderEntries(Map<?,?> input)
    {
        // minor optimization: may already be sorted?
        if (input instanceof SortedMap<?,?>) {
            return input;
        }
        try {
            return new TreeMap<Object,Object>(input);
        } catch (Throwable ignored) {
            return input
        }
    }

This would also cover the case were keys aren't Comparable (see TreeMap.compare(Object k1, Object k2) called by put(K key, V value) which would throw a ClassCastException in that case).

Catching Throwable instead of explicitly catching ClassCastException and NullPointerException would have the advantage that it would also eat OutOfMemoryException (in case of huge maps that won't fit into memory twice) and any unexpected exceptions caused by buggy implementations of the Comparable interface.

I'm usually not a fan of using exceptions for logic but think this is an exception to the rule since it would impose zero performance overhead in the "good" case while still returning correct, albeit unsorted, values in the "bad" case.

Sorry I didn't catch the problem before the 2.8.4 release. :(

[cowtowncoder]

I disagree in sending unordered entries, however, so I don't want to do that. I am bit ambivalent on best way to proceed here; perhaps throwing exception is better then. I can do that, but give proper explanation.

The thing is this: JSON does not allow null as a key, so I don't really care too much about this case being supported well. Throwing an exception no matter what might be the best way; sorting or not.

[huxi]

Uh, sorry. I wasn't aware that null is an invalid key in JSON. That changes the problem quite a bit.

An ObjectMapper without SerializationFeature.ORDER_MAP_ENTRIES_BY_KEYS enabled would currently throw a com.fasterxml.jackson.core.JsonGenerationException: Null key for a Map not allowed in JSON (use a converting NullKeySerializer?) by default, but it looks like this can be changed...

Enabling or disabling ORDER_MAP_ENTRIES_BY_KEYS should not change anything other than the order, so the remainder of the serialization logic should still behave the same, i.e. use a NullKeySerializer if available and any registered keySerializer for complex types.

We are talking about new behavior here:
A null key has previously generated a NullPointerException and keys that did not implement Comparable caused a ClassCastException with ORDER_MAP_ENTRIES_BY_KEYS enabled. So people that previously stumbled over this problem likely already disabled this option in their code. We aren't dealing with something that suddenly produces different, unsorted output but with something that would suddenly produce unsorted output instead of causing an exception. So this most likely isn't causing compatibility issues, at least. We are talking about currently undefined (AFAIK) behavior that you could fix by defining it.

I'd suggest that you document over at ORDER_MAP_ENTRIES_BY_KEYS that this is only a best-effort option, i.e. that it will only have an effect if there is no null key and if the keys implement Comparable. That would be fair game and just catching any exception during construction of the TreeMap, returning to unsorted Map, would provide the effect that all the other Map-serialization-logic configured by the user would still have the chance to kick in.

My rationale for this is that ORDER_MAP_ENTRIES_BY_KEYS is (AFAIK) a "global" setting of ObjectMapper so it's not possible (again, AFAIK) to say "Please sort this map in my object but not that map". So defining ORDER_MAP_ENTRIES_BY_KEYS as a best-effort option would still behave reasonable in most cases and put the ball in the ballpark of the developer using the library in the edge-cases we are talking about right now.

I'm just trying to be helpful here, throwing my brain and 2 cents into your brainstorming pool, but you are way deeper involved in the topic than I am so I'm not sure about the actual value of my rambling. This isn't meant as a "I am right and you are wrong!" argument at all.

I also feel sorry about the whole issue since I was the one who originally suggested ORDER_MAP_ENTRIES_BY_KEYS to you... it was only meant to make generated JSON more easily consumable by humans. If you had told me to just "use a SortedMap in your data if you want that" back then, then you wouldn't have this problem now... So I kind of feel responsible for your current misery/ambivalence.

[cowtowncoder]

Actually there is per-property override also using

@JsonFormat(with = JsonFormat.Feature.WRITE_SORTED_MAP_ENTRIES)

I think I do have an idea of what to do with null entry... I'll have to double-check to see how null entries are handled by default. Since _orderEntries is called in place where START_OBJECT has been written, it may be possible to actually write matching entry first, then sort all but null-keyed one, and everyone might be happy enough.

I do agree in that quietly dropping an entry is not desirable.