SPR-7308

SPR-7736
+ clarify storage of null values

org.springframework.context.support/src/main/java/org/springframework/cache/Cache.java

@@ -18,7 +18,11 @@ package org.springframework.cache;
 
 
 /**
- * Interface that defines the common cache operations. 
+ * Interface that defines the common cache operations.
+ * 
+ * <b>Note:</b> Due to the generic use of caching, it is recommended that 
+ * implementations allow storage of <tt>null</tt> values (for example to 
+ * cache methods that return null).
  * 
  * @author Costin Leau
  */

org.springframework.context.support/src/main/java/org/springframework/cache/concurrent/ConcurrentCache.java

@@ -22,9 +22,9 @@ import java.util.concurrent.ConcurrentMap;
 import org.springframework.cache.Cache;
 import org.springframework.cache.support.AbstractDelegatingCache;
 
-/** 
+/**
- * Simple {@link Cache} implementation based on the JDK 1.5+ java.util.concurrent package.
+ * Simple {@link Cache} implementation based on the JDK 1.5+
- * Useful for testing or simple caching scenarios.
+ * java.util.concurrent package. Useful for testing or simple caching scenarios.
  * 
  * @author Costin Leau
  */
@@ -42,7 +42,7 @@ public class ConcurrentCache<K, V> extends AbstractDelegatingCache<K, V> {
 	}
 
 	public ConcurrentCache(ConcurrentMap<K, V> delegate, String name) {
-		super(delegate);
+		super(delegate, true);
 		this.store = delegate;
 		this.name = name;
 	}
@@ -55,19 +55,51 @@ public class ConcurrentCache<K, V> extends AbstractDelegatingCache<K, V> {
 		return store;
 	}
 
+	@SuppressWarnings("unchecked")
 	public V putIfAbsent(K key, V value) {
-		return store.putIfAbsent(key, value);
+		if (getAllowNullValues()) {
+			if (value == null) {
+				ConcurrentMap raw = store;
+				return filterNull((V) raw.putIfAbsent(key, NULL_HOLDER));
+			}
+		}
+		return filterNull(store.putIfAbsent(key, value));
 	}
 
+	@SuppressWarnings("unchecked")
 	public boolean remove(Object key, Object value) {
+		if (getAllowNullValues()) {
+			if (value == null) {
+				ConcurrentMap raw = store;
+				return raw.remove(key, NULL_HOLDER);
+			}
+		}
+
 		return store.remove(key, value);
 	}
 
+	@SuppressWarnings("unchecked")
 	public boolean replace(K key, V oldValue, V newValue) {
+		if (getAllowNullValues()) {
+			Object rawOldValue = (oldValue == null ? NULL_HOLDER : oldValue);
+			Object rawNewValue = (newValue == null ? NULL_HOLDER : newValue);
+
+			ConcurrentMap raw = store;
+			return raw.replace(key, rawOldValue, rawNewValue);
+		}
+
 		return store.replace(key, oldValue, newValue);
 	}
 
+	@SuppressWarnings("unchecked")
 	public V replace(K key, V value) {
-		return store.replace(key, value);
+		if (getAllowNullValues()) {
+			if (value == null) {
+				ConcurrentMap raw = store;
+				return filterNull((V) raw.replace(key, NULL_HOLDER));
+			}
+		}
+
+		return filterNull(store.replace(key, value));
 	}
 }

org.springframework.context.support/src/main/java/org/springframework/cache/interceptor/CacheAspectSupport.java

@@ -16,7 +16,6 @@
 
 package org.springframework.cache.interceptor;
 
-import java.io.Serializable;
 import java.lang.reflect.Method;
 import java.util.ArrayList;
 import java.util.Collection;
@@ -61,14 +60,6 @@ import org.springframework.util.StringUtils;
  */
 public abstract class CacheAspectSupport implements InitializingBean {
 
-	private static class EmptyHolder implements Serializable {
-
-		private static final long serialVersionUID = 1L;
-	}
-
-	// TODO: can null values be properly stored into user caches?
-	private static final Object NULL_RETURN = new EmptyHolder();
-
 	protected final Log logger = LogFactory.getLog(getClass());
 
 	private CacheManager cacheManager;
@@ -124,7 +115,6 @@ public abstract class CacheAspectSupport implements InitializingBean {
 	}
 
 	protected Collection<Cache<?, ?>> getCaches(CacheDefinition definition) {
-		// TODO: add behaviour for the default cache
 		Set<String> cacheNames = definition.getCacheNames();
 
 		Collection<Cache<?,?>> caches = new ArrayList<Cache<?,?>>(cacheNames.size());

org.springframework.context.support/src/main/java/org/springframework/cache/support/AbstractDelegatingCache.java

@@ -16,6 +16,7 @@
 
 package org.springframework.cache.support;
 
+import java.io.Serializable;
 import java.util.Map;
 
 import org.springframework.cache.Cache;
@@ -25,15 +26,47 @@ import org.springframework.util.Assert;
  * Abstract base class delegating most of the {@link Map}-like methods
  * to the underlying cache.
  * 
+ * <b>Note:</b>Allows null values to be stored, even if the underlying map
+ * does not support them.
+ * 
  * @author Costin Leau
  */
 public abstract class AbstractDelegatingCache<K, V> implements Cache<K, V> {
 
-	private final Map<K, V> delegate;
+	private static class NullHolder implements Serializable {
+		private static final long serialVersionUID = 1L;
+	}
 
+	public static final Object NULL_HOLDER = new NullHolder();
+
+	private final Map<K, V> delegate;
+	private final boolean allowNullValues;
+
+	/**
+	 * Creates a new instance using the given delegate.
+	 * 
+	 * @param <D> map type
+	 * @param delegate map delegate
+	 */
 	public <D extends Map<K, V>> AbstractDelegatingCache(D delegate) {
+		this(delegate, false);
+	}
+
+	/**
+	 * Creates a new instance using the given delegate.
+	 * 
+	 * @param <D> map type
+	 * @param delegate map delegate
+	 * @param allowNullValues flag indicating whether null values are allowed or not
+	 */
+	public <D extends Map<K, V>> AbstractDelegatingCache(D delegate, boolean allowNullValues) {
 		Assert.notNull(delegate);
 		this.delegate = delegate;
+		this.allowNullValues = allowNullValues;
+	}
+
+	public boolean getAllowNullValues() {
+		return allowNullValues;
 	}
 
 	public void clear() {
@@ -45,14 +78,31 @@ public abstract class AbstractDelegatingCache<K, V> implements Cache<K, V> {
 	}
 
 	public V get(Object key) {
-		return delegate.get(key);
+		return filterNull(delegate.get(key));
 	}
 
+	@SuppressWarnings("unchecked")
 	public V put(K key, V value) {
-		return delegate.put(key, value);
+		if (allowNullValues && value == null) {
+			Map map = delegate;
+			Object val = map.put(key, NULL_HOLDER);
+			if (val == NULL_HOLDER) {
+				return null;
+			}
+			return (V) val;
+		}
+
+		return filterNull(delegate.put(key, value));
 	}
 
 	public V remove(Object key) {
-		return delegate.remove(key);
+		return filterNull(delegate.remove(key));
+	}
+
+	protected V filterNull(V val) {
+		if (allowNullValues && val == NULL_HOLDER) {
+			return null;
+		}
+		return val;
 	}
 }

org.springframework.context.support/src/test/java/org/springframework/cache/config/AbstractAnnotationTest.java

@@ -16,8 +16,7 @@
 
 package org.springframework.cache.config;
 
-import static org.junit.Assert.assertNotSame;
+import static org.junit.Assert.*;
-import static org.junit.Assert.assertSame;
 
 import org.junit.Before;
 import org.junit.Test;
@@ -26,6 +25,7 @@ import org.springframework.context.support.ClassPathXmlApplicationContext;
 
 /**
  * Abstract annotation test (containing several reusable methods).
+ * 
  * @author Costin Leau
  */
 public abstract class AbstractAnnotationTest {
@@ -72,7 +72,8 @@ public abstract class AbstractAnnotationTest {
 		assertSame(r3, r4);
 	}
 
-	public void testConditionalExpression(CacheableService service) throws Exception {
+	public void testConditionalExpression(CacheableService service)
+			throws Exception {
 		Object r1 = service.conditional(4);
 		Object r2 = service.conditional(4);
 
@@ -96,6 +97,16 @@ public abstract class AbstractAnnotationTest {
 		assertNotSame(r3, r4);
 	}
 
+	public void testNullValue(CacheableService service) throws Exception {
+		Object key = new Object();
+		assertNull(service.nullValue(key));
+		int nr = service.nullInvocations().intValue();
+		assertNull(service.nullValue(key));
+		assertEquals(nr, service.nullInvocations().intValue());
+		assertNull(service.nullValue(new Object()));
+		assertEquals(nr + 1, service.nullInvocations().intValue());
+	}
+
 	@Test
 	public void testCacheable() throws Exception {
 		testCacheable(cs);
@@ -126,4 +137,22 @@ public abstract class AbstractAnnotationTest {
 		testInvalidate(ccs);
 	}
 
+	@Test
+	public void testNullValue() throws Exception {
+		testNullValue(cs);
+	}
+
+	@Test
+	public void testClassNullValue() throws Exception {
+		Object key = new Object();
+		assertNull(ccs.nullValue(key));
+		int nr = ccs.nullInvocations().intValue();
+		assertNull(ccs.nullValue(key));
+		assertEquals(nr, ccs.nullInvocations().intValue());
+		assertNull(ccs.nullValue(new Object()));
+		// the check method is also cached
+		assertEquals(nr, ccs.nullInvocations().intValue());
+		assertEquals(nr + 1, AnnotatedClassCacheableService.nullInvocations
+				.intValue());
+	}
 }

org.springframework.context.support/src/test/java/org/springframework/cache/config/AnnotatedClassCacheableService.java

@@ -27,7 +27,8 @@ import org.springframework.cache.annotation.Cacheable;
 @Cacheable("default")
 public class AnnotatedClassCacheableService implements CacheableService {
 
-	private AtomicLong counter = new AtomicLong();
+	private final AtomicLong counter = new AtomicLong();
+	public static final AtomicLong nullInvocations = new AtomicLong();
 
 	public Object cache(Object arg1) {
 		return counter.getAndIncrement();
@@ -45,4 +46,13 @@ public class AnnotatedClassCacheableService implements CacheableService {
 	public Object key(Object arg1, Object arg2) {
 		return counter.getAndIncrement();
 	}
+
+	public Object nullValue(Object arg1) {
+		nullInvocations.incrementAndGet();
+		return null;
+	}
+
+	public Number nullInvocations() {
+		return nullInvocations.get();
+	}
 }

org.springframework.context.support/src/test/java/org/springframework/cache/config/CacheableService.java

@@ -16,6 +16,7 @@
 
 package org.springframework.cache.config;
 
+
 /**
  * Basic service interface.
  * 
@@ -31,4 +32,8 @@ public interface CacheableService<T> {
 
 	T key(Object arg1, Object arg2);
 
+	T nullValue(Object arg1);
+
+	Number nullInvocations();
+
 }

org.springframework.context.support/src/test/java/org/springframework/cache/config/DefaultCacheableService.java

@@ -21,7 +21,6 @@ import java.util.concurrent.atomic.AtomicLong;
 import org.springframework.cache.annotation.CacheEvict;
 import org.springframework.cache.annotation.Cacheable;
 
-
 /**
  * Simple cacheable service
  * 
@@ -29,7 +28,8 @@ import org.springframework.cache.annotation.Cacheable;
  */
 public class DefaultCacheableService implements CacheableService<Long> {
 
-	private AtomicLong counter = new AtomicLong();
+	private final AtomicLong counter = new AtomicLong();
+	private final AtomicLong nullInvocations = new AtomicLong();
 
 	@Cacheable("default")
 	public Long cache(Object arg1) {
@@ -49,4 +49,14 @@ public class DefaultCacheableService implements CacheableService<Long> {
 	public Long key(Object arg1, Object arg2) {
 		return counter.getAndIncrement();
 	}
+
+	@Cacheable("default")
+	public Long nullValue(Object arg1) {
+		nullInvocations.incrementAndGet();
+		return null;
+	}
+
+	public Number nullInvocations() {
+		return nullInvocations.get();
+	}
 }