From 0d29adc974d72f93552dad53ebd8b4ecab2ce810 Mon Sep 17 00:00:00 2001 From: Mila <107142260+milaGGL@users.noreply.github.com> Date: Tue, 21 Nov 2023 19:24:42 -0500 Subject: [PATCH] Fix docs unnecessarily undergo limbo resolution while resuming query bug (#7740) --- .changeset/olive-dogs-play.md | 7 +++ .../firestore/src/core/firestore_client.ts | 2 +- .../firestore/src/core/sync_engine_impl.ts | 9 ++-- packages/firestore/src/core/view.ts | 30 +++++++++---- .../firestore/test/unit/core/view.test.ts | 8 ++-- .../test/unit/local/query_engine.test.ts | 2 +- .../test/unit/specs/limbo_spec.test.ts | 44 ++++++++++++++++++- .../test/unit/specs/limit_spec.test.ts | 1 + 8 files changed, 83 insertions(+), 20 deletions(-) create mode 100644 .changeset/olive-dogs-play.md diff --git a/.changeset/olive-dogs-play.md b/.changeset/olive-dogs-play.md new file mode 100644 index 00000000000..406c6d86417 --- /dev/null +++ b/.changeset/olive-dogs-play.md @@ -0,0 +1,7 @@ +--- +'@firebase/firestore': patch +'firebase': patch +--- + +Fixed an issue in the local cache synchronization logic where all locally-cached documents that matched a resumed query would be unnecessarily re-downloaded; with the fix it now only downloads the documents that are known to be out-of-sync. + diff --git a/packages/firestore/src/core/firestore_client.ts b/packages/firestore/src/core/firestore_client.ts index fc51133f312..d7819bd7905 100644 --- a/packages/firestore/src/core/firestore_client.ts +++ b/packages/firestore/src/core/firestore_client.ts @@ -729,7 +729,7 @@ async function executeQueryFromCache( const viewDocChanges = view.computeDocChanges(queryResult.documents); const viewChange = view.applyChanges( viewDocChanges, - /* updateLimboDocuments= */ false + /* limboResolutionEnabled= */ false ); result.resolve(viewChange.snapshot!); } catch (e) { diff --git a/packages/firestore/src/core/sync_engine_impl.ts b/packages/firestore/src/core/sync_engine_impl.ts index 60a36d0dcaf..18a1dc681f5 100644 --- a/packages/firestore/src/core/sync_engine_impl.ts +++ b/packages/firestore/src/core/sync_engine_impl.ts @@ -368,7 +368,7 @@ async function initializeViewAndComputeSnapshot( ); const viewChange = view.applyChanges( viewDocChanges, - /* updateLimboDocuments= */ syncEngineImpl.isPrimaryClient, + /* limboResolutionEnabled= */ syncEngineImpl.isPrimaryClient, synthesizedTargetChange ); updateTrackedLimbos(syncEngineImpl, targetId, viewChange.limboChanges); @@ -1081,10 +1081,13 @@ async function applyDocChanges( const targetChange = remoteEvent && remoteEvent.targetChanges.get(queryView.targetId); + const targetIsPendingReset = + remoteEvent && remoteEvent.targetMismatches.get(queryView.targetId) != null; const viewChange = queryView.view.applyChanges( viewDocChanges, - /* updateLimboDocuments= */ syncEngineImpl.isPrimaryClient, - targetChange + /* limboResolutionEnabled= */ syncEngineImpl.isPrimaryClient, + targetChange, + targetIsPendingReset ); updateTrackedLimbos( syncEngineImpl, diff --git a/packages/firestore/src/core/view.ts b/packages/firestore/src/core/view.ts index de66d883ac3..b0a07bd783c 100644 --- a/packages/firestore/src/core/view.ts +++ b/packages/firestore/src/core/view.ts @@ -271,17 +271,21 @@ export class View { * Updates the view with the given ViewDocumentChanges and optionally updates * limbo docs and sync state from the provided target change. * @param docChanges - The set of changes to make to the view's docs. - * @param updateLimboDocuments - Whether to update limbo documents based on + * @param limboResolutionEnabled - Whether to update limbo documents based on * this change. * @param targetChange - A target change to apply for computing limbo docs and * sync state. + * @param targetIsPendingReset - Whether the target is pending to reset due to + * existence filter mismatch. If not explicitly specified, it is treated + * equivalently to `false`. * @returns A new ViewChange with the given docs, changes, and sync state. */ // PORTING NOTE: The iOS/Android clients always compute limbo document changes. applyChanges( docChanges: ViewDocumentChanges, - updateLimboDocuments: boolean, - targetChange?: TargetChange + limboResolutionEnabled: boolean, + targetChange?: TargetChange, + targetIsPendingReset?: boolean ): ViewChange { debugAssert( !docChanges.needsRefill, @@ -300,10 +304,18 @@ export class View { }); this.applyTargetChange(targetChange); - const limboChanges = updateLimboDocuments - ? this.updateLimboDocuments() - : []; - const synced = this.limboDocuments.size === 0 && this.current; + + targetIsPendingReset = targetIsPendingReset ?? false; + const limboChanges = + limboResolutionEnabled && !targetIsPendingReset + ? this.updateLimboDocuments() + : []; + + // We are at synced state if there is no limbo docs are waiting to be resolved, view is current + // with the backend, and the query is not pending to reset due to existence filter mismatch. + const synced = + this.limboDocuments.size === 0 && this.current && !targetIsPendingReset; + const newSyncState = synced ? SyncState.Synced : SyncState.Local; const syncStateChanged = newSyncState !== this.syncState; this.syncState = newSyncState; @@ -350,7 +362,7 @@ export class View { mutatedKeys: this.mutatedKeys, needsRefill: false }, - /* updateLimboDocuments= */ false + /* limboResolutionEnabled= */ false ); } else { // No effect, just return a no-op ViewChange. @@ -458,7 +470,7 @@ export class View { this._syncedDocuments = queryResult.remoteKeys; this.limboDocuments = documentKeySet(); const docChanges = this.computeDocChanges(queryResult.documents); - return this.applyChanges(docChanges, /*updateLimboDocuments=*/ true); + return this.applyChanges(docChanges, /* limboResolutionEnabled= */ true); } /** diff --git a/packages/firestore/test/unit/core/view.test.ts b/packages/firestore/test/unit/core/view.test.ts index b47cc0ea337..7e9f61bd6a3 100644 --- a/packages/firestore/test/unit/core/view.test.ts +++ b/packages/firestore/test/unit/core/view.test.ts @@ -304,19 +304,19 @@ describe('View', () => { let changes = view.computeDocChanges(documentUpdates(doc1)); let viewChange = view.applyChanges( changes, - /* updateLimboDocuments= */ true + /* limboResolutionEnabled= */ true ); expect(viewChange.snapshot!.fromCache).to.be.true; // Add doc2 to generate a snapshot. Doc1 is still missing. changes = view.computeDocChanges(documentUpdates(doc2)); - viewChange = view.applyChanges(changes, /* updateLimboDocuments= */ true); + viewChange = view.applyChanges(changes, /* limboResolutionEnabled= */ true); expect(viewChange.snapshot!.fromCache).to.be.true; // Add doc2 to the backend's result set. viewChange = view.applyChanges( changes, - /* updateLimboDocuments= */ true, + /* limboResolutionEnabled= */ true, updateMapping(version(0), [doc2], [], [], /* current= */ true) ); // We are CURRENT but doc1 is in limbo. @@ -325,7 +325,7 @@ describe('View', () => { // Add doc1 to the backend's result set. viewChange = view.applyChanges( changes, - /* updateLimboDocuments= */ true, + /* limboResolutionEnabled= */ true, updateMapping(version(0), [doc1], [], [], /* current= */ true) ); expect(viewChange.snapshot!.fromCache).to.be.false; diff --git a/packages/firestore/test/unit/local/query_engine.test.ts b/packages/firestore/test/unit/local/query_engine.test.ts index bf8ada916ef..d65626acf53 100644 --- a/packages/firestore/test/unit/local/query_engine.test.ts +++ b/packages/firestore/test/unit/local/query_engine.test.ts @@ -253,7 +253,7 @@ function genericQueryEngineTest( const viewDocChanges = view.computeDocChanges(docs); return view.applyChanges( viewDocChanges, - /*updateLimboDocuments=*/ true + /* limboResolutionEnabled= */ true ).snapshot!.docs; }); }); diff --git a/packages/firestore/test/unit/specs/limbo_spec.test.ts b/packages/firestore/test/unit/specs/limbo_spec.test.ts index 5d1c978b101..0a4052cc72b 100644 --- a/packages/firestore/test/unit/specs/limbo_spec.test.ts +++ b/packages/firestore/test/unit/specs/limbo_spec.test.ts @@ -889,6 +889,7 @@ describeSpec('Limbo Documents:', [], () => { // to just send the docBs with an existence filter with a count of 3. .watchSends({ affects: [query1] }, docB1, docB2, docB3) .watchFilters([query1], [docB1.key, docB2.key, docB3.key]) + .watchCurrents(query1, 'resume-token-1001') .watchSnapshots(1001) .expectEvents(query1, { added: [docB1, docB2, docB3], @@ -927,6 +928,46 @@ describeSpec('Limbo Documents:', [], () => { } ); + // Regression test for the bug in https://github.com/firebase/firebase-android-sdk/issues/5357 + specTest( + 'Limbo resolution should wait for full re-query result if there is an existence filter mismatch ', + [], + () => { + const query1 = query('collection'); + const docA = doc('collection/a', 1000, { v: 1 }); + const docB = doc('collection/b', 1000, { v: 2 }); + return ( + spec() + .userListens(query1) + .watchAcksFull(query1, 1000, docA, docB) + .expectEvents(query1, { added: [docA, docB] }) + // Simulate that the client loses network connection. + .disableNetwork() + .expectEvents(query1, { fromCache: true }) + .enableNetwork() + .restoreListen(query1, 'resume-token-1000') + .watchAcks(query1) + // DocB is deleted in the next sync. + .watchFilters([query1], [docA.key]) + .watchCurrents(query1, 'resume-token-2000') + .watchSnapshots(2000) + .expectActiveTargets({ + query: query1, + resumeToken: '', + targetPurpose: TargetPurpose.ExistenceFilterMismatch + }) + .watchRemoves(query1) + .watchAcksFull(query1, 3000, docA) + // Only the deleted doc is moved to limbo after re-query result. + .expectLimboDocs(docB.key) + .ackLimbo(3000, deletedDoc(docB.key.toString(), 3000)) + .expectLimboDocs() + .expectEvents(query1, { + removed: [docB] + }) + ); + } + ); specTest( 'Limbo resolution throttling with bloom filter application', [], @@ -968,6 +1009,7 @@ describeSpec('Limbo Documents:', [], () => { [docB1.key, docB2.key, docB3.key], bloomFilterProto ) + .watchCurrents(query1, 'resume-token-1001') .watchSnapshots(1001) .expectEvents(query1, { added: [docB1, docB2, docB3], @@ -978,8 +1020,6 @@ describeSpec('Limbo Documents:', [], () => { // existence filter mismatch. Bloom filter checks membership of the // docs, and filters out docAs, while docBs returns true. Number of // existing docs matches the expected count, so skip the re-query. - .watchCurrents(query1, 'resume-token-1002') - .watchSnapshots(1002) // The docAs are now in limbo; the client begins limbo resolution. .expectLimboDocs(docA1.key, docA2.key) .expectEnqueuedLimboDocs(docA3.key) diff --git a/packages/firestore/test/unit/specs/limit_spec.test.ts b/packages/firestore/test/unit/specs/limit_spec.test.ts index bff90c4a324..4788bd4e93d 100644 --- a/packages/firestore/test/unit/specs/limit_spec.test.ts +++ b/packages/firestore/test/unit/specs/limit_spec.test.ts @@ -343,6 +343,7 @@ describeSpec('Limits:', [], () => { // out of sync. .watchSends({ affects: [limitQuery] }, secondDocument) .watchFilters([limitQuery], [secondDocument.key]) + .watchCurrents(limitQuery, 'resume-token-1004') .watchSnapshots(1004) .expectActiveTargets({ query: limitQuery,