From 1e4f06d230f4db9c43737c51ca1859e3f73343b5 Mon Sep 17 00:00:00 2001 From: Brian Chen Date: Thu, 29 Oct 2020 15:33:01 -0700 Subject: [PATCH 1/3] Allow null/NAN values in filters --- Firestore/CHANGELOG.md | 6 +- .../Tests/Integration/API/FIRQueryTests.mm | 60 +++++++++++++++++-- .../Integration/API/FIRValidationTests.mm | 28 --------- Firestore/core/src/api/query_core.cc | 18 ------ Firestore/core/src/core/field_filter.cc | 19 ------ 5 files changed, 61 insertions(+), 70 deletions(-) diff --git a/Firestore/CHANGELOG.md b/Firestore/CHANGELOG.md index ca5fcc6ee74..d8918028196 100644 --- a/Firestore/CHANGELOG.md +++ b/Firestore/CHANGELOG.md @@ -1,5 +1,9 @@ # Unreleased -- [Fixed] Remove explicit MobileCoreServices library linkage from podspec. (#6850) +- [fixed] Remove explicit MobileCoreServices library linkage from podspec. (#6850) +- [fixed] Removed excess validation of null and NaN values in query filters. + This more closely aligns the SDK with the Firestore backend, which has always + accepted null and NaN for all operators, even though this isn't necessarily + useful. # v7.0.0 - [changed] **Breaking change:** Removed the `areTimestampsInSnapshotsEnabled` diff --git a/Firestore/Example/Tests/Integration/API/FIRQueryTests.mm b/Firestore/Example/Tests/Integration/API/FIRQueryTests.mm index 53041d9067a..187b0e7f311 100644 --- a/Firestore/Example/Tests/Integration/API/FIRQueryTests.mm +++ b/Firestore/Example/Tests/Integration/API/FIRQueryTests.mm @@ -452,7 +452,9 @@ - (void)testQueriesCanUseArrayContainsFilters { @"a" : @{@"array" : @[ @42 ]}, @"b" : @{@"array" : @[ @"a", @42, @"c" ]}, @"c" : @{@"array" : @[ @41.999, @"42", @{@"a" : @[ @42 ]} ]}, - @"d" : @{@"array" : @[ @42 ], @"array2" : @[ @"bingo" ]} + @"d" : @{@"array" : @[ @42 ], @"array2" : @[ @"bingo" ]}, + @"e" : @{@"array" : @[ [NSNull null] ]}, + @"f" : @{@"array" : @[ @(NAN) ]}, }; FIRCollectionReference *collection = [self collectionRefWithDocuments:testDocs]; @@ -462,8 +464,15 @@ - (void)testQueriesCanUseArrayContainsFilters { XCTAssertEqualObjects(FIRQuerySnapshotGetData(snapshot), (@[ testDocs[@"a"], testDocs[@"b"], testDocs[@"d"] ])); - // NOTE: The backend doesn't currently support null, NaN, objects, or arrays, so there isn't much - // of anything else interesting to test. + // With null. + snapshot = [self readDocumentSetForRef:[collection queryWhereField:@"array" + arrayContains:[NSNull null]]]; + XCTAssertTrue(snapshot.isEmpty); + + // With NAN. + snapshot = [self readDocumentSetForRef:[collection queryWhereField:@"array" + arrayContains:@(NAN)]]; + XCTAssertTrue(snapshot.isEmpty); } - (void)testQueriesCanUseInFilters { @@ -474,7 +483,9 @@ - (void)testQueriesCanUseInFilters { @"d" : @{@"zip" : @[ @98101 ]}, @"e" : @{@"zip" : @[ @"98101", @{@"zip" : @98101} ]}, @"f" : @{@"zip" : @{@"code" : @500}}, - @"g" : @{@"zip" : @[ @98101, @98102 ]} + @"g" : @{@"zip" : @[ @98101, @98102 ]}, + @"h" : @{@"zip" : [NSNull null]}, + @"i" : @{@"zip" : @(NAN)} }; FIRCollectionReference *collection = [self collectionRefWithDocuments:testDocs]; @@ -489,6 +500,24 @@ - (void)testQueriesCanUseInFilters { snapshot = [self readDocumentSetForRef:[collection queryWhereField:@"zip" in:@[ @{@"code" : @500} ]]]; XCTAssertEqualObjects(FIRQuerySnapshotGetData(snapshot), (@[ testDocs[@"f"] ])); + + // With null. + snapshot = [self readDocumentSetForRef:[collection queryWhereField:@"zip" in:@[ [NSNull null] ]]]; + XCTAssertTrue(snapshot.isEmpty); + + // With null and a value. + snapshot = [self readDocumentSetForRef:[collection queryWhereField:@"zip" + in:@[ [NSNull null], @98101 ]]]; + XCTAssertEqualObjects(FIRQuerySnapshotGetData(snapshot), (@[ testDocs[@"a"] ])); + + // With NAN. + snapshot = [self readDocumentSetForRef:[collection queryWhereField:@"zip" in:@[ @(NAN) ]]]; + XCTAssertTrue(snapshot.isEmpty); + + // With NAN and a value. + snapshot = [self readDocumentSetForRef:[collection queryWhereField:@"zip" + in:@[ @(NAN), @98101 ]]]; + XCTAssertEqualObjects(FIRQuerySnapshotGetData(snapshot), (@[ testDocs[@"a"] ])); } - (void)testQueriesCanUseInFiltersWithDocIds { @@ -584,6 +613,9 @@ - (void)testQueriesCanUseArrayContainsAnyFilters { @"e" : @{@"array" : @[ @43 ]}, @"f" : @{@"array" : @[ @{@"a" : @42} ]}, @"g" : @{@"array" : @42}, + @"h" : @{@"array" : @[ [NSNull null] ]}, + @"g" : @{@"array" : @[ @(NAN) ]}, + }; FIRCollectionReference *collection = [self collectionRefWithDocuments:testDocs]; @@ -599,6 +631,26 @@ - (void)testQueriesCanUseArrayContainsAnyFilters { XCTAssertEqualObjects(FIRQuerySnapshotGetData(snapshot), (@[ testDocs[@"f"], ])); + + // With null. + snapshot = [self readDocumentSetForRef:[collection queryWhereField:@"zip" + arrayContainsAny:@[ [NSNull null] ]]]; + XCTAssertTrue(snapshot.isEmpty); + + // With null and a value. + snapshot = [self readDocumentSetForRef:[collection queryWhereField:@"zip" + arrayContainsAny:@[ [NSNull null], @43 ]]]; + XCTAssertEqualObjects(FIRQuerySnapshotGetData(snapshot), (@[ testDocs[@"e"] ])); + + // With NAN. + snapshot = [self readDocumentSetForRef:[collection queryWhereField:@"zip" + arrayContainsAny:@[ @(NAN) ]]]; + XCTAssertTrue(snapshot.isEmpty); + + // With NAN and a value. + snapshot = [self readDocumentSetForRef:[collection queryWhereField:@"zip" + arrayContainsAny:@[ @(NAN), @43 ]]]; + XCTAssertEqualObjects(FIRQuerySnapshotGetData(snapshot), (@[ testDocs[@"e"] ])); } - (void)testCollectionGroupQueries { diff --git a/Firestore/Example/Tests/Integration/API/FIRValidationTests.mm b/Firestore/Example/Tests/Integration/API/FIRValidationTests.mm index ee6ed3db4d3..cdfe9040b8f 100644 --- a/Firestore/Example/Tests/Integration/API/FIRValidationTests.mm +++ b/Firestore/Example/Tests/Integration/API/FIRValidationTests.mm @@ -415,20 +415,6 @@ - (void)testQueryWithNonPositiveLimitFails { @"Invalid Query. Query limit (-1) is invalid. Limit must be positive."); } -- (void)testNonEqualityQueriesOnNullOrNaNFail { - NSString *expected = @"Invalid Query. Null supports only 'equalTo' and 'notEqualTo' comparisons."; - FSTAssertThrows([[self collectionRef] queryWhereField:@"a" isGreaterThan:nil], expected); - FSTAssertThrows([[self collectionRef] queryWhereField:@"a" isGreaterThan:[NSNull null]], - expected); - FSTAssertThrows([[self collectionRef] queryWhereField:@"a" arrayContains:nil], expected); - FSTAssertThrows([[self collectionRef] queryWhereField:@"a" arrayContains:[NSNull null]], - expected); - - expected = @"Invalid Query. NaN supports only 'equalTo' and 'notEqualTo' comparisons."; - FSTAssertThrows([[self collectionRef] queryWhereField:@"a" isGreaterThan:@(NAN)], expected); - FSTAssertThrows([[self collectionRef] queryWhereField:@"a" arrayContains:@(NAN)], expected); -} - - (void)testQueryCannotBeCreatedFromDocumentsMissingSortValues { FIRCollectionReference *testCollection = [self collectionRefWithDocuments:@{@"f" : @{@"v" : @"f", @"nosort" : @1.0}}]; @@ -787,20 +773,6 @@ - (void)testQueriesInAndArrayContainsAnyArrayRules { FSTAssertThrows( [coll queryWhereField:@"foo" notIn:values], @"Invalid Query. 'notIn' filters support a maximum of 10 elements in the value array."); - - NSArray *withNullValues = @[ @1, [NSNull null] ]; - FSTAssertThrows([coll queryWhereField:@"foo" in:withNullValues], - @"Invalid Query. 'in' filters cannot contain 'null' in the value array."); - FSTAssertThrows( - [coll queryWhereField:@"foo" arrayContainsAny:withNullValues], - @"Invalid Query. 'arrayContainsAny' filters cannot contain 'null' in the value array."); - - NSArray *withNaNValues = @[ @2, @(NAN) ]; - FSTAssertThrows([coll queryWhereField:@"foo" in:withNaNValues], - @"Invalid Query. 'in' filters cannot contain 'NaN' in the value array."); - FSTAssertThrows( - [coll queryWhereField:@"foo" arrayContainsAny:withNaNValues], - @"Invalid Query. 'arrayContainsAny' filters cannot contain 'NaN' in the value array."); } #pragma mark - GeoPoint Validation diff --git a/Firestore/core/src/api/query_core.cc b/Firestore/core/src/api/query_core.cc index 3d6f2df5e47..45440ef44f3 100644 --- a/Firestore/core/src/api/query_core.cc +++ b/Firestore/core/src/api/query_core.cc @@ -394,24 +394,6 @@ void Query::ValidateDisjunctiveFilterElements( " elements in the value array.", Describe(op)); } - - std::vector array = field_value.array_value(); - for (const auto& val : array) { - if (op == Operator::In || op == Operator::ArrayContainsAny) { - if (val.is_null()) { - ThrowInvalidArgument( - "Invalid Query. '%s' filters cannot contain 'null' in" - " the value array.", - Describe(op)); - } - if (val.is_nan()) { - ThrowInvalidArgument( - "Invalid Query. '%s' filters cannot contain 'NaN' in" - " the value array.", - Describe(op)); - } - } - } } FieldValue Query::ParseExpectedReferenceValue( diff --git a/Firestore/core/src/core/field_filter.cc b/Firestore/core/src/core/field_filter.cc index 89b5a499d8e..f0b066f5c77 100644 --- a/Firestore/core/src/core/field_filter.cc +++ b/Firestore/core/src/core/field_filter.cc @@ -94,25 +94,6 @@ FieldFilter FieldFilter::Create(FieldPath path, CanonicalName(op)); return KeyFieldFilter(std::move(path), op, std::move(value_rhs)); } - - } else if (value_rhs.type() == FieldValue::Type::Null) { - if (op != Filter::Operator::Equal && op != Filter::Operator::NotEqual) { - ThrowInvalidArgument( - "Invalid Query. Null supports only 'equalTo' and 'notEqualTo' " - "comparisons."); - } - Rep filter(std::move(path), op, std::move(value_rhs)); - return FieldFilter(std::make_shared(std::move(filter))); - - } else if (value_rhs.is_nan()) { - if (op != Filter::Operator::Equal && op != Filter::Operator::NotEqual) { - ThrowInvalidArgument( - "Invalid Query. NaN supports only 'equalTo' and 'notEqualTo' " - "comparisons."); - } - Rep filter(std::move(path), op, std::move(value_rhs)); - return FieldFilter(std::make_shared(std::move(filter))); - } else if (op == Operator::ArrayContains) { return ArrayContainsFilter(std::move(path), std::move(value_rhs)); From c09a8a1f249d926295b00fd9102152efd31ef884 Mon Sep 17 00:00:00 2001 From: Brian Chen Date: Thu, 29 Oct 2020 16:42:45 -0700 Subject: [PATCH 2/3] fix test --- Firestore/Example/Tests/Integration/API/FIRQueryTests.mm | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/Firestore/Example/Tests/Integration/API/FIRQueryTests.mm b/Firestore/Example/Tests/Integration/API/FIRQueryTests.mm index 187b0e7f311..4ca23933819 100644 --- a/Firestore/Example/Tests/Integration/API/FIRQueryTests.mm +++ b/Firestore/Example/Tests/Integration/API/FIRQueryTests.mm @@ -633,22 +633,22 @@ - (void)testQueriesCanUseArrayContainsAnyFilters { ])); // With null. - snapshot = [self readDocumentSetForRef:[collection queryWhereField:@"zip" + snapshot = [self readDocumentSetForRef:[collection queryWhereField:@"array" arrayContainsAny:@[ [NSNull null] ]]]; XCTAssertTrue(snapshot.isEmpty); // With null and a value. - snapshot = [self readDocumentSetForRef:[collection queryWhereField:@"zip" + snapshot = [self readDocumentSetForRef:[collection queryWhereField:@"array" arrayContainsAny:@[ [NSNull null], @43 ]]]; XCTAssertEqualObjects(FIRQuerySnapshotGetData(snapshot), (@[ testDocs[@"e"] ])); // With NAN. - snapshot = [self readDocumentSetForRef:[collection queryWhereField:@"zip" + snapshot = [self readDocumentSetForRef:[collection queryWhereField:@"array" arrayContainsAny:@[ @(NAN) ]]]; XCTAssertTrue(snapshot.isEmpty); // With NAN and a value. - snapshot = [self readDocumentSetForRef:[collection queryWhereField:@"zip" + snapshot = [self readDocumentSetForRef:[collection queryWhereField:@"array" arrayContainsAny:@[ @(NAN), @43 ]]]; XCTAssertEqualObjects(FIRQuerySnapshotGetData(snapshot), (@[ testDocs[@"e"] ])); } From 611dd6e9ac3db0751d211b5e5c715c016b14dc68 Mon Sep 17 00:00:00 2001 From: Brian Chen Date: Mon, 2 Nov 2020 12:00:15 -0600 Subject: [PATCH 3/3] fix period in changelog --- Firestore/CHANGELOG.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Firestore/CHANGELOG.md b/Firestore/CHANGELOG.md index d8918028196..0a387ba8907 100644 --- a/Firestore/CHANGELOG.md +++ b/Firestore/CHANGELOG.md @@ -1,5 +1,6 @@ # Unreleased -- [fixed] Remove explicit MobileCoreServices library linkage from podspec. (#6850) +- [fixed] Remove explicit MobileCoreServices library linkage from podspec + (#6850). - [fixed] Removed excess validation of null and NaN values in query filters. This more closely aligns the SDK with the Firestore backend, which has always accepted null and NaN for all operators, even though this isn't necessarily