Skip to content

Commit f3396f5

Browse files
author
Hui-Wu
authored
fix latency-compensated query (#3363)
* fix latency compensated query * addressing comments #1 + Changelog * add link to issue * add missing include * Minor nit fix
1 parent 57b4fb5 commit f3396f5

File tree

4 files changed

+249
-0
lines changed

4 files changed

+249
-0
lines changed

‎Firestore/CHANGELOG.md

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,9 @@
11
# Unreleased
22

33
# 1.4.2
4+
- [fixed] Fixed an issue where query results were temporarily missing documents
5+
that previously had not matched but had been updated to now match the query
6+
(https://github.com/firebase/firebase-android-sdk/issues/155).
47
- [fixed] Fixed an internal assertion that was triggered when an update
58
with a `FieldValue.serverTimestamp()` and an update with a
69
`FieldValue.increment()` were pending for the same document.

‎Firestore/Example/Tests/SpecTests/json/query_spec_test.json

Lines changed: 203 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1166,5 +1166,208 @@
11661166
]
11671167
}
11681168
]
1169+
},
1170+
"Latency-compensated updates are included in query results": {
1171+
"describeName": "Queries:",
1172+
"itName": "Latency-compensated updates are included in query results",
1173+
"tags": [],
1174+
"config": {
1175+
"useGarbageCollection": true,
1176+
"numClients": 1
1177+
},
1178+
"steps": [
1179+
{
1180+
"userListen": [
1181+
2,
1182+
{
1183+
"path": "collection",
1184+
"filters": [],
1185+
"orderBys": []
1186+
}
1187+
],
1188+
"stateExpect": {
1189+
"activeTargets": {
1190+
"2": {
1191+
"query": {
1192+
"path": "collection",
1193+
"filters": [],
1194+
"orderBys": []
1195+
},
1196+
"resumeToken": ""
1197+
}
1198+
}
1199+
}
1200+
},
1201+
{
1202+
"watchAck": [
1203+
2
1204+
]
1205+
},
1206+
{
1207+
"watchEntity": {
1208+
"docs": [
1209+
{
1210+
"key": "collection/a",
1211+
"version": 1000,
1212+
"value": {
1213+
"match": false
1214+
},
1215+
"options": {
1216+
"hasLocalMutations": false,
1217+
"hasCommittedMutations": false
1218+
}
1219+
}
1220+
],
1221+
"targets": [
1222+
2
1223+
]
1224+
}
1225+
},
1226+
{
1227+
"watchCurrent": [
1228+
[
1229+
2
1230+
],
1231+
"resume-token-1000"
1232+
]
1233+
},
1234+
{
1235+
"watchSnapshot": {
1236+
"version": 1000,
1237+
"targetIds": []
1238+
},
1239+
"expect": [
1240+
{
1241+
"query": {
1242+
"path": "collection",
1243+
"filters": [],
1244+
"orderBys": []
1245+
},
1246+
"added": [
1247+
{
1248+
"key": "collection/a",
1249+
"version": 1000,
1250+
"value": {
1251+
"match": false
1252+
},
1253+
"options": {
1254+
"hasLocalMutations": false,
1255+
"hasCommittedMutations": false
1256+
}
1257+
}
1258+
],
1259+
"errorCode": 0,
1260+
"fromCache": false,
1261+
"hasPendingWrites": false
1262+
}
1263+
]
1264+
},
1265+
{
1266+
"userPatch": [
1267+
"collection/a",
1268+
{
1269+
"match": true
1270+
}
1271+
],
1272+
"expect": [
1273+
{
1274+
"query": {
1275+
"path": "collection",
1276+
"filters": [],
1277+
"orderBys": []
1278+
},
1279+
"modified": [
1280+
{
1281+
"key": "collection/a",
1282+
"version": 1000,
1283+
"value": {
1284+
"match": true
1285+
},
1286+
"options": {
1287+
"hasLocalMutations": true,
1288+
"hasCommittedMutations": false
1289+
}
1290+
}
1291+
],
1292+
"errorCode": 0,
1293+
"fromCache": false,
1294+
"hasPendingWrites": true
1295+
}
1296+
]
1297+
},
1298+
{
1299+
"userListen": [
1300+
4,
1301+
{
1302+
"path": "collection",
1303+
"filters": [
1304+
[
1305+
"match",
1306+
"==",
1307+
true
1308+
]
1309+
],
1310+
"orderBys": []
1311+
}
1312+
],
1313+
"stateExpect": {
1314+
"activeTargets": {
1315+
"2": {
1316+
"query": {
1317+
"path": "collection",
1318+
"filters": [],
1319+
"orderBys": []
1320+
},
1321+
"resumeToken": ""
1322+
},
1323+
"4": {
1324+
"query": {
1325+
"path": "collection",
1326+
"filters": [
1327+
[
1328+
"match",
1329+
"==",
1330+
true
1331+
]
1332+
],
1333+
"orderBys": []
1334+
},
1335+
"resumeToken": ""
1336+
}
1337+
}
1338+
},
1339+
"expect": [
1340+
{
1341+
"query": {
1342+
"path": "collection",
1343+
"filters": [
1344+
[
1345+
"match",
1346+
"==",
1347+
true
1348+
]
1349+
],
1350+
"orderBys": []
1351+
},
1352+
"added": [
1353+
{
1354+
"key": "collection/a",
1355+
"version": 1000,
1356+
"value": {
1357+
"match": true
1358+
},
1359+
"options": {
1360+
"hasLocalMutations": true,
1361+
"hasCommittedMutations": false
1362+
}
1363+
}
1364+
],
1365+
"errorCode": 0,
1366+
"fromCache": true,
1367+
"hasPendingWrites": true
1368+
}
1369+
]
1370+
}
1371+
]
11691372
}
11701373
}

‎Firestore/core/src/firebase/firestore/local/local_documents_view.h

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -103,6 +103,19 @@ class LocalDocumentsView {
103103
/** Queries the remote documents and overlays mutations. */
104104
model::DocumentMap GetDocumentsMatchingCollectionQuery(FSTQuery* query);
105105

106+
/**
107+
* It is possible that a `PatchMutation` can make a document match a query,
108+
* even if the version in the `RemoteDocumentCache` is not a match yet
109+
* (waiting for server to ack). To handle this, we find all document keys
110+
* affected by the `PatchMutation`s that are not in `existingDocs` yet, and
111+
* back fill them via `remote_document_cache_->GetAll`, otherwise those
112+
* `PatchMutation`s will be ignored because no base document can be found, and
113+
* lead to missing results for the query.
114+
*/
115+
model::DocumentMap AddMissingBaseDocuments(
116+
const std::vector<FSTMutationBatch*>& matching_batches,
117+
model::DocumentMap existing_docs);
118+
106119
RemoteDocumentCache* remote_document_cache_;
107120
MutationQueue* mutation_queue_;
108121
IndexManager* index_manager_;

‎Firestore/core/src/firebase/firestore/local/local_documents_view.mm

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717
#import "Firestore/core/src/firebase/firestore/local/local_documents_view.h"
1818

1919
#include <string>
20+
#include <utility>
2021

2122
#import "Firestore/Source/Core/FSTQuery.h"
2223
#import "Firestore/Source/Model/FSTDocument.h"
@@ -171,6 +172,8 @@
171172
std::vector<FSTMutationBatch*> matchingBatches =
172173
mutation_queue_->AllMutationBatchesAffectingQuery(query);
173174

175+
results = AddMissingBaseDocuments(matchingBatches, std::move(results));
176+
174177
for (FSTMutationBatch* batch : matchingBatches) {
175178
for (FSTMutation* mutation : [batch mutations]) {
176179
// Only process documents belonging to the collection.
@@ -215,6 +218,33 @@
215218
return results;
216219
}
217220

221+
DocumentMap LocalDocumentsView::AddMissingBaseDocuments(
222+
const std::vector<FSTMutationBatch*>& matching_batches,
223+
DocumentMap existing_docs) {
224+
DocumentKeySet missing_doc_keys;
225+
for (FSTMutationBatch* batch : matching_batches) {
226+
for (FSTMutation* mutation : [batch mutations]) {
227+
if ([mutation isKindOfClass:[FSTPatchMutation class]] &&
228+
existing_docs.underlying_map().find([mutation key]) ==
229+
existing_docs.underlying_map().end()) {
230+
missing_doc_keys = missing_doc_keys.insert([mutation key]);
231+
}
232+
}
233+
}
234+
235+
MaybeDocumentMap missing_docs =
236+
remote_document_cache_->GetAll(missing_doc_keys);
237+
for (const auto& kv : missing_docs) {
238+
FSTMaybeDocument* maybe_doc = kv.second;
239+
if (maybe_doc != nil && [maybe_doc isKindOfClass:[FSTDocument class]]) {
240+
existing_docs =
241+
existing_docs.insert(kv.first, static_cast<FSTDocument*>(maybe_doc));
242+
}
243+
}
244+
245+
return existing_docs;
246+
}
247+
218248
} // namespace local
219249
} // namespace firestore
220250
} // namespace firebase

0 commit comments

Comments
 (0)