Correctly reset DataCommunicator when its DataProvider is changed (#8138)

* Correctly reset DataCommunicator when its DataProvider is changed

* Improve ReplaceDataProviderTest

* Remove return type from AbstractListing.readItems

server/src/main/java/com/vaadin/data/provider/DataCommunicator.java

@@ -178,6 +178,12 @@ public class DataCommunicator<T, F> extends AbstractExtension {
             // Drop the registered key
             getKeyMapper().remove(data);
         }
+
+        @Override
+        public void destroyAllData() {
+            activeData.clear();
+            getKeyMapper().removeAll();
+        }
     }
 
     private final Collection<DataGenerator<T>> generators = new LinkedHashSet<>();
@@ -354,6 +360,13 @@ public class DataCommunicator<T, F> extends AbstractExtension {
         }
     }
 
+    private void dropAllData() {
+        for (DataGenerator<T> g : generators) {
+            g.destroyAllData();
+        }
+        handler.destroyAllData();
+    }
+
     /**
      * Informs the DataProvider that the collection has changed.
      */
@@ -460,6 +473,7 @@ public class DataCommunicator<T, F> extends AbstractExtension {
         Objects.requireNonNull(dataProvider, "data provider cannot be null");
         this.dataProvider = dataProvider;
         detachDataProviderListener();
+        dropAllData();
         /*
          * This introduces behavior which influence on the client-server
          * communication: now the very first response to the client will always

server/src/main/java/com/vaadin/data/provider/DataGenerator.java

@@ -55,4 +55,11 @@ public interface DataGenerator<T> extends Serializable {
      */
     public default void destroyData(T item) {
     }
+
+    /**
+     * Informs the {@code DataGenerator} that all data has been dropped. This
+     * method should clean up any unneeded information stored for items.
+     */
+    public default void destroyAllData() {
+    }
 }

server/src/main/java/com/vaadin/ui/AbstractListing.java

@@ -15,14 +15,12 @@
  */
 package com.vaadin.ui;
 
-import java.util.List;
 import java.util.Objects;
 
 import org.jsoup.nodes.Attributes;
 import org.jsoup.nodes.Element;
 
 import com.vaadin.data.Listing;
-import com.vaadin.data.SelectionModel;
 import com.vaadin.data.provider.DataCommunicator;
 import com.vaadin.data.provider.DataGenerator;
 import com.vaadin.data.provider.DataProvider;
@@ -374,10 +372,7 @@ public abstract class AbstractListing<T> extends AbstractComponent
         setItemIconGenerator(
                 new DeclarativeIconGenerator<>(getItemIconGenerator()));
 
-        List<T> readItems = readItems(design, context);
-        if (!readItems.isEmpty() && this instanceof Listing) {
-            ((Listing<T, ?>) this).setItems(readItems);
-        }
+        readItems(design, context);
     }
 
     /**
@@ -387,10 +382,8 @@ public abstract class AbstractListing<T> extends AbstractComponent
      *            The element to obtain the state from
      * @param context
      *            The DesignContext instance used for parsing the design
-     *
-     * @return the items read from the design
      */
-    protected abstract List<T> readItems(Element design, DesignContext context);
+    protected abstract void readItems(Element design, DesignContext context);
 
     /**
      * Reads an Item from a design and inserts it into the data source.

server/src/main/java/com/vaadin/ui/AbstractMultiSelect.java

@@ -25,10 +25,10 @@ import java.util.Optional;
 import java.util.Set;
 import java.util.stream.Collectors;
 
-import com.vaadin.server.SerializableConsumer;
 import org.jsoup.nodes.Element;
 
 import com.vaadin.data.HasValue;
+import com.vaadin.data.Listing;
 import com.vaadin.data.SelectionModel;
 import com.vaadin.data.SelectionModel.Multi;
 import com.vaadin.data.provider.DataGenerator;
@@ -36,6 +36,7 @@ import com.vaadin.event.selection.MultiSelectionEvent;
 import com.vaadin.event.selection.MultiSelectionListener;
 import com.vaadin.server.Resource;
 import com.vaadin.server.ResourceReference;
+import com.vaadin.server.SerializableConsumer;
 import com.vaadin.server.SerializablePredicate;
 import com.vaadin.shared.Registration;
 import com.vaadin.shared.data.selection.MultiSelectServerRpc;
@@ -117,6 +118,11 @@ public abstract class AbstractMultiSelect<T> extends AbstractListing<T>
         @Override
         public void destroyData(T data) {
         }
+
+        @Override
+        public void destroyAllData() {
+            AbstractMultiSelect.this.deselectAll();
+        }
     }
 
     /**
@@ -422,14 +428,16 @@ public abstract class AbstractMultiSelect<T> extends AbstractListing<T>
     }
 
     @Override
-    protected List<T> readItems(Element design, DesignContext context) {
+    protected void readItems(Element design, DesignContext context) {
         Set<T> selected = new HashSet<>();
         List<T> items = design.children().stream()
                 .map(child -> readItem(child, selected, context))
                 .collect(Collectors.toList());
         deselectAll();
+        if (!items.isEmpty() && this instanceof Listing) {
+            ((Listing<T, ?>) this).setItems(items);
+        }
         selected.forEach(this::select);
-        return items;
     }
 
     /**

server/src/main/java/com/vaadin/ui/AbstractSingleSelect.java

@@ -26,7 +26,7 @@ import java.util.stream.Collectors;
 import org.jsoup.nodes.Element;
 
 import com.vaadin.data.HasValue;
-import com.vaadin.data.SelectionModel;
+import com.vaadin.data.Listing;
 import com.vaadin.data.SelectionModel.Single;
 import com.vaadin.data.provider.DataCommunicator;
 import com.vaadin.event.selection.SingleSelectionEvent;
@@ -321,13 +321,15 @@ public abstract class AbstractSingleSelect<T> extends AbstractListing<T>
     }
 
     @Override
-    protected List<T> readItems(Element design, DesignContext context) {
+    protected void readItems(Element design, DesignContext context) {
         Set<T> selected = new HashSet<>();
         List<T> items = design.children().stream()
                 .map(child -> readItem(child, selected, context))
                 .collect(Collectors.toList());
+        if (!items.isEmpty() && this instanceof Listing) {
+            ((Listing<T, ?>) this).setItems(items);
+        }
         selected.forEach(this::setValue);
-        return items;
     }
 
     /**

server/src/main/java/com/vaadin/ui/CheckBoxGroup.java

@@ -17,7 +17,6 @@
 package com.vaadin.ui;
 
 import java.util.Collection;
-import java.util.List;
 import java.util.Set;
 
 import org.jsoup.nodes.Element;
@@ -167,9 +166,9 @@ public class CheckBoxGroup<T> extends AbstractMultiSelect<T>
     }
 
     @Override
-    protected List<T> readItems(Element design, DesignContext context) {
+    protected void readItems(Element design, DesignContext context) {
         setItemEnabledProvider(new DeclarativeItemEnabledProvider<>());
-        return super.readItems(design, context);
+        super.readItems(design, context);
     }
 
     @SuppressWarnings({ "unchecked", "rawtypes" })

server/src/main/java/com/vaadin/ui/ComboBox.java

@@ -18,7 +18,6 @@ package com.vaadin.ui;
 
 import java.util.Collection;
 import java.util.HashMap;
-import java.util.List;
 import java.util.Map;
 import java.util.Objects;
 import java.util.Set;
@@ -610,9 +609,9 @@ public class ComboBox<T> extends AbstractSingleSelect<T>
     }
 
     @Override
-    protected List<T> readItems(Element design, DesignContext context) {
+    protected void readItems(Element design, DesignContext context) {
         setStyleGenerator(new DeclarativeStyleGenerator<>(getStyleGenerator()));
-        return super.readItems(design, context);
+        super.readItems(design, context);
     }
 
     @SuppressWarnings({ "unchecked", "rawtypes" })

server/src/main/java/com/vaadin/ui/Grid.java

@@ -2895,8 +2895,8 @@ public class Grid<T> extends AbstractListing<T> implements HasComponents,
     }
 
     @Override
-    protected List<T> readItems(Element design, DesignContext context) {
-        return Collections.emptyList();
+    protected void readItems(Element design, DesignContext context) {
+        // Grid handles reading of items in Grid#readData
     }
 
     @Override
@@ -3043,11 +3043,12 @@ public class Grid<T> extends AbstractListing<T> implements HasComponents,
             List<DeclarativeValueProvider<T>> providers) {
         getSelectionModel().deselectAll();
         List<T> items = new ArrayList<>();
+        List<T> selectedItems = new ArrayList<>();
         for (Element row : body.children()) {
             T item = deserializeDeclarativeRepresentation(row.attr("item"));
             items.add(item);
             if (row.hasAttr("selected")) {
-                getSelectionModel().select(item);
+                selectedItems.add(item);
             }
             Elements cells = row.children();
             int i = 0;
@@ -3058,6 +3059,7 @@ public class Grid<T> extends AbstractListing<T> implements HasComponents,
         }
 
         setItems(items);
+        selectedItems.forEach(getSelectionModel()::select);
     }
 
     private void writeStructure(Element design, DesignContext designContext) {

server/src/main/java/com/vaadin/ui/RadioButtonGroup.java

@@ -17,7 +17,6 @@
 package com.vaadin.ui;
 
 import java.util.Collection;
-import java.util.List;
 import java.util.Objects;
 import java.util.Set;
 
@@ -236,9 +235,9 @@ public class RadioButtonGroup<T> extends AbstractSingleSelect<T>
     }
 
     @Override
-    protected List<T> readItems(Element design, DesignContext context) {
+    protected void readItems(Element design, DesignContext context) {
         setItemEnabledProvider(new DeclarativeItemEnabledProvider<>());
-        return super.readItems(design, context);
+        super.readItems(design, context);
     }
 
     @SuppressWarnings({ "unchecked", "rawtypes" })

server/src/main/java/com/vaadin/ui/components/grid/AbstractSelectionModel.java

@@ -43,6 +43,11 @@ public abstract class AbstractSelectionModel<T> extends AbstractGridExtension<T>
         }
     }
 
+    @Override
+    public void destroyAllData() {
+        deselectAll();
+    }
+
     @Override
     protected AbstractSelectionModelState getState() {
         return (AbstractSelectionModelState) super.getState();

server/src/test/java/com/vaadin/data/provider/ReplaceDataProviderTest.java

@@ -0,0 +1,110 @@
+package com.vaadin.data.provider;
+
+import static org.junit.Assert.assertNotEquals;
+import static org.junit.Assert.assertNull;
+import static org.junit.Assert.assertSame;
+
+import java.util.List;
+import java.util.function.IntFunction;
+import java.util.stream.Collectors;
+import java.util.stream.IntStream;
+
+import org.junit.Test;
+
+import com.vaadin.server.SerializablePredicate;
+
+public class ReplaceDataProviderTest {
+
+    public static class BeanWithEquals extends Bean {
+
+        BeanWithEquals(int id) {
+            super(id);
+        }
+
+        @Override
+        public boolean equals(Object o) {
+            if (this == o) {
+                return true;
+            }
+            if (o == null || getClass() != o.getClass()) {
+                return false;
+            }
+
+            BeanWithEquals that = (BeanWithEquals) o;
+
+            return id == that.id;
+        }
+
+        @Override
+        public int hashCode() {
+            return id;
+        }
+    }
+
+    public static class Bean {
+        protected final int id;
+        private final String fluff;
+
+        Bean(int id) {
+            this.id = id;
+            this.fluff = "Fluff #" + id;
+        }
+
+        public int getId() {
+            return id;
+        }
+
+        @SuppressWarnings("unused")
+        public String getFluff() {
+            return fluff;
+        }
+
+    }
+
+    @Test
+    public void testBeanEquals() {
+        doTest(BeanWithEquals::new);
+    }
+
+    @Test
+    public void testBeanSame() {
+        doTest(Bean::new);
+    }
+
+    private <SOME_BEAN> void doTest(IntFunction<SOME_BEAN> beanConstructor) {
+
+        DataCommunicator<SOME_BEAN, SerializablePredicate<SOME_BEAN>> dataCommunicator = new DataCommunicator<>();
+
+        List<SOME_BEAN> beans1 = createCollection(beanConstructor);
+
+        ListDataProvider<SOME_BEAN> dataProvider = new ListDataProvider<>(
+                beans1);
+
+        dataCommunicator.setDataProvider(dataProvider);
+        dataCommunicator.pushData(1, beans1.stream());
+
+        SOME_BEAN bean1_17 = beans1.get(17);
+        String key1_17 = dataCommunicator.getKeyMapper().key(bean1_17);
+
+        assertSame(bean1_17, dataCommunicator.getKeyMapper().get(key1_17));
+
+        List<SOME_BEAN> beans2 = createCollection(beanConstructor);
+
+        dataProvider = new ListDataProvider<>(beans2);
+        dataCommunicator.setDataProvider(dataProvider);
+        dataCommunicator.pushData(1, beans2.stream());
+
+        SOME_BEAN bean2_17 = beans2.get(17);
+        String key2_17 = dataCommunicator.getKeyMapper().key(bean2_17);
+
+        assertSame(bean2_17, dataCommunicator.getKeyMapper().get(key2_17));
+        assertNotEquals(key2_17, key1_17);
+        assertNull(dataCommunicator.getKeyMapper().get(key1_17));
+    }
+
+    private <SOME_BEAN> List<SOME_BEAN> createCollection(
+            IntFunction<SOME_BEAN> beanConstructor) {
+        return IntStream.range(1, 100).mapToObj(beanConstructor)
+                .collect(Collectors.toList());
+    }
+}

server/src/test/java/com/vaadin/ui/AbstractListingTest.java

@@ -40,9 +40,8 @@ public class AbstractListingTest {
         }
 
         @Override
-        protected List<String> readItems(Element design,
+        protected void readItems(Element design,
                 DesignContext context) {
-            return null;
         }
 
         @Override

server/src/test/java/com/vaadin/ui/AbstractSingleSelectTest.java

@@ -60,9 +60,8 @@ public class AbstractSingleSelectTest {
         }
 
         @Override
-        protected List<Person> readItems(Element design,
+        protected void readItems(Element design,
                 DesignContext context) {
-            return null;
         }
 
         @Override
@@ -254,9 +253,8 @@ public class AbstractSingleSelectTest {
             }
 
             @Override
-            protected List<String> readItems(Element design,
+            protected void readItems(Element design,
                     DesignContext context) {
-                return null;
             }
         };
 

uitest/src/main/java/com/vaadin/tests/data/ReplaceDataProvider.java

@@ -0,0 +1,67 @@
+package com.vaadin.tests.data;
+
+import java.util.List;
+import java.util.stream.Collectors;
+import java.util.stream.IntStream;
+
+import com.vaadin.server.VaadinRequest;
+import com.vaadin.tests.components.AbstractTestUI;
+import com.vaadin.ui.Button;
+import com.vaadin.ui.Grid;
+
+public class ReplaceDataProvider extends AbstractTestUI {
+
+    private static class TestClass {
+        public String someField;
+        public int hash;
+
+        public TestClass(int hash) {
+            this.hash = hash;
+            someField = "a";
+        }
+
+        @Override
+        public int hashCode() {
+            return hash;
+        }
+
+        @Override
+        public boolean equals(Object obj) {
+            return true;
+        }
+    }
+
+    @Override
+    protected void setup(VaadinRequest request) {
+        Grid<TestClass> grid = new Grid<>();
+        grid.addColumn(item -> item.someField);
+
+        List<TestClass> listOfClasses = IntStream.range(0, 10)
+                .mapToObj(TestClass::new).collect(Collectors.toList());
+        for (int i = 0; i < 10; i++) {
+            listOfClasses.add(new TestClass(10));
+        }
+
+        grid.setItems(listOfClasses);
+
+        Button replaceBtn = new Button("replace data provider");
+        replaceBtn.addClickListener(clickEvent -> {
+            List<TestClass> newList = IntStream.range(0, 10)
+                    .mapToObj(TestClass::new).collect(Collectors.toList());
+            newList.get(0).someField = "b";
+            grid.setItems(newList);
+        });
+
+        Button replaceAndSelectBtn = new Button(
+                "replace data provider and select second");
+        replaceAndSelectBtn.addClickListener(clickEvent -> {
+            List<TestClass> newList = IntStream.range(0, 10)
+                    .mapToObj(TestClass::new).collect(Collectors.toList());
+            newList.get(0).someField = "b";
+            grid.setItems(newList);
+            grid.getSelectionModel().select(newList.get(1));
+        });
+
+        addComponents(replaceBtn, replaceAndSelectBtn, grid);
+    }
+}

uitest/src/test/java/com/vaadin/tests/data/ReplaceDataProviderTest.java

@@ -0,0 +1,53 @@
+package com.vaadin.tests.data;
+
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Test;
+
+import com.vaadin.testbench.elements.ButtonElement;
+import com.vaadin.testbench.elements.GridElement;
+import com.vaadin.tests.tb3.SingleBrowserTest;
+
+public class ReplaceDataProviderTest extends SingleBrowserTest {
+
+    @Before
+    public void setUp() {
+        openTestURL();
+    }
+
+    @Test
+    public void test_grid_data_communication_with_replaced_data_provider() {
+        GridElement grid = $(GridElement.class).first();
+        ButtonElement replaceDataProviderButton = $(ButtonElement.class)
+                .first();
+
+        Assert.assertEquals(20, grid.getRowCount());
+        grid.getCell(0, 0).click();
+        assertCellText("a", 0, 0);
+
+        replaceDataProviderButton.click();
+
+        Assert.assertEquals(10, grid.getRowCount());
+        assertCellText("b", 0, 0);
+        for (int i = 1; i < 10; i++) {
+            assertCellText("a", i, 0);
+        }
+
+        Assert.assertFalse(grid.getRow(0).isSelected());
+
+        grid.getCell(0, 0).click();
+        assertCellText("b", 0, 0);
+
+        // This button should replace the data provider and do a server side
+        // select on the second item
+        $(ButtonElement.class).get(1).click();
+        grid.getRow(1).isSelected();
+    }
+
+    private void assertCellText(String text, int rowIndex, int colIndex) {
+        String firstCellText = $(GridElement.class).first()
+                .getCell(rowIndex, colIndex)
+                .getText();
+        Assert.assertEquals(text, firstCellText);
+    }
+}