From 56cd2aa049910d932f753f34c6beee861ffeaa53 Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Thu, 10 Apr 2025 14:44:12 -0400 Subject: [PATCH 1/3] fix: remove null value inclusion from != and not-in filter results --- .../Tests/Integration/API/FIRQueryTests.mm | 52 +++++++++++++++++++ Firestore/core/src/core/field_filter.cc | 3 +- Firestore/core/src/core/not_in_filter.cc | 5 +- Firestore/core/test/unit/core/query_test.cc | 4 +- 4 files changed, 60 insertions(+), 4 deletions(-) diff --git a/Firestore/Example/Tests/Integration/API/FIRQueryTests.mm b/Firestore/Example/Tests/Integration/API/FIRQueryTests.mm index 0b92a36ea48..c45db1370a6 100644 --- a/Firestore/Example/Tests/Integration/API/FIRQueryTests.mm +++ b/Firestore/Example/Tests/Integration/API/FIRQueryTests.mm @@ -537,6 +537,31 @@ - (void)testQueriesCanUseNotEqualFiltersWithDocIds { (@[ testDocs[@"ab"], testDocs[@"ba"], testDocs[@"bb"] ])); } +- (void)testSDKUsesNotEqualFiltersSameAsServer { + NSDictionary *testDocs = @{ + @"a" : @{@"zip" : @(NAN)}, + @"b" : @{@"zip" : @91102}, + @"c" : @{@"zip" : @98101}, + @"d" : @{@"zip" : @"98101"}, + @"e" : @{@"zip" : @[ @98101 ]}, + @"f" : @{@"zip" : @[ @98101, @98102 ]}, + @"g" : @{@"zip" : @[ @"98101", @{@"zip" : @98101} ]}, + @"h" : @{@"zip" : @{@"code" : @500}}, + @"i" : @{@"zip" : [NSNull null]}, + @"j" : @{@"code" : @500}, + }; + FIRCollectionReference *collection = [self collectionRefWithDocuments:testDocs]; + + // populate cache with all documents first to ensure getDocsFromCache() scans all docs + [self readDocumentSetForRef:collection]; + + [self checkOnlineAndOfflineQuery:[collection queryWhereField:@"zip" isNotEqualTo:@98101] + matchesResult:@[ @"a", @"b", @"d", @"e", @"f", @"g", @"h" ]]; + + [self checkOnlineAndOfflineQuery:[collection queryWhereField:@"zip" isNotEqualTo:@(NAN)] + matchesResult:@[ @"b", @"c", @"d", @"e", @"f", @"g", @"h" ]]; +} + - (void)testQueriesCanUseArrayContainsFilters { NSDictionary *testDocs = @{ @"a" : @{@"array" : @[ @42 ]}, @@ -694,6 +719,33 @@ - (void)testQueriesCanUseNotInFiltersWithDocIds { XCTAssertEqualObjects(FIRQuerySnapshotGetData(snapshot), (@[ testDocs[@"ba"], testDocs[@"bb"] ])); } +- (void)testSDKUsesNotInFiltersSameAsServer { + NSDictionary *testDocs = @{ + @"a" : @{@"zip" : @(NAN)}, + @"b" : @{@"zip" : @91102}, + @"c" : @{@"zip" : @98101}, + @"d" : @{@"zip" : @"98101"}, + @"e" : @{@"zip" : @[ @98101 ]}, + @"f" : @{@"zip" : @[ @98101, @98102 ]}, + @"g" : @{@"zip" : @[ @"98101", @{@"zip" : @98101} ]}, + @"h" : @{@"zip" : @{@"code" : @500}}, + @"i" : @{@"zip" : [NSNull null]}, + @"j" : @{@"code" : @500}, + }; + FIRCollectionReference *collection = [self collectionRefWithDocuments:testDocs]; + + // populate cache with all documents first to ensure getDocsFromCache() scans all docs + [self readDocumentSetForRef:collection]; + + [self checkOnlineAndOfflineQuery:[collection + queryWhereField:@"zip" + notIn:@[ @98101, @98103, @[ @98101, @98102 ] ]] + matchesResult:@[ @"a", @"b", @"d", @"e", @"g", @"h" ]]; + + [self checkOnlineAndOfflineQuery:[collection queryWhereField:@"zip" notIn:@[ [NSNull null] ]] + matchesResult:@[]]; +} + - (void)testQueriesCanUseArrayContainsAnyFilters { NSDictionary *testDocs = @{ @"a" : @{@"array" : @[ @42 ]}, diff --git a/Firestore/core/src/core/field_filter.cc b/Firestore/core/src/core/field_filter.cc index 2867e3ba8ba..29425f007d3 100644 --- a/Firestore/core/src/core/field_filter.cc +++ b/Firestore/core/src/core/field_filter.cc @@ -157,7 +157,8 @@ bool FieldFilter::Rep::Matches(const model::Document& doc) const { // Types do not have to match in NotEqual filters. if (op_ == Operator::NotEqual) { - return MatchesComparison(Compare(lhs, *value_rhs_)); + return lhs.which_value_type != google_firestore_v1_Value_null_value_tag && + MatchesComparison(Compare(lhs, *value_rhs_)); } // Only compare types with matching backend order (such as double and int). diff --git a/Firestore/core/src/core/not_in_filter.cc b/Firestore/core/src/core/not_in_filter.cc index ef162b1f410..48ba3e481ef 100644 --- a/Firestore/core/src/core/not_in_filter.cc +++ b/Firestore/core/src/core/not_in_filter.cc @@ -62,7 +62,10 @@ bool NotInFilter::Rep::Matches(const Document& doc) const { return false; } absl::optional maybe_lhs = doc->field(field()); - return maybe_lhs && !Contains(array_value, *maybe_lhs); + return maybe_lhs && + maybe_lhs->which_value_type != + google_firestore_v1_Value_null_value_tag && + !Contains(array_value, *maybe_lhs); } } // namespace core diff --git a/Firestore/core/test/unit/core/query_test.cc b/Firestore/core/test/unit/core/query_test.cc index c71590a8f33..8515aabdeae 100644 --- a/Firestore/core/test/unit/core/query_test.cc +++ b/Firestore/core/test/unit/core/query_test.cc @@ -279,7 +279,7 @@ TEST(QueryTest, NanFilter) { EXPECT_THAT(query, Matches(doc3)); EXPECT_THAT(query, Matches(doc4)); EXPECT_THAT(query, Matches(doc5)); - EXPECT_THAT(query, Matches(doc6)); + EXPECT_THAT(query, Not(Matches(doc6))); } TEST(QueryTest, ArrayContainsFilter) { @@ -383,7 +383,7 @@ TEST(QueryTest, NotInFilters) { // Null match. doc = Doc("collection/1", 0, Map("zip", nullptr)); - EXPECT_THAT(query, Matches(doc)); + EXPECT_THAT(query, Not(Matches(doc))); // NAN match. doc = Doc("collection/1", 0, Map("zip", NAN)); From 93dcdbaf548db7ff93f64a662022d3d8ad2b9242 Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Thu, 10 Apr 2025 14:53:54 -0400 Subject: [PATCH 2/3] Update CHANGELOG.md --- Firestore/CHANGELOG.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/Firestore/CHANGELOG.md b/Firestore/CHANGELOG.md index d06b932af10..fdc4ede71cb 100644 --- a/Firestore/CHANGELOG.md +++ b/Firestore/CHANGELOG.md @@ -1,3 +1,6 @@ +# Unreleased +- [fixed] Fixed the `null` value handling in `isNotEqualTo` and `notIn` filters. + # 11.11.0 - [fixed] Fixed the customized priority queue compare function used cache index manager. (#14496) From 9a72c62aa850914994a0ca4545312e8666b83885 Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Thu, 10 Apr 2025 16:49:00 -0400 Subject: [PATCH 3/3] add the "!= null" test case --- Firestore/Example/Tests/Integration/API/FIRQueryTests.mm | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/Firestore/Example/Tests/Integration/API/FIRQueryTests.mm b/Firestore/Example/Tests/Integration/API/FIRQueryTests.mm index c45db1370a6..71e1172190d 100644 --- a/Firestore/Example/Tests/Integration/API/FIRQueryTests.mm +++ b/Firestore/Example/Tests/Integration/API/FIRQueryTests.mm @@ -560,6 +560,10 @@ - (void)testSDKUsesNotEqualFiltersSameAsServer { [self checkOnlineAndOfflineQuery:[collection queryWhereField:@"zip" isNotEqualTo:@(NAN)] matchesResult:@[ @"b", @"c", @"d", @"e", @"f", @"g", @"h" ]]; + + [self checkOnlineAndOfflineQuery:[collection queryWhereField:@"zip" + isNotEqualTo:@[ [NSNull null] ]] + matchesResult:@[ @"a", @"b", @"c", @"d", @"e", @"f", @"g", @"h" ]]; } - (void)testQueriesCanUseArrayContainsFilters {