Skip to content

Commit 40af4aa

Browse files
MarkDuckworthMarkDuckworth
committed
Fix handling of multiple out of order query updates
1 parent 40d2b33 commit 40af4aa

File tree

3 files changed

+21
-289
lines changed

3 files changed

+21
-289
lines changed

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

Lines changed: 20 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -682,10 +682,16 @@ export class WatchChangeAggregator {
682682
const targetState = this.ensureTargetState(targetId);
683683
targetState.addDocumentChange(document.key, changeType);
684684

685-
this.pendingDocumentUpdates = this.pendingDocumentUpdates.insert(
686-
document.key,
687-
document
688-
);
685+
const pendingDocumentUpdate = this.pendingDocumentUpdates.get(document.key);
686+
if (
687+
pendingDocumentUpdate === null ||
688+
document.version.compareTo(pendingDocumentUpdate.version) > 0
689+
) {
690+
this.pendingDocumentUpdates = this.pendingDocumentUpdates.insert(
691+
document.key,
692+
document
693+
);
694+
}
689695

690696
this.pendingDocumentUpdatesByTarget =
691697
this.pendingDocumentUpdatesByTarget.insert(
@@ -739,10 +745,16 @@ export class WatchChangeAggregator {
739745
);
740746

741747
if (updatedDocument) {
742-
this.pendingDocumentUpdates = this.pendingDocumentUpdates.insert(
743-
key,
744-
updatedDocument
745-
);
748+
const pendingDocumentUpdate = this.pendingDocumentUpdates.get(key);
749+
if (
750+
pendingDocumentUpdate === null ||
751+
updatedDocument.version.compareTo(pendingDocumentUpdate.version) > 0
752+
) {
753+
this.pendingDocumentUpdates = this.pendingDocumentUpdates.insert(
754+
key,
755+
updatedDocument
756+
);
757+
}
746758
}
747759
}
748760

‎packages/firestore/test/unit/remote/remote_event.test.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -323,7 +323,7 @@ describe('RemoteEvent', () => {
323323
const targets = listens(1, 2);
324324

325325
const doc1a = doc('docs/1', 1, { value: 1 });
326-
const doc1b = doc('docs/1', 1, { value: 2 });
326+
const doc1b = doc('docs/1', 2, { value: 2 });
327327

328328
const change1 = new DocumentWatchChange([1], [2], doc1a.key, doc1a);
329329
const change2 = new DocumentWatchChange([2], [1], doc1b.key, doc1b);

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

Lines changed: 0 additions & 280 deletions
Original file line numberDiff line numberDiff line change
@@ -1171,284 +1171,4 @@ describeSpec('Limbo Documents:', [], () => {
11711171
);
11721172
}
11731173
);
1174-
1175-
specTest(
1176-
'Fix #8474 - Limbo resolution for document is removed even if document updates for the document occurred before documentDelete in the global snapshot window',
1177-
[],
1178-
() => {
1179-
// onSnapshot(fullQuery)
1180-
const fullQuery = query('collection');
1181-
1182-
// getDocs(filterQuery)
1183-
const filterQuery = query('collection', filter('included', '==', true));
1184-
1185-
const docA = doc('collection/a', 1000, { key: 'a', included: false });
1186-
const docA2 = doc('collection/a', 1007, { key: 'a', included: true });
1187-
const docC = doc('collection/c', 1002, { key: 'c', included: true });
1188-
1189-
const limboQueryC = newQueryForPath(docC.key.path);
1190-
1191-
return (
1192-
spec()
1193-
// onSnapshot(fullQuery) - fullQuery is listening to documents in the collection for the full test
1194-
.userListens(fullQuery)
1195-
.watchAcksFull(fullQuery, 1001, docA, docC)
1196-
.expectEvents(fullQuery, {
1197-
fromCache: false,
1198-
added: [docA, docC]
1199-
})
1200-
1201-
// docC was deleted, this puts docC in limbo and causes a snapshot from cache (metadata-only change)
1202-
.watchRemovesDoc(docC.key, fullQuery)
1203-
.watchCurrents(fullQuery, 'resume-token-1002')
1204-
.watchSnapshots(1002)
1205-
.expectLimboDocs(docC.key)
1206-
.expectEvents(fullQuery, {
1207-
fromCache: true
1208-
})
1209-
1210-
// User begins getDocs(filterQuery)
1211-
.userListensForGet(filterQuery)
1212-
1213-
// getDocs(filterQuery) will not resolve on the snapshot from cache
1214-
.expectEvents(filterQuery, {
1215-
fromCache: true,
1216-
added: [docC]
1217-
})
1218-
1219-
// Watch acks limbo and filter queries
1220-
.watchAcks(limboQueryC)
1221-
.watchAcks(filterQuery)
1222-
1223-
// Watch responds to limboQueryC - docC was deleted
1224-
.watchDeletesDoc(docC.key, 1009, limboQueryC)
1225-
.watchCurrents(limboQueryC, 'resume-token-1009')
1226-
.watchSnapshots(1009, [limboQueryC, fullQuery])
1227-
1228-
// However, docC is still in limbo because there has not been a global snapshot
1229-
.expectLimboDocs(docC.key)
1230-
1231-
// Rapid events of document update and delete caused by application
1232-
.watchRemovesDoc(docA.key, filterQuery)
1233-
.watchCurrents(filterQuery, 'resume-token-1004')
1234-
.watchSends({ affects: [filterQuery] }, docC)
1235-
.watchCurrents(filterQuery, 'resume-token-1005')
1236-
.watchRemovesDoc(docC.key, filterQuery)
1237-
.watchSends({ affects: [filterQuery] }, docA2)
1238-
.watchCurrents(filterQuery, 'resume-token-1007')
1239-
1240-
.watchSnapshots(1010, [fullQuery, limboQueryC])
1241-
1242-
// All changes are current and we get a global snapshot
1243-
.watchSnapshots(1010, [])
1244-
1245-
// Now docC is out of limbo
1246-
.expectLimboDocs()
1247-
.expectEvents(fullQuery, {
1248-
fromCache: false,
1249-
modified: [docA2],
1250-
removed: [docC]
1251-
})
1252-
// Now getDocs(filterQuery) can be resolved
1253-
.expectEvents(filterQuery, {
1254-
fromCache: false,
1255-
removed: [docC],
1256-
added: [docA2]
1257-
})
1258-
1259-
// No more expected events
1260-
.watchSnapshots(1100, [])
1261-
);
1262-
}
1263-
);
1264-
1265-
specTest(
1266-
'Fix #8474 - Limbo resolution for document is removed even if document updates for the document occurred in the global snapshot window and no document delete was received for the limbo resolution query',
1267-
[],
1268-
() => {
1269-
// onSnapshot(fullQuery)
1270-
const fullQuery = query('collection');
1271-
1272-
// getDocs(filterQuery)
1273-
const filterQuery = query('collection', filter('included', '==', true));
1274-
1275-
const docA = doc('collection/a', 1000, { key: 'a', included: false });
1276-
const docA2 = doc('collection/a', 1007, { key: 'a', included: true });
1277-
const docC = doc('collection/c', 1002, { key: 'c', included: true });
1278-
1279-
const limboQueryC = newQueryForPath(docC.key.path);
1280-
1281-
return (
1282-
spec()
1283-
// onSnapshot(fullQuery) - fullQuery is listening to documents in the collection for the full test
1284-
.userListens(fullQuery)
1285-
.watchAcksFull(fullQuery, 1001, docA, docC)
1286-
.expectEvents(fullQuery, {
1287-
fromCache: false,
1288-
added: [docA, docC]
1289-
})
1290-
1291-
// docC was deleted, this puts docC in limbo and causes a snapshot from cache (metadata-only change)
1292-
.watchRemovesDoc(docC.key, fullQuery)
1293-
.watchCurrents(fullQuery, 'resume-token-1002')
1294-
.watchSnapshots(1002)
1295-
.expectLimboDocs(docC.key)
1296-
.expectEvents(fullQuery, {
1297-
fromCache: true
1298-
})
1299-
1300-
// User begins getDocs(filterQuery)
1301-
.userListensForGet(filterQuery)
1302-
1303-
// getDocs(filterQuery) will not resolve on the snapshot from cache
1304-
.expectEvents(filterQuery, {
1305-
fromCache: true,
1306-
added: [docC]
1307-
})
1308-
1309-
// Watch acks limbo and filter queries
1310-
.watchAcks(limboQueryC)
1311-
.watchAcks(filterQuery)
1312-
1313-
// Watch currents the limbo query, but did not send a document delete.
1314-
// This is and unexpected code path, but something that is called
1315-
// out as possible in the watch change aggregator.
1316-
.watchCurrents(limboQueryC, 'resume-token-1009')
1317-
.watchSnapshots(1009, [limboQueryC, fullQuery])
1318-
1319-
// However, docC is still in limbo because there has not been a global snapshot
1320-
.expectLimboDocs(docC.key)
1321-
1322-
// Rapid events of document update and delete caused by application
1323-
.watchRemovesDoc(docA.key, filterQuery)
1324-
.watchCurrents(filterQuery, 'resume-token-1004')
1325-
.watchSends({ affects: [filterQuery] }, docC)
1326-
.watchCurrents(filterQuery, 'resume-token-1005')
1327-
.watchRemovesDoc(docC.key, filterQuery)
1328-
.watchSends({ affects: [filterQuery] }, docA2)
1329-
.watchCurrents(filterQuery, 'resume-token-1007')
1330-
1331-
.watchSnapshots(1010, [fullQuery, limboQueryC])
1332-
1333-
// All changes are current and we get a global snapshot
1334-
.watchSnapshots(1010, [])
1335-
1336-
// Now docC is out of limbo
1337-
.expectLimboDocs()
1338-
.expectEvents(fullQuery, {
1339-
fromCache: false,
1340-
modified: [docA2],
1341-
removed: [docC]
1342-
})
1343-
// Now getDocs(filterQuery) can be resolved
1344-
.expectEvents(filterQuery, {
1345-
fromCache: false,
1346-
removed: [docC],
1347-
added: [docA2]
1348-
})
1349-
1350-
// No more expected events
1351-
.watchSnapshots(1100, [])
1352-
);
1353-
}
1354-
);
1355-
1356-
specTest(
1357-
'Fix #8474 - Handles code path of no ack for limbo resolution query before global snapshot',
1358-
[],
1359-
() => {
1360-
// onSnapshot(fullQuery)
1361-
const fullQuery = query('collection');
1362-
1363-
// getDocs(filterQuery)
1364-
const filterQuery = query('collection', filter('included', '==', true));
1365-
1366-
const docA = doc('collection/a', 1000, { key: 'a', included: false });
1367-
const docA2 = doc('collection/a', 1007, { key: 'a', included: true });
1368-
const docC = doc('collection/c', 1002, { key: 'c', included: true });
1369-
1370-
const limboQueryC = newQueryForPath(docC.key.path);
1371-
1372-
return (
1373-
spec()
1374-
// onSnapshot(fullQuery) - fullQuery is listening to documents in the collection for the full test
1375-
.userListens(fullQuery)
1376-
.watchAcksFull(fullQuery, 1001, docA, docC)
1377-
.expectEvents(fullQuery, {
1378-
fromCache: false,
1379-
added: [docA, docC]
1380-
})
1381-
1382-
// docC was deleted, this puts docC in limbo and causes a snapshot from cache (metadata-only change)
1383-
.watchRemovesDoc(docC.key, fullQuery)
1384-
.watchCurrents(fullQuery, 'resume-token-1002')
1385-
.watchSnapshots(1002)
1386-
.expectLimboDocs(docC.key)
1387-
.expectEvents(fullQuery, {
1388-
fromCache: true
1389-
})
1390-
1391-
// User begins getDocs(filterQuery)
1392-
.userListensForGet(filterQuery)
1393-
1394-
// getDocs(filterQuery) will not resolve on the snapshot from cache
1395-
.expectEvents(filterQuery, {
1396-
fromCache: true,
1397-
added: [docC]
1398-
})
1399-
1400-
// Watch filter query
1401-
.watchAcks(filterQuery)
1402-
1403-
// However, docC is still in limbo because there has not been a global snapshot
1404-
.expectLimboDocs(docC.key)
1405-
1406-
// Rapid events of document update and delete caused by application
1407-
.watchRemovesDoc(docA.key, filterQuery)
1408-
.watchCurrents(filterQuery, 'resume-token-1004')
1409-
.watchSends({ affects: [filterQuery] }, docC)
1410-
.watchCurrents(filterQuery, 'resume-token-1005')
1411-
.watchRemovesDoc(docC.key, filterQuery)
1412-
.watchSends({ affects: [filterQuery] }, docA2)
1413-
.watchCurrents(filterQuery, 'resume-token-1007')
1414-
1415-
.watchSnapshots(1010, [fullQuery, limboQueryC])
1416-
1417-
// All changes are current and we get a global snapshot
1418-
.watchSnapshots(1010, [])
1419-
1420-
.expectEvents(fullQuery, {
1421-
fromCache: true,
1422-
modified: [docA2]
1423-
})
1424-
// Now getDocs(filterQuery) can be resolved
1425-
.expectEvents(filterQuery, {
1426-
fromCache: true,
1427-
added: [docA2]
1428-
})
1429-
1430-
// Watch acks limbo query
1431-
.watchAcks(limboQueryC)
1432-
1433-
// Watch responds to limboQueryC - docC was deleted
1434-
.watchDeletesDoc(docC.key, 1009, limboQueryC)
1435-
.watchCurrents(limboQueryC, 'resume-token-1009')
1436-
.watchSnapshots(1100, [limboQueryC])
1437-
1438-
// No more expected events
1439-
.watchSnapshots(1101, [])
1440-
1441-
.expectLimboDocs()
1442-
.expectEvents(fullQuery, {
1443-
fromCache: false,
1444-
removed: [docC]
1445-
})
1446-
// Now getDocs(filterQuery) can be resolved
1447-
.expectEvents(filterQuery, {
1448-
fromCache: false,
1449-
removed: [docC]
1450-
})
1451-
);
1452-
}
1453-
);
14541174
});

0 commit comments

Comments
 (0)