From 13e20ee4df2d963c9fca498100008411bd990413 Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Wed, 24 Oct 2018 13:40:01 -0700 Subject: [PATCH 1/3] View changes for held write acks --- .../Tests/API/FIRQuerySnapshotTests.mm | 8 +- Firestore/Example/Tests/API/FSTAPIHelpers.mm | 14 +++- .../Tests/Core/FSTQueryListenerTests.mm | 16 ++-- .../Example/Tests/Core/FSTViewSnapshotTest.mm | 6 +- Firestore/Example/Tests/Core/FSTViewTests.mm | 73 ++++++++++++++++- Firestore/Source/API/FIRDocumentChange.mm | 22 +++--- Firestore/Source/API/FIRDocumentReference.mm | 7 +- .../Source/API/FIRDocumentSnapshot+Internal.h | 3 +- Firestore/Source/API/FIRDocumentSnapshot.mm | 32 +++++--- Firestore/Source/API/FIRQuerySnapshot.mm | 11 ++- Firestore/Source/API/FIRTransaction.mm | 3 +- Firestore/Source/Core/FSTEventManager.mm | 4 +- Firestore/Source/Core/FSTFirestoreClient.mm | 3 +- Firestore/Source/Core/FSTView.mm | 79 +++++++++++++------ Firestore/Source/Core/FSTViewSnapshot.h | 9 ++- Firestore/Source/Core/FSTViewSnapshot.mm | 19 +++-- .../Source/Local/FSTLocalDocumentsView.mm | 6 +- Firestore/Source/Model/FSTDocument.mm | 3 +- 18 files changed, 230 insertions(+), 88 deletions(-) diff --git a/Firestore/Example/Tests/API/FIRQuerySnapshotTests.mm b/Firestore/Example/Tests/API/FIRQuerySnapshotTests.mm index 98fd8990f7f..fd91013eebc 100644 --- a/Firestore/Example/Tests/API/FIRQuerySnapshotTests.mm +++ b/Firestore/Example/Tests/API/FIRQuerySnapshotTests.mm @@ -93,7 +93,7 @@ - (void)testIncludeMetadataChanges { oldDocuments:oldDocuments documentChanges:documentChanges fromCache:NO - hasPendingWrites:NO + mutatedKeys:DocumentKeySet {} syncStateChanged:YES]; FIRSnapshotMetadata *metadata = [FIRSnapshotMetadata snapshotMetadataWithPendingWrites:NO fromCache:NO]; @@ -105,11 +105,13 @@ - (void)testIncludeMetadataChanges { FIRQueryDocumentSnapshot *doc1Snap = [FIRQueryDocumentSnapshot snapshotWithFirestore:firestore documentKey:doc1New.key document:doc1New - fromCache:NO]; + fromCache:NO + hasPendingWrites:NO]; FIRQueryDocumentSnapshot *doc2Snap = [FIRQueryDocumentSnapshot snapshotWithFirestore:firestore documentKey:doc2New.key document:doc2New - fromCache:NO]; + fromCache:NO + hasPendingWrites:NO]; NSArray *changesWithoutMetadata = @[ [[FIRDocumentChange alloc] initWithType:FIRDocumentChangeTypeModified diff --git a/Firestore/Example/Tests/API/FSTAPIHelpers.mm b/Firestore/Example/Tests/API/FSTAPIHelpers.mm index 860e3ff770a..8f352cb59e2 100644 --- a/Firestore/Example/Tests/API/FSTAPIHelpers.mm +++ b/Firestore/Example/Tests/API/FSTAPIHelpers.mm @@ -70,7 +70,8 @@ return [FIRDocumentSnapshot snapshotWithFirestore:FSTTestFirestore() documentKey:testutil::Key(path) document:doc - fromCache:fromCache]; + fromCache:fromCache + hasPendingWrites:hasMutations]; } FIRCollectionReference *FSTTestCollectionRef(const absl::string_view path) { @@ -93,16 +94,22 @@ FIRSnapshotMetadata *metadata = [FIRSnapshotMetadata snapshotMetadataWithPendingWrites:hasPendingWrites fromCache:fromCache]; FSTDocumentSet *oldDocuments = FSTTestDocSet(FSTDocumentComparatorByKey, @[]); + DocumentKeySet mutatedKeys; for (NSString *key in oldDocs) { + const absl::string_view documentKey = util::StringFormat("%s/%s", path, key); oldDocuments = [oldDocuments documentSetByAddingDocument:FSTTestDoc(util::StringFormat("%s/%s", path, key), 1, oldDocs[key], hasPendingWrites ? FSTDocumentStateLocalMutations : FSTDocumentStateSynced)]; + if (hasPendingWrites) { + mutatedKeys = mutatedKeys.insert(testutil::Key(documentKey)); + } } FSTDocumentSet *newDocuments = oldDocuments; NSArray *documentChanges = [NSArray array]; for (NSString *key in docsToAdd) { + const absl::string_view documentKey = util::StringFormat("%s/%s", path, key); FSTDocument *docToAdd = FSTTestDoc(util::StringFormat("%s/%s", path, key), 1, docsToAdd[key], hasPendingWrites ? FSTDocumentStateLocalMutations : FSTDocumentStateSynced); @@ -111,13 +118,16 @@ arrayByAddingObject:[FSTDocumentViewChange changeWithDocument:docToAdd type:FSTDocumentViewChangeTypeAdded]]; + if (hasPendingWrites) { + mutatedKeys = mutatedKeys.insert(testutil::Key(documentKey)); + } } FSTViewSnapshot *viewSnapshot = [[FSTViewSnapshot alloc] initWithQuery:FSTTestQuery(path) documents:newDocuments oldDocuments:oldDocuments documentChanges:documentChanges fromCache:fromCache - hasPendingWrites:hasPendingWrites + mutatedKeys:mutatedKeys syncStateChanged:YES]; return [FIRQuerySnapshot snapshotWithFirestore:FSTTestFirestore() originalQuery:FSTTestQuery(path) diff --git a/Firestore/Example/Tests/Core/FSTQueryListenerTests.mm b/Firestore/Example/Tests/Core/FSTQueryListenerTests.mm index 6aa9fe4ffdc..0ee08f7a293 100644 --- a/Firestore/Example/Tests/Core/FSTQueryListenerTests.mm +++ b/Firestore/Example/Tests/Core/FSTQueryListenerTests.mm @@ -91,7 +91,7 @@ - (void)testRaisesCollectionEvents { oldDocuments:[FSTDocumentSet documentSetWithComparator:snap2.query.comparator] documentChanges:@[ change1, change4 ] fromCache:snap2.fromCache - hasPendingWrites:snap2.hasPendingWrites + mutatedKeys:snap2.mutatedKeys syncStateChanged:YES]; XCTAssertEqualObjects(otherAccum, (@[ expectedSnap2 ])); } @@ -290,7 +290,7 @@ - (void)testRaisesQueryMetadataEventsOnlyWhenHasPendingWritesOnTheQueryChanges { oldDocuments:snap3.documents documentChanges:@[] fromCache:snap4.fromCache - hasPendingWrites:NO + mutatedKeys:snap4.mutatedKeys syncStateChanged:snap4.syncStateChanged]; XCTAssertEqualObjects(fullAccum, (@[ snap1, snap3, expectedSnap4 ])); } @@ -324,7 +324,7 @@ - (void)testMetadataOnlyDocumentChangesAreFilteredOutWhenIncludeDocumentMetadata oldDocuments:snap1.documents documentChanges:@[ change3 ] fromCache:snap2.isFromCache - hasPendingWrites:snap2.hasPendingWrites + mutatedKeys:snap2.mutatedKeys syncStateChanged:snap2.syncStateChanged]; XCTAssertEqualObjects(filteredAccum, (@[ snap1, expectedSnap2 ])); } @@ -365,7 +365,7 @@ - (void)testWillWaitForSyncIfOnline { oldDocuments:[FSTDocumentSet documentSetWithComparator:snap3.query.comparator] documentChanges:@[ change1, change2 ] fromCache:NO - hasPendingWrites:NO + mutatedKeys:snap3.mutatedKeys syncStateChanged:YES]; XCTAssertEqualObjects(events, (@[ expectedSnap ])); } @@ -404,14 +404,14 @@ - (void)testWillRaiseInitialEventWhenGoingOffline { oldDocuments:[FSTDocumentSet documentSetWithComparator:snap1.query.comparator] documentChanges:@[ change1 ] fromCache:YES - hasPendingWrites:NO + mutatedKeys:snap1.mutatedKeys syncStateChanged:YES]; FSTViewSnapshot *expectedSnap2 = [[FSTViewSnapshot alloc] initWithQuery:query documents:snap2.documents oldDocuments:snap1.documents documentChanges:@[ change2 ] fromCache:YES - hasPendingWrites:NO + mutatedKeys:snap2.mutatedKeys syncStateChanged:NO]; XCTAssertEqualObjects(events, (@[ expectedSnap1, expectedSnap2 ])); } @@ -437,7 +437,7 @@ - (void)testWillRaiseInitialEventWhenGoingOfflineAndThereAreNoDocs { oldDocuments:[FSTDocumentSet documentSetWithComparator:snap1.query.comparator] documentChanges:@[] fromCache:YES - hasPendingWrites:NO + mutatedKeys:snap1.mutatedKeys syncStateChanged:YES]; XCTAssertEqualObjects(events, (@[ expectedSnap ])); } @@ -462,7 +462,7 @@ - (void)testWillRaiseInitialEventWhenStartingOfflineAndThereAreNoDocs { oldDocuments:[FSTDocumentSet documentSetWithComparator:snap1.query.comparator] documentChanges:@[] fromCache:YES - hasPendingWrites:NO + mutatedKeys:snap1.mutatedKeys syncStateChanged:YES]; XCTAssertEqualObjects(events, (@[ expectedSnap ])); } diff --git a/Firestore/Example/Tests/Core/FSTViewSnapshotTest.mm b/Firestore/Example/Tests/Core/FSTViewSnapshotTest.mm index 21210b82b26..a6cc2f8811f 100644 --- a/Firestore/Example/Tests/Core/FSTViewSnapshotTest.mm +++ b/Firestore/Example/Tests/Core/FSTViewSnapshotTest.mm @@ -116,7 +116,7 @@ - (void)testViewSnapshotConstructor { type:FSTDocumentViewChangeTypeAdded] ]; BOOL fromCache = YES; - BOOL hasPendingWrites = NO; + DocumentKeySet mutatedKeys; BOOL syncStateChanged = YES; FSTViewSnapshot *snapshot = [[FSTViewSnapshot alloc] initWithQuery:query @@ -124,7 +124,7 @@ - (void)testViewSnapshotConstructor { oldDocuments:oldDocuments documentChanges:documentChanges fromCache:fromCache - hasPendingWrites:hasPendingWrites + mutatedKeys:mutatedKeys syncStateChanged:syncStateChanged]; XCTAssertEqual(snapshot.query, query); @@ -132,7 +132,7 @@ - (void)testViewSnapshotConstructor { XCTAssertEqual(snapshot.oldDocuments, oldDocuments); XCTAssertEqual(snapshot.documentChanges, documentChanges); XCTAssertEqual(snapshot.fromCache, fromCache); - XCTAssertEqual(snapshot.hasPendingWrites, hasPendingWrites); + XCTAssertEqual(snapshot.mutatedKeys, mutatedKeys); XCTAssertEqual(snapshot.syncStateChanged, syncStateChanged); } diff --git a/Firestore/Example/Tests/Core/FSTViewTests.mm b/Firestore/Example/Tests/Core/FSTViewTests.mm index 34c61a7d41e..9ca3cea1a75 100644 --- a/Firestore/Example/Tests/Core/FSTViewTests.mm +++ b/Firestore/Example/Tests/Core/FSTViewTests.mm @@ -601,8 +601,7 @@ - (void)testRemovesKeysFromMutatedKeysWhenNewDocHasNoLocalChanges { [view applyChangesToDocuments:changes]; XCTAssertEqual(changes.mutatedKeys, (DocumentKeySet{doc2.key})); - FSTDocument *doc2Prime = - FSTTestDoc("rooms/eros/messages/1", 0, @{}, FSTDocumentStateSynced); + FSTDocument *doc2Prime = FSTTestDoc("rooms/eros/messages/1", 0, @{}, FSTDocumentStateSynced); changes = [view computeChangesWithDocuments:FSTTestDocUpdates(@[ doc2Prime ])]; [view applyChangesToDocuments:changes]; XCTAssertEqual(changes.mutatedKeys, DocumentKeySet{}); @@ -642,6 +641,76 @@ - (void)testRemembersLocalMutationsFromPreviousCallToComputeChangesWithDocuments XCTAssertEqual(changes.mutatedKeys, (DocumentKeySet{doc2.key})); } +- (void)testRaisesHasPendingWritesForPendingMutationsInInitialSnapshot { + FSTQuery *query = [self queryForMessages]; + FSTDocument *doc1 = FSTTestDoc("rooms/eros/messages/1", 0, @{}, FSTDocumentStateLocalMutations); + FSTView *view = [[FSTView alloc] initWithQuery:query remoteDocuments:DocumentKeySet{}]; + FSTViewDocumentChanges *changes = [view computeChangesWithDocuments:FSTTestDocUpdates(@[ doc1 ])]; + FSTViewChange *viewChange = [view applyChangesToDocuments:changes]; + XCTAssertTrue(viewChange.snapshot.hasPendingWrites); +} + +- (void)testDoesntRaiseHasPendingWritesForCommittedMutationsInInitialSnapshot { + FSTQuery *query = [self queryForMessages]; + FSTDocument *doc1 = + FSTTestDoc("rooms/eros/messages/1", 0, @{}, FSTDocumentStateCommittedMutations); + FSTView *view = [[FSTView alloc] initWithQuery:query remoteDocuments:DocumentKeySet{}]; + FSTViewDocumentChanges *changes = [view computeChangesWithDocuments:FSTTestDocUpdates(@[ doc1 ])]; + FSTViewChange *viewChange = [view applyChangesToDocuments:changes]; + XCTAssertFalse(viewChange.snapshot.hasPendingWrites); +} + +- (void)testSuppressesWriteAcknowledgementIfWatchHasNotCaughtUp { + // This test verifies that we don't get three events for an FSTServerTimestamp mutation. We + // suppress the event generated by the write acknowledgement and instead wait for Watch to catch + // up. + + FSTQuery *query = [self queryForMessages]; + FSTDocument *doc1 = FSTTestDoc("rooms/eros/messages/1", 1, + @{@"time" : @1}, FSTDocumentStateLocalMutations); + FSTDocument *doc1Committed = FSTTestDoc("rooms/eros/messages/1", 2, + @{@"time" : @2}, FSTDocumentStateCommittedMutations); + FSTDocument *doc1Acknowledged = FSTTestDoc("rooms/eros/messages/1", 2, + @{@"time" : @2}, FSTDocumentStateSynced); + FSTDocument *doc2 = FSTTestDoc("rooms/eros/messages/2", 1, + @{@"time" : @1}, FSTDocumentStateLocalMutations); + FSTDocument *doc2Modified = FSTTestDoc("rooms/eros/messages/2", 2, + @{@"time" : @3}, FSTDocumentStateLocalMutations); + FSTDocument *doc2Acknowledged = FSTTestDoc("rooms/eros/messages/2", 2, + @{@"time" : @3}, FSTDocumentStateSynced); + FSTView *view = [[FSTView alloc] initWithQuery:query remoteDocuments:DocumentKeySet{}]; + FSTViewDocumentChanges *changes = + [view computeChangesWithDocuments:FSTTestDocUpdates(@[ doc1, doc2 ])]; + FSTViewChange *viewChange = [view applyChangesToDocuments:changes]; + + XCTAssertEqualObjects( + (@[ + [FSTDocumentViewChange changeWithDocument:doc1 type:FSTDocumentViewChangeTypeAdded], + [FSTDocumentViewChange changeWithDocument:doc2 type:FSTDocumentViewChangeTypeAdded] + ]), + viewChange.snapshot.documentChanges); + + changes = [view computeChangesWithDocuments:FSTTestDocUpdates(@[ doc1Committed, doc2Modified ])]; + viewChange = [view applyChangesToDocuments:changes]; + // The 'doc1Committed' update is suppressed + XCTAssertEqualObjects( + (@[ [FSTDocumentViewChange changeWithDocument:doc2Modified + type:FSTDocumentViewChangeTypeModified] ]), + viewChange.snapshot.documentChanges); + + changes = + [view computeChangesWithDocuments:FSTTestDocUpdates(@[ doc1Acknowledged, doc2Acknowledged ])]; + viewChange = [view applyChangesToDocuments:changes]; + XCTAssertEqualObjects( + (@[ + [FSTDocumentViewChange changeWithDocument:doc1Acknowledged + type:FSTDocumentViewChangeTypeModified], + [FSTDocumentViewChange changeWithDocument:doc2Acknowledged + type:FSTDocumentViewChangeTypeMetadata] + ]), + viewChange.snapshot.documentChanges); +} + @end NS_ASSUME_NONNULL_END diff --git a/Firestore/Source/API/FIRDocumentChange.mm b/Firestore/Source/API/FIRDocumentChange.mm index 0b1413f5dca..973e4f73a9a 100644 --- a/Firestore/Source/API/FIRDocumentChange.mm +++ b/Firestore/Source/API/FIRDocumentChange.mm @@ -60,11 +60,12 @@ + (FIRDocumentChangeType)documentChangeTypeForChange:(FSTDocumentViewChange *)ch NSUInteger index = 0; NSMutableArray *changes = [NSMutableArray array]; for (FSTDocumentViewChange *change in snapshot.documentChanges) { - FIRQueryDocumentSnapshot *document = - [FIRQueryDocumentSnapshot snapshotWithFirestore:firestore - documentKey:change.document.key - document:change.document - fromCache:snapshot.isFromCache]; + FIRQueryDocumentSnapshot *document = [FIRQueryDocumentSnapshot + snapshotWithFirestore:firestore + documentKey:change.document.key + document:change.document + fromCache:snapshot.isFromCache + hasPendingWrites:snapshot.mutatedKeys.contains(change.document.key)]; HARD_ASSERT(change.type == FSTDocumentViewChangeTypeAdded, "Invalid event type for first snapshot"); HARD_ASSERT(!lastDocument || snapshot.query.comparator(lastDocument, change.document) == @@ -86,11 +87,12 @@ + (FIRDocumentChangeType)documentChangeTypeForChange:(FSTDocumentViewChange *)ch continue; } - FIRQueryDocumentSnapshot *document = - [FIRQueryDocumentSnapshot snapshotWithFirestore:firestore - documentKey:change.document.key - document:change.document - fromCache:snapshot.isFromCache]; + FIRQueryDocumentSnapshot *document = [FIRQueryDocumentSnapshot + snapshotWithFirestore:firestore + documentKey:change.document.key + document:change.document + fromCache:snapshot.isFromCache + hasPendingWrites:snapshot.mutatedKeys.contains(change.document.key)]; NSUInteger oldIndex = NSNotFound; NSUInteger newIndex = NSNotFound; diff --git a/Firestore/Source/API/FIRDocumentReference.mm b/Firestore/Source/API/FIRDocumentReference.mm index 54d9c73d640..0a0034f95af 100644 --- a/Firestore/Source/API/FIRDocumentReference.mm +++ b/Firestore/Source/API/FIRDocumentReference.mm @@ -274,10 +274,15 @@ - (void)getDocumentWithSource:(FIRFirestoreSource)source HARD_ASSERT(snapshot.documents.count <= 1, "Too many document returned on a document query"); FSTDocument *document = [snapshot.documents documentForKey:key]; + BOOL hasPendingWrites = document + ? snapshot.mutatedKeys.contains(key) + : NO; // We don't raise `hasPendingWrites` for deleted documents. + FIRDocumentSnapshot *result = [FIRDocumentSnapshot snapshotWithFirestore:firestore documentKey:key document:document - fromCache:snapshot.fromCache]; + fromCache:snapshot.fromCache + hasPendingWrites:hasPendingWrites]; listener(result, nil); }; diff --git a/Firestore/Source/API/FIRDocumentSnapshot+Internal.h b/Firestore/Source/API/FIRDocumentSnapshot+Internal.h index f4cef9a5e91..fdc5ac8a3d5 100644 --- a/Firestore/Source/API/FIRDocumentSnapshot+Internal.h +++ b/Firestore/Source/API/FIRDocumentSnapshot+Internal.h @@ -29,7 +29,8 @@ NS_ASSUME_NONNULL_BEGIN + (instancetype)snapshotWithFirestore:(FIRFirestore *)firestore documentKey:(firebase::firestore::model::DocumentKey)documentKey document:(nullable FSTDocument *)document - fromCache:(BOOL)fromCache; + fromCache:(BOOL)fromCache + hasPendingWrites:(BOOL)pendingWrites; @property(nonatomic, strong, readonly, nullable) FSTDocument *internalDocument; diff --git a/Firestore/Source/API/FIRDocumentSnapshot.mm b/Firestore/Source/API/FIRDocumentSnapshot.mm index dff5b7fa3e2..6696d552b4c 100644 --- a/Firestore/Source/API/FIRDocumentSnapshot.mm +++ b/Firestore/Source/API/FIRDocumentSnapshot.mm @@ -59,13 +59,15 @@ @interface FIRDocumentSnapshot () - (instancetype)initWithFirestore:(FIRFirestore *)firestore documentKey:(DocumentKey)documentKey document:(nullable FSTDocument *)document - fromCache:(BOOL)fromCache NS_DESIGNATED_INITIALIZER; + fromCache:(BOOL)fromCache + hasPendingWrites:(BOOL)pendingWrites NS_DESIGNATED_INITIALIZER; - (const DocumentKey &)internalKey; @property(nonatomic, strong, readonly) FIRFirestore *firestore; @property(nonatomic, strong, readonly, nullable) FSTDocument *internalDocument; @property(nonatomic, assign, readonly) BOOL fromCache; +@property(nonatomic, assign, readonly) BOOL pendingWrites; @end @@ -74,11 +76,13 @@ @implementation FIRDocumentSnapshot (Internal) + (instancetype)snapshotWithFirestore:(FIRFirestore *)firestore documentKey:(DocumentKey)documentKey document:(nullable FSTDocument *)document - fromCache:(BOOL)fromCache { + fromCache:(BOOL)fromCache + hasPendingWrites:(BOOL)pendingWrites { return [[[self class] alloc] initWithFirestore:firestore documentKey:std::move(documentKey) document:document - fromCache:fromCache]; + fromCache:fromCache + hasPendingWrites:pendingWrites]; } @end @@ -93,12 +97,14 @@ @implementation FIRDocumentSnapshot { - (instancetype)initWithFirestore:(FIRFirestore *)firestore documentKey:(DocumentKey)documentKey document:(nullable FSTDocument *)document - fromCache:(BOOL)fromCache { + fromCache:(BOOL)fromCache + hasPendingWrites:(BOOL)pendingWrites { if (self = [super init]) { _firestore = firestore; _internalKey = std::move(documentKey); _internalDocument = document; _fromCache = fromCache; + _pendingWrites = pendingWrites; } return self; } @@ -123,13 +129,15 @@ - (BOOL)isEqualToSnapshot:(nullable FIRDocumentSnapshot *)snapshot { return [self.firestore isEqual:snapshot.firestore] && self.internalKey == snapshot.internalKey && (self.internalDocument == snapshot.internalDocument || [self.internalDocument isEqual:snapshot.internalDocument]) && - self.fromCache == snapshot.fromCache; + _pendingWrites == snapshot->_pendingWrites; + self.fromCache == snapshot.fromCache; } - (NSUInteger)hash { NSUInteger hash = [self.firestore hash]; hash = hash * 31u + self.internalKey.Hash(); hash = hash * 31u + [self.internalDocument hash]; + hash = hash * 31u + (_pendingWrites ? 1 : 0); hash = hash * 31u + (self.fromCache ? 1 : 0); return hash; } @@ -150,9 +158,8 @@ - (NSString *)documentID { - (FIRSnapshotMetadata *)metadata { if (!_cachedMetadata) { - _cachedMetadata = [FIRSnapshotMetadata - snapshotMetadataWithPendingWrites:self.internalDocument.hasLocalMutations - fromCache:self.fromCache]; + _cachedMetadata = [FIRSnapshotMetadata snapshotMetadataWithPendingWrites:_pendingWrites + fromCache:self.fromCache]; } return _cachedMetadata; } @@ -256,7 +263,8 @@ @interface FIRQueryDocumentSnapshot () - (instancetype)initWithFirestore:(FIRFirestore *)firestore documentKey:(DocumentKey)documentKey document:(FSTDocument *)document - fromCache:(BOOL)fromCache NS_DESIGNATED_INITIALIZER; + fromCache:(BOOL)fromCache + hasPendingWrites:(BOOL)pendingWrites NS_DESIGNATED_INITIALIZER; @end @@ -265,11 +273,13 @@ @implementation FIRQueryDocumentSnapshot - (instancetype)initWithFirestore:(FIRFirestore *)firestore documentKey:(DocumentKey)documentKey document:(FSTDocument *)document - fromCache:(BOOL)fromCache { + fromCache:(BOOL)fromCache + hasPendingWrites:(BOOL)pendingWrites { self = [super initWithFirestore:firestore documentKey:std::move(documentKey) document:document - fromCache:fromCache]; + fromCache:fromCache + hasPendingWrites:pendingWrites]; return self; } diff --git a/Firestore/Source/API/FIRQuerySnapshot.mm b/Firestore/Source/API/FIRQuerySnapshot.mm index 7a6018bac6c..028d99f2b93 100644 --- a/Firestore/Source/API/FIRQuerySnapshot.mm +++ b/Firestore/Source/API/FIRQuerySnapshot.mm @@ -130,10 +130,13 @@ - (NSInteger)count { NSMutableArray *result = [NSMutableArray array]; for (FSTDocument *document in documentSet.documentEnumerator) { - [result addObject:[FIRQueryDocumentSnapshot snapshotWithFirestore:firestore - documentKey:document.key - document:document - fromCache:fromCache]]; + [result + addObject:[FIRQueryDocumentSnapshot + snapshotWithFirestore:firestore + documentKey:document.key + document:document + fromCache:fromCache + hasPendingWrites:self.snapshot.mutatedKeys.contains(document.key)]]; } _documents = result; diff --git a/Firestore/Source/API/FIRTransaction.mm b/Firestore/Source/API/FIRTransaction.mm index e8c75979dfb..773c594e984 100644 --- a/Firestore/Source/API/FIRTransaction.mm +++ b/Firestore/Source/API/FIRTransaction.mm @@ -124,7 +124,8 @@ - (void)getDocument:(FIRDocumentReference *)document [FIRDocumentSnapshot snapshotWithFirestore:self.firestore documentKey:internalDoc.key document:(FSTDocument *)internalDoc - fromCache:NO]; + fromCache:NO + hasPendingWrites:NO]; completion(doc, nil); } else if ([internalDoc isKindOfClass:[FSTDeletedDocument class]]) { completion(nil, nil); diff --git a/Firestore/Source/Core/FSTEventManager.mm b/Firestore/Source/Core/FSTEventManager.mm index d54a0d13ef9..cf634500ce5 100644 --- a/Firestore/Source/Core/FSTEventManager.mm +++ b/Firestore/Source/Core/FSTEventManager.mm @@ -136,7 +136,7 @@ - (void)queryDidChangeViewSnapshot:(FSTViewSnapshot *)snapshot { oldDocuments:snapshot.oldDocuments documentChanges:changes fromCache:snapshot.fromCache - hasPendingWrites:snapshot.hasPendingWrites + mutatedKeys:snapshot.mutatedKeys syncStateChanged:snapshot.syncStateChanged]; } @@ -214,7 +214,7 @@ - (void)raiseInitialEventForSnapshot:(FSTViewSnapshot *)snapshot { oldDocuments:[FSTDocumentSet documentSetWithComparator:snapshot.query.comparator] documentChanges:[FSTQueryListener getInitialViewChangesFor:snapshot] fromCache:snapshot.fromCache - hasPendingWrites:snapshot.hasPendingWrites + mutatedKeys:snapshot.mutatedKeys syncStateChanged:YES]; self.raisedInitialEvent = YES; self.viewSnapshotHandler(snapshot, nil); diff --git a/Firestore/Source/Core/FSTFirestoreClient.mm b/Firestore/Source/Core/FSTFirestoreClient.mm index af3a9b098cb..9f040292103 100644 --- a/Firestore/Source/Core/FSTFirestoreClient.mm +++ b/Firestore/Source/Core/FSTFirestoreClient.mm @@ -292,7 +292,8 @@ - (void)getDocumentFromLocalCache:(FIRDocumentReference *)doc result = [FIRDocumentSnapshot snapshotWithFirestore:doc.firestore documentKey:doc.key document:document - fromCache:YES]; + fromCache:YES + hasPendingWrites:document.hasLocalMutations]; } else { error = [NSError errorWithDomain:FIRFirestoreErrorDomain code:FIRFirestoreErrorCodeUnavailable diff --git a/Firestore/Source/Core/FSTView.mm b/Firestore/Source/Core/FSTView.mm index c02f221a1cc..0ebb4b987a0 100644 --- a/Firestore/Source/Core/FSTView.mm +++ b/Firestore/Source/Core/FSTView.mm @@ -213,6 +213,7 @@ - (FSTViewDocumentChanges *)computeChangesWithDocuments:(FSTMaybeDocumentDiction __block DocumentKeySet newMutatedKeys = previousChanges ? previousChanges.mutatedKeys : _mutatedKeys; + __block DocumentKeySet oldMutatedKeys = _mutatedKeys; __block FSTDocumentSet *newDocumentSet = oldDocumentSet; __block BOOL needsRefill = NO; @@ -242,57 +243,73 @@ - (FSTViewDocumentChanges *)computeChangesWithDocuments:(FSTMaybeDocumentDiction newDoc = nil; } } - if (newDoc) { - newDocumentSet = [newDocumentSet documentSetByAddingDocument:newDoc]; - if (newDoc.hasLocalMutations) { - newMutatedKeys = newMutatedKeys.insert(key); - } else { - newMutatedKeys = newMutatedKeys.erase(key); - } - } else { - newDocumentSet = [newDocumentSet documentSetByRemovingKey:key]; - newMutatedKeys = newMutatedKeys.erase(key); - } + BOOL oldDocHadPendingMutations = oldDoc && oldMutatedKeys.contains(oldDoc.key); + + // We only consider committed mutations for documents that were mutated during the lifetime of + // the view. + BOOL newDocHasPendingMutations = + newDoc && (newDoc.hasLocalMutations || + (oldMutatedKeys.contains(newDoc.key) && newDoc.hasCommittedMutations)); + + BOOL changeApplied = NO; // Calculate change if (oldDoc && newDoc) { BOOL docsEqual = [oldDoc.data isEqual:newDoc.data]; - if (!docsEqual || oldDoc.hasLocalMutations != newDoc.hasLocalMutations) { - // only report a change if document actually changed. - if (docsEqual) { - [changeSet addChange:[FSTDocumentViewChange - changeWithDocument:newDoc - type:FSTDocumentViewChangeTypeMetadata]]; - } else { + if (!docsEqual) { + if (![self shouldWaitForSyncedDocument:newDoc oldDocument:oldDoc]) { [changeSet addChange:[FSTDocumentViewChange changeWithDocument:newDoc type:FSTDocumentViewChangeTypeModified]]; - } + changeApplied = YES; - if (lastDocInLimit && self.query.comparator(newDoc, lastDocInLimit) > 0) { - // This doc moved from inside the limit to after the limit. That means there may be some - // doc in the local cache that's actually less than this one. - needsRefill = YES; + if (lastDocInLimit && self.query.comparator(newDoc, lastDocInLimit) > 0) { + // This doc moved from inside the limit to after the limit. That means there may be + // some doc in the local cache that's actually less than this one. + needsRefill = YES; + } } + } else if (oldDocHadPendingMutations != newDocHasPendingMutations) { + [changeSet + addChange:[FSTDocumentViewChange changeWithDocument:newDoc + type:FSTDocumentViewChangeTypeMetadata]]; + changeApplied = true; } + } else if (!oldDoc && newDoc) { [changeSet addChange:[FSTDocumentViewChange changeWithDocument:newDoc type:FSTDocumentViewChangeTypeAdded]]; + changeApplied = true; } else if (oldDoc && !newDoc) { [changeSet addChange:[FSTDocumentViewChange changeWithDocument:oldDoc type:FSTDocumentViewChangeTypeRemoved]]; + changeApplied = true; + if (lastDocInLimit) { // A doc was removed from a full limit query. We'll need to re-query from the local cache // to see if we know about some other doc that should be in the results. needsRefill = YES; } } + + if (changeApplied) { + if (newDoc) { + newDocumentSet = [newDocumentSet documentSetByAddingDocument:newDoc]; + if (newDoc.hasLocalMutations) { + newMutatedKeys = newMutatedKeys.insert(key); + } else { + newMutatedKeys = newMutatedKeys.erase(key); + } + } else { + newDocumentSet = [newDocumentSet documentSetByRemovingKey:key]; + newMutatedKeys = newMutatedKeys.erase(key); + } + } }]; if (self.query.limit) { - // TODO(klimt): Make DocumentSet size be constant time. - while (newDocumentSet.count > self.query.limit) { + for (long i = newDocumentSet.count - self.query.limit; i > 0; --i) { FSTDocument *oldDoc = [newDocumentSet lastDocument]; newDocumentSet = [newDocumentSet documentSetByRemovingKey:oldDoc.key]; newMutatedKeys = newMutatedKeys.erase(oldDoc.key); @@ -311,6 +328,16 @@ - (FSTViewDocumentChanges *)computeChangesWithDocuments:(FSTMaybeDocumentDiction mutatedKeys:newMutatedKeys]; } +- (BOOL)shouldWaitForSyncedDocument:(FSTDocument *)newDoc oldDocument:(FSTDocument *)oldDoc { + // We suppress the initial change event for documents that were modified as part of a write + // acknowledgment (e.g. when the value of a server transform is applied) as Watch will send us + // the same document again. By suppressing the event, we only raise two user visible events (one + // with `hasPendingWrites` and the final state of the document) instead of three (one with + // `hasPendingWrites`, the modified document with `hasPendingWrites` and the final state of the + // document). + return (oldDoc.hasLocalMutations && newDoc.hasCommittedMutations && !newDoc.hasLocalMutations); +} + - (FSTViewChange *)applyChangesToDocuments:(FSTViewDocumentChanges *)docChanges { return [self applyChangesToDocuments:docChanges targetChange:nil]; } @@ -350,7 +377,7 @@ - (FSTViewChange *)applyChangesToDocuments:(FSTViewDocumentChanges *)docChanges oldDocuments:oldDocuments documentChanges:changes fromCache:newSyncState == FSTSyncStateLocal - hasPendingWrites:!docChanges.mutatedKeys.empty() + mutatedKeys:docChanges.mutatedKeys syncStateChanged:syncStateChanged]; return [FSTViewChange changeWithSnapshot:snapshot limboChanges:limboChanges]; diff --git a/Firestore/Source/Core/FSTViewSnapshot.h b/Firestore/Source/Core/FSTViewSnapshot.h index 3db61083273..f516bd74430 100644 --- a/Firestore/Source/Core/FSTViewSnapshot.h +++ b/Firestore/Source/Core/FSTViewSnapshot.h @@ -16,6 +16,10 @@ #import +#include "Firestore/core/src/firebase/firestore/model/document_key_set.h" + +using firebase::firestore::model::DocumentKeySet; + @class FSTDocument; @class FSTQuery; @class FSTDocumentSet; @@ -86,7 +90,7 @@ typedef void (^FSTViewSnapshotHandler)(FSTViewSnapshot *_Nullable snapshot, oldDocuments:(FSTDocumentSet *)oldDocuments documentChanges:(NSArray *)documentChanges fromCache:(BOOL)fromCache - hasPendingWrites:(BOOL)hasPendingWrites + mutatedKeys:(DocumentKeySet)mutatedKeys syncStateChanged:(BOOL)syncStateChanged NS_DESIGNATED_INITIALIZER; - (instancetype)init NS_UNAVAILABLE; @@ -112,6 +116,9 @@ typedef void (^FSTViewSnapshotHandler)(FSTViewSnapshot *_Nullable snapshot, /** Whether the sync state changed as part of this snapshot. */ @property(nonatomic, assign, readonly) BOOL syncStateChanged; +/** The document in this snapshot that have unconfirmed writes. */ +@property(nonatomic, assign, readonly) DocumentKeySet mutatedKeys; + @end NS_ASSUME_NONNULL_END diff --git a/Firestore/Source/Core/FSTViewSnapshot.mm b/Firestore/Source/Core/FSTViewSnapshot.mm index ac5b376ba0b..086ad5e88d1 100644 --- a/Firestore/Source/Core/FSTViewSnapshot.mm +++ b/Firestore/Source/Core/FSTViewSnapshot.mm @@ -178,7 +178,7 @@ - (instancetype)initWithQuery:(FSTQuery *)query oldDocuments:(FSTDocumentSet *)oldDocuments documentChanges:(NSArray *)documentChanges fromCache:(BOOL)fromCache - hasPendingWrites:(BOOL)hasPendingWrites + mutatedKeys:(DocumentKeySet)mutatedKeys syncStateChanged:(BOOL)syncStateChanged { self = [super init]; if (self) { @@ -187,18 +187,22 @@ - (instancetype)initWithQuery:(FSTQuery *)query _oldDocuments = oldDocuments; _documentChanges = documentChanges; _fromCache = fromCache; - _hasPendingWrites = hasPendingWrites; + _mutatedKeys = mutatedKeys; _syncStateChanged = syncStateChanged; } return self; } +- (BOOL)hasPendingWrites { + return _mutatedKeys.size() != 0; +} + - (NSString *)description { return [NSString stringWithFormat: @"", + "fromCache:%@ mutatedKeys:%zd syncStateChanged:%@>", self.query, self.documents, self.oldDocuments, self.documentChanges, - (self.fromCache ? @"YES" : @"NO"), (self.hasPendingWrites ? @"YES" : @"NO"), + (self.fromCache ? @"YES" : @"NO"), self.mutatedKeys.size(), (self.syncStateChanged ? @"YES" : @"NO")]; } @@ -213,17 +217,20 @@ - (BOOL)isEqual:(id)object { return [self.query isEqual:other.query] && [self.documents isEqual:other.documents] && [self.oldDocuments isEqual:other.oldDocuments] && [self.documentChanges isEqualToArray:other.documentChanges] && - self.fromCache == other.fromCache && self.hasPendingWrites == other.hasPendingWrites && + self.fromCache == other.fromCache && self.mutatedKeys == other.mutatedKeys && self.syncStateChanged == other.syncStateChanged; } - (NSUInteger)hash { + // Note: We are omitting `mutatedKeys` from the hash, since we don't have a straightforward + // way to compute its hash value. Since `FSTViewSnapshot` is currently not stored in an + // NSDictionary, this has no side effects. + NSUInteger result = [self.query hash]; result = 31 * result + [self.documents hash]; result = 31 * result + [self.oldDocuments hash]; result = 31 * result + [self.documentChanges hash]; result = 31 * result + (self.fromCache ? 1231 : 1237); - result = 31 * result + (self.hasPendingWrites ? 1231 : 1237); result = 31 * result + (self.syncStateChanged ? 1231 : 1237); return result; } diff --git a/Firestore/Source/Local/FSTLocalDocumentsView.mm b/Firestore/Source/Local/FSTLocalDocumentsView.mm index 3c745461642..f25dc6142cd 100644 --- a/Firestore/Source/Local/FSTLocalDocumentsView.mm +++ b/Firestore/Source/Local/FSTLocalDocumentsView.mm @@ -135,12 +135,10 @@ - (FSTDocumentDictionary *)documentsMatchingCollectionQuery:(FSTQuery *)query { baseDocument:baseDoc localWriteTime:batch.localWriteTime]; - if (!mutatedDoc || [mutatedDoc isKindOfClass:[FSTDeletedDocument class]]) { - results = [results dictionaryByRemovingObjectForKey:key]; - } else if ([mutatedDoc isKindOfClass:[FSTDocument class]]) { + if ([mutatedDoc isKindOfClass:[FSTDocument class]]) { results = [results dictionaryBySettingObject:(FSTDocument *)mutatedDoc forKey:key]; } else { - HARD_FAIL("Unknown document: %s", mutatedDoc); + results = [results dictionaryByRemovingObjectForKey:key]; } } } diff --git a/Firestore/Source/Model/FSTDocument.mm b/Firestore/Source/Model/FSTDocument.mm index 3962207a991..88c37e99681 100644 --- a/Firestore/Source/Model/FSTDocument.mm +++ b/Firestore/Source/Model/FSTDocument.mm @@ -163,7 +163,6 @@ + (instancetype)documentWithKey:(DocumentKey)key return deletedDocument; } - - (BOOL)hasCommittedMutations { return _hasCommittedMutations; } @@ -188,7 +187,7 @@ - (BOOL)isEqual:(id)other { - (NSUInteger)hash { NSUInteger result = [self.key hash]; result = result * 31 + self.version.Hash(); - result = result * 31 + _hasCommittedMutations ? 1 : 0; + result = result * 31 + (_hasCommittedMutations ? 1 : 0); return result; } From 7ec18e031643ef281747a8b35365fdfbc941a7e7 Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Thu, 25 Oct 2018 15:45:35 -0700 Subject: [PATCH 2/3] Held Write Acks Unit Test Fixes --- .../Example/Tests/Local/FSTLocalStoreTests.mm | 569 +++++++++--------- Firestore/Source/API/FIRDocumentSnapshot.mm | 3 +- Firestore/Source/Local/FSTLocalStore.mm | 2 +- 3 files changed, 289 insertions(+), 285 deletions(-) diff --git a/Firestore/Example/Tests/Local/FSTLocalStoreTests.mm b/Firestore/Example/Tests/Local/FSTLocalStoreTests.mm index 99267b4bc07..e5399fb8e31 100644 --- a/Firestore/Example/Tests/Local/FSTLocalStoreTests.mm +++ b/Firestore/Example/Tests/Local/FSTLocalStoreTests.mm @@ -256,83 +256,83 @@ - (void)testHandlesSetMutationThenDocument { FSTAssertContains(FSTTestDoc("foo/bar", 2, @{@"foo" : @"bar"}, FSTDocumentStateLocalMutations)); } -// TODO(heldwriteacks): Uncomment this test after LocalStore changes -//- (void)testHandlesAckThenRejectThenRemoteEvent { -// if ([self isTestBaseClass]) return; -// -// // Start a query that requires acks to be held. -// FSTQuery *query = FSTTestQuery("foo"); -// TargetId targetID = [self allocateQuery:query]; -// -// [self writeMutation:FSTTestSetMutation(@"foo/bar", @{@"foo" : @"bar"})]; -// FSTAssertChanged( -// @[ FSTTestDoc("foo/bar", 0, @{@"foo" : @"bar"}, FSTDocumentStateLocalMutations) ]); -// FSTAssertContains(FSTTestDoc("foo/bar", 0, @{@"foo" : @"bar"}, FSTDocumentStateLocalMutations)); -// -// // The last seen version is zero, so this ack must be held. -// [self acknowledgeMutationWithVersion:1]; -// FSTAssertChanged(@[]); -// -// // Under eager GC, there is no longer a reference for the document, and it should be -// // deleted. -// if ([self gcIsEager]) { -// FSTAssertNotContains(@"foo/bar"); -// } else { -// FSTAssertContains(FSTTestDoc("foo/bar", 1, @{@"foo" : @"bar"}, -// FSTDocumentStateCommittedMutations)); -// } -// -// [self writeMutation:FSTTestSetMutation(@"bar/baz", @{@"bar" : @"baz"})]; -// FSTAssertChanged( -// @[ FSTTestDoc("bar/baz", 0, @{@"bar" : @"baz"}, FSTDocumentStateLocalMutations) ]); -// FSTAssertContains(FSTTestDoc("bar/baz", 0, @{@"bar" : @"baz"}, FSTDocumentStateLocalMutations)); -// -// [self rejectMutation]; -// FSTAssertRemoved(@[ @"bar/baz" ]); -// FSTAssertNotContains(@"bar/baz"); -// -// [self applyRemoteEvent:FSTTestAddedRemoteEvent(FSTTestDoc("foo/bar", 2, @{@"it" : @"changed"}, -// FSTDocumentStateSynced), -// @[ @(targetID) ])]; -// FSTAssertChanged(@[ FSTTestDoc("foo/bar", 2, @{@"it" : @"changed"}, FSTDocumentStateSynced) ]); -// FSTAssertContains(FSTTestDoc("foo/bar", 2, @{@"it" : @"changed"}, FSTDocumentStateSynced)); -// FSTAssertNotContains(@"bar/baz"); -//} - -// TODO(heldwriteacks): Uncomment this test after LocalStore changes -//- (void)testHandlesDeletedDocumentThenSetMutationThenAck { -// if ([self isTestBaseClass]) return; -// -// FSTQuery *query = FSTTestQuery("foo"); -// TargetId targetID = [self allocateQuery:query]; -// -// [self applyRemoteEvent:FSTTestUpdateRemoteEvent(FSTTestDeletedDoc("foo/bar", 2, NO), -// @[ @(targetID) ], @[])]; -// FSTAssertRemoved(@[ @"foo/bar" ]); -// // Under eager GC, there is no longer a reference for the document, and it should be -// // deleted. -// if (![self gcIsEager]) { -// FSTAssertContains(FSTTestDeletedDoc("foo/bar", 2, NO)); -// } else { -// FSTAssertNotContains(@"foo/bar"); -// } -// -// [self writeMutation:FSTTestSetMutation(@"foo/bar", @{@"foo" : @"bar"})]; -// FSTAssertChanged( -// @[ FSTTestDoc("foo/bar", 0, @{@"foo" : @"bar"}, FSTDocumentStateLocalMutations) ]); -// FSTAssertContains(FSTTestDoc("foo/bar", 0, @{@"foo" : @"bar"}, FSTDocumentStateLocalMutations)); -// // Can now remove the target, since we have a mutation pinning the document -// [self.localStore releaseQuery:query]; -// // Verify we didn't lose anything -// FSTAssertContains(FSTTestDoc("foo/bar", 0, @{@"foo" : @"bar"}, FSTDocumentStateLocalMutations)); -// -// [self acknowledgeMutationWithVersion:3]; -// FSTAssertChanged(@[ FSTTestDoc("foo/bar", 0, @{@"foo" : @"bar"}, FSTDocumentStateSynced) ]); -// // It has been acknowledged, and should no longer be retained as there is no target and mutation -// if ([self gcIsEager]) { -// FSTAssertNotContains(@"foo/bar"); -// } -//} +- (void)testHandlesAckThenRejectThenRemoteEvent { + if ([self isTestBaseClass]) return; + + // Start a query that requires acks to be held. + FSTQuery *query = FSTTestQuery("foo"); + TargetId targetID = [self allocateQuery:query]; + + [self writeMutation:FSTTestSetMutation(@"foo/bar", @{@"foo" : @"bar"})]; + FSTAssertChanged( + @[ FSTTestDoc("foo/bar", 0, @{@"foo" : @"bar"}, FSTDocumentStateLocalMutations) ]); + FSTAssertContains(FSTTestDoc("foo/bar", 0, @{@"foo" : @"bar"}, FSTDocumentStateLocalMutations)); + + // The last seen version is zero, so this ack must be held. + [self acknowledgeMutationWithVersion:1]; + FSTAssertChanged( + @[ FSTTestDoc("foo/bar", 1, @{@"foo" : @"bar"}, FSTDocumentStateCommittedMutations) ]); + + // Under eager GC, there is no longer a reference for the document, and it should be + // deleted. + if ([self gcIsEager]) { + FSTAssertNotContains(@"foo/bar"); + } else { + FSTAssertContains( + FSTTestDoc("foo/bar", 1, @{@"foo" : @"bar"}, FSTDocumentStateCommittedMutations)); + } + + [self writeMutation:FSTTestSetMutation(@"bar/baz", @{@"bar" : @"baz"})]; + FSTAssertChanged( + @[ FSTTestDoc("bar/baz", 0, @{@"bar" : @"baz"}, FSTDocumentStateLocalMutations) ]); + FSTAssertContains(FSTTestDoc("bar/baz", 0, @{@"bar" : @"baz"}, FSTDocumentStateLocalMutations)); + + [self rejectMutation]; + FSTAssertRemoved(@[ @"bar/baz" ]); + FSTAssertNotContains(@"bar/baz"); + + [self applyRemoteEvent:FSTTestAddedRemoteEvent(FSTTestDoc("foo/bar", 2, @{@"it" : @"changed"}, + FSTDocumentStateSynced), + @[ @(targetID) ])]; + FSTAssertChanged(@[ FSTTestDoc("foo/bar", 2, @{@"it" : @"changed"}, FSTDocumentStateSynced) ]); + FSTAssertContains(FSTTestDoc("foo/bar", 2, @{@"it" : @"changed"}, FSTDocumentStateSynced)); + FSTAssertNotContains(@"bar/baz"); +} + +- (void)testHandlesDeletedDocumentThenSetMutationThenAck { + if ([self isTestBaseClass]) return; + + FSTQuery *query = FSTTestQuery("foo"); + TargetId targetID = [self allocateQuery:query]; + + [self applyRemoteEvent:FSTTestUpdateRemoteEvent(FSTTestDeletedDoc("foo/bar", 2, NO), + @[ @(targetID) ], @[])]; + FSTAssertRemoved(@[ @"foo/bar" ]); + // Under eager GC, there is no longer a reference for the document, and it should be + // deleted. + if (![self gcIsEager]) { + FSTAssertContains(FSTTestDeletedDoc("foo/bar", 2, NO)); + } else { + FSTAssertNotContains(@"foo/bar"); + } + + [self writeMutation:FSTTestSetMutation(@"foo/bar", @{@"foo" : @"bar"})]; + FSTAssertChanged( + @[ FSTTestDoc("foo/bar", 0, @{@"foo" : @"bar"}, FSTDocumentStateLocalMutations) ]); + FSTAssertContains(FSTTestDoc("foo/bar", 0, @{@"foo" : @"bar"}, FSTDocumentStateLocalMutations)); + // Can now remove the target, since we have a mutation pinning the document + [self.localStore releaseQuery:query]; + // Verify we didn't lose anything + FSTAssertContains(FSTTestDoc("foo/bar", 0, @{@"foo" : @"bar"}, FSTDocumentStateLocalMutations)); + + [self acknowledgeMutationWithVersion:3]; + FSTAssertChanged( + @[ FSTTestDoc("foo/bar", 3, @{@"foo" : @"bar"}, FSTDocumentStateCommittedMutations) ]); + // It has been acknowledged, and should no longer be retained as there is no target and mutation + if ([self gcIsEager]) { + FSTAssertNotContains(@"foo/bar"); + } +} - (void)testHandlesSetMutationThenDeletedDocument { if ([self isTestBaseClass]) return; @@ -351,37 +351,37 @@ - (void)testHandlesSetMutationThenDeletedDocument { FSTAssertContains(FSTTestDoc("foo/bar", 0, @{@"foo" : @"bar"}, FSTDocumentStateLocalMutations)); } -// TODO(heldwriteacks): Uncomment this test after LocalStore changes -//- (void)testHandlesDocumentThenSetMutationThenAckThenDocument { -// if ([self isTestBaseClass]) return; -// -// // Start a query that requires acks to be held. -// FSTQuery *query = FSTTestQuery("foo"); -// TargetId targetID = [self allocateQuery:query]; -// -// [self applyRemoteEvent:FSTTestAddedRemoteEvent( -// FSTTestDoc("foo/bar", 2, @{@"it" : @"base"}, FSTDocumentStateSynced), -// @[ @(targetID) ])]; -// FSTAssertChanged(@[ FSTTestDoc("foo/bar", 2, @{@"it" : @"base"}, FSTDocumentStateSynced) ]); -// FSTAssertContains(FSTTestDoc("foo/bar", 2, @{@"it" : @"base"}, FSTDocumentStateSynced)); -// -// [self writeMutation:FSTTestSetMutation(@"foo/bar", @{@"foo" : @"bar"})]; -// FSTAssertChanged( -// @[ FSTTestDoc("foo/bar", 2, @{@"foo" : @"bar"}, FSTDocumentStateLocalMutations) ]); -// FSTAssertContains(FSTTestDoc("foo/bar", 2, @{@"foo" : @"bar"}, FSTDocumentStateLocalMutations)); -// -// [self acknowledgeMutationWithVersion:3]; -// // we haven't seen the remote event yet, so the write is still held. -// FSTAssertChanged(@[]); -// FSTAssertContains( -// FSTTestDoc("foo/bar", 2, @{@"foo" : @"bar"}, FSTDocumentStateCommittedMutations)); -// -// [self applyRemoteEvent:FSTTestUpdateRemoteEvent(FSTTestDoc("foo/bar", 3, @{@"it" : @"changed"}, -// FSTDocumentStateSynced), -// @[ @(targetID) ], @[])]; -// FSTAssertChanged(@[ FSTTestDoc("foo/bar", 3, @{@"it" : @"changed"}, FSTDocumentStateSynced) ]); -// FSTAssertContains(FSTTestDoc("foo/bar", 3, @{@"it" : @"changed"}, FSTDocumentStateSynced)); -//} +- (void)testHandlesDocumentThenSetMutationThenAckThenDocument { + if ([self isTestBaseClass]) return; + + // Start a query that requires acks to be held. + FSTQuery *query = FSTTestQuery("foo"); + TargetId targetID = [self allocateQuery:query]; + + [self applyRemoteEvent:FSTTestAddedRemoteEvent( + FSTTestDoc("foo/bar", 2, @{@"it" : @"base"}, FSTDocumentStateSynced), + @[ @(targetID) ])]; + FSTAssertChanged(@[ FSTTestDoc("foo/bar", 2, @{@"it" : @"base"}, FSTDocumentStateSynced) ]); + FSTAssertContains(FSTTestDoc("foo/bar", 2, @{@"it" : @"base"}, FSTDocumentStateSynced)); + + [self writeMutation:FSTTestSetMutation(@"foo/bar", @{@"foo" : @"bar"})]; + FSTAssertChanged( + @[ FSTTestDoc("foo/bar", 2, @{@"foo" : @"bar"}, FSTDocumentStateLocalMutations) ]); + FSTAssertContains(FSTTestDoc("foo/bar", 2, @{@"foo" : @"bar"}, FSTDocumentStateLocalMutations)); + + [self acknowledgeMutationWithVersion:3]; + // we haven't seen the remote event yet, so the write is still held. + FSTAssertChanged( + @[ FSTTestDoc("foo/bar", 3, @{@"foo" : @"bar"}, FSTDocumentStateCommittedMutations) ]); + FSTAssertContains( + FSTTestDoc("foo/bar", 3, @{@"foo" : @"bar"}, FSTDocumentStateCommittedMutations)); + + [self applyRemoteEvent:FSTTestUpdateRemoteEvent(FSTTestDoc("foo/bar", 3, @{@"it" : @"changed"}, + FSTDocumentStateSynced), + @[ @(targetID) ], @[])]; + FSTAssertChanged(@[ FSTTestDoc("foo/bar", 3, @{@"it" : @"changed"}, FSTDocumentStateSynced) ]); + FSTAssertContains(FSTTestDoc("foo/bar", 3, @{@"it" : @"changed"}, FSTDocumentStateSynced)); +} - (void)testHandlesPatchWithoutPriorDocument { if ([self isTestBaseClass]) return; @@ -399,65 +399,69 @@ - (void)testHandlesPatchWithoutPriorDocument { } } -// TODO(heldwriteacks): Uncomment this test after LocalStore changes -//- (void)testHandlesPatchMutationThenDocumentThenAck { -// if ([self isTestBaseClass]) return; -// -// [self writeMutation:FSTTestPatchMutation("foo/bar", @{@"foo" : @"bar"}, {})]; -// FSTAssertRemoved(@[ @"foo/bar" ]); -// FSTAssertNotContains(@"foo/bar"); -// -// FSTQuery *query = FSTTestQuery("foo"); -// TargetId targetID = [self allocateQuery:query]; -// -// [self applyRemoteEvent:FSTTestAddedRemoteEvent( -// FSTTestDoc("foo/bar", 1, @{@"it" : @"base"}, FSTDocumentStateSynced), -// @[ @(targetID) ])]; -// FSTAssertChanged(@[ FSTTestDoc("foo/bar", 1, @{@"foo" : @"bar", @"it" : @"base"}, -// FSTDocumentStateLocalMutations) ]); -// FSTAssertContains(FSTTestDoc("foo/bar", 1, @{@"foo" : @"bar", @"it" : @"base"}, -// FSTDocumentStateLocalMutations)); -// -// [self acknowledgeMutationWithVersion:2]; -// // We still haven't seen the remote events for the patch, so the local changes remain, and there -// // are no changes -// FSTAssertChanged(@[FSTTestDoc("foo/bar", 2, @{@"foo" : @"bar", @"it" : @"base"}, -// FSTDocumentStateCommittedMutations)]); -// FSTAssertContains(FSTTestDoc("foo/bar", 2, @{@"foo" : @"bar", @"it" : @"base"}, -// FSTDocumentStateCommittedMutations)); -// -// [self applyRemoteEvent:FSTTestUpdateRemoteEvent( -// FSTTestDoc("foo/bar", 2, @{@"foo" : @"bar", @"it" : @"base"}, -// FSTDocumentStateSynced), -// @[], @[])]; -// -// FSTAssertChanged( -// @[ FSTTestDoc("foo/bar", 2, @{@"foo" : @"bar", @"it" : @"base"}, FSTDocumentStateSynced) ]); -// FSTAssertContains( -// FSTTestDoc("foo/bar", 2, @{@"foo" : @"bar", @"it" : @"base"}, FSTDocumentStateSynced)); -//} - -// TODO(heldwriteacks): Uncomment this test after LocalStore changes -//- (void)testHandlesPatchMutationThenAckThenDocument { -// if ([self isTestBaseClass]) return; -// -// [self writeMutation:FSTTestPatchMutation("foo/bar", @{@"foo" : @"bar"}, {})]; -// FSTAssertRemoved(@[ @"foo/bar" ]); -// FSTAssertNotContains(@"foo/bar"); -// -// [self acknowledgeMutationWithVersion:1]; -// FSTAssertRemoved(@[ @"foo/bar" ]); -// FSTAssertNotContains(@"foo/bar"); -// -// FSTQuery *query = FSTTestQuery("foo"); -// TargetId targetID = [self allocateQuery:query]; -// -// [self applyRemoteEvent:FSTTestUpdateRemoteEvent( -// FSTTestDoc("foo/bar", 1, @{@"it" : @"base"}, FSTDocumentStateSynced), -// @[ @(targetID) ], @[])]; -// FSTAssertChanged(@[ FSTTestDoc("foo/bar", 1, @{@"it" : @"base"}, FSTDocumentStateSynced) ]); -// FSTAssertContains(FSTTestDoc("foo/bar", 1, @{@"it" : @"base"}, FSTDocumentStateSynced)); -//} +- (void)testHandlesPatchMutationThenDocumentThenAck { + if ([self isTestBaseClass]) return; + + [self writeMutation:FSTTestPatchMutation("foo/bar", @{@"foo" : @"bar"}, {})]; + FSTAssertRemoved(@[ @"foo/bar" ]); + FSTAssertNotContains(@"foo/bar"); + + FSTQuery *query = FSTTestQuery("foo"); + TargetId targetID = [self allocateQuery:query]; + + [self applyRemoteEvent:FSTTestAddedRemoteEvent( + FSTTestDoc("foo/bar", 1, @{@"it" : @"base"}, FSTDocumentStateSynced), + @[ @(targetID) ])]; + FSTAssertChanged(@[ FSTTestDoc("foo/bar", 1, @{@"foo" : @"bar", @"it" : @"base"}, + FSTDocumentStateLocalMutations) ]); + FSTAssertContains(FSTTestDoc("foo/bar", 1, @{@"foo" : @"bar", @"it" : @"base"}, + FSTDocumentStateLocalMutations)); + + [self acknowledgeMutationWithVersion:2]; + // We still haven't seen the remote events for the patch, so the local changes remain, and there + // are no changes + FSTAssertChanged(@[ FSTTestDoc("foo/bar", 2, @{@"foo" : @"bar", @"it" : @"base"}, + FSTDocumentStateCommittedMutations) ]); + FSTAssertContains(FSTTestDoc("foo/bar", 2, @{@"foo" : @"bar", @"it" : @"base"}, + FSTDocumentStateCommittedMutations)); + + [self applyRemoteEvent:FSTTestUpdateRemoteEvent( + FSTTestDoc("foo/bar", 2, @{@"foo" : @"bar", @"it" : @"base"}, + FSTDocumentStateSynced), + @[ @(targetID) ], @[])]; + + FSTAssertChanged( + @[ FSTTestDoc("foo/bar", 2, @{@"foo" : @"bar", @"it" : @"base"}, FSTDocumentStateSynced) ]); + FSTAssertContains( + FSTTestDoc("foo/bar", 2, @{@"foo" : @"bar", @"it" : @"base"}, FSTDocumentStateSynced)); +} + +- (void)testHandlesPatchMutationThenAckThenDocument { + if ([self isTestBaseClass]) return; + + [self writeMutation:FSTTestPatchMutation("foo/bar", @{@"foo" : @"bar"}, {})]; + FSTAssertRemoved(@[ @"foo/bar" ]); + FSTAssertNotContains(@"foo/bar"); + + [self acknowledgeMutationWithVersion:1]; + FSTAssertChanged(@[ FSTTestUnknownDoc("foo/bar", 1) ]); + + // There's no target pinning the doc, and we've ack'd the mutation. + if ([self gcIsEager]) { + FSTAssertNotContains(@"foo/bar"); + } else { + FSTAssertContains(FSTTestUnknownDoc("foo/bar", 1)); + } + + FSTQuery *query = FSTTestQuery("foo"); + TargetId targetID = [self allocateQuery:query]; + + [self applyRemoteEvent:FSTTestUpdateRemoteEvent( + FSTTestDoc("foo/bar", 1, @{@"it" : @"base"}, FSTDocumentStateSynced), + @[ @(targetID) ], @[])]; + FSTAssertChanged(@[ FSTTestDoc("foo/bar", 1, @{@"it" : @"base"}, FSTDocumentStateSynced) ]); + FSTAssertContains(FSTTestDoc("foo/bar", 1, @{@"it" : @"base"}, FSTDocumentStateSynced)); +} - (void)testHandlesDeleteMutationThenAck { if ([self isTestBaseClass]) return; @@ -474,33 +478,32 @@ - (void)testHandlesDeleteMutationThenAck { } } -// TODO(heldwriteacks): Uncomment this test after LocalStore changes -//- (void)testHandlesDocumentThenDeleteMutationThenAck { -// if ([self isTestBaseClass]) return; -// -// FSTQuery *query = FSTTestQuery("foo"); -// TargetId targetID = [self allocateQuery:query]; -// -// [self applyRemoteEvent:FSTTestUpdateRemoteEvent( -// FSTTestDoc("foo/bar", 1, @{@"it" : @"base"}, FSTDocumentStateSynced), -// @[ @(targetID) ], @[])]; -// FSTAssertChanged(@[ FSTTestDoc("foo/bar", 1, @{@"it" : @"base"}, FSTDocumentStateSynced) ]); -// FSTAssertContains(FSTTestDoc("foo/bar", 1, @{@"it" : @"base"}, FSTDocumentStateSynced)); -// -// [self writeMutation:FSTTestDeleteMutation(@"foo/bar")]; -// FSTAssertRemoved(@[ @"foo/bar" ]); -// FSTAssertContains(FSTTestDeletedDoc("foo/bar", 0, YES)); -// -// // Remove the target so only the mutation is pinning the document -// [self.localStore releaseQuery:query]; -// -// [self acknowledgeMutationWithVersion:2]; -// FSTAssertRemoved(@[ @"foo/bar" ]); -// if ([self gcIsEager]) { -// // Neither the target nor the mutation pin the document, it should be gone. -// FSTAssertNotContains(@"foo/bar"); -// } -//} +- (void)testHandlesDocumentThenDeleteMutationThenAck { + if ([self isTestBaseClass]) return; + + FSTQuery *query = FSTTestQuery("foo"); + TargetId targetID = [self allocateQuery:query]; + + [self applyRemoteEvent:FSTTestUpdateRemoteEvent( + FSTTestDoc("foo/bar", 1, @{@"it" : @"base"}, FSTDocumentStateSynced), + @[ @(targetID) ], @[])]; + FSTAssertChanged(@[ FSTTestDoc("foo/bar", 1, @{@"it" : @"base"}, FSTDocumentStateSynced) ]); + FSTAssertContains(FSTTestDoc("foo/bar", 1, @{@"it" : @"base"}, FSTDocumentStateSynced)); + + [self writeMutation:FSTTestDeleteMutation(@"foo/bar")]; + FSTAssertRemoved(@[ @"foo/bar" ]); + FSTAssertContains(FSTTestDeletedDoc("foo/bar", 0, NO)); + + // Remove the target so only the mutation is pinning the document + [self.localStore releaseQuery:query]; + + [self acknowledgeMutationWithVersion:2]; + FSTAssertRemoved(@[ @"foo/bar" ]); + if ([self gcIsEager]) { + // Neither the target nor the mutation pin the document, it should be gone. + FSTAssertNotContains(@"foo/bar"); + } +} - (void)testHandlesDeleteMutationThenDocumentThenAck { if ([self isTestBaseClass]) return; @@ -531,71 +534,72 @@ - (void)testHandlesDeleteMutationThenDocumentThenAck { } } -// TODO(heldwriteacks): Uncomment this test after LocalStore changes -//- (void)testHandlesDocumentThenDeletedDocumentThenDocument { -// if ([self isTestBaseClass]) return; -// -// FSTQuery *query = FSTTestQuery("foo"); -// TargetId targetID = [self allocateQuery:query]; -// -// [self applyRemoteEvent:FSTTestUpdateRemoteEvent( -// FSTTestDoc("foo/bar", 1, @{@"it" : @"base"}, FSTDocumentStateSynced), -// @[ @(targetID) ], @[])]; -// FSTAssertChanged(@[ FSTTestDoc("foo/bar", 1, @{@"it" : @"base"}, FSTDocumentStateSynced) ]); -// FSTAssertContains(FSTTestDoc("foo/bar", 1, @{@"it" : @"base"}, FSTDocumentStateSynced)); -// -// [self applyRemoteEvent:FSTTestUpdateRemoteEvent(FSTTestDeletedDoc("foo/bar", 2, NO), -// @[ @(targetID) ], @[])]; -// FSTAssertRemoved(@[ @"foo/bar" ]); -// if (![self gcIsEager]) { -// FSTAssertContains(FSTTestDeletedDoc("foo/bar", 2, NO)); -// } -// -// [self applyRemoteEvent:FSTTestUpdateRemoteEvent(FSTTestDoc("foo/bar", 3, @{@"it" : @"changed"}, -// FSTDocumentStateSynced), -// @[ @(targetID) ], @[])]; -// FSTAssertChanged(@[ FSTTestDoc("foo/bar", 3, @{@"it" : @"changed"}, FSTDocumentStateSynced) ]); -// FSTAssertContains(FSTTestDoc("foo/bar", 3, @{@"it" : @"changed"}, FSTDocumentStateSynced)); -//} - -// TODO(heldwriteacks): Uncomment this test after LocalStore changes -//- (void)testHandlesSetMutationThenPatchMutationThenDocumentThenAckThenAck { -// if ([self isTestBaseClass]) return; -// -// [self writeMutation:FSTTestSetMutation(@"foo/bar", @{@"foo" : @"old"})]; -// FSTAssertChanged( -// @[ FSTTestDoc("foo/bar", 0, @{@"foo" : @"old"}, FSTDocumentStateLocalMutations) ]); -// FSTAssertContains(FSTTestDoc("foo/bar", 0, @{@"foo" : @"old"}, FSTDocumentStateLocalMutations)); -// -// [self writeMutation:FSTTestPatchMutation("foo/bar", @{@"foo" : @"bar"}, {})]; -// FSTAssertChanged( -// @[ FSTTestDoc("foo/bar", 0, @{@"foo" : @"bar"}, FSTDocumentStateLocalMutations) ]); -// FSTAssertContains(FSTTestDoc("foo/bar", 0, @{@"foo" : @"bar"}, FSTDocumentStateLocalMutations)); -// -// FSTQuery *query = FSTTestQuery("foo"); -// TargetId targetID = [self allocateQuery:query]; -// -// [self applyRemoteEvent:FSTTestUpdateRemoteEvent( -// FSTTestDoc("foo/bar", 1, @{@"it" : @"base"}, FSTDocumentStateSynced), -// @[ @(targetID) ], @[])]; -// FSTAssertChanged( -// @[ FSTTestDoc("foo/bar", 1, @{@"foo" : @"bar"}, FSTDocumentStateLocalMutations) ]); -// FSTAssertContains(FSTTestDoc("foo/bar", 1, @{@"foo" : @"bar"}, FSTDocumentStateLocalMutations)); -// -// [self.localStore releaseQuery:query]; -// [self acknowledgeMutationWithVersion:2]; // delete mutation -// FSTAssertChanged( -// @[ FSTTestDoc("foo/bar", 1, @{@"foo" : @"bar"}, FSTDocumentStateCommittedMutations) ]); -// FSTAssertContains( -// FSTTestDoc("foo/bar", 1, @{@"foo" : @"bar"}, FSTDocumentStateCommittedMutations)); -// -// [self acknowledgeMutationWithVersion:3]; // patch mutation -// FSTAssertChanged(@[ FSTTestDoc("foo/bar", 1, @{@"foo" : @"bar"}, FSTDocumentStateSynced) ]); -// if ([self gcIsEager]) { -// // we've ack'd all of the mutations, nothing is keeping this pinned anymore -// FSTAssertNotContains(@"foo/bar"); -// } -//} +- (void)testHandlesDocumentThenDeletedDocumentThenDocument { + if ([self isTestBaseClass]) return; + + FSTQuery *query = FSTTestQuery("foo"); + TargetId targetID = [self allocateQuery:query]; + + [self applyRemoteEvent:FSTTestUpdateRemoteEvent( + FSTTestDoc("foo/bar", 1, @{@"it" : @"base"}, FSTDocumentStateSynced), + @[ @(targetID) ], @[])]; + FSTAssertChanged(@[ FSTTestDoc("foo/bar", 1, @{@"it" : @"base"}, FSTDocumentStateSynced) ]); + FSTAssertContains(FSTTestDoc("foo/bar", 1, @{@"it" : @"base"}, FSTDocumentStateSynced)); + + [self applyRemoteEvent:FSTTestUpdateRemoteEvent(FSTTestDeletedDoc("foo/bar", 2, NO), + @[ @(targetID) ], @[])]; + FSTAssertRemoved(@[ @"foo/bar" ]); + if (![self gcIsEager]) { + FSTAssertContains(FSTTestDeletedDoc("foo/bar", 2, NO)); + } + + [self applyRemoteEvent:FSTTestUpdateRemoteEvent(FSTTestDoc("foo/bar", 3, @{@"it" : @"changed"}, + FSTDocumentStateSynced), + @[ @(targetID) ], @[])]; + FSTAssertChanged(@[ FSTTestDoc("foo/bar", 3, @{@"it" : @"changed"}, FSTDocumentStateSynced) ]); + FSTAssertContains(FSTTestDoc("foo/bar", 3, @{@"it" : @"changed"}, FSTDocumentStateSynced)); +} + +- (void)testHandlesSetMutationThenPatchMutationThenDocumentThenAckThenAck { + if ([self isTestBaseClass]) return; + + [self writeMutation:FSTTestSetMutation(@"foo/bar", @{@"foo" : @"old"})]; + FSTAssertChanged( + @[ FSTTestDoc("foo/bar", 0, @{@"foo" : @"old"}, FSTDocumentStateLocalMutations) ]); + FSTAssertContains(FSTTestDoc("foo/bar", 0, @{@"foo" : @"old"}, FSTDocumentStateLocalMutations)); + + [self writeMutation:FSTTestPatchMutation("foo/bar", @{@"foo" : @"bar"}, {})]; + FSTAssertChanged( + @[ FSTTestDoc("foo/bar", 0, @{@"foo" : @"bar"}, FSTDocumentStateLocalMutations) ]); + FSTAssertContains(FSTTestDoc("foo/bar", 0, @{@"foo" : @"bar"}, FSTDocumentStateLocalMutations)); + + FSTQuery *query = FSTTestQuery("foo"); + TargetId targetID = [self allocateQuery:query]; + + [self applyRemoteEvent:FSTTestUpdateRemoteEvent( + FSTTestDoc("foo/bar", 1, @{@"it" : @"base"}, FSTDocumentStateSynced), + @[ @(targetID) ], @[])]; + FSTAssertChanged( + @[ FSTTestDoc("foo/bar", 1, @{@"foo" : @"bar"}, FSTDocumentStateLocalMutations) ]); + FSTAssertContains(FSTTestDoc("foo/bar", 1, @{@"foo" : @"bar"}, FSTDocumentStateLocalMutations)); + + [self.localStore releaseQuery:query]; + [self acknowledgeMutationWithVersion:2]; // delete mutation + FSTAssertChanged( + @[ FSTTestDoc("foo/bar", 2, @{@"foo" : @"bar"}, FSTDocumentStateLocalMutations) ]); + FSTAssertContains(FSTTestDoc("foo/bar", 2, @{@"foo" : @"bar"}, FSTDocumentStateLocalMutations)); + + [self acknowledgeMutationWithVersion:3]; // patch mutation + FSTAssertChanged( + @[ FSTTestDoc("foo/bar", 3, @{@"foo" : @"bar"}, FSTDocumentStateCommittedMutations) ]); + if ([self gcIsEager]) { + // we've ack'd all of the mutations, nothing is keeping this pinned anymore + FSTAssertNotContains(@"foo/bar"); + } else { + FSTAssertContains( + FSTTestDoc("foo/bar", 3, @{@"foo" : @"bar"}, FSTDocumentStateCommittedMutations)); + } +} - (void)testHandlesSetMutationAndPatchMutationTogether { if ([self isTestBaseClass]) return; @@ -644,29 +648,30 @@ - (void)testHandlesSetMutationsAndPatchMutationOfJustOneTogether { FSTAssertContains(FSTTestDoc("bar/baz", 0, @{@"bar" : @"baz"}, FSTDocumentStateLocalMutations)); } -// TODO(heldwriteacks): Uncomment this test after LocalStore changes -//- (void)testHandlesDeleteMutationThenPatchMutationThenAckThenAck { -// if ([self isTestBaseClass]) return; -// -// [self writeMutation:FSTTestDeleteMutation(@"foo/bar")]; -// FSTAssertRemoved(@[ @"foo/bar" ]); -// FSTAssertContains(FSTTestDeletedDoc("foo/bar", 0, NO)); -// -// [self writeMutation:FSTTestPatchMutation("foo/bar", @{@"foo" : @"bar"}, {})]; -// FSTAssertRemoved(@[ @"foo/bar" ]); -// FSTAssertContains(FSTTestDeletedDoc("foo/bar", 0, NO)); -// -// [self acknowledgeMutationWithVersion:2]; // delete mutation -// FSTAssertRemoved(@[ @"foo/bar" ]); -// FSTAssertContains(FSTTestDeletedDoc("foo/bar", 0, YES)); -// -// [self acknowledgeMutationWithVersion:3]; // patch mutation -// FSTAssertRemoved(@[ @"foo/bar" ]); -// if ([self gcIsEager]) { -// // There are no more pending mutations, the doc has been dropped -// FSTAssertNotContains(@"foo/bar"); -// } -//} +- (void)testHandlesDeleteMutationThenPatchMutationThenAckThenAck { + if ([self isTestBaseClass]) return; + + [self writeMutation:FSTTestDeleteMutation(@"foo/bar")]; + FSTAssertRemoved(@[ @"foo/bar" ]); + FSTAssertContains(FSTTestDeletedDoc("foo/bar", 0, NO)); + + [self writeMutation:FSTTestPatchMutation("foo/bar", @{@"foo" : @"bar"}, {})]; + FSTAssertRemoved(@[ @"foo/bar" ]); + FSTAssertContains(FSTTestDeletedDoc("foo/bar", 0, NO)); + + [self acknowledgeMutationWithVersion:2]; // delete mutation + FSTAssertRemoved(@[ @"foo/bar" ]); + FSTAssertContains(FSTTestDeletedDoc("foo/bar", 2, YES)); + + [self acknowledgeMutationWithVersion:3]; // patch mutation + FSTAssertChanged(@[ FSTTestUnknownDoc("foo/bar", 3) ]); + if ([self gcIsEager]) { + // There are no more pending mutations, the doc has been dropped + FSTAssertNotContains(@"foo/bar"); + } else { + FSTAssertContains(FSTTestUnknownDoc("foo/bar", 3)); + } +} - (void)testCollectsGarbageAfterChangeBatchWithNoTargetIDs { if ([self isTestBaseClass]) return; diff --git a/Firestore/Source/API/FIRDocumentSnapshot.mm b/Firestore/Source/API/FIRDocumentSnapshot.mm index 6696d552b4c..d38644ce77b 100644 --- a/Firestore/Source/API/FIRDocumentSnapshot.mm +++ b/Firestore/Source/API/FIRDocumentSnapshot.mm @@ -129,8 +129,7 @@ - (BOOL)isEqualToSnapshot:(nullable FIRDocumentSnapshot *)snapshot { return [self.firestore isEqual:snapshot.firestore] && self.internalKey == snapshot.internalKey && (self.internalDocument == snapshot.internalDocument || [self.internalDocument isEqual:snapshot.internalDocument]) && - _pendingWrites == snapshot->_pendingWrites; - self.fromCache == snapshot.fromCache; + _pendingWrites == snapshot->_pendingWrites && self.fromCache == snapshot.fromCache; } - (NSUInteger)hash { diff --git a/Firestore/Source/Local/FSTLocalStore.mm b/Firestore/Source/Local/FSTLocalStore.mm index 2b0eb11aa0f..e29885ffb85 100644 --- a/Firestore/Source/Local/FSTLocalStore.mm +++ b/Firestore/Source/Local/FSTLocalStore.mm @@ -274,7 +274,7 @@ - (FSTMaybeDocumentDictionary *)applyRemoteEvent:(FSTRemoteEvent *)remoteEvent { // manufactured events (e.g. in the case of a limbo document resolution failing). if (!existingDoc || doc.version == SnapshotVersion::None() || authoritativeUpdates.contains(doc.key) || - (doc.version >= existingDoc.version && existingDoc.hasPendingWrites)) { + (doc.version >= existingDoc.version && !existingDoc.hasPendingWrites)) { [self.remoteDocumentCache addEntry:doc]; } else { LOG_DEBUG( From 4e8e8bc809cda25db85ae15d8105abd08a42077c Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Thu, 25 Oct 2018 17:36:55 -0700 Subject: [PATCH 3/3] Fix LocalStore --- Firestore/Source/Local/FSTLocalStore.mm | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Firestore/Source/Local/FSTLocalStore.mm b/Firestore/Source/Local/FSTLocalStore.mm index e29885ffb85..b9eb893bd18 100644 --- a/Firestore/Source/Local/FSTLocalStore.mm +++ b/Firestore/Source/Local/FSTLocalStore.mm @@ -273,8 +273,8 @@ - (FSTMaybeDocumentDictionary *)applyRemoteEvent:(FSTRemoteEvent *)remoteEvent { // to the remote cache. We make an exception for SnapshotVersion.MIN which can happen for // manufactured events (e.g. in the case of a limbo document resolution failing). if (!existingDoc || doc.version == SnapshotVersion::None() || - authoritativeUpdates.contains(doc.key) || - (doc.version >= existingDoc.version && !existingDoc.hasPendingWrites)) { + (authoritativeUpdates.contains(doc.key) && !existingDoc.hasPendingWrites) || + doc.version >= existingDoc.version) { [self.remoteDocumentCache addEntry:doc]; } else { LOG_DEBUG(