Fix for repoGet request (#6273)

Fixed issue where `get()` saved results incorrectly for non-default queries.

Co-authored-by: Jan Wyszynski <[email protected]>

Author: maneesht

.changeset/rotten-tables-brush.md

@@ -0,0 +1,5 @@
+---
+"@firebase/database": patch
+---
+
+Fixed issue where `get()` saved results incorrectly for non-default queries.

.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"

packages/database/package.json

@@ -56,7 +56,8 @@
     "@firebase/app": "0.7.26",
     "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",

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,

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<Node> {
   }
   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);

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,

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_;