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)
