Fix NPE when grid is sorted by column with null values

Fixes #8282

2 files changed, 180 insertions, 25 deletions

diff --git a/server/src/main/java/com/vaadin/ui/Grid.java b/server/src/main/java/com/vaadin/ui/Grid.java
index 9a09ab3e7a..93afdc2d24 100644
--- a/server/src/main/java/com/vaadin/ui/Grid.java
+++ b/server/src/main/java/com/vaadin/ui/Grid.java
@@ -798,7 +798,7 @@ public class Grid<T> extends AbstractListing<T> implements HasComponents,
          *            the type of value
          */
         protected Column(ValueProvider<T, ? extends V> valueProvider,
-                Renderer<V> renderer) {
+                         Renderer<V> renderer) {
             Objects.requireNonNull(valueProvider,
                     "Value provider can't be null");
             Objects.requireNonNull(renderer, "Renderer can't be null");
@@ -819,11 +819,7 @@ public class Grid<T> extends AbstractListing<T> implements HasComponents,
             Class<V> valueType = renderer.getPresentationType();
 
             if (Comparable.class.isAssignableFrom(valueType)) {
-                comparator = (a, b) -> {
-                    @SuppressWarnings("unchecked")
-                    Comparable<V> comp = (Comparable<V>) valueProvider.apply(a);
-                    return comp.compareTo(valueProvider.apply(b));
-                };
+                comparator = (a, b) -> compareComparables(valueProvider.apply(a), valueProvider.apply(b));
                 state.sortable = true;
             } else if (Number.class.isAssignableFrom(valueType)) {
                 /*
@@ -831,10 +827,8 @@ public class Grid<T> extends AbstractListing<T> implements HasComponents,
                  * Provide explicit comparison support in this case even though
                  * Number itself isn't Comparable.
                  */
-                comparator = (a, b) -> {
-                    return compareNumbers((Number) valueProvider.apply(a),
-                            (Number) valueProvider.apply(b));
-                };
+                comparator = (a, b) -> compareNumbers((Number) valueProvider.apply(a),
+                        (Number) valueProvider.apply(b));
                 state.sortable = true;
             } else {
                 state.sortable = false;
@@ -842,21 +836,26 @@ public class Grid<T> extends AbstractListing<T> implements HasComponents,
         }
 
         @SuppressWarnings("unchecked")
-        private static int compareNumbers(Number a, Number b) {
-            assert a.getClass() == b.getClass();
+        private static int compareComparables(Object a, Object b) {
+            return ((Comparator) Comparator.nullsLast(Comparator.naturalOrder())).compare(a, b);
+        }
 
+        @SuppressWarnings("unchecked")
+        private static int compareNumbers(Number a, Number b) {
+            Number valueA = a != null ? a : Double.POSITIVE_INFINITY;
+            Number valueB = b != null ? b : Double.POSITIVE_INFINITY;
             // Most Number implementations are Comparable
-            if (a instanceof Comparable && a.getClass().isInstance(b)) {
-                return ((Comparable<Number>) a).compareTo(b);
-            } else if (a.equals(b)) {
+            if (valueA instanceof Comparable && valueA.getClass().isInstance(valueB)) {
+                return ((Comparable<Number>) valueA).compareTo(valueB);
+            } else if (valueA.equals(valueB)) {
                 return 0;
             } else {
                 // Fall back to comparing based on potentially truncated values
-                int compare = Long.compare(a.longValue(), b.longValue());
+                int compare = Long.compare(valueA.longValue(), valueB.longValue());
                 if (compare == 0) {
                     // This might still produce 0 even though the values are not
                     // equals, but there's nothing more we can do about that
-                    compare = Double.compare(a.doubleValue(), b.doubleValue());
+                    compare = Double.compare(valueA.doubleValue(), valueB.doubleValue());
                 }
                 return compare;
             }
@@ -3259,14 +3258,7 @@ public class Grid<T> extends AbstractListing<T> implements HasComponents,
     private void sort(boolean userOriginated) {
         // Set sort orders
         // In-memory comparator
-        BinaryOperator<SerializableComparator<T>> operator = (comparator1,
-                comparator2) -> SerializableComparator
-                        .asInstance((Comparator<T> & Serializable) comparator1
-                                .thenComparing(comparator2));
-        SerializableComparator<T> comparator = sortOrder.stream().map(
-                order -> order.getSorted().getComparator(order.getDirection()))
-                .reduce((x, y) -> 0, operator);
-        getDataCommunicator().setInMemorySorting(comparator);
+        getDataCommunicator().setInMemorySorting(createSortingComparator());
 
         // Back-end sort properties
         List<QuerySortOrder> sortProperties = new ArrayList<>();
@@ -3283,4 +3275,20 @@ public class Grid<T> extends AbstractListing<T> implements HasComponents,
                 userOriginated));
     }
 
+    /**
+     * Creates a comparator for grid to sort rows.
+     *
+     * @return the comparator based on column sorting information.
+     */
+
+    protected SerializableComparator<T> createSortingComparator() {
+        BinaryOperator<SerializableComparator<T>> operator = (comparator1,
+                                                              comparator2) -> SerializableComparator
+                .asInstance((Comparator<T> & Serializable) comparator1
+                        .thenComparing(comparator2));
+        return sortOrder.stream().map(
+                order -> order.getSorted().getComparator(order.getDirection()))
+                .reduce((x, y) -> 0, operator);
+    }
+
 }
diff --git a/server/src/test/java/com/vaadin/tests/components/grid/GridNullValueSort.java b/server/src/test/java/com/vaadin/tests/components/grid/GridNullValueSort.java
new file mode 100644
index 0000000000..4a3a0315c5
--- /dev/null
+++ b/server/src/test/java/com/vaadin/tests/components/grid/GridNullValueSort.java
@@ -0,0 +1,147 @@
+package com.vaadin.tests.components.grid;
+
+import com.vaadin.server.SerializableComparator;
+import com.vaadin.server.VaadinSession;
+import com.vaadin.shared.data.sort.SortDirection;
+import com.vaadin.ui.Grid;
+import com.vaadin.ui.renderers.AbstractRenderer;
+import com.vaadin.ui.renderers.NumberRenderer;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Test;
+
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.List;
+
+public class GridNullValueSort {
+
+    private static AbstractRenderer<Integer, Boolean> booleanRenderer() {
+        return new AbstractRenderer<Integer, Boolean>(Boolean.class) {
+        };
+    }
+
+    private static class TestGrid extends Grid<Integer> {
+        @Override
+        public SerializableComparator<Integer> createSortingComparator() {
+            return super.createSortingComparator();
+        }
+    }
+
+    private TestGrid grid;
+
+    @Before
+    public void setup() {
+        VaadinSession.setCurrent(null);
+        grid = new TestGrid();
+        grid.addColumn(i -> i, new NumberRenderer()).setId("int").setSortable(true);
+        grid.addColumn(i -> i == null ? null : String.valueOf(i)).setId("String").setSortable(true);
+        grid.addColumn(i -> i == null ? null : i != 1, booleanRenderer()).setId("Boolean").setSortable(true);
+    }
+
+    @Test
+    public void testNumbersNotNulls() {
+        grid.sort(grid.getColumn("int"), SortDirection.ASCENDING);
+        performSort(
+                Arrays.asList(2, 1, 3),
+                Arrays.asList(1, 2, 3));
+    }
+
+    @Test
+    public void testNumbers() {
+        grid.sort(grid.getColumn("int"), SortDirection.ASCENDING);
+        performSort(
+                Arrays.asList(1, 2, null, 3, null, null),
+                Arrays.asList(1, 2, 3, null, null, null));
+    }
+
+    @Test
+    public void testNumbersNotNullsDescending() {
+        grid.sort(grid.getColumn("int"), SortDirection.DESCENDING);
+        performSort(
+                Arrays.asList(1, 2, 3),
+                Arrays.asList(3, 2, 1));
+    }
+
+    @Test
+    public void testNumbersDescending() {
+        grid.sort(grid.getColumn("int"), SortDirection.DESCENDING);
+        performSort(
+                Arrays.asList(1, 3, null, null, null, 2),
+                Arrays.asList(null, null, null, 3, 2, 1));
+    }
+
+    @Test
+    public void testStringsNotNulls() {
+        grid.sort(grid.getColumn("String"), SortDirection.ASCENDING);
+        performSort(
+                Arrays.asList(2, 1, 3),
+                Arrays.asList(1, 2, 3));
+    }
+
+    @Test
+    public void testStrings() {
+        grid.sort(grid.getColumn("String"), SortDirection.ASCENDING);
+        performSort(
+                Arrays.asList(1, 2, null, 3, null, null),
+                Arrays.asList(1, 2, 3, null, null, null));
+    }
+
+    @Test
+    public void testStringsNotNullsDescending() {
+        grid.sort(grid.getColumn("String"), SortDirection.DESCENDING);
+        performSort(
+                Arrays.asList(1, 2, 3),
+                Arrays.asList(3, 2, 1));
+    }
+
+    @Test
+    public void testStringsDescending() {
+        grid.sort(grid.getColumn("String"), SortDirection.DESCENDING);
+        performSort(
+                Arrays.asList(1, 3, null, null, null, 2),
+                Arrays.asList(null, null, null, 3, 2, 1));
+    }
+
+
+    @Test
+    public void testBooleansNotNulls() {
+        grid.sort(grid.getColumn("Boolean"), SortDirection.ASCENDING);
+        performSort(
+                Arrays.asList(2, 1),
+                Arrays.asList(1, 2));
+    }
+
+    @Test
+    public void testBooleans() {
+        grid.sort(grid.getColumn("Boolean"), SortDirection.ASCENDING);
+        performSort(
+                Arrays.asList(1, null, 2, null, null),
+                Arrays.asList(1, 2, null, null, null));
+    }
+
+    @Test
+    public void testBooleansNotNullsDescending() {
+        grid.sort(grid.getColumn("Boolean"), SortDirection.DESCENDING);
+        performSort(
+                Arrays.asList(1, 2),
+                Arrays.asList(2, 1));
+    }
+
+    @Test
+    public void testBooleansDescending() {
+        grid.sort(grid.getColumn("Boolean"), SortDirection.DESCENDING);
+        performSort(
+                Arrays.asList(1, null, null, null, 2),
+                Arrays.asList(null, null, null, 2, 1));
+    }
+
+
+    private void performSort(List<Integer> source, List<Integer> expected) {
+        SerializableComparator<Integer> sortingComparator = grid.createSortingComparator();
+        List<Integer> data = new ArrayList<>(source);
+        data.sort(sortingComparator);
+        Assert.assertEquals(expected, data);
+    }
+
+}