Fix an issue where TargetIndexMatcher rejects valid queries (#7335)

firebase · May 30, 2023 · 60e8538 · 60e8538
1 parent e642fc7
commit 60e8538
Show file tree

Hide file tree

Showing 3 changed files with 155 additions and 11 deletions.
diff --git a/packages/firestore/src/model/target_index_matcher.ts b/packages/firestore/src/model/target_index_matcher.ts
@@ -118,18 +118,19 @@ export class TargetIndexMatcher {
     }
 
     const segments = fieldIndexGetDirectionalSegments(index);
+    let equalitySegments = new Set<string>();
     let segmentIndex = 0;
     let orderBysIndex = 0;
 
     // Process all equalities first. Equalities can appear out of order.
     for (; segmentIndex < segments.length; ++segmentIndex) {
       // We attempt to greedily match all segments to equality filters. If a
       // filter matches an index segment, we can mark the segment as used.
-      // Since it is not possible to use the same field path in both an equality
-      // and inequality/oderBy clause, we do not have to consider the possibility
-      // that a matching equality segment should instead be used to map to an
-      // inequality filter or orderBy clause.
-      if (!this.hasMatchingEqualityFilter(segments[segmentIndex])) {
+      if (this.hasMatchingEqualityFilter(segments[segmentIndex])) {
+        equalitySegments = equalitySegments.add(
+          segments[segmentIndex].fieldPath.canonicalString()
+        );
+      } else {
         // If we cannot find a matching filter, we need to verify whether the
         // remaining segments map to the target's inequality and its orderBy
         // clauses.
@@ -144,16 +145,22 @@ export class TargetIndexMatcher {
       return true;
     }
 
-    // If there is an inequality filter, the next segment must match both the
-    // filter and the first orderBy clause.
     if (this.inequalityFilter !== undefined) {
-      const segment = segments[segmentIndex];
+      // If there is an inequality filter and the field was not in one of the
+      // equality filters above, the next segment must match both the filter
+      // and the first orderBy clause.
       if (
-        !this.matchesFilter(this.inequalityFilter, segment) ||
-        !this.matchesOrderBy(this.orderBys[orderBysIndex++], segment)
+        !equalitySegments.has(this.inequalityFilter.field.canonicalString())
       ) {
-        return false;
+        const segment = segments[segmentIndex];
+        if (
+          !this.matchesFilter(this.inequalityFilter, segment) ||
+          !this.matchesOrderBy(this.orderBys[orderBysIndex++], segment)
+        ) {
+          return false;
+        }
       }
+
       ++segmentIndex;
     }
 

diff --git a/packages/firestore/test/unit/local/index_manager.test.ts b/packages/firestore/test/unit/local/index_manager.test.ts
@@ -939,6 +939,69 @@ describe('IndexedDbIndexManager', async () => {
     await verifyResults(testingQuery, 'coll/val2');
   });
 
+  it('can have filters on the same field', async () => {
+    await indexManager.addFieldIndex(
+      fieldIndex('coll', { fields: [['a', IndexKind.ASCENDING]] })
+    );
+    await indexManager.addFieldIndex(
+      fieldIndex('coll', {
+        fields: [
+          ['a', IndexKind.ASCENDING],
+          ['b', IndexKind.ASCENDING]
+        ]
+      })
+    );
+    await addDoc('coll/val1', { 'a': 1, 'b': 1 });
+    await addDoc('coll/val2', { 'a': 2, 'b': 2 });
+    await addDoc('coll/val3', { 'a': 3, 'b': 3 });
+    await addDoc('coll/val4', { 'a': 4, 'b': 4 });
+
+    let testingQuery = queryWithAddedFilter(
+      queryWithAddedFilter(query('coll'), filter('a', '>', 1)),
+      filter('a', '==', 2)
+    );
+    await verifyResults(testingQuery, 'coll/val2');
+
+    testingQuery = queryWithAddedFilter(
+      queryWithAddedFilter(query('coll'), filter('a', '<=', 1)),
+      filter('a', '==', 2)
+    );
+    await verifyResults(testingQuery);
+
+    testingQuery = queryWithAddedOrderBy(
+      queryWithAddedFilter(
+        queryWithAddedFilter(query('coll'), filter('a', '>', 1)),
+        filter('a', '==', 2)
+      ),
+      orderBy('a')
+    );
+    await verifyResults(testingQuery, 'coll/val2');
+
+    testingQuery = queryWithAddedOrderBy(
+      queryWithAddedOrderBy(
+        queryWithAddedFilter(
+          queryWithAddedFilter(query('coll'), filter('a', '>', 1)),
+          filter('a', '==', 2)
+        ),
+        orderBy('a')
+      ),
+      orderBy('__name__', 'desc')
+    );
+    await verifyResults(testingQuery, 'coll/val2');
+
+    testingQuery = queryWithAddedOrderBy(
+      queryWithAddedOrderBy(
+        queryWithAddedFilter(
+          queryWithAddedFilter(query('coll'), filter('a', '>', 1)),
+          filter('a', '==', 3)
+        ),
+        orderBy('a')
+      ),
+      orderBy('b', 'desc')
+    );
+    await verifyResults(testingQuery, 'coll/val3');
+  });
+
   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_index_matcher.test.ts b/packages/firestore/test/unit/model/target_index_matcher.test.ts
@@ -639,6 +639,80 @@ describe('Target Bounds', () => {
     validateServesTarget(q, 'a', IndexKind.ASCENDING);
   });
 
+  it('with equality and inequality on the same field', () => {
+    let q = queryWithAddedFilter(
+      queryWithAddedFilter(query('collId'), filter('a', '>=', 5)),
+      filter('a', '==', 0)
+    );
+    validateServesTarget(q, 'a', IndexKind.ASCENDING);
+
+    q = queryWithAddedOrderBy(
+      queryWithAddedFilter(
+        queryWithAddedFilter(query('collId'), filter('a', '>=', 5)),
+        filter('a', '==', 0)
+      ),
+      orderBy('a')
+    );
+    validateServesTarget(q, 'a', IndexKind.ASCENDING);
+
+    q = queryWithAddedOrderBy(
+      queryWithAddedOrderBy(
+        queryWithAddedFilter(
+          queryWithAddedFilter(query('collId'), filter('a', '>=', 5)),
+          filter('a', '==', 0)
+        ),
+        orderBy('a')
+      ),
+      orderBy('__name__')
+    );
+    validateServesTarget(q, 'a', IndexKind.ASCENDING);
+
+    q = queryWithAddedOrderBy(
+      queryWithAddedOrderBy(
+        queryWithAddedFilter(
+          queryWithAddedFilter(query('collId'), filter('a', '>=', 5)),
+          filter('a', '==', 0)
+        ),
+        orderBy('a')
+      ),
+      orderBy('__name__', 'desc')
+    );
+    validateServesTarget(q, 'a', IndexKind.ASCENDING);
+
+    q = queryWithAddedOrderBy(
+      queryWithAddedOrderBy(
+        queryWithAddedOrderBy(
+          queryWithAddedFilter(
+            queryWithAddedFilter(query('collId'), filter('a', '>=', 5)),
+            filter('a', '==', 0)
+          ),
+          orderBy('a')
+        ),
+        orderBy('b')
+      ),
+      orderBy('__name__', 'desc')
+    );
+    validateServesTarget(
+      q,
+      'a',
+      IndexKind.ASCENDING,
+      'b',
+      IndexKind.DESCENDING
+    );
+
+    q = queryWithAddedOrderBy(
+      queryWithAddedOrderBy(
+        queryWithAddedFilter(
+          queryWithAddedFilter(query('collId'), filter('a', '>=', 5)),
+          filter('a', '==', 0)
+        ),
+        orderBy('a')
+      ),
+      orderBy('__name__', 'desc')
+    );
+    validateServesTarget(q, 'a', IndexKind.ASCENDING);
+  });
+
   function validateServesTarget(
     query: Query,
     field: string,