Grid.removeColumn() not to fail silently (#10215)8.1.6

Fixes #8056

2 files changed, 107 insertions, 38 deletions

diff --git a/server/src/main/java/com/vaadin/ui/Grid.java b/server/src/main/java/com/vaadin/ui/Grid.java
index d5d5e666bb..68c605703e 100644
--- a/server/src/main/java/com/vaadin/ui/Grid.java
+++ b/server/src/main/java/com/vaadin/ui/Grid.java
@@ -977,10 +977,9 @@ public class Grid<T> extends AbstractListing<T> implements HasComponents,
         private static int compareMaybeComparables(Object a, Object b) {
             if (hasCommonComparableBaseType(a, b)) {
                 return compareComparables(a, b);
-            } else {
-                return compareComparables(Objects.toString(a, ""),
-                        Objects.toString(b, ""));
             }
+            return compareComparables(Objects.toString(a, ""),
+                    Objects.toString(b, ""));
         }
 
         private static boolean hasCommonComparableBaseType(Object a, Object b) {
@@ -1006,7 +1005,7 @@ public class Grid<T> extends AbstractListing<T> implements HasComponents,
             return false;
         }
 
-        @SuppressWarnings("unchecked")
+        @SuppressWarnings({ "unchecked", "rawtypes" })
         private static int compareComparables(Object a, Object b) {
             return ((Comparator) Comparator
                     .nullsLast(Comparator.naturalOrder())).compare(a, b);
@@ -1020,20 +1019,19 @@ public class Grid<T> extends AbstractListing<T> implements HasComponents,
             if (valueA instanceof Comparable
                     && valueA.getClass().isInstance(valueB)) {
                 return ((Comparable<Number>) valueA).compareTo(valueB);
-            } else if (valueA.equals(valueB)) {
+            }
+            if (valueA.equals(valueB)) {
                 return 0;
-            } else {
-                // Fall back to comparing based on potentially truncated values
-                int compare = Long.compare(valueA.longValue(),
-                        valueB.longValue());
-                if (compare == 0) {
-                    // This might still produce 0 even though the values are not
-                    // equals, but there's nothing more we can do about that
-                    compare = Double.compare(valueA.doubleValue(),
-                            valueB.doubleValue());
-                }
-                return compare;
             }
+            // Fall back to comparing based on potentially truncated values
+            int compare = Long.compare(valueA.longValue(), valueB.longValue());
+            if (compare == 0) {
+                // This might still produce 0 even though the values are not
+                // equals, but there's nothing more we can do about that
+                compare = Double.compare(valueA.doubleValue(),
+                        valueB.doubleValue());
+            }
+            return compare;
         }
 
         @SuppressWarnings("unchecked")
@@ -2721,6 +2719,9 @@ public class Grid<T> extends AbstractListing<T> implements HasComponents,
      *
      * @param column
      *            the column to remove
+     *
+     * @throws IllegalArgumentException
+     *             if the column is not a valid one
      */
     public void removeColumn(Column<T, ?> column) {
         if (columnSet.remove(column)) {
@@ -2738,6 +2739,9 @@ public class Grid<T> extends AbstractListing<T> implements HasComponents,
             if (displayIndex < getFrozenColumnCount()) {
                 setFrozenColumnCount(getFrozenColumnCount() - 1);
             }
+        } else {
+            throw new IllegalArgumentException("Column with id "
+                    + column.getId() + " cannot be removed from the grid");
         }
     }
 
@@ -2935,10 +2939,12 @@ public class Grid<T> extends AbstractListing<T> implements HasComponents,
         if (rows <= 0.0d) {
             throw new IllegalArgumentException(
                     "More than zero rows must be shown.");
-        } else if (Double.isInfinite(rows)) {
+        }
+        if (Double.isInfinite(rows)) {
             throw new IllegalArgumentException(
                     "Grid doesn't support infinite heights");
-        } else if (Double.isNaN(rows)) {
+        }
+        if (Double.isNaN(rows)) {
             throw new IllegalArgumentException("NaN is not a valid row count");
         }
         getState().heightMode = HeightMode.ROW;
@@ -3684,7 +3690,6 @@ public class Grid<T> extends AbstractListing<T> implements HasComponents,
         } else {
             addExtension(selectionModel);
         }
-
     }
 
     /**
@@ -4190,7 +4195,8 @@ public class Grid<T> extends AbstractListing<T> implements HasComponents,
         SelectionMode selectionMode = getSelectionMode();
         if (SelectionMode.SINGLE.equals(selectionMode)) {
             return asSingleSelect().isReadOnly();
-        } else if (SelectionMode.MULTI.equals(selectionMode)) {
+        }
+        if (SelectionMode.MULTI.equals(selectionMode)) {
             return asMultiSelect().isReadOnly();
         }
         return false;
diff --git a/server/src/test/java/com/vaadin/tests/server/component/grid/GridTest.java b/server/src/test/java/com/vaadin/tests/server/component/grid/GridTest.java
index 5f8327bb8d..03993db260 100644
--- a/server/src/test/java/com/vaadin/tests/server/component/grid/GridTest.java
+++ b/server/src/test/java/com/vaadin/tests/server/component/grid/GridTest.java
@@ -23,15 +23,17 @@ import java.util.concurrent.atomic.AtomicReference;
 import java.util.stream.Collectors;
 import java.util.stream.Stream;
 
-import com.vaadin.data.provider.DataCommunicator;
 import org.easymock.Capture;
 import org.junit.Assert;
 import org.junit.Before;
+import org.junit.Rule;
 import org.junit.Test;
+import org.junit.rules.ExpectedException;
 
 import com.vaadin.data.Binder.Binding;
 import com.vaadin.data.ValidationException;
 import com.vaadin.data.ValueProvider;
+import com.vaadin.data.provider.DataCommunicator;
 import com.vaadin.data.provider.DataGenerator;
 import com.vaadin.data.provider.GridSortOrder;
 import com.vaadin.data.provider.QuerySortOrder;
@@ -59,6 +61,9 @@ public class GridTest {
     private Column<String, Object> objectColumn;
     private Column<String, String> randomColumn;
 
+    @Rule
+    public ExpectedException thrown = ExpectedException.none();
+
     @Before
     public void setUp() {
         grid = new Grid<>();
@@ -73,9 +78,10 @@ public class GridTest {
 
     @Test
     public void testCreateGridWithDataCommunicator() {
-        DataCommunicator specificDataCommunicator = new DataCommunicator<>();
+        DataCommunicator<String> specificDataCommunicator = new DataCommunicator<>();
 
-        TestGrid<String> grid = new TestGrid(String.class, specificDataCommunicator);
+        TestGrid<String> grid = new TestGrid<>(String.class,
+                specificDataCommunicator);
 
         assertEquals(specificDataCommunicator, grid.getDataCommunicator());
     }
@@ -92,17 +98,25 @@ public class GridTest {
                 HeightMode.CSS, grid.getHeightMode());
     }
 
-    @Test(expected = IllegalArgumentException.class)
+    @Test
     public void testFrozenColumnCountTooBig() {
+        thrown.expect(IllegalArgumentException.class);
+        thrown.expectMessage(
+                "count must be between -1 and the current number of columns (4): 5");
+
         grid.setFrozenColumnCount(5);
     }
 
-    @Test(expected = IllegalArgumentException.class)
+    @Test
     public void testFrozenColumnCountTooSmall() {
+        thrown.expect(IllegalArgumentException.class);
+        thrown.expectMessage(
+                "count must be between -1 and the current number of columns (4): -2");
+
         grid.setFrozenColumnCount(-2);
     }
 
-    @Test()
+    @Test
     public void testSetFrozenColumnCount() {
         for (int i = -1; i < 2; ++i) {
             grid.setFrozenColumnCount(i);
@@ -118,8 +132,11 @@ public class GridTest {
                 grid.getHeaderRow(0).getCell("foo").getText());
     }
 
-    @Test(expected = IllegalArgumentException.class)
+    @Test
     public void testGridMultipleColumnsWithSameIdentifier() {
+        thrown.expect(IllegalArgumentException.class);
+        thrown.expectMessage("Duplicate ID for columns");
+
         grid.addColumn(t -> t).setId("foo");
     }
 
@@ -207,8 +224,12 @@ public class GridTest {
         assertEquals(0, event.getAllSelectedItems().size());
     }
 
-    @Test(expected = UnsupportedOperationException.class)
+    @Test
     public void testAddSelectionListener_noSelectionMode() {
+        thrown.expect(UnsupportedOperationException.class);
+        thrown.expectMessage(
+                "This selection model doesn't allow selection, cannot add selection listeners to it");
+
         grid.setSelectionMode(SelectionMode.NONE);
 
         grid.addSelectionListener(
@@ -354,8 +375,13 @@ public class GridTest {
         Assert.assertEquals("Ipsum", person.getName());
     }
 
-    @Test(expected = IllegalStateException.class)
+    @Test
     public void oneArgSetEditor_nonBeanGrid() {
+        thrown.expect(IllegalStateException.class);
+        thrown.expectMessage(
+                "A Grid created without a bean type class literal or a custom property set"
+                        + " doesn't support finding properties by name.");
+
         Grid<Person> grid = new Grid<>();
         Column<Person, String> nameCol = grid.addColumn(Person::getName)
                 .setId("name");
@@ -363,8 +389,11 @@ public class GridTest {
         nameCol.setEditorComponent(new TextField());
     }
 
-    @Test(expected = IllegalStateException.class)
+    @Test
     public void addExistingColumnById_throws() {
+        thrown.expect(IllegalStateException.class);
+        thrown.expectMessage("There is already a column for name");
+
         Grid<Person> grid = new Grid<>(Person.class);
         grid.addColumn("name");
     }
@@ -393,8 +422,11 @@ public class GridTest {
 
     @Test
     public void removeColumnByColumn_alreadyRemoved() {
+        thrown.expect(IllegalArgumentException.class);
+        thrown.expectMessage(
+                "Column with id foo cannot be removed from the grid");
+
         grid.removeColumn(fooColumn);
-        // Questionable that this doesn't throw, but that's a separate ticket...
         grid.removeColumn(fooColumn);
 
         Assert.assertEquals(
@@ -402,8 +434,11 @@ public class GridTest {
                 grid.getColumns());
     }
 
-    @Test(expected = IllegalStateException.class)
+    @Test
     public void removeColumnById_alreadyRemoved() {
+        thrown.expect(IllegalStateException.class);
+        thrown.expectMessage("There is no column with the id foo");
+
         grid.removeColumn("foo");
         grid.removeColumn("foo");
     }
@@ -479,8 +514,13 @@ public class GridTest {
         Assert.assertEquals("foo", columns.get(1).getId());
     }
 
-    @Test(expected = IllegalStateException.class)
+    @Test
     public void setColumns_addColumn_notBeangrid() {
+        thrown.expect(IllegalStateException.class);
+        thrown.expectMessage(
+                "A Grid created without a bean type class literal or a custom property set"
+                        + " doesn't support finding properties by name.");
+
         // Not possible to add a column in a grid that cannot add columns based
         // on a string
         grid.setColumns("notHere");
@@ -509,8 +549,12 @@ public class GridTest {
                 objectColumn), grid.getColumns());
     }
 
-    @Test(expected = IllegalStateException.class)
+    @Test
     public void setColumnOrder_byColumn_removedColumn() {
+        thrown.expect(IllegalStateException.class);
+        thrown.expectMessage("setColumnOrder should not be called "
+                + "with columns that are not in the grid.");
+
         grid.removeColumn(randomColumn);
         grid.setColumnOrder(randomColumn, lengthColumn);
     }
@@ -523,8 +567,11 @@ public class GridTest {
                 objectColumn), grid.getColumns());
     }
 
-    @Test(expected = IllegalStateException.class)
+    @Test
     public void setColumnOrder_byString_removedColumn() {
+        thrown.expect(IllegalStateException.class);
+        thrown.expectMessage("There is no column with the id randomColumnId");
+
         grid.removeColumn("randomColumnId");
         grid.setColumnOrder("randomColumnId", "length");
     }
@@ -590,8 +637,13 @@ public class GridTest {
         Assert.assertEquals(formattedValue, "2,017");
     }
 
-    @Test(expected = IllegalArgumentException.class)
+    @Test
     public void addBeanColumn_invalidRenderer() {
+        thrown.expect(IllegalArgumentException.class);
+        thrown.expectMessage("NumberRenderer");
+        thrown.expectMessage(
+                " cannot be used with a property of type java.lang.String");
+
         Grid<Person> grid = new Grid<>(Person.class);
 
         grid.removeColumn("name");
@@ -659,9 +711,8 @@ public class GridTest {
     }
 
     private static Method findDataGeneratorGetterMethod() {
-        Method getter;
         try {
-            getter = Column.class.getDeclaredMethod("getDataGenerator",
+            Method getter = Column.class.getDeclaredMethod("getDataGenerator",
                     new Class<?>[] {});
             getter.setAccessible(true);
             return getter;
@@ -670,4 +721,16 @@ public class GridTest {
                     "Cannot get DataGenerator from Column");
         }
     }
+
+    @Test
+    public void removeColumnToThrowForInvalidColumn() {
+        thrown.expect(IllegalArgumentException.class);
+        thrown.expectMessage(
+                "Column with id null cannot be removed from the grid");
+
+        Grid<Person> grid1 = new Grid<>();
+        Grid<Person> grid2 = new Grid<>();
+        Column<Person, ?> column1 = grid1.addColumn(ValueProvider.identity());
+        grid2.removeColumn(column1);
+    }
 }