From 2fec016c081b8413c91c7ee2d62a52818d0cc315 Mon Sep 17 00:00:00 2001 From: Mark Duckworth <1124037+MarkDuckworth@users.noreply.github.com> Date: Wed, 23 Oct 2024 12:09:55 -0600 Subject: [PATCH 1/7] Spec test for 8474 --- .../test/unit/specs/collection_spec.test.ts | 103 +++++++++++++++++- .../firestore/test/unit/specs/spec_builder.ts | 6 + 2 files changed, 108 insertions(+), 1 deletion(-) diff --git a/packages/firestore/test/unit/specs/collection_spec.test.ts b/packages/firestore/test/unit/specs/collection_spec.test.ts index 819894907d3..3eadd356ac6 100644 --- a/packages/firestore/test/unit/specs/collection_spec.test.ts +++ b/packages/firestore/test/unit/specs/collection_spec.test.ts @@ -15,12 +15,113 @@ * limitations under the License. */ -import { doc, query } from '../../util/helpers'; +import {doc, filter, query} from '../../util/helpers'; import { describeSpec, specTest } from './describe_spec'; import { spec } from './spec_builder'; +import {newQueryForPath} from "../../../src/core/query"; describeSpec('Collections:', [], () => { + + specTest('8474', ['exclusive', 'durable-persistence'], () => { + + // 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) + .watchSends({removed: [fullQuery]}, docC) + .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 + .watchSends({affects: [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 + .watchSends({removed: [filterQuery]}, docA) + .watchCurrents(filterQuery, 'resume-token-1004') + .watchSends({affects: [filterQuery]}, docC) + .watchCurrents(filterQuery, 'resume-token-1005') + .watchSends({removed: [filterQuery]}, docC) + .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, []) + + // UNEXPECTED docC is still in limbo + .expectLimboDocs(docC.key) + + // This causes snapshots for fullQuery and filterQuery + // to be raised as from cache + .expectEvents(fullQuery, { + fromCache: true, + modified: [docA2] + }) + + // getDocs(filterQuery) will not be resolved with this snapsho + // because it is from cache + .expectEvents(filterQuery, { + fromCache: true, + added: [docA2] + }) + + // 45-seconds later on heartbeat/global-snapshot + .watchSnapshots(1100, []) + // Now docC is out of limbo + .expectLimboDocs() + .expectEvents(fullQuery, { + fromCache: false, + removed: [docC] + }) + // Now getDocs(filterQuery) can be resolved + .expectEvents(filterQuery, { + fromCache: false, + removed: [docC] + }); + }); + specTest('Events are raised after watch ack', [], () => { const query1 = query('collection'); const doc1 = doc('collection/key', 1000, { foo: 'bar' }); diff --git a/packages/firestore/test/unit/specs/spec_builder.ts b/packages/firestore/test/unit/specs/spec_builder.ts index d79cca9cd82..313b6af4611 100644 --- a/packages/firestore/test/unit/specs/spec_builder.ts +++ b/packages/firestore/test/unit/specs/spec_builder.ts @@ -315,6 +315,12 @@ 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; From 14882282c283b2262c4924862acf46193a54a816 Mon Sep 17 00:00:00 2001 From: Mark Duckworth <1124037+MarkDuckworth@users.noreply.github.com> Date: Thu, 24 Oct 2024 15:35:10 -0600 Subject: [PATCH 2/7] Fix for 8474 --- packages/firestore/src/remote/watch_change.ts | 27 ++++- .../test/unit/specs/collection_spec.test.ts | 98 ++++++++++++++++--- 2 files changed, 109 insertions(+), 16 deletions(-) diff --git a/packages/firestore/src/remote/watch_change.ts b/packages/firestore/src/remote/watch_change.ts index 38e10a23e35..12496a46554 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(); @@ -596,7 +597,7 @@ export class WatchChangeAggregator { // 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,11 @@ 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 +731,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 +795,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/collection_spec.test.ts b/packages/firestore/test/unit/specs/collection_spec.test.ts index 3eadd356ac6..7f71a1ce956 100644 --- a/packages/firestore/test/unit/specs/collection_spec.test.ts +++ b/packages/firestore/test/unit/specs/collection_spec.test.ts @@ -15,15 +15,15 @@ * limitations under the License. */ -import {doc, filter, query} from '../../util/helpers'; +import {newQueryForPath} from "../../../src/core/query"; +import {deletedDoc, doc, filter, query} from '../../util/helpers'; import { describeSpec, specTest } from './describe_spec'; import { spec } from './spec_builder'; -import {newQueryForPath} from "../../../src/core/query"; describeSpec('Collections:', [], () => { - specTest('8474', ['exclusive', 'durable-persistence'], () => { + specTest('8474', [], () => { // onSnapshot(fullQuery) const fullQuery = query('collection'); @@ -90,36 +90,104 @@ describeSpec('Collections:', [], () => { // All changes are current and we get a global snapshot .watchSnapshots(1010, []) - // UNEXPECTED docC is still in limbo - .expectLimboDocs(docC.key) + // 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] + }); + }); + specTest('8474-deleted', [], () => { - // This causes snapshots for fullQuery and filterQuery - // to be raised as from cache + // 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 docCDeleted = deletedDoc('collection/c', 1009); + + 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) + .watchSends({removed: [fullQuery]}, docC) + .watchCurrents(fullQuery, 'resume-token-1002') + .watchSnapshots(1002) + .expectLimboDocs(docC.key) .expectEvents(fullQuery, { fromCache: true, - modified: [docA2] }) - // getDocs(filterQuery) will not be resolved with this snapsho - // because it is from cache + // User begins getDocs(filterQuery) + .userListensForGet(filterQuery) + + // getDocs(filterQuery) will not resolve on the snapshot from cache .expectEvents(filterQuery, { fromCache: true, - added: [docA2] + added: [docC] }) - // 45-seconds later on heartbeat/global-snapshot - .watchSnapshots(1100, []) + // Watch acks limbo and filter queries + .watchAcks(limboQueryC) + .watchAcks(filterQuery) + + // Watch responds to limboQueryC - docC was deleted + .watchSends({affects: [limboQueryC]}, docCDeleted) + .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 + .watchSends({removed: [filterQuery]}, docA) + .watchCurrents(filterQuery, 'resume-token-1004') + .watchSends({affects: [filterQuery]}, docC) + .watchCurrents(filterQuery, 'resume-token-1005') + .watchSends({removed: [filterQuery]}, docC) + .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] - }); + removed: [docC], + added: [docA2] + }) + + // No more expected events + .watchSnapshots(1100, []); }); specTest('Events are raised after watch ack', [], () => { From 3cb206e17a8abb215c404ae8f60f55a8b7837e96 Mon Sep 17 00:00:00 2001 From: Mark Duckworth <1124037+MarkDuckworth@users.noreply.github.com> Date: Thu, 24 Oct 2024 15:36:06 -0600 Subject: [PATCH 3/7] code cleanup --- packages/firestore/src/remote/watch_change.ts | 3 +- .../test/unit/specs/collection_spec.test.ts | 293 +++++++++--------- .../firestore/test/unit/specs/spec_builder.ts | 5 +- 3 files changed, 153 insertions(+), 148 deletions(-) diff --git a/packages/firestore/src/remote/watch_change.ts b/packages/firestore/src/remote/watch_change.ts index 12496a46554..e1a70f6b9ef 100644 --- a/packages/firestore/src/remote/watch_change.ts +++ b/packages/firestore/src/remote/watch_change.ts @@ -690,7 +690,8 @@ export class WatchChangeAggregator { this.pendingDocumentUpdatesByTarget = this.pendingDocumentUpdatesByTarget.insert( document.key, - this.ensureDocumentUpdateByTarget(document.key).add(targetId)); + this.ensureDocumentUpdateByTarget(document.key).add(targetId) + ); this.pendingDocumentTargetMapping = this.pendingDocumentTargetMapping.insert( diff --git a/packages/firestore/test/unit/specs/collection_spec.test.ts b/packages/firestore/test/unit/specs/collection_spec.test.ts index 7f71a1ce956..56bdcf23ef8 100644 --- a/packages/firestore/test/unit/specs/collection_spec.test.ts +++ b/packages/firestore/test/unit/specs/collection_spec.test.ts @@ -15,179 +15,180 @@ * limitations under the License. */ -import {newQueryForPath} from "../../../src/core/query"; -import {deletedDoc, doc, filter, query} from '../../util/helpers'; +import { newQueryForPath } from '../../../src/core/query'; +import { deletedDoc, doc, filter, query } from '../../util/helpers'; import { describeSpec, specTest } from './describe_spec'; import { spec } from './spec_builder'; describeSpec('Collections:', [], () => { - specTest('8474', [], () => { - // 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 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) - .watchSends({removed: [fullQuery]}, docC) - .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 - .watchSends({affects: [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 - .watchSends({removed: [filterQuery]}, docA) - .watchCurrents(filterQuery, 'resume-token-1004') - .watchSends({affects: [filterQuery]}, docC) - .watchCurrents(filterQuery, 'resume-token-1005') - .watchSends({removed: [filterQuery]}, docC) - .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] - }); + 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) + .watchSends({ removed: [fullQuery] }, docC) + .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 + .watchSends({ affects: [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 + .watchSends({ removed: [filterQuery] }, docA) + .watchCurrents(filterQuery, 'resume-token-1004') + .watchSends({ affects: [filterQuery] }, docC) + .watchCurrents(filterQuery, 'resume-token-1005') + .watchSends({ removed: [filterQuery] }, docC) + .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] + }) + ); }); specTest('8474-deleted', [], () => { - // 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 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 docCDeleted = deletedDoc('collection/c', 1009); 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) - .watchSends({removed: [fullQuery]}, docC) - .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 - .watchSends({affects: [limboQueryC]}, docCDeleted) - .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 - .watchSends({removed: [filterQuery]}, docA) - .watchCurrents(filterQuery, 'resume-token-1004') - .watchSends({affects: [filterQuery]}, docC) - .watchCurrents(filterQuery, 'resume-token-1005') - .watchSends({removed: [filterQuery]}, docC) - .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, []); + 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) + .watchSends({ removed: [fullQuery] }, docC) + .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 + .watchSends({ affects: [limboQueryC] }, docCDeleted) + .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 + .watchSends({ removed: [filterQuery] }, docA) + .watchCurrents(filterQuery, 'resume-token-1004') + .watchSends({ affects: [filterQuery] }, docC) + .watchCurrents(filterQuery, 'resume-token-1005') + .watchSends({ removed: [filterQuery] }, docC) + .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('Events are raised after watch ack', [], () => { diff --git a/packages/firestore/test/unit/specs/spec_builder.ts b/packages/firestore/test/unit/specs/spec_builder.ts index 313b6af4611..ccd5436d8d1 100644 --- a/packages/firestore/test/unit/specs/spec_builder.ts +++ b/packages/firestore/test/unit/specs/spec_builder.ts @@ -317,7 +317,10 @@ export class SpecBuilder { /** 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 }); + this.addUserListenStep(query, resume, { + includeMetadataChanges: true, + waitForSyncWhenOnline: true + }); return this; } From 39721dfc62089f7e3c299ef30b3adeeba09ec27a Mon Sep 17 00:00:00 2001 From: Mark Duckworth <1124037+MarkDuckworth@users.noreply.github.com> Date: Thu, 24 Oct 2024 16:42:29 -0600 Subject: [PATCH 4/7] New test case and test re-org --- .../test/unit/specs/collection_spec.test.ts | 172 +---------- .../test/unit/specs/limbo_spec.test.ts | 274 ++++++++++++++++++ 2 files changed, 275 insertions(+), 171 deletions(-) diff --git a/packages/firestore/test/unit/specs/collection_spec.test.ts b/packages/firestore/test/unit/specs/collection_spec.test.ts index 56bdcf23ef8..819894907d3 100644 --- a/packages/firestore/test/unit/specs/collection_spec.test.ts +++ b/packages/firestore/test/unit/specs/collection_spec.test.ts @@ -15,182 +15,12 @@ * limitations under the License. */ -import { newQueryForPath } from '../../../src/core/query'; -import { deletedDoc, doc, filter, query } from '../../util/helpers'; +import { doc, query } from '../../util/helpers'; import { describeSpec, specTest } from './describe_spec'; import { spec } from './spec_builder'; describeSpec('Collections:', [], () => { - specTest('8474', [], () => { - // 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) - .watchSends({ removed: [fullQuery] }, docC) - .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 - .watchSends({ affects: [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 - .watchSends({ removed: [filterQuery] }, docA) - .watchCurrents(filterQuery, 'resume-token-1004') - .watchSends({ affects: [filterQuery] }, docC) - .watchCurrents(filterQuery, 'resume-token-1005') - .watchSends({ removed: [filterQuery] }, docC) - .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] - }) - ); - }); - specTest('8474-deleted', [], () => { - // 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 docCDeleted = deletedDoc('collection/c', 1009); - - 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) - .watchSends({ removed: [fullQuery] }, docC) - .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 - .watchSends({ affects: [limboQueryC] }, docCDeleted) - .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 - .watchSends({ removed: [filterQuery] }, docA) - .watchCurrents(filterQuery, 'resume-token-1004') - .watchSends({ affects: [filterQuery] }, docC) - .watchCurrents(filterQuery, 'resume-token-1005') - .watchSends({ removed: [filterQuery] }, docC) - .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('Events are raised after watch ack', [], () => { const query1 = query('collection'); const doc1 = doc('collection/key', 1000, { foo: 'bar' }); diff --git a/packages/firestore/test/unit/specs/limbo_spec.test.ts b/packages/firestore/test/unit/specs/limbo_spec.test.ts index 0a4052cc72b..acc2912d4df 100644 --- a/packages/firestore/test/unit/specs/limbo_spec.test.ts +++ b/packages/firestore/test/unit/specs/limbo_spec.test.ts @@ -1171,4 +1171,278 @@ describeSpec('Limbo Documents:', [], () => { ); } ); + + specTest( + 'Fix #8474 - Limbo resolution for documentC is removed even if documentC updates occurred earlier 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) + .watchSends({ removed: [fullQuery] }, docC) + .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 + .watchSends({ affects: [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 + .watchSends({ removed: [filterQuery] }, docA) + .watchCurrents(filterQuery, 'resume-token-1004') + .watchSends({ affects: [filterQuery] }, docC) + .watchCurrents(filterQuery, 'resume-token-1005') + .watchSends({ removed: [filterQuery] }, docC) + .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] + }) + ); + } + ); + + specTest('Fix #8474 - Watch sends deletedDoc on limbo resolution', [], () => { + // 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 docCDeleted = deletedDoc('collection/c', 1009); + + 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) + .watchSends({ removed: [fullQuery] }, docC) + .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 + .watchSends({ affects: [limboQueryC] }, docCDeleted) + .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 + .watchSends({ removed: [filterQuery] }, docA) + .watchCurrents(filterQuery, 'resume-token-1004') + .watchSends({ affects: [filterQuery] }, docC) + .watchCurrents(filterQuery, 'resume-token-1005') + .watchSends({ removed: [filterQuery] }, docC) + .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 docCDeleted = deletedDoc('collection/c', 1009); + + 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) + .watchSends({ removed: [fullQuery] }, docC) + .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 + .watchSends({ removed: [filterQuery] }, docA) + .watchCurrents(filterQuery, 'resume-token-1004') + .watchSends({ affects: [filterQuery] }, docC) + .watchCurrents(filterQuery, 'resume-token-1005') + .watchSends({ removed: [filterQuery] }, docC) + .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 + .watchSends({ affects: [limboQueryC] }, docCDeleted) + .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] + }) + ); + } + ); }); From 40d2b33ac71216b5bfd48f2fc3c94c25a63d268c Mon Sep 17 00:00:00 2001 From: Mark Duckworth <1124037+MarkDuckworth@users.noreply.github.com> Date: Fri, 25 Oct 2024 12:01:47 -0600 Subject: [PATCH 5/7] Test cleanup and better documentation. --- packages/firestore/src/remote/watch_change.ts | 6 +- .../test/unit/specs/limbo_spec.test.ts | 168 +++++++++--------- .../firestore/test/unit/specs/spec_builder.ts | 19 +- packages/firestore/test/util/helpers.ts | 13 +- 4 files changed, 118 insertions(+), 88 deletions(-) diff --git a/packages/firestore/src/remote/watch_change.ts b/packages/firestore/src/remote/watch_change.ts index e1a70f6b9ef..0c69163095f 100644 --- a/packages/firestore/src/remote/watch_change.ts +++ b/packages/firestore/src/remote/watch_change.ts @@ -588,9 +588,9 @@ 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 diff --git a/packages/firestore/test/unit/specs/limbo_spec.test.ts b/packages/firestore/test/unit/specs/limbo_spec.test.ts index acc2912d4df..f6043a7fc9b 100644 --- a/packages/firestore/test/unit/specs/limbo_spec.test.ts +++ b/packages/firestore/test/unit/specs/limbo_spec.test.ts @@ -1173,7 +1173,7 @@ describeSpec('Limbo Documents:', [], () => { ); specTest( - 'Fix #8474 - Limbo resolution for documentC is removed even if documentC updates occurred earlier in the global snapshot window', + '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) @@ -1199,7 +1199,7 @@ describeSpec('Limbo Documents:', [], () => { }) // docC was deleted, this puts docC in limbo and causes a snapshot from cache (metadata-only change) - .watchSends({ removed: [fullQuery] }, docC) + .watchRemovesDoc(docC.key, fullQuery) .watchCurrents(fullQuery, 'resume-token-1002') .watchSnapshots(1002) .expectLimboDocs(docC.key) @@ -1221,7 +1221,7 @@ describeSpec('Limbo Documents:', [], () => { .watchAcks(filterQuery) // Watch responds to limboQueryC - docC was deleted - .watchSends({ affects: [limboQueryC] }) + .watchDeletesDoc(docC.key, 1009, limboQueryC) .watchCurrents(limboQueryC, 'resume-token-1009') .watchSnapshots(1009, [limboQueryC, fullQuery]) @@ -1229,11 +1229,11 @@ describeSpec('Limbo Documents:', [], () => { .expectLimboDocs(docC.key) // Rapid events of document update and delete caused by application - .watchSends({ removed: [filterQuery] }, docA) + .watchRemovesDoc(docA.key, filterQuery) .watchCurrents(filterQuery, 'resume-token-1004') .watchSends({ affects: [filterQuery] }, docC) .watchCurrents(filterQuery, 'resume-token-1005') - .watchSends({ removed: [filterQuery] }, docC) + .watchRemovesDoc(docC.key, filterQuery) .watchSends({ affects: [filterQuery] }, docA2) .watchCurrents(filterQuery, 'resume-token-1007') @@ -1255,96 +1255,103 @@ describeSpec('Limbo Documents:', [], () => { removed: [docC], added: [docA2] }) + + // No more expected events + .watchSnapshots(1100, []) ); } ); - specTest('Fix #8474 - Watch sends deletedDoc on limbo resolution', [], () => { - // onSnapshot(fullQuery) - const fullQuery = query('collection'); + 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)); + // 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 docCDeleted = deletedDoc('collection/c', 1009); + 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); + 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] - }) + 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) - .watchSends({ removed: [fullQuery] }, docC) - .watchCurrents(fullQuery, 'resume-token-1002') - .watchSnapshots(1002) - .expectLimboDocs(docC.key) - .expectEvents(fullQuery, { - fromCache: true - }) + // 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) + // User begins getDocs(filterQuery) + .userListensForGet(filterQuery) - // getDocs(filterQuery) will not resolve on the snapshot from cache - .expectEvents(filterQuery, { - fromCache: true, - added: [docC] - }) + // 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 acks limbo and filter queries + .watchAcks(limboQueryC) + .watchAcks(filterQuery) - // Watch responds to limboQueryC - docC was deleted - .watchSends({ affects: [limboQueryC] }, docCDeleted) - .watchCurrents(limboQueryC, 'resume-token-1009') - .watchSnapshots(1009, [limboQueryC, fullQuery]) + // 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) + // 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 - .watchSends({ removed: [filterQuery] }, docA) - .watchCurrents(filterQuery, 'resume-token-1004') - .watchSends({ affects: [filterQuery] }, docC) - .watchCurrents(filterQuery, 'resume-token-1005') - .watchSends({ removed: [filterQuery] }, docC) - .watchSends({ affects: [filterQuery] }, docA2) - .watchCurrents(filterQuery, 'resume-token-1007') + // 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]) + .watchSnapshots(1010, [fullQuery, limboQueryC]) - // All changes are current and we get a global snapshot - .watchSnapshots(1010, []) + // 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] - }) + // 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, []) - ); - }); + // No more expected events + .watchSnapshots(1100, []) + ); + } + ); specTest( 'Fix #8474 - Handles code path of no ack for limbo resolution query before global snapshot', @@ -1359,7 +1366,6 @@ describeSpec('Limbo Documents:', [], () => { 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 docCDeleted = deletedDoc('collection/c', 1009); const limboQueryC = newQueryForPath(docC.key.path); @@ -1374,7 +1380,7 @@ describeSpec('Limbo Documents:', [], () => { }) // docC was deleted, this puts docC in limbo and causes a snapshot from cache (metadata-only change) - .watchSends({ removed: [fullQuery] }, docC) + .watchRemovesDoc(docC.key, fullQuery) .watchCurrents(fullQuery, 'resume-token-1002') .watchSnapshots(1002) .expectLimboDocs(docC.key) @@ -1398,11 +1404,11 @@ describeSpec('Limbo Documents:', [], () => { .expectLimboDocs(docC.key) // Rapid events of document update and delete caused by application - .watchSends({ removed: [filterQuery] }, docA) + .watchRemovesDoc(docA.key, filterQuery) .watchCurrents(filterQuery, 'resume-token-1004') .watchSends({ affects: [filterQuery] }, docC) .watchCurrents(filterQuery, 'resume-token-1005') - .watchSends({ removed: [filterQuery] }, docC) + .watchRemovesDoc(docC.key, filterQuery) .watchSends({ affects: [filterQuery] }, docA2) .watchCurrents(filterQuery, 'resume-token-1007') @@ -1425,7 +1431,7 @@ describeSpec('Limbo Documents:', [], () => { .watchAcks(limboQueryC) // Watch responds to limboQueryC - docC was deleted - .watchSends({ affects: [limboQueryC] }, docCDeleted) + .watchDeletesDoc(docC.key, 1009, limboQueryC) .watchCurrents(limboQueryC, 'resume-token-1009') .watchSnapshots(1100, [limboQueryC]) diff --git a/packages/firestore/test/unit/specs/spec_builder.ts b/packages/firestore/test/unit/specs/spec_builder.ts index ccd5436d8d1..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 { @@ -830,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( From ff3803da956a70341de805192ecf86b1b494c196 Mon Sep 17 00:00:00 2001 From: Mark Duckworth <1124037+MarkDuckworth@users.noreply.github.com> Date: Fri, 25 Oct 2024 12:16:47 -0600 Subject: [PATCH 6/7] Create tasty-boxes-brake.md --- .changeset/tasty-boxes-brake.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/tasty-boxes-brake.md diff --git a/.changeset/tasty-boxes-brake.md b/.changeset/tasty-boxes-brake.md new file mode 100644 index 00000000000..48f1b7dff38 --- /dev/null +++ b/.changeset/tasty-boxes-brake.md @@ -0,0 +1,5 @@ +--- +"@firebase/firestore": patch +--- + +Fix for Issue #8474 - Prevent a possible condition of slow snapshots, caused by a rapid series of document update(s) followed by a delete. From 91fa31533f933f129f61f77dc99f4e17ef279c66 Mon Sep 17 00:00:00 2001 From: Mark Duckworth <1124037+MarkDuckworth@users.noreply.github.com> Date: Fri, 25 Oct 2024 13:27:10 -0600 Subject: [PATCH 7/7] Update tasty-boxes-brake.md --- .changeset/tasty-boxes-brake.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changeset/tasty-boxes-brake.md b/.changeset/tasty-boxes-brake.md index 48f1b7dff38..beb8b626f36 100644 --- a/.changeset/tasty-boxes-brake.md +++ b/.changeset/tasty-boxes-brake.md @@ -2,4 +2,4 @@ "@firebase/firestore": patch --- -Fix for Issue #8474 - Prevent a possible condition of slow snapshots, caused by a rapid series of document update(s) followed by a delete. +Prevent a possible condition of slow snapshots, caused by a rapid series of document update(s) followed by a delete.