Check for duplicate property ids when setting Grid columns or column order (#20386)

Change-Id: I76be83642f0e56e55b0c0e502ac6769de1ee8af0

4 files changed, 102 insertions, 0 deletions

diff --git a/server/src/main/java/com/vaadin/ui/Grid.java b/server/src/main/java/com/vaadin/ui/Grid.java
index 8ca1a3de11..e7eed7725f 100644
--- a/server/src/main/java/com/vaadin/ui/Grid.java
+++ b/server/src/main/java/com/vaadin/ui/Grid.java
@@ -104,6 +104,7 @@ import com.vaadin.shared.ui.grid.selection.MultiSelectionModelState;
 import com.vaadin.shared.ui.grid.selection.SingleSelectionModelServerRpc;
 import com.vaadin.shared.ui.grid.selection.SingleSelectionModelState;
 import com.vaadin.shared.util.SharedUtil;
+import com.vaadin.ui.Grid.SelectionModel;
 import com.vaadin.ui.declarative.DesignAttributeHandler;
 import com.vaadin.ui.declarative.DesignContext;
 import com.vaadin.ui.declarative.DesignException;
@@ -5305,6 +5306,11 @@ public class Grid extends AbstractFocusable implements SelectionNotifier,
      *            properties in the desired column order
      */
     public void setColumns(Object... propertyIds) {
+        if (SharedUtil.containsDuplicates(propertyIds)) {
+            throw new IllegalArgumentException(
+                    "The propertyIds array contains duplicates: "
+                            + SharedUtil.getDuplicates(propertyIds));
+        }
         Set<?> removePids = new HashSet<Object>(columns.keySet());
         removePids.removeAll(Arrays.asList(propertyIds));
         for (Object removePid : removePids) {
@@ -5327,6 +5333,11 @@ public class Grid extends AbstractFocusable implements SelectionNotifier,
      *            properties in the order columns should be
      */
     public void setColumnOrder(Object... propertyIds) {
+        if (SharedUtil.containsDuplicates(propertyIds)) {
+            throw new IllegalArgumentException(
+                    "The propertyIds array contains duplicates: "
+                            + SharedUtil.getDuplicates(propertyIds));
+        }
         List<String> columnOrder = new ArrayList<String>();
         for (Object propertyId : propertyIds) {
             if (columns.containsKey(propertyId)) {
diff --git a/server/src/test/java/com/vaadin/tests/server/component/grid/GridColumnsTest.java b/server/src/test/java/com/vaadin/tests/server/component/grid/GridColumnsTest.java
index bf321f7ee9..24bd8b7f2a 100644
--- a/server/src/test/java/com/vaadin/tests/server/component/grid/GridColumnsTest.java
+++ b/server/src/test/java/com/vaadin/tests/server/component/grid/GridColumnsTest.java
@@ -410,4 +410,22 @@ public class GridColumnsTest {
 
         assertThat(firstColumn.getHeaderCaption(), is("Column0"));
     }
+
+    @Test(expected = IllegalStateException.class)
+    public void addColumnManyTimes() {
+        grid.removeAllColumns();
+        grid.addColumn("column0");
+        grid.addColumn("column0");
+    }
+
+    @Test(expected = IllegalArgumentException.class)
+    public void setColumnDuplicates() {
+        grid.removeAllColumns();
+        grid.setColumns("column0", "column0");
+    }
+
+    @Test(expected = IllegalArgumentException.class)
+    public void setColumnOrderDuplicates() {
+        grid.setColumnOrder("column0", "column0");
+    }
 }
diff --git a/shared/src/main/java/com/vaadin/shared/util/SharedUtil.java b/shared/src/main/java/com/vaadin/shared/util/SharedUtil.java
index 271e8d0e5e..141da10c35 100644
--- a/shared/src/main/java/com/vaadin/shared/util/SharedUtil.java
+++ b/shared/src/main/java/com/vaadin/shared/util/SharedUtil.java
@@ -16,6 +16,9 @@
 package com.vaadin.shared.util;
 
 import java.io.Serializable;
+import java.util.Arrays;
+import java.util.HashSet;
+import java.util.LinkedHashSet;
 import java.util.Locale;
 
 /**
@@ -145,6 +148,9 @@ public class SharedUtil implements Serializable {
      * @return The constructed string of words and separators
      */
     public static String join(String[] parts, String separator) {
+        if (parts.length == 0) {
+            return "";
+        }
         StringBuilder sb = new StringBuilder();
         for (int i = 0; i < parts.length; i++) {
             sb.append(parts[i]);
@@ -266,4 +272,39 @@ public class SharedUtil implements Serializable {
         return join(parts, "");
     }
 
+    /**
+     * Checks if the given array contains duplicates (according to
+     * {@link Object#equals(Object)}.
+     * 
+     * @param values
+     *            the array to check for duplicates
+     * @return <code>true</code> if the array contains duplicates,
+     *         <code>false</code> otherwise
+     */
+    public static boolean containsDuplicates(Object[] values) {
+        int uniqueCount = new HashSet<Object>(Arrays.asList(values)).size();
+        return uniqueCount != values.length;
+    }
+
+    /**
+     * Return duplicate values in the given array in the format
+     * "duplicateValue1, duplicateValue2".
+     * 
+     * @param values
+     *            the values to check for duplicates
+     * @return a comma separated string of duplicates or an empty string if no
+     *         duplicates were found
+     */
+    public static String getDuplicates(Object[] values) {
+        HashSet<Object> set = new HashSet<Object>();
+        LinkedHashSet<String> duplicates = new LinkedHashSet<String>();
+        for (Object o : values) {
+            if (!set.add(o)) {
+                duplicates.add(String.valueOf(o));
+            }
+
+        }
+        return join(duplicates.toArray(new String[duplicates.size()]), ", ");
+    }
+
 }
diff --git a/shared/src/test/java/com/vaadin/shared/util/SharedUtilTest.java b/shared/src/test/java/com/vaadin/shared/util/SharedUtilTest.java
index 081f594a97..6d8cab4fea 100644
--- a/shared/src/test/java/com/vaadin/shared/util/SharedUtilTest.java
+++ b/shared/src/test/java/com/vaadin/shared/util/SharedUtilTest.java
@@ -118,4 +118,36 @@ public class SharedUtilTest {
         }
     }
 
+    @Test
+    public void duplicatesInArray() {
+        Assert.assertTrue(
+                SharedUtil.containsDuplicates(new Object[] { "a", "a" }));
+        Assert.assertTrue(
+                SharedUtil.containsDuplicates(new Object[] { "a", "b", "a" }));
+        Assert.assertTrue(SharedUtil
+                .containsDuplicates(new Object[] { "a", "b", "a", "b" }));
+        Assert.assertTrue(
+                SharedUtil.containsDuplicates(new Object[] { 1, "b", 1 }));
+
+        Assert.assertFalse(SharedUtil.containsDuplicates(new Object[] {}));
+        Assert.assertFalse(SharedUtil.containsDuplicates(new Object[] { "a" }));
+        Assert.assertFalse(
+                SharedUtil.containsDuplicates(new Object[] { "a", "b" }));
+        Assert.assertFalse(
+                SharedUtil.containsDuplicates(new Object[] { "1", 1 }));
+    }
+
+    @Test
+    public void getDuplicates() {
+        Assert.assertEquals("", SharedUtil.getDuplicates(new Object[] { "a" }));
+        Assert.assertEquals("a",
+                SharedUtil.getDuplicates(new Object[] { "a", "a" }));
+        Assert.assertEquals("a, b",
+                SharedUtil.getDuplicates(new Object[] { "a", "b", "a", "b" }));
+        Assert.assertEquals("a, b, c", SharedUtil
+                .getDuplicates(new Object[] { "c", "a", "b", "a", "b", "c" }));
+        Assert.assertEquals("1.2",
+                SharedUtil.getDuplicates(new Object[] { 1.2, "a", 1.2 }));
+    }
+
 }