Skip to content

Commit 92e269c

Browse files
dconeybedconeybe
authored
Fix Firestore failing to return empty results from the local cache (#10437)
1 parent 17fe7ad commit 92e269c

File tree

17 files changed

+221
-72
lines changed

17 files changed

+221
-72
lines changed

‎Firestore/CHANGELOG.md‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,8 @@
22
- [fixed] Fix FAILED_PRECONDITION when writing to a deleted document in a
33
transaction (#10431).
44
- [fixed] Fixed data race in credentials provider (#10393).
5+
- [fixed] Fix Firestore failing to raise initial snapshot from empty local cache
6+
result (#10437).
57

68
# 10.0.0
79
- [feature] Added `Query.count()`, which fetches the number of documents in the

‎Firestore/Example/Tests/API/FIRQuerySnapshotTests.mm‎

Lines changed: 10 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -60,16 +60,18 @@ @interface FIRQuerySnapshotTests : XCTestCase
6060
@implementation FIRQuerySnapshotTests
6161

6262
- (void)testEquals {
63-
FIRQuerySnapshot *foo = FSTTestQuerySnapshot("foo", @{}, @{@"a" : @{@"a" : @1}}, true, false);
64-
FIRQuerySnapshot *fooDup = FSTTestQuerySnapshot("foo", @{}, @{@"a" : @{@"a" : @1}}, true, false);
63+
FIRQuerySnapshot *foo =
64+
FSTTestQuerySnapshot("foo", @{}, @{@"a" : @{@"a" : @1}}, true, false, false);
65+
FIRQuerySnapshot *fooDup =
66+
FSTTestQuerySnapshot("foo", @{}, @{@"a" : @{@"a" : @1}}, true, false, false);
6567
FIRQuerySnapshot *differentPath =
66-
FSTTestQuerySnapshot("bar", @{}, @{@"a" : @{@"a" : @1}}, true, false);
68+
FSTTestQuerySnapshot("bar", @{}, @{@"a" : @{@"a" : @1}}, true, false, false);
6769
FIRQuerySnapshot *differentDoc =
68-
FSTTestQuerySnapshot("foo", @{@"a" : @{@"b" : @1}}, @{}, true, false);
70+
FSTTestQuerySnapshot("foo", @{@"a" : @{@"b" : @1}}, @{}, true, false, false);
6971
FIRQuerySnapshot *noPendingWrites =
70-
FSTTestQuerySnapshot("foo", @{}, @{@"a" : @{@"a" : @1}}, false, false);
72+
FSTTestQuerySnapshot("foo", @{}, @{@"a" : @{@"a" : @1}}, false, false, false);
7173
FIRQuerySnapshot *fromCache =
72-
FSTTestQuerySnapshot("foo", @{}, @{@"a" : @{@"a" : @1}}, true, true);
74+
FSTTestQuerySnapshot("foo", @{}, @{@"a" : @{@"a" : @1}}, true, true, true);
7375
XCTAssertEqualObjects(foo, fooDup);
7476
XCTAssertNotEqualObjects(foo, differentPath);
7577
XCTAssertNotEqualObjects(foo, differentDoc);
@@ -103,7 +105,8 @@ ViewSnapshot viewSnapshot(query, newDocuments, oldDocuments, std::move(documentC
103105
/*mutated_keys=*/DocumentKeySet(),
104106
/*from_cache=*/false,
105107
/*sync_state_changed=*/true,
106-
/*excludes_metadata_changes=*/false);
108+
/*excludes_metadata_changes=*/false,
109+
/*has_cached_results=*/false);
107110
SnapshotMetadata metadata(/*pending_writes=*/false, /*from_cache=*/false);
108111
FIRQuerySnapshot *snapshot = [[FIRQuerySnapshot alloc] initWithFirestore:firestore
109112
originalQuery:query

‎Firestore/Example/Tests/API/FSTAPIHelpers.h‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -58,14 +58,16 @@ FIRDocumentReference *FSTTestDocRef(const char *path);
5858
* contents.
5959
* @param hasPendingWrites Whether the query snapshot has pending writes to the server.
6060
* @param fromCache Whether the query snapshot is cache result.
61+
* @param hasCachedResults Whether the query snapshot has results in the cache.
6162
* @return A query snapshot that consists of both sets of documents.
6263
*/
6364
FIRQuerySnapshot *FSTTestQuerySnapshot(
6465
const char *path,
6566
NSDictionary<NSString *, NSDictionary<NSString *, id> *> *oldDocs,
6667
NSDictionary<NSString *, NSDictionary<NSString *, id> *> *docsToAdd,
6768
BOOL hasPendingWrites,
68-
BOOL fromCache);
69+
BOOL fromCache,
70+
BOOL hasCachedResults);
6971

7072
#if __cplusplus
7173
} // extern "C"

‎Firestore/Example/Tests/API/FSTAPIHelpers.mm‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -42,10 +42,10 @@
4242
#include "Firestore/core/src/util/string_apple.h"
4343
#include "Firestore/core/test/unit/testutil/testutil.h"
4444

45+
using firebase::firestore::google_firestore_v1_Value;
4546
using firebase::firestore::api::SnapshotMetadata;
4647
using firebase::firestore::core::DocumentViewChange;
4748
using firebase::firestore::core::ViewSnapshot;
48-
using firebase::firestore::google_firestore_v1_Value;
4949
using firebase::firestore::model::DatabaseId;
5050
using firebase::firestore::model::Document;
5151
using firebase::firestore::model::DocumentComparator;
@@ -117,7 +117,8 @@
117117
NSDictionary<NSString *, NSDictionary<NSString *, id> *> *oldDocs,
118118
NSDictionary<NSString *, NSDictionary<NSString *, id> *> *docsToAdd,
119119
BOOL hasPendingWrites,
120-
BOOL fromCache) {
120+
BOOL fromCache,
121+
BOOL hasCachedResults) {
121122
FSTUserDataReader *reader = FSTTestUserDataReader();
122123

123124
SnapshotMetadata metadata(hasPendingWrites, fromCache);
@@ -154,7 +155,8 @@
154155
mutatedKeys,
155156
static_cast<bool>(fromCache),
156157
/*sync_state_changed=*/true,
157-
/*excludes_metadata_changes=*/false};
158+
/*excludes_metadata_changes=*/false,
159+
static_cast<bool>(hasCachedResults)};
158160
return [[FIRQuerySnapshot alloc] initWithFirestore:FSTTestFirestore().wrapped
159161
originalQuery:Query(path)
160162
snapshot:std::move(viewSnapshot)

‎Firestore/Example/Tests/Integration/API/FIRQueryTests.mm‎

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -395,6 +395,50 @@ - (void)testQueriesFireFromCacheWhenOffline {
395395
[registration remove];
396396
}
397397

398+
- (void)testQueriesCanRaiseInitialSnapshotFromCachedEmptyResults {
399+
FIRCollectionReference *collection = [self collectionRefWithDocuments:@{}];
400+
401+
// Populate the cache with empty query result.
402+
FIRQuerySnapshot *querySnapshotA = [self readDocumentSetForRef:collection];
403+
XCTAssertFalse(querySnapshotA.metadata.fromCache);
404+
XCTAssertEqualObjects(FIRQuerySnapshotGetData(querySnapshotA), @[]);
405+
406+
// Add a snapshot listener whose first event should be raised from cache.
407+
id<FIRListenerRegistration> registration = [collection
408+
addSnapshotListenerWithIncludeMetadataChanges:YES
409+
listener:self.eventAccumulator.valueEventHandler];
410+
FIRQuerySnapshot *querySnapshotB = [self.eventAccumulator awaitEventWithName:@"initial event"];
411+
XCTAssertTrue(querySnapshotB.metadata.fromCache);
412+
XCTAssertEqualObjects(FIRQuerySnapshotGetData(querySnapshotB), @[]);
413+
414+
[registration remove];
415+
}
416+
417+
- (void)testQueriesCanRaiseInitialSnapshotFromEmptyDueToDeleteCachedResults {
418+
NSDictionary *testDocs = @{
419+
@"a" : @{@"foo" : @1},
420+
};
421+
FIRCollectionReference *collection = [self collectionRefWithDocuments:testDocs];
422+
// Populate the cache with a single document.
423+
FIRQuerySnapshot *querySnapshotA = [self readDocumentSetForRef:collection];
424+
XCTAssertFalse(querySnapshotA.metadata.fromCache);
425+
XCTAssertEqualObjects(FIRQuerySnapshotGetData(querySnapshotA), @[ @{@"foo" : @1} ]);
426+
427+
// Delete the document, making the cached query result empty.
428+
FIRDocumentReference *docRef = [collection documentWithPath:@"a"];
429+
[self deleteDocumentRef:docRef];
430+
431+
// Add a snapshot listener whose first event should be raised from cache.
432+
id<FIRListenerRegistration> registration = [collection
433+
addSnapshotListenerWithIncludeMetadataChanges:YES
434+
listener:self.eventAccumulator.valueEventHandler];
435+
FIRQuerySnapshot *querySnapshotB = [self.eventAccumulator awaitEventWithName:@"initial event"];
436+
XCTAssertTrue(querySnapshotB.metadata.fromCache);
437+
XCTAssertEqualObjects(FIRQuerySnapshotGetData(querySnapshotB), @[]);
438+
439+
[registration remove];
440+
}
441+
398442
- (void)testDocumentChangesUseNSNotFound {
399443
NSDictionary *testDocs = @{
400444
@"a" : @{@"foo" : @1},

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

Lines changed: 52 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2261,7 +2261,6 @@
22612261
"describeName": "Listens:",
22622262
"itName": "Empty initial snapshot is raised from cache",
22632263
"tags": [
2264-
"no-ios"
22652264
],
22662265
"config": {
22672266
"numClients": 1,
@@ -2689,7 +2688,6 @@
26892688
"describeName": "Listens:",
26902689
"itName": "Empty-due-to-delete initial snapshot is raised from cache",
26912690
"tags": [
2692-
"no-ios"
26932691
],
26942692
"config": {
26952693
"numClients": 1,
@@ -3858,6 +3856,25 @@
38583856
},
38593857
"targetId": 2
38603858
},
3859+
"expectedSnapshotEvents": [
3860+
{
3861+
"errorCode": 0,
3862+
"fromCache": true,
3863+
"hasPendingWrites": false,
3864+
"query": {
3865+
"filters": [
3866+
[
3867+
"visible",
3868+
"==",
3869+
true
3870+
]
3871+
],
3872+
"orderBys": [
3873+
],
3874+
"path": "collection"
3875+
}
3876+
}
3877+
],
38613878
"expectedState": {
38623879
"activeTargets": {
38633880
"2": {
@@ -3979,6 +3996,20 @@
39793996
},
39803997
"targetId": 4
39813998
},
3999+
"expectedSnapshotEvents": [
4000+
{
4001+
"errorCode": 0,
4002+
"fromCache": true,
4003+
"hasPendingWrites": false,
4004+
"query": {
4005+
"filters": [
4006+
],
4007+
"orderBys": [
4008+
],
4009+
"path": "collection"
4010+
}
4011+
}
4012+
],
39824013
"expectedState": {
39834014
"activeTargets": {
39844015
"4": {
@@ -4355,6 +4386,25 @@
43554386
},
43564387
"targetId": 2
43574388
},
4389+
"expectedSnapshotEvents": [
4390+
{
4391+
"errorCode": 0,
4392+
"fromCache": true,
4393+
"hasPendingWrites": false,
4394+
"query": {
4395+
"filters": [
4396+
[
4397+
"visible",
4398+
"==",
4399+
true
4400+
]
4401+
],
4402+
"orderBys": [
4403+
],
4404+
"path": "collection"
4405+
}
4406+
}
4407+
],
43584408
"expectedState": {
43594409
"activeTargets": {
43604410
"2": {

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

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1012,6 +1012,20 @@
10121012
},
10131013
"targetId": 2
10141014
},
1015+
"expectedSnapshotEvents": [
1016+
{
1017+
"errorCode": 0,
1018+
"fromCache": true,
1019+
"hasPendingWrites": false,
1020+
"query": {
1021+
"filters": [
1022+
],
1023+
"orderBys": [
1024+
],
1025+
"path": "collection"
1026+
}
1027+
}
1028+
],
10151029
"expectedState": {
10161030
"activeTargets": {
10171031
"2": {

‎Firestore/core/src/core/query_listener.cc‎

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -89,7 +89,8 @@ bool QueryListener::OnViewSnapshot(ViewSnapshot snapshot) {
8989
snapshot.mutated_keys(),
9090
snapshot.from_cache(),
9191
snapshot.sync_state_changed(),
92-
/*excludes_metadata_changes=*/true};
92+
/*excludes_metadata_changes=*/true,
93+
snapshot.has_cached_results()};
9394
}
9495

9596
if (!raised_initial_event_) {
@@ -147,8 +148,10 @@ bool QueryListener::ShouldRaiseInitialEvent(const ViewSnapshot& snapshot,
147148
return false;
148149
}
149150

150-
// Raise data from cache if we have any documents or we are offline
151-
return !snapshot.documents().empty() || online_state == OnlineState::Offline;
151+
// Raise data from cache if we have any documents, have cached results before,
152+
// or we are offline.
153+
return !snapshot.documents().empty() || snapshot.has_cached_results() ||
154+
online_state == OnlineState::Offline;
152155
}
153156

154157
bool QueryListener::ShouldRaiseEvent(const ViewSnapshot& snapshot) const {
@@ -178,7 +181,8 @@ void QueryListener::RaiseInitialEvent(const ViewSnapshot& snapshot) {
178181

179182
ViewSnapshot modified_snapshot = ViewSnapshot::FromInitialDocuments(
180183
snapshot.query(), snapshot.documents(), snapshot.mutated_keys(),
181-
snapshot.from_cache(), snapshot.excludes_metadata_changes());
184+
snapshot.from_cache(), snapshot.excludes_metadata_changes(),
185+
snapshot.has_cached_results());
182186
raised_initial_event_ = true;
183187
listener_->OnEvent(std::move(modified_snapshot));
184188
}

‎Firestore/core/src/core/sync_engine.cc‎

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -110,10 +110,11 @@ TargetId SyncEngine::Listen(Query query) {
110110

111111
TargetData target_data = local_store_->AllocateTarget(query.ToTarget());
112112
TargetId target_id = target_data.target_id();
113+
nanopb::ByteString resume_token = target_data.resume_token();
113114
remote_store_->Listen(std::move(target_data));
114115

115-
ViewSnapshot view_snapshot =
116-
InitializeViewAndComputeSnapshot(query, target_id);
116+
ViewSnapshot view_snapshot = InitializeViewAndComputeSnapshot(
117+
query, target_id, std::move(resume_token));
117118
std::vector<ViewSnapshot> snapshots;
118119
// Not using the `std::initializer_list` constructor to avoid extra copies.
119120
snapshots.push_back(std::move(view_snapshot));
@@ -122,8 +123,8 @@ TargetId SyncEngine::Listen(Query query) {
122123
return target_id;
123124
}
124125

125-
ViewSnapshot SyncEngine::InitializeViewAndComputeSnapshot(const Query& query,
126-
TargetId target_id) {
126+
ViewSnapshot SyncEngine::InitializeViewAndComputeSnapshot(
127+
const Query& query, TargetId target_id, nanopb::ByteString resume_token) {
127128
QueryResult query_result =
128129
local_store_->ExecuteQuery(query, /* use_previous_results= */ true);
129130

@@ -135,9 +136,9 @@ ViewSnapshot SyncEngine::InitializeViewAndComputeSnapshot(const Query& query,
135136
const Query& mirror_query = queries_by_target_[target_id][0];
136137
current_sync_state =
137138
query_views_by_query_[mirror_query]->view().sync_state();
138-
synthesized_current_change = TargetChange::CreateSynthesizedTargetChange(
139-
current_sync_state == SyncState::Synced);
140139
}
140+
synthesized_current_change = TargetChange::CreateSynthesizedTargetChange(
141+
current_sync_state == SyncState::Synced, std::move(resume_token));
141142

142143
View view(query, query_result.remote_keys());
143144
ViewDocumentChanges view_doc_changes =

‎Firestore/core/src/core/sync_engine.h‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -233,8 +233,10 @@ class SyncEngine : public remote::RemoteStoreCallback, public QueryEventSource {
233233

234234
void AssertCallbackExists(absl::string_view source);
235235

236-
ViewSnapshot InitializeViewAndComputeSnapshot(const Query& query,
237-
model::TargetId target_id);
236+
ViewSnapshot InitializeViewAndComputeSnapshot(
237+
const Query& query,
238+
model::TargetId target_id,
239+
nanopb::ByteString resume_token);
238240

239241
void RemoveAndCleanupTarget(model::TargetId target_id, util::Status status);
240242

0 commit comments

Comments
 (0)