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
vaadin · Jan 10, 2017 · ae54094 · ae54094
1 parent 9ee9273
commit ae54094
Show file tree

Hide file tree

Showing 15 changed files with 288 additions and 33 deletions.
diff --git a/server/src/main/java/com/vaadin/data/provider/DataCommunicator.java b/server/src/main/java/com/vaadin/data/provider/DataCommunicator.java
@@ -178,6 +178,12 @@ public void destroyData(T data) {
             // 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 @@ private void dropData(Collection<String> droppedKeys) {
         }
     }
 
+    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 void setDataProvider(DataProvider<T, F> dataProvider) {
         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

diff --git a/server/src/main/java/com/vaadin/data/provider/DataGenerator.java b/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() {
+    }
 }
diff --git a/server/src/main/java/com/vaadin/ui/AbstractListing.java b/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 @@ protected void doReadDesign(Element design, DesignContext context) {
         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 @@ protected void doReadDesign(Element design, DesignContext context) {
      *            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.

diff --git a/server/src/main/java/com/vaadin/ui/AbstractMultiSelect.java b/server/src/main/java/com/vaadin/ui/AbstractMultiSelect.java
@@ -25,17 +25,18 @@
 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;
 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 void generateData(T data, JsonObject jsonObject) {
         @Override
         public void destroyData(T data) {
         }
+
+        @Override
+        public void destroyAllData() {
+            AbstractMultiSelect.this.deselectAll();
+        }
     }
 
     /**
@@ -422,14 +428,16 @@ protected Element writeItem(Element design, T item, DesignContext context) {
     }
 
     @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;
     }
 
     /**

diff --git a/server/src/main/java/com/vaadin/ui/AbstractSingleSelect.java b/server/src/main/java/com/vaadin/ui/AbstractSingleSelect.java
@@ -26,7 +26,7 @@
 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 @@ protected Element writeItem(Element design, T item, DesignContext context) {
     }
 
     @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;
     }
 
     /**

diff --git a/server/src/main/java/com/vaadin/ui/CheckBoxGroup.java b/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 Registration addBlurListener(BlurListener listener) {
     }
 
     @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" })

diff --git a/server/src/main/java/com/vaadin/ui/ComboBox.java b/server/src/main/java/com/vaadin/ui/ComboBox.java
@@ -18,7 +18,6 @@
 
 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 @@ protected Element writeItem(Element design, T item, DesignContext context) {
     }
 
     @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" })

diff --git a/server/src/main/java/com/vaadin/ui/Grid.java b/server/src/main/java/com/vaadin/ui/Grid.java
@@ -2895,8 +2895,8 @@ private void fireColumnResizeEvent(Column<?, ?> column,
     }
 
     @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 @@ private void readData(Element body,
             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 @@ private void readData(Element body,
         }
 
         setItems(items);
+        selectedItems.forEach(getSelectionModel()::select);
     }
 
     private void writeStructure(Element design, DesignContext designContext) {

diff --git a/server/src/main/java/com/vaadin/ui/RadioButtonGroup.java b/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 Registration addBlurListener(BlurListener listener) {
     }
 
     @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" })

diff --git a/server/src/main/java/com/vaadin/ui/components/grid/AbstractSelectionModel.java b/server/src/main/java/com/vaadin/ui/components/grid/AbstractSelectionModel.java
@@ -43,6 +43,11 @@ public void generateData(T item, JsonObject jsonObject) {
         }
     }
 
+    @Override
+    public void destroyAllData() {
+        deselectAll();
+    }
+
     @Override
     protected AbstractSelectionModelState getState() {
         return (AbstractSelectionModelState) super.getState();

diff --git a/server/src/test/java/com/vaadin/data/provider/ReplaceDataProviderTest.java b/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());
+    }
+}
diff --git a/server/src/test/java/com/vaadin/ui/AbstractListingTest.java b/server/src/test/java/com/vaadin/ui/AbstractListingTest.java
@@ -40,9 +40,8 @@ protected Element writeItem(Element design, String item,
         }
 
         @Override
-        protected List<String> readItems(Element design,
+        protected void readItems(Element design,
                 DesignContext context) {
-            return null;
         }
 
         @Override