[jira] [Created] (COLLECTIONS-887) ConcurrentReferenceHashMap.remove(key), remove(key, value), replace(key, value), and replace(key, oldValue, newValue) throw NullPointerException inconsistently depending on map configuration, which contradicts javadoc

["Partha Paul (Jira)" <ji...@apache.org>]

Partha Paul created COLLECTIONS-887:
---------------------------------------

             Summary: ConcurrentReferenceHashMap.remove(key), remove(key, value), replace(key, value), and replace(key, oldValue, newValue) throw NullPointerException inconsistently depending on map configuration, which contradicts javadoc
                 Key: COLLECTIONS-887
                 URL: https://issues.apache.org/jira/browse/COLLECTIONS-887
             Project: Commons Collections
          Issue Type: Bug
    Affects Versions: 4.5.0
         Environment: * commons-collections: 4.5.0 (and earlier versions)
 * Java version: (javac 17.0.12)
            Reporter: Partha Paul


The Javadoc for the following methods in {{ConcurrentReferenceHashMap}} explicitly states that a {{NullPointerException}} should be thrown when a null key is passed. However, none of the methods have an explicit null guard. They rely on {{hashOf(key)}} to throw {{NullPointerException}} accidentally via {{{}null.hashCode(){}}}. This means the null key behaviour is inconsistent depending on the map configuration:
 - When {{IDENTITY_COMPARISONS}} is disabled: {{hashOf(null)}} calls {{null.hashCode()}} which accidentally throws {{NullPointerException}}
 - When {{IDENTITY_COMPARISONS}} is enabled: {{hashOf(null)}} calls {{System.identityHashCode(null)}} which returns {{0}} and no {{NullPointerException}} is thrown at all

The Javadoc contract says {{@throws NullPointerException if the specified key is null}} unconditionally, with no mention of map configuration affecting this behavior.
Affected methods:
{code:java}
remove(Object key)
remove(Object key, Object value)
replace(K key, V value)
replace(K key, V oldValue, V newValue)
{code}
h5. Javadoc (Current)
{code:java}
remove(Object key): @throws NullPointerException if the specified key is null

remove(Object key, Object value): @throws NullPointerException if the specified key is null

replace(K key, V value): @throws NullPointerException if the specified key or value is null

replace(K key, V oldValue, V newValue): @throws NullPointerException if any of the arguments are null
{code}
h5. Steps to Reproduce
{code:java}
@Test
void testNullKey() {
  ConcurrentReferenceHashMap<String, String> map = ConcurrentReferenceHashMap
       .<String, String>builder()
       .weakKeys()
       .softValues()
     //.setOptions(EnumSet.of(Option.IDENTITY_COMPARISONS))
       .get();
  
  map.put("testKey", "testValue");

  assertThrows(NullPointerException.class, () -> map.remove(null, "testValue"));
  assertThrows(NullPointerException.class, () -> map.remove(null));
  assertThrows(NullPointerException.class, () -> map.replace(null, "value"));
  assertThrows(NullPointerException.class, () -> map.replace(null, "oldValue", "newValue"));
}
{code}
The test above passes only when {{IDENTITY_COMPARISONS}} is not set. When {{IDENTITY_COMPARISONS}} is enabled, all four assertions fail because no {{NullPointerException}} is thrown.
h5. Implementation (Current)
{code:java}
@Override
public V remove(final Object key) {
    final int hash = hashOf(key);
    return segmentFor(hash).remove(key, hash, null, false);
}

@Override
public boolean remove(final Object key, final Object value) {
    final int hash = hashOf(key);
    ....
    return segmentFor(hash).remove(key, hash, value, false) != null;
}

@Override
public V replace(final K key, final V value) {
    Objects.requireNonNull(value, "value");
    final int hash = hashOf(key);
    return segmentFor(hash).replace(key, hash, value);
}

@Override
public boolean replace(final K key, final V oldValue, final V newValue) {
    Objects.requireNonNull(oldValue, "oldValue");
    Objects.requireNonNull(newValue, "newValue");
    final int hash = hashOf(key);
    return segmentFor(hash).replace(key, hash, oldValue, newValue);
}
{code}
Happy to submit a pull request for this if this involves adding an explicit null check via {{Objects.requireNonNull(key, "key")}} to the affected methods or updating the documentation to better reflect the current behavior.



--
This message was sent by Atlassian Jira
(v8.20.10#820010)