Reliable null value handling in ConcurrentMapCache, GuavaCache, JCacheCache

The 4.2 variant of this fix includes a common AbstractValueAdaptingCache base class and a common NullValue holder class.

Issue: SPR-13553

spring-context/src/main/java/org/springframework/cache/concurrent/ConcurrentMapCache.java

@@ -16,17 +16,15 @@
 
 package org.springframework.cache.concurrent;
 
-import java.io.Serializable;
 import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.ConcurrentMap;
 
-import org.springframework.cache.Cache;
-import org.springframework.cache.support.SimpleValueWrapper;
+import org.springframework.cache.support.AbstractValueAdaptingCache;
 import org.springframework.util.Assert;
 
 /**
- * Simple {@link Cache} implementation based on the core JDK
- * {@code java.util.concurrent} package.
+ * Simple {@link org.springframework.cache.Cache} implementation based on the
+ * core JDK {@code java.util.concurrent} package.
  *
  * <p>Useful for testing or simple caching scenarios, typically in combination
  * with {@link org.springframework.cache.support.SimpleCacheManager} or
@@ -41,16 +39,12 @@ import org.springframework.util.Assert;
  * @author Juergen Hoeller
  * @since 3.1
  */
-public class ConcurrentMapCache implements Cache {
-
-	private static final Object NULL_HOLDER = new NullHolder();
+public class ConcurrentMapCache extends AbstractValueAdaptingCache {
 
 	private final String name;
 
 	private final ConcurrentMap<Object, Object> store;
 
-	private final boolean allowNullValues;
-
 
 	/**
 	 * Create a new ConcurrentMapCache with the specified name.
@@ -79,11 +73,11 @@ public class ConcurrentMapCache implements Cache {
 	 * (adapting them to an internal null holder value)
 	 */
 	public ConcurrentMapCache(String name, ConcurrentMap<Object, Object> store, boolean allowNullValues) {
+		super(allowNullValues);
 		Assert.notNull(name, "Name must not be null");
 		Assert.notNull(store, "Store must not be null");
 		this.name = name;
 		this.store = store;
-		this.allowNullValues = allowNullValues;
 	}
 
 
@@ -97,24 +91,9 @@ public class ConcurrentMapCache implements Cache {
 		return this.store;
 	}
 
-	public final boolean isAllowNullValues() {
-		return this.allowNullValues;
-	}
-
 	@Override
-	public ValueWrapper get(Object key) {
-		Object value = this.store.get(key);
-		return toWrapper(value);
-	}
-
-	@Override
-	@SuppressWarnings("unchecked")
-	public <T> T get(Object key, Class<T> type) {
-		Object value = fromStoreValue(this.store.get(key));
-		if (value != null && type != null && !type.isInstance(value)) {
-			throw new IllegalStateException("Cached value is not of required type [" + type.getName() + "]: " + value);
-		}
-		return (T) value;
+	protected Object lookup(Object key) {
+		return this.store.get(key);
 	}
 
 	@Override
@@ -125,7 +104,7 @@ public class ConcurrentMapCache implements Cache {
 	@Override
 	public ValueWrapper putIfAbsent(Object key, Object value) {
 		Object existing = this.store.putIfAbsent(key, toStoreValue(value));
-		return toWrapper(existing);
+		return toValueWrapper(existing);
 	}
 
 	@Override
@@ -138,40 +117,4 @@ public class ConcurrentMapCache implements Cache {
 		this.store.clear();
 	}
 
-
-	/**
-	 * Convert the given value from the internal store to a user value
-	 * returned from the get method (adapting {@code null}).
-	 * @param storeValue the store value
-	 * @return the value to return to the user
-	 */
-	protected Object fromStoreValue(Object storeValue) {
-		if (this.allowNullValues && storeValue == NULL_HOLDER) {
-			return null;
-		}
-		return storeValue;
-	}
-
-	/**
-	 * Convert the given user value, as passed into the put method,
-	 * to a value in the internal store (adapting {@code null}).
-	 * @param userValue the given user value
-	 * @return the value to store
-	 */
-	protected Object toStoreValue(Object userValue) {
-		if (this.allowNullValues && userValue == null) {
-			return NULL_HOLDER;
-		}
-		return userValue;
-	}
-
-	private ValueWrapper toWrapper(Object value) {
-		return (value != null ? new SimpleValueWrapper(fromStoreValue(value)) : null);
-	}
-
-
-	@SuppressWarnings("serial")
-	private static class NullHolder implements Serializable {
-	}
-
 }

spring-context/src/main/java/org/springframework/cache/support/AbstractValueAdaptingCache.java

@@ -0,0 +1,116 @@
+/*
+ * Copyright 2002-2015 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.cache.support;
+
+import org.springframework.cache.Cache;
+
+/**
+ * Common base class for {@link Cache} implementations that need to adapt
+ * {@code null} values (and potentially other such special values) before
+ * passing them on to the underlying store.
+ *
+ * <p>Transparently replaces given {@code null} user values with an internal
+ * {@link NullValue#INSTANCE}, if configured to support {@code null} values
+ * (as indicated by {@link #isAllowNullValues()}.
+ *
+ * @author Juergen Hoeller
+ * @since 4.2.2
+ */
+public abstract class AbstractValueAdaptingCache implements Cache {
+
+	private final boolean allowNullValues;
+
+
+	/**
+	 * Create an {@code AbstractValueAdaptingCache} with the given setting.
+	 * @param allowNullValues whether to allow for {@code null} values
+	 */
+	protected AbstractValueAdaptingCache(boolean allowNullValues) {
+		this.allowNullValues = allowNullValues;
+	}
+
+
+	/**
+	 * Return whether {@code null} values are allowed in this cache.
+	 */
+	public final boolean isAllowNullValues() {
+		return this.allowNullValues;
+	}
+
+	@Override
+	public ValueWrapper get(Object key) {
+		Object value = lookup(key);
+		return toValueWrapper(value);
+	}
+
+	@Override
+	@SuppressWarnings("unchecked")
+	public <T> T get(Object key, Class<T> type) {
+		Object value = fromStoreValue(lookup(key));
+		if (value != null && type != null && !type.isInstance(value)) {
+			throw new IllegalStateException("Cached value is not of required type [" + type.getName() + "]: " + value);
+		}
+		return (T) value;
+	}
+
+	/**
+	 * Perform an actual lookup in the underlying store.
+	 * @param key the key whose associated value is to be returned
+	 * @return the raw store value for the key
+	 */
+	protected abstract Object lookup(Object key);
+
+
+	/**
+	 * Convert the given value from the internal store to a user value
+	 * returned from the get method (adapting {@code null}).
+	 * @param storeValue the store value
+	 * @return the value to return to the user
+	 */
+	protected Object fromStoreValue(Object storeValue) {
+		if (this.allowNullValues && storeValue == NullValue.INSTANCE) {
+			return null;
+		}
+		return storeValue;
+	}
+
+	/**
+	 * Convert the given user value, as passed into the put method,
+	 * to a value in the internal store (adapting {@code null}).
+	 * @param userValue the given user value
+	 * @return the value to store
+	 */
+	protected Object toStoreValue(Object userValue) {
+		if (this.allowNullValues && userValue == null) {
+			return NullValue.INSTANCE;
+		}
+		return userValue;
+	}
+
+	/**
+	 * Wrap the given store value with a {@link SimpleValueWrapper}, also going
+	 * through {@link #fromStoreValue} conversion. Useful for {@link #get(Object)}
+	 * and {@link #putIfAbsent(Object, Object)} implementations.
+	 * @param storeValue the original value
+	 * @return the wrapped value
+	 */
+	protected Cache.ValueWrapper toValueWrapper(Object storeValue) {
+		return (storeValue != null ? new SimpleValueWrapper(fromStoreValue(storeValue)) : null);
+	}
+
+
+}

spring-context/src/main/java/org/springframework/cache/support/NullValue.java

@@ -0,0 +1,43 @@
+/*
+ * Copyright 2002-2015 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.cache.support;
+
+import java.io.Serializable;
+
+/**
+ * Simple serializable class that serves as a {@code null} replacement
+ * for cache stores which otherwise do not support {@code null} values.
+ *
+ * @author Juergen Hoeller
+ * @since 4.2.2
+ * @see AbstractValueAdaptingCache
+ */
+public final class NullValue implements Serializable {
+
+	static final Object INSTANCE = new NullValue();
+
+	private static final long serialVersionUID = 1L;
+
+
+	private NullValue() {
+	}
+
+	private Object readResolve() {
+		return INSTANCE;
+	}
+
+}