fix: replace usages of transform proto with update_transform #213

thebrianchen · 2020-05-13T00:12:12Z

Porting from node. Also added additional system test port as well.

Conformance tests are passing locally against the custom [branch] of modified conformance tests (https://github.com/googleapis/java-conformance-tests/tree/bump/2020-05-12_141750) in googleapis/java-conformance-test. According to @BenWhitehead, once this PR is LGTM'd, we will:

Merge the conformance test PR into conformance-tests.
Create new release of java-conformance-tests.
Update java-firestore to the new release.
Rebuild this PR and ensure everything passes before merging.

schmidt-sebastian

We should probably merge this into a feature branch and only merge to master once this PR and the conformance test update is ready. We need the updated conformance tests to validate these changes thoroughly.

schmidt-sebastian · 2020-05-13T18:43:05Z

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

@@ -535,13 +527,11 @@ public boolean allowTransform() {
    Mutation mutation = addMutation();


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

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

schmidt-sebastian · 2020-05-13T18:44:08Z

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

-
-      if (mutation.document != null) {
-        request.addWrites(mutation.document);
+        mutation.document.setCurrentDocument(mutation.precondition);


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

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?

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

schmidt-sebastian · 2020-05-13T18:45:00Z

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

-                responseIterator.next();
-              }
-
+              mutationIterator.next();


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

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

You should iterator over writeResults and drop mutationIterator altogether.

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

schmidt-sebastian · 2020-05-13T18:48:26Z

google-cloud-firestore/src/test/java/com/google/cloud/firestore/FirestoreTest.java

@@ -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(


What do you think of exposing an overload to commit?

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

done. Got rid of writeWithTransform.

thebrianchen

We should probably merge this into a feature branch and only merge to master once this PR and the conformance test update is ready. We need the updated conformance tests to validate these changes thoroughly.

I detailed the merge steps in the PR description. I talked to Ben about this, and he said that he will help facilitate merging this PR in once the new conformance tests have been added. I've run these against the updated conformance tests locally and they are passing. Merging this into a separate branch might make things easier for him though.

thebrianchen · 2020-05-14T00:25:28Z

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

-                responseIterator.next();
-              }
-
+              mutationIterator.next();


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

thebrianchen · 2020-05-14T00:25:48Z

google-cloud-firestore/src/test/java/com/google/cloud/firestore/FirestoreTest.java

@@ -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(


done. Got rid of writeWithTransform.

thebrianchen · 2020-05-14T00:50:24Z

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

-
-      if (mutation.document != null) {
-        request.addWrites(mutation.document);
+        mutation.document.setCurrentDocument(mutation.precondition);


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?

thebrianchen · 2020-05-14T00:51:04Z

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

@@ -535,13 +527,11 @@ public boolean allowTransform() {
    Mutation mutation = addMutation();


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

schmidt-sebastian

LGTM with one final nit (the while loop)

Thanks for forcing me to read the PR description.

schmidt-sebastian · 2020-05-14T02:10:22Z

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

-    } else if (hasDocumentData || documentTransform.isEmpty()) {
-      mutation.document = documentSnapshot.toPb();
-      mutation.document.setUpdateMask(documentMask.toPb());
+    verifyNotCommitted();


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

schmidt-sebastian · 2020-05-14T02:13:01Z

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

-
-      if (mutation.document != null) {
-        request.addWrites(mutation.document);
+        mutation.document.setCurrentDocument(mutation.precondition);


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

schmidt-sebastian · 2020-05-14T02:13:38Z

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

-                responseIterator.next();
-              }
-
+              mutationIterator.next();


You should iterator over writeResults and drop mutationIterator altogether.

schmidt-sebastian

LGTM

BenWhitehead · 2020-05-14T21:29:50Z

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

    return (T) this;
  }

-  /** Adds a new mutation to the batch. */
-  private Mutation addMutation() {
+  /** Adds a new write to the batch. */


Incorrect javadoc now that the mutation has been removed.

Removed the javadoc since the method is pretty straightforward now.

BenWhitehead · 2020-05-15T19:57:02Z

The release of the conformance tests is available. @thebrianchen Can you bump the dependency like is done in this PR: #218 that should allow your builds to be green.

codecov · 2020-05-15T21:43:25Z

Codecov Report

Merging #213 into master will decrease coverage by 0.15%.
The diff coverage is 97.43%.

@@             Coverage Diff              @@
##             master     #213      +/-   ##
============================================
- Coverage     73.09%   72.94%   -0.16%     
+ Complexity     1031     1015      -16     
============================================
  Files            63       63              
  Lines          5434     5407      -27     
  Branches        627      617      -10     
============================================
- Hits           3972     3944      -28     
- Misses         1264     1266       +2     
+ Partials        198      197       -1

Impacted Files	Coverage Δ	Complexity Δ
...java/com/google/cloud/firestore/UpdateBuilder.java	`94.70% <97.22%> (-0.56%)`	`57.00 <15.00> (-13.00)`
.../com/google/cloud/firestore/DocumentTransform.java	`98.03% <100.00%> (-0.18%)`	`22.00 <2.00> (ø)`
...ain/java/com/google/cloud/firestore/FieldMask.java	`84.37% <0.00%> (-3.13%)`	`14.00% <0.00%> (-1.00%)`
...a/com/google/cloud/firestore/DocumentSnapshot.java	`83.05% <0.00%> (ø)`	`53.00% <0.00%> (-2.00%)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7c86b71...069eba2. Read the comment docs.

BenWhitehead · 2020-05-18T18:27:54Z

Pinged someone to update the branch protection settings to unblock this.

fix: replace usages of transform proto with update_transform

2d6b08b

thebrianchen self-assigned this May 13, 2020

googlebot added the cla: yes This human has signed the Contributor License Agreement. label May 13, 2020

thebrianchen requested a review from schmidt-sebastian May 13, 2020 05:55

thebrianchen assigned schmidt-sebastian and unassigned thebrianchen May 13, 2020

schmidt-sebastian suggested changes May 13, 2020

View reviewed changes

schmidt-sebastian assigned thebrianchen and unassigned schmidt-sebastian May 13, 2020

Brian Chen added 3 commits May 13, 2020 17:44

Merge branch 'master' into bc/update-proto

3c1ddae

remove Mutation, resolve other comments

56a6236

revert to getMutationSize() and revert POM file

a1bbee4

thebrianchen commented May 14, 2020

View reviewed changes

thebrianchen requested a review from schmidt-sebastian May 14, 2020 01:00

thebrianchen assigned schmidt-sebastian and unassigned thebrianchen May 14, 2020

schmidt-sebastian approved these changes May 14, 2020

View reviewed changes

schmidt-sebastian removed their assignment May 14, 2020

move verifyNotCommitted() and remove writeIterator use

6ef32c6

schmidt-sebastian approved these changes May 14, 2020

View reviewed changes

BenWhitehead reviewed May 14, 2020

View reviewed changes

remove javadoc for verifyNotCommitted

6dfd910

BenWhitehead mentioned this pull request May 15, 2020

deps: update dependency com.google.cloud:google-cloud-conformance-tests to v0.0.11 #218

Closed

1 task

Brian Chen added 3 commits May 15, 2020 13:13

bump version

1aa5e44

Merge branch 'master' into bc/update-proto

3f8b653

update writebatch test

069eba2

thebrianchen added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 16, 2020

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 16, 2020

thebrianchen added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 18, 2020

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 18, 2020

thebrianchen self-assigned this May 18, 2020

thebrianchen added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 18, 2020

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 18, 2020

BenWhitehead merged commit 46a3c51 into master May 19, 2020

BenWhitehead deleted the bc/update-proto branch May 19, 2020 19:17

quartzmo mentioned this pull request Nov 20, 2020

firestore: Replace usage of Write.transform with Write.update_transforms googleapis/google-cloud-ruby#8160

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: replace usages of transform proto with update_transform #213

fix: replace usages of transform proto with update_transform #213

thebrianchen commented May 13, 2020

schmidt-sebastian left a comment

schmidt-sebastian May 13, 2020

thebrianchen May 14, 2020

schmidt-sebastian May 13, 2020

thebrianchen May 14, 2020

schmidt-sebastian May 14, 2020

schmidt-sebastian May 13, 2020

thebrianchen May 14, 2020

schmidt-sebastian May 14, 2020

thebrianchen May 14, 2020

schmidt-sebastian May 13, 2020

thebrianchen May 14, 2020

thebrianchen left a comment

thebrianchen May 14, 2020

thebrianchen May 14, 2020

thebrianchen May 14, 2020

thebrianchen May 14, 2020

schmidt-sebastian left a comment

schmidt-sebastian May 14, 2020

thebrianchen May 14, 2020

schmidt-sebastian May 14, 2020

schmidt-sebastian May 14, 2020

schmidt-sebastian left a comment

BenWhitehead May 14, 2020

thebrianchen May 14, 2020

BenWhitehead commented May 15, 2020

codecov bot commented May 15, 2020 •

edited

BenWhitehead commented May 18, 2020

		@@ -535,13 +527,11 @@ public boolean allowTransform() {
		Mutation mutation = addMutation();

fix: replace usages of transform proto with update_transform #213

fix: replace usages of transform proto with update_transform #213

Conversation

thebrianchen commented May 13, 2020

schmidt-sebastian left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thebrianchen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schmidt-sebastian left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schmidt-sebastian left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BenWhitehead commented May 15, 2020

codecov bot commented May 15, 2020 • edited

Codecov Report

BenWhitehead commented May 18, 2020

codecov bot commented May 15, 2020 •

edited