Skip to content

Commit 931c9da

Browse files
schmidt-sebastianschmidt-sebastian
authored
View changes for held write acks (#41)
1 parent 001a515 commit 931c9da

24 files changed

+282
-101
lines changed

‎firebase-firestore/src/main/java/com/google/firebase/firestore/DocumentChange.java

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -142,7 +142,11 @@ static List<DocumentChange> changesFromSnapshot(
142142
for (DocumentViewChange change : snapshot.getChanges()) {
143143
Document document = change.getDocument();
144144
QueryDocumentSnapshot documentSnapshot =
145-
QueryDocumentSnapshot.fromDocument(firestore, document, snapshot.isFromCache());
145+
QueryDocumentSnapshot.fromDocument(
146+
firestore,
147+
document,
148+
snapshot.isFromCache(),
149+
snapshot.getMutatedKeys().contains(document.getKey()));
146150
hardAssert(
147151
change.getType() == DocumentViewChange.Type.ADDED,
148152
"Invalid added event for first snapshot");
@@ -163,7 +167,11 @@ static List<DocumentChange> changesFromSnapshot(
163167
}
164168
Document document = change.getDocument();
165169
QueryDocumentSnapshot documentSnapshot =
166-
QueryDocumentSnapshot.fromDocument(firestore, document, snapshot.isFromCache());
170+
QueryDocumentSnapshot.fromDocument(
171+
firestore,
172+
document,
173+
snapshot.isFromCache(),
174+
snapshot.getMutatedKeys().contains(document.getKey()));
167175
int oldIndex, newIndex;
168176
Type type = getType(change);
169177
if (type != Type.ADDED) {

‎firebase-firestore/src/main/java/com/google/firebase/firestore/DocumentReference.java

Lines changed: 13 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -317,8 +317,12 @@ public Task<DocumentSnapshot> get(Source source) {
317317
.getDocumentFromLocalCache(key)
318318
.continueWith(
319319
Executors.DIRECT_EXECUTOR,
320-
(Task<Document> doc) ->
321-
new DocumentSnapshot(firestore, key, doc.getResult(), /*isFromCache=*/ true));
320+
(Task<Document> task) -> {
321+
Document doc = task.getResult();
322+
boolean hasPendingWrites = doc != null && doc.hasLocalMutations();
323+
return new DocumentSnapshot(
324+
firestore, key, doc, /*isFromCache=*/ true, hasPendingWrites);
325+
});
322326
} else {
323327
return getViaSnapshotListener(source);
324328
}
@@ -523,11 +527,16 @@ private ListenerRegistration addSnapshotListenerInternal(
523527
Document document = snapshot.getDocuments().getDocument(key);
524528
DocumentSnapshot documentSnapshot;
525529
if (document != null) {
530+
boolean hasPendingWrites = snapshot.getMutatedKeys().contains(document.getKey());
526531
documentSnapshot =
527-
DocumentSnapshot.fromDocument(firestore, document, snapshot.isFromCache());
532+
DocumentSnapshot.fromDocument(
533+
firestore, document, snapshot.isFromCache(), hasPendingWrites);
528534
} else {
535+
// We don't raise `hasPendingWrites` for deleted documents.
536+
boolean hasPendingWrites = false;
529537
documentSnapshot =
530-
DocumentSnapshot.fromNoDocument(firestore, key, snapshot.isFromCache());
538+
DocumentSnapshot.fromNoDocument(
539+
firestore, key, snapshot.isFromCache(), hasPendingWrites);
531540
}
532541
listener.onEvent(documentSnapshot, null);
533542
} else {

‎firebase-firestore/src/main/java/com/google/firebase/firestore/DocumentSnapshot.java

Lines changed: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -90,22 +90,25 @@ public enum ServerTimestampBehavior {
9090
private final SnapshotMetadata metadata;
9191

9292
DocumentSnapshot(
93-
FirebaseFirestore firestore, DocumentKey key, @Nullable Document doc, boolean isFromCache) {
93+
FirebaseFirestore firestore,
94+
DocumentKey key,
95+
@Nullable Document doc,
96+
boolean isFromCache,
97+
boolean hasPendingWrites) {
9498
this.firestore = checkNotNull(firestore);
9599
this.key = checkNotNull(key);
96100
this.doc = doc;
97-
boolean hasPendingWrites = this.doc != null && this.doc.hasLocalMutations();
98101
this.metadata = new SnapshotMetadata(hasPendingWrites, isFromCache);
99102
}
100103

101104
static DocumentSnapshot fromDocument(
102-
FirebaseFirestore firestore, Document doc, boolean fromCache) {
103-
return new DocumentSnapshot(firestore, doc.getKey(), doc, fromCache);
105+
FirebaseFirestore firestore, Document doc, boolean fromCache, boolean hasPendingWrites) {
106+
return new DocumentSnapshot(firestore, doc.getKey(), doc, fromCache, hasPendingWrites);
104107
}
105108

106109
static DocumentSnapshot fromNoDocument(
107-
FirebaseFirestore firestore, DocumentKey key, boolean fromCache) {
108-
return new DocumentSnapshot(firestore, key, null, fromCache);
110+
FirebaseFirestore firestore, DocumentKey key, boolean fromCache, boolean hasPendingWrites) {
111+
return new DocumentSnapshot(firestore, key, null, fromCache, hasPendingWrites);
109112
}
110113

111114
/** @return The id of the document. */

‎firebase-firestore/src/main/java/com/google/firebase/firestore/QueryDocumentSnapshot.java

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -41,13 +41,17 @@
4141
public class QueryDocumentSnapshot extends DocumentSnapshot {
4242

4343
private QueryDocumentSnapshot(
44-
FirebaseFirestore firestore, DocumentKey key, @Nullable Document doc, boolean isFromCache) {
45-
super(firestore, key, doc, isFromCache);
44+
FirebaseFirestore firestore,
45+
DocumentKey key,
46+
@Nullable Document doc,
47+
boolean isFromCache,
48+
boolean hasPendingWrites) {
49+
super(firestore, key, doc, isFromCache, hasPendingWrites);
4650
}
4751

4852
static QueryDocumentSnapshot fromDocument(
49-
FirebaseFirestore firestore, Document doc, boolean fromCache) {
50-
return new QueryDocumentSnapshot(firestore, doc.getKey(), doc, fromCache);
53+
FirebaseFirestore firestore, Document doc, boolean fromCache, boolean hasPendingWrites) {
54+
return new QueryDocumentSnapshot(firestore, doc.getKey(), doc, fromCache, hasPendingWrites);
5155
}
5256

5357
/**

‎firebase-firestore/src/main/java/com/google/firebase/firestore/QuerySnapshot.java

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -194,7 +194,11 @@ public <T> List<T> toObjects(
194194
}
195195

196196
private QueryDocumentSnapshot convertDocument(Document document) {
197-
return QueryDocumentSnapshot.fromDocument(firestore, document, snapshot.isFromCache());
197+
return QueryDocumentSnapshot.fromDocument(
198+
firestore,
199+
document,
200+
snapshot.isFromCache(),
201+
snapshot.getMutatedKeys().contains(document.getKey()));
198202
}
199203

200204
@Override

‎firebase-firestore/src/main/java/com/google/firebase/firestore/Transaction.java

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -242,10 +242,10 @@ private Task<DocumentSnapshot> getAsync(DocumentReference documentRef) {
242242
MaybeDocument doc = docs.get(0);
243243
if (doc instanceof Document) {
244244
return DocumentSnapshot.fromDocument(
245-
firestore, (Document) doc, /*fromCache=*/ false);
245+
firestore, (Document) doc, /*fromCache=*/ false, /*hasPendingWrites=*/ false);
246246
} else if (doc instanceof NoDocument) {
247247
return DocumentSnapshot.fromNoDocument(
248-
firestore, doc.getKey(), /*fromCache=*/ false);
248+
firestore, doc.getKey(), /*fromCache=*/ false, /*hasPendingWrites=*/ false);
249249
} else {
250250
throw fail(
251251
"BatchGetDocumentsRequest returned unexpected document type: "

‎firebase-firestore/src/main/java/com/google/firebase/firestore/core/QueryListener.java

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -81,7 +81,7 @@ public void onViewSnapshot(ViewSnapshot newSnapshot) {
8181
newSnapshot.getOldDocuments(),
8282
documentChanges,
8383
newSnapshot.isFromCache(),
84-
newSnapshot.hasPendingWrites(),
84+
newSnapshot.getMutatedKeys(),
8585
newSnapshot.didSyncStateChange());
8686
}
8787

@@ -160,7 +160,7 @@ private void raiseInitialEvent(ViewSnapshot snapshot) {
160160
DocumentSet.emptySet(snapshot.getQuery().comparator()),
161161
QueryListener.getInitialViewChanges(snapshot),
162162
snapshot.isFromCache(),
163-
snapshot.hasPendingWrites(),
163+
snapshot.getMutatedKeys(),
164164
/*didSyncStateChange=*/ true);
165165
raisedInitialEvent = true;
166166
listener.onEvent(snapshot, null);

‎firebase-firestore/src/main/java/com/google/firebase/firestore/core/SyncEngine.java

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -364,7 +364,9 @@ public void handleRejectedListen(int targetId, Status error) {
364364
// Ideally, we would have a method in the local store to purge a document. However, it would
365365
// be tricky to keep all of the local store's invariants with another method.
366366
Map<DocumentKey, MaybeDocument> documentUpdates =
367-
Collections.singletonMap(limboKey, new NoDocument(limboKey, SnapshotVersion.NONE));
367+
Collections.singletonMap(
368+
limboKey,
369+
new NoDocument(limboKey, SnapshotVersion.NONE, /*hasCommittedMutations=*/ false));
368370
Set<DocumentKey> limboDocuments = Collections.singleton(limboKey);
369371
RemoteEvent event =
370372
new RemoteEvent(

‎firebase-firestore/src/main/java/com/google/firebase/firestore/core/View.java

Lines changed: 49 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -169,49 +169,66 @@ public <D extends MaybeDocument> DocumentChanges computeDocChanges(
169169
}
170170
}
171171

172-
if (newDoc != null) {
173-
newDocumentSet = newDocumentSet.add(newDoc);
174-
if (newDoc.hasLocalMutations()) {
175-
newMutatedKeys = newMutatedKeys.insert(newDoc.getKey());
176-
} else {
177-
newMutatedKeys = newMutatedKeys.remove(newDoc.getKey());
178-
}
179-
} else {
180-
newDocumentSet = newDocumentSet.remove(key);
181-
newMutatedKeys = newMutatedKeys.remove(key);
182-
}
172+
boolean oldDocHadPendingMutations =
173+
oldDoc != null && this.mutatedKeys.contains(oldDoc.getKey());
174+
175+
// We only consider committed mutations for documents that were mutated during the lifetime of
176+
// the view.
177+
boolean newDocHasPendingMutations =
178+
newDoc != null
179+
&& (newDoc.hasLocalMutations()
180+
|| (this.mutatedKeys.contains(newDoc.getKey())
181+
&& newDoc.hasCommittedMutations()));
182+
183+
boolean changeApplied = false;
184+
183185
// Calculate change
184186
if (oldDoc != null && newDoc != null) {
185187
boolean docsEqual = oldDoc.getData().equals(newDoc.getData());
186-
if (!docsEqual || oldDoc.hasLocalMutations() != newDoc.hasLocalMutations()) {
187-
// only report a change if document actually changed.
188-
if (docsEqual) {
189-
changeSet.addChange(DocumentViewChange.create(Type.METADATA, newDoc));
190-
} else {
188+
if (!docsEqual) {
189+
if (!shouldWaitForSyncedDocument(oldDoc, newDoc)) {
191190
changeSet.addChange(DocumentViewChange.create(Type.MODIFIED, newDoc));
191+
changeApplied = true;
192192
}
193-
194193
if (lastDocInLimit != null && query.comparator().compare(newDoc, lastDocInLimit) > 0) {
195194
// This doc moved from inside the limit to after the limit. That means there may be some
196195
// doc in the local cache that's actually less than this one.
197196
needsRefill = true;
198197
}
198+
} else if (oldDocHadPendingMutations != newDocHasPendingMutations) {
199+
changeSet.addChange(DocumentViewChange.create(Type.METADATA, newDoc));
200+
changeApplied = true;
199201
}
200202
} else if (oldDoc == null && newDoc != null) {
201203
changeSet.addChange(DocumentViewChange.create(Type.ADDED, newDoc));
204+
changeApplied = true;
202205
} else if (oldDoc != null && newDoc == null) {
203206
changeSet.addChange(DocumentViewChange.create(Type.REMOVED, oldDoc));
207+
changeApplied = true;
204208
if (lastDocInLimit != null) {
205209
// A doc was removed from a full limit query. We'll need to requery from the local cache
206210
// to see if we know about some other doc that should be in the results.
207211
needsRefill = true;
208212
}
209213
}
214+
215+
if (changeApplied) {
216+
if (newDoc != null) {
217+
newDocumentSet = newDocumentSet.add(newDoc);
218+
if (newDoc.hasLocalMutations()) {
219+
newMutatedKeys = newMutatedKeys.insert(newDoc.getKey());
220+
} else {
221+
newMutatedKeys = newMutatedKeys.remove(newDoc.getKey());
222+
}
223+
} else {
224+
newDocumentSet = newDocumentSet.remove(key);
225+
newMutatedKeys = newMutatedKeys.remove(key);
226+
}
227+
}
210228
}
211229

212230
if (query.hasLimit()) {
213-
// TODO: Make QuerySnapshot size be constant time.
214-
while (newDocumentSet.size() > query.getLimit()) {
231+
for (long i = newDocumentSet.size() - this.query.getLimit(); i > 0; --i) {
215232
Document oldDoc = newDocumentSet.getLastDocument();
216233
newDocumentSet = newDocumentSet.remove(oldDoc.getKey());
217234
newMutatedKeys = newMutatedKeys.remove(oldDoc.getKey());
@@ -226,6 +243,18 @@ public <D extends MaybeDocument> DocumentChanges computeDocChanges(
226243
return new DocumentChanges(newDocumentSet, changeSet, newMutatedKeys, needsRefill);
227244
}
228245

246+
private boolean shouldWaitForSyncedDocument(Document oldDoc, Document newDoc) {
247+
// We suppress the initial change event for documents that were modified as part of a write
248+
// acknowledgment (e.g. when the value of a server transform is applied) as Watch will send us
249+
// the same document again. By suppressing the event, we only raise two user visible events (one
250+
// with `hasPendingWrites` and the final state of the document) instead of three (one with
251+
// `hasPendingWrites`, the modified document with `hasPendingWrites` and the final state of the
252+
// document).
253+
return (oldDoc.hasLocalMutations()
254+
&& newDoc.hasCommittedMutations()
255+
&& !newDoc.hasLocalMutations());
256+
}
257+
229258
/**
230259
* Updates the view with the given ViewDocumentChanges and updates limbo docs and sync state from
231260
* the given (optional) target change.
@@ -273,15 +302,14 @@ public ViewChange applyChanges(DocumentChanges docChanges, TargetChange targetCh
273302
ViewSnapshot snapshot = null;
274303
if (viewChanges.size() != 0 || syncStatedChanged) {
275304
boolean fromCache = newSyncState == SyncState.LOCAL;
276-
boolean hasPendingWrites = !docChanges.mutatedKeys.isEmpty();
277305
snapshot =
278306
new ViewSnapshot(
279307
query,
280308
docChanges.documentSet,
281309
oldDocumentSet,
282310
viewChanges,
283311
fromCache,
284-
hasPendingWrites,
312+
docChanges.mutatedKeys,
285313
syncStatedChanged);
286314
}
287315
return new ViewChange(snapshot, limboDocumentChanges);

‎firebase-firestore/src/main/java/com/google/firebase/firestore/core/ViewSnapshot.java

Lines changed: 16 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,8 @@
1414

1515
package com.google.firebase.firestore.core;
1616

17+
import com.google.firebase.database.collection.ImmutableSortedSet;
18+
import com.google.firebase.firestore.model.DocumentKey;
1719
import com.google.firebase.firestore.model.DocumentSet;
1820
import java.util.List;
1921

@@ -31,7 +33,7 @@ public enum SyncState {
3133
private final DocumentSet oldDocuments;
3234
private final List<DocumentViewChange> changes;
3335
private final boolean isFromCache;
34-
private final boolean hasPendingWrites;
36+
private final ImmutableSortedSet<DocumentKey> mutatedKeys;
3537
private final boolean didSyncStateChange;
3638

3739
public ViewSnapshot(
@@ -40,14 +42,14 @@ public ViewSnapshot(
4042
DocumentSet oldDocuments,
4143
List<DocumentViewChange> changes,
4244
boolean isFromCache,
43-
boolean hasPendingWrites,
45+
ImmutableSortedSet<DocumentKey> mutatedKeys,
4446
boolean didSyncStateChange) {
4547
this.query = query;
4648
this.documents = documents;
4749
this.oldDocuments = oldDocuments;
4850
this.changes = changes;
4951
this.isFromCache = isFromCache;
50-
this.hasPendingWrites = hasPendingWrites;
52+
this.mutatedKeys = mutatedKeys;
5153
this.didSyncStateChange = didSyncStateChange;
5254
}
5355

@@ -72,7 +74,11 @@ public boolean isFromCache() {
7274
}
7375

7476
public boolean hasPendingWrites() {
75-
return hasPendingWrites;
77+
return !mutatedKeys.isEmpty();
78+
}
79+
80+
public ImmutableSortedSet<DocumentKey> getMutatedKeys() {
81+
return mutatedKeys;
7682
}
7783

7884
public boolean didSyncStateChange() {
@@ -93,15 +99,15 @@ public boolean equals(Object o) {
9399
if (isFromCache != that.isFromCache) {
94100
return false;
95101
}
96-
if (hasPendingWrites != that.hasPendingWrites) {
97-
return false;
98-
}
99102
if (didSyncStateChange != that.didSyncStateChange) {
100103
return false;
101104
}
102105
if (!query.equals(that.query)) {
103106
return false;
104107
}
108+
if (!mutatedKeys.equals(that.mutatedKeys)) {
109+
return false;
110+
}
105111
if (!documents.equals(that.documents)) {
106112
return false;
107113
}
@@ -117,8 +123,8 @@ public int hashCode() {
117123
result = 31 * result + documents.hashCode();
118124
result = 31 * result + oldDocuments.hashCode();
119125
result = 31 * result + changes.hashCode();
126+
result = 31 * result + mutatedKeys.hashCode();
120127
result = 31 * result + (isFromCache ? 1 : 0);
121-
result = 31 * result + (hasPendingWrites ? 1 : 0);
122128
result = 31 * result + (didSyncStateChange ? 1 : 0);
123129
return result;
124130
}
@@ -135,8 +141,8 @@ public String toString() {
135141
+ changes
136142
+ ", isFromCache="
137143
+ isFromCache
138-
+ ", hasPendingWrites="
139-
+ hasPendingWrites
144+
+ ", mutatedKeys="
145+
+ mutatedKeys.size()
140146
+ ", didSyncStateChange="
141147
+ didSyncStateChange
142148
+ ")";

0 commit comments

Comments
 (0)