Better handling of negation in filterToMongoQuery

Similar to SQL, negating a null should still be null.

lib/local-cache.ts

@@ -22,7 +22,7 @@ import * as crypto from "crypto";
 import * as config from "./config";
 import * as db from "./db";
 import * as cache from "./cache";
-import { mongoQueryToFilter } from "./mongodb-functions";
+import { convertOldPrecondition } from "./mongodb-functions";
 import * as logger from "./logger";
 import * as scheduling from "./scheduling";
 import * as expression from "./common/expression";
@@ -173,7 +173,9 @@ async function fetchPresets(): Promise<Preset[]> {
       try {
         precondition = parse(preset["precondition"]);
       } catch (error) {
-        precondition = mongoQueryToFilter(JSON.parse(preset["precondition"]));
+        precondition = convertOldPrecondition(
+          JSON.parse(preset["precondition"])
+        );
       }
 
       // Simplify expression

lib/mongodb-functions.ts

@@ -19,7 +19,7 @@
 
 import { ObjectID } from "mongodb";
 import { map, parse, stringify, parseList } from "./common/expression-parser";
-import { likePatternToRegExp, evaluate } from "./common/expression";
+import { likePatternToRegExp } from "./common/expression";
 import { Expression, Fault, Task } from "./types";
 
 const isArray = Array.isArray;
@@ -44,12 +44,10 @@ export function processDeviceFilter(filter: Expression): Expression {
       exp[1][1].startsWith("Tags.")
     ) {
       const t = exp[1][1].slice(5);
-      if (exp[0] === "IS NULL") return ["<>", ["PARAM", "_tags"], t];
+      if (exp[0] === "IS NULL") return ["_tags", t, false];
-      else if (exp[0] === "IS NOT NULL") return ["=", ["PARAM", "_tags"], t];
+      else if (exp[0] === "IS NOT NULL") return ["_tags", t, true];
-      else if (exp[0] === "=" && exp[2] === true)
+      else if (exp[0] === "=" && exp[2] === true) return ["_tags", t, true];
-        return ["=", ["PARAM", "_tags"], t];
+      else if (exp[0] === "<>" && exp[2] !== true) return ["_tags", t, true];
-      else if (exp[0] === "<>" && exp[2] !== true)
-        return ["=", ["PARAM", "_tags"], t];
     } else if (
       [
         "=",
@@ -117,140 +115,131 @@ export function processFaultsFilter(filter: Expression): Expression {
 }
 
 export function filterToMongoQuery(exp: Expression): Record<string, unknown> {
-  const ops = {
+  function recursive(filter, negate): Record<string, unknown> {
-    OR: 0,
-    AND: 0,
-    NOT: 0,
-    "=": 1,
-    "<>": 1,
-    ">": 1,
-    ">=": 1,
-    "<": 1,
-    "<=": 1,
-    LIKE: 1,
-    "NOT LIKE": 1,
-    "IS NULL": 1,
-    "IS NOT NULL": 1,
-  };
-
-  function recursive(filter, negate, res = {}): Record<string, unknown> {
     const op = filter[0];
 
-    if (ops[op] === 0) {
+    if (op === "AND") {
-      if (op === "AND") filter = filter.filter((f) => f !== true);
+      filter = filter.filter((f) => f !== true);
-      if (op === "OR") filter = filter.filter((f) => f !== false);
+      if (filter.length === 2) return recursive(filter[1], negate);
-      if (filter.length === 2) return recursive(filter[1], negate, res);
-
-      for (let i = 1; i < filter.length; ++i) {
-        if (!isArray(filter[i]) || ops[filter[i][0]] == null)
-          throw new Error(`Invalid expression in ${op} clause`);
     }
 
-      if ((!negate && op === "AND") || (negate && op === "OR")) {
+    if (op === "OR") {
-        res["$and"] = res["$and"] || [];
+      filter = filter.filter((f) => f !== false);
-        for (let i = 1; i < filter.length; ++i)
+      if (filter.length === 2) return recursive(filter[1], negate);
-          res["$and"].push(recursive(filter[i], negate));
-      } else if ((!negate && op === "OR") || (negate && op === "AND")) {
-        res["$or"] = res["$or"] || [];
-
-        for (let i = 1; i < filter.length; ++i)
-          res["$or"].push(recursive(filter[i], negate));
-      } else if (op === "NOT") {
-        recursive(filter[1], !negate, res);
-      }
-    } else if (ops[op] === 1) {
-      if (isArray(filter[2])) {
-        throw new Error(`Right operand of ${op} clause is not a primitive`);
-      } else if (
-        ["LIKE", "NOT LIKE"].includes(op) &&
-        isArray(filter[1]) &&
-        filter[1][0] === "FUNC" &&
-        ["UPPER", "LOWER"].includes(filter[1][1])
-      ) {
-        if (
-          (filter[1][1] === "UPPER" && filter[2] !== filter[2].toUpperCase()) ||
-          (filter[1][1] === "LOWER" && filter[2] !== filter[2].toLowerCase())
-        ) {
-          throw new Error(
-            `Cannot compare ${
-              filter[1][1]
-            }() against non-${filter[1][1].toLowerCase()}case pattern`
-          );
-        }
-      } else if (!isArray(filter[1]) || filter[1][0] !== "PARAM") {
-        throw new Error(`Left operand of ${op} clause is not a parameter`);
     }
 
-      if (op === "=") {
+    if ((!negate && op === "AND") || (negate && op === "OR"))
-        const param = filter[1][1];
+      return { $and: filter.slice(1).map((f) => recursive(f, negate)) };
-        const p = (res[param] = res[param] || {});
+
-        if (negate) p["$ne"] = filter[2];
+    if ((!negate && op === "OR") || (negate && op === "AND"))
-        else p["$eq"] = filter[2];
+      return { $or: filter.slice(1).map((f) => recursive(f, negate)) };
-      } else if (op === "<>") {
+
-        const param = filter[1][1];
+    if (op === "NOT") return recursive(filter[1], !negate);
-        let p = (res[param] = res[param] || {});
+
-        if (negate) p = p["$not"] = p["$not"] || {};
+    if (isArray(filter[2]))
-        p["$ne"] = filter[2];
+      throw new Error(`Invalid RHS operand of ${op} clause`);
-        if (param !== "_tags") p["$exists"] = true;
+
-      } else if (op === ">") {
+    if (op === "LIKE" || op === "NOT LIKE") {
-        const param = filter[1][1];
-        let p = (res[param] = res[param] || {});
-        if (negate) p = p["$not"] = p["$not"] || {};
-        p["$gt"] = filter[2];
-      } else if (op === ">=") {
-        const param = filter[1][1];
-        let p = (res[param] = res[param] || {});
-        if (negate) p = p["$not"] = p["$not"] || {};
-        p["$gte"] = filter[2];
-      } else if (op === "<") {
-        const param = filter[1][1];
-        let p = (res[param] = res[param] || {});
-        if (negate) p = p["$not"] = p["$not"] || {};
-        p["$lt"] = filter[2];
-      } else if (op === "<=") {
-        const param = filter[1][1];
-        let p = (res[param] = res[param] || {});
-        if (negate) p = p["$not"] = p["$not"] || {};
-        p["$lte"] = filter[2];
-      } else if (op === "IS NULL") {
-        const param = filter[1][1];
-        res[param] = { $exists: negate };
-      } else if (op === "IS NOT NULL") {
-        const param = filter[1][1];
-        res[param] = { $exists: !negate };
-      } else {
       if (op === "NOT LIKE") negate = !negate;
       let param;
       let flags;
+      if (!Array.isArray(filter[1]))
+        throw new Error(`Invalid LHS operand of ${op} clause`);
       if (filter[1][0] === "FUNC" && filter[1][1] === "UPPER") {
+        if (filter[2] !== filter[2].toUpperCase())
+          throw new Error(`Invalid RHS operand of ${op} clause`);
         param = filter[1][2][1];
         flags = "i";
       } else if (filter[1][0] === "FUNC" && filter[1][1] === "LOWER") {
+        if (filter[2] !== filter[2].toLowerCase())
+          throw new Error(`Invalid RHS operand of ${op} clause`);
         param = filter[1][2][1];
         flags = "i";
-        } else {
+      } else if (filter[1][0] === "PARAM") {
         param = filter[1][1];
         flags = "";
+      } else {
+        throw new Error(`Invalid LHS operand of ${op} clause`);
       }
       const r = likePatternToRegExp(filter[2], filter[3], flags);
-        if (negate) res[param] = { $not: r };
+      if (negate) return { [param]: { $nin: [r, null] } };
-        else res[param] = r;
+      else return { [param]: r };
     }
-    } else {
+
+    if (op === "_tags") {
+      let t = filter[2];
+      if (negate) t = !t;
+      if (t) return { _tags: filter[1] };
+      else return { _tags: { $ne: filter[1] } };
+    }
+
+    if (!isArray(filter[1]) || filter[1][0] !== "PARAM")
+      throw new Error(`Invalid LHS operand of ${op} clause`);
+
+    if (op === "IS NULL") {
+      const p = filter[1][1];
+      return { [p]: null };
+    }
+
+    if (op === "IS NOT NULL") {
+      const p = filter[1][1];
+      return { [p]: { $ne: null } };
+    }
+
+    if (isArray(filter[2]) || filter[2] == null)
+      throw new Error(`Invalid RHS operand of ${op} clause`);
+
+    if (op === "=") {
+      const p = filter[1][1];
+      const v = filter[2];
+      if (!negate) return { [p]: v };
+      else return { [p]: { $nin: [v, null] } };
+    }
+
+    if (op === "<>") {
+      const p = filter[1][1];
+      const v = filter[2];
+      if (negate) return { [p]: v };
+      else return { [p]: { $nin: [v, null] } };
+    }
+
+    if (op === ">") {
+      const p = filter[1][1];
+      const v = filter[2];
+      if (negate) return { [p]: { $lte: v } };
+      else return { [p]: { $gt: v } };
+    }
+
+    if (op === ">=") {
+      const p = filter[1][1];
+      const v = filter[2];
+      if (negate) return { [p]: { $lt: v } };
+      else return { [p]: { $gte: v } };
+    }
+
+    if (op === "<") {
+      const p = filter[1][1];
+      const v = filter[2];
+      if (negate) return { [p]: { $gte: v } };
+      else return { [p]: { $lt: v } };
+    }
+
+    if (op === "<=") {
+      const p = filter[1][1];
+      const v = filter[2];
+      if (negate) return { [p]: { $gt: v } };
+      else return { [p]: { $lte: v } };
+    }
+
     throw new Error(`Unrecognized operator ${op}`);
   }
 
-    return res;
+  if (!isArray(exp)) {
-  }
+    if (exp === true) return {};
-
-  const _exp = evaluate(exp, null, Date.now());
-
-  if (!isArray(_exp)) {
-    if (_exp === true) return {};
     throw new Error("Primitives are not valid queries");
   }
 
-  return recursive(_exp, false);
+  return recursive(exp, false);
 }
 
 export function processDeviceProjection(
@@ -495,7 +484,9 @@ export function flattenTask(task: unknown): Task {
   return t;
 }
 
-export function mongoQueryToFilter(query: Record<string, unknown>): Expression {
+export function convertOldPrecondition(
+  query: Record<string, unknown>
+): Expression {
   function recursive(_query): Expression {
     const expressions: Expression[] = [];
     for (const [k, v] of Object.entries(_query)) {
@@ -514,12 +505,22 @@ export function mongoQueryToFilter(query: Record<string, unknown>): Expression {
       } else if (k === "_tags") {
         if (typeof v === "object") {
           if (isArray(v)) throw new Error(`Invalid type`);
-
+          const conjs: Expression[] = [];
-          if (v.hasOwnProperty("$ne"))
+          for (const [op, val] of Object.entries(v)) {
-            expressions.push(["IS NULL", ["PARAM", `Tags.${v["$ne"]}`]]);
+            if (op === "$ne") {
-          else if (v.hasOwnProperty("$eq"))
+              if (typeof v["$ne"] !== "string")
-            expressions.push(["IS NOT NULL", ["PARAM", `Tags.${v["$eq"]}`]]);
+                throw new Error("Only string values are allowed for _tags");
-          else throw new Error(`Invalid tag query`);
+              conjs.push(["IS NULL", ["PARAM", `Tags.${val}`]]);
+            } else if (op === "$eq") {
+              if (typeof v["$eq"] !== "string")
+                throw new Error("Only string values are allowed for _tags");
+              conjs.push(["IS NOT NULL", ["PARAM", `Tags.${val}`]]);
+            } else {
+              throw new Error(`Invalid tag query`);
+            }
+          }
+          if (conjs.length === 1) expressions.push(conjs[0]);
+          else if (conjs.length > 1) expressions.push(["AND", ...conjs]);
         } else {
           expressions.push(["IS NOT NULL", ["PARAM", `Tags.${v}`]]);
         }
@@ -537,34 +538,31 @@ export function mongoQueryToFilter(query: Record<string, unknown>): Expression {
 
         const exps: Expression[] = [];
         for (const [kk, vv] of Object.entries(v)) {
-          let op;
+          if (kk === "$eq") {
-          switch (kk) {
+            exps.push(["=", ["PARAM", k], vv]);
-            case "$eq":
+          } else if (kk === "$ne") {
-              op = "=";
+            exps.push([
-              break;
+              "OR",
-            case "$ne":
+              ["<>", ["PARAM", k], vv],
-              op = "<>";
+              ["IS NULL", ["PARAM", k]],
-              break;
+            ]);
-            case "$lt":
+          } else if (kk === "$lt") {
-              op = "<";
+            exps.push(["<", ["PARAM", k], vv]);
-              break;
+          } else if (kk === "$lte") {
-            case "$lte":
+            exps.push(["<=", ["PARAM", k], vv]);
-              op = "<=";
+          } else if (kk === "$gt") {
-              break;
+            exps.push([">", ["PARAM", k], vv]);
-            case "$gt":
+          } else if (kk === "$gte") {
-              op = ">";
+            exps.push([">=", ["PARAM", k], vv]);
-              break;
+          } else {
-            case "$gte":
-              op = ">=";
-              break;
-            default:
             throw new Error(`Operator ${kk} not supported`);
           }
-          exps.push([op, ["PARAM", k], vv]);
+          if (!["string", "number", "boolean"].includes(typeof vv))
+            throw new Error(`Invalid value for ${kk} operator`);
         }
         if (exps.length === 1) {
           expressions.push(exps[0]);
-        } else {
+        } else if (exps.length > 1) {
           const and: Expression = ["AND"];
           expressions.push(and.concat(exps));
         }
@@ -573,8 +571,8 @@ export function mongoQueryToFilter(query: Record<string, unknown>): Expression {
       }
     }
     if (expressions.length === 1) return expressions[0];
-    const and: Expression = ["AND"];
+    if (expressions.length === 0) return true;
-    return and.concat(expressions);
+    return ["AND", ...expressions];
   }
 
   // empty filter
@@ -592,7 +590,9 @@ export function flattenPreset(
       // Try parse to check expression validity
       parse(p.precondition as string);
     } catch (error) {
-      p.precondition = mongoQueryToFilter(JSON.parse(p.precondition as string));
+      p.precondition = convertOldPrecondition(
+        JSON.parse(p.precondition as string)
+      );
       p.precondition = (p.precondition as string).length
         ? stringify(p.precondition as Expression)
         : "";

test/mongodb-functions.ts

@@ -1,13 +1,13 @@
 import ava from "ava";
 import * as mongodbFunctions from "../lib/mongodb-functions";
-import { stringify } from "../lib/common/expression-parser";
+import { stringify, parse } from "../lib/common/expression-parser";
 
-ava("mongoQueryToFilter", (t) => {
+ava("convertOldPrecondition", (t) => {
   const tests = [
     [{}, "true"],
     [{ test: "test" }, 'test = "test"'],
     [{ test: { $eq: "test" } }, 'test = "test"'],
-    [{ test: { $ne: "test" } }, 'test <> "test"'],
+    [{ test: { $ne: "test" } }, 'test <> "test" OR test IS NULL'],
     [{ test: { $gte: "test" } }, 'test >= "test"'],
     [{ "Tags.test": true }, "Tags.test IS NOT NULL"],
     [{ "Tags.test": false }, "Tags.test IS NULL"],
@@ -22,20 +22,20 @@ ava("mongoQueryToFilter", (t) => {
     [{ _tags: { $eq: "test" } }, "Tags.test IS NOT NULL"],
     [
       { $and: [{ test: "test" }, { test: { $ne: "test" } }] },
-      'test = "test" AND test <> "test"',
+      'test = "test" AND (test <> "test" OR test IS NULL)',
     ],
     [{ test: "test", test2: "test2" }, 'test = "test" AND test2 = "test2"'],
     [
       { $or: [{ test: "test" }, { test: { $ne: "test" } }] },
-      'test = "test" OR test <> "test"',
+      'test = "test" OR (test <> "test" OR test IS NULL)',
     ],
     [
       { test: { $gte: "test1", $ne: "test2" } },
-      'test >= "test1" AND test <> "test2"',
+      'test >= "test1" AND (test <> "test2" OR test IS NULL)',
     ],
     [
       { test: "test", test2: { $ne: "test2" } },
-      'test = "test" AND test2 <> "test2"',
+      'test = "test" AND (test2 <> "test2" OR test2 IS NULL)',
     ],
   ];
 
@@ -50,13 +50,83 @@ ava("mongoQueryToFilter", (t) => {
 
   t.plan(tests.length + 2 * shouldFailTests.length);
   for (const test of tests)
-    t.is(stringify(mongodbFunctions.mongoQueryToFilter(test[0])), test[1]);
+    t.is(stringify(mongodbFunctions.convertOldPrecondition(test[0])), test[1]);
 
   for (const test of shouldFailTests) {
     const func = (): void => {
-      mongodbFunctions.mongoQueryToFilter(test[0] as Record<string, unknown>);
+      mongodbFunctions.convertOldPrecondition(
+        test[0] as Record<string, unknown>
+      );
     };
     const error = t.throws(func, { instanceOf: Error });
     t.is(error.message, test[1]);
   }
 });
+
+ava("filterToMongoQuery", async (t) => {
+  const queries: [string, Record<string, unknown>][] = [
+    ["true", {}],
+    ["Tags.tag1 = true", { _tags: "tag1" }],
+    ["Tags.tag1 <> false", { _tags: "tag1" }],
+    ["Tags.tag1 IS NULL", { _tags: { $ne: "tag1" } }],
+    ["Tags.tag1 = 123", { "Tags.tag1": 123 }],
+    ["Param1 = 'value1'", { "Param1._value": "value1" }],
+    ["Param1 <> 'value1'", { "Param1._value": { $nin: ["value1", null] } }],
+    ["Param1 > 'value'", { "Param1._value": { $gt: "value" } }],
+    ["Param1 IS NOT NULL", { "Param1._value": { $ne: null } }],
+    ["Param1 LIKE 'value'", { "Param1._value": /^value$/ }],
+    ["LOWER(Param1) LIKE 'value'", { "Param1._value": /^value$/i }],
+    [
+      "Param1 <> 'value2' OR NOT (Param2 = 'value1' OR Param1 < 'value2')",
+      {
+        $or: [
+          { "Param1._value": { $nin: ["value2", null] } },
+          {
+            $and: [
+              { "Param2._value": { $nin: ["value1", null] } },
+              { "Param1._value": { $gte: "value2" } },
+            ],
+          },
+        ],
+      },
+    ],
+    [
+      "Param1 <> 'value2' OR Param1 IS NULL",
+      {
+        $or: [
+          { "Param1._value": { $nin: ["value2", null] } },
+          { "Param1._value": null },
+        ],
+      },
+    ],
+  ];
+
+  for (const [expStr, expect] of queries) {
+    const exp = parse(expStr);
+    const query = mongodbFunctions.filterToMongoQuery(
+      mongodbFunctions.processDeviceFilter(exp)
+    );
+    t.deepEqual(query, expect);
+  }
+
+  const failQueries: [any, string][] = [
+    ["Param1 = Param2", "Invalid RHS operand of = clause"],
+    ["Param1 LIKE Param2", "Invalid RHS operand of LIKE clause"],
+    ["1 LIKE '1'", "Invalid LHS operand of LIKE clause"],
+    ["1 = 2", "Invalid LHS operand of = clause"],
+    ["1 IS NULL", "Invalid LHS operand of IS NULL clause"],
+    ["false", "Primitives are not valid queries"],
+    ["UPPER(Param1) LIKE 'value'", "Invalid RHS operand of LIKE clause"],
+  ];
+
+  for (const [expStr, err] of failQueries) {
+    const exp = parse(expStr);
+    t.throws(
+      () =>
+        mongodbFunctions.filterToMongoQuery(
+          mongodbFunctions.processDeviceFilter(exp)
+        ),
+      { message: err }
+    );
+  }
+});