From ea598c2dad9fd82d7c8ec5fd7b41e04efa29653e Mon Sep 17 00:00:00 2001
From: Mark Paluch <mpaluch@pivotal.io>
Date: Thu, 6 Oct 2016 13:39:08 +0200
Subject: [PATCH] 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: #224
---
 .../data/redis/cache/RedisCache.java          | 27 +++++----
 .../data/redis/cache/RedisCacheUnitTests.java | 55 +++++++++++++++++++
 2 files changed, 72 insertions(+), 10 deletions(-)

diff --git a/src/main/java/org/springframework/data/redis/cache/RedisCache.java b/src/main/java/org/springframework/data/redis/cache/RedisCache.java
index 76071ae80..a34c34b30 100644
--- a/src/main/java/org/springframework/data/redis/cache/RedisCache.java
+++ b/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.core.RedisOperations;
 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 class RedisCache implements Cache {
 		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;
 		}
 	}
 
diff --git a/src/test/java/org/springframework/data/redis/cache/RedisCacheUnitTests.java b/src/test/java/org/springframework/data/redis/cache/RedisCacheUnitTests.java
index 5a9b7338b..8f1df5421 100644
--- a/src/test/java/org/springframework/data/redis/cache/RedisCacheUnitTests.java
+++ b/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.rules.ExpectedException;
 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 @@ import org.springframework.data.redis.serializer.RedisSerializer;
 
 /**
  * @author Christoph Strobl
+ * @author Mark Paluch
  */
 @SuppressWarnings("rawtypes")
 @RunWith(MockitoJUnitRunner.class)
@@ -154,6 +159,56 @@ public class RedisCacheUnitTests {
 		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
 	 */