Repository: jsonata-js/jsonata (js)

Issue:
Range operator allows invalid expressions
The range operator (`..`) expects both operands to be integers. It usually throws an error if this isn't true. However in some cases the type checking is bypassed (meaning that invalid expressions are evaluated without error). Two such cases are described below. The first definitely seems like a bug. The second looks like a bug but it's possible that this is the intended behaviour.

#### Case 1. When lhs > rhs

The range operator returns `undefined` if the left side is greater than the right side. This means that expressions like `[10..1]` evaluate to an empty array. But because this check comes *before* the type validation, any types that satisfy the JavaScript condition `lhs > rhs` trigger an `undefined` result and never get validated.

The following expressions are all invalid yet they all evaluate to `[]` instead of throwing a type error.

```
[10..1.5]

[true..false]

["dogs".."cats"]

[1..""]

[1..[]]

["a"..{}]

[1..false]

[2..true]
```

If you reverse the operands in any of these expressions, the condition `lhs > rhs` is no longer true and you get a type error as expected.

#### Case 2. Undefined inputs

The range operator returns `undefined` if either of its inputs evaluate to `undefined`. But again, this test happens before the types have been validated. So an undefined input can override a type error on the other input, e.g.

```
[0..true]   // error: right side is not an integer
[$x..true]  // returns []

[false..10]  // error: left side is not an integer
[false..$x]  // returns []
```

It's odd that the test for undefined inputs takes precedence over type checking. I would expect to be notified of the type violation even if the other input is undefined. After all, no value of `$x` makes those expressions valid.

The range operator isn't the only example of this. Numeric operators behave in the same way:

```
false + 1   // error: left side is not a number
false + $x  // returns undefined
```

And so do the comparison operators that validate types:

```
false > 1   // error: operands must be numbers or strings
false > $x  // returns false
```
It *doesn't* happen with function calls though:

```
$substringBefore("hello", 3)  // error: argument 2 does not match signature
$substringBefore($x, 3)       // same error
```

Pull request:
stricter type checking of numeric operands
Perform type checking on numeric operators before the 'undefined test' is done.  More consistent with function type checking.

resolves #242 

Patch:
diff --git a/src/jsonata.js b/src/jsonata.js
index 31c1d4d..126bb3b 100644
--- a/src/jsonata.js
+++ b/src/jsonata.js
@@ -332,6 +332,8 @@ var jsonata = (function() {
                     break;
                 case '=':
                 case '!=':
+                    result = evaluateEqualityExpression(lhs, rhs, op);
+                    break
                 case '<':
                 case '<=':
                 case '>':
@@ -535,19 +537,14 @@ var jsonata = (function() {
     function evaluateNumericExpression(lhs, rhs, op) {
         var result;
 
-        if (typeof lhs === 'undefined' || typeof rhs === 'undefined') {
-            // if either side is undefined, the result is undefined
-            return result;
-        }
-
-        if (!isNumeric(lhs)) {
+        if (typeof lhs !== 'undefined' && !isNumeric(lhs)) {
             throw {
                 code: "T2001",
                 stack: (new Error()).stack,
                 value: lhs
             };
         }
-        if (!isNumeric(rhs)) {
+        if (typeof rhs !== 'undefined' && !isNumeric(rhs)) {
             throw {
                 code: "T2002",
                 stack: (new Error()).stack,
@@ -555,6 +552,11 @@ var jsonata = (function() {
             };
         }
 
+        if (typeof lhs === 'undefined' || typeof rhs === 'undefined') {
+            // if either side is undefined, the result is undefined
+            return result;
+        }
+
         switch (op) {
             case '+':
                 result = lhs + rhs;
@@ -576,13 +578,13 @@ var jsonata = (function() {
     }
 
     /**
-     * Evaluate comparison expression against input data
+     * Evaluate equality expression against input data
      * @param {Object} lhs - LHS value
      * @param {Object} rhs - RHS value
      * @param {Object} op - opcode
      * @returns {*} Result
      */
-    function evaluateComparisonExpression(lhs, rhs, op) {
+    function evaluateEqualityExpression(lhs, rhs, op) {
         var result;
 
         // type checks
@@ -594,27 +596,6 @@ var jsonata = (function() {
             return false;
         }
 
-        var validate = function() {
-            // if aa or bb are not string or numeric values, then throw an error
-            if (!(ltype === 'string' || ltype === 'number') || !(rtype === 'string' || rtype === 'number')) {
-                throw {
-                    code: "T2010",
-                    stack: (new Error()).stack,
-                    value: !(ltype === 'string' || ltype === 'number') ? lhs : rhs
-                };
-            }
-
-            //if aa and bb are not of the same type
-            if (ltype !== rtype) {
-                throw {
-                    code: "T2009",
-                    stack: (new Error()).stack,
-                    value: lhs,
-                    value2: rhs
-                };
-            }
-        };
-
         switch (op) {
             case '=':
                 result = lhs === rhs;
@@ -622,20 +603,62 @@ var jsonata = (function() {
             case '!=':
                 result = (lhs !== rhs);
                 break;
+        }
+        return result;
+    }
+
+    /**
+     * Evaluate comparison expression against input data
+     * @param {Object} lhs - LHS value
+     * @param {Object} rhs - RHS value
+     * @param {Object} op - opcode
+     * @returns {*} Result
+     */
+    function evaluateComparisonExpression(lhs, rhs, op) {
+        var result;
+
+        // type checks
+        var ltype = typeof lhs;
+        var rtype = typeof rhs;
+
+        var lcomparable = (ltype === 'undefined' || ltype === 'string' || ltype === 'number');
+        var rcomparable = (rtype === 'undefined' || rtype === 'string' || rtype === 'number');
+
+        // if either aa or bb are not comparable (string or numeric) values, then throw an error
+        if (!lcomparable || !rcomparable) {
+            throw {
+                code: "T2010",
+                stack: (new Error()).stack,
+                value: !(ltype === 'string' || ltype === 'number') ? lhs : rhs
+            };
+        }
+
+        // if either side is undefined, the result is undefined
+        if (ltype === 'undefined' || rtype === 'undefined') {
+            return undefined;
+        }
+
+        //if aa and bb are not of the same type
+        if (ltype !== rtype) {
+            throw {
+                code: "T2009",
+                stack: (new Error()).stack,
+                value: lhs,
+                value2: rhs
+            };
+        }
+
+        switch (op) {
             case '<':
-                validate();
                 result = lhs < rhs;
                 break;
             case '<=':
-                validate();
                 result = lhs <= rhs;
                 break;
             case '>':
-                validate();
                 result = lhs > rhs;
                 break;
             case '>=':
-                validate();
                 result = lhs >= rhs;
                 break;
         }
@@ -797,24 +820,14 @@ var jsonata = (function() {
     function evaluateRangeExpression(lhs, rhs) {
         var result;
 
-        if (typeof lhs === 'undefined' || typeof rhs === 'undefined') {
-            // if either side is undefined, the result is undefined
-            return result;
-        }
-
-        if (lhs > rhs) {
-            // if the lhs is greater than the rhs, return undefined
-            return result;
-        }
-
-        if (!Number.isInteger(lhs)) {
+        if (typeof lhs !== 'undefined' && !Number.isInteger(lhs)) {
             throw {
                 code: "T2003",
                 stack: (new Error()).stack,
                 value: lhs
             };
         }
-        if (!Number.isInteger(rhs)) {
+        if (typeof rhs !== 'undefined' && !Number.isInteger(rhs)) {
             throw {
                 code: "T2004",
                 stack: (new Error()).stack,
@@ -822,6 +835,16 @@ var jsonata = (function() {
             };
         }
 
+        if (typeof lhs === 'undefined' || typeof rhs === 'undefined') {
+            // if either side is undefined, the result is undefined
+            return result;
+        }
+
+        if (lhs > rhs) {
+            // if the lhs is greater than the rhs, return undefined
+            return result;
+        }
+
         result = new Array(rhs - lhs + 1);
         for (var item = lhs, index = 0; item <= rhs; item++, index++) {
             result[index] = item;


Test patch:
diff --git a/test/test-suite/groups/comparison-operators/case025.json b/test/test-suite/groups/comparison-operators/case025.json
new file mode 100644
index 0000000..8cf6150
--- /dev/null
+++ b/test/test-suite/groups/comparison-operators/case025.json
@@ -0,0 +1,5 @@
+{
+    "expr": "false > 1",
+    "data": {},
+    "code": "T2010"
+}
diff --git a/test/test-suite/groups/comparison-operators/case026.json b/test/test-suite/groups/comparison-operators/case026.json
new file mode 100644
index 0000000..64db7ae
--- /dev/null
+++ b/test/test-suite/groups/comparison-operators/case026.json
@@ -0,0 +1,5 @@
+{
+    "expr": "false > $x",
+    "data": {},
+    "code": "T2010"
+}
diff --git a/test/test-suite/groups/comparison-operators/case027.json b/test/test-suite/groups/comparison-operators/case027.json
new file mode 100644
index 0000000..b4d526c
--- /dev/null
+++ b/test/test-suite/groups/comparison-operators/case027.json
@@ -0,0 +1,5 @@
+{
+    "expr": "3 > $x",
+    "data": {},
+    "undefinedResult": true
+}
diff --git a/test/test-suite/groups/comparison-operators/case028.json b/test/test-suite/groups/comparison-operators/case028.json
new file mode 100644
index 0000000..194cae7
--- /dev/null
+++ b/test/test-suite/groups/comparison-operators/case028.json
@@ -0,0 +1,5 @@
+{
+    "expr": "$x <= 'hello'",
+    "data": {},
+    "undefinedResult": true
+}
diff --git a/test/test-suite/groups/numeric-operators/case017.json b/test/test-suite/groups/numeric-operators/case017.json
new file mode 100644
index 0000000..94dc7ef
--- /dev/null
+++ b/test/test-suite/groups/numeric-operators/case017.json
@@ -0,0 +1,5 @@
+{
+    "expr": "false + 1",
+    "data": {},
+    "code": "T2001"
+}
diff --git a/test/test-suite/groups/numeric-operators/case018.json b/test/test-suite/groups/numeric-operators/case018.json
new file mode 100644
index 0000000..b88402c
--- /dev/null
+++ b/test/test-suite/groups/numeric-operators/case018.json
@@ -0,0 +1,5 @@
+{
+    "expr": "false + $x",
+    "data": {},
+    "code": "T2001"
+}
diff --git a/test/test-suite/groups/range-operator/case011.json b/test/test-suite/groups/range-operator/case011.json
new file mode 100644
index 0000000..950f50f
--- /dev/null
+++ b/test/test-suite/groups/range-operator/case011.json
@@ -0,0 +1,6 @@
+{
+    "expr": "[10..1.5]",
+    "dataset": null,
+    "bindings": {},
+    "code": "T2004"
+}
diff --git a/test/test-suite/groups/range-operator/case012.json b/test/test-suite/groups/range-operator/case012.json
new file mode 100644
index 0000000..091b6a3
--- /dev/null
+++ b/test/test-suite/groups/range-operator/case012.json
@@ -0,0 +1,6 @@
+{
+    "expr": "[true..false]",
+    "dataset": null,
+    "bindings": {},
+    "code": "T2003"
+}
diff --git a/test/test-suite/groups/range-operator/case013.json b/test/test-suite/groups/range-operator/case013.json
new file mode 100644
index 0000000..225761d
--- /dev/null
+++ b/test/test-suite/groups/range-operator/case013.json
@@ -0,0 +1,6 @@
+{
+    "expr": "['dogs'..'cats']",
+    "dataset": null,
+    "bindings": {},
+    "code": "T2003"
+}
diff --git a/test/test-suite/groups/range-operator/case014.json b/test/test-suite/groups/range-operator/case014.json
new file mode 100644
index 0000000..02eda91
--- /dev/null
+++ b/test/test-suite/groups/range-operator/case014.json
@@ -0,0 +1,6 @@
+{
+    "expr": "[1..'']",
+    "dataset": null,
+    "bindings": {},
+    "code": "T2004"
+}
diff --git a/test/test-suite/groups/range-operator/case015.json b/test/test-suite/groups/range-operator/case015.json
new file mode 100644
index 0000000..b1bd786
--- /dev/null
+++ b/test/test-suite/groups/range-operator/case015.json
@@ -0,0 +1,6 @@
+{
+    "expr": "[1..[]]",
+    "dataset": null,
+    "bindings": {},
+    "code": "T2004"
+}
diff --git a/test/test-suite/groups/range-operator/case016.json b/test/test-suite/groups/range-operator/case016.json
new file mode 100644
index 0000000..f53de23
--- /dev/null
+++ b/test/test-suite/groups/range-operator/case016.json
@@ -0,0 +1,6 @@
+{
+    "expr": "[1..{}]",
+    "dataset": null,
+    "bindings": {},
+    "code": "T2004"
+}
diff --git a/test/test-suite/groups/range-operator/case017.json b/test/test-suite/groups/range-operator/case017.json
new file mode 100644
index 0000000..ff9cd86
--- /dev/null
+++ b/test/test-suite/groups/range-operator/case017.json
@@ -0,0 +1,6 @@
+{
+    "expr": "[1..false]",
+    "dataset": null,
+    "bindings": {},
+    "code": "T2004"
+}
diff --git a/test/test-suite/groups/range-operator/case018.json b/test/test-suite/groups/range-operator/case018.json
new file mode 100644
index 0000000..8957e63
--- /dev/null
+++ b/test/test-suite/groups/range-operator/case018.json
@@ -0,0 +1,6 @@
+{
+    "expr": "[2..true]",
+    "dataset": null,
+    "bindings": {},
+    "code": "T2004"
+}
diff --git a/test/test-suite/groups/range-operator/case019.json b/test/test-suite/groups/range-operator/case019.json
new file mode 100644
index 0000000..5a15beb
--- /dev/null
+++ b/test/test-suite/groups/range-operator/case019.json
@@ -0,0 +1,6 @@
+{
+    "expr": "[$x..true]",
+    "dataset": null,
+    "bindings": {},
+    "code": "T2004"
+}
diff --git a/test/test-suite/groups/range-operator/case020.json b/test/test-suite/groups/range-operator/case020.json
new file mode 100644
index 0000000..61e8c63
--- /dev/null
+++ b/test/test-suite/groups/range-operator/case020.json
@@ -0,0 +1,6 @@
+{
+    "expr": "[false..$x]",
+    "dataset": null,
+    "bindings": {},
+    "code": "T2003"
+}