Skip to content

Commit 03a9c22

Browse files
dconeybedconeybe
committed
Apply code review feedback
1 parent 9bbd652 commit 03a9c22

File tree

4 files changed

+93
-86
lines changed

4 files changed

+93
-86
lines changed

‎.changeset/eight-bananas-nail.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,4 +2,4 @@
22
'@firebase/firestore': patch
33
---
44

5-
Fix a bug where local cache inconsistencies were unnecessarily being resolved.
5+
Fixes a bug where local cache inconsistencies were unnecessarily being resolved.

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

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -211,6 +211,10 @@ class SyncEngineImpl implements SyncEngine {
211211
* The keys of documents that are in limbo for which we haven't yet started a
212212
* limbo resolution query. The strings in this set are the result of calling
213213
* `key.path.canonicalString()` where `key` is a `DocumentKey` object.
214+
*
215+
* The `Set` type was chosen because it provides efficient lookup and removal
216+
* of arbitrary elements and it also maintains insertion order, providing the
217+
* desired queue-like FIFO semantics.
214218
*/
215219
enqueuedLimboResolutions = new Set<string>();
216220
/**

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

Lines changed: 82 additions & 82 deletions
Original file line numberDiff line numberDiff line change
@@ -924,146 +924,146 @@ describeSpec('Limbo Documents:', [], () => {
924924
);
925925

926926
specTest(
927-
'A limbo resolution for a document should be removed from the queue when the last query listen stops',
927+
'A limbo resolution for a document should not be started if one is already active',
928928
[],
929929
() => {
930-
const doc1 = doc('collection1/doc', 1000, { key: 1 });
931-
const query1 = query('collection1');
932-
const filteredQuery1 = query('collection1', filter('key', '==', 1));
933-
934-
const doc2 = doc('collection2/doc', 1000, { key: 2 });
935-
const query2 = query('collection2');
936-
const filteredQuery2 = query('collection2', filter('key', '==', 2));
937-
const filteredQuery3 = query('collection2', filter('key', '>=', 2));
930+
const doc1 = doc('collection/doc', 1000, { key: 1 });
931+
const fullQuery = query('collection');
932+
const filteredQuery1 = query('collection', filter('key', '==', 1));
933+
const filteredQuery2 = query('collection', filter('key', '>=', 1));
938934

939935
return (
940936
spec()
941937
.withGCEnabled(false)
942-
.withMaxConcurrentLimboResolutions(1)
943938

944-
// Max out the number of active limbo resolutions.
945-
.userListens(query1)
946-
.watchAcksFull(query1, 1000, doc1)
947-
.expectEvents(query1, { added: [doc1] })
948-
.userUnlistens(query1)
939+
// Start a limbo resolution listen for a document (doc1).
940+
.userListens(fullQuery)
941+
.watchAcksFull(fullQuery, 1000, doc1)
942+
.expectEvents(fullQuery, { added: [doc1] })
943+
.userUnlistens(fullQuery)
949944
.userListens(filteredQuery1)
950945
.expectEvents(filteredQuery1, { added: [doc1], fromCache: true })
951946
.watchAcksFull(filteredQuery1, 1001)
952947
.expectLimboDocs(doc1.key)
948+
.expectEnqueuedLimboDocs()
953949

954-
// Enqueue a limbo resolution for doc2.
955-
.userListens(query2)
956-
.watchAcksFull(query2, 1002, doc2)
957-
.expectEvents(query2, { added: [doc2] })
958-
.userUnlistens(query2)
950+
// Put doc1 into limbo in a different query; verify that another limbo
951+
// resolution is neither started nor enqueued.
959952
.userListens(filteredQuery2)
960-
.expectEvents(filteredQuery2, { added: [doc2], fromCache: true })
961-
.watchAcksFull(filteredQuery2, 1003)
962-
.expectLimboDocs(doc1.key)
963-
.expectEnqueuedLimboDocs(doc2.key)
964-
965-
// Start another query that puts doc2 into limbo again.
966-
.userListens(filteredQuery3)
967-
.expectEvents(filteredQuery3, { added: [doc2], fromCache: true })
968-
.watchAcksFull(filteredQuery3, 1004)
969-
.expectLimboDocs(doc1.key)
970-
.expectEnqueuedLimboDocs(doc2.key)
971-
972-
// Stop one of the queries that enqueued a limbo resolution for doc2;
973-
// verify that doc2 is not removed from the limbo resolution queue.
974-
.userUnlistens(filteredQuery3)
975-
.expectLimboDocs(doc1.key)
976-
.expectEnqueuedLimboDocs(doc2.key)
977-
978-
// Stop the other query that enqueued a limbo resolution for doc2;
979-
// verify that doc2 *is* removed from the limbo resolution queue.
980-
.userUnlistens(filteredQuery2)
953+
.expectEvents(filteredQuery2, { added: [doc1], fromCache: true })
954+
.watchAcksFull(filteredQuery2, 1002)
981955
.expectLimboDocs(doc1.key)
982956
.expectEnqueuedLimboDocs()
983957
);
984958
}
985959
);
986960

987961
specTest(
988-
'A limbo resolution for a document should not be started if one is already active',
962+
'A limbo resolution for a document should not be enqueued if one is already enqueued',
989963
[],
990964
() => {
991-
const doc1 = doc('collection/doc', 1000, { key: 1 });
992-
const query1 = query('collection');
993-
const filteredQuery1 = query('collection', filter('key', '==', 1));
994-
const filteredQuery2 = query('collection', filter('key', '>=', 1));
965+
const doc1 = doc('collection1/doc1', 1000, { key: 1 });
966+
const fullQuery1 = query('collection1');
967+
const filteredQuery1 = query('collection1', filter('key', '==', 1));
968+
const doc2 = doc('collection2/doc2', 1000, { key: 2 });
969+
const fullQuery2 = query('collection2');
970+
const filteredQuery2a = query('collection2', filter('key', '==', 2));
971+
const filteredQuery2b = query('collection2', filter('key', '>=', 2));
995972

996973
return (
997974
spec()
998975
.withGCEnabled(false)
976+
.withMaxConcurrentLimboResolutions(1)
999977

1000-
// Start a limbo resolution listen for a document (doc1).
1001-
.userListens(query1)
1002-
.watchAcksFull(query1, 1000, doc1)
1003-
.expectEvents(query1, { added: [doc1] })
1004-
.userUnlistens(query1)
978+
// Max out the number of active limbo resolutions.
979+
.userListens(fullQuery1)
980+
.watchAcksFull(fullQuery1, 1000, doc1)
981+
.expectEvents(fullQuery1, { added: [doc1] })
982+
.userUnlistens(fullQuery1)
1005983
.userListens(filteredQuery1)
1006984
.expectEvents(filteredQuery1, { added: [doc1], fromCache: true })
1007985
.watchAcksFull(filteredQuery1, 1001)
1008986
.expectLimboDocs(doc1.key)
1009-
.expectEnqueuedLimboDocs()
1010987

1011-
// Put doc1 into limbo in a different query; verify that another limbo
1012-
// resolution is neither started nor enqueued.
1013-
.userListens(filteredQuery2)
1014-
.expectEvents(filteredQuery2, { added: [doc1], fromCache: true })
1015-
.watchAcksFull(filteredQuery2, 1002)
988+
// Start a limbo resolution listen for a different document (doc2).
989+
.userListens(fullQuery2)
990+
.watchAcksFull(fullQuery2, 1002, doc2)
991+
.expectEvents(fullQuery2, { added: [doc2] })
992+
.userUnlistens(fullQuery2)
993+
.userListens(filteredQuery2a)
994+
.expectEvents(filteredQuery2a, { added: [doc2], fromCache: true })
995+
.watchAcksFull(filteredQuery2a, 1003)
1016996
.expectLimboDocs(doc1.key)
1017-
.expectEnqueuedLimboDocs()
997+
.expectEnqueuedLimboDocs(doc2.key)
998+
999+
// Put doc2 into limbo in a different query and verify that it's not
1000+
// added to the limbo resolution queue again.
1001+
.userListens(filteredQuery2b)
1002+
.expectEvents(filteredQuery2b, { added: [doc2], fromCache: true })
1003+
.watchAcksFull(filteredQuery2b, 1004)
1004+
.expectLimboDocs(doc1.key)
1005+
.expectEnqueuedLimboDocs(doc2.key)
10181006
);
10191007
}
10201008
);
10211009

10221010
specTest(
1023-
'A limbo resolution for a document should not be enqueued if one is already enqueued',
1011+
'A limbo resolution for a document should be removed from the queue when the last query listen stops',
10241012
[],
10251013
() => {
1026-
const doc1 = doc('collection1/doc1', 1000, { key: 1 });
1027-
const query1 = query('collection1');
1014+
const doc1 = doc('collection1/doc', 1000, { key: 1 });
1015+
const fullQuery1 = query('collection1');
10281016
const filteredQuery1 = query('collection1', filter('key', '==', 1));
1029-
const doc2 = doc('collection2/doc2', 1000, { key: 2 });
1030-
const query2 = query('collection2');
1031-
const filteredQuery2 = query('collection2', filter('key', '==', 2));
1032-
const filteredQuery3 = query('collection2', filter('key', '>=', 2));
1017+
1018+
const doc2 = doc('collection2/doc', 1000, { key: 2 });
1019+
const fullQuery2 = query('collection2');
1020+
const filteredQuery2a = query('collection2', filter('key', '==', 2));
1021+
const filteredQuery2b = query('collection2', filter('key', '>=', 2));
10331022

10341023
return (
10351024
spec()
10361025
.withGCEnabled(false)
10371026
.withMaxConcurrentLimboResolutions(1)
10381027

10391028
// Max out the number of active limbo resolutions.
1040-
.userListens(query1)
1041-
.watchAcksFull(query1, 1000, doc1)
1042-
.expectEvents(query1, { added: [doc1] })
1043-
.userUnlistens(query1)
1029+
.userListens(fullQuery1)
1030+
.watchAcksFull(fullQuery1, 1000, doc1)
1031+
.expectEvents(fullQuery1, { added: [doc1] })
1032+
.userUnlistens(fullQuery1)
10441033
.userListens(filteredQuery1)
10451034
.expectEvents(filteredQuery1, { added: [doc1], fromCache: true })
10461035
.watchAcksFull(filteredQuery1, 1001)
10471036
.expectLimboDocs(doc1.key)
10481037

1049-
// Start a limbo resolution listen for a different document (doc2).
1050-
.userListens(query2)
1051-
.watchAcksFull(query2, 1002, doc2)
1052-
.expectEvents(query2, { added: [doc2] })
1053-
.userUnlistens(query2)
1054-
.userListens(filteredQuery2)
1055-
.expectEvents(filteredQuery2, { added: [doc2], fromCache: true })
1056-
.watchAcksFull(filteredQuery2, 1003)
1038+
// Enqueue a limbo resolution for doc2.
1039+
.userListens(fullQuery2)
1040+
.watchAcksFull(fullQuery2, 1002, doc2)
1041+
.expectEvents(fullQuery2, { added: [doc2] })
1042+
.userUnlistens(fullQuery2)
1043+
.userListens(filteredQuery2a)
1044+
.expectEvents(filteredQuery2a, { added: [doc2], fromCache: true })
1045+
.watchAcksFull(filteredQuery2a, 1003)
10571046
.expectLimboDocs(doc1.key)
10581047
.expectEnqueuedLimboDocs(doc2.key)
10591048

1060-
// Put doc2 into limbo in a different query and verify that it's not
1061-
// added to the limbo resolution queue again.
1062-
.userListens(filteredQuery3)
1063-
.expectEvents(filteredQuery3, { added: [doc2], fromCache: true })
1064-
.watchAcksFull(filteredQuery3, 1004)
1049+
// Start another query that puts doc2 into limbo again.
1050+
.userListens(filteredQuery2b)
1051+
.expectEvents(filteredQuery2b, { added: [doc2], fromCache: true })
1052+
.watchAcksFull(filteredQuery2b, 1004)
1053+
.expectLimboDocs(doc1.key)
1054+
.expectEnqueuedLimboDocs(doc2.key)
1055+
1056+
// Stop one of the queries that enqueued a limbo resolution for doc2;
1057+
// verify that doc2 is not removed from the limbo resolution queue.
1058+
.userUnlistens(filteredQuery2b)
10651059
.expectLimboDocs(doc1.key)
10661060
.expectEnqueuedLimboDocs(doc2.key)
1061+
1062+
// Stop the other query that enqueued a limbo resolution for doc2;
1063+
// verify that doc2 *is* removed from the limbo resolution queue.
1064+
.userUnlistens(filteredQuery2a)
1065+
.expectLimboDocs(doc1.key)
1066+
.expectEnqueuedLimboDocs()
10671067
);
10681068
}
10691069
);

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

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1009,11 +1009,14 @@ abstract class TestRunner {
10091009
private validateEnqueuedLimboDocs(): void {
10101010
const actualLimboDocs = Array.from(
10111011
syncEngineGetEnqueuedLimboDocumentResolutions(this.syncEngine)
1012-
).sort();
1012+
);
10131013
const expectedLimboDocs = Array.from(this.expectedEnqueuedLimboDocs, key =>
10141014
key.path.canonicalString()
1015-
).sort();
1016-
expect(actualLimboDocs).to.deep.equal(expectedLimboDocs);
1015+
);
1016+
expect(actualLimboDocs).to.have.members(
1017+
expectedLimboDocs,
1018+
'The set of enqueued limbo documents is incorrect'
1019+
);
10171020
}
10181021

10191022
private async validateActiveTargets(): Promise<void> {

0 commit comments

Comments
 (0)