From c2db2ee11442f7fc6daf85cd88086ccd8f2dfa95 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 c01785ea2..c37c62518 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;
@@ -35,6 +34,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.
@@ -700,20 +700,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 b344bb088..4da39b586 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.RedisConnection;
 import org.springframework.data.redis.connection.RedisConnectionFactory;
@@ -39,6 +43,7 @@ import org.springframework.data.redis.serializer.RedisSerializer;
 
 /**
  * @author Christoph Strobl
+ * @author Mark Paluch
  */
 @SuppressWarnings("rawtypes")
 @RunWith(MockitoJUnitRunner.class)
@@ -153,6 +158,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
 	 */