invertase / react-native-firebase

🔥 A well-tested feature-rich modular Firebase implementation for React Native. Supports both iOS & Android platforms for all Firebase services.
https://rnfirebase.io
Other
11.71k stars 2.22k forks source link

[Firestore] orderBy stops onSnapshot from listening, order incorrect as well #568

Closed rtman closed 6 years ago

rtman commented 7 years ago

Hey, I'm noticing some odd behaviour with orderBy when used in conjunction with onSnapshot. Not sure if it's isolated to just using them together or not.

Here is my code:

this.unsubscribeChatsRef = this.chatsRef
      .where(`users.${this.user.uid}`, '==', true)
      .orderBy('lastMessage.order')
      .onSnapshot((querySnapshot) => {
          console.log('ListenForItems chat query', querySnapshot)
          var listItems = [];
          querySnapshot.forEach((doc) => {
              listItems.push({
                groupName: doc.data().groupName,
                groupNameKey: doc.data().groupName.toLowerCase(),
                groupChat: doc.data().groupChat,
                docId: doc.data().docId,
                lastMessage: doc.data().lastMessage,
                users: doc.data().users,
                createdAt: doc.data().createdAt,
              }); 
              this.setState({
                data: listItems,
                loading: false
              })'
          });
      });
  1. I'm seeing that orderBy seems to be disabling onSnapshot from listening.

When I run the above code, it picks up all the data correctly on the first go around. But if I send any updates to the docs retrieved in the query, the data is not updated on the app end.

I then commented out .orderBy('lastMessage.order) and it works as intended. The listener is functioning and updating the data properly.

I'm seeing the following in adb logcat:, (i left out all the object data). Not sure it's relevant tho as I see it in both of the above cases:

10-31 13:26:13.108  3627  3627 W unknown:ReactNative: Calling JS function after bridge has been destroyed: RCTDeviceEventEmitter.emit(["firestore_collection_sync_event",{"querySnapshot":{"metadata":{"hasPendingWrites":true,"fromCache":false},"documents":[{"metadata":{"hasPendingWrites":false,"fromCache":false},"
  1. When I run the above code (with orderBy) the order of the docs is all wrong. I tested this by placing numbers going sequentially up in the lastMessage.order field and it was incorrect when presented in the app. On both ascending and descending.

Environment

  1. Application Target Platform: Both, but currently testing in Android

  2. Development Operating System: Windows 10

  3. Build Tools:

  4. React Native version: 0.49.3

  5. RNFirebase Version: 3.0.3 Updated to 3.0.6 no change

  6. Firebase Module: Cloud Firestore

chrisbianca commented 7 years ago

@rtman Sounds like a strange issue. You mention that the target platform is both, but you're currently testing in Android. Are you able to confirm whether this errant behaviour is the same on iOS too or it is just limited to Android?

rtman commented 7 years ago

@chrisbianca

No unfortunately, the eventual goal is iOS as well, but I am just testing on Android for now.

ghost commented 7 years ago

Hi @rtman,

My understanding is that your query is actually not valid in firestore unless you have an index on both 'users.uid' and lastMessage.order. See here https://firebase.google.com/docs/firestore/query-data/indexing

Peter

rtman commented 7 years ago

@peterdivvito

In the past when I've needed a composite index it tells me directly in the logcat, I didn't see that this time. However I'll give it a go! Not sure how I can set an index for the users.${this.user.uid} field though since it's dynamic.

rtman commented 7 years ago

@peterdivvito @chrisbianca

Ok so creating a composite index did indeed fix my ordering issue, but using orderBy is still killing the onSnapshot unfortunately. I wish there was a yellow box warning regarding the need to create a composite index.

Progress tho!

Let me know if I can provide additional info to help diagnose this.

chrisbianca commented 7 years ago

@rtman I've just set up a test case for this in our test app, but am unable to reproduce the issue. Both iOS and Android are receiving events in onSnapshot when the data changes.

My test case is as follows:

Data setup:

const COL_1 = {
  baz: true,
  daz: 123,
  foo: 'bar',
  gaz: 12.1234567,
  geopoint: new firebase.native.firestore.GeoPoint(0, 0),
  naz: null,
  timestamp: new Date(2017, 2, 10, 10, 0, 0),
};

const collectionTests = firebase.firestore().collection('collection-tests2');
  await Promise.all([
    collectionTests.doc('col1').set({ ...COL_1, foo: 'bar0' }),
    collectionTests.doc('col2').set({ ...COL_1, foo: 'bar1', daz: 234, timestamp: new Date(2017, 2, 11, 10, 0, 0) }),
    collectionTests.doc('col3').set({ ...COL_1, foo: 'bar2', daz: 345, timestamp: new Date(2017, 2, 12, 10, 0, 0) }),
    collectionTests.doc('col4').set({ ...COL_1, foo: 'bar3', daz: 456, timestamp: new Date(2017, 2, 13, 10, 0, 0) }),
    collectionTests.doc('col5').set({ ...COL_1, foo: 'bar4', daz: 567, timestamp: new Date(2017, 2, 14, 10, 0, 0) }),
  ]);

Test:

          const collectionRef = collectionTests.orderBy('timestamp').endAt(new Date(2017, 2, 12, 10, 0, 0));
          const newDocValue = { ...COL_1, foo: 'updated' };

          const callback = sinon.spy();

          // Test

          let unsubscribe;
          await new Promise((resolve2) => {
            unsubscribe = collectionRef.onSnapshot((snapshot) => {
              callback(snapshot.docs.map(doc => doc.data().daz));
              resolve2();
            });
          });

          callback.should.be.calledWith([123, 234, 345]);

          const docRef = firebase.firestore().doc('collection-tests2/col1');
          await docRef.set(newDocValue);

          await new Promise((resolve2) => {
            setTimeout(() => resolve2(), 5);
          });

          // Assertions

          callback.should.be.calledWith([123, 234, 345]);
          callback.should.be.calledTwice();

          // Tear down

          unsubscribe();
chrisbianca commented 7 years ago

I just updated my test to use a complex field path, i.e. object.field just to rule that out and it still worked fine

rtman commented 7 years ago

@chrisbianca

Looks ok, your test setup is bit confusing me to me but I get what is going on.

One thing I notice is that you are testing with .endAt(new Date(2017, 2, 12, 10, 0, 0) which I am not. Is using endAt required when using orderBy?

Also my orderBy was after a where, I wonder if that would make a difference.

I'll have another go at debugging this issue in the next day or two but please let me know if I can provide more detail on how to figure this out, I'm kind of at a loss right now.

rtman commented 7 years ago

@chrisbianca I have another section in my code that is actually working with orderBy and onSnapShot. It is as follows:

        this.unsubscribeListenForItems = this.chatsRef
            .doc(this.chat.docId)
            .collection('messages')
            .orderBy('order')
            .onSnapshot((snapshot) => {
                ...
            }

The difference being that this does not use where. Can you re test with a where query followed by orderBy and onSnapShot to see if you can reproduce the issue?

gvillenave commented 6 years ago

I'm seeing the same behavior when using a combination of where/orderBy/limit, onSnapshot is not being called. It works when I remove the orderBy clause.

rtman commented 6 years ago

@chrisbianca Have you had a chance to review my feedback? Seems like this is a real issue now that there is another user experiencing the same issue.

mutazmq commented 6 years ago

Me too, I have the same issue when using combination where/orderBy/onSnapshot

kkusanagi commented 6 years ago

me too. I don't know why just removing orderBy and it work. Adding orderBy just make thing worst.

    this.unsubscribe = dbFireStore.collection('room').where('user_id.'+user_id,'==',true).orderBy('last_update','desc')
    .onSnapshot((querySnapshot)=>{
});
ghost commented 6 years ago

The answer to this seems to be addressed on this page.

https://firebase.google.com/docs/firestore/query-data/order-limit-data

However, if you have a filter with a range comparison (<, <=, >, >=), your first ordering must be on the same field:

rtman commented 6 years ago

@peterdivvito

I'm not using a range comparison just '=='. So that shouldn't apply?

kkusanagi commented 6 years ago

Had tried on using get(). This is not working as well. According to @peterdivvito, it should be for range comparison only. but not like below using '=='.

dbFireStore.collection('room').where('user_id.'+global.obj_user.user_id,'==',true).orderBy('last_update').get().then((qs)=>{
      console.log(qs);
    });
jasonmerino commented 6 years ago

I'm seeing this behavior on iOS as well. The onSnapshot function is called initially for all the results that currently exist in firestore, but when a new value is added it does not get called again. Here's the code that I'm using.

const app = firebase.app();
const userId = app.auth().currentUser.uid;
app
  .firestore()
  .collection("entries")
  .where("userId", "==", userId)
  .orderBy("dateCreated")
  .onSnapshot(snapshot => {
    // handle change
  });

I have verified that when I remove the .orderBy("dateCreated") portion the onSnapshot function is called as expected and get's live updates as data is added. But then I need to sort client side which would be less than ideal with large data sets.

priyankinfinnov commented 6 years ago

FYI: AFAIK the where and orderBy has to be on same key. Source: https://firebase.google.com/docs/firestore/query-data/order-limit-data#order_and_limit_data Check the bottom most code example

Please check and close this issue, if you find it correct. cc @chrisbianca

rtman commented 6 years ago

@priyankinfinnov It says this is specifically for non equal comparison (range comparison). However, if you have a filter with a range comparison (<, <=, >, >=), your first ordering must be on the same field My original issue is with an equality comparison.

@Salakar @chrisbianca

Can you guys weigh in on this issue please?

Salakar commented 6 years ago

If you could provide a reproducible code + data sample then that'd be super helpful. Currently we've been unable to reproduce this issue.

cc @chrisbianca

rtman commented 6 years ago

@Salakar Are you referring to the test case that @chrisbianca posted earlier? Or has there been an update? Because the earlier test case didn't test a where query followed by an orderBy which I believe is the issue here.

chrisbianca commented 6 years ago

Ok, new test case added (https://github.com/invertase/react-native-firebase/commit/b099d13730711ac02d2a49e96f40f7cad54a20fe) which uses the following ref. Note that it uses a where and orderBy on different fields.

const collectionRef = collectionTests.where('baz', '==', true).orderBy('daz');

1) It sets up an onSnapshot listener, which returns 5 items initially, in the correct order. 2) It then adds a new item to the end of the collection. 3) The onSnapshot listener fires a second time with the 6 items.

I'm sorry to say that this doesn't appear to be an issue.

gvillenave commented 6 years ago

That's really odd, this is still happening consistently for me. Example of query:

const query = firebase
          .firestore()
          .collection('events')
          .where('type', '==', 'comment')
          .where('parent.id', '==', this.itemId)
          .orderBy('created', 'asc');

As soon as I remove the "orderBy" clause (I also tried with a single "where" instead of multiple), the query works as expected, but adding the "orderBy" always returns "Operation was rejected because the system is not in a state required for the operation's execution". Could it be a bug on Firebase's side, would it be worth it to start involving Google's support?

gvillenave commented 6 years ago

Found the issue! Turns out that Firestore needed indexes for these queries. I found out because I have some cloud functions that use similar queries and I saw the error message in the logs there. These queries work fine on the client too now after adding the indexes.

@chrisbianca Ideally the React Native client should show the same error message in that case, right? Is this something missing in RNFirebase, or a limitation of the native SDKs? Error: The query requires an index. You can create it here: https://console.firebase.google.com/project/...

rtman commented 6 years ago

@chrisbianca Hmm that is interesting, but since so many people are encountering this scenario something must be up. I'll see about providing reproducible code and a data sample.

@gvillenave You're seeing an actual error message? I don't see anything in logcat or console. Are you sure it's the same issue as the one I've posted?

Edit* just saw your post, that is a separate issue that was tracked here

chrisbianca commented 6 years ago

@gvillenave Unfortunately, this error doesn't seem to make it into the onSnapshot error block so we're unable to return it back to the error handler.

@rtman I agree, there's certainly something strange going on, but without a reproducible example for us to investigate, then we're a bit stuck this end. If anybody tracking this is able to extract a small data + code example that exhibits this behaviour then I'm happy to keep investigating!

chrisbianca commented 6 years ago

I'm going to mark this as closed, as we've been unable to reproduce and our tests are all working correctly.

Please re-open with a reproducible example if you're still experiencing problems.

RileyDavidson-Evans commented 6 years ago

For anyone still experiencing this issue, you need to create the indexes for your query ! This will allow the snapshot to receive the real time updates, as an example :

firebase.firestore().collection('Workouts')
      .where('userId', '==', firebase.auth().currentUser.uid)
      .orderBy('timestamp', 'desc')
      .onSnapshot((snapshot) => {
        const records = [];
        snapshot.forEach(record => records.push(record.data()))
        this.setState({ records });
      });

For this to work, I need to go into my firestore indexes and CREATE a compound index with BOTH timeStamp ( set to desc ) and userId. Do this any your snapshots will receive real time updates !

kevboh commented 6 years ago

I also just ran into this. I wish the "create an index" prompts + url would log as errors to the console like firebase admin does.

rtman commented 6 years ago

If someone has the time please upload a sample for them to reproduce this issue, it's clearly a thing as it's happening to so many people. Unfortunately I've been so busy I haven't been able to do it yet

cawfree commented 6 years ago

I'm still getting this issue for any compound query that uses a non-== where in addition to a == where, or orderBy, in onSnapshot. Applying indexing on any field doesn't make any difference whatsoever. However, using a conventional get() request works just fine.

RileyDavidson-Evans commented 6 years ago

@Cawfree Can you please post a code snippet of the snapshot query ?

cawfree commented 6 years ago

@RileyDavidson-Evans

Hey, here's an example, using "react-native-firebase": "^3.2.2",:

firebase.firestore().collection('table').where('deleted', '==', false).where('ready', '==', true).where('score', '>', 30)

Just for total clarity, the table collection contains documents that have the fields deleted, ready and score, of types boolean, boolean, integer respectively.

I've tried setting up combinations of all kind of indexing mechanisms. The initial query will return, but any modifications to that dataset later on will not be received; i.e., adding a new element that should satisfy the query. It makes me think that the onSnapshot function is only capable of listening to elements that satisfied the query at the time it was executed.

I have similar difficulties for related queries, such as:

firebase.firestore().collection('table').where('deleted', '==', false).where('ready', '==', true).orderBy('score') // also problematic using orderBy('score', 'desc')

The only time I have success using where conditions that aren't simple comparisons is when they're the only entity in the query, i.e. non-compound.

The exact same queries, using get(), instead of onSnapshot(), work flawlessly.

rtman commented 6 years ago

@RileyDavidson-Evans composite index also didn't work for me as discussed earlier in the thread

At this point a code snippet isn't enough the devs need a full working example in expo or something similar

cawfree commented 6 years ago

I might be wrong, but something I've noticed that seems to have an effect is the order of the indexed fields defined on a manually created index on Firebase. i.e.

firebase.firestore().collection('collection').where('user', '==', userId).orderBy('created', 'desc')

Requires the corresponding index: index

If the index on Firebase does not match the order of the query (i.e. if instead we defined created first and then user), this query no longer seemed to return as expected. It also seems to help when orderBy is the last action in constructing your query.

Testing Between Changes When the index is created, your affected data still won't have updated for a while, even after the index is marked as enabled. When starting out, it is useful to wipe all old data altogether to verify whether your query/index configuration works immediately; otherwise you'll be benchmarking your solution against old/incompatible data and get an invalid understanding of what is going on.

There also seems to be some caching behavior in the local installation, so after deleting your data you should fully uninstall your RN application and re-run react-native run-*.

Complex Queries

So, for a more complex example (notice we are using a >= / orderBy / limit ):

firebase.firestore().collection('collection').where('score`, `==`, 30).where('deleted', '==', false).where('ready', '==', true).where('created', '>=', `12091012`).orderBy('created', 'desc').limit(5)

collection would require a dedicated index for this specific query with the following structure:

score (asc), deleted (asc), ready (asc), created (desc)

I think this implies that your queries need to be deterministic, i.e. you know the range of fields they index upon are beforehand. For example, if you had a query which could dynamically index a field like:

const num = Math.floor(Math.random() * 5)
const query = firebase.firestore().collection('collection').where(`num${num}`, `>=`, 30)

You'd need to be sure that individual dedicated indexes on the table collection for each variation, i.e. num0, num1, num2, num3, num4, are defined. (This doesn't mean one index on the table collection that contains all of these fields, it means five different indexes for each field name.)

rtman commented 6 years ago

@Cawfree

Are you saying you got it working in the end?

To refocus this issue, the original issue is not a complex query, its an equality query.

After some investigation it seems that it might have something to do with my where query. The actual query value is a dynamic value so I can't pinpoint the index exactly when defining it (or I would have to create one for every user in the system).

    this.chatsRef
     .where(`users.${this.props.user.uid}.active`, '==', true)
     .orderBy('lastMessage.order')
     .onSnapshot((querySnapshot) => { })

Another portion of my app has a where + order query that is now working fine, it doesn't have the dynamic value in the where part.

        this.ordersRef
            .where('customer.uid', '==', this.props.user.uid)
            .orderBy('timeStamp')
            .onSnapshot((querySnapshot) => { })

@chrisbianca Can you verify if a dynamic value in the where query works for your tests? Also do you have a test firebase account/credentials I could use to try and recreate this issue in?

cawfree commented 6 years ago

@rtman Yes, the steps outlined above solved this issue for me.

I see you're using lastMessage.order; after some experimentation, I've found that for some reason you cannot orderBy the child value of an object you save. This isn't anything specific to react-native-firebase, as this behaviour can be exhibited using the Firebase web library. If you elevate order to the root of the document, your query will function as expected.

rtman commented 6 years ago

@Cawfree

Hey thanks for the info, but I have found that this statement:

I've found that for some reason you cannot orderBy the child value of an object you save. This isn't anything specific to react-native-firebase, as this behaviour can be exhibited using the Firebase web library. If you elevate order to the root of the document, your query will function as expected.

Is incorrect. You can orderBy object keys. The problem was with the way I was storing my data, and as I suspected it was the dynamic user specific variable users.${this.props.user.uid}.active. I was using it in this way:

    this.chatsRef
     .where(`users.${this.props.user.uid}.active`, '==', true)
     .orderBy('lastMessage.order')
     .onSnapshot((querySnapshot) => { })

If you are using a where + orderBy with a specific path that changes with every user or situation, you will need to specify a compound index for every query possible in your app. So in my case that would be every user uid ever to be created in my app which is ofcourse impossible.

Once I understood the problem I was able to search more accurately and found a firestore article on exactly this issue which provided a solution for me that requires no compound index at all. The data stored to the db is in a less then desireable shape now but it does work.

Firestore working with arrays

More specifically it is this section of the above article that is of interest:

Invalid query

db.collection('posts')
.where('categories.cats', '==', true)
.orderBy('timestamp');
)

Queries on multiple fields with range filters require a composite index. Unfortunately, this is not possible with the approach shown above. Indexes must be defined on specific field paths. You would have to create an index on each possible field path (such as categories.cats or categories.opinion) which is impossible to do in advance if your categories are user generated.

You can work around this limitation by encoding all of the information for the query into the map values:

// The value of each entry in 'categories' is a unix timestamp
{
  title: "My great post",
  categories: {
    technology: 1502144665,
    opinion: 1502144665,
    cats: 1502144665
  }
}

Now you can express the desired query as a set of conditions on a single field, avoiding the need for a compound index:

Valid Query

 db.collection('posts')
    .where('categories.cats', '>', 0)
    .orderBy('categories.cats');
)

This issue can be closed now @chrisbianca

chrisbianca commented 6 years ago

@rtman thanks for the update, that's very good to know!

sfratini commented 6 years ago

While this has been widely confirmed, I had the same issue so I wanted to add my example:

firebase.firestore().collection("messages")
        .where("chatId", "==", this.props.chatId)
        .orderBy("createdAt", "desc")
        .limit(5)
        .onSnapshot(querySnapshot => {
            let messages = [];
            querySnapshot.forEach(snapshot => {
                messages.push(snapshot.data());
            });
            this.setState({
                isLoading: false,
                messages: messages
            })
        })

Basically I had to add a compound index in messages with chatId and createdAt both desc. It started working after that.

naveenkumardot25 commented 6 years ago

Hey @rtman, Use this after querySnapshot.forEach

listItems.sort(function (a, b) { return parseFloat(a.lastMessage.order) - parseFloat(b.lastMessage.order); }); return listItems;

Remove orderBy() and Just use Where()

Otherwise, Use the Server Time as order Time and it will be like timestamp: Fri Jul 20 2018 13:53:47 GMT+0530 (India Standard Time)

Example,

var array = [ {date: "Fri Jul 20 2018 00:53:47 GMT+0530 (India Standard Time)"}, {date: "Fri Jul 20 2018 16:53:47 GMT+0530 (India Standard Time)"}, {date: "Fri Jul 20 2018 10:53:47 GMT+0530 (India Standard Time)"}, {date: "Fri Jul 20 2018 01:53:47 GMT+0530 (India Standard Time)"}, ]; array.sort(function(a,b){ var c = new Date(a.date); var d = new Date(b.date); return d-c; // or c-d; });

console.log(array);

katwal-dipak commented 6 years ago

In my case the issue was with composite index.

We can create it manually from firebase console. Another option would be to see the error log for query. It will show error like The query requires an index with URL, just click the URL it will suggest appropriate Indexing.

The query requires an index. You can create it here: 

https://console.firebase.google.com/project/<project-id>/database/firestore/indexes?create_index=EgVwb3N0cxoUChBzZWxlY3RlZENhdGVnb3.....
metawort commented 3 years ago

Even with a composite index, I still experience this issue with the following query:

firebase.firestore().collection('chats')
    .where('participantKeys', 'array-contains', userId)
    .orderBy('lastMessage.timestamp', 'desc')
    .onSnapshot(/* ... */)

with

"@react-native-firebase/firestore": "^10.0.0",

Usual problem, the query works initially, but doesn't get updates.

I tried both, having participantKeys or lastMessage.timestamp as the first field of the index, but it doesn't work either way.

Could the issue be that participantKeys is an array field?

mikehardy commented 3 years ago

Hmm @metawort this has been closed a while, might be best to open a new issue but more important than anything is a clean / minimal reproduction - specifically if you could post a fully self-contained App.js that creates a data fixture then demonstrates the issue, that will be critical in isolating this. I see you just updated the post - definitely make sure you can reproduce with v11.2.0 react-native-firebase and 26.8.0 firebase-android-sdk (which should be included in v11.2.0 react-native-firebase, but you may have overridden it)

russelh15 commented 3 years ago

You need to create indexes for it to work. The error will appear in the Xcode debugger, not the Javascript debugger.