From e5abb85c41d9ec39945c4be428a5b30542a19c2e Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Thu, 22 Sep 2022 13:04:56 -0700 Subject: [PATCH 01/18] add resumeToken to viewSnapshot and raiseInitialEvent condition check --- packages/firestore/src/core/event_manager.ts | 12 +++++++++--- .../firestore/src/core/sync_engine_impl.ts | 19 +++++++++++++------ packages/firestore/src/core/view.ts | 8 ++++++-- packages/firestore/src/core/view_snapshot.ts | 13 +++++++++---- packages/firestore/src/remote/remote_event.ts | 11 +++++++---- .../test/integration/api/query.test.ts | 17 +++++++++++++++++ 6 files changed, 61 insertions(+), 19 deletions(-) diff --git a/packages/firestore/src/core/event_manager.ts b/packages/firestore/src/core/event_manager.ts index ecce1109150..44512bffd8d 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.resumeToken ); } let raisedEvent = false; @@ -372,7 +373,11 @@ export class QueryListener { } // Raise data from cache if we have any documents or we are offline - return !snap.docs.isEmpty() || onlineState === OnlineState.Offline; + return ( + !snap.docs.isEmpty() || + snap.resumeToken.approximateByteSize() > 0 || + onlineState === OnlineState.Offline + ); } private shouldRaiseEvent(snap: ViewSnapshot): boolean { @@ -405,7 +410,8 @@ export class QueryListener { snap.query, snap.docs, snap.mutatedKeys, - snap.fromCache + snap.fromCache, + snap.resumeToken ); 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 339865549a7..e871b6d317b 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, @@ -1379,7 +1383,8 @@ async function synchronizeQueryViewsAndRaiseSnapshots( syncEngineImpl, synthesizeTargetToQuery(target!), targetId, - /*current=*/ false + /*current=*/ false, + targetData.resumeToken ); } @@ -1451,7 +1456,8 @@ export async function syncEngineApplyTargetState( const synthesizedRemoteEvent = RemoteEvent.createSynthesizedRemoteEventForCurrentChange( targetId, - state === 'current' + state === 'current', + ByteString.EMPTY_BYTE_STRING ); await syncEngineEmitNewSnapsAndNotifyLocalStore( syncEngineImpl, @@ -1506,7 +1512,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..df6d8117599 100644 --- a/packages/firestore/src/core/view.ts +++ b/packages/firestore/src/core/view.ts @@ -26,6 +26,7 @@ import { DocumentKey } from '../model/document_key'; import { DocumentSet } from '../model/document_set'; import { TargetChange } from '../remote/remote_event'; import { debugAssert, fail } from '../util/assert'; +import { ByteString } from '../util/byte_string'; import { LimitType, newQueryComparator, Query, queryMatches } from './query'; import { OnlineState } from './types'; @@ -72,6 +73,7 @@ export interface ViewChange { */ export class View { private syncState: SyncState | null = null; + private resumeToken: ByteString | null = null; /** * 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 +321,8 @@ export class View { docChanges.mutatedKeys, newSyncState === SyncState.Local, syncStateChanged, - /* excludesMetadataChanges= */ false + /* excludesMetadataChanges= */ false, + targetChange?.resumeToken ?? ByteString.EMPTY_BYTE_STRING ); return { snapshot: snap, @@ -468,7 +471,8 @@ export class View { this.query, this.documentSet, this.mutatedKeys, - this.syncState === SyncState.Local + this.syncState === SyncState.Local, + this.resumeToken ?? ByteString.EMPTY_BYTE_STRING ); } } diff --git a/packages/firestore/src/core/view_snapshot.ts b/packages/firestore/src/core/view_snapshot.ts index e631a3dcac0..8d85f5d59a9 100644 --- a/packages/firestore/src/core/view_snapshot.ts +++ b/packages/firestore/src/core/view_snapshot.ts @@ -20,6 +20,7 @@ import { Document } from '../model/document'; import { DocumentKey } from '../model/document_key'; import { DocumentSet } from '../model/document_set'; import { fail } from '../util/assert'; +import { ByteString } from '../util/byte_string'; import { SortedMap } from '../util/sorted_map'; import { Query, queryEquals } from './query'; @@ -146,7 +147,8 @@ export class ViewSnapshot { readonly mutatedKeys: DocumentKeySet, readonly fromCache: boolean, readonly syncStateChanged: boolean, - readonly excludesMetadataChanges: boolean + readonly excludesMetadataChanges: boolean, + readonly resumeToken: ByteString ) {} /** Returns a view snapshot as if all documents in the snapshot were added. */ @@ -154,7 +156,8 @@ export class ViewSnapshot { query: Query, documents: DocumentSet, mutatedKeys: DocumentKeySet, - fromCache: boolean + fromCache: boolean, + resumeToken: ByteString ): ViewSnapshot { const changes: DocumentViewChange[] = []; documents.forEach(doc => { @@ -169,7 +172,8 @@ export class ViewSnapshot { mutatedKeys, fromCache, /* syncStateChanged= */ true, - /* excludesMetadataChanges= */ false + /* excludesMetadataChanges= */ false, + resumeToken ); } @@ -184,7 +188,8 @@ export class ViewSnapshot { !this.mutatedKeys.isEqual(other.mutatedKeys) || !queryEquals(this.query, other.query) || !this.docs.isEqual(other.docs) || - !this.oldDocs.isEqual(other.oldDocs) + !this.oldDocs.isEqual(other.oldDocs) || + this.resumeToken !== other.resumeToken ) { return false; } 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..5a1d82815c6 100644 --- a/packages/firestore/test/integration/api/query.test.ts +++ b/packages/firestore/test/integration/api/query.test.ts @@ -1288,6 +1288,23 @@ apiDescribe('Queries', (persistence: boolean) => { expect(toDataArray(snapshot)).to.deep.equal([{ map: { nested: 'foo' } }]); }); }); + + // eslint-disable-next-line no-restricted-properties + (persistence ? it.only : it.skip)('empty query results are cached', () => { + // Reproduces https://github.com/firebase/firebase-js-sdk/issues/5873 + 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([]); + }); + }); }); function verifyDocumentChange( From 7bb2cfa6938b23a913dd6edbded0c713a95bc045 Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Thu, 22 Sep 2022 13:11:23 -0700 Subject: [PATCH 02/18] update test helper with resume token, remove it.only --- packages/firestore/test/integration/api/query.test.ts | 2 +- packages/firestore/test/util/api_helpers.ts | 6 +++++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/packages/firestore/test/integration/api/query.test.ts b/packages/firestore/test/integration/api/query.test.ts index 5a1d82815c6..b05066be5ef 100644 --- a/packages/firestore/test/integration/api/query.test.ts +++ b/packages/firestore/test/integration/api/query.test.ts @@ -1290,7 +1290,7 @@ apiDescribe('Queries', (persistence: boolean) => { }); // eslint-disable-next-line no-restricted-properties - (persistence ? it.only : it.skip)('empty query results are cached', () => { + (persistence ? it : it.skip)('empty query results are cached', () => { // Reproduces https://github.com/firebase/firebase-js-sdk/issues/5873 return withTestCollection(persistence, {}, async coll => { const snapshot1 = await getDocs(coll); // Populate the cache diff --git a/packages/firestore/test/util/api_helpers.ts b/packages/firestore/test/util/api_helpers.ts index 2dd8a6199e0..3df4a9d4165 100644 --- a/packages/firestore/test/util/api_helpers.ts +++ b/packages/firestore/test/util/api_helpers.ts @@ -43,6 +43,7 @@ import { import { DocumentKeySet } from '../../src/model/collections'; import { DocumentSet } from '../../src/model/document_set'; import { JsonObject } from '../../src/model/object_value'; +import { ByteString } from '../../src/util/byte_string'; import { TEST_PROJECT } from '../unit/local/persistence_test_helpers'; import { doc, key, path as pathFrom } from './helpers'; @@ -144,6 +145,8 @@ export function querySnapshot( newDocuments = newDocuments.add(docToAdd); documentChanges.push({ type: ChangeType.Added, doc: docToAdd }); }); + const resumeToken = ByteString.EMPTY_BYTE_STRING; + const viewSnapshot: ViewSnapshot = new ViewSnapshot( query, newDocuments, @@ -152,7 +155,8 @@ export function querySnapshot( mutatedKeys, fromCache, syncStateChanged, - false + false, + resumeToken ); const db = firestore(); return new QuerySnapshot( From 0ae4afceedfebcd76e2f3e3f697441738f3c49d3 Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Thu, 22 Sep 2022 23:40:26 -0700 Subject: [PATCH 03/18] modify failing tests --- packages/firestore/src/remote/remote_event.ts | 4 +-- .../test/integration/api/query.test.ts | 2 +- .../test/unit/core/event_manager.test.ts | 21 +++++++++----- .../test/unit/specs/listen_spec.test.ts | 28 +++++++++---------- .../firestore/test/unit/specs/spec_builder.ts | 12 +++++++- packages/firestore/test/util/api_helpers.ts | 1 - 6 files changed, 42 insertions(+), 26 deletions(-) diff --git a/packages/firestore/src/remote/remote_event.ts b/packages/firestore/src/remote/remote_event.ts index 5d48d32bb3f..978f7a78d54 100644 --- a/packages/firestore/src/remote/remote_event.ts +++ b/packages/firestore/src/remote/remote_event.ts @@ -137,10 +137,10 @@ export class TargetChange { static createSynthesizedTargetChangeForCurrentChange( targetId: TargetId, current: boolean, - resumeToken: ByteString + resumeToken?: ByteString ): TargetChange { return new TargetChange( - resumeToken, + resumeToken ?? ByteString.EMPTY_BYTE_STRING, current, documentKeySet(), documentKeySet(), diff --git a/packages/firestore/test/integration/api/query.test.ts b/packages/firestore/test/integration/api/query.test.ts index b05066be5ef..699b0892e3a 100644 --- a/packages/firestore/test/integration/api/query.test.ts +++ b/packages/firestore/test/integration/api/query.test.ts @@ -1290,7 +1290,7 @@ apiDescribe('Queries', (persistence: boolean) => { }); // eslint-disable-next-line no-restricted-properties - (persistence ? it : it.skip)('empty query results are cached', () => { + it ('empty query results are cached', () => { // Reproduces https://github.com/firebase/firebase-js-sdk/issues/5873 return withTestCollection(persistence, {}, async coll => { const snapshot1 = await getDocs(coll); // Populate the cache diff --git a/packages/firestore/test/unit/core/event_manager.test.ts b/packages/firestore/test/unit/core/event_manager.test.ts index 8fab4653bd7..ab461057316 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(), + resumeToken: snap2.resumeToken }; 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, + resumeToken: snap2.resumeToken }; expect(filteredEvents).to.deep.equal([snap1, expectedSnap2]); } @@ -482,7 +484,8 @@ describe('QueryListener', () => { ], fromCache: false, syncStateChanged: true, - mutatedKeys: keys() + mutatedKeys: keys(), + resumeToken: snap3.resumeToken }; 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(), + resumeToken: snap1.resumeToken }; const expectedSnap2 = { query: query1, @@ -526,7 +530,8 @@ describe('QueryListener', () => { docChanges: [{ type: ChangeType.Added, doc: doc2 }], fromCache: true, syncStateChanged: false, - mutatedKeys: keys() + mutatedKeys: keys(), + resumeToken: snap2.resumeToken }; expect(events).to.deep.equal([expectedSnap1, expectedSnap2]); }); @@ -552,7 +557,8 @@ describe('QueryListener', () => { docChanges: [], fromCache: true, syncStateChanged: true, - mutatedKeys: keys() + mutatedKeys: keys(), + resumeToken: snap1.resumeToken }; expect(events).to.deep.equal([expectedSnap]); }); @@ -577,7 +583,8 @@ describe('QueryListener', () => { docChanges: [], fromCache: true, syncStateChanged: true, - mutatedKeys: keys() + mutatedKeys: keys(), + resumeToken: snap1.resumeToken }; 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 d6cc4e7fffd..355598ff4e4 100644 --- a/packages/firestore/test/unit/specs/listen_spec.test.ts +++ b/packages/firestore/test/unit/specs/listen_spec.test.ts @@ -324,7 +324,7 @@ describeSpec('Listens:', [], () => { } ); - specTest('Individual documents cannot revert', [], () => { + specTest('Individual documents cannot revert', ['exclusive'], () => { const allQuery = query('collection'); const visibleQuery = query('collection', filter('visible', '==', true)); const docAv1 = doc('collection/a', 1000, { visible: true, v: 'v1000' }); @@ -350,19 +350,19 @@ describeSpec('Listens:', [], () => { // us up to docAV2 since that's the last relevant change to the query // (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' }) - .watchAcks(visibleQuery) - .watchSends({ removed: [visibleQuery] }, docAv2) - .watchCurrents(visibleQuery, 'resume-token-5000') - .watchSnapshots(5000) - .expectEvents(visibleQuery, { fromCache: false }) - .userUnlistens(visibleQuery) - .watchRemoves(visibleQuery) - // Listen to allQuery again and make sure we still get docAv3. - .userListens(allQuery, { resumeToken: 'resume-token-4000' }) - .expectEvents(allQuery, { added: [docAv3], fromCache: true }) - .watchAcksFull(allQuery, 6000) - .expectEvents(allQuery, { fromCache: false }) + .userListens(visibleQuery, { resumeToken: 'resume-token-4000' }) + // .watchAcks(visibleQuery) + // .watchSends({ removed: [visibleQuery] }, docAv2) + // .watchCurrents(visibleQuery, 'resume-token-5000') + // .watchSnapshots(5000) + // .expectEvents(visibleQuery, { fromCache: false }) + // .userUnlistens(visibleQuery) + // .watchRemoves(visibleQuery) + // // Listen to allQuery again and make sure we still get docAv3. + // .userListens(allQuery, { resumeToken: 'resume-token-4000' }) + // .expectEvents(allQuery, { added: [docAv3], fromCache: true }) + // .watchAcksFull(allQuery, 6000) + // .expectEvents(allQuery, { fromCache: false }) ); }); diff --git a/packages/firestore/test/unit/specs/spec_builder.ts b/packages/firestore/test/unit/specs/spec_builder.ts index 6f17689a882..dc7d63c1959 100644 --- a/packages/firestore/test/unit/specs/spec_builder.ts +++ b/packages/firestore/test/unit/specs/spec_builder.ts @@ -288,6 +288,7 @@ export class SpecBuilder { resume?.resumeToken, resume?.readTime ); + console.log("?") this.currentStep = { userListen: { targetId, query: SpecBuilder.queryToSpec(query) }, expectedState: { activeTargets: { ...this.activeTargets } } @@ -821,10 +822,19 @@ export class SpecBuilder { version: TestSnapshotVersion, ...docs: Document[] ): this { + console.log(...docs) this.watchAcks(query); + console.log("1") this.watchSends({ affects: [query] }, ...docs); + console.log("1") + this.watchCurrents(query, 'resume-token-' + version); + console.log("1") + this.watchSnapshots(version); + + console.log("version",version) + return this; } @@ -909,7 +919,7 @@ export class SpecBuilder { metadata: events.metadata && events.metadata.map(SpecBuilder.docToSpec), errorCode: mapRpcCodeFromCode(events.errorCode), fromCache: events.fromCache || false, - hasPendingWrites: events.hasPendingWrites || false + hasPendingWrites: events.hasPendingWrites || false, }); return this; } diff --git a/packages/firestore/test/util/api_helpers.ts b/packages/firestore/test/util/api_helpers.ts index 3df4a9d4165..28eafb04801 100644 --- a/packages/firestore/test/util/api_helpers.ts +++ b/packages/firestore/test/util/api_helpers.ts @@ -146,7 +146,6 @@ export function querySnapshot( documentChanges.push({ type: ChangeType.Added, doc: docToAdd }); }); const resumeToken = ByteString.EMPTY_BYTE_STRING; - const viewSnapshot: ViewSnapshot = new ViewSnapshot( query, newDocuments, From 2e388bc0bddca6d3a3486d50cd3a78912cea9c20 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Fri, 23 Sep 2022 14:56:28 -0400 Subject: [PATCH 04/18] fix spec tests in listen_spec.test.ts and recovery_spec.test.ts --- .../test/unit/specs/listen_spec.test.ts | 29 ++++++++++--------- .../test/unit/specs/recovery_spec.test.ts | 1 + 2 files changed, 17 insertions(+), 13 deletions(-) diff --git a/packages/firestore/test/unit/specs/listen_spec.test.ts b/packages/firestore/test/unit/specs/listen_spec.test.ts index 355598ff4e4..7ca5202ac3f 100644 --- a/packages/firestore/test/unit/specs/listen_spec.test.ts +++ b/packages/firestore/test/unit/specs/listen_spec.test.ts @@ -350,19 +350,20 @@ describeSpec('Listens:', [], () => { // us up to docAV2 since that's the last relevant change to the query // (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-4000' }) - // .watchAcks(visibleQuery) - // .watchSends({ removed: [visibleQuery] }, docAv2) - // .watchCurrents(visibleQuery, 'resume-token-5000') - // .watchSnapshots(5000) - // .expectEvents(visibleQuery, { fromCache: false }) - // .userUnlistens(visibleQuery) - // .watchRemoves(visibleQuery) - // // Listen to allQuery again and make sure we still get docAv3. - // .userListens(allQuery, { resumeToken: 'resume-token-4000' }) - // .expectEvents(allQuery, { added: [docAv3], fromCache: true }) - // .watchAcksFull(allQuery, 6000) - // .expectEvents(allQuery, { fromCache: false }) + .userListens(visibleQuery, { resumeToken: 'resume-token-1000' }) + .expectEvents(visibleQuery, { fromCache: true }) + .watchAcks(visibleQuery) + .watchSends({ removed: [visibleQuery] }, docAv2) + .watchCurrents(visibleQuery, 'resume-token-5000') + .watchSnapshots(5000) + .expectEvents(visibleQuery, { fromCache: false }) + .userUnlistens(visibleQuery) + .watchRemoves(visibleQuery) + // Listen to allQuery again and make sure we still get docAv3. + .userListens(allQuery, { resumeToken: 'resume-token-4000' }) + .expectEvents(allQuery, { added: [docAv3], fromCache: true }) + .watchAcksFull(allQuery, 6000) + .expectEvents(allQuery, { fromCache: false }) ); }); @@ -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 }) ); 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 }) ); } ); From 7050b294f491584fecbfd1a0248e3b622ae25b2c Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Fri, 23 Sep 2022 12:32:05 -0700 Subject: [PATCH 05/18] format to pass lint test --- .../firestore/test/integration/api/query.test.ts | 2 +- .../firestore/test/unit/specs/listen_spec.test.ts | 2 +- packages/firestore/test/unit/specs/spec_builder.ts | 12 +----------- 3 files changed, 3 insertions(+), 13 deletions(-) diff --git a/packages/firestore/test/integration/api/query.test.ts b/packages/firestore/test/integration/api/query.test.ts index 699b0892e3a..b05066be5ef 100644 --- a/packages/firestore/test/integration/api/query.test.ts +++ b/packages/firestore/test/integration/api/query.test.ts @@ -1290,7 +1290,7 @@ apiDescribe('Queries', (persistence: boolean) => { }); // eslint-disable-next-line no-restricted-properties - it ('empty query results are cached', () => { + (persistence ? it : it.skip)('empty query results are cached', () => { // Reproduces https://github.com/firebase/firebase-js-sdk/issues/5873 return withTestCollection(persistence, {}, async coll => { const snapshot1 = await getDocs(coll); // Populate the cache diff --git a/packages/firestore/test/unit/specs/listen_spec.test.ts b/packages/firestore/test/unit/specs/listen_spec.test.ts index 7ca5202ac3f..ddb52cf31e3 100644 --- a/packages/firestore/test/unit/specs/listen_spec.test.ts +++ b/packages/firestore/test/unit/specs/listen_spec.test.ts @@ -324,7 +324,7 @@ describeSpec('Listens:', [], () => { } ); - specTest('Individual documents cannot revert', ['exclusive'], () => { + specTest('Individual documents cannot revert', [], () => { const allQuery = query('collection'); const visibleQuery = query('collection', filter('visible', '==', true)); const docAv1 = doc('collection/a', 1000, { visible: true, v: 'v1000' }); diff --git a/packages/firestore/test/unit/specs/spec_builder.ts b/packages/firestore/test/unit/specs/spec_builder.ts index dc7d63c1959..6f17689a882 100644 --- a/packages/firestore/test/unit/specs/spec_builder.ts +++ b/packages/firestore/test/unit/specs/spec_builder.ts @@ -288,7 +288,6 @@ export class SpecBuilder { resume?.resumeToken, resume?.readTime ); - console.log("?") this.currentStep = { userListen: { targetId, query: SpecBuilder.queryToSpec(query) }, expectedState: { activeTargets: { ...this.activeTargets } } @@ -822,19 +821,10 @@ export class SpecBuilder { version: TestSnapshotVersion, ...docs: Document[] ): this { - console.log(...docs) this.watchAcks(query); - console.log("1") this.watchSends({ affects: [query] }, ...docs); - console.log("1") - this.watchCurrents(query, 'resume-token-' + version); - console.log("1") - this.watchSnapshots(version); - - console.log("version",version) - return this; } @@ -919,7 +909,7 @@ export class SpecBuilder { metadata: events.metadata && events.metadata.map(SpecBuilder.docToSpec), errorCode: mapRpcCodeFromCode(events.errorCode), fromCache: events.fromCache || false, - hasPendingWrites: events.hasPendingWrites || false, + hasPendingWrites: events.hasPendingWrites || false }); return this; } From fdb8f4bfeff160055c01fa886c77f4e359056f2b Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Tue, 27 Sep 2022 12:29:37 -0700 Subject: [PATCH 06/18] add more test cases --- .../test/integration/api/query.test.ts | 77 +++++++++++++++---- 1 file changed, 64 insertions(+), 13 deletions(-) diff --git a/packages/firestore/test/integration/api/query.test.ts b/packages/firestore/test/integration/api/query.test.ts index b05066be5ef..1f403dc8955 100644 --- a/packages/firestore/test/integration/api/query.test.ts +++ b/packages/firestore/test/integration/api/query.test.ts @@ -1290,19 +1290,70 @@ apiDescribe('Queries', (persistence: boolean) => { }); // eslint-disable-next-line no-restricted-properties - (persistence ? it : it.skip)('empty query results are cached', () => { - // Reproduces https://github.com/firebase/firebase-js-sdk/issues/5873 - 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([]); + // Reproduces https://github.com/firebase/firebase-js-sdk/issues/5873 + (persistence ? describe.only : describe.skip)('Caching empty results ', () => { + it('can cache empty query results', () => { + 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 empty cached collection and raise snapshot from it', () => { + 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 + deleteDoc(doc(coll, 'a')); + + // Add a snapshot listener whose first event should be raised from cache. + const storeEvent = new EventsAccumulator(); + onSnapshot( + coll, + { includeMetadataChanges: true }, + storeEvent.storeEvent + ); + const snapshot2 = await storeEvent.awaitEvent(); + expect(snapshot2.metadata.fromCache).to.be.true; + expect(toDataArray(snapshot2)).to.deep.equal([]); + + // why this if fromCahe:false ???? + const snapshot3 = await storeEvent.awaitEvent(); + expect(snapshot3.metadata.fromCache).to.be.false; + expect(toDataArray(snapshot3)).to.deep.equal([]); + }); + }); + + it('can add new doc to cached empty query result', () => { + return withTestCollection(persistence, {}, async coll => { + await getDocs(coll); // Populate the cache + + const storeEvent = new EventsAccumulator(); + onSnapshot(coll, storeEvent.storeEvent); + const snapshot1 = await storeEvent.awaitEvent(); + expect(snapshot1.metadata.fromCache).to.be.true; + expect(toDataArray(snapshot1)).to.deep.equal([]); + + await addDoc(coll, { key: 'a' }); + + const snapshot2 = await storeEvent.awaitEvent(); + expect(snapshot2.metadata.fromCache).to.be.true; + expect(toDataArray(snapshot2)).to.deep.equal([{ key: 'a' }]); + expect(snapshot2.metadata.hasPendingWrites).to.equal(true); + }); }); }); }); From f9f6ff88ef3bc8ba91b1b2012eba8ffef7815803 Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Tue, 27 Sep 2022 14:27:09 -0700 Subject: [PATCH 07/18] update tests --- .../test/integration/api/query.test.ts | 73 ++++++++++++------- 1 file changed, 46 insertions(+), 27 deletions(-) diff --git a/packages/firestore/test/integration/api/query.test.ts b/packages/firestore/test/integration/api/query.test.ts index 1f403dc8955..8070b90ff92 100644 --- a/packages/firestore/test/integration/api/query.test.ts +++ b/packages/firestore/test/integration/api/query.test.ts @@ -1291,7 +1291,7 @@ apiDescribe('Queries', (persistence: boolean) => { // eslint-disable-next-line no-restricted-properties // Reproduces https://github.com/firebase/firebase-js-sdk/issues/5873 - (persistence ? describe.only : describe.skip)('Caching empty results ', () => { + (persistence ? describe : describe.skip)('Caching empty results ', () => { it('can cache empty query results', () => { return withTestCollection(persistence, {}, async coll => { const snapshot1 = await getDocs(coll); // Populate the cache @@ -1321,39 +1321,58 @@ apiDescribe('Queries', (persistence: boolean) => { // Add a snapshot listener whose first event should be raised from cache. const storeEvent = new EventsAccumulator(); - onSnapshot( - coll, - { includeMetadataChanges: true }, - storeEvent.storeEvent - ); + onSnapshot(coll, storeEvent.storeEvent); const snapshot2 = await storeEvent.awaitEvent(); expect(snapshot2.metadata.fromCache).to.be.true; expect(toDataArray(snapshot2)).to.deep.equal([]); - - // why this if fromCahe:false ???? - const snapshot3 = await storeEvent.awaitEvent(); - expect(snapshot3.metadata.fromCache).to.be.false; - expect(toDataArray(snapshot3)).to.deep.equal([]); }); }); - it('can add new doc to cached empty query result', () => { - return withTestCollection(persistence, {}, async coll => { - await getDocs(coll); // Populate the cache - - const storeEvent = new EventsAccumulator(); - onSnapshot(coll, storeEvent.storeEvent); - const snapshot1 = await storeEvent.awaitEvent(); - expect(snapshot1.metadata.fromCache).to.be.true; - expect(toDataArray(snapshot1)).to.deep.equal([]); - - await addDoc(coll, { key: 'a' }); + it('can raise snapshot from a cached collection which was emptied offline', () => { + const testDocs = { + a: { key: 'a' } + }; + return withTestCollection( + persistence, + testDocs, + async (coll, firestore) => { + await getDocs(coll); // Populate the cache + const storeEvent = new EventsAccumulator(); + onSnapshot(coll, storeEvent.storeEvent); + await storeEvent.awaitEvent(); + + await disableNetwork(firestore); + deleteDoc(doc(coll, 'a')); + await enableNetwork(firestore); + + const snapshot = await storeEvent.awaitEvent(); + expect(snapshot.metadata.fromCache).to.be.true; + expect(toDataArray(snapshot)).to.deep.equal([]); + } + ); + }); - const snapshot2 = await storeEvent.awaitEvent(); - expect(snapshot2.metadata.fromCache).to.be.true; - expect(toDataArray(snapshot2)).to.deep.equal([{ key: 'a' }]); - expect(snapshot2.metadata.hasPendingWrites).to.equal(true); - }); + it('can register a listener and empty cache offline, and raise snaoshot from it when came back online', () => { + const testDocs = { + a: { key: 'a' } + }; + return withTestCollection( + persistence, + testDocs, + async (coll, firestore) => { + await getDocs(coll); // Populate the cache + await disableNetwork(firestore); + const storeEvent = new EventsAccumulator(); + onSnapshot(coll, storeEvent.storeEvent); + await storeEvent.awaitEvent(); + deleteDoc(doc(coll, 'a')); + await enableNetwork(firestore); + + const snapshot = await storeEvent.awaitEvent(); + expect(snapshot.metadata.fromCache).to.be.true; + expect(toDataArray(snapshot)).to.deep.equal([]); + } + ); }); }); }); From 4651a5be1f41ad2b164d4bc37430b063c4438dda Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Tue, 27 Sep 2022 14:28:50 -0700 Subject: [PATCH 08/18] reformat to pass lint --- packages/firestore/test/integration/api/query.test.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/firestore/test/integration/api/query.test.ts b/packages/firestore/test/integration/api/query.test.ts index 8070b90ff92..8c4cd4628ae 100644 --- a/packages/firestore/test/integration/api/query.test.ts +++ b/packages/firestore/test/integration/api/query.test.ts @@ -1289,8 +1289,8 @@ apiDescribe('Queries', (persistence: boolean) => { }); }); - // eslint-disable-next-line no-restricted-properties // 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 cache empty query results', () => { return withTestCollection(persistence, {}, async coll => { @@ -1317,7 +1317,7 @@ apiDescribe('Queries', (persistence: boolean) => { expect(snapshot1.metadata.fromCache).to.be.false; expect(toDataArray(snapshot1)).to.deep.equal([{ key: 'a' }]); //empty the collection - deleteDoc(doc(coll, 'a')); + void deleteDoc(doc(coll, 'a')); // Add a snapshot listener whose first event should be raised from cache. const storeEvent = new EventsAccumulator(); @@ -1342,7 +1342,7 @@ apiDescribe('Queries', (persistence: boolean) => { await storeEvent.awaitEvent(); await disableNetwork(firestore); - deleteDoc(doc(coll, 'a')); + void deleteDoc(doc(coll, 'a')); await enableNetwork(firestore); const snapshot = await storeEvent.awaitEvent(); @@ -1365,7 +1365,7 @@ apiDescribe('Queries', (persistence: boolean) => { const storeEvent = new EventsAccumulator(); onSnapshot(coll, storeEvent.storeEvent); await storeEvent.awaitEvent(); - deleteDoc(doc(coll, 'a')); + void deleteDoc(doc(coll, 'a')); await enableNetwork(firestore); const snapshot = await storeEvent.awaitEvent(); From a1cf3d11c0c3b8ff63994c0d4b25f66cb7fbdc79 Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Thu, 29 Sep 2022 15:28:41 -0700 Subject: [PATCH 09/18] resolve comments --- packages/firestore/src/core/event_manager.ts | 2 +- packages/firestore/src/core/view_snapshot.ts | 3 +- .../test/integration/api/query.test.ts | 64 +++---------------- .../firestore/test/unit/api/database.test.ts | 44 +++++++++++++ packages/firestore/test/util/api_helpers.ts | 6 +- 5 files changed, 57 insertions(+), 62 deletions(-) diff --git a/packages/firestore/src/core/event_manager.ts b/packages/firestore/src/core/event_manager.ts index 44512bffd8d..1dcf22cdcd5 100644 --- a/packages/firestore/src/core/event_manager.ts +++ b/packages/firestore/src/core/event_manager.ts @@ -372,7 +372,7 @@ export class QueryListener { return false; } - // Raise data from cache if we have any documents or we are offline + // Raise data from cache if we have any documents or resume token, or we are offline. return ( !snap.docs.isEmpty() || snap.resumeToken.approximateByteSize() > 0 || diff --git a/packages/firestore/src/core/view_snapshot.ts b/packages/firestore/src/core/view_snapshot.ts index 8d85f5d59a9..adddd0d377d 100644 --- a/packages/firestore/src/core/view_snapshot.ts +++ b/packages/firestore/src/core/view_snapshot.ts @@ -188,8 +188,7 @@ export class ViewSnapshot { !this.mutatedKeys.isEqual(other.mutatedKeys) || !queryEquals(this.query, other.query) || !this.docs.isEqual(other.docs) || - !this.oldDocs.isEqual(other.oldDocs) || - this.resumeToken !== other.resumeToken + !this.oldDocs.isEqual(other.oldDocs) ) { return false; } diff --git a/packages/firestore/test/integration/api/query.test.ts b/packages/firestore/test/integration/api/query.test.ts index 8c4cd4628ae..73521f45176 100644 --- a/packages/firestore/test/integration/api/query.test.ts +++ b/packages/firestore/test/integration/api/query.test.ts @@ -1281,7 +1281,7 @@ 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')) ); @@ -1291,12 +1291,12 @@ apiDescribe('Queries', (persistence: boolean) => { // 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 cache empty query results', () => { + (persistence ? describe : describe.skip)('Caching empty results', () => { + it('can raises initial snapshot from cache, even if it is empty', () => { return withTestCollection(persistence, {}, async coll => { - const snapshot1 = await getDocs(coll); // Populate the cache + const snapshot1 = await getDocs(coll); // Populate the cache. expect(snapshot1.metadata.fromCache).to.be.false; - expect(toDataArray(snapshot1)).to.deep.equal([]); // Precondition check + expect(toDataArray(snapshot1)).to.deep.equal([]); // Precondition check. // Add a snapshot listener whose first event should be raised from cache. const storeEvent = new EventsAccumulator(); @@ -1307,19 +1307,18 @@ apiDescribe('Queries', (persistence: boolean) => { }); }); - it('can empty cached collection and raise snapshot from it', () => { + it('can raises initial snapshot from cache, even if it has become empty', () => { const testDocs = { a: { key: 'a' } }; return withTestCollection(persistence, testDocs, async coll => { - // Populate the cache + // 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 + // Empty the collection. void deleteDoc(doc(coll, 'a')); - // 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(); @@ -1327,53 +1326,6 @@ apiDescribe('Queries', (persistence: boolean) => { expect(toDataArray(snapshot2)).to.deep.equal([]); }); }); - - it('can raise snapshot from a cached collection which was emptied offline', () => { - const testDocs = { - a: { key: 'a' } - }; - return withTestCollection( - persistence, - testDocs, - async (coll, firestore) => { - await getDocs(coll); // Populate the cache - const storeEvent = new EventsAccumulator(); - onSnapshot(coll, storeEvent.storeEvent); - await storeEvent.awaitEvent(); - - await disableNetwork(firestore); - void deleteDoc(doc(coll, 'a')); - await enableNetwork(firestore); - - const snapshot = await storeEvent.awaitEvent(); - expect(snapshot.metadata.fromCache).to.be.true; - expect(toDataArray(snapshot)).to.deep.equal([]); - } - ); - }); - - it('can register a listener and empty cache offline, and raise snaoshot from it when came back online', () => { - const testDocs = { - a: { key: 'a' } - }; - return withTestCollection( - persistence, - testDocs, - async (coll, firestore) => { - await getDocs(coll); // Populate the cache - await disableNetwork(firestore); - const storeEvent = new EventsAccumulator(); - onSnapshot(coll, storeEvent.storeEvent); - await storeEvent.awaitEvent(); - void deleteDoc(doc(coll, 'a')); - await enableNetwork(firestore); - - const snapshot = await storeEvent.awaitEvent(); - expect(snapshot.metadata.fromCache).to.be.true; - expect(toDataArray(snapshot)).to.deep.equal([]); - } - ); - }); }); }); diff --git a/packages/firestore/test/unit/api/database.test.ts b/packages/firestore/test/unit/api/database.test.ts index 71b798282f5..638f450cdcf 100644 --- a/packages/firestore/test/unit/api/database.test.ts +++ b/packages/firestore/test/unit/api/database.test.ts @@ -25,6 +25,8 @@ import { } from '../../../src'; import { EmulatorAuthCredentialsProvider } from '../../../src/api/credentials'; import { User } from '../../../src/auth/user'; +import { encodeBase64 } from '../../../src/platform/base64'; +import { ByteString } from '../../../src/util/byte_string'; import { collectionReference, documentReference, @@ -179,6 +181,48 @@ describe('QuerySnapshot', () => { ).to.be.false; }); + it('resume token should not effect querySnapshot equality', () => { + const resumeToken1 = ByteString.fromBase64String( + encodeBase64('ResumeToken1') + ); + const resumeToken1Copy = ByteString.fromBase64String( + encodeBase64('ResumeToken1') + ); + const resumeToken2 = ByteString.fromBase64String( + encodeBase64('ResumeToken2') + ); + + const snapshot1 = querySnapshot( + 'foo', + {}, + { a: { a: 1 } }, + keys(), + false, + false, + resumeToken1 + ); + const snapshot1Copy = querySnapshot( + 'foo', + {}, + { a: { a: 1 } }, + keys(), + false, + false, + resumeToken1Copy + ); + const snapshot2 = querySnapshot( + 'foo', + {}, + { a: { a: 1 } }, + keys(), + false, + false, + resumeToken2 + ); + expect(snapshotEqual(snapshot1, snapshot1Copy)).to.be.true; + expect(snapshotEqual(snapshot1, snapshot2)).to.be.true; + }); + it('JSON.stringify() does not throw', () => { JSON.stringify( querySnapshot('foo', {}, { a: { a: 1 } }, keys(), false, false) diff --git a/packages/firestore/test/util/api_helpers.ts b/packages/firestore/test/util/api_helpers.ts index 28eafb04801..68d6f8a0b96 100644 --- a/packages/firestore/test/util/api_helpers.ts +++ b/packages/firestore/test/util/api_helpers.ts @@ -131,7 +131,8 @@ export function querySnapshot( docsToAdd: { [key: string]: JsonObject }, mutatedKeys: DocumentKeySet, fromCache: boolean, - syncStateChanged: boolean + syncStateChanged: boolean, + resumeToken?: ByteString ): QuerySnapshot { const query: InternalQuery = newQueryForPath(pathFrom(path)); let oldDocuments: DocumentSet = new DocumentSet(); @@ -145,7 +146,6 @@ export function querySnapshot( newDocuments = newDocuments.add(docToAdd); documentChanges.push({ type: ChangeType.Added, doc: docToAdd }); }); - const resumeToken = ByteString.EMPTY_BYTE_STRING; const viewSnapshot: ViewSnapshot = new ViewSnapshot( query, newDocuments, @@ -155,7 +155,7 @@ export function querySnapshot( fromCache, syncStateChanged, false, - resumeToken + resumeToken ?? ByteString.EMPTY_BYTE_STRING ); const db = firestore(); return new QuerySnapshot( From f06eb5f83dff2190842aff3708c346a8a3e415c2 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Fri, 30 Sep 2022 12:59:23 -0400 Subject: [PATCH 10/18] Add two spec tests for raising initial empty snapshot from cache --- .../test/unit/specs/listen_spec.test.ts | 49 +++++++++++++++++++ 1 file changed, 49 insertions(+) diff --git a/packages/firestore/test/unit/specs/listen_spec.test.ts b/packages/firestore/test/unit/specs/listen_spec.test.ts index 596374e0697..9b2dfd8b2a7 100644 --- a/packages/firestore/test/unit/specs/listen_spec.test.ts +++ b/packages/firestore/test/unit/specs/listen_spec.test.ts @@ -1698,4 +1698,53 @@ 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 }) + ); + } + ); }); From da6dcc02f84ad5e55ea8754021d9c444b580d046 Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Mon, 3 Oct 2022 09:31:02 -0700 Subject: [PATCH 11/18] resolve part of the comments --- packages/firestore/src/core/event_manager.ts | 3 ++- packages/firestore/src/remote/remote_event.ts | 4 ++-- packages/firestore/test/integration/api/query.test.ts | 4 ++-- repo-scripts/prune-dts/tests/firestore.input.d.ts | 3 ++- 4 files changed, 8 insertions(+), 6 deletions(-) diff --git a/packages/firestore/src/core/event_manager.ts b/packages/firestore/src/core/event_manager.ts index 1dcf22cdcd5..51cafa0e5cc 100644 --- a/packages/firestore/src/core/event_manager.ts +++ b/packages/firestore/src/core/event_manager.ts @@ -372,7 +372,8 @@ export class QueryListener { return false; } - // Raise data from cache if we have any documents or resume token, or we are offline. + // Raise data from cache if we have any documents or resume token, + // or we are offline. return ( !snap.docs.isEmpty() || snap.resumeToken.approximateByteSize() > 0 || diff --git a/packages/firestore/src/remote/remote_event.ts b/packages/firestore/src/remote/remote_event.ts index 978f7a78d54..5d48d32bb3f 100644 --- a/packages/firestore/src/remote/remote_event.ts +++ b/packages/firestore/src/remote/remote_event.ts @@ -137,10 +137,10 @@ export class TargetChange { static createSynthesizedTargetChangeForCurrentChange( targetId: TargetId, current: boolean, - resumeToken?: ByteString + resumeToken: ByteString ): TargetChange { return new TargetChange( - resumeToken ?? 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 73521f45176..85e21c024e3 100644 --- a/packages/firestore/test/integration/api/query.test.ts +++ b/packages/firestore/test/integration/api/query.test.ts @@ -1292,7 +1292,7 @@ apiDescribe('Queries', (persistence: boolean) => { // 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 raises initial snapshot from cache, even if it is empty', () => { + 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; @@ -1307,7 +1307,7 @@ apiDescribe('Queries', (persistence: boolean) => { }); }); - it('can raises initial snapshot from cache, even if it has become empty', () => { + it('can raise initial snapshot from cache, even if it has become empty', () => { const testDocs = { a: { key: 'a' } }; diff --git a/repo-scripts/prune-dts/tests/firestore.input.d.ts b/repo-scripts/prune-dts/tests/firestore.input.d.ts index 73e938a1b17..6de435007df 100644 --- a/repo-scripts/prune-dts/tests/firestore.input.d.ts +++ b/repo-scripts/prune-dts/tests/firestore.input.d.ts @@ -4988,7 +4988,8 @@ declare class TargetChange { */ static createSynthesizedTargetChangeForCurrentChange( targetId: TargetId, - current: boolean + current: boolean, + resumeToken: ByteString ): TargetChange; } From ffe67118713d3526ef29106794006e31e6076e3a Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Tue, 4 Oct 2022 14:45:18 -0700 Subject: [PATCH 12/18] add spec test for multi-tab --- .../test/unit/specs/listen_spec.test.ts | 68 +++++++++++++++++++ 1 file changed, 68 insertions(+) diff --git a/packages/firestore/test/unit/specs/listen_spec.test.ts b/packages/firestore/test/unit/specs/listen_spec.test.ts index 9b2dfd8b2a7..b32f3fa0c0f 100644 --- a/packages/firestore/test/unit/specs/listen_spec.test.ts +++ b/packages/firestore/test/unit/specs/listen_spec.test.ts @@ -1747,4 +1747,72 @@ describeSpec('Listens:', [], () => { ); } ); + + 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 }) + // Query is shared in second client + .client(1) + .expectListen(query1) + .client(0) + .userUnlistens(query1) + .watchRemoves(query1) + .client(1) + .expectUnlisten(query1) + // Re-listen to the query in second client and verify that the empty + // snapshot is raised from cache. + .userListens(query1, { resumeToken: 'resume-token-1000' }) + .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) + .expectListen(query1, { resumeToken: 'resume-token-2000' }) + .expectEvents(query1, { fromCache: false }) + ); + } + ); }); From dc88dd5d081e9552fa325ab1b7df059b2593b457 Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Wed, 5 Oct 2022 11:47:48 -0700 Subject: [PATCH 13/18] resolve comments --- .changeset/cool-grapes-attend.md | 5 +++++ packages/firestore/test/unit/specs/listen_spec.test.ts | 6 ------ repo-scripts/prune-dts/tests/firestore.input.d.ts | 3 +-- 3 files changed, 6 insertions(+), 8 deletions(-) create mode 100644 .changeset/cool-grapes-attend.md diff --git a/.changeset/cool-grapes-attend.md b/.changeset/cool-grapes-attend.md new file mode 100644 index 00000000000..d848b6e3013 --- /dev/null +++ b/.changeset/cool-grapes-attend.md @@ -0,0 +1,5 @@ +--- +'@firebase/firestore': patch +--- + +Fix Firestore failing to raise initial snapshot from empty local cache result diff --git a/packages/firestore/test/unit/specs/listen_spec.test.ts b/packages/firestore/test/unit/specs/listen_spec.test.ts index b32f3fa0c0f..054110a6486 100644 --- a/packages/firestore/test/unit/specs/listen_spec.test.ts +++ b/packages/firestore/test/unit/specs/listen_spec.test.ts @@ -1759,14 +1759,9 @@ describeSpec('Listens:', [], () => { .userListens(query1) .watchAcksFull(query1, 1000) .expectEvents(query1, { fromCache: false }) - // Query is shared in second client - .client(1) - .expectListen(query1) - .client(0) .userUnlistens(query1) .watchRemoves(query1) .client(1) - .expectUnlisten(query1) // Re-listen to the query in second client and verify that the empty // snapshot is raised from cache. .userListens(query1, { resumeToken: 'resume-token-1000' }) @@ -1810,7 +1805,6 @@ describeSpec('Listens:', [], () => { .writeAcks('collection/a', 2000) .watchAcksFull(query1, 2000, doc1Deleted) .client(1) - .expectListen(query1, { resumeToken: 'resume-token-2000' }) .expectEvents(query1, { fromCache: false }) ); } diff --git a/repo-scripts/prune-dts/tests/firestore.input.d.ts b/repo-scripts/prune-dts/tests/firestore.input.d.ts index 6de435007df..73e938a1b17 100644 --- a/repo-scripts/prune-dts/tests/firestore.input.d.ts +++ b/repo-scripts/prune-dts/tests/firestore.input.d.ts @@ -4988,8 +4988,7 @@ declare class TargetChange { */ static createSynthesizedTargetChangeForCurrentChange( targetId: TargetId, - current: boolean, - resumeToken: ByteString + current: boolean ): TargetChange; } From 67d0ee75d235ae27de95e7b2df1d1bbe951dce47 Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Thu, 6 Oct 2022 09:34:22 -0700 Subject: [PATCH 14/18] add firebase to changeset, remove unnecessary resume token --- .changeset/cool-grapes-attend.md | 1 + packages/firestore/test/unit/specs/listen_spec.test.ts | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/.changeset/cool-grapes-attend.md b/.changeset/cool-grapes-attend.md index d848b6e3013..8f6d8b289b6 100644 --- a/.changeset/cool-grapes-attend.md +++ b/.changeset/cool-grapes-attend.md @@ -1,5 +1,6 @@ --- '@firebase/firestore': patch +'firebase': patch --- Fix Firestore failing to raise initial snapshot from empty local cache result diff --git a/packages/firestore/test/unit/specs/listen_spec.test.ts b/packages/firestore/test/unit/specs/listen_spec.test.ts index 054110a6486..8bc62391838 100644 --- a/packages/firestore/test/unit/specs/listen_spec.test.ts +++ b/packages/firestore/test/unit/specs/listen_spec.test.ts @@ -1764,7 +1764,7 @@ describeSpec('Listens:', [], () => { .client(1) // Re-listen to the query in second client and verify that the empty // snapshot is raised from cache. - .userListens(query1, { resumeToken: 'resume-token-1000' }) + .userListens(query1) .expectEvents(query1, { fromCache: true }) .client(0) .expectListen(query1, { resumeToken: 'resume-token-1000' }) From 9f25aab03c95861ed893f68ad9d608ac4db3dde6 Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Thu, 6 Oct 2022 12:57:01 -0700 Subject: [PATCH 15/18] use hasCachedResults instead of resumeToken --- packages/firestore/src/core/event_manager.ts | 6 +++--- packages/firestore/src/core/view.ts | 8 ++++---- packages/firestore/src/core/view_snapshot.ts | 7 +++---- packages/firestore/test/util/api_helpers.ts | 2 +- 4 files changed, 11 insertions(+), 12 deletions(-) diff --git a/packages/firestore/src/core/event_manager.ts b/packages/firestore/src/core/event_manager.ts index 51cafa0e5cc..77bac22beac 100644 --- a/packages/firestore/src/core/event_manager.ts +++ b/packages/firestore/src/core/event_manager.ts @@ -308,7 +308,7 @@ export class QueryListener { snap.fromCache, snap.syncStateChanged, /* excludesMetadataChanges= */ true, - snap.resumeToken + snap.hasCachedResults ); } let raisedEvent = false; @@ -376,7 +376,7 @@ export class QueryListener { // or we are offline. return ( !snap.docs.isEmpty() || - snap.resumeToken.approximateByteSize() > 0 || + snap.hasCachedResults || onlineState === OnlineState.Offline ); } @@ -412,7 +412,7 @@ export class QueryListener { snap.docs, snap.mutatedKeys, snap.fromCache, - snap.resumeToken + snap.hasCachedResults ); this.raisedInitialEvent = true; this.queryObserver.next(snap); diff --git a/packages/firestore/src/core/view.ts b/packages/firestore/src/core/view.ts index df6d8117599..9f571f48eda 100644 --- a/packages/firestore/src/core/view.ts +++ b/packages/firestore/src/core/view.ts @@ -26,7 +26,6 @@ import { DocumentKey } from '../model/document_key'; import { DocumentSet } from '../model/document_set'; import { TargetChange } from '../remote/remote_event'; import { debugAssert, fail } from '../util/assert'; -import { ByteString } from '../util/byte_string'; import { LimitType, newQueryComparator, Query, queryMatches } from './query'; import { OnlineState } from './types'; @@ -73,7 +72,7 @@ export interface ViewChange { */ export class View { private syncState: SyncState | null = null; - private resumeToken: ByteString | 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 @@ -322,7 +321,8 @@ export class View { newSyncState === SyncState.Local, syncStateChanged, /* excludesMetadataChanges= */ false, - targetChange?.resumeToken ?? ByteString.EMPTY_BYTE_STRING + targetChange !== undefined && + targetChange.resumeToken.approximateByteSize() > 0 ); return { snapshot: snap, @@ -472,7 +472,7 @@ export class View { this.documentSet, this.mutatedKeys, this.syncState === SyncState.Local, - this.resumeToken ?? ByteString.EMPTY_BYTE_STRING + this.hasCachedResults ); } } diff --git a/packages/firestore/src/core/view_snapshot.ts b/packages/firestore/src/core/view_snapshot.ts index adddd0d377d..d55f18f612e 100644 --- a/packages/firestore/src/core/view_snapshot.ts +++ b/packages/firestore/src/core/view_snapshot.ts @@ -20,7 +20,6 @@ import { Document } from '../model/document'; import { DocumentKey } from '../model/document_key'; import { DocumentSet } from '../model/document_set'; import { fail } from '../util/assert'; -import { ByteString } from '../util/byte_string'; import { SortedMap } from '../util/sorted_map'; import { Query, queryEquals } from './query'; @@ -148,7 +147,7 @@ export class ViewSnapshot { readonly fromCache: boolean, readonly syncStateChanged: boolean, readonly excludesMetadataChanges: boolean, - readonly resumeToken: ByteString + readonly hasCachedResults: boolean ) {} /** Returns a view snapshot as if all documents in the snapshot were added. */ @@ -157,7 +156,7 @@ export class ViewSnapshot { documents: DocumentSet, mutatedKeys: DocumentKeySet, fromCache: boolean, - resumeToken: ByteString + hasCachedResults: boolean ): ViewSnapshot { const changes: DocumentViewChange[] = []; documents.forEach(doc => { @@ -173,7 +172,7 @@ export class ViewSnapshot { fromCache, /* syncStateChanged= */ true, /* excludesMetadataChanges= */ false, - resumeToken + hasCachedResults ); } diff --git a/packages/firestore/test/util/api_helpers.ts b/packages/firestore/test/util/api_helpers.ts index 68d6f8a0b96..a76a7ca9c62 100644 --- a/packages/firestore/test/util/api_helpers.ts +++ b/packages/firestore/test/util/api_helpers.ts @@ -155,7 +155,7 @@ export function querySnapshot( fromCache, syncStateChanged, false, - resumeToken ?? ByteString.EMPTY_BYTE_STRING + resumeToken !== undefined && resumeToken.approximateByteSize() > 0 ); const db = firestore(); return new QuerySnapshot( From a9bee4edeaf721b2bd1e47fb1b7e29abaebba2b3 Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Thu, 6 Oct 2022 13:05:51 -0700 Subject: [PATCH 16/18] add hasCachedResults to viewSnapshot equality --- packages/firestore/src/core/view_snapshot.ts | 1 + .../firestore/test/unit/core/event_manager.test.ts | 14 +++++++------- 2 files changed, 8 insertions(+), 7 deletions(-) diff --git a/packages/firestore/src/core/view_snapshot.ts b/packages/firestore/src/core/view_snapshot.ts index d55f18f612e..f15c5ccb409 100644 --- a/packages/firestore/src/core/view_snapshot.ts +++ b/packages/firestore/src/core/view_snapshot.ts @@ -183,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/test/unit/core/event_manager.test.ts b/packages/firestore/test/unit/core/event_manager.test.ts index ab461057316..293514ceb6d 100644 --- a/packages/firestore/test/unit/core/event_manager.test.ts +++ b/packages/firestore/test/unit/core/event_manager.test.ts @@ -225,7 +225,7 @@ describe('QueryListener', () => { fromCache: snap2.fromCache, syncStateChanged: true, mutatedKeys: keys(), - resumeToken: snap2.resumeToken + hasCachedResults: snap2.hasCachedResults }; expect(otherEvents).to.deep.equal([expectedSnap2]); }); @@ -398,7 +398,7 @@ describe('QueryListener', () => { fromCache: snap2.fromCache, syncStateChanged: snap2.syncStateChanged, mutatedKeys: snap2.mutatedKeys, - resumeToken: snap2.resumeToken + hasCachedResults: snap2.hasCachedResults }; expect(filteredEvents).to.deep.equal([snap1, expectedSnap2]); } @@ -485,7 +485,7 @@ describe('QueryListener', () => { fromCache: false, syncStateChanged: true, mutatedKeys: keys(), - resumeToken: snap3.resumeToken + hasCachedResults: snap3.hasCachedResults }; expect(events).to.deep.equal([expectedSnap]); }); @@ -521,7 +521,7 @@ describe('QueryListener', () => { fromCache: true, syncStateChanged: true, mutatedKeys: keys(), - resumeToken: snap1.resumeToken + hasCachedResults: snap1.hasCachedResults }; const expectedSnap2 = { query: query1, @@ -531,7 +531,7 @@ describe('QueryListener', () => { fromCache: true, syncStateChanged: false, mutatedKeys: keys(), - resumeToken: snap2.resumeToken + hasCachedResults: snap2.hasCachedResults }; expect(events).to.deep.equal([expectedSnap1, expectedSnap2]); }); @@ -558,7 +558,7 @@ describe('QueryListener', () => { fromCache: true, syncStateChanged: true, mutatedKeys: keys(), - resumeToken: snap1.resumeToken + hasCachedResults: snap1.hasCachedResults }; expect(events).to.deep.equal([expectedSnap]); }); @@ -584,7 +584,7 @@ describe('QueryListener', () => { fromCache: true, syncStateChanged: true, mutatedKeys: keys(), - resumeToken: snap1.resumeToken + hasCachedResults: snap1.hasCachedResults }; expect(events).to.deep.equal([expectedSnap]); }); From 91f720d3a7765303d9f81bf7243db8c5ff4a0585 Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Thu, 6 Oct 2022 13:20:57 -0700 Subject: [PATCH 17/18] remove unnecessary resumeToken test --- packages/firestore/src/core/event_manager.ts | 2 +- .../firestore/test/unit/api/database.test.ts | 89 ++++++++++--------- packages/firestore/test/util/api_helpers.ts | 5 +- 3 files changed, 48 insertions(+), 48 deletions(-) diff --git a/packages/firestore/src/core/event_manager.ts b/packages/firestore/src/core/event_manager.ts index 77bac22beac..1723abd15d9 100644 --- a/packages/firestore/src/core/event_manager.ts +++ b/packages/firestore/src/core/event_manager.ts @@ -372,7 +372,7 @@ export class QueryListener { return false; } - // Raise data from cache if we have any documents or resume token, + // Raise data from cache if we have any documents, have cached results before, // or we are offline. return ( !snap.docs.isEmpty() || diff --git a/packages/firestore/test/unit/api/database.test.ts b/packages/firestore/test/unit/api/database.test.ts index 638f450cdcf..b0d6574716d 100644 --- a/packages/firestore/test/unit/api/database.test.ts +++ b/packages/firestore/test/unit/api/database.test.ts @@ -25,8 +25,6 @@ import { } from '../../../src'; import { EmulatorAuthCredentialsProvider } from '../../../src/api/credentials'; import { User } from '../../../src/auth/user'; -import { encodeBase64 } from '../../../src/platform/base64'; -import { ByteString } from '../../../src/util/byte_string'; import { collectionReference, documentReference, @@ -179,48 +177,51 @@ describe('QuerySnapshot', () => { querySnapshot('foo', {}, { a: { a: 1 } }, keys('foo/a'), false, true) ) ).to.be.false; - }); - - it('resume token should not effect querySnapshot equality', () => { - const resumeToken1 = ByteString.fromBase64String( - encodeBase64('ResumeToken1') - ); - const resumeToken1Copy = ByteString.fromBase64String( - encodeBase64('ResumeToken1') - ); - const resumeToken2 = ByteString.fromBase64String( - encodeBase64('ResumeToken2') - ); - - const snapshot1 = querySnapshot( - 'foo', - {}, - { a: { a: 1 } }, - keys(), - false, - false, - resumeToken1 - ); - const snapshot1Copy = querySnapshot( - 'foo', - {}, - { a: { a: 1 } }, - keys(), - false, - false, - resumeToken1Copy - ); - const snapshot2 = querySnapshot( - 'foo', - {}, - { a: { a: 1 } }, - keys(), - false, - false, - resumeToken2 - ); - expect(snapshotEqual(snapshot1, snapshot1Copy)).to.be.true; - expect(snapshotEqual(snapshot1, snapshot2)).to.be.true; + // hasCachedResults should effect 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/util/api_helpers.ts b/packages/firestore/test/util/api_helpers.ts index a76a7ca9c62..762b5258a29 100644 --- a/packages/firestore/test/util/api_helpers.ts +++ b/packages/firestore/test/util/api_helpers.ts @@ -43,7 +43,6 @@ import { import { DocumentKeySet } from '../../src/model/collections'; import { DocumentSet } from '../../src/model/document_set'; import { JsonObject } from '../../src/model/object_value'; -import { ByteString } from '../../src/util/byte_string'; import { TEST_PROJECT } from '../unit/local/persistence_test_helpers'; import { doc, key, path as pathFrom } from './helpers'; @@ -132,7 +131,7 @@ export function querySnapshot( mutatedKeys: DocumentKeySet, fromCache: boolean, syncStateChanged: boolean, - resumeToken?: ByteString + hasCachedResults?: boolean ): QuerySnapshot { const query: InternalQuery = newQueryForPath(pathFrom(path)); let oldDocuments: DocumentSet = new DocumentSet(); @@ -155,7 +154,7 @@ export function querySnapshot( fromCache, syncStateChanged, false, - resumeToken !== undefined && resumeToken.approximateByteSize() > 0 + hasCachedResults ?? false ); const db = firestore(); return new QuerySnapshot( From 3e1495b44214421d592398c42d3fda5bdd046417 Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Fri, 7 Oct 2022 10:47:41 -0700 Subject: [PATCH 18/18] resolve comments --- packages/firestore/src/core/view.ts | 5 +++-- packages/firestore/test/unit/api/database.test.ts | 2 +- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/packages/firestore/src/core/view.ts b/packages/firestore/src/core/view.ts index 9f571f48eda..de66d883ac3 100644 --- a/packages/firestore/src/core/view.ts +++ b/packages/firestore/src/core/view.ts @@ -321,8 +321,9 @@ export class View { newSyncState === SyncState.Local, syncStateChanged, /* excludesMetadataChanges= */ false, - targetChange !== undefined && - targetChange.resumeToken.approximateByteSize() > 0 + targetChange + ? targetChange.resumeToken.approximateByteSize() > 0 + : false ); return { snapshot: snap, diff --git a/packages/firestore/test/unit/api/database.test.ts b/packages/firestore/test/unit/api/database.test.ts index b0d6574716d..edc1a520f07 100644 --- a/packages/firestore/test/unit/api/database.test.ts +++ b/packages/firestore/test/unit/api/database.test.ts @@ -177,7 +177,7 @@ describe('QuerySnapshot', () => { querySnapshot('foo', {}, { a: { a: 1 } }, keys('foo/a'), false, true) ) ).to.be.false; - // hasCachedResults should effect querySnapshot equality + // hasCachedResults should affect querySnapshot equality expect( snapshotEqual( querySnapshot(