Fix grid sorting on init and sorting with unused properties (#16192)

Change-Id: I247a981c6a38bf78936f81f087ce3d5e6d354149

4 files changed, 76 insertions, 12 deletions

diff --git a/client/src/com/vaadin/client/connectors/GridConnector.java b/client/src/com/vaadin/client/connectors/GridConnector.java
index 98562a871e..5a68bbf4b8 100644
--- a/client/src/com/vaadin/client/connectors/GridConnector.java
+++ b/client/src/com/vaadin/client/connectors/GridConnector.java
@@ -517,6 +517,11 @@ public class GridConnector extends AbstractHasComponentsConnector implements
             updateFooterFromState(getState().footer);
         }
 
+        if (stateChangeEvent.hasPropertyChanged("sortColumns")
+                || stateChangeEvent.hasPropertyChanged("sortDirs")) {
+            onSortStateChange();
+        }
+
         if (stateChangeEvent.hasPropertyChanged("editorEnabled")) {
             getWidget().setEditorEnabled(getState().editorEnabled);
         }
@@ -837,7 +842,6 @@ public class GridConnector extends AbstractHasComponentsConnector implements
         }
     }
 
-    @OnStateChange({ "sortColumns", "sortDirs" })
     private void onSortStateChange() {
         List<SortOrder> sortOrder = new ArrayList<SortOrder>();
 
diff --git a/server/src/com/vaadin/ui/Grid.java b/server/src/com/vaadin/ui/Grid.java
index 443e51fba3..0d74c01027 100644
--- a/server/src/com/vaadin/ui/Grid.java
+++ b/server/src/com/vaadin/ui/Grid.java
@@ -3638,9 +3638,18 @@ public class Grid extends AbstractComponent implements SelectionNotifier,
     /**
      * Sets the current sort order using the fluid Sort API. Read the
      * documentation for {@link Sort} for more information.
+     * <p>
+     * <em>Note:</em> Sorting by a property that has no column in Grid will hide
+     * all possible sorting indicators.
      * 
      * @param s
      *            a sort instance
+     * 
+     * @throws IllegalStateException
+     *             if container is not sortable (does not implement
+     *             Container.Sortable)
+     * @throws IllegalArgumentException
+     *             if trying to sort by non-existing property
      */
     public void sort(Sort s) {
         setSortOrder(s.build());
@@ -3648,9 +3657,18 @@ public class Grid extends AbstractComponent implements SelectionNotifier,
 
     /**
      * Sort this Grid in ascending order by a specified property.
+     * <p>
+     * <em>Note:</em> Sorting by a property that has no column in Grid will hide
+     * all possible sorting indicators.
      * 
      * @param propertyId
      *            a property ID
+     * 
+     * @throws IllegalStateException
+     *             if container is not sortable (does not implement
+     *             Container.Sortable)
+     * @throws IllegalArgumentException
+     *             if trying to sort by non-existing property
      */
     public void sort(Object propertyId) {
         sort(propertyId, SortDirection.ASCENDING);
@@ -3658,11 +3676,20 @@ public class Grid extends AbstractComponent implements SelectionNotifier,
 
     /**
      * Sort this Grid in user-specified {@link SortOrder} by a property.
+     * <p>
+     * <em>Note:</em> Sorting by a property that has no column in Grid will hide
+     * all possible sorting indicators.
      * 
      * @param propertyId
      *            a property ID
      * @param direction
      *            a sort order value (ascending/descending)
+     * 
+     * @throws IllegalStateException
+     *             if container is not sortable (does not implement
+     *             Container.Sortable)
+     * @throws IllegalArgumentException
+     *             if trying to sort by non-existing property
      */
     public void sort(Object propertyId, SortDirection direction) {
         sort(Sort.by(propertyId, direction));
@@ -3677,20 +3704,26 @@ public class Grid extends AbstractComponent implements SelectionNotifier,
     }
 
     /**
-     * Sets the sort order to use. This method throws
-     * {@link IllegalStateException} if the attached container is not a
-     * {@link Container.Sortable}, and {@link IllegalArgumentException} if a
-     * property in the list is not recognized by the container, or if the
-     * 'order' parameter is null.
+     * Sets the sort order to use.
+     * <p>
+     * <em>Note:</em> Sorting by a property that has no column in Grid will hide
+     * all possible sorting indicators.
      * 
      * @param order
      *            a sort order list.
+     * 
+     * @throws IllegalStateException
+     *             if container is not sortable (does not implement
+     *             Container.Sortable)
+     * @throws IllegalArgumentException
+     *             if order is null or trying to sort by non-existing property
      */
     public void setSortOrder(List<SortOrder> order) {
         setSortOrder(order, false);
     }
 
-    private void setSortOrder(List<SortOrder> order, boolean userOriginated) {
+    private void setSortOrder(List<SortOrder> order, boolean userOriginated)
+            throws IllegalStateException, IllegalArgumentException {
         if (!(getContainerDataSource() instanceof Container.Sortable)) {
             throw new IllegalStateException(
                     "Attached container is not sortable (does not implement Container.Sortable)");
@@ -3740,15 +3773,12 @@ public class Grid extends AbstractComponent implements SelectionNotifier,
             Object[] propertyIds = new Object[items];
             boolean[] directions = new boolean[items];
 
-            String[] columnKeys = new String[items];
             SortDirection[] stateDirs = new SortDirection[items];
 
             for (int i = 0; i < items; ++i) {
                 SortOrder order = sortOrder.get(i);
 
-                columnKeys[i] = this.columnKeys.key(order.getPropertyId());
                 stateDirs[i] = order.getDirection();
-
                 propertyIds[i] = order.getPropertyId();
                 switch (order.getDirection()) {
                 case ASCENDING:
@@ -3768,8 +3798,18 @@ public class Grid extends AbstractComponent implements SelectionNotifier,
             fireEvent(new SortEvent(this, new ArrayList<SortOrder>(sortOrder),
                     userOriginated));
 
-            getState().sortColumns = columnKeys;
-            getState(false).sortDirs = stateDirs;
+            if (columns.keySet().containsAll(Arrays.asList(propertyIds))) {
+                String[] columnKeys = new String[items];
+                for (int i = 0; i < items; ++i) {
+                    columnKeys[i] = this.columnKeys.key(propertyIds[i]);
+                }
+                getState().sortColumns = columnKeys;
+                getState(false).sortDirs = stateDirs;
+            } else {
+                // Not all sorted properties are in Grid. Remove any indicators.
+                getState().sortColumns = new String[] {};
+                getState(false).sortDirs = new SortDirection[] {};
+            }
         } else {
             throw new IllegalStateException(
                     "Container is not sortable (does not implement Container.Sortable)");
diff --git a/uitest/src/com/vaadin/tests/components/grid/GridGeneratedProperties.java b/uitest/src/com/vaadin/tests/components/grid/GridGeneratedProperties.java
index 294c23ffe5..2782a5fc6c 100644
--- a/uitest/src/com/vaadin/tests/components/grid/GridGeneratedProperties.java
+++ b/uitest/src/com/vaadin/tests/components/grid/GridGeneratedProperties.java
@@ -27,6 +27,7 @@ import com.vaadin.data.util.PropertyValueGenerator;
 import com.vaadin.data.util.filter.Compare;
 import com.vaadin.data.util.filter.UnsupportedFilterException;
 import com.vaadin.server.VaadinRequest;
+import com.vaadin.shared.data.sort.SortDirection;
 import com.vaadin.tests.components.AbstractTestUI;
 import com.vaadin.ui.Button;
 import com.vaadin.ui.Button.ClickEvent;
@@ -130,6 +131,7 @@ public class GridGeneratedProperties extends AbstractTestUI {
                 });
 
         addComponent(filterButton);
+        grid.sort(Sort.by("km").then("bar", SortDirection.DESCENDING));
     }
 
     private Indexed createContainer() {
diff --git a/uitest/src/com/vaadin/tests/components/grid/GridGeneratedPropertiesTest.java b/uitest/src/com/vaadin/tests/components/grid/GridGeneratedPropertiesTest.java
index c71dd80360..ffcd4c448f 100644
--- a/uitest/src/com/vaadin/tests/components/grid/GridGeneratedPropertiesTest.java
+++ b/uitest/src/com/vaadin/tests/components/grid/GridGeneratedPropertiesTest.java
@@ -23,6 +23,7 @@ import org.junit.Test;
 
 import com.vaadin.testbench.elements.GridElement;
 import com.vaadin.testbench.elements.GridElement.GridCellElement;
+import com.vaadin.testbench.elements.NotificationElement;
 import com.vaadin.tests.annotations.TestCategory;
 import com.vaadin.tests.tb3.MultiBrowserTest;
 
@@ -69,4 +70,21 @@ public class GridGeneratedPropertiesTest extends MultiBrowserTest {
         assertTrue("Column baz was not sorted descending", bazHeader
                 .getAttribute("class").contains("sort-desc"));
     }
+
+    @Test
+    public void testInitialSorting() {
+        // Grid is sorted in this case by one visible and one nonexistent
+        // column. There should be no sort indicator.
+        setDebug(true);
+        openTestURL();
+
+        GridElement grid = $(GridElement.class).first();
+
+        GridCellElement kmHeader = grid.getHeaderCell(0, 1);
+        assertFalse("Column km was unexpectedly sorted",
+                kmHeader.getAttribute("class").contains("sort-asc")
+                        || kmHeader.getAttribute("class").contains("sort-desc"));
+        assertFalse("Unexpected client-side exception was visible",
+                isElementPresent(NotificationElement.class));
+    }
 }