Refactor how DataCommunicator passes filters to its DataProvider (#8309)

The immediate benefit of this change is that ComboBox doesn't have to do
any wrapping when a ListDataProvider is set. A secondary benefit is that
a bunch of redundant type parameters and unsafe casts can be removed.

This is one of many steps towards #8245

9 files changed, 162 insertions, 75 deletions

diff --git a/server/src/main/java/com/vaadin/data/HasFilterableDataProvider.java b/server/src/main/java/com/vaadin/data/HasFilterableDataProvider.java
index 03e2e30379..4d2964b2e5 100644
--- a/server/src/main/java/com/vaadin/data/HasFilterableDataProvider.java
+++ b/server/src/main/java/com/vaadin/data/HasFilterableDataProvider.java
@@ -16,6 +16,7 @@
 package com.vaadin.data;
 
 import com.vaadin.data.provider.DataProvider;
+import com.vaadin.server.SerializableFunction;
 
 /**
  * A generic interface for listing components that use a filterable data
@@ -39,16 +40,32 @@ public interface HasFilterableDataProvider<T, F> extends HasItems<T> {
     /**
      * Returns the source of data items used by this listing.
      *
-     * @return the data provider, not null
+     * @return the data provider, not <code>null</code>
      */
-    public DataProvider<T, F> getDataProvider();
+    public DataProvider<T, ?> getDataProvider();
 
     /**
      * Sets the data provider for this listing. The data provider is queried for
      * displayed items as needed.
      *
      * @param dataProvider
-     *            the data provider, not null
+     *            the data provider, not <code>null</code>
      */
-    public void setDataProvider(DataProvider<T, F> dataProvider);
+    public default void setDataProvider(DataProvider<T, F> dataProvider) {
+        setDataProvider(dataProvider, SerializableFunction.identity());
+    }
+
+    /**
+     * Sets the data provider and filter converter for this listing. The data
+     * provider is queried for displayed items as needed.
+     *
+     * @param dataProvider
+     *            the data provider, not <code>null</code>
+     * @param filterConverter
+     *            a function that converts filter values produced by this
+     *            listing into filter values expected by the provided data
+     *            provider, not <code>null</code>
+     */
+    public <C> void setDataProvider(DataProvider<T, C> dataProvider,
+            SerializableFunction<F, C> filterConverter);
 }
diff --git a/server/src/main/java/com/vaadin/data/provider/DataCommunicator.java b/server/src/main/java/com/vaadin/data/provider/DataCommunicator.java
index 7b909b831d..021623555e 100644
--- a/server/src/main/java/com/vaadin/data/provider/DataCommunicator.java
+++ b/server/src/main/java/com/vaadin/data/provider/DataCommunicator.java
@@ -29,6 +29,7 @@ import java.util.stream.Stream;
 
 import com.vaadin.server.AbstractExtension;
 import com.vaadin.server.KeyMapper;
+import com.vaadin.server.SerializableConsumer;
 import com.vaadin.shared.Range;
 import com.vaadin.shared.Registration;
 import com.vaadin.shared.data.DataCommunicatorClientRpc;
@@ -48,12 +49,10 @@ import elemental.json.JsonObject;
  *
  * @param <T>
  *            the bean type
- * @param <F>
- *            the filter type
  *
  * @since 8.0
  */
-public class DataCommunicator<T, F> extends AbstractExtension {
+public class DataCommunicator<T> extends AbstractExtension {
 
     private Registration dataProviderUpdateRegistration;
 
@@ -190,7 +189,7 @@ public class DataCommunicator<T, F> extends AbstractExtension {
     private final ActiveDataHandler handler = new ActiveDataHandler();
 
     /** Empty default data provider */
-    private DataProvider<T, F> dataProvider = new CallbackDataProvider<>(
+    private DataProvider<T, ?> dataProvider = new CallbackDataProvider<>(
             q -> Stream.empty(), q -> 0);
     private final DataKeyMapper<T> keyMapper;
 
@@ -199,7 +198,7 @@ public class DataCommunicator<T, F> extends AbstractExtension {
     private int minPushSize = 40;
     private Range pushRows = Range.withLength(0, minPushSize);
 
-    private F filter;
+    private Object filter;
     private Comparator<T> inMemorySorting;
     private final List<QuerySortOrder> backEndSorting = new ArrayList<>();
     private final DataCommunicatorClientRpc rpc;
@@ -236,7 +235,8 @@ public class DataCommunicator<T, F> extends AbstractExtension {
         }
 
         if (initial || reset) {
-            int dataProviderSize = getDataProvider().size(new Query<>(filter));
+            @SuppressWarnings({ "rawtypes", "unchecked" })
+            int dataProviderSize = getDataProvider().size(new Query(filter));
             rpc.reset(dataProviderSize);
         }
 
@@ -244,7 +244,8 @@ public class DataCommunicator<T, F> extends AbstractExtension {
             int offset = pushRows.getStart();
             int limit = pushRows.length();
 
-            Stream<T> rowsToPush = getDataProvider().fetch(new Query<>(offset,
+            @SuppressWarnings({ "rawtypes", "unchecked" })
+            Stream<T> rowsToPush = getDataProvider().fetch(new Query(offset,
                     limit, backEndSorting, inMemorySorting, filter));
 
             pushData(offset, rowsToPush);
@@ -399,17 +400,6 @@ public class DataCommunicator<T, F> extends AbstractExtension {
     }
 
     /**
-     * Sets the filter to use.
-     *
-     * @param filter
-     *            the filter
-     */
-    public void setFilter(F filter) {
-        this.filter = filter;
-        reset();
-    }
-
-    /**
      * Sets the {@link Comparator} to use with in-memory sorting.
      *
      * @param comparator
@@ -459,19 +449,31 @@ public class DataCommunicator<T, F> extends AbstractExtension {
      *
      * @return the data provider
      */
-    public DataProvider<T, F> getDataProvider() {
+    public DataProvider<T, ?> getDataProvider() {
         return dataProvider;
     }
 
     /**
      * Sets the current data provider for this DataCommunicator.
+     * <p>
+     * The returned consumer can be used to set some other filter value that
+     * should be included in queries sent to the data provider. It is only valid
+     * until another data provider is set.
      *
      * @param dataProvider
-     *            the data provider to set, not null
+     *            the data provider to set, not <code>null</code>
+     * @param initialFilter
+     *            the initial filter value to use, or <code>null</code> to not
+     *            use any initial filter value
+     * @return a consumer that accepts a new filter value to use
      */
-    public void setDataProvider(DataProvider<T, F> dataProvider) {
+    public <F> SerializableConsumer<F> setDataProvider(
+            DataProvider<T, F> dataProvider, F initialFilter) {
         Objects.requireNonNull(dataProvider, "data provider cannot be null");
+
+        filter = initialFilter;
         this.dataProvider = dataProvider;
+
         detachDataProviderListener();
         dropAllData();
         /*
@@ -491,6 +493,18 @@ public class DataCommunicator<T, F> extends AbstractExtension {
             attachDataProviderListener();
         }
         reset();
+
+        return filter -> {
+            if (this.dataProvider != dataProvider) {
+                throw new IllegalStateException(
+                        "Filter slot is no longer valid after data provider has been changed");
+            }
+
+            if (!Objects.equals(this.filter, filter)) {
+                this.filter = filter;
+                reset();
+            }
+        };
     }
 
     /**
diff --git a/server/src/main/java/com/vaadin/ui/AbstractListing.java b/server/src/main/java/com/vaadin/ui/AbstractListing.java
index c3328683a1..dfb8ac103e 100644
--- a/server/src/main/java/com/vaadin/ui/AbstractListing.java
+++ b/server/src/main/java/com/vaadin/ui/AbstractListing.java
@@ -29,6 +29,7 @@ import com.vaadin.data.provider.DataProvider;
 import com.vaadin.data.provider.Query;
 import com.vaadin.server.AbstractExtension;
 import com.vaadin.server.Resource;
+import com.vaadin.server.SerializableConsumer;
 import com.vaadin.shared.extension.abstractlisting.AbstractListingExtensionState;
 import com.vaadin.shared.ui.abstractlisting.AbstractListingState;
 import com.vaadin.ui.Component.Focusable;
@@ -132,7 +133,7 @@ public abstract class AbstractListing<T> extends AbstractComponent
         }
     }
 
-    private final DataCommunicator<T, ?> dataCommunicator;
+    private final DataCommunicator<T> dataCommunicator;
 
     /**
      * Creates a new {@code AbstractListing} with a default data communicator.
@@ -154,7 +155,7 @@ public abstract class AbstractListing<T> extends AbstractComponent
      * @param dataCommunicator
      *            the data communicator to use, not null
      */
-    protected AbstractListing(DataCommunicator<T, ?> dataCommunicator) {
+    protected AbstractListing(DataCommunicator<T> dataCommunicator) {
         Objects.requireNonNull(dataCommunicator,
                 "dataCommunicator cannot be null");
 
@@ -162,10 +163,14 @@ public abstract class AbstractListing<T> extends AbstractComponent
         addExtension(dataCommunicator);
     }
 
-    @SuppressWarnings({ "rawtypes", "unchecked" })
     protected void internalSetDataProvider(DataProvider<T, ?> dataProvider) {
-        // cast needed by compiler
-        getDataCommunicator().setDataProvider((DataProvider) dataProvider);
+        internalSetDataProvider(dataProvider, null);
+    }
+
+    protected <F> SerializableConsumer<F> internalSetDataProvider(
+            DataProvider<T, F> dataProvider, F initialFilter) {
+        return getDataCommunicator().setDataProvider(dataProvider,
+                initialFilter);
     }
 
     protected DataProvider<T, ?> internalGetDataProvider() {
@@ -257,7 +262,7 @@ public abstract class AbstractListing<T> extends AbstractComponent
      *
      * @return the data communicator, not null
      */
-    public DataCommunicator<T, ?> getDataCommunicator() {
+    public DataCommunicator<T> getDataCommunicator() {
         return dataCommunicator;
     }
 
diff --git a/server/src/main/java/com/vaadin/ui/AbstractSingleSelect.java b/server/src/main/java/com/vaadin/ui/AbstractSingleSelect.java
index 224d6823e7..2b0e6b7d01 100644
--- a/server/src/main/java/com/vaadin/ui/AbstractSingleSelect.java
+++ b/server/src/main/java/com/vaadin/ui/AbstractSingleSelect.java
@@ -74,7 +74,7 @@ public abstract class AbstractSingleSelect<T> extends AbstractListing<T>
      * @param dataCommunicator
      *            the data communicator to use, not null
      */
-    protected AbstractSingleSelect(DataCommunicator<T, ?> dataCommunicator) {
+    protected AbstractSingleSelect(DataCommunicator<T> dataCommunicator) {
         super(dataCommunicator);
         init();
     }
@@ -152,9 +152,9 @@ public abstract class AbstractSingleSelect<T> extends AbstractListing<T>
     @Override
     public Registration addValueChangeListener(
             HasValue.ValueChangeListener<T> listener) {
-        return addSelectionListener(event -> listener.valueChange(
-                new ValueChangeEvent<>(this, event.getOldValue(),
-                        event.isUserOriginated())));
+        return addSelectionListener(
+                event -> listener.valueChange(new ValueChangeEvent<>(this,
+                        event.getOldValue(), event.isUserOriginated())));
     }
 
     @Override
diff --git a/server/src/main/java/com/vaadin/ui/ComboBox.java b/server/src/main/java/com/vaadin/ui/ComboBox.java
index d08f269fa3..515c581510 100644
--- a/server/src/main/java/com/vaadin/ui/ComboBox.java
+++ b/server/src/main/java/com/vaadin/ui/ComboBox.java
@@ -43,6 +43,7 @@ import com.vaadin.server.Resource;
 import com.vaadin.server.ResourceReference;
 import com.vaadin.server.SerializableBiPredicate;
 import com.vaadin.server.SerializableConsumer;
+import com.vaadin.server.SerializableFunction;
 import com.vaadin.shared.Registration;
 import com.vaadin.shared.data.DataCommunicatorConstants;
 import com.vaadin.shared.ui.combobox.ComboBoxConstants;
@@ -125,7 +126,8 @@ public class ComboBox<T> extends AbstractSingleSelect<T>
 
         @Override
         public void setFilter(String filterText) {
-            getDataCommunicator().setFilter(filterText);
+            currentFilterText = filterText;
+            filterSlot.accept(filterText);
         }
     };
 
@@ -136,13 +138,19 @@ public class ComboBox<T> extends AbstractSingleSelect<T>
 
     private StyleGenerator<T> itemStyleGenerator = item -> null;
 
+    private String currentFilterText;
+
+    private SerializableConsumer<String> filterSlot = filter -> {
+        // Just ignore when neither setDataProvider nor setItems has been called
+    };
+
     /**
      * Constructs an empty combo box without a caption. The content of the combo
      * box can be set with {@link #setDataProvider(DataProvider)} or
      * {@link #setItems(Collection)}
      */
     public ComboBox() {
-        super(new DataCommunicator<T, String>() {
+        super(new DataCommunicator<T>() {
             @Override
             protected DataKeyMapper<T> createKeyMapper() {
                 return new KeyMapper<T>() {
@@ -294,11 +302,6 @@ public class ComboBox<T> extends AbstractSingleSelect<T>
      * Sets a list data provider with an item caption filter as the data
      * provider of this combo box. The caption filter is used to compare the
      * displayed caption of each item to the filter text entered by the user.
-     * <p>
-     * Note that this is a shorthand that calls
-     * {@link #setDataProvider(DataProvider)} with a wrapper of the provided
-     * list data provider. This means that {@link #getDataProvider()} will
-     * return the wrapper instead of the original list data provider.
      *
      * @param captionFilter
      *            filter to check if an item is shown when user typed some text
@@ -313,9 +316,8 @@ public class ComboBox<T> extends AbstractSingleSelect<T>
 
         // Must do getItemCaptionGenerator() for each operation since it might
         // not be the same as when this method was invoked
-        DataProvider<T, String> provider = listDataProvider.filteringBy(
-                item -> getItemCaptionGenerator().apply(item), captionFilter);
-        setDataProvider(provider);
+        setDataProvider(listDataProvider, filterText -> item -> captionFilter
+                .test(getItemCaptionGenerator().apply(item), filterText));
     }
 
     /**
@@ -702,22 +704,30 @@ public class ComboBox<T> extends AbstractSingleSelect<T>
     }
 
     @Override
-    @SuppressWarnings("unchecked")
-    public DataProvider<T, String> getDataProvider() {
-        return (DataProvider<T, String>) internalGetDataProvider();
+    public DataProvider<T, ?> getDataProvider() {
+        return internalGetDataProvider();
     }
 
     @Override
-    public void setDataProvider(DataProvider<T, String> dataProvider) {
-        internalSetDataProvider(dataProvider);
-    }
+    public <C> void setDataProvider(DataProvider<T, C> dataProvider,
+            SerializableFunction<String, C> filterConverter) {
+        Objects.requireNonNull(dataProvider, "dataProvider cannot be null");
+        Objects.requireNonNull(filterConverter,
+                "filterConverter cannot be null");
+
+        SerializableFunction<String, C> convertOrNull = filterText -> {
+            if (filterText == null) {
+                return null;
+            }
 
-    @Override
-    @SuppressWarnings("unchecked")
-    public DataCommunicator<T, String> getDataCommunicator() {
-        // Not actually an unsafe cast. DataCommunicator is final and set by
-        // ComboBox.
-        return (DataCommunicator<T, String>) super.getDataCommunicator();
+            return filterConverter.apply(filterText);
+        };
+
+        SerializableConsumer<C> providerFilterSlot = internalSetDataProvider(
+                dataProvider, convertOrNull.apply(currentFilterText));
+
+        filterSlot = filter -> providerFilterSlot
+                .accept(convertOrNull.apply(filter));
     }
 
     @Override
diff --git a/server/src/main/java/com/vaadin/ui/components/grid/MultiSelectionModelImpl.java b/server/src/main/java/com/vaadin/ui/components/grid/MultiSelectionModelImpl.java
index 89e7374bc6..551918ef85 100644
--- a/server/src/main/java/com/vaadin/ui/components/grid/MultiSelectionModelImpl.java
+++ b/server/src/main/java/com/vaadin/ui/components/grid/MultiSelectionModelImpl.java
@@ -436,7 +436,7 @@ public class MultiSelectionModelImpl<T> extends AbstractSelectionModel<T>
             set.addAll(addedItems);
 
             // refresh method is NOOP for items that are not present client side
-            DataCommunicator<T, ?> dataCommunicator = getGrid()
+            DataCommunicator<T> dataCommunicator = getGrid()
                     .getDataCommunicator();
             removedItems.forEach(dataCommunicator::refresh);
             addedItems.forEach(dataCommunicator::refresh);
diff --git a/server/src/test/java/com/vaadin/data/provider/DataCommunicatorTest.java b/server/src/test/java/com/vaadin/data/provider/DataCommunicatorTest.java
index f49dbf1f88..50a3ebc337 100644
--- a/server/src/test/java/com/vaadin/data/provider/DataCommunicatorTest.java
+++ b/server/src/test/java/com/vaadin/data/provider/DataCommunicatorTest.java
@@ -22,7 +22,6 @@ import org.junit.Test;
 import org.mockito.Mockito;
 
 import com.vaadin.server.MockVaadinSession;
-import com.vaadin.server.SerializablePredicate;
 import com.vaadin.server.VaadinRequest;
 import com.vaadin.server.VaadinService;
 import com.vaadin.server.VaadinSession;
@@ -81,8 +80,7 @@ public class DataCommunicatorTest {
 
     }
 
-    private static class TestDataCommunicator
-            extends DataCommunicator<Object, SerializablePredicate<Object>> {
+    private static class TestDataCommunicator extends DataCommunicator<Object> {
         protected void extend(UI ui) {
             super.extend(ui);
         }
@@ -100,7 +98,7 @@ public class DataCommunicatorTest {
         TestDataCommunicator communicator = new TestDataCommunicator();
 
         TestDataProvider dataProvider = new TestDataProvider();
-        communicator.setDataProvider(dataProvider);
+        communicator.setDataProvider(dataProvider, null);
 
         Assert.assertFalse(dataProvider.isListenerAdded());
 
@@ -118,7 +116,7 @@ public class DataCommunicatorTest {
         TestDataCommunicator communicator = new TestDataCommunicator();
 
         TestDataProvider dataProvider = new TestDataProvider();
-        communicator.setDataProvider(dataProvider);
+        communicator.setDataProvider(dataProvider, null);
 
         communicator.extend(ui);
 
diff --git a/server/src/test/java/com/vaadin/data/provider/ReplaceDataProviderTest.java b/server/src/test/java/com/vaadin/data/provider/ReplaceDataProviderTest.java
index 0510022e5e..f64d6b27c5 100644
--- a/server/src/test/java/com/vaadin/data/provider/ReplaceDataProviderTest.java
+++ b/server/src/test/java/com/vaadin/data/provider/ReplaceDataProviderTest.java
@@ -11,8 +11,6 @@ import java.util.stream.IntStream;
 
 import org.junit.Test;
 
-import com.vaadin.server.SerializablePredicate;
-
 public class ReplaceDataProviderTest {
 
     public static class BeanWithEquals extends Bean {
@@ -73,14 +71,14 @@ public class ReplaceDataProviderTest {
 
     private <SOME_BEAN> void doTest(IntFunction<SOME_BEAN> beanConstructor) {
 
-        DataCommunicator<SOME_BEAN, SerializablePredicate<SOME_BEAN>> dataCommunicator = new DataCommunicator<>();
+        DataCommunicator<SOME_BEAN> dataCommunicator = new DataCommunicator<>();
 
         List<SOME_BEAN> beans1 = createCollection(beanConstructor);
 
         ListDataProvider<SOME_BEAN> dataProvider = new ListDataProvider<>(
                 beans1);
 
-        dataCommunicator.setDataProvider(dataProvider);
+        dataCommunicator.setDataProvider(dataProvider, null);
         dataCommunicator.pushData(1, beans1.stream());
 
         SOME_BEAN bean1_17 = beans1.get(17);
@@ -91,7 +89,7 @@ public class ReplaceDataProviderTest {
         List<SOME_BEAN> beans2 = createCollection(beanConstructor);
 
         dataProvider = new ListDataProvider<>(beans2);
-        dataCommunicator.setDataProvider(dataProvider);
+        dataCommunicator.setDataProvider(dataProvider, null);
         dataCommunicator.pushData(1, beans2.stream());
 
         SOME_BEAN bean2_17 = beans2.get(17);
diff --git a/server/src/test/java/com/vaadin/tests/server/component/combobox/ComboBoxFilteringTest.java b/server/src/test/java/com/vaadin/tests/server/component/combobox/ComboBoxFilteringTest.java
index e8c8d3dd6e..159af4ad0b 100644
--- a/server/src/test/java/com/vaadin/tests/server/component/combobox/ComboBoxFilteringTest.java
+++ b/server/src/test/java/com/vaadin/tests/server/component/combobox/ComboBoxFilteringTest.java
@@ -15,6 +15,7 @@
  */
 package com.vaadin.tests.server.component.combobox;
 
+import java.util.Collections;
 import java.util.List;
 import java.util.Locale;
 import java.util.stream.Collectors;
@@ -24,13 +25,16 @@ import org.junit.Assert;
 import org.junit.Before;
 import org.junit.Test;
 
+import com.vaadin.data.provider.DataCommunicator;
 import com.vaadin.data.provider.DataProvider;
 import com.vaadin.data.provider.ListDataProvider;
-import com.vaadin.data.provider.Query;
+import com.vaadin.server.ClientMethodInvocation;
+import com.vaadin.shared.ui.combobox.ComboBoxServerRpc;
 import com.vaadin.tests.data.bean.Address;
 import com.vaadin.tests.data.bean.Person;
 import com.vaadin.tests.data.bean.Sex;
 import com.vaadin.ui.ComboBox;
+import com.vaadin.ui.ComponentTest;
 
 /**
  * Test for ComboBox data providers and filtering.
@@ -178,21 +182,62 @@ public class ComboBoxFilteringTest {
         checkFiltering("t", "Engel", 2, 1);
     }
 
+    @Test
+    public void filterEmptyComboBox() {
+        // Testing that filtering doesn't cause problems in the edge case where
+        // neither setDataProvider nor setItems has been called
+        checkFiltering("foo", "bar", 0, 0);
+    }
+
+    @Test
+    public void setListDataProvider_notWrapped() {
+        ListDataProvider<Person> provider = new ListDataProvider<>(
+                Collections.emptyList());
+
+        comboBox.setDataProvider(provider);
+
+        Assert.assertSame(provider, comboBox.getDataProvider());
+    }
+
+    @Test
+    public void setItems_hasListDataProvider() {
+        comboBox.setItems();
+
+        Assert.assertEquals(ListDataProvider.class,
+                comboBox.getDataProvider().getClass());
+    }
+
     private void checkFiltering(String filterText, String nonMatchingFilterText,
             int totalMatches, int matchingResults) {
-        DataProvider<Person, String> dataProvider = comboBox.getDataProvider();
-
         Assert.assertEquals(
                 "ComboBox filtered out results with no filter applied",
-                totalMatches, dataProvider.size(new Query<>()));
+                totalMatches, comboBoxSizeWithFilter(null));
         Assert.assertEquals(
                 "ComboBox filtered out results with empty filter string",
-                totalMatches, dataProvider.size(new Query<>("")));
+                totalMatches, comboBoxSizeWithFilter(""));
         Assert.assertEquals("ComboBox filtered out wrong number of results",
-                matchingResults, dataProvider.size(new Query<>(filterText)));
+                matchingResults, comboBoxSizeWithFilter(filterText));
         Assert.assertEquals(
                 "ComboBox should have no results with a non-matching filter", 0,
-                dataProvider.size(new Query<>(nonMatchingFilterText)));
+                comboBoxSizeWithFilter(nonMatchingFilterText));
+    }
+
+    private int comboBoxSizeWithFilter(String filter) {
+        DataCommunicator<Person> dataCommunicator = comboBox
+                .getDataCommunicator();
+
+        // Discard any currently pending RPC calls
+        dataCommunicator.retrievePendingRpcCalls();
+
+        ComponentTest.getRpcProxy(comboBox, ComboBoxServerRpc.class)
+                .setFilter(filter);
+        dataCommunicator.beforeClientResponse(true);
+
+        ClientMethodInvocation resetInvocation = dataCommunicator
+                .retrievePendingRpcCalls().get(0);
+        assert resetInvocation.getMethodName().equals("reset");
+
+        return ((Integer) resetInvocation.getParameters()[0]).intValue();
     }
 
     private List<Person> getPersonCollection() {