fix: Prevent IllegalArgumentException with sortable columns (#468)

* fix: Prevent IllegalArgumentException when sortable columns are removed and recreated

Fixes: vaadin/vaadin-grid#1969
Warranty: Fixes exception related to sorting when recreating columns 

Co-authored-by: Manolo Carrasco <[email protected]>
Co-authored-by: Tulio Garcia <[email protected]>

vaadin-grid-flow-parent/vaadin-grid-flow-integration-tests/pom.xml

@@ -117,6 +117,12 @@
             <version>${project.version}</version>
             <scope>test</scope>
         </dependency>
+        <dependency>
+            <groupId>com.vaadin</groupId>
+            <artifactId>vaadin-select-testbench</artifactId>
+            <version>${project.version}</version>
+            <scope>test</scope>
+        </dependency>
         <dependency>
             <groupId>org.openjdk.jol</groupId>
             <artifactId>jol-core</artifactId>
@@ -136,6 +142,12 @@
             <groupId>com.vaadin</groupId>
             <artifactId>flow-polymer-template</artifactId>
         </dependency>
+		<!-- https://mvnrepository.com/artifact/javax.servlet/javax.servlet-api -->
+		<dependency>
+			<groupId>javax.servlet</groupId>
+			<artifactId>javax.servlet-api</artifactId>
+			<scope>provided</scope>
+		</dependency>
     </dependencies>
     <build>
         <plugins>

vaadin-grid-flow-parent/vaadin-grid-flow-integration-tests/src/main/java/com/vaadin/flow/component/grid/it/RemoveSortableColumnPage.java

@@ -0,0 +1,155 @@
+
+/*
+ * Copyright 2000-2020 Vaadin Ltd.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License"); you may not
+ * use this file except in compliance with the License. You may obtain a copy of
+ * the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations under
+ * the License.
+ */
+package com.vaadin.flow.component.grid.it;
+
+import com.vaadin.flow.component.AttachEvent;
+import com.vaadin.flow.component.Component;
+import com.vaadin.flow.component.button.Button;
+import com.vaadin.flow.component.grid.Grid;
+import com.vaadin.flow.component.grid.GridSortOrderBuilder;
+import com.vaadin.flow.component.grid.HeaderRow;
+import com.vaadin.flow.component.html.Span;
+import com.vaadin.flow.component.orderedlayout.VerticalLayout;
+import com.vaadin.flow.component.select.Select;
+import com.vaadin.flow.function.ValueProvider;
+import com.vaadin.flow.router.Route;
+
+import java.util.Arrays;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.function.Function;
+
+@Route("vaadin-grid/remove-sortable-column")
+public class RemoveSortableColumnPage extends VerticalLayout {
+
+    private MyGrid grid;
+    private Select<String>[] selects;
+
+    private final List<String> headers = Arrays.asList("firstName", "lastName",
+            "age", "gender", "nickname");
+    private final Map<String, ValueProvider<Person, ?>> headerToValueProvider = new HashMap<String, ValueProvider<Person, ?>>() {
+        {
+            put("firstName", person -> person.firstName);
+            put("lastName", person -> person.lastName);
+            put("age", person -> person.age);
+            put("gender", person -> person.gender);
+            put("nickname", person -> person.nickname);
+        }
+    };
+    private final Person[] people = new Person[] {
+            new Person("Ben", "Hanks", 25, "Male", "Bensy"),
+            new Person("Zackhary", "Smith", 26, "Male", "Zacsy"),
+            new Person("Tom", "Jones", 27, "Male", "Tomsy"),
+            new Person("Jerry", "Stallone", 28, "Male", "Jersey"),
+            new Person("Bob", "Rourke", 29, "Male", "Bobsy") };
+
+    class Person {
+        String firstName;
+        String lastName;
+        Integer age;
+        String gender;
+        String nickname;
+
+        Person(String firstName, String lastName, Integer age, String gender,
+                String nickname) {
+            this.firstName = firstName;
+            this.lastName = lastName;
+            this.age = age;
+            this.gender = gender;
+            this.nickname = nickname;
+        }
+    }
+
+    class MyGrid extends Grid<Person> {
+
+        private Function<Integer, Component> headerGenerator;
+        private HeaderRow headerRow;
+
+        public void setHeaderGenerator(
+                Function<Integer, Component> headerGenerator) {
+            headerRow = addFirstHeaderRow();
+            this.headerGenerator = headerGenerator;
+            redraw();
+        }
+
+        void setData() {
+            setItems(people);
+        }
+
+        public void redraw() {
+            setData();
+            addColumnsToContainer();
+            getDataCommunicator().reset();
+        }
+
+        void addColumnsToContainer() {
+            removeAllColumns();
+            for (int i = 0; i < 5; ++i) {
+                String value = selects[i].getValue();
+                addColumn(headerToValueProvider.get(value)).setSortable(true);
+                headerRow.getCells().get(i)
+                        .setComponent(headerGenerator.apply(i));
+            }
+        }
+    }
+
+    public Function<Integer, Component> getHeaderGenerator() {
+        return columnIndex -> selects[columnIndex];
+    }
+
+    @Override
+    public void onAttach(AttachEvent attachEvent) {
+        attachEvent.getUI().getSession().setErrorHandler(handler -> {
+            Span text = new Span("Error");
+            text.setId("error-handler-message");
+            add(text);
+        });
+    }
+
+    public RemoveSortableColumnPage() {
+        selects = new Select[5];
+        for (int i = 0; i < 5; ++i) {
+            Select<String> select = new Select<>();
+            select.setId("select" + i);
+            select.setItems(headers);
+            select.setValue(headers.get(i));
+            select.getElement().addEventListener("click", e -> {
+            }).addEventData("event.stopPropagation()");
+            select.addValueChangeListener(e -> {
+                grid.redraw();
+            });
+            selects[i] = select;
+
+        }
+        grid = new MyGrid();
+        grid.setHeaderGenerator(getHeaderGenerator());
+
+        add(grid);
+        grid.redraw();
+
+        Button sort = new Button("Sort");
+        sort.setId("sort");
+        sort.addClickListener(event -> {
+            GridSortOrderBuilder<Person> sortOrderBuilder = new GridSortOrderBuilder<>();
+            sortOrderBuilder.thenAsc(grid.getColumns().get(0));
+            grid.sort(sortOrderBuilder.build());
+        });
+        add(sort);
+    }
+
+}

vaadin-grid-flow-parent/vaadin-grid-flow-integration-tests/src/test/java/com/vaadin/flow/component/grid/it/RemoveSortableColumnsIT.java

@@ -0,0 +1,39 @@
+/*
+ * Copyright 2000-2020 Vaadin Ltd.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License"); you may not
+ * use this file except in compliance with the License. You may obtain a copy of
+ * the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations under
+ * the License.
+ */
+package com.vaadin.flow.component.grid.it;
+
+import org.junit.Assert;
+import org.junit.Test;
+import org.openqa.selenium.By;
+import org.openqa.selenium.NoSuchElementException;
+
+import com.vaadin.flow.component.select.testbench.SelectElement;
+import com.vaadin.flow.testutil.TestPath;
+import com.vaadin.tests.AbstractComponentIT;
+
+@TestPath("vaadin-grid/remove-sortable-column")
+public class RemoveSortableColumnsIT extends AbstractComponentIT {
+
+    @Test
+    public void removeSortableColumn_shouldNotGenerateAnException() {
+        open();
+        findElement(By.id("sort")).click();
+        SelectElement select = $(SelectElement.class).first();
+        select.selectItemByIndex(2);
+        Assert.assertThrows(NoSuchElementException.class,
+                () -> findElement(By.id("error-handler-message")));
+    }
+}

vaadin-grid-flow-parent/vaadin-grid-flow/src/main/java/com/vaadin/flow/component/grid/Grid.java

@@ -2924,6 +2924,8 @@ public void removeColumn(Column<T> column) {
         Objects.requireNonNull(column, "column should not be null");
 
         ensureOwner(column);
+        List<GridSortOrder<T>> order = new ArrayList<>();
+        setSortOrder(order, false);
         removeColumnAndColumnGroupsIfNeeded(column);
         column.destroyDataGenerators();
         keyToColumnMap.remove(column.getKey());