DATAREDIS-542 - Fix key expiration for RedisCache.putIfAbsent.

RedisCache now expires keys using putIfAbsent if the key was set. Previously, the key was only expired if the value was already present and the value matched the value stored inside of Redis. Original Pull Request: spring-projects#224

Author: mp911de

src/main/java/org/springframework/data/redis/cache/RedisCache.java

@@ -17,7 +17,6 @@
 package org.springframework.data.redis.cache;
 
 import static org.springframework.util.Assert.*;
-import static org.springframework.util.ObjectUtils.*;
 
 import java.lang.reflect.Constructor;
 import java.util.Arrays;
@@ -37,6 +36,7 @@
 import org.springframework.data.redis.serializer.RedisSerializer;
 import org.springframework.data.redis.serializer.StringRedisSerializer;
 import org.springframework.util.ClassUtils;
+import org.springframework.util.ObjectUtils;
 
 /**
  * Cache implementation on top of Redis.
@@ -720,20 +720,27 @@ public RedisCachePutIfAbsentCallback(BinaryRedisCacheElement element, RedisCache
 public byte[] doInRedis(BinaryRedisCacheElement element, RedisConnection connection) throws DataAccessException {
 
 waitForLock(connection);
-byte[] resultValue = put(element, connection);
 
-if (nullSafeEquals(element.get(), resultValue)) {
+byte existingValue[] = null;
+
+boolean keyMaintenance;
+
+byte[] keyBytes = element.getKeyBytes();
+byte[] value = element.get();
+
+if (connection.setNX(keyBytes, value)) {
+keyMaintenance = true;
+} else {
+existingValue = connection.get(keyBytes);
+keyMaintenance = ObjectUtils.nullSafeEquals(value, existingValue);
+}
+
+if (keyMaintenance) {
 processKeyExpiration(element, connection);
 maintainKnownKeys(element, connection);
 }
 
-return resultValue;
-}
-
-private byte[] put(BinaryRedisCacheElement element, RedisConnection connection) {
-
-boolean valueWasSet = connection.setNX(element.getKeyBytes(), element.get());
-return valueWasSet ? null : connection.get(element.getKeyBytes());
+return existingValue;
 }
 }
 

src/test/java/org/springframework/data/redis/cache/RedisCacheUnitTests.java

@@ -15,6 +15,9 @@
  */
 package org.springframework.data.redis.cache;
 
+import static org.hamcrest.Matchers.is;
+import static org.hamcrest.Matchers.notNullValue;
+import static org.hamcrest.Matchers.nullValue;
 import static org.hamcrest.core.IsEqual.*;
 import static org.junit.Assert.*;
 import static org.mockito.Matchers.*;
@@ -30,6 +33,7 @@
 import org.junit.runner.RunWith;
 import org.mockito.Mock;
 import org.mockito.runners.MockitoJUnitRunner;
+import org.springframework.cache.Cache;
 import org.springframework.data.redis.RedisSystemException;
 import org.springframework.data.redis.connection.RedisClusterConnection;
 import org.springframework.data.redis.connection.RedisConnection;
@@ -40,6 +44,7 @@
 
 /**
  * @author Christoph Strobl
+ * @author Mark Paluch
  */
 @SuppressWarnings("rawtypes")
 @RunWith(MockitoJUnitRunner.class)
@@ -154,6 +159,56 @@ public void putShouldNotExpireKnownKeysSetWhenTtlIsZero() {
 verify(connectionMock, never()).expire(eq(KNOWN_KEYS_SET_NAME_BYTES), anyLong());
 }
 
+/**
+ * @see DATAREDIS-542
+ */
+@Test
+public void putIfAbsentShouldExpireWhenValueWasSet() {
+
+when(connectionMock.setNX(KEY_BYTES, VALUE_BYTES)).thenReturn(true);
+
+cache = new RedisCache(CACHE_NAME, NO_PREFIX_BYTES, templateSpy, 10L);
+Cache.ValueWrapper valueWrapper = cache.putIfAbsent(KEY, VALUE);
+
+assertThat(valueWrapper, is(nullValue()));
+verify(connectionMock).setNX(KEY_BYTES, VALUE_BYTES);
+verify(connectionMock).expire(eq(KEY_BYTES), anyLong());
+}
+
+/**
+ * @see DATAREDIS-542
+ */
+@Test
+public void putIfAbsentShouldNotExpireWhenValueWasNotSetAndRedisContainsOtherData() {
+
+String other = "other";
+when(connectionMock.setNX(KEY_BYTES, VALUE_BYTES)).thenReturn(false);
+when(connectionMock.get(KEY_BYTES)).thenReturn(other.getBytes());
+when(valueSerializerMock.deserialize(eq(other.getBytes()))).thenReturn(other);
+
+cache = new RedisCache(CACHE_NAME, NO_PREFIX_BYTES, templateSpy, 10L);
+Cache.ValueWrapper valueWrapper = cache.putIfAbsent(KEY, VALUE);
+
+assertThat(valueWrapper, is(notNullValue()));
+verify(connectionMock, never()).expire(eq(KEY_BYTES), anyLong());
+}
+
+/**
+ * @see DATAREDIS-542
+ */
+@Test
+public void putIfAbsentShouldExpireWhenValueWasNotSetAndRedisContainsSameData() {
+
+when(connectionMock.setNX(KEY_BYTES, VALUE_BYTES)).thenReturn(false);
+when(connectionMock.get(KEY_BYTES)).thenReturn(VALUE_BYTES);
+
+cache = new RedisCache(CACHE_NAME, NO_PREFIX_BYTES, templateSpy, 10L);
+Cache.ValueWrapper valueWrapper = cache.putIfAbsent(KEY, VALUE);
+
+assertThat(valueWrapper, is(notNullValue()));
+verify(connectionMock).expire(eq(KEY_BYTES), anyLong());
+}
+
 /**
  * @see DATAREDIS-443
  */