Fix multiple book keeping problems in ContainerOrderedWrapper (#5934, #18422)

Change-Id: Ia749252ebf7034da5f3273ef117ab4ba35ad39b6

6 files changed, 291 insertions, 64 deletions

diff --git a/server/src/com/vaadin/data/util/ContainerOrderedWrapper.java b/server/src/com/vaadin/data/util/ContainerOrderedWrapper.java
index 4bb4e4c1b2..4329219e96 100644
--- a/server/src/com/vaadin/data/util/ContainerOrderedWrapper.java
+++ b/server/src/com/vaadin/data/util/ContainerOrderedWrapper.java
@@ -51,12 +51,14 @@ public class ContainerOrderedWrapper implements Container.Ordered,
     private final Container container;
 
     /**
-     * Ordering information, ie. the mapping from Item ID to the next item ID
+     * Ordering information, ie. the mapping from Item ID to the next item ID.
+     * The last item id should not be present
      */
     private Hashtable<Object, Object> next;
 
     /**
-     * Reverse ordering information for convenience and performance reasons.
+     * Reverse ordering information for convenience and performance reasons. The
+     * first item id should not be present
      */
     private Hashtable<Object, Object> prev;
 
@@ -124,13 +126,21 @@ public class ContainerOrderedWrapper implements Container.Ordered,
                 first = nid;
             }
             if (last.equals(id)) {
-                first = pid;
+                last = pid;
             }
             if (nid != null) {
-                prev.put(nid, pid);
+                if (pid == null) {
+                    prev.remove(nid);
+                } else {
+                    prev.put(nid, pid);
+                }
             }
             if (pid != null) {
-                next.put(pid, nid);
+                if (nid == null) {
+                    next.remove(pid);
+                } else {
+                    next.put(pid, nid);
+                }
             }
             next.remove(id);
             prev.remove(id);
@@ -200,7 +210,7 @@ public class ContainerOrderedWrapper implements Container.Ordered,
             final Collection<?> ids = container.getItemIds();
 
             // Recreates ordering if some parts of it are missing
-            if (next == null || first == null || last == null || prev != null) {
+            if (next == null || first == null || last == null || prev == null) {
                 first = null;
                 last = null;
                 next = new Hashtable<Object, Object>();
@@ -219,7 +229,7 @@ public class ContainerOrderedWrapper implements Container.Ordered,
             // Adds missing items
             for (final Iterator<?> i = ids.iterator(); i.hasNext();) {
                 final Object id = i.next();
-                if (!next.containsKey(id)) {
+                if (!next.containsKey(id) && last != id) {
                     addToOrderWrapper(id);
                 }
             }
diff --git a/server/tests/src/com/vaadin/data/util/AbstractContainerTestBase.java b/server/tests/src/com/vaadin/data/util/AbstractContainerTestBase.java
index 54cbc5305d..55dccc6455 100644
--- a/server/tests/src/com/vaadin/data/util/AbstractContainerTestBase.java
+++ b/server/tests/src/com/vaadin/data/util/AbstractContainerTestBase.java
@@ -11,6 +11,7 @@ import com.vaadin.data.Container;
 import com.vaadin.data.Container.Filterable;
 import com.vaadin.data.Container.ItemSetChangeEvent;
 import com.vaadin.data.Container.ItemSetChangeListener;
+import com.vaadin.data.Container.Ordered;
 import com.vaadin.data.Container.Sortable;
 import com.vaadin.data.Item;
 import com.vaadin.data.util.filter.SimpleStringFilter;
@@ -161,54 +162,54 @@ public abstract class AbstractContainerTestBase extends TestCase {
         validateContainer(container, sampleData[0],
                 sampleData[sampleData.length - 1], sampleData[10], "abc", true,
                 sampleData.length);
+
+        validateRemovingItems(container);
+    }
+
+    protected void validateRemovingItems(Container container) {
+        int sizeBeforeRemoving = container.size();
+
+        List<Object> itemIdList = new ArrayList<Object>(container.getItemIds());
+        // There should be at least four items in the list
+        Object first = itemIdList.get(0);
+        Object middle = itemIdList.get(2);
+        Object last = itemIdList.get(itemIdList.size() - 1);
+
+        container.removeItem(first);
+        container.removeItem(middle); // Middle now that first has been removed
+        container.removeItem(last);
+
+        assertEquals(sizeBeforeRemoving - 3, container.size());
+
+        container.removeAllItems();
+
+        assertEquals(0, container.size());
     }
 
     protected void testContainerOrdered(Container.Ordered container) {
+        // addItem with empty container
         Object id = container.addItem();
-        assertNotNull(id);
+        assertOrderedContents(container, id);
         Item item = container.getItem(id);
         assertNotNull(item);
 
-        assertEquals(id, container.firstItemId());
-        assertEquals(id, container.lastItemId());
-
-        // isFirstId
-        assertTrue(container.isFirstId(id));
-        assertTrue(container.isFirstId(container.firstItemId()));
-        // isLastId
-        assertTrue(container.isLastId(id));
-        assertTrue(container.isLastId(container.lastItemId()));
+        // addItemAfter with empty container
+        container.removeAllItems();
+        assertOrderedContents(container);
+        id = container.addItemAfter(null);
+        assertOrderedContents(container, id);
+        item = container.getItem(id);
+        assertNotNull(item);
 
         // Add a new item before the first
         // addItemAfter
         Object newFirstId = container.addItemAfter(null);
-        assertNotNull(newFirstId);
-        assertNotNull(container.getItem(newFirstId));
-
-        // isFirstId
-        assertTrue(container.isFirstId(newFirstId));
-        assertTrue(container.isFirstId(container.firstItemId()));
-        // isLastId
-        assertTrue(container.isLastId(id));
-        assertTrue(container.isLastId(container.lastItemId()));
-
-        // nextItemId
-        assertEquals(id, container.nextItemId(newFirstId));
-        assertNull(container.nextItemId(id));
-        assertNull(container.nextItemId("not-in-container"));
-
-        // prevItemId
-        assertEquals(newFirstId, container.prevItemId(id));
-        assertNull(container.prevItemId(newFirstId));
-        assertNull(container.prevItemId("not-in-container"));
+        assertOrderedContents(container, newFirstId, id);
 
         // addItemAfter(Object)
         Object newSecondItemId = container.addItemAfter(newFirstId);
         // order is now: newFirstId, newSecondItemId, id
-        assertNotNull(newSecondItemId);
-        assertNotNull(container.getItem(newSecondItemId));
-        assertEquals(id, container.nextItemId(newSecondItemId));
-        assertEquals(newFirstId, container.prevItemId(newSecondItemId));
+        assertOrderedContents(container, newFirstId, newSecondItemId, id);
 
         // addItemAfter(Object,Object)
         String fourthId = "id of the fourth item";
@@ -216,8 +217,8 @@ public abstract class AbstractContainerTestBase extends TestCase {
         // order is now: newFirstId, fourthId, newSecondItemId, id
         assertNotNull(fourth);
         assertEquals(fourth, container.getItem(fourthId));
-        assertEquals(newSecondItemId, container.nextItemId(fourthId));
-        assertEquals(newFirstId, container.prevItemId(fourthId));
+        assertOrderedContents(container, newFirstId, fourthId, newSecondItemId,
+                id);
 
         // addItemAfter(Object,Object)
         Object fifthId = new Object();
@@ -225,8 +226,86 @@ public abstract class AbstractContainerTestBase extends TestCase {
         // order is now: fifthId, newFirstId, fourthId, newSecondItemId, id
         assertNotNull(fifth);
         assertEquals(fifth, container.getItem(fifthId));
-        assertEquals(newFirstId, container.nextItemId(fifthId));
-        assertNull(container.prevItemId(fifthId));
+        assertOrderedContents(container, fifthId, newFirstId, fourthId,
+                newSecondItemId, id);
+
+        // addItemAfter(Object,Object)
+        Object sixthId = new Object();
+        Item sixth = container.addItemAfter(id, sixthId);
+        // order is now: fifthId, newFirstId, fourthId, newSecondItemId, id,
+        // sixthId
+        assertNotNull(sixth);
+        assertEquals(sixth, container.getItem(sixthId));
+        assertOrderedContents(container, fifthId, newFirstId, fourthId,
+                newSecondItemId, id, sixthId);
+
+        // Test order after removing first item 'fifthId'
+        container.removeItem(fifthId);
+        // order is now: newFirstId, fourthId, newSecondItemId, id, sixthId
+        assertOrderedContents(container, newFirstId, fourthId, newSecondItemId,
+                id, sixthId);
+
+        // Test order after removing last item 'sixthId'
+        container.removeItem(sixthId);
+        // order is now: newFirstId, fourthId, newSecondItemId, id
+        assertOrderedContents(container, newFirstId, fourthId, newSecondItemId,
+                id);
+
+        // Test order after removing item from the middle 'fourthId'
+        container.removeItem(fourthId);
+        // order is now: newFirstId, newSecondItemId, id
+        assertOrderedContents(container, newFirstId, newSecondItemId, id);
+
+        // Delete remaining items
+        container.removeItem(newFirstId);
+        container.removeItem(newSecondItemId);
+        container.removeItem(id);
+        assertOrderedContents(container);
+
+        Object finalItem = container.addItem();
+        assertOrderedContents(container, finalItem);
+    }
+
+    private void assertOrderedContents(Ordered container, Object... ids) {
+        assertEquals(ids.length, container.size());
+        for (int i = 0; i < ids.length - 1; i++) {
+            assertNotNull("The item id should not be null", ids[i]);
+        }
+        if (ids.length == 0) {
+            assertNull("The first id is wrong", container.firstItemId());
+            assertNull("The last id is wrong", container.lastItemId());
+            return;
+        }
+
+        assertEquals("The first id is wrong", ids[0], container.firstItemId());
+        assertEquals("The last id is wrong", ids[ids.length - 1],
+                container.lastItemId());
+
+        // isFirstId & isLastId
+        assertTrue(container.isFirstId(container.firstItemId()));
+        assertTrue(container.isLastId(container.lastItemId()));
+
+        // nextId
+        Object ref = container.firstItemId();
+        for (int i = 1; i < ids.length; i++) {
+            Object next = container.nextItemId(ref);
+            assertEquals("The id after " + ref + " is wrong", ids[i], next);
+            ref = next;
+        }
+        assertNull("The last id should not have a next id",
+                container.nextItemId(ids[ids.length - 1]));
+        assertNull(container.nextItemId("not-in-container"));
+
+        // prevId
+        ref = container.lastItemId();
+        for (int i = ids.length - 2; i >= 0; i--) {
+            Object prev = container.prevItemId(ref);
+            assertEquals("The id before " + ref + " is wrong", ids[i], prev);
+            ref = prev;
+        }
+        assertNull("The first id should not have a prev id",
+                container.prevItemId(ids[0]));
+        assertNull(container.prevItemId("not-in-container"));
 
     }
 
diff --git a/server/tests/src/com/vaadin/data/util/AbstractHierarchicalContainerTestBase.java b/server/tests/src/com/vaadin/data/util/AbstractHierarchicalContainerTestBase.java
index 3bd00cce3c..9cede77162 100644
--- a/server/tests/src/com/vaadin/data/util/AbstractHierarchicalContainerTestBase.java
+++ b/server/tests/src/com/vaadin/data/util/AbstractHierarchicalContainerTestBase.java
@@ -253,4 +253,33 @@ public abstract class AbstractHierarchicalContainerTestBase extends
         }
     }
 
+    protected void testRemoveHierarchicalWrapperSubtree(
+            Container.Hierarchical container) {
+        initializeContainer(container);
+
+        // remove root item
+        removeItemRecursively(container, "org");
+
+        int packages = 21 + 3 - 3;
+        int expectedSize = sampleData.length + packages - 1;
+
+        validateContainer(container, "com", "com.vaadin.util.SerializerHelper",
+                "com.vaadin.server.ApplicationResource", "blah", true,
+                expectedSize);
+
+        // rootItemIds
+        Collection<?> rootIds = container.rootItemIds();
+        assertEquals(1, rootIds.size());
+    }
+
+    private void removeItemRecursively(Container.Hierarchical container,
+            Object itemId) {
+        if (container instanceof ContainerHierarchicalWrapper) {
+            ((ContainerHierarchicalWrapper) container)
+                    .removeItemRecursively("org");
+        } else {
+            HierarchicalContainer.removeItemRecursively(container, itemId);
+        }
+    }
+
 }
diff --git a/server/tests/src/com/vaadin/data/util/ContainerHierarchicalWrapperTest.java b/server/tests/src/com/vaadin/data/util/ContainerHierarchicalWrapperTest.java
index 2fd21ef118..8e554d5030 100644
--- a/server/tests/src/com/vaadin/data/util/ContainerHierarchicalWrapperTest.java
+++ b/server/tests/src/com/vaadin/data/util/ContainerHierarchicalWrapperTest.java
@@ -1,6 +1,5 @@
 package com.vaadin.data.util;
 
-import java.util.Collection;
 
 public class ContainerHierarchicalWrapperTest extends
         AbstractHierarchicalContainerTestBase {
@@ -20,23 +19,4 @@ public class ContainerHierarchicalWrapperTest extends
                 new IndexedContainer()));
     }
 
-    protected void testRemoveHierarchicalWrapperSubtree(
-            ContainerHierarchicalWrapper container) {
-        initializeContainer(container);
-
-        // remove root item
-        container.removeItemRecursively("org");
-
-        int packages = 21 + 3 - 3;
-        int expectedSize = sampleData.length + packages - 1;
-
-        validateContainer(container, "com", "com.vaadin.util.SerializerHelper",
-                "com.vaadin.server.ApplicationResource", "blah", true,
-                expectedSize);
-
-        // rootItemIds
-        Collection<?> rootIds = container.rootItemIds();
-        assertEquals(1, rootIds.size());
-    }
-
 }
diff --git a/server/tests/src/com/vaadin/data/util/ContainerOrderedWrapperTest.java b/server/tests/src/com/vaadin/data/util/ContainerOrderedWrapperTest.java
new file mode 100644
index 0000000000..422e9756f8
--- /dev/null
+++ b/server/tests/src/com/vaadin/data/util/ContainerOrderedWrapperTest.java
@@ -0,0 +1,102 @@
+package com.vaadin.data.util;
+
+import java.util.Collection;
+
+import com.vaadin.data.Container;
+import com.vaadin.data.Item;
+import com.vaadin.data.Property;
+
+public class ContainerOrderedWrapperTest extends AbstractContainerTestBase {
+
+    // This class is needed to get an implementation of container
+    // which is not an implementation of Ordered interface.
+    private class NotOrderedContainer implements Container {
+
+        private IndexedContainer container;
+
+        public NotOrderedContainer() {
+            container = new IndexedContainer();
+        }
+
+        @Override
+        public Item getItem(Object itemId) {
+            return container.getItem(itemId);
+        }
+
+        @Override
+        public Collection<?> getContainerPropertyIds() {
+            return container.getContainerPropertyIds();
+        }
+
+        @Override
+        public Collection<?> getItemIds() {
+            return container.getItemIds();
+        }
+
+        @Override
+        public Property getContainerProperty(Object itemId, Object propertyId) {
+            return container.getContainerProperty(itemId, propertyId);
+        }
+
+        @Override
+        public Class<?> getType(Object propertyId) {
+            return container.getType(propertyId);
+        }
+
+        @Override
+        public int size() {
+            return container.size();
+        }
+
+        @Override
+        public boolean containsId(Object itemId) {
+            return container.containsId(itemId);
+        }
+
+        @Override
+        public Item addItem(Object itemId) throws UnsupportedOperationException {
+            return container.addItem(itemId);
+        }
+
+        @Override
+        public Object addItem() throws UnsupportedOperationException {
+            return container.addItem();
+        }
+
+        @Override
+        public boolean removeItem(Object itemId)
+                throws UnsupportedOperationException {
+            return container.removeItem(itemId);
+        }
+
+        @Override
+        public boolean addContainerProperty(Object propertyId, Class<?> type,
+                Object defaultValue) throws UnsupportedOperationException {
+            return container.addContainerProperty(propertyId, type,
+                    defaultValue);
+        }
+
+        @Override
+        public boolean removeContainerProperty(Object propertyId)
+                throws UnsupportedOperationException {
+            return container.removeContainerProperty(propertyId);
+        }
+
+        @Override
+        public boolean removeAllItems() throws UnsupportedOperationException {
+            return container.removeAllItems();
+        }
+
+    }
+
+    public void testBasicOperations() {
+        testBasicContainerOperations(new ContainerOrderedWrapper(
+                new NotOrderedContainer()));
+    }
+
+    public void testOrdered() {
+        testContainerOrdered(new ContainerOrderedWrapper(
+                new NotOrderedContainer()));
+    }
+
+}
diff --git a/server/tests/src/com/vaadin/data/util/HierarchicalContainerOrderedWrapperTest.java b/server/tests/src/com/vaadin/data/util/HierarchicalContainerOrderedWrapperTest.java
new file mode 100644
index 0000000000..7ecf59c309
--- /dev/null
+++ b/server/tests/src/com/vaadin/data/util/HierarchicalContainerOrderedWrapperTest.java
@@ -0,0 +1,27 @@
+package com.vaadin.data.util;
+
+public class HierarchicalContainerOrderedWrapperTest extends
+        AbstractHierarchicalContainerTestBase {
+
+    private HierarchicalContainerOrderedWrapper createContainer() {
+        return new HierarchicalContainerOrderedWrapper(
+                new ContainerHierarchicalWrapper(new IndexedContainer()));
+    }
+
+    public void testBasicOperations() {
+        testBasicContainerOperations(createContainer());
+    }
+
+    public void testHierarchicalContainer() {
+        testHierarchicalContainer(createContainer());
+    }
+
+    public void testContainerOrdered() {
+        testContainerOrdered(createContainer());
+    }
+
+    public void testRemoveSubtree() {
+        testRemoveHierarchicalWrapperSubtree(createContainer());
+    }
+
+}