Skip to content

Commit

Permalink
OR queries - relaxing in restrictions (#7024)
Browse files Browse the repository at this point in the history 
Relaxing query validations performed by the Firestore SDK package
  • Loading branch information
@MarkDuckworth
MarkDuckworth authored Feb 23, 2023
1 parent b970dc5 commit 36558bd
Show file tree
Hide file tree
Showing 6 changed files with 256 additions and 381 deletions.
6 changes: 6 additions & 0 deletions .changeset/heavy-starfishes-count.md
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
"@firebase/firestore-compat": feat
"@firebase/firestore": feat
---

Relaxing query validation performed by the SDK
164 changes: 0 additions & 164 deletions packages/firestore-compat/test/validation.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -901,47 +901,6 @@ apiDescribe('Validation:', (persistence: boolean) => {
}
);

validationIt(persistence, 'with multiple array filters fail', db => {
expect(() =>
db
.collection('test')
.where('foo', 'array-contains', 1)
.where('foo', 'array-contains', 2)
).to.throw(
"Invalid query. You cannot use more than one 'array-contains' filter."
);

expect(() =>
db
.collection('test')
.where('foo', 'array-contains', 1)
.where('foo', 'array-contains-any', [2, 3])
).to.throw(
"Invalid query. You cannot use 'array-contains-any' filters with " +
"'array-contains' filters."
);

expect(() =>
db
.collection('test')
.where('foo', 'array-contains-any', [2, 3])
.where('foo', 'array-contains', 1)
).to.throw(
"Invalid query. You cannot use 'array-contains' filters with " +
"'array-contains-any' filters."
);

expect(() =>
db
.collection('test')
.where('foo', 'not-in', [2, 3])
.where('foo', 'array-contains', 1)
).to.throw(
"Invalid query. You cannot use 'array-contains' filters with " +
"'not-in' filters."
);
});

validationIt(persistence, 'with != and not-in filters fail', db => {
expect(() =>
db
Expand All @@ -963,13 +922,6 @@ apiDescribe('Validation:', (persistence: boolean) => {
});

validationIt(persistence, 'with multiple disjunctive filters fail', db => {
expect(() =>
db
.collection('test')
.where('foo', 'in', [1, 2])
.where('foo', 'in', [2, 3])
).to.throw("Invalid query. You cannot use more than one 'in' filter.");

expect(() =>
db
.collection('test')
Expand All @@ -979,36 +931,6 @@ apiDescribe('Validation:', (persistence: boolean) => {
"Invalid query. You cannot use more than one 'not-in' filter."
);

expect(() =>
db
.collection('test')
.where('foo', 'array-contains-any', [1, 2])
.where('foo', 'array-contains-any', [2, 3])
).to.throw(
"Invalid query. You cannot use more than one 'array-contains-any'" +
' filter.'
);

expect(() =>
db
.collection('test')
.where('foo', 'array-contains-any', [2, 3])
.where('foo', 'in', [2, 3])
).to.throw(
"Invalid query. You cannot use 'in' filters with " +
"'array-contains-any' filters."
);

expect(() =>
db
.collection('test')
.where('foo', 'in', [2, 3])
.where('foo', 'array-contains-any', [2, 3])
).to.throw(
"Invalid query. You cannot use 'array-contains-any' filters with " +
"'in' filters."
);

expect(() =>
db
.collection('test')
Expand Down Expand Up @@ -1046,51 +968,6 @@ apiDescribe('Validation:', (persistence: boolean) => {
).to.throw(
"Invalid query. You cannot use 'not-in' filters with 'in' filters."
);

// This is redundant with the above tests, but makes sure our validation
// doesn't get confused.
expect(() =>
db
.collection('test')
.where('foo', 'in', [2, 3])
.where('foo', 'array-contains', 1)
.where('foo', 'array-contains-any', [2])
).to.throw(
"Invalid query. You cannot use 'array-contains-any' filters with 'in' filters."
);

expect(() =>
db
.collection('test')
.where('foo', 'array-contains', 1)
.where('foo', 'in', [2, 3])
.where('foo', 'array-contains-any', [2])
).to.throw(
"Invalid query. You cannot use 'array-contains-any' filters with " +
"'array-contains' filters."
);

expect(() =>
db
.collection('test')
.where('foo', 'not-in', [2, 3])
.where('foo', 'array-contains', 2)
.where('foo', 'array-contains-any', [2])
).to.throw(
"Invalid query. You cannot use 'array-contains' filters with " +
"'not-in' filters."
);

expect(() =>
db
.collection('test')
.where('foo', 'array-contains', 2)
.where('foo', 'in', [2])
.where('foo', 'not-in', [2, 3])
).to.throw(
"Invalid query. You cannot use 'not-in' filters with " +
"'array-contains' filters."
);
});

validationIt(
Expand All @@ -1110,24 +987,6 @@ apiDescribe('Validation:', (persistence: boolean) => {
.where('foo', 'in', [2, 3])
.where('foo', 'array-contains', 1)
).not.to.throw();

expect(() =>
db
.collection('test')
.where('foo', 'in', [2, 3])
.where('foo', 'array-contains', 1)
.where('foo', 'array-contains', 2)
).to.throw(
"Invalid query. You cannot use more than one 'array-contains' filter."
);

expect(() =>
db
.collection('test')
.where('foo', 'array-contains', 1)
.where('foo', 'in', [2, 3])
.where('foo', 'in', [2, 3])
).to.throw("Invalid query. You cannot use more than one 'in' filter.");
}
);

Expand All @@ -1146,29 +1005,6 @@ apiDescribe('Validation:', (persistence: boolean) => {
"'array-contains-any' filters."
);

expect(() =>
db
.collection('test')
// The 10 element max includes duplicates.
.where('foo', 'in', [1, 2, 3, 4, 5, 6, 7, 8, 9, 9, 9])
).to.throw(
"Invalid Query. 'in' filters support a maximum of 10 elements in " +
'the value array.'
);

expect(() =>
db
.collection('test')
.where(
'foo',
'array-contains-any',
[1, 2, 3, 4, 5, 6, 7, 8, 9, 9, 9]
)
).to.throw(
"Invalid Query. 'array-contains-any' filters support a maximum of " +
'10 elements in the value array.'
);

expect(() => db.collection('test').where('foo', 'in', [])).to.throw(
"Invalid Query. A non-empty array is required for 'in' filters."
);
Expand Down
9 changes: 1 addition & 8 deletions packages/firestore/src/core/query.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ import {
boundSortsAfterDocument,
boundSortsBeforeDocument
} from './bound';
import { CompositeFilter, Filter } from './filter';
import { Filter } from './filter';
import { Direction, OrderBy } from './order_by';
import {
canonifyTarget,
Expand Down Expand Up @@ -165,13 +165,6 @@ export function queryMatchesAllDocuments(query: Query): boolean {
);
}

export function queryContainsCompositeFilters(query: Query): boolean {
return (
query.filters.find(filter => filter instanceof CompositeFilter) !==
undefined
);
}

export function getFirstOrderByField(query: Query): FieldPath | null {
return query.explicitOrderBy.length > 0
? query.explicitOrderBy[0].field
Expand Down
36 changes: 7 additions & 29 deletions packages/firestore/src/lite-api/query.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1050,49 +1050,27 @@ function validateDisjunctiveFilterElements(
`'${operator.toString()}' filters.`
);
}
if (value.length > 10) {
throw new FirestoreError(
Code.INVALID_ARGUMENT,
`Invalid Query. '${operator.toString()}' filters support a ` +
'maximum of 10 elements in the value array.'
);
}
}

/**
* Given an operator, returns the set of operators that cannot be used with it.
*
* Operators in a query must adhere to the following set of rules:
* 1. Only one array operator is allowed.
* 2. Only one disjunctive operator is allowed.
* 3. `NOT_EQUAL` cannot be used with another `NOT_EQUAL` operator.
* 4. `NOT_IN` cannot be used with array, disjunctive, or `NOT_EQUAL` operators.
* This is not a comprehensive check, and this function should be removed in the
* long term. Validations should occur in the Firestore backend.
*
* Array operators: `ARRAY_CONTAINS`, `ARRAY_CONTAINS_ANY`
* Disjunctive operators: `IN`, `ARRAY_CONTAINS_ANY`, `NOT_IN`
* Operators in a query must adhere to the following set of rules:
* 1. Only one inequality per query.
* 2. `NOT_IN` cannot be used with array, disjunctive, or `NOT_EQUAL` operators.
*/
function conflictingOps(op: Operator): Operator[] {
switch (op) {
case Operator.NOT_EQUAL:
return [Operator.NOT_EQUAL, Operator.NOT_IN];
case Operator.ARRAY_CONTAINS:
return [
Operator.ARRAY_CONTAINS,
Operator.ARRAY_CONTAINS_ANY,
Operator.NOT_IN
];
case Operator.IN:
return [Operator.ARRAY_CONTAINS_ANY, Operator.IN, Operator.NOT_IN];
case Operator.ARRAY_CONTAINS_ANY:
return [
Operator.ARRAY_CONTAINS,
Operator.ARRAY_CONTAINS_ANY,
Operator.IN,
Operator.NOT_IN
];
case Operator.IN:
return [Operator.NOT_IN];
case Operator.NOT_IN:
return [
Operator.ARRAY_CONTAINS,
Operator.ARRAY_CONTAINS_ANY,
Operator.IN,
Operator.NOT_IN,
Expand Down
Loading

0 comments on commit 36558bd

Please sign in to comment.