Make is_json_scalar throw on invalid input

is_json_scalar returns null on invalid input. However, this can be too lenient. Make it more strict for now and we can consider relax it later if necessary.
trinodb · Feb 28, 2018 · a8b88c9 · a8b88c9
1 parent 362b3c4
commit a8b88c9
Show file tree

Hide file tree

Showing 2 changed files with 17 additions and 11 deletions.
diff --git a/presto-main/src/main/java/com/facebook/presto/operator/scalar/JsonFunctions.java b/presto-main/src/main/java/com/facebook/presto/operator/scalar/JsonFunctions.java
@@ -46,6 +46,7 @@
 import static com.facebook.presto.spi.StandardErrorCode.INVALID_FUNCTION_ARGUMENT;
 import static com.facebook.presto.spi.type.Chars.padSpaces;
 import static com.facebook.presto.util.JsonUtil.createJsonParser;
+import static com.facebook.presto.util.JsonUtil.truncateIfNecessaryForErrorMessage;
 import static com.fasterxml.jackson.core.JsonFactory.Feature.CANONICALIZE_FIELD_NAMES;
 import static com.fasterxml.jackson.core.JsonParser.NumberType;
 import static com.fasterxml.jackson.core.JsonToken.END_ARRAY;
@@ -88,38 +89,41 @@ public static JsonPath castCharToJsonPath(@LiteralParameter("x") Long charLength
         return new JsonPath(padSpaces(pattern, charLength.intValue()).toStringUtf8());
     }
 
-    @SqlNullable
     @ScalarFunction("is_json_scalar")
     @LiteralParameters("x")
     @SqlType(StandardTypes.BOOLEAN)
-    public static Boolean varcharIsJsonScalar(@SqlType("varchar(x)") Slice json)
+    public static boolean varcharIsJsonScalar(@SqlType("varchar(x)") Slice json)
     {
         return isJsonScalar(json);
     }
 
-    @SqlNullable
     @ScalarFunction
     @SqlType(StandardTypes.BOOLEAN)
-    public static Boolean isJsonScalar(@SqlType(StandardTypes.JSON) Slice json)
+    public static boolean isJsonScalar(@SqlType(StandardTypes.JSON) Slice json)
     {
         try (JsonParser parser = createJsonParser(JSON_FACTORY, json)) {
             JsonToken nextToken = parser.nextToken();
             if (nextToken == null) {
-                return null;
+                throw new PrestoException(INVALID_FUNCTION_ARGUMENT, "Invalid JSON value: " + truncateIfNecessaryForErrorMessage(json));
             }
 
             if (nextToken == START_ARRAY || nextToken == START_OBJECT) {
                 parser.skipChildren();
                 if (parser.nextToken() != null) {
-                    // Invalid JSON: trailing tokens
-                    return null;
+                    // extra trailing token after json array/object
+                    throw new PrestoException(INVALID_FUNCTION_ARGUMENT, "Invalid JSON value: " + truncateIfNecessaryForErrorMessage(json));
                 }
                 return false;
             }
-            return parser.nextToken() == null ? true : null;
+
+            if (parser.nextToken() != null) {
+                // extra trailing token after json scalar
+                throw new PrestoException(INVALID_FUNCTION_ARGUMENT, "Invalid JSON value: " + truncateIfNecessaryForErrorMessage(json));
+            }
+            return true;
         }
         catch (IOException e) {
-            return null;
+            throw new PrestoException(INVALID_FUNCTION_ARGUMENT, "Invalid JSON value: " + truncateIfNecessaryForErrorMessage(json));
         }
     }
 

diff --git a/presto-main/src/test/java/com/facebook/presto/operator/scalar/TestJsonFunctions.java b/presto-main/src/test/java/com/facebook/presto/operator/scalar/TestJsonFunctions.java
@@ -45,8 +45,10 @@ public void testIsJsonScalar()
         assertFunction("IS_JSON_SCALAR('[1, 2, 3]')", BOOLEAN, false);
         assertFunction("IS_JSON_SCALAR('{\"a\": 1, \"b\": 2}')", BOOLEAN, false);
 
-        assertFunction("IS_JSON_SCALAR('')", BOOLEAN, null);
-        assertFunction("IS_JSON_SCALAR('[1')", BOOLEAN, null);
+        assertInvalidFunction("IS_JSON_SCALAR('')", INVALID_FUNCTION_ARGUMENT, "Invalid JSON value: ");
+        assertInvalidFunction("IS_JSON_SCALAR('[1')", INVALID_FUNCTION_ARGUMENT, "Invalid JSON value: [1");
+        assertInvalidFunction("IS_JSON_SCALAR('1 trailing')", INVALID_FUNCTION_ARGUMENT, "Invalid JSON value: 1 trailing");
+        assertInvalidFunction("IS_JSON_SCALAR('[1, 2] trailing')", INVALID_FUNCTION_ARGUMENT, "Invalid JSON value: [1, 2] trailing");
     }
 
     @Test