DATAREDIS-1103 - Polishing.

christophstrobl · christophstrobl · commit 0f54fcf796a9 · 2020-10-08T16:29:20.000+02:00
Move keepTTL to Expiration and remove superfluous methods from interfaces. Add tests for reactive variant and work around an open issue in Jedis to support KEEPTTL though the API does not offer that option directly. Original Pull Request: #562
diff --git a/src/main/java/org/springframework/data/redis/connection/DefaultStringRedisConnection.java b/src/main/java/org/springframework/data/redis/connection/DefaultStringRedisConnection.java
@@ -993,15 +993,6 @@ public Boolean set(byte[] key, byte[] value, Expiration expiration, SetOption op
 		return convertAndReturn(delegate.set(key, value, expiration, option), identityConverter);
 	}
 
-	/*
-	 * (non-Javadoc)
-	 * @see org.springframework.data.redis.connection.RedisStringCommands#set(byte[], byte[], org.springframework.data.redis.core.types.Expiration, org.springframework.data.redis.connection.RedisStringCommands.SetOptions, boolean)
-	 */
-	@Override
-	public Boolean set(byte[] key, byte[] value, Expiration expiration, SetOption option, boolean keepTtl) {
-		return convertAndReturn(delegate.set(key, value, expiration, option, keepTtl), identityConverter);
-	}
-
 	/*
 	 * (non-Javadoc)
 	 * @see org.springframework.data.redis.connection.RedisStringCommands#setBit(byte[], long, boolean)
diff --git a/src/main/java/org/springframework/data/redis/connection/DefaultedRedisConnection.java b/src/main/java/org/springframework/data/redis/connection/DefaultedRedisConnection.java
@@ -294,13 +294,6 @@ default Boolean set(byte[] key, byte[] value, Expiration expiration, SetOption o
 		return stringCommands().set(key, value, expiration, option);
 	}
 
-	/** @deprecated in favor of {@link RedisConnection#stringCommands()}}. */
-	@Override
-	@Deprecated
-	default Boolean set(byte[] key, byte[] value, Expiration expiration, SetOption option, boolean keepTtl) {
-		return stringCommands().set(key, value, expiration, option, keepTtl);
-	}
-
 	/** @deprecated in favor of {@link RedisConnection#stringCommands()}}. */
 	@Override
 	@Deprecated
diff --git a/src/main/java/org/springframework/data/redis/connection/ReactiveStringCommands.java b/src/main/java/org/springframework/data/redis/connection/ReactiveStringCommands.java
@@ -169,7 +169,8 @@ default Mono<Boolean> set(ByteBuffer key, ByteBuffer value) {
 	 *
 	 * @param key must not be {@literal null}.
 	 * @param value must not be {@literal null}.
-	 * @param expiration must not be {@literal null}.
+	 * @param expiration must not be {@literal null}. Use {@link Expiration#persistent()} for no expiration time or
+	 *          {@link Expiration#keepTtl()} to keep the existing.
 	 * @param option must not be {@literal null}.
 	 * @return
 	 * @see <a href="https://redis.io/commands/set">Redis Documentation: SET</a>
diff --git a/src/main/java/org/springframework/data/redis/connection/RedisStringCommands.java b/src/main/java/org/springframework/data/redis/connection/RedisStringCommands.java
@@ -83,7 +83,8 @@ enum BitOperation {
 	 *
 	 * @param key must not be {@literal null}.
 	 * @param value must not be {@literal null}.
-	 * @param expiration must not be {@literal null}. Use {@link Expiration#persistent()} to not set any ttl.
+	 * @param expiration must not be {@literal null}. Use {@link Expiration#persistent()} to not set any ttl or
+	 *          {@link Expiration#keepTtl()} to keep the existing expiration.
 	 * @param option must not be {@literal null}. Use {@link SetOption#upsert()} to add non existing.
 	 * @return {@literal null} when used in pipeline / transaction.
 	 * @since 1.7
@@ -92,22 +93,6 @@ enum BitOperation {
 	@Nullable
 	Boolean set(byte[] key, byte[] value, Expiration expiration, SetOption option);
 
-	/**
-	 * Set {@code value} for {@code key} applying timeouts from {@code expiration} if set and inserting/updating values
-	 * depending on {@code option}.
-	 *
-	 * @param key must not be {@literal null}.
-	 * @param value must not be {@literal null}.
-	 * @param expiration must not be {@literal null}. Use {@link Expiration#persistent()} to not set any ttl.
-	 * @param option must not be {@literal null}. Use {@link SetOption#upsert()} to add non existing.
-	 * @param keepTtl set the value and retain the existing TTL.
-	 * @return {@literal null} when used in pipeline / transaction.
-	 * @since 2.4
-	 * @see <a href="https://redis.io/commands/set">Redis Documentation: SET</a>
-	 */
-	@Nullable
-	Boolean set(byte[] key, byte[] value, Expiration expiration, SetOption option, boolean keepTtl);
-
 	/**
 	 * Set {@code value} for {@code key}, only if {@code key} does not exist.
 	 *
diff --git a/src/main/java/org/springframework/data/redis/connection/StringRedisConnection.java b/src/main/java/org/springframework/data/redis/connection/StringRedisConnection.java
@@ -420,7 +420,8 @@ interface StringTuple extends Tuple {
 	 *
 	 * @param key must not be {@literal null}.
 	 * @param value must not be {@literal null}.
-	 * @param expiration can be {@literal null}. Defaulted to {@link Expiration#persistent()}.
+	 * @param expiration can be {@literal null}. Defaulted to {@link Expiration#persistent()}. Use
+	 *          {@link Expiration#keepTtl()} to keep the existing expiration.
 	 * @param option can be {@literal null}. Defaulted to {@link SetOption#UPSERT}.
 	 * @since 1.7
 	 * @see <a href="https://redis.io/commands/set">Redis Documentation: SET</a>
diff --git a/src/main/java/org/springframework/data/redis/connection/jedis/JedisClusterStringCommands.java b/src/main/java/org/springframework/data/redis/connection/jedis/JedisClusterStringCommands.java
@@ -127,20 +127,11 @@ public Boolean set(byte[] key, byte[] value) {
 	 */
 	@Override
 	public Boolean set(byte[] key, byte[] value, Expiration expiration, SetOption option) {
-		return set(key, value, expiration, option, false);
-	}
 
-	/*
-	 * (non-Javadoc)
-	 * @see org.springframework.data.redis.connection.RedisStringCommands#set(byte[], byte[], org.springframework.data.redis.core.types.Expiration, org.springframework.data.redis.connection.RedisStringCommands.SetOptions, boolean)
-	 */
-	@Override
-	public Boolean set(byte[] key, byte[] value, Expiration expiration, SetOption option, boolean keepTtl) {
 		Assert.notNull(key, "Key must not be null!");
 		Assert.notNull(value, "Value must not be null!");
 		Assert.notNull(expiration, "Expiration must not be null!");
 		Assert.notNull(option, "Option must not be null!");
-		Assert.isTrue(keepTtl, "KEEPTTL is currently not supported in jedis!");
 
 		SetParams setParams = JedisConverters.toSetCommandExPxArgument(expiration,
 				JedisConverters.toSetCommandNxXxArgument(option));
diff --git a/src/main/java/org/springframework/data/redis/connection/jedis/JedisConverters.java b/src/main/java/org/springframework/data/redis/connection/jedis/JedisConverters.java
@@ -533,6 +533,24 @@ public static SetParams toSetCommandExPxArgument(Expiration expiration, SetParam
 
 		SetParams paramsToUse = params == null ? SetParams.setParams() : params;
 
+		if (expiration.isKeepTtl()) {
+
+			// TODO: remove once jedis supports KEEPTTL (https://github.com/xetorthio/jedis/issues/2248)
+			return new SetParams() {
+
+				@Override
+				public byte[][] getByteParams(byte[]... args) {
+
+					ArrayList<byte[]> byteParams = new ArrayList<>();
+					for (byte[] arg : paramsToUse.getByteParams(args)) {
+						byteParams.add(arg);
+					}
+					byteParams.add(SafeEncoder.encode("keepttl"));
+					return byteParams.toArray(new byte[byteParams.size()][]);
+				}
+			};
+		}
+
 		if (!expiration.isPersistent()) {
 			if (expiration.getTimeUnit() == TimeUnit.MILLISECONDS) {
 				return paramsToUse.px(expiration.getExpirationTime());
diff --git a/src/main/java/org/springframework/data/redis/connection/jedis/JedisStringCommands.java b/src/main/java/org/springframework/data/redis/connection/jedis/JedisStringCommands.java
@@ -153,20 +153,11 @@ public Boolean set(byte[] key, byte[] value) {
 	 */
 	@Override
 	public Boolean set(byte[] key, byte[] value, Expiration expiration, SetOption option) {
-		return set(key, value, expiration, option, false);
-	}
 
-	/*
-	 * (non-Javadoc)
-	 * @see org.springframework.data.redis.connection.RedisStringCommands#set(byte[], byte[], org.springframework.data.redis.core.types.Expiration, org.springframework.data.redis.connection.RedisStringCommands.SetOption, boolean)
-	 */
-	@Override
-	public Boolean set(byte[] key, byte[] value, Expiration expiration, SetOption option, boolean keepTtl) {
 		Assert.notNull(key, "Key must not be null!");
 		Assert.notNull(value, "Value must not be null!");
 		Assert.notNull(expiration, "Expiration must not be null!");
 		Assert.notNull(option, "Option must not be null!");
-		Assert.isTrue(keepTtl, "KEEPTTL is currently not supported in jedis!");
 
 		SetParams params = JedisConverters.toSetCommandExPxArgument(expiration,
 				JedisConverters.toSetCommandNxXxArgument(option));
diff --git a/src/main/java/org/springframework/data/redis/connection/lettuce/LettuceConverters.java b/src/main/java/org/springframework/data/redis/connection/lettuce/LettuceConverters.java
@@ -768,34 +768,24 @@ public static RedisClusterNode toRedisClusterNode(io.lettuce.core.cluster.models
 	 * @since 1.7
 	 */
 	public static SetArgs toSetArgs(@Nullable Expiration expiration, @Nullable SetOption option) {
-		return toSetArgs(expiration, option, false);
-	}
-
-	/**
-	 * Converts a given {@link Expiration} and {@link SetOption} to the according {@link SetArgs}.<br />
-	 *
-	 * @param expiration can be {@literal null}.
-	 * @param option can be {@literal null}.
-	 * @param keepTtl set the value and retain the existing TTL.
-	 * @since 2.4
-	 */
-	public static SetArgs toSetArgs(@Nullable Expiration expiration, @Nullable SetOption option, boolean keepTtl) {
 
 		SetArgs args = new SetArgs();
-		if (expiration != null && !expiration.isPersistent()) {
 
-			switch (expiration.getTimeUnit()) {
-				case SECONDS:
-					args.ex(expiration.getExpirationTime());
-					break;
-				default:
-					args.px(expiration.getConverted(TimeUnit.MILLISECONDS));
-					break;
-			}
-		}
+		if (expiration != null) {
+
+			if (expiration.isKeepTtl()) {
+				args.keepttl();
+			} else if (!expiration.isPersistent()) {
 
-		if (keepTtl) {
-			args.keepttl();
+				switch (expiration.getTimeUnit()) {
+					case SECONDS:
+						args.ex(expiration.getExpirationTime());
+						break;
+					default:
+						args.px(expiration.getConverted(TimeUnit.MILLISECONDS));
+						break;
+				}
+			}
 		}
 
 		if (option != null) {
diff --git a/src/main/java/org/springframework/data/redis/connection/lettuce/LettuceStringCommands.java b/src/main/java/org/springframework/data/redis/connection/lettuce/LettuceStringCommands.java
@@ -157,15 +157,7 @@ public Boolean set(byte[] key, byte[] value) {
 	 */
 	@Override
 	public Boolean set(byte[] key, byte[] value, Expiration expiration, SetOption option) {
-		return set(key, value, expiration, option, false);
-	}
 
-	/*
-	 * (non-Javadoc)
-	 * @see org.springframework.data.redis.connection.RedisStringCommands#set(byte[], byte[], org.springframework.data.redis.core.types.Expiration, org.springframework.data.redis.connection.RedisStringCommands.SetOption, boolean)
-	 */
-	@Override
-	public Boolean set(byte[] key, byte[] value, Expiration expiration, SetOption option, boolean keepTtl) {
 		Assert.notNull(key, "Key must not be null!");
 		Assert.notNull(value, "Value must not be null!");
 		Assert.notNull(expiration, "Expiration must not be null!");
@@ -174,18 +166,18 @@ public Boolean set(byte[] key, byte[] value, Expiration expiration, SetOption op
 		try {
 			if (isPipelined()) {
 				pipeline(connection.newLettuceResult(
-						getAsyncConnection().set(key, value, LettuceConverters.toSetArgs(expiration, option, keepTtl)),
+						getAsyncConnection().set(key, value, LettuceConverters.toSetArgs(expiration, option)),
 						Converters.stringToBooleanConverter(), () -> false));
 				return null;
 			}
 			if (isQueueing()) {
 				transaction(connection.newLettuceResult(
-						getAsyncConnection().set(key, value, LettuceConverters.toSetArgs(expiration, option, keepTtl)),
+						getAsyncConnection().set(key, value, LettuceConverters.toSetArgs(expiration, option)),
 						Converters.stringToBooleanConverter(), () -> false));
 				return null;
 			}
 			return Converters
-					.stringToBoolean(getConnection().set(key, value, LettuceConverters.toSetArgs(expiration, option, keepTtl)));
+					.stringToBoolean(getConnection().set(key, value, LettuceConverters.toSetArgs(expiration, option)));
 		} catch (Exception ex) {
 			throw convertLettuceAccessException(ex);
 		}
diff --git a/src/main/java/org/springframework/data/redis/core/types/Expiration.java b/src/main/java/org/springframework/data/redis/core/types/Expiration.java
@@ -115,6 +115,19 @@ public static Expiration milliseconds(long expirationTime) {
 		return new Expiration(expirationTime, TimeUnit.MILLISECONDS);
 	}
 
+	/**
+	 * Obtain an {@link Expiration} that indicates to keep the existing one. Eg. when sending a {@code SET} command.
+	 * <p />
+	 * <strong>NOTE: </strong>Please follow the documentation of the individual commands to see if {@link #keepTtl()} is
+	 * applicable.
+	 *
+	 * @return never {@literal null}.
+	 * @since 2.4
+	 */
+	public static Expiration keepTtl() {
+		return KeepTtl.INSTANCE;
+	}
+
 	/**
 	 * Creates new {@link Expiration} with the provided {@link TimeUnit}. Greater units than {@link TimeUnit#SECONDS} are
 	 * converted to {@link TimeUnit#SECONDS}. Units smaller than {@link TimeUnit#MILLISECONDS} are converted to
@@ -175,4 +188,30 @@ public static Expiration persistent() {
 	public boolean isPersistent() {
 		return expirationTime == -1;
 	}
+
+	/**
+	 * @return {@literal true} if {@link Expiration} of existing key should not be modified.
+	 * @since 2.4
+	 */
+	public boolean isKeepTtl() {
+		return false;
+	}
+
+	/**
+	 * @author Christoph Strobl
+	 * @since 2.4
+	 */
+	private static class KeepTtl extends Expiration {
+
+		static KeepTtl INSTANCE = new KeepTtl();
+
+		private KeepTtl() {
+			super(-2, null);
+		}
+
+		@Override
+		public boolean isKeepTtl() {
+			return true;
+		}
+	}
 }
diff --git a/src/test/java/org/springframework/data/redis/connection/AbstractConnectionIntegrationTests.java b/src/test/java/org/springframework/data/redis/connection/AbstractConnectionIntegrationTests.java
@@ -179,6 +179,20 @@ public void testExpire() throws Exception {
 		assertThat(waitFor(new KeyExpired("exp"), 3000l)).isTrue();
 	}
 
+	@Test // DATAREDIS-1103
+	public void testSetWithKeepTTL() {
+
+		actual.add(connection.set("exp", "true"));
+		actual.add(connection.expire("exp", 10));
+		actual.add(connection.set("exp", "changed", Expiration.keepTtl(), SetOption.upsert()));
+		actual.add(connection.ttl("exp"));
+
+		List<Object> results = getResults();
+
+		assertThat(results.get(2)).isEqualTo(true);
+		assertThat((Long) results.get(3)).isCloseTo(10L, Offset.offset(5L));
+	}
+
 	@Test
 	@IfProfileValue(name = "runLongTests", value = "true")
 	public void testExpireAt() throws Exception {
diff --git a/src/test/java/org/springframework/data/redis/connection/lettuce/LettuceClusterConnectionTests.java b/src/test/java/org/springframework/data/redis/connection/lettuce/LettuceClusterConnectionTests.java
@@ -16,7 +16,6 @@
 package org.springframework.data.redis.connection.lettuce;
 
 import static org.assertj.core.api.Assertions.*;
-import static org.assertj.core.api.Assertions.assertThat;
 import static org.assertj.core.data.Offset.offset;
 import static org.springframework.data.redis.connection.BitFieldSubCommands.*;
 import static org.springframework.data.redis.connection.BitFieldSubCommands.BitFieldIncrBy.Overflow.*;
@@ -37,12 +36,12 @@
 import java.util.*;
 import java.util.concurrent.TimeUnit;
 
+import org.assertj.core.data.Offset;
 import org.junit.After;
 import org.junit.Before;
 import org.junit.ClassRule;
 import org.junit.Rule;
 import org.junit.Test;
-
 import org.springframework.dao.DataAccessException;
 import org.springframework.data.domain.Range.Bound;
 import org.springframework.data.geo.Circle;
@@ -2472,14 +2471,13 @@ public void bitfieldShouldWorkUsingNonZeroBasedOffset() {
 	public void setKeepTTL() {
 
 		long expireSeconds = 10;
-		assertThat(clusterConnection.setEx(KEY_1_BYTES, expireSeconds, VALUE_2_BYTES));
-
-		assertThat(clusterConnection.get(KEY_1_BYTES)).isEqualTo(VALUE_2_BYTES);
-
-		assertThat(clusterConnection.set(KEY_1_BYTES, VALUE_2_BYTES, Expiration.persistent(), SetOption.upsert(), true));
+		nativeConnection.setex(KEY_1, expireSeconds, VALUE_1);
 
-		long ttl = clusterConnection.ttl(KEY_1_BYTES);
+		assertThat(
+				clusterConnection.stringCommands().set(KEY_1_BYTES, VALUE_2_BYTES, Expiration.keepTtl(), SetOption.upsert()))
+						.isTrue();
 
-		assertThat(0 < ttl && ttl <= expireSeconds);
+		assertThat(nativeConnection.ttl(KEY_1)).isCloseTo(expireSeconds, Offset.offset(5L));
+		assertThat(nativeConnection.get(KEY_1)).isEqualTo(VALUE_2);
 	}
 }
diff --git a/src/test/java/org/springframework/data/redis/connection/lettuce/LettuceReactiveStringCommandsTests.java b/src/test/java/org/springframework/data/redis/connection/lettuce/LettuceReactiveStringCommandsTests.java
@@ -37,8 +37,8 @@
 import java.util.stream.Collectors;
 import java.util.stream.Stream;
 
+import org.assertj.core.data.Offset;
 import org.junit.Test;
-
 import org.springframework.data.domain.Range;
 import org.springframework.data.domain.Range.Bound;
 import org.springframework.data.redis.connection.ReactiveRedisConnection;
@@ -49,6 +49,7 @@
 import org.springframework.data.redis.connection.ReactiveRedisConnection.RangeCommand;
 import org.springframework.data.redis.connection.ReactiveStringCommands.SetCommand;
 import org.springframework.data.redis.connection.RedisStringCommands.BitOperation;
+import org.springframework.data.redis.connection.RedisStringCommands.SetOption;
 import org.springframework.data.redis.core.types.Expiration;
 import org.springframework.data.redis.test.util.HexStringUtils;
 import org.springframework.data.redis.util.ByteUtils;
@@ -509,4 +510,18 @@ public void bitPosShouldReturnPositionInRangeCorrectly() {
 				.expectNext(16L).verifyComplete();
 	}
 
+	@Test // DATAREDIS-1103
+	public void setKeepTTL() {
+
+		long expireSeconds = 10;
+		nativeCommands.setex(KEY_1, expireSeconds, VALUE_1);
+
+		connection.stringCommands().set(KEY_1_BBUFFER, VALUE_2_BBUFFER, Expiration.keepTtl(), SetOption.upsert())
+				.as(StepVerifier::create) //
+				.expectNext(true) //
+				.verifyComplete();
+
+		assertThat(nativeBinaryCommands.ttl(KEY_1_BBUFFER)).isCloseTo(expireSeconds, Offset.offset(5L));
+		assertThat(nativeCommands.get(KEY_1)).isEqualTo(VALUE_2);
+	}
 }
