Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove null and NaN checks in filters #3960

Merged
merged 5 commits into from
Oct 20, 2020
Merged

Remove null and NaN checks in filters #3960

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
cf911bf
Remove null and NaN checks in filters
Oct 16, 2020
f7bd398
lint
Oct 19, 2020
0842b65
revert common/ file
Oct 19, 2020
4a6ea59
Merge branch 'master' into bc/remove-null-check
Oct 20, 2020
efd4ed1
resolve gil comments
Oct 20, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/wicked-cups-search.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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.
16 changes: 0 additions & 16 deletions packages/firestore/src/api/database.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.'
);
}
}
}

/**
Expand Down
19 changes: 0 additions & 19 deletions packages/firestore/src/core/query.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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) {
Expand Down
63 changes: 60 additions & 3 deletions packages/firestore/test/integration/api/query.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 => {
Expand All @@ -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([]);
});
});

Expand All @@ -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 => {
Expand All @@ -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();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need a test that shows that "in" with null/nan and something else still finds the something else?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

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 }]);
});
});

Expand Down Expand Up @@ -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 => {
Expand All @@ -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] }]);
});
});

Expand Down
73 changes: 0 additions & 73 deletions packages/firestore/test/integration/api/validation.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand 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.'
);
}
);

Expand Down