From 199716c53fd6c27a530e2ca3250e8a88b16aafac Mon Sep 17 00:00:00 2001 From: Stephen Colebourne Date: Thu, 1 Apr 2004 22:18:12 +0000 Subject: [PATCH] Bug fix where Map/BidiMap implementations only checked key and not value in entrySet contains(Object) and remove(Object) git-svn-id: https://svn.apache.org/repos/asf/jakarta/commons/proper/collections/trunk@131620 13f79535-47bb-0310-9956-ffa450edef68 --- RELEASE-NOTES.html | 1 + .../bidimap/AbstractDualBidiMap.java | 14 ++-- .../collections/map/AbstractHashedMap.java | 30 +++++---- .../collections/map/ListOrderedMap.java | 15 +++-- .../collections/map/AbstractTestMap.java | 64 ++++++++++++++++++- 5 files changed, 97 insertions(+), 27 deletions(-) diff --git a/RELEASE-NOTES.html b/RELEASE-NOTES.html index 6993cfcc8..3f8635d50 100644 --- a/RELEASE-NOTES.html +++ b/RELEASE-NOTES.html @@ -43,6 +43,7 @@ No interface changes, or deprecations have occurred.

BUG FIXES

diff --git a/src/java/org/apache/commons/collections/bidimap/AbstractDualBidiMap.java b/src/java/org/apache/commons/collections/bidimap/AbstractDualBidiMap.java index b29b3e6af..dadb37cb9 100644 --- a/src/java/org/apache/commons/collections/bidimap/AbstractDualBidiMap.java +++ b/src/java/org/apache/commons/collections/bidimap/AbstractDualBidiMap.java @@ -36,7 +36,7 @@ import org.apache.commons.collections.keyvalue.AbstractMapEntryDecorator; * @see DualHashBidiMap * @see DualTreeBidiMap * @since Commons Collections 3.0 - * @version $Id: AbstractDualBidiMap.java,v 1.10 2004/02/18 00:57:39 scolebourne Exp $ + * @version $Id: AbstractDualBidiMap.java,v 1.11 2004/04/01 22:18:12 scolebourne Exp $ * * @author Matthew Hawthorne * @author Stephen Colebourne @@ -515,10 +515,14 @@ public abstract class AbstractDualBidiMap implements BidiMap { return false; } Map.Entry entry = (Map.Entry) obj; - if (parent.containsKey(entry.getKey())) { - Object value = parent.maps[0].remove(entry.getKey()); - parent.maps[1].remove(value); - return true; + Object key = entry.getKey(); + if (parent.containsKey(key)) { + Object value = parent.maps[0].get(key); + if (value == null ? entry.getValue() == null : value.equals(entry.getValue())) { + parent.maps[0].remove(key); + parent.maps[1].remove(value); + return true; + } } return false; } diff --git a/src/java/org/apache/commons/collections/map/AbstractHashedMap.java b/src/java/org/apache/commons/collections/map/AbstractHashedMap.java index c29a9cd8b..e5b4e829a 100644 --- a/src/java/org/apache/commons/collections/map/AbstractHashedMap.java +++ b/src/java/org/apache/commons/collections/map/AbstractHashedMap.java @@ -46,7 +46,7 @@ import org.apache.commons.collections.MapIterator; * need for unusual subclasses is here. * * @since Commons Collections 3.0 - * @version $Revision: 1.13 $ $Date: 2004/03/13 15:54:34 $ + * @version $Revision: 1.14 $ $Date: 2004/04/01 22:18:12 $ * * @author java util HashMap * @author Stephen Colebourne @@ -367,12 +367,12 @@ public class AbstractHashedMap implements IterableMap { } /** - * Compares two keys for equals. - * This implementation uses the equals method. + * Compares two keys, in internal converted form, to see if they are equal. + * This implementation uses the equals method and assumes neither key is null. * Subclasses can override this to match differently. * - * @param key1 the first key to compare - * @param key2 the second key to compare + * @param key1 the first key to compare passed in from outside + * @param key2 the second key extracted from the entry via entry.key * @return true if equal */ protected boolean isEqualKey(Object key1, Object key2) { @@ -380,12 +380,12 @@ public class AbstractHashedMap implements IterableMap { } /** - * Compares two values for equals. - * This implementation uses the equals method. + * Compares two values, in external form, to see if they are equal. + * This implementation uses the equals method and assumes neither key is null. * Subclasses can override this to match differently. * - * @param value1 the first value to compare - * @param value2 the second value to compare + * @param value1 the first value to compare passed in from outside + * @param value2 the second value extracted from the entry via getValue() * @return true if equal */ protected boolean isEqualValue(Object value1, Object value2) { @@ -753,8 +753,6 @@ public class AbstractHashedMap implements IterableMap { /** * Gets the entrySet view of the map. * Changes made to the view affect this map. - * The Map Entry is not an independent object and changes as the - * iterator progresses. * To simply iterate through the entries, use {@link #mapIterator()}. * * @return the entrySet view @@ -801,7 +799,9 @@ public class AbstractHashedMap implements IterableMap { public boolean contains(Object entry) { if (entry instanceof Map.Entry) { - return parent.containsKey(((Map.Entry) entry).getKey()); + Map.Entry e = (Map.Entry) entry; + Entry match = parent.getEntry(e.getKey()); + return (match != null && match.equals(e)); } return false; } @@ -810,11 +810,13 @@ public class AbstractHashedMap implements IterableMap { if (obj instanceof Map.Entry == false) { return false; } + if (contains(obj) == false) { + return false; + } Map.Entry entry = (Map.Entry) obj; Object key = entry.getKey(); - boolean result = parent.containsKey(key); parent.remove(key); - return result; + return true; } public Iterator iterator() { diff --git a/src/java/org/apache/commons/collections/map/ListOrderedMap.java b/src/java/org/apache/commons/collections/map/ListOrderedMap.java index 4a2474244..29f4c3a00 100644 --- a/src/java/org/apache/commons/collections/map/ListOrderedMap.java +++ b/src/java/org/apache/commons/collections/map/ListOrderedMap.java @@ -35,7 +35,8 @@ import org.apache.commons.collections.keyvalue.AbstractMapEntry; import org.apache.commons.collections.list.UnmodifiableList; /** - * Decorates a Map to ensure that the order of addition is retained. + * Decorates a Map to ensure that the order of addition is retained + * using a List to maintain order. *

* The order will be used via the iterators and toArray methods on the views. * The order is also returned by the MapIterator. @@ -47,7 +48,7 @@ import org.apache.commons.collections.list.UnmodifiableList; * original position in the iteration. * * @since Commons Collections 3.0 - * @version $Revision: 1.12 $ $Date: 2004/02/18 01:13:19 $ + * @version $Revision: 1.13 $ $Date: 2004/04/01 22:18:12 $ * * @author Henri Yandell * @author Stephen Colebourne @@ -385,12 +386,12 @@ public class ListOrderedMap if (obj instanceof Map.Entry == false) { return false; } - Object key = ((Map.Entry) obj).getKey(); - if (parent.getMap().containsKey(key) == false) { - return false; + if (getEntrySet().contains(obj)) { + Object key = ((Map.Entry) obj).getKey(); + parent.remove(key); + return true; } - parent.remove(key); - return true; + return false; } public void clear() { diff --git a/src/test/org/apache/commons/collections/map/AbstractTestMap.java b/src/test/org/apache/commons/collections/map/AbstractTestMap.java index 843d91462..0ce5b5fd5 100644 --- a/src/test/org/apache/commons/collections/map/AbstractTestMap.java +++ b/src/test/org/apache/commons/collections/map/AbstractTestMap.java @@ -28,6 +28,7 @@ import java.util.Set; import org.apache.commons.collections.AbstractTestObject; import org.apache.commons.collections.BulkTest; import org.apache.commons.collections.collection.AbstractTestCollection; +import org.apache.commons.collections.keyvalue.DefaultMapEntry; import org.apache.commons.collections.set.AbstractTestSet; /** @@ -117,7 +118,7 @@ import org.apache.commons.collections.set.AbstractTestSet; * @author Rodney Waldhoff * @author Paul Jack * @author Stephen Colebourne - * @version $Revision: 1.8 $ $Date: 2004/02/18 01:20:37 $ + * @version $Revision: 1.9 $ $Date: 2004/04/01 22:18:12 $ */ public abstract class AbstractTestMap extends AbstractTestObject { @@ -1005,6 +1006,67 @@ public abstract class AbstractTestMap extends AbstractTestObject { assertTrue(map.size() == 0); assertTrue(entrySet.size() == 0); } + + //----------------------------------------------------------------------- + public void testEntrySetContains1() { + resetFull(); + Set entrySet = map.entrySet(); + Map.Entry entry = (Map.Entry) entrySet.iterator().next(); + assertEquals(true, entrySet.contains(entry)); + } + public void testEntrySetContains2() { + resetFull(); + Set entrySet = map.entrySet(); + Map.Entry entry = (Map.Entry) entrySet.iterator().next(); + Map.Entry test = new DefaultMapEntry(entry); + assertEquals(true, entrySet.contains(test)); + } + public void testEntrySetContains3() { + resetFull(); + Set entrySet = map.entrySet(); + Map.Entry entry = (Map.Entry) entrySet.iterator().next(); + Map.Entry test = new DefaultMapEntry(entry.getKey(), "A VERY DIFFERENT VALUE"); + assertEquals(false, entrySet.contains(test)); + } + + public void testEntrySetRemove1() { + if (!isRemoveSupported()) return; + resetFull(); + int size = map.size(); + Set entrySet = map.entrySet(); + Map.Entry entry = (Map.Entry) entrySet.iterator().next(); + Object key = entry.getKey(); + + assertEquals(true, entrySet.remove(entry)); + assertEquals(false, map.containsKey(key)); + assertEquals(size - 1, map.size()); + } + public void testEntrySetRemove2() { + if (!isRemoveSupported()) return; + resetFull(); + int size = map.size(); + Set entrySet = map.entrySet(); + Map.Entry entry = (Map.Entry) entrySet.iterator().next(); + Object key = entry.getKey(); + Map.Entry test = new DefaultMapEntry(entry); + + assertEquals(true, entrySet.remove(test)); + assertEquals(false, map.containsKey(key)); + assertEquals(size - 1, map.size()); + } + public void testEntrySetRemove3() { + if (!isRemoveSupported()) return; + resetFull(); + int size = map.size(); + Set entrySet = map.entrySet(); + Map.Entry entry = (Map.Entry) entrySet.iterator().next(); + Object key = entry.getKey(); + Map.Entry test = new DefaultMapEntry(entry.getKey(), "A VERY DIFFERENT VALUE"); + + assertEquals(false, entrySet.remove(test)); + assertEquals(true, map.containsKey(key)); + assertEquals(size, map.size()); + } //----------------------------------------------------------------------- /**