Skip to content

Commit

Permalink
Multiple inequality support (#11626)
Browse files Browse the repository at this point in the history
  • Loading branch information
milaGGL authored and andrewheard committed Sep 20, 2023
1 parent 214191e commit 29efb85
Show file tree
Hide file tree
Showing 17 changed files with 802 additions and 250 deletions.
510 changes: 510 additions & 0 deletions Firestore/Example/Tests/Integration/API/FIRQueryTests.mm

Large diffs are not rendered by default.

109 changes: 28 additions & 81 deletions Firestore/Example/Tests/Integration/API/FIRValidationTests.mm
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Expand Down Expand Up @@ -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"];

Expand Down
45 changes: 0 additions & 45 deletions Firestore/core/src/api/query_core.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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 "
Expand Down Expand Up @@ -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));
Expand Down Expand Up @@ -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(
Expand Down
3 changes: 0 additions & 3 deletions Firestore/core/src/api/query_core.h
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
11 changes: 0 additions & 11 deletions Firestore/core/src/core/composite_filter.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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([&]() {
Expand Down
2 changes: 0 additions & 2 deletions Firestore/core/src/core/composite_filter.h
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
Expand Down
7 changes: 0 additions & 7 deletions Firestore/core/src/core/field_filter.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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
2 changes: 0 additions & 2 deletions Firestore/core/src/core/field_filter.h
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
10 changes: 0 additions & 10 deletions Firestore/core/src/core/filter.h
Original file line number Diff line number Diff line change
Expand Up @@ -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.
*/
Expand Down Expand Up @@ -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;
Expand Down
Loading

0 comments on commit 29efb85

Please sign in to comment.