diff --git a/.changeset/tasty-boxes-brake.md b/.changeset/tasty-boxes-brake.md new file mode 100644 index 00000000000..beb8b626f36 --- /dev/null +++ b/.changeset/tasty-boxes-brake.md @@ -0,0 +1,5 @@ +--- +"@firebase/firestore": patch +--- + +Prevent a possible condition of slow snapshots, caused by a rapid series of document update(s) followed by a delete. diff --git a/packages/firestore/src/remote/watch_change.ts b/packages/firestore/src/remote/watch_change.ts index 38e10a23e35..0c69163095f 100644 --- a/packages/firestore/src/remote/watch_change.ts +++ b/packages/firestore/src/remote/watch_change.ts @@ -291,6 +291,7 @@ export class WatchChangeAggregator { /** Keeps track of the documents to update since the last raised snapshot. */ private pendingDocumentUpdates = mutableDocumentMap(); + private pendingDocumentUpdatesByTarget = documentTargetMap(); /** A mapping of document keys to their set of target IDs. */ private pendingDocumentTargetMapping = documentTargetMap(); @@ -587,16 +588,16 @@ export class WatchChangeAggregator { if (targetState.current && targetIsDocumentTarget(targetData.target)) { // Document queries for document that don't exist can produce an empty // result set. To update our local cache, we synthesize a document - // delete if we have not previously received the document. This - // resolves the limbo state of the document, removing it from - // limboDocumentRefs. + // delete if we have not previously received the document for this + // target. This resolves the limbo state of the document, removing it + // from limboDocumentRefs. // // TODO(dimond): Ideally we would have an explicit lookup target // instead resulting in an explicit delete message and we could // remove this special logic. const key = new DocumentKey(targetData.target.path); if ( - this.pendingDocumentUpdates.get(key) === null && + !this.ensureDocumentUpdateByTarget(key).has(targetId) && !this.targetContainsDocument(targetId, key) ) { this.removeDocumentFromTarget( @@ -655,6 +656,7 @@ export class WatchChangeAggregator { ); this.pendingDocumentUpdates = mutableDocumentMap(); + this.pendingDocumentUpdatesByTarget = documentTargetMap(); this.pendingDocumentTargetMapping = documentTargetMap(); this.pendingTargetResets = new SortedMap( primitiveComparator @@ -685,6 +687,12 @@ export class WatchChangeAggregator { document ); + this.pendingDocumentUpdatesByTarget = + this.pendingDocumentUpdatesByTarget.insert( + document.key, + this.ensureDocumentUpdateByTarget(document.key).add(targetId) + ); + this.pendingDocumentTargetMapping = this.pendingDocumentTargetMapping.insert( document.key, @@ -724,6 +732,12 @@ export class WatchChangeAggregator { this.ensureDocumentTargetMapping(key).delete(targetId) ); + this.pendingDocumentTargetMapping = + this.pendingDocumentTargetMapping.insert( + key, + this.ensureDocumentTargetMapping(key).add(targetId) + ); + if (updatedDocument) { this.pendingDocumentUpdates = this.pendingDocumentUpdates.insert( key, @@ -782,6 +796,18 @@ export class WatchChangeAggregator { return targetMapping; } + private ensureDocumentUpdateByTarget(key: DocumentKey): SortedSet { + let targetMapping = this.pendingDocumentUpdatesByTarget.get(key); + + if (!targetMapping) { + targetMapping = new SortedSet(primitiveComparator); + this.pendingDocumentUpdatesByTarget = + this.pendingDocumentUpdatesByTarget.insert(key, targetMapping); + } + + return targetMapping; + } + /** * Verifies that the user is still interested in this target (by calling * `getTargetDataForTarget()`) and that we are not waiting for pending ADDs diff --git a/packages/firestore/test/unit/specs/limbo_spec.test.ts b/packages/firestore/test/unit/specs/limbo_spec.test.ts index 0a4052cc72b..f6043a7fc9b 100644 --- a/packages/firestore/test/unit/specs/limbo_spec.test.ts +++ b/packages/firestore/test/unit/specs/limbo_spec.test.ts @@ -1171,4 +1171,284 @@ describeSpec('Limbo Documents:', [], () => { ); } ); + + specTest( + 'Fix #8474 - Limbo resolution for document is removed even if document updates for the document occurred before documentDelete in the global snapshot window', + [], + () => { + // onSnapshot(fullQuery) + const fullQuery = query('collection'); + + // getDocs(filterQuery) + const filterQuery = query('collection', filter('included', '==', true)); + + const docA = doc('collection/a', 1000, { key: 'a', included: false }); + const docA2 = doc('collection/a', 1007, { key: 'a', included: true }); + const docC = doc('collection/c', 1002, { key: 'c', included: true }); + + const limboQueryC = newQueryForPath(docC.key.path); + + return ( + spec() + // onSnapshot(fullQuery) - fullQuery is listening to documents in the collection for the full test + .userListens(fullQuery) + .watchAcksFull(fullQuery, 1001, docA, docC) + .expectEvents(fullQuery, { + fromCache: false, + added: [docA, docC] + }) + + // docC was deleted, this puts docC in limbo and causes a snapshot from cache (metadata-only change) + .watchRemovesDoc(docC.key, fullQuery) + .watchCurrents(fullQuery, 'resume-token-1002') + .watchSnapshots(1002) + .expectLimboDocs(docC.key) + .expectEvents(fullQuery, { + fromCache: true + }) + + // User begins getDocs(filterQuery) + .userListensForGet(filterQuery) + + // getDocs(filterQuery) will not resolve on the snapshot from cache + .expectEvents(filterQuery, { + fromCache: true, + added: [docC] + }) + + // Watch acks limbo and filter queries + .watchAcks(limboQueryC) + .watchAcks(filterQuery) + + // Watch responds to limboQueryC - docC was deleted + .watchDeletesDoc(docC.key, 1009, limboQueryC) + .watchCurrents(limboQueryC, 'resume-token-1009') + .watchSnapshots(1009, [limboQueryC, fullQuery]) + + // However, docC is still in limbo because there has not been a global snapshot + .expectLimboDocs(docC.key) + + // Rapid events of document update and delete caused by application + .watchRemovesDoc(docA.key, filterQuery) + .watchCurrents(filterQuery, 'resume-token-1004') + .watchSends({ affects: [filterQuery] }, docC) + .watchCurrents(filterQuery, 'resume-token-1005') + .watchRemovesDoc(docC.key, filterQuery) + .watchSends({ affects: [filterQuery] }, docA2) + .watchCurrents(filterQuery, 'resume-token-1007') + + .watchSnapshots(1010, [fullQuery, limboQueryC]) + + // All changes are current and we get a global snapshot + .watchSnapshots(1010, []) + + // Now docC is out of limbo + .expectLimboDocs() + .expectEvents(fullQuery, { + fromCache: false, + modified: [docA2], + removed: [docC] + }) + // Now getDocs(filterQuery) can be resolved + .expectEvents(filterQuery, { + fromCache: false, + removed: [docC], + added: [docA2] + }) + + // No more expected events + .watchSnapshots(1100, []) + ); + } + ); + + specTest( + 'Fix #8474 - Limbo resolution for document is removed even if document updates for the document occurred in the global snapshot window and no document delete was received for the limbo resolution query', + [], + () => { + // onSnapshot(fullQuery) + const fullQuery = query('collection'); + + // getDocs(filterQuery) + const filterQuery = query('collection', filter('included', '==', true)); + + const docA = doc('collection/a', 1000, { key: 'a', included: false }); + const docA2 = doc('collection/a', 1007, { key: 'a', included: true }); + const docC = doc('collection/c', 1002, { key: 'c', included: true }); + + const limboQueryC = newQueryForPath(docC.key.path); + + return ( + spec() + // onSnapshot(fullQuery) - fullQuery is listening to documents in the collection for the full test + .userListens(fullQuery) + .watchAcksFull(fullQuery, 1001, docA, docC) + .expectEvents(fullQuery, { + fromCache: false, + added: [docA, docC] + }) + + // docC was deleted, this puts docC in limbo and causes a snapshot from cache (metadata-only change) + .watchRemovesDoc(docC.key, fullQuery) + .watchCurrents(fullQuery, 'resume-token-1002') + .watchSnapshots(1002) + .expectLimboDocs(docC.key) + .expectEvents(fullQuery, { + fromCache: true + }) + + // User begins getDocs(filterQuery) + .userListensForGet(filterQuery) + + // getDocs(filterQuery) will not resolve on the snapshot from cache + .expectEvents(filterQuery, { + fromCache: true, + added: [docC] + }) + + // Watch acks limbo and filter queries + .watchAcks(limboQueryC) + .watchAcks(filterQuery) + + // Watch currents the limbo query, but did not send a document delete. + // This is and unexpected code path, but something that is called + // out as possible in the watch change aggregator. + .watchCurrents(limboQueryC, 'resume-token-1009') + .watchSnapshots(1009, [limboQueryC, fullQuery]) + + // However, docC is still in limbo because there has not been a global snapshot + .expectLimboDocs(docC.key) + + // Rapid events of document update and delete caused by application + .watchRemovesDoc(docA.key, filterQuery) + .watchCurrents(filterQuery, 'resume-token-1004') + .watchSends({ affects: [filterQuery] }, docC) + .watchCurrents(filterQuery, 'resume-token-1005') + .watchRemovesDoc(docC.key, filterQuery) + .watchSends({ affects: [filterQuery] }, docA2) + .watchCurrents(filterQuery, 'resume-token-1007') + + .watchSnapshots(1010, [fullQuery, limboQueryC]) + + // All changes are current and we get a global snapshot + .watchSnapshots(1010, []) + + // Now docC is out of limbo + .expectLimboDocs() + .expectEvents(fullQuery, { + fromCache: false, + modified: [docA2], + removed: [docC] + }) + // Now getDocs(filterQuery) can be resolved + .expectEvents(filterQuery, { + fromCache: false, + removed: [docC], + added: [docA2] + }) + + // No more expected events + .watchSnapshots(1100, []) + ); + } + ); + + specTest( + 'Fix #8474 - Handles code path of no ack for limbo resolution query before global snapshot', + [], + () => { + // onSnapshot(fullQuery) + const fullQuery = query('collection'); + + // getDocs(filterQuery) + const filterQuery = query('collection', filter('included', '==', true)); + + const docA = doc('collection/a', 1000, { key: 'a', included: false }); + const docA2 = doc('collection/a', 1007, { key: 'a', included: true }); + const docC = doc('collection/c', 1002, { key: 'c', included: true }); + + const limboQueryC = newQueryForPath(docC.key.path); + + return ( + spec() + // onSnapshot(fullQuery) - fullQuery is listening to documents in the collection for the full test + .userListens(fullQuery) + .watchAcksFull(fullQuery, 1001, docA, docC) + .expectEvents(fullQuery, { + fromCache: false, + added: [docA, docC] + }) + + // docC was deleted, this puts docC in limbo and causes a snapshot from cache (metadata-only change) + .watchRemovesDoc(docC.key, fullQuery) + .watchCurrents(fullQuery, 'resume-token-1002') + .watchSnapshots(1002) + .expectLimboDocs(docC.key) + .expectEvents(fullQuery, { + fromCache: true + }) + + // User begins getDocs(filterQuery) + .userListensForGet(filterQuery) + + // getDocs(filterQuery) will not resolve on the snapshot from cache + .expectEvents(filterQuery, { + fromCache: true, + added: [docC] + }) + + // Watch filter query + .watchAcks(filterQuery) + + // However, docC is still in limbo because there has not been a global snapshot + .expectLimboDocs(docC.key) + + // Rapid events of document update and delete caused by application + .watchRemovesDoc(docA.key, filterQuery) + .watchCurrents(filterQuery, 'resume-token-1004') + .watchSends({ affects: [filterQuery] }, docC) + .watchCurrents(filterQuery, 'resume-token-1005') + .watchRemovesDoc(docC.key, filterQuery) + .watchSends({ affects: [filterQuery] }, docA2) + .watchCurrents(filterQuery, 'resume-token-1007') + + .watchSnapshots(1010, [fullQuery, limboQueryC]) + + // All changes are current and we get a global snapshot + .watchSnapshots(1010, []) + + .expectEvents(fullQuery, { + fromCache: true, + modified: [docA2] + }) + // Now getDocs(filterQuery) can be resolved + .expectEvents(filterQuery, { + fromCache: true, + added: [docA2] + }) + + // Watch acks limbo query + .watchAcks(limboQueryC) + + // Watch responds to limboQueryC - docC was deleted + .watchDeletesDoc(docC.key, 1009, limboQueryC) + .watchCurrents(limboQueryC, 'resume-token-1009') + .watchSnapshots(1100, [limboQueryC]) + + // No more expected events + .watchSnapshots(1101, []) + + .expectLimboDocs() + .expectEvents(fullQuery, { + fromCache: false, + removed: [docC] + }) + // Now getDocs(filterQuery) can be resolved + .expectEvents(filterQuery, { + fromCache: false, + removed: [docC] + }) + ); + } + ); }); diff --git a/packages/firestore/test/unit/specs/spec_builder.ts b/packages/firestore/test/unit/specs/spec_builder.ts index d79cca9cd82..80dcd6519de 100644 --- a/packages/firestore/test/unit/specs/spec_builder.ts +++ b/packages/firestore/test/unit/specs/spec_builder.ts @@ -51,7 +51,7 @@ import { forEach } from '../../../src/util/obj'; import { ObjectMap } from '../../../src/util/obj_map'; import { isNullOrUndefined } from '../../../src/util/types'; import { firestore } from '../../util/api_helpers'; -import { TestSnapshotVersion } from '../../util/helpers'; +import { deletedDoc, TestSnapshotVersion } from '../../util/helpers'; import { RpcError } from './spec_rpc_error'; import { @@ -315,6 +315,15 @@ export class SpecBuilder { return this; } + /** Listen to query using the same options as executing a getDoc or getDocs */ + userListensForGet(query: Query, resume?: ResumeSpec): this { + this.addUserListenStep(query, resume, { + includeMetadataChanges: true, + waitForSyncWhenOnline: true + }); + return this; + } + userListensToCache(query: Query, resume?: ResumeSpec): this { this.addUserListenStep(query, resume, { source: Source.Cache }); return this; @@ -821,6 +830,23 @@ export class SpecBuilder { return this; } + watchDeletesDoc( + key: DocumentKey, + version: TestSnapshotVersion, + ...targets: Query[] + ): this { + this.nextStep(); + this.currentStep = { + watchEntity: { + doc: SpecBuilder.docToSpec( + deletedDoc(key.path.canonicalString(), version) + ), + removedTargets: targets.map(query => this.getTargetId(query)) + } + }; + return this; + } + watchFilters( queries: Query[], docs: DocumentKey[] = [], diff --git a/packages/firestore/test/util/helpers.ts b/packages/firestore/test/util/helpers.ts index dded004ebfd..c5865c3e0f7 100644 --- a/packages/firestore/test/util/helpers.ts +++ b/packages/firestore/test/util/helpers.ts @@ -172,12 +172,19 @@ export function doc( } export function deletedDoc( - keyStr: string, + keyStrOrDocumentKey: string | DocumentKey, ver: TestSnapshotVersion ): MutableDocument { - return MutableDocument.newNoDocument(key(keyStr), version(ver)).setReadTime( + if ( + keyStrOrDocumentKey instanceof String || + typeof keyStrOrDocumentKey === 'string' + ) { + keyStrOrDocumentKey = key(keyStrOrDocumentKey as string); + } + return MutableDocument.newNoDocument( + keyStrOrDocumentKey, version(ver) - ); + ).setReadTime(version(ver)); } export function unknownDoc(