fix: Resolve issues with datetime comparisons in scenarios where micr…

…osecond precision is provided in the underlying value. Fixes Filtering by Dates with Table Service no longer working in 3.25.0 #2069
Azure · Jul 27, 2023 · 647475c · 647475c
1 parent bb90e25
commit 647475c
Show file tree

Hide file tree

Showing 11 changed files with 246 additions and 35 deletions.
diff --git a/src/table/persistence/QueryInterpreter/QueryNodes/DateTimeNode.ts b/src/table/persistence/QueryInterpreter/QueryNodes/DateTimeNode.ts
@@ -1,5 +1,6 @@
 import { IQueryContext } from "../IQueryContext";
 import IQueryNode from "./IQueryNode";
+import ValueNode from "./ValueNode";
 
 /**
  * Represents a constant value of type `datetime` which is stored in its underlying JavaScript representation.
@@ -9,18 +10,29 @@ import IQueryNode from "./IQueryNode";
  * the query `PartitionKey eq datetime'2019-01-01T00:00:00.000Z'` would contain a `DateTimeNode` with the value
  * `2019-01-01T00:00:00.000Z`.
  */
-export default class DateTimeNode<T> implements IQueryNode {
-  constructor(private value: Date) { }
-
+export default class DateTimeNode<T> extends ValueNode {
   get name(): string {
     return "datetime";
   }
 
-  evaluate(_context: IQueryContext): any {
-    return this.value.toISOString();
-  }
+  compare(context: IQueryContext, other: IQueryNode): number {
+    // NOTE(notheotherben): This approach leverages the fact that the `Date` constructor will parse ISO8601 strings
+    //                 however it runs into a limitation of the accuracy of JS dates (which are limited to millisecond
+    //                 resolution). As a result, we're effectively truncating the value to millisecond precision by doing
+    //                 this. This is fundamentally a trade-off between enforcing valid datetime values and providing perfect
+    //                 accuracy, and we've opted to enforce valid datetime values as those are more likely to cause problems
+    //                 when moving to production.
+    const thisValue = new Date(this.value);
+    const otherValue = new Date(other.evaluate(context));
 
-  toString(): string {
-    return `(${this.name} ${this.value.toISOString()})`;
+    if (thisValue.valueOf() < otherValue.valueOf()) {
+      return -1;
+    } else if (thisValue.valueOf() > otherValue.valueOf()) {
+      return 1;
+    } else if (thisValue.valueOf() === otherValue.valueOf()) {
+      return 0;
+    } else {
+      return NaN;
+    }
   }
 }
diff --git a/src/table/persistence/QueryInterpreter/QueryNodes/EqualsNode.ts b/src/table/persistence/QueryInterpreter/QueryNodes/EqualsNode.ts
@@ -1,6 +1,6 @@
 import { IQueryContext } from "../IQueryContext";
 import BinaryOperatorNode from "./BinaryOperatorNode";
-import GuidNode from "./GuidNode";
+import ValueNode from "./ValueNode";
 
 /**
  * Represents a logical equality operation between two nodes (the `eq` query operator).
@@ -21,13 +21,14 @@ export default class EqualsNode extends BinaryOperatorNode {
   }
 
   evaluate(context: IQueryContext): any {
-    return this.left.evaluate(context) === this.right.evaluate(context) || this.backwardsCompatibleGuidEvaluate(context);
-  }
+    if (this.left instanceof ValueNode) {
+      return this.left.compare(context, this.right) === 0;
+    }
 
-  private backwardsCompatibleGuidEvaluate(context: IQueryContext): boolean {
-    const left = this.left instanceof GuidNode ? this.left.legacyStorageFormat() : this.left.evaluate(context);
-    const right = this.right instanceof GuidNode ? this.right.legacyStorageFormat() : this.right.evaluate(context);
+    if (this.right instanceof ValueNode) {
+      return this.right.compare(context, this.left) === 0;
+    }
 
-    return left === right;
+    return this.left.evaluate(context) === this.right.evaluate(context);
   }
 }
diff --git a/src/table/persistence/QueryInterpreter/QueryNodes/GreaterThanEqualNode.ts b/src/table/persistence/QueryInterpreter/QueryNodes/GreaterThanEqualNode.ts
@@ -1,5 +1,6 @@
 import { IQueryContext } from "../IQueryContext";
 import BinaryOperatorNode from "./BinaryOperatorNode";
+import ValueNode from "./ValueNode";
 
 /**
  * Represents a logical greater than or equal operation between two nodes (the `ge` query operator).
@@ -17,6 +18,14 @@ export default class GreaterThanEqualNode extends BinaryOperatorNode {
   }
 
   evaluate(context: IQueryContext): any {
+    if (this.left instanceof ValueNode) {
+      return this.left.compare(context, this.right) >= 0;
+    }
+
+    if (this.right instanceof ValueNode) {
+      return this.right.compare(context, this.left) <= 0;
+    }
+
     return this.left.evaluate(context) >= this.right.evaluate(context);
   }
 }
diff --git a/src/table/persistence/QueryInterpreter/QueryNodes/GreaterThanNode.ts b/src/table/persistence/QueryInterpreter/QueryNodes/GreaterThanNode.ts
@@ -1,5 +1,6 @@
 import { IQueryContext } from "../IQueryContext";
 import BinaryOperatorNode from "./BinaryOperatorNode";
+import ValueNode from "./ValueNode";
 
 /**
  * Represents a logical greater than operation between two nodes (the `gt` query operator).
@@ -17,6 +18,14 @@ export default class GreaterThanNode extends BinaryOperatorNode {
   }
 
   evaluate(context: IQueryContext): any {
+    if (this.left instanceof ValueNode) {
+      return this.left.compare(context, this.right) > 0;
+    }
+
+    if (this.right instanceof ValueNode) {
+      return this.right.compare(context, this.left) < 0;
+    }
+
     return this.left.evaluate(context) > this.right.evaluate(context);
   }
 }
diff --git a/src/table/persistence/QueryInterpreter/QueryNodes/GuidNode.ts b/src/table/persistence/QueryInterpreter/QueryNodes/GuidNode.ts
@@ -1,5 +1,6 @@
 import { IQueryContext } from "../IQueryContext";
 import IQueryNode from "./IQueryNode";
+import ValueNode from "./ValueNode";
 
 /**
  * Represents a constant value of type GUID which can be compared against the base64 representation of the GUID
@@ -12,9 +13,7 @@ import IQueryNode from "./IQueryNode";
  * NOTE: This node type also exposes a `legacyStorageFormat()` method which returns the GUID in its string representation
  *       for backwards compatibility with the legacy table storage format.
  */
-export default class GuidNode<T> implements IQueryNode {
-  constructor(private value: string) { }
-
+export default class GuidNode<T> extends ValueNode {
   get name(): string {
     return "guid";
   }
@@ -23,11 +22,21 @@ export default class GuidNode<T> implements IQueryNode {
     return Buffer.from(this.value).toString("base64");
   }
 
-  legacyStorageFormat(): string {
-    return this.value;
-  }
+  compare(context: IQueryContext, other: IQueryNode): number {
+    const otherValue = other.evaluate(context);
+    let thisValue = this.value;
+
+    // If the other value is not in its raw GUID format, then let's convert this value to its base64 representation
+    if (!/[0-9a-f]{8}-([0-9a-f]{4}-){3}[0-9a-f]{12}/i.test(otherValue)) {
+      thisValue = Buffer.from(this.value).toString("base64");
+    }
 
-  toString(): string {
-    return `(${this.name} ${this.value})`;
+    if (thisValue < otherValue) {
+      return -1;
+    } else if (thisValue > otherValue) {
+      return 1;
+    } else {
+      return 0;
+    }
   }
 }
diff --git a/src/table/persistence/QueryInterpreter/QueryNodes/LessThanEqualNode.ts b/src/table/persistence/QueryInterpreter/QueryNodes/LessThanEqualNode.ts
@@ -1,5 +1,6 @@
 import { IQueryContext } from "../IQueryContext";
 import BinaryOperatorNode from "./BinaryOperatorNode";
+import ValueNode from "./ValueNode";
 
 /**
  * Represents a logical less than or equal operation between two nodes (the `le` query operator).
@@ -17,6 +18,14 @@ export default class LessThanEqualNode extends BinaryOperatorNode {
   }
 
   evaluate(context: IQueryContext): any {
+    if (this.left instanceof ValueNode) {
+      return this.left.compare(context, this.right) <= 0;
+    }
+
+    if (this.right instanceof ValueNode) {
+      return this.right.compare(context, this.left) >= 0;
+    }
+
     return this.left.evaluate(context) <= this.right.evaluate(context);
   }
 }
diff --git a/src/table/persistence/QueryInterpreter/QueryNodes/LessThanNode.ts b/src/table/persistence/QueryInterpreter/QueryNodes/LessThanNode.ts
@@ -1,5 +1,6 @@
 import { IQueryContext } from "../IQueryContext";
 import BinaryOperatorNode from "./BinaryOperatorNode";
+import ValueNode from "./ValueNode";
 
 /**
  * Represents a logical less than operation between two nodes (the `lt` query operator).
@@ -17,6 +18,14 @@ export default class LessThanNode extends BinaryOperatorNode {
   }
 
   evaluate(context: IQueryContext): any {
+    if (this.left instanceof ValueNode) {
+      return this.left.compare(context, this.right) < 0;
+    }
+
+    if (this.right instanceof ValueNode) {
+      return this.right.compare(context, this.left) > 0;
+    }
+
     return this.left.evaluate(context) < this.right.evaluate(context);
   }
 }
diff --git a/src/table/persistence/QueryInterpreter/QueryNodes/NotEqualsNode.ts b/src/table/persistence/QueryInterpreter/QueryNodes/NotEqualsNode.ts
@@ -1,6 +1,6 @@
 import { IQueryContext } from "../IQueryContext";
 import BinaryOperatorNode from "./BinaryOperatorNode";
-import GuidNode from "./GuidNode";
+import ValueNode from "./ValueNode";
 
 /**
  * Represents a logical not equal operation between two nodes (the `ne` query operator).
@@ -21,18 +21,17 @@ export default class NotEqualsNode extends BinaryOperatorNode {
   }
 
   evaluate(context: IQueryContext): any {
-    const left = this.left.evaluate(context);
-    const right = this.right.evaluate(context);
+    if (this.left instanceof ValueNode) {
+      return this.left.compare(context, this.right) !== 0;
+    }
 
-    // If either side is undefined, we should not match - this only occurs in scenarios where
-    // the field itself doesn't exist on the entity.
-    return left !== right && left !== undefined && right !== undefined && this.backwardsCompatibleGuidEvaluate(context);
-  }
+    if (this.right instanceof ValueNode) {
+      return this.right.compare(context, this.left) !== 0;
+    }
 
-  private backwardsCompatibleGuidEvaluate(context: IQueryContext): boolean {
-    const left = this.left instanceof GuidNode ? this.left.legacyStorageFormat() : this.left.evaluate(context);
-    const right = this.right instanceof GuidNode ? this.right.legacyStorageFormat() : this.right.evaluate(context);
+    const left = this.left.evaluate(context);
+    const right = this.right.evaluate(context);
 
-    return left !== right;
+    return left !== right && left !== undefined && right !== undefined;
   }
 }
diff --git a/src/table/persistence/QueryInterpreter/QueryNodes/ValueNode.ts b/src/table/persistence/QueryInterpreter/QueryNodes/ValueNode.ts
@@ -0,0 +1,30 @@
+import { IQueryContext } from "../IQueryContext";
+import IQueryNode from "./IQueryNode";
+
+/**
+ * Represents a typed value which can implement its own comparison logic.
+ */
+export default abstract class ValueNode implements IQueryNode {
+  constructor(protected value: string) { }
+
+  abstract get name(): string;
+
+  evaluate(_context: IQueryContext): any {
+    return this.value;
+  }
+
+  compare(context: IQueryContext, other: IQueryNode): number {
+    const otherValue = other.evaluate(context);
+    if (this.value < otherValue) {
+      return -1;
+    } else if (this.value > otherValue) {
+      return 1;
+    } else {
+      return 0;
+    }
+  }
+
+  toString(): string {
+    return `(${this.name} ${this.value})`;
+  }
+}
diff --git a/src/table/persistence/QueryInterpreter/QueryParser.ts b/src/table/persistence/QueryInterpreter/QueryParser.ts
@@ -223,7 +223,7 @@ class QueryParser {
 
     switch (typeHint.value?.toLowerCase()) {
       case "datetime":
-        return new DateTimeNode(new Date(value.value!));
+        return new DateTimeNode(value.value!);
       case "guid":
         return new GuidNode(value.value!);
       case "binary":