Reduce Grid's sort complexity. (#11566)

- limit DataCommunicator workaround from #11320 to ComboBox only
- don't reset DataCommunicator before modifying all sorting data

Fixes #11532

6 files changed, 157 insertions, 16 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
index 35dad20c49..4b12919176 100644
--- a/server/src/main/java/com/vaadin/data/provider/DataCommunicator.java
+++ b/server/src/main/java/com/vaadin/data/provider/DataCommunicator.java
@@ -40,6 +40,7 @@ import com.vaadin.shared.data.DataCommunicatorClientRpc;
 import com.vaadin.shared.data.DataCommunicatorConstants;
 import com.vaadin.shared.data.DataRequestRpc;
 import com.vaadin.shared.extension.datacommunicator.DataCommunicatorState;
+import com.vaadin.ui.ComboBox;
 
 import elemental.json.Json;
 import elemental.json.JsonArray;
@@ -530,7 +531,9 @@ public class DataCommunicator<T> extends AbstractExtension {
     public void reset() {
         // Only needed if a full reset is not pending.
         if (!reset) {
-            beforeClientResponse(true);
+            if (getParent() instanceof ComboBox) {
+                beforeClientResponse(true);
+            }
             // Soft reset through client-side re-request.
             getClientRpc().reset(getDataProviderSize());
         }
@@ -574,10 +577,28 @@ public class DataCommunicator<T> extends AbstractExtension {
      *
      * @param comparator
      *            comparator used to sort data
+     * @param immediateReset
+     *            {@code true} if an internal reset should be performed
+     *            immediately after updating the comparator (unless full reset
+     *            is already pending), {@code false} if you are going to trigger
+     *            reset separately later
+     */
+    public void setInMemorySorting(Comparator<T> comparator,
+            boolean immediateReset) {
+        inMemorySorting = comparator;
+        if (immediateReset) {
+            reset();
+        }
+    }
+
+    /**
+     * Sets the {@link Comparator} to use with in-memory sorting.
+     *
+     * @param comparator
+     *            comparator used to sort data
      */
     public void setInMemorySorting(Comparator<T> comparator) {
-        inMemorySorting = comparator;
-        reset();
+        setInMemorySorting(comparator, true);
     }
 
     /**
@@ -595,11 +616,29 @@ public class DataCommunicator<T> extends AbstractExtension {
      *
      * @param sortOrder
      *            list of sort order information to pass to a query
-     */
-    public void setBackEndSorting(List<QuerySortOrder> sortOrder) {
+     * @param immediateReset
+     *            {@code true} if an internal reset should be performed
+     *            immediately after updating the comparator (unless full reset
+     *            is already pending), {@code false} if you are going to trigger
+     *            reset separately later
+     */
+    public void setBackEndSorting(List<QuerySortOrder> sortOrder,
+            boolean immediateReset) {
         backEndSorting.clear();
         backEndSorting.addAll(sortOrder);
-        reset();
+        if (immediateReset) {
+            reset();
+        }
+    }
+
+    /**
+     * Sets the {@link QuerySortOrder}s to use with backend sorting.
+     *
+     * @param sortOrder
+     *            list of sort order information to pass to a query
+     */
+    public void setBackEndSorting(List<QuerySortOrder> sortOrder) {
+        setBackEndSorting(sortOrder, true);
     }
 
     /**
diff --git a/server/src/main/java/com/vaadin/data/provider/HierarchicalDataCommunicator.java b/server/src/main/java/com/vaadin/data/provider/HierarchicalDataCommunicator.java
index 73bd9e0221..706f0a9391 100644
--- a/server/src/main/java/com/vaadin/data/provider/HierarchicalDataCommunicator.java
+++ b/server/src/main/java/com/vaadin/data/provider/HierarchicalDataCommunicator.java
@@ -406,19 +406,31 @@ public class HierarchicalDataCommunicator<T> extends DataCommunicator<T> {
     }
 
     @Override
-    public void setBackEndSorting(List<QuerySortOrder> sortOrder) {
+    public void setBackEndSorting(List<QuerySortOrder> sortOrder,
+            boolean immediateReset) {
         if (mapper != null) {
             mapper.setBackEndSorting(sortOrder);
         }
-        super.setBackEndSorting(sortOrder);
+        super.setBackEndSorting(sortOrder, immediateReset);
     }
 
     @Override
-    public void setInMemorySorting(Comparator<T> comparator) {
+    public void setBackEndSorting(List<QuerySortOrder> sortOrder) {
+        setBackEndSorting(sortOrder, true);
+    }
+
+    @Override
+    public void setInMemorySorting(Comparator<T> comparator,
+            boolean immediateReset) {
         if (mapper != null) {
             mapper.setInMemorySorting(comparator);
         }
-        super.setInMemorySorting(comparator);
+        super.setInMemorySorting(comparator, immediateReset);
+    }
+
+    @Override
+    public void setInMemorySorting(Comparator<T> comparator) {
+        setInMemorySorting(comparator, true);
     }
 
     @Override
diff --git a/server/src/main/java/com/vaadin/ui/Grid.java b/server/src/main/java/com/vaadin/ui/Grid.java
index 74cd9a4626..3c9116fab3 100644
--- a/server/src/main/java/com/vaadin/ui/Grid.java
+++ b/server/src/main/java/com/vaadin/ui/Grid.java
@@ -4909,14 +4909,15 @@ public class Grid<T> extends AbstractListing<T> implements HasComponents,
     private void sort(boolean userOriginated) {
         // Set sort orders
         // In-memory comparator
-        getDataCommunicator().setInMemorySorting(createSortingComparator());
+        getDataCommunicator().setInMemorySorting(createSortingComparator(),
+                false);
 
         // Back-end sort properties
         List<QuerySortOrder> sortProperties = new ArrayList<>();
         sortOrder.stream().map(
                 order -> order.getSorted().getSortOrder(order.getDirection()))
                 .forEach(s -> s.forEach(sortProperties::add));
-        getDataCommunicator().setBackEndSorting(sortProperties);
+        getDataCommunicator().setBackEndSorting(sortProperties, true);
 
         // Close grid editor if it's open.
         if (getEditor().isOpen()) {
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 98ae758b6d..c187c91471 100644
--- a/server/src/test/java/com/vaadin/data/provider/DataCommunicatorTest.java
+++ b/server/src/test/java/com/vaadin/data/provider/DataCommunicatorTest.java
@@ -1,5 +1,9 @@
 package com.vaadin.data.provider;
 
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+
 import java.util.ArrayList;
 import java.util.Collections;
 import java.util.concurrent.Future;
@@ -21,9 +25,6 @@ import com.vaadin.ui.UI;
 import elemental.json.Json;
 import elemental.json.JsonArray;
 import elemental.json.JsonObject;
-import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertFalse;
-import static org.junit.Assert.assertTrue;
 
 /**
  * @author Vaadin Ltd
@@ -302,7 +303,7 @@ public class DataCommunicatorTest {
         // Mark communicator clean
         ui.getConnectorTracker().markClean(communicator);
 
-        assertFalse("Communicator should not be marked for hard reset",
+        assertTrue("Communicator should be marked for hard reset",
                 communicator.reset);
         assertFalse("DataCommunicator should not be marked as dirty",
                 ui.getConnectorTracker().isDirty(communicator));
diff --git a/uitest/src/main/java/com/vaadin/tests/components/grid/GridSortComplexity.java b/uitest/src/main/java/com/vaadin/tests/components/grid/GridSortComplexity.java
new file mode 100644
index 0000000000..4a6076554f
--- /dev/null
+++ b/uitest/src/main/java/com/vaadin/tests/components/grid/GridSortComplexity.java
@@ -0,0 +1,64 @@
+package com.vaadin.tests.components.grid;
+
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+
+import com.vaadin.data.provider.DataProvider;
+import com.vaadin.server.VaadinRequest;
+import com.vaadin.tests.components.AbstractTestUIWithLog;
+import com.vaadin.ui.Grid;
+import com.vaadin.ui.Grid.SelectionMode;
+
+public class GridSortComplexity extends AbstractTestUIWithLog {
+
+    @Override
+    protected int getLogSize() {
+        return 10;
+    }
+
+    @Override
+    protected void setup(VaadinRequest request) {
+        List<Integer> data = new ArrayList<>();
+        data.add(0);
+        data.add(1);
+        data.add(2);
+
+        Grid<Integer> grid = new Grid<>();
+        grid.addSortListener(event -> {
+            log("ON SORT: "
+                    + event.getSortOrder().get(0).getDirection().name());
+        });
+
+        grid.addColumn(Integer::valueOf).setCaption("ID").setId("id")
+                .setSortable(true).setSortProperty("id");
+
+        grid.setDataProvider(DataProvider.fromCallbacks(query -> {
+            log("FETCH");
+            if (query.getSortOrders().isEmpty() || "ASCENDING".equals(
+                    query.getSortOrders().get(0).getDirection().name())) {
+                return data.stream();
+            } else {
+                return data.stream().sorted(Collections.reverseOrder());
+            }
+        }, query -> {
+            log("SIZE");
+            return data.size();
+        }));
+
+        grid.setSelectionMode(SelectionMode.NONE);
+        grid.setWidth("250px");
+        grid.setHeightByRows(3);
+        addComponent(grid);
+    }
+
+    @Override
+    protected String getTestDescription() {
+        return "Sorting Grid should not fetch data and recalculate size multiple times.";
+    }
+
+    @Override
+    protected Integer getTicketNumber() {
+        return 11532;
+    }
+}
diff --git a/uitest/src/test/java/com/vaadin/tests/components/grid/GridSortComplexityTest.java b/uitest/src/test/java/com/vaadin/tests/components/grid/GridSortComplexityTest.java
new file mode 100644
index 0000000000..e7f4721df0
--- /dev/null
+++ b/uitest/src/test/java/com/vaadin/tests/components/grid/GridSortComplexityTest.java
@@ -0,0 +1,24 @@
+package com.vaadin.tests.components.grid;
+
+import static org.junit.Assert.assertEquals;
+
+import org.junit.Test;
+
+import com.vaadin.testbench.elements.GridElement;
+import com.vaadin.tests.tb3.MultiBrowserTest;
+
+public class GridSortComplexityTest extends MultiBrowserTest {
+
+    @Test
+    public void testOperationCountUponSorting() {
+        openTestURL();
+        GridElement grid = $(GridElement.class).first();
+        assertEquals("2. FETCH", getLogRow(0));
+        assertEquals("1. SIZE", getLogRow(1));
+
+        grid.getHeaderCell(0, 0).click();
+        assertEquals("5. FETCH", getLogRow(0));
+        assertEquals("4. ON SORT: ASCENDING", getLogRow(1));
+        assertEquals("3. SIZE", getLogRow(2));
+    }
+}