diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/DocumentChange.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/DocumentChange.java index 3aa564d96ca..f439dbea985 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/DocumentChange.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/DocumentChange.java @@ -142,7 +142,11 @@ static List changesFromSnapshot( for (DocumentViewChange change : snapshot.getChanges()) { Document document = change.getDocument(); QueryDocumentSnapshot documentSnapshot = - QueryDocumentSnapshot.fromDocument(firestore, document, snapshot.isFromCache()); + QueryDocumentSnapshot.fromDocument( + firestore, + document, + snapshot.isFromCache(), + snapshot.getMutatedKeys().contains(document.getKey())); hardAssert( change.getType() == DocumentViewChange.Type.ADDED, "Invalid added event for first snapshot"); @@ -163,7 +167,11 @@ static List changesFromSnapshot( } Document document = change.getDocument(); QueryDocumentSnapshot documentSnapshot = - QueryDocumentSnapshot.fromDocument(firestore, document, snapshot.isFromCache()); + QueryDocumentSnapshot.fromDocument( + firestore, + document, + snapshot.isFromCache(), + snapshot.getMutatedKeys().contains(document.getKey())); int oldIndex, newIndex; Type type = getType(change); if (type != Type.ADDED) { diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/DocumentReference.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/DocumentReference.java index 3eb29d4accf..4fe7323b62f 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/DocumentReference.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/DocumentReference.java @@ -317,8 +317,12 @@ public Task get(Source source) { .getDocumentFromLocalCache(key) .continueWith( Executors.DIRECT_EXECUTOR, - (Task doc) -> - new DocumentSnapshot(firestore, key, doc.getResult(), /*isFromCache=*/ true)); + (Task task) -> { + Document doc = task.getResult(); + boolean hasPendingWrites = doc != null && doc.hasLocalMutations(); + return new DocumentSnapshot( + firestore, key, doc, /*isFromCache=*/ true, hasPendingWrites); + }); } else { return getViaSnapshotListener(source); } @@ -523,11 +527,16 @@ private ListenerRegistration addSnapshotListenerInternal( Document document = snapshot.getDocuments().getDocument(key); DocumentSnapshot documentSnapshot; if (document != null) { + boolean hasPendingWrites = snapshot.getMutatedKeys().contains(document.getKey()); documentSnapshot = - DocumentSnapshot.fromDocument(firestore, document, snapshot.isFromCache()); + DocumentSnapshot.fromDocument( + firestore, document, snapshot.isFromCache(), hasPendingWrites); } else { + // We don't raise `hasPendingWrites` for deleted documents. + boolean hasPendingWrites = false; documentSnapshot = - DocumentSnapshot.fromNoDocument(firestore, key, snapshot.isFromCache()); + DocumentSnapshot.fromNoDocument( + firestore, key, snapshot.isFromCache(), hasPendingWrites); } listener.onEvent(documentSnapshot, null); } else { diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/DocumentSnapshot.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/DocumentSnapshot.java index 6aae9c80d0c..e3444ab2e5e 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/DocumentSnapshot.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/DocumentSnapshot.java @@ -90,22 +90,25 @@ public enum ServerTimestampBehavior { private final SnapshotMetadata metadata; DocumentSnapshot( - FirebaseFirestore firestore, DocumentKey key, @Nullable Document doc, boolean isFromCache) { + FirebaseFirestore firestore, + DocumentKey key, + @Nullable Document doc, + boolean isFromCache, + boolean hasPendingWrites) { this.firestore = checkNotNull(firestore); this.key = checkNotNull(key); this.doc = doc; - boolean hasPendingWrites = this.doc != null && this.doc.hasLocalMutations(); this.metadata = new SnapshotMetadata(hasPendingWrites, isFromCache); } static DocumentSnapshot fromDocument( - FirebaseFirestore firestore, Document doc, boolean fromCache) { - return new DocumentSnapshot(firestore, doc.getKey(), doc, fromCache); + FirebaseFirestore firestore, Document doc, boolean fromCache, boolean hasPendingWrites) { + return new DocumentSnapshot(firestore, doc.getKey(), doc, fromCache, hasPendingWrites); } static DocumentSnapshot fromNoDocument( - FirebaseFirestore firestore, DocumentKey key, boolean fromCache) { - return new DocumentSnapshot(firestore, key, null, fromCache); + FirebaseFirestore firestore, DocumentKey key, boolean fromCache, boolean hasPendingWrites) { + return new DocumentSnapshot(firestore, key, null, fromCache, hasPendingWrites); } /** @return The id of the document. */ diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/QueryDocumentSnapshot.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/QueryDocumentSnapshot.java index dd5add25986..1b9e7a49556 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/QueryDocumentSnapshot.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/QueryDocumentSnapshot.java @@ -41,13 +41,17 @@ public class QueryDocumentSnapshot extends DocumentSnapshot { private QueryDocumentSnapshot( - FirebaseFirestore firestore, DocumentKey key, @Nullable Document doc, boolean isFromCache) { - super(firestore, key, doc, isFromCache); + FirebaseFirestore firestore, + DocumentKey key, + @Nullable Document doc, + boolean isFromCache, + boolean hasPendingWrites) { + super(firestore, key, doc, isFromCache, hasPendingWrites); } static QueryDocumentSnapshot fromDocument( - FirebaseFirestore firestore, Document doc, boolean fromCache) { - return new QueryDocumentSnapshot(firestore, doc.getKey(), doc, fromCache); + FirebaseFirestore firestore, Document doc, boolean fromCache, boolean hasPendingWrites) { + return new QueryDocumentSnapshot(firestore, doc.getKey(), doc, fromCache, hasPendingWrites); } /** diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/QuerySnapshot.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/QuerySnapshot.java index df47ffc047d..929cd46f36e 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/QuerySnapshot.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/QuerySnapshot.java @@ -194,7 +194,11 @@ public List toObjects( } private QueryDocumentSnapshot convertDocument(Document document) { - return QueryDocumentSnapshot.fromDocument(firestore, document, snapshot.isFromCache()); + return QueryDocumentSnapshot.fromDocument( + firestore, + document, + snapshot.isFromCache(), + snapshot.getMutatedKeys().contains(document.getKey())); } @Override diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/Transaction.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/Transaction.java index 8514eb9628b..11f7601d49c 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/Transaction.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/Transaction.java @@ -242,10 +242,10 @@ private Task getAsync(DocumentReference documentRef) { MaybeDocument doc = docs.get(0); if (doc instanceof Document) { return DocumentSnapshot.fromDocument( - firestore, (Document) doc, /*fromCache=*/ false); + firestore, (Document) doc, /*fromCache=*/ false, /*hasPendingWrites=*/ false); } else if (doc instanceof NoDocument) { return DocumentSnapshot.fromNoDocument( - firestore, doc.getKey(), /*fromCache=*/ false); + firestore, doc.getKey(), /*fromCache=*/ false, /*hasPendingWrites=*/ false); } else { throw fail( "BatchGetDocumentsRequest returned unexpected document type: " diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/QueryListener.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/QueryListener.java index b878a7f75cd..a0074874414 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/QueryListener.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/QueryListener.java @@ -81,7 +81,7 @@ public void onViewSnapshot(ViewSnapshot newSnapshot) { newSnapshot.getOldDocuments(), documentChanges, newSnapshot.isFromCache(), - newSnapshot.hasPendingWrites(), + newSnapshot.getMutatedKeys(), newSnapshot.didSyncStateChange()); } @@ -160,7 +160,7 @@ private void raiseInitialEvent(ViewSnapshot snapshot) { DocumentSet.emptySet(snapshot.getQuery().comparator()), QueryListener.getInitialViewChanges(snapshot), snapshot.isFromCache(), - snapshot.hasPendingWrites(), + snapshot.getMutatedKeys(), /*didSyncStateChange=*/ true); raisedInitialEvent = true; listener.onEvent(snapshot, null); diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/SyncEngine.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/SyncEngine.java index 3bb60e8d4ca..f512e891183 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/SyncEngine.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/SyncEngine.java @@ -364,7 +364,9 @@ public void handleRejectedListen(int targetId, Status error) { // Ideally, we would have a method in the local store to purge a document. However, it would // be tricky to keep all of the local store's invariants with another method. Map documentUpdates = - Collections.singletonMap(limboKey, new NoDocument(limboKey, SnapshotVersion.NONE)); + Collections.singletonMap( + limboKey, + new NoDocument(limboKey, SnapshotVersion.NONE, /*hasCommittedMutations=*/ false)); Set limboDocuments = Collections.singleton(limboKey); RemoteEvent event = new RemoteEvent( diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/View.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/View.java index 0cf6e8e37b3..1d4bb456709 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/View.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/View.java @@ -169,49 +169,66 @@ public DocumentChanges computeDocChanges( } } - if (newDoc != null) { - newDocumentSet = newDocumentSet.add(newDoc); - if (newDoc.hasLocalMutations()) { - newMutatedKeys = newMutatedKeys.insert(newDoc.getKey()); - } else { - newMutatedKeys = newMutatedKeys.remove(newDoc.getKey()); - } - } else { - newDocumentSet = newDocumentSet.remove(key); - newMutatedKeys = newMutatedKeys.remove(key); - } + boolean oldDocHadPendingMutations = + oldDoc != null && this.mutatedKeys.contains(oldDoc.getKey()); + + // We only consider committed mutations for documents that were mutated during the lifetime of + // the view. + boolean newDocHasPendingMutations = + newDoc != null + && (newDoc.hasLocalMutations() + || (this.mutatedKeys.contains(newDoc.getKey()) + && newDoc.hasCommittedMutations())); + + boolean changeApplied = false; + // Calculate change if (oldDoc != null && newDoc != null) { boolean docsEqual = oldDoc.getData().equals(newDoc.getData()); - if (!docsEqual || oldDoc.hasLocalMutations() != newDoc.hasLocalMutations()) { - // only report a change if document actually changed. - if (docsEqual) { - changeSet.addChange(DocumentViewChange.create(Type.METADATA, newDoc)); - } else { + if (!docsEqual) { + if (!shouldWaitForSyncedDocument(oldDoc, newDoc)) { changeSet.addChange(DocumentViewChange.create(Type.MODIFIED, newDoc)); + changeApplied = true; } - if (lastDocInLimit != null && query.comparator().compare(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 = true; } + } else if (oldDocHadPendingMutations != newDocHasPendingMutations) { + changeSet.addChange(DocumentViewChange.create(Type.METADATA, newDoc)); + changeApplied = true; } } else if (oldDoc == null && newDoc != null) { changeSet.addChange(DocumentViewChange.create(Type.ADDED, newDoc)); + changeApplied = true; } else if (oldDoc != null && newDoc == null) { changeSet.addChange(DocumentViewChange.create(Type.REMOVED, oldDoc)); + changeApplied = true; if (lastDocInLimit != null) { // A doc was removed from a full limit query. We'll need to requery from the local cache // to see if we know about some other doc that should be in the results. needsRefill = true; } } + + if (changeApplied) { + if (newDoc != null) { + newDocumentSet = newDocumentSet.add(newDoc); + if (newDoc.hasLocalMutations()) { + newMutatedKeys = newMutatedKeys.insert(newDoc.getKey()); + } else { + newMutatedKeys = newMutatedKeys.remove(newDoc.getKey()); + } + } else { + newDocumentSet = newDocumentSet.remove(key); + newMutatedKeys = newMutatedKeys.remove(key); + } + } } if (query.hasLimit()) { - // TODO: Make QuerySnapshot size be constant time. - while (newDocumentSet.size() > query.getLimit()) { + for (long i = newDocumentSet.size() - this.query.getLimit(); i > 0; --i) { Document oldDoc = newDocumentSet.getLastDocument(); newDocumentSet = newDocumentSet.remove(oldDoc.getKey()); newMutatedKeys = newMutatedKeys.remove(oldDoc.getKey()); @@ -226,6 +243,18 @@ public DocumentChanges computeDocChanges( return new DocumentChanges(newDocumentSet, changeSet, newMutatedKeys, needsRefill); } + private boolean shouldWaitForSyncedDocument(Document oldDoc, Document newDoc) { + // 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()); + } + /** * Updates the view with the given ViewDocumentChanges and updates limbo docs and sync state from * the given (optional) target change. @@ -273,7 +302,6 @@ public ViewChange applyChanges(DocumentChanges docChanges, TargetChange targetCh ViewSnapshot snapshot = null; if (viewChanges.size() != 0 || syncStatedChanged) { boolean fromCache = newSyncState == SyncState.LOCAL; - boolean hasPendingWrites = !docChanges.mutatedKeys.isEmpty(); snapshot = new ViewSnapshot( query, @@ -281,7 +309,7 @@ public ViewChange applyChanges(DocumentChanges docChanges, TargetChange targetCh oldDocumentSet, viewChanges, fromCache, - hasPendingWrites, + docChanges.mutatedKeys, syncStatedChanged); } return new ViewChange(snapshot, limboDocumentChanges); diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/ViewSnapshot.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/ViewSnapshot.java index fe333185575..ad54a67a1eb 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/ViewSnapshot.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/ViewSnapshot.java @@ -14,6 +14,8 @@ package com.google.firebase.firestore.core; +import com.google.firebase.database.collection.ImmutableSortedSet; +import com.google.firebase.firestore.model.DocumentKey; import com.google.firebase.firestore.model.DocumentSet; import java.util.List; @@ -31,7 +33,7 @@ public enum SyncState { private final DocumentSet oldDocuments; private final List changes; private final boolean isFromCache; - private final boolean hasPendingWrites; + private final ImmutableSortedSet mutatedKeys; private final boolean didSyncStateChange; public ViewSnapshot( @@ -40,14 +42,14 @@ public ViewSnapshot( DocumentSet oldDocuments, List changes, boolean isFromCache, - boolean hasPendingWrites, + ImmutableSortedSet mutatedKeys, boolean didSyncStateChange) { this.query = query; this.documents = documents; this.oldDocuments = oldDocuments; this.changes = changes; this.isFromCache = isFromCache; - this.hasPendingWrites = hasPendingWrites; + this.mutatedKeys = mutatedKeys; this.didSyncStateChange = didSyncStateChange; } @@ -72,7 +74,11 @@ public boolean isFromCache() { } public boolean hasPendingWrites() { - return hasPendingWrites; + return !mutatedKeys.isEmpty(); + } + + public ImmutableSortedSet getMutatedKeys() { + return mutatedKeys; } public boolean didSyncStateChange() { @@ -93,15 +99,15 @@ public boolean equals(Object o) { if (isFromCache != that.isFromCache) { return false; } - if (hasPendingWrites != that.hasPendingWrites) { - return false; - } if (didSyncStateChange != that.didSyncStateChange) { return false; } if (!query.equals(that.query)) { return false; } + if (!mutatedKeys.equals(that.mutatedKeys)) { + return false; + } if (!documents.equals(that.documents)) { return false; } @@ -117,8 +123,8 @@ public int hashCode() { result = 31 * result + documents.hashCode(); result = 31 * result + oldDocuments.hashCode(); result = 31 * result + changes.hashCode(); + result = 31 * result + mutatedKeys.hashCode(); result = 31 * result + (isFromCache ? 1 : 0); - result = 31 * result + (hasPendingWrites ? 1 : 0); result = 31 * result + (didSyncStateChange ? 1 : 0); return result; } @@ -135,8 +141,8 @@ public String toString() { + changes + ", isFromCache=" + isFromCache - + ", hasPendingWrites=" - + hasPendingWrites + + ", mutatedKeys=" + + mutatedKeys.size() + ", didSyncStateChange=" + didSyncStateChange + ")"; diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/LocalDocumentsView.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/LocalDocumentsView.java index c2a2e144b82..6f82a815ddd 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/LocalDocumentsView.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/LocalDocumentsView.java @@ -16,7 +16,6 @@ import static com.google.firebase.firestore.model.DocumentCollections.emptyDocumentMap; import static com.google.firebase.firestore.model.DocumentCollections.emptyMaybeDocumentMap; -import static com.google.firebase.firestore.util.Assert.fail; import com.google.firebase.database.collection.ImmutableSortedMap; import com.google.firebase.firestore.core.Query; @@ -86,7 +85,7 @@ ImmutableSortedMap getDocuments(Iterable getDocumentsMatchingCollection MaybeDocument baseDoc = results.get(key); MaybeDocument mutatedDoc = mutation.applyToLocalView(baseDoc, baseDoc, batch.getLocalWriteTime()); - if (mutatedDoc == null || mutatedDoc instanceof NoDocument) { - results = results.remove(key); - } else if (mutatedDoc instanceof Document) { + if (mutatedDoc instanceof Document) { results = results.insert(key, (Document) mutatedDoc); } else { - throw fail("Unknown document type: %s", mutatedDoc); + results = results.remove(key); } } } diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/LocalSerializer.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/LocalSerializer.java index a4d908271a0..55c8bd146ab 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/LocalSerializer.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/LocalSerializer.java @@ -73,7 +73,7 @@ MaybeDocument decodeMaybeDocument(com.google.firebase.firestore.proto.MaybeDocum return decodeDocument(proto.getDocument(), proto.getHasCommittedMutations()); case NO_DOCUMENT: - return decodeNoDocument(proto.getNoDocument()); + return decodeNoDocument(proto.getNoDocument(), proto.getHasCommittedMutations()); case UNKNOWN_DOCUMENT: return decodeUnknownDocument(proto.getUnknownDocument()); @@ -128,10 +128,11 @@ private com.google.firebase.firestore.proto.NoDocument encodeNoDocument(NoDocume } /** Decodes a NoDocument proto to the equivalent model. */ - private NoDocument decodeNoDocument(com.google.firebase.firestore.proto.NoDocument proto) { + private NoDocument decodeNoDocument( + com.google.firebase.firestore.proto.NoDocument proto, boolean hasCommittedMutations) { DocumentKey key = rpcSerializer.decodeKey(proto.getName()); SnapshotVersion version = rpcSerializer.decodeVersion(proto.getReadTime()); - return new NoDocument(key, version); + return new NoDocument(key, version, hasCommittedMutations); } /** Encodes a UnknownDocument value to the equivalent proto. */ diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/model/NoDocument.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/model/NoDocument.java index e75bb3bc328..74b6d6cad54 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/model/NoDocument.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/model/NoDocument.java @@ -17,8 +17,11 @@ /** Represents that no documents exists for the key at the given version. */ public class NoDocument extends MaybeDocument { - public NoDocument(DocumentKey key, SnapshotVersion version) { + private boolean hasCommittedMutations; + + public NoDocument(DocumentKey key, SnapshotVersion version, boolean hasCommittedMutations) { super(key, version); + this.hasCommittedMutations = hasCommittedMutations; } @Override @@ -32,7 +35,9 @@ public boolean equals(Object o) { NoDocument that = (NoDocument) o; - return getVersion().equals(that.getVersion()) && getKey().equals(that.getKey()); + return hasCommittedMutations == that.hasCommittedMutations + && getVersion().equals(that.getVersion()) + && getKey().equals(that.getKey()); } @Override @@ -41,20 +46,25 @@ public boolean hasPendingWrites() { } public boolean hasCommittedMutations() { - // We currently don't raise `hasPendingWrites` for deleted documents, even if Watch hasn't - // caught up to the deleted version yet. - return false; + return hasCommittedMutations; } @Override public int hashCode() { int result = getKey().hashCode(); + result = 31 * result + (hasCommittedMutations ? 1 : 0); result = 31 * result + getVersion().hashCode(); return result; } @Override public String toString() { - return "NoDocument{key=" + getKey() + ", version=" + getVersion() + '}'; + return "NoDocument{key=" + + getKey() + + ", version=" + + getVersion() + + ", hasCommittedMutations=" + + hasCommittedMutations() + + "}"; } } diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/model/mutation/DeleteMutation.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/model/mutation/DeleteMutation.java index 6794a49783d..fb46cb8d6fb 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/model/mutation/DeleteMutation.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/model/mutation/DeleteMutation.java @@ -67,7 +67,7 @@ public MaybeDocument applyToRemoteDocument( // We store the deleted document at the commit version of the delete. Any document version // that the server sends us before the delete was applied is discarded - return new NoDocument(getKey(), mutationResult.getVersion()); + return new NoDocument(getKey(), mutationResult.getVersion(), /*hasCommittedMutations=*/ true); } @Nullable @@ -80,6 +80,6 @@ public MaybeDocument applyToLocalView( return maybeDoc; } - return new NoDocument(getKey(), SnapshotVersion.NONE); + return new NoDocument(getKey(), SnapshotVersion.NONE, /*hasCommittedMutations=*/ false); } } diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/RemoteSerializer.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/RemoteSerializer.java index d80cc22b9d4..fc4eb36bd30 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/RemoteSerializer.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/RemoteSerializer.java @@ -421,7 +421,7 @@ private NoDocument decodeMissingDocument(BatchGetDocumentsResponse response) { hardAssert( !version.equals(SnapshotVersion.NONE), "Got a no document response with no snapshot version"); - return new NoDocument(key, version); + return new NoDocument(key, version, /*hasCommittedMutations=*/ false); } // Mutations @@ -1023,7 +1023,7 @@ public WatchChange decodeWatchChange(ListenResponse protoChange) { key = decodeKey(docDelete.getDocument()); // Note that version might be unset in which case we use SnapshotVersion.NONE version = decodeVersion(docDelete.getReadTime()); - NoDocument doc = new NoDocument(key, version); + NoDocument doc = new NoDocument(key, version, /*hasCommittedMutations=*/ false); watchChange = new WatchChange.DocumentChange(Collections.emptyList(), removed, doc.getKey(), doc); break; diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/WatchChangeAggregator.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/WatchChangeAggregator.java index 3b3555ef7da..10018cc82ef 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/WatchChangeAggregator.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/WatchChangeAggregator.java @@ -184,7 +184,10 @@ public void handleExistenceFilter(ExistenceFilterWatchChange watchChange) { // deleted document there might be another query that will raise this document as part of // a snapshot until it is resolved, essentially exposing inconsistency between queries. DocumentKey key = DocumentKey.fromPath(query.getPath()); - removeDocumentFromTarget(targetId, key, new NoDocument(key, SnapshotVersion.NONE)); + removeDocumentFromTarget( + targetId, + key, + new NoDocument(key, SnapshotVersion.NONE, /*hasCommittedMutations=*/ false)); } else { hardAssert( expectedCount == 1, "Single document existence filter with count: %d", expectedCount); @@ -221,7 +224,10 @@ public RemoteEvent createRemoteEvent(SnapshotVersion snapshotVersion) { // limboDocumentRefs. DocumentKey key = DocumentKey.fromPath(queryData.getQuery().getPath()); if (pendingDocumentUpdates.get(key) == null && !targetContainsDocument(targetId, key)) { - removeDocumentFromTarget(targetId, key, new NoDocument(key, snapshotVersion)); + removeDocumentFromTarget( + targetId, + key, + new NoDocument(key, snapshotVersion, /*hasCommittedMutations=*/ false)); } } diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/QuerySnapshotTest.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/QuerySnapshotTest.java index 0381627f069..73621e834f6 100644 --- a/firebase-firestore/src/test/java/com/google/firebase/firestore/QuerySnapshotTest.java +++ b/firebase-firestore/src/test/java/com/google/firebase/firestore/QuerySnapshotTest.java @@ -16,6 +16,7 @@ import static com.google.firebase.firestore.testutil.TestUtil.doc; import static com.google.firebase.firestore.testutil.TestUtil.docSet; +import static com.google.firebase.firestore.testutil.TestUtil.keySet; import static com.google.firebase.firestore.testutil.TestUtil.map; import static com.google.firebase.firestore.testutil.TestUtil.query; import static com.google.firebase.firestore.testutil.TestUtil.wrapObject; @@ -122,16 +123,18 @@ public void testIncludeMetadataChanges() { oldDocuments, documentChanges, /*isFromCache=*/ false, - /*hasPendingWrites=*/ false, + /*mutatedKeys=*/ keySet(), /*didSyncStateChange=*/ true); QuerySnapshot snapshot = new QuerySnapshot(new Query(fooQuery, firestore), viewSnapshot, firestore); QueryDocumentSnapshot doc1Snap = - QueryDocumentSnapshot.fromDocument(firestore, doc1New, /*fromCache=*/ false); + QueryDocumentSnapshot.fromDocument( + firestore, doc1New, /*fromCache=*/ false, /*hasPendingWrites=*/ false); QueryDocumentSnapshot doc2Snap = - QueryDocumentSnapshot.fromDocument(firestore, doc2New, /*fromCache=*/ false); + QueryDocumentSnapshot.fromDocument( + firestore, doc2New, /*fromCache=*/ false, /*hasPendingWrites=*/ false); assertEquals(1, snapshot.getDocumentChanges().size()); List changesWithoutMetadata = diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/TestUtil.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/TestUtil.java index a4198c2d6bc..5272456732b 100644 --- a/firebase-firestore/src/test/java/com/google/firebase/firestore/TestUtil.java +++ b/firebase-firestore/src/test/java/com/google/firebase/firestore/TestUtil.java @@ -58,9 +58,11 @@ public static DocumentReference documentReference(String path) { public static DocumentSnapshot documentSnapshot( String path, Map data, boolean isFromCache) { if (data == null) { - return DocumentSnapshot.fromNoDocument(FIRESTORE, key(path), isFromCache); + return DocumentSnapshot.fromNoDocument( + FIRESTORE, key(path), isFromCache, /*hasPendingWrites=*/ false); } else { - return DocumentSnapshot.fromDocument(FIRESTORE, doc(path, 1L, data), isFromCache); + return DocumentSnapshot.fromDocument( + FIRESTORE, doc(path, 1L, data), isFromCache, /*hasPendingWrites=*/ false); } } @@ -87,23 +89,30 @@ public static QuerySnapshot querySnapshot( boolean hasPendingWrites, boolean isFromCache) { DocumentSet oldDocuments = docSet(Document.keyComparator()); + ImmutableSortedSet mutatedKeys = DocumentKey.emptyKeySet(); for (Map.Entry pair : oldDocs.entrySet()) { + String docKey = path + "/" + pair.getKey(); oldDocuments = oldDocuments.add( doc( - path + "/" + pair.getKey(), + docKey, 1L, pair.getValue(), hasPendingWrites ? Document.DocumentState.SYNCED : Document.DocumentState.LOCAL_MUTATIONS)); + + if (hasPendingWrites) { + mutatedKeys = mutatedKeys.insert(key(docKey)); + } } DocumentSet newDocuments = docSet(Document.keyComparator()); List documentChanges = new ArrayList<>(); for (Map.Entry pair : docsToAdd.entrySet()) { + String docKey = path + "/" + pair.getKey(); Document docToAdd = doc( - path + "/" + pair.getKey(), + docKey, 1L, pair.getValue(), hasPendingWrites @@ -111,6 +120,10 @@ public static QuerySnapshot querySnapshot( : Document.DocumentState.LOCAL_MUTATIONS); newDocuments = newDocuments.add(docToAdd); documentChanges.add(DocumentViewChange.create(Type.ADDED, docToAdd)); + + if (hasPendingWrites) { + mutatedKeys = mutatedKeys.insert(key(docKey)); + } } ViewSnapshot viewSnapshot = new ViewSnapshot( @@ -119,7 +132,7 @@ public static QuerySnapshot querySnapshot( oldDocuments, documentChanges, isFromCache, - hasPendingWrites, + mutatedKeys, true); return new QuerySnapshot(query(path), viewSnapshot, FIRESTORE); } diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/core/QueryListenerTest.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/core/QueryListenerTest.java index 4711065deb9..982711b94c4 100644 --- a/firebase-firestore/src/test/java/com/google/firebase/firestore/core/QueryListenerTest.java +++ b/firebase-firestore/src/test/java/com/google/firebase/firestore/core/QueryListenerTest.java @@ -106,7 +106,7 @@ public void testRaisesCollectionEvents() { DocumentSet.emptySet(snap2.getQuery().comparator()), asList(change1, change4), snap2.isFromCache(), - snap2.hasPendingWrites(), + snap2.getMutatedKeys(), /* didSyncStateChange= */ true); assertEquals(asList(snap2Prime), otherAccum); } @@ -253,7 +253,7 @@ public void testRaisesQueryMetadataEventsOnlyWhenHasPendingWritesOnTheQueryChang snap3.getDocuments(), asList(), snap4.isFromCache(), - snap4.hasPendingWrites(), + snap4.getMutatedKeys(), snap4.didSyncStateChange()); assertEquals(asList(snap1, snap3, expectedSnapshot4), fullAccum); } @@ -287,7 +287,7 @@ public void testMetadataOnlyDocumentChangesAreFilteredOut() { snap1.getDocuments(), asList(change3), snap2.isFromCache(), - snap2.hasPendingWrites(), + snap2.getMutatedKeys(), snap2.didSyncStateChange()); assertEquals(asList(snap1, expectedSnapshot2), filteredAccum); } @@ -326,7 +326,7 @@ public void testWillWaitForSyncIfOnline() { DocumentSet.emptySet(snap3.getQuery().comparator()), asList(change1, change2), /* isFromCache= */ false, - /*hasPendingWrites=*/ false, + snap3.getMutatedKeys(), /* didSyncStateChange= */ true); assertEquals(asList(expectedSnapshot), events); } @@ -363,7 +363,7 @@ public void testWillRaiseInitialEventWhenGoingOffline() { DocumentSet.emptySet(snap1.getQuery().comparator()), asList(change1), /* isFromCache= */ true, - /*hasPendingWrites=*/ false, + snap1.getMutatedKeys(), /* didSyncStateChange= */ true); ViewSnapshot expectedSnapshot2 = new ViewSnapshot( @@ -372,7 +372,7 @@ public void testWillRaiseInitialEventWhenGoingOffline() { snap1.getDocuments(), asList(change2), /* isFromCache= */ true, - /*hasPendingWrites=*/ false, + snap2.getMutatedKeys(), /* didSyncStateChange= */ false); assertEquals(asList(expectedSnapshot1, expectedSnapshot2), events); } @@ -398,7 +398,7 @@ public void testWillRaiseInitialEventWhenGoingOfflineAndThereAreNoDocs() { DocumentSet.emptySet(snap1.getQuery().comparator()), asList(), /* isFromCache= */ true, - /*hasPendingWrites=*/ false, + snap1.getMutatedKeys(), /* didSyncStateChange= */ true); assertEquals(asList(expectedSnapshot), events); } @@ -423,7 +423,7 @@ public void testWillRaiseInitialEventWhenStartingOfflineAndThereAreNoDocs() { DocumentSet.emptySet(snap1.getQuery().comparator()), asList(), /* isFromCache= */ true, - /*hasPendingWrites=*/ false, + snap1.getMutatedKeys(), /* didSyncStateChange= */ true); assertEquals(asList(expectedSnapshot), events); } diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/core/ViewSnapshotTest.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/core/ViewSnapshotTest.java index 8e184dfad45..859a0dbf908 100644 --- a/firebase-firestore/src/test/java/com/google/firebase/firestore/core/ViewSnapshotTest.java +++ b/firebase-firestore/src/test/java/com/google/firebase/firestore/core/ViewSnapshotTest.java @@ -15,11 +15,15 @@ package com.google.firebase.firestore.core; import static com.google.firebase.firestore.testutil.TestUtil.doc; +import static com.google.firebase.firestore.testutil.TestUtil.key; +import static com.google.firebase.firestore.testutil.TestUtil.keySet; import static com.google.firebase.firestore.testutil.TestUtil.map; import static org.junit.Assert.assertEquals; +import com.google.firebase.database.collection.ImmutableSortedSet; import com.google.firebase.firestore.core.DocumentViewChange.Type; import com.google.firebase.firestore.model.Document; +import com.google.firebase.firestore.model.DocumentKey; import com.google.firebase.firestore.model.DocumentSet; import com.google.firebase.firestore.model.ResourcePath; import java.util.Arrays; @@ -40,19 +44,20 @@ public void testConstructor() { DocumentSet oldDocs = DocumentSet.emptySet(Document.keyComparator()); List changes = Arrays.asList(DocumentViewChange.create(Type.ADDED, doc("c/foo", 1, map()))); + ImmutableSortedSet mutatedKeys = keySet(key("c/foo")); boolean fromCache = true; boolean hasPendingWrites = true; boolean syncStateChanges = true; ViewSnapshot snapshot = - new ViewSnapshot( - query, docs, oldDocs, changes, fromCache, hasPendingWrites, syncStateChanges); + new ViewSnapshot(query, docs, oldDocs, changes, fromCache, mutatedKeys, syncStateChanges); assertEquals(query, snapshot.getQuery()); assertEquals(docs, snapshot.getDocuments()); assertEquals(oldDocs, snapshot.getOldDocuments()); assertEquals(changes, snapshot.getChanges()); assertEquals(fromCache, snapshot.isFromCache()); + assertEquals(mutatedKeys, snapshot.getMutatedKeys()); assertEquals(hasPendingWrites, snapshot.hasPendingWrites()); assertEquals(syncStateChanges, snapshot.didSyncStateChange()); } diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/core/ViewTest.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/core/ViewTest.java index d54ef332caa..efac909cf79 100644 --- a/firebase-firestore/src/test/java/com/google/firebase/firestore/core/ViewTest.java +++ b/firebase-firestore/src/test/java/com/google/firebase/firestore/core/ViewTest.java @@ -39,6 +39,7 @@ import com.google.firebase.firestore.model.ResourcePath; import com.google.firebase.firestore.remote.TargetChange; import com.google.protobuf.ByteString; +import java.util.Collections; import org.junit.Test; import org.junit.runner.RunWith; import org.robolectric.RobolectricTestRunner; @@ -558,4 +559,78 @@ public void testRemembersLocalMutationsFromPreviousCallToComputeChanges() { changes = view.computeDocChanges(docUpdates(doc3), changes); assertEquals(keySet(doc2.getKey()), changes.mutatedKeys); } + + @Test + public void testRaisesHasPendingWritesForPendingMutationsInInitialSnapshot() { + Query query = messageQuery(); + Document doc1 = doc("rooms/eros/messages/1", 0, map(), Document.DocumentState.LOCAL_MUTATIONS); + View view = new View(query, DocumentKey.emptyKeySet()); + + View.DocumentChanges changes = view.computeDocChanges(docUpdates(doc1)); + ViewChange viewChange = view.applyChanges(changes); + + assertTrue(viewChange.getSnapshot().hasPendingWrites()); + } + + @Test + public void testDoesntRaiseHasPendingWritesForCommittedMutationsInInitialSnapshot() { + Query query = messageQuery(); + Document doc1 = + doc("rooms/eros/messages/1", 0, map(), Document.DocumentState.COMMITTED_MUTATIONS); + View view = new View(query, DocumentKey.emptyKeySet()); + + View.DocumentChanges changes = view.computeDocChanges(docUpdates(doc1)); + ViewChange viewChange = view.applyChanges(changes); + + assertFalse(viewChange.getSnapshot().hasPendingWrites()); + } + + @Test + public void testSuppressesWriteAcknowledgementIfWatchHasNotCaughtUp() { + // This test verifies that we don't get three events for a ServerTimestamp mutation. We suppress + // the event generated by the write acknowledgement and instead wait for Watch to catch up. + Query query = messageQuery(); + + Document doc1 = + doc("rooms/eros/messages/1", 1, map("time", 1), Document.DocumentState.LOCAL_MUTATIONS); + Document doc1Committed = + doc("rooms/eros/messages/1", 2, map("time", 2), Document.DocumentState.COMMITTED_MUTATIONS); + Document doc1Acknowledged = + doc("rooms/eros/messages/1", 2, map("time", 2), Document.DocumentState.SYNCED); + + Document doc2 = + doc("rooms/eros/messages/2", 1, map("time", 1), Document.DocumentState.LOCAL_MUTATIONS); + Document doc2Modified = + doc("rooms/eros/messages/2", 2, map("time", 3), Document.DocumentState.LOCAL_MUTATIONS); + Document doc2Acknowledged = + doc("rooms/eros/messages/2", 2, map("time", 3), Document.DocumentState.SYNCED); + + View view = new View(query, DocumentKey.emptyKeySet()); + + View.DocumentChanges changes = view.computeDocChanges(docUpdates(doc1, doc2)); + ViewChange snap = view.applyChanges(changes); + + assertEquals( + asList( + DocumentViewChange.create(Type.ADDED, doc1), + DocumentViewChange.create(Type.ADDED, doc2)), + snap.getSnapshot().getChanges()); + + changes = view.computeDocChanges(docUpdates(doc1Committed, doc2Modified)); + snap = view.applyChanges(changes); + + // The 'doc1Committed' update is suppressed + assertEquals( + Collections.singletonList(DocumentViewChange.create(Type.MODIFIED, doc2Modified)), + snap.getSnapshot().getChanges()); + + changes = view.computeDocChanges(docUpdates(doc1Acknowledged, doc2Acknowledged)); + snap = view.applyChanges(changes); + + assertEquals( + asList( + DocumentViewChange.create(Type.MODIFIED, doc1Acknowledged), + DocumentViewChange.create(Type.METADATA, doc2Acknowledged)), + snap.getSnapshot().getChanges()); + } } diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/local/LocalStoreTestCase.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/local/LocalStoreTestCase.java index dc438ac317d..5144a91e62e 100644 --- a/firebase-firestore/src/test/java/com/google/firebase/firestore/local/LocalStoreTestCase.java +++ b/firebase-firestore/src/test/java/com/google/firebase/firestore/local/LocalStoreTestCase.java @@ -249,14 +249,16 @@ public void testHandlesSetMutationThenAckThenRelease() { allocateQuery(query); writeMutation(setMutation("foo/bar", map("foo", "bar"))); + notifyLocalViewChanges(viewChanges(2, asList("foo/bar"), emptyList())); + assertChanged(doc("foo/bar", 0, map("foo", "bar"), Document.DocumentState.LOCAL_MUTATIONS)); assertContains(doc("foo/bar", 0, map("foo", "bar"), Document.DocumentState.LOCAL_MUTATIONS)); acknowledgeMutation(1); - notifyLocalViewChanges(viewChanges(2, asList("foo/bar"), emptyList())); assertChanged(doc("foo/bar", 1, map("foo", "bar"), Document.DocumentState.COMMITTED_MUTATIONS)); - assertContains(doc("foo/bar", 1, map("foo", "bar"), Document.DocumentState.COMMITTED_MUTATIONS)); + assertContains( + doc("foo/bar", 1, map("foo", "bar"), Document.DocumentState.COMMITTED_MUTATIONS)); releaseQuery(query); @@ -264,7 +266,8 @@ public void testHandlesSetMutationThenAckThenRelease() { if (garbageCollectorIsEager()) { assertNotContains("foo/bar"); } else { - assertContains(doc("foo/bar", 0, map("foo", "bar"), Document.DocumentState.COMMITTED_MUTATIONS)); + assertContains( + doc("foo/bar", 1, map("foo", "bar"), Document.DocumentState.COMMITTED_MUTATIONS)); } } @@ -463,7 +466,7 @@ public void testHandlesDeleteMutationThenAck() { if (garbageCollectorIsEager()) { assertNotContains("foo/bar"); } else { - assertContains(deletedDoc("foo/bar", 1)); + assertContains(deletedDoc("foo/bar", 1, /*hasCommittedMutations=*/ true)); } } @@ -487,7 +490,7 @@ public void testHandlesDocumentThenDeleteMutationThenAck() { // Neither the target nor the mutation pin the document, it should be gone. assertNotContains("foo/bar"); } else { - assertContains(deletedDoc("foo/bar", 2)); + assertContains(deletedDoc("foo/bar", 2, /*hasCommittedMutations=*/ true)); } } @@ -512,7 +515,7 @@ public void testHandlesDeleteMutationThenDocumentThenAck() { // anymore. assertNotContains("foo/bar"); } else { - assertContains(deletedDoc("foo/bar", 2)); + assertContains(deletedDoc("foo/bar", 2, /*hasCommittedMutations=*/ true)); } } @@ -631,7 +634,7 @@ public void testHandlesDeleteMutationThenPatchMutationThenAckThenAck() { acknowledgeMutation(2); // delete mutation assertRemoved("foo/bar"); - assertContains(deletedDoc("foo/bar", 2)); + assertContains(deletedDoc("foo/bar", 2, true)); acknowledgeMutation(3); // patch mutation assertChanged(unknownDoc("foo/bar", 3)); diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/model/MutationTest.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/model/MutationTest.java index 90b23df0099..d686c4829ef 100644 --- a/firebase-firestore/src/test/java/com/google/firebase/firestore/model/MutationTest.java +++ b/firebase-firestore/src/test/java/com/google/firebase/firestore/model/MutationTest.java @@ -435,7 +435,7 @@ public void testTransitions() { Mutation transform = transformMutation("collection/key", map()); Mutation delete = deleteMutation("collection/key"); - NoDocument docV7Deleted = deletedDoc("collection/key", 7); + NoDocument docV7Deleted = deletedDoc("collection/key", 7, /*hasCommittedMutations=*/ true); Document docV7Committed = doc("collection/key", 7, map(), Document.DocumentState.COMMITTED_MUTATIONS); UnknownDocument docV7Unknown = unknownDoc("collection/key", 7); diff --git a/firebase-firestore/src/testUtil/java/com/google/firebase/firestore/testutil/TestUtil.java b/firebase-firestore/src/testUtil/java/com/google/firebase/firestore/testutil/TestUtil.java index a639fc1048e..d6d5384fd5f 100644 --- a/firebase-firestore/src/testUtil/java/com/google/firebase/firestore/testutil/TestUtil.java +++ b/firebase-firestore/src/testUtil/java/com/google/firebase/firestore/testutil/TestUtil.java @@ -182,7 +182,11 @@ public static Document doc( } public static NoDocument deletedDoc(String key, long version) { - return new NoDocument(key(key), version(version)); + return deletedDoc(key, version, /*hasCommittedMutations=*/ false); + } + + public static NoDocument deletedDoc(String key, long version, boolean hasCommittedMutations) { + return new NoDocument(key(key), version(version), hasCommittedMutations); } public static UnknownDocument unknownDoc(String key, long version) {