search

FasterXML / jackson-datatypes-collections

Jackson project that contains various collection-oriented datatype libraries: Eclipse Collections, Guava, HPPC, PCollections
Apache License 2.0
79 stars 53 forks source link

[Guava] Allow `Cache` serialization #112

Closed JooHyukKim closed 1 year ago

JooHyukKim commented 1 year ago

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

cowtowncoder commented 1 year ago

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 commented 1 year ago

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~

cowtowncoder commented 1 year ago

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

JooHyukKim commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

@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.