Skip to content

Simplify code and improve test coverage #7512

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 5 commits into from
Aug 8, 2023
Merged

Simplify code and improve test coverage #7512

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c827ca1
update tests
milaGGL Aug 1, 2023
21b2919
Update query.test.ts
milaGGL Aug 1, 2023
d57f1ab
improvise the code
milaGGL Aug 1, 2023
c237dce
Merge branch 'mila/Multiple-Inequality-support' into mila/polish-up-t…
milaGGL Aug 2, 2023
bdcfeb6
use FieldPath.fromServerFormat for code simplicity
milaGGL Aug 3, 2023
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
17 changes: 0 additions & 17 deletions packages/firestore/src/core/filter.ts
View file
Open in desktop
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 getInequalityFilters(): readonly FieldFilter[];
}

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

getInequalityFilters(): readonly FieldFilter[] {
if (this.isInequality()) {
return [this];
}
return [];
}
}

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

// Performs a depth-first search to find and return the inequality FieldFilters in the composite
// filter. Returns an empty array if none of the FieldFilters has inequality filters.
getInequalityFilters(): readonly FieldFilter[] {
return this.getFlattenedFilters().filter((filter: FieldFilter) =>
filter.isInequality()
);
}
}

export function compositeFilterIsConjunction(
Expand Down
12 changes: 6 additions & 6 deletions packages/firestore/src/core/query.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ import {
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 @@ -170,11 +170,11 @@ export function queryMatchesAllDocuments(query: Query): boolean {
export function getInequalityFilterFields(query: Query): SortedSet<FieldPath> {
let result = new SortedSet<FieldPath>(FieldPath.comparator);
query.filters.forEach((filter: Filter) => {
const inequalityFields = filter
.getInequalityFilters()
.map(filter => filter.field);
inequalityFields.forEach((field: FieldPath) => {
result = result.add(field);
const subFilters = filter.getFlattenedFilters();
subFilters.forEach((filter: FieldFilter) => {
if (filter.isInequality()) {
result = result.add(filter.field);
}
});
});
return result;
Expand Down
137 changes: 1 addition & 136 deletions packages/firestore/test/integration/api/validation.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1175,146 +1175,11 @@ apiDescribe('Validation:', persistence => {
}
);

// Multiple Inequality validation tests
validationIt(persistence, 'can have multiple inequality fields', db => {
const coll = collection(db, 'test');
expect(() =>
query(coll, where('x', '>=', 32), where('y', '<', 42))
).not.to.throw();
});

validationIt(
persistence,
'can have != and inequality queries on different fields',
db => {
const coll = collection(db, 'test');
expect(() =>
query(coll, where('x', '>', 32), where('y', '!=', 42))
).not.to.throw();
}
);

validationIt(
persistence,
'can have not-in and inequality queries on different fields',
'conflicting operators inside a nested composite filter',
db => {
const coll = collection(db, 'test');
expect(() =>
query(coll, where('y', '>=', 32), where('x', 'not-in', [42]))
).not.to.throw();
}
);

validationIt(
persistence,
'can have inequality different than orderBy',
db => {
const coll = collection(db, 'test');
// single inequality
expect(() =>
query(coll, where('x', '>', 32), orderBy('y'))
).not.to.throw();
expect(() =>
query(coll, orderBy('y'), where('x', '>', 32))
).not.to.throw();
expect(() =>
query(coll, where('x', '>', 32), orderBy('y'), orderBy('x'))
).not.to.throw();
expect(() =>
query(coll, orderBy('y'), orderBy('x'), where('x', '>', 32))
).not.to.throw();
expect(() =>
query(coll, where('x', '!=', 32), orderBy('y'))
).not.to.throw();

// multiple inequality
expect(() =>
query(coll, where('x', '>', 32), where('y', '!=', 42), orderBy('z'))
).not.to.throw();
expect(() =>
query(coll, orderBy('y'), where('x', '>', 32), where('y', '<=', 42))
).not.to.throw();
expect(() =>
query(
coll,
where('x', '>', 32),
where('y', '!=', 42),
orderBy('y'),
orderBy('x')
)
).not.to.throw();
expect(() =>
query(
coll,
orderBy('y'),
orderBy('z'),
where('x', '>', 32),
where('y', '!=', 42)
)
).not.to.throw();
}
);

validationIt(
persistence,
'multiple inequalities inside a nested composite filter',
db => {
// Multiple inequality on different fields inside a nested composite filter.
const coll = collection(db, 'test');
expect(() =>
query(
coll,
or(
and(where('a', '==', 'b'), where('c', '>', 'd')),
and(where('e', '<=', 'f'), where('g', '==', 'h'))
)
)
).not.to.throw();

// Multiple inequality on different fields between a field filter and a composite filter.
expect(() =>
query(
coll,
and(
or(
and(where('a', '==', 'b'), where('c', '>=', 'd')),
and(where('e', '==', 'f'), where('g', '!=', 'h'))
),
where('r', '<', 's')
)
)
).not.to.throw();

// OrderBy and multiple inequality on different fields.
expect(() =>
query(
coll,
or(
and(where('a', '==', 'b'), where('c', '>', 'd')),
and(where('e', '==', 'f'), where('g', '!=', 'h'))
),
orderBy('r'),
orderBy('a')
)
).not.to.throw();

// Multiple inequality inside two composite filters.
expect(() =>
query(
coll,
and(
or(
and(where('a', '==', 'b'), where('c', '>=', 'd')),
and(where('e', '==', 'f'), where('g', '!=', 'h'))
),
or(
and(where('i', '==', 'j'), where('k', '>', 'l')),
and(where('m', '==', 'n'), where('o', '<', 'p'))
)
)
)
).not.to.throw();

// Composite queries can validate conflicting operators.
expect(() =>
query(
Expand Down
93 changes: 93 additions & 0 deletions packages/firestore/test/unit/core/query.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -777,6 +777,99 @@ describe('Query', () => {
);
});

it("generates the correct implicit order by's for multiple inequality", () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

double checked with backend result: https://cider-v.corp.google.com/?ws=milamamat/CS-QueryToConstraintSpecConverterTest-2023-07-31_173943&sid=7af76dfd-ea54-4bdc-8a62-7e5d4550dd35

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 Thanks

assertImplicitOrderBy(
query(
'foo',
filter('a', '<', 5),
filter('aa', '>', 5),
filter('b', '>', 5),
filter('A', '>', 5)
),
orderBy('A'),
orderBy('a'),
orderBy('aa'),
orderBy('b'),
orderBy(DOCUMENT_KEY_NAME)
);

// numbers
assertImplicitOrderBy(
query(
'foo',
filter('a', '<', 5),
filter('1', '>', 5),
filter('19', '>', 5),
filter('2', '>', 5)
),
orderBy('1'),
orderBy('19'),
orderBy('2'),
orderBy('a'),

orderBy(DOCUMENT_KEY_NAME)
);

// nested fields
assertImplicitOrderBy(
query(
'foo',
filter('a', '<', 5),
filter('aa', '>', 5),
filter('a.a', '>', 5)
),
orderBy('a'),
orderBy('a.a'),
orderBy('aa'),
orderBy(DOCUMENT_KEY_NAME)
);

// special characters
assertImplicitOrderBy(
query(
'foo',
filter('a', '<', 5),
filter('_a', '>', 5),
filter('a.a', '>', 5)
),
orderBy('_a'),
orderBy('a'),
orderBy('a.a'),
orderBy(DOCUMENT_KEY_NAME)
);

// field name with dot
assertImplicitOrderBy(
query(
'foo',
filter('a', '<', 5),
filter('a.z', '>', 5), // nested field
filter('`a.a`', '>', 5) // field name with dot
),
orderBy('a'),
orderBy('a.z'),
orderBy('`a.a`'),
orderBy(DOCUMENT_KEY_NAME)
);

// composite filter
assertImplicitOrderBy(
query(
'foo',
filter('a', '<', 5),
andFilter(
orFilter(filter('b', '>=', 1), filter('c', '<=', 1)),
orFilter(filter('d', '>', 1), filter('e', '==', 1))
)
),
orderBy('a'),
orderBy('b'),
orderBy('c'),
orderBy('d'),
orderBy(DOCUMENT_KEY_NAME)
);
});

it('matchesAllDocuments() considers filters, orders and bounds', () => {
const baseQuery = newQueryForPath(ResourcePath.fromString('collection'));
expect(queryMatchesAllDocuments(baseQuery)).to.be.true;
Expand Down
2 changes: 1 addition & 1 deletion packages/firestore/test/util/helpers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -227,7 +227,7 @@ export function path(path: string, offset?: number): ResourcePath {
}

export function field(path: string): FieldPath {
return new FieldPath(path.split('.'));
return FieldPath.fromServerFormat(path);
}

export function fieldIndex(
Expand Down