Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: replace usages of transform proto with update_transform #213

Merged
merged 9 commits into from
May 19, 2020
Merged

fix: replace usages of transform proto with update_transform #213

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
2d6b08b
fix: replace usages of transform proto with update_transform
May 13, 2020
3c1ddae
Merge branch 'master' into bc/update-proto
May 14, 2020
56a6236
remove Mutation, resolve other comments
May 14, 2020
a1bbee4
revert to getMutationSize() and revert POM file
May 14, 2020
6ef32c6
move verifyNotCommitted() and remove writeIterator use
May 14, 2020
6dfd910
remove javadoc for verifyNotCommitted
May 14, 2020
1aa5e44
bump version
May 15, 2020
3f8b653
Merge branch 'master' into bc/update-proto
May 15, 2020
069eba2
update writebatch test
May 15, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 5 additions & 12 deletions google-cloud-firestore/src/main/java/com/google/cloud/firestore/DocumentTransform.java
View file
Open in desktop
Original file line number Diff line number Diff line change
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();
}
}
132 changes: 43 additions & 89 deletions google-cloud-firestore/src/main/java/com/google/cloud/firestore/UpdateBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,19 +46,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 @@ -123,27 +117,23 @@ private T performCreate(
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();
}
verifyNotCommitted();
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() {
/** Adds a new write to the batch. */
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Incorrect javadoc now that the mutation has been removed.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed the javadoc since the method is pretty straightforward now.

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 @@ -248,31 +238,26 @@ 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());
verifyNotCommitted();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels out of place now. Move to top of method?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

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 @@ -532,17 +517,15 @@ 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());
}
verifyNotCommitted();
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 +556,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 +573,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,27 +601,12 @@ 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<Write.Builder> writeIterator = writes.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();
}

while (writeIterator.hasNext()) {
writeIterator.next();
result.add(
WriteResult.fromProto(responseIterator.next(), commitResponse.getCommitTime()));
}
Expand All @@ -665,11 +619,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
Open in desktop
Original file line number Diff line number Diff line change
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
13 changes: 10 additions & 3 deletions google-cloud-firestore/src/test/java/com/google/cloud/firestore/FirestoreTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,13 @@
import static com.google.cloud.firestore.LocalFirestoreHelper.SINGLE_FIELD_OBJECT;
import static com.google.cloud.firestore.LocalFirestoreHelper.SINGLE_FIELD_PROTO;
import static com.google.cloud.firestore.LocalFirestoreHelper.SINGLE_FIELD_VALUE;
import static com.google.cloud.firestore.LocalFirestoreHelper.UPDATE_PRECONDITION;
import static com.google.cloud.firestore.LocalFirestoreHelper.arrayRemove;
import static com.google.cloud.firestore.LocalFirestoreHelper.arrayUnion;
import static com.google.cloud.firestore.LocalFirestoreHelper.commit;
import static com.google.cloud.firestore.LocalFirestoreHelper.commitResponse;
import static com.google.cloud.firestore.LocalFirestoreHelper.getAllResponse;
import static com.google.cloud.firestore.LocalFirestoreHelper.transform;
import static com.google.cloud.firestore.LocalFirestoreHelper.update;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertNotEquals;
import static org.junit.Assert.fail;
Expand All @@ -40,6 +40,9 @@
import com.google.firestore.v1.CommitRequest;
import com.google.firestore.v1.CommitResponse;
import com.google.firestore.v1.ListCollectionIdsRequest;
import com.google.firestore.v1.Value;
import java.util.ArrayList;
import java.util.Collections;
import java.util.List;
import java.util.concurrent.ExecutionException;
import org.junit.Test;
Expand Down Expand Up @@ -196,7 +199,9 @@ public void arrayUnionWithPojo() throws ExecutionException, InterruptedException
doc.update("array", FieldValue.arrayUnion(SINGLE_FIELD_OBJECT)).get();

CommitRequest expectedRequest =
commit(transform(UPDATE_PRECONDITION, "array", arrayUnion(SINGLE_FIELD_VALUE)));
commit(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think of exposing an overload to commit?

 public static CommitRequest commit(Write write, List<FieldTransform> transform) {

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done. Got rid of writeWithTransform.

update(Collections.<String, Value>emptyMap(), new ArrayList<String>()),
transform("array", arrayUnion(SINGLE_FIELD_VALUE)));
CommitRequest actualRequest = commitCapture.getValue();
assertEquals(expectedRequest, actualRequest);
}
Expand All @@ -212,7 +217,9 @@ public void arrayRemoveWithPojo() throws ExecutionException, InterruptedExceptio
doc.update("array", FieldValue.arrayRemove(SINGLE_FIELD_OBJECT)).get();

CommitRequest expectedRequest =
commit(transform(UPDATE_PRECONDITION, "array", arrayRemove(SINGLE_FIELD_VALUE)));
commit(
update(Collections.<String, Value>emptyMap(), new ArrayList<String>()),
transform("array", arrayRemove(SINGLE_FIELD_VALUE)));
CommitRequest actualRequest = commitCapture.getValue();
assertEquals(expectedRequest, actualRequest);
}
Expand Down
Loading