[jira] [Updated] (COLLECTIONS-887) ConcurrentReferenceHashMap.remove(key), remove(key, value), replace(key, value), and replace(key, oldValue, newValue) throw NullPointerException inconsistently

["Gary D. Gregory (Jira)" <ji...@apache.org>]

     [ https://issues.apache.org/jira/browse/COLLECTIONS-887?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

Gary D. Gregory updated COLLECTIONS-887:
----------------------------------------
    Summary: ConcurrentReferenceHashMap.remove(key), remove(key, value), replace(key, value), and replace(key, oldValue, newValue) throw NullPointerException inconsistently  (was: ConcurrentReferenceHashMap.remove(key), remove(key, value), replace(key, value), and replace(key, oldValue, newValue) throw NullPointerException inconsistently depending on map configuration, which contradicts javadoc)

> ConcurrentReferenceHashMap.remove(key), remove(key, value), replace(key, value), and replace(key, oldValue, newValue) throw NullPointerException inconsistently
> ---------------------------------------------------------------------------------------------------------------------------------------------------------------
>
>                 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}
> h5. Proposed Change
> Add {{Objects.requireNonNull(key, "key")}} to the affected methods to reflect the current behavior as in the class level the documentation states: {{this class does <em>not</em> allow \{@code null} to be used as a key or value.}}
> Submitted a pr [https://github.com/apache/commons-collections/pull/674] which added {{Objects.requireNonNull(key, "key")}} to the affected methods to reflect the current behavior.



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