MINOR: Fix bugs in handling zero-length ImplicitLinkedHashCollections (…

…apache#7163) Reviewers: Colin P. McCabe <cmccabe@apache.org>
linkedin · Aug 15, 2019 · 9cb27f5 · 9cb27f5
1 parent 4d0cc43
commit 9cb27f5
Show file tree

Hide file tree

Showing 4 changed files with 24 additions and 4 deletions.
diff --git a/clients/src/main/java/org/apache/kafka/common/utils/ImplicitLinkedHashCollection.java b/clients/src/main/java/org/apache/kafka/common/utils/ImplicitLinkedHashCollection.java
@@ -304,7 +304,7 @@ final int slot(Element[] curElements, Object e) {
      * @return                  The match index, or INVALID_INDEX if no match was found.
      */
     final private int findIndexOfEqualElement(Object key) {
-        if (key == null) {
+        if (key == null || size == 0) {
             return INVALID_INDEX;
         }
         int slot = slot(elements, key);

diff --git a/clients/src/main/java/org/apache/kafka/common/utils/ImplicitLinkedHashMultiCollection.java b/clients/src/main/java/org/apache/kafka/common/utils/ImplicitLinkedHashMultiCollection.java
@@ -91,7 +91,7 @@ int addInternal(Element newElement, Element[] addElements) {
      */
     @Override
     int findElementToRemove(Object key) {
-        if (key == null) {
+        if (key == null || size() == 0) {
             return INVALID_INDEX;
         }
         int slot = slot(elements, key);
@@ -120,7 +120,7 @@ int findElementToRemove(Object key) {
      * @return          All of the matching elements.
      */
     final public List<E> findAll(E key) {
-        if (key == null) {
+        if (key == null || size() == 0) {
             return Collections.<E>emptyList();
         }
         ArrayList<E> results = new ArrayList<>();

diff --git a/clients/src/test/java/org/apache/kafka/common/utils/ImplicitLinkedHashCollectionTest.java b/clients/src/test/java/org/apache/kafka/common/utils/ImplicitLinkedHashCollectionTest.java
@@ -31,6 +31,7 @@
 import java.util.Set;
 
 import static org.junit.Assert.assertNotEquals;
+import static org.junit.Assert.assertNull;
 import static org.junit.Assert.assertTrue;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertEquals;
@@ -271,7 +272,7 @@ public void testListViewModification() {
     @Test
     public void testEmptyListIterator() {
         ImplicitLinkedHashCollection<TestElement> coll = new ImplicitLinkedHashCollection<>();
-        ListIterator iter = coll.valuesList().listIterator();
+        ListIterator<TestElement> iter = coll.valuesList().listIterator();
         assertFalse(iter.hasNext());
         assertFalse(iter.hasPrevious());
         assertEquals(0, iter.nextIndex());
@@ -513,6 +514,14 @@ public void testEquals() {
         assertNotEquals(coll2, coll3);
     }
 
+    @Test
+    public void testFindContainsRemoveOnEmptyCollection() {
+        ImplicitLinkedHashCollection<TestElement> coll = new ImplicitLinkedHashCollection<>();
+        assertNull(coll.find(new TestElement(2)));
+        assertFalse(coll.contains(new TestElement(2)));
+        assertFalse(coll.remove(new TestElement(2)));
+    }
+
     private void addRandomElement(Random random, LinkedHashSet<Integer> existing,
                                   ImplicitLinkedHashCollection<TestElement> set) {
         int next;
@@ -523,6 +532,7 @@ private void addRandomElement(Random random, LinkedHashSet<Integer> existing,
         set.add(new TestElement(next));
     }
 
+    @SuppressWarnings("unlikely-arg-type")
     private void removeRandomElement(Random random, Collection<Integer> existing,
                                      ImplicitLinkedHashCollection<TestElement> coll) {
         int removeIdx = random.nextInt(existing.size());

diff --git a/...ts/src/test/java/org/apache/kafka/common/utils/ImplicitLinkedHashMultiCollectionTest.java b/...ts/src/test/java/org/apache/kafka/common/utils/ImplicitLinkedHashMultiCollectionTest.java
@@ -28,6 +28,7 @@
 
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertNull;
 import static org.junit.Assert.assertTrue;
 import static org.junit.Assert.fail;
 
@@ -44,6 +45,15 @@ public void testNullForbidden() {
         assertFalse(multiSet.add(null));
     }
 
+    @Test
+    public void testFindFindAllContainsRemoveOnEmptyCollection() {
+        ImplicitLinkedHashMultiCollection<TestElement> coll = new ImplicitLinkedHashMultiCollection<>();
+        assertNull(coll.find(new TestElement(2)));
+        assertFalse(coll.contains(new TestElement(2)));
+        assertFalse(coll.remove(new TestElement(2)));
+        assertTrue(coll.findAll(new TestElement(2)).isEmpty());
+    }
+
     @Test
     public void testInsertDelete() {
         ImplicitLinkedHashMultiCollection<TestElement> multiSet = new ImplicitLinkedHashMultiCollection<>(100);