Skip to content

Commit 46a3c51

Browse files
author
Brian Chen
authored
fix: replace usages of transform proto with update_transform (#213)
1 parent 7c86b71 commit 46a3c51

File tree

8 files changed

+97
-152
lines changed

8 files changed

+97
-152
lines changed

‎google-cloud-firestore/pom.xml‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -162,7 +162,7 @@
162162
<dependency>
163163
<groupId>com.google.cloud</groupId>
164164
<artifactId>google-cloud-conformance-tests</artifactId>
165-
<version>0.0.10</version>
165+
<version>0.0.11</version>
166166
<scope>test</scope>
167167
</dependency>
168168
<dependency>

‎google-cloud-firestore/src/main/java/com/google/cloud/firestore/DocumentTransform.java‎

Lines changed: 5 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@
1717
package com.google.cloud.firestore;
1818

1919
import com.google.firestore.v1.DocumentTransform.FieldTransform;
20-
import com.google.firestore.v1.Write;
20+
import java.util.Collection;
2121
import java.util.Collections;
2222
import java.util.List;
2323
import java.util.Map;
@@ -30,12 +30,9 @@
3030
*/
3131
final class DocumentTransform {
3232

33-
private DocumentReference documentReference;
3433
private final SortedMap<FieldPath, FieldTransform> transforms; // Sorted for testing.
3534

36-
private DocumentTransform(
37-
DocumentReference documentReference, SortedMap<FieldPath, FieldTransform> transforms) {
38-
this.documentReference = documentReference;
35+
private DocumentTransform(SortedMap<FieldPath, FieldTransform> transforms) {
3936
this.transforms = transforms;
4037
}
4138

@@ -61,7 +58,7 @@ static DocumentTransform fromFieldPathMap(
6158
}
6259
}
6360

64-
return new DocumentTransform(documentReference, transforms);
61+
return new DocumentTransform(transforms);
6562
}
6663

6764
private static SortedMap<FieldPath, FieldTransform> extractFromMap(
@@ -116,11 +113,7 @@ Set<FieldPath> getFields() {
116113
return Collections.unmodifiableSet(transforms.keySet());
117114
}
118115

119-
Write.Builder toPb() {
120-
Write.Builder write = Write.newBuilder();
121-
com.google.firestore.v1.DocumentTransform.Builder transform = write.getTransformBuilder();
122-
transform.addAllFieldTransforms(transforms.values());
123-
transform.setDocument(documentReference.getName());
124-
return write;
116+
Collection<FieldTransform> toPb() {
117+
return transforms.values();
125118
}
126119
}

‎google-cloud-firestore/src/main/java/com/google/cloud/firestore/UpdateBuilder.java‎

Lines changed: 41 additions & 95 deletions
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,6 @@
3131
import io.opencensus.trace.Tracing;
3232
import java.util.ArrayList;
3333
import java.util.HashMap;
34-
import java.util.Iterator;
3534
import java.util.List;
3635
import java.util.Map;
3736
import java.util.Map.Entry;
@@ -46,19 +45,13 @@
4645
*/
4746
public abstract class UpdateBuilder<T extends UpdateBuilder> {
4847

49-
private static class Mutation {
50-
Write.Builder document;
51-
Write.Builder transform;
52-
com.google.firestore.v1.Precondition precondition;
53-
}
54-
5548
final FirestoreImpl firestore;
56-
private final List<Mutation> mutations;
49+
private final List<Write.Builder> writes;
5750
private boolean committed;
5851

5952
UpdateBuilder(FirestoreImpl firestore) {
6053
this.firestore = firestore;
61-
this.mutations = new ArrayList<>();
54+
this.writes = new ArrayList<>();
6255
}
6356

6457
/**
@@ -116,34 +109,29 @@ public T create(
116109

117110
private T performCreate(
118111
@Nonnull DocumentReference documentReference, @Nonnull Map<String, Object> fields) {
112+
verifyNotCommitted();
119113
DocumentSnapshot documentSnapshot =
120114
DocumentSnapshot.fromObject(
121115
firestore, documentReference, fields, UserDataConverter.NO_DELETES);
122116
DocumentTransform documentTransform =
123117
DocumentTransform.fromFieldPathMap(
124118
documentReference, convertToFieldPaths(fields, /* splitOnDots= */ false));
125119

126-
Mutation mutation = addMutation();
127-
mutation.precondition = Precondition.exists(false).toPb();
128-
129-
if (!documentSnapshot.isEmpty() || documentTransform.isEmpty()) {
130-
mutation.document = documentSnapshot.toPb();
131-
}
120+
Write.Builder write = documentSnapshot.toPb();
121+
write.setCurrentDocument(Precondition.exists(false).toPb());
132122

133123
if (!documentTransform.isEmpty()) {
134-
mutation.transform = documentTransform.toPb();
124+
write.addAllUpdateTransforms(documentTransform.toPb());
135125
}
136126

127+
writes.add(write);
128+
137129
return (T) this;
138130
}
139131

140-
/** Adds a new mutation to the batch. */
141-
private Mutation addMutation() {
132+
private void verifyNotCommitted() {
142133
Preconditions.checkState(
143134
!committed, "Cannot modify a WriteBatch that has already been committed.");
144-
Mutation mutation = new Mutation();
145-
mutations.add(mutation);
146-
return mutation;
147135
}
148136

149137
/**
@@ -233,6 +221,7 @@ private T performSet(
233221
@Nonnull DocumentReference documentReference,
234222
@Nonnull Map<String, Object> fields,
235223
@Nonnull SetOptions options) {
224+
verifyNotCommitted();
236225
Map<FieldPath, Object> documentData;
237226

238227
if (options.getFieldMask() != null) {
@@ -248,31 +237,25 @@ private T performSet(
248237
DocumentTransform documentTransform =
249238
DocumentTransform.fromFieldPathMap(documentReference, documentData);
250239

251-
if (options.isMerge()) {
252-
if (options.getFieldMask() != null) {
253-
List<FieldPath> fieldMask = new ArrayList<>(options.getFieldMask());
254-
fieldMask.removeAll(documentTransform.getFields());
255-
documentMask = new FieldMask(fieldMask);
256-
} else {
257-
documentMask = FieldMask.fromObject(fields);
258-
}
240+
if (options.getFieldMask() != null) {
241+
List<FieldPath> fieldMask = new ArrayList<>(options.getFieldMask());
242+
fieldMask.removeAll(documentTransform.getFields());
243+
documentMask = new FieldMask(fieldMask);
244+
} else if (options.isMerge()) {
245+
documentMask = FieldMask.fromObject(fields);
259246
}
260247

261-
Mutation mutation = addMutation();
262-
263-
boolean hasDocumentData = !documentSnapshot.isEmpty() || !documentMask.isEmpty();
264-
265-
if (!options.isMerge()) {
266-
mutation.document = documentSnapshot.toPb();
267-
} else if (hasDocumentData || documentTransform.isEmpty()) {
268-
mutation.document = documentSnapshot.toPb();
269-
mutation.document.setUpdateMask(documentMask.toPb());
248+
Write.Builder write = documentSnapshot.toPb();
249+
if (!documentTransform.isEmpty()) {
250+
write.addAllUpdateTransforms(documentTransform.toPb());
270251
}
271252

272-
if (!documentTransform.isEmpty()) {
273-
mutation.transform = documentTransform.toPb();
253+
if (options.isMerge() || options.getFieldMask() != null) {
254+
write.setUpdateMask(documentMask.toPb());
274255
}
275256

257+
writes.add(write);
258+
276259
return (T) this;
277260
}
278261

@@ -507,6 +490,7 @@ private T performUpdate(
507490
@Nonnull DocumentReference documentReference,
508491
@Nonnull final Map<FieldPath, Object> fields,
509492
@Nonnull Precondition precondition) {
493+
verifyNotCommitted();
510494
Preconditions.checkArgument(!fields.isEmpty(), "Data for update() cannot be empty.");
511495

512496
Map<String, Object> deconstructedMap = expandObject(fields);
@@ -532,17 +516,14 @@ public boolean allowTransform() {
532516
fieldPaths.removeAll(documentTransform.getFields());
533517
FieldMask fieldMask = new FieldMask(fieldPaths);
534518

535-
Mutation mutation = addMutation();
536-
mutation.precondition = precondition.toPb();
537-
538-
if (!documentSnapshot.isEmpty() || !fieldMask.isEmpty()) {
539-
mutation.document = documentSnapshot.toPb();
540-
mutation.document.setUpdateMask(fieldMask.toPb());
541-
}
519+
Write.Builder write = documentSnapshot.toPb();
520+
write.setCurrentDocument(precondition.toPb());
521+
write.setUpdateMask(fieldMask.toPb());
542522

543523
if (!documentTransform.isEmpty()) {
544-
mutation.transform = documentTransform.toPb();
524+
write.addAllUpdateTransforms(documentTransform.toPb());
545525
}
526+
writes.add(write);
546527

547528
return (T) this;
548529
}
@@ -573,12 +554,13 @@ public T delete(@Nonnull DocumentReference documentReference) {
573554

574555
private T performDelete(
575556
@Nonnull DocumentReference documentReference, @Nonnull Precondition precondition) {
576-
Mutation mutation = addMutation();
577-
mutation.document = Write.newBuilder().setDelete(documentReference.getName());
557+
verifyNotCommitted();
558+
Write.Builder write = Write.newBuilder().setDelete(documentReference.getName());
578559

579560
if (!precondition.isEmpty()) {
580-
mutation.precondition = precondition.toPb();
561+
write.setCurrentDocument(precondition.toPb());
581562
}
563+
writes.add(write);
582564

583565
return (T) this;
584566
}
@@ -589,28 +571,13 @@ ApiFuture<List<WriteResult>> commit(@Nullable ByteString transactionId) {
589571
.getCurrentSpan()
590572
.addAnnotation(
591573
"CloudFirestore.Commit",
592-
ImmutableMap.of("numDocuments", AttributeValue.longAttributeValue(mutations.size())));
574+
ImmutableMap.of("numDocuments", AttributeValue.longAttributeValue(writes.size())));
593575

594576
final CommitRequest.Builder request = CommitRequest.newBuilder();
595577
request.setDatabase(firestore.getDatabaseName());
596578

597-
for (Mutation mutation : mutations) {
598-
Preconditions.checkState(
599-
mutation.document != null || mutation.transform != null,
600-
"Either a write or transform must be set");
601-
602-
if (mutation.precondition != null) {
603-
(mutation.document != null ? mutation.document : mutation.transform)
604-
.setCurrentDocument(mutation.precondition);
605-
}
606-
607-
if (mutation.document != null) {
608-
request.addWrites(mutation.document);
609-
}
610-
611-
if (mutation.transform != null) {
612-
request.addWrites(mutation.transform);
613-
}
579+
for (Write.Builder write : writes) {
580+
request.addWrites(write);
614581
}
615582

616583
if (transactionId != null) {
@@ -632,29 +599,8 @@ public List<WriteResult> apply(CommitResponse commitResponse) {
632599

633600
List<WriteResult> result = new ArrayList<>();
634601

635-
Preconditions.checkState(
636-
request.getWritesCount() == writeResults.size(),
637-
"Expected one write result per operation, but got %s results for %s operations.",
638-
writeResults.size(),
639-
request.getWritesCount());
640-
641-
Iterator<Mutation> mutationIterator = mutations.iterator();
642-
Iterator<com.google.firestore.v1.WriteResult> responseIterator =
643-
writeResults.iterator();
644-
645-
while (mutationIterator.hasNext()) {
646-
Mutation mutation = mutationIterator.next();
647-
648-
// Don't return both write results for a write that contains a transform, as the fact
649-
// that we have to split one write operation into two distinct write requests is an
650-
// implementation detail.
651-
if (mutation.document != null && mutation.transform != null) {
652-
// The document transform is always sent last and produces the latest update time.
653-
responseIterator.next();
654-
}
655-
656-
result.add(
657-
WriteResult.fromProto(responseIterator.next(), commitResponse.getCommitTime()));
602+
for (com.google.firestore.v1.WriteResult writeResult : writeResults) {
603+
result.add(WriteResult.fromProto(writeResult, commitResponse.getCommitTime()));
658604
}
659605

660606
return result;
@@ -665,11 +611,11 @@ public List<WriteResult> apply(CommitResponse commitResponse) {
665611

666612
/** Checks whether any updates have been queued. */
667613
boolean isEmpty() {
668-
return mutations.isEmpty();
614+
return writes.isEmpty();
669615
}
670616

671-
/** Get the number of mutations. */
617+
/** Get the number of writes. */
672618
public int getMutationsSize() {
673-
return mutations.size();
619+
return writes.size();
674620
}
675621
}

‎google-cloud-firestore/src/test/java/com/google/cloud/firestore/DocumentReferenceTest.java‎

Lines changed: 16 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -20,15 +20,13 @@
2020
import static com.google.cloud.firestore.LocalFirestoreHelper.ALL_SUPPORTED_TYPES_OBJECT;
2121
import static com.google.cloud.firestore.LocalFirestoreHelper.ALL_SUPPORTED_TYPES_PROTO;
2222
import static com.google.cloud.firestore.LocalFirestoreHelper.BLOB;
23-
import static com.google.cloud.firestore.LocalFirestoreHelper.CREATE_PRECONDITION;
2423
import static com.google.cloud.firestore.LocalFirestoreHelper.DATE;
2524
import static com.google.cloud.firestore.LocalFirestoreHelper.DOCUMENT_NAME;
2625
import static com.google.cloud.firestore.LocalFirestoreHelper.DOCUMENT_PATH;
2726
import static com.google.cloud.firestore.LocalFirestoreHelper.FIELD_TRANSFORM_COMMIT_RESPONSE;
2827
import static com.google.cloud.firestore.LocalFirestoreHelper.GEO_POINT;
2928
import static com.google.cloud.firestore.LocalFirestoreHelper.NESTED_CLASS_OBJECT;
3029
import static com.google.cloud.firestore.LocalFirestoreHelper.SERVER_TIMESTAMP_PROTO;
31-
import static com.google.cloud.firestore.LocalFirestoreHelper.SERVER_TIMESTAMP_TRANSFORM;
3230
import static com.google.cloud.firestore.LocalFirestoreHelper.SINGLE_DELETE_COMMIT_RESPONSE;
3331
import static com.google.cloud.firestore.LocalFirestoreHelper.SINGLE_FIELD_MAP;
3432
import static com.google.cloud.firestore.LocalFirestoreHelper.SINGLE_FIELD_OBJECT;
@@ -406,8 +404,8 @@ public void createWithServerTimestamp() throws Exception {
406404

407405
CommitRequest create =
408406
commit(
409-
transform(
410-
CREATE_PRECONDITION, "foo", serverTimestamp(), "inner.bar", serverTimestamp()));
407+
create(Collections.<String, Value>emptyMap()),
408+
transform("foo", serverTimestamp(), "inner.bar", serverTimestamp()));
411409

412410
List<CommitRequest> commitRequests = commitCapture.getAllValues();
413411
assertCommitEquals(create, commitRequests.get(0));
@@ -424,7 +422,10 @@ public void setWithServerTimestamp() throws Exception {
424422
documentReference.set(LocalFirestoreHelper.SERVER_TIMESTAMP_MAP).get();
425423
documentReference.set(LocalFirestoreHelper.SERVER_TIMESTAMP_OBJECT).get();
426424

427-
CommitRequest set = commit(set(SERVER_TIMESTAMP_PROTO), SERVER_TIMESTAMP_TRANSFORM);
425+
CommitRequest set =
426+
commit(
427+
set(SERVER_TIMESTAMP_PROTO),
428+
transform("foo", serverTimestamp(), "inner.bar", serverTimestamp()));
428429

429430
List<CommitRequest> commitRequests = commitCapture.getAllValues();
430431
assertCommitEquals(set, commitRequests.get(0));
@@ -443,7 +444,7 @@ public void updateWithServerTimestamp() throws Exception {
443444
CommitRequest update =
444445
commit(
445446
update(Collections.<String, Value>emptyMap(), Collections.singletonList("inner")),
446-
SERVER_TIMESTAMP_TRANSFORM);
447+
transform("foo", serverTimestamp(), "inner.bar", serverTimestamp()));
447448

448449
assertCommitEquals(update, commitCapture.getValue());
449450

@@ -452,8 +453,11 @@ public void updateWithServerTimestamp() throws Exception {
452453

453454
update =
454455
commit(
455-
transform(
456-
UPDATE_PRECONDITION, "foo", serverTimestamp(), "inner.bar", serverTimestamp()));
456+
update(
457+
Collections.<String, Value>emptyMap(),
458+
new ArrayList<String>(),
459+
UPDATE_PRECONDITION),
460+
transform("foo", serverTimestamp(), "inner.bar", serverTimestamp()));
457461

458462
assertCommitEquals(update, commitCapture.getValue());
459463
}
@@ -472,7 +476,10 @@ public void mergeWithServerTimestamps() throws Exception {
472476
.set(LocalFirestoreHelper.SERVER_TIMESTAMP_OBJECT, SetOptions.mergeFields("inner.bar"))
473477
.get();
474478

475-
CommitRequest set = commit(transform("inner.bar", serverTimestamp()));
479+
CommitRequest set =
480+
commit(
481+
set(SERVER_TIMESTAMP_PROTO, new ArrayList<String>()),
482+
transform("inner.bar", serverTimestamp()));
476483

477484
List<CommitRequest> commitRequests = commitCapture.getAllValues();
478485
assertCommitEquals(set, commitRequests.get(0));

0 commit comments

Comments
 (0)