Fix bulk loading of entries with PassthroughLoadingCache

During the review of I5906a1f2 the PassthroughLoadingCache added
the mechanism for bulk-loading in the getAll() method. However,
the new flavour of loading was not covered by additional tests
and then failed to honour the method because of the casting of
a regular Map to an ImmutableMap.

Rectify the situation and add one extra test to document the
expected behaviour.

Release-Notes: Fix bulk loading of entries for disabled caches
Change-Id: I7a24deebeef7f6ae853d11c1ee458b53296e1a44

2 files changed, 25 insertions, 2 deletions

diff --git a/java/com/google/gerrit/server/cache/mem/PassthroughLoadingCache.java b/java/com/google/gerrit/server/cache/mem/PassthroughLoadingCache.java
index 4ed8cfaf44..ded21aae5b 100644
--- a/java/com/google/gerrit/server/cache/mem/PassthroughLoadingCache.java
+++ b/java/com/google/gerrit/server/cache/mem/PassthroughLoadingCache.java
@@ -122,8 +122,8 @@ public class PassthroughLoadingCache<K, V> implements LoadingCache<K, V> {
   }
 
   @SuppressWarnings("unchecked")
-  public ImmutableMap<K, V> getAllBulk(Iterable<? extends K> keys) throws Exception {
-    return (ImmutableMap<K, V>) cacheLoader.loadAll(keys);
+  private ImmutableMap<K, V> getAllBulk(Iterable<? extends K> keys) throws Exception {
+    return (ImmutableMap<K, V>) ImmutableMap.copyOf(cacheLoader.loadAll(keys));
   }
 
   @Override
diff --git a/javatests/com/google/gerrit/server/cache/mem/DefaultMemoryCacheFactoryTest.java b/javatests/com/google/gerrit/server/cache/mem/DefaultMemoryCacheFactoryTest.java
index e0c0c34a78..9b504a94f7 100644
--- a/javatests/com/google/gerrit/server/cache/mem/DefaultMemoryCacheFactoryTest.java
+++ b/javatests/com/google/gerrit/server/cache/mem/DefaultMemoryCacheFactoryTest.java
@@ -20,10 +20,14 @@ import static com.google.common.truth.Truth.assertThat;
 import com.google.common.cache.CacheLoader;
 import com.google.common.cache.LoadingCache;
 import com.google.common.cache.Weigher;
+import com.google.common.collect.ImmutableMap;
 import com.google.gerrit.server.cache.CacheBackend;
 import com.google.gerrit.server.cache.CacheDef;
 import com.google.inject.TypeLiteral;
 import java.time.Duration;
+import java.util.Arrays;
+import java.util.List;
+import java.util.Map;
 import java.util.concurrent.BrokenBarrierException;
 import java.util.concurrent.CyclicBarrier;
 import java.util.concurrent.ExecutionException;
@@ -33,6 +37,8 @@ import java.util.concurrent.ScheduledFuture;
 import java.util.concurrent.TimeUnit;
 import java.util.concurrent.TimeoutException;
 import java.util.function.Function;
+import java.util.stream.Collectors;
+import java.util.stream.StreamSupport;
 import org.eclipse.jgit.lib.Config;
 import org.junit.Before;
 import org.junit.Test;
@@ -90,6 +96,17 @@ public class DefaultMemoryCacheFactoryTest {
     assertThat(cacheValue.get()).isEqualTo(TEST_CACHE_KEY);
   }
 
+  @Test
+  public void shouldLoadAllKeysWithDisabledCache() throws Exception {
+    LoadingCache<Integer, Integer> disabledCache =
+        memoryCacheFactory.build(newCacheDef(0), newCacheLoader(identity()), CacheBackend.CAFFEINE);
+
+    List<Integer> keys = Arrays.asList(1, 2);
+    ImmutableMap<Integer, Integer> entries = disabledCache.getAll(keys);
+
+    assertThat(entries).containsExactly(1, 1, 2, 2);
+  }
+
   private void assertCacheEvictionIsNotBlocking(LoadingCache<Integer, Integer> disabledCache)
       throws InterruptedException, BrokenBarrierException, TimeoutException, ExecutionException {
     ScheduledFuture<Integer> cacheValue =
@@ -120,6 +137,12 @@ public class DefaultMemoryCacheFactoryTest {
         }
         return v;
       }
+
+      @Override
+      public Map<Integer, Integer> loadAll(Iterable<? extends Integer> keys) throws Exception {
+        return StreamSupport.stream(keys.spliterator(), false)
+            .collect(Collectors.toMap(identity(), identity()));
+      }
     };
   }