Skip to content

Commit a0fb0aa

Browse files
MarkDuckworthMarkDuckworth
authored
OR queries more validation (#6785)
* Collapsing duplicate tests that covered with and without client side indexing into one test case that executes with both configurations. * Additional validation and additional validation tests for where(..), and(...), and or(...). * Tagging all new OR Query API members as internal until server support is offered. * Constrained the parameters of the query() function implementation. It's still compatible with the overload signatures, but the constrained parameters simplifies the implementation.
1 parent 89dac6c commit a0fb0aa

File tree

4 files changed

+102
-48
lines changed

4 files changed

+102
-48
lines changed

‎common/api-review/firestore-lite.api.md

Lines changed: 0 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -17,9 +17,6 @@ export type AddPrefixToKeys<Prefix extends string, T extends Record<string, unkn
1717
[K in keyof T & string as `${Prefix}.${K}`]+?: T[K];
1818
};
1919

20-
// @public
21-
export function and(...queryConstraints: QueryFilterConstraint[]): QueryCompositeFilterConstraint;
22-
2320
// @public
2421
export function arrayRemove(...elements: unknown[]): FieldValue;
2522

@@ -203,9 +200,6 @@ export type NestedUpdateFields<T extends Record<string, unknown>> = UnionToInter
203200
[K in keyof T & string]: ChildUpdateFields<K, T[K]>;
204201
}[keyof T & string]>;
205202

206-
// @public
207-
export function or(...queryConstraints: QueryFilterConstraint[]): QueryCompositeFilterConstraint;
208-
209203
// @public
210204
export function orderBy(fieldPath: string | FieldPath, directionStr?: OrderByDirection): QueryOrderByConstraint;
211205

@@ -230,17 +224,9 @@ export class Query<T = DocumentData> {
230224
withConverter<U>(converter: FirestoreDataConverter<U>): Query<U>;
231225
}
232226

233-
// @public
234-
export function query<T>(query: Query<T>, compositeFilter: QueryCompositeFilterConstraint, ...queryConstraints: QueryNonFilterConstraint[]): Query<T>;
235-
236227
// @public
237228
export function query<T>(query: Query<T>, ...queryConstraints: QueryConstraint[]): Query<T>;
238229

239-
// @public
240-
export class QueryCompositeFilterConstraint {
241-
readonly type: 'or' | 'and';
242-
}
243-
244230
// @public
245231
export abstract class QueryConstraint {
246232
abstract readonly type: QueryConstraintType;
@@ -268,9 +254,6 @@ export class QueryFieldFilterConstraint extends QueryConstraint {
268254
readonly type = "where";
269255
}
270256

271-
// @public
272-
export type QueryFilterConstraint = QueryFieldFilterConstraint | QueryCompositeFilterConstraint;
273-
274257
// @public
275258
export class QueryLimitConstraint extends QueryConstraint {
276259
readonly type: 'limit' | 'limitToLast';

‎common/api-review/firestore.api.md

Lines changed: 0 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -17,9 +17,6 @@ export type AddPrefixToKeys<Prefix extends string, T extends Record<string, unkn
1717
[K in keyof T & string as `${Prefix}.${K}`]+?: T[K];
1818
};
1919

20-
// @public
21-
export function and(...queryConstraints: QueryFilterConstraint[]): QueryCompositeFilterConstraint;
22-
2320
// @public
2421
export function arrayRemove(...elements: unknown[]): FieldValue;
2522

@@ -327,9 +324,6 @@ export function onSnapshotsInSync(firestore: Firestore, observer: {
327324
// @public
328325
export function onSnapshotsInSync(firestore: Firestore, onSync: () => void): Unsubscribe;
329326

330-
// @public
331-
export function or(...queryConstraints: QueryFilterConstraint[]): QueryCompositeFilterConstraint;
332-
333327
// @public
334328
export function orderBy(fieldPath: string | FieldPath, directionStr?: OrderByDirection): QueryOrderByConstraint;
335329

@@ -359,17 +353,9 @@ export class Query<T = DocumentData> {
359353
withConverter<U>(converter: FirestoreDataConverter<U>): Query<U>;
360354
}
361355

362-
// @public
363-
export function query<T>(query: Query<T>, compositeFilter: QueryCompositeFilterConstraint, ...queryConstraints: QueryNonFilterConstraint[]): Query<T>;
364-
365356
// @public
366357
export function query<T>(query: Query<T>, ...queryConstraints: QueryConstraint[]): Query<T>;
367358

368-
// @public
369-
export class QueryCompositeFilterConstraint {
370-
readonly type: 'or' | 'and';
371-
}
372-
373359
// @public
374360
export abstract class QueryConstraint {
375361
abstract readonly type: QueryConstraintType;
@@ -397,9 +383,6 @@ export class QueryFieldFilterConstraint extends QueryConstraint {
397383
readonly type = "where";
398384
}
399385

400-
// @public
401-
export type QueryFilterConstraint = QueryFieldFilterConstraint | QueryCompositeFilterConstraint;
402-
403386
// @public
404387
export class QueryLimitConstraint extends QueryConstraint {
405388
readonly type: 'limit' | 'limitToLast';

‎packages/firestore/src/lite-api/query.ts

Lines changed: 41 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -130,6 +130,7 @@ export abstract class QueryConstraint extends AppliableConstraint {
130130
* apply (e.g. {@link orderBy}, {@link limit}).
131131
* @throws if any of the provided query constraints cannot be combined with the
132132
* existing or new constraints.
133+
* @internal TODO remove this internal tag with OR Query support in the server
133134
*/
134135
export function query<T>(
135136
query: Query<T>,
@@ -154,24 +155,21 @@ export function query<T>(
154155

155156
export function query<T>(
156157
query: Query<T>,
157-
// eslint-disable-next-line @typescript-eslint/no-explicit-any
158-
filterOrQueryConstraints: any,
159-
// eslint-disable-next-line @typescript-eslint/no-explicit-any
160-
...nonFilters: any
158+
queryConstraint: QueryCompositeFilterConstraint | QueryConstraint | undefined,
159+
...additionalQueryConstraints: Array<
160+
QueryConstraint | QueryNonFilterConstraint
161+
>
161162
): Query<T> {
162163
let queryConstraints: AppliableConstraint[] = [];
163-
if (filterOrQueryConstraints instanceof AppliableConstraint) {
164-
queryConstraints.push(filterOrQueryConstraints);
165-
} else if (Array.isArray(filterOrQueryConstraints)) {
166-
queryConstraints.concat(filterOrQueryConstraints as AppliableConstraint[]);
167-
}
168164

169-
if (nonFilters !== undefined) {
170-
queryConstraints = queryConstraints.concat(
171-
nonFilters as QueryNonFilterConstraint[]
172-
);
165+
if (queryConstraint instanceof AppliableConstraint) {
166+
queryConstraints.push(queryConstraint);
173167
}
174168

169+
queryConstraints = queryConstraints.concat(additionalQueryConstraints);
170+
171+
validateQueryConstraintArray(queryConstraints);
172+
175173
for (const constraint of queryConstraints) {
176174
query = constraint._apply(query);
177175
}
@@ -278,6 +276,7 @@ export function where(
278276
* `QueryCompositeFilterConstraint`s are created by invoking {@link or} or
279277
* {@link and} and can then be passed to {@link query} to create a new query
280278
* instance that also contains the `QueryCompositeFilterConstraint`.
279+
* @internal TODO remove this internal tag with OR Query support in the server
281280
*/
282281
export class QueryCompositeFilterConstraint extends AppliableConstraint {
283282
/**
@@ -358,6 +357,7 @@ export type QueryNonFilterConstraint =
358357
* `QueryFilterConstraint`s are created by invoking {@link or} or {@link and}
359358
* and can then be passed to {@link query} to create a new query instance that
360359
* also contains the `QueryConstraint`.
360+
* @internal TODO remove this internal tag with OR Query support in the server
361361
*/
362362
export type QueryFilterConstraint =
363363
| QueryFieldFilterConstraint
@@ -371,6 +371,7 @@ export type QueryFilterConstraint =
371371
* for OR operation. These must be created with calls to {@link where},
372372
* {@link or}, or {@link and}.
373373
* @returns The created {@link QueryCompositeFilterConstraint}.
374+
* @internal TODO remove this internal tag with OR Query support in the server
374375
*/
375376
export function or(
376377
...queryConstraints: QueryFilterConstraint[]
@@ -394,6 +395,7 @@ export function or(
394395
* for AND operation. These must be created with calls to {@link where},
395396
* {@link or}, or {@link and}.
396397
* @returns The created {@link QueryCompositeFilterConstraint}.
398+
* @internal TODO remove this internal tag with OR Query support in the server
397399
*/
398400
export function and(
399401
...queryConstraints: QueryFilterConstraint[]
@@ -1214,7 +1216,32 @@ export function validateQueryFilterConstraint(
12141216
) {
12151217
throw new FirestoreError(
12161218
Code.INVALID_ARGUMENT,
1217-
`Function ${functionName}() requires AppliableContraints created with a call to 'where(...)', 'or(...)', or 'and(...)'.`
1219+
`Function ${functionName}() requires AppliableConstraints created with a call to 'where(...)', 'or(...)', or 'and(...)'.`
1220+
);
1221+
}
1222+
}
1223+
1224+
function validateQueryConstraintArray(
1225+
queryConstraint: AppliableConstraint[]
1226+
): void {
1227+
const compositeFilterCount = queryConstraint.filter(
1228+
filter => filter instanceof QueryCompositeFilterConstraint
1229+
).length;
1230+
const fieldFilterCount = queryConstraint.filter(
1231+
filter => filter instanceof QueryFieldFilterConstraint
1232+
).length;
1233+
1234+
if (
1235+
compositeFilterCount > 1 ||
1236+
(compositeFilterCount > 0 && fieldFilterCount > 0)
1237+
) {
1238+
throw new FirestoreError(
1239+
Code.INVALID_ARGUMENT,
1240+
'InvalidQuery. When using composite filters, you cannot use ' +
1241+
'more than one filter at the top level. Consider nesting the multiple ' +
1242+
'filters within an `and(...)` statement. For example: ' +
1243+
'change `query(query, where(...), or(...))` to ' +
1244+
'`query(query, and(where(...), or(...)))`.'
12181245
);
12191246
}
12201247
}

‎packages/firestore/test/integration/api/validation.test.ts

Lines changed: 61 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1399,7 +1399,68 @@ apiDescribe('Validation:', (persistence: boolean) => {
13991399
).to.throw(
14001400
"Invalid query. You cannot use 'not-in' filters with 'in' filters."
14011401
);
1402+
1403+
// Multiple top level composite filters
1404+
expect(() =>
1405+
// @ts-ignore
1406+
query(coll, and(where('a', '==', 'b')), or(where('b', '==', 'a')))
1407+
).to.throw(
1408+
'InvalidQuery. When using composite filters, you cannot use ' +
1409+
'more than one filter at the top level. Consider nesting the multiple ' +
1410+
'filters within an `and(...)` statement. For example: ' +
1411+
'change `query(query, where(...), or(...))` to ' +
1412+
'`query(query, and(where(...), or(...)))`.'
1413+
);
1414+
1415+
// Once top level composite filter and one top level field filter
1416+
expect(() =>
1417+
// @ts-ignore
1418+
query(coll, or(where('a', '==', 'b')), where('b', '==', 'a'))
1419+
).to.throw(
1420+
'InvalidQuery. When using composite filters, you cannot use ' +
1421+
'more than one filter at the top level. Consider nesting the multiple ' +
1422+
'filters within an `and(...)` statement. For example: ' +
1423+
'change `query(query, where(...), or(...))` to ' +
1424+
'`query(query, and(where(...), or(...)))`.'
1425+
);
14021426
});
1427+
1428+
validationIt(
1429+
persistence,
1430+
'passing non-filters to composite operators fails',
1431+
db => {
1432+
const compositeOperators = [
1433+
{ name: 'or', func: or },
1434+
{ name: 'and', func: and }
1435+
];
1436+
const nonFilterOps = [
1437+
limit(1),
1438+
limitToLast(2),
1439+
startAt(1),
1440+
startAfter(1),
1441+
endAt(1),
1442+
endBefore(1),
1443+
orderBy('a')
1444+
];
1445+
1446+
for (const compositeOp of compositeOperators) {
1447+
for (const nonFilterOp of nonFilterOps) {
1448+
const coll = collection(db, 'test');
1449+
expect(() =>
1450+
query(
1451+
coll,
1452+
compositeOp.func(
1453+
// @ts-ignore
1454+
nonFilterOp
1455+
)
1456+
)
1457+
).to.throw(
1458+
`Function ${compositeOp.name}() requires AppliableConstraints created with a call to 'where(...)', 'or(...)', or 'and(...)'.`
1459+
);
1460+
}
1461+
}
1462+
}
1463+
);
14031464
});
14041465
});
14051466

0 commit comments

Comments
 (0)