[
https://issues.apache.org/jira/browse/COLLECTIONS-887?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18069314#comment-18069314
]
Gary D. Gregory commented on COLLECTIONS-887:
---------------------------------------------
Hello [~pppaul]
Thank you for your report. The changes are now in git master and snapshot
builds in
https://repository.apache.org/content/repositories/snapshots/org/apache/commons/commons-collections4/4.6.0-SNAPSHOT/
> 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)
