consistant behavior for null lookups to into unmodifiable maps (fixes #…

…864) While a null lookup is not allowed for cache the operations (e.g. asMap.get), if an operation returned a map then this might not be consistant. Originally all of these cases used Collections' emptyMap() or unmodifiableMap(m), but a few cases switched to Map's of() or copyOf() alternatives. The former allows for null queries whereas the latter does not, and this could vary depending on if the result was populated. For consistency and retaining the original behavior, this is restored to the Collections' methods.
ben-manes · Feb 2, 2023 · 277155d · 277155d
1 parent 6fc12bb
commit 277155d
Show file tree

Hide file tree

Showing 10 changed files with 99 additions and 14 deletions.
diff --git a/caffeine/src/main/java/com/github/benmanes/caffeine/cache/BoundedLocalCache.java b/caffeine/src/main/java/com/github/benmanes/caffeine/cache/BoundedLocalCache.java
@@ -49,6 +49,7 @@
 import java.util.Collection;
 import java.util.Collections;
 import java.util.Comparator;
+import java.util.HashMap;
 import java.util.IdentityHashMap;
 import java.util.Iterator;
 import java.util.LinkedHashMap;
@@ -1661,8 +1662,7 @@ void rescheduleCleanUpIfIncomplete() {
     }
 
     // If a scheduler was configured then the maintenance can be deferred onto the custom executor
-    // to be run in the near future. This is only used if there is no scheduled set, else the next
-    // run depends on other activity to trigger it.
+    // and run in the near future. Otherwise, it will be handled due to other cache activity.
     var pacer = pacer();
     if ((pacer != null) && !pacer.isScheduled() && evictionLock.tryLock()) {
       try {
@@ -3985,7 +3985,7 @@ static final class BoundedPolicy<K, V> implements Policy<K, V> {
     @Override public Map<K, CompletableFuture<V>> refreshes() {
       var refreshes = cache.refreshes;
       if ((refreshes == null) || refreshes.isEmpty()) {
-        return Map.of();
+        return Collections.emptyMap();
       } else if (cache.collectKeys()) {
         var inFlight = new IdentityHashMap<K, CompletableFuture<V>>(refreshes.size());
         for (var entry : refreshes.entrySet()) {
@@ -4001,7 +4001,7 @@ static final class BoundedPolicy<K, V> implements Policy<K, V> {
       }
       @SuppressWarnings("unchecked")
       var castedRefreshes = (Map<K, CompletableFuture<V>>) (Object) refreshes;
-      return Map.copyOf(castedRefreshes);
+      return Collections.unmodifiableMap(new HashMap<>(castedRefreshes));
     }
     @Override public Optional<Eviction<K, V>> eviction() {
       return cache.evicts()

diff --git a/caffeine/src/main/java/com/github/benmanes/caffeine/cache/LocalAsyncCache.java b/caffeine/src/main/java/com/github/benmanes/caffeine/cache/LocalAsyncCache.java
@@ -160,7 +160,7 @@ default CompletableFuture<Map<K, V>> getAll(Iterable<? extends K> keys,
    */
   static <K, V> CompletableFuture<Map<K, V>> composeResult(Map<K, CompletableFuture<V>> futures) {
     if (futures.isEmpty()) {
-      return CompletableFuture.completedFuture(Map.of());
+      return CompletableFuture.completedFuture(Collections.emptyMap());
     }
     @SuppressWarnings("rawtypes")
     CompletableFuture<?>[] array = futures.values().toArray(new CompletableFuture[0]);

diff --git a/caffeine/src/main/java/com/github/benmanes/caffeine/cache/UnboundedLocalCache.java b/caffeine/src/main/java/com/github/benmanes/caffeine/cache/UnboundedLocalCache.java
@@ -32,6 +32,7 @@
 import java.util.AbstractSet;
 import java.util.Collection;
 import java.util.Collections;
+import java.util.HashMap;
 import java.util.Iterator;
 import java.util.LinkedHashMap;
 import java.util.Map;
@@ -1069,11 +1070,11 @@ static final class UnboundedPolicy<K, V> implements Policy<K, V> {
     @Override public Map<K, CompletableFuture<V>> refreshes() {
       var refreshes = cache.refreshes;
       if (refreshes == null) {
-        return Map.of();
+        return Collections.emptyMap();
       }
       @SuppressWarnings("unchecked")
       var castedRefreshes = (Map<K, CompletableFuture<V>>) (Object) refreshes;
-      return Map.copyOf(castedRefreshes);
+      return Collections.unmodifiableMap(new HashMap<>(castedRefreshes));
     }
     @Override public Optional<Eviction<K, V>> eviction() {
       return Optional.empty();

diff --git a/caffeine/src/test/java/com/github/benmanes/caffeine/cache/AsyncLoadingCacheTest.java b/caffeine/src/test/java/com/github/benmanes/caffeine/cache/AsyncLoadingCacheTest.java
@@ -21,6 +21,7 @@
 import static com.github.benmanes.caffeine.testing.Awaits.await;
 import static com.github.benmanes.caffeine.testing.CollectionSubject.assertThat;
 import static com.github.benmanes.caffeine.testing.FutureSubject.assertThat;
+import static com.github.benmanes.caffeine.testing.IntSubject.assertThat;
 import static com.github.benmanes.caffeine.testing.MapSubject.assertThat;
 import static com.google.common.collect.ImmutableMap.toImmutableMap;
 import static com.google.common.truth.Truth.assertThat;
@@ -207,6 +208,15 @@ public void getAll_immutable_result(AsyncLoadingCache<Int, Int> cache, CacheCont
     cache.getAll(context.absentKeys()).join().clear();
   }
 
+  @CacheSpec
+  @Test(dataProvider = "caches")
+  public void getAll_nullLookup(AsyncLoadingCache<Int, Int> cache, CacheContext context) {
+    var result = cache.getAll(context.firstMiddleLastKeys()).join();
+    assertThat(result.containsValue(null)).isFalse();
+    assertThat(result.containsKey(null)).isFalse();
+    assertThat(result.get(null)).isNull();
+  }
+
   @CacheSpec(loader = Loader.BULK_NULL)
   @Test(dataProvider = "caches")
   public void getAll_absent_bulkNull(AsyncLoadingCache<Int, Int> cache, CacheContext context) {

diff --git a/caffeine/src/test/java/com/github/benmanes/caffeine/cache/CacheTest.java b/caffeine/src/test/java/com/github/benmanes/caffeine/cache/CacheTest.java
@@ -236,6 +236,15 @@ public void getAllPresent_immutable(Cache<Int, Int> cache, CacheContext context)
     cache.getAllPresent(context.absentKeys()).clear();
   }
 
+  @Test(dataProvider = "caches")
+  @CacheSpec(removalListener = { Listener.DISABLED, Listener.REJECTING })
+  public void getAllPresent_nullLookup(Cache<Int, Int> cache, CacheContext context) {
+    var result = cache.getAllPresent(context.firstMiddleLastKeys());
+    assertThat(result.containsValue(null)).isFalse();
+    assertThat(result.containsKey(null)).isFalse();
+    assertThat(result.get(null)).isNull();
+  }
+
   @Test(dataProvider = "caches")
   @CacheSpec(population = { Population.PARTIAL, Population.FULL },
       removalListener = { Listener.DISABLED, Listener.REJECTING })
@@ -368,6 +377,15 @@ public void getAll_immutable_result(Cache<Int, Int> cache, CacheContext context)
     result.clear();
   }
 
+  @CacheSpec
+  @Test(dataProvider = "caches")
+  public void getAll_nullLookup(Cache<Int, Int> cache, CacheContext context) {
+    var result = cache.getAll(context.firstMiddleLastKeys(), bulkMappingFunction());
+    assertThat(result.containsValue(null)).isFalse();
+    assertThat(result.containsKey(null)).isFalse();
+    assertThat(result.get(null)).isNull();
+  }
+
   @CacheSpec
   @Test(dataProvider = "caches", expectedExceptions = IllegalStateException.class)
   public void getAll_absent_throwsException(Cache<Int, Int> cache, CacheContext context) {
@@ -965,6 +983,14 @@ public void refreshes_unmodifiable(Cache<Int, Int> cache, CacheContext context)
     cache.policy().refreshes().clear();
   }
 
+  @CacheSpec
+  @Test(dataProvider = "caches")
+  public void refreshes_nullLookup(Cache<Int, Int> cache, CacheContext context) {
+    assertThat(cache.policy().refreshes().containsValue(null)).isFalse();
+    assertThat(cache.policy().refreshes().containsKey(null)).isFalse();
+    assertThat(cache.policy().refreshes().get(null)).isNull();
+  }
+
   /* --------------- Policy: CacheEntry --------------- */
 
   @Test(expectedExceptions = UnsupportedOperationException.class)

diff --git a/caffeine/src/test/java/com/github/benmanes/caffeine/cache/LoadingCacheTest.java b/caffeine/src/test/java/com/github/benmanes/caffeine/cache/LoadingCacheTest.java
@@ -200,6 +200,15 @@ public void getAll_immutable_result(LoadingCache<Int, Int> cache, CacheContext c
     cache.getAll(context.absentKeys()).clear();
   }
 
+  @CacheSpec
+  @Test(dataProvider = "caches")
+  public void getAll_nullLookup(LoadingCache<Int, Int> cache, CacheContext context) {
+    var result = cache.getAll(context.firstMiddleLastKeys());
+    assertThat(result.containsValue(null)).isFalse();
+    assertThat(result.containsKey(null)).isFalse();
+    assertThat(result.get(null)).isNull();
+  }
+
   @CheckNoEvictions
   @Test(dataProvider = "caches")
   @CacheSpec(loader = Loader.NULL)
@@ -967,6 +976,15 @@ public void refreshAll_nullKey(LoadingCache<Int, Int> cache, CacheContext contex
     cache.refreshAll(Collections.singletonList(null));
   }
 
+  @CacheSpec
+  @Test(dataProvider = "caches")
+  public void refreshAll_nullLookup(LoadingCache<Int, Int> cache, CacheContext context) {
+    var result = cache.refreshAll(context.firstMiddleLastKeys()).join();
+    assertThat(result.containsValue(null)).isFalse();
+    assertThat(result.containsKey(null)).isFalse();
+    assertThat(result.get(null)).isNull();
+  }
+
   @CheckNoEvictions
   @Test(dataProvider = "caches")
   @CacheSpec(removalListener = { Listener.DISABLED, Listener.REJECTING })
@@ -1170,4 +1188,17 @@ public void refreshes(LoadingCache<Int, Int> cache, CacheContext context) {
     future2.cancel(true);
     assertThat(cache.policy().refreshes()).isExhaustivelyEmpty();
   }
+
+  @Test(dataProvider = "caches")
+  @CacheSpec(implementation = Implementation.Caffeine, loader = Loader.ASYNC_INCOMPLETE)
+  public void refreshes_nullLookup(LoadingCache<Int, Int> cache, CacheContext context) {
+    cache.refreshAll(context.absentKeys());
+    assertThat(cache.policy().refreshes().get(null)).isNull();
+    assertThat(cache.policy().refreshes().containsKey(null)).isFalse();
+    assertThat(cache.policy().refreshes().containsValue(null)).isFalse();
+
+    for (var future : cache.policy().refreshes().values()) {
+      future.cancel(true);
+    }
+  }
 }
diff --git a/caffeine/src/test/java/com/github/benmanes/caffeine/cache/RefreshAfterWriteTest.java b/caffeine/src/test/java/com/github/benmanes/caffeine/cache/RefreshAfterWriteTest.java
@@ -857,6 +857,21 @@ public void refreshes(LoadingCache<Int, Int> cache, CacheContext context) {
     assertThat(cache).containsEntry(context.firstKey(), Int.MAX_VALUE);
   }
 
+  @Test(dataProvider = "caches")
+  @CacheSpec(implementation = Implementation.Caffeine, loader = Loader.ASYNC_INCOMPLETE,
+      refreshAfterWrite = Expire.ONE_MINUTE, population = Population.FULL)
+  public void refreshes_nullLookup(LoadingCache<Int, Int> cache, CacheContext context) {
+    context.ticker().advance(2, TimeUnit.MINUTES);
+    cache.getIfPresent(context.firstKey());
+    var future = cache.policy().refreshes().get(context.firstKey());
+
+    assertThat(cache.policy().refreshes().get(null)).isNull();
+    assertThat(cache.policy().refreshes().containsKey(null)).isFalse();
+    assertThat(cache.policy().refreshes().containsValue(null)).isFalse();
+
+    future.cancel(true);
+  }
+
   /* --------------- Policy: refreshAfterWrite --------------- */
 
   @Test(dataProvider = "caches")

diff --git a/caffeine/src/test/java/com/github/benmanes/caffeine/cache/testing/CacheContext.java b/caffeine/src/test/java/com/github/benmanes/caffeine/cache/testing/CacheContext.java
@@ -224,7 +224,9 @@ public Int lastKey() {
   }
 
   public ImmutableSet<Int> firstMiddleLastKeys() {
-    return ImmutableSet.of(firstKey(), middleKey(), lastKey());
+    return (firstKey == null)
+        ? ImmutableSet.of()
+        : ImmutableSet.of(firstKey(), middleKey(), lastKey());
   }
 
   public void cleanUp() {

diff --git a/caffeine/src/test/java/com/github/benmanes/caffeine/cache/testing/GuavaCacheFromContext.java b/caffeine/src/test/java/com/github/benmanes/caffeine/cache/testing/GuavaCacheFromContext.java
@@ -470,7 +470,7 @@ final class GuavaPolicy implements Policy<K, V> {
         return new GuavaCacheEntry<>(key, value, snapshotAt);
       }
       @Override public Map<K, CompletableFuture<V>> refreshes() {
-        return Map.of();
+        return Collections.emptyMap();
       }
       @Override public Optional<Eviction<K, V>> eviction() {
         return Optional.empty();
@@ -568,7 +568,7 @@ public CompletableFuture<Map<K, V>> refreshAll(Iterable<? extends K> keys) {
 
     CompletableFuture<Map<K, V>> composeResult(Map<K, CompletableFuture<V>> futures) {
       if (futures.isEmpty()) {
-        return CompletableFuture.completedFuture(Map.of());
+        return CompletableFuture.completedFuture(Collections.emptyMap());
       }
       @SuppressWarnings("rawtypes")
       CompletableFuture<?>[] array = futures.values().toArray(new CompletableFuture[0]);

diff --git a/gradle/dependencies.gradle b/gradle/dependencies.gradle
@@ -54,7 +54,7 @@ ext {
     jmh: '1.36',
     joor: '0.9.14',
     jsr330: '1',
-    nullaway: '0.10.8',
+    nullaway: '0.10.9',
     ohc: '0.6.1',
     osgiComponentAnnotations: '1.5.1',
     picocli: '4.7.1',
@@ -80,7 +80,7 @@ ext {
     junit5: '5.9.2',
     junitTestNG: '1.0.4',
     lincheck: '2.16',
-    mockito: '5.0.0',
+    mockito: '5.1.1',
     osgiUtilFunction: '1.2.0',
     osgiUtilPromise: '1.3.0',
     paxExam: '4.13.5',
@@ -90,7 +90,7 @@ ext {
   ]
   pluginVersions = [
     bnd: '6.4.0',
-    checkstyle: '10.6.0',
+    checkstyle: '10.7.0',
     coveralls: '2.12.0',
     dependencyCheck: '8.0.2',
     errorprone: '3.0.1',
@@ -108,7 +108,7 @@ ext {
     spotbugs: '4.7.3',
     spotbugsContrib: '7.4.7',
     spotbugsPlugin: '5.0.13',
-    versions: '0.44.0',
+    versions: '0.45.0',
   ]
   platformVersions = [
     asm: '9.4',