diff --git a/.changeset/cool-grapes-attend.md b/.changeset/cool-grapes-attend.md new file mode 100644 index 00000000000..8f6d8b289b6 --- /dev/null +++ b/.changeset/cool-grapes-attend.md @@ -0,0 +1,6 @@ +--- +'@firebase/firestore': patch +'firebase': patch +--- + +Fix Firestore failing to raise initial snapshot from empty local cache result diff --git a/packages/firestore/src/core/event_manager.ts b/packages/firestore/src/core/event_manager.ts index ecce1109150..1723abd15d9 100644 --- a/packages/firestore/src/core/event_manager.ts +++ b/packages/firestore/src/core/event_manager.ts @@ -307,7 +307,8 @@ export class QueryListener { snap.mutatedKeys, snap.fromCache, snap.syncStateChanged, - /* excludesMetadataChanges= */ true + /* excludesMetadataChanges= */ true, + snap.hasCachedResults ); } let raisedEvent = false; @@ -371,8 +372,13 @@ export class QueryListener { return false; } - // Raise data from cache if we have any documents or we are offline - return !snap.docs.isEmpty() || onlineState === OnlineState.Offline; + // Raise data from cache if we have any documents, have cached results before, + // or we are offline. + return ( + !snap.docs.isEmpty() || + snap.hasCachedResults || + onlineState === OnlineState.Offline + ); } private shouldRaiseEvent(snap: ViewSnapshot): boolean { @@ -405,7 +411,8 @@ export class QueryListener { snap.query, snap.docs, snap.mutatedKeys, - snap.fromCache + snap.fromCache, + snap.hasCachedResults ); this.raisedInitialEvent = true; this.queryObserver.next(snap); diff --git a/packages/firestore/src/core/sync_engine_impl.ts b/packages/firestore/src/core/sync_engine_impl.ts index 67405db0b1b..cbe66dce8c4 100644 --- a/packages/firestore/src/core/sync_engine_impl.ts +++ b/packages/firestore/src/core/sync_engine_impl.ts @@ -64,6 +64,7 @@ import { import { debugAssert, debugCast, fail, hardAssert } from '../util/assert'; import { wrapInUserErrorIfRecoverable } from '../util/async_queue'; import { BundleReader } from '../util/bundle_reader'; +import { ByteString } from '../util/byte_string'; import { Code, FirestoreError } from '../util/error'; import { logDebug, logWarn } from '../util/log'; import { primitiveComparator } from '../util/misc'; @@ -327,7 +328,8 @@ export async function syncEngineListen( syncEngineImpl, query, targetId, - status === 'current' + status === 'current', + targetData.resumeToken ); } @@ -342,7 +344,8 @@ async function initializeViewAndComputeSnapshot( syncEngineImpl: SyncEngineImpl, query: Query, targetId: TargetId, - current: boolean + current: boolean, + resumeToken: ByteString ): Promise { // PORTING NOTE: On Web only, we inject the code that registers new Limbo // targets based on view changes. This allows us to only depend on Limbo @@ -360,7 +363,8 @@ async function initializeViewAndComputeSnapshot( const synthesizedTargetChange = TargetChange.createSynthesizedTargetChangeForCurrentChange( targetId, - current && syncEngineImpl.onlineState !== OnlineState.Offline + current && syncEngineImpl.onlineState !== OnlineState.Offline, + resumeToken ); const viewChange = view.applyChanges( viewDocChanges, @@ -1385,7 +1389,8 @@ async function synchronizeQueryViewsAndRaiseSnapshots( syncEngineImpl, synthesizeTargetToQuery(target!), targetId, - /*current=*/ false + /*current=*/ false, + targetData.resumeToken ); } @@ -1457,7 +1462,8 @@ export async function syncEngineApplyTargetState( const synthesizedRemoteEvent = RemoteEvent.createSynthesizedRemoteEventForCurrentChange( targetId, - state === 'current' + state === 'current', + ByteString.EMPTY_BYTE_STRING ); await syncEngineEmitNewSnapsAndNotifyLocalStore( syncEngineImpl, @@ -1512,7 +1518,8 @@ export async function syncEngineApplyActiveTargetsChange( syncEngineImpl, synthesizeTargetToQuery(target), targetData.targetId, - /*current=*/ false + /*current=*/ false, + targetData.resumeToken ); remoteStoreListen(syncEngineImpl.remoteStore, targetData); } diff --git a/packages/firestore/src/core/view.ts b/packages/firestore/src/core/view.ts index 21ab049b9af..de66d883ac3 100644 --- a/packages/firestore/src/core/view.ts +++ b/packages/firestore/src/core/view.ts @@ -72,6 +72,7 @@ export interface ViewChange { */ export class View { private syncState: SyncState | null = null; + private hasCachedResults: boolean = false; /** * A flag whether the view is current with the backend. A view is considered * current after it has seen the current flag from the backend and did not @@ -319,7 +320,10 @@ export class View { docChanges.mutatedKeys, newSyncState === SyncState.Local, syncStateChanged, - /* excludesMetadataChanges= */ false + /* excludesMetadataChanges= */ false, + targetChange + ? targetChange.resumeToken.approximateByteSize() > 0 + : false ); return { snapshot: snap, @@ -468,7 +472,8 @@ export class View { this.query, this.documentSet, this.mutatedKeys, - this.syncState === SyncState.Local + this.syncState === SyncState.Local, + this.hasCachedResults ); } } diff --git a/packages/firestore/src/core/view_snapshot.ts b/packages/firestore/src/core/view_snapshot.ts index e631a3dcac0..f15c5ccb409 100644 --- a/packages/firestore/src/core/view_snapshot.ts +++ b/packages/firestore/src/core/view_snapshot.ts @@ -146,7 +146,8 @@ export class ViewSnapshot { readonly mutatedKeys: DocumentKeySet, readonly fromCache: boolean, readonly syncStateChanged: boolean, - readonly excludesMetadataChanges: boolean + readonly excludesMetadataChanges: boolean, + readonly hasCachedResults: boolean ) {} /** Returns a view snapshot as if all documents in the snapshot were added. */ @@ -154,7 +155,8 @@ export class ViewSnapshot { query: Query, documents: DocumentSet, mutatedKeys: DocumentKeySet, - fromCache: boolean + fromCache: boolean, + hasCachedResults: boolean ): ViewSnapshot { const changes: DocumentViewChange[] = []; documents.forEach(doc => { @@ -169,7 +171,8 @@ export class ViewSnapshot { mutatedKeys, fromCache, /* syncStateChanged= */ true, - /* excludesMetadataChanges= */ false + /* excludesMetadataChanges= */ false, + hasCachedResults ); } @@ -180,6 +183,7 @@ export class ViewSnapshot { isEqual(other: ViewSnapshot): boolean { if ( this.fromCache !== other.fromCache || + this.hasCachedResults !== other.hasCachedResults || this.syncStateChanged !== other.syncStateChanged || !this.mutatedKeys.isEqual(other.mutatedKeys) || !queryEquals(this.query, other.query) || diff --git a/packages/firestore/src/remote/remote_event.ts b/packages/firestore/src/remote/remote_event.ts index e35e1dbae12..5d48d32bb3f 100644 --- a/packages/firestore/src/remote/remote_event.ts +++ b/packages/firestore/src/remote/remote_event.ts @@ -67,14 +67,16 @@ export class RemoteEvent { // PORTING NOTE: Multi-tab only static createSynthesizedRemoteEventForCurrentChange( targetId: TargetId, - current: boolean + current: boolean, + resumeToken: ByteString ): RemoteEvent { const targetChanges = new Map(); targetChanges.set( targetId, TargetChange.createSynthesizedTargetChangeForCurrentChange( targetId, - current + current, + resumeToken ) ); return new RemoteEvent( @@ -134,10 +136,11 @@ export class TargetChange { */ static createSynthesizedTargetChangeForCurrentChange( targetId: TargetId, - current: boolean + current: boolean, + resumeToken: ByteString ): TargetChange { return new TargetChange( - ByteString.EMPTY_BYTE_STRING, + resumeToken, current, documentKeySet(), documentKeySet(), diff --git a/packages/firestore/test/integration/api/query.test.ts b/packages/firestore/test/integration/api/query.test.ts index 66397ed179c..85e21c024e3 100644 --- a/packages/firestore/test/integration/api/query.test.ts +++ b/packages/firestore/test/integration/api/query.test.ts @@ -1281,13 +1281,52 @@ apiDescribe('Queries', (persistence: boolean) => { }; return withTestCollection(persistence, testDocs, async coll => { - await getDocs(query(coll)); // Populate the cache + await getDocs(query(coll)); // Populate the cache. const snapshot = await getDocs( query(coll, where('map.nested', '==', 'foo')) ); expect(toDataArray(snapshot)).to.deep.equal([{ map: { nested: 'foo' } }]); }); }); + + // Reproduces https://github.com/firebase/firebase-js-sdk/issues/5873 + // eslint-disable-next-line no-restricted-properties + (persistence ? describe : describe.skip)('Caching empty results', () => { + it('can raise initial snapshot from cache, even if it is empty', () => { + return withTestCollection(persistence, {}, async coll => { + const snapshot1 = await getDocs(coll); // Populate the cache. + expect(snapshot1.metadata.fromCache).to.be.false; + expect(toDataArray(snapshot1)).to.deep.equal([]); // Precondition check. + + // Add a snapshot listener whose first event should be raised from cache. + const storeEvent = new EventsAccumulator(); + onSnapshot(coll, storeEvent.storeEvent); + const snapshot2 = await storeEvent.awaitEvent(); + expect(snapshot2.metadata.fromCache).to.be.true; + expect(toDataArray(snapshot2)).to.deep.equal([]); + }); + }); + + it('can raise initial snapshot from cache, even if it has become empty', () => { + const testDocs = { + a: { key: 'a' } + }; + return withTestCollection(persistence, testDocs, async coll => { + // Populate the cache. + const snapshot1 = await getDocs(coll); + expect(snapshot1.metadata.fromCache).to.be.false; + expect(toDataArray(snapshot1)).to.deep.equal([{ key: 'a' }]); + // Empty the collection. + void deleteDoc(doc(coll, 'a')); + + const storeEvent = new EventsAccumulator(); + onSnapshot(coll, storeEvent.storeEvent); + const snapshot2 = await storeEvent.awaitEvent(); + expect(snapshot2.metadata.fromCache).to.be.true; + expect(toDataArray(snapshot2)).to.deep.equal([]); + }); + }); + }); }); function verifyDocumentChange( diff --git a/packages/firestore/test/unit/api/database.test.ts b/packages/firestore/test/unit/api/database.test.ts index 71b798282f5..edc1a520f07 100644 --- a/packages/firestore/test/unit/api/database.test.ts +++ b/packages/firestore/test/unit/api/database.test.ts @@ -177,6 +177,51 @@ describe('QuerySnapshot', () => { querySnapshot('foo', {}, { a: { a: 1 } }, keys('foo/a'), false, true) ) ).to.be.false; + // hasCachedResults should affect querySnapshot equality + expect( + snapshotEqual( + querySnapshot( + 'foo', + {}, + { a: { a: 1 } }, + keys('foo/a'), + false, + false, + true + ), + querySnapshot( + 'foo', + {}, + { a: { a: 1 } }, + keys('foo/a'), + false, + false, + true + ) + ) + ).to.be.true; + expect( + snapshotEqual( + querySnapshot( + 'foo', + {}, + { a: { a: 1 } }, + keys('foo/a'), + false, + false, + true + ), + querySnapshot( + 'foo', + {}, + { a: { a: 1 } }, + keys('foo/a'), + false, + false, + false + ) + ) + ).to.be.false; }); it('JSON.stringify() does not throw', () => { diff --git a/packages/firestore/test/unit/core/event_manager.test.ts b/packages/firestore/test/unit/core/event_manager.test.ts index 8fab4653bd7..293514ceb6d 100644 --- a/packages/firestore/test/unit/core/event_manager.test.ts +++ b/packages/firestore/test/unit/core/event_manager.test.ts @@ -224,7 +224,8 @@ describe('QueryListener', () => { docChanges: [change1, change4], fromCache: snap2.fromCache, syncStateChanged: true, - mutatedKeys: keys() + mutatedKeys: keys(), + hasCachedResults: snap2.hasCachedResults }; expect(otherEvents).to.deep.equal([expectedSnap2]); }); @@ -396,7 +397,8 @@ describe('QueryListener', () => { docChanges: [change3], fromCache: snap2.fromCache, syncStateChanged: snap2.syncStateChanged, - mutatedKeys: snap2.mutatedKeys + mutatedKeys: snap2.mutatedKeys, + hasCachedResults: snap2.hasCachedResults }; expect(filteredEvents).to.deep.equal([snap1, expectedSnap2]); } @@ -482,7 +484,8 @@ describe('QueryListener', () => { ], fromCache: false, syncStateChanged: true, - mutatedKeys: keys() + mutatedKeys: keys(), + hasCachedResults: snap3.hasCachedResults }; expect(events).to.deep.equal([expectedSnap]); }); @@ -517,7 +520,8 @@ describe('QueryListener', () => { docChanges: [{ type: ChangeType.Added, doc: doc1 }], fromCache: true, syncStateChanged: true, - mutatedKeys: keys() + mutatedKeys: keys(), + hasCachedResults: snap1.hasCachedResults }; const expectedSnap2 = { query: query1, @@ -526,7 +530,8 @@ describe('QueryListener', () => { docChanges: [{ type: ChangeType.Added, doc: doc2 }], fromCache: true, syncStateChanged: false, - mutatedKeys: keys() + mutatedKeys: keys(), + hasCachedResults: snap2.hasCachedResults }; expect(events).to.deep.equal([expectedSnap1, expectedSnap2]); }); @@ -552,7 +557,8 @@ describe('QueryListener', () => { docChanges: [], fromCache: true, syncStateChanged: true, - mutatedKeys: keys() + mutatedKeys: keys(), + hasCachedResults: snap1.hasCachedResults }; expect(events).to.deep.equal([expectedSnap]); }); @@ -577,7 +583,8 @@ describe('QueryListener', () => { docChanges: [], fromCache: true, syncStateChanged: true, - mutatedKeys: keys() + mutatedKeys: keys(), + hasCachedResults: snap1.hasCachedResults }; expect(events).to.deep.equal([expectedSnap]); }); diff --git a/packages/firestore/test/unit/specs/listen_spec.test.ts b/packages/firestore/test/unit/specs/listen_spec.test.ts index 8ebf5d7cbc0..8bc62391838 100644 --- a/packages/firestore/test/unit/specs/listen_spec.test.ts +++ b/packages/firestore/test/unit/specs/listen_spec.test.ts @@ -351,6 +351,7 @@ describeSpec('Listens:', [], () => { // (the document falls out) and send us a snapshot that's ahead of // docAv3 (which is already in our cache). .userListens(visibleQuery, { resumeToken: 'resume-token-1000' }) + .expectEvents(visibleQuery, { fromCache: true }) .watchAcks(visibleQuery) .watchSends({ removed: [visibleQuery] }, docAv2) .watchCurrents(visibleQuery, 'resume-token-5000') @@ -396,6 +397,7 @@ describeSpec('Listens:', [], () => { // (the document falls out) and send us a snapshot that's ahead of // docAv3 (which is already in our cache). .userListens(visibleQuery, { resumeToken: 'resume-token-1000' }) + .expectEvents(visibleQuery, { fromCache: true }) .watchAcks(visibleQuery) .watchSends({ removed: [visibleQuery] }, docAv2) .watchCurrents(visibleQuery, 'resume-token-5000') @@ -405,6 +407,7 @@ describeSpec('Listens:', [], () => { .watchRemoves(visibleQuery) // Listen to allQuery again and make sure we still get no docs. .userListens(allQuery, { resumeToken: 'resume-token-4000' }) + .expectEvents(allQuery, { fromCache: true }) .watchAcksFull(allQuery, 6000) .expectEvents(allQuery, { fromCache: false }) ); @@ -1695,4 +1698,115 @@ describeSpec('Listens:', [], () => { }) .expectSnapshotsInSyncEvent(2); }); + + specTest('Empty initial snapshot is raised from cache', [], () => { + const query1 = query('collection'); + return ( + spec() + // Disable GC so the cache persists across listens. + .withGCEnabled(false) + // Populate the cache with the empty query results. + .userListens(query1) + .watchAcksFull(query1, 1000) + .expectEvents(query1, { fromCache: false }) + .userUnlistens(query1) + .watchRemoves(query1) + // Listen to the query again and verify that the empty snapshot is + // raised from cache. + .userListens(query1, { resumeToken: 'resume-token-1000' }) + .expectEvents(query1, { fromCache: true }) + // Verify that another snapshot is raised once the query result comes + // back from Watch. + .watchAcksFull(query1, 2000) + .expectEvents(query1, { fromCache: false }) + ); + }); + + specTest( + 'Empty-due-to-delete initial snapshot is raised from cache', + [], + () => { + const query1 = query('collection'); + const doc1 = doc('collection/a', 1000, { v: 1 }); + return ( + spec() + // Disable GC so the cache persists across listens. + .withGCEnabled(false) + // Populate the cache with the empty query results. + .userListens(query1) + .watchAcksFull(query1, 1000, doc1) + .expectEvents(query1, { added: [doc1] }) + .userUnlistens(query1) + .watchRemoves(query1) + // Delete the only document in the result set locally on the client. + .userDeletes('collection/a') + // Listen to the query again and verify that the empty snapshot is + // raised from cache, even though the write is not yet acknowledged. + .userListens(query1, { resumeToken: 'resume-token-1000' }) + .expectEvents(query1, { fromCache: true }) + ); + } + ); + + specTest( + 'Empty initial snapshot is raised from cache in multiple tabs', + ['multi-client'], + () => { + const query1 = query('collection'); + return ( + client(0, /* withGcEnabled= */ false) + // Populate the cache with the empty query results. + .userListens(query1) + .watchAcksFull(query1, 1000) + .expectEvents(query1, { fromCache: false }) + .userUnlistens(query1) + .watchRemoves(query1) + .client(1) + // Re-listen to the query in second client and verify that the empty + // snapshot is raised from cache. + .userListens(query1) + .expectEvents(query1, { fromCache: true }) + .client(0) + .expectListen(query1, { resumeToken: 'resume-token-1000' }) + // Verify that another snapshot is raised once the query result comes + // back from Watch. + .watchAcksFull(query1, 2000) + .client(1) + .expectEvents(query1, { fromCache: false }) + ); + } + ); + specTest( + 'Empty-due-to-delete initial snapshot is raised from cache in multiple tabs', + ['multi-client'], + () => { + const query1 = query('collection'); + const doc1 = doc('collection/a', 1000, { v: 1 }); + const doc1Deleted = deletedDoc('collection/a', 2000); + + return ( + client(0, /* withGcEnabled= */ false) + // Populate the cache with the empty query results. + .userListens(query1) + .watchAcksFull(query1, 1000, doc1) + .expectEvents(query1, { added: [doc1] }) + .userUnlistens(query1) + .watchRemoves(query1) + // Delete the only document in the result set locally on the client. + .userDeletes('collection/a') + // Re-listen to the query in second client and verify that the empty + // snapshot is raised from cache with local mutation. + .client(1) + .userListens(query1) + .expectEvents(query1, { fromCache: true }) + // Should get events once stream is caught up. + .client(0) + .expectListen(query1, { resumeToken: 'resume-token-1000' }) + .writeAcks('collection/a', 2000) + .watchAcksFull(query1, 2000, doc1Deleted) + .client(1) + .expectEvents(query1, { fromCache: false }) + ); + } + ); }); diff --git a/packages/firestore/test/unit/specs/recovery_spec.test.ts b/packages/firestore/test/unit/specs/recovery_spec.test.ts index f14aa5642c4..3da50a0b556 100644 --- a/packages/firestore/test/unit/specs/recovery_spec.test.ts +++ b/packages/firestore/test/unit/specs/recovery_spec.test.ts @@ -489,6 +489,7 @@ describeSpec('Persistence Recovery', ['no-ios', 'no-android'], () => { .userUnlistens(query1) // No event since the document was removed .userListens(query1, { resumeToken: 'resume-token-1000' }) + .expectEvents(query1, { fromCache: true }) ); } ); diff --git a/packages/firestore/test/util/api_helpers.ts b/packages/firestore/test/util/api_helpers.ts index 2dd8a6199e0..762b5258a29 100644 --- a/packages/firestore/test/util/api_helpers.ts +++ b/packages/firestore/test/util/api_helpers.ts @@ -130,7 +130,8 @@ export function querySnapshot( docsToAdd: { [key: string]: JsonObject }, mutatedKeys: DocumentKeySet, fromCache: boolean, - syncStateChanged: boolean + syncStateChanged: boolean, + hasCachedResults?: boolean ): QuerySnapshot { const query: InternalQuery = newQueryForPath(pathFrom(path)); let oldDocuments: DocumentSet = new DocumentSet(); @@ -152,7 +153,8 @@ export function querySnapshot( mutatedKeys, fromCache, syncStateChanged, - false + false, + hasCachedResults ?? false ); const db = firestore(); return new QuerySnapshot(