Skip to content

Commit 9daee19

Browse files
dconeybedconeybe
committed
POC fix for returning empty result set from cache
1 parent 4ddf556 commit 9daee19

File tree

8 files changed

+117
-50
lines changed

8 files changed

+117
-50
lines changed

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

Lines changed: 7 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.resumeToken
311312
);
312313
}
313314
let raisedEvent = false;
@@ -371,8 +372,9 @@ 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 previously executed the query and have
376+
// cached data or we are offline
377+
return snap.resumeToken.approximateByteSize() > 0 || onlineState === OnlineState.Offline;
376378
}
377379

378380
private shouldRaiseEvent(snap: ViewSnapshot): boolean {
@@ -405,7 +407,8 @@ export class QueryListener {
405407
snap.query,
406408
snap.docs,
407409
snap.mutatedKeys,
408-
snap.fromCache
410+
snap.fromCache,
411+
snap.resumeToken
409412
);
410413
this.raisedInitialEvent = true;
411414
this.queryObserver.next(snap);

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

Lines changed: 15 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -114,6 +114,7 @@ import {
114114
ViewChange
115115
} from './view';
116116
import { ViewSnapshot } from './view_snapshot';
117+
import {ByteString} from '../util/byte_string';
117118

118119
const LOG_TAG = 'SyncEngine';
119120

@@ -323,7 +324,8 @@ export async function syncEngineListen(
323324
syncEngineImpl,
324325
query,
325326
targetId,
326-
status === 'current'
327+
status === 'current',
328+
targetData.resumeToken
327329
);
328330
if (syncEngineImpl.isPrimaryClient) {
329331
remoteStoreListen(syncEngineImpl.remoteStore, targetData);
@@ -341,7 +343,8 @@ async function initializeViewAndComputeSnapshot(
341343
syncEngineImpl: SyncEngineImpl,
342344
query: Query,
343345
targetId: TargetId,
344-
current: boolean
346+
current: boolean,
347+
resumeToken: ByteString
345348
): Promise<ViewSnapshot> {
346349
// PORTING NOTE: On Web only, we inject the code that registers new Limbo
347350
// targets based on view changes. This allows us to only depend on Limbo
@@ -359,12 +362,13 @@ async function initializeViewAndComputeSnapshot(
359362
const synthesizedTargetChange =
360363
TargetChange.createSynthesizedTargetChangeForCurrentChange(
361364
targetId,
362-
current && syncEngineImpl.onlineState !== OnlineState.Offline
365+
current && syncEngineImpl.onlineState !== OnlineState.Offline,
366+
resumeToken
363367
);
364368
const viewChange = view.applyChanges(
365369
viewDocChanges,
366370
/* updateLimboDocuments= */ syncEngineImpl.isPrimaryClient,
367-
synthesizedTargetChange
371+
synthesizedTargetChange,
368372
);
369373
updateTrackedLimbos(syncEngineImpl, targetId, viewChange.limboChanges);
370374

@@ -1018,7 +1022,7 @@ export async function syncEngineEmitNewSnapsAndNotifyLocalStore(
10181022
if (syncEngineImpl.isPrimaryClient) {
10191023
syncEngineImpl.sharedClientState.updateQueryState(
10201024
queryView.targetId,
1021-
viewSnapshot.fromCache ? 'not-current' : 'current'
1025+
viewSnapshot.fromCache ? 'not-current' : 'current',
10221026
);
10231027
}
10241028
newSnaps.push(viewSnapshot);
@@ -1369,7 +1373,8 @@ async function synchronizeQueryViewsAndRaiseSnapshots(
13691373
syncEngineImpl,
13701374
synthesizeTargetToQuery(target!),
13711375
targetId,
1372-
/*current=*/ false
1376+
/*current=*/ false,
1377+
targetData.resumeToken
13731378
);
13741379
}
13751380

@@ -1439,7 +1444,8 @@ export async function syncEngineApplyTargetState(
14391444
const synthesizedRemoteEvent =
14401445
RemoteEvent.createSynthesizedRemoteEventForCurrentChange(
14411446
targetId,
1442-
state === 'current'
1447+
state === 'current',
1448+
ByteString.EMPTY_BYTE_STRING
14431449
);
14441450
await syncEngineEmitNewSnapsAndNotifyLocalStore(
14451451
syncEngineImpl,
@@ -1494,7 +1500,8 @@ export async function syncEngineApplyActiveTargetsChange(
14941500
syncEngineImpl,
14951501
synthesizeTargetToQuery(target),
14961502
targetData.targetId,
1497-
/*current=*/ false
1503+
/*current=*/ false,
1504+
targetData.resumeToken
14981505
);
14991506
remoteStoreListen(syncEngineImpl.remoteStore, targetData);
15001507
}

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

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,7 @@ import {
4141
SyncState,
4242
ViewSnapshot
4343
} from './view_snapshot';
44+
import {ByteString} from '../util/byte_string';
4445

4546
export type LimboDocumentChange = AddedLimboDocument | RemovedLimboDocument;
4647
export class AddedLimboDocument {
@@ -78,6 +79,7 @@ export interface ViewChange {
7879
*/
7980
export class View {
8081
private syncState: SyncState | null = null;
82+
private resumeToken: ByteString | null = null;
8183
/**
8284
* A flag whether the view is current with the backend. A view is considered
8385
* current after it has seen the current flag from the backend and did not
@@ -309,11 +311,13 @@ export class View {
309311
const newSyncState = synced ? SyncState.Synced : SyncState.Local;
310312
const syncStateChanged = newSyncState !== this.syncState;
311313
this.syncState = newSyncState;
314+
this.resumeToken = targetChange?.resumeToken ?? null;
312315

313316
if (changes.length === 0 && !syncStateChanged) {
314317
// no changes
315318
return { limboChanges };
316319
} else {
320+
317321
const snap: ViewSnapshot = new ViewSnapshot(
318322
this.query,
319323
docChanges.documentSet,
@@ -322,7 +326,8 @@ export class View {
322326
docChanges.mutatedKeys,
323327
newSyncState === SyncState.Local,
324328
syncStateChanged,
325-
/* excludesMetadataChanges= */ false
329+
/* excludesMetadataChanges= */ false,
330+
targetChange?.resumeToken ?? ByteString.EMPTY_BYTE_STRING
326331
);
327332
return {
328333
snapshot: snap,
@@ -471,7 +476,8 @@ export class View {
471476
this.query,
472477
this.documentSet,
473478
this.mutatedKeys,
474-
this.syncState === SyncState.Local
479+
this.syncState === SyncState.Local,
480+
this.resumeToken ?? ByteString.EMPTY_BYTE_STRING
475481
);
476482
}
477483
}

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

Lines changed: 12 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ import { fail } from '../util/assert';
2323
import { SortedMap } from '../util/sorted_map';
2424

2525
import { Query, queryEquals } from './query';
26+
import {ByteString} from '../util/byte_string';
2627

2728
export const enum ChangeType {
2829
Added,
@@ -146,15 +147,19 @@ export class ViewSnapshot {
146147
readonly mutatedKeys: DocumentKeySet,
147148
readonly fromCache: boolean,
148149
readonly syncStateChanged: boolean,
149-
readonly excludesMetadataChanges: boolean
150-
) {}
150+
readonly excludesMetadataChanges: boolean,
151+
readonly resumeToken: ByteString
152+
) {
153+
1 == 1;
154+
}
151155

152156
/** Returns a view snapshot as if all documents in the snapshot were added. */
153157
static fromInitialDocuments(
154158
query: Query,
155159
documents: DocumentSet,
156160
mutatedKeys: DocumentKeySet,
157-
fromCache: boolean
161+
fromCache: boolean,
162+
resumeToken: ByteString
158163
): ViewSnapshot {
159164
const changes: DocumentViewChange[] = [];
160165
documents.forEach(doc => {
@@ -169,7 +174,8 @@ export class ViewSnapshot {
169174
mutatedKeys,
170175
fromCache,
171176
/* syncStateChanged= */ true,
172-
/* excludesMetadataChanges= */ false
177+
/* excludesMetadataChanges= */ false,
178+
resumeToken
173179
);
174180
}
175181

@@ -184,7 +190,8 @@ export class ViewSnapshot {
184190
!this.mutatedKeys.isEqual(other.mutatedKeys) ||
185191
!queryEquals(this.query, other.query) ||
186192
!this.docs.isEqual(other.docs) ||
187-
!this.oldDocs.isEqual(other.oldDocs)
193+
!this.oldDocs.isEqual(other.oldDocs) ||
194+
this.resumeToken !== other.resumeToken
188195
) {
189196
return false;
190197
}

‎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: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1200,6 +1200,25 @@ apiDescribe('Queries', (persistence: boolean) => {
12001200
expect(toDataArray(snapshot)).to.deep.equal([{ map: { nested: 'foo' } }]);
12011201
});
12021202
});
1203+
1204+
// eslint-disable-next-line no-restricted-properties
1205+
(persistence ? it.only : it.skip)('empty query results are cached', () => {
1206+
// Reproduces https://github.com/firebase/firebase-js-sdk/issues/5873
1207+
return withTestCollection(persistence, {}, async coll => {
1208+
const snapshot1 = await coll.get(); // Populate the cache
1209+
expect(snapshot1.metadata.fromCache).to.be.false;
1210+
expect(toDataArray(snapshot1)).to.deep.equal([]); // Precondition check
1211+
1212+
// Add a snapshot listener whose first event should be raised from cache.
1213+
const storeEvent = new EventsAccumulator<firestore.QuerySnapshot>();
1214+
coll.onSnapshot(storeEvent.storeEvent);
1215+
const snapshot2 = await storeEvent.awaitEvent();
1216+
1217+
expect(snapshot2.metadata.fromCache).to.be.true;
1218+
expect(toDataArray(snapshot2)).to.deep.equal([]);
1219+
});
1220+
});
1221+
12031222
});
12041223

12051224
function verifyDocumentChange<T>(

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

Lines changed: 43 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,8 @@ import {
2828
querySnapshot
2929
} from '../../util/api_helpers';
3030
import { expectEqual, expectNotEqual, keys } from '../../util/helpers';
31+
import {ByteString} from '../../../src/util/byte_string';
32+
import {encodeBase64} from '../../../src/platform/base64';
3133

3234
describe('CollectionReference', () => {
3335
it('support equality checking with isEqual()', () => {
@@ -101,50 +103,64 @@ describe('Query', () => {
101103

102104
describe('QuerySnapshot', () => {
103105
it('support equality checking with isEqual()', () => {
106+
const resumeToken1 = ByteString.fromBase64String(encodeBase64("ResumeToken1"));
107+
const resumeToken1Copy = ByteString.fromBase64String(encodeBase64("ResumeToken1"));
108+
const resumeToken2 = ByteString.fromBase64String(encodeBase64("ResumeToken2"));
109+
110+
expectEqual(
111+
querySnapshot('foo', {}, { a: { a: 1 } }, keys(), false, false, resumeToken1),
112+
querySnapshot('foo', {}, { a: { a: 1 } }, keys(), false, false, resumeToken1)
113+
);
104114
expectEqual(
105-
querySnapshot('foo', {}, { a: { a: 1 } }, keys(), false, false),
106-
querySnapshot('foo', {}, { a: { a: 1 } }, keys(), false, false)
115+
querySnapshot('foo', {}, { a: { a: 1 } }, keys(), false, false, resumeToken1),
116+
querySnapshot('foo', {}, { a: { a: 1 } }, keys(), false, false, resumeToken1Copy)
107117
);
108118
expectNotEqual(
109-
querySnapshot('foo', {}, { a: { a: 1 } }, keys(), false, false),
110-
querySnapshot('bar', {}, { a: { a: 1 } }, keys(), false, false)
119+
querySnapshot('foo', {}, { a: { a: 1 } }, keys(), false, false, resumeToken1),
120+
querySnapshot('bar', {}, { a: { a: 1 } }, keys(), false, false, resumeToken1)
111121
);
112122
expectNotEqual(
113-
querySnapshot('foo', {}, { a: { a: 1 } }, keys(), false, false),
123+
querySnapshot('foo', {}, { a: { a: 1 } }, keys(), false, false, resumeToken1),
114124
querySnapshot(
115125
'foo',
116126
{ b: { b: 1 } },
117127
{ a: { a: 1 } },
118128
keys(),
119129
false,
120-
false
130+
false,
131+
resumeToken1
121132
)
122133
);
123134
expectNotEqual(
124-
querySnapshot('foo', {}, { a: { a: 1 } }, keys(), false, false),
125-
querySnapshot('foo', {}, { a: { b: 1 } }, keys(), false, false)
135+
querySnapshot('foo', {}, { a: { a: 1 } }, keys(), false, false, resumeToken1),
136+
querySnapshot('foo', {}, { a: { b: 1 } }, keys(), false, false, resumeToken1)
126137
);
127138
expectNotEqual(
128-
querySnapshot('foo', {}, { a: { a: 1 } }, keys('foo/a'), false, false),
129-
querySnapshot('foo', {}, { a: { a: 1 } }, keys(), false, false)
139+
querySnapshot('foo', {}, { a: { a: 1 } }, keys('foo/a'), false, false, resumeToken1),
140+
querySnapshot('foo', {}, { a: { a: 1 } }, keys(), false, false, resumeToken1)
130141
);
131142
expectNotEqual(
132-
querySnapshot('foo', {}, { a: { a: 1 } }, keys('foo/a'), false, false),
133-
querySnapshot('foo', {}, { a: { a: 1 } }, keys('foo/b'), false, false)
143+
querySnapshot('foo', {}, { a: { a: 1 } }, keys('foo/a'), false, false, resumeToken1),
144+
querySnapshot('foo', {}, { a: { a: 1 } }, keys('foo/b'), false, false, resumeToken1)
134145
);
135146
expectNotEqual(
136-
querySnapshot('foo', {}, { a: { a: 1 } }, keys('foo/a'), false, false),
137-
querySnapshot('foo', {}, { a: { a: 1 } }, keys('foo/a'), true, false)
147+
querySnapshot('foo', {}, { a: { a: 1 } }, keys('foo/a'), false, false, resumeToken1),
148+
querySnapshot('foo', {}, { a: { a: 1 } }, keys('foo/a'), true, false, resumeToken1)
138149
);
139150
expectNotEqual(
140-
querySnapshot('foo', {}, { a: { a: 1 } }, keys('foo/a'), false, false),
141-
querySnapshot('foo', {}, { a: { a: 1 } }, keys('foo/a'), false, true)
151+
querySnapshot('foo', {}, { a: { a: 1 } }, keys('foo/a'), false, false, resumeToken1),
152+
querySnapshot('foo', {}, { a: { a: 1 } }, keys('foo/a'), false, true, resumeToken1)
153+
);
154+
expectNotEqual(
155+
querySnapshot('foo', {}, { a: { a: 1 } }, keys('foo/a'), false, false, resumeToken1),
156+
querySnapshot('foo', {}, { a: { a: 1 } }, keys('foo/a'), false, false, resumeToken2),
142157
);
143158
});
144159

145160
it('JSON.stringify() does not throw', () => {
161+
const resumeToken = ByteString.fromBase64String(encodeBase64("ResumeToken"));
146162
JSON.stringify(
147-
querySnapshot('foo', {}, { a: { a: 1 } }, keys(), false, false)
163+
querySnapshot('foo', {}, { a: { a: 1 } }, keys(), false, false, resumeToken)
148164
);
149165
});
150166
});
@@ -162,21 +178,23 @@ describe('SnapshotMetadata', () => {
162178
});
163179

164180
it('from QuerySnapshot support equality checking with isEqual()', () => {
181+
const resumeToken = ByteString.fromBase64String(encodeBase64("ResumeToken"));
182+
165183
expectEqual(
166-
querySnapshot('foo', {}, {}, keys('foo/a'), true, false).metadata,
167-
querySnapshot('foo', {}, {}, keys('foo/a'), true, false).metadata
184+
querySnapshot('foo', {}, {}, keys('foo/a'), true, false, resumeToken).metadata,
185+
querySnapshot('foo', {}, {}, keys('foo/a'), true, false, resumeToken).metadata
168186
);
169187
expectNotEqual(
170-
querySnapshot('foo', {}, {}, keys('foo/a'), true, false).metadata,
171-
querySnapshot('foo', {}, {}, keys(), true, false).metadata
188+
querySnapshot('foo', {}, {}, keys('foo/a'), true, false, resumeToken).metadata,
189+
querySnapshot('foo', {}, {}, keys(), true, false, resumeToken).metadata
172190
);
173191
expectNotEqual(
174-
querySnapshot('foo', {}, {}, keys('foo/a'), true, false).metadata,
175-
querySnapshot('foo', {}, {}, keys('foo/a'), false, false).metadata
192+
querySnapshot('foo', {}, {}, keys('foo/a'), true, false, resumeToken).metadata,
193+
querySnapshot('foo', {}, {}, keys('foo/a'), false, false, resumeToken).metadata
176194
);
177195
expectNotEqual(
178-
querySnapshot('foo', {}, {}, keys('foo/a'), true, false).metadata,
179-
querySnapshot('foo', {}, {}, keys(), false, false).metadata
196+
querySnapshot('foo', {}, {}, keys('foo/a'), true, false, resumeToken).metadata,
197+
querySnapshot('foo', {}, {}, keys(), false, false, resumeToken).metadata
180198
);
181199
});
182200
});

0 commit comments

Comments
 (0)