Fix inconsistencies in unmodifiable maps. #1617
Conversation
| { | ||
| this.put(entry.getKey(), entry.getValue()); | ||
| } | ||
| map.forEach(this::put); |
There was a problem hiding this comment.
This change fixes assertions in HashBiMapNoIteratorTest, which now inherits more tests/coverage from MapTestCase.
| { | ||
| throw new UnsupportedOperationException("Cannot mutate " + this.getClass().getSimpleName()); | ||
| } | ||
| return result; |
There was a problem hiding this comment.
Implementations like these are debatable, but are done consistently elsewhere like in UnmodifiableMutableMap. The trait-based testing approach is applying identical assertions to all the unmodifiable map wrappers and exposing differences like these.
| @Override | ||
| public V putIfAbsent(K key, V value) | ||
| { | ||
| throw new UnsupportedOperationException("Cannot call putIfAbsent() on " + this.getClass().getSimpleName()); |
There was a problem hiding this comment.
Note that other composite methods like putIfAbsent and the compute* methods all consistently throw, regardless of if the map would be mutated. These overrides make sure that the exception is consistently UnsupportedOperationException and that any lambdas passed into the compute* methods are never invoked.
| { | ||
| throw new UnsupportedOperationException("Cannot call replaceAll() on " + this.getClass().getSimpleName()); | ||
| } | ||
|
|
There was a problem hiding this comment.
We still rely on abstract classes a lot, rather than an interface hierarchy with default methods, leading to lots of duplication. Cleaning that up is a bigger task.
| @@ -770,7 +770,7 @@ public static <K, V> void forEachKeyValue(Map<K, V> map, Procedure2<? super K, ? | |||
| { | |||
| if (map == null) | |||
| { | |||
| throw new IllegalArgumentException("Cannot perform a forEachKeyValue on null"); | |||
| throw new NullPointerException("Cannot perform a forEachKeyValue on null"); | |||
There was a problem hiding this comment.
Most implementations of putAll() already throw NPE when the argument is null, but a few delegate to this method and throw IllegalArgumentException.
| @@ -34,7 +35,7 @@ | |||
| import static org.junit.Assert.assertThrows; | |||
| import static org.junit.Assert.assertTrue; | |||
|
|
|||
| public interface MapIterableTestCase extends RichIterableWithDuplicatesTestCase | |||
| public interface MapIterableTestCase extends RichIterableWithDuplicatesTestCase, MapTestCase | |||
There was a problem hiding this comment.
This is a tricky detail. Every MapIterable in Eclipse Collections implements Map, even the immutable ones. That means we ought to be running tests against methods like Map.put() for every MapIterable. This is the core of the change that led to discovering so many inconsistencies between Mutable, Immutable, and Unmodifiable maps.
| @Test | ||
| default void Map_put() | ||
| { | ||
| Map<Integer, String> map = (Map<Integer, String>) this.newWithKeysValues(3, "Three", 2, "Two", 1, "One"); |
There was a problem hiding this comment.
newWithKeysValues() returns Object in these tests, to allow for Map and MapIterable to have the same assertions. The result of the method call is cast to Map.
|
|
||
| <K, V> Map<K, V> newWithKeysValues(Object... elements); | ||
| // Returns Object to allow subclasses to return either Map or MapIterable, and be cast to Map either way | ||
| <K, V> Object newWithKeysValues(Object... elements); |
There was a problem hiding this comment.
Here's where we return Object to allow testing of Map and MapIterable together. The generics are funny here. Deleting them leads to the overriding methods not being recognized as overrides.
| { | ||
| assertNull(result.put(random.nextDouble(), each)); | ||
| } | ||
| return Collections.unmodifiableMap(result); |
There was a problem hiding this comment.
This is an important test to ensure consistency across implementations. This runs the same assertions against Collections.unmodifiableMap(new HashMap<>())
| return null; | ||
| })); | ||
| Assert.assertEquals(this.newWithKeysValues(1, "1", 2, "2", 3, "3"), map); | ||
| } |
There was a problem hiding this comment.
These assertions are pulled up and now apply to all MutableMapIterables, not just MutableMaps. This led to finding some inconsistencies in BiMaps.
motlin
force-pushed
the
unmodifiable-map
branch
3 times, most recently
from
b674cee
to
62d2e24
Compare
motlin
force-pushed
the
unmodifiable-map
branch
2 times, most recently
from
8968fab
to
e374155
Compare
motlin
force-pushed
the
unmodifiable-map
branch
3 times, most recently
from
45caeb7
to
7975b90
Compare
motlin
force-pushed
the
unmodifiable-map
branch
from
7975b90
to
b1b96aa
Compare
No description provided.