diff --git a/.changeset/rotten-tables-brush.md b/.changeset/rotten-tables-brush.md new file mode 100644 index 00000000000..6db6bcd8618 --- /dev/null +++ b/.changeset/rotten-tables-brush.md @@ -0,0 +1,5 @@ +--- +"@firebase/database": patch +--- + +Fixed issue where `get()` saved results incorrectly for non-default queries. diff --git a/.vscode/launch.json b/.vscode/launch.json index c727a20b4e7..b2d5d88b94d 100644 --- a/.vscode/launch.json +++ b/.vscode/launch.json @@ -8,14 +8,15 @@ "type": "node", "request": "launch", "name": "RTDB Unit Tests (Node)", - "program": "${workspaceRoot}/packages/firebase/node_modules/.bin/_mocha", + "program": "${workspaceRoot}/node_modules/.bin/_mocha", "cwd": "${workspaceRoot}/packages/database", "args": [ "test/{,!(browser)/**/}*.test.ts", - "--file", "index.node.ts", - "--config", "../../config/mocharc.node.js" + "--file", "src/index.node.ts", + "--config", "../../config/mocharc.node.js", ], "env": { + "TS_NODE_FILES":true, "TS_NODE_CACHE": "NO", "TS_NODE_COMPILER_OPTIONS" : "{\"module\":\"commonjs\"}" }, @@ -30,7 +31,7 @@ "cwd": "${workspaceRoot}/packages/firestore", "args": [ "--require", "babel-register.js", - "--require", "index.node.ts", + "--require", "src/index.node.ts", "--timeout", "5000", "test/{,!(browser|integration)/**/}*.test.ts", "--exit" diff --git a/packages/database/package.json b/packages/database/package.json index 054862c91ad..745c8e69b75 100644 --- a/packages/database/package.json +++ b/packages/database/package.json @@ -56,7 +56,8 @@ "@firebase/app": "0.7.25", "rollup": "2.72.1", "rollup-plugin-typescript2": "0.31.2", - "typescript": "4.2.2" + "typescript": "4.2.2", + "uuid": "^8.3.2" }, "repository": { "directory": "packages/database", diff --git a/packages/database/src/core/PersistentConnection.ts b/packages/database/src/core/PersistentConnection.ts index 62b27a37686..beeaa4e4389 100644 --- a/packages/database/src/core/PersistentConnection.ts +++ b/packages/database/src/core/PersistentConnection.ts @@ -206,12 +206,6 @@ export class PersistentConnection extends ServerActions { onComplete: (message: { [k: string]: unknown }) => { const payload = message['d'] as string; if (message['s'] === 'ok') { - this.onDataUpdate_( - request['p'], - payload, - /*isMerge*/ false, - /*tag*/ null - ); deferred.resolve(payload); } else { deferred.reject(payload); @@ -265,7 +259,7 @@ export class PersistentConnection extends ServerActions { ); assert( !this.listens.get(pathString)!.has(queryId), - 'listen() called twice for same path/queryId.' + `listen() called twice for same path/queryId.` ); const listenSpec: ListenSpec = { onComplete, diff --git a/packages/database/src/core/Repo.ts b/packages/database/src/core/Repo.ts index 9479f620529..3c2a457ba59 100644 --- a/packages/database/src/core/Repo.ts +++ b/packages/database/src/core/Repo.ts @@ -60,7 +60,8 @@ import { syncTreeApplyUserOverwrite, syncTreeCalcCompleteEventCache, syncTreeGetServerValue, - syncTreeRemoveEventRegistration + syncTreeRemoveEventRegistration, + syncTreeRegisterQuery } from './SyncTree'; import { Indexable } from './util/misc'; import { @@ -466,16 +467,36 @@ export function repoGetValue(repo: Repo, query: QueryContext): Promise { } return repo.server_.get(query).then( payload => { - const node = nodeFromJSON(payload as string).withIndex( + const node = nodeFromJSON(payload).withIndex( query._queryParams.getIndex() ); - const events = syncTreeApplyServerOverwrite( + // if this is a filtered query, then overwrite at path + if (query._queryParams.loadsAllData()) { + 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( + 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( repo.serverSyncTree_, - query._path, - node + query, + null ); - eventQueueRaiseEventsAtPath(repo.eventQueue_, query._path, events); - return Promise.resolve(node); + if (cancels.length > 0) { + repoLog(repo, 'unexpected cancel events in repoGetValue'); + } + return node; }, err => { repoLog(repo, 'get for query ' + stringify(query) + ' failed: ' + err); diff --git a/packages/database/src/core/SyncTree.ts b/packages/database/src/core/SyncTree.ts index a2d0888f03c..bffb4aeb104 100644 --- a/packages/database/src/core/SyncTree.ts +++ b/packages/database/src/core/SyncTree.ts @@ -423,6 +423,38 @@ export function syncTreeRemoveEventRegistration( return cancelEvents; } +/** + * This function was added to support non-listener queries, + * specifically for use in repoGetValue. It sets up all the same + * local cache data-structures (SyncPoint + View) that are + * needed for listeners without installing an event registration. + * If `query` is not `loadsAllData`, it will also provision a tag for + * the query so that query results can be merged into the sync + * tree using existing logic for tagged listener queries. + * + * @param syncTree - Synctree to add the query to. + * @param query - Query to register + * @returns tag as a string if query is not a default query, null if query is not. + */ +export function syncTreeRegisterQuery(syncTree: SyncTree, query: QueryContext) { + const { syncPoint, serverCache, writesCache, serverCacheComplete } = + syncTreeRegisterSyncPoint(query, syncTree); + const view = syncPointGetView( + syncPoint, + query, + writesCache, + serverCache, + serverCacheComplete + ); + if (!syncPoint.views.has(query._queryIdentifier)) { + syncPoint.views.set(query._queryIdentifier, view); + } + if (!query._queryParams.loadsAllData()) { + return syncTreeTagForQuery_(syncTree, query); + } + return null; +} + /** * Apply new server data for the specified tagged query. * @@ -483,15 +515,14 @@ export function syncTreeApplyTaggedQueryMerge( } /** - * Add an event callback for the specified query. - * - * @returns Events to raise. + * Creates a new syncpoint for a query and creates a tag if the view doesn't exist. + * Extracted from addEventRegistration to allow `repoGetValue` to properly set up the SyncTree + * without actually listening on a query. */ -export function syncTreeAddEventRegistration( - syncTree: SyncTree, +export function syncTreeRegisterSyncPoint( query: QueryContext, - eventRegistration: EventRegistration -): Event[] { + syncTree: SyncTree +) { const path = query._path; let serverCache: Node | null = null; @@ -550,6 +581,35 @@ export function syncTreeAddEventRegistration( syncTree.tagToQueryMap.set(tag, queryKey); } const writesCache = writeTreeChildWrites(syncTree.pendingWriteTree_, path); + return { + syncPoint, + writesCache, + serverCache, + serverCacheComplete, + foundAncestorDefaultView, + viewAlreadyExists + }; +} + +/** + * Add an event callback for the specified query. + * + * @returns Events to raise. + */ +export function syncTreeAddEventRegistration( + syncTree: SyncTree, + query: QueryContext, + eventRegistration: EventRegistration +): Event[] { + const { + syncPoint, + serverCache, + writesCache, + serverCacheComplete, + viewAlreadyExists, + foundAncestorDefaultView + } = syncTreeRegisterSyncPoint(query, syncTree); + let events = syncPointAddEventRegistration( syncPoint, query, @@ -937,7 +997,6 @@ function syncTreeSetupListener_( const path = query._path; const tag = syncTreeTagForQuery_(syncTree, query); const listener = syncTreeCreateListenerForView_(syncTree, view); - const events = syncTree.listenProvider_.startListening( syncTreeQueryForListening_(query), tag, diff --git a/packages/database/src/core/util/Path.ts b/packages/database/src/core/util/Path.ts index c0ddcf5061b..6aba15bb98f 100644 --- a/packages/database/src/core/util/Path.ts +++ b/packages/database/src/core/util/Path.ts @@ -230,7 +230,7 @@ export function pathEquals(path: Path, other: Path): boolean { } /** - * @returns True if this path is a parent (or the same as) other + * @returns True if this path is a parent of (or the same as) other */ export function pathContains(path: Path, other: Path): boolean { let i = path.pieceNum_; diff --git a/packages/database/test/exp/integration.test.ts b/packages/database/test/exp/integration.test.ts index 0d0858c28c8..c695a3ee6be 100644 --- a/packages/database/test/exp/integration.test.ts +++ b/packages/database/test/exp/integration.test.ts @@ -15,29 +15,40 @@ * limitations under the License. */ -// eslint-disable-next-line import/no-extraneous-dependencies import { initializeApp, deleteApp } from '@firebase/app'; import { Deferred } from '@firebase/util'; import { expect } from 'chai'; -import { onValue, set } from '../../src/api/Reference_impl'; import { get, + limitToFirst, + onValue, + query, + refFromURL, + set, + startAt +} from '../../src/api/Reference_impl'; +import { getDatabase, goOffline, goOnline, push, ref, - refFromURL, runTransaction } from '../../src/index'; import { EventAccumulatorFactory } from '../helpers/EventAccumulator'; -import { DATABASE_ADDRESS, DATABASE_URL } from '../helpers/util'; +import { + DATABASE_ADDRESS, + DATABASE_URL, + getUniqueRef, + waitFor +} from '../helpers/util'; export function createTestApp() { return initializeApp({ databaseURL: DATABASE_URL }); } +// Note: these run in parallel with the node environment. If you use the same paths in parallel, you may experience race conditions. describe('Database@exp Tests', () => { let defaultApp; @@ -84,8 +95,8 @@ describe('Database@exp Tests', () => { const db = getDatabase(defaultApp); const fooRef = ref(db, 'foo'); - const ea = EventAccumulatorFactory.waitsForCount(2); - onValue(fooRef, snap => { + const ea = EventAccumulatorFactory.waitsForExactCount(2); + const unsubscribe = onValue(fooRef, snap => { ea.addEvent(snap.val()); }); @@ -95,14 +106,124 @@ describe('Database@exp Tests', () => { const [snap1, snap2] = await ea.promise; expect(snap1).to.equal('a'); expect(snap2).to.equal('b'); + unsubscribe(); + }); + + // 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); + const { ref: testRef } = getUniqueRef(db); + const queries = [ + query(testRef, limitToFirst(1)), + query(testRef, startAt('child1')), + query(testRef, startAt('child2')), + query(testRef, limitToFirst(2)) + ]; + await Promise.all( + queries.map(async q => { + const initial = [{ name: 'child1' }, { name: 'child2' }]; + const ec = EventAccumulatorFactory.waitsForExactCount(1); + + await set(testRef, initial); + const unsubscribe = onValue(testRef, snapshot => { + ec.addEvent(snapshot.val()); + }); + await get(q); + await waitFor(2000); + const [snap] = await ec.promise; + expect(snap).to.deep.equal(initial); + unsubscribe(); + }) + ); + }); + + it('calls onValue and expects no issues with removing the listener', async () => { + const db = getDatabase(defaultApp); + const { ref: testRef } = getUniqueRef(db); + const initial = [{ name: 'child1' }, { name: 'child2' }]; + const ea = EventAccumulatorFactory.waitsForExactCount(1); + await set(testRef, initial); + const unsubscribe = onValue(testRef, snapshot => { + ea.addEvent(snapshot.val()); + }); + await get(query(testRef)); + await waitFor(2000); + const update = [{ name: 'child1' }, { name: 'child20' }]; + unsubscribe(); + await set(testRef, update); + const [snap1] = await ea.promise; + expect(snap1).to.deep.eq(initial); + }); + + it('calls onValue only once after get request with a default query', async () => { + const db = getDatabase(defaultApp); + const { ref: testRef } = getUniqueRef(db); + const initial = [{ name: 'child1' }, { name: 'child2' }]; + const ea = EventAccumulatorFactory.waitsForExactCount(1); + + await set(testRef, initial); + const unsubscribe = onValue(testRef, snapshot => { + ea.addEvent(snapshot.val()); + expect(snapshot.val()).to.deep.eq(initial); + }); + await get(query(testRef)); + await waitFor(2000); + const [snap] = await ea.promise; + expect(snap).to.deep.equal(initial); + unsubscribe(); + }); + it('calls onValue only once after get request with a nested query', async () => { + const db = getDatabase(defaultApp); + const ea = EventAccumulatorFactory.waitsForExactCount(1); + const { ref: testRef, path } = getUniqueRef(db); + const initial = { + test: { + abc: 123 + } + }; + + await set(testRef, initial); + const unsubscribe = onValue(testRef, snapshot => { + ea.addEvent(snapshot.val()); + }); + const nestedRef = ref(db, path + '/test'); + const result = await get(query(nestedRef)); + await waitFor(2000); + const [snap] = await ea.promise; + expect(snap).to.deep.equal(initial); + expect(result.val()).to.deep.eq(initial.test); + unsubscribe(); + }); + it('calls onValue only once after parent get request', async () => { + const db = getDatabase(defaultApp); + const { ref: testRef, path } = getUniqueRef(db); + const ea = EventAccumulatorFactory.waitsForExactCount(1); + const initial = { + test: { + abc: 123 + } + }; + + await set(testRef, initial); + const nestedRef = ref(db, path + '/test'); + const unsubscribe = onValue(nestedRef, snapshot => { + ea.addEvent(snapshot.val()); + }); + const result = await get(query(testRef)); + const events = await ea.promise; + await waitFor(2000); + expect(events.length).to.equal(1); + expect(events[0]).to.deep.eq(initial.test); + expect(result.val()).to.deep.equal(initial); + unsubscribe(); }); it('Can use onlyOnce', async () => { const db = getDatabase(defaultApp); const fooRef = ref(db, 'foo'); - const ea = EventAccumulatorFactory.waitsForCount(1); - onValue( + const ea = EventAccumulatorFactory.waitsForExactCount(1); + const unsubscribe = onValue( fooRef, snap => { ea.addEvent(snap.val()); @@ -114,14 +235,15 @@ describe('Database@exp Tests', () => { await set(fooRef, 'b'); const [snap1] = await ea.promise; - expect(snap1).to.equal('a'); + expect(snap1).to.equal('a'); // This doesn't test that onValue was only triggered once + unsubscribe(); }); it('Can unsubscribe', async () => { const db = getDatabase(defaultApp); const fooRef = ref(db, 'foo'); - const ea = EventAccumulatorFactory.waitsForCount(1); + const ea = EventAccumulatorFactory.waitsForExactCount(1); const unsubscribe = onValue(fooRef, snap => { ea.addEvent(snap.val()); }); @@ -130,8 +252,9 @@ describe('Database@exp Tests', () => { unsubscribe(); await set(fooRef, 'b'); - const [snap1] = await ea.promise; - expect(snap1).to.equal('a'); + const events = await ea.promise; + expect(events.length).to.equal(1); + expect(events[0]).to.equal('a'); }); it('Can goOffline/goOnline', async () => { @@ -163,7 +286,7 @@ describe('Database@exp Tests', () => { const database = getDatabase(defaultApp); const counterRef = push(ref(database, 'counter')); - onValue(counterRef, snap => { + const unsubscribe = onValue(counterRef, snap => { latestValue = snap.val(); deferred.resolve(); }); @@ -184,5 +307,6 @@ describe('Database@exp Tests', () => { expect(latestValue).to.equal(1); await incrementViaTransaction(); expect(latestValue).to.equal(2); + unsubscribe(); }); }); diff --git a/packages/database/test/helpers/EventAccumulator.ts b/packages/database/test/helpers/EventAccumulator.ts index 798e31405d0..fc293b28b05 100644 --- a/packages/database/test/helpers/EventAccumulator.ts +++ b/packages/database/test/helpers/EventAccumulator.ts @@ -17,6 +17,7 @@ export const EventAccumulatorFactory = { waitsForCount: maxCount => { + // Note: This should be used sparingly as it can result in more events being raised than expected let count = 0; const condition = () => count >= maxCount; const ea = new EventAccumulator(condition); @@ -48,6 +49,7 @@ export const EventAccumulatorFactory = { }; export class EventAccumulator { + // TODO: make these typesafe eventData = []; promise; resolve; diff --git a/packages/database/test/helpers/util.ts b/packages/database/test/helpers/util.ts index d50d42bd311..83ef94cdd43 100644 --- a/packages/database/test/helpers/util.ts +++ b/packages/database/test/helpers/util.ts @@ -15,6 +15,7 @@ * limitations under the License. */ +import { Database, ref } from '../../src'; import { ConnectionTarget } from '../../src/api/test_access'; // eslint-disable-next-line @typescript-eslint/no-require-imports @@ -70,3 +71,27 @@ export function shuffle(arr, randFn = Math.random) { arr[j] = tmp; } } + +// Waits for specific number of milliseconds before resolving +// Example: await waitFor(4000) will wait until 4 seconds to execute the next line of code. +export function waitFor(waitTimeInMS: number) { + return new Promise(resolve => setTimeout(resolve, waitTimeInMS)); +} + +/** + * Copied from https://stackoverflow.com/a/2117523 + * TODO: extract this into @firebase/util + */ +export function uuidv4(): string { + return 'xxxxxxxx-xxxx-4xxx-yxxx-xxxxxxxxxxxx'.replace(/[xy]/g, c => { + const r = (Math.random() * 16) | 0, + v = c === 'x' ? r : (r & 0x3) | 0x8; + return v.toString(16); + }); +} + +// Creates a unique reference using uuid +export function getUniqueRef(db: Database) { + const path = uuidv4(); + return { ref: ref(db, path), path }; +} diff --git a/packages/database/test/transport.test.ts b/packages/database/test/transport.test.ts index 4f4407bf3f2..7daee52f5db 100644 --- a/packages/database/test/transport.test.ts +++ b/packages/database/test/transport.test.ts @@ -47,6 +47,7 @@ describe('Force Transport', () => { WebSocketConnection.forceDisallow_ = false; mySandbox.restore(); }); + // Fails without retry due to running tests in async it('should enable websockets and disable longPolling', () => { forceWebSockets(); expect(spyWarn.called).to.equal(false);