Fixed faulty transaction bug (#6508)

Fixed issue where if a listener on a filtered query with a server-side index is created with a listener at a non-intersecting path would cause incorrect results to be reported.

Author: maneesht

.changeset/warm-pillows-know.md

@@ -0,0 +1,6 @@
+---
+"@firebase/database-compat": patch
+"@firebase/database": patch
+---
+
+Fixed faulty transaction bug causing filtered index queries to override default queries.

README.md

@@ -126,7 +126,8 @@ command, as follows:
 
 
 ```bash
-# Select the Firebase project via the text-based UI.
+# Select the Firebase project via the text-based UI. This will run tools/config.js
+# and deploy from config/ to your Firebase project.
 $ yarn test:setup
 
 # Specify the Firebase project via the command-line arguments.

config/database.rules.json

@@ -1,6 +1,9 @@
 {
   "rules": {
     ".read": true,
-    ".write": true
+    ".write": true,
+    "testing": {
+      ".indexOn": "testIndex"
+    }
   }
 }

packages/database-compat/test/helpers/util.ts

@@ -146,7 +146,7 @@ export function getFreshRepo(path: Path) {
     'ISOLATED_REPO_' + freshRepoId++
   );
   activeFreshApps.push(app);
-  return (app as any).database().ref(path.toString());
+  return (app as any).database().ref(path.toString()); // TODO(mtewani): Remove explicit any
 }
 
 export function getFreshRepoFromReference(ref) {

packages/database/src/core/view/ViewProcessor.ts

@@ -606,7 +606,7 @@ function viewProcessorApplyServerMerge(
   // and event snap.  I'm not sure if this will result in edge cases when a child is in one but
   // not the other.
   let curViewCache = viewCache;
-  let viewMergeTree;
+  let viewMergeTree: ImmutableTree<Node>;
   if (pathIsEmpty(path)) {
     viewMergeTree = changedChildren;
   } else {
@@ -641,7 +641,7 @@ function viewProcessorApplyServerMerge(
   viewMergeTree.children.inorderTraversal((childKey, childMergeTree) => {
     const isUnknownDeepMerge =
       !viewCache.serverCache.isCompleteForChild(childKey) &&
-      childMergeTree.value === undefined;
+      childMergeTree.value === null;
     if (!serverNode.hasChild(childKey) && !isUnknownDeepMerge) {
       const serverChild = viewCache.serverCache
         .getNode()

packages/database/test/exp/integration.test.ts

@@ -24,11 +24,14 @@ import {
   child,
   get,
   limitToFirst,
+  onChildAdded,
   onValue,
+  orderByChild,
   query,
   refFromURL,
   set,
   startAt,
+  update,
   orderByKey
 } from '../../src/api/Reference_impl';
 import {
@@ -117,6 +120,54 @@ describe('Database@exp Tests', () => {
     unsubscribe();
   });
 
+  it('can properly handle unknown deep merges', async () => {
+    // Note: This test requires `testIndex` to be added as an index.
+    // Please run `yarn test:setup` to ensure that this gets added.
+    const database = getDatabase(defaultApp);
+    const root = ref(database, 'testing');
+    await set(root, {});
+
+    const q = query(root, orderByChild('testIndex'), limitToFirst(2));
+
+    const i1 = child(root, 'i1');
+    await set(root, {
+      i1: {
+        testIndex: 3,
+        timestamp: Date.now(),
+        action: 'test'
+      },
+      i2: {
+        testIndex: 1,
+        timestamp: Date.now(),
+        action: 'test'
+      },
+      i3: {
+        testIndex: 2,
+        timestamp: Date.now(),
+        action: 'test'
+      }
+    });
+    const ec = EventAccumulatorFactory.waitsForExactCount(2);
+    const onChildAddedCb = onChildAdded(q, snap => {
+      ec.addEvent(snap);
+    });
+    const onValueCb = onValue(i1, () => {
+      //no-op
+    });
+    await update(i1, {
+      timestamp: `${Date.now()}|1`
+    });
+    const results = await ec.promise;
+    results.forEach(result => {
+      const value = result.val();
+      expect(value).to.haveOwnProperty('timestamp');
+      expect(value).to.haveOwnProperty('action');
+      expect(value).to.haveOwnProperty('testIndex');
+    });
+    onChildAddedCb();
+    onValueCb();
+  });
+
   // 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 { readerRef } = getRWRefs(getDatabase(defaultApp));

packages/database/test/helpers/EventAccumulator.ts

@@ -16,6 +16,7 @@
  */
 
 export const EventAccumulatorFactory = {
+  // TODO: Convert to use generics to take the most advantage of types.
   waitsForCount: maxCount => {
     // Note: This should be used sparingly as it can result in more events being raised than expected
     let count = 0;

scripts/emulator-testing/emulators/database-emulator.ts

@@ -19,6 +19,8 @@ import * as request from 'request';
 
 import { Emulator } from './emulator';
 
+import * as rulesJSON from '../../../config/database.rules.json';
+
 export class DatabaseEmulator extends Emulator {
   namespace: string;
 
@@ -35,13 +37,14 @@ export class DatabaseEmulator extends Emulator {
   }
 
   setPublicRules(): Promise<number> {
-    console.log('Setting rule {".read": true, ".write": true} to emulator ...');
+    const jsonRules = JSON.stringify(rulesJSON);
+    console.log(`Setting rule ${jsonRules} to emulator ...`);
     return new Promise<number>((resolve, reject) => {
       request.put(
         {
           uri: `http://localhost:${this.port}/.settings/rules.json?ns=${this.namespace}`,
           headers: { Authorization: 'Bearer owner' },
-          body: '{ "rules": { ".read": true, ".write": true } }'
+          body: jsonRules
         },
         (error, response, body) => {
           if (error) reject(error);