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 2 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
127 changes: 80 additions & 47 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 @@ -1684,14 +1688,17 @@ function validateDisjunctiveFilterElements(
'maximum of 10 elements in the value array.'
);
}
if (value.indexOf(null) >= 0) {
if (value.indexOf(null) >= 0 && operator !== Operator.NOT_IN) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Avoid double negatives if you can. In this case, the only valid operators are IN and NOT_IN, so you could make this check === Operator.IN and be clearer without loss of functionality.

Also, if you put the operator check first then we won't have to search the array for null in cases where that doesn't apply.

Same feedback below, though also consider factoring out the common if check:

if (operator === Operator.IN) {
  if (contains null) {
    ...
  }
  if (contains NaN) {
    ...
  }
}

Copy link
Author

Choose a reason for hiding this comment

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

Refactored. However, it's possible for the operator to be ARRAY_CONTAINS_ANY inside newQueryFilter(), so I added that as well.

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) {
if (
value.filter(element => Number.isNaN(element)).length > 0 &&
operator !== Operator.NOT_IN
) {
throw new FirestoreError(
Code.INVALID_ARGUMENT,
`Invalid Query. '${operator.toString()}' filters cannot contain 'NaN' ` +
Expand All @@ -1700,14 +1707,41 @@ function validateDisjunctiveFilterElements(
}
}

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_IN, Operator.NOT_EQUAL];
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 +1758,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) {
// We special case when it's a duplicate op to give a slightly clearer error message.
Copy link
Contributor

Choose a reason for hiding this comment

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

When commenting in code like this, you can drop the "we" and write as if you're telling the computer what to do (i.e. in the imperative mood with an implicit subject).

Special case when ...

(Though I see you're just moving the comment from below.)

Copy link
Author

Choose a reason for hiding this comment

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

done.

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 +1832,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
97 changes: 84 additions & 13 deletions packages/firestore/src/core/query.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -566,10 +566,12 @@ export const enum Operator {
LESS_THAN = '<',
LESS_THAN_OR_EQUAL = '<=',
EQUAL = '==',
NOT_EQUAL = '!=',
GREATER_THAN = '>',
GREATER_THAN_OR_EQUAL = '>=',
ARRAY_CONTAINS = 'array-contains',
IN = 'in',
NOT_IN = 'not-in',
ARRAY_CONTAINS_ANY = 'array-contains-any'
}

Expand All @@ -587,16 +589,8 @@ export class FieldFilter extends Filter {
*/
static create(field: FieldPath, op: Operator, value: api.Value): FieldFilter {
if (field.isKeyField()) {
if (op === Operator.IN) {
debugAssert(
isArray(value),
'Comparing on key with IN, but filter value not an ArrayValue'
);
debugAssert(
(value.arrayValue.values || []).every(elem => isReferenceValue(elem)),
'Comparing on key with IN, but an array value was not a RefValue'
);
return new KeyFieldInFilter(field, value);
if (op === Operator.IN || op === Operator.NOT_IN) {
return this.createKeyFieldInFilter(field, op, value);
} else {
debugAssert(
isReferenceValue(value),
Expand All @@ -609,15 +603,17 @@ export class FieldFilter extends Filter {
return new KeyFieldFilter(field, op, value);
}
} else if (isNullValue(value)) {
if (op !== Operator.EQUAL) {
if (op !== Operator.EQUAL && op !== Operator.NOT_EQUAL) {
// TODO(ne-queries): Update error message to include != comparison.
throw new FirestoreError(
Code.INVALID_ARGUMENT,
'Invalid query. Null supports only equality comparisons.'
);
}
return new FieldFilter(field, op, value);
} else if (isNanValue(value)) {
if (op !== Operator.EQUAL) {
if (op !== Operator.EQUAL && op !== Operator.NOT_EQUAL) {
// TODO(ne-queries): Update error message to include != comparison.
throw new FirestoreError(
Code.INVALID_ARGUMENT,
'Invalid query. NaN supports only equality comparisons.'
Expand All @@ -632,6 +628,12 @@ export class FieldFilter extends Filter {
'IN filter has invalid value: ' + value.toString()
);
return new InFilter(field, value);
} else if (op === Operator.NOT_IN) {
debugAssert(
isArray(value),
'NOT_IN filter has invalid value: ' + value.toString()
);
return new NotInFilter(field, value);
} else if (op === Operator.ARRAY_CONTAINS_ANY) {
debugAssert(
isArray(value),
Expand All @@ -643,8 +645,40 @@ export class FieldFilter extends Filter {
}
}

private static createKeyFieldInFilter(
field: FieldPath,
op: Operator,
value: api.Value
): FieldFilter {
debugAssert(
Copy link
Contributor

Choose a reason for hiding this comment

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

You can avoid this assertion by making this a part of the type of the parameter. I believe you can declare op like this:

op: Operator.IN | Operator.NOT_IN

and then the caller will have to be in a context where this is true.

Copy link
Author

Choose a reason for hiding this comment

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

Much cleaner, thanks!

op === Operator.IN || op === Operator.NOT_IN,
'createKeyFieldInFilter requires an IN or NOT_IN operator'
);
debugAssert(
isArray(value),
`Comparing on key with ${op.toString()}` +
', but filter value not an ArrayValue'
);
debugAssert(
(value.arrayValue.values || []).every(elem => isReferenceValue(elem)),
`Comparing on key with ${op.toString()}` +
', but an array value was not a RefValue'
);

return op === Operator.IN
? new KeyFieldInFilter(field, value)
: new KeyFieldNotInFilter(field, value);
}

matches(doc: Document): boolean {
const other = doc.field(this.field);
// Types do not have to match in NOT_EQUAL filters.
if (this.op === Operator.NOT_EQUAL) {
return (
other !== null &&
this.matchesComparison(valueCompare(other!, this.value))
);
}

// Only compare types with matching backend order (such as double and int).
return (
Expand All @@ -662,6 +696,8 @@ export class FieldFilter extends Filter {
return comparison <= 0;
case Operator.EQUAL:
return comparison === 0;
case Operator.NOT_EQUAL:
return comparison !== 0;
case Operator.GREATER_THAN:
return comparison > 0;
case Operator.GREATER_THAN_OR_EQUAL:
Expand All @@ -677,7 +713,8 @@ export class FieldFilter extends Filter {
Operator.LESS_THAN,
Operator.LESS_THAN_OR_EQUAL,
Operator.GREATER_THAN,
Operator.GREATER_THAN_OR_EQUAL
Operator.GREATER_THAN_OR_EQUAL,
Operator.NOT_EQUAL
].indexOf(this.op) >= 0
);
}
Expand Down Expand Up @@ -762,6 +799,27 @@ export class KeyFieldInFilter extends FieldFilter {
}
}

/** Filter that matches on key fields not present within an array. */
export class KeyFieldNotInFilter extends FieldFilter {
private readonly keys: DocumentKey[];

constructor(field: FieldPath, value: api.Value) {
super(field, Operator.NOT_IN, value);
debugAssert(isArray(value), 'KeyFieldNotInFilter expects an ArrayValue');
this.keys = (value.arrayValue.values || []).map(v => {
debugAssert(
isReferenceValue(v),
'Comparing on key with NOT_IN, but an array value was not a ReferenceValue'
);
return DocumentKey.fromName(v.referenceValue);
});
}

matches(doc: Document): boolean {
return !this.keys.some(key => key.isEqual(doc.key));
Copy link
Contributor

Choose a reason for hiding this comment

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

The difference between these types is the operator name and this exclamation point. It seems like you could either:

  • extract a common class for this that takes the operator as a parameter;
  • extract a helper function for creating the keys array;
  • make KeyFiieldInFilter the base class and have it take the operator as a parameter;
  • move the construction of the keys array into the caller (passing it as an extra argument) since you now have a dedicated createKeyFieldInFilter.

(listed in order of my decreasing preference, but my preference isn't strong.)

Just duplicating the code seems size unfriendly.

Copy link
Author

Choose a reason for hiding this comment

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

Extracted into helper.

}
}

/** A Filter that implements the array-contains operator. */
export class ArrayContainsFilter extends FieldFilter {
constructor(field: FieldPath, value: api.Value) {
Expand All @@ -787,6 +845,19 @@ export class InFilter extends FieldFilter {
}
}

/** A Filter that implements the not-in operator. */
export class NotInFilter extends FieldFilter {
constructor(field: FieldPath, value: api.Value) {
super(field, Operator.NOT_IN, value);
debugAssert(isArray(value), 'NotInFilter expects an ArrayValue');
}

matches(doc: Document): boolean {
const other = doc.field(this.field);
return other !== null && !arrayValueContains(this.value.arrayValue!, other);
}
}

/** A Filter that implements the array-contains-any operator. */
export class ArrayContainsAnyFilter extends FieldFilter {
constructor(field: FieldPath, value: api.Value) {
Expand Down
Loading