From effced9ccc49888d6c633fa5c719e8510c29f4a8 Mon Sep 17 00:00:00 2001
From: Mark Duckworth <markduckworth@google.com>
Date: Fri, 24 Jun 2022 14:53:46 -0600
Subject: [PATCH 1/5] Add composite filters. Port of
 https://github.com/firebase/firebase-android-sdk/pull/3290

---
 common/api-review/firestore-lite.api.md      |   2 +-
 common/api-review/firestore.api.md           |   2 +-
 packages/firestore/src/core/query.ts         |  48 ++---
 packages/firestore/src/core/target.ts        | 134 ++++++++++++-
 packages/firestore/src/lite-api/query.ts     | 190 ++++++++++++++++---
 packages/firestore/src/local/query_engine.ts |  11 +-
 6 files changed, 321 insertions(+), 66 deletions(-)

diff --git a/common/api-review/firestore-lite.api.md b/common/api-review/firestore-lite.api.md
index a96f3ada5cb..957216013d2 100644
--- a/common/api-review/firestore-lite.api.md
+++ b/common/api-review/firestore-lite.api.md
@@ -230,7 +230,7 @@ export abstract class QueryConstraint {
 }
 
 // @public
-export type QueryConstraintType = 'where' | 'orderBy' | 'limit' | 'limitToLast' | 'startAt' | 'startAfter' | 'endAt' | 'endBefore';
+export type QueryConstraintType = 'where' | 'orderBy' | 'limit' | 'limitToLast' | 'startAt' | 'startAfter' | 'endAt' | 'endBefore' | 'and' | 'or';
 
 // @public
 export class QueryDocumentSnapshot<T = DocumentData> extends DocumentSnapshot<T> {
diff --git a/common/api-review/firestore.api.md b/common/api-review/firestore.api.md
index db36b60482c..e13eb1dc716 100644
--- a/common/api-review/firestore.api.md
+++ b/common/api-review/firestore.api.md
@@ -359,7 +359,7 @@ export abstract class QueryConstraint {
 }
 
 // @public
-export type QueryConstraintType = 'where' | 'orderBy' | 'limit' | 'limitToLast' | 'startAt' | 'startAfter' | 'endAt' | 'endBefore';
+export type QueryConstraintType = 'where' | 'orderBy' | 'limit' | 'limitToLast' | 'startAt' | 'startAfter' | 'endAt' | 'endBefore' | 'and' | 'or';
 
 // @public
 export class QueryDocumentSnapshot<T = DocumentData> extends DocumentSnapshot<T> {
diff --git a/packages/firestore/src/core/query.ts b/packages/firestore/src/core/query.ts
index 2b4fca74c3a..90a55d04979 100644
--- a/packages/firestore/src/core/query.ts
+++ b/packages/firestore/src/core/query.ts
@@ -33,7 +33,8 @@ import {
   stringifyTarget,
   Target,
   targetEquals,
-  boundSortsAfterDocument
+  boundSortsAfterDocument,
+  CompositeFilter
 } from './target';
 
 export const enum LimitType {
@@ -166,6 +167,13 @@ export function queryMatchesAllDocuments(query: Query): boolean {
   );
 }
 
+export function queryContainsCompositeFilters(query: Query): boolean {
+  return (
+    query.filters.find(filter => filter instanceof CompositeFilter) !==
+    undefined
+  );
+}
+
 export function getFirstOrderByField(query: Query): FieldPath | null {
   return query.explicitOrderBy.length > 0
     ? query.explicitOrderBy[0].field
@@ -174,34 +182,12 @@ export function getFirstOrderByField(query: Query): FieldPath | null {
 
 export function getInequalityFilterField(query: Query): FieldPath | null {
   for (const filter of query.filters) {
-    debugAssert(
-      filter instanceof FieldFilter,
-      'Only FieldFilters are supported'
-    );
-    if (filter.isInequality()) {
-      return filter.field;
+    const result = filter.getFirstInequalityField();
+    if (result !== null) {
+      return result;
     }
   }
-  return null;
-}
 
-/**
- * Checks if any of the provided Operators are included in the query and
- * returns the first one that is, or null if none are.
- */
-export function findFilterOperator(
-  query: Query,
-  operators: Operator[]
-): Operator | null {
-  for (const filter of query.filters) {
-    debugAssert(
-      filter instanceof FieldFilter,
-      'Only FieldFilters are supported'
-    );
-    if (operators.indexOf(filter.op) >= 0) {
-      return filter.op;
-    }
-  }
   return null;
 }
 
@@ -337,11 +323,13 @@ export function queryToTarget(query: Query): Target {
 }
 
 export function queryWithAddedFilter(query: Query, filter: Filter): Query {
+  let newInequalityField = filter.getFirstInequalityField();
+  let queryInequalityField = getInequalityFilterField(query);
+
   debugAssert(
-    getInequalityFilterField(query) == null ||
-      !(filter instanceof FieldFilter) ||
-      !filter.isInequality() ||
-      filter.field.isEqual(getInequalityFilterField(query)!),
+    queryInequalityField == null ||
+      newInequalityField == null ||
+      newInequalityField.isEqual(queryInequalityField),
     'Query must only have one inequality field.'
   );
 
diff --git a/packages/firestore/src/core/target.ts b/packages/firestore/src/core/target.ts
index 72a760a4529..d5bb0e7f93e 100644
--- a/packages/firestore/src/core/target.ts
+++ b/packages/firestore/src/core/target.ts
@@ -536,6 +536,12 @@ export function targetGetSegmentCount(target: Target): number {
 
 export abstract class Filter {
   abstract matches(doc: Document): boolean;
+
+  abstract getFlattenedFilters(): FieldFilter[];
+
+  abstract getFilters(): Filter[];
+
+  abstract getFirstInequalityField(): FieldPath | null;
 }
 
 export const enum Operator {
@@ -551,6 +557,11 @@ export const enum Operator {
   ARRAY_CONTAINS_ANY = 'array-contains-any'
 }
 
+export const enum CompositeOperator {
+  OR = 'or',
+  AND = 'and'
+}
+
 /**
  * The direction of sorting in an order by.
  */
@@ -685,21 +696,124 @@ export class FieldFilter extends Filter {
       ].indexOf(this.op) >= 0
     );
   }
+
+  public getFlattenedFilters(): FieldFilter[] {
+    return [this];
+  }
+
+  public getFilters(): Filter[] {
+    return [this];
+  }
+
+  public getFirstInequalityField(): FieldPath | null {
+    if (this.isInequality()) {
+      return this.field;
+    }
+    return null;
+  }
+}
+
+export class CompositeFilter extends Filter {
+  protected constructor(
+    public filters: Filter[],
+    public op: CompositeOperator
+  ) {
+    super();
+  }
+
+  /**
+   * Creates a filter based on the provided arguments.
+   */
+  static create(filters: Filter[], op: CompositeOperator): CompositeFilter {
+    return new CompositeFilter(filters, op);
+  }
+
+  matches(doc: Document): boolean {
+    if (this.isConjunction()) {
+      // For conjunctions, all filters must match, so return false if any filter doesn't match.
+      return this.filters.find(filter => !filter.matches(doc)) === undefined;
+    } else {
+      // For disjunctions, at least one filter should match.
+      return this.filters.find(filter => filter.matches(doc)) !== undefined;
+    }
+  }
+
+  public getFlattenedFilters(): FieldFilter[] {
+    // TODO(orquery): memoize this result if this method is used more than once
+    let result: FieldFilter[] = [];
+    result = this.filters.reduce((result, subfilter) => {
+      return result.concat(subfilter.getFlattenedFilters());
+    }, result);
+    return result;
+  }
+
+  public getFilters(): Filter[] {
+    return this.filters;
+  }
+
+  public getOperator(): CompositeOperator {
+    return this.op;
+  }
+
+  public getFirstInequalityField(): FieldPath | null {
+    let found = this.findFirstMatchingFilter(filter => filter.isInequality());
+
+    if (found !== null) {
+      return found.field;
+    }
+    return null;
+  }
+
+  // Performs a depth-first search to find and return the first FieldFilter in the composite filter
+  // that satisfies the predicate. Returns `null` if none of the FieldFilters satisfy the
+  // predicate.
+  private findFirstMatchingFilter(
+    predicate: (filter: FieldFilter) => boolean
+  ): FieldFilter | null {
+    for (const filter of this.filters) {
+      if (filter instanceof FieldFilter && predicate(filter)) {
+        return filter as FieldFilter;
+      } else if (filter instanceof CompositeFilter) {
+        const found = (filter as CompositeFilter).findFirstMatchingFilter(
+          predicate
+        );
+        if (found !== null) {
+          return found;
+        }
+      }
+    }
+
+    return null;
+  }
+
+  public isConjunction(): boolean {
+    return this.op === CompositeOperator.AND;
+  }
 }
 
 export function canonifyFilter(filter: Filter): string {
   debugAssert(
-    filter instanceof FieldFilter,
-    'canonifyFilter() only supports FieldFilters'
-  );
-  // TODO(b/29183165): Technically, this won't be unique if two values have
-  // the same description, such as the int 3 and the string "3". So we should
-  // add the types in here somehow, too.
-  return (
-    filter.field.canonicalString() +
-    filter.op.toString() +
-    canonicalId(filter.value)
+    filter instanceof FieldFilter || filter instanceof CompositeFilter,
+    'canonifyFilter() only supports FieldFilters and CompositeFilters'
   );
+
+  if (filter instanceof FieldFilter) {
+    // TODO(b/29183165): Technically, this won't be unique if two values have
+    // the same description, such as the int 3 and the string "3". So we should
+    // add the types in here somehow, too.
+    return (
+      filter.field.canonicalString() +
+      filter.op.toString() +
+      canonicalId(filter.value)
+    );
+  } else {
+    // filter instanceof CompositeFilter
+    const canonicalIdsString = filter.filters
+      .map(filter => canonifyFilter(filter))
+      .join(',');
+    const opString = filter.isConjunction() ? 'and' : 'or';
+    return `${opString}(${canonicalIdsString})`;
+  }
 }
 
 export function filterEquals(f1: Filter, f2: Filter): boolean {
diff --git a/packages/firestore/src/lite-api/query.ts b/packages/firestore/src/lite-api/query.ts
index 0a762918b33..c075cdeba63 100644
--- a/packages/firestore/src/lite-api/query.ts
+++ b/packages/firestore/src/lite-api/query.ts
@@ -19,7 +19,6 @@ import { getModularInstance } from '@firebase/util';
 
 import { DatabaseId } from '../core/database_info';
 import {
-  findFilterOperator,
   getFirstOrderByField,
   getInequalityFilterField,
   isCollectionGroupQuery,
@@ -38,7 +37,9 @@ import {
   FieldFilter,
   Filter,
   Operator,
-  OrderBy
+  CompositeOperator,
+  OrderBy,
+  CompositeFilter
 } from '../core/target';
 import { Document } from '../model/document';
 import { DocumentKey } from '../model/document_key';
@@ -85,7 +86,9 @@ export type QueryConstraintType =
   | 'startAt'
   | 'startAfter'
   | 'endAt'
-  | 'endBefore';
+  | 'endBefore'
+  | 'and'
+  | 'or';
 
 /**
  * A `QueryConstraint` is used to narrow the set of documents returned by a
@@ -137,6 +140,16 @@ class QueryFilterConstraint extends QueryConstraint {
   }
 
   _apply<T>(query: Query<T>): Query<T> {
+    const filter = this._parse(query);
+    validateNewFieldFilter(query._query, filter);
+    return new Query(
+      query.firestore,
+      query.converter,
+      queryWithAddedFilter(query._query, filter)
+    );
+  }
+
+  _parse<T>(query: Query<T>): FieldFilter {
     const reader = newUserDataReader(query.firestore);
     const filter = newQueryFilter(
       query._query,
@@ -147,11 +160,7 @@ class QueryFilterConstraint extends QueryConstraint {
       this._op,
       this._value
     );
-    return new Query(
-      query.firestore,
-      query.converter,
-      queryWithAddedFilter(query._query, filter)
-    );
+    return filter;
   }
 }
 
@@ -181,7 +190,7 @@ export type WhereFilterOp =
  * @param opStr - The operation string (e.g "&lt;", "&lt;=", "==", "&lt;",
  *   "&lt;=", "!=").
  * @param value - The value for comparison
- * @returns The created {@link Query}.
+ * @returns The created {@link QueryConstraint}.
  */
 export function where(
   fieldPath: string | FieldPath,
@@ -193,6 +202,92 @@ export function where(
   return new QueryFilterConstraint(field, op, value);
 }
 
+class QueryCompositeFilterConstraint extends QueryConstraint {
+  constructor(
+    readonly type: 'or' | 'and',
+    private readonly _queryConstraints: QueryFilterConstraint[]
+  ) {
+    super();
+  }
+
+  _parse<T>(query: Query<T>): Filter {
+    const parsedFilters = this._queryConstraints
+      .map(queryConstraint => {
+        return queryConstraint._parse(query);
+      })
+      .filter(parsedFilter => parsedFilter.getFilters().length > 0);
+
+    if (parsedFilters.length == 1) {
+      return parsedFilters[0];
+    }
+
+    return CompositeFilter.create(parsedFilters, this.getOperator());
+  }
+
+  _apply<T>(query: Query<T>): Query<T> {
+    const parsedFilter = this._parse(query);
+    if (parsedFilter.getFilters().length === 0) {
+      // Return the existing query if not adding any more filters (e.g. an empty composite filter).
+      return query;
+    }
+    validateNewFilter(query._query, parsedFilter);
+
+    return new Query(
+      query.firestore,
+      query.converter,
+      queryWithAddedFilter(query._query, parsedFilter)
+    );
+  }
+
+  public getQueryConstraints(): QueryConstraint[] {
+    return this._queryConstraints;
+  }
+
+  public getOperator(): CompositeOperator {
+    return this.type == 'and' ? CompositeOperator.AND : CompositeOperator.OR;
+  }
+}
+
+/**
+ * Creates a {@link QueryConstraint} that performs a logical OR
+ * of all the provided `queryConstraints`
+ *
+ * @param queryConstraints - Optional. The {@link queryConstraints} for OR operation. These must be
+ * created with calls to {@link where}, {@link or}, or {@link and}.
+ * @returns The created {@link QueryConstraint}.
+ */
+export function or(...queryConstraints: QueryConstraint[]): QueryConstraint {
+  // Only support QueryFilterConstraints
+  queryConstraints.forEach(queryConstraint =>
+    validateQueryFilterConstraint('or', queryConstraint)
+  );
+
+  return new QueryCompositeFilterConstraint(
+    CompositeOperator.OR,
+    queryConstraints as QueryFilterConstraint[]
+  );
+}
+
+/**
+ * Creates a {@link QueryConstraint} that performs a logical AND
+ * of all the provided `queryConstraints`
+ *
+ * @param queryConstraints - Optional. The {@link queryConstraints} for AND operation. These must be
+ * created with calls to {@link where}, {@link or}, or {@link and}.
+ * @returns The created {@link QueryConstraint}.
+ */
+export function and(...queryConstraints: QueryConstraint[]): QueryConstraint {
+  // Only support QueryFilterConstraints
+  queryConstraints.forEach(queryConstraint =>
+    validateQueryFilterConstraint('andQuery', queryConstraint)
+  );
+
+  return new QueryCompositeFilterConstraint(
+    CompositeOperator.AND,
+    queryConstraints as QueryFilterConstraint[]
+  );
+}
+
 class QueryOrderByConstraint extends QueryConstraint {
   readonly type = 'orderBy';
 
@@ -518,7 +613,6 @@ export function newQueryFilter(
     );
   }
   const filter = FieldFilter.create(fieldPath, op, fieldValue);
-  validateNewFilter(query, filter);
   return filter;
 }
 
@@ -792,46 +886,86 @@ function conflictingOps(op: Operator): Operator[] {
   }
 }
 
-function validateNewFilter(query: InternalQuery, filter: Filter): void {
-  debugAssert(filter instanceof FieldFilter, 'Only FieldFilters are supported');
+function validateNewFieldFilter(
+  query: InternalQuery,
+  fieldFilter: FieldFilter
+): void {
+  const filterOp = fieldFilter.op;
 
-  if (filter.isInequality()) {
-    const existingField = getInequalityFilterField(query);
-    if (existingField !== null && !existingField.isEqual(filter.field)) {
+  if (fieldFilter.isInequality()) {
+    const existingInequality = getInequalityFilterField(query);
+    const newInequality = fieldFilter.field;
+
+    if (
+      existingInequality !== null &&
+      !existingInequality.isEqual(newInequality)
+    ) {
       throw new FirestoreError(
         Code.INVALID_ARGUMENT,
         'Invalid query. All where filters with an inequality' +
           ' (<, <=, !=, not-in, >, or >=) must be on the same field. But you have' +
-          ` inequality filters on '${existingField.toString()}'` +
-          ` and '${filter.field.toString()}'`
+          ` inequality filters on '${existingInequality.toString()}'` +
+          ` and '${newInequality.toString()}'`
       );
     }
 
     const firstOrderByField = getFirstOrderByField(query);
     if (firstOrderByField !== null) {
-      validateOrderByAndInequalityMatch(query, filter.field, firstOrderByField);
+      validateOrderByAndInequalityMatch(
+        query,
+        newInequality,
+        firstOrderByField
+      );
     }
   }
 
-  const conflictingOp = findFilterOperator(query, conflictingOps(filter.op));
+  const conflictingOp = findOpInsideFilters(
+    query.filters,
+    conflictingOps(fieldFilter.op)
+  );
   if (conflictingOp !== null) {
     // Special case when it's a duplicate op to give a slightly clearer error message.
-    if (conflictingOp === filter.op) {
+    if (conflictingOp === fieldFilter.op) {
       throw new FirestoreError(
         Code.INVALID_ARGUMENT,
         'Invalid query. You cannot use more than one ' +
-          `'${filter.op.toString()}' filter.`
+          `'${fieldFilter.op.toString()}' filter.`
       );
     } else {
       throw new FirestoreError(
         Code.INVALID_ARGUMENT,
-        `Invalid query. You cannot use '${filter.op.toString()}' filters ` +
+        `Invalid query. You cannot use '${fieldFilter.op.toString()}' filters ` +
           `with '${conflictingOp.toString()}' filters.`
       );
     }
   }
 }
 
+function validateNewFilter(query: InternalQuery, filter: Filter): void {
+  let testQuery = query;
+  const subFilters = filter.getFlattenedFilters();
+  for (let subFilter of subFilters) {
+    validateNewFieldFilter(testQuery, subFilter);
+    testQuery = queryWithAddedFilter(testQuery, subFilter);
+  }
+}
+
+// Checks if any of the provided filter operators are included in the given list of filters and
+// returns the first one that is, or null if none are.
+function findOpInsideFilters(
+  filters: Filter[],
+  operators: Operator[]
+): Operator | null {
+  for (const filter of filters) {
+    for (const fieldFilter of filter.getFlattenedFilters()) {
+      if (operators.indexOf(fieldFilter.op) >= 0) {
+        return fieldFilter.op;
+      }
+    }
+  }
+  return null;
+}
+
 function validateNewOrderBy(query: InternalQuery, orderBy: OrderBy): void {
   if (getFirstOrderByField(query) === null) {
     // This is the first order by. It must match any inequality.
@@ -858,3 +992,15 @@ function validateOrderByAndInequalityMatch(
     );
   }
 }
+
+export function validateQueryFilterConstraint(
+  functionName: string,
+  queryConstraint: QueryConstraint
+): void {
+  if (!(queryConstraint instanceof QueryFilterConstraint)) {
+    throw new FirestoreError(
+      Code.INVALID_ARGUMENT,
+      `Function ${functionName}() requires QueryContraints created with a call to 'where(...)'.`
+    );
+  }
+}
diff --git a/packages/firestore/src/local/query_engine.ts b/packages/firestore/src/local/query_engine.ts
index f6b89aed66d..d968155363c 100644
--- a/packages/firestore/src/local/query_engine.ts
+++ b/packages/firestore/src/local/query_engine.ts
@@ -19,6 +19,7 @@ import {
   LimitType,
   newQueryComparator,
   Query,
+  queryContainsCompositeFilters,
   queryMatches,
   queryMatchesAllDocuments,
   queryToTarget,
@@ -138,7 +139,10 @@ export class QueryEngine {
       return PersistencePromise.resolve<DocumentMap | null>(null);
     }
 
-    if (queryMatchesAllDocuments(query)) {
+    if (
+      queryMatchesAllDocuments(query) ||
+      queryContainsCompositeFilters(query)
+    ) {
       // Queries that match all documents don't benefit from using
       // key-based lookups. It is more efficient to scan all documents in a
       // collection, rather than to perform individual lookups.
@@ -227,7 +231,10 @@ export class QueryEngine {
     remoteKeys: DocumentKeySet,
     lastLimboFreeSnapshotVersion: SnapshotVersion
   ): PersistencePromise<DocumentMap> {
-    if (queryMatchesAllDocuments(query)) {
+    if (
+      queryMatchesAllDocuments(query) ||
+      queryContainsCompositeFilters(query)
+    ) {
       // Queries that match all documents don't benefit from using
       // key-based lookups. It is more efficient to scan all documents in a
       // collection, rather than to perform individual lookups.

From eba3b84a6c8ee8e7bf93d009d8290bf2d1094fea Mon Sep 17 00:00:00 2001
From: Mark Duckworth <markduckworth@google.com>
Date: Fri, 24 Jun 2022 15:16:39 -0600
Subject: [PATCH 2/5] Cleanup: lint

---
 packages/firestore/src/core/query.ts     |  6 ++----
 packages/firestore/src/core/target.ts    | 18 +++++++++---------
 packages/firestore/src/lite-api/query.ts | 13 +++++--------
 3 files changed, 16 insertions(+), 21 deletions(-)

diff --git a/packages/firestore/src/core/query.ts b/packages/firestore/src/core/query.ts
index 90a55d04979..eeabb93fb9b 100644
--- a/packages/firestore/src/core/query.ts
+++ b/packages/firestore/src/core/query.ts
@@ -24,10 +24,8 @@ import {
   Bound,
   canonifyTarget,
   Direction,
-  FieldFilter,
   Filter,
   newTarget,
-  Operator,
   OrderBy,
   boundSortsBeforeDocument,
   stringifyTarget,
@@ -323,8 +321,8 @@ export function queryToTarget(query: Query): Target {
 }
 
 export function queryWithAddedFilter(query: Query, filter: Filter): Query {
-  let newInequalityField = filter.getFirstInequalityField();
-  let queryInequalityField = getInequalityFilterField(query);
+  const newInequalityField = filter.getFirstInequalityField();
+  const queryInequalityField = getInequalityFilterField(query);
 
   debugAssert(
     queryInequalityField == null ||
diff --git a/packages/firestore/src/core/target.ts b/packages/firestore/src/core/target.ts
index d5bb0e7f93e..e58ac68e166 100644
--- a/packages/firestore/src/core/target.ts
+++ b/packages/firestore/src/core/target.ts
@@ -697,15 +697,15 @@ export class FieldFilter extends Filter {
     );
   }
 
-  public getFlattenedFilters(): FieldFilter[] {
+  getFlattenedFilters(): FieldFilter[] {
     return [this];
   }
 
-  public getFilters(): Filter[] {
+  getFilters(): Filter[] {
     return [this];
   }
 
-  public getFirstInequalityField(): FieldPath | null {
+  getFirstInequalityField(): FieldPath | null {
     if (this.isInequality()) {
       return this.field;
     }
@@ -738,7 +738,7 @@ export class CompositeFilter extends Filter {
     }
   }
 
-  public getFlattenedFilters(): FieldFilter[] {
+  getFlattenedFilters(): FieldFilter[] {
     // TODO(orquery): memoize this result if this method is used more than once
     let result: FieldFilter[] = [];
     result = this.filters.reduce((result, subfilter) => {
@@ -747,16 +747,16 @@ export class CompositeFilter extends Filter {
     return result;
   }
 
-  public getFilters(): Filter[] {
+  getFilters(): Filter[] {
     return this.filters;
   }
 
-  public getOperator(): CompositeOperator {
+  getOperator(): CompositeOperator {
     return this.op;
   }
 
-  public getFirstInequalityField(): FieldPath | null {
-    let found = this.findFirstMatchingFilter(filter => filter.isInequality());
+  getFirstInequalityField(): FieldPath | null {
+    const found = this.findFirstMatchingFilter(filter => filter.isInequality());
 
     if (found !== null) {
       return found.field;
@@ -786,7 +786,7 @@ export class CompositeFilter extends Filter {
     return null;
   }
 
-  public isConjunction(): boolean {
+  isConjunction(): boolean {
     return this.op === CompositeOperator.AND;
   }
 }
diff --git a/packages/firestore/src/lite-api/query.ts b/packages/firestore/src/lite-api/query.ts
index c075cdeba63..1b84fab234e 100644
--- a/packages/firestore/src/lite-api/query.ts
+++ b/packages/firestore/src/lite-api/query.ts
@@ -47,7 +47,6 @@ import { FieldPath as InternalFieldPath, ResourcePath } from '../model/path';
 import { isServerTimestamp } from '../model/server_timestamps';
 import { refValue } from '../model/values';
 import { Value as ProtoValue } from '../protos/firestore_proto_api';
-import { debugAssert } from '../util/assert';
 import { Code, FirestoreError } from '../util/error';
 import {
   validatePositiveNumber,
@@ -217,7 +216,7 @@ class QueryCompositeFilterConstraint extends QueryConstraint {
       })
       .filter(parsedFilter => parsedFilter.getFilters().length > 0);
 
-    if (parsedFilters.length == 1) {
+    if (parsedFilters.length === 1) {
       return parsedFilters[0];
     }
 
@@ -239,12 +238,12 @@ class QueryCompositeFilterConstraint extends QueryConstraint {
     );
   }
 
-  public getQueryConstraints(): QueryConstraint[] {
+  getQueryConstraints(): QueryConstraint[] {
     return this._queryConstraints;
   }
 
-  public getOperator(): CompositeOperator {
-    return this.type == 'and' ? CompositeOperator.AND : CompositeOperator.OR;
+  getOperator(): CompositeOperator {
+    return this.type === 'and' ? CompositeOperator.AND : CompositeOperator.OR;
   }
 }
 
@@ -890,8 +889,6 @@ function validateNewFieldFilter(
   query: InternalQuery,
   fieldFilter: FieldFilter
 ): void {
-  const filterOp = fieldFilter.op;
-
   if (fieldFilter.isInequality()) {
     const existingInequality = getInequalityFilterField(query);
     const newInequality = fieldFilter.field;
@@ -944,7 +941,7 @@ function validateNewFieldFilter(
 function validateNewFilter(query: InternalQuery, filter: Filter): void {
   let testQuery = query;
   const subFilters = filter.getFlattenedFilters();
-  for (let subFilter of subFilters) {
+  for (const subFilter of subFilters) {
     validateNewFieldFilter(testQuery, subFilter);
     testQuery = queryWithAddedFilter(testQuery, subFilter);
   }

From 4967b9e57fe61a4bde093393a52197ad71858b5a Mon Sep 17 00:00:00 2001
From: Mark Duckworth <markduckworth@google.com>
Date: Wed, 6 Jul 2022 09:46:46 -0600
Subject: [PATCH 3/5] Addressing code review comments.

---
 packages/firestore/src/core/target.ts    | 58 +++++++++++-------------
 packages/firestore/src/lite-api/query.ts |  2 +-
 2 files changed, 27 insertions(+), 33 deletions(-)

diff --git a/packages/firestore/src/core/target.ts b/packages/firestore/src/core/target.ts
index e58ac68e166..8f6a78fbb9a 100644
--- a/packages/firestore/src/core/target.ts
+++ b/packages/firestore/src/core/target.ts
@@ -537,9 +537,9 @@ export function targetGetSegmentCount(target: Target): number {
 export abstract class Filter {
   abstract matches(doc: Document): boolean;
 
-  abstract getFlattenedFilters(): FieldFilter[];
+  abstract getFlattenedFilters(): readonly FieldFilter[];
 
-  abstract getFilters(): Filter[];
+  abstract getFilters(): readonly Filter[];
 
   abstract getFirstInequalityField(): FieldPath | null;
 }
@@ -570,11 +570,12 @@ export const enum Direction {
   DESCENDING = 'desc'
 }
 
+// TODO(orquery) move Filter classes to a new file, e.g. filter.ts
 export class FieldFilter extends Filter {
   protected constructor(
-    public field: FieldPath,
-    public op: Operator,
-    public value: ProtoValue
+    public readonly field: FieldPath,
+    public readonly op: Operator,
+    public readonly value: ProtoValue
   ) {
     super();
   }
@@ -697,11 +698,11 @@ export class FieldFilter extends Filter {
     );
   }
 
-  getFlattenedFilters(): FieldFilter[] {
+  getFlattenedFilters(): readonly FieldFilter[] {
     return [this];
   }
 
-  getFilters(): Filter[] {
+  getFilters(): readonly Filter[] {
     return [this];
   }
 
@@ -714,9 +715,11 @@ export class FieldFilter extends Filter {
 }
 
 export class CompositeFilter extends Filter {
+  private memoizedFlattenedFilters: FieldFilter[] | null = null;
+
   protected constructor(
-    public filters: Filter[],
-    public op: CompositeOperator
+    public readonly filters: readonly Filter[],
+    public readonly op: CompositeOperator
   ) {
     super();
   }
@@ -738,21 +741,20 @@ export class CompositeFilter extends Filter {
     }
   }
 
-  getFlattenedFilters(): FieldFilter[] {
-    // TODO(orquery): memoize this result if this method is used more than once
-    let result: FieldFilter[] = [];
-    result = this.filters.reduce((result, subfilter) => {
+  getFlattenedFilters(): readonly FieldFilter[] {
+    if (this.memoizedFlattenedFilters !== null) {
+      return this.memoizedFlattenedFilters;
+    }
+
+    this.memoizedFlattenedFilters = this.filters.reduce((result, subfilter) => {
       return result.concat(subfilter.getFlattenedFilters());
-    }, result);
-    return result;
-  }
+    }, [] as FieldFilter[]);
 
-  getFilters(): Filter[] {
-    return this.filters;
+    return this.memoizedFlattenedFilters;
   }
 
-  getOperator(): CompositeOperator {
-    return this.op;
+  getFilters(): readonly Filter[] {
+    return this.filters;
   }
 
   getFirstInequalityField(): FieldPath | null {
@@ -770,16 +772,9 @@ export class CompositeFilter extends Filter {
   private findFirstMatchingFilter(
     predicate: (filter: FieldFilter) => boolean
   ): FieldFilter | null {
-    for (const filter of this.filters) {
-      if (filter instanceof FieldFilter && predicate(filter)) {
-        return filter as FieldFilter;
-      } else if (filter instanceof CompositeFilter) {
-        const found = (filter as CompositeFilter).findFirstMatchingFilter(
-          predicate
-        );
-        if (found !== null) {
-          return found;
-        }
+    for (const fieldFilter of this.getFlattenedFilters()) {
+      if (predicate(fieldFilter)) {
+        return fieldFilter;
       }
     }
 
@@ -811,8 +806,7 @@ export function canonifyFilter(filter: Filter): string {
     const canonicalIdsString = filter.filters
       .map(filter => canonifyFilter(filter))
       .join(',');
-    const opString = filter.isConjunction() ? 'and' : 'or';
-    return `${opString}(${canonicalIdsString})`;
+    return `${filter.op}(${canonicalIdsString})`;
   }
 }
 
diff --git a/packages/firestore/src/lite-api/query.ts b/packages/firestore/src/lite-api/query.ts
index 1b84fab234e..14b657d923f 100644
--- a/packages/firestore/src/lite-api/query.ts
+++ b/packages/firestore/src/lite-api/query.ts
@@ -278,7 +278,7 @@ export function or(...queryConstraints: QueryConstraint[]): QueryConstraint {
 export function and(...queryConstraints: QueryConstraint[]): QueryConstraint {
   // Only support QueryFilterConstraints
   queryConstraints.forEach(queryConstraint =>
-    validateQueryFilterConstraint('andQuery', queryConstraint)
+    validateQueryFilterConstraint('and', queryConstraint)
   );
 
   return new QueryCompositeFilterConstraint(

From 06236afdb9c47d4cfada26cea17283e80b11d79d Mon Sep 17 00:00:00 2001
From: Mark Duckworth <markduckworth@google.com>
Date: Mon, 18 Jul 2022 10:47:31 -0400
Subject: [PATCH 4/5] This change set does not require a release.

---
 .changeset/strong-starfishes-deny.md | 2 ++
 1 file changed, 2 insertions(+)
 create mode 100644 .changeset/strong-starfishes-deny.md

diff --git a/.changeset/strong-starfishes-deny.md b/.changeset/strong-starfishes-deny.md
new file mode 100644
index 00000000000..a845151cc84
--- /dev/null
+++ b/.changeset/strong-starfishes-deny.md
@@ -0,0 +1,2 @@
+---
+---

From ed6a600a866f3b69799ee2d8739af0b1c341b7ce Mon Sep 17 00:00:00 2001
From: Mark Duckworth <markduckworth@google.com>
Date: Mon, 25 Jul 2022 15:47:34 -0600
Subject: [PATCH 5/5] Revert "This change set does not require a release."

This reverts commit 06236afdb9c47d4cfada26cea17283e80b11d79d.
---
 .changeset/strong-starfishes-deny.md | 2 --
 1 file changed, 2 deletions(-)
 delete mode 100644 .changeset/strong-starfishes-deny.md

diff --git a/.changeset/strong-starfishes-deny.md b/.changeset/strong-starfishes-deny.md
deleted file mode 100644
index a845151cc84..00000000000
--- a/.changeset/strong-starfishes-deny.md
+++ /dev/null
@@ -1,2 +0,0 @@
----
----