Skip to content

Commit b82f471

Browse files
author
Brian Chen
committed
resolve comments
1 parent 283773f commit b82f471

File tree

10 files changed

+44
-81
lines changed

10 files changed

+44
-81
lines changed

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

Lines changed: 9 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@
2828
import com.google.firebase.firestore.model.UnknownDocument;
2929
import com.google.firebase.firestore.model.mutation.Mutation;
3030
import com.google.firebase.firestore.model.mutation.MutationBatch;
31+
import com.google.firebase.firestore.proto.WriteBatch;
3132
import com.google.firebase.firestore.remote.RemoteSerializer;
3233
import com.google.firestore.v1.DocumentTransform.FieldTransform;
3334
import com.google.firestore.v1.Write;
@@ -179,30 +180,28 @@ MutationBatch decodeMutationBatch(com.google.firebase.firestore.proto.WriteBatch
179180
// representing `transforms` with `update_transforms` on the SDK means that old `transform`
180181
// mutations stored in IndexedDB need to be updated to `update_transforms`.
181182
// TODO(b/174608374): Remove this code once we perform a schema migration.
183+
WriteBatch.Builder squashedBatchBuilder = WriteBatch.newBuilder();
182184
for (int i = batch.getWritesCount() - 1; i >= 0; --i) {
183185
Write mutation = batch.getWrites(i);
184-
if (mutation.getTransform().getFieldTransformsCount() != 0) {
186+
if (mutation.hasTransform()) {
185187
hardAssert(
186-
i >= 1
187-
&& batch.getWrites(i - 1).getTransform().getFieldTransformsCount() == 0
188-
&& batch.getWrites(i - 1).getUpdate().getFieldsCount() != 0,
188+
i >= 1 && !batch.getWrites(i - 1).hasTransform() && batch.getWrites(i - 1).hasUpdate(),
189189
"TransformMutation should be preceded by a patch or set mutation");
190190
Write mutationToJoin = batch.getWrites(i - 1);
191191
Builder newMutationBuilder = Write.newBuilder(mutationToJoin);
192192
for (FieldTransform fieldTransform : mutation.getTransform().getFieldTransformsList()) {
193193
newMutationBuilder.addUpdateTransforms(fieldTransform);
194194
}
195+
squashedBatchBuilder.addWrites(0, newMutationBuilder.build());
195196

196-
batch =
197-
com.google.firebase.firestore.proto.WriteBatch.newBuilder(batch)
198-
.removeWrites(i)
199-
.removeWrites(i - 1)
200-
.addWrites(i - 1, newMutationBuilder.build())
201-
.build();
202197
--i;
198+
} else {
199+
squashedBatchBuilder.addWrites(0, mutation);
203200
}
204201
}
205202

203+
batch = squashedBatchBuilder.build();
204+
206205
int mutationsCount = batch.getWritesCount();
207206
List<Mutation> mutations = new ArrayList<>(mutationsCount);
208207
for (int i = 0; i < mutationsCount; i++) {

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -219,7 +219,7 @@ public LocalWriteResult writeLocally(List<Mutation> mutations) {
219219
List<Mutation> baseMutations = new ArrayList<>();
220220
for (Mutation mutation : mutations) {
221221
ObjectValue baseValue =
222-
mutation.extractBaseValue(existingDocuments.get(mutation.getKey()));
222+
mutation.extractTransformBaseValue(existingDocuments.get(mutation.getKey()));
223223
if (baseValue != null) {
224224
// NOTE: The base state should only be applied if there's some existing
225225
// document to override, so use a Precondition of exists=true

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

Lines changed: 0 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,6 @@
2121
import com.google.firebase.firestore.model.DocumentKey;
2222
import com.google.firebase.firestore.model.MaybeDocument;
2323
import com.google.firebase.firestore.model.NoDocument;
24-
import com.google.firebase.firestore.model.ObjectValue;
2524
import com.google.firebase.firestore.model.SnapshotVersion;
2625

2726
/** Represents a Delete operation */
@@ -83,10 +82,4 @@ public MaybeDocument applyToLocalView(
8382

8483
return new NoDocument(getKey(), SnapshotVersion.NONE, /*hasCommittedMutations=*/ false);
8584
}
86-
87-
@Nullable
88-
@Override
89-
public ObjectValue extractBaseValue(@Nullable MaybeDocument maybeDoc) {
90-
return null;
91-
}
9285
}

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

Lines changed: 3 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -60,7 +60,7 @@
6060
* return an `UnknownDocument` and rely on Watch to send us the updated version.
6161
*
6262
* <p>Field transforms are used only with Patch and Set Mutations. We use the `updateTransforms`
63-
* field to store transforms, rather than the `transforms` field.
63+
* field to store transforms, rather than the `transforms` message.
6464
*/
6565
public abstract class Mutation {
6666
private final DocumentKey key;
@@ -123,25 +123,6 @@ public abstract MaybeDocument applyToRemoteDocument(
123123
public abstract MaybeDocument applyToLocalView(
124124
@Nullable MaybeDocument maybeDoc, @Nullable MaybeDocument baseDoc, Timestamp localWriteTime);
125125

126-
/**
127-
* If applicable, returns the base value to persist with this mutation. If a base value is
128-
* provided, the mutation is always applied to this base value, even if document has already been
129-
* updated.
130-
*
131-
* <p>The base value is a sparse object that consists of only the document fields for which this
132-
* mutation contains a non-idempotent transformation (e.g. a numeric increment). The provided
133-
* value guarantees consistent behavior for non-idempotent transforms and allow us to return the
134-
* same latency-compensated value even if the backend has already applied the mutation. The base
135-
* value is null for idempotent mutations, as they can be re-played even if the backend has
136-
* already applied them.
137-
*
138-
* @return a base value to store along with the mutation, or null for idempotent mutations.
139-
*/
140-
@Nullable
141-
public ObjectValue extractBaseValue(@Nullable MaybeDocument maybeDoc) {
142-
return extractTransformBaseValue(maybeDoc);
143-
}
144-
145126
/** Helper for derived classes to implement .equals(). */
146127
boolean hasSameKeyAndPrecondition(Mutation other) {
147128
return key.equals(other.key) && precondition.equals(other.precondition);
@@ -187,7 +168,7 @@ static SnapshotVersion getPostMutationVersion(@Nullable MaybeDocument maybeDoc)
187168
* @param serverTransformResults The transform results received by the server.
188169
* @return The transform results list.
189170
*/
190-
List<Value> serverTransformResults(
171+
protected List<Value> serverTransformResults(
191172
@Nullable MaybeDocument baseDoc, List<Value> serverTransformResults) {
192173
ArrayList<Value> transformResults = new ArrayList<>(fieldTransforms.size());
193174
hardAssert(
@@ -220,7 +201,7 @@ List<Value> serverTransformResults(
220201
* @param baseDoc The document prior to applying this mutation batch.
221202
* @return The transform results list.
222203
*/
223-
List<Value> localTransformResults(
204+
protected List<Value> localTransformResults(
224205
Timestamp localWriteTime, @Nullable MaybeDocument maybeDoc, @Nullable MaybeDocument baseDoc) {
225206
ArrayList<Value> transformResults = new ArrayList<>(fieldTransforms.size());
226207
for (FieldTransform fieldTransform : fieldTransforms) {

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

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -101,9 +101,8 @@ public MaybeDocument applyToLocalView(
101101
return maybeDoc;
102102
}
103103

104-
ObjectValue newData = value;
105104
List<Value> transformResults = localTransformResults(localWriteTime, maybeDoc, baseDoc);
106-
newData = transformObject(newData, transformResults);
105+
ObjectValue newData = transformObject(value, transformResults);
107106

108107
SnapshotVersion version = getPostMutationVersion(maybeDoc);
109108
return new Document(getKey(), version, newData, Document.DocumentState.LOCAL_MUTATIONS);

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

Lines changed: 0 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,6 @@
1818
import com.google.firebase.Timestamp;
1919
import com.google.firebase.firestore.model.DocumentKey;
2020
import com.google.firebase.firestore.model.MaybeDocument;
21-
import com.google.firebase.firestore.model.ObjectValue;
2221
import com.google.firebase.firestore.util.Assert;
2322

2423
/**
@@ -69,10 +68,4 @@ public MaybeDocument applyToLocalView(
6968
@Nullable MaybeDocument maybeDoc, @Nullable MaybeDocument baseDoc, Timestamp localWriteTime) {
7069
throw Assert.fail("VerifyMutation should only be used in Transactions.");
7170
}
72-
73-
@Nullable
74-
@Override
75-
public ObjectValue extractBaseValue(@Nullable MaybeDocument maybeDoc) {
76-
throw Assert.fail("VerifyMutation should only be used in Transactions.");
77-
}
7871
}

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

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -274,10 +274,8 @@ public com.google.firestore.v1.Write encodeMutation(Mutation mutation) {
274274
throw fail("unknown mutation type %s", mutation.getClass());
275275
}
276276

277-
if (mutation.getFieldTransforms().size() > 0) {
278-
for (FieldTransform fieldTransform : mutation.getFieldTransforms()) {
279-
builder.addUpdateTransforms(encodeFieldTransform(fieldTransform));
280-
}
277+
for (FieldTransform fieldTransform : mutation.getFieldTransforms()) {
278+
builder.addUpdateTransforms(encodeFieldTransform(fieldTransform));
281279
}
282280

283281
if (!mutation.getPrecondition().isNone()) {

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

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -68,7 +68,7 @@ public final class LocalSerializerTest {
6868
private LocalSerializer serializer;
6969

7070
private Timestamp writeTime = Timestamp.now();
71-
com.google.protobuf.Timestamp writeTimeProto =
71+
private com.google.protobuf.Timestamp writeTimeProto =
7272
com.google.protobuf.Timestamp.newBuilder()
7373
.setSeconds(writeTime.getSeconds())
7474
.setNanos(writeTime.getNanoseconds())
@@ -167,9 +167,9 @@ public void testSetMutationAndTransFormMutationAreSquashed() {
167167
MutationBatch decoded = serializer.decodeMutationBatch(batchProto);
168168
assertEquals(1, decoded.getMutations().size());
169169
assertTrue(decoded.getMutations().get(0) instanceof SetMutation);
170-
Write serialized = remoteSerializer.encodeMutation(decoded.getMutations().get(0));
170+
Write encoded = remoteSerializer.encodeMutation(decoded.getMutations().get(0));
171171
Write expected = new TestWriteBuilder().addSet().addUpdateTransforms().build();
172-
assertEquals(expected, serialized);
172+
assertEquals(expected, encoded);
173173
}
174174

175175
// TODO(b/174608374): Remove these tests once we perform a schema migration.
@@ -185,9 +185,9 @@ public void testPatchMutationAndTransFormMutationAreSquashed() {
185185
MutationBatch decoded = serializer.decodeMutationBatch(batchProto);
186186
assertEquals(1, decoded.getMutations().size());
187187
assertTrue(decoded.getMutations().get(0) instanceof PatchMutation);
188-
Write serialized = remoteSerializer.encodeMutation(decoded.getMutations().get(0));
188+
Write encoded = remoteSerializer.encodeMutation(decoded.getMutations().get(0));
189189
Write expected = new TestWriteBuilder().addPatch().addUpdateTransforms().build();
190-
assertEquals(expected, serialized);
190+
assertEquals(expected, encoded);
191191
}
192192

193193
// TODO(b/174608374): Remove these tests once we perform a schema migration.
@@ -260,8 +260,8 @@ public void testMultipleMutationsAreSquashed() {
260260
new TestWriteBuilder().addPatch().build());
261261
for (int i = 0; i < decoded.getMutations().size(); i++) {
262262
Mutation mutation = decoded.getMutations().get(i);
263-
Write serialized = remoteSerializer.encodeMutation(mutation);
264-
assertEquals(allExpected.get(i), serialized);
263+
Write encoded = remoteSerializer.encodeMutation(mutation);
264+
assertEquals(allExpected.get(i), encoded);
265265
}
266266
}
267267

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

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -618,13 +618,13 @@ public void testNonTransformMutationBaseValue() {
618618
Document baseDoc = doc("collection/key", 0, data);
619619

620620
Mutation set = setMutation("collection/key", map("foo", "bar"));
621-
assertNull(set.extractBaseValue(baseDoc));
621+
assertNull(set.extractTransformBaseValue(baseDoc));
622622

623623
Mutation patch = patchMutation("collection/key", map("foo", "bar"));
624-
assertNull(patch.extractBaseValue(baseDoc));
624+
assertNull(patch.extractTransformBaseValue(baseDoc));
625625

626626
Mutation delete = deleteMutation("collection/key");
627-
assertNull(delete.extractBaseValue(baseDoc));
627+
assertNull(delete.extractTransformBaseValue(baseDoc));
628628
}
629629

630630
@Test
@@ -638,7 +638,7 @@ public void testServerTimestampBaseValue() {
638638

639639
// Server timestamps are idempotent and don't have base values.
640640
Mutation mutation = patchMutation("collection/key", allTransforms);
641-
assertNull(mutation.extractBaseValue(baseDoc));
641+
assertNull(mutation.extractTransformBaseValue(baseDoc));
642642
}
643643

644644
@Test
@@ -663,7 +663,7 @@ public void testNumericIncrementBaseValue() {
663663
allTransforms.put("nested", new HashMap<>(allTransforms));
664664

665665
Mutation mutation = patchMutation("collection/key", allTransforms);
666-
ObjectValue baseValue = mutation.extractBaseValue(baseDoc);
666+
ObjectValue baseValue = mutation.extractTransformBaseValue(baseDoc);
667667

668668
Value expected =
669669
wrap(

‎firebase-firestore/src/test/java/com/google/firebase/firestore/remote/RemoteSerializerTest.java

Lines changed: 16 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -380,95 +380,95 @@ public void testEncodesPatchMutationWithFieldMask() {
380380
@Test
381381
public void testEncodesServerTimestampMutation() {
382382
Mutation mutation =
383-
patchMutation(
383+
setMutation(
384384
"docs/1",
385385
map(
386386
"a",
387387
com.google.firebase.firestore.FieldValue.serverTimestamp(),
388-
"bar.baz",
388+
"bar",
389389
com.google.firebase.firestore.FieldValue.serverTimestamp()));
390390
Write expected =
391391
Write.newBuilder()
392392
.setUpdate(Document.newBuilder().setName("projects/p/databases/d/documents/docs/1"))
393-
.setUpdateMask(DocumentMask.newBuilder().build())
394393
.addUpdateTransforms(
395394
FieldTransform.newBuilder()
396395
.setFieldPath("a")
397396
.setSetToServerValue(ServerValue.REQUEST_TIME))
398397
.addUpdateTransforms(
399398
FieldTransform.newBuilder()
400-
.setFieldPath("bar.baz")
399+
.setFieldPath("bar")
401400
.setSetToServerValue(ServerValue.REQUEST_TIME))
402-
.setCurrentDocument(Precondition.newBuilder().setExists(true))
403401
.build();
404402
assertRoundTripForMutation(mutation, expected);
405403

406404
mutation =
407-
setMutation(
405+
patchMutation(
408406
"docs/1",
409407
map(
410408
"a",
411409
com.google.firebase.firestore.FieldValue.serverTimestamp(),
412-
"bar",
410+
"bar.baz",
413411
com.google.firebase.firestore.FieldValue.serverTimestamp()));
414412
expected =
415413
Write.newBuilder()
416414
.setUpdate(Document.newBuilder().setName("projects/p/databases/d/documents/docs/1"))
415+
.setUpdateMask(DocumentMask.newBuilder().build())
417416
.addUpdateTransforms(
418417
FieldTransform.newBuilder()
419418
.setFieldPath("a")
420419
.setSetToServerValue(ServerValue.REQUEST_TIME))
421420
.addUpdateTransforms(
422421
FieldTransform.newBuilder()
423-
.setFieldPath("bar")
422+
.setFieldPath("bar.baz")
424423
.setSetToServerValue(ServerValue.REQUEST_TIME))
424+
.setCurrentDocument(Precondition.newBuilder().setExists(true))
425425
.build();
426426
assertRoundTripForMutation(mutation, expected);
427427
}
428428

429429
@Test
430430
public void testEncodesArrayMutations() {
431431
Mutation mutation =
432-
patchMutation(
432+
setMutation(
433433
"docs/1",
434434
map(
435435
"a", com.google.firebase.firestore.FieldValue.arrayUnion("a", 2),
436-
"bar.baz", com.google.firebase.firestore.FieldValue.arrayRemove(map("x", 1))));
436+
"bar", com.google.firebase.firestore.FieldValue.arrayRemove(map("x", 1))));
437437
Write expected =
438438
Write.newBuilder()
439439
.setUpdate(Document.newBuilder().setName("projects/p/databases/d/documents/docs/1"))
440-
.setUpdateMask(DocumentMask.newBuilder().build())
441440
.addUpdateTransforms(
442441
FieldTransform.newBuilder()
443442
.setFieldPath("a")
444443
.setAppendMissingElements(
445444
ArrayValue.newBuilder().addValues(wrap("a")).addValues(wrap(2))))
446445
.addUpdateTransforms(
447446
FieldTransform.newBuilder()
448-
.setFieldPath("bar.baz")
447+
.setFieldPath("bar")
449448
.setRemoveAllFromArray(ArrayValue.newBuilder().addValues(wrap(map("x", 1)))))
450-
.setCurrentDocument(Precondition.newBuilder().setExists(true))
451449
.build();
452450
assertRoundTripForMutation(mutation, expected);
453451

454452
mutation =
455-
setMutation(
453+
patchMutation(
456454
"docs/1",
457455
map(
458456
"a", com.google.firebase.firestore.FieldValue.arrayUnion("a", 2),
459-
"bar", com.google.firebase.firestore.FieldValue.arrayRemove(map("x", 1))));
457+
"bar.baz", com.google.firebase.firestore.FieldValue.arrayRemove(map("x", 1))));
460458
expected =
461459
Write.newBuilder()
462460
.setUpdate(Document.newBuilder().setName("projects/p/databases/d/documents/docs/1"))
461+
.setUpdateMask(DocumentMask.newBuilder().build())
463462
.addUpdateTransforms(
464463
FieldTransform.newBuilder()
465464
.setFieldPath("a")
466465
.setAppendMissingElements(
467466
ArrayValue.newBuilder().addValues(wrap("a")).addValues(wrap(2))))
468467
.addUpdateTransforms(
469468
FieldTransform.newBuilder()
470-
.setFieldPath("bar")
469+
.setFieldPath("bar.baz")
471470
.setRemoveAllFromArray(ArrayValue.newBuilder().addValues(wrap(map("x", 1)))))
471+
.setCurrentDocument(Precondition.newBuilder().setExists(true))
472472
.build();
473473
assertRoundTripForMutation(mutation, expected);
474474
}

0 commit comments

Comments
 (0)