Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Queries to the same database path get mixed up and return incorrect results depending on order of execution #6038

Closed
stevenpal opened this issue Feb 28, 2022 · 5 comments

Comments

@stevenpal
Copy link

stevenpal commented Feb 28, 2022

[REQUIRED] Describe your environment

  • Operating System version: MacOS 10.15.7
  • Browser version: React Native (0.64.3)
  • Firebase SDK version: 9.6.5
  • Firebase Product: database

[REQUIRED] Describe the problem

Queries to the same realtime database path get mixed up and return incorrect results depending on the order in which you define queries, execute get requests, and trigger onChild listener callbacks. In developing an app that makes both get requests and that have onChild callbacks triggered on the same realtime database path, I found that depending on the order in which database references and queries are defined and executed, you get unexpected behavior and incorrect results.

It appears that if a database query is defined while another query or reference is being executed (either via a get or via an onChild listener returning results), the newly defined query somehow overwrites other references or queries that were defined, causing both the currently executing query or onChild listener to return incorrect results and subsequent queries to the same path to return incorrect (or no) results.

This means as a developer you have to ensure that only one query can ever be executing at any given time against a database path. This is extremely problematic or near impossible to ensure in an app that involves collaboration between multiple users across different instances/apps.

This issue was observed on v9. I haven't tried reverting to v8 to see if the issue exists there as well.

Steps to reproduce:

Step 1. Create realtime database with sample data and rules

Create a database (or use an existing one). At the database path /todos, import the following sample data.

{
  "-MnRQNp76jfDkd25KlS5" : {
    "isCompleted" : false,
    "text" : "Todo 1"
  },
  "-MvPbrl8o_Afb0MdeqJb" : {
    "isCompleted" : false,
    "text" : "Todo 2"
  },
  "-MvPc1CNYHYsHOGKFGqR" : {
    "completedAt" : 1644345435041,
    "isCompleted" : true,
    "text" : "Todo 3 - completed"
  },
  "-MvPdfdB5nyYPo1rczp9" : {
    "isCompleted" : false,
    "text" : "Todo 4"
  },
  "-MvPdidjFC4oXqHsvrRG" : {
    "completedAt" : 1644345879235,
    "isCompleted" : true,
    "text" : "Todo 5 - completed"
  },
  "-MvQ06v44sbWMLMHBQT4" : {
    "isCompleted" : false,
    "text" : "Todo 6"
  }
}

Create rules that allow reads of the path /todos and an index on the property isCompleted:

{
  "rules": {
    "todos": {
      ".read": true,
      ".write": false,
      ".indexOn": ["isCompleted"]
    }
  }
}

Step 2. Create test that performs both a get and onChild subscription

The following code creates one database reference and two queries. The database reference is used for the onChild subscriptions and the queries are used to perform gets with two different filter criteria (i.e. one that gets open todos and another that gets closed todos).

Create a test that calls the createErrorAsync function.

import { initializeApp } from 'firebase/app';
import {
  getDatabase,
  ref,
  onChildAdded,
  onChildChanged,
  onChildRemoved,
  get,
  query,
  orderByChild,
  equalTo,
} from 'firebase/database';

const FIREBASE_CONFIG = {
  // firebase config goes here ...
};

/*
  If you create multiple references or queries before executing them with a
  get or listener, they seem to be combined somehow, which is NOT as expected.
*/
export const createErrorAsync = async () => {
  // connect
  const app = await initializeApp(FIREBASE_CONFIG);

  // database path
  const dbRefPath = 'todos';

  /*
    Here we'll create three separate queries / database references that will
    be used to fetch or subscribe to the same path in the database.
  */

  // create reference/query 1 --> used to get open todos
  const dbRef1 = query(
    ref(getDatabase(app), dbRefPath),
    orderByChild('isCompleted'),
    equalTo(false),
  );

  // create reference/query 2 --> used to listen for changes
  const dbRef2 = ref(getDatabase(app), dbRefPath);

  // create reference/query 3 --> used to get closed todos
  const dbRef3 = query(
    ref(getDatabase(app), dbRefPath),
    orderByChild('isCompleted'),
    equalTo(true),
  );

  // create listeners on reference 2
  onChildAdded(dbRef2, (snap) => {
    console.log('dbRef2 - ADDED -', snap.key);
  });

  onChildChanged(dbRef2, (snap) => {
    console.log('dbRef2 - CHANGED -', snap.key);
  });

  onChildRemoved(dbRef2, (snap) => {
    console.log('dbRef2 - REMOVED -', snap.key);
  });

  // execute get on reference 1
  const snap1 = await get(dbRef1);

  // observe data is returned
  console.log('dbRef1', snap1.val());

  // execute get on reference 3
  const snap3 = await get(dbRef3);

  // observe data is returned
  console.log('dbRef3', snap3.val());
}

Expected Results:

After running, I would expect the following output when the createErrorAsync function is called. The onChild callbacks should return all six entities, the first get should return four entities, and the second get should return two entities.

dbRef1 Object {
  "-MnRQNp76jfDkd25KlS5": Object {
    "isCompleted": false,
    "text": "Todo 1",
  },
  "-MvPbrl8o_Afb0MdeqJb": Object {
    "isCompleted": false,
    "text": "Todo 2",
  },
  "-MvPdfdB5nyYPo1rczp9": Object {
    "isCompleted": false,
    "text": "Todo 4",
  },
  "-MvQ06v44sbWMLMHBQT4": Object {
    "isCompleted": false,
    "text": "Todo 6",
  },
}
dbRef2 - ADDED - -MnRQNp76jfDkd25KlS5
dbRef2 - ADDED - -MvPbrl8o_Afb0MdeqJb
dbRef2 - ADDED - -MvPc1CNYHYsHOGKFGqR
dbRef2 - ADDED - -MvPdfdB5nyYPo1rczp9
dbRef2 - ADDED - -MvPdidjFC4oXqHsvrRG
dbRef2 - ADDED - -MvQ06v44sbWMLMHBQT4
dbRef3 Object {
  "-MvPc1CNYHYsHOGKFGqR": Object {
    "completedAt": 1644345435041,
    "isCompleted": true,
    "text": "Todo 3 - completed",
  },
  "-MvPdidjFC4oXqHsvrRG": Object {
    "completedAt": 1644345879235,
    "isCompleted": true,
    "text": "Todo 5 - completed",
  },
}

Actual Results:

What is actually returned is that the onChild listeners trigger six onChildAdded callbacks followed by two onChildRemoved callbacks (when nothing was removed). Then the four entities are returned by the first get request. And then, the final get returns no results at all.

dbRef2 - ADDED - -MnRQNp76jfDkd25KlS5
dbRef2 - ADDED - -MvPbrl8o_Afb0MdeqJb
dbRef2 - ADDED - -MvPc1CNYHYsHOGKFGqR
dbRef2 - ADDED - -MvPdfdB5nyYPo1rczp9
dbRef2 - ADDED - -MvPdidjFC4oXqHsvrRG
dbRef2 - ADDED - -MvQ06v44sbWMLMHBQT4
dbRef2 - REMOVED - -MvPc1CNYHYsHOGKFGqR
dbRef2 - REMOVED - -MvPdidjFC4oXqHsvrRG
dbRef1 Object {
  "-MnRQNp76jfDkd25KlS5": Object {
    "isCompleted": false,
    "text": "Todo 1",
  },
  "-MvPbrl8o_Afb0MdeqJb": Object {
    "isCompleted": false,
    "text": "Todo 2",
  },
  "-MvPdfdB5nyYPo1rczp9": Object {
    "isCompleted": false,
    "text": "Todo 4",
  },
  "-MvQ06v44sbWMLMHBQT4": Object {
    "isCompleted": false,
    "text": "Todo 6",
  },
}
dbRef3 null

Partial Workaround:

Through a lot of trial-and-error, I discovered a partial workaround for the issue. You basically need to try (as best you can) to ensure the queries are never executed at the same time. As I mentioned in the earlier description, it seems pretty much impossible to ensure this in a multi-user app where there can be database mutations made by anyone that can trigger onChild callbacks at any time.

If you modify the above code sample to define the queries / database references in a different order and implement a timeout/delay to ensure the onChild callbacks have finished executing, you seem to get the results you expect. I have no idea how robust this workaround is though and would much rather have someone from the Firebase team comment.

Here's the modified code for reference:

import { initializeApp } from 'firebase/app';
import {
  getDatabase,
  ref,
  onChildAdded,
  onChildChanged,
  onChildRemoved,
  get,
  query,
  orderByChild,
  equalTo,
} from 'firebase/database';

const FIREBASE_CONFIG = {
  // firebase config goes here
};

/*
  When you create a database reference or query and then execute it
  immediately and wait for the result before doing anything else,
  things seem to work as expected.
*/
export const errorWorkAroundAsync = async () => {
  // connect
  const app = await initializeApp(FIREBASE_CONFIG);

  // database path
  const dbRefPath = 'todos';

  // create reference/query 1
  const dbRef1 = query(
    ref(getDatabase(app), dbRefPath),
    orderByChild('isCompleted'),
    equalTo(false),
  );

  // execute get on reference 1
  const snap1 = await get(dbRef1);

  // observe data is returned
  console.log('dbRef1', snap1.val());

  // create reference/query 2
  const dbRef2 = ref(getDatabase(app), dbRefPath);

  // create listeners on reference 2
  onChildAdded(dbRef2, (snap) => {
    console.log('dbRef2 - ADDED -', snap.key);
  });

  onChildChanged(dbRef2, (snap) => {
    console.log('dbRef2 - CHANGED -', snap.key);
  });

  onChildRemoved(dbRef2, (snap) => {
    console.log('dbRef2 - REMOVED -', snap.key);
  });

  /*
    Here I'm setting a delay to let the listener complete.  If you don't
    do this, this third query is somehow combined/mixed with the above
    two queries, causing the listener to return strange results. Specfically,
    it returns both ADDED events and REMOVED events (when nothing was removed).
    If you remove this timeout, you can see this occur.
  */
  await new Promise(resolve => setTimeout(resolve, 10000));

  // create reference/query 3
  const dbRef3 = query(
    ref(getDatabase(app), dbRefPath),
    orderByChild('isCompleted'),
    equalTo(true),
  );

  // execute get on reference 3
  const snap3 = await get(dbRef3);

  // observe data is returned
  console.log('dbRef3', snap3.val());
}

If you remove the delay, you can see how the queries get mixed up and change the behavior of the get and onChild listeners. After removing the delay, the onChild listeners return six onChildAdded callbacks and then four onChildRemoved callbacks (when nothing was removed).

dbRef1 Object {
  "-MnRQNp76jfDkd25KlS5": Object {
    "isCompleted": false,
    "text": "Todo 1",
  },
  "-MvPbrl8o_Afb0MdeqJb": Object {
    "isCompleted": false,
    "text": "Todo 2",
  },
  "-MvPdfdB5nyYPo1rczp9": Object {
    "isCompleted": false,
    "text": "Todo 4",
  },
  "-MvQ06v44sbWMLMHBQT4": Object {
    "isCompleted": false,
    "text": "Todo 6",
  },
}
dbRef2 - ADDED - -MnRQNp76jfDkd25KlS5
dbRef2 - ADDED - -MvPbrl8o_Afb0MdeqJb
dbRef2 - ADDED - -MvPc1CNYHYsHOGKFGqR
dbRef2 - ADDED - -MvPdfdB5nyYPo1rczp9
dbRef2 - ADDED - -MvPdidjFC4oXqHsvrRG
dbRef2 - ADDED - -MvQ06v44sbWMLMHBQT4
dbRef2 - REMOVED - -MnRQNp76jfDkd25KlS5
dbRef2 - REMOVED - -MvPbrl8o_Afb0MdeqJb
dbRef2 - REMOVED - -MvPdfdB5nyYPo1rczp9
dbRef2 - REMOVED - -MvQ06v44sbWMLMHBQT4
dbRef3 Object {
  "-MvPc1CNYHYsHOGKFGqR": Object {
    "completedAt": 1644345435041,
    "isCompleted": true,
    "text": "Todo 3 - completed",
  },
  "-MvPdidjFC4oXqHsvrRG": Object {
    "completedAt": 1644345879235,
    "isCompleted": true,
    "text": "Todo 5 - completed",
  },
}
@schmidt-sebastian
Copy link
Contributor

@stevenpal Thank you for this report. It looks like the get() query removes data from the cache, when it really should not since it does not actually fetch the full data set. I will pass this on to our RTDB engineers.

@stevenpal
Copy link
Author

I heard back from the Firebase support team with a useful workaround that I thought I would share here in case anyone else is facing this issue. The workaround is to use onValue instead of get (as that seems to cause the issue).

So instead of doing something like the following with get:

const snap1 = await get(dbRef1);

You can instead just use onValue with a onlyOnce: true for the listen options:

const snap1 = await new Promise((resolve, reject) => {
  onValue(dbRef1, (snap) => resolve(snap), { onlyOnce: true });
});

I tested this with the above code example and confirmed that it works.

@StringMatcher
Copy link

StringMatcher commented Jun 23, 2022

This error still exists in SDK v9.8.3

@maneesht
Copy link
Contributor

@stevenpal - we have made several improvements on the existing get() API. Can you please test with the latest version of the SDK?

@stevenpal
Copy link
Author

@maneesht Thanks so much for the update! I just tested this with Firebase 9.9.3 and using get is working as expected now. Really appreciate the follow-up!

@firebase firebase locked and limited conversation to collaborators Sep 30, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

7 participants