Skip to content

Expose ValueWrapper to differentiate between cached null values and absent cache mapping. #2785

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.

Sign up for GitHub

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

Closed
wants to merge 4 commits into from
Closed

Expose ValueWrapper to differentiate between cached null values and absent cache mapping. #2785

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b01140a
Prepare issue branch.
mp911de Nov 22, 2023
8fb816c
Build against Spring 6.1.1 snapshots.
mp911de Nov 22, 2023
e81d686
Expose ValueWrapper to differentiate between cached null values and a…
mp911de Nov 22, 2023
8229f68
Polishing.
mp911de Nov 22, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@

<groupId>org.springframework.data</groupId>
<artifactId>spring-data-redis</artifactId>
<version>3.3.0-SNAPSHOT</version>
<version>3.3.0-GH-2783-SNAPSHOT</version>

<name>Spring Data Redis</name>
<description>Spring Data module for Redis</description>
Expand All @@ -29,6 +29,7 @@
<multithreadedtc>1.01</multithreadedtc>
<netty>4.1.100.Final</netty>
<java-module-name>spring.data.redis</java-module-name>
<spring>6.1.1-SNAPSHOT</spring>
</properties>

<scm>
Expand Down
53 changes: 27 additions & 26 deletions src/main/java/org/springframework/data/redis/cache/RedisCache.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,8 +62,7 @@ public class RedisCache extends AbstractValueAdaptingCache {

static final byte[] BINARY_NULL_VALUE = RedisSerializer.java().serialize(NullValue.INSTANCE);

static final String CACHE_RETRIEVAL_UNSUPPORTED_OPERATION_EXCEPTION_MESSAGE =
"The Redis driver configured with RedisCache through RedisCacheWriter does not support CompletableFuture-based retrieval";
static final String CACHE_RETRIEVAL_UNSUPPORTED_OPERATION_EXCEPTION_MESSAGE = "The Redis driver configured with RedisCache through RedisCacheWriter does not support CompletableFuture-based retrieval";

private final Lock lock = new ReentrantLock();

Expand All @@ -74,16 +73,16 @@ public class RedisCache extends AbstractValueAdaptingCache {
private final String name;

/**
* Create a new {@link RedisCache} with the given {@link String name} and {@link RedisCacheConfiguration},
* using the {@link RedisCacheWriter} to execute Redis commands supporting the cache operations.
* Create a new {@link RedisCache} with the given {@link String name} and {@link RedisCacheConfiguration}, using the
* {@link RedisCacheWriter} to execute Redis commands supporting the cache operations.
*
* @param name {@link String name} for this {@link Cache}; must not be {@literal null}.
* @param cacheWriter {@link RedisCacheWriter} used to perform {@link RedisCache} operations
* by executing the necessary Redis commands; must not be {@literal null}.
* @param cacheConfiguration {@link RedisCacheConfiguration} applied to this {@link RedisCache} on creation;
* must not be {@literal null}.
* @param cacheWriter {@link RedisCacheWriter} used to perform {@link RedisCache} operations by executing the
* necessary Redis commands; must not be {@literal null}.
* @param cacheConfiguration {@link RedisCacheConfiguration} applied to this {@link RedisCache} on creation; must not
* be {@literal null}.
* @throws IllegalArgumentException if either the given {@link RedisCacheWriter} or {@link RedisCacheConfiguration}
* are {@literal null} or the given {@link String} name for this {@link RedisCache} is {@literal null}.
* are {@literal null} or the given {@link String} name for this {@link RedisCache} is {@literal null}.
*/
protected RedisCache(String name, RedisCacheWriter cacheWriter, RedisCacheConfiguration cacheConfiguration) {

Expand Down Expand Up @@ -117,11 +116,11 @@ protected RedisCacheWriter getCacheWriter() {
}

/**
* Gets the configured {@link ConversionService} used to convert {@link Object cache keys} to a {@link String}
* when accessing entries in the cache.
* Gets the configured {@link ConversionService} used to convert {@link Object cache keys} to a {@link String} when
* accessing entries in the cache.
*
* @return the configured {@link ConversionService} used to convert {@link Object cache keys} to a {@link String}
* when accessing entries in the cache.
* @return the configured {@link ConversionService} used to convert {@link Object cache keys} to a {@link String} when
* accessing entries in the cache.
* @see RedisCacheConfiguration#getConversionService()
* @see #getCacheConfiguration()
*/
Expand Down Expand Up @@ -175,8 +174,8 @@ private <T> T getSynchronized(Object key, Callable<T> valueLoader) {
}

/**
* Loads the {@link Object} using the given {@link Callable valueLoader} and {@link #put(Object, Object) puts}
* the {@link Object loaded value} in the cache.
* Loads the {@link Object} using the given {@link Callable valueLoader} and {@link #put(Object, Object) puts} the
* {@link Object loaded value} in the cache.
*
* @param <T> {@link Class type} of the loaded {@link Object cache value}.
* @param key {@link Object key} mapped to the loaded {@link Object cache value}.
Expand Down Expand Up @@ -281,13 +280,13 @@ public void evict(Object key) {
}

@Override
public CompletableFuture<?> retrieve(Object key) {
public CompletableFuture<ValueWrapper> retrieve(Object key) {

if (!getCacheWriter().supportsAsyncRetrieve()) {
throw new UnsupportedOperationException(CACHE_RETRIEVAL_UNSUPPORTED_OPERATION_EXCEPTION_MESSAGE);
}

return retrieveValue(key).thenApply(this::nullSafeDeserializedStoreValue);
return retrieveValue(key);
}

@Override
Expand All @@ -298,10 +297,10 @@ public <T> CompletableFuture<T> retrieve(Object key, Supplier<CompletableFuture<
throw new UnsupportedOperationException(CACHE_RETRIEVAL_UNSUPPORTED_OPERATION_EXCEPTION_MESSAGE);
}

return retrieveValue(key).thenCompose(bytes -> {
return retrieveValue(key).thenCompose(wrapper -> {

if (bytes != null) {
return CompletableFuture.completedFuture((T) nullSafeDeserializedStoreValue(bytes));
if (wrapper != null) {
return CompletableFuture.completedFuture((T) wrapper.get());
}

return valueLoader.get().thenCompose(value -> {
Expand All @@ -313,8 +312,7 @@ public <T> CompletableFuture<T> retrieve(Object key, Supplier<CompletableFuture<

Duration timeToLive = getTimeToLive(key, cacheValue);

return getCacheWriter().store(getName(), binaryKey, binaryValue, timeToLive)
.thenApply(v -> value);
return getCacheWriter().store(getName(), binaryKey, binaryValue, timeToLive).thenApply(v -> value);
});
});
}
Expand Down Expand Up @@ -440,15 +438,18 @@ protected String convertKey(Object key) {
return key.toString();
}

String message = String.format("Cannot convert cache key %s to String; Please register a suitable Converter"
+ " via 'RedisCacheConfiguration.configureKeyConverters(...)' or override '%s.toString()'",
String message = String.format(
"Cannot convert cache key %s to String; Please register a suitable Converter"
+ " via 'RedisCacheConfiguration.configureKeyConverters(...)' or override '%s.toString()'",
source, key.getClass().getName());

throw new IllegalStateException(message);
}

private CompletableFuture<byte[]> retrieveValue(Object key) {
return getCacheWriter().retrieve(getName(), createAndConvertCacheKey(key));
private CompletableFuture<ValueWrapper> retrieveValue(Object key) {
return getCacheWriter().retrieve(getName(), createAndConvertCacheKey(key)) //
.thenApply(binaryValue -> binaryValue != null ? deserializeCacheValue(binaryValue) : null) //
.thenApply(this::toValueWrapper);
}

@Nullable
Expand Down
66 changes: 49 additions & 17 deletions src/test/java/org/springframework/data/redis/cache/RedisCacheTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,8 @@
*/
package org.springframework.data.redis.cache;

import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatExceptionOfType;
import static org.assertj.core.api.Assertions.assertThatIllegalStateException;
import static org.awaitility.Awaitility.await;
import static org.assertj.core.api.Assertions.*;
import static org.awaitility.Awaitility.*;

import io.netty.util.concurrent.DefaultThreadFactory;

Expand Down Expand Up @@ -575,8 +573,7 @@ void cacheGetWithTimeToIdleExpirationAfterEntryExpiresShouldReturnNull() {
void retrieveCacheValueUsingJedis() {

assertThatExceptionOfType(UnsupportedOperationException.class)
.isThrownBy(() -> this.cache.retrieve(this.binaryCacheKey))
.withMessageContaining("RedisCache");
.isThrownBy(() -> this.cache.retrieve(this.binaryCacheKey)).withMessageContaining("RedisCache");
}

@ParameterizedRedisTest // GH-2650
Expand All @@ -590,23 +587,51 @@ void retrieveLoadedValueUsingJedis() {

@ParameterizedRedisTest // GH-2650
@EnabledOnRedisDriver(RedisDriver.LETTUCE)
@SuppressWarnings("unchecked")
void retrieveReturnsCachedValue() throws Exception {

doWithConnection(connection -> connection.stringCommands().set(this.binaryCacheKey, this.binarySample));

RedisCache cache = new RedisCache("cache", usingLockingRedisCacheWriter(),
usingRedisCacheConfiguration().disableCachingNullValues());

CompletableFuture<ValueWrapper> value = cache.retrieve(this.key);

assertThat(value).isNotNull();
assertThat(value.get(5, TimeUnit.SECONDS)).isNotNull();
assertThat(value.get().get()).isEqualTo(this.sample);
assertThat(value).isDone();
}

@ParameterizedRedisTest // GH-2650
@EnabledOnRedisDriver(RedisDriver.LETTUCE)
void retrieveReturnsCachedNullableValue() throws Exception {

doWithConnection(connection -> connection.stringCommands().set(this.binaryCacheKey, this.binarySample));

RedisCache cache = new RedisCache("cache", usingLockingRedisCacheWriter(), usingRedisCacheConfiguration());

CompletableFuture<Person> value = (CompletableFuture<Person>) cache.retrieve(this.key);
CompletableFuture<ValueWrapper> value = cache.retrieve(this.key);

assertThat(value).isNotNull();
assertThat(value.get()).isEqualTo(this.sample);
assertThat(value.get().get()).isEqualTo(this.sample);
assertThat(value).isDone();
}

@ParameterizedRedisTest // GH-2783
@EnabledOnRedisDriver(RedisDriver.LETTUCE)
void retrieveReturnsCachedNullValue() throws Exception {

doWithConnection(connection -> connection.set(binaryCacheKey, binaryNullValue));

CompletableFuture<ValueWrapper> value = (CompletableFuture<ValueWrapper>) cache.retrieve(this.key);
ValueWrapper wrapper = value.get(5, TimeUnit.SECONDS);

assertThat(wrapper).isNotNull();
assertThat(wrapper.get()).isNull();
}

@ParameterizedRedisTest // GH-2650
@EnabledOnRedisDriver(RedisDriver.LETTUCE)
@SuppressWarnings("unchecked")
void retrieveReturnsCachedValueWhenLockIsReleased() throws Exception {

String testValue = "TestValue";
Expand All @@ -622,13 +647,12 @@ void retrieveReturnsCachedValueWhenLockIsReleased() throws Exception {

cacheWriter.lock("cache");

CompletableFuture<String> value = (CompletableFuture<String>) cache.retrieve(this.key);

CompletableFuture<ValueWrapper> value = cache.retrieve(this.key);
assertThat(value).isNotDone();

cacheWriter.unlock("cache");

assertThat(value.get(15L, TimeUnit.MILLISECONDS)).isEqualTo(testValue);
assertThat(value.get(15L, TimeUnit.MILLISECONDS).get()).isEqualTo(testValue);
assertThat(value).isDone();
}

Expand Down Expand Up @@ -666,8 +690,9 @@ void retrieveStoresLoadedValue() throws Exception {

cache.retrieve(this.key, valueLoaderSupplier).get();

doWithConnection(connection ->
assertThat(connection.keyCommands().exists("cache::key-1".getBytes(StandardCharsets.UTF_8))).isTrue());
doWithConnection(
connection -> assertThat(connection.keyCommands().exists("cache::key-1".getBytes(StandardCharsets.UTF_8)))
.isTrue());
}

@ParameterizedRedisTest // GH-2650
Expand All @@ -678,11 +703,18 @@ void retrieveReturnsNull() throws Exception {

RedisCache cache = new RedisCache("cache", usingLockingRedisCacheWriter(), usingRedisCacheConfiguration());

CompletableFuture<?> value = cache.retrieve(this.key);
CompletableFuture<ValueWrapper> value = cache.retrieve(this.key);

assertThat(value).isNotNull();
assertThat(value.get()).isNull();
assertThat(value.get(5, TimeUnit.SECONDS).get()).isNull();
assertThat(value).isDone();

doWithConnection(connection -> connection.keyCommands().del(this.binaryCacheKey));

value = cache.retrieve(this.key);

assertThat(value).isNotNull();
assertThat(value.get(5, TimeUnit.SECONDS)).isNull();
}

private <T> CompletableFuture<T> usingCompletedFuture(T value) {
Expand Down
21 changes: 7 additions & 14 deletions src/test/java/org/springframework/data/redis/cache/RedisCacheUnitTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,20 +15,14 @@
*/
package org.springframework.data.redis.cache;

import static org.assertj.core.api.Assertions.assertThat;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.ArgumentMatchers.anyString;
import static org.mockito.ArgumentMatchers.eq;
import static org.mockito.ArgumentMatchers.isA;
import static org.mockito.Mockito.doReturn;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.times;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.verifyNoMoreInteractions;
import static org.assertj.core.api.Assertions.*;
import static org.mockito.ArgumentMatchers.*;
import static org.mockito.Mockito.*;

import java.util.concurrent.CompletableFuture;

import org.junit.jupiter.api.Test;
import org.springframework.cache.Cache.ValueWrapper;
import org.springframework.data.redis.serializer.RedisSerializationContext.SerializationPair;

/**
Expand All @@ -40,7 +34,6 @@
class RedisCacheUnitTests {

@Test // GH-2650
@SuppressWarnings("unchecked")
void cacheRetrieveValueCallsCacheWriterRetrieveCorrectly() throws Exception {

RedisCacheWriter mockCacheWriter = mock(RedisCacheWriter.class);
Expand All @@ -53,17 +46,17 @@ void cacheRetrieveValueCallsCacheWriterRetrieveCorrectly() throws Exception {

RedisCache cache = new RedisCache("TestCache", mockCacheWriter, cacheConfiguration);

CompletableFuture<byte[]> value = (CompletableFuture<byte[]>) cache.retrieve("TestKey");
CompletableFuture<ValueWrapper> value = cache.retrieve("TestKey");

assertThat(value).isNotNull();
assertThat(new String(value.get())).isEqualTo("TEST");
assertThat(new String((byte[]) value.get().get())).isEqualTo("TEST");

verify(mockCacheWriter, times(1)).retrieve(eq("TestCache"), isA(byte[].class));
verify(mockCacheWriter).supportsAsyncRetrieve();
verifyNoMoreInteractions(mockCacheWriter);
}

private <T> CompletableFuture<T> usingCompletedFuture(T value) {
return CompletableFuture.completedFuture(value);
return CompletableFuture.completedFuture(value);
}
}