From 47fb4689ce4a98134d8af533d166ee7e31feadb8 Mon Sep 17 00:00:00 2001 From: Maneesh Tewani Date: Thu, 4 Aug 2022 15:17:41 -0700 Subject: [PATCH 01/20] Fixed faulty transaction bug --- packages/database/src/core/view/ViewProcessor.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/database/src/core/view/ViewProcessor.ts b/packages/database/src/core/view/ViewProcessor.ts index 5011d192b7a..6bd20be1ecd 100644 --- a/packages/database/src/core/view/ViewProcessor.ts +++ b/packages/database/src/core/view/ViewProcessor.ts @@ -641,7 +641,7 @@ function viewProcessorApplyServerMerge( viewMergeTree.children.inorderTraversal((childKey, childMergeTree) => { const isUnknownDeepMerge = !viewCache.serverCache.isCompleteForChild(childKey) && - childMergeTree.value === undefined; + childMergeTree.value === null; if (!serverNode.hasChild(childKey) && !isUnknownDeepMerge) { const serverChild = viewCache.serverCache .getNode() From 7c103097b956192a40cd3b3ced10e97ccef0af35 Mon Sep 17 00:00:00 2001 From: Maneesh Tewani Date: Sun, 7 Aug 2022 15:04:24 -0700 Subject: [PATCH 02/20] WIP --- packages/database-compat/test/helpers/util.ts | 2 +- .../database-compat/test/transaction.test.ts | 34 +++++++++++++++++++ packages/database/src/core/Repo.ts | 11 +++--- .../database/src/core/view/ViewProcessor.ts | 2 +- 4 files changed, 41 insertions(+), 8 deletions(-) diff --git a/packages/database-compat/test/helpers/util.ts b/packages/database-compat/test/helpers/util.ts index a932c929843..a7625152ac2 100644 --- a/packages/database-compat/test/helpers/util.ts +++ b/packages/database-compat/test/helpers/util.ts @@ -146,7 +146,7 @@ export function getFreshRepo(path: Path) { 'ISOLATED_REPO_' + freshRepoId++ ); activeFreshApps.push(app); - return (app as any).database().ref(path.toString()); + return (app as any).database().ref(path.toString()); // TODO(mtewani): Remove explicit any } export function getFreshRepoFromReference(ref) { diff --git a/packages/database-compat/test/transaction.test.ts b/packages/database-compat/test/transaction.test.ts index 6ca010bee8a..0f1ac2ed45e 100644 --- a/packages/database-compat/test/transaction.test.ts +++ b/packages/database-compat/test/transaction.test.ts @@ -20,6 +20,7 @@ import { _TEST_ACCESS_hijackHash as hijackHash } from '@firebase/database'; import { Deferred } from '@firebase/util'; import { expect } from 'chai'; +import { Path } from '../../database/src/core/util/Path'; import { EventAccumulator, EventAccumulatorFactory @@ -29,7 +30,9 @@ import { Reference } from '../src/api/Reference'; import { eventTestHelper } from './helpers/events'; import { canCreateExtraConnections, + getFreshRepo, getFreshRepoFromReference, + getPath, getRandomNode, getVal } from './helpers/util'; @@ -1530,4 +1533,35 @@ describe('Transaction Tests', () => { await incrementViaTransaction(); expect(latestValue).to.equal(2); }); + it.only('can send valid input with concurrent writes', async() => { + const rwRef = getFreshRepo(new Path('/')) as Reference; + const wRef = getFreshRepo(new Path('/')) as Reference; + async function runTransactions() { + while(1) { + console.log(getPath(rwRef)); + await rwRef.child('abc').transaction((data: any) => { + console.log(data); + if(data) { + expect(!!data.test && !!data.timestamp).to.be.true; + } + return data; + }); + await new Promise(resolve => setTimeout(resolve, 150)); + } + console.log('done read'); + } + async function runWrites() { + console.log(getPath(wRef)); + while(1) { + await wRef.child('abc').set({ + test: 'abc', + timestamp: Date.now() + }); + } + console.log('done writing'); + } + runWrites(); + runTransactions(); + await new Promise(resolve => setTimeout(resolve, 20000)); + }).timeout(30000); }); diff --git a/packages/database/src/core/Repo.ts b/packages/database/src/core/Repo.ts index 3c2a457ba59..4e628d94e89 100644 --- a/packages/database/src/core/Repo.ts +++ b/packages/database/src/core/Repo.ts @@ -443,7 +443,6 @@ function repoUpdateInfo(repo: Repo, pathString: string, value: unknown): void { function repoGetNextWriteId(repo: Repo): number { return repo.nextWriteId_++; } - /** * The purpose of `getValue` is to return the latest known value * satisfying `query`. @@ -952,7 +951,7 @@ export function repoStartTransaction( // Mark as run and add to our queue. transaction.status = TransactionStatus.RUN; const queueNode = treeSubTree(repo.transactionQueueTree_, path); - const nodeQueue = treeGetValue(queueNode) || []; + const nodeQueue = treeGetValue(queueNode) ?? []; nodeQueue.push(transaction); treeSetValue(queueNode, nodeQueue); @@ -1037,7 +1036,7 @@ function repoSendReadyTransactions( repoPruneCompletedTransactionsBelowNode(repo, node); } - if (treeGetValue(node)) { + if (treeGetValue(node) !== undefined) { const queue = repoBuildTransactionQueue(repo, node); assert(queue.length > 0, 'Sending zero length transaction queue'); @@ -1413,7 +1412,7 @@ function repoAggregateTransactionQueuesForNode( queue: Transaction[] ): void { const nodeQueue = treeGetValue(node); - if (nodeQueue) { + if (nodeQueue !== undefined) { for (let i = 0; i < nodeQueue.length; i++) { queue.push(nodeQueue[i]); } @@ -1432,7 +1431,7 @@ function repoPruneCompletedTransactionsBelowNode( node: Tree ): void { const queue = treeGetValue(node); - if (queue) { + if (queue !== undefined) { let to = 0; for (let from = 0; from < queue.length; from++) { if (queue[from].status !== TransactionStatus.COMPLETED) { @@ -1484,7 +1483,7 @@ function repoAbortTransactionsOnNode( node: Tree ): void { const queue = treeGetValue(node); - if (queue) { + if (queue !== undefined) { // Queue up the callbacks and fire them after cleaning up all of our // transaction state, since the callback could trigger more transactions // or sets. diff --git a/packages/database/src/core/view/ViewProcessor.ts b/packages/database/src/core/view/ViewProcessor.ts index 6bd20be1ecd..557447f437b 100644 --- a/packages/database/src/core/view/ViewProcessor.ts +++ b/packages/database/src/core/view/ViewProcessor.ts @@ -606,7 +606,7 @@ function viewProcessorApplyServerMerge( // and event snap. I'm not sure if this will result in edge cases when a child is in one but // not the other. let curViewCache = viewCache; - let viewMergeTree; + let viewMergeTree: ImmutableTree; if (pathIsEmpty(path)) { viewMergeTree = changedChildren; } else { From 452abc5a454265d05f5b52f9715d3e229a7a86a3 Mon Sep 17 00:00:00 2001 From: Maneesh Tewani Date: Wed, 10 Aug 2022 14:15:32 -0700 Subject: [PATCH 03/20] Included repro test --- packages/database-compat/test/query.test.ts | 35 ++++++++++++++++++- .../database-compat/test/transaction.test.ts | 10 +++--- 2 files changed, 39 insertions(+), 6 deletions(-) diff --git a/packages/database-compat/test/query.test.ts b/packages/database-compat/test/query.test.ts index 76b57da3ff8..002f6e4c00b 100644 --- a/packages/database-compat/test/query.test.ts +++ b/packages/database-compat/test/query.test.ts @@ -30,7 +30,13 @@ import { } from '../../database/test/helpers/EventAccumulator'; import { DataSnapshot, Query, Reference } from '../src/api/Reference'; -import { getFreshRepo, getPath, getRandomNode, pause } from './helpers/util'; +import { + createTestApp, + getFreshRepo, + getPath, + getRandomNode, + pause +} from './helpers/util'; use(chaiAsPromised); @@ -4664,6 +4670,33 @@ describe('Query Tests', () => { }); }); }); + it.only('can properly handle unknown deep merges', async () => { + const app = createTestApp(); + const root = app.database().ref().child('testing'); + await root.remove(); + + const query = root.orderByChild('lease').limitToFirst(2); + + await root + .child('i|1') + .set({ lease: 3, timestamp: Date.now(), action: 'test' }); + await root + .child('i|2') + .set({ lease: 1, timestamp: Date.now(), action: 'test' }); + await root + .child('i|3') + .set({ lease: 2, timestamp: Date.now(), action: 'test' }); + query.on('child_added', snap => { + const value = snap.val(); + expect(value).to.haveOwnProperty('timestamp'); + expect(value).to.haveOwnProperty('action'); + }); + root.child('i|1').on('value', snap => { + //no-op + }); + await root.child('i|1').update({ timestamp: `${Date.now()}|1` }); + await new Promise(resolve => setTimeout(resolve, 4000)); + }); it('Can JSON serialize refs', () => { const ref = getRandomNode() as Reference; diff --git a/packages/database-compat/test/transaction.test.ts b/packages/database-compat/test/transaction.test.ts index 0f1ac2ed45e..d7ef47a9333 100644 --- a/packages/database-compat/test/transaction.test.ts +++ b/packages/database-compat/test/transaction.test.ts @@ -1533,15 +1533,15 @@ describe('Transaction Tests', () => { await incrementViaTransaction(); expect(latestValue).to.equal(2); }); - it.only('can send valid input with concurrent writes', async() => { + it.only('can send valid input with concurrent writes', async () => { const rwRef = getFreshRepo(new Path('/')) as Reference; const wRef = getFreshRepo(new Path('/')) as Reference; async function runTransactions() { - while(1) { + while (1) { console.log(getPath(rwRef)); await rwRef.child('abc').transaction((data: any) => { console.log(data); - if(data) { + if (data) { expect(!!data.test && !!data.timestamp).to.be.true; } return data; @@ -1551,8 +1551,8 @@ describe('Transaction Tests', () => { console.log('done read'); } async function runWrites() { - console.log(getPath(wRef)); - while(1) { + console.log(getPath(wRef)); + while (1) { await wRef.child('abc').set({ test: 'abc', timestamp: Date.now() From 07fc4657ae760ac0a589d35521ee58a55d7c2e51 Mon Sep 17 00:00:00 2001 From: Maneesh Tewani Date: Wed, 10 Aug 2022 14:22:03 -0700 Subject: [PATCH 04/20] Added todo --- packages/database-compat/test/query.test.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/database-compat/test/query.test.ts b/packages/database-compat/test/query.test.ts index 002f6e4c00b..c2842755a3a 100644 --- a/packages/database-compat/test/query.test.ts +++ b/packages/database-compat/test/query.test.ts @@ -4673,6 +4673,7 @@ describe('Query Tests', () => { it.only('can properly handle unknown deep merges', async () => { const app = createTestApp(); const root = app.database().ref().child('testing'); + // TODO(mtewani): This repro requires an index to be created. await root.remove(); const query = root.orderByChild('lease').limitToFirst(2); From 52669cdb65055da5febcf539ad1373eb3a5b4050 Mon Sep 17 00:00:00 2001 From: Maneesh Tewani Date: Thu, 11 Aug 2022 16:18:03 -0700 Subject: [PATCH 05/20] WIP --- config/database.rules.json | 5 ++- packages/database-compat/test/query.test.ts | 13 +++++-- .../database-compat/test/transaction.test.ts | 34 ------------------- 3 files changed, 15 insertions(+), 37 deletions(-) diff --git a/config/database.rules.json b/config/database.rules.json index b104e9c240e..610e79ba337 100644 --- a/config/database.rules.json +++ b/config/database.rules.json @@ -1,6 +1,9 @@ { "rules": { ".read": true, - ".write": true + ".write": true, + "testing": { + ".indexOn": "lease" + } } } \ No newline at end of file diff --git a/packages/database-compat/test/query.test.ts b/packages/database-compat/test/query.test.ts index c2842755a3a..91831ea7c92 100644 --- a/packages/database-compat/test/query.test.ts +++ b/packages/database-compat/test/query.test.ts @@ -4687,16 +4687,25 @@ describe('Query Tests', () => { await root .child('i|3') .set({ lease: 2, timestamp: Date.now(), action: 'test' }); - query.on('child_added', snap => { + const childAdded = query.on('child_added', snap => { const value = snap.val(); + console.log('onChildAdded', value); + if(value.timestamp && !value.lease) { + console.log("ERR: missing value"); + } expect(value).to.haveOwnProperty('timestamp'); expect(value).to.haveOwnProperty('action'); + expect(value).to.haveOwnProperty('lease'); }); - root.child('i|1').on('value', snap => { + const onValue = root.child('i|1').on('value', snap => { + console.log('onValue', snap.val()); //no-op }); await root.child('i|1').update({ timestamp: `${Date.now()}|1` }); + console.log('wrote value'); await new Promise(resolve => setTimeout(resolve, 4000)); + root.child('i|1').off('value', onValue); + query.off('child_added', childAdded); }); it('Can JSON serialize refs', () => { diff --git a/packages/database-compat/test/transaction.test.ts b/packages/database-compat/test/transaction.test.ts index d7ef47a9333..6ca010bee8a 100644 --- a/packages/database-compat/test/transaction.test.ts +++ b/packages/database-compat/test/transaction.test.ts @@ -20,7 +20,6 @@ import { _TEST_ACCESS_hijackHash as hijackHash } from '@firebase/database'; import { Deferred } from '@firebase/util'; import { expect } from 'chai'; -import { Path } from '../../database/src/core/util/Path'; import { EventAccumulator, EventAccumulatorFactory @@ -30,9 +29,7 @@ import { Reference } from '../src/api/Reference'; import { eventTestHelper } from './helpers/events'; import { canCreateExtraConnections, - getFreshRepo, getFreshRepoFromReference, - getPath, getRandomNode, getVal } from './helpers/util'; @@ -1533,35 +1530,4 @@ describe('Transaction Tests', () => { await incrementViaTransaction(); expect(latestValue).to.equal(2); }); - it.only('can send valid input with concurrent writes', async () => { - const rwRef = getFreshRepo(new Path('/')) as Reference; - const wRef = getFreshRepo(new Path('/')) as Reference; - async function runTransactions() { - while (1) { - console.log(getPath(rwRef)); - await rwRef.child('abc').transaction((data: any) => { - console.log(data); - if (data) { - expect(!!data.test && !!data.timestamp).to.be.true; - } - return data; - }); - await new Promise(resolve => setTimeout(resolve, 150)); - } - console.log('done read'); - } - async function runWrites() { - console.log(getPath(wRef)); - while (1) { - await wRef.child('abc').set({ - test: 'abc', - timestamp: Date.now() - }); - } - console.log('done writing'); - } - runWrites(); - runTransactions(); - await new Promise(resolve => setTimeout(resolve, 20000)); - }).timeout(30000); }); From fbe94aa8cc556a4e0e7e47d9d89eae5686e3325b Mon Sep 17 00:00:00 2001 From: Maneesh Tewani Date: Fri, 12 Aug 2022 15:28:53 -0700 Subject: [PATCH 06/20] Included index in our tests --- config/database.rules.json | 2 +- packages/database-compat/test/query.test.ts | 14 ++--- .../database/test/exp/integration.test.ts | 52 ++++++++++++++++++- packages/rules-unit-testing/src/initialize.ts | 1 + .../emulators/database-emulator.ts | 2 +- 5 files changed, 59 insertions(+), 12 deletions(-) diff --git a/config/database.rules.json b/config/database.rules.json index 610e79ba337..1ca1cb35b58 100644 --- a/config/database.rules.json +++ b/config/database.rules.json @@ -3,7 +3,7 @@ ".read": true, ".write": true, "testing": { - ".indexOn": "lease" + ".indexOn": "testIndex" } } } \ No newline at end of file diff --git a/packages/database-compat/test/query.test.ts b/packages/database-compat/test/query.test.ts index 91831ea7c92..080b439cc5c 100644 --- a/packages/database-compat/test/query.test.ts +++ b/packages/database-compat/test/query.test.ts @@ -4676,26 +4676,22 @@ describe('Query Tests', () => { // TODO(mtewani): This repro requires an index to be created. await root.remove(); - const query = root.orderByChild('lease').limitToFirst(2); + const query = root.orderByChild('testIndex').limitToFirst(2); await root .child('i|1') - .set({ lease: 3, timestamp: Date.now(), action: 'test' }); + .set({ testIndex: 3, timestamp: Date.now(), action: 'test' }); await root .child('i|2') - .set({ lease: 1, timestamp: Date.now(), action: 'test' }); + .set({ testIndex: 1, timestamp: Date.now(), action: 'test' }); await root .child('i|3') - .set({ lease: 2, timestamp: Date.now(), action: 'test' }); + .set({ testIndex: 2, timestamp: Date.now(), action: 'test' }); const childAdded = query.on('child_added', snap => { const value = snap.val(); - console.log('onChildAdded', value); - if(value.timestamp && !value.lease) { - console.log("ERR: missing value"); - } expect(value).to.haveOwnProperty('timestamp'); expect(value).to.haveOwnProperty('action'); - expect(value).to.haveOwnProperty('lease'); + expect(value).to.haveOwnProperty('testIndex'); }); const onValue = root.child('i|1').on('value', snap => { console.log('onValue', snap.val()); diff --git a/packages/database/test/exp/integration.test.ts b/packages/database/test/exp/integration.test.ts index 3789aac7e46..c50abf108c6 100644 --- a/packages/database/test/exp/integration.test.ts +++ b/packages/database/test/exp/integration.test.ts @@ -21,13 +21,18 @@ import { expect, use } from 'chai'; import chaiAsPromised from 'chai-as-promised'; import { + child, get, limitToFirst, + onChildAdded, onValue, + orderByChild, query, refFromURL, + remove, set, - startAt + startAt, + update } from '../../src/api/Reference_impl'; import { getDatabase, @@ -112,6 +117,51 @@ describe('Database@exp Tests', () => { unsubscribe(); }); + it('can properly handle unknown deep merges', async () => { + const database = getDatabase(defaultApp); + const root = ref(database, 'testing'); + await remove(root); + + const q = query(root, orderByChild('testIndex'), limitToFirst(2)); + + const i1 = child(root, 'i1'); + await set(root, { + i1: { + testIndex: 3, + timestamp: Date.now(), + action: 'test' + }, + i2: { + testIndex: 1, + timestamp: Date.now(), + action: 'test' + }, + i3: { + testIndex: 2, + timestamp: Date.now(), + action: 'test' + } + }); + const ec = EventAccumulatorFactory.waitsForExactCount(1); + const onChildAddedCb = onChildAdded(q, snap => { + const value = snap.val(); + ec.addEvent(snap); + }); + const onValueCb = onValue(i1, () => { + //no-op + }); + await update(i1, { + timestamp: `${Date.now()}|1` + }); + const [result] = await ec.promise; + const value = result.val(); + expect(value).to.haveOwnProperty('timestamp'); + expect(value).to.haveOwnProperty('action'); + expect(value).to.haveOwnProperty('testIndex'); + onChildAddedCb(); + onValueCb(); + }); + // Tests to make sure onValue's data does not get mutated after calling get it('calls onValue only once after get request with a non-default query', async () => { const db = getDatabase(defaultApp); diff --git a/packages/rules-unit-testing/src/initialize.ts b/packages/rules-unit-testing/src/initialize.ts index d0442e24524..d8049022397 100644 --- a/packages/rules-unit-testing/src/initialize.ts +++ b/packages/rules-unit-testing/src/initialize.ts @@ -82,6 +82,7 @@ export async function initializeTestEnvironment( } if (config.database?.rules) { + console.log('RUNNING RULES'); assertEmulatorRunning(emulators, 'database'); await loadDatabaseRules( emulators.database, diff --git a/scripts/emulator-testing/emulators/database-emulator.ts b/scripts/emulator-testing/emulators/database-emulator.ts index 0d6675e9a5b..37111544083 100644 --- a/scripts/emulator-testing/emulators/database-emulator.ts +++ b/scripts/emulator-testing/emulators/database-emulator.ts @@ -41,7 +41,7 @@ export class DatabaseEmulator extends Emulator { { uri: `http://localhost:${this.port}/.settings/rules.json?ns=${this.namespace}`, headers: { Authorization: 'Bearer owner' }, - body: '{ "rules": { ".read": true, ".write": true } }' + body: '{ "rules": { ".read": true, ".write": true, "testing": { ".indexOn": "testIndex" } } }' }, (error, response, body) => { if (error) reject(error); From 9e09bd2f9daf7c6b8668674468075cfc64d07304 Mon Sep 17 00:00:00 2001 From: Maneesh Tewani Date: Fri, 12 Aug 2022 15:30:18 -0700 Subject: [PATCH 07/20] Create warm-pillows-know.md --- .changeset/warm-pillows-know.md | 7 +++++++ 1 file changed, 7 insertions(+) create mode 100644 .changeset/warm-pillows-know.md diff --git a/.changeset/warm-pillows-know.md b/.changeset/warm-pillows-know.md new file mode 100644 index 00000000000..eb7be7d68f2 --- /dev/null +++ b/.changeset/warm-pillows-know.md @@ -0,0 +1,7 @@ +--- +"@firebase/database-compat": patch +"@firebase/database": patch +"@firebase/rules-unit-testing": patch +--- + +Fixed faulty transaction bug causing filtered index queries to override default queries. From 76dfbf67e5528ad734772e5e3f940430026c8739 Mon Sep 17 00:00:00 2001 From: Maneesh Tewani Date: Fri, 12 Aug 2022 15:33:14 -0700 Subject: [PATCH 08/20] Removed unnecessary changes --- packages/database-compat/test/query.test.ts | 41 +--------- packages/database/src/core/Repo.ts | 84 +++++++-------------- 2 files changed, 28 insertions(+), 97 deletions(-) diff --git a/packages/database-compat/test/query.test.ts b/packages/database-compat/test/query.test.ts index 080b439cc5c..76b57da3ff8 100644 --- a/packages/database-compat/test/query.test.ts +++ b/packages/database-compat/test/query.test.ts @@ -30,13 +30,7 @@ import { } from '../../database/test/helpers/EventAccumulator'; import { DataSnapshot, Query, Reference } from '../src/api/Reference'; -import { - createTestApp, - getFreshRepo, - getPath, - getRandomNode, - pause -} from './helpers/util'; +import { getFreshRepo, getPath, getRandomNode, pause } from './helpers/util'; use(chaiAsPromised); @@ -4670,39 +4664,6 @@ describe('Query Tests', () => { }); }); }); - it.only('can properly handle unknown deep merges', async () => { - const app = createTestApp(); - const root = app.database().ref().child('testing'); - // TODO(mtewani): This repro requires an index to be created. - await root.remove(); - - const query = root.orderByChild('testIndex').limitToFirst(2); - - await root - .child('i|1') - .set({ testIndex: 3, timestamp: Date.now(), action: 'test' }); - await root - .child('i|2') - .set({ testIndex: 1, timestamp: Date.now(), action: 'test' }); - await root - .child('i|3') - .set({ testIndex: 2, timestamp: Date.now(), action: 'test' }); - const childAdded = query.on('child_added', snap => { - const value = snap.val(); - expect(value).to.haveOwnProperty('timestamp'); - expect(value).to.haveOwnProperty('action'); - expect(value).to.haveOwnProperty('testIndex'); - }); - const onValue = root.child('i|1').on('value', snap => { - console.log('onValue', snap.val()); - //no-op - }); - await root.child('i|1').update({ timestamp: `${Date.now()}|1` }); - console.log('wrote value'); - await new Promise(resolve => setTimeout(resolve, 4000)); - root.child('i|1').off('value', onValue); - query.off('child_added', childAdded); - }); it('Can JSON serialize refs', () => { const ref = getRandomNode() as Reference; diff --git a/packages/database/src/core/Repo.ts b/packages/database/src/core/Repo.ts index 809c1556d81..3c2a457ba59 100644 --- a/packages/database/src/core/Repo.ts +++ b/packages/database/src/core/Repo.ts @@ -24,8 +24,6 @@ import { stringify } from '@firebase/util'; -import { ValueEventRegistration } from '../api/Reference_impl'; - import { AppCheckTokenProvider } from './AppCheckTokenProvider'; import { AuthTokenProvider } from './AuthTokenProvider'; import { PersistentConnection } from './PersistentConnection'; @@ -63,7 +61,7 @@ import { syncTreeCalcCompleteEventCache, syncTreeGetServerValue, syncTreeRemoveEventRegistration, - syncTreeTagForQuery + syncTreeRegisterQuery } from './SyncTree'; import { Indexable } from './util/misc'; import { @@ -445,6 +443,7 @@ function repoUpdateInfo(repo: Repo, pathString: string, value: unknown): void { function repoGetNextWriteId(repo: Repo): number { return repo.nextWriteId_++; } + /** * The purpose of `getValue` is to return the latest known value * satisfying `query`. @@ -453,18 +452,14 @@ function repoGetNextWriteId(repo: Repo): number { * belonging to active listeners. If they are found, such values * are considered to be the most up-to-date. * - * If the client is not connected, this method will wait until the - * repo has established a connection and then request the value for `query`. - * If the client is not able to retrieve the query result for another reason, - * it reports an error. + * If the client is not connected, this method will try to + * establish a connection and request the value for `query`. If + * the client is not able to retrieve the query result, it reports + * an error. * * @param query - The query to surface a value for. */ -export function repoGetValue( - repo: Repo, - query: QueryContext, - eventRegistration: ValueEventRegistration -): Promise { +export function repoGetValue(repo: Repo, query: QueryContext): Promise { // Only active queries are cached. There is no persisted cache. const cached = syncTreeGetServerValue(repo.serverSyncTree_, query); if (cached != null) { @@ -475,57 +470,32 @@ export function repoGetValue( const node = nodeFromJSON(payload).withIndex( query._queryParams.getIndex() ); - /** - * Below we simulate the actions of an `onlyOnce` `onValue()` event where: - * Add an event registration, - * Update data at the path, - * Raise any events, - * Cleanup the SyncTree - */ - syncTreeAddEventRegistration( - repo.serverSyncTree_, - query, - eventRegistration, - true - ); - let events: Event[]; + // if this is a filtered query, then overwrite at path if (query._queryParams.loadsAllData()) { - events = syncTreeApplyServerOverwrite( - repo.serverSyncTree_, - query._path, - node - ); + syncTreeApplyServerOverwrite(repo.serverSyncTree_, query._path, node); } else { - const tag = syncTreeTagForQuery(repo.serverSyncTree_, query); - events = syncTreeApplyTaggedQueryOverwrite( + // Simulate `syncTreeAddEventRegistration` without events/listener setup. + // We do this (along with the syncTreeRemoveEventRegistration` below) so that + // `repoGetValue` results have the same cache effects as initial listener(s) + // updates. + const tag = syncTreeRegisterQuery(repo.serverSyncTree_, query); + syncTreeApplyTaggedQueryOverwrite( repo.serverSyncTree_, query._path, node, tag ); + // Call `syncTreeRemoveEventRegistration` with a null event registration, since there is none. + // Note: The below code essentially unregisters the query and cleans up any views/syncpoints temporarily created above. } - /* - * We need to raise events in the scenario where `get()` is called at a parent path, and - * while the `get()` is pending, `onValue` is called at a child location. While get() is waiting - * for the data, `onValue` will register a new event. Then, get() will come back, and update the syncTree - * and its corresponding serverCache, including the child location where `onValue` is called. Then, - * `onValue` will receive the event from the server, but look at the syncTree and see that the data received - * from the server is already at the SyncPoint, and so the `onValue` callback will never get fired. - * Calling `eventQueueRaiseEventsForChangedPath()` is the correct way to propagate the events and - * ensure the corresponding child events will get fired. - */ - eventQueueRaiseEventsForChangedPath( - repo.eventQueue_, - query._path, - events - ); - syncTreeRemoveEventRegistration( + const cancels = syncTreeRemoveEventRegistration( repo.serverSyncTree_, query, - eventRegistration, - null, - true + null ); + if (cancels.length > 0) { + repoLog(repo, 'unexpected cancel events in repoGetValue'); + } return node; }, err => { @@ -982,7 +952,7 @@ export function repoStartTransaction( // Mark as run and add to our queue. transaction.status = TransactionStatus.RUN; const queueNode = treeSubTree(repo.transactionQueueTree_, path); - const nodeQueue = treeGetValue(queueNode) ?? []; + const nodeQueue = treeGetValue(queueNode) || []; nodeQueue.push(transaction); treeSetValue(queueNode, nodeQueue); @@ -1067,7 +1037,7 @@ function repoSendReadyTransactions( repoPruneCompletedTransactionsBelowNode(repo, node); } - if (treeGetValue(node) !== undefined) { + if (treeGetValue(node)) { const queue = repoBuildTransactionQueue(repo, node); assert(queue.length > 0, 'Sending zero length transaction queue'); @@ -1443,7 +1413,7 @@ function repoAggregateTransactionQueuesForNode( queue: Transaction[] ): void { const nodeQueue = treeGetValue(node); - if (nodeQueue !== undefined) { + if (nodeQueue) { for (let i = 0; i < nodeQueue.length; i++) { queue.push(nodeQueue[i]); } @@ -1462,7 +1432,7 @@ function repoPruneCompletedTransactionsBelowNode( node: Tree ): void { const queue = treeGetValue(node); - if (queue !== undefined) { + if (queue) { let to = 0; for (let from = 0; from < queue.length; from++) { if (queue[from].status !== TransactionStatus.COMPLETED) { @@ -1514,7 +1484,7 @@ function repoAbortTransactionsOnNode( node: Tree ): void { const queue = treeGetValue(node); - if (queue !== undefined) { + if (queue) { // Queue up the callbacks and fire them after cleaning up all of our // transaction state, since the callback could trigger more transactions // or sets. From e1b3014aa5781bf99def09ad82d466c2a01a4e0c Mon Sep 17 00:00:00 2001 From: Maneesh Tewani Date: Fri, 12 Aug 2022 15:33:41 -0700 Subject: [PATCH 09/20] Undid change --- packages/database/src/core/Repo.ts | 73 +++++++++++++++++++++--------- 1 file changed, 52 insertions(+), 21 deletions(-) diff --git a/packages/database/src/core/Repo.ts b/packages/database/src/core/Repo.ts index 3c2a457ba59..2d76af39a08 100644 --- a/packages/database/src/core/Repo.ts +++ b/packages/database/src/core/Repo.ts @@ -24,6 +24,8 @@ import { stringify } from '@firebase/util'; +import { ValueEventRegistration } from '../api/Reference_impl'; + import { AppCheckTokenProvider } from './AppCheckTokenProvider'; import { AuthTokenProvider } from './AuthTokenProvider'; import { PersistentConnection } from './PersistentConnection'; @@ -61,7 +63,7 @@ import { syncTreeCalcCompleteEventCache, syncTreeGetServerValue, syncTreeRemoveEventRegistration, - syncTreeRegisterQuery + syncTreeTagForQuery } from './SyncTree'; import { Indexable } from './util/misc'; import { @@ -452,14 +454,18 @@ function repoGetNextWriteId(repo: Repo): number { * belonging to active listeners. If they are found, such values * are considered to be the most up-to-date. * - * If the client is not connected, this method will try to - * establish a connection and request the value for `query`. If - * the client is not able to retrieve the query result, it reports - * an error. + * If the client is not connected, this method will wait until the + * repo has established a connection and then request the value for `query`. + * If the client is not able to retrieve the query result for another reason, + * it reports an error. * * @param query - The query to surface a value for. */ -export function repoGetValue(repo: Repo, query: QueryContext): Promise { +export function repoGetValue( + repo: Repo, + query: QueryContext, + eventRegistration: ValueEventRegistration +): Promise { // Only active queries are cached. There is no persisted cache. const cached = syncTreeGetServerValue(repo.serverSyncTree_, query); if (cached != null) { @@ -470,32 +476,57 @@ export function repoGetValue(repo: Repo, query: QueryContext): Promise { const node = nodeFromJSON(payload).withIndex( query._queryParams.getIndex() ); - // if this is a filtered query, then overwrite at path + /** + * Below we simulate the actions of an `onlyOnce` `onValue()` event where: + * Add an event registration, + * Update data at the path, + * Raise any events, + * Cleanup the SyncTree + */ + syncTreeAddEventRegistration( + repo.serverSyncTree_, + query, + eventRegistration, + true + ); + let events: Event[]; if (query._queryParams.loadsAllData()) { - syncTreeApplyServerOverwrite(repo.serverSyncTree_, query._path, node); + events = syncTreeApplyServerOverwrite( + repo.serverSyncTree_, + query._path, + node + ); } else { - // Simulate `syncTreeAddEventRegistration` without events/listener setup. - // We do this (along with the syncTreeRemoveEventRegistration` below) so that - // `repoGetValue` results have the same cache effects as initial listener(s) - // updates. - const tag = syncTreeRegisterQuery(repo.serverSyncTree_, query); - syncTreeApplyTaggedQueryOverwrite( + const tag = syncTreeTagForQuery(repo.serverSyncTree_, query); + events = syncTreeApplyTaggedQueryOverwrite( repo.serverSyncTree_, query._path, node, tag ); - // Call `syncTreeRemoveEventRegistration` with a null event registration, since there is none. - // Note: The below code essentially unregisters the query and cleans up any views/syncpoints temporarily created above. } - const cancels = syncTreeRemoveEventRegistration( + /* + * We need to raise events in the scenario where `get()` is called at a parent path, and + * while the `get()` is pending, `onValue` is called at a child location. While get() is waiting + * for the data, `onValue` will register a new event. Then, get() will come back, and update the syncTree + * and its corresponding serverCache, including the child location where `onValue` is called. Then, + * `onValue` will receive the event from the server, but look at the syncTree and see that the data received + * from the server is already at the SyncPoint, and so the `onValue` callback will never get fired. + * Calling `eventQueueRaiseEventsForChangedPath()` is the correct way to propagate the events and + * ensure the corresponding child events will get fired. + */ + eventQueueRaiseEventsForChangedPath( + repo.eventQueue_, + query._path, + events + ); + syncTreeRemoveEventRegistration( repo.serverSyncTree_, query, - null + eventRegistration, + null, + true ); - if (cancels.length > 0) { - repoLog(repo, 'unexpected cancel events in repoGetValue'); - } return node; }, err => { From 8e67f2d9c4032598de75492c71b057c6978b7253 Mon Sep 17 00:00:00 2001 From: Maneesh Tewani Date: Fri, 12 Aug 2022 15:34:06 -0700 Subject: [PATCH 10/20] Removed console log --- packages/rules-unit-testing/src/initialize.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/rules-unit-testing/src/initialize.ts b/packages/rules-unit-testing/src/initialize.ts index d8049022397..d0442e24524 100644 --- a/packages/rules-unit-testing/src/initialize.ts +++ b/packages/rules-unit-testing/src/initialize.ts @@ -82,7 +82,6 @@ export async function initializeTestEnvironment( } if (config.database?.rules) { - console.log('RUNNING RULES'); assertEmulatorRunning(emulators, 'database'); await loadDatabaseRules( emulators.database, From 858d6a1ab59b29251efcd811c1bb207bc84b4309 Mon Sep 17 00:00:00 2001 From: Maneesh Tewani Date: Fri, 12 Aug 2022 15:43:52 -0700 Subject: [PATCH 11/20] Replaced count check --- .../database/test/exp/integration.test.ts | 19 ++++++++++--------- .../database/test/helpers/EventAccumulator.ts | 2 +- 2 files changed, 11 insertions(+), 10 deletions(-) diff --git a/packages/database/test/exp/integration.test.ts b/packages/database/test/exp/integration.test.ts index dc4a1365bcc..15d1df61b32 100644 --- a/packages/database/test/exp/integration.test.ts +++ b/packages/database/test/exp/integration.test.ts @@ -121,10 +121,10 @@ describe('Database@exp Tests', () => { unsubscribe(); }); - it('can properly handle unknown deep merges', async () => { + it.only('can properly handle unknown deep merges', async () => { const database = getDatabase(defaultApp); const root = ref(database, 'testing'); - await remove(root); + await set(root, {}); const q = query(root, orderByChild('testIndex'), limitToFirst(2)); @@ -146,9 +146,8 @@ describe('Database@exp Tests', () => { action: 'test' } }); - const ec = EventAccumulatorFactory.waitsForExactCount(1); + const ec = EventAccumulatorFactory.waitsForExactCount(2); const onChildAddedCb = onChildAdded(q, snap => { - const value = snap.val(); ec.addEvent(snap); }); const onValueCb = onValue(i1, () => { @@ -157,11 +156,13 @@ describe('Database@exp Tests', () => { await update(i1, { timestamp: `${Date.now()}|1` }); - const [result] = await ec.promise; - const value = result.val(); - expect(value).to.haveOwnProperty('timestamp'); - expect(value).to.haveOwnProperty('action'); - expect(value).to.haveOwnProperty('testIndex'); + const results = await ec.promise; + results.forEach(result => { + const value = result.val(); + expect(value).to.haveOwnProperty('timestamp'); + expect(value).to.haveOwnProperty('action'); + expect(value).to.haveOwnProperty('testIndex'); + }); onChildAddedCb(); onValueCb(); }); diff --git a/packages/database/test/helpers/EventAccumulator.ts b/packages/database/test/helpers/EventAccumulator.ts index fc293b28b05..da6cb2343b5 100644 --- a/packages/database/test/helpers/EventAccumulator.ts +++ b/packages/database/test/helpers/EventAccumulator.ts @@ -15,7 +15,7 @@ * limitations under the License. */ -export const EventAccumulatorFactory = { +export const EventAccumulatorFactory = { // TODO: Convert to use generics to take the most advantage of types. waitsForCount: maxCount => { // Note: This should be used sparingly as it can result in more events being raised than expected let count = 0; From e13f2cd8e1a805feb226ab9c0f04fd0e9b63bed8 Mon Sep 17 00:00:00 2001 From: Maneesh Tewani Date: Fri, 12 Aug 2022 15:44:11 -0700 Subject: [PATCH 12/20] Updated formatting --- packages/database/test/helpers/EventAccumulator.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/database/test/helpers/EventAccumulator.ts b/packages/database/test/helpers/EventAccumulator.ts index da6cb2343b5..332443f5412 100644 --- a/packages/database/test/helpers/EventAccumulator.ts +++ b/packages/database/test/helpers/EventAccumulator.ts @@ -15,7 +15,8 @@ * limitations under the License. */ -export const EventAccumulatorFactory = { // TODO: Convert to use generics to take the most advantage of types. +export const EventAccumulatorFactory = { + // TODO: Convert to use generics to take the most advantage of types. waitsForCount: maxCount => { // Note: This should be used sparingly as it can result in more events being raised than expected let count = 0; From 88ca7d629ec0e5e6dd2343701a60971a23888d86 Mon Sep 17 00:00:00 2001 From: Maneesh Tewani Date: Fri, 12 Aug 2022 15:47:16 -0700 Subject: [PATCH 13/20] Removed unnecessary import --- packages/database/test/exp/integration.test.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/database/test/exp/integration.test.ts b/packages/database/test/exp/integration.test.ts index 15d1df61b32..5c7a1211b3b 100644 --- a/packages/database/test/exp/integration.test.ts +++ b/packages/database/test/exp/integration.test.ts @@ -29,7 +29,6 @@ import { orderByChild, query, refFromURL, - remove, set, startAt, update, From 5b1c04b1407f79b1a0f062545f8d3f964e094841 Mon Sep 17 00:00:00 2001 From: Maneesh Tewani Date: Mon, 15 Aug 2022 09:32:13 -0700 Subject: [PATCH 14/20] Removed only and added comment --- packages/database/test/exp/integration.test.ts | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/packages/database/test/exp/integration.test.ts b/packages/database/test/exp/integration.test.ts index 5c7a1211b3b..e288fde73e1 100644 --- a/packages/database/test/exp/integration.test.ts +++ b/packages/database/test/exp/integration.test.ts @@ -120,7 +120,11 @@ describe('Database@exp Tests', () => { unsubscribe(); }); - it.only('can properly handle unknown deep merges', async () => { + it('can properly handle unknown deep merges', async () => { + // Note: This test requires `testIndex` to be added as an index. + // Without an index, the test will erroneously pass. + // But we are unable to add indexes on the fly to the actual db, so + // running on the emulator is the best way to test this. const database = getDatabase(defaultApp); const root = ref(database, 'testing'); await set(root, {}); From 8611f5a6d394462f8ef0de0a38bd3839209d3580 Mon Sep 17 00:00:00 2001 From: Maneesh Tewani Date: Mon, 15 Aug 2022 09:55:56 -0700 Subject: [PATCH 15/20] Removed changes to database.rules.json --- config/database.rules.json | 5 +---- scripts/emulator-testing/emulators/database-emulator.ts | 2 +- 2 files changed, 2 insertions(+), 5 deletions(-) diff --git a/config/database.rules.json b/config/database.rules.json index 1ca1cb35b58..b104e9c240e 100644 --- a/config/database.rules.json +++ b/config/database.rules.json @@ -1,9 +1,6 @@ { "rules": { ".read": true, - ".write": true, - "testing": { - ".indexOn": "testIndex" - } + ".write": true } } \ No newline at end of file diff --git a/scripts/emulator-testing/emulators/database-emulator.ts b/scripts/emulator-testing/emulators/database-emulator.ts index 37111544083..76730af0895 100644 --- a/scripts/emulator-testing/emulators/database-emulator.ts +++ b/scripts/emulator-testing/emulators/database-emulator.ts @@ -41,7 +41,7 @@ export class DatabaseEmulator extends Emulator { { uri: `http://localhost:${this.port}/.settings/rules.json?ns=${this.namespace}`, headers: { Authorization: 'Bearer owner' }, - body: '{ "rules": { ".read": true, ".write": true, "testing": { ".indexOn": "testIndex" } } }' + body: '{ "rules": { ".read": true, ".write": true, "testing": { ".indexOn": "testIndex" } } }' // TODO: If this gets more complex, we should move this to a file. }, (error, response, body) => { if (error) reject(error); From 89dfd2a78b7dcd2e2d1102576a1e974cfe68e31e Mon Sep 17 00:00:00 2001 From: Maneesh Tewani Date: Mon, 15 Aug 2022 10:05:11 -0700 Subject: [PATCH 16/20] Fixed formatting --- packages/database/test/exp/integration.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/database/test/exp/integration.test.ts b/packages/database/test/exp/integration.test.ts index e288fde73e1..22c30d7b4aa 100644 --- a/packages/database/test/exp/integration.test.ts +++ b/packages/database/test/exp/integration.test.ts @@ -123,7 +123,7 @@ describe('Database@exp Tests', () => { it('can properly handle unknown deep merges', async () => { // Note: This test requires `testIndex` to be added as an index. // Without an index, the test will erroneously pass. - // But we are unable to add indexes on the fly to the actual db, so + // But we are unable to add indexes on the fly to the actual db, so // running on the emulator is the best way to test this. const database = getDatabase(defaultApp); const root = ref(database, 'testing'); From f85a1beb135a6bd394681c43bcc94dcd25fbdacd Mon Sep 17 00:00:00 2001 From: Maneesh Tewani Date: Mon, 15 Aug 2022 13:49:10 -0700 Subject: [PATCH 17/20] Updated database emulator script and rules --- config/database.rules.json | 5 ++++- packages/database/test/exp/integration.test.ts | 4 +--- scripts/emulator-testing/emulators/database-emulator.ts | 7 +++++-- 3 files changed, 10 insertions(+), 6 deletions(-) diff --git a/config/database.rules.json b/config/database.rules.json index b104e9c240e..1ca1cb35b58 100644 --- a/config/database.rules.json +++ b/config/database.rules.json @@ -1,6 +1,9 @@ { "rules": { ".read": true, - ".write": true + ".write": true, + "testing": { + ".indexOn": "testIndex" + } } } \ No newline at end of file diff --git a/packages/database/test/exp/integration.test.ts b/packages/database/test/exp/integration.test.ts index 22c30d7b4aa..3be79d5c447 100644 --- a/packages/database/test/exp/integration.test.ts +++ b/packages/database/test/exp/integration.test.ts @@ -122,9 +122,7 @@ describe('Database@exp Tests', () => { it('can properly handle unknown deep merges', async () => { // Note: This test requires `testIndex` to be added as an index. - // Without an index, the test will erroneously pass. - // But we are unable to add indexes on the fly to the actual db, so - // running on the emulator is the best way to test this. + // Please run `yarn test:setup` to ensure that this gets added. const database = getDatabase(defaultApp); const root = ref(database, 'testing'); await set(root, {}); diff --git a/scripts/emulator-testing/emulators/database-emulator.ts b/scripts/emulator-testing/emulators/database-emulator.ts index 76730af0895..7c9576c4b4c 100644 --- a/scripts/emulator-testing/emulators/database-emulator.ts +++ b/scripts/emulator-testing/emulators/database-emulator.ts @@ -19,6 +19,8 @@ import * as request from 'request'; import { Emulator } from './emulator'; +import * as rulesJSON from '../../../config/database.rules.json'; + export class DatabaseEmulator extends Emulator { namespace: string; @@ -35,13 +37,14 @@ export class DatabaseEmulator extends Emulator { } setPublicRules(): Promise { - console.log('Setting rule {".read": true, ".write": true} to emulator ...'); + const jsonRules = JSON.stringify(rulesJSON); + console.log(`Setting rule ${jsonRules} to emulator ...`); return new Promise((resolve, reject) => { request.put( { uri: `http://localhost:${this.port}/.settings/rules.json?ns=${this.namespace}`, headers: { Authorization: 'Bearer owner' }, - body: '{ "rules": { ".read": true, ".write": true, "testing": { ".indexOn": "testIndex" } } }' // TODO: If this gets more complex, we should move this to a file. + body: jsonRules }, (error, response, body) => { if (error) reject(error); From f645eba73fb2190a4b6779d7f0e2fee3f4c4f983 Mon Sep 17 00:00:00 2001 From: Maneesh Tewani Date: Mon, 15 Aug 2022 13:50:44 -0700 Subject: [PATCH 18/20] Updated formatting and README --- README.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index f9810c99638..89aafc10c79 100644 --- a/README.md +++ b/README.md @@ -126,7 +126,8 @@ command, as follows: ```bash -# Select the Firebase project via the text-based UI. +# Select the Firebase project via the text-based UI. This will run tools/config.js +# and deploy from config/ to your local project. $ yarn test:setup # Specify the Firebase project via the command-line arguments. From 13b9247b43000b29f130a7323ae86ca075885671 Mon Sep 17 00:00:00 2001 From: Maneesh Tewani Date: Mon, 15 Aug 2022 14:14:30 -0700 Subject: [PATCH 19/20] Removed extra affected package --- .changeset/warm-pillows-know.md | 1 - 1 file changed, 1 deletion(-) diff --git a/.changeset/warm-pillows-know.md b/.changeset/warm-pillows-know.md index eb7be7d68f2..760499ea4e1 100644 --- a/.changeset/warm-pillows-know.md +++ b/.changeset/warm-pillows-know.md @@ -1,7 +1,6 @@ --- "@firebase/database-compat": patch "@firebase/database": patch -"@firebase/rules-unit-testing": patch --- Fixed faulty transaction bug causing filtered index queries to override default queries. From 23080226a2eff9b5afe1f294329ccea6efd2a441 Mon Sep 17 00:00:00 2001 From: Maneesh Tewani Date: Mon, 15 Aug 2022 14:19:20 -0700 Subject: [PATCH 20/20] Addressed comments --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 89aafc10c79..25b95289b9b 100644 --- a/README.md +++ b/README.md @@ -127,7 +127,7 @@ command, as follows: ```bash # Select the Firebase project via the text-based UI. This will run tools/config.js -# and deploy from config/ to your local project. +# and deploy from config/ to your Firebase project. $ yarn test:setup # Specify the Firebase project via the command-line arguments.