diff --git a/Firestore/Example/Tests/SpecTests/json/limbo_spec_test.json b/Firestore/Example/Tests/SpecTests/json/limbo_spec_test.json index 9b64e799977..29b2b981cc3 100644 --- a/Firestore/Example/Tests/SpecTests/json/limbo_spec_test.json +++ b/Firestore/Example/Tests/SpecTests/json/limbo_spec_test.json @@ -1211,9 +1211,9 @@ } ] }, - "Limbo documents handle receiving ack and then current": { + "Limbo resolution handles snapshot before CURRENT": { "describeName": "Limbo Documents:", - "itName": "Limbo documents handle receiving ack and then current", + "itName": "Limbo resolution handles snapshot before CURRENT", "tags": [], "config": { "useGarbageCollection": false @@ -1545,6 +1545,11 @@ ] } }, + { + "watchSnapshot": { + "version": 2000 + } + }, { "watchCurrent": [ [ @@ -1637,5 +1642,413 @@ } } ] + }, + "Limbo resolution handles snapshot before CURRENT [no document update]": { + "describeName": "Limbo Documents:", + "itName": "Limbo resolution handles snapshot before CURRENT [no document update]", + "tags": [], + "config": { + "useGarbageCollection": false + }, + "steps": [ + { + "userListen": [ + 2, + { + "path": "collection", + "filters": [], + "orderBys": [] + } + ], + "stateExpect": { + "activeTargets": { + "2": { + "query": { + "path": "collection", + "filters": [], + "orderBys": [] + }, + "resumeToken": "" + } + } + } + }, + { + "watchAck": [ + 2 + ] + }, + { + "watchEntity": { + "docs": [ + [ + "collection/a", + 1000, + { + "include": true, + "key": "a" + } + ], + [ + "collection/b", + 1000, + { + "include": true, + "key": "b" + } + ] + ], + "targets": [ + 2 + ] + } + }, + { + "watchCurrent": [ + [ + 2 + ], + "resume-token-1000" + ] + }, + { + "watchSnapshot": { + "version": 1000 + }, + "expect": [ + { + "query": { + "path": "collection", + "filters": [], + "orderBys": [] + }, + "added": [ + [ + "collection/a", + 1000, + { + "include": true, + "key": "a" + } + ], + [ + "collection/b", + 1000, + { + "include": true, + "key": "b" + } + ] + ], + "errorCode": 0, + "fromCache": false, + "hasPendingWrites": false + } + ] + }, + { + "userUnlisten": [ + 2, + { + "path": "collection", + "filters": [], + "orderBys": [] + } + ], + "stateExpect": { + "activeTargets": {} + } + }, + { + "userListen": [ + 4, + { + "path": "collection", + "limit": 1, + "filters": [ + [ + "include", + "==", + true + ] + ], + "orderBys": [] + } + ], + "stateExpect": { + "activeTargets": { + "4": { + "query": { + "path": "collection", + "limit": 1, + "filters": [ + [ + "include", + "==", + true + ] + ], + "orderBys": [] + }, + "resumeToken": "" + } + } + }, + "expect": [ + { + "query": { + "path": "collection", + "limit": 1, + "filters": [ + [ + "include", + "==", + true + ] + ], + "orderBys": [] + }, + "added": [ + [ + "collection/a", + 1000, + { + "include": true, + "key": "a" + } + ] + ], + "errorCode": 0, + "fromCache": true, + "hasPendingWrites": false + } + ] + }, + { + "watchAck": [ + 4 + ] + }, + { + "watchEntity": { + "docs": [ + [ + "collection/a", + 1000, + { + "include": true, + "key": "a" + } + ] + ], + "targets": [ + 4 + ] + } + }, + { + "watchCurrent": [ + [ + 4 + ], + "resume-token-2000" + ] + }, + { + "watchSnapshot": { + "version": 2000 + }, + "expect": [ + { + "query": { + "path": "collection", + "limit": 1, + "filters": [ + [ + "include", + "==", + true + ] + ], + "orderBys": [] + }, + "errorCode": 0, + "fromCache": false, + "hasPendingWrites": false + } + ] + }, + { + "userPatch": [ + "collection/a", + { + "include": false + } + ], + "expect": [ + { + "query": { + "path": "collection", + "limit": 1, + "filters": [ + [ + "include", + "==", + true + ] + ], + "orderBys": [] + }, + "added": [ + [ + "collection/b", + 1000, + { + "include": true, + "key": "b" + } + ] + ], + "removed": [ + [ + "collection/a", + 1000, + { + "include": true, + "key": "a" + } + ] + ], + "errorCode": 0, + "fromCache": true, + "hasPendingWrites": false + } + ], + "stateExpect": { + "limboDocs": [ + "collection/b" + ], + "activeTargets": { + "1": { + "query": { + "path": "collection/b", + "filters": [], + "orderBys": [] + }, + "resumeToken": "" + }, + "4": { + "query": { + "path": "collection", + "limit": 1, + "filters": [ + [ + "include", + "==", + true + ] + ], + "orderBys": [] + }, + "resumeToken": "" + } + } + } + }, + { + "watchAck": [ + 1 + ] + }, + { + "watchSnapshot": { + "version": 2000 + } + }, + { + "watchCurrent": [ + [ + 1 + ], + "resume-token-3000" + ] + }, + { + "watchSnapshot": { + "version": 3000 + }, + "stateExpect": { + "limboDocs": [], + "activeTargets": { + "4": { + "query": { + "path": "collection", + "limit": 1, + "filters": [ + [ + "include", + "==", + true + ] + ], + "orderBys": [] + }, + "resumeToken": "" + } + } + }, + "expect": [ + { + "query": { + "path": "collection", + "limit": 1, + "filters": [ + [ + "include", + "==", + true + ] + ], + "orderBys": [] + }, + "removed": [ + [ + "collection/b", + 1000, + { + "include": true, + "key": "b" + } + ] + ], + "errorCode": 0, + "fromCache": false, + "hasPendingWrites": false + } + ] + }, + { + "watchEntity": { + "docs": [ + [ + "collection/a", + 1000, + { + "include": true, + "key": "a" + } + ] + ], + "removedTargets": [ + 4 + ] + } + }, + { + "watchSnapshot": { + "version": 4000 + } + } + ] } } diff --git a/Firestore/Source/Core/FSTSyncEngine.mm b/Firestore/Source/Core/FSTSyncEngine.mm index bf7b053964b..dad81d4cd3b 100644 --- a/Firestore/Source/Core/FSTSyncEngine.mm +++ b/Firestore/Source/Core/FSTSyncEngine.mm @@ -114,6 +114,27 @@ - (instancetype)initWithQuery:(FSTQuery *)query @end +#pragma mark - LimboResolution + +/** Tracks a limbo resolution. */ +class LimboResolution { + public: + LimboResolution() { + } + + explicit LimboResolution(const DocumentKey &key) : key{key} { + } + + DocumentKey key; + + /** + * Set to true once we've received a document. This is used in remoteKeysForTarget and + * ultimately used by FSTWatchChangeAggregator to decide whether it needs to manufacture a delete + * event for the target once the target is CURRENT. + */ + bool document_received = false; +}; + #pragma mark - FSTSyncEngine @interface FSTSyncEngine () @@ -151,8 +172,11 @@ @implementation FSTSyncEngine { */ std::map _limboTargetsByKey; - /** The inverse of _limboTargetsByKey, a map of TargetId to the key of the limbo doc. */ - std::map _limboKeysByTarget; + /** + * Basically the inverse of limboTargetsByKey, a map of target ID to a LimboResolution (which + * includes the DocumentKey as well as whether we've received a document for the target). + */ + std::map _limboResolutionsByTarget; User _currentUser; } @@ -288,6 +312,35 @@ - (void)transactionWithRetries:(int)retries - (void)applyRemoteEvent:(FSTRemoteEvent *)remoteEvent { [self assertDelegateExistsForSelector:_cmd]; + // Update `receivedDocument` as appropriate for any limbo targets. + for (const auto &entry : remoteEvent.targetChanges) { + FSTTargetID targetID = entry.first; + FSTTargetChange *change = entry.second; + const auto iter = _limboResolutionsByTarget.find(targetID); + if (iter != _limboResolutionsByTarget.end()) { + LimboResolution &limboResolution = iter->second; + // Since this is a limbo resolution lookup, it's for a single document and it could be + // added, modified, or removed, but not a combination. + HARD_ASSERT(change.addedDocuments.size() + change.modifiedDocuments.size() + + change.removedDocuments.size() <= + 1, + "Limbo resolution for single document contains multiple changes."); + + if (change.addedDocuments.size() > 0) { + limboResolution.document_received = true; + } else if (change.modifiedDocuments.size() > 0) { + HARD_ASSERT(limboResolution.document_received, + "Received change for limbo target document without add."); + } else if (change.removedDocuments.size() > 0) { + HARD_ASSERT(limboResolution.document_received, + "Received remove for limbo target document without add."); + limboResolution.document_received = false; + } else { + // This was probably just a CURRENT targetChange or similar. + } + } + } + FSTMaybeDocumentDictionary *changes = [self.localStore applyRemoteEvent:remoteEvent]; [self emitNewSnapshotsWithChanges:changes remoteEvent:remoteEvent]; } @@ -310,13 +363,13 @@ - (void)applyChangedOnlineState:(FSTOnlineState)onlineState { - (void)rejectListenWithTargetID:(const TargetId)targetID error:(NSError *)error { [self assertDelegateExistsForSelector:_cmd]; - const auto iter = _limboKeysByTarget.find(targetID); - if (iter != _limboKeysByTarget.end()) { - const DocumentKey limboKey = iter->second; + const auto iter = _limboResolutionsByTarget.find(targetID); + if (iter != _limboResolutionsByTarget.end()) { + const DocumentKey limboKey = iter->second.key; // Since this query failed, we won't want to manually unlisten to it. // So go ahead and remove it from bookkeeping. _limboTargetsByKey.erase(limboKey); - _limboKeysByTarget.erase(targetID); + _limboResolutionsByTarget.erase(targetID); // TODO(dimond): Retry on transient errors? @@ -484,7 +537,7 @@ - (void)trackLimboChange:(FSTLimboDocumentChange *)limboChange { targetID:limboTargetID listenSequenceNumber:kIrrelevantSequenceNumber purpose:FSTQueryPurposeLimboResolution]; - _limboKeysByTarget[limboTargetID] = key; + _limboResolutionsByTarget.emplace(limboTargetID, LimboResolution{key}); [self.remoteStore listenToTargetWithQueryData:queryData]; _limboTargetsByKey[key] = limboTargetID; } @@ -499,7 +552,7 @@ - (void)removeLimboTargetForKey:(const DocumentKey &)key { TargetId limboTargetID = iter->second; [self.remoteStore stopListeningToTargetID:limboTargetID]; _limboTargetsByKey.erase(key); - _limboKeysByTarget.erase(limboTargetID); + _limboResolutionsByTarget.erase(limboTargetID); } // Used for testing @@ -520,8 +573,13 @@ - (void)userDidChange:(const User &)user { } - (firebase::firestore::model::DocumentKeySet)remoteKeysForTarget:(FSTBoxedTargetID *)targetId { - FSTQueryView *queryView = self.queryViewsByTarget[targetId]; - return queryView ? queryView.view.syncedDocuments : DocumentKeySet{}; + const auto iter = _limboResolutionsByTarget.find([targetId intValue]); + if (iter != _limboResolutionsByTarget.end() && iter->second.document_received) { + return DocumentKeySet{iter->second.key}; + } else { + FSTQueryView *queryView = self.queryViewsByTarget[targetId]; + return queryView ? queryView.view.syncedDocuments : DocumentKeySet{}; + } } @end