Fix the cursor expanding result set bug (#6166)

wu-hui · web-flow · commit dc0e9263251b · 2022-04-21T14:54:34.000-04:00
diff --git a/packages/firestore/src/core/target.ts b/packages/firestore/src/core/target.ts
@@ -31,13 +31,13 @@ import {
   isReferenceValue,
   MAX_VALUE,
   MIN_VALUE,
+  lowerBoundCompare,
   typeOrder,
+  upperBoundCompare,
   valueCompare,
   valueEquals,
   valuesGetLowerBound,
-  valuesGetUpperBound,
-  valuesMax,
-  valuesMin
+  valuesGetUpperBound
 } from '../model/values';
 import { Value as ProtoValue } from '../protos/firestore_proto_api';
 import { debugAssert, debugCast, fail } from '../util/assert';
@@ -293,13 +293,13 @@ export function targetGetNotInValues(
 
 /**
  * Returns a lower bound of field values that can be used as a starting point to
- * scan the index defined by `fieldIndex`. Returns `null` if no lower bound
+ * scan the index defined by `fieldIndex`. Returns `MIN_VALUE` if no lower bound
  * exists.
  */
 export function targetGetLowerBound(
   target: Target,
   fieldIndex: FieldIndex
-): Bound | null {
+): Bound {
   const values: ProtoValue[] = [];
   let inclusive = true;
 
@@ -311,10 +311,6 @@ export function targetGetLowerBound(
         ? targetGetAscendingBound(target, segment.fieldPath, target.startAt)
         : targetGetDescendingBound(target, segment.fieldPath, target.startAt);
 
-    if (!segmentBound.value) {
-      // No lower bound exists
-      return null;
-    }
     values.push(segmentBound.value);
     inclusive &&= segmentBound.inclusive;
   }
@@ -323,13 +319,13 @@ export function targetGetLowerBound(
 
 /**
  * Returns an upper bound of field values that can be used as an ending point
- * when scanning the index defined by `fieldIndex`. Returns `null` if no
+ * when scanning the index defined by `fieldIndex`. Returns `MAX_VALUE` if no
  * upper bound exists.
  */
 export function targetGetUpperBound(
   target: Target,
   fieldIndex: FieldIndex
-): Bound | null {
+): Bound {
   const values: ProtoValue[] = [];
   let inclusive = true;
 
@@ -341,10 +337,6 @@ export function targetGetUpperBound(
         ? targetGetDescendingBound(target, segment.fieldPath, target.endAt)
         : targetGetAscendingBound(target, segment.fieldPath, target.endAt);
 
-    if (!segmentBound.value) {
-      // No upper bound exists
-      return null;
-    }
     values.push(segmentBound.value);
     inclusive &&= segmentBound.inclusive;
   }
@@ -360,13 +352,14 @@ function targetGetAscendingBound(
   target: Target,
   fieldPath: FieldPath,
   bound: Bound | null
-): { value: ProtoValue | undefined; inclusive: boolean } {
-  let value: ProtoValue | undefined = undefined;
+): { value: ProtoValue; inclusive: boolean } {
+  let value: ProtoValue = MIN_VALUE;
+
   let inclusive = true;
 
   // Process all filters to find a value for the current field segment
   for (const fieldFilter of targetGetFieldFiltersForPath(target, fieldPath)) {
-    let filterValue: ProtoValue | undefined = undefined;
+    let filterValue: ProtoValue = MIN_VALUE;
     let filterInclusive = true;
 
     switch (fieldFilter.op) {
@@ -391,7 +384,12 @@ function targetGetAscendingBound(
       // Remaining filters cannot be used as lower bounds.
     }
 
-    if (valuesMax(value, filterValue) === filterValue) {
+    if (
+      lowerBoundCompare(
+        { value, inclusive },
+        { value: filterValue, inclusive: filterInclusive }
+      ) < 0
+    ) {
       value = filterValue;
       inclusive = filterInclusive;
     }
@@ -404,7 +402,12 @@ function targetGetAscendingBound(
       const orderBy = target.orderBy[i];
       if (orderBy.field.isEqual(fieldPath)) {
         const cursorValue = bound.position[i];
-        if (valuesMax(value, cursorValue) === cursorValue) {
+        if (
+          lowerBoundCompare(
+            { value, inclusive },
+            { value: cursorValue, inclusive: bound.inclusive }
+          ) < 0
+        ) {
           value = cursorValue;
           inclusive = bound.inclusive;
         }
@@ -424,13 +427,13 @@ function targetGetDescendingBound(
   target: Target,
   fieldPath: FieldPath,
   bound: Bound | null
-): { value: ProtoValue | undefined; inclusive: boolean } {
-  let value: ProtoValue | undefined = undefined;
+): { value: ProtoValue; inclusive: boolean } {
+  let value: ProtoValue = MAX_VALUE;
   let inclusive = true;
 
   // Process all filters to find a value for the current field segment
   for (const fieldFilter of targetGetFieldFiltersForPath(target, fieldPath)) {
-    let filterValue: ProtoValue | undefined = undefined;
+    let filterValue: ProtoValue = MAX_VALUE;
     let filterInclusive = true;
 
     switch (fieldFilter.op) {
@@ -456,7 +459,12 @@ function targetGetDescendingBound(
       // Remaining filters cannot be used as upper bounds.
     }
 
-    if (valuesMin(value, filterValue) === filterValue) {
+    if (
+      upperBoundCompare(
+        { value, inclusive },
+        { value: filterValue, inclusive: filterInclusive }
+      ) > 0
+    ) {
       value = filterValue;
       inclusive = filterInclusive;
     }
@@ -469,7 +477,12 @@ function targetGetDescendingBound(
       const orderBy = target.orderBy[i];
       if (orderBy.field.isEqual(fieldPath)) {
         const cursorValue = bound.position[i];
-        if (valuesMin(value, cursorValue) === cursorValue) {
+        if (
+          upperBoundCompare(
+            { value, inclusive },
+            { value: cursorValue, inclusive: bound.inclusive }
+          ) > 0
+        ) {
           value = cursorValue;
           inclusive = bound.inclusive;
         }
diff --git a/packages/firestore/src/local/indexeddb_index_manager.ts b/packages/firestore/src/local/indexeddb_index_manager.ts
@@ -285,9 +285,9 @@ export class IndexedDbIndexManager implements IndexManager {
             index!.indexId,
             arrayValues,
             lowerBoundEncoded,
-            !!lowerBound && lowerBound.inclusive,
+            lowerBound.inclusive,
             upperBoundEncoded,
-            !!upperBound && upperBound.inclusive,
+            upperBound.inclusive,
             notInEncoded
           );
           return PersistencePromise.forEach(
@@ -331,9 +331,9 @@ export class IndexedDbIndexManager implements IndexManager {
   private generateIndexRanges(
     indexId: number,
     arrayValues: ProtoValue[] | null,
-    lowerBounds: Uint8Array[] | null,
+    lowerBounds: Uint8Array[],
     lowerBoundInclusive: boolean,
-    upperBounds: Uint8Array[] | null,
+    upperBounds: Uint8Array[],
     upperBoundInclusive: boolean,
     notInValues: Uint8Array[]
   ): IDBKeyRange[] {
@@ -343,10 +343,7 @@ export class IndexedDbIndexManager implements IndexManager {
     // combined with the values from the query bounds.
     const totalScans =
       (arrayValues != null ? arrayValues.length : 1) *
-      Math.max(
-        lowerBounds != null ? lowerBounds.length : 1,
-        upperBounds != null ? upperBounds.length : 1
-      );
+      Math.max(lowerBounds.length, upperBounds.length);
     const scansPerArrayElement =
       totalScans / (arrayValues != null ? arrayValues.length : 1);
 
@@ -356,22 +353,18 @@ export class IndexedDbIndexManager implements IndexManager {
         ? this.encodeSingleElement(arrayValues[i / scansPerArrayElement])
         : EMPTY_VALUE;
 
-      const lowerBound = lowerBounds
-        ? this.generateLowerBound(
-            indexId,
-            arrayValue,
-            lowerBounds[i % scansPerArrayElement],
-            lowerBoundInclusive
-          )
-        : this.generateEmptyBound(indexId);
-      const upperBound = upperBounds
-        ? this.generateUpperBound(
-            indexId,
-            arrayValue,
-            upperBounds[i % scansPerArrayElement],
-            upperBoundInclusive
-          )
-        : this.generateEmptyBound(indexId + 1);
+      const lowerBound = this.generateLowerBound(
+        indexId,
+        arrayValue,
+        lowerBounds[i % scansPerArrayElement],
+        lowerBoundInclusive
+      );
+      const upperBound = this.generateUpperBound(
+        indexId,
+        arrayValue,
+        upperBounds[i % scansPerArrayElement],
+        upperBoundInclusive
+      );
 
       const notInBound = notInValues.map(notIn =>
         this.generateLowerBound(
@@ -420,19 +413,6 @@ export class IndexedDbIndexManager implements IndexManager {
     return inclusive ? entry.successor() : entry;
   }
 
-  /**
-   * Generates an empty bound that scopes the index scan to the current index
-   * and user.
-   */
-  private generateEmptyBound(indexId: number): IndexEntry {
-    return new IndexEntry(
-      indexId,
-      DocumentKey.empty(),
-      EMPTY_VALUE,
-      EMPTY_VALUE
-    );
-  }
-
   private getFieldIndex(
     transaction: PersistenceTransaction,
     target: Target
@@ -572,11 +552,8 @@ export class IndexedDbIndexManager implements IndexManager {
   private encodeBound(
     fieldIndex: FieldIndex,
     target: Target,
-    bound: Bound | null
-  ): Uint8Array[] | null {
-    if (bound == null) {
-      return null;
-    }
+    bound: Bound
+  ): Uint8Array[] {
     return this.encodeValues(fieldIndex, target, bound.position);
   }
 
diff --git a/packages/firestore/src/model/values.ts b/packages/firestore/src/model/values.ts
@@ -78,7 +78,7 @@ export function typeOrder(value: Value): TypeOrder {
     if (isServerTimestamp(value)) {
       return TypeOrder.ServerTimestampValue;
     } else if (isMaxValue(value)) {
-      return TypeOrder.ArrayValue;
+      return TypeOrder.MaxValue;
     }
     return TypeOrder.ObjectValue;
   } else {
@@ -350,6 +350,14 @@ function compareArrays(left: ArrayValue, right: ArrayValue): number {
 }
 
 function compareMaps(left: MapValue, right: MapValue): number {
+  if (left === MAX_VALUE.mapValue && right === MAX_VALUE.mapValue) {
+    return 0;
+  } else if (left === MAX_VALUE.mapValue) {
+    return 1;
+  } else if (right === MAX_VALUE.mapValue) {
+    return -1;
+  }
+
   const leftMap = left.fields || {};
   const leftKeys = Object.keys(leftMap);
   const rightMap = right.fields || {};
@@ -670,28 +678,38 @@ export function valuesGetUpperBound(value: Value): Value {
   }
 }
 
-export function valuesMax(
-  left: Value | undefined,
-  right: Value | undefined
-): Value | undefined {
-  if (left === undefined) {
-    return right;
-  } else if (right === undefined) {
-    return left;
-  } else {
-    return valueCompare(left, right) > 0 ? left : right;
+export function lowerBoundCompare(
+  left: { value: Value; inclusive: boolean },
+  right: { value: Value; inclusive: boolean }
+): number {
+  const cmp = valueCompare(left.value, right.value);
+  if (cmp !== 0) {
+    return cmp;
   }
+
+  if (left.inclusive && !right.inclusive) {
+    return -1;
+  } else if (!left.inclusive && right.inclusive) {
+    return 1;
+  }
+
+  return 0;
 }
 
-export function valuesMin(
-  left: Value | undefined,
-  right: Value | undefined
-): Value | undefined {
-  if (left === undefined) {
-    return right;
-  } else if (right === undefined) {
-    return left;
-  } else {
-    return valueCompare(left, right) < 0 ? left : right;
+export function upperBoundCompare(
+  left: { value: Value; inclusive: boolean },
+  right: { value: Value; inclusive: boolean }
+): number {
+  const cmp = valueCompare(left.value, right.value);
+  if (cmp !== 0) {
+    return cmp;
   }
+
+  if (left.inclusive && !right.inclusive) {
+    return 1;
+  } else if (!left.inclusive && right.inclusive) {
+    return -1;
+  }
+
+  return 0;
 }
diff --git a/packages/firestore/test/unit/local/index_manager.test.ts b/packages/firestore/test/unit/local/index_manager.test.ts
@@ -886,6 +886,35 @@ describe('IndexedDbIndexManager', async () => {
     await verifyResults(queryWithRestrictedBound, 'coll/val4');
   });
 
+  it('cannot expand result set from a cursor', async () => {
+    await indexManager.addFieldIndex(
+      fieldIndex('coll', { fields: [['c', IndexKind.ASCENDING]] })
+    );
+    await indexManager.addFieldIndex(
+      fieldIndex('coll', { fields: [['c', IndexKind.DESCENDING]] })
+    );
+    await addDoc('coll/val1', { 'a': 1, 'b': 1, 'c': 3 });
+    await addDoc('coll/val2', { 'a': 2, 'b': 2, 'c': 2 });
+
+    let testingQuery = queryWithStartAt(
+      queryWithAddedOrderBy(
+        queryWithAddedFilter(query('coll'), filter('c', '>', 2)),
+        orderBy('c', 'asc')
+      ),
+      bound([2], /* inclusive= */ true)
+    );
+    await verifyResults(testingQuery, 'coll/val1');
+
+    testingQuery = queryWithStartAt(
+      queryWithAddedOrderBy(
+        queryWithAddedFilter(query('coll'), filter('c', '<', 3)),
+        orderBy('c', 'desc')
+      ),
+      bound([3], /* inclusive= */ true)
+    );
+    await verifyResults(testingQuery, 'coll/val2');
+  });
+
   it('support advances queries', async () => {
     // This test compares local query results with those received from the Java
     // Server SDK.
diff --git a/packages/firestore/test/unit/model/target.test.ts b/packages/firestore/test/unit/model/target.test.ts
