From 75733fdfe7421190f25f291ddaee3ac82d1c36ec Mon Sep 17 00:00:00 2001 From: Naveed Khan Date: Mon, 5 Oct 2026 23:11:38 +0530 Subject: [PATCH] keep DualTreeBidiMap consistent when changed through a sub map A sub map view decorates an inner bidi map whose normalMap is restricted to the range while its reverseMap is the full map. Clearing the keySet, values or entrySet of a sub map wiped the whole reverse map, put could leave two keys with one value or drop a mapping before rejecting a key out of range, and values().remove() could drop the reverse entry of a value outside the range. The view now decorates a ViewBidiMap that handles the restricted normalMap. --- .../bidimap/AbstractDualBidiMap.java | 7 +- .../collections4/bidimap/DualTreeBidiMap.java | 53 +++++++++- .../bidimap/AbstractSortedBidiMapTest.java | 99 +++++++++++++++++++ 3 files changed, 154 insertions(+), 5 deletions(-) diff --git a/src/main/java/org/apache/commons/collections4/bidimap/AbstractDualBidiMap.java b/src/main/java/org/apache/commons/collections4/bidimap/AbstractDualBidiMap.java index b7d6a2532f..6e07125389 100644 --- a/src/main/java/org/apache/commons/collections4/bidimap/AbstractDualBidiMap.java +++ b/src/main/java/org/apache/commons/collections4/bidimap/AbstractDualBidiMap.java @@ -387,7 +387,7 @@ protected Values(final AbstractDualBidiMap parent) { @Override public boolean contains(final Object value) { - return parent.reverseMap.containsKey(value); + return parent.containsValue(value); } @Override @@ -397,9 +397,8 @@ public Iterator 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; diff --git a/src/main/java/org/apache/commons/collections4/bidimap/DualTreeBidiMap.java b/src/main/java/org/apache/commons/collections4/bidimap/DualTreeBidiMap.java index dbf80c8cef..66399fd0ed 100644 --- a/src/main/java/org/apache/commons/collections4/bidimap/DualTreeBidiMap.java +++ b/src/main/java/org/apache/commons/collections4/bidimap/DualTreeBidiMap.java @@ -167,6 +167,57 @@ public String toString() { } } + /** + * The bidi map decorated by a {@link ViewMap}. + *

+ * 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. + *

+ * + * @param The type of the keys. + * @param The type of the values. + */ + private static final class ViewBidiMap extends DualTreeBidiMap { + + private static final long serialVersionUID = 1L; + + /** The normal map of the full bidi map. */ + private final transient Map fullMap; + + ViewBidiMap(final DualTreeBidiMap bidi, final SortedMap sm) { + super(sm, bidi.reverseMap, bidi.inverseBidiMap); + fullMap = bidi instanceof ViewBidiMap ? ((ViewBidiMap) 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. * @@ -185,7 +236,7 @@ protected ViewMap(final DualTreeBidiMap bidi, final SortedMap 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 diff --git a/src/test/java/org/apache/commons/collections4/bidimap/AbstractSortedBidiMapTest.java b/src/test/java/org/apache/commons/collections4/bidimap/AbstractSortedBidiMapTest.java index 4e5ae32e24..6a04508cd7 100644 --- a/src/test/java/org/apache/commons/collections4/bidimap/AbstractSortedBidiMapTest.java +++ b/src/test/java/org/apache/commons/collections4/bidimap/AbstractSortedBidiMapTest.java @@ -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; @@ -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, V extends Comparable> extends AbstractOrderedBidiMapTest { + /** + * Every way of removing all the mappings of a sub map through its collection views. + */ + static Stream getViewClears() { + return Stream.of( + arguments("keySet().clear", (Consumer>) map -> map.keySet().clear()), + arguments("keySet().retainAll", (Consumer>) map -> map.keySet().retainAll(Collections.emptySet())), + arguments("values().clear", (Consumer>) map -> map.values().clear()), + arguments("values().retainAll", (Consumer>) map -> map.values().retainAll(Collections.emptySet())), + arguments("entrySet().clear", (Consumer>) map -> map.entrySet().clear()), + arguments("entrySet().retainAll", (Consumer>) map -> map.entrySet().retainAll(Collections.emptySet()))); + } + protected List sortedKeys; protected List sortedValues = new ArrayList<>(); protected SortedSet sortedNewValues = new TreeSet<>(); @@ -87,6 +108,14 @@ public BulkTest bulkTestTailMap() { return new AbstractSortedMapTest.TestTailMap<>((AbstractBidiMapTest) this); } + /** + * {@inheritDoc} + */ + @Override + public SortedBidiMap getMap() { + return (SortedBidiMap) super.getMap(); + } + @Override public boolean isAllowNullKey() { return false; @@ -168,6 +197,20 @@ void testBidiClearByHeadMap() { assertFalse(sub.containsValue(toKeyValue)); } + @ParameterizedTest(name = "{0}") + @MethodSource("getViewClears") + void testBidiClearByHeadMapView(final String description, final Consumer> 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) getConfirmed()).headMap(toKey)); + verify(); + } + @Test void testBidiClearBySubMap() { if (!isRemoveSupported()) { @@ -297,6 +340,47 @@ void testBidiHeadMapContains() { assertFalse(head.containsValue(secondValue)); } + @Test + void testBidiPutByHeadMapKeyOutOfRange() { + if (!isPutAddSupported()) { + return; + } + resetFull(); + final List keys = new ArrayList<>(getMap().keySet()); + final K toKey = keys.get(2); + final V firstValue = getMap().get(keys.get(0)); + final SortedMap 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 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 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()) { @@ -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 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()) {