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 2e994f6436e..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,13 +1081,13 @@ async function applyDocChanges( const targetChange = remoteEvent && remoteEvent.targetChanges.get(queryView.targetId); - const waitForRequeryResult = + const targetIsPendingReset = remoteEvent && remoteEvent.targetMismatches.get(queryView.targetId) != null; const viewChange = queryView.view.applyChanges( viewDocChanges, - /* updateLimboDocuments= */ syncEngineImpl.isPrimaryClient, + /* limboResolutionEnabled= */ syncEngineImpl.isPrimaryClient, targetChange, - waitForRequeryResult + targetIsPendingReset ); updateTrackedLimbos( syncEngineImpl, diff --git a/packages/firestore/src/core/view.ts b/packages/firestore/src/core/view.ts index 8897d33abc9..7784b82021d 100644 --- a/packages/firestore/src/core/view.ts +++ b/packages/firestore/src/core/view.ts @@ -271,20 +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 waitForRequeryResult - Whether the target is pending to run a full - * re-query due to existence filter mismatch. + * @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, + limboResolutionEnabled: boolean, targetChange?: TargetChange, - waitForRequeryResult?: boolean + targetIsPendingReset?: boolean ): ViewChange { debugAssert( !docChanges.needsRefill, @@ -303,15 +304,17 @@ export class View { }); this.applyTargetChange(targetChange); - const limboChanges = updateLimboDocuments - ? this.updateLimboDocuments(waitForRequeryResult ?? false) - : []; + + 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 for full re-query result due to existence - // filter mismatch. + // with the backend, and the query is not pending to reset due to existence filter mismatch. const synced = - this.limboDocuments.size === 0 && this.current && !waitForRequeryResult; + this.limboDocuments.size === 0 && this.current && !targetIsPendingReset; const newSyncState = synced ? SyncState.Synced : SyncState.Local; const syncStateChanged = newSyncState !== this.syncState; @@ -359,7 +362,7 @@ export class View { mutatedKeys: this.mutatedKeys, needsRefill: false }, - /* updateLimboDocuments= */ false + /* limboResolutionEnabled= */ false ); } else { // No effect, just return a no-op ViewChange. @@ -412,11 +415,9 @@ export class View { } } - private updateLimboDocuments( - waitForRequeryResult: boolean - ): LimboDocumentChange[] { + private updateLimboDocuments(): LimboDocumentChange[] { // We can only determine limbo documents when we're in-sync with the server. - if (waitForRequeryResult || !this.current) { + if (!this.current) { return []; } @@ -469,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 d048167fbfc..0a4052cc72b 100644 --- a/packages/firestore/test/unit/specs/limbo_spec.test.ts +++ b/packages/firestore/test/unit/specs/limbo_spec.test.ts @@ -928,7 +928,7 @@ describeSpec('Limbo Documents:', [], () => { } ); - // Reproduce the bug in b/238823695 + // 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 ', [], @@ -949,8 +949,6 @@ describeSpec('Limbo Documents:', [], () => { .watchAcks(query1) // DocB is deleted in the next sync. .watchFilters([query1], [docA.key]) - // Bugged behavior: Missing watchCurrent here will move - // all the docs to limbo unnecessarily. .watchCurrents(query1, 'resume-token-2000') .watchSnapshots(2000) .expectActiveTargets({ @@ -962,7 +960,7 @@ describeSpec('Limbo Documents:', [], () => { .watchAcksFull(query1, 3000, docA) // Only the deleted doc is moved to limbo after re-query result. .expectLimboDocs(docB.key) - .ackLimbo(3000, deletedDoc('collection/b', 3000)) + .ackLimbo(3000, deletedDoc(docB.key.toString(), 3000)) .expectLimboDocs() .expectEvents(query1, { removed: [docB]