Skip to content

Commit 0a112bd

Browse files
milaGGLmilaGGL
authored
Fix Firestore failing to return empty results from the local cache (#6624)
1 parent 397317b commit 0a112bd

File tree

12 files changed

+269
-29
lines changed

12 files changed

+269
-29
lines changed

‎.changeset/cool-grapes-attend.md

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
---
2+
'@firebase/firestore': patch
3+
'firebase': patch
4+
---
5+
6+
Fix Firestore failing to raise initial snapshot from empty local cache result

‎packages/firestore/src/core/event_manager.ts

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -307,7 +307,8 @@ export class QueryListener {
307307
snap.mutatedKeys,
308308
snap.fromCache,
309309
snap.syncStateChanged,
310-
/* excludesMetadataChanges= */ true
310+
/* excludesMetadataChanges= */ true,
311+
snap.hasCachedResults
311312
);
312313
}
313314
let raisedEvent = false;
@@ -371,8 +372,13 @@ export class QueryListener {
371372
return false;
372373
}
373374

374-
// Raise data from cache if we have any documents or we are offline
375-
return !snap.docs.isEmpty() || onlineState === OnlineState.Offline;
375+
// Raise data from cache if we have any documents, have cached results before,
376+
// or we are offline.
377+
return (
378+
!snap.docs.isEmpty() ||
379+
snap.hasCachedResults ||
380+
onlineState === OnlineState.Offline
381+
);
376382
}
377383

378384
private shouldRaiseEvent(snap: ViewSnapshot): boolean {
@@ -405,7 +411,8 @@ export class QueryListener {
405411
snap.query,
406412
snap.docs,
407413
snap.mutatedKeys,
408-
snap.fromCache
414+
snap.fromCache,
415+
snap.hasCachedResults
409416
);
410417
this.raisedInitialEvent = true;
411418
this.queryObserver.next(snap);

‎packages/firestore/src/core/sync_engine_impl.ts

Lines changed: 13 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -64,6 +64,7 @@ import {
6464
import { debugAssert, debugCast, fail, hardAssert } from '../util/assert';
6565
import { wrapInUserErrorIfRecoverable } from '../util/async_queue';
6666
import { BundleReader } from '../util/bundle_reader';
67+
import { ByteString } from '../util/byte_string';
6768
import { Code, FirestoreError } from '../util/error';
6869
import { logDebug, logWarn } from '../util/log';
6970
import { primitiveComparator } from '../util/misc';
@@ -327,7 +328,8 @@ export async function syncEngineListen(
327328
syncEngineImpl,
328329
query,
329330
targetId,
330-
status === 'current'
331+
status === 'current',
332+
targetData.resumeToken
331333
);
332334
}
333335

@@ -342,7 +344,8 @@ async function initializeViewAndComputeSnapshot(
342344
syncEngineImpl: SyncEngineImpl,
343345
query: Query,
344346
targetId: TargetId,
345-
current: boolean
347+
current: boolean,
348+
resumeToken: ByteString
346349
): Promise<ViewSnapshot> {
347350
// PORTING NOTE: On Web only, we inject the code that registers new Limbo
348351
// targets based on view changes. This allows us to only depend on Limbo
@@ -360,7 +363,8 @@ async function initializeViewAndComputeSnapshot(
360363
const synthesizedTargetChange =
361364
TargetChange.createSynthesizedTargetChangeForCurrentChange(
362365
targetId,
363-
current && syncEngineImpl.onlineState !== OnlineState.Offline
366+
current && syncEngineImpl.onlineState !== OnlineState.Offline,
367+
resumeToken
364368
);
365369
const viewChange = view.applyChanges(
366370
viewDocChanges,
@@ -1385,7 +1389,8 @@ async function synchronizeQueryViewsAndRaiseSnapshots(
13851389
syncEngineImpl,
13861390
synthesizeTargetToQuery(target!),
13871391
targetId,
1388-
/*current=*/ false
1392+
/*current=*/ false,
1393+
targetData.resumeToken
13891394
);
13901395
}
13911396

@@ -1457,7 +1462,8 @@ export async function syncEngineApplyTargetState(
14571462
const synthesizedRemoteEvent =
14581463
RemoteEvent.createSynthesizedRemoteEventForCurrentChange(
14591464
targetId,
1460-
state === 'current'
1465+
state === 'current',
1466+
ByteString.EMPTY_BYTE_STRING
14611467
);
14621468
await syncEngineEmitNewSnapsAndNotifyLocalStore(
14631469
syncEngineImpl,
@@ -1512,7 +1518,8 @@ export async function syncEngineApplyActiveTargetsChange(
15121518
syncEngineImpl,
15131519
synthesizeTargetToQuery(target),
15141520
targetData.targetId,
1515-
/*current=*/ false
1521+
/*current=*/ false,
1522+
targetData.resumeToken
15161523
);
15171524
remoteStoreListen(syncEngineImpl.remoteStore, targetData);
15181525
}

‎packages/firestore/src/core/view.ts

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -72,6 +72,7 @@ export interface ViewChange {
7272
*/
7373
export class View {
7474
private syncState: SyncState | null = null;
75+
private hasCachedResults: boolean = false;
7576
/**
7677
* A flag whether the view is current with the backend. A view is considered
7778
* current after it has seen the current flag from the backend and did not
@@ -319,7 +320,10 @@ export class View {
319320
docChanges.mutatedKeys,
320321
newSyncState === SyncState.Local,
321322
syncStateChanged,
322-
/* excludesMetadataChanges= */ false
323+
/* excludesMetadataChanges= */ false,
324+
targetChange
325+
? targetChange.resumeToken.approximateByteSize() > 0
326+
: false
323327
);
324328
return {
325329
snapshot: snap,
@@ -468,7 +472,8 @@ export class View {
468472
this.query,
469473
this.documentSet,
470474
this.mutatedKeys,
471-
this.syncState === SyncState.Local
475+
this.syncState === SyncState.Local,
476+
this.hasCachedResults
472477
);
473478
}
474479
}

‎packages/firestore/src/core/view_snapshot.ts

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -146,15 +146,17 @@ export class ViewSnapshot {
146146
readonly mutatedKeys: DocumentKeySet,
147147
readonly fromCache: boolean,
148148
readonly syncStateChanged: boolean,
149-
readonly excludesMetadataChanges: boolean
149+
readonly excludesMetadataChanges: boolean,
150+
readonly hasCachedResults: boolean
150151
) {}
151152

152153
/** Returns a view snapshot as if all documents in the snapshot were added. */
153154
static fromInitialDocuments(
154155
query: Query,
155156
documents: DocumentSet,
156157
mutatedKeys: DocumentKeySet,
157-
fromCache: boolean
158+
fromCache: boolean,
159+
hasCachedResults: boolean
158160
): ViewSnapshot {
159161
const changes: DocumentViewChange[] = [];
160162
documents.forEach(doc => {
@@ -169,7 +171,8 @@ export class ViewSnapshot {
169171
mutatedKeys,
170172
fromCache,
171173
/* syncStateChanged= */ true,
172-
/* excludesMetadataChanges= */ false
174+
/* excludesMetadataChanges= */ false,
175+
hasCachedResults
173176
);
174177
}
175178

@@ -180,6 +183,7 @@ export class ViewSnapshot {
180183
isEqual(other: ViewSnapshot): boolean {
181184
if (
182185
this.fromCache !== other.fromCache ||
186+
this.hasCachedResults !== other.hasCachedResults ||
183187
this.syncStateChanged !== other.syncStateChanged ||
184188
!this.mutatedKeys.isEqual(other.mutatedKeys) ||
185189
!queryEquals(this.query, other.query) ||

‎packages/firestore/src/remote/remote_event.ts

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -67,14 +67,16 @@ export class RemoteEvent {
6767
// PORTING NOTE: Multi-tab only
6868
static createSynthesizedRemoteEventForCurrentChange(
6969
targetId: TargetId,
70-
current: boolean
70+
current: boolean,
71+
resumeToken: ByteString
7172
): RemoteEvent {
7273
const targetChanges = new Map<TargetId, TargetChange>();
7374
targetChanges.set(
7475
targetId,
7576
TargetChange.createSynthesizedTargetChangeForCurrentChange(
7677
targetId,
77-
current
78+
current,
79+
resumeToken
7880
)
7981
);
8082
return new RemoteEvent(
@@ -134,10 +136,11 @@ export class TargetChange {
134136
*/
135137
static createSynthesizedTargetChangeForCurrentChange(
136138
targetId: TargetId,
137-
current: boolean
139+
current: boolean,
140+
resumeToken: ByteString
138141
): TargetChange {
139142
return new TargetChange(
140-
ByteString.EMPTY_BYTE_STRING,
143+
resumeToken,
141144
current,
142145
documentKeySet(),
143146
documentKeySet(),

‎packages/firestore/test/integration/api/query.test.ts

Lines changed: 40 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1281,13 +1281,52 @@ apiDescribe('Queries', (persistence: boolean) => {
12811281
};
12821282

12831283
return withTestCollection(persistence, testDocs, async coll => {
1284-
await getDocs(query(coll)); // Populate the cache
1284+
await getDocs(query(coll)); // Populate the cache.
12851285
const snapshot = await getDocs(
12861286
query(coll, where('map.nested', '==', 'foo'))
12871287
);
12881288
expect(toDataArray(snapshot)).to.deep.equal([{ map: { nested: 'foo' } }]);
12891289
});
12901290
});
1291+
1292+
// Reproduces https://github.com/firebase/firebase-js-sdk/issues/5873
1293+
// eslint-disable-next-line no-restricted-properties
1294+
(persistence ? describe : describe.skip)('Caching empty results', () => {
1295+
it('can raise initial snapshot from cache, even if it is empty', () => {
1296+
return withTestCollection(persistence, {}, async coll => {
1297+
const snapshot1 = await getDocs(coll); // Populate the cache.
1298+
expect(snapshot1.metadata.fromCache).to.be.false;
1299+
expect(toDataArray(snapshot1)).to.deep.equal([]); // Precondition check.
1300+
1301+
// Add a snapshot listener whose first event should be raised from cache.
1302+
const storeEvent = new EventsAccumulator<QuerySnapshot>();
1303+
onSnapshot(coll, storeEvent.storeEvent);
1304+
const snapshot2 = await storeEvent.awaitEvent();
1305+
expect(snapshot2.metadata.fromCache).to.be.true;
1306+
expect(toDataArray(snapshot2)).to.deep.equal([]);
1307+
});
1308+
});
1309+
1310+
it('can raise initial snapshot from cache, even if it has become empty', () => {
1311+
const testDocs = {
1312+
a: { key: 'a' }
1313+
};
1314+
return withTestCollection(persistence, testDocs, async coll => {
1315+
// Populate the cache.
1316+
const snapshot1 = await getDocs(coll);
1317+
expect(snapshot1.metadata.fromCache).to.be.false;
1318+
expect(toDataArray(snapshot1)).to.deep.equal([{ key: 'a' }]);
1319+
// Empty the collection.
1320+
void deleteDoc(doc(coll, 'a'));
1321+
1322+
const storeEvent = new EventsAccumulator<QuerySnapshot>();
1323+
onSnapshot(coll, storeEvent.storeEvent);
1324+
const snapshot2 = await storeEvent.awaitEvent();
1325+
expect(snapshot2.metadata.fromCache).to.be.true;
1326+
expect(toDataArray(snapshot2)).to.deep.equal([]);
1327+
});
1328+
});
1329+
});
12911330
});
12921331

12931332
function verifyDocumentChange<T>(

‎packages/firestore/test/unit/api/database.test.ts

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -177,6 +177,51 @@ describe('QuerySnapshot', () => {
177177
querySnapshot('foo', {}, { a: { a: 1 } }, keys('foo/a'), false, true)
178178
)
179179
).to.be.false;
180+
// hasCachedResults should affect querySnapshot equality
181+
expect(
182+
snapshotEqual(
183+
querySnapshot(
184+
'foo',
185+
{},
186+
{ a: { a: 1 } },
187+
keys('foo/a'),
188+
false,
189+
false,
190+
true
191+
),
192+
querySnapshot(
193+
'foo',
194+
{},
195+
{ a: { a: 1 } },
196+
keys('foo/a'),
197+
false,
198+
false,
199+
true
200+
)
201+
)
202+
).to.be.true;
203+
expect(
204+
snapshotEqual(
205+
querySnapshot(
206+
'foo',
207+
{},
208+
{ a: { a: 1 } },
209+
keys('foo/a'),
210+
false,
211+
false,
212+
true
213+
),
214+
querySnapshot(
215+
'foo',
216+
{},
217+
{ a: { a: 1 } },
218+
keys('foo/a'),
219+
false,
220+
false,
221+
false
222+
)
223+
)
224+
).to.be.false;
180225
});
181226

182227
it('JSON.stringify() does not throw', () => {

‎packages/firestore/test/unit/core/event_manager.test.ts

Lines changed: 14 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -224,7 +224,8 @@ describe('QueryListener', () => {
224224
docChanges: [change1, change4],
225225
fromCache: snap2.fromCache,
226226
syncStateChanged: true,
227-
mutatedKeys: keys()
227+
mutatedKeys: keys(),
228+
hasCachedResults: snap2.hasCachedResults
228229
};
229230
expect(otherEvents).to.deep.equal([expectedSnap2]);
230231
});
@@ -396,7 +397,8 @@ describe('QueryListener', () => {
396397
docChanges: [change3],
397398
fromCache: snap2.fromCache,
398399
syncStateChanged: snap2.syncStateChanged,
399-
mutatedKeys: snap2.mutatedKeys
400+
mutatedKeys: snap2.mutatedKeys,
401+
hasCachedResults: snap2.hasCachedResults
400402
};
401403
expect(filteredEvents).to.deep.equal([snap1, expectedSnap2]);
402404
}
@@ -482,7 +484,8 @@ describe('QueryListener', () => {
482484
],
483485
fromCache: false,
484486
syncStateChanged: true,
485-
mutatedKeys: keys()
487+
mutatedKeys: keys(),
488+
hasCachedResults: snap3.hasCachedResults
486489
};
487490
expect(events).to.deep.equal([expectedSnap]);
488491
});
@@ -517,7 +520,8 @@ describe('QueryListener', () => {
517520
docChanges: [{ type: ChangeType.Added, doc: doc1 }],
518521
fromCache: true,
519522
syncStateChanged: true,
520-
mutatedKeys: keys()
523+
mutatedKeys: keys(),
524+
hasCachedResults: snap1.hasCachedResults
521525
};
522526
const expectedSnap2 = {
523527
query: query1,
@@ -526,7 +530,8 @@ describe('QueryListener', () => {
526530
docChanges: [{ type: ChangeType.Added, doc: doc2 }],
527531
fromCache: true,
528532
syncStateChanged: false,
529-
mutatedKeys: keys()
533+
mutatedKeys: keys(),
534+
hasCachedResults: snap2.hasCachedResults
530535
};
531536
expect(events).to.deep.equal([expectedSnap1, expectedSnap2]);
532537
});
@@ -552,7 +557,8 @@ describe('QueryListener', () => {
552557
docChanges: [],
553558
fromCache: true,
554559
syncStateChanged: true,
555-
mutatedKeys: keys()
560+
mutatedKeys: keys(),
561+
hasCachedResults: snap1.hasCachedResults
556562
};
557563
expect(events).to.deep.equal([expectedSnap]);
558564
});
@@ -577,7 +583,8 @@ describe('QueryListener', () => {
577583
docChanges: [],
578584
fromCache: true,
579585
syncStateChanged: true,
580-
mutatedKeys: keys()
586+
mutatedKeys: keys(),
587+
hasCachedResults: snap1.hasCachedResults
581588
};
582589
expect(events).to.deep.equal([expectedSnap]);
583590
});

0 commit comments

Comments
 (0)