Skip to content

keep DualTreeBidiMap consistent when changed through a sub map #745

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
rootvector2 wants to merge 1 commit into
base: master
Choose a base branch
from
Open

keep DualTreeBidiMap consistent when changed through a sub map #745

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 3 additions & 4 deletions src/main/java/org/apache/commons/collections4/bidimap/AbstractDualBidiMap.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -387,7 +387,7 @@ protected Values(final AbstractDualBidiMap<?, V> parent) {

@Override
public boolean contains(final Object value) {
return parent.reverseMap.containsKey(value);
return parent.containsValue(value);
}

@Override
Expand All @@ -397,9 +397,8 @@ public Iterator<V> iterator() {

@Override
public boolean remove(final Object value) {
if (parent.reverseMap.containsKey(value)) {
final Object key = parent.reverseMap.remove(value);
parent.normalMap.remove(key);
if (parent.containsValue(value)) {
parent.removeValue(value);
return true;
}
return false;
Expand Down
53 changes: 52 additions & 1 deletion src/main/java/org/apache/commons/collections4/bidimap/DualTreeBidiMap.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,57 @@ public String toString() {
}
}

/**
* The bidi map decorated by a {@link ViewMap}.
* <p>
* The normal map is restricted to the range of the view but the reverse map is the full map, so the operations
* which expect both maps to hold the same mappings are overridden.
* </p>
*
* @param <K> The type of the keys.
* @param <V> The type of the values.
*/
private static final class ViewBidiMap<K, V> extends DualTreeBidiMap<K, V> {

private static final long serialVersionUID = 1L;

/** The normal map of the full bidi map. */
private final transient Map<K, V> fullMap;

ViewBidiMap(final DualTreeBidiMap<K, V> bidi, final SortedMap<K, V> sm) {
super(sm, bidi.reverseMap, bidi.inverseBidiMap);
fullMap = bidi instanceof ViewBidiMap ? ((ViewBidiMap<K, V>) bidi).fullMap : bidi.normalMap;
}

@Override
public void clear() {
normalMap.values().forEach(reverseMap::remove);
normalMap.clear();
}

@Override
public boolean containsValue(final Object value) {
return reverseMap.containsKey(value) && normalMap.containsKey(reverseMap.get(value));
}

@Override
public V put(final K key, final V value) {
if (normalMap.containsKey(key)) {
reverseMap.remove(normalMap.get(key));
}
// the value may be mapped to a key out of range, which only the full map can remove
final boolean containsValue = reverseMap.containsKey(value);
final K valueKey = reverseMap.get(value);
// put first as a key out of range must be rejected before that mapping is removed
final V obj = normalMap.put(key, value);
if (containsValue) {
fullMap.remove(valueKey);
}
reverseMap.put(value, key);
return obj;
}
}

/**
* Internal sorted map view.
*
Expand All @@ -185,7 +236,7 @@ protected ViewMap(final DualTreeBidiMap<K, V> bidi, final SortedMap<K, V> sm) {
// the implementation is not great here...
// use the normalMap as the filtered map, but reverseMap as the full map
// this forces containsValue and clear to be overridden
super(new DualTreeBidiMap<>(sm, bidi.reverseMap, bidi.inverseBidiMap));
super(new ViewBidiMap<>(bidi, sm));
}

@Override
Expand Down
99 changes: 99 additions & 0 deletions src/test/java/org/apache/commons/collections4/bidimap/AbstractSortedBidiMapTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,12 @@

import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertThrows;
import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.junit.jupiter.params.provider.Arguments.arguments;

import java.util.ArrayList;
import java.util.Collection;
import java.util.Collections;
import java.util.Iterator;
import java.util.List;
Expand All @@ -30,17 +33,35 @@
import java.util.SortedSet;
import java.util.TreeMap;
import java.util.TreeSet;
import java.util.function.Consumer;
import java.util.stream.Stream;

import org.apache.commons.collections4.BulkTest;
import org.apache.commons.collections4.SortedBidiMap;
import org.apache.commons.collections4.map.AbstractSortedMapTest;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.Arguments;
import org.junit.jupiter.params.provider.MethodSource;

/**
* Tests {@link SortedBidiMap}.
*/
public abstract class AbstractSortedBidiMapTest<K extends Comparable<K>, V extends Comparable<V>> extends AbstractOrderedBidiMapTest<K, V> {

/**
* Every way of removing all the mappings of a sub map through its collection views.
*/
static Stream<Arguments> getViewClears() {
return Stream.of(
arguments("keySet().clear", (Consumer<Map<?, ?>>) map -> map.keySet().clear()),
arguments("keySet().retainAll", (Consumer<Map<?, ?>>) map -> map.keySet().retainAll(Collections.emptySet())),
arguments("values().clear", (Consumer<Map<?, ?>>) map -> map.values().clear()),
arguments("values().retainAll", (Consumer<Map<?, ?>>) map -> map.values().retainAll(Collections.emptySet())),
arguments("entrySet().clear", (Consumer<Map<?, ?>>) map -> map.entrySet().clear()),
arguments("entrySet().retainAll", (Consumer<Map<?, ?>>) map -> map.entrySet().retainAll(Collections.emptySet())));
}

protected List<K> sortedKeys;
protected List<V> sortedValues = new ArrayList<>();
protected SortedSet<V> sortedNewValues = new TreeSet<>();
Expand Down Expand Up @@ -87,6 +108,14 @@ public BulkTest bulkTestTailMap() {
return new AbstractSortedMapTest.TestTailMap<>((AbstractBidiMapTest) this);
}

/**
* {@inheritDoc}
*/
@Override
public SortedBidiMap<K, V> getMap() {
return (SortedBidiMap<K, V>) super.getMap();
}

@Override
public boolean isAllowNullKey() {
return false;
Expand Down Expand Up @@ -168,6 +197,20 @@ void testBidiClearByHeadMap() {
assertFalse(sub.containsValue(toKeyValue));
}

@ParameterizedTest(name = "{0}")
@MethodSource("getViewClears")
void testBidiClearByHeadMapView(final String description, final Consumer<Map<?, ?>> clear) {
if (!isRemoveSupported()) {
return;
}
resetFull();
final K toKey = new ArrayList<>(getMap().keySet()).get(2);
// the mappings out of the range of the sub map must be kept in both directions
clear.accept(getMap().headMap(toKey));
clear.accept(((SortedMap<K, V>) getConfirmed()).headMap(toKey));
verify();
}

@Test
void testBidiClearBySubMap() {
if (!isRemoveSupported()) {
Expand Down Expand Up @@ -297,6 +340,47 @@ void testBidiHeadMapContains() {
assertFalse(head.containsValue(secondValue));
}

@Test
void testBidiPutByHeadMapKeyOutOfRange() {
if (!isPutAddSupported()) {
return;
}
resetFull();
final List<K> keys = new ArrayList<>(getMap().keySet());
final K toKey = keys.get(2);
final V firstValue = getMap().get(keys.get(0));
final SortedMap<K, V> sub = getMap().headMap(toKey);
// the rejected put must not remove the mapping of the sub map which holds the value
assertThrows(IllegalArgumentException.class, () -> sub.put(toKey, firstValue));
verify();
}

@Test
void testBidiPutByTailMapValueOutOfRange() {
if (!isPutChangeSupported()) {
return;
}
resetFull();
final List<K> keys = new ArrayList<>(getMap().keySet());
final K first = keys.get(0);
final K second = keys.get(1);
final K fromKey = keys.get(2);
final K nestedFromKey = keys.get(3);
final V firstValue = getMap().get(first);
final V secondValue = getMap().get(second);
// as with a put in the full map, the key out of the range of the sub map loses the value
final SortedMap<K, V> sub = getMap().tailMap(fromKey);
assertEquals(getConfirmed().get(fromKey), sub.put(fromKey, firstValue));
getConfirmed().remove(first);
getConfirmed().put(fromKey, firstValue);
verify();
// and so it does through a sub map of the sub map
assertEquals(getConfirmed().get(nestedFromKey), sub.tailMap(nestedFromKey).put(nestedFromKey, secondValue));
getConfirmed().remove(second);
getConfirmed().put(nestedFromKey, secondValue);
verify();
}

@Test
void testBidiRemoveByHeadMap() {
if (!isRemoveSupported()) {
Expand Down Expand Up @@ -395,6 +479,21 @@ void testBidiRemoveByHeadMapEntrySet() {
assertFalse(set.contains(secondEntry));
}

@Test
void testBidiRemoveByHeadMapValuesOutOfRange() {
if (!isRemoveSupported()) {
return;
}
resetFull();
final K toKey = new ArrayList<>(getMap().keySet()).get(2);
final V toKeyValue = getMap().get(toKey);
final Collection<V> values = getMap().headMap(toKey).values();
// the value is mapped to a key out of the range of the sub map
assertFalse(values.contains(toKeyValue));
assertFalse(values.remove(toKeyValue));
verify();
}

@Test
void testBidiRemoveBySubMap() {
if (!isRemoveSupported()) {
Expand Down