Merge pull request #719 from surevine/of1156

OF-1156 Warn (and possibly die) on null storage in DefaultCache

src/java/org/jivesoftware/util/cache/Cache.java

@@ -30,7 +30,7 @@ package org.jivesoftware.util.cache;
  *
  * If the cache does grow too large, objects will be removed such that those
  * that are accessed least frequently are removed first. Because expiration
- * happens automatically, the cache makes <b>no</b> gaurantee as to how long
+ * happens automatically, the cache makes <b>no</b> guarantee as to how long
  * an object will remain in cache after it is put in.<p>
  *
  * Optionally, a maximum lifetime for all objects can be specified. In that
@@ -41,6 +41,9 @@ package org.jivesoftware.util.cache;
  *
  * All cache operations are thread safe.<p>
  *
+ * Note that neither keys or values can be null; A {@link NullPointerException}
+ * will be thrown attempting to place or retrieve null values in to the cache.
+ *
  * @see Cacheable
  */
 public interface Cache<K,V> extends java.util.Map<K,V> {

src/java/org/jivesoftware/util/cache/DefaultCache.java

@@ -28,6 +28,7 @@ import java.util.Map;
 import java.util.NoSuchElementException;
 import java.util.Set;
 
+import org.jivesoftware.util.JiveGlobals;
 import org.jivesoftware.util.LinkedListNode;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
@@ -59,7 +60,11 @@ import org.slf4j.LoggerFactory;
  */
 public class DefaultCache<K, V> implements Cache<K, V> {
 
-	private static final Logger Log = LoggerFactory.getLogger(DefaultCache.class);
+    private static final String NULL_KEY_IS_NOT_ALLOWED = "Null key is not allowed!";
+    private static final String NULL_VALUE_IS_NOT_ALLOWED = "Null value is not allowed!";
+    private static final boolean allowNull = JiveGlobals.getBooleanProperty("cache.allow.null", true);
+
+    private static final Logger Log = LoggerFactory.getLogger(DefaultCache.class);
 
     /**
      * The map the keys and values are stored in.
@@ -133,6 +138,8 @@ public class DefaultCache<K, V> implements Cache<K, V> {
 
     @Override
     public synchronized V put(K key, V value) {
+        checkNotNull(key, NULL_KEY_IS_NOT_ALLOWED);
+        checkNotNull(value, NULL_VALUE_IS_NOT_ALLOWED);
         // Delete an old entry if it exists.
         V answer = remove(key);
 
@@ -174,6 +181,7 @@ public class DefaultCache<K, V> implements Cache<K, V> {
 
     @Override
     public synchronized V get(Object key) {
+        checkNotNull(key, NULL_KEY_IS_NOT_ALLOWED);
         // First, clear all entries that have been in cache longer than the
         // maximum defined age.
         deleteExpiredEntries();
@@ -200,6 +208,7 @@ public class DefaultCache<K, V> implements Cache<K, V> {
 
     @Override
     public synchronized V remove(Object key) {
+        checkNotNull(key, NULL_KEY_IS_NOT_ALLOWED);
         DefaultCache.CacheObject<V> cacheObject = map.get(key);
         // If the object is not in cache, stop trying to remove it.
         if (cacheObject == null) {
@@ -285,6 +294,7 @@ public class DefaultCache<K, V> implements Cache<K, V> {
 
         @Override
         public boolean contains(Object o) {
+            checkNotNull(o, NULL_KEY_IS_NOT_ALLOWED);
             Iterator<V> it = iterator();
             while (it.hasNext()) {
                 if (it.next().equals(o)) {
@@ -391,6 +401,7 @@ public class DefaultCache<K, V> implements Cache<K, V> {
 
     @Override
     public boolean containsKey(Object key) {
+        checkNotNull(key, NULL_KEY_IS_NOT_ALLOWED);
         // First, clear all entries that have been in cache longer than the
         // maximum defined age.
         deleteExpiredEntries();
@@ -409,14 +420,11 @@ public class DefaultCache<K, V> implements Cache<K, V> {
 
     @Override
     public boolean containsValue(Object value) {
+        checkNotNull(value, NULL_VALUE_IS_NOT_ALLOWED);
         // First, clear all entries that have been in cache longer than the
         // maximum defined age.
         deleteExpiredEntries();
 
-        if(value == null) {
-            return containsNullValue();
-        }
-
         Iterator it = values().iterator();
         while(it.hasNext()) {
             if(value.equals(it.next())) {
@@ -426,16 +434,6 @@ public class DefaultCache<K, V> implements Cache<K, V> {
         return false;
     }
 
-    private boolean containsNullValue() {
-        Iterator it = values().iterator();
-        while(it.hasNext()) {
-            if(it.next() == null) {
-                return true;
-            }
-        }
-        return false;
-    }
-
     @Override
     public Set<Entry<K, V>> entrySet() {
         // First, clear all entries that have been in cache longer than the
@@ -703,4 +701,18 @@ public class DefaultCache<K, V> implements Cache<K, V> {
             this.size = size;
         }
     }
+
+    private void checkNotNull(final Object argument, final String message) {
+        try {
+            if (argument == null) {
+                throw new NullPointerException(message);
+            }
+        } catch (NullPointerException e) {
+            if (allowNull) {
+                Log.error("Ignoring null store in Cache: ", e); // Gives us a trace for debugging.
+            } else {
+                throw e;
+            }
+        }
+    }
 }