From 3ff965d015d508382a4f66964732fbe6112d2511 Mon Sep 17 00:00:00 2001 From: Simulant Date: Thu, 3 Feb 2022 21:07:10 +0100 Subject: [PATCH 01/11] COLLECTIONS-803: prevent duplicate call to convertKey on put for CaseInsensitiveMap --- .../collections4/map/CaseInsensitiveMap.java | 64 +++++++++++++++++++ 1 file changed, 64 insertions(+) diff --git a/src/main/java/org/apache/commons/collections4/map/CaseInsensitiveMap.java b/src/main/java/org/apache/commons/collections4/map/CaseInsensitiveMap.java index e1e7daf011..9f14fb4a38 100644 --- a/src/main/java/org/apache/commons/collections4/map/CaseInsensitiveMap.java +++ b/src/main/java/org/apache/commons/collections4/map/CaseInsensitiveMap.java @@ -141,6 +141,70 @@ protected Object convertKey(final Object key) { return AbstractHashedMap.NULL; } + /** + * Puts a key-value mapping into this map. + * + * @param key the key to add + * @param value the value to add + * @return the value previously mapped to this key, null if none + */ + @Override + public V put(final K key, final V value) { + final Object convertedKey = convertKey(key); + final int hashCode = hash(convertedKey); + final int index = hashIndex(hashCode, data.length); + HashEntry entry = data[index]; + while (entry != null) { + if (entry.hashCode == hashCode && isEqualKey(convertedKey, entry.key)) { + final V oldValue = entry.getValue(); + updateEntry(entry, value); + return oldValue; + } + entry = entry.next; + } + + addMappingWithoutKeyConversion(index, hashCode, convertedKey, value); + return null; + } + + /** + * Adds a new key-value mapping into this map. + *

+ * This implementation calls {@code createEntry()}, {@code addEntry()} + * and {@code checkCapacity()}. + * It also handles changes to {@code modCount} and {@code size}. + * Subclasses could override to fully control adds to the map. + * + * @param hashIndex the index into the data array to store at + * @param hashCode the hash code of the key to add + * @param key the key to add + * @param value the value to add + */ + private void addMappingWithoutKeyConversion(final int hashIndex, final int hashCode, final Object key, final V value) { + modCount++; + final HashEntry entry = createEntryWithoutKeyConversion(data[hashIndex], hashCode, key, value); + addEntry(entry, hashIndex); + size++; + checkCapacity(); + } + + /** + * Creates an entry to store the key-value data. + *

+ * This implementation creates a new HashEntry instance. + * Subclasses can override this to return a different storage class, + * or implement caching. + * + * @param next the next entry in sequence + * @param hashCode the hash code to use + * @param key the key to store + * @param value the value to store + * @return the newly created entry + */ + private HashEntry createEntryWithoutKeyConversion(final HashEntry next, final int hashCode, final Object key, final V value) { + return new HashEntry<>(next, hashCode, key, value); + } + /** * Clones the map without cloning the keys or values. * From 065dd8bb4700cf134b281d4fb21f1d23137b9f4a Mon Sep 17 00:00:00 2001 From: Simulant Date: Thu, 3 Feb 2022 23:16:27 +0100 Subject: [PATCH 02/11] COLLECTIONS-803: add test verify convertKey is called only once --- .../collections4/map/CaseInsensitiveMapTest.java | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/src/test/java/org/apache/commons/collections4/map/CaseInsensitiveMapTest.java b/src/test/java/org/apache/commons/collections4/map/CaseInsensitiveMapTest.java index 1f6a0ef0b8..5c739e2621 100644 --- a/src/test/java/org/apache/commons/collections4/map/CaseInsensitiveMapTest.java +++ b/src/test/java/org/apache/commons/collections4/map/CaseInsensitiveMapTest.java @@ -24,6 +24,7 @@ import junit.framework.Test; import org.apache.commons.collections4.BulkTest; +import org.easymock.EasyMock; /** * Tests for the {@link CaseInsensitiveMap} implementation. @@ -147,4 +148,19 @@ public void testPutAll() { || !caseInsensitiveMap.containsValue("Three")); // ones collapsed assertEquals("Four", caseInsensitiveMap.get(null)); } + + // COLLECTIONS-803 + public void testPutConvertKeyOnlyOnce() { + CaseInsensitiveMap mock = EasyMock.partialMockBuilder(CaseInsensitiveMap.class) + .withConstructor(Map.class) + .withArgs(new HashMap<>()) + .addMockedMethod("convertKey") + .createMock(); + EasyMock.expect(mock.convertKey("Key")).andReturn("key").once(); + EasyMock.replay(mock); + + mock.put("Key", "value"); + + EasyMock.verify(mock); + } } From c4cb26e3f5998d5a034340c735026df91eb04b33 Mon Sep 17 00:00:00 2001 From: Simulant Date: Fri, 4 Feb 2022 11:20:32 +0100 Subject: [PATCH 03/11] COLLECTIONS-803: add unchecked suppression to new test case --- .../apache/commons/collections4/map/CaseInsensitiveMapTest.java | 1 + 1 file changed, 1 insertion(+) diff --git a/src/test/java/org/apache/commons/collections4/map/CaseInsensitiveMapTest.java b/src/test/java/org/apache/commons/collections4/map/CaseInsensitiveMapTest.java index 5c739e2621..5d4f1d8e4d 100644 --- a/src/test/java/org/apache/commons/collections4/map/CaseInsensitiveMapTest.java +++ b/src/test/java/org/apache/commons/collections4/map/CaseInsensitiveMapTest.java @@ -150,6 +150,7 @@ public void testPutAll() { } // COLLECTIONS-803 + @SuppressWarnings("unchecked") public void testPutConvertKeyOnlyOnce() { CaseInsensitiveMap mock = EasyMock.partialMockBuilder(CaseInsensitiveMap.class) .withConstructor(Map.class) From 3ac374a8ac5fec23e40d5d4fb4838149742ef33a Mon Sep 17 00:00:00 2001 From: Simulant Date: Tue, 10 May 2022 22:19:07 +0200 Subject: [PATCH 04/11] [COLLECTIONS-803] add JUnit Test annotation to new test case --- .../apache/commons/collections4/map/CaseInsensitiveMapTest.java | 1 + 1 file changed, 1 insertion(+) diff --git a/src/test/java/org/apache/commons/collections4/map/CaseInsensitiveMapTest.java b/src/test/java/org/apache/commons/collections4/map/CaseInsensitiveMapTest.java index 79d183b8be..46443053a3 100644 --- a/src/test/java/org/apache/commons/collections4/map/CaseInsensitiveMapTest.java +++ b/src/test/java/org/apache/commons/collections4/map/CaseInsensitiveMapTest.java @@ -155,6 +155,7 @@ public void testPutAll() { } // COLLECTIONS-803 + @Test @SuppressWarnings("unchecked") public void testPutConvertKeyOnlyOnce() { CaseInsensitiveMap mock = EasyMock.partialMockBuilder(CaseInsensitiveMap.class) From 70e7b29a4203185f2a14fb76bdfe2599389011f0 Mon Sep 17 00:00:00 2001 From: Simulant Date: Thu, 3 Feb 2022 21:07:10 +0100 Subject: [PATCH 05/11] COLLECTIONS-803: prevent duplicate call to convertKey on put for CaseInsensitiveMap --- .../collections4/map/CaseInsensitiveMap.java | 64 +++++++++++++++++++ 1 file changed, 64 insertions(+) diff --git a/src/main/java/org/apache/commons/collections4/map/CaseInsensitiveMap.java b/src/main/java/org/apache/commons/collections4/map/CaseInsensitiveMap.java index e1e7daf011..9f14fb4a38 100644 --- a/src/main/java/org/apache/commons/collections4/map/CaseInsensitiveMap.java +++ b/src/main/java/org/apache/commons/collections4/map/CaseInsensitiveMap.java @@ -141,6 +141,70 @@ protected Object convertKey(final Object key) { return AbstractHashedMap.NULL; } + /** + * Puts a key-value mapping into this map. + * + * @param key the key to add + * @param value the value to add + * @return the value previously mapped to this key, null if none + */ + @Override + public V put(final K key, final V value) { + final Object convertedKey = convertKey(key); + final int hashCode = hash(convertedKey); + final int index = hashIndex(hashCode, data.length); + HashEntry entry = data[index]; + while (entry != null) { + if (entry.hashCode == hashCode && isEqualKey(convertedKey, entry.key)) { + final V oldValue = entry.getValue(); + updateEntry(entry, value); + return oldValue; + } + entry = entry.next; + } + + addMappingWithoutKeyConversion(index, hashCode, convertedKey, value); + return null; + } + + /** + * Adds a new key-value mapping into this map. + *

+ * This implementation calls {@code createEntry()}, {@code addEntry()} + * and {@code checkCapacity()}. + * It also handles changes to {@code modCount} and {@code size}. + * Subclasses could override to fully control adds to the map. + * + * @param hashIndex the index into the data array to store at + * @param hashCode the hash code of the key to add + * @param key the key to add + * @param value the value to add + */ + private void addMappingWithoutKeyConversion(final int hashIndex, final int hashCode, final Object key, final V value) { + modCount++; + final HashEntry entry = createEntryWithoutKeyConversion(data[hashIndex], hashCode, key, value); + addEntry(entry, hashIndex); + size++; + checkCapacity(); + } + + /** + * Creates an entry to store the key-value data. + *

+ * This implementation creates a new HashEntry instance. + * Subclasses can override this to return a different storage class, + * or implement caching. + * + * @param next the next entry in sequence + * @param hashCode the hash code to use + * @param key the key to store + * @param value the value to store + * @return the newly created entry + */ + private HashEntry createEntryWithoutKeyConversion(final HashEntry next, final int hashCode, final Object key, final V value) { + return new HashEntry<>(next, hashCode, key, value); + } + /** * Clones the map without cloning the keys or values. * From ce8d7ba0b4d9b5d28922e0a393727899255e6af1 Mon Sep 17 00:00:00 2001 From: Simulant Date: Thu, 3 Feb 2022 23:16:27 +0100 Subject: [PATCH 06/11] COLLECTIONS-803: add test verify convertKey is called only once --- .../collections4/map/CaseInsensitiveMapTest.java | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/src/test/java/org/apache/commons/collections4/map/CaseInsensitiveMapTest.java b/src/test/java/org/apache/commons/collections4/map/CaseInsensitiveMapTest.java index 13d503b3e2..3454dbe99e 100644 --- a/src/test/java/org/apache/commons/collections4/map/CaseInsensitiveMapTest.java +++ b/src/test/java/org/apache/commons/collections4/map/CaseInsensitiveMapTest.java @@ -21,6 +21,7 @@ import java.util.Map; import java.util.Set; +import org.easymock.EasyMock; import org.junit.jupiter.api.Test; /** @@ -147,4 +148,19 @@ public void testPutAll() { || !caseInsensitiveMap.containsValue("Three")); // ones collapsed assertEquals("Four", caseInsensitiveMap.get(null)); } + + // COLLECTIONS-803 + public void testPutConvertKeyOnlyOnce() { + CaseInsensitiveMap mock = EasyMock.partialMockBuilder(CaseInsensitiveMap.class) + .withConstructor(Map.class) + .withArgs(new HashMap<>()) + .addMockedMethod("convertKey") + .createMock(); + EasyMock.expect(mock.convertKey("Key")).andReturn("key").once(); + EasyMock.replay(mock); + + mock.put("Key", "value"); + + EasyMock.verify(mock); + } } From ae406907e4e5def495680966871a16bf47b24dc2 Mon Sep 17 00:00:00 2001 From: Simulant Date: Fri, 4 Feb 2022 11:20:32 +0100 Subject: [PATCH 07/11] COLLECTIONS-803: add unchecked suppression to new test case --- .../apache/commons/collections4/map/CaseInsensitiveMapTest.java | 1 + 1 file changed, 1 insertion(+) diff --git a/src/test/java/org/apache/commons/collections4/map/CaseInsensitiveMapTest.java b/src/test/java/org/apache/commons/collections4/map/CaseInsensitiveMapTest.java index 3454dbe99e..12e424dfd8 100644 --- a/src/test/java/org/apache/commons/collections4/map/CaseInsensitiveMapTest.java +++ b/src/test/java/org/apache/commons/collections4/map/CaseInsensitiveMapTest.java @@ -150,6 +150,7 @@ public void testPutAll() { } // COLLECTIONS-803 + @SuppressWarnings("unchecked") public void testPutConvertKeyOnlyOnce() { CaseInsensitiveMap mock = EasyMock.partialMockBuilder(CaseInsensitiveMap.class) .withConstructor(Map.class) From 651d7a3d0b021b60b53a8ea717ad177a2142fbd7 Mon Sep 17 00:00:00 2001 From: Simulant Date: Tue, 10 May 2022 22:19:07 +0200 Subject: [PATCH 08/11] [COLLECTIONS-803] add JUnit Test annotation to new test case --- .../apache/commons/collections4/map/CaseInsensitiveMapTest.java | 1 + 1 file changed, 1 insertion(+) diff --git a/src/test/java/org/apache/commons/collections4/map/CaseInsensitiveMapTest.java b/src/test/java/org/apache/commons/collections4/map/CaseInsensitiveMapTest.java index 12e424dfd8..b37c7c5814 100644 --- a/src/test/java/org/apache/commons/collections4/map/CaseInsensitiveMapTest.java +++ b/src/test/java/org/apache/commons/collections4/map/CaseInsensitiveMapTest.java @@ -150,6 +150,7 @@ public void testPutAll() { } // COLLECTIONS-803 + @Test @SuppressWarnings("unchecked") public void testPutConvertKeyOnlyOnce() { CaseInsensitiveMap mock = EasyMock.partialMockBuilder(CaseInsensitiveMap.class) From 7cf07ca361cdf620d54ed175ee9e26875ddf675c Mon Sep 17 00:00:00 2001 From: Simulant Date: Wed, 22 Mar 2023 21:34:51 +0100 Subject: [PATCH 09/11] [COLLECTIONS-803] init data on mock --- .../commons/collections4/map/CaseInsensitiveMapTest.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/test/java/org/apache/commons/collections4/map/CaseInsensitiveMapTest.java b/src/test/java/org/apache/commons/collections4/map/CaseInsensitiveMapTest.java index b37c7c5814..d5b7be237d 100644 --- a/src/test/java/org/apache/commons/collections4/map/CaseInsensitiveMapTest.java +++ b/src/test/java/org/apache/commons/collections4/map/CaseInsensitiveMapTest.java @@ -154,10 +154,10 @@ public void testPutAll() { @SuppressWarnings("unchecked") public void testPutConvertKeyOnlyOnce() { CaseInsensitiveMap mock = EasyMock.partialMockBuilder(CaseInsensitiveMap.class) - .withConstructor(Map.class) - .withArgs(new HashMap<>()) .addMockedMethod("convertKey") .createMock(); + mock.data = new AbstractHashedMap.HashEntry[16]; + EasyMock.expect(mock.convertKey("Key")).andReturn("key").once(); EasyMock.replay(mock); From 7f7b52f59634e36f8d0a8819157e579f85a8cccf Mon Sep 17 00:00:00 2001 From: Simulant87 Date: Thu, 23 Mar 2023 12:35:32 +0100 Subject: [PATCH 10/11] COLLECTIONS-803: parameterise generics in CaseInsensitiveMapTest --- .../apache/commons/collections4/map/CaseInsensitiveMapTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/java/org/apache/commons/collections4/map/CaseInsensitiveMapTest.java b/src/test/java/org/apache/commons/collections4/map/CaseInsensitiveMapTest.java index 6a1a5507a9..a635b00ba8 100644 --- a/src/test/java/org/apache/commons/collections4/map/CaseInsensitiveMapTest.java +++ b/src/test/java/org/apache/commons/collections4/map/CaseInsensitiveMapTest.java @@ -153,7 +153,7 @@ public void testPutAll() { @Test @SuppressWarnings("unchecked") public void testPutConvertKeyOnlyOnce() { - CaseInsensitiveMap mock = EasyMock.partialMockBuilder(CaseInsensitiveMap.class) + CaseInsensitiveMap mock = EasyMock.partialMockBuilder(CaseInsensitiveMap.class) .addMockedMethod("convertKey") .createMock(); mock.data = new AbstractHashedMap.HashEntry[16]; From 11349088b0fafae745140b73b429e9bcc914c816 Mon Sep 17 00:00:00 2001 From: Simulant Date: Thu, 23 Mar 2023 21:28:55 +0100 Subject: [PATCH 11/11] [COLLECTIONS-803] make private methods protected for CaseInsensitiveMap in order to allow overwrites by subclasses. Updated JavaDoc and parameter names to point out required key conversion. --- .../collections4/map/CaseInsensitiveMap.java | 21 +++++++------------ 1 file changed, 7 insertions(+), 14 deletions(-) diff --git a/src/main/java/org/apache/commons/collections4/map/CaseInsensitiveMap.java b/src/main/java/org/apache/commons/collections4/map/CaseInsensitiveMap.java index 9f14fb4a38..a6238be41d 100644 --- a/src/main/java/org/apache/commons/collections4/map/CaseInsensitiveMap.java +++ b/src/main/java/org/apache/commons/collections4/map/CaseInsensitiveMap.java @@ -141,13 +141,6 @@ protected Object convertKey(final Object key) { return AbstractHashedMap.NULL; } - /** - * Puts a key-value mapping into this map. - * - * @param key the key to add - * @param value the value to add - * @return the value previously mapped to this key, null if none - */ @Override public V put(final K key, final V value) { final Object convertedKey = convertKey(key); @@ -170,19 +163,19 @@ public V put(final K key, final V value) { /** * Adds a new key-value mapping into this map. *

- * This implementation calls {@code createEntry()}, {@code addEntry()} + * This implementation calls {@code createEntryWithoutKeyConversion()}, {@code addEntry()} * and {@code checkCapacity()}. * It also handles changes to {@code modCount} and {@code size}. * Subclasses could override to fully control adds to the map. * * @param hashIndex the index into the data array to store at * @param hashCode the hash code of the key to add - * @param key the key to add + * @param convertedKey the key to add, already converted to lower case by {@code convertKey()} * @param value the value to add */ - private void addMappingWithoutKeyConversion(final int hashIndex, final int hashCode, final Object key, final V value) { + protected void addMappingWithoutKeyConversion(final int hashIndex, final int hashCode, final Object convertedKey, final V value) { modCount++; - final HashEntry entry = createEntryWithoutKeyConversion(data[hashIndex], hashCode, key, value); + final HashEntry entry = createEntryWithoutKeyConversion(data[hashIndex], hashCode, convertedKey, value); addEntry(entry, hashIndex); size++; checkCapacity(); @@ -197,12 +190,12 @@ private void addMappingWithoutKeyConversion(final int hashIndex, final int hashC * * @param next the next entry in sequence * @param hashCode the hash code to use - * @param key the key to store + * @param convertedKey the key to add, already converted to lower case by {@code convertKey()} * @param value the value to store * @return the newly created entry */ - private HashEntry createEntryWithoutKeyConversion(final HashEntry next, final int hashCode, final Object key, final V value) { - return new HashEntry<>(next, hashCode, key, value); + protected HashEntry createEntryWithoutKeyConversion(final HashEntry next, final int hashCode, final Object convertedKey, final V value) { + return new HashEntry<>(next, hashCode, convertedKey, value); } /**