Skip to content

Commit

Permalink
Multiple inequality support (#7453)
Browse files Browse the repository at this point in the history
  • Loading branch information
@milaGGL
milaGGL committed Sep 13, 2023
1 parent 98cfcbd commit e5b96d7
Show file tree
Hide file tree
Showing 13 changed files with 1,113 additions and 322 deletions.
53 changes: 18 additions & 35 deletions packages/firestore-compat/test/validation.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -825,15 +825,11 @@ apiDescribe('Validation:', (persistence: boolean) => {
}
);

validationIt(persistence, 'with different inequality fields fail', db => {
validationIt(persistence, 'can have different inequality fields', db => {
const collection = db.collection('test');
expect(() =>
collection.where('x', '>=', 32).where('y', '<', 'cat')
).to.throw(
'Invalid query. All where filters with an ' +
'inequality (<, <=, !=, not-in, >, or >=) must be on the same field.' +
` But you have inequality filters on 'x' and 'y'`
);
).not.to.throw();
});

validationIt(persistence, 'with more than one != query fail', db => {
Expand All @@ -845,59 +841,46 @@ apiDescribe('Validation:', (persistence: boolean) => {

validationIt(
persistence,
'with != and inequality queries on different fields fail',
'can have != and inequality queries on different fields',
db => {
const collection = db.collection('test');
expect(() =>
collection.where('y', '>', 32).where('x', '!=', 33)
).to.throw(
'Invalid query. All where filters with an ' +
'inequality (<, <=, !=, not-in, >, or >=) must be on the same field.' +
` But you have inequality filters on 'y' and 'x`
);
).not.to.throw();
}
);

validationIt(
persistence,
'with != and inequality queries on different fields fail',
'can have NOT-IN and inequality queries on different fields',
db => {
const collection = db.collection('test');
expect(() =>
collection.where('y', '>', 32).where('x', 'not-in', [33])
).to.throw(
'Invalid query. All where filters with an ' +
'inequality (<, <=, !=, not-in, >, or >=) must be on the same field.' +
` But you have inequality filters on 'y' and 'x`
);
).not.to.throw();
}
);

validationIt(
persistence,
'with inequality different than first orderBy fail.',
'can have inequality different than first orderBy',
db => {
const collection = db.collection('test');
const reason =
`Invalid query. You have a where filter with an ` +
`inequality (<, <=, !=, not-in, >, or >=) on field 'x' and so you must also ` +
`use 'x' as your first argument to Query.orderBy(), but your first ` +
`orderBy() is on field 'y' instead.`;
expect(() => collection.where('x', '>', 32).orderBy('y')).to.throw(
reason
);
expect(() => collection.orderBy('y').where('x', '>', 32)).to.throw(
reason
);
expect(() =>
collection.where('x', '>', 32).orderBy('y')
).not.to.throw();
expect(() =>
collection.orderBy('y').where('x', '>', 32)
).not.to.throw();
expect(() =>
collection.where('x', '>', 32).orderBy('y').orderBy('x')
).to.throw(reason);
).not.to.throw();
expect(() =>
collection.orderBy('y').orderBy('x').where('x', '>', 32)
).to.throw(reason);
expect(() => collection.where('x', '!=', 32).orderBy('y')).to.throw(
reason
);
).not.to.throw();
expect(() =>
collection.where('x', '!=', 32).orderBy('y')
).not.to.throw();
}
);

Expand Down
33 changes: 0 additions & 33 deletions packages/firestore/src/core/filter.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -56,8 +56,6 @@ export abstract class Filter {
abstract getFlattenedFilters(): readonly FieldFilter[];

abstract getFilters(): Filter[];

abstract getFirstInequalityField(): FieldPath | null;
}

export class FieldFilter extends Filter {
Expand Down Expand Up @@ -194,13 +192,6 @@ export class FieldFilter extends Filter {
getFilters(): Filter[] {
return [this];
}

getFirstInequalityField(): FieldPath | null {
if (this.isInequality()) {
return this.field;
}
return null;
}
}

export class CompositeFilter extends Filter {
Expand Down Expand Up @@ -246,30 +237,6 @@ export class CompositeFilter extends Filter {
getFilters(): Filter[] {
return Object.assign([], this.filters);
}

getFirstInequalityField(): FieldPath | null {
const found = this.findFirstMatchingFilter(filter => filter.isInequality());

if (found !== null) {
return found.field;
}
return null;
}

// Performs a depth-first search to find and return the first FieldFilter in the composite filter
// that satisfies the predicate. Returns `null` if none of the FieldFilters satisfy the
// predicate.
private findFirstMatchingFilter(
predicate: (filter: FieldFilter) => boolean
): FieldFilter | null {
for (const fieldFilter of this.getFlattenedFilters()) {
if (predicate(fieldFilter)) {
return fieldFilter;
}
}

return null;
}
}

export function compositeFilterIsConjunction(
Expand Down
108 changes: 47 additions & 61 deletions packages/firestore/src/core/query.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,14 @@ import { compareDocumentsByField, Document } from '../model/document';
import { DocumentKey } from '../model/document_key';
import { FieldPath, ResourcePath } from '../model/path';
import { debugAssert, debugCast, fail } from '../util/assert';
import { SortedSet } from '../util/sorted_set';

import {
Bound,
boundSortsAfterDocument,
boundSortsBeforeDocument
} from './bound';
import { Filter } from './filter';
import { FieldFilter, Filter } from './filter';
import { Direction, OrderBy } from './order_by';
import {
canonifyTarget,
Expand Down Expand Up @@ -172,21 +173,18 @@ export function queryMatchesAllDocuments(query: Query): boolean {
);
}

export function getFirstOrderByField(query: Query): FieldPath | null {
return query.explicitOrderBy.length > 0
? query.explicitOrderBy[0].field
: null;
}

export function getInequalityFilterField(query: Query): FieldPath | null {
for (const filter of query.filters) {
const result = filter.getFirstInequalityField();
if (result !== null) {
return result;
}
}

return null;
// Returns the sorted set of inequality filter fields used in this query.
export function getInequalityFilterFields(query: Query): SortedSet<FieldPath> {
let result = new SortedSet<FieldPath>(FieldPath.comparator);
query.filters.forEach((filter: Filter) => {
const subFilters = filter.getFlattenedFilters();
subFilters.forEach((filter: FieldFilter) => {
if (filter.isInequality()) {
result = result.add(filter.field);
}
});
});
return result;
}

/**
Expand Down Expand Up @@ -228,45 +226,43 @@ export function queryNormalizedOrderBy(query: Query): OrderBy[] {
const queryImpl = debugCast(query, QueryImpl);
if (queryImpl.memoizedNormalizedOrderBy === null) {
queryImpl.memoizedNormalizedOrderBy = [];
const fieldsNormalized = new Set<string>();

const inequalityField = getInequalityFilterField(queryImpl);
const firstOrderByField = getFirstOrderByField(queryImpl);
if (inequalityField !== null && firstOrderByField === null) {
// In order to implicitly add key ordering, we must also add the
// inequality filter field for it to be a valid query.
// Note that the default inequality field and key ordering is ascending.
if (!inequalityField.isKeyField()) {
queryImpl.memoizedNormalizedOrderBy.push(new OrderBy(inequalityField));
// Any explicit order by fields should be added as is.
for (const orderBy of queryImpl.explicitOrderBy) {
queryImpl.memoizedNormalizedOrderBy.push(orderBy);
fieldsNormalized.add(orderBy.field.canonicalString());
}

// The order of the implicit ordering always matches the last explicit order by.
const lastDirection =
queryImpl.explicitOrderBy.length > 0
? queryImpl.explicitOrderBy[queryImpl.explicitOrderBy.length - 1].dir
: Direction.ASCENDING;

// Any inequality fields not explicitly ordered should be implicitly ordered in a lexicographical
// order. When there are multiple inequality filters on the same field, the field should be added
// only once.
// Note: `SortedSet<FieldPath>` sorts the key field before other fields. However, we want the key
// field to be sorted last.
const inequalityFields: SortedSet<FieldPath> =
getInequalityFilterFields(queryImpl);
inequalityFields.forEach(field => {
if (
!fieldsNormalized.has(field.canonicalString()) &&
!field.isKeyField()
) {
queryImpl.memoizedNormalizedOrderBy!.push(
new OrderBy(field, lastDirection)
);
}
});

// Add the document key field to the last if it is not explicitly ordered.
if (!fieldsNormalized.has(FieldPath.keyField().canonicalString())) {
queryImpl.memoizedNormalizedOrderBy.push(
new OrderBy(FieldPath.keyField(), Direction.ASCENDING)
);
} else {
debugAssert(
inequalityField === null ||
(firstOrderByField !== null &&
inequalityField.isEqual(firstOrderByField)),
'First orderBy should match inequality field.'
new OrderBy(FieldPath.keyField(), lastDirection)
);
let foundKeyOrdering = false;
for (const orderBy of queryImpl.explicitOrderBy) {
queryImpl.memoizedNormalizedOrderBy.push(orderBy);
if (orderBy.field.isKeyField()) {
foundKeyOrdering = true;
}
}
if (!foundKeyOrdering) {
// The order of the implicit key ordering always matches the last
// explicit order-by
const lastDirection =
queryImpl.explicitOrderBy.length > 0
? queryImpl.explicitOrderBy[queryImpl.explicitOrderBy.length - 1]
.dir
: Direction.ASCENDING;
queryImpl.memoizedNormalizedOrderBy.push(
new OrderBy(FieldPath.keyField(), lastDirection)
);
}
}
}
return queryImpl.memoizedNormalizedOrderBy;
Expand Down Expand Up @@ -350,16 +346,6 @@ function _queryToTarget(queryImpl: QueryImpl, orderBys: OrderBy[]): Target {
}

export function queryWithAddedFilter(query: Query, filter: Filter): Query {
const newInequalityField = filter.getFirstInequalityField();
const queryInequalityField = getInequalityFilterField(query);

debugAssert(
queryInequalityField == null ||
newInequalityField == null ||
newInequalityField.isEqual(queryInequalityField),
'Query must only have one inequality field.'
);

debugAssert(
!isDocumentQuery(query),
'No filtering allowed for document query'
Expand Down
57 changes: 0 additions & 57 deletions packages/firestore/src/lite-api/query.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,6 @@ import {
} from '../core/filter';
import { Direction, OrderBy } from '../core/order_by';
import {
getFirstOrderByField,
getInequalityFilterField,
isCollectionGroupQuery,
LimitType,
Query as InternalQuery,
Expand Down Expand Up @@ -868,7 +866,6 @@ export function newQueryOrderBy(
);
}
const orderBy = new OrderBy(fieldPath, direction);
validateNewOrderBy(query, orderBy);
return orderBy;
}

Expand Down Expand Up @@ -1100,33 +1097,6 @@ function validateNewFieldFilter(
query: InternalQuery,
fieldFilter: FieldFilter
): void {
if (fieldFilter.isInequality()) {
const existingInequality = getInequalityFilterField(query);
const newInequality = fieldFilter.field;

if (
existingInequality !== null &&
!existingInequality.isEqual(newInequality)
) {
throw new FirestoreError(
Code.INVALID_ARGUMENT,
'Invalid query. All where filters with an inequality' +
' (<, <=, !=, not-in, >, or >=) must be on the same field. But you have' +
` inequality filters on '${existingInequality.toString()}'` +
` and '${newInequality.toString()}'`
);
}

const firstOrderByField = getFirstOrderByField(query);
if (firstOrderByField !== null) {
validateOrderByAndInequalityMatch(
query,
newInequality,
firstOrderByField
);
}
}

const conflictingOp = findOpInsideFilters(
query.filters,
conflictingOps(fieldFilter.op)
Expand Down Expand Up @@ -1174,33 +1144,6 @@ function findOpInsideFilters(
return null;
}

function validateNewOrderBy(query: InternalQuery, orderBy: OrderBy): void {
if (getFirstOrderByField(query) === null) {
// This is the first order by. It must match any inequality.
const inequalityField = getInequalityFilterField(query);
if (inequalityField !== null) {
validateOrderByAndInequalityMatch(query, inequalityField, orderBy.field);
}
}
}

function validateOrderByAndInequalityMatch(
baseQuery: InternalQuery,
inequality: InternalFieldPath,
orderBy: InternalFieldPath
): void {
if (!orderBy.isEqual(inequality)) {
throw new FirestoreError(
Code.INVALID_ARGUMENT,
`Invalid query. You have a where filter with an inequality ` +
`(<, <=, !=, not-in, >, or >=) on field '${inequality.toString()}' ` +
`and so you must also use '${inequality.toString()}' ` +
`as your first argument to orderBy(), but your first orderBy() ` +
`is on field '${orderBy.toString()}' instead.`
);
}
}

export function validateQueryFilterConstraint(
functionName: string,
queryConstraint: AppliableConstraint
Expand Down
8 changes: 4 additions & 4 deletions packages/firestore/src/local/indexeddb_index_manager.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -275,10 +275,10 @@ export class IndexedDbIndexManager implements IndexManager {
return this.getIndexType(transaction, subTarget).next(type => {
if (type === IndexType.NONE || type === IndexType.PARTIAL) {
const targetIndexMatcher = new TargetIndexMatcher(subTarget);
return this.addFieldIndex(
transaction,
targetIndexMatcher.buildTargetIndex()
);
const fieldIndex = targetIndexMatcher.buildTargetIndex();
if (fieldIndex != null) {
return this.addFieldIndex(transaction, fieldIndex);
}
}
});
}
Expand Down
Loading

0 comments on commit e5b96d7

Please sign in to comment.