Multiple inequality support (#7453)

Author: milaGGL

packages/firestore-compat/test/validation.test.ts

@@ -825,15 +825,11 @@ apiDescribe('Validation:', (persistence: boolean) => {
       }
     );
 
-    validationIt(persistence, 'with different inequality fields fail', db => {
+    validationIt(persistence, 'can have different inequality fields', db => {
       const collection = db.collection('test');
       expect(() =>
         collection.where('x', '>=', 32).where('y', '<', 'cat')
-      ).to.throw(
-        'Invalid query. All where filters with an ' +
-          'inequality (<, <=, !=, not-in, >, or >=) must be on the same field.' +
-          ` But you have inequality filters on 'x' and 'y'`
-      );
+      ).not.to.throw();
     });
 
     validationIt(persistence, 'with more than one != query fail', db => {
@@ -845,59 +841,46 @@ apiDescribe('Validation:', (persistence: boolean) => {
 
     validationIt(
       persistence,
-      'with != and inequality queries on different fields fail',
+      'can have != and inequality queries on different fields',
       db => {
         const collection = db.collection('test');
         expect(() =>
           collection.where('y', '>', 32).where('x', '!=', 33)
-        ).to.throw(
-          'Invalid query. All where filters with an ' +
-            'inequality (<, <=, !=, not-in, >, or >=) must be on the same field.' +
-            ` But you have inequality filters on 'y' and 'x`
-        );
+        ).not.to.throw();
       }
     );
 
     validationIt(
       persistence,
-      'with != and inequality queries on different fields fail',
+      'can have NOT-IN and inequality queries on different fields',
       db => {
         const collection = db.collection('test');
         expect(() =>
           collection.where('y', '>', 32).where('x', 'not-in', [33])
-        ).to.throw(
-          'Invalid query. All where filters with an ' +
-            'inequality (<, <=, !=, not-in, >, or >=) must be on the same field.' +
-            ` But you have inequality filters on 'y' and 'x`
-        );
+        ).not.to.throw();
       }
     );
 
     validationIt(
       persistence,
-      'with inequality different than first orderBy fail.',
+      'can have inequality different than first orderBy',
       db => {
         const collection = db.collection('test');
-        const reason =
-          `Invalid query. You have a where filter with an ` +
-          `inequality (<, <=, !=, not-in, >, or >=) on field 'x' and so you must also ` +
-          `use 'x' as your first argument to Query.orderBy(), but your first ` +
-          `orderBy() is on field 'y' instead.`;
-        expect(() => collection.where('x', '>', 32).orderBy('y')).to.throw(
-          reason
-        );
-        expect(() => collection.orderBy('y').where('x', '>', 32)).to.throw(
-          reason
-        );
+        expect(() =>
+          collection.where('x', '>', 32).orderBy('y')
+        ).not.to.throw();
+        expect(() =>
+          collection.orderBy('y').where('x', '>', 32)
+        ).not.to.throw();
         expect(() =>
           collection.where('x', '>', 32).orderBy('y').orderBy('x')
-        ).to.throw(reason);
+        ).not.to.throw();
         expect(() =>
           collection.orderBy('y').orderBy('x').where('x', '>', 32)
-        ).to.throw(reason);
-        expect(() => collection.where('x', '!=', 32).orderBy('y')).to.throw(
-          reason
-        );
+        ).not.to.throw();
+        expect(() =>
+          collection.where('x', '!=', 32).orderBy('y')
+        ).not.to.throw();
       }
     );
 

packages/firestore/src/core/filter.ts

@@ -56,8 +56,6 @@ export abstract class Filter {
   abstract getFlattenedFilters(): readonly FieldFilter[];
 
   abstract getFilters(): Filter[];
-
-  abstract getFirstInequalityField(): FieldPath | null;
 }
 
 export class FieldFilter extends Filter {
@@ -194,13 +192,6 @@ export class FieldFilter extends Filter {
   getFilters(): Filter[] {
     return [this];
   }
-
-  getFirstInequalityField(): FieldPath | null {
-    if (this.isInequality()) {
-      return this.field;
-    }
-    return null;
-  }
 }
 
 export class CompositeFilter extends Filter {
@@ -246,30 +237,6 @@ export class CompositeFilter extends Filter {
   getFilters(): Filter[] {
     return Object.assign([], this.filters);
   }
-
-  getFirstInequalityField(): FieldPath | null {
-    const 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 fieldFilter of this.getFlattenedFilters()) {
-      if (predicate(fieldFilter)) {
-        return fieldFilter;
-      }
-    }
-
-    return null;
-  }
 }
 
 export function compositeFilterIsConjunction(

packages/firestore/src/core/query.ts

@@ -19,13 +19,14 @@ import { compareDocumentsByField, Document } from '../model/document';
 import { DocumentKey } from '../model/document_key';
 import { FieldPath, ResourcePath } from '../model/path';
 import { debugAssert, debugCast, fail } from '../util/assert';
+import { SortedSet } from '../util/sorted_set';
 
 import {
   Bound,
   boundSortsAfterDocument,
   boundSortsBeforeDocument
 } from './bound';
-import { Filter } from './filter';
+import { FieldFilter, Filter } from './filter';
 import { Direction, OrderBy } from './order_by';
 import {
   canonifyTarget,
@@ -172,21 +173,18 @@ export function queryMatchesAllDocuments(query: Query): boolean {
   );
 }
 
-export function getFirstOrderByField(query: Query): FieldPath | null {
-  return query.explicitOrderBy.length > 0
-    ? query.explicitOrderBy[0].field
-    : null;
-}
-
-export function getInequalityFilterField(query: Query): FieldPath | null {
-  for (const filter of query.filters) {
-    const result = filter.getFirstInequalityField();
-    if (result !== null) {
-      return result;
-    }
-  }
-
-  return null;
+// Returns the sorted set of inequality filter fields used in this query.
+export function getInequalityFilterFields(query: Query): SortedSet<FieldPath> {
+  let result = new SortedSet<FieldPath>(FieldPath.comparator);
+  query.filters.forEach((filter: Filter) => {
+    const subFilters = filter.getFlattenedFilters();
+    subFilters.forEach((filter: FieldFilter) => {
+      if (filter.isInequality()) {
+        result = result.add(filter.field);
+      }
+    });
+  });
+  return result;
 }
 
 /**
@@ -228,45 +226,43 @@ export function queryNormalizedOrderBy(query: Query): OrderBy[] {
   const queryImpl = debugCast(query, QueryImpl);
   if (queryImpl.memoizedNormalizedOrderBy === null) {
     queryImpl.memoizedNormalizedOrderBy = [];
+    const fieldsNormalized = new Set<string>();
 
-    const inequalityField = getInequalityFilterField(queryImpl);
-    const firstOrderByField = getFirstOrderByField(queryImpl);
-    if (inequalityField !== null && firstOrderByField === null) {
-      // In order to implicitly add key ordering, we must also add the
-      // inequality filter field for it to be a valid query.
-      // Note that the default inequality field and key ordering is ascending.
-      if (!inequalityField.isKeyField()) {
-        queryImpl.memoizedNormalizedOrderBy.push(new OrderBy(inequalityField));
+    // Any explicit order by fields should be added as is.
+    for (const orderBy of queryImpl.explicitOrderBy) {
+      queryImpl.memoizedNormalizedOrderBy.push(orderBy);
+      fieldsNormalized.add(orderBy.field.canonicalString());
+    }
+
+    // The order of the implicit ordering always matches the last explicit order by.
+    const lastDirection =
+      queryImpl.explicitOrderBy.length > 0
+        ? queryImpl.explicitOrderBy[queryImpl.explicitOrderBy.length - 1].dir
+        : Direction.ASCENDING;
+
+    // Any inequality fields not explicitly ordered should be implicitly ordered in a lexicographical
+    // order. When there are multiple inequality filters on the same field, the field should be added
+    // only once.
+    // Note: `SortedSet<FieldPath>` sorts the key field before other fields. However, we want the key
+    // field to be sorted last.
+    const inequalityFields: SortedSet<FieldPath> =
+      getInequalityFilterFields(queryImpl);
+    inequalityFields.forEach(field => {
+      if (
+        !fieldsNormalized.has(field.canonicalString()) &&
+        !field.isKeyField()
+      ) {
+        queryImpl.memoizedNormalizedOrderBy!.push(
+          new OrderBy(field, lastDirection)
+        );
       }
+    });
+
+    // Add the document key field to the last if it is not explicitly ordered.
+    if (!fieldsNormalized.has(FieldPath.keyField().canonicalString())) {
       queryImpl.memoizedNormalizedOrderBy.push(
-        new OrderBy(FieldPath.keyField(), Direction.ASCENDING)
-      );
-    } else {
-      debugAssert(
-        inequalityField === null ||
-          (firstOrderByField !== null &&
-            inequalityField.isEqual(firstOrderByField)),
-        'First orderBy should match inequality field.'
+        new OrderBy(FieldPath.keyField(), lastDirection)
       );
-      let foundKeyOrdering = false;
-      for (const orderBy of queryImpl.explicitOrderBy) {
-        queryImpl.memoizedNormalizedOrderBy.push(orderBy);
-        if (orderBy.field.isKeyField()) {
-          foundKeyOrdering = true;
-        }
-      }
-      if (!foundKeyOrdering) {
-        // The order of the implicit key ordering always matches the last
-        // explicit order-by
-        const lastDirection =
-          queryImpl.explicitOrderBy.length > 0
-            ? queryImpl.explicitOrderBy[queryImpl.explicitOrderBy.length - 1]
-                .dir
-            : Direction.ASCENDING;
-        queryImpl.memoizedNormalizedOrderBy.push(
-          new OrderBy(FieldPath.keyField(), lastDirection)
-        );
-      }
     }
   }
   return queryImpl.memoizedNormalizedOrderBy;
@@ -350,16 +346,6 @@ function _queryToTarget(queryImpl: QueryImpl, orderBys: OrderBy[]): Target {
 }
 
 export function queryWithAddedFilter(query: Query, filter: Filter): Query {
-  const newInequalityField = filter.getFirstInequalityField();
-  const queryInequalityField = getInequalityFilterField(query);
-
-  debugAssert(
-    queryInequalityField == null ||
-      newInequalityField == null ||
-      newInequalityField.isEqual(queryInequalityField),
-    'Query must only have one inequality field.'
-  );
-
   debugAssert(
     !isDocumentQuery(query),
     'No filtering allowed for document query'

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

@@ -28,8 +28,6 @@ import {
 } from '../core/filter';
 import { Direction, OrderBy } from '../core/order_by';
 import {
-  getFirstOrderByField,
-  getInequalityFilterField,
   isCollectionGroupQuery,
   LimitType,
   Query as InternalQuery,
@@ -868,7 +866,6 @@ export function newQueryOrderBy(
     );
   }
   const orderBy = new OrderBy(fieldPath, direction);
-  validateNewOrderBy(query, orderBy);
   return orderBy;
 }
 
@@ -1100,33 +1097,6 @@ function validateNewFieldFilter(
   query: InternalQuery,
   fieldFilter: FieldFilter
 ): void {
-  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 '${existingInequality.toString()}'` +
-          ` and '${newInequality.toString()}'`
-      );
-    }
-
-    const firstOrderByField = getFirstOrderByField(query);
-    if (firstOrderByField !== null) {
-      validateOrderByAndInequalityMatch(
-        query,
-        newInequality,
-        firstOrderByField
-      );
-    }
-  }
-
   const conflictingOp = findOpInsideFilters(
     query.filters,
     conflictingOps(fieldFilter.op)
@@ -1174,33 +1144,6 @@ function findOpInsideFilters(
   return null;
 }
 
-function validateNewOrderBy(query: InternalQuery, orderBy: OrderBy): void {
-  if (getFirstOrderByField(query) === null) {
-    // This is the first order by. It must match any inequality.
-    const inequalityField = getInequalityFilterField(query);
-    if (inequalityField !== null) {
-      validateOrderByAndInequalityMatch(query, inequalityField, orderBy.field);
-    }
-  }
-}
-
-function validateOrderByAndInequalityMatch(
-  baseQuery: InternalQuery,
-  inequality: InternalFieldPath,
-  orderBy: InternalFieldPath
-): void {
-  if (!orderBy.isEqual(inequality)) {
-    throw new FirestoreError(
-      Code.INVALID_ARGUMENT,
-      `Invalid query. You have a where filter with an inequality ` +
-        `(<, <=, !=, not-in, >, or >=) on field '${inequality.toString()}' ` +
-        `and so you must also use '${inequality.toString()}' ` +
-        `as your first argument to orderBy(), but your first orderBy() ` +
-        `is on field '${orderBy.toString()}' instead.`
-    );
-  }
-}
-
 export function validateQueryFilterConstraint(
   functionName: string,
   queryConstraint: AppliableConstraint

packages/firestore/src/local/indexeddb_index_manager.ts

@@ -275,10 +275,10 @@ export class IndexedDbIndexManager implements IndexManager {
         return this.getIndexType(transaction, subTarget).next(type => {
           if (type === IndexType.NONE || type === IndexType.PARTIAL) {
             const targetIndexMatcher = new TargetIndexMatcher(subTarget);
-            return this.addFieldIndex(
-              transaction,
-              targetIndexMatcher.buildTargetIndex()
-            );
+            const fieldIndex = targetIndexMatcher.buildTargetIndex();
+            if (fieldIndex != null) {
+              return this.addFieldIndex(transaction, fieldIndex);
+            }
           }
         });
       }