Skip to content

Commit 32def67

Browse files
wu-huiwu-hui
committed
All tests pass
1 parent a88a8ba commit 32def67

File tree

15 files changed

+70
-51
lines changed

15 files changed

+70
-51
lines changed

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

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -84,7 +84,7 @@ public static SetOptions mergeFields(@NonNull List<String> fields) {
8484
fieldPaths.add(FieldPath.fromDotSeparatedPath(field).getInternalPath());
8585
}
8686

87-
return new SetOptions(true, FieldMask.someFieldsMask(fieldPaths));
87+
return new SetOptions(true, FieldMask.fromSet(fieldPaths));
8888
}
8989

9090
/**
@@ -105,7 +105,7 @@ public static SetOptions mergeFields(String... fields) {
105105
fieldPaths.add(FieldPath.fromDotSeparatedPath(field).getInternalPath());
106106
}
107107

108-
return new SetOptions(true, FieldMask.someFieldsMask(fieldPaths));
108+
return new SetOptions(true, FieldMask.fromSet(fieldPaths));
109109
}
110110

111111
/**
@@ -125,7 +125,7 @@ public static SetOptions mergeFieldPaths(@NonNull List<FieldPath> fields) {
125125
fieldPaths.add(field.getInternalPath());
126126
}
127127

128-
return new SetOptions(true, FieldMask.someFieldsMask(fieldPaths));
128+
return new SetOptions(true, FieldMask.fromSet(fieldPaths));
129129
}
130130

131131
@Override

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -138,7 +138,7 @@ void addToFieldTransforms(FieldPath fieldPath, TransformOperation transformOpera
138138
*/
139139
public ParsedSetData toMergeData(ObjectValue data) {
140140
return new ParsedSetData(
141-
data, FieldMask.someFieldsMask(fieldMask), unmodifiableList(fieldTransforms));
141+
data, FieldMask.fromSet(fieldMask), unmodifiableList(fieldTransforms));
142142
}
143143

144144
/**
@@ -184,7 +184,7 @@ public ParsedSetData toSetData(ObjectValue data) {
184184
*/
185185
public ParsedUpdateData toUpdateData(ObjectValue data) {
186186
return new ParsedUpdateData(
187-
data, FieldMask.someFieldsMask(fieldMask), unmodifiableList(fieldTransforms));
187+
data, FieldMask.fromSet(fieldMask), unmodifiableList(fieldTransforms));
188188
}
189189
}
190190

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

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,6 @@
1818
import static com.google.firebase.firestore.util.Assert.hardAssert;
1919

2020
import androidx.annotation.VisibleForTesting;
21-
2221
import com.google.firebase.database.collection.ImmutableSortedMap;
2322
import com.google.firebase.firestore.core.Query;
2423
import com.google.firebase.firestore.model.Document;
@@ -30,7 +29,6 @@
3029
import com.google.firebase.firestore.model.mutation.Mutation;
3130
import com.google.firebase.firestore.model.mutation.MutationBatch;
3231
import com.google.firebase.firestore.model.mutation.PatchMutation;
33-
3432
import java.util.HashSet;
3533
import java.util.List;
3634
import java.util.Map;
@@ -213,7 +211,8 @@ private ImmutableSortedMap<DocumentKey, Document> getDocumentsMatchingCollection
213211
remoteDocuments = remoteDocuments.insert(key, document);
214212
}
215213
// TODO(Overlay): Here we should be reading squashed mutation and apply that instead.
216-
mutation.applyToLocalView(document, batch.getLocalWriteTime(), FieldMask.someFieldsMask(new HashSet<>()));
214+
mutation.applyToLocalView(
215+
document, batch.getLocalWriteTime(), FieldMask.fromSet(new HashSet<>()));
217216
if (!document.isFoundDocument()) {
218217
remoteDocuments = remoteDocuments.remove(key);
219218
}

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

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -89,7 +89,7 @@ private FieldMask extractFieldMask(MapValue value) {
8989
fields.add(currentPath);
9090
}
9191
}
92-
return FieldMask.someFieldsMask(fields);
92+
return FieldMask.fromSet(fields);
9393
}
9494

9595
/**
@@ -123,8 +123,6 @@ private Value extractNestedValue(Value value, FieldPath fieldPath) {
123123
* <p>This method applies any outstanding modifications and memoizes the result. Further
124124
* invocations are based on this memoized result.
125125
*/
126-
// TODO(Overlay): This might become a hot spot with Mutation.getFieldUpdate introduction, maybe
127-
// we can memorize result here.
128126
private Value buildProto() {
129127
MapValue mergedResult = applyOverlay(FieldPath.EMPTY_PATH, overlayMap);
130128
if (mergedResult != null) {

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

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -70,7 +70,8 @@ public void applyToRemoteDocument(MutableDocument document, MutationResult mutat
7070
}
7171

7272
@Override
73-
public FieldMask applyToLocalView(MutableDocument document, Timestamp localWriteTime, FieldMask mask) {
73+
public FieldMask applyToLocalView(
74+
MutableDocument document, Timestamp localWriteTime, FieldMask mask) {
7475
verifyKeyMatches(document);
7576

7677
if (getPrecondition().isValidFor(document)) {

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

Lines changed: 15 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,6 @@
1515
package com.google.firebase.firestore.model.mutation;
1616

1717
import com.google.firebase.firestore.model.FieldPath;
18-
1918
import java.util.HashSet;
2019
import java.util.Set;
2120

@@ -28,25 +27,27 @@
2827
* object foo. If foo is not an object, foo is replaced with an object containing foo.
2928
*/
3029
public final class FieldMask {
31-
public static FieldMask someFieldsMask(Set<FieldPath> mask) {
32-
return new FieldMask(mask, Scope.Some);
30+
public static FieldMask fromSet(Set<FieldPath> mask) {
31+
return new FieldMask(mask, Scope.BY_MASK);
3332
}
3433

3534
public static FieldMask allFieldsMask() {
36-
return new FieldMask(new HashSet<FieldPath>(), Scope.All_Fields);
35+
return new FieldMask(new HashSet<FieldPath>(), Scope.ALL_FIELDS);
3736
}
3837

3938
public static FieldMask emptyMask() {
40-
return new FieldMask(new HashSet<FieldPath>(), Scope.None);
39+
return new FieldMask(new HashSet<FieldPath>(), Scope.NONE);
4140
}
4241

4342
private final Set<FieldPath> mask;
44-
private enum Scope{
45-
All_Fields,
46-
None,
47-
Some
43+
44+
private enum Scope {
45+
ALL_FIELDS,
46+
NONE,
47+
BY_MASK
4848
}
49-
private Scope scope = Scope.None;
49+
50+
private Scope scope = Scope.NONE;
5051

5152
private FieldMask(Set<FieldPath> mask, Scope scope) {
5253
this.mask = mask;
@@ -96,14 +97,14 @@ public Set<FieldPath> getMask() {
9697
}
9798

9899
public boolean isAllFields() {
99-
return scope == Scope.All_Fields;
100+
return scope == Scope.ALL_FIELDS;
100101
}
101102

102103
public boolean isNoneFields() {
103-
return scope == Scope.None;
104+
return scope == Scope.NONE;
104105
}
105106

106-
public boolean isSomeFields() {
107-
return scope == Scope.Some;
107+
public boolean isByMask() {
108+
return scope == Scope.BY_MASK;
108109
}
109110
}

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

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -116,7 +116,8 @@ public abstract void applyToRemoteDocument(
116116
* @param mask The fields that have been updated before applying this mutation.
117117
* @return A {@code FieldMask} representing the fields that are changed by applying this mutation.
118118
*/
119-
public abstract FieldMask applyToLocalView(MutableDocument document, Timestamp localWriteTime, FieldMask mask);
119+
public abstract FieldMask applyToLocalView(
120+
MutableDocument document, Timestamp localWriteTime, FieldMask mask);
120121

121122
/** Helper for derived classes to implement .equals(). */
122123
boolean hasSameKeyAndPrecondition(Mutation other) {

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -99,7 +99,7 @@ public void applyToRemoteDocument(MutableDocument document, MutationBatchResult
9999

100100
/** Computes the local view of a document given all the mutations in this batch. */
101101
public void applyToLocalView(MutableDocument document) {
102-
FieldMask mutatedFields = FieldMask.someFieldsMask(new HashSet<>());
102+
FieldMask mutatedFields = FieldMask.fromSet(new HashSet<>());
103103
// First, apply the base state. This allows us to apply non-idempotent transform against a
104104
// consistent set of values.
105105
for (int i = 0; i < baseMutations.size(); i++) {

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

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -127,7 +127,8 @@ public void applyToRemoteDocument(MutableDocument document, MutationResult mutat
127127
}
128128

129129
@Override
130-
public FieldMask applyToLocalView(MutableDocument document, Timestamp localWriteTime, FieldMask mask) {
130+
public FieldMask applyToLocalView(
131+
MutableDocument document, Timestamp localWriteTime, FieldMask mask) {
131132
verifyKeyMatches(document);
132133

133134
if (!getPrecondition().isValidFor(document)) {
@@ -143,14 +144,14 @@ public FieldMask applyToLocalView(MutableDocument document, Timestamp localWrite
143144
.convertToFoundDocument(getPostMutationVersion(document), document.getData())
144145
.setHasLocalMutations();
145146

146-
if(mask.isAllFields()) {
147+
if (mask.isAllFields()) {
147148
return mask;
148149
}
149150

150151
HashSet<FieldPath> mergedMaskSet = new HashSet<>(mask.getMask());
151152
mergedMaskSet.addAll(this.mask.getMask());
152153
mergedMaskSet.addAll(getFieldTransformPaths());
153-
return FieldMask.someFieldsMask(mergedMaskSet);
154+
return FieldMask.fromSet(mergedMaskSet);
154155
}
155156

156157
private Map<FieldPath, Value> getPatch() {

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

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -87,7 +87,8 @@ public void applyToRemoteDocument(MutableDocument document, MutationResult mutat
8787
}
8888

8989
@Override
90-
public FieldMask applyToLocalView(MutableDocument document, Timestamp localWriteTime, FieldMask mask) {
90+
public FieldMask applyToLocalView(
91+
MutableDocument document, Timestamp localWriteTime, FieldMask mask) {
9192
verifyKeyMatches(document);
9293

9394
if (!this.getPrecondition().isValidFor(document)) {

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,6 @@
1717
import androidx.annotation.Nullable;
1818
import com.google.firebase.Timestamp;
1919
import com.google.firebase.firestore.model.DocumentKey;
20-
import com.google.firebase.firestore.model.FieldPath;
2120
import com.google.firebase.firestore.model.MutableDocument;
2221
import com.google.firebase.firestore.model.ObjectValue;
2322
import com.google.firebase.firestore.util.Assert;
@@ -64,7 +63,8 @@ public void applyToRemoteDocument(MutableDocument document, MutationResult mutat
6463
}
6564

6665
@Override
67-
public FieldMask applyToLocalView(MutableDocument document, Timestamp localWriteTime, FieldMask mask) {
66+
public FieldMask applyToLocalView(
67+
MutableDocument document, Timestamp localWriteTime, FieldMask mask) {
6868
throw Assert.fail("VerifyMutation should only be used in Transactions.");
6969
}
7070

‎firebase-firestore/src/main/java/com/google/firebase/firestore/remote/RemoteSerializer.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -368,7 +368,7 @@ private FieldMask decodeDocumentMask(DocumentMask mask) {
368368
for (int i = 0; i < count; i++) {
369369
paths.add(FieldPath.fromServerFormat(mask.getFieldPaths(i)));
370370
}
371-
return FieldMask.someFieldsMask(paths);
371+
return FieldMask.fromSet(paths);
372372
}
373373

374374
private DocumentTransform.FieldTransform encodeFieldTransform(FieldTransform fieldTransform) {

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -259,7 +259,7 @@ public void testEncodesMutationBatch() {
259259
new PatchMutation(
260260
key("foo/bar"),
261261
TestUtil.wrapObject(map("a", "b")),
262-
FieldMask.someFieldsMask(Collections.singleton(field("a"))),
262+
FieldMask.fromSet(Collections.singleton(field("a"))),
263263
com.google.firebase.firestore.model.mutation.Precondition.NONE);
264264
Mutation set = setMutation("foo/bar", map("a", "b", "num", 1));
265265
Mutation patch =

‎firebase-firestore/src/test/java/com/google/firebase/firestore/model/MutationSquashTest.java

Lines changed: 30 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,6 @@
2626
import static org.junit.Assert.assertEquals;
2727

2828
import androidx.annotation.Nullable;
29-
3029
import com.google.common.collect.Lists;
3130
import com.google.common.collect.Sets;
3231
import com.google.firebase.Timestamp;
@@ -38,9 +37,8 @@
3837
import com.google.firebase.firestore.model.mutation.Precondition;
3938
import com.google.firebase.firestore.model.mutation.SetMutation;
4039
import com.google.firestore.v1.Value;
41-
4240
import java.util.Arrays;
43-
import java.util.HashMap;
41+
import java.util.HashSet;
4442
import java.util.List;
4543
import java.util.Map;
4644
import java.util.Set;
@@ -407,30 +405,49 @@ private void squashRoundTrips(MutableDocument doc, Mutation... mutations) {
407405

408406
Mutation squashed = getSquashedMutation(doc, mask);
409407

410-
if(squashed != null) {
408+
if (squashed != null) {
411409
squashed.applyToLocalView(toApplySquashedMutation, now, FieldMask.emptyMask());
412410
}
413411

414412
assertEquals(doc, toApplySquashedMutation);
415413
}
416414

417415
@Nullable
416+
// TODO(Overlay): This is production code, find a place for this.
418417
private Mutation getSquashedMutation(MutableDocument doc, FieldMask mask) {
419418
Mutation squashed = null;
420-
if(mask.isAllFields()) {
421-
if(doc.isNoDocument()) {
419+
if (mask.isAllFields()) {
420+
if (doc.isNoDocument()) {
422421
squashed = new DeleteMutation(doc.getKey(), Precondition.NONE);
423422
} else {
424423
squashed = new SetMutation(doc.getKey(), doc.getData(), Precondition.NONE);
425424
}
426-
} else if (mask.isSomeFields()){
427-
HashMap<FieldPath, Value> value = new HashMap<>();
428-
for(FieldPath path: mask.getMask()) {
429-
value.put(path, doc.getData().get(path));
430-
}
425+
} else if (mask.isByMask()) {
426+
ObjectValue docValue = doc.getData();
431427
ObjectValue objectValue = new ObjectValue();
432-
objectValue.setAll(value);
433-
squashed = new PatchMutation(doc.getKey(), doc.getData(), mask, Precondition.NONE);
428+
HashSet<FieldPath> maskSet = new HashSet<>();
429+
for (FieldPath path : mask.getMask()) {
430+
if (!maskSet.contains(path)) {
431+
Value value = docValue.get(path);
432+
// If it is deleting a nested field, we take the immediate parent as the mask used to
433+
// construct resulting mutation.
434+
// Justification: Nested fields can create parent fields implicitly, if they are
435+
// deleted in later mutations, there is no trace that the empty parent fields should
436+
// still remain (which is expected).
437+
// Consider mutation (foo.bar 1), then mutation (foo.bar delete()).
438+
// This leaves the final result (foo, {}). Despite `doc` has the correct result,
439+
// `foo` is not in `mask`, and the resulting mutation will miss `foo` completely without
440+
// this logic.
441+
if (value == null && path.length() > 1) {
442+
path = path.popLast();
443+
}
444+
objectValue.set(path, docValue.get(path));
445+
maskSet.add(path);
446+
}
447+
}
448+
squashed =
449+
new PatchMutation(
450+
doc.getKey(), objectValue, FieldMask.fromSet(maskSet), Precondition.NONE);
434451
}
435452
return squashed;
436453
}

‎firebase-firestore/src/testUtil/java/com/google/firebase/firestore/testutil/TestUtil.java

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -125,7 +125,7 @@ public static FieldMask fieldMask(String... fields) {
125125
for (int i = 0; i < fields.length; i++) {
126126
mask[i] = field(fields[i]);
127127
}
128-
return FieldMask.someFieldsMask(new HashSet<>(Arrays.asList(mask)));
128+
return FieldMask.fromSet(new HashSet<>(Arrays.asList(mask)));
129129
}
130130

131131
public static final Map<String, Object> EMPTY_MAP = new HashMap<>();
@@ -530,7 +530,7 @@ private static PatchMutation patchMutationHelper(
530530
return new PatchMutation(
531531
key(path),
532532
parsed.getData(),
533-
FieldMask.someFieldsMask(fieldMaskPaths),
533+
FieldMask.fromSet(fieldMaskPaths),
534534
precondition,
535535
fieldTransforms);
536536
}

0 commit comments

Comments
 (0)