Skip to content

Commit

Permalink
8308167: SequencedMap::firstEntry throws NPE when first entry has nul…
Browse files Browse the repository at this point in the history
…l key or value

Reviewed-by: bchristi
  • Loading branch information
Stuart Marks committed Jun 6, 2023
1 parent 4b15349 commit 6d155a4
Show file tree
Hide file tree
Showing 4 changed files with 214 additions and 6 deletions.
10 changes: 6 additions & 4 deletions src/java.base/share/classes/java/util/SequencedMap.java
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@

package java.util;

import jdk.internal.util.NullableKeyValueHolder;

/**
* A Map that has a well-defined encounter order, that supports operations at both ends, and
* that is reversible. The <a href="SequencedCollection.html#encounter">encounter order</a>
Expand Down Expand Up @@ -148,7 +150,7 @@ public interface SequencedMap<K, V> extends Map<K, V> {
*/
default Map.Entry<K,V> firstEntry() {
var it = entrySet().iterator();
return it.hasNext() ? Map.Entry.copyOf(it.next()) : null;
return it.hasNext() ? new NullableKeyValueHolder<>(it.next()) : null;
}

/**
Expand All @@ -165,7 +167,7 @@ default Map.Entry<K,V> firstEntry() {
*/
default Map.Entry<K,V> lastEntry() {
var it = reversed().entrySet().iterator();
return it.hasNext() ? Map.Entry.copyOf(it.next()) : null;
return it.hasNext() ? new NullableKeyValueHolder<>(it.next()) : null;
}

/**
Expand All @@ -185,7 +187,7 @@ default Map.Entry<K,V> lastEntry() {
default Map.Entry<K,V> pollFirstEntry() {
var it = entrySet().iterator();
if (it.hasNext()) {
var entry = Map.Entry.copyOf(it.next());
var entry = new NullableKeyValueHolder<>(it.next());
it.remove();
return entry;
} else {
Expand All @@ -210,7 +212,7 @@ default Map.Entry<K,V> pollFirstEntry() {
default Map.Entry<K,V> pollLastEntry() {
var it = reversed().entrySet().iterator();
if (it.hasNext()) {
var entry = Map.Entry.copyOf(it.next());
var entry = new NullableKeyValueHolder<>(it.next());
it.remove();
return entry;
} else {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,166 @@
/*
* Copyright (c) 2023, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
* under the terms of the GNU General Public License version 2 only, as
* published by the Free Software Foundation. Oracle designates this
* particular file as subject to the "Classpath" exception as provided
* by Oracle in the LICENSE file that accompanied this code.
*
* This code is distributed in the hope that it will be useful, but WITHOUT
* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
* FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
* version 2 for more details (a copy is included in the LICENSE file that
* accompanied this code).
*
* You should have received a copy of the GNU General Public License version
* 2 along with this work; if not, write to the Free Software Foundation,
* Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
*
* Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
* or visit www.oracle.com if you need additional information or have any
* questions.
*/

package jdk.internal.util;

import java.util.Map;
import java.util.Objects;

/**
* An immutable container for a key and a value, both of which are nullable.
*
* <p>This is a <a href="{@docRoot}/java.base/java/lang/doc-files/ValueBased.html">value-based</a>
* class; programmers should treat instances that are
* {@linkplain #equals(Object) equal} as interchangeable and should not
* use instances for synchronization, or unpredictable behavior may
* occur. For example, in a future release, synchronization may fail.
*
* @apiNote
* This class is not exported. Instances are created by various Map implementations
* when they need a Map.Entry that isn't connected to the Map.
*
* <p>This class differs from AbstractMap.SimpleImmutableEntry in that it is not
* serializable and that it is final. This class differs from java.util.KeyValueHolder
* in that the key and value are nullable.
*
* <p>In principle this class could be a variation on KeyValueHolder. However,
* making that class selectively support nullable keys and values is quite intricate.
* Various specifications (such as Map.ofEntries and Map.entry) specify non-nullability
* of the key and the value. Map.Entry.copyOf also requires non-null keys and values;
* but it simply passes through KeyValueHolder instances, assuming their keys and values
* are non-nullable. If a KVH with nullable keys and values were introduced, some way
* to distinguish it would be necessary. This could be done by introducing a subclass
* (requiring KVH to be made non-final) or by introducing some kind of "mode" field
* (potentially increasing the size of every KVH instance, though another field could
* probably fit into the object's padding in most JVMs.) More critically, a mode field
* would have to be checked in all the right places to get the right behavior.
*
* <p>A longer range possibility is to selectively relax the restrictions against nulls in
* Map.entry and Map.Entry.copyOf. This would also require some intricate specification
* changes and corresponding implementation changes (e.g., the implementations backing
* Map.of might still need to reject nulls, and so would Map.ofEntries) but allowing
* a Map.Entry itself to contain nulls seems beneficial in general. If this is done,
* merging KeyValueHolder and NullableKeyValueHolder should be reconsidered.
*
* @param <K> the key type
* @param <V> the value type
*/
@jdk.internal.ValueBased
public final class NullableKeyValueHolder<K,V> implements Map.Entry<K,V> {
final K key;
final V value;

/**
* Constructs a NullableKeyValueHolder.
*
* @param k the key, may be null
* @param v the value, may be null
*/
public NullableKeyValueHolder(K k, V v) {
key = k;
value = v;
}

/**
* Constructs a NullableKeyValueHolder from a Map.Entry. No need for an
* idempotent copy at this time.
*
* @param entry the entry, must not be null
*/
public NullableKeyValueHolder(Map.Entry<K,V> entry) {
Objects.requireNonNull(entry);
key = entry.getKey();
value = entry.getValue();
}

/**
* Gets the key from this holder.
*
* @return the key, may be null
*/
@Override
public K getKey() {
return key;
}

/**
* Gets the value from this holder.
*
* @return the value, may be null
*/
@Override
public V getValue() {
return value;
}

/**
* Throws {@link UnsupportedOperationException}.
*
* @param value ignored
* @return never returns normally
*/
@Override
public V setValue(V value) {
throw new UnsupportedOperationException("not supported");
}

/**
* Compares the specified object with this entry for equality.
* Returns {@code true} if the given object is also a map entry and
* the two entries' keys and values are equal.
*/
@Override
public boolean equals(Object o) {
return o instanceof Map.Entry<?, ?> e
&& Objects.equals(key, e.getKey())
&& Objects.equals(value, e.getValue());
}

private int hash(Object obj) {
return (obj == null) ? 0 : obj.hashCode();
}

/**
* Returns the hash code value for this map entry. The hash code
* is {@code key.hashCode() ^ value.hashCode()}.
*/
@Override
public int hashCode() {
return hash(key) ^ hash(value);
}

/**
* Returns a String representation of this map entry. This
* implementation returns the string representation of this
* entry's key followed by the equals character ("{@code =}")
* followed by the string representation of this entry's value.
*
* @return a String representation of this map entry
*/
@Override
public String toString() {
return key + "=" + value;
}
}
12 changes: 10 additions & 2 deletions test/jdk/java/util/AbstractMap/SimpleEntries.java
Original file line number Diff line number Diff line change
Expand Up @@ -23,12 +23,14 @@

/*
* @test
* @bug 4904074 6328220 6330389
* @summary Basic tests for SimpleEntry, SimpleImmutableEntry
* @bug 4904074 6328220 6330389 8308167
* @modules java.base/jdk.internal.util
* @summary Basic tests for several Map.Entry implementations
* @author Martin Buchholz
*/

import java.util.Map;
import jdk.internal.util.NullableKeyValueHolder;

import static java.util.AbstractMap.SimpleEntry;
import static java.util.AbstractMap.SimpleImmutableEntry;
Expand All @@ -40,8 +42,12 @@ public class SimpleEntries {
private static void realMain(String[] args) throws Throwable {
testEntry(new SimpleEntry<String,Long>(k,v));
testEntry(new SimpleImmutableEntry<String,Long>(k,v));
testEntry(Map.entry(k,v));
testEntry(Map.Entry.copyOf(Map.entry(k,v)));
testEntry(new NullableKeyValueHolder(k,v));
testNullEntry(new SimpleEntry<String,Long>(null,null));
testNullEntry(new SimpleImmutableEntry<String,Long>(null,null));
testNullEntry(new NullableKeyValueHolder(null,null));
}

private static void testEntry(Map.Entry<String,Long> e) {
Expand All @@ -52,6 +58,7 @@ private static void testEntry(Map.Entry<String,Long> e) {
check(! e.equals(null));
equal(e, new SimpleImmutableEntry<String,Long>(k,v));
equal(e.toString(), k+"="+v);
check(e.hashCode() == 101575); // hash("foo") ^ hash(1L)
if (e instanceof SimpleEntry) {
equal(e.setValue(v2), v);
equal(e.getValue(), v2);
Expand All @@ -70,6 +77,7 @@ private static void testNullEntry(Map.Entry<String,Long> e) {
equal(e, new SimpleEntry<String,Long>(null, null));
equal(e, new SimpleImmutableEntry<String,Long>(null, null));
equal(e.toString(), "null=null");
check(e.hashCode() == 0);
if (e instanceof SimpleEntry) {
equal(e.setValue(v), null);
equal(e.getValue(), v);
Expand Down
32 changes: 32 additions & 0 deletions test/jdk/java/util/SequencedCollection/BasicMap.java
Original file line number Diff line number Diff line change
Expand Up @@ -258,6 +258,16 @@ public Iterator<Object[]> viewAddThrows() {
return cases.iterator();
}

@DataProvider(name="nullableEntries")
public Iterator<Object[]> nullableEntries() {
return Arrays.asList(
new Object[] { "firstEntry" },
new Object[] { "lastEntry" },
new Object[] { "pollFirstEntry" },
new Object[] { "pollLastEntry" }
).iterator();
}

// ========== Assertions ==========

/**
Expand Down Expand Up @@ -519,6 +529,11 @@ public void checkChecked(SequencedMap<String, Integer> map) {
assertThrows(CCE, () -> { objMap.reversed().sequencedEntrySet().reversed().getFirst().setValue(new Object()); });
}

public void checkEntry(Map.Entry<String, Integer> entry, String key, Integer value) {
assertEquals(entry.getKey(), key);
assertEquals(entry.getValue(), value);
}

// ========== Tests ==========

@Test(dataProvider="all")
Expand Down Expand Up @@ -889,4 +904,21 @@ public void testEntrySetAddThrows(String label,
assertThrows(UOE, () -> entrySet.addFirst(Map.entry("x", 99)));
checkContents(map, baseref);
}

@Test(dataProvider="nullableEntries")
public void testNullableKeyValue(String mode) {
// TODO this relies on LHM to inherit SequencedMap default
// methods which are actually being tested here.
SequencedMap<String, Integer> map = new LinkedHashMap<>();
map.put(null, 1);
map.put("two", null);

switch (mode) {
case "firstEntry" -> checkEntry(map.firstEntry(), null, 1);
case "lastEntry" -> checkEntry(map.lastEntry(), "two", null);
case "pollFirstEntry" -> checkEntry(map.pollFirstEntry(), null, 1);
case "pollLastEntry" -> checkEntry(map.pollLastEntry(), "two", null);
default -> throw new AssertionError("illegal mode " + mode);
}
}
}

1 comment on commit 6d155a4

@openjdk-notifier
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please sign in to comment.