Fix filtered get() cache (#6497)

Fix issue where `get()` incorrectly updated non-default query cache.

Also fixes #6397

Author: maneesht

.changeset/smart-crabs-warn.md

@@ -0,0 +1,6 @@
+---
+"@firebase/database": patch
+---
+
+Fix issue with how get results for filtered queries are added to cache.
+Fix issue with events not getting propagated to listeners by get.

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]).not.to.be.greaterThan(0);
+    expect(offsets[0]).to.be.a('number');
 
     // Make sure push still works
     ref.push();

packages/database/src/api/Reference_impl.ts

@@ -39,13 +39,13 @@ 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 +246,6 @@ function validateLimit(params: QueryParams) {
     );
   }
 }
-
 /**
  * @internal
  */
@@ -465,6 +464,7 @@ export class DataSnapshot {
     return this._node.val();
   }
 }
+
 /**
  *
  * Returns a `Reference` representing the location in the Database
@@ -525,7 +525,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,15 +810,16 @@ export function update(ref: DatabaseReference, values: object): Promise<void> {
  */
 export function get(query: Query): Promise<DataSnapshot> {
   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),
       query._queryParams.getIndex()
     );
   });
 }
-
 /**
  * Represents registration for 'value' events.
  */

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<Node> {
+export function repoGetValue(
+  repo: Repo,
+  query: QueryContext,
+  eventRegistration: ValueEventRegistration
+): Promise<Node> {
   // 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<Node> {
       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 => {