Skip to content

Commit f54ed81

Browse files
Porting the remainder of Held Write Acks
1 parent 6c67df6 commit f54ed81

28 files changed

+1603
-311
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: 95 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -114,6 +114,52 @@ public <D extends MaybeDocument> DocumentChanges computeDocChanges(
114114
return computeDocChanges(docChanges, null);
115115
}
116116

117+
/**
118+
* Computes the initial set of document changes based on the provided documents.
119+
*
120+
* <p>Unlike `computeDocChanges`, documents with committed mutations don't raise
121+
* `hasPendingWrites`. This distinction allows us to only raise `hasPendingWrite` events for
122+
* documents that changed during the lifetime of the View.
123+
*
124+
* @param docs The docs to apply to this view.
125+
* @return A new set of docs, changes, and refill flag.
126+
*/
127+
public <D extends MaybeDocument> DocumentChanges computeInitialChanges(
128+
ImmutableSortedMap<DocumentKey, D> docs) {
129+
hardAssert(
130+
this.documentSet.isEmpty(), "computeInitialChanges() called when docs are already present");
131+
132+
DocumentViewChangeSet changeSet = new DocumentViewChangeSet();
133+
ImmutableSortedSet<DocumentKey> newMutatedKeys = this.mutatedKeys;
134+
DocumentSet newDocumentSet = this.documentSet;
135+
136+
for (Map.Entry<DocumentKey, ? extends MaybeDocument> entry : docs) {
137+
DocumentKey key = entry.getKey();
138+
MaybeDocument maybeDoc = entry.getValue();
139+
140+
if (maybeDoc instanceof Document) {
141+
Document doc = (Document) maybeDoc;
142+
if (this.query.matches(doc)) {
143+
changeSet.addChange(DocumentViewChange.create(Type.ADDED, doc));
144+
newDocumentSet = newDocumentSet.add(doc);
145+
if (doc.hasLocalMutations()) {
146+
newMutatedKeys = newMutatedKeys.insert(key);
147+
}
148+
}
149+
}
150+
}
151+
152+
if (this.query.hasLimit()) {
153+
for (long i = newDocumentSet.size() - this.query.getLimit(); i > 0; --i) {
154+
Document oldDoc = newDocumentSet.getLastDocument();
155+
newDocumentSet = newDocumentSet.remove(oldDoc.getKey());
156+
newMutatedKeys = newMutatedKeys.remove(oldDoc.getKey());
157+
changeSet.addChange(DocumentViewChange.create(Type.REMOVED, oldDoc));
158+
}
159+
}
160+
return new DocumentChanges(newDocumentSet, changeSet, newMutatedKeys, /*needsRefill=*/ false);
161+
}
162+
117163
/**
118164
* Iterates over a set of doc changes, applies the query limit, and computes what the new results
119165
* should be, what the changes were, and whether we may need to go back to the local cache for
@@ -169,49 +215,66 @@ public <D extends MaybeDocument> DocumentChanges computeDocChanges(
169215
}
170216
}
171217

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-
}
218+
boolean oldDocHadPendingMutations =
219+
oldDoc != null && this.mutatedKeys.contains(oldDoc.getKey());
220+
221+
// We only consider committed mutations for documents that were mutated during the lifetime of
222+
// the view.
223+
boolean newDocHasPendingMutations =
224+
newDoc != null
225+
&& (newDoc.hasLocalMutations()
226+
|| (this.mutatedKeys.contains(newDoc.getKey())
227+
&& newDoc.hasCommittedMutations()));
228+
229+
boolean changeApplied = false;
230+
183231
// Calculate change
184232
if (oldDoc != null && newDoc != null) {
185233
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 {
234+
if (!docsEqual) {
235+
if (!shouldWaitForSyncedDocument(oldDoc, newDoc)) {
191236
changeSet.addChange(DocumentViewChange.create(Type.MODIFIED, newDoc));
237+
changeApplied = true;
192238
}
193-
194239
if (lastDocInLimit != null && query.comparator().compare(newDoc, lastDocInLimit) > 0) {
195240
// This doc moved from inside the limit to after the limit. That means there may be some
196241
// doc in the local cache that's actually less than this one.
197242
needsRefill = true;
198243
}
244+
} else if (oldDocHadPendingMutations != newDocHasPendingMutations) {
245+
changeSet.addChange(DocumentViewChange.create(Type.METADATA, newDoc));
246+
changeApplied = true;
199247
}
200248
} else if (oldDoc == null && newDoc != null) {
201249
changeSet.addChange(DocumentViewChange.create(Type.ADDED, newDoc));
250+
changeApplied = true;
202251
} else if (oldDoc != null && newDoc == null) {
203252
changeSet.addChange(DocumentViewChange.create(Type.REMOVED, oldDoc));
253+
changeApplied = true;
204254
if (lastDocInLimit != null) {
205255
// A doc was removed from a full limit query. We'll need to requery from the local cache
206256
// to see if we know about some other doc that should be in the results.
207257
needsRefill = true;
208258
}
209259
}
260+
261+
if (changeApplied) {
262+
if (newDoc != null) {
263+
newDocumentSet = newDocumentSet.add(newDoc);
264+
if (newDoc.hasLocalMutations()) {
265+
newMutatedKeys = newMutatedKeys.insert(newDoc.getKey());
266+
} else {
267+
newMutatedKeys = newMutatedKeys.remove(newDoc.getKey());
268+
}
269+
} else {
270+
newDocumentSet = newDocumentSet.remove(key);
271+
newMutatedKeys = newMutatedKeys.remove(key);
272+
}
273+
}
210274
}
211275

212276
if (query.hasLimit()) {
213-
// TODO: Make QuerySnapshot size be constant time.
214-
while (newDocumentSet.size() > query.getLimit()) {
277+
for (long i = newDocumentSet.size() - this.query.getLimit(); i > 0; --i) {
215278
Document oldDoc = newDocumentSet.getLastDocument();
216279
newDocumentSet = newDocumentSet.remove(oldDoc.getKey());
217280
newMutatedKeys = newMutatedKeys.remove(oldDoc.getKey());
@@ -226,6 +289,18 @@ public <D extends MaybeDocument> DocumentChanges computeDocChanges(
226289
return new DocumentChanges(newDocumentSet, changeSet, newMutatedKeys, needsRefill);
227290
}
228291

292+
private boolean shouldWaitForSyncedDocument(Document oldDoc, Document newDoc) {
293+
// We suppress the initial change event for documents that were modified as part of a write
294+
// acknowledgment (e.g. when the value of a server transform is applied) as Watch will send us
295+
// the same document again. By suppressing the event, we only raise two user visible events (one
296+
// with `hasPendingWrites` and the final state of the document) instead of three (one with
297+
// `hasPendingWrites`, the modified document with `hasPendingWrites` and the final state of the
298+
// document).
299+
return (oldDoc.hasLocalMutations()
300+
&& newDoc.hasCommittedMutations()
301+
&& !newDoc.hasLocalMutations());
302+
}
303+
229304
/**
230305
* Updates the view with the given ViewDocumentChanges and updates limbo docs and sync state from
231306
* the given (optional) target change.
@@ -273,15 +348,14 @@ public ViewChange applyChanges(DocumentChanges docChanges, TargetChange targetCh
273348
ViewSnapshot snapshot = null;
274349
if (viewChanges.size() != 0 || syncStatedChanged) {
275350
boolean fromCache = newSyncState == SyncState.LOCAL;
276-
boolean hasPendingWrites = !docChanges.mutatedKeys.isEmpty();
277351
snapshot =
278352
new ViewSnapshot(
279353
query,
280354
docChanges.documentSet,
281355
oldDocumentSet,
282356
viewChanges,
283357
fromCache,
284-
hasPendingWrites,
358+
docChanges.mutatedKeys,
285359
syncStatedChanged);
286360
}
287361
return new ViewChange(snapshot, limboDocumentChanges);

0 commit comments

Comments
 (0)