From cf911bfc780fadb778e4ea7ca7fc90179f3628be Mon Sep 17 00:00:00 2001 From: Brian Chen Date: Fri, 16 Oct 2020 17:47:25 -0500 Subject: [PATCH 1/4] Remove null and NaN checks in filters --- .changeset/wicked-cups-search.md | 5 ++ packages/firestore/src/api/database.ts | 21 +----- packages/firestore/src/core/query.ts | 16 ---- .../test/integration/api/query.test.ts | 41 ++++++++++- .../test/integration/api/validation.test.ts | 73 ------------------- 5 files changed, 44 insertions(+), 112 deletions(-) create mode 100644 .changeset/wicked-cups-search.md diff --git a/.changeset/wicked-cups-search.md b/.changeset/wicked-cups-search.md new file mode 100644 index 00000000000..f369e5ace14 --- /dev/null +++ b/.changeset/wicked-cups-search.md @@ -0,0 +1,5 @@ +--- +'@firebase/firestore': minor +--- + +Removed validation of `null` and `NaN` values in filters. However, note that only `==` and `!=` filters match against `null` and `NaN`. diff --git a/packages/firestore/src/api/database.ts b/packages/firestore/src/api/database.ts index bf64f8970e9..38e548cd132 100644 --- a/packages/firestore/src/api/database.ts +++ b/packages/firestore/src/api/database.ts @@ -111,10 +111,7 @@ import { valueDescription, validateIsNotUsedTogether } from '../util/input_validation'; -import { - setLogLevel as setClientLogLevel, - logWarn -} from '../util/log'; +import { setLogLevel as setClientLogLevel, logWarn } from '../util/log'; import { AutoId } from '../util/misc'; import { Deferred } from '../util/promise'; import { FieldPath as ExternalFieldPath } from './field_path'; @@ -1819,22 +1816,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 index 8081fc74beb..a9c83c073ce 100644 --- a/packages/firestore/src/core/query.ts +++ b/packages/firestore/src/core/query.ts @@ -603,22 +603,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 index 4e7a2c1543e..98c0dd6143e 100644 --- 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,14 @@ 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 NaN. + const snapshot4 = await coll.where('zip', 'in', [Number.NaN]).get(); + expect(toDataArray(snapshot4)).to.deep.equal([]); }); }); @@ -913,7 +934,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 +955,18 @@ 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('zip', 'array-contains-any', [null]) + .get(); + expect(toDataArray(snapshot3)).to.deep.equal([]); + + // With NaN. + const snapshot4 = await coll + .where('zip', 'array-contains-any', [Number.NaN]) + .get(); + expect(toDataArray(snapshot4)).to.deep.equal([]); }); }); diff --git a/packages/firestore/test/integration/api/validation.test.ts b/packages/firestore/test/integration/api/validation.test.ts index 223081dad95..6202dbcada7 100644 --- a/packages/firestore/test/integration/api/validation.test.ts +++ b/packages/firestore/test/integration/api/validation.test.ts @@ -943,51 +943,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 @@ -1478,34 +1433,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.' - ); } ); From f7bd39823a9c6f2f5a41053f56b35d3388c14f40 Mon Sep 17 00:00:00 2001 From: Brian Chen Date: Mon, 19 Oct 2020 09:42:24 -0500 Subject: [PATCH 2/4] lint --- common/api-review/functions-exp.api.md | 2 +- packages/firestore/src/core/query.ts | 3 --- 2 files changed, 1 insertion(+), 4 deletions(-) diff --git a/common/api-review/functions-exp.api.md b/common/api-review/functions-exp.api.md index 947534f6548..4f719b67983 100644 --- a/common/api-review/functions-exp.api.md +++ b/common/api-review/functions-exp.api.md @@ -16,7 +16,7 @@ export function getFunctions(app: FirebaseApp, regionOrCustomDomain?: string): F export function httpsCallable(functionsInstance: Functions, name: string, options?: HttpsCallableOptions): HttpsCallable; // @public -export function useFunctionsEmulator(functionsInstance: Functions, origin: string): void; +export function useFunctionsEmulator(functionsInstance: Functions, host: string, port: number): void; // (No @packageDocumentation comment for this package) diff --git a/packages/firestore/src/core/query.ts b/packages/firestore/src/core/query.ts index a9c83c073ce..2bb83e75699 100644 --- a/packages/firestore/src/core/query.ts +++ b/packages/firestore/src/core/query.ts @@ -23,8 +23,6 @@ import { arrayValueContains, canonicalId, isArray, - isNanValue, - isNullValue, isReferenceValue, typeOrder, valueCompare, @@ -32,7 +30,6 @@ import { } 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, From 0842b6580d6e40f744d27125f36c39ee3792f2bd Mon Sep 17 00:00:00 2001 From: Brian Chen Date: Mon, 19 Oct 2020 11:09:59 -0500 Subject: [PATCH 3/4] revert common/ file --- common/api-review/functions-exp.api.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/common/api-review/functions-exp.api.md b/common/api-review/functions-exp.api.md index 4f719b67983..947534f6548 100644 --- a/common/api-review/functions-exp.api.md +++ b/common/api-review/functions-exp.api.md @@ -16,7 +16,7 @@ export function getFunctions(app: FirebaseApp, regionOrCustomDomain?: string): F export function httpsCallable(functionsInstance: Functions, name: string, options?: HttpsCallableOptions): HttpsCallable; // @public -export function useFunctionsEmulator(functionsInstance: Functions, host: string, port: number): void; +export function useFunctionsEmulator(functionsInstance: Functions, origin: string): void; // (No @packageDocumentation comment for this package) From efd4ed15086529d2fa720752c9b9a3881a55e0e5 Mon Sep 17 00:00:00 2001 From: Brian Chen Date: Tue, 20 Oct 2020 13:45:25 -0500 Subject: [PATCH 4/4] resolve gil comments --- .changeset/wicked-cups-search.md | 2 +- .../test/integration/api/query.test.ts | 34 +++++++++++++++---- .../test/integration/api/validation.test.ts | 8 ----- 3 files changed, 29 insertions(+), 15 deletions(-) diff --git a/.changeset/wicked-cups-search.md b/.changeset/wicked-cups-search.md index f369e5ace14..abfaf518003 100644 --- a/.changeset/wicked-cups-search.md +++ b/.changeset/wicked-cups-search.md @@ -2,4 +2,4 @@ '@firebase/firestore': minor --- -Removed validation of `null` and `NaN` values in filters. However, note that only `==` and `!=` filters match against `null` and `NaN`. +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/test/integration/api/query.test.ts b/packages/firestore/test/integration/api/query.test.ts index 98c0dd6143e..f8936e1fe32 100644 --- a/packages/firestore/test/integration/api/query.test.ts +++ b/packages/firestore/test/integration/api/query.test.ts @@ -818,9 +818,19 @@ apiDescribe('Queries', (persistence: boolean) => { 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 snapshot4 = await coll.where('zip', 'in', [Number.NaN]).get(); - expect(toDataArray(snapshot4)).to.deep.equal([]); + 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 }]); }); }); @@ -958,15 +968,27 @@ apiDescribe('Queries', (persistence: boolean) => { // With null. const snapshot3 = await coll - .where('zip', 'array-contains-any', [null]) + .where('array', 'array-contains-any', [null]) .get(); expect(toDataArray(snapshot3)).to.deep.equal([]); - // With NaN. + // With null and a value. const snapshot4 = await coll - .where('zip', 'array-contains-any', [Number.NaN]) + .where('array', 'array-contains-any', [43, null]) .get(); - expect(toDataArray(snapshot4)).to.deep.equal([]); + 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 index 1de6d07b601..a5f7beff894 100644 --- a/packages/firestore/test/integration/api/validation.test.ts +++ b/packages/firestore/test/integration/api/validation.test.ts @@ -711,14 +711,6 @@ apiDescribe('Validation:', (persistence: boolean) => { ); }); - validationIt(persistence, 'enum', db => { - const collection = db.collection('test') as any; - expect(() => collection.where('a', 'foo' as any, 'b')).to.throw( - 'Invalid value "foo" provided to function Query.where() for its second argument. ' + - 'Acceptable values: <, <=, ==, !=, >=, >, array-contains, in, array-contains-any, not-in' - ); - }); - it('cannot be created from documents missing sort values', () => { const testDocs = { f: { k: 'f', nosort: 1 } // should not show up