Skip to content

Commit

Permalink
fix: replace usages of transform proto with update_transform (#213)
Browse files Browse the repository at this point in the history
  • Loading branch information
Brian Chen committed May 19, 2020
1 parent 7c86b71 commit 46a3c51
Show file tree
Hide file tree
Showing 8 changed files with 97 additions and 152 deletions.
2 changes: 1 addition & 1 deletion google-cloud-firestore/pom.xml
View file
Expand Up @@ -162,7 +162,7 @@
<dependency>
<groupId>com.google.cloud</groupId>
<artifactId>google-cloud-conformance-tests</artifactId>
<version>0.0.10</version>
<version>0.0.11</version>
<scope>test</scope>
</dependency>
<dependency>
Expand Down
17 changes: 5 additions & 12 deletions google-cloud-firestore/src/main/java/com/google/cloud/firestore/DocumentTransform.java
View file
Expand Up @@ -17,7 +17,7 @@
package com.google.cloud.firestore;

import com.google.firestore.v1.DocumentTransform.FieldTransform;
import com.google.firestore.v1.Write;
import java.util.Collection;
import java.util.Collections;
import java.util.List;
import java.util.Map;
Expand All @@ -30,12 +30,9 @@
*/
final class DocumentTransform {

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

private DocumentTransform(
DocumentReference documentReference, SortedMap<FieldPath, FieldTransform> transforms) {
this.documentReference = documentReference;
private DocumentTransform(SortedMap<FieldPath, FieldTransform> transforms) {
this.transforms = transforms;
}

Expand All @@ -61,7 +58,7 @@ static DocumentTransform fromFieldPathMap(
}
}

return new DocumentTransform(documentReference, transforms);
return new DocumentTransform(transforms);
}

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

Write.Builder toPb() {
Write.Builder write = Write.newBuilder();
com.google.firestore.v1.DocumentTransform.Builder transform = write.getTransformBuilder();
transform.addAllFieldTransforms(transforms.values());
transform.setDocument(documentReference.getName());
return write;
Collection<FieldTransform> toPb() {
return transforms.values();
}
}
136 changes: 41 additions & 95 deletions google-cloud-firestore/src/main/java/com/google/cloud/firestore/UpdateBuilder.java
View file
Expand Up @@ -31,7 +31,6 @@
import io.opencensus.trace.Tracing;
import java.util.ArrayList;
import java.util.HashMap;
import java.util.Iterator;
import java.util.List;
import java.util.Map;
import java.util.Map.Entry;
Expand All @@ -46,19 +45,13 @@
*/
public abstract class UpdateBuilder<T extends UpdateBuilder> {

private static class Mutation {
Write.Builder document;
Write.Builder transform;
com.google.firestore.v1.Precondition precondition;
}

final FirestoreImpl firestore;
private final List<Mutation> mutations;
private final List<Write.Builder> writes;
private boolean committed;

UpdateBuilder(FirestoreImpl firestore) {
this.firestore = firestore;
this.mutations = new ArrayList<>();
this.writes = new ArrayList<>();
}

/**
Expand Down Expand Up @@ -116,34 +109,29 @@ public T create(

private T performCreate(
@Nonnull DocumentReference documentReference, @Nonnull Map<String, Object> fields) {
verifyNotCommitted();
DocumentSnapshot documentSnapshot =
DocumentSnapshot.fromObject(
firestore, documentReference, fields, UserDataConverter.NO_DELETES);
DocumentTransform documentTransform =
DocumentTransform.fromFieldPathMap(
documentReference, convertToFieldPaths(fields, /* splitOnDots= */ false));

Mutation mutation = addMutation();
mutation.precondition = Precondition.exists(false).toPb();

if (!documentSnapshot.isEmpty() || documentTransform.isEmpty()) {
mutation.document = documentSnapshot.toPb();
}
Write.Builder write = documentSnapshot.toPb();
write.setCurrentDocument(Precondition.exists(false).toPb());

if (!documentTransform.isEmpty()) {
mutation.transform = documentTransform.toPb();
write.addAllUpdateTransforms(documentTransform.toPb());
}

writes.add(write);

return (T) this;
}

/** Adds a new mutation to the batch. */
private Mutation addMutation() {
private void verifyNotCommitted() {
Preconditions.checkState(
!committed, "Cannot modify a WriteBatch that has already been committed.");
Mutation mutation = new Mutation();
mutations.add(mutation);
return mutation;
}

/**
Expand Down Expand Up @@ -233,6 +221,7 @@ private T performSet(
@Nonnull DocumentReference documentReference,
@Nonnull Map<String, Object> fields,
@Nonnull SetOptions options) {
verifyNotCommitted();
Map<FieldPath, Object> documentData;

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

if (options.isMerge()) {
if (options.getFieldMask() != null) {
List<FieldPath> fieldMask = new ArrayList<>(options.getFieldMask());
fieldMask.removeAll(documentTransform.getFields());
documentMask = new FieldMask(fieldMask);
} else {
documentMask = FieldMask.fromObject(fields);
}
if (options.getFieldMask() != null) {
List<FieldPath> fieldMask = new ArrayList<>(options.getFieldMask());
fieldMask.removeAll(documentTransform.getFields());
documentMask = new FieldMask(fieldMask);
} else if (options.isMerge()) {
documentMask = FieldMask.fromObject(fields);
}

Mutation mutation = addMutation();

boolean hasDocumentData = !documentSnapshot.isEmpty() || !documentMask.isEmpty();

if (!options.isMerge()) {
mutation.document = documentSnapshot.toPb();
} else if (hasDocumentData || documentTransform.isEmpty()) {
mutation.document = documentSnapshot.toPb();
mutation.document.setUpdateMask(documentMask.toPb());
Write.Builder write = documentSnapshot.toPb();
if (!documentTransform.isEmpty()) {
write.addAllUpdateTransforms(documentTransform.toPb());
}

if (!documentTransform.isEmpty()) {
mutation.transform = documentTransform.toPb();
if (options.isMerge() || options.getFieldMask() != null) {
write.setUpdateMask(documentMask.toPb());
}

writes.add(write);

return (T) this;
}

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

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

Mutation mutation = addMutation();
mutation.precondition = precondition.toPb();

if (!documentSnapshot.isEmpty() || !fieldMask.isEmpty()) {
mutation.document = documentSnapshot.toPb();
mutation.document.setUpdateMask(fieldMask.toPb());
}
Write.Builder write = documentSnapshot.toPb();
write.setCurrentDocument(precondition.toPb());
write.setUpdateMask(fieldMask.toPb());

if (!documentTransform.isEmpty()) {
mutation.transform = documentTransform.toPb();
write.addAllUpdateTransforms(documentTransform.toPb());
}
writes.add(write);

return (T) this;
}
Expand Down Expand Up @@ -573,12 +554,13 @@ public T delete(@Nonnull DocumentReference documentReference) {

private T performDelete(
@Nonnull DocumentReference documentReference, @Nonnull Precondition precondition) {
Mutation mutation = addMutation();
mutation.document = Write.newBuilder().setDelete(documentReference.getName());
verifyNotCommitted();
Write.Builder write = Write.newBuilder().setDelete(documentReference.getName());

if (!precondition.isEmpty()) {
mutation.precondition = precondition.toPb();
write.setCurrentDocument(precondition.toPb());
}
writes.add(write);

return (T) this;
}
Expand All @@ -589,28 +571,13 @@ ApiFuture<List<WriteResult>> commit(@Nullable ByteString transactionId) {
.getCurrentSpan()
.addAnnotation(
"CloudFirestore.Commit",
ImmutableMap.of("numDocuments", AttributeValue.longAttributeValue(mutations.size())));
ImmutableMap.of("numDocuments", AttributeValue.longAttributeValue(writes.size())));

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

for (Mutation mutation : mutations) {
Preconditions.checkState(
mutation.document != null || mutation.transform != null,
"Either a write or transform must be set");

if (mutation.precondition != null) {
(mutation.document != null ? mutation.document : mutation.transform)
.setCurrentDocument(mutation.precondition);
}

if (mutation.document != null) {
request.addWrites(mutation.document);
}

if (mutation.transform != null) {
request.addWrites(mutation.transform);
}
for (Write.Builder write : writes) {
request.addWrites(write);
}

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

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

Preconditions.checkState(
request.getWritesCount() == writeResults.size(),
"Expected one write result per operation, but got %s results for %s operations.",
writeResults.size(),
request.getWritesCount());

Iterator<Mutation> mutationIterator = mutations.iterator();
Iterator<com.google.firestore.v1.WriteResult> responseIterator =
writeResults.iterator();

while (mutationIterator.hasNext()) {
Mutation mutation = mutationIterator.next();

// Don't return both write results for a write that contains a transform, as the fact
// that we have to split one write operation into two distinct write requests is an
// implementation detail.
if (mutation.document != null && mutation.transform != null) {
// The document transform is always sent last and produces the latest update time.
responseIterator.next();
}

result.add(
WriteResult.fromProto(responseIterator.next(), commitResponse.getCommitTime()));
for (com.google.firestore.v1.WriteResult writeResult : writeResults) {
result.add(WriteResult.fromProto(writeResult, commitResponse.getCommitTime()));
}

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

/** Checks whether any updates have been queued. */
boolean isEmpty() {
return mutations.isEmpty();
return writes.isEmpty();
}

/** Get the number of mutations. */
/** Get the number of writes. */
public int getMutationsSize() {
return mutations.size();
return writes.size();
}
}
25 changes: 16 additions & 9 deletions google-cloud-firestore/src/test/java/com/google/cloud/firestore/DocumentReferenceTest.java
View file
Expand Up @@ -20,15 +20,13 @@
import static com.google.cloud.firestore.LocalFirestoreHelper.ALL_SUPPORTED_TYPES_OBJECT;
import static com.google.cloud.firestore.LocalFirestoreHelper.ALL_SUPPORTED_TYPES_PROTO;
import static com.google.cloud.firestore.LocalFirestoreHelper.BLOB;
import static com.google.cloud.firestore.LocalFirestoreHelper.CREATE_PRECONDITION;
import static com.google.cloud.firestore.LocalFirestoreHelper.DATE;
import static com.google.cloud.firestore.LocalFirestoreHelper.DOCUMENT_NAME;
import static com.google.cloud.firestore.LocalFirestoreHelper.DOCUMENT_PATH;
import static com.google.cloud.firestore.LocalFirestoreHelper.FIELD_TRANSFORM_COMMIT_RESPONSE;
import static com.google.cloud.firestore.LocalFirestoreHelper.GEO_POINT;
import static com.google.cloud.firestore.LocalFirestoreHelper.NESTED_CLASS_OBJECT;
import static com.google.cloud.firestore.LocalFirestoreHelper.SERVER_TIMESTAMP_PROTO;
import static com.google.cloud.firestore.LocalFirestoreHelper.SERVER_TIMESTAMP_TRANSFORM;
import static com.google.cloud.firestore.LocalFirestoreHelper.SINGLE_DELETE_COMMIT_RESPONSE;
import static com.google.cloud.firestore.LocalFirestoreHelper.SINGLE_FIELD_MAP;
import static com.google.cloud.firestore.LocalFirestoreHelper.SINGLE_FIELD_OBJECT;
Expand Down Expand Up @@ -406,8 +404,8 @@ public void createWithServerTimestamp() throws Exception {

CommitRequest create =
commit(
transform(
CREATE_PRECONDITION, "foo", serverTimestamp(), "inner.bar", serverTimestamp()));
create(Collections.<String, Value>emptyMap()),
transform("foo", serverTimestamp(), "inner.bar", serverTimestamp()));

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

CommitRequest set = commit(set(SERVER_TIMESTAMP_PROTO), SERVER_TIMESTAMP_TRANSFORM);
CommitRequest set =
commit(
set(SERVER_TIMESTAMP_PROTO),
transform("foo", serverTimestamp(), "inner.bar", serverTimestamp()));

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

assertCommitEquals(update, commitCapture.getValue());

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

update =
commit(
transform(
UPDATE_PRECONDITION, "foo", serverTimestamp(), "inner.bar", serverTimestamp()));
update(
Collections.<String, Value>emptyMap(),
new ArrayList<String>(),
UPDATE_PRECONDITION),
transform("foo", serverTimestamp(), "inner.bar", serverTimestamp()));

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

CommitRequest set = commit(transform("inner.bar", serverTimestamp()));
CommitRequest set =
commit(
set(SERVER_TIMESTAMP_PROTO, new ArrayList<String>()),
transform("inner.bar", serverTimestamp()));

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

0 comments on commit 46a3c51

Please sign in to comment.