angular / angularfire

Angular + Firebase = ❤️
https://firebaseopensource.com/projects/angular/angularfire2
MIT License
7.7k stars 2.19k forks source link

unexpected: AngularFirestore#collection() returns cached value on initial subscription #2012

Closed jorroll closed 4 years ago

jorroll commented 5 years ago

Version info

Angular: 7.2.5 Firebase: 5.8.3 AngularFire: 5.1.1 Other (e.g. Ionic/Cordova, Node, browser, operating system): node 10.14.2, macOs X

How to reproduce these conditions

I've had some trouble isolating the exact problem. Basically, I have a people collection. If I fetch an individual document from the people collection (lets call it the person A doc), and then subscribe to the entire people collection, I'm seeing that the collection('people').valueChanges() subscription immediately returns an array that just contains person A. The subscription then fires again shortly later with the full results of the person collection.

If I invoke collection('people').valueChanges() before fetching an individual document from the people collection, then collection('people').valueChanges() returns the correct results on the first try.

What I think is happening, is that collection('people') is immediately returning results from an in-memory cache and then updating those results when it hears back from the server. I could see scenarios where this behavior would be desirable, but it is not desirable in my case, was unexpected and hard to diagnose, and there doesn't seem to be an option to disable it. I'm not sure if this is intended behavior or a bug.

I think it's possible that issue #1975 is being caused by the same problem.

In my case, the problem with this behavior (other than the fact that it is unexpected) is that I want to take(1) from the collection('people').valueChanges() query, except now the return value of that action is indefinite.

Debug output

I have none. I did try to set firebase.database().enableLogging(true); as suggested, but this.angularFirestore.app.database() does not have an enableLogging() function.

Expected behavior

By default, I expected query results to not be returned unless they accurately reflect the query.

I could see value in returning results immediately from a cache, but I don't think this behavior should be the default as it's unexpected, hard to diagnose, and can lead to unwanted behavior.

Actual behavior

AngularFirestore#collection() seems to return results immediately from the cache and then update the results when it hears back from the server.

diginikkari commented 5 years ago

I have been hit by this very badly. In my opinion it is a bug.

example:

you have collection where you have following docs: [0, 1, 2, 3, 4, 5, 6, 7, 8, 9]

If you have open subscription to item no 9 and you make query where you limit to 5 first items with take(1) you will receive item [9] instead of [0, 1, 2, 3, 4].

Sample project: https://angular-f6nfor.stackblitz.io

This makes e.g. pagination queries unreliable. When you relay that last item received will be last item of your previous pagination query and you use this item as startAfter for next query.

jamesdaniels commented 5 years ago

This is how the Firestore JS SDK works. I'm considering an API change that would allow you to inspect the metadata and say filter { snap -> !snap.metadata.fromCache } on snapshotChanges()

jek-bao-choo commented 5 years ago

Experiencing this bug too!

tmk1991 commented 5 years ago

Anything would help! Glad someone brought up my old issue from a while ago.

jek-bao-choo commented 5 years ago

@tmk1991 How did overcome the problem all these while?

jek-bao-choo commented 5 years ago

I used a worked around...

you have collection where you have following docs: [0, 1, 2, 3, 4, 5, 6, 7, 8, 9]

If you have open subscription to item no 9 and you make query where you limit to 5 first items and explicitly with a where clause that says item that is not previously projected.

Put another way, query with exclusion to not include no 9.

tmk1991 commented 5 years ago

I use the get() call for one of the queries and snapshotChanges() for the other.

jorroll commented 5 years ago

@jamesdaniels I can't speak for everyone, but your proposed update would address the problems I am experiencing.

jek-bao-choo commented 5 years ago

I use the get() call for one of the queries and snapshotChanges() for the other.

Sounds like another good workaround without adding additional index :)

ColinBrilliantLabs commented 5 years ago

I'm having this same issue but with the first valueChanges() being for an auth user, and the second valueChanges() to display all users. It displays the logged in user first, and then after a second or two displays all of them. I'm using ionic 4 and angular 6, but I can't figure out how to use the get() method as suggested. It seems to return metadata. Does anyone have another solution? I need this asap for work haha.

ColinBrilliantLabs commented 5 years ago

So I came up with a solution, but it's pretty hacky. Basically, I have a value change that fires off every time the db is updated, and then in that value change I have a get() for the exact same documents. This way the get it called every time the db updates, and doesn't get the cached results.

For those that had trouble figuring out how to use the get method: this.userService.getUsersCollection().valueChanges().subscribe(() =>{ this.userService.getUsersCollection().ref.get().then(usersArray=>{

             usersArray.forEach(user => {

                 this.users.push(user.data());
tmk1991 commented 5 years ago

@ColinBrilliantLabs that would cause duplicate read costs. You can use get() as a promise based return of the documents

ColinBrilliantLabs commented 5 years ago

Yeah I know but get() doesn't provide live reloading, unless I did something wrong. If static content is what you need, then get() by itself is fine. The duplicate read cost is what I found to work with live reloading. If you can do that with get() though, please correct me.

tmk1991 commented 5 years ago

you cant. tradeoffs sir :)

ColinBrilliantLabs commented 5 years ago

Yeah, unfortunately I need live reloading so I'll have to do it like this until they allow you to use valueChanges without cache.

jorroll commented 5 years ago

@ColinBrilliantLabs a limitation of firestore is that you can only use a where filter on a single field in a query. If you don't need the where filter for another purpose, you can combine it with an updatedAt field to only return the results that have changed since your last query. For example,

const lastQuery = new Date();
const users = await this.userService.getUsersCollection().ref.get();
// then read the snapshot results to see what has been added, updated, or removed and update the users value as appropriate
this.userService.getUsersCollection({after: lastQuery}).snapshotChanges();

Because firestore only charges you for documents returned, this method wouldn't charge you twice. Obviously it's a lot of work for this use case, but I also use this general strategy to ensure that a user navigating between pages doesn't reload documents which have already been loaded and cached. See https://github.com/firebase/firebase-js-sdk/issues/1551 for some more info.

ColinBrilliantLabs commented 5 years ago

Thanks a lot!!

LanderBeeuwsaert commented 5 years ago

Is it possible to give an update on this? the: filter { snap -> !snap.metadata.fromCache } on snapshotChanges() would be great.

jorroll commented 5 years ago

@LanderBeeuwsaert I believe the AngularFire team is preoccupied by a number of releases surrounding Google I/O as well as, I imagine, the impending Angular 8 release. I wouldn't expect anything to be done with this PR for several months or longer.

LanderBeeuwsaert commented 5 years ago

@thefliik yeah, I've seen the 5.2 beta tickets. Cool stuff coming. Too bad this isn't in it. But yeah, they probably have their hands more than full.

hiepxanh commented 5 years ago

my workaround: Remember this only use for get data one time, cannot listen realtime

.ts
// for note purpose:
this.dbft.collection('users')
  // .valueChanges() // NOTE: remember, we not using this anymore, change to get from server and pipe snapshot => snapshotData
  .get({ source: 'server' })
  .pipe(
    map(actions => {
      const data = [];
      actions.forEach(a => {
        const item = a.data() as Appointment;
        data.push(item);
      });
      return data;
    })
  )
  .subscribe(value => console.log('fresh value', value));
jrodl3r commented 5 years ago

any update on this one? Thx

thameurr commented 5 years ago

any solution ?

jek-bao-choo commented 5 years ago

My workaround.

Use firebase library and indicate no cache.

On Wed, 4 Dec 2019, 7:55 PM thameurr, notifications@github.com wrote:

any solution ?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/angular/angularfire/issues/2012?email_source=notifications&email_token=ABRK4QHSOUL5UVZUQBIP4ZTQW6LBZA5CNFSM4GY6HV3KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEF4YJEQ#issuecomment-561611922, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABRK4QEWWFQNFVZPJ6JC2R3QW6LBZANCNFSM4GY6HV3A .

pshields commented 4 years ago

I'm running into this as well.

The valueChanges documentation currently states that this method returns "the current state of your collection." The documentation does not say anything about the potential for partial or incomplete state from local cache being emitted first. I agree with what others have said about the current behavior being counterintuitive and unexpected.

I view the current behavior as a bug, and my strongest preference would be for the current behavior to be fixed. If the Firebase team does not consider the current behavior to be a bug, at the very least, can the valueChanges documentation be updated to describe the current behavior (how the first valueChanges emission might be incomplete and from local cache), as well as how it can be worked around? Otherwise, I think this behavior will continue to surprise people.

thameurr commented 4 years ago

I try to use this lines, its works without duplicated items or cache because i reset the array of users each time we have snapshot.

const ref = this.afs.firestore.collection('users').onSnapshot( (snap) => { this.user = []; snap.forEach( usr => { this.user.push(usr.data()); }); });

ondokuzz commented 4 years ago

it took me a lot of time to understand that the problem is about caching. i was just doing collection.snapshotChanges().pipe(take(1), map(...)) and the results were partial. i've tried to disable caching but couldn't find how to do it on angularfire? i finally applied the @hiepxanh's work around.

dkeulen commented 4 years ago

Currently have the same issue, and using @hiepxanh's work around no longes makes it fire when there is a change to the content it is subscribed to which is what i need.

leandroz commented 4 years ago

I have the same issue where a single record is returned first (because I was seeing the details of that record previously) and then the correct result.

I use a cursor based approach with limit, so if the number of records is not the same as the limit then I don't take the result in consideration.

hiepxanh commented 4 years ago

Currently have the same issue, and using @hiepxanh's work around no longes makes it fire when there is a change to the content it is subscribed to which is what i need.

I dont undertstand what you expect? if you listen it realtime, it will alway return cache data first, then it will emit the latest data right away, that is what it mean to be, try to init cache data to make it feeling really fast. I think it's working correct, what behavior do you expect?

leandroz commented 4 years ago

@hiepxanh in my case for example, I have a list of 30 tickets, when you click one, you open the details of that ticket. Then we you go back to the list of 30, first 1 appears, and then the 30. That is not the expected behavior.

I am not sure about the best solution so I think the easiest way is to expose information about if the record is served from the cache or not and then let the developer take control.

hiepxanh commented 4 years ago

@leandroz can you repeat it on stackbliz example repository, I will debug it

jorroll commented 4 years ago

@hiepxanh not sure why you feel an example repository is necessary. The behavior @leandroz describes (which this issue is specifically addressing) is not in question. It was confirmed as "working as intended" by @jamesdaniels in a comment above. James also proposed a solution to address this issue in that comment, which (I think) would close this issue from a technical perspective (separately, these is also the issue of "surprise," which could be addressed by some documentation improvements).

somervda commented 4 years ago

I think I have a similar problem. Again because it is how firestore caches in angular it was a tough one to find. Basically the problem is : If I query on the same firestore collection twice in the same angular library then the second query is based on the subset of documents returned in the first query.

In my case I am loading a subset of documents (Checklists of type template) during onInit where the document has a property that isTemplate=true. Latter in the component I want to extract another set of documents from the same collection, but it is always selecting from the "isTemplate=True" subcollection done in the OnInit. See attached video for example.

As a work around I am going to move getting the list of templates to another component but I shouldn't need to do this. It would be nice to be able to tell AngularFirestore library not to cache the results of the first query, in this case.

My Video3.zip

sagits commented 4 years ago

So I came up with a solution, but it's pretty hacky. Basically, I have a value change that fires off every time the db is updated, and then in that value change I have a get() for the exact same documents. This way the get it called every time the db updates, and doesn't get the cached results.

For those that had trouble figuring out how to use the get method: this.userService.getUsersCollection().valueChanges().subscribe(() =>{ this.userService.getUsersCollection().ref.get().then(usersArray=>{

             usersArray.forEach(user => {

                 this.users.push(user.data());

I'm trying this approach because we wanted to show lives from youtube in our Ionic app and we were having a problem where after opening the app (through a notification) the user could see a past live from another day that was cached (i know that firestore sends the server data after the cached data, but this don't happen sometimes, not sure why).

I was worried because .get usually takes a lot of time, for instance, we have a news page where we query 10 news from a 5000 news collection and sometimes it takes 5sec over .get. But doing .get() after a .snapshotChanges() brings the results almost in real time (50ms for every request).

jamesdaniels commented 4 years ago

Metadata is now included in snapshots in 6.1, so you can filter out the cache if you want.

jorroll commented 4 years ago

Much appreciated!!!!!!

DogAndDoll commented 4 years ago

@thefliik, may I ask if this solution helped in your case?

jek-bao-choo commented 4 years ago

@jamesdaniels May I know how to use the metadata to filter out the cache as you described?

DogAndDoll commented 4 years ago

@choopage I don't think that provided solution completely solves the problem that was described originally, but I so far came up with the following solution to skip the first cached push:

 public collectionSnapshotChanges(query?: QueryFn): Observable<Array<T>> {
        return this.getPrivateCollectionRef(query).pipe(
            mergeMap(ref => ref.snapshotChanges()),
            filter((actions, index) => index > 0 || !actions.every(({payload: {doc: {metadata}}}) => metadata.fromCache)),
            map(actions => actions.map(({payload: {doc}}) => ({...doc.data(), id: doc.id}))),
        );
    }
jamesdaniels commented 4 years ago

@DogAndDoll Correct this doesn't solve the "problem" as this is Firestore behaving as designed, this is however a much better work around. Thanks for typing up the example.

matteobucci commented 3 years ago

@DogAndDoll I loved your solution, and it worked at the beginning, but now I'm not receiving any data on an observable.

What I thought is that, if during the first call I have N cached items returned and the second time D items where N is different from D filter is called twice. However, if the cached data corresponds to the remote data the library executes the filter only once, hence not being passed to the component. Is this possible? It's really annoying having such an unpredictable behaviour

danday74 commented 2 years ago

sorry but this issue really does crown angular fire as a hunk a junk - you cant even turn off the cache behaviour :( you instead have to use @DogAndDoll's solution, switching every use of valueChanges to snapshotChanges - unbelievable