Fix missing query results when it is a match after a local patch mutation. #1957

ghost · 2019-07-10T15:28:36Z

Fixing firebase/firebase-android-sdk#155

packages/firestore/src/local/local_documents_view.ts

var-const · 2019-07-10T20:05:49Z

packages/firestore/src/local/local_documents_view.ts

+        );
+      })
+      .next(baseDocsForPatching => {
+        baseDocsForPatching.forEach((key, doc) => {


Optional: if you decide to follow the suggestion to create a helper function, perhaps you can move this filtering step there as well.

As i wrote in other reply, i'd like to keep the block as-is.

var-const · 2019-07-10T20:07:25Z

packages/firestore/src/local/local_documents_view.ts

+          missingBaseDocEntriesForPatching
+        );
+      })
+      .next(baseDocsForPatching => {


Nit: this variable name is perhaps slightly misleading; without context, it sounds like it contains all base documents, but in fact it's only a subset of those documents (those that have pending writes making them potentially match). Consider renaming (docsWithPendingWrites? docsWithLocalPatches?).

changed to docsWithPendingPatches.

var-const · 2019-07-10T21:14:56Z

@schmidt-sebastian Sebastian, since you previously volunteered, could you please check this implementation doesn't leave out any important steps? It looks fine to me, but I'm concerned I might be missing some of the context here.

packages/firestore/src/local/local_documents_view.ts

schmidt-sebastian · 2019-07-10T21:46:17Z

packages/firestore/src/local/local_documents_view.ts

        for (const batch of matchingMutationBatches) {
+          for (const mutation of batch.mutations) {
+            // Only process documents belonging to the collection.
+            if (!query.path.isImmediateParentOf(mutation.key.path)) {


Is this needed? I would think that these mutations would already be filtered out by getAllMutationBatchesAffectingQuery.

Yeah, getAllMutationBatchesAffectingQuery seems to be filtering that already. I keep it because the block below also has it..I removed that as well.

packages/firestore/src/local/local_documents_view.ts

schmidt-sebastian · 2019-07-10T21:49:41Z

packages/firestore/src/local/local_documents_view.ts

+          missingBaseDocEntriesForPatching
+        );
+      })
+      .next(docsWithPendingPatches => {


You could improve the readability here if you chained this off the invocation ending in 252. That would move the code closer to its surrounding logic.

schmidt-sebastian · 2019-07-10T21:51:03Z

packages/firestore/test/unit/specs/query_spec.test.ts

@@ -98,4 +98,41 @@ describeSpec('Queries:', [], () => {
        hasPendingWrites: true
      });
  });
+
+  specTest(
+    'Ensure correct query results for latency-compensated updates',


Maybe 'Latency-compensated updates are included in query results'?

schmidt-sebastian · 2019-07-10T21:53:53Z

packages/firestore/test/unit/specs/query_spec.test.ts

+    'Ensure correct query results for latency-compensated updates',
+    [],
+    () => {
+      const fullQuery = Query.atPath(ResourcePath.fromString('collection'));


This can be Query.atPath(path('collection'))

schmidt-sebastian · 2019-07-10T22:08:52Z

packages/firestore/test/unit/specs/query_spec.test.ts

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


I believe that if you do userUnlisten(fullQuery) here then you can remove the event right below and further narrow down your test case.

I'm not sure i understand..can you explain more?

I am myself no longer fully sold on my idea. But let me explain what I had in mind:

spec() .withGcEnabled(false) .userListens(fullQuery) .watchAcksFull(fullQuery, 1000, docA) .expectEvents(fullQuery, { added: [docA] }) .userUnlistens(fullQuery) .userPatches('collection/a', { match: true }) .userListens(filteredQuery) .expectEvents(filteredQuery, { added: [docAv2Local], fromCache: true, hasPendingWrites: true })

I believe this would work and it would remove the event in line 124 that is not directly related to your change. The reason I am no longer fully sold on this is that you would have to run the test with GC disabled to make this work. Otherwise, the SDK would delete docA from the RemoteDocumentCache as the user unlistens.

Yep, can confirm what you described. Without GC disabled this is not working.

I am keep it as-is as that is slightly more likely what happens in real world.

schmidt-sebastian · 2019-07-11T07:24:09Z

packages/firestore/src/local/local_documents_view.ts

-              results = results.remove(key);
+        mutationBatches = matchingMutationBatches;
+        // It is possible that a PatchMutation can make a document a match to the query,
+        // but the version in `remoteDocumentCache` is not a match yet (waiting for server


Nit: s/remoteDocumentCache/the RemoteDocumentCache/ (since it is a widely used concept in Firestore)

schmidt-sebastian · 2019-07-11T07:24:32Z

packages/firestore/src/local/local_documents_view.ts

+        mutationBatches = matchingMutationBatches;
+        // It is possible that a PatchMutation can make a document a match to the query,
+        // but the version in `remoteDocumentCache` is not a match yet (waiting for server
+        // to ack). To handle this, we find all document keys affected by some`PatchMutation`s


s/somePatchMutations/the PatchMutations/

schmidt-sebastian · 2019-07-11T07:26:47Z

packages/firestore/src/local/local_documents_view.ts

+        // that are not in `result` yet, and back fill them via `remoteDocumentCache.getEntries`,
+        // otherwise those `PatchMutations` will be ignored because no base document can be found,
+        // and lead to missing result for the query.
+        return this.getDocumentWithPendingPatches(transaction, mutationBatches, results)


getDocumentWithPendingPatches only conveys half of the story. Maybe `getMissingBaseDocuments()?

schmidt-sebastian · 2019-07-11T07:27:56Z

packages/firestore/src/local/local_documents_view.ts

+    transaction: PersistenceTransaction,
+    matchingMutationBatches: MutationBatch[],
+    results: DocumentMap
+  ): PersistencePromise<NullableMaybeDocumentMap> {


Would it make sense for this to return a DocumentMap? It would simplify the code above, but you would need to post-filter in here. Alternatively, you could also assemble the final result map here and make this a populateDocumentMapWithMissingBaseDocuments() or something similar. Optional.

My understanding is DocumentMap being an immutable structure makes it awkward to update from the helper function. I have not tried it..but don't expect it to work without extra tricks.

You can get return the combined set of existingDocuments and the result of this function. The goal would be to simplify the getDocumentsMatchingCollectionQuery function.

Optional.

Decide not to do it because it is not a significant improvement..

schmidt-sebastian · 2019-07-11T07:29:01Z

packages/firestore/src/local/local_documents_view.ts

+  private getDocumentWithPendingPatches(
+    transaction: PersistenceTransaction,
+    matchingMutationBatches: MutationBatch[],
+    results: DocumentMap


results is not very clear. Maybe something like existingDocuments?

schmidt-sebastian · 2019-07-11T07:31:48Z

packages/firestore/src/local/local_documents_view.ts

+              if (doc !== null && doc instanceof Document) {
+                results = results.insert(key, doc);
+              }
+            });


Nit: Add empty line to convey that the loop below does something very different (we move from one stage to another).

wu-hui added 2 commits July 10, 2019 11:25

Fix missing results for latency-compensatoed update/patch

71f2411

[AUTOMATED]: Prettier Code Styling

a6b40ba

ghost requested a review from var-const July 10, 2019 16:36

ghost assigned var-const Jul 10, 2019

var-const reviewed Jul 10, 2019

View reviewed changes

var-const assigned ghost and unassigned var-const Jul 10, 2019

address comments and fixing nits

96aed4d

ghost assigned var-const and unassigned ghost Jul 10, 2019

var-const approved these changes Jul 10, 2019

View reviewed changes

var-const requested a review from schmidt-sebastian July 10, 2019 21:13

var-const assigned schmidt-sebastian and unassigned var-const Jul 10, 2019

schmidt-sebastian reviewed Jul 10, 2019

View reviewed changes

schmidt-sebastian assigned ghost and unassigned schmidt-sebastian Jul 10, 2019

wu-hui added 2 commits July 10, 2019 21:02

addressing comments #2

4c726ef

[AUTOMATED]: Prettier Code Styling

0a49cd0

schmidt-sebastian reviewed Jul 11, 2019

View reviewed changes

address comments#3

a6c5635

ghost assigned schmidt-sebastian and unassigned ghost Jul 11, 2019

schmidt-sebastian approved these changes Jul 12, 2019

View reviewed changes

ghost merged commit 0d2e108 into master Jul 12, 2019

firebase locked and limited conversation to collaborators Oct 9, 2019

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix missing query results when it is a match after a local patch mutation. #1957

Fix missing query results when it is a match after a local patch mutation. #1957

ghost commented Jul 10, 2019

var-const Jul 10, 2019

ghost Jul 10, 2019

var-const Jul 10, 2019

ghost Jul 10, 2019

var-const commented Jul 10, 2019

schmidt-sebastian Jul 10, 2019

ghost Jul 11, 2019

schmidt-sebastian Jul 10, 2019

ghost Jul 11, 2019

schmidt-sebastian Jul 10, 2019

ghost Jul 11, 2019

schmidt-sebastian Jul 10, 2019

ghost Jul 11, 2019

schmidt-sebastian Jul 10, 2019

ghost Jul 11, 2019

schmidt-sebastian Jul 11, 2019

ghost Jul 11, 2019

schmidt-sebastian Jul 11, 2019

ghost Jul 11, 2019

schmidt-sebastian Jul 11, 2019

ghost Jul 11, 2019

schmidt-sebastian Jul 11, 2019

ghost Jul 11, 2019

schmidt-sebastian Jul 11, 2019

ghost Jul 11, 2019

schmidt-sebastian Jul 12, 2019

ghost Jul 12, 2019

schmidt-sebastian Jul 11, 2019

ghost Jul 11, 2019

schmidt-sebastian Jul 11, 2019

ghost Jul 11, 2019

Fix missing query results when it is a match after a local patch mutation. #1957

Fix missing query results when it is a match after a local patch mutation. #1957

Conversation

ghost commented Jul 10, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

var-const commented Jul 10, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment