Fixed Map.entrySet.contains(o) to use reference equality

ben-manes · Nov 29, 2021 · 76349c2 · 76349c2
1 parent 1eed56b
commit 76349c2
Show file tree

Hide file tree

Showing 5 changed files with 99 additions and 7 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
@@ -45,7 +45,6 @@
 import java.util.List;
 import java.util.Map;
 import java.util.NoSuchElementException;
-import java.util.Objects;
 import java.util.Optional;
 import java.util.OptionalInt;
 import java.util.OptionalLong;
@@ -3134,8 +3133,13 @@ public boolean contains(Object obj) {
         return false;
       }
       Entry<?, ?> entry = (Entry<?, ?>) obj;
-      Node<K, V> node = cache.data.get(cache.nodeFactory.newLookupKey(entry.getKey()));
-      return (node != null) && Objects.equals(node.getValue(), entry.getValue());
+      Object key = entry.getKey();
+      Object value = entry.getValue();
+      if ((key == null) || (value == null)) {
+        return false;
+      }
+      Node<K, V> node = cache.data.get(cache.nodeFactory.newLookupKey(key));
+      return (node != null) && node.containsValue(value);
     }
 
     @Override

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
@@ -1002,8 +1002,13 @@ public boolean contains(Object o) {
           return false;
         }
         Entry<?, ?> entry = (Entry<?, ?>) o;
-        V value = AsMapView.this.get(entry.getKey());
-        return (value != null) && value.equals(entry.getValue());
+        Object key = entry.getKey();
+        Object value = entry.getValue();
+        if ((key == null) || (value == null)) {
+          return false;
+        }
+        V cachedValue = AsMapView.this.get(key);
+        return (cachedValue != null) && cachedValue.equals(value);
       }
 
       @Override

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
@@ -733,8 +733,13 @@ public boolean contains(Object o) {
         return false;
       }
       Entry<?, ?> entry = (Entry<?, ?>) o;
-      V value = cache.get(entry.getKey());
-      return (value != null) && value.equals(entry.getValue());
+      Object key = entry.getKey();
+      Object value = entry.getValue();
+      if ((key == null) || (value == null)) {
+        return false;
+      }
+      V cachedValue = cache.get(key);
+      return (cachedValue != null) && cachedValue.equals(value);
     }
 
     @Override

diff --git a/caffeine/src/test/java/com/github/benmanes/caffeine/cache/AsMapTest.java b/caffeine/src/test/java/com/github/benmanes/caffeine/cache/AsMapTest.java
@@ -33,6 +33,7 @@
 import static org.hamcrest.Matchers.not;
 import static org.hamcrest.Matchers.nullValue;
 
+import java.util.AbstractMap;
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collection;
@@ -2163,6 +2164,43 @@ public void entriesToArray(Map<Integer, Integer> map, CacheContext context) {
     assertThat(Arrays.asList(array).containsAll(context.original().entrySet()), is(true));
   }
 
+  @CheckNoStats
+  @Test(dataProvider = "caches")
+  @CacheSpec(population = Population.FULL,
+      removalListener = { Listener.DEFAULT, Listener.REJECTING })
+  public void entrySet_contains_nullKey(Map<Integer, Integer> map, CacheContext context) {
+    Map.Entry<Integer, Integer> entry = new AbstractMap.SimpleEntry<>(
+        null, context.original().get(context.firstKey()));
+    assertThat(map.entrySet().contains(entry), is(false));
+  }
+
+  @CheckNoStats
+  @Test(dataProvider = "caches")
+  @CacheSpec(population = Population.FULL,
+      removalListener = { Listener.DEFAULT, Listener.REJECTING })
+  public void entrySet_contains_nullValue(Map<Integer, Integer> map, CacheContext context) {
+    Map.Entry<Integer, Integer> entry = new AbstractMap.SimpleEntry<>(context.firstKey(), null);
+    assertThat(map.entrySet().contains(entry), is(false));
+  }
+
+  @CheckNoStats
+  @Test(dataProvider = "caches")
+  @CacheSpec(removalListener = { Listener.DEFAULT, Listener.REJECTING })
+  public void entrySet_contains_absent(Map<Integer, Integer> map, CacheContext context) {
+    Map.Entry<Integer, Integer> entry = immutableEntry(context.absentKey(), context.absentValue());
+    assertThat(map.entrySet().contains(entry), is(false));
+  }
+
+  @CheckNoStats
+  @Test(dataProvider = "caches")
+  @CacheSpec(population = Population.FULL,
+      removalListener = { Listener.DEFAULT, Listener.REJECTING })
+  public void entrySet_contains_present(Map<Integer, Integer> map, CacheContext context) {
+    Map.Entry<Integer, Integer> entry = immutableEntry(
+        context.firstKey(), context.original().get(context.firstKey()));
+    assertThat(map.entrySet().contains(entry), is(true));
+  }
+
   @CheckNoWriter @CheckNoStats
   @Test(dataProvider = "caches")
   @CacheSpec(population = Population.EMPTY,

diff --git a/caffeine/src/test/java/com/github/benmanes/caffeine/cache/AsyncAsMapTest.java b/caffeine/src/test/java/com/github/benmanes/caffeine/cache/AsyncAsMapTest.java
@@ -35,6 +35,7 @@
 import static org.hamcrest.Matchers.not;
 import static org.hamcrest.Matchers.nullValue;
 
+import java.util.AbstractMap;
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collection;
@@ -1692,6 +1693,45 @@ public void entrySet_empty(AsyncCache<Integer, Integer> cache, CacheContext cont
     assertThat(cache.asMap().entrySet(), is(deeplyEmpty()));
   }
 
+  @CheckNoStats
+  @Test(dataProvider = "caches")
+  @CacheSpec(population = Population.FULL,
+      removalListener = { Listener.DEFAULT, Listener.REJECTING })
+  public void entrySet_contains_nullKey(AsyncCache<Integer, Integer> cache, CacheContext context) {
+    Map.Entry<Integer, CompletableFuture<Integer>> entry =
+        new AbstractMap.SimpleEntry<>(null, cache.asMap().get(context.firstKey()));
+    assertThat(cache.asMap().entrySet().contains(entry), is(false));
+  }
+
+  @CheckNoStats
+  @Test(dataProvider = "caches")
+  @CacheSpec(population = Population.FULL,
+      removalListener = { Listener.DEFAULT, Listener.REJECTING })
+  public void entrySet_contains_nullValue(AsyncCache<Integer, Integer> cache, CacheContext context) {
+    Map.Entry<Integer, CompletableFuture<Integer>> entry =
+        new AbstractMap.SimpleEntry<>(context.firstKey(), null);
+    assertThat(cache.asMap().entrySet().contains(entry), is(false));
+  }
+
+  @CheckNoStats
+  @Test(dataProvider = "caches")
+  @CacheSpec(removalListener = { Listener.DEFAULT, Listener.REJECTING })
+  public void entrySet_contains_absent(AsyncCache<Integer, Integer> cache, CacheContext context) {
+    Map.Entry<Integer, CompletableFuture<Integer>> entry = immutableEntry(
+        context.absentKey(), CompletableFuture.completedFuture(context.absentValue()));
+    assertThat(cache.asMap().entrySet().contains(entry), is(false));
+  }
+
+  @CheckNoStats
+  @Test(dataProvider = "caches")
+  @CacheSpec(population = Population.FULL,
+      removalListener = { Listener.DEFAULT, Listener.REJECTING })
+  public void entrySet_contains_present(AsyncCache<Integer, Integer> cache, CacheContext context) {
+    Map.Entry<Integer, CompletableFuture<Integer>> entry = immutableEntry(
+        context.firstKey(), cache.asMap().get(context.firstKey()));
+    assertThat(cache.asMap().entrySet().contains(entry), is(true));
+  }
+
   @CheckNoStats
   @CacheSpec(population = Population.EMPTY, removalListener = Listener.DEFAULT)
   @Test(dataProvider = "caches", expectedExceptions = UnsupportedOperationException.class)