Further improvements to the EntityLookupCache for NULL values

- Supports entities that map IDs to potentially-null values
 - Fix some bugs


git-svn-id: https://svn.alfresco.com/repos/alfresco-enterprise/alfresco/HEAD/root@15555 c4b6b30b-aa2e-2d43-bbcb-ca4b014f7261

source/java/org/alfresco/repo/cache/lookup/EntityLookupCache.java

@@ -40,6 +40,14 @@ import org.alfresco.util.ParameterCheck;
  * <p>
  * All keys will be unique to the given cache region, allowing the cache to be shared
  * between instances of this class.
+ * <p>
+ * Generics:
+ * <ul>
+ *   <li>K:  The database unique identifier.</li>
+ *   <li>V:  The value stored against K.</li>
+ *   <li>VK: The a value-derived key that will be used as a cache key when caching K for lookups by V.
+ *           This can be the value itself if it is itself a good key.</li>
+ * </ul>
  * 
  * @author Derek Hulley
  * @since 3.3
@@ -53,19 +61,27 @@ public class EntityLookupCache<K extends Serializable, V extends Object, VK exte
     {
         /**
          * Resolve the given value into a unique value key that can be used to find the entity's ID.
-         * <p>
+         * A return value should be small and efficient; don't return a value if this is not possible.
+         * <p/>
          * Implementations will often return the value itself, provided that the value is both
          * serializable and has a good <code>equals</code> and <code>hashCode</code>.
+         * <p/>
+         * Were no adequate key can be generated for the value, then <tt>null</tt> can be returned.
+         * In this case, the {@link #findByValue(Object) findByValue} method might not even do a search
+         * and just return <tt>null</tt> itself  i.e. if it is difficult to look the value up in storage
+         * then it is probably difficult to generate a cache key from it, too..  In this scenario, the
+         * cache will be purely for key-based lookups 
          * 
-         * @param value         the full value being keyed
+         * @param value         the full value being keyed (never <tt>null</tt>)
-         * @return              Returns the business key representing the entity
+         * @return              Returns the business key representing the entity, or <tt>null</tt>
+         *                      if an economical key cannot be generated.
          */
         VK1 getValueKey(V1 value);
         
         /**
          * Find an entity for a given key.
          * 
-         * @param key           the key (ID) used to identify the entity
+         * @param key           the key (ID) used to identify the entity (never <tt>null</tt>)
          * @return              Return the entity or <tt>null</tt> if no entity is exists for the ID
          */
         Pair<K1, V1> findByKey(K1 key);
@@ -74,18 +90,46 @@ public class EntityLookupCache<K extends Serializable, V extends Object, VK exte
          * Find and entity using the given value key.  The <code>equals</code> and <code>hashCode</code>
          * methods of the value object should respect case-sensitivity in the same way that this
          * lookup treats case-sensitivity i.e. if the <code>equals</code> method is <b>case-sensitive</b>
-         * then this method should look the entity up using a <b>case-sensitive</b> search.  Where the
+         * then this method should look the entity up using a <b>case-sensitive</b> search.
-         * behaviour is configurable, 
+         * <p/>
+         * Since this is a cache backed by some sort of database, <tt>null</tt> values are allowed by the
+         * cache.  The implementation of this method can throw an exception if <tt>null</tt> is not
+         * appropriate for the use-case.
+         * <p/>
+         * If the search is impossible or expensive, this method should just return <tt>null</tt>.  This
+         * would usually be the case if the {@link #getValueKey(Object) getValueKey} method also returned
+         * <tt>null</tt> i.e. if it is difficult to look the value up in storage then it is probably
+         * difficult to generate a cache key from it, too.
          * 
-         * @param value         the value (business object) used to identify the entity
+         * @param value         the value (business object) used to identify the entity (<tt>null</tt> allowed).
          * @return              Return the entity or <tt>null</tt> if no entity matches the given value
          */
         Pair<K1, V1> findByValue(V1 value);
         
+        /**
+         * Create an entity using the given values.  It is valid to assume that the entity does not exist
+         * within the current transaction at least.
+         * <p/>
+         * Since persistence mechanisms often allow <tt>null</tt> values, these can be expected here.  The
+         * implementation  must throw an exception if <tt>null</tt> is not allowed for the specific use-case.
+         * 
+         * @param value         the value (business object) used to identify the entity (<tt>null</tt> allowed).
+         * @return              Return the newly-created entity ID-value pair
+         */
         Pair<K1, V1> createValue(V1 value);
     }
     
-    private static final String NULL_VALUE = "@@NULL_VALUE@@";
+    /**
+     * A valid <code>null</code> value i.e. a value that has been <u>persisted</u> as null.
+     */
+    private static final Serializable VALUE_NULL = "@@VALUE_NULL@@";
+    /**
+     * A value that was not found or persisted.
+     */
+    private static final Serializable VALUE_NOT_FOUND = "@@VALUE_NOT_FOUND@@";
+    /**
+     * The cache region that will be used (see {@link CacheRegionKey}) in all the cache keys
+     */
     private static final String CACHE_REGION_DEFAULT = "DEFAULT";
     
     private final SimpleCache<Serializable, Object> cache;
@@ -139,6 +183,10 @@ public class EntityLookupCache<K extends Serializable, V extends Object, VK exte
     @SuppressWarnings("unchecked")
     public Pair<K, V> getByKey(K key)
     {
+        if (key == null)
+        {
+            throw new IllegalArgumentException("An entity lookup key may not be null");
+        }
         // Handle missing cache
         if (cache == null)
         {
@@ -148,26 +196,36 @@ public class EntityLookupCache<K extends Serializable, V extends Object, VK exte
         CacheRegionKey cacheKey = new CacheRegionKey(cacheRegion, key);
         // Look in the cache
         V value = (V) cache.get(cacheKey);
-        if (value != null && value.equals(NULL_VALUE))
+        if (value != null)
+        {
+            if (value.equals(VALUE_NOT_FOUND))
             {
                 // We checked before
                 return null;
             }
-        else if (value != null)
+            else if (value.equals(VALUE_NULL))
+            {
+                return new Pair<K, V>(key, null);
+            }
+            else
             {
                 return new Pair<K, V>(key, value);
             }
+        }
         // Resolve it
         Pair<K, V> entityPair = entityLookup.findByKey(key);
         if (entityPair == null)
         {
-            // Cache nulls
+            // Cache "not found"
-            cache.put(cacheKey, NULL_VALUE);
+            cache.put(cacheKey, VALUE_NOT_FOUND);
         }
         else
         {
+            value = entityPair.getSecond();
             // Cache the value
-            cache.put(cacheKey, entityPair.getSecond());
+            cache.put(
+                    cacheKey,
+                    (value == null ? VALUE_NULL : value));
         }
         // Done
         return entityPair;
@@ -182,33 +240,49 @@ public class EntityLookupCache<K extends Serializable, V extends Object, VK exte
             return entityLookup.findByValue(value);
         }
         
-        // Get the value key
+        // Get the value key.
-        VK valueKey = entityLookup.getValueKey(value);
+        // The cast to (VK) is counter-intuitive, but works because they're all just Serializable
-        CacheRegionKey cacheKey = new CacheRegionKey(cacheRegion, valueKey);
+        // It's nasty, but hidden from the cache client code.
-        // Look in the cache
+        VK valueKey = (value == null) ? (VK)VALUE_NULL : entityLookup.getValueKey(value);
-        K key = (K) cache.get(cacheKey);
+        // Check if the value has a good key
-        // Check if we have looked this up already
+        if (valueKey == null)
-        if (key != null && key.equals(NULL_VALUE))
         {
-            // We checked before
+            return entityLookup.findByValue(value);
+        }
+        
+        // Look in the cache
+        CacheRegionKey valueCacheKey = new CacheRegionKey(cacheRegion, valueKey);
+        K key = (K) cache.get(valueCacheKey);
+        // Check if we have looked this up already
+        if (key != null)
+        {
+            // We checked before and ...
+            if (key.equals(VALUE_NOT_FOUND))
+            {
+                // ... it didn't exist
                 return null;
             }
-        else if (key != null)
+            else
             {
+                // ... it did exist
                 return new Pair<K, V>(key, value);
             }
+        }
         // Resolve it
         Pair<K, V> entityPair = entityLookup.findByValue(value);
         if (entityPair == null)
         {
-            // Cache a null
+            // Cache "not found"
-            cache.put(cacheKey, NULL_VALUE);
+            cache.put(valueCacheKey, VALUE_NOT_FOUND);
         }
         else
         {
             key = entityPair.getFirst();
             // Cache the key
-            cache.put(cacheKey, key);
+            cache.put(valueCacheKey, key);
+            cache.put(
+                    new CacheRegionKey(cacheRegion, key),
+                    (value == null ? VALUE_NULL : value));
         }
         // Done
         return entityPair;
@@ -229,12 +303,25 @@ public class EntityLookupCache<K extends Serializable, V extends Object, VK exte
         }
         
         // Get the value key
-        VK valueKey = entityLookup.getValueKey(value);
+        // The cast to (VK) is counter-intuitive, but works because they're all just Serializable.
-        CacheRegionKey cacheKey = new CacheRegionKey(cacheRegion, valueKey);
+        // It's nasty, but hidden from the cache client code.
+        VK valueKey = (value == null) ? (VK)VALUE_NULL : entityLookup.getValueKey(value);
+        // Check if the value has a good key
+        if (valueKey == null)
+        {
+            Pair<K, V> entityPair = entityLookup.findByValue(value);
+            if (entityPair == null)
+            {
+                entityPair = entityLookup.createValue(value);
+            }
+            return entityPair;
+        }
+        
         // Look in the cache
-        K key = (K) cache.get(cacheKey);
+        CacheRegionKey valueCacheKey = new CacheRegionKey(cacheRegion, valueKey);
+        K key = (K) cache.get(valueCacheKey);
         // Check if the value is already mapped to a key
-        if (key != null && !key.equals(NULL_VALUE))
+        if (key != null && !key.equals(VALUE_NOT_FOUND))
         {
             return new Pair<K, V>(key, value);
         }
@@ -247,8 +334,10 @@ public class EntityLookupCache<K extends Serializable, V extends Object, VK exte
         }
         key = entityPair.getFirst();
         // Cache the key and value
-        cache.put(cacheKey, key);
+        cache.put(valueCacheKey, key);
-        cache.put(new CacheRegionKey(cacheRegion, key), value);
+        cache.put(
+                new CacheRegionKey(cacheRegion, key),
+                (value == null ? VALUE_NULL : value));
         // Done
         return entityPair;
     }
@@ -258,7 +347,7 @@ public class EntityLookupCache<K extends Serializable, V extends Object, VK exte
     {
         CacheRegionKey keyCacheKey = new CacheRegionKey(cacheRegion, key);
         V value = (V) cache.get(keyCacheKey);
-        if (value != null && !value.equals(NULL_VALUE))
+        if (value != null && !value.equals(VALUE_NOT_FOUND))
         {
             // Get the value key and remove it
             VK valueKey = entityLookup.getValueKey(value);

source/java/org/alfresco/repo/cache/lookup/EntityLookupCacheTest.java

@@ -33,6 +33,7 @@ import junit.framework.TestCase;
 import org.alfresco.repo.cache.MemoryCache;
 import org.alfresco.repo.cache.SimpleCache;
 import org.alfresco.repo.cache.lookup.EntityLookupCache.EntityLookupCallbackDAO;
+import org.alfresco.util.EqualsHelper;
 import org.alfresco.util.Pair;
 
 /**
@@ -142,9 +143,26 @@ public class EntityLookupCacheTest extends TestCase implements EntityLookupCallb
         
         // Now cross-check against the caches and make sure that the cache 
         entityPairBBB = entityLookupCacheA.getByValue(valueBBB);
-        assertEquals(5, cache.getKeys().size());
-        entityPairBBB = entityLookupCacheB.getByValue(valueAAA);
         assertEquals(6, cache.getKeys().size());
+        entityPairBBB = entityLookupCacheB.getByValue(valueAAA);
+        assertEquals(8, cache.getKeys().size());
+    }
+
+    public void testNullLookups() throws Exception
+    {
+        TestValue valueNull = null;
+        Pair<Long, Object> entityPairNull = entityLookupCacheA.getOrCreateByValue(valueNull);
+        assertNotNull(entityPairNull);
+        assertTrue(database.containsKey(entityPairNull.getFirst()));
+        assertNull(database.get(entityPairNull.getFirst()));
+        assertEquals(2, cache.getKeys().size());
+        
+        // Look it up again
+        Pair<Long, Object> entityPairCheck = entityLookupCacheA.getOrCreateByValue(valueNull);
+        assertNotNull(entityPairNull);
+        assertTrue(database.containsKey(entityPairNull.getFirst()));
+        assertNull(database.get(entityPairNull.getFirst()));
+        assertEquals(entityPairNull, entityPairCheck);
     }        
 
     /**
@@ -182,30 +200,6 @@ public class EntityLookupCacheTest extends TestCase implements EntityLookupCallb
         return dbValue;
     }
 
-    /**
-     * Simulate creation of a new database entry
-     */
-    public Pair<Long, Object> createValue(Object value)
-    {
-        assertNotNull(value);
-        assertTrue(value instanceof TestValue);
-        String dbValue = ((TestValue)value).val;
-        
-        // Get the last key
-        Long lastKey = database.isEmpty() ? null : database.lastKey();
-        Long newKey = null;
-        if (lastKey == null)
-        {
-            newKey = new Long(1);
-        }
-        else
-        {
-            newKey = new Long(lastKey.longValue() + 1);
-        }
-        database.put(newKey, dbValue);
-        return new Pair<Long, Object>(newKey, value);
-    }
-
     public Pair<Long, Object> findByKey(Long key)
     {
         assertNotNull(key);
@@ -222,17 +216,39 @@ public class EntityLookupCacheTest extends TestCase implements EntityLookupCallb
 
     public Pair<Long, Object> findByValue(Object value)
     {
-        assertNotNull(value);
+        assertTrue(value == null || value instanceof TestValue);
-        assertTrue(value instanceof TestValue);
+        String dbValue = (value == null) ? null : ((TestValue)value).val;
-        String dbValue = ((TestValue)value).val;
 
         for (Map.Entry<Long, String> entry : database.entrySet())
         {
-            if (entry.getValue().equals(dbValue))
+            if (EqualsHelper.nullSafeEquals(entry.getValue(), dbValue))
             {
                 return new Pair<Long, Object>(entry.getKey(), entry.getValue());
             }
         }
         return null;
     }
+
+    /**
+     * Simulate creation of a new database entry
+     */
+    public Pair<Long, Object> createValue(Object value)
+    {
+        assertTrue(value == null || value instanceof TestValue);
+        String dbValue = (value == null) ? null : ((TestValue)value).val;
+        
+        // Get the last key
+        Long lastKey = database.isEmpty() ? null : database.lastKey();
+        Long newKey = null;
+        if (lastKey == null)
+        {
+            newKey = new Long(1);
+        }
+        else
+        {
+            newKey = new Long(lastKey.longValue() + 1);
+        }
+        database.put(newKey, dbValue);
+        return new Pair<Long, Object>(newKey, value);
+    }
 }