Skip to content

View changes for held write acks #2004

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Oct 26, 2018
Merged

View changes for held write acks #2004

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
13e20ee
View changes for held write acks
schmidt-sebastian Oct 24, 2018
ff43d95
Merge branch 'held_acks_master' into mrschmidt-viewchanges
schmidt-sebastian Oct 25, 2018
5113f55
Merge branch 'held_acks_master' into mrschmidt-viewchanges
schmidt-sebastian Oct 26, 2018
902c44c
Review comments
schmidt-sebastian Oct 26, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions Firestore/Example/Tests/API/FIRQuerySnapshotTests.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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];
Expand All @@ -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<FIRDocumentChange *> *changesWithoutMetadata = @[
[[FIRDocumentChange alloc] initWithType:FIRDocumentChangeTypeModified
Expand Down
14 changes: 12 additions & 2 deletions Firestore/Example/Tests/API/FSTAPIHelpers.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -93,12 +94,17 @@
FIRSnapshotMetadata *metadata =
[FIRSnapshotMetadata snapshotMetadataWithPendingWrites:hasPendingWrites fromCache:fromCache];
FSTDocumentSet *oldDocuments = FSTTestDocSet(FSTDocumentComparatorByKey, @[]);
DocumentKeySet mutatedKeys;
for (NSString *key in oldDocs) {
oldDocuments = [oldDocuments
documentSetByAddingDocument:FSTTestDoc(util::StringFormat("%s/%s", path, key), 1,
oldDocs[key],
hasPendingWrites ? FSTDocumentStateLocalMutations
: FSTDocumentStateSynced)];
if (hasPendingWrites) {
const absl::string_view documentKey = util::StringFormat("%s/%s", path, key);
mutatedKeys = mutatedKeys.insert(testutil::Key(documentKey));
}
}
FSTDocumentSet *newDocuments = oldDocuments;
NSArray<FSTDocumentViewChange *> *documentChanges = [NSArray array];
Expand All @@ -111,13 +117,17 @@
arrayByAddingObject:[FSTDocumentViewChange
changeWithDocument:docToAdd
type:FSTDocumentViewChangeTypeAdded]];
if (hasPendingWrites) {
const absl::string_view documentKey = util::StringFormat("%s/%s", path, key);
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)
Expand Down
16 changes: 8 additions & 8 deletions Firestore/Example/Tests/Core/FSTQueryListenerTests.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 ]));
}
Expand Down Expand Up @@ -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 ]));
}
Expand Down Expand Up @@ -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 ]));
}
Expand Down Expand Up @@ -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 ]));
}
Expand Down Expand Up @@ -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 ]));
}
Expand All @@ -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 ]));
}
Expand All @@ -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 ]));
}
Expand Down
6 changes: 3 additions & 3 deletions Firestore/Example/Tests/Core/FSTViewSnapshotTest.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,23 +116,23 @@ - (void)testViewSnapshotConstructor {
type:FSTDocumentViewChangeTypeAdded] ];

BOOL fromCache = YES;
BOOL hasPendingWrites = NO;
DocumentKeySet mutatedKeys;
BOOL syncStateChanged = YES;

FSTViewSnapshot *snapshot = [[FSTViewSnapshot alloc] initWithQuery:query
documents:documents
oldDocuments:oldDocuments
documentChanges:documentChanges
fromCache:fromCache
hasPendingWrites:hasPendingWrites
mutatedKeys:mutatedKeys
syncStateChanged:syncStateChanged];

XCTAssertEqual(snapshot.query, query);
XCTAssertEqual(snapshot.documents, documents);
XCTAssertEqual(snapshot.oldDocuments, oldDocuments);
XCTAssertEqual(snapshot.documentChanges, documentChanges);
XCTAssertEqual(snapshot.fromCache, fromCache);
XCTAssertEqual(snapshot.hasPendingWrites, hasPendingWrites);
XCTAssertEqual(snapshot.mutatedKeys, mutatedKeys);
XCTAssertEqual(snapshot.syncStateChanged, syncStateChanged);
}

Expand Down
70 changes: 70 additions & 0 deletions Firestore/Example/Tests/Core/FSTViewTests.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -641,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
22 changes: 12 additions & 10 deletions Firestore/Source/API/FIRDocumentChange.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,11 +60,12 @@ + (FIRDocumentChangeType)documentChangeTypeForChange:(FSTDocumentViewChange *)ch
NSUInteger index = 0;
NSMutableArray<FIRDocumentChange *> *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) ==
Expand All @@ -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;
Expand Down
7 changes: 6 additions & 1 deletion Firestore/Source/API/FIRDocumentReference.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
};

Expand Down
3 changes: 2 additions & 1 deletion Firestore/Source/API/FIRDocumentSnapshot+Internal.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down
Loading