Remove null and NaN checks in filters (#3960)

firebase · Oct 20, 2020 · 9719635 · 9719635
1 parent b247ffa
commit 9719635
Show file tree

Hide file tree

Showing 5 changed files with 65 additions and 111 deletions.
diff --git a/.changeset/wicked-cups-search.md b/.changeset/wicked-cups-search.md
@@ -0,0 +1,5 @@
+---
+'@firebase/firestore': minor
+---
+
+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.
diff --git a/packages/firestore/src/api/database.ts b/packages/firestore/src/api/database.ts
@@ -1672,22 +1672,6 @@ function validateDisjunctiveFilterElements(
         'maximum of 10 elements in the value array.'
     );
   }
-  if (operator === Operator.IN || operator === Operator.ARRAY_CONTAINS_ANY) {
-    if (value.indexOf(null) >= 0) {
-      throw new FirestoreError(
-        Code.INVALID_ARGUMENT,
-        `Invalid Query. '${operator.toString()}' filters cannot contain 'null' ` +
-          'in the value array.'
-      );
-    }
-    if (value.filter(element => Number.isNaN(element)).length > 0) {
-      throw new FirestoreError(
-        Code.INVALID_ARGUMENT,
-        `Invalid Query. '${operator.toString()}' filters cannot contain 'NaN' ` +
-          'in the value array.'
-      );
-    }
-  }
 }
 
 /**

diff --git a/packages/firestore/src/core/query.ts b/packages/firestore/src/core/query.ts
@@ -23,16 +23,13 @@ import {
   arrayValueContains,
   canonicalId,
   isArray,
-  isNanValue,
-  isNullValue,
   isReferenceValue,
   typeOrder,
   valueCompare,
   valueEquals
 } from '../model/values';
 import { FieldPath, ResourcePath } from '../model/path';
 import { debugAssert, debugCast, fail } from '../util/assert';
-import { Code, FirestoreError } from '../util/error';
 import { isNullOrUndefined } from '../util/types';
 import {
   canonifyTarget,
@@ -603,22 +600,6 @@ export class FieldFilter extends Filter {
         );
         return new KeyFieldFilter(field, op, value);
       }
-    } else if (isNullValue(value)) {
-      if (op !== Operator.EQUAL && op !== Operator.NOT_EQUAL) {
-        throw new FirestoreError(
-          Code.INVALID_ARGUMENT,
-          "Invalid query. Null only supports '==' and '!=' comparisons."
-        );
-      }
-      return new FieldFilter(field, op, value);
-    } else if (isNanValue(value)) {
-      if (op !== Operator.EQUAL && op !== Operator.NOT_EQUAL) {
-        throw new FirestoreError(
-          Code.INVALID_ARGUMENT,
-          "Invalid query. NaN only supports '==' and '!=' comparisons."
-        );
-      }
-      return new FieldFilter(field, op, value);
     } else if (op === Operator.ARRAY_CONTAINS) {
       return new ArrayContainsFilter(field, value);
     } else if (op === Operator.IN) {

diff --git a/packages/firestore/test/integration/api/query.test.ts b/packages/firestore/test/integration/api/query.test.ts
@@ -759,7 +759,9 @@ apiDescribe('Queries', (persistence: boolean) => {
       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: [null] },
+      f: { array: [Number.NaN] }
     };
 
     await withTestCollection(persistence, testDocs, async coll => {
@@ -773,6 +775,15 @@ apiDescribe('Queries', (persistence: boolean) => {
 
       // 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.
+      const snapshot3 = await coll.where('zip', 'array-contains', null).get();
+      expect(toDataArray(snapshot3)).to.deep.equal([]);
+
+      // With NaN.
+      const snapshot4 = await coll
+        .where('zip', 'array-contains', Number.NaN)
+        .get();
+      expect(toDataArray(snapshot4)).to.deep.equal([]);
     });
   });
 
@@ -784,7 +795,9 @@ apiDescribe('Queries', (persistence: boolean) => {
       d: { zip: [98101] },
       e: { zip: ['98101', { zip: 98101 }] },
       f: { zip: { code: 500 } },
-      g: { zip: [98101, 98102] }
+      g: { zip: [98101, 98102] },
+      h: { zip: null },
+      i: { zip: Number.NaN }
     };
 
     await withTestCollection(persistence, testDocs, async coll => {
@@ -800,6 +813,24 @@ apiDescribe('Queries', (persistence: boolean) => {
       // With objects.
       const snapshot2 = await coll.where('zip', 'in', [{ code: 500 }]).get();
       expect(toDataArray(snapshot2)).to.deep.equal([{ zip: { code: 500 } }]);
+
+      // With null.
+      const snapshot3 = await coll.where('zip', 'in', [null]).get();
+      expect(toDataArray(snapshot3)).to.deep.equal([]);
+
+      // With null and a value.
+      const snapshot4 = await coll.where('zip', 'in', [98101, null]).get();
+      expect(toDataArray(snapshot4)).to.deep.equal([{ zip: 98101 }]);
+
+      // With NaN.
+      const snapshot5 = await coll.where('zip', 'in', [Number.NaN]).get();
+      expect(toDataArray(snapshot5)).to.deep.equal([]);
+
+      // With NaN and a value.
+      const snapshot6 = await coll
+        .where('zip', 'in', [98101, Number.NaN])
+        .get();
+      expect(toDataArray(snapshot6)).to.deep.equal([{ zip: 98101 }]);
     });
   });
 
@@ -913,7 +944,9 @@ apiDescribe('Queries', (persistence: boolean) => {
       d: { array: [42], array2: ['bingo'] },
       e: { array: [43] },
       f: { array: [{ a: 42 }] },
-      g: { array: 42 }
+      g: { array: 42 },
+      h: { array: [null] },
+      i: { array: [Number.NaN] }
     };
 
     await withTestCollection(persistence, testDocs, async coll => {
@@ -932,6 +965,30 @@ apiDescribe('Queries', (persistence: boolean) => {
         .where('array', 'array-contains-any', [{ a: 42 }])
         .get();
       expect(toDataArray(snapshot2)).to.deep.equal([{ array: [{ a: 42 }] }]);
+
+      // With null.
+      const snapshot3 = await coll
+        .where('array', 'array-contains-any', [null])
+        .get();
+      expect(toDataArray(snapshot3)).to.deep.equal([]);
+
+      // With null and a value.
+      const snapshot4 = await coll
+        .where('array', 'array-contains-any', [43, null])
+        .get();
+      expect(toDataArray(snapshot4)).to.deep.equal([{ array: [43] }]);
+
+      // With NaN.
+      const snapshot5 = await coll
+        .where('array', 'array-contains-any', [Number.NaN])
+        .get();
+      expect(toDataArray(snapshot5)).to.deep.equal([]);
+
+      // With NaN and a value.
+      const snapshot6 = await coll
+        .where('array', 'array-contains-any', [43, Number.NaN])
+        .get();
+      expect(toDataArray(snapshot6)).to.deep.equal([{ array: [43] }]);
     });
   });
 

diff --git a/packages/firestore/test/integration/api/validation.test.ts b/packages/firestore/test/integration/api/validation.test.ts
@@ -711,51 +711,6 @@ apiDescribe('Validation:', (persistence: boolean) => {
       );
     });
 
-    validationIt(
-      persistence,
-      'with null or NaN non-equality filters fail',
-      db => {
-        const collection = db.collection('test');
-        expect(() => collection.where('a', '>', null)).to.throw(
-          "Invalid query. Null only supports '==' and '!=' comparisons."
-        );
-        expect(() => collection.where('a', 'array-contains', null)).to.throw(
-          "Invalid query. Null only supports '==' and '!=' comparisons."
-        );
-        expect(() => collection.where('a', 'in', null)).to.throw(
-          "Invalid Query. A non-empty array is required for 'in' filters."
-        );
-        expect(() => collection.where('a', 'not-in', null)).to.throw(
-          "Invalid Query. A non-empty array is required for 'not-in' filters."
-        );
-        expect(() =>
-          collection.where('a', 'array-contains-any', null)
-        ).to.throw(
-          "Invalid Query. A non-empty array is required for 'array-contains-any' filters."
-        );
-
-        expect(() => collection.where('a', '>', Number.NaN)).to.throw(
-          "Invalid query. NaN only supports '==' and '!=' comparisons."
-        );
-        expect(() =>
-          collection.where('a', 'array-contains', Number.NaN)
-        ).to.throw(
-          "Invalid query. NaN only supports '==' and '!=' comparisons."
-        );
-        expect(() => collection.where('a', 'in', Number.NaN)).to.throw(
-          "Invalid Query. A non-empty array is required for 'in' filters."
-        );
-        expect(() => collection.where('a', 'not-in', Number.NaN)).to.throw(
-          "Invalid Query. A non-empty array is required for 'not-in' filters."
-        );
-        expect(() =>
-          collection.where('a', 'array-contains-any', Number.NaN)
-        ).to.throw(
-          "Invalid Query. A non-empty array is required for 'array-contains-any' filters."
-        );
-      }
-    );
-
     it('cannot be created from documents missing sort values', () => {
       const testDocs = {
         f: { k: 'f', nosort: 1 } // should not show up
@@ -1246,34 +1201,6 @@ apiDescribe('Validation:', (persistence: boolean) => {
           'Invalid Query. A non-empty array is required for ' +
             "'array-contains-any' filters."
         );
-
-        expect(() =>
-          db.collection('test').where('foo', 'in', [3, null])
-        ).to.throw(
-          "Invalid Query. 'in' filters cannot contain 'null' in the value array."
-        );
-
-        expect(() =>
-          db.collection('test').where('foo', 'array-contains-any', [3, null])
-        ).to.throw(
-          "Invalid Query. 'array-contains-any' filters cannot contain 'null' " +
-            'in the value array.'
-        );
-
-        expect(() =>
-          db.collection('test').where('foo', 'in', [2, Number.NaN])
-        ).to.throw(
-          "Invalid Query. 'in' filters cannot contain 'NaN' in the value array."
-        );
-
-        expect(() =>
-          db
-            .collection('test')
-            .where('foo', 'array-contains-any', [2, Number.NaN])
-        ).to.throw(
-          "Invalid Query. 'array-contains-any' filters cannot contain 'NaN' " +
-            'in the value array.'
-        );
       }
     );