quarkusio / quarkus

Quarkus: Supersonic Subatomic Java.
https://quarkus.io
Apache License 2.0
13.54k stars 2.61k forks source link

Redis cache is not supporting with null values #39547

Open amitgol1 opened 5 months ago

amitgol1 commented 5 months ago

Describe the bug

In-memory cache : Annotation of @CacheResult on method that return null value will throw exception from redis cache, how to put the redis cache in ignore mode fro null values?

Expected behavior

Ignore if value is null

Actual behavior

Exception about null in the cache

How to Reproduce?

No response

Output of uname -a or ver

No response

Output of java -version

No response

Quarkus version or git rev

No response

Build tool (ie. output of mvnw --version or gradlew --version)

No response

Additional information

No response

quarkus-bot[bot] commented 5 months ago

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

cescoffier commented 5 months ago

Redis does not support 'null'. It's not part of the protocol. The semantic can be ambiguous:

This, I don't think we will fix it.

karesti commented 5 months ago

@cescoffier Spring Cache SPI allows telling if we want to cache or ignore null values. Even if a null value can't be added into the key/value in a cache provider ( which is the case for Redis and Infinispan for example), we can add an special wrapper representing a null value. Null value caching can be enabled or disabled too on the SPI. I would consider exploring this in the caching SPI

karesti commented 5 months ago

@cescoffier @geoand @gwenneg

Concerning the null values the cache extension states: Null value caching is supported

https://quarkus.io/guides/cache#negative-cache

Some approaches to caching with annotations involve conditional caching through metadata. The provided annotations allow for the explicit caching of method returns based on certain conditions, including unless statements and options for caching null values.

In our current cache extension, I believe a complex conditional caching mechanism might be unnecessary, at least for now. However, the issue of caching null returns versus non-null returns is a common problem worth considering. Including a property property to disable the null value caching in the @CacheResult annotation, maybe ?

cescoffier commented 5 months ago

I don't believe we should encourage caching null but instead recommend that the user use a custom sentinel value in this case. If null is supported by the caching provider, it can use null as a sentinel value; otherwise, a custom (non-null) sentinel value will need to be used.

karesti commented 5 months ago

@cescoffier what if you want to cache unless it's null ? This is a common use case

cescoffier commented 5 months ago

In this case, we would not cache null right? You will only cache the value that is not null.

karesti commented 5 months ago

As I said in my comment, the caching extension states that allows for storing null values, thus caching providers should do that too. However, there are cases where some methods return null, and caching that result is desirable, while in other cases, we prefer not to cache null to ensure method execution. This feature of conditional caching is commonly needed and enables us to control caching behavior as needed.

cescoffier commented 5 months ago

I agree; that's the ambiguity behind null. You do not know if you want to cache it or ignore that call completely.

amitgol1 commented 5 months ago

Thanks for the details that you shared. @karesti @cescoffier

What is the conclusion? Can you provide a flag to ignore null if we want?

cescoffier commented 5 months ago

@amitgol1 No, no conclusion so far.

I'm still not convinced that caching null is correct. Too much ambiguity.

anadinema commented 1 month ago

@cescoffier, just sharing over the use case which I think we may have in mind, let's say we have a method on which we are using the @CacheResult annotation to cache the response from that method, but in some of the cases the method is returning null. Now in that case we get a exception as the null can't be cached. However, what we want is to ignore that null response so that the next method executions can work and also, returning null will not break the request as it is still a valid request returning null and not caching that response.

Wouldn't it be possible to have a flag to control that? Not to cache null, but to ignore null or not to ignore it (current implementation of throwing exception).

cescoffier commented 1 month ago

I understand the use case, but it has drawbacks: you cannot distinguish a cache miss from a cached null value (without doing 2 calls to the backend in a transaction which would be quite expensive). Thus, your metrics will be off, as, again, it is going to be counted as a cache miss.

At the same time, a simple sentinel value disambiguates that situation.

anadinema commented 1 month ago

Okay, I understand now, but here we do not want to cache null value rather ignore that, so if is there any way we can provide a configuration parameter like cacheInfo.ignoreNullValues, using which we can safely ignore the null value instead of caching it?

cescoffier commented 1 month ago

I think that should be possible, but it's a different issue (can you create one).

The implementation is very simple: in the cache impl ignores when the method return a null value.

anadinema commented 1 month ago

Thanks @cescoffier, Just created one https://github.com/quarkusio/quarkus/issues/42184.

The implementation is very simple: in the cache impl ignores when the method return a null value.

I can give it a try, if that's okay, and raise a PR for the same?

cescoffier commented 1 week ago

Started a discussion about null value in cache: https://github.com/quarkusio/quarkus/discussions/42940