Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Support for Caching List Results in Quarks Redis Cache #41301

Closed
ngChris opened this issue Jun 18, 2024 · 15 comments · Fixed by #43320
Closed

Add Support for Caching List Results in Quarks Redis Cache #41301

ngChris opened this issue Jun 18, 2024 · 15 comments · Fixed by #43320
Labels
Milestone

Comments

@ngChris
Copy link

ngChris commented Jun 18, 2024

Description

Currently, the Quarkus Redis Cache implementation does not support caching methods that return a list or a collection of objects. For example, if we use caching as shown in the example below, Quarkus complains that it cannot determine the value type of the cache.

@ApplicationScoped
public class MyService {

   @CacheResult(cacheName = "namesCache")
   public List<String> fetchNames() {
       // Do complex calculation
       ... 
     }
 }

In many applications, methods that return a list of objects are common. Therefore i suggest a mechanism such that the Quarkus Redis Cache implementation can infer the type correctly and that the entire list and its contents can be serialized and deserialized correctly when stored in and retrieved from Redis.

Implementation ideas

No response

@ngChris ngChris added the kind/enhancement New feature or request label Jun 18, 2024
Copy link

quarkus-bot bot commented Jun 18, 2024

/cc @Ladicek (redis), @cescoffier (redis), @gwenneg (cache), @machi1990 (redis)

@cescoffier cescoffier moved this to Under discussion in WG - Quarkus 3.15 LTS Jul 9, 2024
@cescoffier cescoffier moved this to Under discussion in WG - Quarkus 3.15 LTS Jul 9, 2024
@cescoffier cescoffier moved this from Under discussion to Out of scope in WG - Quarkus 3.15 LTS Jul 17, 2024
@Gonzalo-bia
Copy link

Generic types in general such as Optionals, collections, etc. are not supported.

It'd be a great addition.

@cescoffier
Copy link
Member

The type detection is done at build time, not at runtime. Passing generic types will require a bit of work.

@Gonzalo-bia
Copy link

I see. Could the mechanism Caffeine uses be reused here?

@cescoffier
Copy link
Member

No, because Caffeine does not need serialization/deserialization (it's all local). Here, the data must be serialized to be written into Redis and deserialized when retrieved.

@silviu-negoita
Copy link

I would also need this feature. We have a couple of expensive endpoints which return lists of items and we want to cache.

@cescoffier
Copy link
Member

I think workaround would be to use your own custom List implementation like: MyList implements List

@ngChris
Copy link
Author

ngChris commented Sep 11, 2024

We solved the problem by using wrapper classes around our lists and register them for reflection ( with the annotation @RegisterForReflection). With that solution the redis client automatically uses the JSON Codec for serialization/deserialization. Works quite well but the downside is that we have to create a wrapper class for every type we like to cache as a list.

@cescoffier
Copy link
Member

Yes, that approach works.
The main issue to get this working is that I need to pass the types detected at build time to the runtime (and that's not easily serializable, especially when there are many generics). Right now, it passes the classname of the object, and it works. But if you pass a List<X>, the <X> is lost.

@Ladicek
Copy link
Contributor

Ladicek commented Sep 12, 2024

The main issue to get this working is that I need to pass the types detected at build time to the runtime (and that's not easily serializable, especially when there are many generics).

We actually have solved that (mostly? I guess there are a few corner cases, but nothing major) in ArC, where we have code that takes Jandex Type and emits bytecode to instantiate java.lang.reflect.Type. See io.quarkus.arc.processor.Types.getTypeHandle(). Now, I don't know what representation of types you'd need, but this should be a good start.

@cescoffier
Copy link
Member

That would definitely be a big step forward. Then we have to see how we can use this type with Jackson TypeReference I think.

@Ladicek
Copy link
Contributor

Ladicek commented Sep 13, 2024

I'm no expert on the Quarkus Redis client, but it seems that's already been taken care of? See Codecs.JsonCodec -- its constructor takes java.lang.reflect.Type and turns it into a Jackson TypeReference behind the scenes. Not sure if that code has actually been executed or if it contains silent issues waiting to be discovered though :-)

@ngChris
Copy link
Author

ngChris commented Sep 13, 2024

I think the only difficulty is to pass the parameterized type to the JsonCodec. From there Jackson should be able to do the serialization/deserialization.

At the moment the class/type name is stored as a config at deployment time and later used in the RedisCacheImpl to load the class by using the class loader of the current thread (see io.quarkus.cache.redis.runtime.RedisCacheImpl#loadClass). This has to be adapted in order to work for parameterized types as well.

@Ladicek
Copy link
Contributor

Ladicek commented Sep 13, 2024

OK, thanks, so there are 2 actual problems:

  • in addition to figuring out the cached type from the method signature (which is a Jandex Type), there's a configuration property for that as well (which actually has higher priority)
  • the types are transferred from build time to runtime via recorders

Nothing insurmountable, just more work than I expected. The first problem requires parsing the configuration string, the second requires either adding support for transferring types to recorders, or a complete rewrite to custom bytecode generation.

@Ladicek
Copy link
Contributor

Ladicek commented Sep 16, 2024

Draft PR is at #43320

@github-project-automation github-project-automation bot moved this from Out of scope to Done in WG - Quarkus 3.15 LTS Sep 24, 2024
@quarkus-bot quarkus-bot bot added this to the 3.16 - main milestone Sep 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Development

Successfully merging a pull request may close this issue.

5 participants