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

[Firestore] whereArrayContains not working in offline scenario. #155

Closed
IvanBean opened this issue Dec 6, 2018 · 5 comments · Fixed by #622
Closed

[Firestore] whereArrayContains not working in offline scenario. #155

IvanBean opened this issue Dec 6, 2018 · 5 comments · Fixed by #622
Assignees

Comments

@IvanBean
Copy link

IvanBean commented Dec 6, 2018

[REQUIRED] Step 2: Describe your environment

  • Android Studio version: 3.2.1
  • Firebase Component: Firestore
  • Component version: 17.1.1

[REQUIRED] Step 3: Describe the problem

Add a string value to document's array by arrayUnion when the device is offline, the snapshot listener shows it added successfully but later we query by whereArrayContains shows empty result.

Steps to reproduce:

Let's say we have a collection todos containing a single document todo1, which had only one field taskIds as a string array. The schema: https://i.imgur.com/CmAFWgx.png

Step to reproduce:

  1. Add a snapshot listener to observe data change:
firestore.collection("todos").addSnapshotListener { snapshot, exception ->
            snapshot?.documents?.forEach {
                Timber.d("todos ${it.data}")
            }
        }

In the beging the log shows: todos {taskIds=[]}

  1. Turn device to offline, kill and re-launch the app.

  2. Create a task document with auto-generate id, add the id to document "todo1" by arrayUnion:

firestore.collection("todos")
        .document("todo1")
        .update("taskIds", FieldValue.arrayUnion(taskId))

The listener at step 1 now shows log: todos {taskIds=[mMifGffC5WEMjALjR6xR]}, so the taskId is added successfully.

  1. Later, within a click event we query todos collection with whereArrayContains, either by get():
firestore.collection("todos")
        .whereArrayContains("taskIds", taskId)
        .get()
        .addOnCompleteListener { task ->
            Timber.d("size ${task.result?.documents?.size}")
            Timber.d("isFromCache ${task.result?.metadata?.isFromCache}")
        }

or addSnapshotListener:

firestore.collection("todos")
        .whereArrayContains("taskIds", taskId)
        .addSnapshotListener { snapshot, exception ->
            Timber.d("size ${snapshot?.documents?.size}")
            Timber.d("isFromCache ${snapshot?.metadata?.isFromCache}")
        }

The result are the same:

size 0
isFromCache true

But I expect the result size be 1 because document "todo1" should be included.

The same steps work well when device online, both query shows size 1 with addSnapshotListener is from cache and get() isn't.

Any reply were be appreciated, thanks.

@IvanBean IvanBean changed the title [Firestore] whereArrayContains not working in specific offline scenario. [Firestore] whereArrayContains not working in offline scenario. Dec 6, 2018
@samtstern
Copy link
Contributor

@IvanBean thanks for the specific reproduction steps! Sounds like a bug to me, will wait for @gsoltis or @wilhuff to chime in.

@mikelehen
Copy link
Contributor

Hrm. That certainly doesn't sound expected. I'll try to reproduce.

@mikelehen mikelehen self-assigned this Dec 6, 2018
@mikelehen
Copy link
Contributor

@IvanBean Thanks for reporting this! I've reproduced the issue and tracked it down to a bug in our local query processing here.

It will happen when you do an update() that causes a document that used to not-match the query to now match the query. Basically, since the original document didn't match the query, we fail to apply the update(), and so it doesn't get considered in the final query results. :-/ As a workaround you could potentially use a set() or set(..., SetOptions.merge()) instead of the update.

We'll take a look at getting this fixed in an upcoming release. Thanks again for reporting it.

@mikelehen mikelehen assigned var-const and unassigned mikelehen Dec 8, 2018
@mikelehen
Copy link
Contributor

@var-const I wonder if this is something you might be able to look at tackling?

I went to try to fix it on web (so I could write a spec test for validation), but realized I wanted the RemoteDocumentCache.getAll() method that you recently added (but hasn't been ported to web yet). And if I do fix it on web, it may conflict with your porting of that work.

Here's the spec test I wrote (with the intention of adding it to listen_spec.test.ts after the "Ensure correct query results with latency-compensated deletes" test:

  specTest(
    'Ensure correct query results for latency-compensated updates',
    [],
    () => {
      const fullQuery = Query.atPath(path('collection'));
      const filteredQuery = fullQuery.addFilter(filter("match", "==", true));
      const docA = doc('collection/a', 1000, { match: false });
      const docAv2Local = doc('collection/a', 1000, { match: true }, { hasLocalMutations: true });

      return (
        spec()
          .userListens(fullQuery)
          .watchAcksFull(fullQuery, 1000, docA)
          .expectEvents(fullQuery, { added: [docA] })

          // patch docA so that it will now match the filtered query.
          .userPatches("collection/a", {match: true })
          .expectEvents(fullQuery, { modified: [docAv2Local], hasPendingWrites: true })
          // Make sure docA shows up in filtered query.
          .userListens(filteredQuery)
          .expectEvents(filteredQuery, { added: [docAv2Local], fromCache: true,
            hasPendingWrites: true })
      );
    }
  );

NOTE: I verified it fails in the way I expect, but since I haven't fixed the bug can't 100% guarantee it's correct. :-)

Alternatively, I can look at tackling this once you've ported your perf improvements to web. Thanks.

@var-const
Copy link
Contributor

Michael, I think I can do it together with the porting.

@var-const var-const assigned mikelehen and unassigned var-const Jan 14, 2019
@mikelehen mikelehen assigned wilhuff and unassigned mikelehen Jan 29, 2019
ghost pushed a commit to firebase/firebase-js-sdk that referenced this issue Jul 12, 2019
…tion. (#1957)

When a local patch mutation is not acked by server yet, but the mutation makes a document matching a query, the document might not show up in the compensated query results. This PR fixes the bug.

See firebase/firebase-android-sdk#155
@ghost ghost closed this as completed in #622 Jul 16, 2019
wilhuff added a commit that referenced this issue Jul 19, 2019
wilhuff added a commit that referenced this issue Jul 19, 2019
@firebase firebase locked and limited conversation to collaborators Oct 13, 2019
This issue was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants