OR queries more validation

common/api-review/firestore-lite.api.md

@@ -17,9 +17,6 @@ export type AddPrefixToKeys<Prefix extends string, T extends Record<string, unkn
     [K in keyof T & string as `${Prefix}.${K}`]+?: T[K];
 };
 
-// @public
-export function and(...queryConstraints: QueryFilterConstraint[]): QueryCompositeFilterConstraint;
-
 // @public
 export function arrayRemove(...elements: unknown[]): FieldValue;
 
@@ -203,9 +200,6 @@ export type NestedUpdateFields<T extends Record<string, unknown>> = UnionToInter
     [K in keyof T & string]: ChildUpdateFields<K, T[K]>;
 }[keyof T & string]>;
 
-// @public
-export function or(...queryConstraints: QueryFilterConstraint[]): QueryCompositeFilterConstraint;
-
 // @public
 export function orderBy(fieldPath: string | FieldPath, directionStr?: OrderByDirection): QueryOrderByConstraint;
 
@@ -230,17 +224,9 @@ export class Query<T = DocumentData> {
     withConverter<U>(converter: FirestoreDataConverter<U>): Query<U>;
 }
 
-// @public
-export function query<T>(query: Query<T>, compositeFilter: QueryCompositeFilterConstraint, ...queryConstraints: QueryNonFilterConstraint[]): Query<T>;
-
 // @public
 export function query<T>(query: Query<T>, ...queryConstraints: QueryConstraint[]): Query<T>;
 
-// @public
-export class QueryCompositeFilterConstraint {
-    readonly type: 'or' | 'and';
-}
-
 // @public
 export abstract class QueryConstraint {
     abstract readonly type: QueryConstraintType;
@@ -268,9 +254,6 @@ export class QueryFieldFilterConstraint extends QueryConstraint {
     readonly type = "where";
 }
 
-// @public
-export type QueryFilterConstraint = QueryFieldFilterConstraint | QueryCompositeFilterConstraint;
-
 // @public
 export class QueryLimitConstraint extends QueryConstraint {
     readonly type: 'limit' | 'limitToLast';

common/api-review/firestore.api.md

@@ -17,9 +17,6 @@ export type AddPrefixToKeys<Prefix extends string, T extends Record<string, unkn
     [K in keyof T & string as `${Prefix}.${K}`]+?: T[K];
 };
 
-// @public
-export function and(...queryConstraints: QueryFilterConstraint[]): QueryCompositeFilterConstraint;
-
 // @public
 export function arrayRemove(...elements: unknown[]): FieldValue;
 
@@ -327,9 +324,6 @@ export function onSnapshotsInSync(firestore: Firestore, observer: {
 // @public
 export function onSnapshotsInSync(firestore: Firestore, onSync: () => void): Unsubscribe;
 
-// @public
-export function or(...queryConstraints: QueryFilterConstraint[]): QueryCompositeFilterConstraint;
-
 // @public
 export function orderBy(fieldPath: string | FieldPath, directionStr?: OrderByDirection): QueryOrderByConstraint;
 
@@ -359,17 +353,9 @@ export class Query<T = DocumentData> {
     withConverter<U>(converter: FirestoreDataConverter<U>): Query<U>;
 }
 
-// @public
-export function query<T>(query: Query<T>, compositeFilter: QueryCompositeFilterConstraint, ...queryConstraints: QueryNonFilterConstraint[]): Query<T>;
-
 // @public
 export function query<T>(query: Query<T>, ...queryConstraints: QueryConstraint[]): Query<T>;
 
-// @public
-export class QueryCompositeFilterConstraint {
-    readonly type: 'or' | 'and';
-}
-
 // @public
 export abstract class QueryConstraint {
     abstract readonly type: QueryConstraintType;
@@ -397,9 +383,6 @@ export class QueryFieldFilterConstraint extends QueryConstraint {
     readonly type = "where";
 }
 
-// @public
-export type QueryFilterConstraint = QueryFieldFilterConstraint | QueryCompositeFilterConstraint;
-
 // @public
 export class QueryLimitConstraint extends QueryConstraint {
     readonly type: 'limit' | 'limitToLast';

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

@@ -130,6 +130,7 @@ export abstract class QueryConstraint extends AppliableConstraint {
  * apply (e.g. {@link orderBy}, {@link limit}).
  * @throws if any of the provided query constraints cannot be combined with the
  * existing or new constraints.
+ * @internal TODO remove this internal tag with OR Query support in the server
  */
 export function query<T>(
   query: Query<T>,
@@ -154,24 +155,21 @@ export function query<T>(
 
 export function query<T>(
   query: Query<T>,
-  // eslint-disable-next-line @typescript-eslint/no-explicit-any
-  filterOrQueryConstraints: any,
-  // eslint-disable-next-line @typescript-eslint/no-explicit-any
-  ...nonFilters: any
+  queryConstraint: QueryCompositeFilterConstraint | QueryConstraint | undefined,
+  ...additionalQueryConstraints: Array<
+    QueryConstraint | QueryNonFilterConstraint
+  >
 ): Query<T> {
   let queryConstraints: AppliableConstraint[] = [];
-  if (filterOrQueryConstraints instanceof AppliableConstraint) {
-    queryConstraints.push(filterOrQueryConstraints);
-  } else if (Array.isArray(filterOrQueryConstraints)) {
-    queryConstraints.concat(filterOrQueryConstraints as AppliableConstraint[]);
-  }
 
-  if (nonFilters !== undefined) {
-    queryConstraints = queryConstraints.concat(
-      nonFilters as QueryNonFilterConstraint[]
-    );
+  if (queryConstraint instanceof AppliableConstraint) {
+    queryConstraints.push(queryConstraint);
   }
 
+  queryConstraints = queryConstraints.concat(additionalQueryConstraints);
+
+  validateQueryConstraintArray(queryConstraints);
+
   for (const constraint of queryConstraints) {
     query = constraint._apply(query);
   }
@@ -278,6 +276,7 @@ export function where(
  * `QueryCompositeFilterConstraint`s are created by invoking {@link or} or
  * {@link and} and can then be passed to {@link query} to create a new query
  * instance that also contains the `QueryCompositeFilterConstraint`.
+ * @internal TODO remove this internal tag with OR Query support in the server
  */
 export class QueryCompositeFilterConstraint extends AppliableConstraint {
   /**
@@ -358,6 +357,7 @@ export type QueryNonFilterConstraint =
  * `QueryFilterConstraint`s are created by invoking {@link or} or {@link and}
  * and can then be passed to {@link query} to create a new query instance that
  * also contains the `QueryConstraint`.
+ * @internal TODO remove this internal tag with OR Query support in the server
  */
 export type QueryFilterConstraint =
   | QueryFieldFilterConstraint
@@ -371,6 +371,7 @@ export type QueryFilterConstraint =
  * for OR operation. These must be created with calls to {@link where},
  * {@link or}, or {@link and}.
  * @returns The created {@link QueryCompositeFilterConstraint}.
+ * @internal TODO remove this internal tag with OR Query support in the server
  */
 export function or(
   ...queryConstraints: QueryFilterConstraint[]
@@ -394,6 +395,7 @@ export function or(
  * for AND operation. These must be created with calls to {@link where},
  * {@link or}, or {@link and}.
  * @returns The created {@link QueryCompositeFilterConstraint}.
+ * @internal TODO remove this internal tag with OR Query support in the server
  */
 export function and(
   ...queryConstraints: QueryFilterConstraint[]
@@ -1214,7 +1216,32 @@ export function validateQueryFilterConstraint(
   ) {
     throw new FirestoreError(
       Code.INVALID_ARGUMENT,
-      `Function ${functionName}() requires AppliableContraints created with a call to 'where(...)', 'or(...)', or 'and(...)'.`
+      `Function ${functionName}() requires AppliableConstraints created with a call to 'where(...)', 'or(...)', or 'and(...)'.`
+    );
+  }
+}
+
+function validateQueryConstraintArray(
+  queryConstraint: AppliableConstraint[]
+): void {
+  const compositeFilterCount = queryConstraint.filter(
+    filter => filter instanceof QueryCompositeFilterConstraint
+  ).length;
+  const fieldFilterCount = queryConstraint.filter(
+    filter => filter instanceof QueryFieldFilterConstraint
+  ).length;
+
+  if (
+    compositeFilterCount > 1 ||
+    (compositeFilterCount > 0 && fieldFilterCount > 0)
+  ) {
+    throw new FirestoreError(
+      Code.INVALID_ARGUMENT,
+      'InvalidQuery. When using composite filters, you cannot use ' +
+        'more than one filter at the top level. Consider nesting the multiple ' +
+        'filters within an `and(...)` statement. For example: ' +
+        'change `query(query, where(...), or(...))` to ' +
+        '`query(query, and(where(...), or(...)))`.'
     );
   }
 }

packages/firestore/test/integration/api/validation.test.ts

@@ -1399,7 +1399,68 @@ apiDescribe('Validation:', (persistence: boolean) => {
       ).to.throw(
         "Invalid query. You cannot use 'not-in' filters with 'in' filters."
       );
+
+      // Multiple top level composite filters
+      expect(() =>
+        // @ts-ignore
+        query(coll, and(where('a', '==', 'b')), or(where('b', '==', 'a')))
+      ).to.throw(
+        'InvalidQuery. When using composite filters, you cannot use ' +
+          'more than one filter at the top level. Consider nesting the multiple ' +
+          'filters within an `and(...)` statement. For example: ' +
+          'change `query(query, where(...), or(...))` to ' +
+          '`query(query, and(where(...), or(...)))`.'
+      );
+
+      // Once top level composite filter and one top level field filter
+      expect(() =>
+        // @ts-ignore
+        query(coll, or(where('a', '==', 'b')), where('b', '==', 'a'))
+      ).to.throw(
+        'InvalidQuery. When using composite filters, you cannot use ' +
+          'more than one filter at the top level. Consider nesting the multiple ' +
+          'filters within an `and(...)` statement. For example: ' +
+          'change `query(query, where(...), or(...))` to ' +
+          '`query(query, and(where(...), or(...)))`.'
+      );
     });
+
+    validationIt(
+      persistence,
+      'passing non-filters to composite operators fails',
+      db => {
+        const compositeOperators = [
+          { name: 'or', func: or },
+          { name: 'and', func: and }
+        ];
+        const nonFilterOps = [
+          limit(1),
+          limitToLast(2),
+          startAt(1),
+          startAfter(1),
+          endAt(1),
+          endBefore(1),
+          orderBy('a')
+        ];
+
+        for (const compositeOp of compositeOperators) {
+          for (const nonFilterOp of nonFilterOps) {
+            const coll = collection(db, 'test');
+            expect(() =>
+              query(
+                coll,
+                compositeOp.func(
+                  // @ts-ignore
+                  nonFilterOp
+                )
+              )
+            ).to.throw(
+              `Function ${compositeOp.name}() requires AppliableConstraints created with a call to 'where(...)', 'or(...)', or 'and(...)'.`
+            );
+          }
+        }
+      }
+    );
   });
 });