Correctly handle data providers with overriden getId (#10728)

Fixes #10647
Fixes #10498

2 files changed, 130 insertions, 4 deletions

diff --git a/server/src/main/java/com/vaadin/ui/AbstractMultiSelect.java b/server/src/main/java/com/vaadin/ui/AbstractMultiSelect.java
index fdc4f5d4a1..558545d653 100644
--- a/server/src/main/java/com/vaadin/ui/AbstractMultiSelect.java
+++ b/server/src/main/java/com/vaadin/ui/AbstractMultiSelect.java
@@ -323,16 +323,25 @@ public abstract class AbstractMultiSelect<T> extends AbstractListing<T>
 
         // if there are duplicates, some item is both added & removed, just
         // discard that and leave things as was before
-        addedItems.removeIf(item -> removedItems.remove(item));
+        DataProvider<T, ?> dataProvider = internalGetDataProvider();
+        addedItems.removeIf(item -> {
+            Object addedId = dataProvider.getId(item);
+            return removedItems.stream().map(dataProvider::getId)
+                    .anyMatch(addedId::equals)? removedItems.remove(item):false;
+        });
 
-        if (selection.containsAll(addedItems)
-                && Collections.disjoint(selection, removedItems)) {
+        if (isAllSelected(addedItems) && isNoneSelected(removedItems)) {
             return;
         }
 
         updateSelection(set -> {
             // order of add / remove does not matter since no duplicates
-            set.removeAll(removedItems);
+            set.removeIf(item -> {
+                Object itemId = dataProvider.getId(item);
+
+                return removedItems.stream().map(dataProvider::getId)
+                        .anyMatch(itemId::equals);
+            });
             set.addAll(addedItems);
         }, userOriginated);
     }
@@ -359,6 +368,26 @@ public abstract class AbstractMultiSelect<T> extends AbstractListing<T>
 
     }
 
+    private boolean isAllSelected(Collection<T> items) {
+        for (T item : items) {
+            if (!isSelected(item)) {
+                return false;
+            }
+        }
+
+        return true;
+    }
+
+    private boolean isNoneSelected(Collection<T> items) {
+        for (T item : items) {
+            if (isSelected(item)) {
+                return false;
+            }
+        }
+
+        return true;
+    }
+
     /**
      * Deselects the given item. If the item is not currently selected, does
      * nothing.
diff --git a/server/src/test/java/com/vaadin/ui/AbstractMultiSelectUsingIdTest.java b/server/src/test/java/com/vaadin/ui/AbstractMultiSelectUsingIdTest.java
new file mode 100644
index 0000000000..4f131ba23d
--- /dev/null
+++ b/server/src/test/java/com/vaadin/ui/AbstractMultiSelectUsingIdTest.java
@@ -0,0 +1,97 @@
+/*
+ * Copyright 2000-2016 Vaadin Ltd.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License"); you may not
+ * use this file except in compliance with the License. You may obtain a copy of
+ * the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations under
+ * the License.
+ */
+package com.vaadin.ui;
+
+import static org.junit.Assert.assertEquals;
+
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.List;
+import java.util.Set;
+import java.util.stream.Collectors;
+
+import org.junit.Before;
+import org.junit.Test;
+
+import com.vaadin.data.provider.ListDataProvider;
+
+public class AbstractMultiSelectUsingIdTest {
+
+    public TwinColSelect<ItemWithId> selectToTest;
+
+    public static class ItemWithId {
+        private int id;
+
+        public ItemWithId() {
+        }
+
+        public ItemWithId(int id) {
+            this.id = id;
+        }
+
+        public int getId() {
+            return id;
+        }
+
+        public void setId(int id) {
+            this.id = id;
+        }
+    }
+
+    @Before
+    public void setUp() {
+        selectToTest = new TwinColSelect<>();
+        List<ItemWithId> items = new ArrayList<>();
+        items.add(new ItemWithId(3));
+        items.add(new ItemWithId(2));
+        items.add(new ItemWithId(1));
+        items.add(new ItemWithId(8));
+        items.add(new ItemWithId(7));
+        items.add(new ItemWithId(4));
+        items.add(new ItemWithId(6));
+        ListDataProvider<ItemWithId> dataProvider = new ListDataProvider<ItemWithId>(
+                items) {
+            @Override
+            public Object getId(ItemWithId item) {
+                return item.getId();
+            }
+        };
+        selectToTest.setDataProvider(dataProvider);
+
+    }
+
+    @Test
+    public void selectTwiceSelectsOnce() {
+        selectToTest.select(new ItemWithId(1));
+        assertSelectionOrder(1);
+        selectToTest.select(new ItemWithId(1));
+        assertSelectionOrder(1);
+    }
+
+    @Test
+    public void deselectWorks() {
+        selectToTest.select(new ItemWithId(1));
+        selectToTest.deselect(new ItemWithId(1));
+        assertSelectionOrder();
+    }
+
+    private void assertSelectionOrder(Integer... selectionOrder) {
+        List<Integer> asList = Arrays.asList(selectionOrder);
+        assertEquals(asList, selectToTest.getSelectedItems().stream()
+                .map(ItemWithId::getId).collect(Collectors.toList()));
+    }
+
+}