Move expires check to dedicated method.

Introduce helper method to unify expiry check and simplify control flow.

Original Pull Request: #1961

Author: christophstrobl

src/main/java/org/springframework/data/redis/core/RedisKeyValueAdapter.java

@@ -236,26 +236,28 @@ public Object put(Object id, Object item, String keyspace) {
 
 			connection.hMSet(objectKey, rdo.getBucket().rawMap());
 
-			if (rdo.getTimeToLive() != null && rdo.getTimeToLive() > 0) {
+			if(isNew) {
+				connection.sAdd(toBytes(rdo.getKeyspace()), key);
+			}
 
+			if (expires(rdo)) {
 				connection.expire(objectKey, rdo.getTimeToLive());
+			}
+
+			if (keepShadowCopy()) { // add phantom key so values can be restored
+
+				byte[] phantomKey = ByteUtils.concat(objectKey, BinaryKeyspaceIdentifier.PHANTOM_SUFFIX);
 
-				if (keepShadowCopy()) { // add phantom key so values can be restored
+				if (expires(rdo)) {
 
-					byte[] phantomKey = ByteUtils.concat(objectKey, BinaryKeyspaceIdentifier.PHANTOM_SUFFIX);
 					connection.del(phantomKey);
 					connection.hMSet(phantomKey, rdo.getBucket().rawMap());
 					connection.expire(phantomKey, rdo.getTimeToLive() + PHANTOM_KEY_TTL);
+				} else if (!isNew) {
+					connection.del(phantomKey);
 				}
 			}
 
-			boolean isNoExpire = rdo.getTimeToLive() == null || rdo.getTimeToLive() != null && rdo.getTimeToLive() < 0;
-			if (isNoExpire && !isNew && keepShadowCopy()){
-				connection.del(ByteUtils.concat(objectKey, BinaryKeyspaceIdentifier.PHANTOM_SUFFIX));
-			}
-
-			connection.sAdd(toBytes(rdo.getKeyspace()), key);
-
 			IndexWriter indexWriter = new IndexWriter(connection, converter);
 			if (isNew) {
 				indexWriter.createIndexes(key, rdo.getIndexedData());
@@ -349,11 +351,14 @@ public <T> T delete(Object id, String keyspace, Class<T> type) {
 				connection.sRem(binKeyspace, binId);
 				new IndexWriter(connection, converter).removeKeyFromIndexes(asString(keyspace), binId);
 
-				RedisPersistentEntity<?> persistentEntity = converter.getMappingContext().getPersistentEntity(type);
-				if (persistentEntity != null && persistentEntity.isExpiring()) {
+				if(RedisKeyValueAdapter.this.keepShadowCopy()) {
 
-					byte[] phantomKey = ByteUtils.concat(keyToDelete, BinaryKeyspaceIdentifier.PHANTOM_SUFFIX);
-					connection.del(phantomKey);
+					RedisPersistentEntity<?> persistentEntity = converter.getMappingContext().getPersistentEntity(type);
+					if (persistentEntity != null && persistentEntity.isExpiring()) {
+
+						byte[] phantomKey = ByteUtils.concat(keyToDelete, BinaryKeyspaceIdentifier.PHANTOM_SUFFIX);
+						connection.del(phantomKey);
+					}
 				}
 				return null;
 			});
@@ -484,7 +489,7 @@ public void update(PartialUpdate<?> update) {
 
 			if (update.isRefreshTtl()) {
 
-				if (rdo.getTimeToLive() != null && rdo.getTimeToLive() > 0) {
+				if (expires(rdo)) {
 
 					connection.expire(redisKey, rdo.getTimeToLive());
 
@@ -498,7 +503,10 @@ public void update(PartialUpdate<?> update) {
 				} else {
 
 					connection.persist(redisKey);
-					connection.del(ByteUtils.concat(redisKey, BinaryKeyspaceIdentifier.PHANTOM_SUFFIX));
+
+					if(keepShadowCopy()) {
+						connection.del(ByteUtils.concat(redisKey, BinaryKeyspaceIdentifier.PHANTOM_SUFFIX));
+					}
 				}
 			}
 
@@ -655,6 +663,16 @@ private <T> T readBackTimeToLiveIfSet(@Nullable byte[] key, @Nullable T target)
 		return target;
 	}
 
+	/**
+	 * @return {@literal true} if {@link RedisData#getTimeToLive()} has a positive value.
+	 *
+	 * @param data must not be {@literal null}.
+	 * @since 2.3.7
+	 */
+	private boolean expires(RedisData data) {
+		return data.getTimeToLive() != null && data.getTimeToLive() > 0;
+	}
+
 	/**
 	 * Configure usage of {@link KeyExpirationEventMessageListener}.
 	 *

src/test/java/org/springframework/data/redis/core/RedisKeyValueAdapterTests.java

@@ -706,10 +706,10 @@ void phantomKeyInsertedOnPutWhenShadowCopyIsInDefaultAndKeyspaceNotificationEnab
 		assertThat(template.hasKey("persons:1:phantom")).isTrue();
 	}
 
-	@Test // DATAREDIS-1955
+	@Test // GH-1955
 	void phantomKeyIsDeletedWhenPutWithNegativeTimeToLiveAndOldEntryTimeToLiveWasPositiveAndWhenShadowCopyIsTurnedOn() {
+
 		ExpiringPerson rand = new ExpiringPerson();
-		rand.id = "1";
 		rand.ttl = 3000L;
 
 		adapter.put("1", rand, "persons");
@@ -723,21 +723,21 @@ void phantomKeyIsDeletedWhenPutWithNegativeTimeToLiveAndOldEntryTimeToLiveWasPos
 		assertThat(template.hasKey("persons:1:phantom")).isFalse();
 	}
 
-	@Test // DATAREDIS-1955
+	@Test // GH-1955
 	void updateWithRefreshTtlAndWithoutPositiveTtlShouldDeletePhantomKey() {
+
 		ExpiringPerson person = new ExpiringPerson();
-		person.id = "1";
 		person.ttl = 100L;
 
 		adapter.put("1", person, "persons");
-
 		assertThat(template.getExpire("persons:1:phantom")).isPositive();
 
 		PartialUpdate<ExpiringPerson> update = new PartialUpdate<>("1", ExpiringPerson.class) //
-				.refreshTtl(true);
+				.set("ttl", -1L).refreshTtl(true);
 
 		adapter.update(update);
 
+		assertThat(template.getExpire("persons:1")).isNotPositive();
 		assertThat(template.hasKey("persons:1:phantom")).isFalse();
 	}