Skip to content

Commit e2fddb0

Browse files
wu-huiwu-hui
committed
Fix overlay bug with multiple field patches
1 parent 21d351e commit e2fddb0

File tree

7 files changed

+150
-17
lines changed

7 files changed

+150
-17
lines changed

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

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -920,4 +920,56 @@ public void testMultipleUpdatesWhileOffline() {
920920
QuerySnapshot snapshot2 = waitFor(collection.get(Source.CACHE));
921921
assertEquals(asList(map("foo", "zzyzx", "bar", "2")), querySnapshotToValues(snapshot2));
922922
}
923+
924+
@Test
925+
public void testMultipleUpdatesWhileOfflineOnRemoteDoc() {
926+
CollectionReference collection = testCollection();
927+
DocumentReference doc = collection.document();
928+
doc.set(map("likes", 0, "stars", 0));
929+
930+
collection.getFirestore().disableNetwork();
931+
932+
doc.update("likes", 1);
933+
QuerySnapshot snapshot1 = waitFor(collection.get(Source.CACHE));
934+
List result1 = querySnapshotToValues(snapshot1);
935+
936+
doc.update("stars", 1);
937+
snapshot1 = waitFor(collection.get(Source.CACHE));
938+
List result2 = querySnapshotToValues(snapshot1);
939+
940+
doc.update("stars", 2);
941+
snapshot1 = waitFor(collection.get(Source.CACHE));
942+
List result3 = querySnapshotToValues(snapshot1);
943+
944+
result3.contains(0);
945+
}
946+
947+
@Test
948+
public void testListenToMultipleUpdatesWhileOffline() {
949+
CollectionReference collection = testCollection();
950+
collection.addSnapshotListener(
951+
(snap, e) -> {
952+
List result = querySnapshotToValues(snap);
953+
result.contains(0);
954+
});
955+
DocumentReference doc = collection.document();
956+
doc.set(map("likes", 0, "stars", 0));
957+
waitFor(doc.set(map("likes", 0, "stars", 0)));
958+
959+
collection.getFirestore().disableNetwork();
960+
961+
doc.update("likes", 1);
962+
QuerySnapshot snapshot1 = waitFor(collection.get(Source.CACHE));
963+
List result1 = querySnapshotToValues(snapshot1);
964+
965+
doc.update("stars", 1);
966+
snapshot1 = waitFor(collection.get(Source.CACHE));
967+
List result2 = querySnapshotToValues(snapshot1);
968+
969+
doc.update("stars", 2);
970+
snapshot1 = waitFor(collection.get(Source.CACHE));
971+
List result3 = querySnapshotToValues(snapshot1);
972+
973+
result3.contains(0);
974+
}
923975
}

‎firebase-firestore/src/main/java/com/google/firebase/firestore/local/LocalDocumentsResult.java

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

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

17+
import static com.google.firebase.firestore.model.DocumentCollections.emptyDocumentMap;
18+
1719
import com.google.firebase.database.collection.ImmutableSortedMap;
1820
import com.google.firebase.firestore.model.Document;
1921
import com.google.firebase.firestore.model.DocumentKey;
22+
import java.util.Map;
2023

2124
/**
2225
* Represents a set of document along with their mutation batch ID.
@@ -33,6 +36,16 @@ public final class LocalDocumentsResult {
3336
this.documents = documents;
3437
}
3538

39+
public static LocalDocumentsResult fromOverlayedDocuments(
40+
int batchId, ImmutableSortedMap<DocumentKey, OverlayedDocument> overlays) {
41+
ImmutableSortedMap<DocumentKey, Document> documents = emptyDocumentMap();
42+
for (Map.Entry<DocumentKey, OverlayedDocument> entry : overlays) {
43+
documents = documents.insert(entry.getKey(), entry.getValue().getOverlay());
44+
}
45+
46+
return new LocalDocumentsResult(batchId, documents);
47+
}
48+
3649
public int getBatchId() {
3750
return batchId;
3851
}

‎firebase-firestore/src/main/java/com/google/firebase/firestore/local/LocalDocumentsView.java

Lines changed: 26 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -119,7 +119,13 @@ ImmutableSortedMap<DocumentKey, Document> getLocalViewOfDocuments(
119119
Map<DocumentKey, MutableDocument> docs, Set<DocumentKey> existenceStateChanged) {
120120
Map<DocumentKey, Overlay> overlays = new HashMap<>();
121121
populateOverlays(overlays, docs.keySet());
122-
return computeViews(docs, overlays, existenceStateChanged);
122+
ImmutableSortedMap<DocumentKey, Document> result = emptyDocumentMap();
123+
for (Map.Entry<DocumentKey, OverlayedDocument> entry :
124+
computeViews(docs, overlays, existenceStateChanged)) {
125+
result = result.insert(entry.getKey(), entry.getValue().getOverlay());
126+
}
127+
128+
return result;
123129
}
124130

125131
/**
@@ -128,20 +134,22 @@ ImmutableSortedMap<DocumentKey, Document> getLocalViewOfDocuments(
128134
*
129135
* @param docs The documents to apply local mutations to get the local views.
130136
*/
131-
ImmutableSortedMap<DocumentKey, Document> getLocalViewOfDocuments(
137+
ImmutableSortedMap<DocumentKey, OverlayedDocument> getLocalViewOfDocuments(
132138
Map<DocumentKey, MutableDocument> docs) {
133139
Map<DocumentKey, Overlay> overlays = new HashMap<>();
134140
populateOverlays(overlays, docs.keySet());
135141
return computeViews(docs, overlays, new HashSet<>());
136142
}
137143

138144
/*Computes the local view for doc */
139-
private ImmutableSortedMap<DocumentKey, Document> computeViews(
145+
private ImmutableSortedMap<DocumentKey, OverlayedDocument> computeViews(
140146
Map<DocumentKey, MutableDocument> docs,
141147
Map<DocumentKey, Overlay> overlays,
142148
Set<DocumentKey> existenceStateChanged) {
143-
ImmutableSortedMap<DocumentKey, Document> results = emptyDocumentMap();
149+
ImmutableSortedMap<DocumentKey, OverlayedDocument> results =
150+
ImmutableSortedMap.Builder.emptyMap(DocumentKey.comparator());
144151
Map<DocumentKey, MutableDocument> recalculateDocuments = new HashMap<>();
152+
Map<DocumentKey, FieldMask> mutatedFields = new HashMap<>();
145153
for (MutableDocument doc : docs.values()) {
146154
Overlay overlay = overlays.get(doc.getKey());
147155
// Recalculate an overlay if the document's existence state is changed due to a remote
@@ -154,19 +162,26 @@ private ImmutableSortedMap<DocumentKey, Document> computeViews(
154162
&& (overlay == null || overlay.getMutation() instanceof PatchMutation)) {
155163
recalculateDocuments.put(doc.getKey(), doc);
156164
} else if (overlay != null) {
165+
mutatedFields.put(doc.getKey(), overlay.getFieldMask());
157166
overlay.getMutation().applyToLocalView(doc, null, Timestamp.now());
158167
}
159168
}
160169

161-
recalculateAndSaveOverlays(recalculateDocuments);
170+
Map<DocumentKey, FieldMask> recalculatedFields =
171+
recalculateAndSaveOverlays(recalculateDocuments);
172+
mutatedFields.putAll(recalculatedFields);
162173

163174
for (Map.Entry<DocumentKey, MutableDocument> entry : docs.entrySet()) {
164-
results = results.insert(entry.getKey(), entry.getValue());
175+
results =
176+
results.insert(
177+
entry.getKey(),
178+
new OverlayedDocument(entry.getValue(), mutatedFields.get(entry.getKey())));
165179
}
166180
return results;
167181
}
168182

169-
private void recalculateAndSaveOverlays(Map<DocumentKey, MutableDocument> docs) {
183+
private Map<DocumentKey, FieldMask> recalculateAndSaveOverlays(
184+
Map<DocumentKey, MutableDocument> docs) {
170185
List<MutationBatch> batches =
171186
mutationQueue.getAllMutationBatchesAffectingDocumentKeys(docs.keySet());
172187

@@ -211,6 +226,8 @@ private void recalculateAndSaveOverlays(Map<DocumentKey, MutableDocument> docs)
211226
}
212227
documentOverlayCache.saveOverlays(entry.getKey(), overlays);
213228
}
229+
230+
return masks;
214231
}
215232

216233
/**
@@ -311,9 +328,9 @@ LocalDocumentsResult getNextDocuments(String collectionGroup, IndexOffset offset
311328
}
312329

313330
populateOverlays(overlays, docs.keySet());
314-
ImmutableSortedMap<DocumentKey, Document> localDocs =
331+
ImmutableSortedMap<DocumentKey, OverlayedDocument> localDocs =
315332
computeViews(docs, overlays, Collections.emptySet());
316-
return new LocalDocumentsResult(largestBatchId, localDocs);
333+
return LocalDocumentsResult.fromOverlayedDocuments(largestBatchId, localDocs);
317334
}
318335

319336
/**

‎firebase-firestore/src/main/java/com/google/firebase/firestore/local/LocalStore.java

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -258,7 +258,7 @@ public LocalDocumentsResult writeLocally(List<Mutation> mutations) {
258258
}
259259
// Load and apply all existing mutations. This lets us compute the current base state for
260260
// all non-idempotent transforms before applying any additional user-provided writes.
261-
ImmutableSortedMap<DocumentKey, Document> documents =
261+
ImmutableSortedMap<DocumentKey, OverlayedDocument> overlayedDocuments =
262262
localDocuments.getLocalViewOfDocuments(remoteDocs);
263263

264264
// For non-idempotent mutations (such as `FieldValue.increment()`), we record the base
@@ -268,7 +268,8 @@ public LocalDocumentsResult writeLocally(List<Mutation> mutations) {
268268
List<Mutation> baseMutations = new ArrayList<>();
269269
for (Mutation mutation : mutations) {
270270
ObjectValue baseValue =
271-
mutation.extractTransformBaseValue(documents.get(mutation.getKey()));
271+
mutation.extractTransformBaseValue(
272+
overlayedDocuments.get(mutation.getKey()).getOverlay());
272273
if (baseValue != null) {
273274
// NOTE: The base state should only be applied if there's some existing
274275
// document to override, so use a Precondition of exists=true
@@ -284,9 +285,10 @@ public LocalDocumentsResult writeLocally(List<Mutation> mutations) {
284285
MutationBatch batch =
285286
mutationQueue.addMutationBatch(localWriteTime, baseMutations, mutations);
286287
Map<DocumentKey, Mutation> overlays =
287-
batch.applyToLocalDocumentSet(documents, docsWithoutRemoteVersion);
288+
batch.applyToLocalDocumentSet(overlayedDocuments, docsWithoutRemoteVersion);
288289
documentOverlayCache.saveOverlays(batch.getBatchId(), overlays);
289-
return new LocalDocumentsResult(batch.getBatchId(), documents);
290+
return LocalDocumentsResult.fromOverlayedDocuments(
291+
batch.getBatchId(), overlayedDocuments);
290292
});
291293
}
292294

‎firebase-firestore/src/main/java/com/google/firebase/firestore/model/mutation/MutationBatch.java

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,7 @@
1919
import androidx.annotation.Nullable;
2020
import com.google.firebase.Timestamp;
2121
import com.google.firebase.database.collection.ImmutableSortedMap;
22-
import com.google.firebase.firestore.model.Document;
22+
import com.google.firebase.firestore.local.OverlayedDocument;
2323
import com.google.firebase.firestore.model.DocumentKey;
2424
import com.google.firebase.firestore.model.MutableDocument;
2525
import com.google.firebase.firestore.model.SnapshotVersion;
@@ -136,7 +136,7 @@ public FieldMask applyToLocalView(MutableDocument document, @Nullable FieldMask
136136
* applications.
137137
*/
138138
public Map<DocumentKey, Mutation> applyToLocalDocumentSet(
139-
ImmutableSortedMap<DocumentKey, Document> documentMap,
139+
ImmutableSortedMap<DocumentKey, OverlayedDocument> documentMap,
140140
Set<DocumentKey> documentsWithoutRemoteVersion) {
141141
// TODO(mrschmidt): This implementation is O(n^2). If we iterate through the mutations first
142142
// (as done in `applyToLocalView(MutableDocument d)`), we can reduce the complexity to
@@ -145,8 +145,8 @@ public Map<DocumentKey, Mutation> applyToLocalDocumentSet(
145145
for (DocumentKey key : getKeys()) {
146146
// TODO(mutabledocuments): This method should take a map of MutableDocuments and we should
147147
// remove this cast.
148-
MutableDocument document = (MutableDocument) documentMap.get(key);
149-
FieldMask mutatedFields = applyToLocalView(document);
148+
MutableDocument document = (MutableDocument) documentMap.get(key).getOverlay();
149+
FieldMask mutatedFields = applyToLocalView(document, documentMap.get(key).getMutatedFields());
150150
// Set mutationFields to null if the document is only from local mutations, this creates
151151
// a Set(or Delete) mutation, instead of trying to create a patch mutation as the overlay.
152152
mutatedFields = documentsWithoutRemoteVersion.contains(key) ? null : mutatedFields;

‎firebase-firestore/src/main/java/com/google/firebase/firestore/model/mutation/Overlay.java

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -36,4 +36,10 @@ public static Overlay create(int largestBatchId, Mutation mutation) {
3636
public DocumentKey getKey() {
3737
return getMutation().getKey();
3838
}
39+
40+
public FieldMask getFieldMask() {
41+
return getMutation() instanceof PatchMutation
42+
? ((PatchMutation) getMutation()).getMask()
43+
: null;
44+
}
3945
}

‎firebase-firestore/src/test/java/com/google/firebase/firestore/local/LocalStoreTestCase.java

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1616,4 +1616,47 @@ public void testCanHandleBatchAckWhenPendingBatchesHaveOtherDocs() {
16161616
assertContains(doc("foo/bar", 0, map("foo", "bar-set")).setHasLocalMutations());
16171617
assertContains(doc("foo/another", 0, map("foo", "another")).setHasLocalMutations());
16181618
}
1619+
1620+
@Test
1621+
public void testMultipleFieldPatchesOnRemoteDocs() {
1622+
Query query = query("foo");
1623+
allocateQuery(query);
1624+
assertTargetId(2);
1625+
1626+
applyRemoteEvent(
1627+
addedRemoteEvent(doc("foo/bar", 1, map("likes", 0, "stars", 0)), asList(2), emptyList()));
1628+
assertChanged(doc("foo/bar", 1, map("likes", 0, "stars", 0)));
1629+
assertContains(doc("foo/bar", 1, map("likes", 0, "stars", 0)));
1630+
1631+
writeMutation(patchMutation("foo/bar", map("likes", 1)));
1632+
assertChanged(doc("foo/bar", 1, map("likes", 1, "stars", 0)).setHasLocalMutations());
1633+
assertContains(doc("foo/bar", 1, map("likes", 1, "stars", 0)).setHasLocalMutations());
1634+
1635+
writeMutation(patchMutation("foo/bar", map("stars", 1)));
1636+
assertChanged(doc("foo/bar", 1, map("likes", 1, "stars", 1)).setHasLocalMutations());
1637+
assertContains(doc("foo/bar", 1, map("likes", 1, "stars", 1)).setHasLocalMutations());
1638+
1639+
writeMutation(patchMutation("foo/bar", map("stars", 2)));
1640+
assertChanged(doc("foo/bar", 1, map("likes", 1, "stars", 2)).setHasLocalMutations());
1641+
assertContains(doc("foo/bar", 1, map("likes", 1, "stars", 2)).setHasLocalMutations());
1642+
}
1643+
1644+
@Test
1645+
public void testMultipleFieldPatchesOnLocalDocs() {
1646+
writeMutation(setMutation("foo/bar", map("likes", 0, "stars", 0)));
1647+
assertChanged(doc("foo/bar", 0, map("likes", 0, "stars", 0)).setHasLocalMutations());
1648+
assertContains(doc("foo/bar", 0, map("likes", 0, "stars", 0)).setHasLocalMutations());
1649+
1650+
writeMutation(patchMutation("foo/bar", map("likes", 1)));
1651+
assertChanged(doc("foo/bar", 0, map("likes", 1, "stars", 0)).setHasLocalMutations());
1652+
assertContains(doc("foo/bar", 0, map("likes", 1, "stars", 0)).setHasLocalMutations());
1653+
1654+
writeMutation(patchMutation("foo/bar", map("stars", 1)));
1655+
assertChanged(doc("foo/bar", 0, map("likes", 1, "stars", 1)).setHasLocalMutations());
1656+
assertContains(doc("foo/bar", 0, map("likes", 1, "stars", 1)).setHasLocalMutations());
1657+
1658+
writeMutation(patchMutation("foo/bar", map("stars", 2)));
1659+
assertChanged(doc("foo/bar", 0, map("likes", 1, "stars", 2)).setHasLocalMutations());
1660+
assertContains(doc("foo/bar", 0, map("likes", 1, "stars", 2)).setHasLocalMutations());
1661+
}
16191662
}

0 commit comments

Comments
 (0)