From 277155d6b999677ef502758ed6089e83fefc8e52 Mon Sep 17 00:00:00 2001 From: Ben Manes Date: Wed, 1 Feb 2023 23:24:38 -0800 Subject: [PATCH] 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. --- .../caffeine/cache/BoundedLocalCache.java | 8 ++--- .../caffeine/cache/LocalAsyncCache.java | 2 +- .../caffeine/cache/UnboundedLocalCache.java | 5 +-- .../caffeine/cache/AsyncLoadingCacheTest.java | 10 ++++++ .../benmanes/caffeine/cache/CacheTest.java | 26 ++++++++++++++++ .../caffeine/cache/LoadingCacheTest.java | 31 +++++++++++++++++++ .../caffeine/cache/RefreshAfterWriteTest.java | 15 +++++++++ .../caffeine/cache/testing/CacheContext.java | 4 ++- .../cache/testing/GuavaCacheFromContext.java | 4 +-- gradle/dependencies.gradle | 8 ++--- 10 files changed, 99 insertions(+), 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 index aac7815d5b..bcbd2bb291 100644 --- 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 implements Policy { @Override public Map> refreshes() { var refreshes = cache.refreshes; if ((refreshes == null) || refreshes.isEmpty()) { - return Map.of(); + return Collections.emptyMap(); } else if (cache.collectKeys()) { var inFlight = new IdentityHashMap>(refreshes.size()); for (var entry : refreshes.entrySet()) { @@ -4001,7 +4001,7 @@ static final class BoundedPolicy implements Policy { } @SuppressWarnings("unchecked") var castedRefreshes = (Map>) (Object) refreshes; - return Map.copyOf(castedRefreshes); + return Collections.unmodifiableMap(new HashMap<>(castedRefreshes)); } @Override public Optional> 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 index 559dd2b0fa..acc1fd7a0c 100644 --- 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> getAll(Iterable keys, */ static CompletableFuture> composeResult(Map> 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 index 452fbc9c46..fab07e2529 100644 --- 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 implements Policy { @Override public Map> refreshes() { var refreshes = cache.refreshes; if (refreshes == null) { - return Map.of(); + return Collections.emptyMap(); } @SuppressWarnings("unchecked") var castedRefreshes = (Map>) (Object) refreshes; - return Map.copyOf(castedRefreshes); + return Collections.unmodifiableMap(new HashMap<>(castedRefreshes)); } @Override public Optional> 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 index dea9317af6..841a356072 100644 --- 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 cache, CacheCont cache.getAll(context.absentKeys()).join().clear(); } + @CacheSpec + @Test(dataProvider = "caches") + public void getAll_nullLookup(AsyncLoadingCache 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 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 index 6954305eed..46d0099c35 100644 --- 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 cache, CacheContext context) cache.getAllPresent(context.absentKeys()).clear(); } + @Test(dataProvider = "caches") + @CacheSpec(removalListener = { Listener.DISABLED, Listener.REJECTING }) + public void getAllPresent_nullLookup(Cache 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 cache, CacheContext context) result.clear(); } + @CacheSpec + @Test(dataProvider = "caches") + public void getAll_nullLookup(Cache 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 cache, CacheContext context) { @@ -965,6 +983,14 @@ public void refreshes_unmodifiable(Cache cache, CacheContext context) cache.policy().refreshes().clear(); } + @CacheSpec + @Test(dataProvider = "caches") + public void refreshes_nullLookup(Cache 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 index fe8ba7bb4b..d2f573faa3 100644 --- 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 cache, CacheContext c cache.getAll(context.absentKeys()).clear(); } + @CacheSpec + @Test(dataProvider = "caches") + public void getAll_nullLookup(LoadingCache 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 cache, CacheContext contex cache.refreshAll(Collections.singletonList(null)); } + @CacheSpec + @Test(dataProvider = "caches") + public void refreshAll_nullLookup(LoadingCache 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 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 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 index 1bddf76cd8..09bbb7f28f 100644 --- 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 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 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 index 8ff275e957..17b430ad60 100644 --- 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 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 index 59aeb1bad3..afc155bd86 100644 --- 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 { return new GuavaCacheEntry<>(key, value, snapshotAt); } @Override public Map> refreshes() { - return Map.of(); + return Collections.emptyMap(); } @Override public Optional> eviction() { return Optional.empty(); @@ -568,7 +568,7 @@ public CompletableFuture> refreshAll(Iterable keys) { CompletableFuture> composeResult(Map> 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 index ff5d9f27e0..5b822f1197 100644 --- 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',