Change Bound.before to Bound.inclusive (#5958)

Author: schmidt-sebastian

packages/firestore/src/core/query.ts

@@ -30,10 +30,11 @@ import {
   newTarget,
   Operator,
   OrderBy,
-  sortsBeforeDocument,
+  boundSortsBeforeDocument,
   stringifyTarget,
   Target,
-  targetEquals
+  targetEquals,
+  boundSortsAfterDocument
 } from './target';
 
 export const enum LimitType {
@@ -323,10 +324,10 @@ export function queryToTarget(query: Query): Target {
 
       // We need to swap the cursors to match the now-flipped query ordering.
       const startAt = queryImpl.endAt
-        ? new Bound(queryImpl.endAt.position, !queryImpl.endAt.before)
+        ? new Bound(queryImpl.endAt.position, !queryImpl.endAt.inclusive)
         : null;
       const endAt = queryImpl.startAt
-        ? new Bound(queryImpl.startAt.position, !queryImpl.startAt.before)
+        ? new Bound(queryImpl.startAt.position, !queryImpl.startAt.inclusive)
         : null;
 
       // Now return as a LimitType.First query.
@@ -512,13 +513,13 @@ function queryMatchesFilters(query: Query, doc: Document): boolean {
 function queryMatchesBounds(query: Query, doc: Document): boolean {
   if (
     query.startAt &&
-    !sortsBeforeDocument(query.startAt, queryOrderBy(query), doc)
+    !boundSortsBeforeDocument(query.startAt, queryOrderBy(query), doc)
   ) {
     return false;
   }
   if (
     query.endAt &&
-    sortsBeforeDocument(query.endAt, queryOrderBy(query), doc)
+    !boundSortsAfterDocument(query.endAt, queryOrderBy(query), doc)
   ) {
     return false;
   }

packages/firestore/src/core/target.ts

@@ -105,28 +105,30 @@ export function canonifyTarget(target: Target): string {
   const targetImpl = debugCast(target, TargetImpl);
 
   if (targetImpl.memoizedCanonicalId === null) {
-    let canonicalId = targetImpl.path.canonicalString();
+    let str = targetImpl.path.canonicalString();
     if (targetImpl.collectionGroup !== null) {
-      canonicalId += '|cg:' + targetImpl.collectionGroup;
+      str += '|cg:' + targetImpl.collectionGroup;
     }
-    canonicalId += '|f:';
-    canonicalId += targetImpl.filters.map(f => canonifyFilter(f)).join(',');
-    canonicalId += '|ob:';
-    canonicalId += targetImpl.orderBy.map(o => canonifyOrderBy(o)).join(',');
+    str += '|f:';
+    str += targetImpl.filters.map(f => canonifyFilter(f)).join(',');
+    str += '|ob:';
+    str += targetImpl.orderBy.map(o => canonifyOrderBy(o)).join(',');
 
     if (!isNullOrUndefined(targetImpl.limit)) {
-      canonicalId += '|l:';
-      canonicalId += targetImpl.limit!;
+      str += '|l:';
+      str += targetImpl.limit!;
     }
     if (targetImpl.startAt) {
-      canonicalId += '|lb:';
-      canonicalId += canonifyBound(targetImpl.startAt);
+      str += '|lb:';
+      str += targetImpl.startAt.inclusive ? 'b:' : 'a:';
+      str += targetImpl.startAt.position.map(p => canonicalId(p)).join(',');
     }
     if (targetImpl.endAt) {
-      canonicalId += '|ub:';
-      canonicalId += canonifyBound(targetImpl.endAt);
+      str += '|ub:';
+      str += targetImpl.endAt.inclusive ? 'a:' : 'b:';
+      str += targetImpl.endAt.position.map(p => canonicalId(p)).join(',');
     }
-    targetImpl.memoizedCanonicalId = canonicalId;
+    targetImpl.memoizedCanonicalId = str;
   }
   return targetImpl.memoizedCanonicalId;
 }
@@ -150,10 +152,14 @@ export function stringifyTarget(target: Target): string {
       .join(', ')}]`;
   }
   if (target.startAt) {
-    str += ', startAt: ' + canonifyBound(target.startAt);
+    str += ', startAt: ';
+    str += target.startAt.inclusive ? 'b:' : 'a:';
+    str += target.startAt.position.map(p => canonicalId(p)).join(',');
   }
   if (target.endAt) {
-    str += ', endAt: ' + canonifyBound(target.endAt);
+    str += ', endAt: ';
+    str += target.endAt.inclusive ? 'a:' : 'b:';
+    str += target.endAt.position.map(p => canonicalId(p)).join(',');
   }
   return `Target(${str})`;
 }
@@ -351,7 +357,7 @@ export function targetGetLowerBound(
           const cursorValue = target.startAt.position[i];
           if (valuesMax(segmentValue, cursorValue) === cursorValue) {
             segmentValue = cursorValue;
-            segmentInclusive = !target.startAt.before;
+            segmentInclusive = target.startAt.inclusive;
           }
           break;
         }
@@ -365,7 +371,7 @@ export function targetGetLowerBound(
     values.push(segmentValue);
     inclusive &&= segmentInclusive;
   }
-  return new Bound(values, !inclusive);
+  return new Bound(values, inclusive);
 }
 /**
  * Returns an upper bound of field values that can be used as an ending point
@@ -436,7 +442,7 @@ export function targetGetUpperBound(
           const cursorValue = target.endAt.position[i];
           if (valuesMin(segmentValue, cursorValue) === cursorValue) {
             segmentValue = cursorValue;
-            segmentInclusive = !target.endAt.before;
+            segmentInclusive = target.endAt.inclusive;
           }
           break;
         }
@@ -451,7 +457,7 @@ export function targetGetUpperBound(
     inclusive &&= segmentInclusive;
   }
 
-  return new Bound(values, !inclusive);
+  return new Bound(values, inclusive);
 }
 
 export abstract class Filter {
@@ -772,7 +778,6 @@ export class ArrayContainsAnyFilter extends FieldFilter {
   }
 }
 
-// TODO(indexing): Change Bound.before to "inclusive"
 /**
  * Represents a bound of a query.
  *
@@ -788,14 +793,7 @@ export class ArrayContainsAnyFilter extends FieldFilter {
  * just after the provided values.
  */
 export class Bound {
-  constructor(readonly position: ProtoValue[], readonly before: boolean) {}
-}
-
-export function canonifyBound(bound: Bound): string {
-  // TODO(b/29183165): Make this collision robust.
-  return `${bound.before ? 'b' : 'a'}:${bound.position
-    .map(p => canonicalId(p))
-    .join(',')}`;
+  constructor(readonly position: ProtoValue[], readonly inclusive: boolean) {}
 }
 
 /**
@@ -821,15 +819,11 @@ export function orderByEquals(left: OrderBy, right: OrderBy): boolean {
   return left.dir === right.dir && left.field.isEqual(right.field);
 }
 
-/**
- * Returns true if a document sorts before a bound using the provided sort
- * order.
- */
-export function sortsBeforeDocument(
+function boundCompareToDocument(
   bound: Bound,
   orderBy: OrderBy[],
   doc: Document
-): boolean {
+): number {
   debugAssert(
     bound.position.length <= orderBy.length,
     "Bound has more components than query's orderBy"
@@ -862,7 +856,33 @@ export function sortsBeforeDocument(
       break;
     }
   }
-  return bound.before ? comparison <= 0 : comparison < 0;
+  return comparison;
+}
+
+/**
+ * Returns true if a document sorts after a bound using the provided sort
+ * order.
+ */
+export function boundSortsAfterDocument(
+  bound: Bound,
+  orderBy: OrderBy[],
+  doc: Document
+): boolean {
+  const comparison = boundCompareToDocument(bound, orderBy, doc);
+  return bound.inclusive ? comparison >= 0 : comparison > 0;
+}
+
+/**
+ * Returns true if a document sorts before a bound using the provided sort
+ * order.
+ */
+export function boundSortsBeforeDocument(
+  bound: Bound,
+  orderBy: OrderBy[],
+  doc: Document
+): boolean {
+  const comparison = boundCompareToDocument(bound, orderBy, doc);
+  return bound.inclusive ? comparison <= 0 : comparison < 0;
 }
 
 export function boundEquals(left: Bound | null, right: Bound | null): boolean {
@@ -873,7 +893,7 @@ export function boundEquals(left: Bound | null, right: Bound | null): boolean {
   }
 
   if (
-    left.before !== right.before ||
+    left.inclusive !== right.inclusive ||
     left.position.length !== right.position.length
   ) {
     return false;

packages/firestore/src/lite-api/query.ts

@@ -282,7 +282,7 @@ class QueryStartAtConstraint extends QueryConstraint {
   constructor(
     readonly type: 'startAt' | 'startAfter',
     private readonly _docOrFields: Array<unknown | DocumentSnapshot<unknown>>,
-    private readonly _before: boolean
+    private readonly _inclusive: boolean
   ) {
     super();
   }
@@ -292,7 +292,7 @@ class QueryStartAtConstraint extends QueryConstraint {
       query,
       this.type,
       this._docOrFields,
-      this._before
+      this._inclusive
     );
     return new Query(
       query.firestore,
@@ -325,7 +325,11 @@ export function startAt(...fieldValues: unknown[]): QueryConstraint;
 export function startAt(
   ...docOrFields: Array<unknown | DocumentSnapshot<unknown>>
 ): QueryConstraint {
-  return new QueryStartAtConstraint('startAt', docOrFields, /*before=*/ true);
+  return new QueryStartAtConstraint(
+    'startAt',
+    docOrFields,
+    /*inclusive=*/ true
+  );
 }
 
 /**
@@ -356,15 +360,15 @@ export function startAfter(
   return new QueryStartAtConstraint(
     'startAfter',
     docOrFields,
-    /*before=*/ false
+    /*inclusive=*/ false
   );
 }
 
 class QueryEndAtConstraint extends QueryConstraint {
   constructor(
     readonly type: 'endBefore' | 'endAt',
     private readonly _docOrFields: Array<unknown | DocumentSnapshot<unknown>>,
-    private readonly _before: boolean
+    private readonly _inclusive: boolean
   ) {
     super();
   }
@@ -374,7 +378,7 @@ class QueryEndAtConstraint extends QueryConstraint {
       query,
       this.type,
       this._docOrFields,
-      this._before
+      this._inclusive
     );
     return new Query(
       query.firestore,
@@ -407,7 +411,11 @@ export function endBefore(...fieldValues: unknown[]): QueryConstraint;
 export function endBefore(
   ...docOrFields: Array<unknown | DocumentSnapshot<unknown>>
 ): QueryConstraint {
-  return new QueryEndAtConstraint('endBefore', docOrFields, /*before=*/ true);
+  return new QueryEndAtConstraint(
+    'endBefore',
+    docOrFields,
+    /*inclusive=*/ false
+  );
 }
 
 /**
@@ -433,15 +441,15 @@ export function endAt(...fieldValues: unknown[]): QueryConstraint;
 export function endAt(
   ...docOrFields: Array<unknown | DocumentSnapshot<unknown>>
 ): QueryConstraint {
-  return new QueryEndAtConstraint('endAt', docOrFields, /*before=*/ false);
+  return new QueryEndAtConstraint('endAt', docOrFields, /*inclusive=*/ true);
 }
 
 /** Helper function to create a bound from a document or fields */
 function newQueryBoundFromDocOrFields<T>(
   query: Query,
   methodName: string,
   docOrFields: Array<unknown | DocumentSnapshot<T>>,
-  before: boolean
+  inclusive: boolean
 ): Bound {
   docOrFields[0] = getModularInstance(docOrFields[0]);
 
@@ -451,7 +459,7 @@ function newQueryBoundFromDocOrFields<T>(
       query.firestore._databaseId,
       methodName,
       docOrFields[0]._document,
-      before
+      inclusive
     );
   } else {
     const reader = newUserDataReader(query.firestore);
@@ -461,7 +469,7 @@ function newQueryBoundFromDocOrFields<T>(
       reader,
       methodName,
       docOrFields,
-      before
+      inclusive
     );
   }
 }
@@ -552,7 +560,7 @@ export function newQueryBoundFromDocument(
   databaseId: DatabaseId,
   methodName: string,
   doc: Document | null,
-  before: boolean
+  inclusive: boolean
 ): Bound {
   if (!doc) {
     throw new FirestoreError(
@@ -598,7 +606,7 @@ export function newQueryBoundFromDocument(
       }
     }
   }
-  return new Bound(components, before);
+  return new Bound(components, inclusive);
 }
 
 /**
@@ -610,7 +618,7 @@ export function newQueryBoundFromFields(
   dataReader: UserDataReader,
   methodName: string,
   values: unknown[],
-  before: boolean
+  inclusive: boolean
 ): Bound {
   // Use explicit order by's because it has to match the query the user made
   const orderBy = query.explicitOrderBy;
@@ -661,7 +669,7 @@ export function newQueryBoundFromFields(
     }
   }
 
-  return new Bound(components, before);
+  return new Bound(components, inclusive);
 }
 
 /**