Fix Column.setSortable being overridden (#10604)

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

@@ -56,6 +56,7 @@ import com.vaadin.data.provider.DataGenerator;
 import com.vaadin.data.provider.DataProvider;
 import com.vaadin.data.provider.GridSortOrder;
 import com.vaadin.data.provider.GridSortOrderBuilder;
+import com.vaadin.data.provider.InMemoryDataProvider;
 import com.vaadin.data.provider.Query;
 import com.vaadin.data.provider.QuerySortOrder;
 import com.vaadin.event.ConnectorEvent;
@@ -840,6 +841,7 @@ public class Grid<T> extends AbstractListing<T> implements HasComponents,
             return Stream.of(new QuerySortOrder(id, direction));
         };
 
+        private boolean sortable = true;
         private SerializableComparator<T> comparator;
         private StyleGenerator<T> styleGenerator = item -> null;
         private DescriptionGenerator<T> descriptionGenerator;
@@ -1151,11 +1153,12 @@ public class Grid<T> extends AbstractListing<T> implements HasComponents,
          */
         public Column<T, V> setId(String id) {
             Objects.requireNonNull(id, "Column identifier cannot be null");
-            if (this.userId != null) {
+            if (userId != null) {
                 throw new IllegalStateException(
                         "Column identifier cannot be changed");
             }
-            this.userId = id;
+
+            userId = id;
             getGrid().setColumnId(id, this);
             updateSortable();
 
@@ -1163,8 +1166,11 @@ public class Grid<T> extends AbstractListing<T> implements HasComponents,
         }
 
         private void updateSortable() {
-            setSortable(getGrid().getDataProvider().isInMemory()
-                    || getSortOrder(SortDirection.ASCENDING).count() != 0);
+            boolean inMemory = getGrid().getDataProvider().isInMemory();
+            boolean hasSortOrder = getSortOrder(SortDirection.ASCENDING)
+                    .count() != 0;
+
+            getState().sortable = this.sortable && (inMemory || hasSortOrder);
         }
 
         /**
@@ -1180,29 +1186,43 @@ public class Grid<T> extends AbstractListing<T> implements HasComponents,
         }
 
         /**
-         * Sets whether the user can sort this column or not.
-         * <p>
-         * By default, a grid using a in-memory data provider has its columns
-         * sortable by default. For a backend data provider, the columns are not
-         * sortable by default.
+         * Sets whether the user can sort this column or not. Whether the column
+         * is actually sortable after {@code setSortable(true)} depends on the
+         * {@link DataProvider} and the defined sort order for this column. When
+         * using an {@link InMemoryDataProvider} sorting can be automatic.
          *
          * @param sortable
-         *            {@code true} if the column can be sorted by the user;
-         *            {@code false} if not
+         *            {@code true} to enable sorting for this column;
+         *            {@code false} to disable it
          * @return this column
          */
         public Column<T, V> setSortable(boolean sortable) {
-            getState().sortable = sortable;
+            if (this.sortable != sortable) {
+                this.sortable = sortable;
+                updateSortable();
+            }
             return this;
         }
 
         /**
-         * Gets whether the user can sort this column or not.
+         * Gets whether sorting is enabled for this column.
          *
-         * @return {@code true} if the column can be sorted by the user;
+         * @return {@code true} if the sorting is enabled for this column;
          *         {@code false} if not
          */
         public boolean isSortable() {
+            return sortable;
+        }
+
+        /**
+         * Gets whether the user can actually sort this column.
+         *
+         * @return {@code true} if the column can be sorted by the user;
+         *         {@code false} if not
+         *
+         * @since
+         */
+        public boolean isSortableByUser() {
             return getState(false).sortable;
         }
 
@@ -2070,8 +2090,7 @@ public class Grid<T> extends AbstractListing<T> implements HasComponents,
          * @return this column
          * @since 8.3
          */
-        public Column<T, V> setHandleWidgetEvents(
-                boolean handleWidgetEvents) {
+        public Column<T, V> setHandleWidgetEvents(boolean handleWidgetEvents) {
             getState().handleWidgetEvents = handleWidgetEvents;
             return this;
         }

server/src/test/java/com/vaadin/tests/server/component/grid/GridDeclarativeTest.java

@@ -82,8 +82,8 @@ public class GridDeclarativeTest extends AbstractListingDeclarativeTest<Grid> {
                         + "<th plain-text column-ids='id'>Id</th></tr>"
                         + "</thead></table></%s>",
                 getComponentTag(),
-                heightMode.toString().toLowerCase(Locale.ROOT),
-                frozenColumns, heightByRows,
+                heightMode.toString().toLowerCase(Locale.ROOT), frozenColumns,
+                heightByRows,
                 SelectionMode.MULTI.toString().toLowerCase(Locale.ROOT),
                 getComponentTag());
 
@@ -169,27 +169,28 @@ public class GridDeclarativeTest extends AbstractListingDeclarativeTest<Grid> {
     public void columnAttributes() {
         Grid<Person> grid = new Grid<>();
 
-        String secondColumnId = "id";
-        Column<Person, String> column1 = grid.addColumn(Person::getFirstName)
-                .setCaption("First Name");
+        String secondColumnId = "sortableColumn";
+        Column<Person, String> notSortableColumn = grid
+                .addColumn(Person::getFirstName).setCaption("First Name");
         Column<Person, String> column2 = grid.addColumn(Person::getLastName)
                 .setId(secondColumnId).setCaption("Id");
 
-        String caption = "test-caption";
-        column1.setCaption(caption);
+        String caption = "not-sortable-column";
+        notSortableColumn.setCaption(caption);
         boolean sortable = false;
-        column1.setSortable(sortable);
+        notSortableColumn.setSortable(sortable);
         boolean editable = true;
-        column1.setEditorComponent(new TextField(), Person::setLastName);
-        column1.setEditable(editable);
+        notSortableColumn.setEditorComponent(new TextField(),
+                Person::setLastName);
+        notSortableColumn.setEditable(editable);
         boolean resizable = false;
-        column1.setResizable(resizable);
+        notSortableColumn.setResizable(resizable);
         boolean hidable = true;
-        column1.setHidable(hidable);
+        notSortableColumn.setHidable(hidable);
         boolean hidden = true;
-        column1.setHidden(hidden);
+        notSortableColumn.setHidden(hidden);
 
-        String hidingToggleCaption = "toggle-caption";
+        String hidingToggleCaption = "sortable-toggle-caption";
         column2.setHidingToggleCaption(hidingToggleCaption);
         double width = 17.3;
         column2.setWidth(width);
@@ -200,20 +201,15 @@ public class GridDeclarativeTest extends AbstractListingDeclarativeTest<Grid> {
         int expandRatio = 83;
         column2.setExpandRatio(expandRatio);
 
-
-        String sortableSuffix = "";
-        if (sortable) {
-            sortableSuffix = "='true'";
-        }
         String design = String.format("<%s><table><colgroup>"
-                + "<col column-id='column0' sortable%s editable resizable='%s' hidable hidden>"
-                + "<col column-id='id' sortable hiding-toggle-caption='%s' width='%s' min-width='%s' max-width='%s' expand='%s'>"
+                + "<col column-id='column0' sortable='%s' editable resizable='%s' hidable hidden>"
+                + "<col column-id='sortableColumn' sortable hiding-toggle-caption='%s' width='%s' min-width='%s' max-width='%s' expand='%s'>"
                 + "</colgroup><thead>"
                 + "<tr default><th plain-text column-ids='column0'>%s</th>"
-                + "<th plain-text column-ids='id'>%s</th>" + "</tr></thead>"
-                + "</table></%s>", getComponentTag(), sortableSuffix, resizable,
-                hidingToggleCaption, width, minWidth, maxWidth, expandRatio,
-                caption, "Id", getComponentTag());
+                + "<th plain-text column-ids='sortableColumn'>%s</th>"
+                + "</tr></thead>" + "</table></%s>", getComponentTag(),
+                sortable, resizable, hidingToggleCaption, width, minWidth,
+                maxWidth, expandRatio, caption, "Id", getComponentTag());
 
         testRead(design, grid, true);
         testWrite(design, grid);
@@ -705,7 +701,7 @@ public class GridDeclarativeTest extends AbstractListingDeclarativeTest<Grid> {
             // column.getId() affects .isSortable()
             if ((id1 != null && id2 != null) || (id1 == null && id2 == null)) {
                 assertEquals(baseError + "Sortable", col1.isSortable(),
-                    col2.isSortable());
+                        col2.isSortable());
             }
             assertEquals(baseError + "Editable", col1.isEditable(),
                     col2.isEditable());

server/src/test/java/com/vaadin/tests/server/component/grid/GridTest.java

@@ -28,6 +28,7 @@ import java.util.stream.Collectors;
 import java.util.stream.Stream;
 
 import org.easymock.Capture;
+import org.junit.Assert;
 import org.junit.Before;
 import org.junit.Rule;
 import org.junit.Test;
@@ -38,8 +39,10 @@ import com.vaadin.data.ValidationException;
 import com.vaadin.data.ValueProvider;
 import com.vaadin.data.provider.DataCommunicator;
 import com.vaadin.data.provider.DataGenerator;
+import com.vaadin.data.provider.DataProvider;
 import com.vaadin.data.provider.GridSortOrder;
 import com.vaadin.data.provider.QuerySortOrder;
+import com.vaadin.data.provider.SortOrder;
 import com.vaadin.data.provider.bov.Person;
 import com.vaadin.event.selection.SelectionEvent;
 import com.vaadin.server.SerializableComparator;
@@ -553,7 +556,7 @@ public class GridTest {
     public void setColumnOrder_byColumn_removedColumn() {
         thrown.expect(IllegalStateException.class);
         thrown.expectMessage("setColumnOrder should not be called "
-                        + "with columns that are not in the grid.");
+                + "with columns that are not in the grid.");
 
         grid.removeColumn(randomColumn);
         grid.setColumnOrder(randomColumn, lengthColumn);
@@ -732,4 +735,51 @@ public class GridTest {
         Column<Person, ?> column1 = grid1.addColumn(ValueProvider.identity());
         grid2.removeColumn(column1);
     }
+
+    @Test
+    public void testColumnSortable() {
+        Column<String, String> column = grid.addColumn(String::toString);
+
+        // Use in-memory data provider
+        grid.setItems(Collections.emptyList());
+
+        Assert.assertTrue("Column should be initially sortable",
+                column.isSortable());
+        Assert.assertTrue("User should be able to sort the column",
+                column.isSortableByUser());
+
+        column.setSortable(false);
+
+        Assert.assertFalse("Column should not be sortable",
+                column.isSortable());
+        Assert.assertFalse(
+                "User should not be able to sort the column with in-memory data",
+                column.isSortableByUser());
+
+        // Use CallBackDataProvider
+        grid.setDataProvider(
+                DataProvider.fromCallbacks(q -> Stream.of(), q -> 0));
+
+        Assert.assertFalse("Column should not be sortable",
+                column.isSortable());
+        Assert.assertFalse("User should not be able to sort the column",
+                column.isSortableByUser());
+
+        column.setSortable(true);
+
+        Assert.assertTrue("Column should be marked sortable",
+                column.isSortable());
+        Assert.assertFalse(
+                "User should not be able to sort the column since no sort order is provided",
+                column.isSortableByUser());
+
+        column.setSortProperty("toString");
+
+        Assert.assertTrue("Column should be marked sortable",
+                column.isSortable());
+        Assert.assertFalse(
+                "User should be able to sort the column with the sort order",
+                column.isSortableByUser());
+    }
+
 }