Skip to content

Commit 8982700

Browse files
schmidt-sebastianschmidt-sebastian
authored
Remove Held Write Acks (#48)
1 parent a31bf37 commit 8982700

File tree

64 files changed

+66107
-37105
lines changed

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

64 files changed

+66107
-37105
lines changed

‎firebase-firestore/src/androidTest/java/com/google/firebase/firestore/FirestoreTest.java

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -74,6 +74,28 @@ public void testCanUpdateAnExistingDocument() {
7474
assertEquals(finalData, doc.getData());
7575
}
7676

77+
@Test
78+
public void testCanUpdateAnUnknownDocument() {
79+
DocumentReference writerRef = testFirestore().collection("collection").document();
80+
DocumentReference readerRef =
81+
testFirestore().collection("collection").document(writerRef.getId());
82+
waitFor(writerRef.set(map("a", "a")));
83+
waitFor(readerRef.update(map("b", "b")));
84+
DocumentSnapshot writerSnap = waitFor(writerRef.get(Source.CACHE));
85+
assertTrue(writerSnap.exists());
86+
try {
87+
waitFor(readerRef.get(Source.CACHE));
88+
fail("Should have thrown exception");
89+
} catch (RuntimeException e) {
90+
assertEquals(
91+
((FirebaseFirestoreException) e.getCause().getCause()).getCode(), Code.UNAVAILABLE);
92+
}
93+
writerSnap = waitFor(writerRef.get());
94+
assertEquals(map("a", "a", "b", "b"), writerSnap.getData());
95+
DocumentSnapshot readerSnap = waitFor(readerRef.get());
96+
assertEquals(map("a", "a", "b", "b"), readerSnap.getData());
97+
}
98+
7799
@Test
78100
public void testCanMergeDataWithAnExistingDocumentUsingSet() {
79101
DocumentReference documentReference = testCollection("rooms").document("eros");

‎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: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515
package com.google.firebase.firestore;
1616

1717
import static com.google.common.base.Preconditions.checkNotNull;
18+
import static com.google.firebase.firestore.util.Assert.fail;
1819

1920
import android.support.annotation.NonNull;
2021
import android.support.annotation.Nullable;
@@ -26,7 +27,6 @@
2627
import com.google.firebase.firestore.model.Document;
2728
import com.google.firebase.firestore.model.MaybeDocument;
2829
import com.google.firebase.firestore.model.NoDocument;
29-
import com.google.firebase.firestore.util.Assert;
3030
import com.google.firebase.firestore.util.Executors;
3131
import com.google.firebase.firestore.util.Util;
3232
import java.util.Collections;
@@ -237,15 +237,19 @@ private Task<DocumentSnapshot> getAsync(DocumentReference documentRef) {
237237
}
238238
List<MaybeDocument> docs = task.getResult();
239239
if (docs.size() != 1) {
240-
throw Assert.fail("Mismatch in docs returned from document lookup.");
240+
throw fail("Mismatch in docs returned from document lookup.");
241241
}
242242
MaybeDocument doc = docs.get(0);
243-
if (doc instanceof NoDocument) {
243+
if (doc instanceof Document) {
244+
return DocumentSnapshot.fromDocument(
245+
firestore, (Document) doc, /*fromCache=*/ false, /*hasPendingWrites=*/ false);
246+
} else if (doc instanceof NoDocument) {
244247
return DocumentSnapshot.fromNoDocument(
245-
firestore, doc.getKey(), /*fromCache=*/ false);
248+
firestore, doc.getKey(), /*fromCache=*/ false, /*hasPendingWrites=*/ false);
246249
} else {
247-
return DocumentSnapshot.fromDocument(
248-
firestore, (Document) doc, /*fromCache=*/ false);
250+
throw fail(
251+
"BatchGetDocumentsRequest returned unexpected document type: "
252+
+ doc.getClass().getCanonicalName());
249253
}
250254
});
251255
}

‎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/Transaction.java

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -14,12 +14,15 @@
1414

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

17+
import static com.google.firebase.firestore.util.Assert.fail;
18+
1719
import com.google.android.gms.tasks.Task;
1820
import com.google.android.gms.tasks.Tasks;
1921
import com.google.firebase.firestore.FirebaseFirestoreException;
2022
import com.google.firebase.firestore.FirebaseFirestoreException.Code;
2123
import com.google.firebase.firestore.core.UserData.ParsedSetData;
2224
import com.google.firebase.firestore.core.UserData.ParsedUpdateData;
25+
import com.google.firebase.firestore.model.Document;
2326
import com.google.firebase.firestore.model.DocumentKey;
2427
import com.google.firebase.firestore.model.MaybeDocument;
2528
import com.google.firebase.firestore.model.NoDocument;
@@ -55,10 +58,14 @@ public Transaction(Datastore d) {
5558
}
5659

5760
private void recordVersion(MaybeDocument doc) throws FirebaseFirestoreException {
58-
SnapshotVersion docVersion = doc.getVersion();
59-
if (doc instanceof NoDocument) {
61+
SnapshotVersion docVersion;
62+
if (doc instanceof Document) {
63+
docVersion = doc.getVersion();
64+
} else if (doc instanceof NoDocument) {
6065
// For nonexistent docs, we must use precondition with version 0 when we overwrite them.
6166
docVersion = SnapshotVersion.NONE;
67+
} else {
68+
throw fail("Unexpected document type in transaction: " + doc.getClass().getCanonicalName());
6269
}
6370

6471
if (readVersions.containsKey(doc.getKey())) {

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

Lines changed: 50 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -169,51 +169,69 @@ 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());
234+
newMutatedKeys = newMutatedKeys.remove(oldDoc.getKey());
217235
changeSet.addChange(DocumentViewChange.create(Type.REMOVED, oldDoc));
218236
}
219237
}
@@ -225,6 +243,18 @@ public <D extends MaybeDocument> DocumentChanges computeDocChanges(
225243
return new DocumentChanges(newDocumentSet, changeSet, newMutatedKeys, needsRefill);
226244
}
227245

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+
228258
/**
229259
* Updates the view with the given ViewDocumentChanges and updates limbo docs and sync state from
230260
* the given (optional) target change.
@@ -272,15 +302,14 @@ public ViewChange applyChanges(DocumentChanges docChanges, TargetChange targetCh
272302
ViewSnapshot snapshot = null;
273303
if (viewChanges.size() != 0 || syncStatedChanged) {
274304
boolean fromCache = newSyncState == SyncState.LOCAL;
275-
boolean hasPendingWrites = !docChanges.mutatedKeys.isEmpty();
276305
snapshot =
277306
new ViewSnapshot(
278307
query,
279308
docChanges.documentSet,
280309
oldDocumentSet,
281310
viewChanges,
282311
fromCache,
283-
hasPendingWrites,
312+
docChanges.mutatedKeys,
284313
syncStatedChanged);
285314
}
286315
return new ViewChange(snapshot, limboDocumentChanges);

0 commit comments

Comments
 (0)