[Guava] Allow `Cache` serialization #112

JooHyukKim · 2023-04-29T06:52:06Z

relates to #90. "Miminal" implementation of Cache serialization.

guava/src/main/java/com/fasterxml/jackson/datatype/guava/ser/CacheSerializer.java

cowtowncoder · 2023-04-29T15:53:41Z

guava/src/main/java/com/fasterxml/jackson/datatype/guava/ser/CacheSerializer.java

+                continue;
+            }
+            gen.writeFieldName(key.toString());
+            provider.defaultSerializeValue(value, gen);


This won't support polymorphism and is not as efficient as if serializer was fetched eagerly (like regular Maps will do, if possible; or if not possible, retain (possibly generic) element JavaType) -- but for a minimal implementation it's a clear improvement so that's fine.

guava/src/main/java/com/fasterxml/jackson/datatype/guava/ser/CacheSerializer.java

cowtowncoder · 2023-04-29T16:05:02Z

Looks good so far. You may want to look at standard MapSerializer to see how to maybe try to add full typed handling (or even partial). That requires constructing instances with resolved JavaType to know actual declared types of Key and Value types, instead of assuming Object. Doing so is required at least for polymorphic types (wrt values), but also to support some key types like java.util.Date where Object.toString() is not what regular Map serializer would use.

I think what I would want to see is at least extracting and passing JavaType.

Now that wrote above, though, I realize that this is not as trivial as thought because Cache is not Map and as such does not automatically extract generic type information.

But! Have a look at GuavaTypeModifier and see how it "upgrade" Multimap into "map-like" type: I think same should be done for Cache. And see how Multimap's generic type information is then available and used in GuavaSerializers method findMapLikeSerializer().
This is the good way to get type information.

I am ok with not necessarily implementing everything this allows, but at least get type information so handling may be improved.

LMK if you need help here, type refinements are pretty advanced functionality.

JooHyukKim · 2023-05-01T04:57:21Z

Looks good so far. You may....

Thank you for your detailed feedback on the PR 🙏🏼 I will work on it one by one and get back soon as I can~

JooHyukKim · 2023-05-01T13:21:39Z

guava/src/main/java/com/fasterxml/jackson/datatype/guava/GuavaSerializers.java

+            Set<String> ignored = (ignorals == null) ? null : ignorals.getIgnored();
+            return new CacheSerializer(type, beanDesc,
+                keySerializer, elementTypeSerializer, elementValueSerializer, ignored, filterId);
+        }


Identical to that of MultimapSerializer right above.

…uch as possible.

JooHyukKim · 2023-05-01T14:21:55Z

guava/src/test/java/com/fasterxml/jackson/datatype/guava/CacheSerializationTest.java

+import java.util.ArrayList;
+import java.util.List;
+
+public class CacheSerializationTest extends ModuleTestBase {


I have collected and converted most of the tests related to MultiMap for Cache. After implementing the deserialization part, we can further expand the range of tests. For now, we have covered the basic functionality, including:

Polymorphic type handling

JsonInclude behavior

Key ordering

Please let me know if there are any additional test cases that should be included! 🙏🏼🙏🏼

JooHyukKim · 2023-05-01T14:29:14Z

guava/src/main/java/com/fasterxml/jackson/datatype/guava/ser/CacheSerializer.java

+ */
+public class CacheSerializer 
+    extends ContainerSerializer<Cache<?, ?>>
+    implements ContextualSerializer


Since we get Map<?, ?> as the snapshot value of current Cache. I thought it would make sense to use the majority of "battle-hardend" implementations from MultimapSerializer class.

I will write notes on which part here in CacheSerializer is different from MultimapSerializer.

That sounds like a good idea to me.

JooHyukKim · 2023-05-01T14:30:19Z

guava/src/main/java/com/fasterxml/jackson/datatype/guava/ser/CacheSerializer.java

+     */

    @Override
+    public JsonSerializer<?> createContextual(SerializerProvider provider, BeanProperty property) throws JsonMappingException {


This whole method is identical to

jackson-datatypes-collections/guava/src/main/java/com/fasterxml/jackson/datatype/guava/ser/MultimapSerializer.java

Line 144 in ed6416d

public JsonSerializer<?> createContextual(SerializerProvider provider,

JooHyukKim · 2023-05-01T14:37:40Z

guava/src/main/java/com/fasterxml/jackson/datatype/guava/ser/CacheSerializer.java

+            // Second, serialize value
+            Object vv = entry.getValue();
+            // is this even possible?
+            if (vv == null) {
+                provider.defaultSerializeNull(gen);
+                continue;
+            }
+            // serialize value
+            JsonSerializer<Object> valueSer = _valueSerializer;
+            if (valueSer == null) {
+                Class<?> cc = vv.getClass();
+                valueSer = serializers.serializerFor(cc);
+                if (valueSer == null) {
+                    valueSer = _findAndAddDynamic(serializers, cc, provider);
+                    serializers = _dynamicValueSerializers;
+                }
+            }
+            if (_valueTypeSerializer == null) {
+                valueSer.serialize(vv, gen, provider);
+            } else {
+                valueSer.serializeWithType(vv, gen, provider, _valueTypeSerializer);
+            }


There is no for-loop here unlike Multimap where value is a List ...

jackson-datatypes-collections/guava/src/main/java/com/fasterxml/jackson/datatype/guava/ser/MultimapSerializer.java

Line 328 in ed6416d

for (Object vv : entry.getValue()) {

JooHyukKim · 2023-05-01T14:41:30Z

guava/src/main/java/com/fasterxml/jackson/datatype/guava/ser/CacheSerializer.java

+            if ((ignored != null) && ignored.contains(key)) {
+                continue;
+            }
+            Object value = entry.getValue();


This serializeFilteredFields(Map<?, ?>, JsonGenerator, SerializerProvider)method is almost identical to that of MultimapSerializer except current line --no collection, just plain object.

jackson-datatypes-collections/guava/src/main/java/com/fasterxml/jackson/datatype/guava/ser/MultimapSerializer.java

Line 364 in ed6416d

Collection<?> value = entry.getValue();

JooHyukKim · 2023-05-01T14:46:51Z

guava/src/main/java/com/fasterxml/jackson/datatype/guava/ser/CacheSerializer.java

+     */
+
+    @Override
+    public void acceptJsonFormatVisitor(JsonFormatVisitorWrapper visitor, JavaType typeHint)


This method is identical to that of MultimapSerializer

JooHyukKim · 2023-05-01T14:47:58Z

guava/src/main/java/com/fasterxml/jackson/datatype/guava/ser/CacheSerializer.java

+            _dynamicValueSerializers = result.map;
+        }
+        return result.serializer;
+    }


This method is identical to that of MultimapSerializer

JooHyukKim · 2023-05-01T14:48:01Z

guava/src/main/java/com/fasterxml/jackson/datatype/guava/ser/CacheSerializer.java

+            _dynamicValueSerializers = result.map;
+        }
+        return result.serializer;
+    }


This method is identical to that of MultimapSerializer

Also used by many databind-provided serializers.

JooHyukKim · 2023-05-01T14:59:18Z

guava/src/main/java/com/fasterxml/jackson/datatype/guava/ser/CacheSerializer.java

+            provider.reportMappingProblem("Failed to sort Multimap entries due to `NullPointerException`: `null` key?");
+            return null;
+        }
+    }


Since we are dealing with the standard JDK Map type (unlike Multimap), it seems appropriate to implement full ordering as in MapSerializer._orderEntries.

However, the fact that "Cache itself does not support null keys" makes me think that using MapSerializer might be an overkill from others' perspectives. Therefore, I figured like to hear others' opinions before proceeding 🥹

Yeah no need to support nulls (i.e. can operate without concern there).
Guava is notoriously null-antagonistic library :)

guava/src/main/java/com/fasterxml/jackson/datatype/guava/ser/CacheSerializer.java

cowtowncoder · 2023-05-01T21:39:33Z

Looks good @JooHyukKim. Is this ready to be merged from your end? (just want to make sure I won't merge too early).

cowtowncoder

LGTM

JooHyukKim · 2023-05-01T21:42:29Z

Looks good @JooHyukKim. Is this ready to be merged from your end? (just want to make sure I won't merge too early).

Yes, it is ~ 🙆🏽‍♂️🙆🏽‍♂️

JooHyukKim · 2023-05-01T22:11:24Z

@cowtowncoder Thank your details reviews as always 👍🏼👍🏼

One question tho, is it reasonable to move on to implementing CacheDeserializer? Or you might have some plan to write some set up code like you did with serializer.

cowtowncoder · 2023-05-01T23:02:16Z

@JooHyukKim I'd guess users would want a deserializer too, yes, so +1 for that. I don't think "empty" implementation would be useful here.

JooHyukKim added 5 commits April 29, 2023 14:45

Implement Cache serialization

122c4b9

ALL

daa0749

Move 90Test to existing one. And clean up Cache serialization

6ca2a34

Resolve linters

2d0afcc

Clean up CacheSer

4900c61

JooHyukKim commented Apr 29, 2023

View reviewed changes

guava/src/main/java/com/fasterxml/jackson/datatype/guava/ser/CacheSerializer.java Show resolved Hide resolved

Add doc

420efd5

JooHyukKim commented Apr 29, 2023

View reviewed changes

guava/src/main/java/com/fasterxml/jackson/datatype/guava/ser/CacheSerializer.java Outdated Show resolved Hide resolved

JooHyukKim added 3 commits April 29, 2023 18:08

Add more test cases

3ecfa09

Fix test

a7ce137

Update CacheTypesTest.java

920d0ae

cowtowncoder reviewed Apr 29, 2023

View reviewed changes

guava/src/main/java/com/fasterxml/jackson/datatype/guava/ser/CacheSerializer.java Outdated Show resolved Hide resolved

cowtowncoder reviewed Apr 29, 2023

View reviewed changes

guava/src/main/java/com/fasterxml/jackson/datatype/guava/ser/CacheSerializer.java Outdated Show resolved Hide resolved

JooHyukKim added 6 commits May 1, 2023 15:00

Partial porting of MultiMap serializer into CacheSewr

1a3bcd9

Implemented everything except ignorals

9cedd01

Implement Polymorphism and JsonInclude

98b547a

Minor clean up

0caafaa

Re arrange props and remove unused test

599788f

All

85c0f26

JooHyukKim commented May 1, 2023

View reviewed changes

JooHyukKim added 4 commits May 1, 2023 22:31

Make CacheSerializer implementation look like MultiMapSerializer as m…

d908075

…uch as possible.

Add nested Cache test

1ffb0b2

Add more tests to CacheTest

9d0593b

Rename cache test

1a27bf5

JooHyukKim commented May 1, 2023

View reviewed changes

Fix formatting

157e69a

JooHyukKim commented May 1, 2023

View reviewed changes

Update CacheSerializationTest.java

db55b36

cowtowncoder reviewed May 1, 2023

View reviewed changes

guava/src/main/java/com/fasterxml/jackson/datatype/guava/ser/CacheSerializer.java Show resolved Hide resolved

cowtowncoder approved these changes May 1, 2023

View reviewed changes

cowtowncoder merged commit a9e2a2e into FasterXML:2.16 May 1, 2023

cowtowncoder modified the milestone: 2.6. May 1, 2023

JooHyukKim mentioned this pull request May 3, 2023

Suppport simple deserialization of Cache #116

Merged

[Guava] Allow Cache serialization #112

[Guava] Allow Cache serialization #112

Uh oh!

Conversation

JooHyukKim commented Apr 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cowtowncoder Apr 29, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

cowtowncoder commented Apr 29, 2023

Uh oh!

JooHyukKim commented May 1, 2023

Uh oh!

JooHyukKim May 1, 2023

Choose a reason for hiding this comment

Uh oh!

JooHyukKim May 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JooHyukKim May 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cowtowncoder May 1, 2023

Choose a reason for hiding this comment

Uh oh!

JooHyukKim May 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JooHyukKim May 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JooHyukKim May 1, 2023

Choose a reason for hiding this comment

Uh oh!

JooHyukKim May 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JooHyukKim May 1, 2023

Choose a reason for hiding this comment

Uh oh!

JooHyukKim May 1, 2023

Choose a reason for hiding this comment

Uh oh!

cowtowncoder May 1, 2023

Choose a reason for hiding this comment

Uh oh!

JooHyukKim May 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cowtowncoder May 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cowtowncoder commented May 1, 2023

Uh oh!

cowtowncoder left a comment

Choose a reason for hiding this comment

Uh oh!

JooHyukKim commented May 1, 2023

Uh oh!

JooHyukKim commented May 1, 2023

Uh oh!

cowtowncoder commented May 1, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

[Guava] Allow `Cache` serialization #112

[Guava] Allow `Cache` serialization #112

JooHyukKim commented Apr 29, 2023 •

edited

Loading

JooHyukKim May 1, 2023 •

edited

Loading

JooHyukKim May 1, 2023 •

edited

Loading

JooHyukKim May 1, 2023 •

edited

Loading

JooHyukKim May 1, 2023 •

edited

Loading

JooHyukKim May 1, 2023 •

edited

Loading

JooHyukKim May 1, 2023 •

edited

Loading

cowtowncoder May 1, 2023 •

edited

Loading