sync problem with a collection and document of overlapping paths

[mesqueeb]

there seem to be some sync issues when there's a collection called users and a doc called users/{userId} both loaded at the same time.

problem 1. not all users get loaded

problem 2. user doc is not updated when that doc is updated through the users module and visa versa

related: #316

[mesqueeb]

From @dsl101

I think this may have had some unintended consequences... I've got a collection with over 100 user documents in it, and 2 modules using v-e-f.

The first module (fsUser) binds the currently logged-in user to the document at /users/{userId}. It can then test if the current user is an admin allowed to edit other user accounts. If so, it binds a second module (fsAllUsers) to the collection at /users.

But, what I'm seeing is that this second module only sees the current user document, not all 100+ users. And the reason is this change I think.

I added some logging to my local copy of the v-e-f package like this:

else if (gotFirstLocalResponse) {
    // it's not the first call and it's a change from cache
    // normally we only need to listen to the server changes, but there's an edge case here:
    // case: "a permission is removed server side, and instead of this being notified
    // by firestore from the _server side_, it only notifies this from the cache...
    if (getters.collectionMode) {
        docChanges = querySnapshot.docChanges();

console.log('docChanges:', docChanges)

        docChanges.forEach(function (change) {
            // only do stuff on "removed" !!
            if (change.type === 'removed') {
                var doc = getters.cleanUpRetrievedDoc(change.doc.data(), change.doc.id);
                dispatch('applyHooksAndUpdateState', {
                    change: change.type,
                    id: change.doc.id,
                    doc: doc,
                });
            }
        });
    }
}

and I can see in that console.log() line that there are 110 entries in docChanges. But then they are ignored, as they're all of type added...

I suspect this is because I already have the /users/{userId} document bound, and perhaps that is changing the behaviour of the firebase library? If I target a completely unrelated collection, I get all the docs as expected. But in this case, I do need to bind it twice in different modules (one as a doc, one as a collection).

If I comment out the line if (change.type === 'removed') { and its closing brace, I get all the docs in the collection.

However, I'm also missing updates on the collection module (e.g. changes in the bound document are synced with firestore, but then not replicated back to the bound collection), so perhaps this isn't the whole story. Should it be possible to have these 2 modules running happily side-by-side? If so, I'll try to make a minimal repro.

[mesqueeb]

I'm having difficulties trying to reliably reproduce this.

The times there's no problem for me:

The times there's a problem:

I have no idea of the cause yet.

[mesqueeb]

added more info

working case:

failing case:

[dsl101]

If it's any consolation, I also see the weirdness when sometimes it works fine and sometimes it doesn't. There seem to be different mechanisms the firebase SDK uses in different scenarios—for example, I'm developing a Vue/Quasar app, and hot-reloaded changes see different results to refreshing the browser page, to refreshing with clear cache, to logging out and in again (firebase authentication).

In my original post on the other thread, the console.log() shows 110 changes of type 'added', and so bypassing the if (change.type === 'removed') test and calling dispatch('applyHooksAndUpdateState', ... on all of them did appear to fix things though. Is it possible that code path should be used for more change types?

[mesqueeb]

on a local build, I comment out the entire part at:

else if (gotFirstLocalResponse) {
// it's not the first call and it's a change from cache

I'm now 100% sure it's not related...

If I, after auth listener triggers I execute:

dispatch('company/openDBChannel', { companyId }, { root: true }).then(() => {
  dispatch('companies/openDBChannel', null, { root: true })
})

then 50~80% of the time the second call will fail. that "companies" collection will execute with fromCache set to true twice...

If I instead do:

dispatch('company/openDBChannel', { companyId }, { root: true })
dispatch('companies/openDBChannel', null, { root: true })

just in parallel, then it works 100% of the time........

[mesqueeb]

@dsl101
Ok, I found out it will fail no matter how long you wait, as long as you do something like this:

dispatch('company/openDBChannel', { companyId }, { root: true })
setTimeout(() => {
  console.log(`waited 5 sec!`)
  dispatch('companies/openDBChannel', null, { root: true })
}, 5000)

now it's reproducible and someone can perhaps report to firebase js sdk. :D maybe I will if I have some time later!

[mesqueeb]

tracked down the bug to the firebase sdk level. I hope to find a workaround soon
firebase/firebase-js-sdk#3053 (comment)

[dsl101]

I've also been able to reproduce it in the browser—but apart from that fromCache flag, everything else in the event seems correct (e.g. docChanges() contains 1 element on the first call, and then n-1 elements on the second call, and they have the correct data in them).

I notice someone just picked up your issue though, so that's promising :)

[mesqueeb]

Making some progress in the firebase thread. Hope that they can look into it.

Based on their solution/answer I will either way have to review how my library handles what happens in the onSnapshot listener.

[louisameline]

One thing is odd in the bug report. You say a prerequisite is to have persistence disabled (no cache). Then how could the first trigger batch of data come from cache? Did I miss something?

[louisameline]

Apart from this oddness, I'm starting to wonder if there actually is an issue. Yes, it's weird that sometimes it starts by a fromCache=true response if there is no cache, but when it does, there is a follow-up fromCache=false response which basically tells you "I have now checked with the server and there are no doc changes since the last times I gave you the docs from cache".

[louisameline]

Basically, the issue is more about having two realtime listeners with a same doc in common, and only one of them triggering an update, right? Maybe because the second one comes "from cache" from Firestore's point of view, and that we haven't thought of that case. Or maybe Firestore doesn't trigger anything at all on the second listener because somehow it considers we've been informed already. Can you please check with metadata changes enabled?

As a side note, in the next version of vef, that won't be an issue anyway, as docs of a collection will be modules on their own, so there wouldn't be two modules storing the same data (which is a bad design choice to begin with imho, even if you felt it was simpler to handle at the time).

[louisameline]

This being said about problem 2: I'm more surprised by problem 1. When you say

problem 1. not all users get loaded

What is it about exactly? One missing (which one, the one streamed in the other module)? Or several?

[mesqueeb]

@dsl101
I agree with @louisameline that:

problem 2. user doc is not updated when that doc is updated through the users module and visa versa

this is a problem with how vuex-easy-firestore 1.0 is built and is difficult to fix with the current architecture. We'll mark this as won't-fix and make sure this doesn't happen in v2.0, which is coming along slowly but surely.

@louisameline

You say a prerequisite is to have persistence disabled (no cache). Then how could the first trigger batch of data come from cache? Did I miss something?

Yeah, I have no idea. I guess that's just how firebase works. It sometimes gives the server results marked fromCache: true while they are actually coming from the server. And enabling includeMetadataChanges will not prevent this, you'll 100% get another fromCache: false trigger in this case, but without sometimes without any documents inside.

You can replicate this sad truth about the weirdness of firebase, unrelated to v-e-f by following my reproduction steps here:
firebase/firebase-js-sdk#3053 (comment)

Now, about problem 1...

@louisameline
I think we have a really clear answer now in the last post of wu-hui:
firebase/firebase-js-sdk#3053 (comment)

I reviewed the code we have in place and I think, if we move the check for pending writes up and do nothing in that case:

if (hasPendingWrites === true) {
  // do nothing
} else if (fromCache === true) {
  // always update docs in vuex state
} else {
  // always update docs in vuex state
}

Then we don't need to listen for the gotFirstLocalResponse anymore and can delete this variable.

An abstraction looks something like this:

However, the gotFirstLocalResponse is something you have added 5 months ago:
https://github.com/mesqueeb/vuex-easy-firestore/blame/dev/src/module/actions.ts/#L630

We need to keep in mind:

1. Make sure we don't break those issues you beautifully mention in the commit (so handy!) 😄

2. Also, I think perhaps we don't need any difference between fromCache === true and false at all, as long as we filter out hasPendingWrites calls.
   Especially because, as we've seen, just opening a sub-document of a collection separately can make it so the collection has 2 subsequent fromCache === true triggers.
   (the first one from the sub-document that was cached, the second from the rest of the docs that apparently are still returned with fromCache === true)

3. Another place I found we need to review is where this comment is written:

// the promise should still be pending at this point only if there is no persistence,
// as only then the first call to our listener will have fromCache === false

Because this is simply not true as we've found out.

I'll hit you up on discord, let's discuss!