Correctly handle data providers with overriden getId (#10728)

Fixes #10647
Fixes #10498

6 files changed, 365 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()));
+    }
+
+}
diff --git a/uitest/src/main/java/com/vaadin/tests/components/checkboxgroup/CheckCheckBoxGroupWithId.java b/uitest/src/main/java/com/vaadin/tests/components/checkboxgroup/CheckCheckBoxGroupWithId.java
new file mode 100644
index 0000000000..408e786a91
--- /dev/null
+++ b/uitest/src/main/java/com/vaadin/tests/components/checkboxgroup/CheckCheckBoxGroupWithId.java
@@ -0,0 +1,71 @@
+package com.vaadin.tests.components.checkboxgroup;
+
+import com.vaadin.data.provider.DataProvider;
+import com.vaadin.data.provider.ListDataProvider;
+import com.vaadin.server.SerializablePredicate;
+import com.vaadin.server.VaadinRequest;
+import com.vaadin.tests.components.AbstractTestUIWithLog;
+import com.vaadin.ui.CheckBoxGroup;
+
+import java.util.Arrays;
+import java.util.HashSet;
+import java.util.Objects;
+
+public class CheckCheckBoxGroupWithId extends AbstractTestUIWithLog {
+
+    @Override
+    protected void setup(VaadinRequest request) {
+        DataProvider<MyObject, SerializablePredicate<MyObject>> dataProvider = new ListDataProvider<MyObject>(
+                Arrays.asList(new MyObject("Yellow", "real"),
+                        new MyObject("Red", "real"))) {
+            @Override
+            public Object getId(MyObject item) {
+                return item.getName();
+            }
+        };
+
+        CheckBoxGroup<MyObject> checkBoxGroup = new CheckBoxGroup<>();
+        checkBoxGroup.setItemCaptionGenerator(MyObject::getName);
+        checkBoxGroup.setDataProvider(dataProvider);
+        checkBoxGroup.setValue(
+                new HashSet<>(Arrays.asList(new MyObject("Yellow", null))));
+
+        addComponent(checkBoxGroup);
+        addButton("Deselect",
+                event -> checkBoxGroup.deselect(new MyObject("Yellow", "XX")));
+    }
+
+    public static class MyObject {
+        private final String name;
+        private final String other;
+
+        public MyObject(String name, String other) {
+            this.name = name;
+            this.other = other;
+        }
+
+        public String getName() {
+            return name;
+        }
+
+        public String getOther() {
+            return other;
+        }
+
+        @Override
+        public boolean equals(Object o) {
+            if (this == o)
+                return true;
+            if (o == null || getClass() != o.getClass())
+                return false;
+            MyObject myObject = (MyObject) o;
+            return Objects.equals(name, myObject.name)
+                    && Objects.equals(other, myObject.other);
+        }
+
+        @Override
+        public int hashCode() {
+            return Objects.hash(name, other);
+        }
+    }
+}
diff --git a/uitest/src/main/java/com/vaadin/tests/components/twincolselect/TwinColSelectAndIdBasedDataProvider.java b/uitest/src/main/java/com/vaadin/tests/components/twincolselect/TwinColSelectAndIdBasedDataProvider.java
new file mode 100644
index 0000000000..e6dce9a443
--- /dev/null
+++ b/uitest/src/main/java/com/vaadin/tests/components/twincolselect/TwinColSelectAndIdBasedDataProvider.java
@@ -0,0 +1,110 @@
+/*
+ * 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.tests.components.twincolselect;
+
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.Objects;
+
+import com.vaadin.annotations.Widgetset;
+import com.vaadin.data.provider.ListDataProvider;
+import com.vaadin.server.VaadinRequest;
+import com.vaadin.tests.components.AbstractTestUIWithLog;
+import com.vaadin.ui.Button;
+import com.vaadin.ui.TwinColSelect;
+
+@Widgetset("com.vaadin.DefaultWidgetSet")
+public class TwinColSelectAndIdBasedDataProvider extends AbstractTestUIWithLog {
+
+    public static class MyObject {
+        private int id;
+        private String name;
+
+        public MyObject() {
+
+        }
+
+        public MyObject(int id, String name) {
+            this.id = id;
+            this.name = name;
+        }
+
+        public int getId() {
+            return id;
+        }
+
+        public void setId(int id) {
+            this.id = id;
+        }
+
+        public String getName() {
+            return name;
+        }
+
+        public void setName(String name) {
+            this.name = name;
+        }
+
+        @Override
+        public String toString() {
+            return "MyObject{" + "id=" + id + ", name='" + name + '\'' + '}';
+        }
+
+        @Override
+        public boolean equals(Object o) {
+            if (this == o) {
+                return true;
+            }
+            if (o == null || getClass() != o.getClass()) {
+                return false;
+            }
+            MyObject myObject = (MyObject) o;
+            return id == myObject.id && Objects.equals(name, myObject.name);
+        }
+
+        @Override
+        public int hashCode() {
+            return Objects.hash(id, name);
+        }
+    }
+
+    @Override
+    protected void setup(VaadinRequest request) {
+        ListDataProvider<MyObject> dataProvider = new ListDataProvider<MyObject>(
+                Arrays.asList(new MyObject(0, "Zero"),
+                        new MyObject(1, "One"))) {
+
+            @Override
+            public Object getId(MyObject item) {
+                return item.getId();
+            }
+        };
+
+        TwinColSelect<MyObject> twinColSelect = new TwinColSelect<>();
+        twinColSelect.setDataProvider(dataProvider);
+
+        twinColSelect.setValue(Collections.singleton(new MyObject(1, null)));
+
+        twinColSelect.addValueChangeListener(
+                event1 -> log.log("value: " + event1.getValue()));
+
+        addComponent(twinColSelect);
+        addComponent(new Button("Deselect id=1", e -> {
+            twinColSelect.deselect(new MyObject(1, "foo"));
+        }));
+    }
+
+}
diff --git a/uitest/src/test/java/com/vaadin/tests/components/checkboxgroup/CheckCheckBoxGroupWithIdTest.java b/uitest/src/test/java/com/vaadin/tests/components/checkboxgroup/CheckCheckBoxGroupWithIdTest.java
new file mode 100644
index 0000000000..afcaae52f3
--- /dev/null
+++ b/uitest/src/test/java/com/vaadin/tests/components/checkboxgroup/CheckCheckBoxGroupWithIdTest.java
@@ -0,0 +1,26 @@
+package com.vaadin.tests.components.checkboxgroup;
+
+import com.vaadin.testbench.elements.ButtonElement;
+import com.vaadin.testbench.elements.CheckBoxGroupElement;
+import com.vaadin.tests.tb3.MultiBrowserTest;
+import org.junit.Before;
+import org.junit.Test;
+
+import static org.junit.Assert.assertEquals;
+
+public class CheckCheckBoxGroupWithIdTest extends MultiBrowserTest {
+    private CheckBoxGroupElement checkBoxGroup;
+
+    @Before
+    public void setUp() {
+        openTestURL();
+        checkBoxGroup = $(CheckBoxGroupElement.class).first();
+    }
+
+    @Test
+    public void TestSelection() {
+        assertEquals(checkBoxGroup.getValue().size(), 1);
+        $(ButtonElement.class).first().click();
+        assertEquals(checkBoxGroup.getValue().size(), 0);
+    }
+}
diff --git a/uitest/src/test/java/com/vaadin/tests/components/twincolselect/TwinColSelectAndIdBasedDataProviderTest.java b/uitest/src/test/java/com/vaadin/tests/components/twincolselect/TwinColSelectAndIdBasedDataProviderTest.java
new file mode 100644
index 0000000000..6475f43718
--- /dev/null
+++ b/uitest/src/test/java/com/vaadin/tests/components/twincolselect/TwinColSelectAndIdBasedDataProviderTest.java
@@ -0,0 +1,28 @@
+package com.vaadin.tests.components.twincolselect;
+
+import com.vaadin.testbench.elements.ButtonElement;
+import com.vaadin.testbench.elements.TwinColSelectElement;
+import com.vaadin.tests.tb3.MultiBrowserTest;
+import org.junit.Before;
+import org.junit.Test;
+
+import static org.junit.Assert.assertEquals;
+
+public class TwinColSelectAndIdBasedDataProviderTest extends MultiBrowserTest {
+
+    @Before
+    public void setUp() {
+        openTestURL();
+    }
+
+    @Test
+    public void TestSelection() {
+        assertEquals(getTwinColElement().getValues().size(), 1);
+        $(ButtonElement.class).first().click();
+        assertEquals(getTwinColElement().getValues().size(), 0);
+    }
+
+    private TwinColSelectElement getTwinColElement() {
+        return $(TwinColSelectElement.class).first();
+    }
+}