Allow null and NaN values in filters (#2115)

Author: Brian Chen

firebase-firestore/CHANGELOG.md

@@ -1,4 +1,10 @@
-# Unreleased (22.0.0)
+# Unreleased
+- [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.
+
+# (22.0.0)
 - [changed] Removed the deprecated `timestampsInSnapshotsEnabled` setting.
   Any timestamps in Firestore documents are now returned as `Timestamps`. To
   convert `Timestamp` classed to `java.util.Date`, use `Timestamp.toDate()`.

firebase-firestore/src/androidTest/java/com/google/firebase/firestore/QueryTest.java

@@ -14,6 +14,7 @@
 
 package com.google.firebase.firestore;
 
+import static com.google.firebase.firestore.testutil.IntegrationTestUtil.nullList;
 import static com.google.firebase.firestore.testutil.IntegrationTestUtil.querySnapshotToIds;
 import static com.google.firebase.firestore.testutil.IntegrationTestUtil.querySnapshotToValues;
 import static com.google.firebase.firestore.testutil.IntegrationTestUtil.testCollection;
@@ -562,15 +563,20 @@ public void testQueriesCanUseArrayContainsFilters() {
     Map<String, Object> docB = map("array", asList("a", 42L, "c"));
     Map<String, Object> docC = map("array", asList(41.999, "42", map("a", asList(42))));
     Map<String, Object> docD = map("array", asList(42L), "array2", asList("bingo"));
+    Map<String, Object> docE = map("array", nullList());
+    Map<String, Object> docF = map("array", asList(Double.NaN));
     CollectionReference collection =
-        testCollectionWithDocs(map("a", docA, "b", docB, "c", docC, "d", docD));
+        testCollectionWithDocs(
+            map("a", docA, "b", docB, "c", docC, "d", docD, "e", docE, "f", docF));
 
     // Search for "array" to contain 42
     QuerySnapshot snapshot = waitFor(collection.whereArrayContains("array", 42L).get());
     assertEquals(asList(docA, docB, docD), querySnapshotToValues(snapshot));
 
-    // NOTE: The backend doesn't currently support null, NaN, objects, or arrays, so there isn't
-    // much of anything else interesting to test.
+    // Note: whereArrayContains() requires a non-null value parameter, so no null test is needed.
+    // With NaN.
+    snapshot = waitFor(collection.whereArrayContains("array", Double.NaN).get());
+    assertEquals(new ArrayList<>(), querySnapshotToValues(snapshot));
   }
 
   @Test
@@ -582,9 +588,14 @@ public void testQueriesCanUseInFilters() {
     Map<String, Object> docE = map("zip", asList("98101", map("zip", 98101L)));
     Map<String, Object> docF = map("zip", map("code", 500L));
     Map<String, Object> docG = map("zip", asList(98101L, 98102L));
+    Map<String, Object> docH = map("zip", null);
+    Map<String, Object> docI = map("zip", Double.NaN);
+
     CollectionReference collection =
         testCollectionWithDocs(
-            map("a", docA, "b", docB, "c", docC, "d", docD, "e", docE, "f", docF, "g", docG));
+            map(
+                "a", docA, "b", docB, "c", docC, "d", docD, "e", docE, "f", docF, "g", docG, "h",
+                docH, "i", docI));
 
     // Search for zips matching 98101, 98103, or [98101, 98102].
     QuerySnapshot snapshot =
@@ -594,6 +605,24 @@ public void testQueriesCanUseInFilters() {
     // With objects.
     snapshot = waitFor(collection.whereIn("zip", asList(map("code", 500L))).get());
     assertEquals(asList(docF), querySnapshotToValues(snapshot));
+
+    // With null.
+    snapshot = waitFor(collection.whereIn("zip", nullList()).get());
+    assertEquals(new ArrayList<>(), querySnapshotToValues(snapshot));
+
+    // With null and a value.
+    List<Object> inputList = nullList();
+    inputList.add(98101L);
+    snapshot = waitFor(collection.whereIn("zip", inputList).get());
+    assertEquals(asList(docA), querySnapshotToValues(snapshot));
+
+    // With NaN.
+    snapshot = waitFor(collection.whereIn("zip", asList(Double.NaN)).get());
+    assertEquals(new ArrayList<>(), querySnapshotToValues(snapshot));
+
+    // With NaN and a value.
+    snapshot = waitFor(collection.whereIn("zip", asList(Double.NaN, 98101L)).get());
+    assertEquals(asList(docA), querySnapshotToValues(snapshot));
   }
 
   @Test
@@ -656,9 +685,7 @@ public void testQueriesCanUseNotInFilters() {
     assertEquals(Lists.newArrayList(expectedDocsMap.values()), querySnapshotToValues(snapshot));
 
     // With Null.
-    List<Object> nullArray = new ArrayList<>();
-    nullArray.add(null);
-    snapshot = waitFor(collection.whereNotIn("zip", nullArray).get());
+    snapshot = waitFor(collection.whereNotIn("zip", nullList()).get());
     assertEquals(new ArrayList<>(), querySnapshotToValues(snapshot));
 
     // With NaN.
@@ -706,10 +733,14 @@ public void testQueriesCanUseArrayContainsAnyFilters() {
     Map<String, Object> docE = map("array", asList(43L));
     Map<String, Object> docF = map("array", asList(map("a", 42L)));
     Map<String, Object> docG = map("array", 42L);
+    Map<String, Object> docH = map("array", nullList());
+    Map<String, Object> docI = map("array", asList(Double.NaN));
 
     CollectionReference collection =
         testCollectionWithDocs(
-            map("a", docA, "b", docB, "c", docC, "d", docD, "e", docE, "f", docF, "g", docG));
+            map(
+                "a", docA, "b", docB, "c", docC, "d", docD, "e", docE, "f", docF, "g", docG, "h",
+                docH, "i", docI));
 
     // Search for "array" to contain [42, 43].
     QuerySnapshot snapshot =
@@ -719,6 +750,24 @@ public void testQueriesCanUseArrayContainsAnyFilters() {
     // With objects.
     snapshot = waitFor(collection.whereArrayContainsAny("array", asList(map("a", 42L))).get());
     assertEquals(asList(docF), querySnapshotToValues(snapshot));
+
+    // With null.
+    snapshot = waitFor(collection.whereArrayContainsAny("array", nullList()).get());
+    assertEquals(new ArrayList<>(), querySnapshotToValues(snapshot));
+
+    // With null and a value.
+    List<Object> inputList = nullList();
+    inputList.add(43L);
+    snapshot = waitFor(collection.whereArrayContainsAny("array", inputList).get());
+    assertEquals(asList(docE), querySnapshotToValues(snapshot));
+
+    // With NaN.
+    snapshot = waitFor(collection.whereArrayContainsAny("array", asList(Double.NaN)).get());
+    assertEquals(new ArrayList<>(), querySnapshotToValues(snapshot));
+
+    // With NaN and a value.
+    snapshot = waitFor(collection.whereArrayContainsAny("array", asList(Double.NaN, 43L)).get());
+    assertEquals(asList(docE), querySnapshotToValues(snapshot));
   }
 
   @Test

firebase-firestore/src/androidTest/java/com/google/firebase/firestore/ValidationTest.java

@@ -409,37 +409,6 @@ public void queriesWithNonPositiveLimitFail() {
         "Invalid Query. Query limitToLast (-1) is invalid. Limit must be positive.");
   }
 
-  @Test
-  public void queriesWithNullOrNaNFiltersOtherThanEqualityFail() {
-    CollectionReference collection = testCollection();
-    expectError(
-        () -> collection.whereGreaterThan("a", null),
-        "Invalid Query. Null only supports comparisons via "
-            + "whereEqualTo() and whereNotEqualTo().");
-    expectError(
-        () -> collection.whereArrayContains("a", null),
-        "Invalid Query. Null only supports comparisons via "
-            + "whereEqualTo() and whereNotEqualTo().");
-    expectError(
-        () -> collection.whereArrayContainsAny("a", null),
-        "Invalid Query. A non-empty array is required for 'array_contains_any' filters.");
-    expectError(
-        () -> collection.whereIn("a", null),
-        "Invalid Query. A non-empty array is required for 'in' filters.");
-    expectError(
-        () -> collection.whereNotIn("a", null),
-        "Invalid Query. A non-empty array is required for 'not_in' filters.");
-
-    expectError(
-        () -> collection.whereGreaterThan("a", Double.NaN),
-        "Invalid Query. NaN only supports comparisons via "
-            + "whereEqualTo() and whereNotEqualTo().");
-    expectError(
-        () -> collection.whereArrayContains("a", Double.NaN),
-        "Invalid Query. NaN only supports comparisons via "
-            + "whereEqualTo() and whereNotEqualTo().");
-  }
-
   @Test
   public void queriesCannotBeCreatedFromDocumentsMissingSortValues() {
     CollectionReference collection = testCollectionWithDocs(map("f", map("k", "f", "nosort", 1.0)));
@@ -757,22 +726,6 @@ public void queriesInAndArrayContainsAnyArrayRules() {
         () ->
             testCollection().whereArrayContainsAny("bar", asList(1, 2, 3, 4, 5, 6, 7, 8, 9, 9, 9)),
         "Invalid Query. 'array_contains_any' filters support a maximum of 10 elements in the value array.");
-
-    expectError(
-        () -> testCollection().whereIn("bar", asList("foo", null)),
-        "Invalid Query. 'in' filters cannot contain 'null' in the value array.");
-
-    expectError(
-        () -> testCollection().whereArrayContainsAny("bar", asList("foo", null)),
-        "Invalid Query. 'array_contains_any' filters cannot contain 'null' in the value array.");
-
-    expectError(
-        () -> testCollection().whereIn("bar", asList("foo", Double.NaN)),
-        "Invalid Query. 'in' filters cannot contain 'NaN' in the value array.");
-
-    expectError(
-        () -> testCollection().whereArrayContainsAny("bar", asList("foo", Float.NaN)),
-        "Invalid Query. 'array_contains_any' filters cannot contain 'NaN' in the value array.");
   }
 
   @Test

firebase-firestore/src/androidTest/java/com/google/firebase/firestore/testutil/IntegrationTestUtil.java

@@ -445,4 +445,10 @@ public static boolean isRunningAgainstEmulator() {
   public static void testChangeUserTo(User user) {
     MockCredentialsProvider.instance().changeUserTo(user);
   }
+
+  public static List<Object> nullList() {
+    List<Object> nullArray = new ArrayList<>();
+    nullArray.add(null);
+    return nullArray;
+  }
 }

firebase-firestore/src/main/java/com/google/firebase/firestore/Query.java

@@ -493,20 +493,6 @@ private void validateDisjunctiveFilterElements(Object value, Operator op) {
               + op.toString()
               + "' filters support a maximum of 10 elements in the value array.");
     }
-    if (op == Operator.IN || op == Operator.ARRAY_CONTAINS_ANY) {
-      if (((List) value).contains(null)) {
-        throw new IllegalArgumentException(
-            "Invalid Query. '"
-                + op.toString()
-                + "' filters cannot contain 'null' in the value array.");
-      }
-      if (((List) value).contains(Double.NaN) || ((List) value).contains(Float.NaN)) {
-        throw new IllegalArgumentException(
-            "Invalid Query. '"
-                + op.toString()
-                + "' filters cannot contain 'NaN' in the value array.");
-      }
-    }
   }
 
   private void validateOrderByFieldMatchesInequality(

firebase-firestore/src/main/java/com/google/firebase/firestore/core/FieldFilter.java

@@ -72,20 +72,6 @@ public static FieldFilter create(FieldPath path, Operator operator, Value value)
             operator.toString() + "queries don't make sense on document keys");
         return new KeyFieldFilter(path, operator, value);
       }
-    } else if (Values.isNullValue(value)) {
-      if (operator != Operator.EQUAL && operator != Operator.NOT_EQUAL) {
-        throw new IllegalArgumentException(
-            "Invalid Query. Null only supports comparisons via "
-                + "whereEqualTo() and whereNotEqualTo().");
-      }
-      return new FieldFilter(path, operator, value);
-    } else if (Values.isNanValue(value)) {
-      if (operator != Operator.EQUAL && operator != Operator.NOT_EQUAL) {
-        throw new IllegalArgumentException(
-            "Invalid Query. NaN only supports comparisons via "
-                + "whereEqualTo() and whereNotEqualTo().");
-      }
-      return new FieldFilter(path, operator, value);
     } else if (operator == Operator.ARRAY_CONTAINS) {
       return new ArrayContainsFilter(path, value);
     } else if (operator == Operator.IN) {

firebase-firestore/src/test/java/com/google/firebase/firestore/core/QueryTest.java

@@ -15,7 +15,6 @@
 package com.google.firebase.firestore.core;
 
 import static com.google.firebase.firestore.model.DocumentKey.KEY_FIELD_NAME;
-import static com.google.firebase.firestore.testutil.Assert.assertThrows;
 import static com.google.firebase.firestore.testutil.TestUtil.doc;
 import static com.google.firebase.firestore.testutil.TestUtil.filter;
 import static com.google.firebase.firestore.testutil.TestUtil.map;
@@ -352,15 +351,6 @@ public void testNullFilter() {
     assertTrue(query.matches(doc6));
   }
 
-  @Test
-  public void testOnlySupportsEqualsForNull() {
-    List<String> invalidOps = asList("<", "<=", ">", ">=");
-    Query query = Query.atPath(ResourcePath.fromString("collection"));
-    for (String op : invalidOps) {
-      assertThrows(IllegalArgumentException.class, () -> query.filter(filter("sort", op, null)));
-    }
-  }
-
   @Test
   public void testComplexObjectFilters() {
     Query query1 =