From 4df8e44818d60ebf8a50dcfc5df3e5e9f1b143ef Mon Sep 17 00:00:00 2001 From: Christoph Strobl Date: Wed, 10 Feb 2021 12:47:41 +0100 Subject: [PATCH] Move expires check to dedicated method. Introduce helper method to unify expiry check and simplify control flow. Original Pull Request: #1961 --- .../data/redis/core/RedisKeyValueAdapter.java | 50 +++++++++++++------ .../redis/core/RedisKeyValueAdapterTests.java | 12 ++--- 2 files changed, 40 insertions(+), 22 deletions(-) diff --git a/src/main/java/org/springframework/data/redis/core/RedisKeyValueAdapter.java b/src/main/java/org/springframework/data/redis/core/RedisKeyValueAdapter.java index 28d9c8e20..22f29d520 100644 --- a/src/main/java/org/springframework/data/redis/core/RedisKeyValueAdapter.java +++ b/src/main/java/org/springframework/data/redis/core/RedisKeyValueAdapter.java @@ -236,26 +236,28 @@ public class RedisKeyValueAdapter extends AbstractKeyValueAdapter 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 + if (keepShadowCopy()) { // add phantom key so values can be restored + + byte[] phantomKey = ByteUtils.concat(objectKey, BinaryKeyspaceIdentifier.PHANTOM_SUFFIX); + + 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 class RedisKeyValueAdapter extends AbstractKeyValueAdapter 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 class RedisKeyValueAdapter extends AbstractKeyValueAdapter if (update.isRefreshTtl()) { - if (rdo.getTimeToLive() != null && rdo.getTimeToLive() > 0) { + if (expires(rdo)) { connection.expire(redisKey, rdo.getTimeToLive()); @@ -498,7 +503,10 @@ public class RedisKeyValueAdapter extends AbstractKeyValueAdapter } 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 @@ public class RedisKeyValueAdapter extends AbstractKeyValueAdapter 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}. * diff --git a/src/test/java/org/springframework/data/redis/core/RedisKeyValueAdapterTests.java b/src/test/java/org/springframework/data/redis/core/RedisKeyValueAdapterTests.java index 60903098b..9bc108c96 100644 --- a/src/test/java/org/springframework/data/redis/core/RedisKeyValueAdapterTests.java +++ b/src/test/java/org/springframework/data/redis/core/RedisKeyValueAdapterTests.java @@ -706,10 +706,10 @@ public class RedisKeyValueAdapterTests { 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 @@ public class RedisKeyValueAdapterTests { 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 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(); }