[
https://issues.apache.org/jira/browse/COLLECTIONS-887?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Partha Paul updated COLLECTIONS-887:
------------------------------------
Description:
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 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.
was:
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.
> 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
> Priority: Major
> Labels: documentation, documentation-update, implementation
>
> 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 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)
