Multiple inequality support (#11626)

firebase · Sep 13, 2023 · f3c7db7 · f3c7db7
1 parent 0c9fe27
commit f3c7db7
Show file tree

Hide file tree

Showing 17 changed files with 802 additions and 250 deletions.
diff --git a/Firestore/Example/Tests/Integration/API/FIRQueryTests.mm b/Firestore/Example/Tests/Integration/API/FIRQueryTests.mm
diff --git a/Firestore/Example/Tests/Integration/API/FIRValidationTests.mm b/Firestore/Example/Tests/Integration/API/FIRValidationTests.mm
@@ -626,111 +626,72 @@ - (void)testQueriesUsingInAndDocumentIdMustHaveProperDocumentReferencesInArray {
 
 - (void)testInvalidQueryFilters {
   FIRCollectionReference *collection = [self collectionRef];
-
-  // Multiple inequalities, one of which is inside a nested composite filter.
-  NSString *reason = @"Invalid Query. All where filters with an inequality (notEqual, lessThan, "
-                      "lessThanOrEqual, greaterThan, or greaterThanOrEqual) must be on the same "
-                      "field. But you have inequality filters on 'c' and 'r'";
-
+  NSString *reason = @"Invalid Query. You cannot use 'notIn' filters with 'in' filters.";
   NSArray<FIRFilter *> *array1 = @[
     [FIRFilter andFilterWithFilters:@[
       [FIRFilter filterWhereField:@"a" isEqualTo:@"b"], [FIRFilter filterWhereField:@"c"
-                                                                      isGreaterThan:@"d"]
+                                                                                 in:@[ @"d", @"e" ]]
     ]],
     [FIRFilter andFilterWithFilters:@[
       [FIRFilter filterWhereField:@"e" isEqualTo:@"f"], [FIRFilter filterWhereField:@"g"
-                                                                          isEqualTo:@"h"]
+                                                                              notIn:@[ @"h", @"i" ]]
     ]]
   ];
+  FSTAssertThrows([collection queryWhereFilter:[FIRFilter orFilterWithFilters:array1]], reason);
 
-  FSTAssertThrows(
-      [[collection queryWhereFilter:[FIRFilter orFilterWithFilters:array1]] queryWhereField:@"r"
-                                                                              isGreaterThan:@"s"],
-      reason);
-
-  // OrderBy and inequality on different fields. Inequality inside a nested composite filter.
-  reason = @"Invalid query. You have a where filter with an inequality (notEqual, lessThan, "
-            "lessThanOrEqual, greaterThan, or greaterThanOrEqual) on field 'c' and so you must "
-            "also use 'c' as your first queryOrderedBy field, but your first queryOrderedBy is "
-            "currently on field 'r' instead.";
-
-  FSTAssertThrows([[collection queryWhereFilter:[FIRFilter orFilterWithFilters:array1]]
-                      queryOrderedByField:@"r"],
-                  reason);
-
-  // Conflicting operations within a composite filter.
-  reason = @"Invalid Query. You cannot use 'notIn' filters with 'in' filters.";
-
+  reason = @"Invalid Query. You cannot use 'notIn' filters with 'notEqual' filters.";
   NSArray<FIRFilter *> *array2 = @[
     [FIRFilter andFilterWithFilters:@[
       [FIRFilter filterWhereField:@"a" isEqualTo:@"b"], [FIRFilter filterWhereField:@"c"
-                                                                                 in:@[ @"d", @"e" ]]
+                                                                       isNotEqualTo:@"d"]
     ]],
     [FIRFilter andFilterWithFilters:@[
-      [FIRFilter filterWhereField:@"e" isEqualTo:@"f"], [FIRFilter filterWhereField:@"c"
-                                                                              notIn:@[ @"f", @"g" ]]
+      [FIRFilter filterWhereField:@"e" isEqualTo:@"f"], [FIRFilter filterWhereField:@"g"
+                                                                              notIn:@[ @"h", @"i" ]]
     ]]
   ];
-
   FSTAssertThrows([collection queryWhereFilter:[FIRFilter orFilterWithFilters:array2]], reason);
 
-  // Conflicting operations between a field filter and a composite filter.
+  reason = @"Invalid Query. You cannot use more than one 'notIn' filter.";
   NSArray<FIRFilter *> *array3 = @[
     [FIRFilter andFilterWithFilters:@[
       [FIRFilter filterWhereField:@"a" isEqualTo:@"b"], [FIRFilter filterWhereField:@"c"
-                                                                                 in:@[ @"d", @"e" ]]
+                                                                              notIn:@[ @"d", @"e" ]]
     ]],
     [FIRFilter andFilterWithFilters:@[
       [FIRFilter filterWhereField:@"e" isEqualTo:@"f"], [FIRFilter filterWhereField:@"g"
-                                                                          isEqualTo:@"h"]
+                                                                              notIn:@[ @"h", @"i" ]]
     ]]
   ];
+  FSTAssertThrows([collection queryWhereFilter:[FIRFilter orFilterWithFilters:array3]], reason);
 
-  NSArray<NSString *> *array4 = @[ @"j", @"k" ];
-
-  FSTAssertThrows(
-      [[collection queryWhereFilter:[FIRFilter orFilterWithFilters:array3]] queryWhereField:@"i"
-                                                                                      notIn:array4],
-      reason);
-
-  // Conflicting operations between two composite filters.
-  NSArray<FIRFilter *> *array5 = @[
+  reason = @"Invalid Query. You cannot use more than one 'notEqual' filter.";
+  NSArray<FIRFilter *> *array4 = @[
     [FIRFilter andFilterWithFilters:@[
-      [FIRFilter filterWhereField:@"i" isEqualTo:@"j"], [FIRFilter filterWhereField:@"l"
-                                                                              notIn:@[ @"m", @"n" ]]
+      [FIRFilter filterWhereField:@"a" isEqualTo:@"b"], [FIRFilter filterWhereField:@"c"
+                                                                       isNotEqualTo:@"d"]
     ]],
     [FIRFilter andFilterWithFilters:@[
-      [FIRFilter filterWhereField:@"o" isEqualTo:@"p"], [FIRFilter filterWhereField:@"q"
-                                                                          isEqualTo:@"r"]
+      [FIRFilter filterWhereField:@"e" isEqualTo:@"f"], [FIRFilter filterWhereField:@"g"
+                                                                       isNotEqualTo:@"h"]
     ]]
   ];
-
-  FSTAssertThrows([[collection queryWhereFilter:[FIRFilter orFilterWithFilters:array3]]
-                      queryWhereFilter:[FIRFilter orFilterWithFilters:array5]],
-                  reason);
+  FSTAssertThrows([collection queryWhereFilter:[FIRFilter orFilterWithFilters:array4]], reason);
 }
 
-- (void)testQueryInequalityFieldMustMatchFirstOrderByField {
+- (void)testQueryInequalityFieldWithMultipleOrderByFields {
   FIRCollectionReference *coll = [self.db collectionWithPath:@"collection"];
   FIRQuery *base = [coll queryWhereField:@"x" isGreaterThanOrEqualTo:@32];
 
-  FSTAssertThrows([base queryWhereField:@"y" isLessThan:@"cat"],
-                  @"Invalid Query. All where filters with an inequality (notEqual, lessThan, "
-                   "lessThanOrEqual, greaterThan, or greaterThanOrEqual) must be on the same "
-                   "field. But you have inequality filters on 'x' and 'y'");
+  XCTAssertNoThrow([base queryWhereField:@"y" isLessThan:@"cat"]);
 
-  NSString *reason =
-      @"Invalid query. You have a where filter with "
-       "an inequality (notEqual, lessThan, lessThanOrEqual, greaterThan, or greaterThanOrEqual) "
-       "on field 'x' and so you must also use 'x' as your first queryOrderedBy field, "
-       "but your first queryOrderedBy is currently on field 'y' instead.";
-  FSTAssertThrows([base queryOrderedByField:@"y"], reason);
-  FSTAssertThrows([[coll queryOrderedByField:@"y"] queryWhereField:@"x" isGreaterThan:@32], reason);
-  FSTAssertThrows([[base queryOrderedByField:@"y"] queryOrderedByField:@"x"], reason);
-  FSTAssertThrows([[[coll queryOrderedByField:@"y"] queryOrderedByField:@"x"] queryWhereField:@"x"
-                                                                                isGreaterThan:@32],
-                  reason);
-  FSTAssertThrows([[coll queryOrderedByField:@"y"] queryWhereField:@"x" isNotEqualTo:@32], reason);
+  XCTAssertNoThrow([base queryOrderedByField:@"y"]);
+  XCTAssertNoThrow([[coll queryOrderedByField:@"y"] queryWhereField:@"x" isGreaterThan:@32]);
+  XCTAssertNoThrow([[base queryOrderedByField:@"y"] queryOrderedByField:@"x"]);
+  XCTAssertNoThrow([[[coll queryOrderedByField:@"y"] queryOrderedByField:@"x"]
+      queryWhereField:@"x"
+        isGreaterThan:@32]);
+  XCTAssertNoThrow([[coll queryOrderedByField:@"y"] queryWhereField:@"x" isNotEqualTo:@32]);
 
   XCTAssertNoThrow([base queryWhereField:@"x" isLessThanOrEqualTo:@"cat"],
                    @"Same inequality fields work");
@@ -758,20 +719,6 @@ - (void)testQueryInequalityFieldMustMatchFirstOrderByField {
                    @"array_contains different than orderBy works.");
 }
 
-- (void)testQueriesWithMultipleNotEqualAndInequalitiesFail {
-  FIRCollectionReference *coll = [self.db collectionWithPath:@"collection"];
-
-  FSTAssertThrows([[coll queryWhereField:@"x" isNotEqualTo:@1] queryWhereField:@"x"
-                                                                  isNotEqualTo:@2],
-                  @"Invalid Query. You cannot use more than one 'notEqual' filter.");
-
-  FSTAssertThrows([[coll queryWhereField:@"x" isNotEqualTo:@1] queryWhereField:@"y"
-                                                                  isNotEqualTo:@2],
-                  @"Invalid Query. All where filters with an inequality (notEqual, lessThan, "
-                   "lessThanOrEqual, greaterThan, or greaterThanOrEqual) must be on "
-                   "the same field. But you have inequality filters on 'x' and 'y'");
-}
-
 - (void)testQueriesWithNotEqualAndNotInFiltersFail {
   FIRCollectionReference *coll = [self.db collectionWithPath:@"collection"];
 

diff --git a/Firestore/core/src/api/query_core.cc b/Firestore/core/src/api/query_core.cc
@@ -277,7 +277,6 @@ Query Query::OrderBy(FieldPath field_path, bool descending) const {
 }
 
 Query Query::OrderBy(FieldPath field_path, Direction direction) const {
-  ValidateNewOrderByPath(field_path);
   if (query_.start_at()) {
     ThrowInvalidArgument(
         "Invalid query. You must not specify a starting point "
@@ -320,26 +319,6 @@ Query Query::EndAt(Bound bound) const {
 
 void Query::ValidateNewFieldFilter(const core::Query& query,
                                    const FieldFilter& field_filter) const {
-  if (field_filter.IsInequality()) {
-    const FieldPath* existing_inequality = query.InequalityFilterField();
-    const FieldPath& new_inequality = field_filter.field();
-
-    if (existing_inequality && *existing_inequality != new_inequality) {
-      ThrowInvalidArgument(
-          "Invalid Query. All where filters with an inequality (notEqual, "
-          "lessThan, lessThanOrEqual, greaterThan, or greaterThanOrEqual) "
-          "must be on the same field. But you have inequality filters on "
-          "'%s' and '%s'",
-          existing_inequality->CanonicalString(),
-          new_inequality.CanonicalString());
-    }
-
-    const FieldPath* first_order_by_field = query.FirstOrderByField();
-    if (first_order_by_field) {
-      ValidateOrderByField(*first_order_by_field, field_filter.field());
-    }
-  }
-
   Operator filter_op = field_filter.op();
   absl::optional<Operator> conflicting_op =
       query.FindOpInsideFilters(ConflictingOps(filter_op));
@@ -367,30 +346,6 @@ void Query::ValidateNewFilter(const Filter& filter) const {
   }
 }
 
-void Query::ValidateNewOrderByPath(const FieldPath& field_path) const {
-  if (!query_.FirstOrderByField()) {
-    // This is the first order by. It must match any inequality.
-    const FieldPath* inequality_field = query_.InequalityFilterField();
-    if (inequality_field) {
-      ValidateOrderByField(field_path, *inequality_field);
-    }
-  }
-}
-
-void Query::ValidateOrderByField(const FieldPath& order_by_field,
-                                 const FieldPath& inequality_field) const {
-  if (order_by_field != inequality_field) {
-    ThrowInvalidArgument(
-        "Invalid query. You have a where filter with an inequality "
-        "(notEqual, lessThan, lessThanOrEqual, greaterThan, or "
-        "greaterThanOrEqual) on field '%s' and so you must also use '%s' as "
-        "your first queryOrderedBy field, but your first queryOrderedBy is "
-        "currently on field '%s' instead.",
-        inequality_field.CanonicalString(), inequality_field.CanonicalString(),
-        order_by_field.CanonicalString());
-  }
-}
-
 void Query::ValidateHasExplicitOrderByForLimitToLast() const {
   if (query_.has_limit_to_last() && query_.explicit_order_bys().empty()) {
     ThrowInvalidArgument(

diff --git a/Firestore/core/src/api/query_core.h b/Firestore/core/src/api/query_core.h
@@ -215,9 +215,6 @@ class Query {
   void ValidateNewFilter(const core::Filter& filter) const;
   void ValidateNewFieldFilter(const core::Query& query,
                               const core::FieldFilter& filter) const;
-  void ValidateNewOrderByPath(const model::FieldPath& field_path) const;
-  void ValidateOrderByField(const model::FieldPath& order_by_field,
-                            const model::FieldPath& inequality_field) const;
   void ValidateHasExplicitOrderByForLimitToLast() const;
   /**
    * Validates that the value passed into a disjunctive filter satisfies all

diff --git a/Firestore/core/src/core/composite_filter.cc b/Firestore/core/src/core/composite_filter.cc
@@ -141,17 +141,6 @@ const FieldFilter* CompositeFilter::Rep::FindFirstMatchingFilter(
   return nullptr;
 }
 
-const model::FieldPath* CompositeFilter::Rep::GetFirstInequalityField() const {
-  CheckFunction condition = [](const FieldFilter& field_filter) {
-    return field_filter.IsInequality();
-  };
-  const FieldFilter* found = FindFirstMatchingFilter(condition);
-  if (found) {
-    return &(found->field());
-  }
-  return nullptr;
-}
-
 const std::vector<FieldFilter>& CompositeFilter::Rep::GetFlattenedFilters()
     const {
   return memoized_flattened_filters_->memoize([&]() {

diff --git a/Firestore/core/src/core/composite_filter.h b/Firestore/core/src/core/composite_filter.h
@@ -140,8 +140,6 @@ class CompositeFilter : public Filter {
 
     const std::vector<FieldFilter>& GetFlattenedFilters() const override;
 
-    const model::FieldPath* GetFirstInequalityField() const override;
-
     std::vector<Filter> GetFilters() const override {
       return filters();
     }

diff --git a/Firestore/core/src/core/field_filter.cc b/Firestore/core/src/core/field_filter.cc
@@ -202,13 +202,6 @@ bool FieldFilter::Rep::Equals(const Filter::Rep& other) const {
          *value_rhs_ == *other_rep.value_rhs_;
 }
 
-const model::FieldPath* FieldFilter::Rep::GetFirstInequalityField() const {
-  if (IsInequality()) {
-    return &field();
-  }
-  return nullptr;
-}
-
 }  // namespace core
 }  // namespace firestore
 }  // namespace firebase
diff --git a/Firestore/core/src/core/field_filter.h b/Firestore/core/src/core/field_filter.h
@@ -117,8 +117,6 @@ class FieldFilter : public Filter {
       return false;
     }
 
-    const model::FieldPath* GetFirstInequalityField() const override;
-
     const std::vector<FieldFilter>& GetFlattenedFilters() const override;
 
     std::vector<Filter> GetFilters() const override;

diff --git a/Firestore/core/src/core/filter.h b/Firestore/core/src/core/filter.h
@@ -95,14 +95,6 @@ class Filter {
     return rep_->IsEmpty();
   }
 
-  /**
-   * Returns the first inequality filter contained within this filter.
-   * Returns nullptr if it does not contain any inequalities.
-   */
-  const model::FieldPath* GetFirstInequalityField() const {
-    return rep_->GetFirstInequalityField();
-  }
-
   /**
    * Returns a list of all field filters that are contained within this filter.
    */
@@ -155,8 +147,6 @@ class Filter {
 
     virtual bool IsEmpty() const = 0;
 
-    virtual const model::FieldPath* GetFirstInequalityField() const = 0;
-
     virtual const std::vector<FieldFilter>& GetFlattenedFilters() const = 0;
 
     virtual std::vector<Filter> GetFilters() const = 0;