sort() handles nulls instead of throwing NPE - fixes #3152 (#3162)

* Add utility helpers to create array of comparable items

* Extend sort() to handle arrays with nulls

- Instead of NullPointerException on nulls, sort them last
- add JSON helpers to return Comparable[] in addition to Object[]
- Non-homogenous arrays or arrays with non-primitive
  objects (array or object) are not sortable
- Add tests for both new and old sort functionality

main/src/com/google/refine/expr/functions/arrays/Sort.java

@@ -35,6 +35,7 @@ package com.google.refine.expr.functions.arrays;
 
 import java.util.Arrays;
 import java.util.Collections;
+import java.util.Comparator;
 import java.util.List;
 import java.util.Properties;
 
@@ -47,29 +48,34 @@ import com.google.refine.util.JSONUtilities;
 public class Sort implements Function {
 
     @Override
-    @SuppressWarnings("unchecked")
+    @SuppressWarnings({ "unchecked", "rawtypes" })
     public Object call(Properties bindings, Object[] args) {
         if (args.length == 1) {
             Object v = args[0];
-            
             if (v != null) {
                 if (v.getClass().isArray()) {
                     Object[] a = (Object[]) v;
-                    Object[] r = a.clone();
+                    Comparable[] r = new Comparable[a.length];
-                    
+                    for (int i = 0; i < r.length; i++) {
-                    Arrays.sort(r, 0, r.length);
+                        if (a[i] instanceof Comparable) {
-                    
+                            r[i] = (Comparable) a[i];
+                        } else if (a[i] == null) {
+                            r[i] = null;
+                        } else {
+                            return new EvalError(ControlFunctionRegistry.getFunctionName(this) + " expects an array of uniform type");
+                        }
+                    }
+                    Arrays.sort(r, Comparator.nullsLast(Comparator.naturalOrder()));
                     return r;
                 } else if (v instanceof ArrayNode) {
-                    Object[] r = JSONUtilities.toArray((ArrayNode) v);
+                    // FIXME: Probably need a test for this
-                    
+                    // Comparable[] is a (slight) lie since nulls can be included, but our comparator will handle them
-                    Arrays.sort(r, 0, r.length);
+                    Comparable[] r = (Comparable[]) JSONUtilities.toSortableArray((ArrayNode) v);
-                    
+                    Arrays.sort(r, Comparator.nullsLast(Comparator.naturalOrder()));
                     return r;
                 } else if (v instanceof List<?>) {
                     List<? extends Comparable<Object>> a = (List<? extends Comparable<Object>>) v;
-                    Collections.sort(a);
+                    Collections.sort(a, Comparator.nullsLast(Comparator.naturalOrder()));
-                    
                     return a;
                 }
             }
@@ -84,7 +90,7 @@ public class Sort implements Function {
     
     @Override
     public String getParams() {
-        return "array a";
+        return "array a of uniform type";
     }
     
     @Override

main/src/com/google/refine/expr/util/JsonValueConverter.java

@@ -35,29 +35,55 @@ import com.fasterxml.jackson.databind.JsonNode;
  */
 public class JsonValueConverter {
 
-	public static Object convert(JsonNode value) {
+    public static Object convert(JsonNode value) {
-		if (value == null) {
+        if (value == null) {
-			return null;
+            return null;
-		}
+        }
-		if (value.isObject()) {
+        if (value.isObject()) {
-			return value;
+            return value;
-		} else if (value.isArray()) {
+        } else if (value.isArray()) {
-    		return value;
+            return value;
-    	} else if (value.isBigDecimal() || value.isDouble() || value.isFloat()) {
+        } else if (value.isBigDecimal() || value.isDouble() || value.isFloat()) {
-    		return value.asDouble();
+            return value.asDouble();
-    	} else if (value.isBigInteger()) {
+        } else if (value.isBigInteger()) {
-    		return value.asLong();
+            return value.asLong();
-    	} else if (value.isInt()) {
+        } else if (value.isInt()) {
-    		return value.asInt();
+            return value.asInt();
-    	} else if (value.isBinary() || value.isTextual()) {
+        } else if (value.isBinary() || value.isTextual()) {
-    		return value.asText();
+            return value.asText();
-    	} else if (value.isBoolean()) {
+        } else if (value.isBoolean()) {
-    		return value.asBoolean();
+            return value.asBoolean();
-    	} else if (value.isNull()) {
+        } else if (value.isNull()) {
-    		return null;
+            return null;
-    	} else {
+        } else {
-    		return null;
+            return null;
-    	}
+        }
-	}
+    }
+
+    @SuppressWarnings("rawtypes")
+    public static Comparable convertComparable(JsonNode value) {
+        if (value == null) {
+            return null;
+        }
+        if (value.isContainerNode()) {
+            // TODO: return null instead (like fallthrough case)
+            throw new IllegalArgumentException("Arrays and objects aren't comparable");
+        } else if (value.isBigDecimal() || value.isDouble() || value.isFloat()) {
+            return value.asDouble();
+        } else if (value.isBigInteger()) {
+            return value.asLong();
+        } else if (value.isInt()) {
+            return value.asInt();
+        } else if (value.isBinary() || value.isTextual()) {
+            return value.asText();
+        } else if (value.isBoolean()) {
+            return value.asBoolean();
+        } else if (value.isNull()) {
+            return null;
+        } else {
+            return null;
+        }
+    }
+
 
 }

main/src/com/google/refine/util/JSONUtilities.java

@@ -182,4 +182,17 @@ public class JSONUtilities {
 		}
 		return result;
 	}
+
+    @SuppressWarnings("rawtypes")
+    public static Comparable[] toSortableArray(ArrayNode v) {
+        if (v == null) {
+            return null;
+        }
+        Comparable[] result = new Comparable[v.size()];
+        for (int i = 0; i != v.size(); i++) {
+            result[i] = JsonValueConverter.convertComparable(v.get(i));
+        }
+        return result;
+    }
+
 }

main/tests/server/src/com/google/refine/RefineTest.java

@@ -350,6 +350,20 @@ public class RefineTest extends PowerMockTestCase {
         Assert.assertEquals(result.toString(), test[1], "Wrong result for expression: " + test[0]);
     }
 
+    /**
+     * Parse and evaluate a GREL expression and compare the result an expected
+     * type using instanceof
+     *
+     * @param bindings
+     * @param test
+     * @throws ParsingException
+     */
+    protected void parseEvalType(Properties bindings, String test, @SuppressWarnings("rawtypes") Class clazz)
+    throws ParsingException {
+        Evaluable eval = MetaParser.parse("grel:" + test);
+        Object result = eval.evaluate(bindings);
+        Assert.assertTrue(clazz.isInstance(result), "Wrong result type for expression: " + test);
+    }
 
     @AfterMethod
     public void TearDown() throws Exception {

main/tests/server/src/com/google/refine/expr/functions/arrays/SortTests.java

@@ -28,13 +28,51 @@ package com.google.refine.expr.functions.arrays;
 
 import org.testng.annotations.Test;
 
+import com.google.refine.RefineTest;
+import com.google.refine.expr.EvalError;
+import com.google.refine.expr.ParsingException;
 import com.google.refine.util.TestUtils;
 
-public class SortTests {
+public class SortTests extends RefineTest {
     @Test
     public void serializeSort() {
-        String json = "{\"description\":\"Sorts array a\",\"params\":\"array a\",\"returns\":\"array\"}";
+        String json = "{\"description\":\"Sorts array a\",\"params\":\"array a of uniform type\",\"returns\":\"array\"}";
         TestUtils.isSerializedTo(new Sort(), json);
     }
+
+    @Test
+    public void sortJsonArray() throws ParsingException {
+        String[] test = {"'[2,1,3]'.parseJson().sort().toString()", "[1, 2, 3]"};
+        parseEval(bindings, test);
+        String[] test1 = {"'[2,null,3]'.parseJson().sort().toString()", "[2, 3, null]"};
+        parseEval(bindings, test1);
+    }
+
+    @Test
+    public void sortArray() throws ParsingException {
+        String[] test = {"[2,1,3].sort().toString()", "[1, 2, 3]"};
+        parseEval(bindings, test);
+        String[] test1 = {"[2,null,3].sort().toString()", "[2, 3, null]"};
+        parseEval(bindings, test1);
+
+        String[] test2 = {"['z','b','c','a'].sort().toString()", "[a, b, c, z]"};
+        parseEval(bindings, test2);
+        String[] test3 = {"['z',null,'c','a'].sort().toString()", "[a, c, z, null]"};
+        parseEval(bindings, test3);
+
+        String[] test4 = {"[toDate(2020), '2018-03-02'.toDate()].sort().toString()", "[2018-03-02T00:00Z, 2020-01-01T00:00Z]"};
+        parseEval(bindings, test4);
+    }
+
+    public void sortMixedArray() throws ParsingException {
+        String test = "[2,1.0,3].sort().toString()";
+        parseEvalType(bindings, test, EvalError.class);
+        test = "[2,'a',3].sort().toString()";
+        parseEvalType(bindings, test, EvalError.class);
+        test = "'[2,\"a\",3]'.parseJson().sort().toString()";
+        parseEvalType(bindings, test, EvalError.class);
+
+    }
+
 }