Skip to content

Fix startAfter/endBefore for orderByKeys queries #4363

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.

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
Feb 1, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 4 additions & 0 deletions .changeset/wet-windows-itch.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
---
'@firebase/database': patch
---
Fixed an issue with startAfter/endBefore when used in orderByKey queries
28 changes: 12 additions & 16 deletions packages/database/src/api/Query.ts
Original file line number Diff line number Diff line change
Expand Up @@ -97,25 +97,19 @@ export class Query {
'Query: When ordering by key, you may only pass one argument to ' +
'startAt(), endAt(), or equalTo().';
const wrongArgTypeError =
'Query: When ordering by key, the argument passed to startAt(), endAt(),' +
'or equalTo() must be a string.';
'Query: When ordering by key, the argument passed to startAt(), startAfter(), ' +
'endAt(), endBefore(), or equalTo() must be a string.';
if (params.hasStart()) {
const startName = params.getIndexStartName();
if (
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These just revert a previous change.

startName !== MIN_NAME &&
!(params.hasStartAfter() && startName === MAX_NAME)
) {
if (startName !== MIN_NAME) {
throw new Error(tooManyArgsError);
} else if (typeof startNode !== 'string') {
throw new Error(wrongArgTypeError);
}
}
if (params.hasEnd()) {
const endName = params.getIndexEndName();
if (
endName !== MAX_NAME &&
!(params.hasEndBefore() && endName === MIN_NAME)
) {
if (endName !== MAX_NAME) {
throw new Error(tooManyArgsError);
} else if (typeof endNode !== 'string') {
throw new Error(wrongArgTypeError);
Expand All @@ -128,7 +122,8 @@ export class Query {
) {
throw new Error(
'Query: When ordering by priority, the first argument passed to startAt(), ' +
'endAt(), or equalTo() must be a valid priority value (null, a number, or a string).'
'startAfter() endAt(), endBefore(), or equalTo() must be a valid priority value ' +
'(null, a number, or a string).'
);
}
} else {
Expand All @@ -142,8 +137,8 @@ export class Query {
(endNode != null && typeof endNode === 'object')
) {
throw new Error(
'Query: First argument passed to startAt(), endAt(), or equalTo() cannot be ' +
'an object.'
'Query: First argument passed to startAt(), startAfter(), endAt(), endBefore(), or ' +
'equalTo() cannot be an object.'
);
}
}
Expand All @@ -162,7 +157,8 @@ export class Query {
!params.hasAnchoredLimit()
) {
throw new Error(
"Query: Can't combine startAt(), endAt(), and limit(). Use limitToFirst() or limitToLast() instead."
"Query: Can't combine startAt(), startAfter(), endAt(), endBefore(), and limit(). Use " +
'limitToFirst() or limitToLast() instead.'
);
}
}
Expand Down Expand Up @@ -617,13 +613,13 @@ export class Query {
validateKey('Query.equalTo', 2, name, true);
if (this.queryParams_.hasStart()) {
throw new Error(
'Query.equalTo: Starting point was already set (by another call to startAt or ' +
'Query.equalTo: Starting point was already set (by another call to startAt/startAfter or ' +
'equalTo).'
);
}
if (this.queryParams_.hasEnd()) {
throw new Error(
'Query.equalTo: Ending point was already set (by another call to endAt or ' +
'Query.equalTo: Ending point was already set (by another call to endAt/endBefore or ' +
'equalTo).'
);
}
Expand Down
34 changes: 25 additions & 9 deletions packages/database/src/core/view/QueryParams.ts
Original file line number Diff line number Diff line change
Expand Up @@ -289,13 +289,21 @@ export class QueryParams {
}

startAfter(indexValue: unknown, key?: string | null): QueryParams {
let childKey: string;
if (key == null) {
childKey = MAX_NAME;
let params: QueryParams;
if (this.index_ === KEY_INDEX) {
if (typeof indexValue === 'string') {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This check makes it so that startAfter still throws exceptions for incorrectly typed keys.

Copy link
Contributor

Choose a reason for hiding this comment

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

Where does it throw? Should we throw at the callsite to preserve the stacktrace of the original mistake?

Copy link
Contributor Author

@jmwski jmwski Jan 29, 2021

Choose a reason for hiding this comment

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

It throws in validateQueryEndpoints_: https://github.com/firebase/firebase-js-sdk/blob/master/packages/database/src/api/Query.ts#L110 (this behavior is tested), which is the same place that startAt() and endAt() throw for this case.

I think that the way it's done now the original mistake, passing a non-string key to startAfter() should still be clear. I'll also update the error message there.

indexValue = successor(indexValue as string);
}
params = this.startAt(indexValue, key);
} else {
childKey = successor(key);
let childKey: string;
if (key == null) {
childKey = MAX_NAME;
} else {
childKey = successor(key);
}
params = this.startAt(indexValue, childKey);
}
const params: QueryParams = this.startAt(indexValue, childKey);
params.startAfterSet_ = true;
return params;
}
Expand Down Expand Up @@ -324,12 +332,20 @@ export class QueryParams {

endBefore(indexValue: unknown, key?: string | null): QueryParams {
let childKey: string;
if (key == null) {
childKey = MIN_NAME;
let params: QueryParams;
if (this.index_ === KEY_INDEX) {
if (typeof indexValue === 'string') {
indexValue = predecessor(indexValue as string);
}
params = this.endAt(indexValue, key);
} else {
childKey = predecessor(key);
if (key == null) {
childKey = MIN_NAME;
} else {
childKey = predecessor(key);
}
params = this.endAt(indexValue, childKey);
}
const params: QueryParams = this.endAt(indexValue, childKey);
params.endBeforeSet_ = true;
return params;
}
Expand Down
22 changes: 22 additions & 0 deletions packages/database/test/query.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1377,6 +1377,28 @@ describe('Query Tests', () => {
expect(removed).to.equal('b ');
});

it('Ensure startAfter on key index works', async () => {
const node = getRandomNode() as Reference;
const childOne = node.push();
const childTwo = node.push();
await childOne.set(1);
await childTwo.set(2);
const snap = await node.orderByKey().startAfter(childOne.key).get();
expect(Object.keys(snap.val())).to.deep.equal([childTwo.key]);
expect(Object.values(snap.val())).to.deep.equal([snap.val()[childTwo.key]]);
});

it('Ensure endBefore on key index works', async () => {
const node = getRandomNode() as Reference;
const childOne = node.push();
const childTwo = node.push();
await childOne.set(1);
await childTwo.set(2);
const snap = await node.orderByKey().endBefore(childTwo.key).get();
expect(Object.keys(snap.val())).to.deep.equal([childOne.key]);
expect(Object.values(snap.val())).to.deep.equal([snap.val()[childOne.key]]);
});

it('Ensure startAt / endAt with priority works.', async () => {
const node = getRandomNode() as Reference;

Expand Down