From 015dcf27e63158523c16fa90b8ae53603b7b8cf7 Mon Sep 17 00:00:00 2001 From: Hsin-pei Toh Date: Wed, 12 Oct 2022 21:48:47 +0000 Subject: [PATCH 1/8] Update REST and wire protocol query constants for startAfter, endBefore --- .../database/src/core/view/QueryParams.ts | 24 ++++++++++++++----- 1 file changed, 18 insertions(+), 6 deletions(-) diff --git a/packages/database/src/core/view/QueryParams.ts b/packages/database/src/core/view/QueryParams.ts index 4deebec0531..985966e498b 100644 --- a/packages/database/src/core/view/QueryParams.ts +++ b/packages/database/src/core/view/QueryParams.ts @@ -36,8 +36,10 @@ import { RangedFilter } from './filter/RangedFilter'; const enum WIRE_PROTOCOL_CONSTANTS { INDEX_START_VALUE = 'sp', INDEX_START_NAME = 'sn', + INDEX_START_IS_INCLUSIVE = 'sin', INDEX_END_VALUE = 'ep', INDEX_END_NAME = 'en', + INDEX_END_IS_INCLUSIVE = 'ein', LIMIT = 'l', VIEW_FROM = 'vf', VIEW_FROM_LEFT = 'l', @@ -53,8 +55,10 @@ const enum REST_QUERY_CONSTANTS { PRIORITY_INDEX = '$priority', VALUE_INDEX = '$value', KEY_INDEX = '$key', + START_AFTER = 'startAfter', START_AT = 'startAt', END_AT = 'endAt', + END_BEFORE = 'endBefore', LIMIT_TO_FIRST = 'limitToFirst', LIMIT_TO_LAST = 'limitToLast' } @@ -374,18 +378,22 @@ export function queryParamsToRestQueryStringParameters( qs[REST_QUERY_CONSTANTS.ORDER_BY] = stringify(orderBy); if (queryParams.startSet_) { - qs[REST_QUERY_CONSTANTS.START_AT] = stringify(queryParams.indexStartValue_); + const startParam = queryParams.startAfterSet_ + ? REST_QUERY_CONSTANTS.START_AFTER + : REST_QUERY_CONSTANTS.START_AT; + qs[startParam] = stringify(queryParams.indexStartValue_); if (queryParams.startNameSet_) { - qs[REST_QUERY_CONSTANTS.START_AT] += - ',' + stringify(queryParams.indexStartName_); + qs[startParam] += ',' + stringify(queryParams.indexStartName_); } } if (queryParams.endSet_) { - qs[REST_QUERY_CONSTANTS.END_AT] = stringify(queryParams.indexEndValue_); + const endParam = queryParams.endBeforeSet_ + ? REST_QUERY_CONSTANTS.END_BEFORE + : REST_QUERY_CONSTANTS.END_AT; + qs[endParam] = stringify(queryParams.indexEndValue_); if (queryParams.endNameSet_) { - qs[REST_QUERY_CONSTANTS.END_AT] += - ',' + stringify(queryParams.indexEndName_); + qs[endParam] += ',' + stringify(queryParams.indexEndName_); } } @@ -411,12 +419,16 @@ export function queryParamsGetQueryObject( obj[WIRE_PROTOCOL_CONSTANTS.INDEX_START_NAME] = queryParams.indexStartName_; } + obj[WIRE_PROTOCOL_CONSTANTS.INDEX_START_IS_INCLUSIVE] = + !queryParams.startAfterSet_; } if (queryParams.endSet_) { obj[WIRE_PROTOCOL_CONSTANTS.INDEX_END_VALUE] = queryParams.indexEndValue_; if (queryParams.endNameSet_) { obj[WIRE_PROTOCOL_CONSTANTS.INDEX_END_NAME] = queryParams.indexEndName_; } + obj[WIRE_PROTOCOL_CONSTANTS.INDEX_END_IS_INCLUSIVE] = + !queryParams.endBeforeSet_; } if (queryParams.limitSet_) { obj[WIRE_PROTOCOL_CONSTANTS.LIMIT] = queryParams.limit_; From 724336df9deb7a7f0de43fbaf6b84f59b38fde6a Mon Sep 17 00:00:00 2001 From: Hsin-pei Toh Date: Wed, 19 Oct 2022 20:58:30 +0000 Subject: [PATCH 2/8] Update query unit tests --- packages/database-compat/test/query.test.ts | 62 ++++++++++++++++--- .../database/src/core/view/QueryParams.ts | 35 ++--------- 2 files changed, 56 insertions(+), 41 deletions(-) diff --git a/packages/database-compat/test/query.test.ts b/packages/database-compat/test/query.test.ts index 76b57da3ff8..a787c0f1663 100644 --- a/packages/database-compat/test/query.test.ts +++ b/packages/database-compat/test/query.test.ts @@ -375,34 +375,76 @@ describe('Query Tests', () => { expect(queryId(path)).to.equal('default'); expect(queryId(path.startAt('pri', 'name'))).to.equal( - '{"sn":"name","sp":"pri"}' + '{"sin":true,"sn":"name","sp":"pri"}' ); expect(queryId(path.startAfter('pri', 'name'))).to.equal( - '{"sn":"name-","sp":"pri"}' + '{"sin":false,"sn":"name","sp":"pri"}' ); + expect(queryId(path.endAt('pri', 'name'))).to.equal( + '{"ein":true,"en":"name","ep":"pri"}' + ); + expect(queryId(path.endBefore('pri', 'name'))).to.equal( + '{"ein":false,"en":"name","ep":"pri"}' + ); + expect(queryId(path.startAt('spri').endAt('epri'))).to.equal( - '{"ep":"epri","sp":"spri"}' + '{"ein":true,"ep":"epri","sin":true,"sp":"spri"}' + ); + expect(queryId(path.startAt('spri').endBefore('epri'))).to.equal( + '{"ein":false,"ep":"epri","sin":true,"sp":"spri"}' ); expect(queryId(path.startAfter('spri').endAt('epri'))).to.equal( - '{"ep":"epri","sn":"[MAX_NAME]","sp":"spri"}' + '{"ein":true,"ep":"epri","sin":false,"sp":"spri"}' + ); + expect(queryId(path.startAfter('spri').endBefore('epri'))).to.equal( + '{"ein":false,"ep":"epri","sin":false,"sp":"spri"}' ); + expect( queryId(path.startAt('spri', 'sname').endAt('epri', 'ename')) - ).to.equal('{"en":"ename","ep":"epri","sn":"sname","sp":"spri"}'); + ).to.equal( + '{"ein":true,"en":"ename","ep":"epri","sin":true,"sn":"sname","sp":"spri"}' + ); + expect( + queryId(path.startAt('spri', 'sname').endBefore('epri', 'ename')) + ).to.equal( + '{"ein":false,"en":"ename","ep":"epri","sin":true,"sn":"sname","sp":"spri"}' + ); expect( queryId(path.startAfter('spri', 'sname').endAt('epri', 'ename')) - ).to.equal('{"en":"ename","ep":"epri","sn":"sname-","sp":"spri"}'); + ).to.equal( + '{"ein":true,"en":"ename","ep":"epri","sin":false,"sn":"sname","sp":"spri"}' + ); + expect( + queryId(path.startAfter('spri', 'sname').endBefore('epri', 'ename')) + ).to.equal( + '{"ein":false,"en":"ename","ep":"epri","sin":false,"sn":"sname","sp":"spri"}' + ); + expect(queryId(path.startAt('pri').limitToFirst(100))).to.equal( - '{"l":100,"sp":"pri","vf":"l"}' + '{"l":100,"sin":true,"sp":"pri","vf":"l"}' ); expect(queryId(path.startAfter('pri').limitToFirst(100))).to.equal( - '{"l":100,"sn":"[MAX_NAME]","sp":"pri","vf":"l"}' + '{"l":100,"sin":false,"sp":"pri","vf":"l"}' + ); + expect(queryId(path.endAt('pri').limitToLast(100))).to.equal( + '{"ein":true,"ep":"pri","l":100,"vf":"r"}' ); + expect(queryId(path.endBefore('pri').limitToLast(100))).to.equal( + '{"ein":false,"ep":"pri","l":100,"vf":"r"}' + ); + expect(queryId(path.startAt('bar').orderByChild('foo'))).to.equal( - '{"i":"foo","sp":"bar"}' + '{"i":"foo","sin":true,"sp":"bar"}' ); expect(queryId(path.startAfter('bar').orderByChild('foo'))).to.equal( - '{"i":"foo","sn":"[MAX_NAME]","sp":"bar"}' + '{"i":"foo","sin":false,"sp":"bar"}' + ); + expect(queryId(path.endAt('bar').orderByChild('foo'))).to.equal( + '{"ein":true,"ep":"bar","i":"foo"}' + ); + expect(queryId(path.endBefore('bar').orderByChild('foo'))).to.equal( + '{"ein":false,"ep":"bar","i":"foo"}' ); }); diff --git a/packages/database/src/core/view/QueryParams.ts b/packages/database/src/core/view/QueryParams.ts index 985966e498b..fdbb89d2d33 100644 --- a/packages/database/src/core/view/QueryParams.ts +++ b/packages/database/src/core/view/QueryParams.ts @@ -22,7 +22,6 @@ import { KEY_INDEX } from '../snap/indexes/KeyIndex'; import { PathIndex } from '../snap/indexes/PathIndex'; import { PRIORITY_INDEX, PriorityIndex } from '../snap/indexes/PriorityIndex'; import { VALUE_INDEX } from '../snap/indexes/ValueIndex'; -import { predecessor, successor } from '../util/NextPushId'; import { MAX_NAME, MIN_NAME } from '../util/util'; import { IndexedFilter } from './filter/IndexedFilter'; @@ -195,10 +194,12 @@ export class QueryParams { copy.limitSet_ = this.limitSet_; copy.limit_ = this.limit_; copy.startSet_ = this.startSet_; + copy.startAfterSet_ = this.startAfterSet_; copy.indexStartValue_ = this.indexStartValue_; copy.startNameSet_ = this.startNameSet_; copy.indexStartName_ = this.indexStartName_; copy.endSet_ = this.endSet_; + copy.endBeforeSet_ = this.endBeforeSet_; copy.indexEndValue_ = this.indexEndValue_; copy.endNameSet_ = this.endNameSet_; copy.indexEndName_ = this.indexEndName_; @@ -277,21 +278,7 @@ export function queryParamsStartAfter( indexValue: unknown, key?: string | null ): QueryParams { - let params: QueryParams; - if (queryParams.index_ === KEY_INDEX) { - if (typeof indexValue === 'string') { - indexValue = successor(indexValue as string); - } - params = queryParamsStartAt(queryParams, indexValue, key); - } else { - let childKey: string; - if (key == null) { - childKey = MAX_NAME; - } else { - childKey = successor(key); - } - params = queryParamsStartAt(queryParams, indexValue, childKey); - } + const params: QueryParams = queryParamsStartAt(queryParams, indexValue, key); params.startAfterSet_ = true; return params; } @@ -322,21 +309,7 @@ export function queryParamsEndBefore( indexValue: unknown, key?: string | null ): QueryParams { - let childKey: string; - let params: QueryParams; - if (queryParams.index_ === KEY_INDEX) { - if (typeof indexValue === 'string') { - indexValue = predecessor(indexValue as string); - } - params = queryParamsEndAt(queryParams, indexValue, key); - } else { - if (key == null) { - childKey = MIN_NAME; - } else { - childKey = predecessor(key); - } - params = queryParamsEndAt(queryParams, indexValue, childKey); - } + const params: QueryParams = queryParamsEndAt(queryParams, indexValue, key); params.endBeforeSet_ = true; return params; } From 4e3106cf0354da569ba91c787daa4c778b49774d Mon Sep 17 00:00:00 2001 From: Hsin-pei Toh Date: Thu, 20 Oct 2022 20:07:49 +0000 Subject: [PATCH 3/8] Update RTDB emulator version --- scripts/emulator-testing/emulators/database-emulator.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/scripts/emulator-testing/emulators/database-emulator.ts b/scripts/emulator-testing/emulators/database-emulator.ts index 0caaa0eef8b..a030a043145 100644 --- a/scripts/emulator-testing/emulators/database-emulator.ts +++ b/scripts/emulator-testing/emulators/database-emulator.ts @@ -26,11 +26,11 @@ export class DatabaseEmulator extends Emulator { constructor(port = 8088, namespace = 'test-emulator') { super( - 'firebase-database-emulator-v4.9.0.jar', + 'firebase-database-emulator-v4.11.0.jar', // Use locked version of emulator for test to be deterministic. // The latest version can be found from database emulator doc: // https://firebase.google.com/docs/database/security/test-rules-emulator - 'https://storage.googleapis.com/firebase-preview-drop/emulator/firebase-database-emulator-v4.9.0.jar', + 'https://storage.googleapis.com/firebase-preview-drop/emulator/firebase-database-emulator-v4.11.0.jar', port ); this.namespace = namespace; From e6b2bc4c0ffa5b47dd89aac1086d4f9a1ce01547 Mon Sep 17 00:00:00 2001 From: Hsin-pei Toh Date: Fri, 21 Oct 2022 19:17:44 +0000 Subject: [PATCH 4/8] Extend syncpoint spec protocol --- packages/database-compat/test/query.test.ts | 14 +-- .../database/src/core/view/QueryParams.ts | 26 +++--- .../src/core/view/filter/RangedFilter.ts | 17 +++- .../database/test/helpers/syncPointSpec.json | 92 +++++++++++++++++++ .../database/test/helpers/syncpoint-util.ts | 6 ++ 5 files changed, 132 insertions(+), 23 deletions(-) diff --git a/packages/database-compat/test/query.test.ts b/packages/database-compat/test/query.test.ts index a787c0f1663..a6bfd8703bb 100644 --- a/packages/database-compat/test/query.test.ts +++ b/packages/database-compat/test/query.test.ts @@ -391,13 +391,13 @@ describe('Query Tests', () => { '{"ein":true,"ep":"epri","sin":true,"sp":"spri"}' ); expect(queryId(path.startAt('spri').endBefore('epri'))).to.equal( - '{"ein":false,"ep":"epri","sin":true,"sp":"spri"}' + '{"ein":false,"en":"[MIN_NAME]","ep":"epri","sin":true,"sp":"spri"}' ); expect(queryId(path.startAfter('spri').endAt('epri'))).to.equal( - '{"ein":true,"ep":"epri","sin":false,"sp":"spri"}' + '{"ein":true,"ep":"epri","sin":false,"sn":"[MAX_NAME]","sp":"spri"}' ); expect(queryId(path.startAfter('spri').endBefore('epri'))).to.equal( - '{"ein":false,"ep":"epri","sin":false,"sp":"spri"}' + '{"ein":false,"en":"[MIN_NAME]","ep":"epri","sin":false,"sn":"[MAX_NAME]","sp":"spri"}' ); expect( @@ -425,26 +425,26 @@ describe('Query Tests', () => { '{"l":100,"sin":true,"sp":"pri","vf":"l"}' ); expect(queryId(path.startAfter('pri').limitToFirst(100))).to.equal( - '{"l":100,"sin":false,"sp":"pri","vf":"l"}' + '{"l":100,"sin":false,"sn":"[MAX_NAME]","sp":"pri","vf":"l"}' ); expect(queryId(path.endAt('pri').limitToLast(100))).to.equal( '{"ein":true,"ep":"pri","l":100,"vf":"r"}' ); expect(queryId(path.endBefore('pri').limitToLast(100))).to.equal( - '{"ein":false,"ep":"pri","l":100,"vf":"r"}' + '{"ein":false,"en":"[MIN_NAME]","ep":"pri","l":100,"vf":"r"}' ); expect(queryId(path.startAt('bar').orderByChild('foo'))).to.equal( '{"i":"foo","sin":true,"sp":"bar"}' ); expect(queryId(path.startAfter('bar').orderByChild('foo'))).to.equal( - '{"i":"foo","sin":false,"sp":"bar"}' + '{"i":"foo","sin":false,"sn":"[MAX_NAME]","sp":"bar"}' ); expect(queryId(path.endAt('bar').orderByChild('foo'))).to.equal( '{"ein":true,"ep":"bar","i":"foo"}' ); expect(queryId(path.endBefore('bar').orderByChild('foo'))).to.equal( - '{"ein":false,"ep":"bar","i":"foo"}' + '{"ein":false,"en":"[MIN_NAME]","ep":"bar","i":"foo"}' ); }); diff --git a/packages/database/src/core/view/QueryParams.ts b/packages/database/src/core/view/QueryParams.ts index fdbb89d2d33..e400ab26501 100644 --- a/packages/database/src/core/view/QueryParams.ts +++ b/packages/database/src/core/view/QueryParams.ts @@ -73,10 +73,10 @@ export class QueryParams { limitSet_ = false; startSet_ = false; startNameSet_ = false; - startAfterSet_ = false; + startAfterSet_ = false; // can only be true if startSet_ is true endSet_ = false; endNameSet_ = false; - endBeforeSet_ = false; + endBeforeSet_ = false; // can only be true if endSet_ is true limit_ = 0; viewFrom_ = ''; indexStartValue_: unknown | null = null; @@ -89,14 +89,6 @@ export class QueryParams { return this.startSet_; } - hasStartAfter(): boolean { - return this.startAfterSet_; - } - - hasEndBefore(): boolean { - return this.endBeforeSet_; - } - /** * @returns True if it would return from left. */ @@ -278,7 +270,12 @@ export function queryParamsStartAfter( indexValue: unknown, key?: string | null ): QueryParams { - const params: QueryParams = queryParamsStartAt(queryParams, indexValue, key); + let params: QueryParams; + if (queryParams.index_ === KEY_INDEX || !!key) { + params = queryParamsStartAt(queryParams, indexValue, key); + } else { + params = queryParamsStartAt(queryParams, indexValue, MAX_NAME); + } params.startAfterSet_ = true; return params; } @@ -309,7 +306,12 @@ export function queryParamsEndBefore( indexValue: unknown, key?: string | null ): QueryParams { - const params: QueryParams = queryParamsEndAt(queryParams, indexValue, key); + let params: QueryParams; + if (queryParams.index_ === KEY_INDEX || !!key) { + params = queryParamsEndAt(queryParams, indexValue, key); + } else { + params = queryParamsEndAt(queryParams, indexValue, MIN_NAME); + } params.endBeforeSet_ = true; return params; } diff --git a/packages/database/src/core/view/filter/RangedFilter.ts b/packages/database/src/core/view/filter/RangedFilter.ts index 7311f1cc826..cd917dfcc26 100644 --- a/packages/database/src/core/view/filter/RangedFilter.ts +++ b/packages/database/src/core/view/filter/RangedFilter.ts @@ -39,11 +39,17 @@ export class RangedFilter implements NodeFilter { private endPost_: NamedNode; + private startIsInclusive_: boolean; + + private endIsInclusive_: boolean; + constructor(params: QueryParams) { this.indexedFilter_ = new IndexedFilter(params.getIndex()); this.index_ = params.getIndex(); this.startPost_ = RangedFilter.getStartPost_(params); this.endPost_ = RangedFilter.getEndPost_(params); + this.startIsInclusive_ = !params.startAfterSet_; + this.endIsInclusive_ = !params.endBeforeSet_; } getStartPost(): NamedNode { @@ -55,10 +61,13 @@ export class RangedFilter implements NodeFilter { } matches(node: NamedNode): boolean { - return ( - this.index_.compare(this.getStartPost(), node) <= 0 && - this.index_.compare(node, this.getEndPost()) <= 0 - ); + const isWithinStart = this.startIsInclusive_ + ? this.index_.compare(this.getStartPost(), node) <= 0 + : this.index_.compare(this.getStartPost(), node) < 0; + const isWithinEnd = this.endIsInclusive_ + ? this.index_.compare(node, this.getEndPost()) <= 0 + : this.index_.compare(node, this.getEndPost()) < 0; + return isWithinStart && isWithinEnd; } updateChild( snap: Node, diff --git a/packages/database/test/helpers/syncPointSpec.json b/packages/database/test/helpers/syncPointSpec.json index f39d29d3a89..91a97858ddb 100644 --- a/packages/database/test/helpers/syncPointSpec.json +++ b/packages/database/test/helpers/syncPointSpec.json @@ -4179,6 +4179,98 @@ } ] }, + { + "name": "Queries with startAfter and endBefore work", + "steps": + [ + { + "type": "listen", + "path": "", + "params": { + "tag": 1, + "orderBy": "index", + "startAfter": { "index": 1 }, + "endBefore": { "index": 10 } + }, + "events": [] + }, + { + ".comment": "update from server sends all data", + "type": "serverUpdate", + "path": "", + "data": { + "a": { "index": 0, "value": "a" }, + "b": { "index": 2, "value": "b" }, + "c": { "index": 9, "value": "c" }, + "d": { "index": 11, "value": "d" } + }, + "events": [ + { + "path": "", + "type": "child_added", + "name": "b", + "prevName": null, + "data": { "index": 2, "value": "b" } + }, + { + "path": "", + "type": "child_added", + "name": "c", + "prevName": null, + "data": { "index": 9, "value": "c" } + }, + { + "path": "", + "type": "value", + "data": { + "b": { "index": 2, "value": "b" }, + "c": { "index": 9, "value": "c" } + } + } + ] + }, + { + ".comment": "update from server to move child c out of query", + "type": "serverUpdate", + "path": "c/index", + "data": 10, + "events": [ + { + "path": "", + "type": "child_removed", + "name": "c", + "data": { "index": 9, "value": "c" } + }, + { + "path": "", + "type": "value", + "data": { + "b": { "index": 2, "value": "b" } + } + } + ] + }, + { + ".comment": "update from server to move child b out of window", + "type": "serverUpdate", + "path": "b/index", + "data": 1, + "events": [ + { + "path": "", + "type": "child_removed", + "name": "b", + "data": { "index": 2, "value": "b" } + }, + { + "path": "", + "type": "value", + "data": {} + } + ] + } + ] + }, { "name": "Update to single child that moves out of window", "steps": diff --git a/packages/database/test/helpers/syncpoint-util.ts b/packages/database/test/helpers/syncpoint-util.ts index eca60da65bf..1b86eed6ab7 100644 --- a/packages/database/test/helpers/syncpoint-util.ts +++ b/packages/database/test/helpers/syncpoint-util.ts @@ -31,9 +31,11 @@ import { ref, limitToFirst, limitToLast, + startAfter, startAt, equalTo, endAt, + endBefore, orderByChild, orderByKey, orderByPriority @@ -366,10 +368,14 @@ export class SyncPointTestParser { q = query(q, limitToFirst(paramValue)); } else if (paramName === 'limitToLast') { q = query(q, limitToLast(paramValue)); + } else if (paramName === 'startAfter') { + q = query(q, startAfter(paramValue.index, paramValue.name)); } else if (paramName === 'startAt') { q = query(q, startAt(paramValue.index, paramValue.name)); } else if (paramName === 'endAt') { q = query(q, endAt(paramValue.index, paramValue.name)); + } else if (paramName === 'endBefore') { + q = query(q, endBefore(paramValue.index, paramValue.name)); } else if (paramName === 'equalTo') { q = query(q, equalTo(paramValue.index, paramValue.name)); } else if (paramName === 'orderBy') { From a20a324a59db2ee8102579f0c47be9dca8b410b5 Mon Sep 17 00:00:00 2001 From: Hsin-pei Toh Date: Mon, 24 Oct 2022 22:04:46 +0000 Subject: [PATCH 5/8] Account for bound inclusivity with LimitedFilter --- .../src/core/view/filter/LimitedFilter.ts | 64 +++++++----- .../database/test/helpers/syncPointSpec.json | 99 +++++++++++++++++++ 2 files changed, 138 insertions(+), 25 deletions(-) diff --git a/packages/database/src/core/view/filter/LimitedFilter.ts b/packages/database/src/core/view/filter/LimitedFilter.ts index 9fb74b4027d..6362fc7570d 100644 --- a/packages/database/src/core/view/filter/LimitedFilter.ts +++ b/packages/database/src/core/view/filter/LimitedFilter.ts @@ -46,11 +46,17 @@ export class LimitedFilter implements NodeFilter { private readonly reverse_: boolean; + private readonly startIsInclusive_: boolean; + + private readonly endIsInclusive_: boolean; + constructor(params: QueryParams) { this.rangedFilter_ = new RangedFilter(params); this.index_ = params.getIndex(); this.limit_ = params.getLimit(); this.reverse_ = !params.isViewFromLeft(); + this.startIsInclusive_ = !params.startAfterSet_; + this.endIsInclusive_ = !params.endBeforeSet_; } updateChild( snap: Node, @@ -119,19 +125,17 @@ export class LimitedFilter implements NodeFilter { let count = 0; while (iterator.hasNext() && count < this.limit_) { const next = iterator.getNext(); - let inRange; - if (this.reverse_) { - inRange = - this.index_.compare(this.rangedFilter_.getStartPost(), next) <= 0; - } else { - inRange = - this.index_.compare(next, this.rangedFilter_.getEndPost()) <= 0; - } + const inRange = + this.withinDirectionalStart(next) && + this.withinDirectionalEnd(next); if (inRange) { filtered = filtered.updateImmediateChild(next.name, next.node); count++; + } else if (!this.withinDirectionalStart(next)) { + // if we have not reached the start, skip to the next element + continue; } else { - // if we have reached the end post, we cannot keep adding elemments + // if we have reached the end, stop adding elements break; } } @@ -142,33 +146,21 @@ export class LimitedFilter implements NodeFilter { filtered = filtered.updatePriority( ChildrenNode.EMPTY_NODE ) as ChildrenNode; - let startPost; - let endPost; - let cmp; + let iterator; if (this.reverse_) { iterator = filtered.getReverseIterator(this.index_); - startPost = this.rangedFilter_.getEndPost(); - endPost = this.rangedFilter_.getStartPost(); - const indexCompare = this.index_.getCompare(); - cmp = (a: NamedNode, b: NamedNode) => indexCompare(b, a); } else { iterator = filtered.getIterator(this.index_); - startPost = this.rangedFilter_.getStartPost(); - endPost = this.rangedFilter_.getEndPost(); - cmp = this.index_.getCompare(); } let count = 0; - let foundStartPost = false; while (iterator.hasNext()) { const next = iterator.getNext(); - if (!foundStartPost && cmp(startPost, next) <= 0) { - // start adding - foundStartPost = true; - } const inRange = - foundStartPost && count < this.limit_ && cmp(next, endPost) <= 0; + count < this.limit_ && + this.withinDirectionalStart(next) && + this.withinDirectionalEnd(next); if (inRange) { count++; } else { @@ -300,4 +292,26 @@ export class LimitedFilter implements NodeFilter { return snap; } } + + private withinDirectionalStart = (node: NamedNode) => + this.reverse_ ? this.withinEndPost(node) : this.withinStartPost(node); + + private withinDirectionalEnd = (node: NamedNode) => + this.reverse_ ? this.withinStartPost(node) : this.withinEndPost(node); + + private withinStartPost = (node: NamedNode) => { + const compareRes = this.index_.getCompare()( + this.rangedFilter_.getStartPost(), + node + ); + return this.startIsInclusive_ ? compareRes <= 0 : compareRes < 0; + }; + + private withinEndPost = (node: NamedNode) => { + const compareRes = this.index_.getCompare()( + node, + this.rangedFilter_.getEndPost() + ); + return this.endIsInclusive_ ? compareRes <= 0 : compareRes < 0; + }; } diff --git a/packages/database/test/helpers/syncPointSpec.json b/packages/database/test/helpers/syncPointSpec.json index 91a97858ddb..505af9dd647 100644 --- a/packages/database/test/helpers/syncPointSpec.json +++ b/packages/database/test/helpers/syncPointSpec.json @@ -4271,6 +4271,105 @@ } ] }, + { + "name": "Queries indexed by key with startAfter, endBefore, and limitToFirst work", + "steps": + [ + { + "type": "listen", + "path": "", + "params": { + "tag": 1, + "orderByKey": true, + "startAfter": { "index": "a" }, + "endBefore": { "index": "d" }, + "limitToFirst": 1 + }, + "events": [] + }, + { + ".comment": "update from server sends all data", + "type": "serverUpdate", + "path": "", + "data": { + "a": { ".value": "a" }, + "b": { ".value": "b" }, + "c": { ".value": "c" }, + "d": { ".value": "d" } + }, + "events": [ + { + "path": "", + "type": "child_added", + "name": "b", + "prevName": null, + "data": { ".value": "b" } + }, + { + "path": "", + "type": "value", + "data": { + "b": { ".value": "b" } + } + } + ] + } + ] + }, + { + "name": "Queries indexed by key with startAfter, endBefore, and limitToLast work", + "steps": + [ + { + "type": "listen", + "path": "", + "params": { + "tag": 1, + "orderByKey": true, + "startAfter": { "index": "a" }, + "endBefore": { "index": "e" }, + "limitToLast": 2 + }, + "events": [] + }, + { + ".comment": "update from server sends all data", + "type": "serverUpdate", + "path": "", + "data": { + "a": { ".value": "a" }, + "b": { ".value": "b" }, + "c": { ".value": "c" }, + "d": { ".value": "d" }, + "e": { ".value": "e" } + }, + "events": [ + { + "path": "", + "type": "child_added", + "name": "c", + "prevName": null, + "data": { ".value": "c" } + }, + { + "path": "", + "type": "child_added", + "name": "d", + "prevName": null, + "data": { ".value": "d" } + }, + { + "path": "", + "type": "value", + "data": { + "c": { ".value": "c" }, + "d": { ".value": "d" } + } + } + ] + } + ] + }, { "name": "Update to single child that moves out of window", "steps": From bb3c4ed9a639df420d0cf36989a3295d4f55377a Mon Sep 17 00:00:00 2001 From: Hsin-pei Toh Date: Thu, 10 Nov 2022 20:20:08 +0000 Subject: [PATCH 6/8] Address PR feedback --- .changeset/empty-doors-brush.md | 6 ++++++ scripts/emulator-testing/emulators/database-emulator.ts | 6 ++++-- 2 files changed, 10 insertions(+), 2 deletions(-) create mode 100644 .changeset/empty-doors-brush.md diff --git a/.changeset/empty-doors-brush.md b/.changeset/empty-doors-brush.md new file mode 100644 index 00000000000..3f974353b8f --- /dev/null +++ b/.changeset/empty-doors-brush.md @@ -0,0 +1,6 @@ +--- +'@firebase/database': patch +'@firebase/database-compat': patch +--- + +Use new wire protocol parameters for startAfter, endBefore. diff --git a/scripts/emulator-testing/emulators/database-emulator.ts b/scripts/emulator-testing/emulators/database-emulator.ts index a030a043145..e14dd7c00d2 100644 --- a/scripts/emulator-testing/emulators/database-emulator.ts +++ b/scripts/emulator-testing/emulators/database-emulator.ts @@ -21,16 +21,18 @@ import { Emulator } from './emulator'; import rulesJSON from '../../../config/database.rules.json'; +const DATABASE_EMULATOR_VERSION = '4.11.0'; + export class DatabaseEmulator extends Emulator { namespace: string; constructor(port = 8088, namespace = 'test-emulator') { super( - 'firebase-database-emulator-v4.11.0.jar', + `firebase-database-emulator-v${DATABASE_EMULATOR_VERSION}.jar`, // Use locked version of emulator for test to be deterministic. // The latest version can be found from database emulator doc: // https://firebase.google.com/docs/database/security/test-rules-emulator - 'https://storage.googleapis.com/firebase-preview-drop/emulator/firebase-database-emulator-v4.11.0.jar', + `https://storage.googleapis.com/firebase-preview-drop/emulator/firebase-database-emulator-v${DATABASE_EMULATOR_VERSION}.jar`, port ); this.namespace = namespace; From 331085549fd42a8b679f149c16c65b83be25e4df Mon Sep 17 00:00:00 2001 From: Hsin-pei Toh Date: Mon, 14 Nov 2022 21:51:15 +0000 Subject: [PATCH 7/8] Add test case with --- .../database/test/helpers/syncPointSpec.json | 52 +++++++++++++++++++ 1 file changed, 52 insertions(+) diff --git a/packages/database/test/helpers/syncPointSpec.json b/packages/database/test/helpers/syncPointSpec.json index 505af9dd647..801c8e76001 100644 --- a/packages/database/test/helpers/syncPointSpec.json +++ b/packages/database/test/helpers/syncPointSpec.json @@ -4271,6 +4271,58 @@ } ] }, + { + "name": "Queries with startAfter, endBefore, and orderByChild work", + "steps": + [ + { + "type": "listen", + "path": "", + "params": { + "tag": 1, + "orderBy": "foo/index", + "startAfter": { "index": 1, "name": "foo" }, + "endBefore": { "index": 10, "name": "foo" } + }, + "events": [] + }, + { + ".comment": "update from server sends all data", + "type": "serverUpdate", + "path": "", + "data": { + "a": { "foo": { "index": 0, "value": "a" } }, + "b": { "foo": { "index": 2, "value": "b" } }, + "c": { "foo": { "index": 9, "value": "c" } }, + "d": { "foo": { "index": 11, "value": "d" } } + }, + "events": [ + { + "path": "", + "type": "child_added", + "name": "b", + "prevName": null, + "data": { "foo": { "index": 2, "value": "b" } } + }, + { + "path": "", + "type": "child_added", + "name": "c", + "prevName": "b", + "data": { "foo": { "index": 9, "value": "c" } } + }, + { + "path": "", + "type": "value", + "data": { + "b": { "foo": { "index": 2, "value": "b" } }, + "c": { "foo": { "index": 9, "value": "c" } } + } + } + ] + } + ] + }, { "name": "Queries indexed by key with startAfter, endBefore, and limitToFirst work", "steps": From 202ac61a3a1963de611396d497924653c29275c7 Mon Sep 17 00:00:00 2001 From: Hsin-pei Toh Date: Tue, 15 Nov 2022 16:31:58 +0000 Subject: [PATCH 8/8] Address PR feedback --- .../src/core/view/filter/LimitedFilter.ts | 17 +++++++---------- .../database/test/helpers/syncPointSpec.json | 4 ++-- 2 files changed, 9 insertions(+), 12 deletions(-) diff --git a/packages/database/src/core/view/filter/LimitedFilter.ts b/packages/database/src/core/view/filter/LimitedFilter.ts index 6362fc7570d..b4848a69b63 100644 --- a/packages/database/src/core/view/filter/LimitedFilter.ts +++ b/packages/database/src/core/view/filter/LimitedFilter.ts @@ -125,18 +125,15 @@ export class LimitedFilter implements NodeFilter { let count = 0; while (iterator.hasNext() && count < this.limit_) { const next = iterator.getNext(); - const inRange = - this.withinDirectionalStart(next) && - this.withinDirectionalEnd(next); - if (inRange) { - filtered = filtered.updateImmediateChild(next.name, next.node); - count++; - } else if (!this.withinDirectionalStart(next)) { + if (!this.withinDirectionalStart(next)) { // if we have not reached the start, skip to the next element continue; - } else { + } else if (!this.withinDirectionalEnd(next)) { // if we have reached the end, stop adding elements break; + } else { + filtered = filtered.updateImmediateChild(next.name, next.node); + count++; } } } else { @@ -300,7 +297,7 @@ export class LimitedFilter implements NodeFilter { this.reverse_ ? this.withinStartPost(node) : this.withinEndPost(node); private withinStartPost = (node: NamedNode) => { - const compareRes = this.index_.getCompare()( + const compareRes = this.index_.compare( this.rangedFilter_.getStartPost(), node ); @@ -308,7 +305,7 @@ export class LimitedFilter implements NodeFilter { }; private withinEndPost = (node: NamedNode) => { - const compareRes = this.index_.getCompare()( + const compareRes = this.index_.compare( node, this.rangedFilter_.getEndPost() ); diff --git a/packages/database/test/helpers/syncPointSpec.json b/packages/database/test/helpers/syncPointSpec.json index 801c8e76001..931193f02b7 100644 --- a/packages/database/test/helpers/syncPointSpec.json +++ b/packages/database/test/helpers/syncPointSpec.json @@ -4216,7 +4216,7 @@ "path": "", "type": "child_added", "name": "c", - "prevName": null, + "prevName": "b", "data": { "index": 9, "value": "c" } }, { @@ -4407,7 +4407,7 @@ "path": "", "type": "child_added", "name": "d", - "prevName": null, + "prevName": "c", "data": { ".value": "d" } }, {