From 8c5bccc160c7eb97cdd3b9c1088f7f4d19ab45e5 Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Tue, 14 May 2024 10:37:22 -0400 Subject: [PATCH 1/5] fix the multi-tab issue and add spec tests --- .../firestore/src/core/sync_engine_impl.ts | 3 +- .../firestore/src/local/local_store_impl.ts | 4 +- packages/firestore/src/model/field_index.ts | 15 ++++ .../test/unit/specs/query_spec.test.ts | 80 ++++++++++++++++++- 4 files changed, 98 insertions(+), 4 deletions(-) diff --git a/packages/firestore/src/core/sync_engine_impl.ts b/packages/firestore/src/core/sync_engine_impl.ts index f4db6f4a5bd..bba07f4f4bc 100644 --- a/packages/firestore/src/core/sync_engine_impl.ts +++ b/packages/firestore/src/core/sync_engine_impl.ts @@ -1095,9 +1095,10 @@ export async function syncEngineEmitNewSnapsAndNotifyLocalStore( // secondary clients to update query state. if (viewSnapshot || remoteEvent) { if (syncEngineImpl.isPrimaryClient) { + const isCurrent = viewSnapshot && !viewSnapshot.fromCache; syncEngineImpl.sharedClientState.updateQueryState( queryView.targetId, - viewSnapshot?.fromCache ? 'not-current' : 'current' + isCurrent ? 'current' : 'not-current' ); } } diff --git a/packages/firestore/src/local/local_store_impl.ts b/packages/firestore/src/local/local_store_impl.ts index 56f2b96f8d1..9894ae8c6e8 100644 --- a/packages/firestore/src/local/local_store_impl.ts +++ b/packages/firestore/src/local/local_store_impl.ts @@ -42,7 +42,7 @@ import { FieldIndex, fieldIndexSemanticComparator, INITIAL_LARGEST_BATCH_ID, - newIndexOffsetSuccessorFromReadTime + newIndexOffsetFromReadTime } from '../model/field_index'; import { mutationExtractBaseValue, @@ -1260,7 +1260,7 @@ export function localStoreGetNewDocumentChanges( localStoreImpl.remoteDocuments.getAllFromCollectionGroup( txn, collectionGroup, - newIndexOffsetSuccessorFromReadTime(readTime, INITIAL_LARGEST_BATCH_ID), + newIndexOffsetFromReadTime(readTime, INITIAL_LARGEST_BATCH_ID), /* limit= */ Number.MAX_SAFE_INTEGER ) ) diff --git a/packages/firestore/src/model/field_index.ts b/packages/firestore/src/model/field_index.ts index 9448c180a93..25b00f6eefb 100644 --- a/packages/firestore/src/model/field_index.ts +++ b/packages/firestore/src/model/field_index.ts @@ -177,6 +177,21 @@ export class IndexState { } } +/** + * Creates an offset that matches all documents with a read time starting from + * the `readTime`. + */ +export function newIndexOffsetFromReadTime( + readTime: SnapshotVersion, + largestBatchId: number +): IndexOffset { + return new IndexOffset( + SnapshotVersion.fromTimestamp(readTime.toTimestamp()), + DocumentKey.empty(), + largestBatchId + ); +} + /** * Creates an offset that matches all documents with a read time higher than * `readTime`. diff --git a/packages/firestore/test/unit/specs/query_spec.test.ts b/packages/firestore/test/unit/specs/query_spec.test.ts index 5e9736db95b..efd9b90039e 100644 --- a/packages/firestore/test/unit/specs/query_spec.test.ts +++ b/packages/firestore/test/unit/specs/query_spec.test.ts @@ -24,7 +24,7 @@ import { Document } from '../../../src/model/document'; import { doc, filter, query } from '../../util/helpers'; import { describeSpec, specTest } from './describe_spec'; -import { spec, SpecBuilder } from './spec_builder'; +import { client, spec, SpecBuilder } from './spec_builder'; // Helper to seed the cache with the specified docs by listening to each one. function specWithCachedDocs(...docs: Document[]): SpecBuilder { @@ -136,4 +136,82 @@ describeSpec('Queries:', [], () => { ); } ); + + specTest( + 'Queries in different tabs will not interferer', + ['multi-client'], + () => { + const query1 = query('collection', filter('key', '==', 'a')); + const query2 = query('collection', filter('key', '==', 'b')); + const docA = doc('collection/a', 1000, { key: 'a' }); + const docB = doc('collection/b', 1000, { key: 'b' }); + + return ( + client(0) + .becomeVisible() + // Listen to the first query in the primary client + .expectPrimaryState(true) + .userListens(query1) + .watchAcks(query1) + .watchSends({ affects: [query1] }, docA) + + // Listen to different query in the secondary client + .client(1) + .userListens(query2) + + .client(0) + .expectListen(query2) + .watchCurrents(query1, 'resume-token-1000') + // Receive global snapshot before the second query is acknowledged + .watchSnapshots(1000) + .expectEvents(query1, { added: [docA] }) + // This should not trigger empty snapshot for second query(bugged behavior) + .client(1) + .client(0) + .watchAcksFull(query2, 2000, docB) + .client(1) + .expectEvents(query2, { added: [docB] }) + ); + } + ); + + specTest( + 'Multiple queries on same collection group', + ['multi-client'], + () => { + const cgQuery = newQueryForCollectionGroup('cg'); + const cgQuery1 = queryWithAddedFilter(cgQuery, filter('val', '==', 1)); + const cgQuery2 = queryWithAddedFilter(cgQuery, filter('val', '!=', 1)); + const docs = [ + doc('cg/1', 1000, { val: 1 }), + doc('cg/2', 1000, { val: 2 }), + doc('not-cg/nope', 1000, { val: 1 }), + doc('cg/1/not-cg/nope', 1000, { val: 1 }) + ]; + return ( + client(0) + .expectPrimaryState(true) + // Listen to different queries on the same collection group + .client(1) + .userListens(cgQuery1) + .userListens(cgQuery2) + + .client(0) + .expectListen(cgQuery1) + .expectListen(cgQuery2) + .watchAcks(cgQuery1) + .watchSends({ affects: [cgQuery1] }, docs[0]) + .watchCurrents(cgQuery1, 'resume-token-2000') + .watchAcks(cgQuery2) + .watchSends({ affects: [cgQuery2] }, docs[1]) + .watchCurrents(cgQuery2, 'resume-token-2000') + .watchSnapshots(2000) + + .client(1) + .expectEvents(cgQuery1, { added: [docs[0]] }) + .expectEvents(cgQuery2, { added: [docs[1]], fromCache: true }) + .expectEvents(cgQuery2, { added: [] }) + ); + } + ); }); From 841ccbec3f294627e3efa888b7fc02266c55f2ea Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Tue, 14 May 2024 10:46:21 -0400 Subject: [PATCH 2/5] remove new index offset --- .../firestore/src/local/local_store_impl.ts | 4 +- packages/firestore/src/model/field_index.ts | 15 ------- .../test/unit/specs/query_spec.test.ts | 40 ------------------- 3 files changed, 2 insertions(+), 57 deletions(-) diff --git a/packages/firestore/src/local/local_store_impl.ts b/packages/firestore/src/local/local_store_impl.ts index 9894ae8c6e8..56f2b96f8d1 100644 --- a/packages/firestore/src/local/local_store_impl.ts +++ b/packages/firestore/src/local/local_store_impl.ts @@ -42,7 +42,7 @@ import { FieldIndex, fieldIndexSemanticComparator, INITIAL_LARGEST_BATCH_ID, - newIndexOffsetFromReadTime + newIndexOffsetSuccessorFromReadTime } from '../model/field_index'; import { mutationExtractBaseValue, @@ -1260,7 +1260,7 @@ export function localStoreGetNewDocumentChanges( localStoreImpl.remoteDocuments.getAllFromCollectionGroup( txn, collectionGroup, - newIndexOffsetFromReadTime(readTime, INITIAL_LARGEST_BATCH_ID), + newIndexOffsetSuccessorFromReadTime(readTime, INITIAL_LARGEST_BATCH_ID), /* limit= */ Number.MAX_SAFE_INTEGER ) ) diff --git a/packages/firestore/src/model/field_index.ts b/packages/firestore/src/model/field_index.ts index 25b00f6eefb..9448c180a93 100644 --- a/packages/firestore/src/model/field_index.ts +++ b/packages/firestore/src/model/field_index.ts @@ -177,21 +177,6 @@ export class IndexState { } } -/** - * Creates an offset that matches all documents with a read time starting from - * the `readTime`. - */ -export function newIndexOffsetFromReadTime( - readTime: SnapshotVersion, - largestBatchId: number -): IndexOffset { - return new IndexOffset( - SnapshotVersion.fromTimestamp(readTime.toTimestamp()), - DocumentKey.empty(), - largestBatchId - ); -} - /** * Creates an offset that matches all documents with a read time higher than * `readTime`. diff --git a/packages/firestore/test/unit/specs/query_spec.test.ts b/packages/firestore/test/unit/specs/query_spec.test.ts index efd9b90039e..95b808cae67 100644 --- a/packages/firestore/test/unit/specs/query_spec.test.ts +++ b/packages/firestore/test/unit/specs/query_spec.test.ts @@ -174,44 +174,4 @@ describeSpec('Queries:', [], () => { ); } ); - - specTest( - 'Multiple queries on same collection group', - ['multi-client'], - () => { - const cgQuery = newQueryForCollectionGroup('cg'); - const cgQuery1 = queryWithAddedFilter(cgQuery, filter('val', '==', 1)); - const cgQuery2 = queryWithAddedFilter(cgQuery, filter('val', '!=', 1)); - const docs = [ - doc('cg/1', 1000, { val: 1 }), - doc('cg/2', 1000, { val: 2 }), - doc('not-cg/nope', 1000, { val: 1 }), - doc('cg/1/not-cg/nope', 1000, { val: 1 }) - ]; - return ( - client(0) - .expectPrimaryState(true) - // Listen to different queries on the same collection group - .client(1) - .userListens(cgQuery1) - .userListens(cgQuery2) - - .client(0) - .expectListen(cgQuery1) - .expectListen(cgQuery2) - .watchAcks(cgQuery1) - .watchSends({ affects: [cgQuery1] }, docs[0]) - .watchCurrents(cgQuery1, 'resume-token-2000') - .watchAcks(cgQuery2) - .watchSends({ affects: [cgQuery2] }, docs[1]) - .watchCurrents(cgQuery2, 'resume-token-2000') - .watchSnapshots(2000) - - .client(1) - .expectEvents(cgQuery1, { added: [docs[0]] }) - .expectEvents(cgQuery2, { added: [docs[1]], fromCache: true }) - .expectEvents(cgQuery2, { added: [] }) - ); - } - ); }); From 748c93a3a6ea3243bac66c716ea757ee70cf9455 Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Tue, 14 May 2024 10:50:42 -0400 Subject: [PATCH 3/5] add changeset --- .changeset/nine-rings-jog.md | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 .changeset/nine-rings-jog.md diff --git a/.changeset/nine-rings-jog.md b/.changeset/nine-rings-jog.md new file mode 100644 index 00000000000..5792292f026 --- /dev/null +++ b/.changeset/nine-rings-jog.md @@ -0,0 +1,6 @@ +--- +'@firebase/firestore': patch +'firestore': patch +--- + +Fix multi-tab persistence raising empty snapshot issue From ffc268dc58da53774809ac544e6d4bf144c1e85f Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Tue, 14 May 2024 11:36:31 -0400 Subject: [PATCH 4/5] update changeset --- .changeset/nine-rings-jog.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changeset/nine-rings-jog.md b/.changeset/nine-rings-jog.md index 5792292f026..20a1d8b3d6d 100644 --- a/.changeset/nine-rings-jog.md +++ b/.changeset/nine-rings-jog.md @@ -1,6 +1,6 @@ --- '@firebase/firestore': patch -'firestore': patch +'firebase': patch --- Fix multi-tab persistence raising empty snapshot issue From f32efea4a95347d0438a7516ffc02a8f476d28f5 Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Tue, 14 May 2024 11:37:16 -0400 Subject: [PATCH 5/5] correct spelling --- packages/firestore/test/unit/specs/query_spec.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/firestore/test/unit/specs/query_spec.test.ts b/packages/firestore/test/unit/specs/query_spec.test.ts index 95b808cae67..9046540a2ff 100644 --- a/packages/firestore/test/unit/specs/query_spec.test.ts +++ b/packages/firestore/test/unit/specs/query_spec.test.ts @@ -138,7 +138,7 @@ describeSpec('Queries:', [], () => { ); specTest( - 'Queries in different tabs will not interferer', + 'Queries in different tabs will not interfere', ['multi-client'], () => { const query1 = query('collection', filter('key', '==', 'a'));