[COLLECTIONS-887] ConcurrentReferenceHashMap.remove(key), remove(key,

value), replace(key, value), and replace(key, oldValue, newValue) throw
NullPointerException inconsistently

Author: garydgregory

src/changes/changes.xml

@@ -44,6 +44,8 @@
     <action type="fix" dev="ggregory" due-to="Paul King, Gary Gregory, Sebb">Add MultiValuedMap.inverted() #665.</action>
     <action type="fix" dev="ggregory" due-to="John Griffin, Gary Gregory" issue="COLLECTIONS-885">PatriciaTrie submap and range view operations silently mutate trie structure resulting in ConcurrentModificationException for any iterating threads #672.</action>
     <action type="fix" dev="ggregory" due-to="Partha Protim Paul" issue="COLLECTIONS-886">[javadoc] CompositeSet.addComposited(Set) does not throw NullPointerException when null is passed #675.</action>
+    <action type="fix" dev="ggregory" due-to="Partha Paul, Gary Gregory" issue="COLLECTIONS-887">ConcurrentReferenceHashMap.remove(key), remove(key, value), replace(key, value), and replace(key, oldValue, newValue) throw NullPointerException inconsistently
+.</action>
     <!-- ADD -->
     <action type="add" dev="ggregory" due-to="Gary Gregory">Add generics to UnmodifiableIterator for the wrapped type.</action>
     <action type="add" dev="ggregory" due-to="Gary Gregory">Add a Maven benchmark profile for JMH.</action>

src/main/java/org/apache/commons/collections4/map/ConcurrentReferenceHashMap.java

@@ -1642,7 +1642,16 @@ public V get(final Object key) {
         return segmentFor(hash).get(key, hash);
     }
 
+    /**
+     * Returns the hash code of the given key, which is either the result of calling {@code hashCode} or {@code System.identityHashCode} depending on
+     * {@code identityComparisons}.
+     * 
+     * @param key The key to hash.
+     * @return the hash code of the given key.
+     * @throws NullPointerException if the specified key is null.
+     */
     private int hashOf(final Object key) {
+        Objects.requireNonNull(key, "key");
         return hash(identityComparisons ? System.identityHashCode(key) : key.hashCode());
     }
 

src/test/java/org/apache/commons/collections4/map/AbstractConcurrentReferenceHashMapTest.java

@@ -17,9 +17,13 @@
 
 package org.apache.commons.collections4.map;
 
+import static org.junit.jupiter.api.Assertions.assertThrows;
+
 import java.util.EnumSet;
 
 import org.apache.commons.collections4.map.ConcurrentReferenceHashMap.Option;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
 
 /**
  * Tests {@link ConcurrentReferenceHashMap}.
@@ -31,6 +35,11 @@ public abstract class AbstractConcurrentReferenceHashMapTest<K, V> extends Abstr
 
     protected static final EnumSet<Option> IDENTITY_COMPARISONS = EnumSet.of(Option.IDENTITY_COMPARISONS);
 
+    @BeforeEach
+    void beforeEachFill() {
+        resetFull();
+    }
+
     @Override
     public boolean isAllowNullKey() {
         return false;
@@ -51,4 +60,94 @@ public boolean isTestSerialization() {
         return false;
     }
 
+    @Test
+    void testComputeNullKey() {
+        final ConcurrentReferenceHashMap<K, V> map = getMap();
+        assertThrows(NullPointerException.class, () -> map.compute(null, (k, v) -> getSampleValues()[0]));
+    }
+
+    @Test
+    void testComputeNullKeyIfAbsent() {
+        final ConcurrentReferenceHashMap<K, V> map = getMap();
+        assertThrows(NullPointerException.class, () -> map.computeIfAbsent(null, k -> getSampleValues()[0]));
+    }
+
+    @Test
+    void testComputeNullKeyIfPresent() {
+        final ConcurrentReferenceHashMap<K, V> map = getMap();
+        assertThrows(NullPointerException.class, () -> map.computeIfPresent(null, (k, v) -> getSampleValues()[0]));
+    }
+
+    @Test
+    void testContainsNullKey() {
+        final ConcurrentReferenceHashMap<K, V> map = getMap();
+        assertThrows(NullPointerException.class, () -> map.containsKey(null));
+    }
+
+    @Test
+    void testContainsNullValue() {
+        final ConcurrentReferenceHashMap<K, V> map = getMap();
+        assertThrows(NullPointerException.class, () -> map.containsValue(null));
+    }
+
+    @Test
+    void testGetNullKey() {
+        final ConcurrentReferenceHashMap<K, V> map = getMap();
+        assertThrows(NullPointerException.class, () -> map.get(null));
+    }
+
+    @Test
+    void testGetNullKeyOrDefault() {
+        final ConcurrentReferenceHashMap<K, V> map = getMap();
+        assertThrows(NullPointerException.class, () -> map.getOrDefault(null, getSampleValues()[0]));
+    }
+
+    @Test
+    void testMergeNullKey() {
+        final ConcurrentReferenceHashMap<K, V> map = getMap();
+        final V[] sampleValues = getSampleValues();
+        assertThrows(NullPointerException.class, () -> map.merge(null, sampleValues[0], (k, v) -> sampleValues[0]));
+    }
+
+    @Test
+    void testMergeNullValue() {
+        final ConcurrentReferenceHashMap<K, V> map = getMap();
+        assertThrows(NullPointerException.class, () -> map.merge(getSampleKeys()[0], null, (k, v) -> null));
+        map.merge(getSampleKeys()[0], getSampleValues()[0], (k, v) -> null);
+    }
+
+    @Test
+    void testPutNullKey() {
+        final ConcurrentReferenceHashMap<K, V> map = getMap();
+        final K key = null;
+        assertThrows(NullPointerException.class, () -> map.put(key, getSampleValues()[0]));
+    }
+
+    @Test
+    void testPutNullValue() {
+        final ConcurrentReferenceHashMap<K, V> map = getMap();
+        assertThrows(NullPointerException.class, () -> map.put(getSampleKeys()[0], null));
+    }
+
+    @Test
+    void testRemoveNullKey() {
+        final ConcurrentReferenceHashMap<K, V> map = getMap();
+        final K key = null;
+        final V[] values = getSampleValues();
+        assertThrows(NullPointerException.class, () -> map.remove(key, values[0]));
+        assertThrows(NullPointerException.class, () -> map.remove(key));
+        assertThrows(NullPointerException.class, () -> map.replace(key, values[0]));
+        assertThrows(NullPointerException.class, () -> map.replace(key, values[0], values[1]));
+    }
+
+    @Test
+    void testReplaceNullKey() {
+        final ConcurrentReferenceHashMap<K, V> map = getMap();
+        final K key = null;
+        final V[] values = getSampleValues();
+        assertThrows(NullPointerException.class, () -> map.remove(key, values[0]));
+        assertThrows(NullPointerException.class, () -> map.remove(key));
+        assertThrows(NullPointerException.class, () -> map.replace(key, values[0]));
+        assertThrows(NullPointerException.class, () -> map.replace(key, values[0], values[1]));
+    }
 }