Allow null/NAN values in filters (#6871)

Author: Brian Chen

Firestore/CHANGELOG.md

@@ -1,5 +1,10 @@
 # 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`

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:@"array"
+                                                    arrayContainsAny:@[ [NSNull null] ]]];
+  XCTAssertTrue(snapshot.isEmpty);
+
+  // With null and a value.
+  snapshot = [self readDocumentSetForRef:[collection queryWhereField:@"array"
+                                                    arrayContainsAny:@[ [NSNull null], @43 ]]];
+  XCTAssertEqualObjects(FIRQuerySnapshotGetData(snapshot), (@[ testDocs[@"e"] ]));
+
+  // With NAN.
+  snapshot = [self readDocumentSetForRef:[collection queryWhereField:@"array"
+                                                    arrayContainsAny:@[ @(NAN) ]]];
+  XCTAssertTrue(snapshot.isEmpty);
+
+  // With NAN and a value.
+  snapshot = [self readDocumentSetForRef:[collection queryWhereField:@"array"
+                                                    arrayContainsAny:@[ @(NAN), @43 ]]];
+  XCTAssertEqualObjects(FIRQuerySnapshotGetData(snapshot), (@[ testDocs[@"e"] ]));
 }
 
 - (void)testCollectionGroupQueries {

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

Firestore/core/src/api/query_core.cc

@@ -394,24 +394,6 @@ void Query::ValidateDisjunctiveFilterElements(
         " elements in the value array.",
         Describe(op));
   }
-
-  std::vector<FieldValue> 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(

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<const Rep>(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<const Rep>(std::move(filter)));
-
   } else if (op == Operator::ArrayContains) {
     return ArrayContainsFilter(std::move(path), std::move(value_rhs));