From 0f54fcf796a9ec1b123acf18e784f77dcabdf359 Mon Sep 17 00:00:00 2001 From: Christoph Strobl Date: Thu, 8 Oct 2020 08:30:02 +0200 Subject: [PATCH] DATAREDIS-1103 - Polishing. 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 --- .../DefaultStringRedisConnection.java | 9 ----- .../connection/DefaultedRedisConnection.java | 7 ---- .../connection/ReactiveStringCommands.java | 3 +- .../redis/connection/RedisStringCommands.java | 19 +-------- .../connection/StringRedisConnection.java | 3 +- .../jedis/JedisClusterStringCommands.java | 9 ----- .../connection/jedis/JedisConverters.java | 18 +++++++++ .../connection/jedis/JedisStringCommands.java | 9 ----- .../connection/lettuce/LettuceConverters.java | 38 +++++++----------- .../lettuce/LettuceStringCommands.java | 14 ++----- .../data/redis/core/types/Expiration.java | 39 +++++++++++++++++++ .../AbstractConnectionIntegrationTests.java | 14 +++++++ .../LettuceClusterConnectionTests.java | 16 ++++---- .../LettuceReactiveStringCommandsTests.java | 17 +++++++- 14 files changed, 117 insertions(+), 98 deletions(-) diff --git a/src/main/java/org/springframework/data/redis/connection/DefaultStringRedisConnection.java b/src/main/java/org/springframework/data/redis/connection/DefaultStringRedisConnection.java index 57b86e8af..56da1a9fd 100644 --- 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 class DefaultStringRedisConnection implements StringRedisConnection, Deco 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 index 0cf9bbbf0..4b66f1fdf 100644 --- 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 @@ public interface DefaultedRedisConnection extends RedisConnection { 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 index 0ceea07cb..093f18b85 100644 --- 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 @@ public interface ReactiveStringCommands { * * @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 Redis Documentation: SET diff --git a/src/main/java/org/springframework/data/redis/connection/RedisStringCommands.java b/src/main/java/org/springframework/data/redis/connection/RedisStringCommands.java index 8e888c343..bb639e52f 100644 --- 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 @@ public interface RedisStringCommands { * * @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 @@ public interface RedisStringCommands { @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 Redis Documentation: SET - */ - @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 index 7730abbeb..7a016c265 100644 --- 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 @@ public interface StringRedisConnection extends RedisConnection { * * @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 Redis Documentation: SET 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 index 99d67bc48..5f57de963 100644 --- 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 @@ class JedisClusterStringCommands implements RedisStringCommands { */ @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 index b7f00edd3..a92b4a4f1 100644 --- 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 @@ abstract public class JedisConverters extends Converters { 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 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 index 44c654ec6..917938bbf 100644 --- 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 @@ class JedisStringCommands implements RedisStringCommands { */ @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 index aa22c4284..a2d680460 100644 --- 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,36 +768,26 @@ abstract public class LettuceConverters extends Converters { * @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}.
- * - * @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()) { + + switch (expiration.getTimeUnit()) { + case SECONDS: + args.ex(expiration.getExpirationTime()); + break; + default: + args.px(expiration.getConverted(TimeUnit.MILLISECONDS)); + break; + } } } - if (keepTtl) { - args.keepttl(); - } - if (option != null) { switch (option) { 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 index f60374898..6788d22c2 100644 --- 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 @@ class LettuceStringCommands implements RedisStringCommands { */ @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 @@ class LettuceStringCommands implements RedisStringCommands { 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 index 94b208e4a..05c6616b6 100644 --- 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 class Expiration { return new Expiration(expirationTime, TimeUnit.MILLISECONDS); } + /** + * Obtain an {@link Expiration} that indicates to keep the existing one. Eg. when sending a {@code SET} command. + *

+ * NOTE: 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 class Expiration { 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 index f8c86978d..49aa2fbfd 100644 --- 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 abstract class AbstractConnectionIntegrationTests { 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 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 index 2d525ed71..0d4a530b4 100644 --- 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.time.Duration; 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 class LettuceClusterConnectionTests implements ClusterConnectionTests { public void setKeepTTL() { long expireSeconds = 10; - assertThat(clusterConnection.setEx(KEY_1_BYTES, expireSeconds, VALUE_2_BYTES)); + nativeConnection.setex(KEY_1, expireSeconds, VALUE_1); - assertThat(clusterConnection.get(KEY_1_BYTES)).isEqualTo(VALUE_2_BYTES); + assertThat( + clusterConnection.stringCommands().set(KEY_1_BYTES, VALUE_2_BYTES, Expiration.keepTtl(), SetOption.upsert())) + .isTrue(); - assertThat(clusterConnection.set(KEY_1_BYTES, VALUE_2_BYTES, Expiration.persistent(), SetOption.upsert(), true)); - - long ttl = clusterConnection.ttl(KEY_1_BYTES); - - 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 index 8b20e7fa5..7d9592ae7 100644 --- 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.Set; 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.MultiVa 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 class LettuceReactiveStringCommandsTests extends LettuceReactiveCommandsT .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); + } }