Make GeneratedPropertyItem addItem return null when appropriate (#18685)

Change-Id: I08f05bee9bf0c82f5767c63654046285c85f233a

2 files changed, 107 insertions, 0 deletions

diff --git a/server/src/com/vaadin/data/util/GeneratedPropertyContainer.java b/server/src/com/vaadin/data/util/GeneratedPropertyContainer.java
index cea1e27ee9..e0b2bb7c19 100644
--- a/server/src/com/vaadin/data/util/GeneratedPropertyContainer.java
+++ b/server/src/com/vaadin/data/util/GeneratedPropertyContainer.java
@@ -564,12 +564,18 @@ public class GeneratedPropertyContainer extends AbstractContainer implements
     public Item addItemAfter(Object previousItemId, Object newItemId)
             throws UnsupportedOperationException {
         Item item = wrappedContainer.addItemAfter(previousItemId, newItemId);
+        if (item == null) {
+            return null;
+        }
         return createGeneratedPropertyItem(newItemId, item);
     }
 
     @Override
     public Item addItem(Object itemId) throws UnsupportedOperationException {
         Item item = wrappedContainer.addItem(itemId);
+        if (item == null) {
+            return null;
+        }
         return createGeneratedPropertyItem(itemId, item);
     }
 
@@ -577,6 +583,9 @@ public class GeneratedPropertyContainer extends AbstractContainer implements
     public Item addItemAt(int index, Object newItemId)
             throws UnsupportedOperationException {
         Item item = wrappedContainer.addItemAt(index, newItemId);
+        if (item == null) {
+            return null;
+        }
         return createGeneratedPropertyItem(newItemId, item);
     }
 
diff --git a/server/tests/src/com/vaadin/data/util/AbstractContainerTestBase.java b/server/tests/src/com/vaadin/data/util/AbstractContainerTestBase.java
index 55dccc6455..52acc5ab76 100644
--- a/server/tests/src/com/vaadin/data/util/AbstractContainerTestBase.java
+++ b/server/tests/src/com/vaadin/data/util/AbstractContainerTestBase.java
@@ -1,6 +1,7 @@
 package com.vaadin.data.util;
 
 import java.util.ArrayList;
+import java.util.Iterator;
 import java.util.List;
 
 import junit.framework.TestCase;
@@ -164,6 +165,14 @@ public abstract class AbstractContainerTestBase extends TestCase {
                 sampleData.length);
 
         validateRemovingItems(container);
+        validateAddItem(container);
+        if (container instanceof Container.Indexed) {
+            validateAddItemAt((Container.Indexed) container);
+        }
+        if (container instanceof Container.Ordered) {
+            validateAddItemAfter((Container.Ordered) container);
+        }
+
     }
 
     protected void validateRemovingItems(Container container) {
@@ -186,6 +195,95 @@ public abstract class AbstractContainerTestBase extends TestCase {
         assertEquals(0, container.size());
     }
 
+    protected void validateAddItem(Container container) {
+        try {
+            container.removeAllItems();
+
+            Object id = container.addItem();
+            Assert.assertTrue(container.containsId(id));
+            Assert.assertNotNull(container.getItem(id));
+
+            Item item = container.addItem("foo");
+            Assert.assertNotNull(item);
+            Assert.assertTrue(container.containsId("foo"));
+            Assert.assertEquals(item, container.getItem("foo"));
+
+            // Add again
+            Item item2 = container.addItem("foo");
+            Assert.assertNull(item2);
+        } catch (UnsupportedOperationException e) {
+            // Ignore contains which do not support addItem*
+        }
+    }
+
+    protected void validateAddItemAt(Container.Indexed container) {
+        try {
+            container.removeAllItems();
+
+            Object id = container.addItemAt(0);
+            Assert.assertTrue(container.containsId(id));
+            Assert.assertEquals(id, container.getIdByIndex(0));
+            Assert.assertNotNull(container.getItem(id));
+
+            Item item = container.addItemAt(0, "foo");
+            Assert.assertNotNull(item);
+            Assert.assertTrue(container.containsId("foo"));
+            Assert.assertEquals(item, container.getItem("foo"));
+            Assert.assertEquals("foo", container.getIdByIndex(0));
+
+            Item itemAtEnd = container.addItemAt(2, "atend");
+            Assert.assertNotNull(itemAtEnd);
+            Assert.assertTrue(container.containsId("atend"));
+            Assert.assertEquals(itemAtEnd, container.getItem("atend"));
+            Assert.assertEquals("atend", container.getIdByIndex(2));
+
+            // Add again
+            Item item2 = container.addItemAt(0, "foo");
+            Assert.assertNull(item2);
+        } catch (UnsupportedOperationException e) {
+            // Ignore contains which do not support addItem*
+        }
+    }
+
+    protected void validateAddItemAfter(Container.Ordered container) {
+        if (container instanceof AbstractBeanContainer) {
+            // Doesn't work as bean container requires beans
+            return;
+        }
+        if (container instanceof ContainerOrderedWrapper) {
+            // Doesn't work because of #19427
+            return;
+        }
+
+        try {
+            container.removeAllItems();
+
+            Assert.assertNotNull(container.addItem(0));
+
+            Item item = container.addItemAfter(null, "foo");
+            Assert.assertNotNull(item);
+            Assert.assertTrue(container.containsId("foo"));
+            Assert.assertEquals(item, container.getItem("foo"));
+            Assert.assertEquals("foo", container.getItemIds().iterator().next());
+
+            Item itemAtEnd = container.addItemAfter(0, "atend");
+            Assert.assertNotNull(itemAtEnd);
+            Assert.assertTrue(container.containsId("atend"));
+            Assert.assertEquals(itemAtEnd, container.getItem("atend"));
+            Iterator<?> i = container.getItemIds().iterator();
+            i.next();
+            i.next();
+            Assert.assertEquals("atend", i.next());
+
+            // Add again
+            Assert.assertNull(container.addItemAfter(null, "foo"));
+            Assert.assertNull(container.addItemAfter("atend", "foo"));
+            Assert.assertNull(container.addItemAfter("nonexistant", "123123"));
+        } catch (UnsupportedOperationException e) {
+            // Ignore contains which do not support addItem*
+        }
+    }
+
     protected void testContainerOrdered(Container.Ordered container) {
         // addItem with empty container
         Object id = container.addItem();