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

Jump to bottom

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 1 commit
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.
Jump to
Jump to file
Failed to load files.
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
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();
}
}
74 changes: 19 additions & 55 deletions google-cloud-firestore/src/main/java/com/google/cloud/firestore/UpdateBuilder.java
View file
Open in desktop
Expand Up @@ -126,12 +126,10 @@ private T performCreate(
Mutation mutation = addMutation();
mutation.precondition = Precondition.exists(false).toPb();

if (!documentSnapshot.isEmpty() || documentTransform.isEmpty()) {
mutation.document = documentSnapshot.toPb();
}
mutation.document = documentSnapshot.toPb();

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

return (T) this;
Expand Down Expand Up @@ -248,29 +246,23 @@ 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());
mutation.document = documentSnapshot.toPb();
if (!documentTransform.isEmpty()) {
mutation.document.addAllUpdateTransforms(documentTransform.toPb());
}

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

return (T) this;
Expand Down Expand Up @@ -535,13 +527,11 @@ public boolean allowTransform() {
Mutation mutation = addMutation();
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can probably remove the Mutation type and replace it with the Proto type. What do you think?

Copy link
Author

Choose a reason for hiding this comment

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

Good call. I remember you did this for node and the code became much cleaner. Removed Mutation and renamed mutation related variables to write.

mutation.precondition = precondition.toPb();

if (!documentSnapshot.isEmpty() || !fieldMask.isEmpty()) {
mutation.document = documentSnapshot.toPb();
mutation.document.setUpdateMask(fieldMask.toPb());
}
mutation.document = documentSnapshot.toPb();
mutation.document.setUpdateMask(fieldMask.toPb());

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

return (T) this;
Expand Down Expand Up @@ -595,22 +585,11 @@ ApiFuture<List<WriteResult>> commit(@Nullable ByteString transactionId) {
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);
mutation.document.setCurrentDocument(mutation.precondition);
Copy link
Contributor

Choose a reason for hiding this comment

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

If we use the Write proto directly instead of Mutation, we should be able to get rid of this line.

Copy link
Author

Choose a reason for hiding this comment

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

I'm using the write proto directly now, but I can't see how to remove this line. Don't we always need to check if the precondition exists before setting it onto the write?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, sorry - I thought this was in "commit()". Looks good.

}

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

if (transactionId != null) {
Expand All @@ -632,27 +611,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<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();
}

mutationIterator.next();
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be simpler now if we just replace it with a for loop.

Copy link
Author

Choose a reason for hiding this comment

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

Tried it with a for loop, intellij recommended I use while loop since the local variable in the for loop was redundant.

Copy link
Contributor

Choose a reason for hiding this comment

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

You should iterator over writeResults and drop mutationIterator altogether.

Copy link
Author

Choose a reason for hiding this comment

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

After changing it to iterate through writeResults, intellij recommended that it be converted to an "enhanced for loop".

result.add(
WriteResult.fromProto(responseIterator.next(), commitResponse.getCommitTime()));
}
Expand Down
56 changes: 36 additions & 20 deletions google-cloud-firestore/src/test/java/com/google/cloud/firestore/DocumentReferenceTest.java
View file
Open in desktop
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 All @@ -55,6 +53,7 @@
import static com.google.cloud.firestore.LocalFirestoreHelper.string;
import static com.google.cloud.firestore.LocalFirestoreHelper.transform;
import static com.google.cloud.firestore.LocalFirestoreHelper.update;
import static com.google.cloud.firestore.LocalFirestoreHelper.writeWithTransform;
import static org.junit.Assert.assertArrayEquals;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertFalse;
Expand Down Expand Up @@ -406,8 +405,9 @@ public void createWithServerTimestamp() throws Exception {

CommitRequest create =
commit(
transform(
CREATE_PRECONDITION, "foo", serverTimestamp(), "inner.bar", serverTimestamp()));
writeWithTransform(
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 +424,11 @@ 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(
writeWithTransform(
set(SERVER_TIMESTAMP_PROTO),
transform("foo", serverTimestamp(), "inner.bar", serverTimestamp())));

List<CommitRequest> commitRequests = commitCapture.getAllValues();
assertCommitEquals(set, commitRequests.get(0));
Expand All @@ -442,8 +446,9 @@ public void updateWithServerTimestamp() throws Exception {

CommitRequest update =
commit(
update(Collections.<String, Value>emptyMap(), Collections.singletonList("inner")),
SERVER_TIMESTAMP_TRANSFORM);
writeWithTransform(
update(Collections.<String, Value>emptyMap(), Collections.singletonList("inner")),
transform("foo", serverTimestamp(), "inner.bar", serverTimestamp())));

assertCommitEquals(update, commitCapture.getValue());

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

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

assertCommitEquals(update, commitCapture.getValue());
}
Expand All @@ -472,7 +481,11 @@ 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(
writeWithTransform(
set(SERVER_TIMESTAMP_PROTO, new ArrayList<String>()),
transform("inner.bar", serverTimestamp())));

List<CommitRequest> commitRequests = commitCapture.getAllValues();
assertCommitEquals(set, commitRequests.get(0));
Expand All @@ -492,12 +505,13 @@ public void setWithIncrement() throws Exception {

CommitRequest set =
commit(
set(Collections.<String, Value>emptyMap()),
transform(
"integer",
increment(Value.newBuilder().setIntegerValue(1).build()),
"double",
increment(Value.newBuilder().setDoubleValue(1.1).build())));
writeWithTransform(
set(Collections.<String, Value>emptyMap()),
transform(
"integer",
increment(Value.newBuilder().setIntegerValue(1).build()),
"double",
increment(Value.newBuilder().setDoubleValue(1.1).build()))));

CommitRequest commitRequest = commitCapture.getValue();
assertCommitEquals(set, commitRequest);
Expand All @@ -514,8 +528,9 @@ public void setWithArrayUnion() throws Exception {

CommitRequest set =
commit(
set(Collections.<String, Value>emptyMap()),
transform("foo", arrayUnion(string("bar"), object("foo", string("baz")))));
writeWithTransform(
set(Collections.<String, Value>emptyMap()),
transform("foo", arrayUnion(string("bar"), object("foo", string("baz"))))));

CommitRequest commitRequest = commitCapture.getValue();
assertCommitEquals(set, commitRequest);
Expand All @@ -532,8 +547,9 @@ public void setWithArrayRemove() throws Exception {

CommitRequest set =
commit(
set(Collections.<String, Value>emptyMap()),
transform("foo", arrayRemove(string("bar"), object("foo", string("baz")))));
writeWithTransform(
set(Collections.<String, Value>emptyMap()),
transform("foo", arrayRemove(string("bar"), object("foo", string("baz"))))));

CommitRequest commitRequest = commitCapture.getValue();
assertCommitEquals(set, commitRequest);
Expand Down
16 changes: 13 additions & 3 deletions google-cloud-firestore/src/test/java/com/google/cloud/firestore/FirestoreTest.java
View file
Open in desktop
Expand Up @@ -19,13 +19,14 @@
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 com.google.cloud.firestore.LocalFirestoreHelper.writeWithTransform;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertNotEquals;
import static org.junit.Assert.fail;
Expand All @@ -40,6 +41,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 +200,10 @@ 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.

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 +219,10 @@ 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(
writeWithTransform(
update(Collections.<String, Value>emptyMap(), new ArrayList<String>()),
transform("array", arrayRemove(SINGLE_FIELD_VALUE))));
CommitRequest actualRequest = commitCapture.getValue();
assertEquals(expectedRequest, actualRequest);
}
Expand Down