Skip to content

Add NOT_IN and != queries (not public) #3557

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

Merged
merged 4 commits into from
Aug 5, 2020
Merged

Add NOT_IN and != queries (not public) #3557

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
df56eb2
add != and NOT_IN queries
Aug 4, 2020
b2346f3
small cleanup
Aug 4, 2020
d914433
resolve comments
Aug 5, 2020
8fef964
clean conflictingOps comments
Aug 5, 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
160 changes: 102 additions & 58 deletions packages/firestore/src/api/database.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1436,7 +1436,7 @@ export function newQueryFilter(
`Invalid Query. You can't perform '${op}' ` +
'queries on FieldPath.documentId().'
);
} else if (op === Operator.IN) {
} else if (op === Operator.IN || op === Operator.NOT_IN) {
validateDisjunctiveFilterElements(value, op);
const referenceList: api.Value[] = [];
for (const arrayValue of value as api.Value[]) {
Expand All @@ -1447,14 +1447,18 @@ export function newQueryFilter(
fieldValue = parseDocumentIdValue(databaseId, query, value);
}
} else {
if (op === Operator.IN || op === Operator.ARRAY_CONTAINS_ANY) {
if (
op === Operator.IN ||
op === Operator.NOT_IN ||
op === Operator.ARRAY_CONTAINS_ANY
) {
validateDisjunctiveFilterElements(value, op);
}
fieldValue = parseQueryValue(
dataReader,
methodName,
value,
op === Operator.IN
/* allowArrays= */ op === Operator.IN || op === Operator.NOT_IN
);
}
const filter = FieldFilter.create(fieldPath, op, fieldValue);
Expand Down Expand Up @@ -1663,7 +1667,7 @@ function parseDocumentIdValue(
}

/**
* Validates that the value passed into a disjunctrive filter satisfies all
* Validates that the value passed into a disjunctive filter satisfies all
* array requirements.
*/
function validateDisjunctiveFilterElements(
Expand All @@ -1684,30 +1688,71 @@ function validateDisjunctiveFilterElements(
'maximum of 10 elements in the value array.'
);
}
if (value.indexOf(null) >= 0) {
throw new FirestoreError(
Code.INVALID_ARGUMENT,
`Invalid Query. '${operator.toString()}' filters cannot contain 'null' ` +
'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.'
);
}
}
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.'
);
}

/**
* 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.
*
* Array operators: ARRAY_CONTAINS, ARRAY_CONTAINS_ANY
* Disjunctive operators: IN, ARRAY_CONTAINS_ANY, NOT_IN
*/
function conflictingOps(op: Operator): Operator[] {
Copy link
Contributor

Choose a reason for hiding this comment

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

Comments please. In particular please document the context in which these operations could be conflicting.

Within the body of this function it would be useful to spell out the rules by which you arrived at these results.

Copy link
Author

Choose a reason for hiding this comment

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

Attempted.

Copy link
Contributor

Choose a reason for hiding this comment

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

These comments are mostly just repeating what the code is doing. What I was hoping for was a higher-level comment (outside the switch statement) that gave the collective rules together. For example, as you've implemented this now, you've repeated "Only one ARRAY operator can be used" in multiple places and I'm prompting for it to be in one place.

Basically: when you constructed these lists, you had to evaluate a list of rules. Then for each operator pair you evaluated those rules to determine if they were incompatible. What I want you to document is the list of rules you used to build this structure.

Copy link
Author

Choose a reason for hiding this comment

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

Makes sense. Documented at the top of the function.

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.NOT_IN:
return [
Operator.ARRAY_CONTAINS,
Operator.ARRAY_CONTAINS_ANY,
Operator.IN,
Operator.NOT_IN,
Operator.NOT_EQUAL
];
default:
return [];
}
}

function validateNewFilter(query: InternalQuery, filter: Filter): void {
debugAssert(filter instanceof FieldFilter, 'Only FieldFilters are supported');

const arrayOps = [Operator.ARRAY_CONTAINS, Operator.ARRAY_CONTAINS_ANY];
const disjunctiveOps = [Operator.IN, Operator.ARRAY_CONTAINS_ANY];
const isArrayOp = arrayOps.indexOf(filter.op) >= 0;
const isDisjunctiveOp = disjunctiveOps.indexOf(filter.op) >= 0;

if (filter.isInequality()) {
const existingField = query.getInequalityFilterField();
if (existingField !== null && !existingField.isEqual(filter.field)) {
Expand All @@ -1724,31 +1769,23 @@ function validateNewFilter(query: InternalQuery, filter: Filter): void {
if (firstOrderByField !== null) {
validateOrderByAndInequalityMatch(query, filter.field, firstOrderByField);
}
} else if (isDisjunctiveOp || isArrayOp) {
// You can have at most 1 disjunctive filter and 1 array filter. Check if
// the new filter conflicts with an existing one.
let conflictingOp: Operator | null = null;
if (isDisjunctiveOp) {
conflictingOp = query.findFilterOperator(disjunctiveOps);
}
if (conflictingOp === null && isArrayOp) {
conflictingOp = query.findFilterOperator(arrayOps);
}
if (conflictingOp !== null) {
// We special case when it's a duplicate op to give a slightly clearer error message.
if (conflictingOp === filter.op) {
throw new FirestoreError(
Code.INVALID_ARGUMENT,
'Invalid query. You cannot use more than one ' +
`'${filter.op.toString()}' filter.`
);
} else {
throw new FirestoreError(
Code.INVALID_ARGUMENT,
`Invalid query. You cannot use '${filter.op.toString()}' filters ` +
`with '${conflictingOp.toString()}' filters.`
);
}
}

const conflictingOp = query.findFilterOperator(conflictingOps(filter.op));
if (conflictingOp !== null) {
// Special case when it's a duplicate op to give a slightly clearer error message.
if (conflictingOp === filter.op) {
throw new FirestoreError(
Code.INVALID_ARGUMENT,
'Invalid query. You cannot use more than one ' +
`'${filter.op.toString()}' filter.`
);
} else {
throw new FirestoreError(
Code.INVALID_ARGUMENT,
`Invalid query. You cannot use '${filter.op.toString()}' filters ` +
`with '${conflictingOp.toString()}' filters.`
);
}
}
}
Expand Down Expand Up @@ -1806,18 +1843,25 @@ export class Query<T = firestore.DocumentData> implements firestore.Query<T> {
validateExactNumberOfArgs('Query.where', arguments, 3);
validateDefined('Query.where', 3, value);

// Enumerated from the WhereFilterOp type in index.d.ts.
const whereFilterOpEnums = [
Operator.LESS_THAN,
Operator.LESS_THAN_OR_EQUAL,
Operator.EQUAL,
Operator.GREATER_THAN_OR_EQUAL,
Operator.GREATER_THAN,
Operator.ARRAY_CONTAINS,
Operator.IN,
Operator.ARRAY_CONTAINS_ANY
];
const op = validateStringEnum('Query.where', whereFilterOpEnums, 2, opStr);
// TODO(ne-queries): Add 'not-in' and '!=' to validation.
let op: Operator;
if ((opStr as unknown) === 'not-in' || (opStr as unknown) === '!=') {
op = opStr as Operator;
} else {
// Enumerated from the WhereFilterOp type in index.d.ts.
const whereFilterOpEnums = [
Operator.LESS_THAN,
Operator.LESS_THAN_OR_EQUAL,
Operator.EQUAL,
Operator.GREATER_THAN_OR_EQUAL,
Operator.GREATER_THAN,
Operator.ARRAY_CONTAINS,
Operator.IN,
Operator.ARRAY_CONTAINS_ANY
];
op = validateStringEnum('Query.where', whereFilterOpEnums, 2, opStr);
}

const fieldPath = fieldPathFromArgument('Query.where', field);
const filter = newQueryFilter(
this._query,
Expand Down
Loading