fixed CachingMapDecorator to support garbage-collected weak references (again)

git-svn-id: https://src.springframework.org/svn/spring-framework/trunk@798 50f2f4bb-b051-0410-bef5-90022cba6387

org.springframework.core/src/main/java/org/springframework/util/CachingMapDecorator.java

@@ -22,6 +22,7 @@ import java.lang.ref.WeakReference;
 import java.util.Collection;
 import java.util.Collections;
 import java.util.HashMap;
+import java.util.Iterator;
 import java.util.LinkedHashMap;
 import java.util.LinkedHashSet;
 import java.util.LinkedList;
@@ -33,7 +34,7 @@ import java.util.WeakHashMap;
  * A simple decorator for a Map, encapsulating the workflow for caching
  * expensive values in a target Map. Supports caching weak or strong keys.
  *
- * <p>This class is also an abstract template. Caching Map implementations
+ * <p>This class is an abstract template. Caching Map implementations
  * should subclass and override the <code>create(key)</code> method which
  * encapsulates expensive creation of a new object.
  * 
@@ -41,9 +42,9 @@ import java.util.WeakHashMap;
  * @author Juergen Hoeller
  * @since 1.2.2
  */
-public class CachingMapDecorator<K, V> implements Map<K, V>, Serializable {
+public abstract class CachingMapDecorator<K, V> implements Map<K, V>, Serializable {
 
-	protected static Object NULL_VALUE = new Object();
+	private static Object NULL_VALUE = new Object();
 
 
 	private final Map<K, Object> targetMap;
@@ -150,7 +151,16 @@ public class CachingMapDecorator<K, V> implements Map<K, V>, Serializable {
 	}
 
 	public V remove(Object key) {
-		return unwrapIfNecessary(this.targetMap.remove(key));
+		return unwrapReturnValue(this.targetMap.remove(key));
+	}
+
+	@SuppressWarnings("unchecked")
+	private V unwrapReturnValue(Object value) {
+		Object returnValue = value;
+		if (returnValue instanceof Reference) {
+			returnValue = ((Reference) returnValue).get();
+		}
+		return (returnValue == NULL_VALUE ? null : (V) returnValue);
 	}
 
 	public void putAll(Map<? extends K, ? extends V> map) {
@@ -186,8 +196,16 @@ public class CachingMapDecorator<K, V> implements Map<K, V>, Serializable {
 	@SuppressWarnings("unchecked")
 	private Collection<V> valuesCopy() {
 		LinkedList<V> values = new LinkedList<V>();
-		for (Object value : this.targetMap.values()) {
-			values.add(value instanceof Reference ? ((Reference<V>) value).get() : (V) value);
+		for (Iterator<Object> it = this.targetMap.values().iterator(); it.hasNext();) {
+			Object value = it.next();
+			if (value instanceof Reference) {
+				value = ((Reference) value).get();
+				if (value == null) {
+					it.remove();
+					continue;
+				}
+			}
+			values.add(value == NULL_VALUE ? null : (V) value);
 		}
 		return values;
 	}
@@ -203,10 +221,20 @@ public class CachingMapDecorator<K, V> implements Map<K, V>, Serializable {
 		}
 	}
 
+	@SuppressWarnings("unchecked")
 	private Set<Map.Entry<K, V>> entryCopy() {
 		Map<K,V> entries = new LinkedHashMap<K, V>();
-		for (Entry<K, Object> entry : this.targetMap.entrySet()) {
-			entries.put(entry.getKey(), unwrapIfNecessary(entry.getValue()));
+		for (Iterator<Entry<K, Object>> it = this.targetMap.entrySet().iterator(); it.hasNext();) {
+			Entry<K, Object> entry = it.next();
+			Object value = entry.getValue();
+			if (value instanceof Reference) {
+				value = ((Reference) value).get();
+				if (value == null) {
+					it.remove();
+					continue;
+				}
+			}
+			entries.put(entry.getKey(), value == NULL_VALUE ? null : (V) value);
 		}
 		return entries.entrySet();
 	}
@@ -223,13 +251,13 @@ public class CachingMapDecorator<K, V> implements Map<K, V>, Serializable {
 			newValue = NULL_VALUE;
 		}
 		else if (useWeakValue(key, value)) {
-			newValue = new WeakReference<V>(value);
+			newValue = new WeakReference<Object>(newValue);
 		}
-		return unwrapIfNecessary(this.targetMap.put(key, newValue));
+		return unwrapReturnValue(this.targetMap.put(key, newValue));
 	}
 
 	/**
-	 * Decide whether use a weak reference for the value of
+	 * Decide whether to use a weak reference for the value of
 	 * the given key-value pair.
 	 * @param key the candidate key
 	 * @param value the candidate value
@@ -252,27 +280,15 @@ public class CachingMapDecorator<K, V> implements Map<K, V>, Serializable {
 	@SuppressWarnings("unchecked")
 	public V get(Object key) {
 		Object value = this.targetMap.get(key);
-		if (value == null) {
-			V newVal = create((K) key);
-			if (newVal != null) {
-				put((K) key, newVal);
-			}
-			return newVal;
-		}
-		return unwrapIfNecessary(value);
-	}
-
-	@SuppressWarnings("unchecked")
-	private V unwrapIfNecessary(Object value) {
 		if (value instanceof Reference) {
-			return ((Reference<V>) value).get();
+			value = ((Reference) value).get();
 		}
-		else if (value != null) {
-			return (V) value;
-		}
-		else {
-			return null;
+		if (value == null) {
+			V newValue = create((K) key);
+			put((K) key, newValue);
+			return newValue;
 		}
+		return (value == NULL_VALUE ? null : (V) value);
 	}
 
 	/**
@@ -281,9 +297,7 @@ public class CachingMapDecorator<K, V> implements Map<K, V>, Serializable {
 	 * @param key the cache key
 	 * @see #get(Object)
 	 */
-	protected V create(K key) {
-		return null;
-	}
+	protected abstract V create(K key);
 
 
 	@Override

org.springframework.core/src/test/java/org/springframework/util/CachingMapDecoratorTests.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2005 the original author or authors.
+ * Copyright 2002-2009 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.
@@ -16,41 +16,91 @@
 
 package org.springframework.util;
 
+import java.util.Collection;
+import java.util.HashSet;
+import java.util.Map;
+import java.util.Set;
+
 import junit.framework.TestCase;
 
 /**
  * @author Keith Donald
+ * @author Juergen Hoeller
  */
 public class CachingMapDecoratorTests extends TestCase {
 
 	public void testValidCache() {
 		MyCachingMap cache = new MyCachingMap();
-		Object value;
 
-		value = cache.get("value key");
+		assertFalse(cache.containsKey("value key"));
+		assertFalse(cache.containsValue("expensive value to cache"));
+		Object value = cache.get("value key");
 		assertTrue(cache.createCalled());
 		assertEquals(value, "expensive value to cache");
+		assertTrue(cache.containsKey("value key"));
+		assertTrue(cache.containsValue("expensive value to cache"));
 
-		cache.get("value key 2");
+		assertFalse(cache.containsKey("value key 2"));
+		value = cache.get("value key 2");
 		assertTrue(cache.createCalled());
+		assertEquals(value, "expensive value to cache");
+		assertTrue(cache.containsKey("value key 2"));
 
 		value = cache.get("value key");
-		assertEquals(cache.createCalled(), false);
+		assertFalse(cache.createCalled());
 		assertEquals(value, "expensive value to cache");
 
 		cache.get("value key 2");
-		assertEquals(cache.createCalled(), false);
+		assertFalse(cache.createCalled());
 		assertEquals(value, "expensive value to cache");
+
+		assertFalse(cache.containsKey(null));
+		assertFalse(cache.containsValue(null));
+		value = cache.get(null);
+		assertTrue(cache.createCalled());
+		assertNull(value);
+		assertTrue(cache.containsKey(null));
+		assertTrue(cache.containsValue(null));
+
+		value = cache.get(null);
+		assertFalse(cache.createCalled());
+		assertNull(value);
+
+		Set<String> keySet = cache.keySet();
+		assertEquals(3, keySet.size());
+		assertTrue(keySet.contains("value key"));
+		assertTrue(keySet.contains("value key 2"));
+		assertTrue(keySet.contains(null));
+
+		Collection<String> values = cache.values();
+		assertEquals(3, values.size());
+		assertTrue(values.contains("expensive value to cache"));
+		assertTrue(values.contains(null));
+
+		Set<Map.Entry<String, String>> entrySet = cache.entrySet();
+		assertEquals(3, entrySet.size());
+		keySet = new HashSet<String>();
+		values = new HashSet<String>();
+		for (Map.Entry<String, String> entry : entrySet) {
+			keySet.add(entry.getKey());
+			values.add(entry.getValue());
+		}
+		assertTrue(keySet.contains("value key"));
+		assertTrue(keySet.contains("value key 2"));
+		assertTrue(keySet.contains(null));
+		assertEquals(2, values.size());
+		assertTrue(values.contains("expensive value to cache"));
+		assertTrue(values.contains(null));
 	}
 
 
-	private static class MyCachingMap extends CachingMapDecorator {
+	private static class MyCachingMap extends CachingMapDecorator<String, String> {
 
 		private boolean createCalled;
 
-		protected Object create(Object key) {
+		protected String create(String key) {
 			createCalled = true;
-			return "expensive value to cache";
+			return (key != null ? "expensive value to cache" : null);
 		}
 
 		public boolean createCalled() {