Skip to content

Commit 41107ad

Browse files
schmidt-sebastianschmidt-sebastian
authored
Merge 4388eda into be7ccb8 
2 parents be7ccb8 + 4388eda commit 41107ad

File tree

6 files changed

+154
-25
lines changed

6 files changed

+154
-25
lines changed

‎.changeset/unlucky-files-smoke.md

+5
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
"@firebase/firestore": patch
3+
---
4+
5+
Fixed an issue that can cause incomplete Query snapshots when the SDK is backgrounded during query execution.

‎packages/firestore/src/local/local_store_impl.ts

+26-21
Original file line numberDiff line numberDiff line change
@@ -501,24 +501,34 @@ export function localStoreApplyRemoteEventToLocalCache(
501501
})
502502
);
503503

504-
const resumeToken = change.resumeToken;
505-
// Update the resume token if the change includes one.
506-
if (resumeToken.approximateByteSize() > 0) {
507-
const newTargetData = oldTargetData
508-
.withResumeToken(resumeToken, remoteVersion)
509-
.withSequenceNumber(txn.currentSequenceNumber);
510-
newTargetDataByTargetMap = newTargetDataByTargetMap.insert(
511-
targetId,
512-
newTargetData
504+
let newTargetData = oldTargetData.withSequenceNumber(
505+
txn.currentSequenceNumber
506+
);
507+
if (remoteEvent.targetMismatches.has(targetId)) {
508+
newTargetData = newTargetData
509+
.withResumeToken(
510+
ByteString.EMPTY_BYTE_STRING,
511+
SnapshotVersion.min()
512+
)
513+
.withLastLimboFreeSnapshotVersion(SnapshotVersion.min());
514+
} else if (change.resumeToken.approximateByteSize() > 0) {
515+
newTargetData = newTargetData.withResumeToken(
516+
change.resumeToken,
517+
remoteVersion
513518
);
519+
}
514520

515-
// Update the target data if there are target changes (or if
516-
// sufficient time has passed since the last update).
517-
if (shouldPersistTargetData(oldTargetData, newTargetData, change)) {
518-
promises.push(
519-
localStoreImpl.targetCache.updateTargetData(txn, newTargetData)
520-
);
521-
}
521+
newTargetDataByTargetMap = newTargetDataByTargetMap.insert(
522+
targetId,
523+
newTargetData
524+
);
525+
526+
// Update the target data if there are target changes (or if
527+
// sufficient time has passed since the last update).
528+
if (shouldPersistTargetData(oldTargetData, newTargetData, change)) {
529+
promises.push(
530+
localStoreImpl.targetCache.updateTargetData(txn, newTargetData)
531+
);
522532
}
523533
});
524534

@@ -675,11 +685,6 @@ function shouldPersistTargetData(
675685
newTargetData: TargetData,
676686
change: TargetChange
677687
): boolean {
678-
hardAssert(
679-
newTargetData.resumeToken.approximateByteSize() > 0,
680-
'Attempted to persist target data with no resume token'
681-
);
682-
683688
// Always persist target data if we don't already have a resume token.
684689
if (oldTargetData.resumeToken.approximateByteSize() === 0) {
685690
return true;

‎packages/firestore/test/unit/generate_spec_json.sh

+3-3
Original file line numberDiff line numberDiff line change
@@ -24,8 +24,8 @@ NPM_BIN_DIR="$(npm bin)"
2424
TSNODE="$NPM_BIN_DIR/ts-node "
2525
GENERATE_SPEC_JS="$DIR/generate_spec_json.js"
2626

27-
export TS_NODE_CACHE=NO
28-
export TS_NODE_COMPILER_OPTIONS='{"module":"commonjs"}'
27+
export TS_NODE_CACHE=NO
28+
export TS_NODE_COMPILER_OPTIONS='{"module":"commonjs"}'
2929
export TS_NODE_PROJECT="$DIR/../../tsconfig.json"
3030

31-
$TSNODE --require ../../index.node.ts $GENERATE_SPEC_JS "$@"
31+
$TSNODE --require ../../src/index.node.ts $GENERATE_SPEC_JS "$@"

‎packages/firestore/test/unit/local/local_store.test.ts

+73-1
Original file line numberDiff line numberDiff line change
@@ -93,6 +93,7 @@ import {
9393
doc,
9494
docAddedRemoteEvent,
9595
docUpdateRemoteEvent,
96+
existenceFilterEvent,
9697
expectEqual,
9798
filter,
9899
key,
@@ -130,6 +131,7 @@ class LocalStoreTester {
130131

131132
constructor(
132133
public localStore: LocalStore,
134+
private readonly persistence: Persistence,
133135
private readonly queryEngine: CountingQueryEngine,
134136
readonly gcIsEager: boolean
135137
) {
@@ -379,6 +381,30 @@ class LocalStoreTester {
379381
return this;
380382
}
381383

384+
toContainTargetData(
385+
target: Target,
386+
snapshotVersion: number,
387+
lastLimboFreeSnapshotVersion: number,
388+
resumeToken: ByteString
389+
): LocalStoreTester {
390+
this.promiseChain = this.promiseChain.then(async () => {
391+
const targetData = await this.persistence.runTransaction(
392+
'getTargetData',
393+
'readonly',
394+
txn => localStoreGetTargetData(this.localStore, txn, target)
395+
);
396+
expect(targetData!.snapshotVersion.isEqual(version(snapshotVersion))).to
397+
.be.true;
398+
expect(
399+
targetData!.lastLimboFreeSnapshotVersion.isEqual(
400+
version(lastLimboFreeSnapshotVersion)
401+
)
402+
).to.be.true;
403+
expect(targetData!.resumeToken.isEqual(resumeToken)).to.be.true;
404+
});
405+
return this;
406+
}
407+
382408
toReturnChanged(...docs: Document[]): LocalStoreTester {
383409
this.promiseChain = this.promiseChain.then(() => {
384410
debugAssert(
@@ -583,7 +609,12 @@ function genericLocalStoreTests(
583609
});
584610

585611
function expectLocalStore(): LocalStoreTester {
586-
return new LocalStoreTester(localStore, queryEngine, gcIsEager);
612+
return new LocalStoreTester(
613+
localStore,
614+
persistence,
615+
queryEngine,
616+
gcIsEager
617+
);
587618
}
588619

589620
it('handles SetMutation', () => {
@@ -1876,6 +1907,47 @@ function genericLocalStoreTests(
18761907
}
18771908
});
18781909

1910+
// eslint-disable-next-line no-restricted-properties
1911+
(gcIsEager ? it.skip : it)(
1912+
'ignores target mapping after existence filter mismatch',
1913+
async () => {
1914+
const query1 = query('foo', filter('matches', '==', true));
1915+
const target = queryToTarget(query1);
1916+
const targetId = 2;
1917+
1918+
return (
1919+
expectLocalStore()
1920+
.afterAllocatingQuery(query1)
1921+
.toReturnTargetId(targetId)
1922+
// Persist a mapping with a single document
1923+
.after(
1924+
docAddedRemoteEvent(
1925+
doc('foo/a', 10, { matches: true }),
1926+
[targetId],
1927+
[],
1928+
[targetId]
1929+
)
1930+
)
1931+
.after(noChangeEvent(targetId, 10, byteStringFromString('foo')))
1932+
.after(localViewChanges(targetId, /* fromCache= */ false, {}))
1933+
.afterExecutingQuery(query1)
1934+
.toReturnChanged(doc('foo/a', 10, { matches: true }))
1935+
.toHaveRead({ documentsByKey: 1 })
1936+
.toContainTargetData(target, 10, 10, byteStringFromString('foo'))
1937+
// Create an existence filter mismatch and verify that the last limbo
1938+
// free snapshot version is deleted
1939+
.after(existenceFilterEvent(targetId, 2, 20))
1940+
.after(noChangeEvent(targetId, 20))
1941+
.toContainTargetData(target, 0, 0, ByteString.EMPTY_BYTE_STRING)
1942+
// Re-run the query as a collection scan
1943+
.afterExecutingQuery(query1)
1944+
.toReturnChanged(doc('foo/a', 10, { matches: true }))
1945+
.toHaveRead({ documentsByQuery: 1 })
1946+
.finish()
1947+
);
1948+
}
1949+
);
1950+
18791951
// eslint-disable-next-line no-restricted-properties
18801952
(gcIsEager ? it.skip : it)(
18811953
'queries include locally modified documents',

‎packages/firestore/test/unit/specs/existence_filter_spec.test.ts

+29
Original file line numberDiff line numberDiff line change
@@ -211,4 +211,33 @@ describeSpec('Existence Filters:', [], () => {
211211
})
212212
);
213213
});
214+
215+
specTest(
216+
'Existence filter clears resume token',
217+
['durable-persistence'],
218+
() => {
219+
// This is a test for https://github.com/firebase/firebase-android-sdk/issues/3249
220+
// In this particular scenario, the user received an existence filter
221+
// mismatch, but the SDK only cleared the target-to-document mapping and
222+
// not the lastLimboFreeSnapshot version. This caused the SDK to resume
223+
// the query but not include old documents.
224+
const query1 = query('collection');
225+
const doc1 = doc('collection/1', 1000, { v: 1 });
226+
const doc2 = doc('collection/2', 1000, { v: 2 });
227+
return (
228+
spec()
229+
.userListens(query1)
230+
.watchAcksFull(query1, 1000, doc1, doc2)
231+
.expectEvents(query1, { added: [doc1, doc2] })
232+
.watchFilters([query1], doc1.key) // doc2 was deleted
233+
.watchSnapshots(2000)
234+
.expectEvents(query1, { fromCache: true })
235+
// The SDK is unable to re-run the query, and does not remove doc2
236+
.restart()
237+
.userListens(query1)
238+
// We check that the data is still consistent with the local cache
239+
.expectEvents(query1, { added: [doc1, doc2], fromCache: true })
240+
);
241+
}
242+
);
214243
});

‎packages/firestore/test/util/helpers.ts

+18
Original file line numberDiff line numberDiff line change
@@ -87,6 +87,7 @@ import {
8787
LimitType as ProtoLimitType
8888
} from '../../src/protos/firestore_bundle_proto';
8989
import * as api from '../../src/protos/firestore_proto_api';
90+
import { ExistenceFilter } from '../../src/remote/existence_filter';
9091
import { RemoteEvent, TargetChange } from '../../src/remote/remote_event';
9192
import {
9293
JsonProtoSerializer,
@@ -98,6 +99,7 @@ import {
9899
} from '../../src/remote/serializer';
99100
import {
100101
DocumentWatchChange,
102+
ExistenceFilterChange,
101103
WatchChangeAggregator,
102104
WatchTargetChange,
103105
WatchTargetChangeState
@@ -352,6 +354,22 @@ export function noChangeEvent(
352354
return aggregator.createRemoteEvent(version(snapshotVersion));
353355
}
354356

357+
export function existenceFilterEvent(
358+
targetId: number,
359+
count: number,
360+
snapshotVersion: number
361+
): RemoteEvent {
362+
const aggregator = new WatchChangeAggregator({
363+
getRemoteKeysForTarget: () => documentKeySet(),
364+
getTargetDataForTarget: targetId =>
365+
targetData(targetId, TargetPurpose.Listen, 'foo')
366+
});
367+
aggregator.handleExistenceFilter(
368+
new ExistenceFilterChange(targetId, new ExistenceFilter(count))
369+
);
370+
return aggregator.createRemoteEvent(version(snapshotVersion));
371+
}
372+
355373
export function docAddedRemoteEvent(
356374
docOrDocs: MutableDocument | MutableDocument[],
357375
updatedInTargets?: TargetId[],

0 commit comments

Comments
 (0)