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 1 commit
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
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
77 changes: 77 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 @@ -50,6 +50,7 @@ import {
doc,
expectCorrectComparisons,
expectEqualitySets,
fieldPath,
filter,
orderBy,
orFilter,
Expand Down Expand Up @@ -777,6 +778,82 @@ 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.a', '>', 5),
filter(fieldPath('a.a'), '>', 5)
),
orderBy('a'),
orderBy('a.a'), // nested field
orderBy(fieldPath('a.a')), // field name with dot
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
15 changes: 11 additions & 4 deletions packages/firestore/test/util/helpers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -226,10 +226,13 @@ export function path(path: string, offset?: number): ResourcePath {
return new ResourcePath(splitPath(path, '/'), offset);
}

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

export function fieldPath(path: string): FieldPath {
return new FieldPath([path]);
}
export function fieldIndex(
collectionGroup: string,
options: {
Expand Down Expand Up @@ -261,7 +264,11 @@ export function blob(...bytes: number[]): Bytes {
return Bytes.fromUint8Array(new Uint8Array(bytes || []));
}

export function filter(path: string, op: string, value: unknown): FieldFilter {
export function filter(
path: string | FieldPath,
op: string,
value: unknown
): FieldFilter {
const dataValue = wrap(value);
const operator = op as Operator;
return FieldFilter.create(field(path), operator, dataValue);
Expand Down Expand Up @@ -736,7 +743,7 @@ export function resumeTokenForSnapshot(
}
}

export function orderBy(path: string, op?: string): OrderBy {
export function orderBy(path: string | FieldPath, op?: string): OrderBy {
op = op || 'asc';
debugAssert(op === 'asc' || op === 'desc', 'Unknown direction: ' + op);
const dir: Direction =
Expand Down