ImmutableSetMultimap deserialization, or completing the TODOs

[michaelhixson]

There are a few TODOs in the source code of GuavaDeserializers. For example:

jackson-datatype-guava/src/main/java/com/fasterxml/jackson/datatype/guava/GuavaDeserializers.java

Line 200 in 151b01c

// TODO

The ImmutableSetMultimap one matters to me because the default deserializer does not have the same semantics for traversal order of entries. The order of the entries in the source is important, but it is lost in deserialization when it is put into a HashMultimap.

I imagine it could be a one line fix, by defaulting to LinkedHashMultimap instead. But I bet that's not what the person who wrote that TODO had in mind.

In general, for TODOs like this one, do you want pull requests?

[cowtowncoder]

Yes, in general, pull requests are good. If patch is single-file, couple of lines changed, including in issue is even simpler.

For this particular one, I'll see if I can figure out a simple way to directly support it. One question I have is with semantics: does ImmutableSetMultimap really specify traversal order? Javadocs do not say much about it.

[cowtowncoder]

Hmmh ok. So the problem is that due to immutability, a builder must be used. That is not problematic per se, but Guava does not use much of OO with builders, so generic construction is difficult: that is, sharing of implementations that use builders (which for Maps are really very similar, API-wise) is difficult if not impossible. This means that quite a bit of almost identical code needs to be duplicated.

So, this is definitely doable, but it does not seem to me like a one-liner. Patches are welcome!

[michaelhixson]

One question I have is with semantics: does ImmutableSetMultimap really specify traversal order? Javadocs do not say much about it.

They could have done a better job of putting it all in one place, but yes they do specify traversal order. The user can specify the order with comparators (one for keys, one for values) or else the multimap will preserve the order the keys and values were inserted. The latter behavior (traversal order == insertion order) is the one I'm referring to in this issue.

Here's a few snippets from Guava's latest on Github. Let me know if it's not convincing. In that case I'll probably file a bug with Guava. I know they're trying to make this clear...

From keySet():
https://github.com/google/guava/blob/master/guava/src/com/google/common/collect/ImmutableMultimap.java#L496-L499

Returns an immutable set of the distinct keys in this multimap, in the same order as they appear in this multimap.

From values():
https://github.com/google/guava/blob/master/guava/src/com/google/common/collect/ImmutableMultimap.java#L649-L653

Returns an immutable collection of the values in this multimap. Its iterator traverses the values for the first key, the values for the second key, and so on.

From entries():
https://github.com/google/guava/blob/7fa5082e6b920d243cb9d273b97de7e8dd3bb1d4/guava-gwt/src-super/com/google/common/collect/super/com/google/common/collect/ImmutableSetMultimap.java#L422-L426

Returns an immutable collection of all key-value pairs in the multimap. Its iterator traverses the values for the first key, the values for the second key, and so on.

From get(key):
https://github.com/google/guava/blob/7fa5082e6b920d243cb9d273b97de7e8dd3bb1d4/guava-gwt/src-super/com/google/common/collect/super/com/google/common/collect/ImmutableSetMultimap.java#L360-L366

Returns an immutable set of the values for the given key. If no mappings in the multimap have the provided key, an empty immutable set is returned. The values are in the same order as the parameters used to build this multimap.

---

Hmmh ok. So the problem is that due to immutability, a builder must be used.

Not necessarily. Not if you're ok with the lazy solution! :)

This module already supports some Immutable__ types without builders by building a mutable instance, then calling Immutable__.copyOf(mutableInstance). That's how ImmutableSetMultimap is supported now.

jackson-datatype-guava/src/main/java/com/fasterxml/jackson/datatype/guava/deser/multimap/GuavaMultimapDeserializer.java

Line 58 in 0b3cbb7

private static Method findTransformer(Class<?> rawType) {

The issue is right here:

jackson-datatype-guava/src/main/java/com/fasterxml/jackson/datatype/guava/GuavaDeserializers.java

Lines 199 to 216 in 151b01c

	if (ImmutableSetMultimap.class.isAssignableFrom(raw)) {
	// TODO
	}
	if (HashMultimap.class.isAssignableFrom(raw)) {
	return new HashMultimapDeserializer(type, keyDeserializer, elementTypeDeserializer,
	elementDeserializer);
	}
	if (LinkedHashMultimap.class.isAssignableFrom(raw)) {
	return new LinkedHashMultimapDeserializer(type, keyDeserializer,
	elementTypeDeserializer, elementDeserializer);
	}
	if (ForwardingSetMultimap.class.isAssignableFrom(raw)) {
	// TODO
	}
	// TODO: Remove the default fall-through once all implementations are covered.
	return new HashMultimapDeserializer(type, keyDeserializer, elementTypeDeserializer,
	elementDeserializer);

It does not have special handling for ImmutableSetMultimap, and so it defaults to using HashMultimapDeserializer. That builds the multimap in a HashMultimap and ends up calling ImmutableSetMultimap.copyOf(hashMultimap) at the end. HashMultimap does not preserve insertion order of keys or values. Sample code demonstrating the issue:

ObjectMapper mapper = new ObjectMapper().registerModule(new GuavaModule());
ImmutableSetMultimap<String, Integer> multimap = mapper.readValue(
    "{\"b\":[1,2],\"a\":[3,4]}",
    new TypeReference<ImmutableSetMultimap<String, Integer>>() {});
System.out.println(multimap);
// For me, this prints {a=[4, 3], b=[1, 2]}

The lazy solution would be to replace this:

if (ImmutableSetMultimap.class.isAssignableFrom(raw)) {
    // TODO
}

with this:

if (ImmutableSetMultimap.class.isAssignableFrom(raw)) {
    return new LinkedHashMultimapDeserializer(type, keyDeserializer,
            elementTypeDeserializer, elementDeserializer);
}

That should work because LinkedHashMultimap preserves insertion order.

http://docs.guava-libraries.googlecode.com/git/javadoc/com/google/common/collect/LinkedHashMultimap.html

Implementation of Multimap that does not allow duplicate key-value entries and that returns collections whose iterators follow the ordering in which the data was added to the multimap.

Would you be ok with that lazy solution for now?

[cowtowncoder]

Very good information, thank you.

Yes, I think the lazy solution is fine: I'd rather have things working correctly first, and only then worry about optimality. :)

It might be best if you did do a PR I think, since I think you know better than I do what exactly to test (and obviously ensure it works for your use case).

[cowwoc]

I just tried serializing a ImmutableSetMultimap using version 2.6.0-rc3 and got this output:

"theMap": 
{
    "empty": false
},

I was under the impression that this issue was meant to fix serialization as well. Should I file a separate bug report?

[cowtowncoder]

@cowwoc I think the report was specifically about deserialization, but title did not reflect that. I updated the title as the first step.

Obviously serialization should also work, did not realize it does not.
Please file a follow-up issue so we can keep release notes organized more easily.