From f02f35a1a4f690e1b9dbc06c051a6d5b5acd0ff2 Mon Sep 17 00:00:00 2001 From: Maneesh Tewani Date: Tue, 2 Aug 2022 13:41:08 -0700 Subject: [PATCH 01/19] Updated tests --- packages/database/src/api/Reference_impl.ts | 16 +- packages/database/src/core/Repo.ts | 78 +++--- packages/database/src/core/SyncTree.ts | 250 +++++++++--------- .../database/test/exp/integration.test.ts | 84 ++++-- packages/database/test/helpers/util.ts | 24 +- 5 files changed, 254 insertions(+), 198 deletions(-) diff --git a/packages/database/src/api/Reference_impl.ts b/packages/database/src/api/Reference_impl.ts index 3a54a6ec725..69383c2d1e2 100644 --- a/packages/database/src/api/Reference_impl.ts +++ b/packages/database/src/api/Reference_impl.ts @@ -34,18 +34,17 @@ import { PRIORITY_INDEX } from '../core/snap/indexes/PriorityIndex'; import { VALUE_INDEX } from '../core/snap/indexes/ValueIndex'; import { Node } from '../core/snap/Node'; import { syncPointSetReferenceConstructor } from '../core/SyncPoint'; -import { syncTreeSetReferenceConstructor } from '../core/SyncTree'; import { parseRepoInfo } from '../core/util/libs/parser'; import { nextPushId } from '../core/util/NextPushId'; import { Path, - pathChild, pathEquals, pathGetBack, pathGetFront, - pathIsEmpty, + pathChild, pathParent, - pathToUrlEncodedString + pathToUrlEncodedString, + pathIsEmpty } from '../core/util/Path'; import { fatal, @@ -246,7 +245,6 @@ function validateLimit(params: QueryParams) { ); } } - /** * @internal */ @@ -465,6 +463,7 @@ export class DataSnapshot { return this._node.val(); } } + /** * * Returns a `Reference` representing the location in the Database @@ -525,7 +524,6 @@ export function refFromURL(db: Database, url: string): DatabaseReference { return ref(db, parsedURL.path.toString()); } - /** * Gets a `Reference` for the location at the specified relative path. * @@ -811,7 +809,9 @@ export function update(ref: DatabaseReference, values: object): Promise { */ export function get(query: Query): Promise { query = getModularInstance(query) as QueryImpl; - return repoGetValue(query._repo, query).then(node => { + const callbackContext = new CallbackContext(() => {}); + const container = new ValueEventRegistration(callbackContext); + return repoGetValue(query._repo, query, container).then(node => { return new DataSnapshot( node, new ReferenceImpl(query._repo, query._path), @@ -819,7 +819,6 @@ export function get(query: Query): Promise { ); }); } - /** * Represents registration for 'value' events. */ @@ -2260,4 +2259,3 @@ export function query( * dependency issues */ syncPointSetReferenceConstructor(ReferenceImpl); -syncTreeSetReferenceConstructor(ReferenceImpl); diff --git a/packages/database/src/core/Repo.ts b/packages/database/src/core/Repo.ts index 3c2a457ba59..612dbd09303 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 + syncTreeAddGetRegistration // TODO: Rename this. } from './SyncTree'; import { Indexable } from './util/misc'; import { @@ -104,7 +106,10 @@ import { eventQueueRaiseEventsAtPath, eventQueueRaiseEventsForChangedPath } from './view/EventQueue'; -import { EventRegistration, QueryContext } from './view/EventRegistration'; +import { + EventRegistration, + QueryContext +} from './view/EventRegistration'; const INTERRUPT_REASON = 'repo_interrupt'; @@ -452,14 +457,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 +479,41 @@ 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 - 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( + /** + * Below we simulate the actions of an `onlyOnce` `onValue()` event where: + * we add an event registration, + * update data at the path, + * trigger the events, + * and then cleanup the SyncTree + */ + const events = syncTreeAddGetRegistration( + repo, + query, + node, + eventRegistration + ); + /* + * 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( // syncTreeAddGetRegistration calls syncTreeAddEventRegistration. We need to clean that up here. repo.serverSyncTree_, query, - null + eventRegistration, + null, + true ); - if (cancels.length > 0) { - repoLog(repo, 'unexpected cancel events in repoGetValue'); - } return node; }, err => { diff --git a/packages/database/src/core/SyncTree.ts b/packages/database/src/core/SyncTree.ts index bffb4aeb104..7313d514b4e 100644 --- a/packages/database/src/core/SyncTree.ts +++ b/packages/database/src/core/SyncTree.ts @@ -18,6 +18,7 @@ import { assert } from '@firebase/util'; import { ReferenceConstructor } from '../api/Reference'; +import { ValueEventRegistration } from '../api/Reference_impl'; import { AckUserWrite } from './operation/AckUserWrite'; import { ListenComplete } from './operation/ListenComplete'; @@ -29,6 +30,7 @@ import { Operation } from './operation/Operation'; import { Overwrite } from './operation/Overwrite'; +import { Repo } from './Repo'; import { ChildrenNode } from './snap/ChildrenNode'; import { Node } from './snap/Node'; import { @@ -56,7 +58,10 @@ import { import { each, errorForServerCode } from './util/util'; import { CacheNode } from './view/CacheNode'; import { Event } from './view/Event'; -import { EventRegistration, QueryContext } from './view/EventRegistration'; +import { + EventRegistration, + QueryContext +} from './view/EventRegistration'; import { View, viewGetCompleteNode, viewGetServerCache } from './view/View'; import { newWriteTree, @@ -324,7 +329,8 @@ export function syncTreeRemoveEventRegistration( syncTree: SyncTree, query: QueryContext, eventRegistration: EventRegistration | null, - cancelError?: Error + cancelError?: Error, + skipListenerDedup = false ): Event[] { // Find the syncPoint first. Then deal with whether or not it has matching listeners const path = query._path; @@ -347,72 +353,76 @@ export function syncTreeRemoveEventRegistration( if (syncPointIsEmpty(maybeSyncPoint)) { syncTree.syncPointTree_ = syncTree.syncPointTree_.remove(path); } + const removed = removedAndEvents.removed; cancelEvents = removedAndEvents.events; - // We may have just removed one of many listeners and can short-circuit this whole process - // We may also not have removed a default listener, in which case all of the descendant listeners should already be - // properly set up. - // - // Since indexed queries can shadow if they don't have other query constraints, check for loadsAllData(), instead of - // queryId === 'default' - const removingDefault = - -1 !== - removed.findIndex(query => { - return query._queryParams.loadsAllData(); - }); - const covered = syncTree.syncPointTree_.findOnPath( - path, - (relativePath, parentSyncPoint) => - syncPointHasCompleteView(parentSyncPoint) - ); - if (removingDefault && !covered) { - const subtree = syncTree.syncPointTree_.subtree(path); - // There are potentially child listeners. Determine what if any listens we need to send before executing the - // removal - if (!subtree.isEmpty()) { - // We need to fold over our subtree and collect the listeners to send - const newViews = syncTreeCollectDistinctViewsForSubTree_(subtree); + if (!skipListenerDedup) { + // We may have just removed one of many listeners and can short-circuit this whole process + // We may also not have removed a default listener, in which case all of the descendant listeners should already be + // properly set up. + // + // Since indexed queries can shadow if they don't have other query constraints, check for loadsAllData(), instead of + // queryId === 'default' + const removingDefault = + -1 !== + removed.findIndex(query => { + return query._queryParams.loadsAllData(); + }); + const covered = syncTree.syncPointTree_.findOnPath( + path, + (relativePath, parentSyncPoint) => + syncPointHasCompleteView(parentSyncPoint) + ); + + if (removingDefault && !covered) { + const subtree = syncTree.syncPointTree_.subtree(path); + // There are potentially child listeners. Determine what if any listens we need to send before executing the + // removal + if (!subtree.isEmpty()) { + // We need to fold over our subtree and collect the listeners to send + const newViews = syncTreeCollectDistinctViewsForSubTree_(subtree); - // Ok, we've collected all the listens we need. Set them up. - for (let i = 0; i < newViews.length; ++i) { - const view = newViews[i], - newQuery = view.query; - const listener = syncTreeCreateListenerForView_(syncTree, view); - syncTree.listenProvider_.startListening( - syncTreeQueryForListening_(newQuery), - syncTreeTagForQuery_(syncTree, newQuery), - listener.hashFn, - listener.onComplete - ); + // Ok, we've collected all the listens we need. Set them up. + for (let i = 0; i < newViews.length; ++i) { + const view = newViews[i], + newQuery = view.query; + const listener = syncTreeCreateListenerForView_(syncTree, view); + syncTree.listenProvider_.startListening( + syncTreeQueryForListening_(newQuery), + syncTreeTagForQuery_(syncTree, newQuery), + listener.hashFn, + listener.onComplete + ); + } + } else { + // There's nothing below us, so nothing we need to start listening on } - } else { - // There's nothing below us, so nothing we need to start listening on } - } - // If we removed anything and we're not covered by a higher up listen, we need to stop listening on this query - // The above block has us covered in terms of making sure we're set up on listens lower in the tree. - // Also, note that if we have a cancelError, it's already been removed at the provider level. - if (!covered && removed.length > 0 && !cancelError) { - // If we removed a default, then we weren't listening on any of the other queries here. Just cancel the one - // default. Otherwise, we need to iterate through and cancel each individual query - if (removingDefault) { - // We don't tag default listeners - const defaultTag: number | null = null; - syncTree.listenProvider_.stopListening( - syncTreeQueryForListening_(query), - defaultTag - ); - } else { - removed.forEach((queryToRemove: QueryContext) => { - const tagToRemove = syncTree.queryToTagMap.get( - syncTreeMakeQueryKey_(queryToRemove) - ); + // If we removed anything and we're not covered by a higher up listen, we need to stop listening on this query + // The above block has us covered in terms of making sure we're set up on listens lower in the tree. + // Also, note that if we have a cancelError, it's already been removed at the provider level. + if (!covered && removed.length > 0 && !cancelError) { + // If we removed a default, then we weren't listening on any of the other queries here. Just cancel the one + // default. Otherwise, we need to iterate through and cancel each individual query + if (removingDefault) { + // We don't tag default listeners + const defaultTag: number | null = null; syncTree.listenProvider_.stopListening( - syncTreeQueryForListening_(queryToRemove), - tagToRemove + syncTreeQueryForListening_(query), + defaultTag ); - }); + } else { + removed.forEach((queryToRemove: QueryContext) => { + const tagToRemove = syncTree.queryToTagMap.get( + syncTreeMakeQueryKey_(queryToRemove) + ); + syncTree.listenProvider_.stopListening( + syncTreeQueryForListening_(queryToRemove), + tagToRemove + ); + }); + } } } // Now, clear all of the tags we're tracking for the removed listens @@ -423,38 +433,6 @@ 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. * @@ -515,14 +493,16 @@ export function syncTreeApplyTaggedQueryMerge( } /** - * 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. + * Add an event callback for the specified query. + * + * @returns Events to raise. */ -export function syncTreeRegisterSyncPoint( +export function syncTreeAddEventRegistration( + syncTree: SyncTree, query: QueryContext, - syncTree: SyncTree -) { + eventRegistration: EventRegistration, + skipSetupListener = false +): Event[] { const path = query._path; let serverCache: Node | null = null; @@ -581,35 +561,6 @@ export function syncTreeRegisterSyncPoint( 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, @@ -618,7 +569,7 @@ export function syncTreeAddEventRegistration( serverCache, serverCacheComplete ); - if (!viewAlreadyExists && !foundAncestorDefaultView) { + if (!viewAlreadyExists && !foundAncestorDefaultView && !skipSetupListener) { const view = syncPointViewForQuery(syncPoint, query); events = events.concat(syncTreeSetupListener_(syncTree, query, view)); } @@ -863,7 +814,7 @@ function syncTreeCreateListenerForView_( /** * Return the tag associated with the given query. */ -function syncTreeTagForQuery_( +export function syncTreeTagForQuery_( syncTree: SyncTree, query: QueryContext ): number | null { @@ -997,6 +948,7 @@ function syncTreeSetupListener_( const path = query._path; const tag = syncTreeTagForQuery_(syncTree, query); const listener = syncTreeCreateListenerForView_(syncTree, view); + const events = syncTree.listenProvider_.startListening( syncTreeQueryForListening_(query), tag, @@ -1049,3 +1001,43 @@ function syncTreeSetupListener_( } return events; } + +/** + * Added to support non-listener data queries (Used by `get()`). + * Simulates `onValue` `onlyOnce` where it adds the event registration, + * and then applies the changes to the tree. + * @param repo - repo reference to the overall Repo. + * @param query - query to be added. + * @param node - Data needed to be added to the tree + * @param eventRegistration - a dummy event registration to add to the SyncTree + */ +export function syncTreeAddGetRegistration( + repo: Repo, + query: QueryContext, + node: Node, + eventRegistration: ValueEventRegistration +) { + syncTreeAddEventRegistration( + repo.serverSyncTree_, + query, + eventRegistration, + true + ); + let events: Event[]; + if (query._queryParams.loadsAllData()) { + events = syncTreeApplyServerOverwrite( + repo.serverSyncTree_, + query._path, + node + ); + } else { + const tag = syncTreeTagForQuery_(repo.serverSyncTree_, query); + events = syncTreeApplyTaggedQueryOverwrite( + repo.serverSyncTree_, + query._path, + node, + tag + ); + } + return events; +} diff --git a/packages/database/test/exp/integration.test.ts b/packages/database/test/exp/integration.test.ts index 3789aac7e46..5b4243b0824 100644 --- a/packages/database/test/exp/integration.test.ts +++ b/packages/database/test/exp/integration.test.ts @@ -41,7 +41,8 @@ import { EventAccumulatorFactory } from '../helpers/EventAccumulator'; import { DATABASE_ADDRESS, DATABASE_URL, - getUniqueRef, + getFreshRepo, + getRWRefs, waitFor } from '../helpers/util'; @@ -114,21 +115,20 @@ describe('Database@exp Tests', () => { // 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 { readerRef } = getRWRefs(getDatabase(defaultApp)); const queries = [ - query(testRef, limitToFirst(1)), - query(testRef, startAt('child1')), - query(testRef, startAt('child2')), - query(testRef, limitToFirst(2)) + query(readerRef, limitToFirst(1)), + query(readerRef, startAt('child1')), + query(readerRef, startAt('child2')), + query(readerRef, 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 => { + const writerPath = getFreshRepo(readerRef._path); + await set(writerPath, initial); + const unsubscribe = onValue(readerRef, snapshot => { ec.addEvent(snapshot.val()); }); await get(q); @@ -140,56 +140,84 @@ describe('Database@exp Tests', () => { ); }); + + + it('calls onValue() listener when get() is called on a parent node', async () => { + // Test that when get() is pending on a parent node, and then onValue is called on a child node, that after the get() comes back, the onValue() listener fires. + const db = getDatabase(defaultApp); + const { readerRef, writerRef } = getRWRefs(db); + await set(writerRef, { + foo1: { + a: 1 + }, + foo2: { + b: 1 + } + }); + let resolved = false; + get(readerRef).then(() => resolved = true); + const childPath = ref(db, readerRef._path + '/foo1'); + expect(resolved).to.be.false; + const ec = EventAccumulatorFactory.waitsForExactCount(1); + onValue(childPath, (snapshot) => { + ec.addEvent(snapshot.val()); + }); + const events = await ec.promise; + expect(events.length).to.eq(1); + const snapshot = events[0]; + expect(snapshot).to.deep.eq({ a: 1}); + }); + it('calls onValue and expects no issues with removing the listener', async () => { const db = getDatabase(defaultApp); - const { ref: testRef } = getUniqueRef(db); + const { readerRef, writerRef } = getRWRefs(db); const initial = [{ name: 'child1' }, { name: 'child2' }]; const ea = EventAccumulatorFactory.waitsForExactCount(1); - await set(testRef, initial); - const unsubscribe = onValue(testRef, snapshot => { + await set(writerRef, initial); + const unsubscribe = onValue(readerRef, snapshot => { ea.addEvent(snapshot.val()); }); - await get(query(testRef)); + await get(query(readerRef)); await waitFor(2000); const update = [{ name: 'child1' }, { name: 'child20' }]; unsubscribe(); - await set(testRef, update); + await set(writerRef, 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 { readerRef, writerRef } = getRWRefs(db); const initial = [{ name: 'child1' }, { name: 'child2' }]; const ea = EventAccumulatorFactory.waitsForExactCount(1); - await set(testRef, initial); - const unsubscribe = onValue(testRef, snapshot => { + await set(writerRef, initial); + const unsubscribe = onValue(readerRef, snapshot => { ea.addEvent(snapshot.val()); expect(snapshot.val()).to.deep.eq(initial); }); - await get(query(testRef)); + await get(query(readerRef)); 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 { readerRef, writerRef } = getRWRefs(db); const initial = { test: { abc: 123 } }; - - await set(testRef, initial); - const unsubscribe = onValue(testRef, snapshot => { + await set(writerRef, initial); + const unsubscribe = onValue(readerRef, snapshot => { ea.addEvent(snapshot.val()); }); - const nestedRef = ref(db, path + '/test'); + const nestedRef = ref(db, readerRef._path + '/test'); const result = await get(query(nestedRef)); await waitFor(2000); const [snap] = await ea.promise; @@ -199,7 +227,7 @@ describe('Database@exp Tests', () => { }); it('calls onValue only once after parent get request', async () => { const db = getDatabase(defaultApp); - const { ref: testRef, path } = getUniqueRef(db); + const { readerRef, writerRef } = getRWRefs(db); const ea = EventAccumulatorFactory.waitsForExactCount(1); const initial = { test: { @@ -207,12 +235,12 @@ describe('Database@exp Tests', () => { } }; - await set(testRef, initial); - const nestedRef = ref(db, path + '/test'); + await set(writerRef, initial); + const nestedRef = ref(db, readerRef._path + '/test'); const unsubscribe = onValue(nestedRef, snapshot => { ea.addEvent(snapshot.val()); }); - const result = await get(query(testRef)); + const result = await get(query(readerRef)); const events = await ea.promise; await waitFor(2000); expect(events.length).to.equal(1); diff --git a/packages/database/test/helpers/util.ts b/packages/database/test/helpers/util.ts index 7b3a9ff0764..46be67c8adc 100644 --- a/packages/database/test/helpers/util.ts +++ b/packages/database/test/helpers/util.ts @@ -15,10 +15,12 @@ * limitations under the License. */ +import { FirebaseApp, initializeApp } from '@firebase/app'; import { uuidv4 } from '@firebase/util'; -import { Database, ref } from '../../src'; +import { Database, getDatabase, ref } from '../../src'; import { ConnectionTarget } from '../../src/api/test_access'; +import { Path } from '../../src/core/util/Path'; // eslint-disable-next-line @typescript-eslint/no-require-imports export const TEST_PROJECT = require('../../../../config/project.json'); @@ -26,6 +28,17 @@ const EMULATOR_PORT = process.env.RTDB_EMULATOR_PORT; const EMULATOR_NAMESPACE = process.env.RTDB_EMULATOR_NAMESPACE; const USE_EMULATOR = !!EMULATOR_PORT; +let freshRepoId = 0; +const activeFreshApps: FirebaseApp[] = []; +export function getFreshRepo(path: Path) { + const app = initializeApp( + { databaseURL: DATABASE_URL }, + 'ISOLATED_REPO_' + freshRepoId++ + ); + activeFreshApps.push(app); + return ref(getDatabase(app), path.toString()); +} + /* * When running against the emulator, the hostname will be "localhost" rather * than ".firebaseio.com", and so we need to append the namespace @@ -83,5 +96,12 @@ export function waitFor(waitTimeInMS: number) { // Creates a unique reference using uuid export function getUniqueRef(db: Database) { const path = uuidv4(); - return { ref: ref(db, path), path }; + return ref(db, path); +} + +// Get separate Reader and Writer Refs to prevent caching. +export function getRWRefs(db: Database) { + const readerRef = getUniqueRef(db); + const writerRef = getFreshRepo(readerRef._path); + return { readerRef, writerRef }; } From 2bf8fe2358e1d6533f37d82478cd4395504bcbae Mon Sep 17 00:00:00 2001 From: Maneesh Tewani Date: Tue, 2 Aug 2022 13:41:32 -0700 Subject: [PATCH 02/19] Ran prettier format --- packages/database/src/core/Repo.ts | 8 +++----- packages/database/src/core/SyncTree.ts | 5 +---- packages/database/test/exp/integration.test.ts | 10 ++++------ 3 files changed, 8 insertions(+), 15 deletions(-) diff --git a/packages/database/src/core/Repo.ts b/packages/database/src/core/Repo.ts index 612dbd09303..4849b28fcaa 100644 --- a/packages/database/src/core/Repo.ts +++ b/packages/database/src/core/Repo.ts @@ -106,10 +106,7 @@ import { eventQueueRaiseEventsAtPath, eventQueueRaiseEventsForChangedPath } from './view/EventQueue'; -import { - EventRegistration, - QueryContext -} from './view/EventRegistration'; +import { EventRegistration, QueryContext } from './view/EventRegistration'; const INTERRUPT_REASON = 'repo_interrupt'; @@ -507,7 +504,8 @@ export function repoGetValue( query._path, events ); - syncTreeRemoveEventRegistration( // syncTreeAddGetRegistration calls syncTreeAddEventRegistration. We need to clean that up here. + syncTreeRemoveEventRegistration( + // syncTreeAddGetRegistration calls syncTreeAddEventRegistration. We need to clean that up here. repo.serverSyncTree_, query, eventRegistration, diff --git a/packages/database/src/core/SyncTree.ts b/packages/database/src/core/SyncTree.ts index 7313d514b4e..d4063db1e7b 100644 --- a/packages/database/src/core/SyncTree.ts +++ b/packages/database/src/core/SyncTree.ts @@ -58,10 +58,7 @@ import { import { each, errorForServerCode } from './util/util'; import { CacheNode } from './view/CacheNode'; import { Event } from './view/Event'; -import { - EventRegistration, - QueryContext -} from './view/EventRegistration'; +import { EventRegistration, QueryContext } from './view/EventRegistration'; import { View, viewGetCompleteNode, viewGetServerCache } from './view/View'; import { newWriteTree, diff --git a/packages/database/test/exp/integration.test.ts b/packages/database/test/exp/integration.test.ts index 5b4243b0824..47977aeb339 100644 --- a/packages/database/test/exp/integration.test.ts +++ b/packages/database/test/exp/integration.test.ts @@ -140,8 +140,6 @@ describe('Database@exp Tests', () => { ); }); - - it('calls onValue() listener when get() is called on a parent node', async () => { // Test that when get() is pending on a parent node, and then onValue is called on a child node, that after the get() comes back, the onValue() listener fires. const db = getDatabase(defaultApp); @@ -155,17 +153,17 @@ describe('Database@exp Tests', () => { } }); let resolved = false; - get(readerRef).then(() => resolved = true); + get(readerRef).then(() => (resolved = true)); const childPath = ref(db, readerRef._path + '/foo1'); expect(resolved).to.be.false; const ec = EventAccumulatorFactory.waitsForExactCount(1); - onValue(childPath, (snapshot) => { + onValue(childPath, snapshot => { ec.addEvent(snapshot.val()); }); const events = await ec.promise; expect(events.length).to.eq(1); const snapshot = events[0]; - expect(snapshot).to.deep.eq({ a: 1}); + expect(snapshot).to.deep.eq({ a: 1 }); }); it('calls onValue and expects no issues with removing the listener', async () => { @@ -207,7 +205,7 @@ describe('Database@exp Tests', () => { it('calls onValue only once after get request with a nested query', async () => { const db = getDatabase(defaultApp); const ea = EventAccumulatorFactory.waitsForExactCount(1); - const { readerRef, writerRef } = getRWRefs(db); + const { readerRef, writerRef } = getRWRefs(db); const initial = { test: { abc: 123 From 070fb85ce658c97b55159d07e49f2f41a666c9be Mon Sep 17 00:00:00 2001 From: Maneesh Tewani Date: Tue, 2 Aug 2022 13:45:28 -0700 Subject: [PATCH 03/19] Create smart-crabs-warn.md --- .changeset/smart-crabs-warn.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/smart-crabs-warn.md diff --git a/.changeset/smart-crabs-warn.md b/.changeset/smart-crabs-warn.md new file mode 100644 index 00000000000..ea9a1e60ba1 --- /dev/null +++ b/.changeset/smart-crabs-warn.md @@ -0,0 +1,5 @@ +--- +"@firebase/database": patch +--- + +Fix filtered get() cache and therefore fixing https://github.com/firebase/firebase-js-sdk/issues/6397 From 5a8fb91ec3086d5bd3d9b92aa2c9a9d6a6063415 Mon Sep 17 00:00:00 2001 From: Maneesh Tewani Date: Tue, 2 Aug 2022 14:00:39 -0700 Subject: [PATCH 04/19] Removed todo --- packages/database/src/core/Repo.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/database/src/core/Repo.ts b/packages/database/src/core/Repo.ts index 4849b28fcaa..49ef37b81da 100644 --- a/packages/database/src/core/Repo.ts +++ b/packages/database/src/core/Repo.ts @@ -63,7 +63,7 @@ import { syncTreeCalcCompleteEventCache, syncTreeGetServerValue, syncTreeRemoveEventRegistration, - syncTreeAddGetRegistration // TODO: Rename this. + syncTreeAddGetRegistration } from './SyncTree'; import { Indexable } from './util/misc'; import { From 3601509d5d3986348f61fd3b246e38d42eeb795c Mon Sep 17 00:00:00 2001 From: Maneesh Tewani Date: Tue, 2 Aug 2022 14:12:32 -0700 Subject: [PATCH 05/19] Fixed removing of setreferenceconstructor --- packages/database/src/api/Reference_impl.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/packages/database/src/api/Reference_impl.ts b/packages/database/src/api/Reference_impl.ts index 69383c2d1e2..697abea2006 100644 --- a/packages/database/src/api/Reference_impl.ts +++ b/packages/database/src/api/Reference_impl.ts @@ -34,6 +34,7 @@ import { PRIORITY_INDEX } from '../core/snap/indexes/PriorityIndex'; import { VALUE_INDEX } from '../core/snap/indexes/ValueIndex'; import { Node } from '../core/snap/Node'; import { syncPointSetReferenceConstructor } from '../core/SyncPoint'; +import { syncTreeSetReferenceConstructor } from '../core/SyncTree'; import { parseRepoInfo } from '../core/util/libs/parser'; import { nextPushId } from '../core/util/NextPushId'; import { @@ -2259,3 +2260,4 @@ export function query( * dependency issues */ syncPointSetReferenceConstructor(ReferenceImpl); +syncTreeSetReferenceConstructor(ReferenceImpl); \ No newline at end of file From b4639d8a94ca13656f42e7c226c5173d82707553 Mon Sep 17 00:00:00 2001 From: Maneesh Tewani Date: Tue, 2 Aug 2022 14:12:49 -0700 Subject: [PATCH 06/19] Fixed formatting --- packages/database/src/api/Reference_impl.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/database/src/api/Reference_impl.ts b/packages/database/src/api/Reference_impl.ts index 697abea2006..97e20ef635d 100644 --- a/packages/database/src/api/Reference_impl.ts +++ b/packages/database/src/api/Reference_impl.ts @@ -2260,4 +2260,4 @@ export function query( * dependency issues */ syncPointSetReferenceConstructor(ReferenceImpl); -syncTreeSetReferenceConstructor(ReferenceImpl); \ No newline at end of file +syncTreeSetReferenceConstructor(ReferenceImpl); From e018abb3c1e4f7c441b8fb23f42508b59a243a30 Mon Sep 17 00:00:00 2001 From: Maneesh Tewani Date: Tue, 2 Aug 2022 14:28:34 -0700 Subject: [PATCH 07/19] Update smart-crabs-warn.md --- .changeset/smart-crabs-warn.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changeset/smart-crabs-warn.md b/.changeset/smart-crabs-warn.md index ea9a1e60ba1..c82b4b39b2f 100644 --- a/.changeset/smart-crabs-warn.md +++ b/.changeset/smart-crabs-warn.md @@ -2,4 +2,4 @@ "@firebase/database": patch --- -Fix filtered get() cache and therefore fixing https://github.com/firebase/firebase-js-sdk/issues/6397 +Fix filtered get() cache and fixed issue where events weren't getting propagated to listeners. From 022bf1120d50dae23fb601fd09bb427966dd4d70 Mon Sep 17 00:00:00 2001 From: Maneesh Tewani Date: Tue, 2 Aug 2022 15:40:31 -0700 Subject: [PATCH 08/19] Updated changeset --- .changeset/smart-crabs-warn.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/.changeset/smart-crabs-warn.md b/.changeset/smart-crabs-warn.md index c82b4b39b2f..604a01f2a89 100644 --- a/.changeset/smart-crabs-warn.md +++ b/.changeset/smart-crabs-warn.md @@ -2,4 +2,5 @@ "@firebase/database": patch --- -Fix filtered get() cache and fixed issue where events weren't getting propagated to listeners. +Fix issue with how get results for filtered queries are added to cache. +Fix issue with events not getting propagated to listeners by get. From 753fed7fd1d3309d9bc59e04620176ba446f2b8a Mon Sep 17 00:00:00 2001 From: Maneesh Tewani Date: Tue, 2 Aug 2022 17:35:57 -0700 Subject: [PATCH 09/19] Updated tests --- packages/database/src/core/Repo.ts | 35 +++- packages/database/src/core/SyncTree.ts | 62 ++----- .../database/test/exp/integration.test.ts | 174 ++++++++++++++++-- 3 files changed, 198 insertions(+), 73 deletions(-) diff --git a/packages/database/src/core/Repo.ts b/packages/database/src/core/Repo.ts index 49ef37b81da..2d76af39a08 100644 --- a/packages/database/src/core/Repo.ts +++ b/packages/database/src/core/Repo.ts @@ -63,7 +63,7 @@ import { syncTreeCalcCompleteEventCache, syncTreeGetServerValue, syncTreeRemoveEventRegistration, - syncTreeAddGetRegistration + syncTreeTagForQuery } from './SyncTree'; import { Indexable } from './util/misc'; import { @@ -478,17 +478,33 @@ export function repoGetValue( ); /** * Below we simulate the actions of an `onlyOnce` `onValue()` event where: - * we add an event registration, - * update data at the path, - * trigger the events, - * and then cleanup the SyncTree + * Add an event registration, + * Update data at the path, + * Raise any events, + * Cleanup the SyncTree */ - const events = syncTreeAddGetRegistration( - repo, + syncTreeAddEventRegistration( + repo.serverSyncTree_, query, - node, - eventRegistration + eventRegistration, + true ); + let events: Event[]; + if (query._queryParams.loadsAllData()) { + events = syncTreeApplyServerOverwrite( + repo.serverSyncTree_, + query._path, + node + ); + } else { + const tag = syncTreeTagForQuery(repo.serverSyncTree_, query); + events = syncTreeApplyTaggedQueryOverwrite( + repo.serverSyncTree_, + query._path, + node, + tag + ); + } /* * 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 @@ -505,7 +521,6 @@ export function repoGetValue( events ); syncTreeRemoveEventRegistration( - // syncTreeAddGetRegistration calls syncTreeAddEventRegistration. We need to clean that up here. repo.serverSyncTree_, query, eventRegistration, diff --git a/packages/database/src/core/SyncTree.ts b/packages/database/src/core/SyncTree.ts index d4063db1e7b..d37c9c0b658 100644 --- a/packages/database/src/core/SyncTree.ts +++ b/packages/database/src/core/SyncTree.ts @@ -18,7 +18,6 @@ import { assert } from '@firebase/util'; import { ReferenceConstructor } from '../api/Reference'; -import { ValueEventRegistration } from '../api/Reference_impl'; import { AckUserWrite } from './operation/AckUserWrite'; import { ListenComplete } from './operation/ListenComplete'; @@ -30,7 +29,6 @@ import { Operation } from './operation/Operation'; import { Overwrite } from './operation/Overwrite'; -import { Repo } from './Repo'; import { ChildrenNode } from './snap/ChildrenNode'; import { Node } from './snap/Node'; import { @@ -355,10 +353,12 @@ export function syncTreeRemoveEventRegistration( cancelEvents = removedAndEvents.events; if (!skipListenerDedup) { - // We may have just removed one of many listeners and can short-circuit this whole process - // We may also not have removed a default listener, in which case all of the descendant listeners should already be - // properly set up. - // + /** + * We may have just removed one of many listeners and can short-circuit this whole process + * We may also not have removed a default listener, in which case all of the descendant listeners should already be + * properly set up. + */ + // Since indexed queries can shadow if they don't have other query constraints, check for loadsAllData(), instead of // queryId === 'default' const removingDefault = @@ -387,7 +387,7 @@ export function syncTreeRemoveEventRegistration( const listener = syncTreeCreateListenerForView_(syncTree, view); syncTree.listenProvider_.startListening( syncTreeQueryForListening_(newQuery), - syncTreeTagForQuery_(syncTree, newQuery), + syncTreeTagForQuery(syncTree, newQuery), listener.hashFn, listener.onComplete ); @@ -779,7 +779,7 @@ function syncTreeCreateListenerForView_( view: View ): { hashFn(): string; onComplete(a: string, b?: unknown): Event[] } { const query = view.query; - const tag = syncTreeTagForQuery_(syncTree, query); + const tag = syncTreeTagForQuery(syncTree, query); return { hashFn: () => { @@ -811,7 +811,7 @@ function syncTreeCreateListenerForView_( /** * Return the tag associated with the given query. */ -export function syncTreeTagForQuery_( +export function syncTreeTagForQuery( syncTree: SyncTree, query: QueryContext ): number | null { @@ -943,7 +943,7 @@ function syncTreeSetupListener_( view: View ): Event[] { const path = query._path; - const tag = syncTreeTagForQuery_(syncTree, query); + const tag = syncTreeTagForQuery(syncTree, query); const listener = syncTreeCreateListenerForView_(syncTree, view); const events = syncTree.listenProvider_.startListening( @@ -992,49 +992,9 @@ function syncTreeSetupListener_( const queryToStop = queriesToStop[i]; syncTree.listenProvider_.stopListening( syncTreeQueryForListening_(queryToStop), - syncTreeTagForQuery_(syncTree, queryToStop) + syncTreeTagForQuery(syncTree, queryToStop) ); } } return events; } - -/** - * Added to support non-listener data queries (Used by `get()`). - * Simulates `onValue` `onlyOnce` where it adds the event registration, - * and then applies the changes to the tree. - * @param repo - repo reference to the overall Repo. - * @param query - query to be added. - * @param node - Data needed to be added to the tree - * @param eventRegistration - a dummy event registration to add to the SyncTree - */ -export function syncTreeAddGetRegistration( - repo: Repo, - query: QueryContext, - node: Node, - eventRegistration: ValueEventRegistration -) { - syncTreeAddEventRegistration( - repo.serverSyncTree_, - query, - eventRegistration, - true - ); - let events: Event[]; - if (query._queryParams.loadsAllData()) { - events = syncTreeApplyServerOverwrite( - repo.serverSyncTree_, - query._path, - node - ); - } else { - const tag = syncTreeTagForQuery_(repo.serverSyncTree_, query); - events = syncTreeApplyTaggedQueryOverwrite( - repo.serverSyncTree_, - query._path, - node, - tag - ); - } - return events; -} diff --git a/packages/database/test/exp/integration.test.ts b/packages/database/test/exp/integration.test.ts index 47977aeb339..974d8f292bd 100644 --- a/packages/database/test/exp/integration.test.ts +++ b/packages/database/test/exp/integration.test.ts @@ -21,13 +21,15 @@ import { expect, use } from 'chai'; import chaiAsPromised from 'chai-as-promised'; import { + child, get, limitToFirst, onValue, query, refFromURL, set, - startAt + startAt, + orderByKey } from '../../src/api/Reference_impl'; import { getDatabase, @@ -129,12 +131,12 @@ describe('Database@exp Tests', () => { const writerPath = getFreshRepo(readerRef._path); await set(writerPath, initial); const unsubscribe = onValue(readerRef, snapshot => { - ec.addEvent(snapshot.val()); + ec.addEvent(snapshot); }); await get(q); await waitFor(2000); const [snap] = await ec.promise; - expect(snap).to.deep.equal(initial); + expect(snap.val()).to.deep.equal(initial); unsubscribe(); }) ); @@ -154,7 +156,7 @@ describe('Database@exp Tests', () => { }); let resolved = false; get(readerRef).then(() => (resolved = true)); - const childPath = ref(db, readerRef._path + '/foo1'); + const childPath = child(readerRef, 'foo1'); expect(resolved).to.be.false; const ec = EventAccumulatorFactory.waitsForExactCount(1); onValue(childPath, snapshot => { @@ -192,13 +194,13 @@ describe('Database@exp Tests', () => { await set(writerRef, initial); const unsubscribe = onValue(readerRef, snapshot => { - ea.addEvent(snapshot.val()); + ea.addEvent(snapshot); expect(snapshot.val()).to.deep.eq(initial); }); await get(query(readerRef)); await waitFor(2000); const [snap] = await ea.promise; - expect(snap).to.deep.equal(initial); + expect(snap.val()).to.deep.equal(initial); unsubscribe(); }); @@ -213,13 +215,13 @@ describe('Database@exp Tests', () => { }; await set(writerRef, initial); const unsubscribe = onValue(readerRef, snapshot => { - ea.addEvent(snapshot.val()); + ea.addEvent(snapshot); }); - const nestedRef = ref(db, readerRef._path + '/test'); + const nestedRef = child(readerRef, 'test'); const result = await get(query(nestedRef)); await waitFor(2000); const [snap] = await ea.promise; - expect(snap).to.deep.equal(initial); + expect(snap.val()).to.deep.equal(initial); expect(result.val()).to.deep.eq(initial.test); unsubscribe(); }); @@ -234,15 +236,15 @@ describe('Database@exp Tests', () => { }; await set(writerRef, initial); - const nestedRef = ref(db, readerRef._path + '/test'); + const nestedRef = child(readerRef, 'test'); const unsubscribe = onValue(nestedRef, snapshot => { - ea.addEvent(snapshot.val()); + ea.addEvent(snapshot); }); const result = await get(query(readerRef)); const events = await ea.promise; await waitFor(2000); expect(events.length).to.equal(1); - expect(events[0]).to.deep.eq(initial.test); + expect(events[0].val()).to.deep.eq(initial.test); expect(result.val()).to.deep.equal(initial); unsubscribe(); }); @@ -318,6 +320,154 @@ describe('Database@exp Tests', () => { expect(resolvedData.val()).to.deep.equal(initial); }); + it('resolves get to serverCache when the database is offline', async () => { + const db = getDatabase(defaultApp); + const { writerRef } = getRWRefs(db); + const expected = { + test: 'abc' + }; + await set(writerRef, expected); + goOffline(db); + const result = await get(writerRef); + expect(result.val()).to.deep.eq(expected); + goOnline(db); + }); + + it('resolves get to serverCache when the database is offline and using a parent-level listener', async () => { + const db = getDatabase(defaultApp); + const { readerRef, writerRef } = getRWRefs(db); + const toWrite = { + test: 'def' + }; + const ec = EventAccumulatorFactory.waitsForExactCount(1); + await set(writerRef, toWrite); + onValue(readerRef, snapshot => { + ec.addEvent(snapshot); + }); + await ec.promise; + goOffline(db); + const result = await get(child(readerRef, 'test')); + expect(result.val()).to.deep.eq(toWrite.test); + goOnline(db); + }); + + // TODO(mtewani): Remove this. This shouldn't work, right? A child level listener doesn't fulfill the full serverCache. + // it.only('resolves get to serverCache when the database is offline and using a child-level listener', async () => { + // const db = getDatabase(defaultApp); + // const { readerRef, writerRef } = getRWRefs(db); + // const toWrite = { + // test: 'ghi' + // }; + // const ec = EventAccumulatorFactory.waitsForExactCount(1); + // await( + // set(writerRef, toWrite) + // ); + // onValue(child(readerRef, 'test'), (snapshot) => { + // ec.addEvent(snapshot); + // }); + // await ec.promise; // TODO: check value of this. + // goOffline(db); + // const result = await get(readerRef); + // expect(result.val()).to.deep.eq(toWrite); + // goOnline(db); + // }); + + it('only fires listener once when calling get with limitTo', async () => { + const db = getDatabase(defaultApp); + const { readerRef, writerRef } = getRWRefs(db); + const ec = EventAccumulatorFactory.waitsForExactCount(1); + const toWrite = { + child1: 'test1', + child2: 'test2' + }; + await set(writerRef, toWrite); + onValue(readerRef, snapshot => { + ec.addEvent(snapshot); + }); + const [snap] = await ec.promise; + expect(snap.val()).to.deep.eq(toWrite); + const q = query(readerRef, limitToFirst(1)); + const snapshot = await get(q); + const expected = { + child1: 'test1' + }; + expect(snapshot.val()).to.deep.eq(expected); + }); + + it('should listen to a disjointed path and get should return the corresponding value', async () => { + const db = getDatabase(defaultApp); + const { readerRef, writerRef } = getRWRefs(db); + const toWrite = { + child1: 'test1', + child2: 'test2', + child3: 'test3' + }; + await set(writerRef, toWrite); + const ec = EventAccumulatorFactory.waitsForExactCount(1); + onValue(readerRef, snapshot => { + ec.addEvent(snapshot); + }); + const events = await ec.promise; + expect(events.length).to.eq(1); + expect(events[0].val()).to.deep.eq(toWrite); + const otherChildrenQuery = query( + readerRef, + orderByKey(), + startAt('child2') + ); + const expected = { + child2: 'test2', + child3: 'test3' + }; + const snapshot = await get(otherChildrenQuery); + expect(snapshot.val()).to.deep.eq(expected); + }); + + it('should test get with limitTo listener only fires once', async () => { + const db = getDatabase(defaultApp); + const { readerRef, writerRef } = getRWRefs(db); + const readQuery = query(readerRef, limitToFirst(1)); + const toWrite = { + child1: 'test1', + child2: 'test2' + }; + await set(writerRef, toWrite); + const ec = EventAccumulatorFactory.waitsForExactCount(1); + onValue(readQuery, snapshot => { + ec.addEvent(snapshot); + }); + const events = await ec.promise; + expect(events.length).to.eq(1); // TODO(mtewani): can get rid of this. + const expected = { child1: 'test1' }; + expect(events[0].val()).to.deep.eq(expected); + const snapshot = await get(readerRef); + expect(snapshot.val()).to.deep.eq(toWrite); + }); + + it('should test start with get with listener only fires once', async () => { + const db = getDatabase(defaultApp); + const { readerRef, writerRef } = getRWRefs(db); + const expected = { + child1: 'test1', + child2: 'test2', + child3: 'test3' + }; + const ec = EventAccumulatorFactory.waitsForExactCount(1); + await set(writerRef, expected); // TODO(mtewani): Can extract this out to something like writeAndValidate() + onValue(readerRef, snapshot => { + ec.addEvent(snapshot); + }); + const [snap] = await ec.promise; + expect(snap.val()).to.deep.eq(expected); + const q = query(readerRef, orderByKey(), startAt('child2')); + const snapshot = await get(q); + const expectedQRes = { + child2: 'test2', + child3: 'test3' + }; + expect(snapshot.val()).to.deep.eq(expectedQRes); + }); + it('Can listen to transaction changes', async () => { // Repro for https://github.com/firebase/firebase-js-sdk/issues/5195 let latestValue = 0; From bb43ba4c3f3b2d53376e8fc66a448567404267f9 Mon Sep 17 00:00:00 2001 From: Maneesh Tewani Date: Tue, 2 Aug 2022 18:22:19 -0700 Subject: [PATCH 10/19] Updated integration tests --- .../database/test/exp/integration.test.ts | 49 +++---------------- packages/database/test/helpers/util.ts | 26 +++++++++- 2 files changed, 31 insertions(+), 44 deletions(-) diff --git a/packages/database/test/exp/integration.test.ts b/packages/database/test/exp/integration.test.ts index 974d8f292bd..460effce8da 100644 --- a/packages/database/test/exp/integration.test.ts +++ b/packages/database/test/exp/integration.test.ts @@ -45,7 +45,8 @@ import { DATABASE_URL, getFreshRepo, getRWRefs, - waitFor + waitFor, + writeAndValidate } from '../helpers/util'; use(chaiAsPromised); @@ -351,27 +352,6 @@ describe('Database@exp Tests', () => { goOnline(db); }); - // TODO(mtewani): Remove this. This shouldn't work, right? A child level listener doesn't fulfill the full serverCache. - // it.only('resolves get to serverCache when the database is offline and using a child-level listener', async () => { - // const db = getDatabase(defaultApp); - // const { readerRef, writerRef } = getRWRefs(db); - // const toWrite = { - // test: 'ghi' - // }; - // const ec = EventAccumulatorFactory.waitsForExactCount(1); - // await( - // set(writerRef, toWrite) - // ); - // onValue(child(readerRef, 'test'), (snapshot) => { - // ec.addEvent(snapshot); - // }); - // await ec.promise; // TODO: check value of this. - // goOffline(db); - // const result = await get(readerRef); - // expect(result.val()).to.deep.eq(toWrite); - // goOnline(db); - // }); - it('only fires listener once when calling get with limitTo', async () => { const db = getDatabase(defaultApp); const { readerRef, writerRef } = getRWRefs(db); @@ -380,12 +360,7 @@ describe('Database@exp Tests', () => { child1: 'test1', child2: 'test2' }; - await set(writerRef, toWrite); - onValue(readerRef, snapshot => { - ec.addEvent(snapshot); - }); - const [snap] = await ec.promise; - expect(snap.val()).to.deep.eq(toWrite); + writeAndValidate(writerRef, readerRef, toWrite, ec); const q = query(readerRef, limitToFirst(1)); const snapshot = await get(q); const expected = { @@ -402,14 +377,8 @@ describe('Database@exp Tests', () => { child2: 'test2', child3: 'test3' }; - await set(writerRef, toWrite); const ec = EventAccumulatorFactory.waitsForExactCount(1); - onValue(readerRef, snapshot => { - ec.addEvent(snapshot); - }); - const events = await ec.promise; - expect(events.length).to.eq(1); - expect(events[0].val()).to.deep.eq(toWrite); + writeAndValidate(writerRef, readerRef, toWrite, ec); const otherChildrenQuery = query( readerRef, orderByKey(), @@ -437,14 +406,13 @@ describe('Database@exp Tests', () => { ec.addEvent(snapshot); }); const events = await ec.promise; - expect(events.length).to.eq(1); // TODO(mtewani): can get rid of this. const expected = { child1: 'test1' }; expect(events[0].val()).to.deep.eq(expected); const snapshot = await get(readerRef); expect(snapshot.val()).to.deep.eq(toWrite); }); - it('should test start with get with listener only fires once', async () => { + it('should test startAt get with listener only fires once', async () => { const db = getDatabase(defaultApp); const { readerRef, writerRef } = getRWRefs(db); const expected = { @@ -453,12 +421,7 @@ describe('Database@exp Tests', () => { child3: 'test3' }; const ec = EventAccumulatorFactory.waitsForExactCount(1); - await set(writerRef, expected); // TODO(mtewani): Can extract this out to something like writeAndValidate() - onValue(readerRef, snapshot => { - ec.addEvent(snapshot); - }); - const [snap] = await ec.promise; - expect(snap.val()).to.deep.eq(expected); + writeAndValidate(writerRef, readerRef, expected, ec); const q = query(readerRef, orderByKey(), startAt('child2')); const snapshot = await get(q); const expectedQRes = { diff --git a/packages/database/test/helpers/util.ts b/packages/database/test/helpers/util.ts index 46be67c8adc..1882e70e38a 100644 --- a/packages/database/test/helpers/util.ts +++ b/packages/database/test/helpers/util.ts @@ -17,10 +17,19 @@ import { FirebaseApp, initializeApp } from '@firebase/app'; import { uuidv4 } from '@firebase/util'; +import { expect } from 'chai'; -import { Database, getDatabase, ref } from '../../src'; +import { + Database, + DatabaseReference, + getDatabase, + onValue, + ref, + set +} from '../../src'; import { ConnectionTarget } from '../../src/api/test_access'; import { Path } from '../../src/core/util/Path'; +import { EventAccumulator } from './EventAccumulator'; // eslint-disable-next-line @typescript-eslint/no-require-imports export const TEST_PROJECT = require('../../../../config/project.json'); @@ -105,3 +114,18 @@ export function getRWRefs(db: Database) { const writerRef = getFreshRepo(readerRef._path); return { readerRef, writerRef }; } + +// Validates that the ref was successfully written to. +export async function writeAndValidate( + writerRef: DatabaseReference, + readerRef: DatabaseReference, + toWrite: unknown, + ec: EventAccumulator +) { + await set(writerRef, toWrite); + onValue(readerRef, snapshot => { + ec.addEvent(snapshot); + }); + const [snap] = await ec.promise; + expect(snap.val()).to.deep.eq(toWrite); +} From 754f63c65710b313e3c9675c92de213e0da55ef2 Mon Sep 17 00:00:00 2001 From: Maneesh Tewani Date: Tue, 2 Aug 2022 18:26:55 -0700 Subject: [PATCH 11/19] Fixed linting issues --- packages/database/test/helpers/util.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/database/test/helpers/util.ts b/packages/database/test/helpers/util.ts index 1882e70e38a..e5242be3e93 100644 --- a/packages/database/test/helpers/util.ts +++ b/packages/database/test/helpers/util.ts @@ -29,6 +29,7 @@ import { } from '../../src'; import { ConnectionTarget } from '../../src/api/test_access'; import { Path } from '../../src/core/util/Path'; + import { EventAccumulator } from './EventAccumulator'; // eslint-disable-next-line @typescript-eslint/no-require-imports From 2de0a4813069eb8fef0ec9187590f673dd48fe4d Mon Sep 17 00:00:00 2001 From: Maneesh Tewani Date: Wed, 3 Aug 2022 11:24:13 -0700 Subject: [PATCH 12/19] Updated to fix servertimeoffset and created smoketest --- packages/database-compat/test/info.test.ts | 2 +- .../database/test/exp/integration.test.ts | 23 +++++++++++++++---- 2 files changed, 20 insertions(+), 5 deletions(-) diff --git a/packages/database-compat/test/info.test.ts b/packages/database-compat/test/info.test.ts index 45afd66f90e..b02253bd25b 100644 --- a/packages/database-compat/test/info.test.ts +++ b/packages/database-compat/test/info.test.ts @@ -131,7 +131,7 @@ describe('.info Tests', function () { await ea.promise; expect(typeof offsets[0]).to.equal('number'); - expect(offsets[0]).not.to.be.greaterThan(0); + expect(offsets[0]).to.be.greaterThanOrEqual(0); // There is no way for us to guarantee that the latency will be exactly 0. // Make sure push still works ref.push(); diff --git a/packages/database/test/exp/integration.test.ts b/packages/database/test/exp/integration.test.ts index 460effce8da..337deb755e4 100644 --- a/packages/database/test/exp/integration.test.ts +++ b/packages/database/test/exp/integration.test.ts @@ -143,7 +143,20 @@ describe('Database@exp Tests', () => { ); }); - it('calls onValue() listener when get() is called on a parent node', async () => { + async function waitUntil(cb: () => boolean, maxRetries = 5) { + let count = 1; + return new Promise((resolve, reject) => { + if(cb()) { + resolve(true); + } else { + if(count++ === maxRetries) { + reject('waited too many times for conditional to be true'); + } + } + }); + } + + it('[smoketest] - calls onValue() listener when get() is called on a parent node', async () => { // Test that when get() is pending on a parent node, and then onValue is called on a child node, that after the get() comes back, the onValue() listener fires. const db = getDatabase(defaultApp); const { readerRef, writerRef } = getRWRefs(db); @@ -155,10 +168,12 @@ describe('Database@exp Tests', () => { b: 1 } }); - let resolved = false; - get(readerRef).then(() => (resolved = true)); + await waitUntil(() => { // Because this is a test reliant on network latency, it can be difficult to reproduce. There are situations when get() resolves immediately, and the above behavior is not observed. + let resolved = false; + get(readerRef).then(() => (resolved = true)); + return !resolved; + }); const childPath = child(readerRef, 'foo1'); - expect(resolved).to.be.false; const ec = EventAccumulatorFactory.waitsForExactCount(1); onValue(childPath, snapshot => { ec.addEvent(snapshot.val()); From da7023885dc9f14b5981abe7be5bb3f1fb5044b4 Mon Sep 17 00:00:00 2001 From: Maneesh Tewani Date: Wed, 3 Aug 2022 11:24:28 -0700 Subject: [PATCH 13/19] Ran prettier --- packages/database/test/exp/integration.test.ts | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/packages/database/test/exp/integration.test.ts b/packages/database/test/exp/integration.test.ts index 337deb755e4..856622e4255 100644 --- a/packages/database/test/exp/integration.test.ts +++ b/packages/database/test/exp/integration.test.ts @@ -146,10 +146,10 @@ describe('Database@exp Tests', () => { async function waitUntil(cb: () => boolean, maxRetries = 5) { let count = 1; return new Promise((resolve, reject) => { - if(cb()) { + if (cb()) { resolve(true); } else { - if(count++ === maxRetries) { + if (count++ === maxRetries) { reject('waited too many times for conditional to be true'); } } @@ -168,7 +168,8 @@ describe('Database@exp Tests', () => { b: 1 } }); - await waitUntil(() => { // Because this is a test reliant on network latency, it can be difficult to reproduce. There are situations when get() resolves immediately, and the above behavior is not observed. + await waitUntil(() => { + // Because this is a test reliant on network latency, it can be difficult to reproduce. There are situations when get() resolves immediately, and the above behavior is not observed. let resolved = false; get(readerRef).then(() => (resolved = true)); return !resolved; From 186977d94f1c8742f363df8b95d7f3720fb9f1b0 Mon Sep 17 00:00:00 2001 From: Maneesh Tewani Date: Wed, 3 Aug 2022 11:59:38 -0700 Subject: [PATCH 14/19] Updated info test and moved util to helper --- packages/database-compat/test/info.test.ts | 3 +-- .../database/test/exp/integration.test.ts | 20 ++++--------------- packages/database/test/helpers/util.ts | 14 +++++++++++++ 3 files changed, 19 insertions(+), 18 deletions(-) diff --git a/packages/database-compat/test/info.test.ts b/packages/database-compat/test/info.test.ts index b02253bd25b..6832488bbf3 100644 --- a/packages/database-compat/test/info.test.ts +++ b/packages/database-compat/test/info.test.ts @@ -130,8 +130,7 @@ describe('.info Tests', function () { await ea.promise; - expect(typeof offsets[0]).to.equal('number'); - expect(offsets[0]).to.be.greaterThanOrEqual(0); // There is no way for us to guarantee that the latency will be exactly 0. + expect(offsets[0]).to.be.a('number'); // Make sure push still works ref.push(); diff --git a/packages/database/test/exp/integration.test.ts b/packages/database/test/exp/integration.test.ts index 856622e4255..a7532bc6110 100644 --- a/packages/database/test/exp/integration.test.ts +++ b/packages/database/test/exp/integration.test.ts @@ -46,6 +46,7 @@ import { getFreshRepo, getRWRefs, waitFor, + waitUntil, writeAndValidate } from '../helpers/util'; @@ -143,19 +144,6 @@ describe('Database@exp Tests', () => { ); }); - async function waitUntil(cb: () => boolean, maxRetries = 5) { - let count = 1; - return new Promise((resolve, reject) => { - if (cb()) { - resolve(true); - } else { - if (count++ === maxRetries) { - reject('waited too many times for conditional to be true'); - } - } - }); - } - it('[smoketest] - calls onValue() listener when get() is called on a parent node', async () => { // Test that when get() is pending on a parent node, and then onValue is called on a child node, that after the get() comes back, the onValue() listener fires. const db = getDatabase(defaultApp); @@ -170,9 +158,9 @@ describe('Database@exp Tests', () => { }); await waitUntil(() => { // Because this is a test reliant on network latency, it can be difficult to reproduce. There are situations when get() resolves immediately, and the above behavior is not observed. - let resolved = false; - get(readerRef).then(() => (resolved = true)); - return !resolved; + let pending = false; + get(readerRef).then(() => (pending = true)); + return !pending; }); const childPath = child(readerRef, 'foo1'); const ec = EventAccumulatorFactory.waitsForExactCount(1); diff --git a/packages/database/test/helpers/util.ts b/packages/database/test/helpers/util.ts index e5242be3e93..91c627c9a14 100644 --- a/packages/database/test/helpers/util.ts +++ b/packages/database/test/helpers/util.ts @@ -130,3 +130,17 @@ export async function writeAndValidate( const [snap] = await ec.promise; expect(snap.val()).to.deep.eq(toWrite); } + +// Waits until callback function returns true +export async function waitUntil(cb: () => boolean, maxRetries = 5) { + let count = 1; + return new Promise((resolve, reject) => { + if (cb()) { + resolve(true); + } else { + if (count++ === maxRetries) { + reject('waited too many times for conditional to be true'); + } + } + }); +} From 49e8b76896ead00c1e40c2a067b631ca0e77ca64 Mon Sep 17 00:00:00 2001 From: Maneesh Tewani Date: Wed, 3 Aug 2022 12:06:47 -0700 Subject: [PATCH 15/19] Fixed test --- packages/database/test/exp/integration.test.ts | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/packages/database/test/exp/integration.test.ts b/packages/database/test/exp/integration.test.ts index a7532bc6110..80c380493d9 100644 --- a/packages/database/test/exp/integration.test.ts +++ b/packages/database/test/exp/integration.test.ts @@ -381,8 +381,13 @@ describe('Database@exp Tests', () => { child2: 'test2', child3: 'test3' }; - const ec = EventAccumulatorFactory.waitsForExactCount(1); + let ec = EventAccumulatorFactory.waitsForExactCount(1); writeAndValidate(writerRef, readerRef, toWrite, ec); + ec = EventAccumulatorFactory.waitsForExactCount(1); + const child1Ref = child(readerRef, 'child1'); + onValue(child1Ref, snapshot => { + ec.addEvent(snapshot); + }); const otherChildrenQuery = query( readerRef, orderByKey(), @@ -392,6 +397,8 @@ describe('Database@exp Tests', () => { child2: 'test2', child3: 'test3' }; + const [child1Snapshot] = await ec.promise; + expect(child1Snapshot.val()).to.eq('test1'); const snapshot = await get(otherChildrenQuery); expect(snapshot.val()).to.deep.eq(expected); }); From 64ce0b6cb342deff6efe47dc5332494ad04c6cce Mon Sep 17 00:00:00 2001 From: Maneesh Tewani Date: Wed, 3 Aug 2022 12:49:55 -0700 Subject: [PATCH 16/19] Made sure to await set calls --- packages/database/test/exp/integration.test.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/database/test/exp/integration.test.ts b/packages/database/test/exp/integration.test.ts index 80c380493d9..e1d79552a33 100644 --- a/packages/database/test/exp/integration.test.ts +++ b/packages/database/test/exp/integration.test.ts @@ -364,7 +364,7 @@ describe('Database@exp Tests', () => { child1: 'test1', child2: 'test2' }; - writeAndValidate(writerRef, readerRef, toWrite, ec); + await writeAndValidate(writerRef, readerRef, toWrite, ec); const q = query(readerRef, limitToFirst(1)); const snapshot = await get(q); const expected = { @@ -382,7 +382,7 @@ describe('Database@exp Tests', () => { child3: 'test3' }; let ec = EventAccumulatorFactory.waitsForExactCount(1); - writeAndValidate(writerRef, readerRef, toWrite, ec); + await writeAndValidate(writerRef, readerRef, toWrite, ec); ec = EventAccumulatorFactory.waitsForExactCount(1); const child1Ref = child(readerRef, 'child1'); onValue(child1Ref, snapshot => { @@ -423,7 +423,7 @@ describe('Database@exp Tests', () => { expect(snapshot.val()).to.deep.eq(toWrite); }); - it('should test startAt get with listener only fires once', async () => { + it.only('should test startAt get with listener only fires once', async () => { const db = getDatabase(defaultApp); const { readerRef, writerRef } = getRWRefs(db); const expected = { @@ -432,7 +432,7 @@ describe('Database@exp Tests', () => { child3: 'test3' }; const ec = EventAccumulatorFactory.waitsForExactCount(1); - writeAndValidate(writerRef, readerRef, expected, ec); + await writeAndValidate(writerRef, readerRef, expected, ec); const q = query(readerRef, orderByKey(), startAt('child2')); const snapshot = await get(q); const expectedQRes = { From 910d5ae5e9564a4ab63462b9dee9fe8489355598 Mon Sep 17 00:00:00 2001 From: Maneesh Tewani Date: Wed, 3 Aug 2022 12:50:17 -0700 Subject: [PATCH 17/19] 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 e1d79552a33..aa86a3fd80a 100644 --- a/packages/database/test/exp/integration.test.ts +++ b/packages/database/test/exp/integration.test.ts @@ -423,7 +423,7 @@ describe('Database@exp Tests', () => { expect(snapshot.val()).to.deep.eq(toWrite); }); - it.only('should test startAt get with listener only fires once', async () => { + it('should test startAt get with listener only fires once', async () => { const db = getDatabase(defaultApp); const { readerRef, writerRef } = getRWRefs(db); const expected = { From b94c8a85f827f29225f18b141d7c3687fcb3345c Mon Sep 17 00:00:00 2001 From: Maneesh Tewani Date: Wed, 3 Aug 2022 13:08:53 -0700 Subject: [PATCH 18/19] Removed unneeded test --- .../database/test/exp/integration.test.ts | 20 ------------------- 1 file changed, 20 deletions(-) diff --git a/packages/database/test/exp/integration.test.ts b/packages/database/test/exp/integration.test.ts index aa86a3fd80a..1e0072cf1ff 100644 --- a/packages/database/test/exp/integration.test.ts +++ b/packages/database/test/exp/integration.test.ts @@ -403,26 +403,6 @@ describe('Database@exp Tests', () => { expect(snapshot.val()).to.deep.eq(expected); }); - it('should test get with limitTo listener only fires once', async () => { - const db = getDatabase(defaultApp); - const { readerRef, writerRef } = getRWRefs(db); - const readQuery = query(readerRef, limitToFirst(1)); - const toWrite = { - child1: 'test1', - child2: 'test2' - }; - await set(writerRef, toWrite); - const ec = EventAccumulatorFactory.waitsForExactCount(1); - onValue(readQuery, snapshot => { - ec.addEvent(snapshot); - }); - const events = await ec.promise; - const expected = { child1: 'test1' }; - expect(events[0].val()).to.deep.eq(expected); - const snapshot = await get(readerRef); - expect(snapshot.val()).to.deep.eq(toWrite); - }); - it('should test startAt get with listener only fires once', async () => { const db = getDatabase(defaultApp); const { readerRef, writerRef } = getRWRefs(db); From b4c6eb6bc21883ec475d0d4af3acd742df41e8e1 Mon Sep 17 00:00:00 2001 From: Maneesh Tewani Date: Thu, 11 Aug 2022 16:22:09 -0700 Subject: [PATCH 19/19] Addressed comments --- packages/database/src/core/SyncTree.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/packages/database/src/core/SyncTree.ts b/packages/database/src/core/SyncTree.ts index d37c9c0b658..93387a2f27f 100644 --- a/packages/database/src/core/SyncTree.ts +++ b/packages/database/src/core/SyncTree.ts @@ -318,6 +318,8 @@ export function syncTreeApplyTaggedListenComplete( * * @param eventRegistration - If null, all callbacks are removed. * @param cancelError - If a cancelError is provided, appropriate cancel events will be returned. + * @param skipListenerDedup - When performing a `get()`, we don't add any new listeners, so no + * deduping needs to take place. This flag allows toggling of that behavior * @returns Cancel events, if cancelError was provided. */ export function syncTreeRemoveEventRegistration( @@ -392,9 +394,8 @@ export function syncTreeRemoveEventRegistration( listener.onComplete ); } - } else { - // There's nothing below us, so nothing we need to start listening on } + // Otherwise there's nothing below us, so nothing we need to start listening on } // If we removed anything and we're not covered by a higher up listen, we need to stop listening on this query // The above block has us covered in terms of making sure we're set up on listens lower in the tree.