Skip to content

Commit 618b08c

Browse files
schmidt-sebastianschmidt-sebastian
authored
Clear the lastLimboFreeSnapshot version on existence filter mismatch (#3267)
1 parent fea8375 commit 618b08c

File tree

6 files changed

+306
-19
lines changed

6 files changed

+306
-19
lines changed

‎firebase-firestore/CHANGELOG.md

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,8 @@ by opting into a release at
77
updates that have not yet been synced with the backend.
88
- [changed] Improved performance for queries against collections that contain
99
subcollections.
10+
- [fixed] Fixed an issue that can result in incomplete Query snapshots when an
11+
app is backgrounded during query execution.
1012

1113
# 24.0.0
1214
- [feature] Added support for Firebase AppCheck.

‎firebase-firestore/src/main/java/com/google/firebase/firestore/local/LocalStore.java

Lines changed: 18 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -417,20 +417,24 @@ public ImmutableSortedMap<DocumentKey, Document> applyRemoteEvent(RemoteEvent re
417417
targetCache.removeMatchingKeys(change.getRemovedDocuments(), targetId);
418418
targetCache.addMatchingKeys(change.getAddedDocuments(), targetId);
419419

420-
ByteString resumeToken = change.getResumeToken();
421-
// Update the resume token if the change includes one.
422-
if (!resumeToken.isEmpty()) {
423-
TargetData newTargetData =
424-
oldTargetData
425-
.withResumeToken(resumeToken, remoteEvent.getSnapshotVersion())
426-
.withSequenceNumber(sequenceNumber);
427-
queryDataByTarget.put(targetId, newTargetData);
428-
429-
// Update the query data if there are target changes (or if sufficient time has
430-
// passed since the last update).
431-
if (shouldPersistTargetData(oldTargetData, newTargetData, change)) {
432-
targetCache.updateTargetData(newTargetData);
433-
}
420+
TargetData newTargetData = oldTargetData.withSequenceNumber(sequenceNumber);
421+
if (remoteEvent.getTargetMismatches().contains(targetId)) {
422+
newTargetData =
423+
newTargetData
424+
.withResumeToken(ByteString.EMPTY, SnapshotVersion.NONE)
425+
.withLastLimboFreeSnapshotVersion(SnapshotVersion.NONE);
426+
} else if (!change.getResumeToken().isEmpty()) {
427+
newTargetData =
428+
newTargetData.withResumeToken(
429+
change.getResumeToken(), remoteEvent.getSnapshotVersion());
430+
}
431+
432+
queryDataByTarget.put(targetId, newTargetData);
433+
434+
// Update the query data if there are target changes (or if sufficient time has passed
435+
// since the last update).
436+
if (shouldPersistTargetData(oldTargetData, newTargetData, change)) {
437+
targetCache.updateTargetData(newTargetData);
434438
}
435439
}
436440

@@ -554,10 +558,6 @@ private DocumentChangeResult populateDocumentChanges(
554558
*/
555559
private static boolean shouldPersistTargetData(
556560
TargetData oldTargetData, TargetData newTargetData, TargetChange change) {
557-
hardAssert(
558-
!newTargetData.getResumeToken().isEmpty(),
559-
"Attempted to persist query data with empty resume token");
560-
561561
// Always persist query data if we don't already have a resume token.
562562
if (oldTargetData.getResumeToken().isEmpty()) return true;
563563

‎firebase-firestore/src/main/java/com/google/firebase/firestore/remote/RemoteStore.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -534,7 +534,7 @@ private void raiseWatchSnapshot(SnapshotVersion snapshotVersion) {
534534
}
535535
}
536536

537-
// Re-establish listens for the targets that have been invalidated by existence filter
537+
// Re-establish listens for the targets that have been invalidated by existence filter
538538
// mismatches.
539539
for (int targetId : remoteEvent.getTargetMismatches()) {
540540
TargetData targetData = this.listenTargets.get(targetId);

‎firebase-firestore/src/test/java/com/google/firebase/firestore/local/LocalStoreTestCase.java

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@
2121
import static com.google.firebase.firestore.testutil.TestUtil.deletedDoc;
2222
import static com.google.firebase.firestore.testutil.TestUtil.doc;
2323
import static com.google.firebase.firestore.testutil.TestUtil.docMap;
24+
import static com.google.firebase.firestore.testutil.TestUtil.existenceFilterEvent;
2425
import static com.google.firebase.firestore.testutil.TestUtil.filter;
2526
import static com.google.firebase.firestore.testutil.TestUtil.key;
2627
import static com.google.firebase.firestore.testutil.TestUtil.keySet;
@@ -75,6 +76,7 @@
7576
import com.google.firebase.firestore.remote.WriteStream;
7677
import com.google.firebase.firestore.testutil.TestUtil;
7778
import com.google.firebase.firestore.util.AsyncQueue;
79+
import com.google.protobuf.ByteString;
7880
import java.util.ArrayList;
7981
import java.util.Arrays;
8082
import java.util.Collection;
@@ -1123,6 +1125,38 @@ public void testUsesTargetMappingToExecuteQueries() {
11231125
assertQueryReturned("foo/a", "foo/b");
11241126
}
11251127

1128+
@Test
1129+
public void testIgnoresTargetMappingAfterExistenceFilterMismatch() {
1130+
assumeFalse(garbageCollectorIsEager());
1131+
1132+
Query query = query("foo").filter(filter("matches", "==", true));
1133+
int targetId = allocateQuery(query);
1134+
1135+
executeQuery(query);
1136+
1137+
// Persist a mapping with a single document
1138+
applyRemoteEvent(
1139+
addedRemoteEvent(
1140+
asList(doc("foo/a", 10, map("matches", true))), asList(targetId), emptyList()));
1141+
applyRemoteEvent(noChangeEvent(targetId, 10));
1142+
updateViews(targetId, /* fromCache= */ false);
1143+
1144+
TargetData cachedTargetData = localStore.getTargetData(query.toTarget());
1145+
Assert.assertEquals(version(10), cachedTargetData.getLastLimboFreeSnapshotVersion());
1146+
1147+
// Create an existence filter mismatch and verify that the last limbo free snapshot version
1148+
// is deleted
1149+
applyRemoteEvent(existenceFilterEvent(targetId, 2, 20));
1150+
cachedTargetData = localStore.getTargetData(query.toTarget());
1151+
Assert.assertEquals(version(0), cachedTargetData.getLastLimboFreeSnapshotVersion());
1152+
Assert.assertEquals(ByteString.EMPTY, cachedTargetData.getResumeToken());
1153+
1154+
// Re-run the query as a collection scan
1155+
executeQuery(query);
1156+
assertRemoteDocumentsRead(/* byKey= */ 0, /* byCollection= */ 1);
1157+
assertQueryReturned("foo/a");
1158+
}
1159+
11261160
@Test
11271161
public void testLastLimboFreeSnapshotIsAdvancedDuringViewProcessing() {
11281162
// This test verifies that the `lastLimboFreeSnapshot` version for TargetData is advanced when

‎firebase-firestore/src/test/resources/json/existence_filter_spec_test.json

Lines changed: 236 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1072,6 +1072,242 @@
10721072
}
10731073
]
10741074
},
1075+
"Existence filter mismatch invalidates index-free query": {
1076+
"describeName": "Existence Filters:",
1077+
"itName": "Existence filter clears resume token",
1078+
"tags": [
1079+
"durable-persistence"
1080+
],
1081+
"config": {
1082+
"numClients": 1,
1083+
"useGarbageCollection": true
1084+
},
1085+
"steps": [
1086+
{
1087+
"userListen": {
1088+
"query": {
1089+
"filters": [
1090+
],
1091+
"orderBys": [
1092+
],
1093+
"path": "collection"
1094+
},
1095+
"targetId": 2
1096+
},
1097+
"expectedState": {
1098+
"activeTargets": {
1099+
"2": {
1100+
"queries": [
1101+
{
1102+
"filters": [
1103+
],
1104+
"orderBys": [
1105+
],
1106+
"path": "collection"
1107+
}
1108+
],
1109+
"resumeToken": ""
1110+
}
1111+
}
1112+
}
1113+
},
1114+
{
1115+
"watchAck": [
1116+
2
1117+
]
1118+
},
1119+
{
1120+
"watchEntity": {
1121+
"docs": [
1122+
{
1123+
"key": "collection/1",
1124+
"options": {
1125+
"hasCommittedMutations": false,
1126+
"hasLocalMutations": false
1127+
},
1128+
"value": {
1129+
"v": 1
1130+
},
1131+
"version": 1000
1132+
},
1133+
{
1134+
"key": "collection/2",
1135+
"options": {
1136+
"hasCommittedMutations": false,
1137+
"hasLocalMutations": false
1138+
},
1139+
"value": {
1140+
"v": 2
1141+
},
1142+
"version": 1000
1143+
}
1144+
],
1145+
"targets": [
1146+
2
1147+
]
1148+
}
1149+
},
1150+
{
1151+
"watchCurrent": [
1152+
[
1153+
2
1154+
],
1155+
"resume-token-1000"
1156+
]
1157+
},
1158+
{
1159+
"watchSnapshot": {
1160+
"targetIds": [
1161+
],
1162+
"version": 1000
1163+
},
1164+
"expectedSnapshotEvents": [
1165+
{
1166+
"added": [
1167+
{
1168+
"key": "collection/1",
1169+
"options": {
1170+
"hasCommittedMutations": false,
1171+
"hasLocalMutations": false
1172+
},
1173+
"value": {
1174+
"v": 1
1175+
},
1176+
"version": 1000
1177+
},
1178+
{
1179+
"key": "collection/2",
1180+
"options": {
1181+
"hasCommittedMutations": false,
1182+
"hasLocalMutations": false
1183+
},
1184+
"value": {
1185+
"v": 2
1186+
},
1187+
"version": 1000
1188+
}
1189+
],
1190+
"errorCode": 0,
1191+
"fromCache": false,
1192+
"hasPendingWrites": false,
1193+
"query": {
1194+
"filters": [
1195+
],
1196+
"orderBys": [
1197+
],
1198+
"path": "collection"
1199+
}
1200+
}
1201+
]
1202+
},
1203+
{
1204+
"watchFilter": [
1205+
[
1206+
2
1207+
],
1208+
"collection/1"
1209+
]
1210+
},
1211+
{
1212+
"watchSnapshot": {
1213+
"targetIds": [
1214+
],
1215+
"version": 2000
1216+
},
1217+
"expectedSnapshotEvents": [
1218+
{
1219+
"errorCode": 0,
1220+
"fromCache": true,
1221+
"hasPendingWrites": false,
1222+
"query": {
1223+
"filters": [
1224+
],
1225+
"orderBys": [
1226+
],
1227+
"path": "collection"
1228+
}
1229+
}
1230+
]
1231+
},
1232+
{
1233+
"restart": true,
1234+
"expectedState": {
1235+
"activeLimboDocs": [
1236+
],
1237+
"activeTargets": {
1238+
},
1239+
"enqueuedLimboDocs": [
1240+
]
1241+
}
1242+
},
1243+
{
1244+
"userListen": {
1245+
"query": {
1246+
"filters": [
1247+
],
1248+
"orderBys": [
1249+
],
1250+
"path": "collection"
1251+
},
1252+
"targetId": 2
1253+
},
1254+
"expectedSnapshotEvents": [
1255+
{
1256+
"added": [
1257+
{
1258+
"key": "collection/1",
1259+
"options": {
1260+
"hasCommittedMutations": false,
1261+
"hasLocalMutations": false
1262+
},
1263+
"value": {
1264+
"v": 1
1265+
},
1266+
"version": 1000
1267+
},
1268+
{
1269+
"key": "collection/2",
1270+
"options": {
1271+
"hasCommittedMutations": false,
1272+
"hasLocalMutations": false
1273+
},
1274+
"value": {
1275+
"v": 2
1276+
},
1277+
"version": 1000
1278+
}
1279+
],
1280+
"errorCode": 0,
1281+
"fromCache": true,
1282+
"hasPendingWrites": false,
1283+
"query": {
1284+
"filters": [
1285+
],
1286+
"orderBys": [
1287+
],
1288+
"path": "collection"
1289+
}
1290+
}
1291+
],
1292+
"expectedState": {
1293+
"activeTargets": {
1294+
"2": {
1295+
"queries": [
1296+
{
1297+
"filters": [
1298+
],
1299+
"orderBys": [
1300+
],
1301+
"path": "collection"
1302+
}
1303+
],
1304+
"resumeToken": ""
1305+
}
1306+
}
1307+
}
1308+
}
1309+
]
1310+
},
10751311
"Existence filter mismatch triggers re-run of query": {
10761312
"describeName": "Existence Filters:",
10771313
"itName": "Existence filter mismatch triggers re-run of query",

‎firebase-firestore/src/testUtil/java/com/google/firebase/firestore/testutil/TestUtil.java

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -71,6 +71,7 @@
7171
import com.google.firebase.firestore.model.mutation.Precondition;
7272
import com.google.firebase.firestore.model.mutation.SetMutation;
7373
import com.google.firebase.firestore.model.mutation.VerifyMutation;
74+
import com.google.firebase.firestore.remote.ExistenceFilter;
7475
import com.google.firebase.firestore.remote.RemoteEvent;
7576
import com.google.firebase.firestore.remote.TargetChange;
7677
import com.google.firebase.firestore.remote.WatchChange;
@@ -426,6 +427,20 @@ public static RemoteEvent addedRemoteEvent(
426427
return addedRemoteEvent(singletonList(doc), updatedInTargets, removedFromTargets);
427428
}
428429

430+
public static RemoteEvent existenceFilterEvent(int targetId, int count, int version) {
431+
TargetData targetData = TestUtil.targetData(targetId, QueryPurpose.LISTEN, "foo");
432+
TestTargetMetadataProvider testTargetMetadataProvider = new TestTargetMetadataProvider();
433+
testTargetMetadataProvider.setSyncedKeys(targetData, DocumentKey.emptyKeySet());
434+
435+
ExistenceFilter existenceFilter = new ExistenceFilter(count);
436+
WatchChangeAggregator aggregator = new WatchChangeAggregator(testTargetMetadataProvider);
437+
438+
WatchChange.ExistenceFilterWatchChange existenceFilterWatchChange =
439+
new WatchChange.ExistenceFilterWatchChange(targetId, existenceFilter);
440+
aggregator.handleExistenceFilter(existenceFilterWatchChange);
441+
return aggregator.createRemoteEvent(version(version));
442+
}
443+
429444
public static RemoteEvent addedRemoteEvent(
430445
List<MutableDocument> docs,
431446
List<Integer> updatedInTargets,

0 commit comments

Comments
 (0)