DATAREDIS-481 - Hacking: API refinements & additional tests.

Author: christophstrobl

src/main/java/org/springframework/data/redis/cache/DefaultRedisCacheWriter.java

@@ -37,7 +37,7 @@ class DefaultRedisCacheWriter implements RedisCacheWriter {
 			.getBytes(Charset.forName("UTF-8"));
 
 	private final RedisConnectionFactory connectionFactory;
-	private final Duration lockTimeout;
+	private final Duration sleepTime;
 
 	/**
 	 * @param connectionFactory must not be {@literal null}.
@@ -48,20 +48,34 @@ class DefaultRedisCacheWriter implements RedisCacheWriter {
 
 	/**
 	 * @param connectionFactory must not be {@literal null}.
-	 * @param lockTimeout must not be {@literal null}. Use {@link Duration#ZERO} to disable locking.
+	 * @param sleepTime sleep time between lock request attempts. Must not be {@literal null}. Use {@link Duration#ZERO}
+	 *          to disable locking.
 	 */
-	DefaultRedisCacheWriter(RedisConnectionFactory connectionFactory, Duration lockTimeout) {
+	DefaultRedisCacheWriter(RedisConnectionFactory connectionFactory, Duration sleepTime) {
 
 		Assert.notNull(connectionFactory, "ConnectionFactory must not be null!");
-		Assert.notNull(lockTimeout, "LockTimeout must not be null!");
+		Assert.notNull(sleepTime, "SleepTime must not be null!");
 
 		this.connectionFactory = connectionFactory;
-		this.lockTimeout = lockTimeout;
+		this.sleepTime = sleepTime;
+	}
+
+	public static DefaultRedisCacheWriter nonLockingRedisCacheWriter(RedisConnectionFactory connectionFactory) {
+		return new DefaultRedisCacheWriter(connectionFactory);
+	}
+
+	public static DefaultRedisCacheWriter lockingRedisCacheWriter(RedisConnectionFactory connectionFactory) {
+		return new DefaultRedisCacheWriter(connectionFactory, Duration.ofMillis(50));
 	}
 
 	@Override
 	public void put(String name, byte[] key, byte[] value, Duration ttl) {
 
+		Assert.notNull(name, "Name must not be null!");
+		Assert.notNull(key, "Key must not be null!");
+		Assert.notNull(value, "Value must not be null!");
+		Assert.notNull(ttl, "Ttl must not be null!");
+
 		execute(name, connection -> {
 
 			if (shouldExpireWithin(ttl)) {
@@ -76,12 +90,21 @@ public void put(String name, byte[] key, byte[] value, Duration ttl) {
 
 	@Override
 	public byte[] get(String name, byte[] key) {
+
+		Assert.notNull(name, "Name must not be null!");
+		Assert.notNull(key, "Key must not be null!");
+
 		return execute(name, connection -> connection.get(key));
 	}
 
 	@Override
 	public byte[] putIfAbsent(String name, byte[] key, byte[] value, Duration ttl) {
 
+		Assert.notNull(name, "Name must not be null!");
+		Assert.notNull(key, "Key must not be null!");
+		Assert.notNull(value, "Value must not be null!");
+		Assert.notNull(ttl, "Ttl must not be null!");
+
 		return execute(name, connection -> {
 
 			if (connection.setNX(key, value)) {
@@ -98,11 +121,15 @@ public byte[] putIfAbsent(String name, byte[] key, byte[] value, Duration ttl) {
 
 	@Override
 	public void remove(String name, byte[] key) {
+
+		Assert.notNull(name, "Name must not be null!");
+		Assert.notNull(key, "Key must not be null!");
+
 		execute(name, connection -> connection.del(key));
 	}
 
 	public void lock(String name) {
-		executeWithoutLockCheck(connection -> doLock(name, connection));
+		execute(name, connection -> doLock(name, connection));
 	}
 
 	private Boolean doLock(String name, RedisConnection connection) {
@@ -128,35 +155,40 @@ private boolean doCheckLock(String name, RedisConnection connection) {
 	@Override
 	public void clean(String name, byte[] pattern) {
 
-		RedisConnection connection = connectionFactory.getConnection();
+		Assert.notNull(name, "Name must not be null!");
+		Assert.notNull(pattern, "Pattern must not be null!");
 
-		if (isLockingCacheWriter()) {
-			doLock(name, connection);
-		}
-
-		try {
-			if (connection instanceof RedisClusterConnection) {
+		execute(name, connection -> {
 
-				byte[][] keys = connection.keys(pattern).stream().toArray(size -> new byte[size][]);
-				connection.del(keys);
-			} else {
-				connection.eval(CLEAN_SCRIPT, ReturnType.INTEGER, 0, pattern);
+			if (isLockingCacheWriter()) {
+				doLock(name, connection);
 			}
-		} finally {
 
-			if (isLockingCacheWriter()) {
-				doUnlock(name, connection);
+			try {
+				if (connection instanceof RedisClusterConnection) {
+
+					byte[][] keys = connection.keys(pattern).stream().toArray(size -> new byte[size][]);
+					connection.del(keys);
+				} else {
+					connection.eval(CLEAN_SCRIPT, ReturnType.INTEGER, 0, pattern);
+				}
+			} finally {
+
+				if (isLockingCacheWriter()) {
+					doUnlock(name, connection);
+				}
 			}
-			connection.close();
-		}
+
+			return "OK";
+		});
 	}
 
 	public <T> T execute(String name, ConnectionCallback<T> callback) {
 
 		RedisConnection connection = connectionFactory.getConnection();
 		try {
 
-			checkAndPotentiallyWaitForLock(name, connection);
+			checkAndPotentiallyWaitUntilUnlocked(name, connection);
 			return callback.doWithConnection(connection);
 		} finally {
 			connection.close();
@@ -175,14 +207,14 @@ private <T> T executeWithoutLockCheck(ConnectionCallback<T> callback) {
 	}
 
 	public boolean isLockingCacheWriter() {
-		return !lockTimeout.isZero() && !lockTimeout.isNegative();
+		return !sleepTime.isZero() && !sleepTime.isNegative();
 	}
 
-	private void checkAndPotentiallyWaitForLock(String name, RedisConnection connection) {
+	private void checkAndPotentiallyWaitUntilUnlocked(String name, RedisConnection connection) {
 
 		if (isLockingCacheWriter()) {
 
-			long timeout = lockTimeout.toMillis();
+			long timeout = sleepTime.toMillis();
 
 			while (doCheckLock(name, connection)) {
 				try {

src/main/java/org/springframework/data/redis/cache/NRedisCacheManager.java

@@ -7,46 +7,71 @@
 import java.util.Map;
 
 import org.springframework.cache.Cache;
-import org.springframework.cache.support.AbstractCacheManager;
+import org.springframework.cache.transaction.AbstractTransactionSupportingCacheManager;
+import org.springframework.data.redis.connection.RedisConnectionFactory;
 import org.springframework.util.Assert;
 
 /**
  * @author Christoph Strobl
  * @since 2017/05
  */
-public class NRedisCacheManager extends AbstractCacheManager {
+public class NRedisCacheManager extends AbstractTransactionSupportingCacheManager {
 
-	final DefaultRedisCacheWriter cacheWriter;
-	final RedisCacheConfiguration defaultCacheOptions;
+	final RedisCacheWriter cacheWriter;
+	final RedisCacheConfiguration defaultCacheConfig;
 	final Map<String, RedisCacheConfiguration> intialCacheConfiguration;
 
-	public NRedisCacheManager(DefaultRedisCacheWriter cacheWriter, RedisCacheConfiguration defaultCacheConfiguration,
-							  String... initialCacheNames) {
+	public NRedisCacheManager(RedisCacheWriter cacheWriter, RedisCacheConfiguration defaultCacheConfiguration,
+			String... initialCacheNames) {
 
 		Assert.notNull(cacheWriter, "CacheWriter must not be null!");
 		Assert.notNull(defaultCacheConfiguration, "DefaultCacheConfiguration must not be null!");
 
 		this.cacheWriter = cacheWriter;
-		this.defaultCacheOptions = defaultCacheConfiguration;
+		this.defaultCacheConfig = defaultCacheConfiguration;
 		this.intialCacheConfiguration = new LinkedHashMap<>(initialCacheNames.length, 1);
 
 		for (String cacheName : initialCacheNames) {
 			this.intialCacheConfiguration.put(cacheName, defaultCacheConfiguration);
 		}
 	}
 
-	public NRedisCacheManager(DefaultRedisCacheWriter cacheWriter, RedisCacheConfiguration defaultCacheConfiguration,
-							  Map<String, RedisCacheConfiguration> initialCacheConfigurations) {
+	public NRedisCacheManager(RedisCacheWriter cacheWriter, RedisCacheConfiguration defaultCacheConfiguration,
+			Map<String, RedisCacheConfiguration> initialCacheConfigurations) {
 
 		Assert.notNull(cacheWriter, "CacheWriter must not be null!");
 		Assert.notNull(defaultCacheConfiguration, "DefaultCacheConfiguration must not be null!");
 		Assert.notNull(initialCacheConfigurations, "InitialCacheConfigurations must not be null!");
 
 		this.cacheWriter = cacheWriter;
-		this.defaultCacheOptions = defaultCacheConfiguration;
+		this.defaultCacheConfig = defaultCacheConfiguration;
 		this.intialCacheConfiguration = new LinkedHashMap<>(initialCacheConfigurations);
 	}
 
+	/**
+	 * Create a new {@link NRedisCacheManager} with defaults applied.
+	 * <dl>
+	 * <dt>locking</dt>
+	 * <dd>disabled</dd>
+	 * <dt>cache configuration</dt>
+	 * <dd>{@link RedisCacheConfiguration#defaultCacheConfig()}</dd>
+	 * <dt>initial caches</dt>
+	 * <dd>none</dd>
+	 * <dt>transaction aware</dt>
+	 * <dd>no</dd>
+	 * </dl>
+	 *
+	 * @param connectionFactory must not be {@literal null}.
+	 * @return
+	 */
+	public static NRedisCacheManager iAmFineWithTheDefaults(RedisConnectionFactory connectionFactory) {
+
+		Assert.notNull(connectionFactory, "ConnectionFactory must not be null!");
+
+		return new NRedisCacheManager(new DefaultRedisCacheWriter(connectionFactory),
+				RedisCacheConfiguration.defaultCacheConfig());
+	}
+
 	@Override
 	protected Collection<NRedisCache> loadCaches() {
 
@@ -59,10 +84,10 @@ protected Collection<NRedisCache> loadCaches() {
 
 	@Override
 	protected Cache getMissingCache(String name) {
-		return createRedisCache(name, defaultCacheOptions);
+		return createRedisCache(name, defaultCacheConfig);
 	}
 
 	NRedisCache createRedisCache(String name, RedisCacheConfiguration cacheConfig) {
-		return new NRedisCache(name, cacheWriter, cacheConfig);
+		return new NRedisCache(name, cacheWriter, cacheConfig != null ? cacheConfig : defaultCacheConfig);
 	}
 }

src/main/java/org/springframework/data/redis/cache/RedisCacheConfiguration.java

@@ -56,8 +56,8 @@ private RedisCacheConfiguration(Duration ttl, Boolean cacheNullValues, Boolean u
 	/**
 	 * Default {@link RedisCacheConfiguration} using the following:
 	 * <dl>
-	 * <dt>entry timeout</dt>
-	 * <dd>0 ms</dd>
+	 * <dt>key expiration</dt>
+	 * <dd>eternal</dd>
 	 * <dt>cache null values</dt>
 	 * <dd>yes</dd>
 	 * <dt>prefix cache keys</dt>
@@ -85,6 +85,8 @@ public static RedisCacheConfiguration defaultCacheConfig() {
 	 * @return new {@link RedisCacheConfiguration}.
 	 */
 	public RedisCacheConfiguration entryTimeout(Duration timeout) {
+
+		Assert.notNull(timeout, "Timeout must not be null!");
 		return new RedisCacheConfiguration(timeout, cacheNullValues, usePrefix, defaultPrefix, keySerializationPair,
 				valueSerializationPair);
 	}
@@ -165,15 +167,15 @@ boolean getAllowCacheNullValues() {
 		return cacheNullValues != null ? cacheNullValues.booleanValue() : true;
 	}
 
-	public SerializationPair<String> getKeySerializationPair() {
+	SerializationPair<String> getKeySerializationPair() {
 		return keySerializationPair;
 	}
 
-	public SerializationPair getValueSerializationPair() {
+	SerializationPair getValueSerializationPair() {
 		return valueSerializationPair;
 	}
 
-	public Duration getTimeout() {
+	Duration getTimeout() {
 		return timeout != null ? timeout : Duration.ZERO;
 	}
 

src/test/java/org/springframework/data/redis/cache/CacheTestParams.java

@@ -0,0 +1,101 @@
+/*
+ * Copyright 2017 the original author or authors.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.springframework.data.redis.cache;
+
+import lombok.RequiredArgsConstructor;
+import lombok.experimental.Delegate;
+
+import java.util.Arrays;
+import java.util.Collection;
+import java.util.stream.Collectors;
+
+import org.springframework.data.redis.SettingsUtils;
+import org.springframework.data.redis.connection.RedisConnectionFactory;
+import org.springframework.data.redis.connection.jedis.JedisConnectionFactory;
+import org.springframework.data.redis.connection.lettuce.LettuceConnectionFactory;
+import org.springframework.data.redis.serializer.GenericJackson2JsonRedisSerializer;
+import org.springframework.data.redis.serializer.JdkSerializationRedisSerializer;
+import org.springframework.data.redis.serializer.OxmSerializer;
+import org.springframework.data.redis.serializer.RedisSerializer;
+import org.springframework.oxm.xstream.XStreamMarshaller;
+
+/**
+ * @author Christoph Strobl
+ */
+class CacheTestParams {
+
+	private static Collection<RedisConnectionFactory> connectionFactories() {
+
+		JedisConnectionFactory jedisConnectionFactory = new JedisConnectionFactory();
+		jedisConnectionFactory.setPort(SettingsUtils.getPort());
+		jedisConnectionFactory.setHostName(SettingsUtils.getHost());
+		jedisConnectionFactory.afterPropertiesSet();
+
+		LettuceConnectionFactory lettuceConnectionFactory = new LettuceConnectionFactory();
+		lettuceConnectionFactory.setPort(SettingsUtils.getPort());
+		lettuceConnectionFactory.setHostName(SettingsUtils.getHost());
+		lettuceConnectionFactory.afterPropertiesSet();
+
+		return Arrays.asList(new FixDamnedJunitParameterizedNameForConnectionFactory(jedisConnectionFactory),
+				new FixDamnedJunitParameterizedNameForConnectionFactory(lettuceConnectionFactory));
+	}
+
+	static Collection<Object[]> justConnectionFactories() {
+		return connectionFactories().stream().map(factory -> new Object[] { factory }).collect(Collectors.toList());
+	}
+
+	static Collection<Object[]> connectionFactoriesAndSerializers() {
+
+		// XStream serializer
+		XStreamMarshaller xstream = new XStreamMarshaller();
+		xstream.afterPropertiesSet();
+
+		OxmSerializer oxmSerializer = new OxmSerializer(xstream, xstream);
+		GenericJackson2JsonRedisSerializer jackson2Serializer = new GenericJackson2JsonRedisSerializer();
+		JdkSerializationRedisSerializer jdkSerializer = new JdkSerializationRedisSerializer();
+
+		return connectionFactories()
+				.stream().flatMap(factory -> Arrays
+						.asList( //
+								new Object[] { factory, new FixDamnedJunitParameterizedNameForRedisSerializer(jdkSerializer) }, //
+								new Object[] { factory, new FixDamnedJunitParameterizedNameForRedisSerializer(jackson2Serializer) }, //
+								new Object[] { factory, new FixDamnedJunitParameterizedNameForRedisSerializer(oxmSerializer) })
+						.stream())
+				.collect(Collectors.toList());
+	}
+
+	@RequiredArgsConstructor
+	static class FixDamnedJunitParameterizedNameForConnectionFactory/* ¯\_(ツ)_/¯ */ implements RedisConnectionFactory {
+
+		final @Delegate RedisConnectionFactory connectionFactory;
+
+		@Override // Why Junit? Why?
+		public String toString() {
+			return connectionFactory.getClass().getSimpleName();
+		}
+	}
+
+	@RequiredArgsConstructor
+	static class FixDamnedJunitParameterizedNameForRedisSerializer/* ¯\_(ツ)_/¯ */ implements RedisSerializer {
+
+		final @Delegate RedisSerializer serializer;
+
+		@Override // Why Junit? Why?
+		public String toString() {
+			return serializer.getClass().getSimpleName();
+		}
+	}
+}