From 27f574154b5e27407370b6e072e5fa13d1d97797 Mon Sep 17 00:00:00 2001
From: Artur Signell <artur@vaadin.com>
Date: Tue, 2 Jun 2015 21:21:44 +0300
Subject: [PATCH] Better error messages for addColumn/setColumns (#18019,
 #17890)

Change-Id: Iadf455ae6cbc60e0ce0b88fe7c12df946ed08cf0
---
 server/src/com/vaadin/ui/Grid.java            | 12 ++++-
 .../component/grid/GridContainerTest.java     | 54 +++++++++++++++++++
 2 files changed, 65 insertions(+), 1 deletion(-)

diff --git a/server/src/com/vaadin/ui/Grid.java b/server/src/com/vaadin/ui/Grid.java
index 96884d7fd5..2c442d6e43 100644
--- a/server/src/com/vaadin/ui/Grid.java
+++ b/server/src/com/vaadin/ui/Grid.java
@@ -4139,8 +4139,18 @@ public class Grid extends AbstractComponent implements SelectionNotifier,
         if (datasource.getContainerPropertyIds().contains(propertyId)
                 && !columns.containsKey(propertyId)) {
             appendColumn(propertyId);
-        } else {
+        } else if (defaultContainer) {
             addColumnProperty(propertyId, String.class, "");
+        } else {
+            if (columns.containsKey(propertyId)) {
+                throw new IllegalStateException("A column for property id '"
+                        + propertyId.toString()
+                        + "' already exists in this grid");
+            } else {
+                throw new IllegalStateException("Property id '"
+                        + propertyId.toString()
+                        + "' does not exist in the container");
+            }
         }
 
         // Inform the data provider of this new column.
diff --git a/server/tests/src/com/vaadin/tests/server/component/grid/GridContainerTest.java b/server/tests/src/com/vaadin/tests/server/component/grid/GridContainerTest.java
index cbd448618b..527568eac8 100644
--- a/server/tests/src/com/vaadin/tests/server/component/grid/GridContainerTest.java
+++ b/server/tests/src/com/vaadin/tests/server/component/grid/GridContainerTest.java
@@ -15,9 +15,11 @@
  */
 package com.vaadin.tests.server.component.grid;
 
+import org.junit.Assert;
 import org.junit.Test;
 
 import com.vaadin.data.util.IndexedContainer;
+import com.vaadin.ui.Grid;
 
 public class GridContainerTest {
 
@@ -43,4 +45,56 @@ public class GridContainerTest {
         container.addItem(0).getItemProperty("x").setValue("y");
         return container;
     }
+
+    @Test
+    public void setColumnsOrder() {
+        Grid grid = new Grid();
+        IndexedContainer ic = new IndexedContainer();
+        ic.addContainerProperty("foo", String.class, "");
+        ic.addContainerProperty("baz", String.class, "");
+        ic.addContainerProperty("bar", String.class, "");
+        grid.setContainerDataSource(ic);
+        grid.setColumns("foo", "baz", "bar");
+
+        Assert.assertEquals("foo", grid.getColumns().get(0).getPropertyId());
+        Assert.assertEquals("baz", grid.getColumns().get(1).getPropertyId());
+        Assert.assertEquals("bar", grid.getColumns().get(2).getPropertyId());
+    }
+
+    @Test
+    public void addColumnNotInContainer() {
+        Grid grid = new Grid();
+        grid.setContainerDataSource(new IndexedContainer());
+        try {
+            grid.addColumn("notInContainer");
+            Assert.fail("Adding a property id not in the container should throw an exception");
+        } catch (IllegalStateException e) {
+            Assert.assertTrue(e.getMessage().contains("notInContainer"));
+            Assert.assertTrue(e.getMessage().contains(
+                    "does not exist in the container"));
+        }
+    }
+
+    @Test
+    public void setColumnsForPropertyIdNotInContainer() {
+        Grid grid = new Grid();
+        grid.setContainerDataSource(new IndexedContainer());
+        try {
+            grid.setColumns("notInContainer", "notThereEither");
+            Assert.fail("Setting columns for property ids not in the container should throw an exception");
+        } catch (IllegalStateException e) {
+            // addColumn is run in random order..
+            Assert.assertTrue(e.getMessage().contains("notInContainer")
+                    || e.getMessage().contains("notThereEither"));
+            Assert.assertTrue(e.getMessage().contains(
+                    "does not exist in the container"));
+        }
+    }
+
+    @Test(expected = IllegalStateException.class)
+    public void multipleAddColumnsForDefaultContainer() {
+        Grid grid = new Grid();
+        grid.addColumn("foo");
+        grid.addColumn("foo");
+    }
 }
-- 
2.39.5