Fix an issue where TargetIndexMatcher rejects valid queries. (#11353)

* Fix an issue where TargetIndexMatcher rejects valid queries.

* Revert certtificates

* fix comment

* style

Author: wu-hui

Firestore/core/src/model/target_index_matcher.cc

@@ -16,6 +16,8 @@
 
 #include "Firestore/core/src/model/target_index_matcher.h"
 
+#include <unordered_set>
+
 #include "Firestore/core/src/util/hard_assert.h"
 
 namespace firebase {
@@ -59,16 +61,16 @@ bool TargetIndexMatcher::ServedByIndex(const model::FieldIndex& index) {
   }
 
   std::vector<Segment> segments = index.GetDirectionalSegments();
+  std::unordered_set<std::string> equality_segments;
   size_t segment_index = 0;
   // Process all equalities first. Equalities can appear out of order.
   for (; segment_index < segments.size(); ++segment_index) {
     // 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 (!HasMatchingEqualityFilter(segments[segment_index])) {
+    // filter matches an index segment, we can mark the segment as used.
+    if (HasMatchingEqualityFilter(segments[segment_index])) {
+      equality_segments.emplace(
+          segments[segment_index].field_path().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.
@@ -86,14 +88,18 @@ bool TargetIndexMatcher::ServedByIndex(const model::FieldIndex& index) {
   // `order_bys_` has at least one element.
   auto order_by_iter = order_bys_.begin();
 
-  // If there is an inequality filter, the next segment must match both the
-  // filter and the first OrderBy clause.
   if (inequality_filter_.has_value()) {
-    if (!MatchesFilter(inequality_filter_, segments[segment_index]) ||
-        !MatchesOrderBy(*order_by_iter, segments[segment_index])) {
-      return false;
+    // 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 (equality_segments.count(
+            inequality_filter_.value().field().CanonicalString()) == 0) {
+      if (!MatchesFilter(inequality_filter_, segments[segment_index]) ||
+          !MatchesOrderBy(*(order_by_iter++), segments[segment_index])) {
+        return false;
+      }
     }
-    ++order_by_iter;
+
     ++segment_index;
   }
 

Firestore/core/test/unit/local/leveldb_index_manager_test.cc

@@ -361,6 +361,58 @@ TEST_F(LevelDbIndexManagerTest, CursorCannotExpandResult) {
   });
 }
 
+TEST_F(LevelDbIndexManagerTest, FiltersOnTheSameField) {
+  persistence_->Run("TestFiltersOnTheSameField", [&]() {
+    index_manager_->Start();
+
+    index_manager_->AddFieldIndex(
+        MakeFieldIndex("coll", "a", model::Segment::kAscending));
+    index_manager_->AddFieldIndex(
+        MakeFieldIndex("coll", "a", model::Segment::kAscending, "b",
+                       model::Segment::kAscending));
+    AddDoc("coll/val1", Map("a", 1, "b", 1));
+    AddDoc("coll/val2", Map("a", 2, "b", 2));
+    AddDoc("coll/val3", Map("a", 3, "b", 3));
+    AddDoc("coll/val4", Map("a", 4, "b", 4));
+
+    {
+      auto query = Query("coll")
+                       .AddingFilter(Filter("a", ">", 1))
+                       .AddingFilter(Filter("a", "==", 2));
+      VerifyResults(query, {"coll/val2"});
+    }
+    {
+      auto query = Query("coll")
+                       .AddingFilter(Filter("a", "<=", 1))
+                       .AddingFilter(Filter("a", "==", 2));
+      VerifyResults(query, {});
+    }
+    {
+      auto query = Query("coll")
+                       .AddingFilter(Filter("a", ">", 1))
+                       .AddingFilter(Filter("a", "==", 2))
+                       .AddingOrderBy(OrderBy("a"));
+      VerifyResults(query, {"coll/val2"});
+    }
+    {
+      auto query = Query("coll")
+                       .AddingFilter(Filter("a", ">", 1))
+                       .AddingFilter(Filter("a", "==", 2))
+                       .AddingOrderBy(OrderBy("a"))
+                       .AddingOrderBy(OrderBy("__name__", "desc"));
+      VerifyResults(query, {"coll/val2"});
+    }
+    {
+      auto query = Query("coll")
+                       .AddingFilter(Filter("a", ">", 1))
+                       .AddingFilter(Filter("a", "==", 3))
+                       .AddingOrderBy(OrderBy("a"))
+                       .AddingOrderBy(OrderBy("b", "desc"));
+      VerifyResults(query, {"coll/val3"});
+    }
+  });
+}
+
 TEST_F(LevelDbIndexManagerTest, EqualityFilter) {
   persistence_->Run("TestEqualityFilter", [&]() {
     index_manager_->Start();

Firestore/core/test/unit/model/target_index_matcher_test.cc

@@ -20,6 +20,7 @@
 #include <vector>
 
 #include "Firestore/core/src/core/query.h"
+#include "Firestore/core/src/model/document_key.h"
 #include "Firestore/core/src/model/field_index.h"
 #include "Firestore/core/test/unit/testutil/testutil.h"
 #include "gtest/gtest.h"
@@ -513,6 +514,49 @@ TEST(TargetIndexMatcher, WithInAndOrderBySameField) {
   ValidateServesTarget(q, "a", Segment::Kind::kAscending);
 }
 
+TEST(TargetIndexMatcher, WithEqualityAndInequalityOnTheSameField) {
+  ValidateServesTarget(testutil::Query("collId")
+                           .AddingFilter(Filter("a", ">=", 5))
+                           .AddingFilter(Filter("a", "==", 0)),
+                       "a", Segment::Kind::kAscending);
+
+  ValidateServesTarget(testutil::Query("collId")
+                           .AddingFilter(Filter("a", ">=", 5))
+                           .AddingFilter(Filter("a", "==", 0))
+                           .AddingOrderBy(OrderBy("a")),
+                       "a", Segment::Kind::kAscending);
+
+  ValidateServesTarget(testutil::Query("collId")
+                           .AddingFilter(Filter("a", ">=", 5))
+                           .AddingFilter(Filter("a", "==", 0))
+                           .AddingOrderBy(OrderBy("a"))
+                           .AddingOrderBy(OrderBy("__name__")),
+                       "a", Segment::Kind::kAscending);
+
+  ValidateServesTarget(testutil::Query("collId")
+                           .AddingFilter(Filter("a", ">=", 5))
+                           .AddingFilter(Filter("a", "==", 0))
+                           .AddingOrderBy(OrderBy("a"))
+                           .AddingOrderBy(OrderBy("__name__", "desc")),
+                       "a", Segment::Kind::kAscending);
+
+  ValidateServesTarget(testutil::Query("collId")
+                           .AddingFilter(Filter("a", ">=", 5))
+                           .AddingFilter(Filter("a", "==", 0))
+                           .AddingOrderBy(OrderBy("a"))
+                           .AddingOrderBy(OrderBy("b"))
+                           .AddingOrderBy(OrderBy("__name__", "desc")),
+                       "a", Segment::Kind::kAscending, "b",
+                       Segment::Kind::kAscending);
+
+  ValidateServesTarget(testutil::Query("collId")
+                           .AddingFilter(Filter("a", ">=", 5))
+                           .AddingFilter(Filter("a", "==", 0))
+                           .AddingOrderBy(OrderBy("a", "desc"))
+                           .AddingOrderBy(OrderBy("__name__", "desc")),
+                       "a", Segment::Kind::kDescending);
+}
+
 }  //  namespace
 }  //  namespace model
 }  //  namespace firestore