Overlay on top of mutation list #2962

wu-hui · 2021-09-08T16:02:58Z

No description provided.

google-oss-bot · 2021-09-08T16:11:32Z

Coverage Report

Affected SDKs

No changes between base commit (cf7680d) and head commit (aa3208b8).

Test Logs

Notes

HTML coverage reports can be produced locally with ./gradlew <product>:checkCoverage.
Report files are located at <product-build-dir>/reports/jacoco/.

Head commit (aa3208b8) is created by Prow via merging commits: cf7680d 95e2542.

google-oss-bot · 2021-09-08T16:13:42Z

Binary Size Report

Affected SDKs

firebase-firestore

Type Base (cf7680d) Head (aa3208b8) Diff

aar 1.20 MB 1.20 MB +1.13 kB (+0.1%)

apk (release) 3.24 MB 3.24 MB +316 B (+0.0%)

Test Logs

Notes

Head commit (aa3208b8) is created by Prow via merging commits: cf7680d 95e2542.

schmidt-sebastian

Thanks for playin with this. Some more ideas for simplification - some or all of which might not work.

firebase-firestore/src/main/java/com/google/firebase/firestore/model/mutation/Mutation.java

...base-firestore/src/main/java/com/google/firebase/firestore/model/mutation/PatchMutation.java

firebase-firestore/src/main/java/com/google/firebase/firestore/model/mutation/Mutation.java

wu-hui

Thanks. I am adding tests back to validate this impl...and cleaning it up.

...base-firestore/src/main/java/com/google/firebase/firestore/model/mutation/PatchMutation.java

firebase-firestore/src/main/java/com/google/firebase/firestore/model/mutation/Mutation.java

wu-hui · 2021-09-09T16:25:44Z

firebase-firestore/src/test/java/com/google/firebase/firestore/model/MutationSquashTest.java

+    assertEquals(doc, original);
+  }
+
+  // TODO(overlay): This test fails because deleting nested fields like bar.baz leaves an empty bar


@schmidt-sebastian This currently fails, but I do not think this is an issue, we should discuss this.

I think this is an issue (thanks for adding the test!). This changes the behavior - we should be left with an empty map. I don't (yet) know how to solve this though.

schmidt-sebastian

This looks good! Most of the comments are nits about a repeated pattern that we should be able to fix easily.

schmidt-sebastian · 2021-09-15T20:11:27Z

...ase-firestore/src/main/java/com/google/firebase/firestore/model/mutation/DeleteMutation.java

+  protected ObjectValue getValue() {
+    return null;
+  }
+
+  @Nullable
+  protected FieldMask getMask() {
+    return null;
  }
+  ;


These are not needed anymore.

schmidt-sebastian · 2021-09-15T20:12:19Z

...ase-firestore/src/main/java/com/google/firebase/firestore/model/mutation/DeleteMutation.java

@@ -68,11 +70,26 @@ public void applyToRemoteDocument(MutableDocument document, MutationResult mutat
  }

  @Override
-  public void applyToLocalView(MutableDocument document, Timestamp localWriteTime) {
+  public FieldMask applyToLocalView(
+      MutableDocument document, Timestamp localWriteTime, FieldMask mask) {


previousMask? You probably also want to re-order the arguments and move localWriteTime last. The first two arguments then together describe the previous state,

schmidt-sebastian · 2021-09-15T20:17:33Z

firebase-firestore/src/main/java/com/google/firebase/firestore/model/mutation/FieldMask.java

+  }
+
+  public static FieldMask emptyMask() {
+    return new FieldMask(new HashSet<FieldPath>(), Scope.NONE);


None+[] should behave similarly to All_Fields+[]. We should be able to reduce the two field mask kinds to set and unset, which we currently represent as null or FieldMask. Is it possible to use a nullable FieldMask again?

null could potentially have two meaning:

ALL_FIELDS as currently used by Set and Delete.

NONE, as the initial mask to specify that no mutations have been applied. If a mutation's precondition failed to match, the resulting mask stays unchanged.

I personally find isAllFields checks carry more meaning then null checks.

I am still trying to get it to work without this change (will continue tomorrow).

Reasons I advocate against it:

It exposes a lot of state on the FieldMask class, some of which is mutually exclusive and only sensical if the consumer understand the underlying states. I wish we could somehow redesign this so that the mask type is hidden from the API.

It breaks getMask()

It breaks covers() (which can be fixed)

We would also need to fix equals+hashCode.

I will report back tomorrow if I can come up with a solution.

Follow up PR: #2983

Let me know what you think.

Note that the changes I made uncover a versioning issue that exists in this PR as well (there is a comment in the code changes explaining it).

LGTM for the follow up, and thanks for doing it!

Re: version issue, this could be fixed by having a version update in the final mutation. We do need to serialize this as well. Thoughts?

schmidt-sebastian · 2021-09-15T20:18:24Z

firebase-firestore/src/main/java/com/google/firebase/firestore/model/mutation/Mutation.java

@@ -183,11 +187,13 @@ static SnapshotVersion getPostMutationVersion(MutableDocument document) {
   * result of applying a transform) for use when applying a transform locally.
   *
   * @param localWriteTime The local time of the mutation (used to generate ServerTimestampValues).
-   * @param mutableDocument The current state of the document after applying all previous mutations.
+   * @param mutableDocument The document to apply transforms on.
+   * @param mutation The mutation to be applied to {@code mutableDocument} to get field values on


The field values come from mutableDocument, don't they?

This is residual I forgot to remove from previous iterations.

schmidt-sebastian · 2021-09-15T20:19:02Z

firebase-firestore/src/main/java/com/google/firebase/firestore/model/mutation/Mutation.java

@@ -198,6 +204,14 @@ static SnapshotVersion getPostMutationVersion(MutableDocument document) {
    return transformResults;
  }

+  protected List<FieldPath> getFieldTransformPaths() {


This can live in PatchMutation.

schmidt-sebastian · 2021-09-15T20:50:11Z

firebase-firestore/src/test/java/com/google/firebase/firestore/model/MutationSquashTest.java

+      if (doc.isNoDocument()) {
+        squashed = new DeleteMutation(doc.getKey(), Precondition.NONE);
+      } else {
+        squashed = new SetMutation(doc.getKey(), doc.getData(), Precondition.NONE);


Do we need to handle the case where all preconditions fail to apply? I think we don't but we should verify via tests.

Added a test.

schmidt-sebastian · 2021-09-15T20:50:39Z

firebase-firestore/src/test/java/com/google/firebase/firestore/model/MutationSquashTest.java

+      ObjectValue docValue = doc.getData();
+      ObjectValue objectValue = new ObjectValue();


Can you improve the names here?

schmidt-sebastian · 2021-09-15T20:59:47Z

firebase-firestore/src/test/java/com/google/firebase/firestore/model/MutationSquashTest.java

+          // If it is deleting a nested field, we take the immediate parent as the mask used to
+          // construct resulting mutation.
+          // Justification: Nested fields can create parent fields implicitly, if they are
+          // deleted in later mutations, there is no trace that the empty parent fields should
+          // still remain (which is expected).
+          // Consider mutation (foo.bar 1), then mutation (foo.bar delete()).
+          // This leaves the final result (foo, {}). Despite `doc` has the correct result,
+          // `foo` is not in `mask`, and the resulting mutation will miss `foo` completely without
+          // this logic.


Suggested change

// If it is deleting a nested field, we take the immediate parent as the mask used to

// construct resulting mutation.

// Justification: Nested fields can create parent fields implicitly, if they are

// deleted in later mutations, there is no trace that the empty parent fields should

// still remain (which is expected).

// Consider mutation (foo.bar 1), then mutation (foo.bar delete()).

// This leaves the final result (foo, {}). Despite `doc` has the correct result,

// `foo` is not in `mask`, and the resulting mutation will miss `foo` completely without

// this logic.

// If we are deleting a nested field, we take the immediate parent as the mask used to

// construct resulting mutation.

// Justification: Nested fields can create parent fields implicitly. If only a leaf entry

// deleted in later mutations, the parent field should still remain, but we may have

// lost this information.

// Consider mutation (foo.bar 1), then mutation (foo.bar delete()).

// This leaves the final result (foo, {}). Despite the fact that `doc` has the correct result,

// `foo` is not in `mask`, and the resulting mutation would then miss `foo`.

Manually applied because code moved.

schmidt-sebastian · 2021-09-15T21:10:19Z

firebase-firestore/src/test/java/com/google/firebase/firestore/model/MutationSquashTest.java

+
+  @Test
+  public void testSquashFieldDeletionOfNestedField() {
+    MutableDocument doc = doc("collection/key", 0, map("bar.baz", 1));


This does not created a nested object. Rather it creates an object with a single key of bar.baz.

Yep, I realized it, I can see it is confusing to leave it, changed to foo.

schmidt-sebastian · 2021-09-15T21:31:02Z

firebase-firestore/src/test/java/com/google/firebase/firestore/model/MutationSquashTest.java

+            path = path.popLast();
+          }
+          objectValue.set(path, docValue.get(path));
+          maskSet.add(path);


This code snippet should be unit tested explicitly once moved to production code.

~~I also just figured out why my original proposal was working: ObjectValue.set('a.b', delete()) creates field 'a', which it should not, but it made my original implementation work.~~

Hm, I think the problem that caused my test to fail was that the both the original and the new doc did not have an entry for "bar". Not sure how this happened, but this points to another thing we need to do - we should convert most test to compare against a hard-coded end state rather than a computed end state.

Agree on explicit unit test.

Test against hard coded end state is hard here because of the tests are generated.

With current approach (most of applyToLocalDocument is unchanged, only added mask logic), we have good confidence that overly mutation works as original logic intended.

wu-hui

Thanks for the review!

wu-hui · 2021-09-16T14:08:48Z

...ase-firestore/src/main/java/com/google/firebase/firestore/model/mutation/DeleteMutation.java

@@ -68,11 +70,26 @@ public void applyToRemoteDocument(MutableDocument document, MutationResult mutat
  }

  @Override
-  public void applyToLocalView(MutableDocument document, Timestamp localWriteTime) {
+  public FieldMask applyToLocalView(
+      MutableDocument document, Timestamp localWriteTime, FieldMask mask) {


wu-hui · 2021-09-16T14:09:51Z

...ase-firestore/src/main/java/com/google/firebase/firestore/model/mutation/DeleteMutation.java

+  protected ObjectValue getValue() {
+    return null;
+  }
+
+  @Nullable
+  protected FieldMask getMask() {
+    return null;
  }
+  ;


wu-hui · 2021-09-16T14:14:19Z

firebase-firestore/src/main/java/com/google/firebase/firestore/model/mutation/FieldMask.java

+  }
+
+  public static FieldMask emptyMask() {
+    return new FieldMask(new HashSet<FieldPath>(), Scope.NONE);


null could potentially have two meaning:

ALL_FIELDS as currently used by Set and Delete.

NONE, as the initial mask to specify that no mutations have been applied. If a mutation's precondition failed to match, the resulting mask stays unchanged.

I personally find isAllFields checks carry more meaning then null checks.

wu-hui · 2021-09-16T14:16:23Z

firebase-firestore/src/main/java/com/google/firebase/firestore/model/mutation/Mutation.java

@@ -183,11 +187,13 @@ static SnapshotVersion getPostMutationVersion(MutableDocument document) {
   * result of applying a transform) for use when applying a transform locally.
   *
   * @param localWriteTime The local time of the mutation (used to generate ServerTimestampValues).
-   * @param mutableDocument The current state of the document after applying all previous mutations.
+   * @param mutableDocument The document to apply transforms on.
+   * @param mutation The mutation to be applied to {@code mutableDocument} to get field values on


This is residual I forgot to remove from previous iterations.

wu-hui · 2021-09-16T14:16:29Z

firebase-firestore/src/main/java/com/google/firebase/firestore/model/mutation/Mutation.java

   * @return A map of fields to transform results.
   */
  protected Map<FieldPath, Value> localTransformResults(
-      Timestamp localWriteTime, MutableDocument mutableDocument) {
+      Timestamp localWriteTime, MutableDocument mutableDocument, Mutation mutation) {


wu-hui · 2021-09-16T14:48:59Z

firebase-firestore/src/test/java/com/google/firebase/firestore/model/MutationSquashTest.java

+            path = path.popLast();
+          }
+          objectValue.set(path, docValue.get(path));
+          maskSet.add(path);


Agree on explicit unit test.

Test against hard coded end state is hard here because of the tests are generated.

With current approach (most of applyToLocalDocument is unchanged, only added mask logic), we have good confidence that overly mutation works as original logic intended.

wu-hui · 2021-09-16T14:57:29Z

firebase-firestore/src/test/java/com/google/firebase/firestore/model/MutationSquashTest.java

+    return builder.toString();
+  }
+
+  private <E> List<List<E>> generatePermutations(List<E> mutations) {


Did not know Collections2! Thanks!

wu-hui · 2021-09-16T15:05:17Z

firebase-firestore/src/test/java/com/google/firebase/firestore/model/MutationSquashTest.java

+/** Tests MutationSquasher */
+@RunWith(RobolectricTestRunner.class)
+@Config(manifest = Config.NONE)
+public class MutationSquashTest {


wu-hui · 2021-09-16T15:05:44Z

firebase-firestore/src/test/java/com/google/firebase/firestore/model/MutationSquashTest.java

+  @Test
+  public void testSquashOneSetMutation() {
+    Map<String, Object> data = map("foo", "foo-value", "baz", "baz-value");
+    squashRoundTrips(


I think with current implementation, overlay is a better name.

wu-hui · 2021-09-16T15:06:56Z

firebase-firestore/src/test/java/com/google/firebase/firestore/model/MutationSquashTest.java

+          // If it is deleting a nested field, we take the immediate parent as the mask used to
+          // construct resulting mutation.
+          // Justification: Nested fields can create parent fields implicitly, if they are
+          // deleted in later mutations, there is no trace that the empty parent fields should
+          // still remain (which is expected).
+          // Consider mutation (foo.bar 1), then mutation (foo.bar delete()).
+          // This leaves the final result (foo, {}). Despite `doc` has the correct result,
+          // `foo` is not in `mask`, and the resulting mutation will miss `foo` completely without
+          // this logic.


Manually applied because code moved.

wu-hui · 2021-09-20T14:44:09Z

Pushed a new commit trying to fix the version issue.

Note I have to change the test testAppliesPatchWithMergeToDocuments to use deletedDoc(version:None) to make it pass. This fails without the new logic added in this PR if the version is 1, because post mutation version for NoDocument is always 0.

schmidt-sebastian

Can you rebase against master?

firebase-firestore/src/test/java/com/google/firebase/firestore/model/mutation/MutationTest.java

…/model/mutation/MutationTest.java Co-authored-by: Sebastian Schmidt <[email protected]>

# Conflicts: # firebase-firestore/src/main/java/com/google/firebase/firestore/model/mutation/DeleteMutation.java # firebase-firestore/src/main/java/com/google/firebase/firestore/model/mutation/Mutation.java # firebase-firestore/src/main/java/com/google/firebase/firestore/model/mutation/SetMutation.java # firebase-firestore/src/test/java/com/google/firebase/firestore/model/mutation/MutationTest.java

wu-hui · 2021-09-22T13:31:39Z

Can you rebase against master?

Done..and thanks for the fix!

schmidt-sebastian

Thank you for working through this! This looks great.

schmidt-sebastian and others added 2 commits September 3, 2021 13:46

Squash in Mutations

49642d1

A third impl based on mrschmidt draft

f2fa96a

googlebot added the cla: yes Override cla label Sep 8, 2021

google-oss-bot added the size/L label Sep 8, 2021

schmidt-sebastian reviewed Sep 8, 2021

View reviewed changes

wu-hui commented Sep 9, 2021

View reviewed changes

All working except nested field deletion.

1479e3f

google-oss-bot added size/XL and removed size/L labels Sep 9, 2021

wu-hui assigned schmidt-sebastian Sep 9, 2021

wu-hui commented Sep 9, 2021

View reviewed changes

Improve tests.

060f4d3

wu-hui changed the title ~~A third way to squash.~~ Squash mutation by materializing field transforms Sep 9, 2021

wu-hui changed the base branch from mrschmidt/squashdraft to master September 14, 2021 14:03

wu-hui added 4 commits September 14, 2021 11:23

Not working yet.

a88a8ba

All tests pass

32def67

Delete some comment.

8a68b18

Merge branch 'master' into wuandy/SquashBasedOnSebastianDraft

9a07b34

wu-hui changed the title ~~Squash mutation by materializing field transforms~~ Squash by applying mutations and keep track of mask Sep 15, 2021

wu-hui changed the title ~~Squash by applying mutations and keep track of mask~~ Squash by applying mutations and keeping track of mask Sep 15, 2021

schmidt-sebastian reviewed Sep 15, 2021

View reviewed changes

schmidt-sebastian assigned wu-hui and unassigned schmidt-sebastian Sep 15, 2021

Review.

35d1595

wu-hui commented Sep 16, 2021

View reviewed changes

wu-hui assigned schmidt-sebastian and unassigned wu-hui Sep 16, 2021

wu-hui changed the title ~~Squash by applying mutations and keeping track of mask~~ Overlay on top of mutation list Sep 16, 2021

schmidt-sebastian assigned wu-hui and unassigned schmidt-sebastian Sep 17, 2021

Remove FieldMask (#2983)

9401155

wu-hui assigned schmidt-sebastian and unassigned wu-hui Sep 20, 2021

Attempt to fix version

92e5fc7

schmidt-sebastian mentioned this pull request Sep 21, 2021

Remove special handling in Mutations #2993

Merged

schmidt-sebastian reviewed Sep 21, 2021

View reviewed changes

firebase-firestore/src/test/java/com/google/firebase/firestore/model/mutation/MutationTest.java Outdated Show resolved Hide resolved

schmidt-sebastian assigned wu-hui and unassigned schmidt-sebastian Sep 21, 2021

wu-hui and others added 3 commits September 22, 2021 09:20

Update firebase-firestore/src/test/java/com/google/firebase/firestore…

58c6b77

…/model/mutation/MutationTest.java Co-authored-by: Sebastian Schmidt <[email protected]>

Merge with master to pick up mutation version fix.

95e2542

wu-hui assigned schmidt-sebastian and unassigned wu-hui Sep 22, 2021

schmidt-sebastian approved these changes Sep 22, 2021

View reviewed changes

schmidt-sebastian assigned wu-hui and unassigned schmidt-sebastian Sep 22, 2021

wu-hui merged commit 9815e45 into master Sep 22, 2021

wu-hui deleted the wuandy/SquashBasedOnSebastianDraft branch September 22, 2021 16:28

firebase locked and limited conversation to collaborators Oct 23, 2021

		ObjectValue docValue = doc.getData();
		ObjectValue objectValue = new ObjectValue();

Overlay on top of mutation list #2962

Overlay on top of mutation list #2962

Uh oh!

Conversation

wu-hui commented Sep 8, 2021

Uh oh!

google-oss-bot commented Sep 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Coverage Report

Affected SDKs

Test Logs

Notes

Uh oh!

google-oss-bot commented Sep 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Binary Size Report

Affected SDKs

firebase-firestore

Test Logs

Notes

Uh oh!

schmidt-sebastian left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wu-hui left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

schmidt-sebastian Sep 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

schmidt-sebastian left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

google-oss-bot commented Sep 8, 2021 •

edited

Loading

google-oss-bot commented Sep 8, 2021 •

edited

Loading

`firebase-firestore`

schmidt-sebastian Sep 9, 2021 •

edited

Loading

schmidt-sebastian Sep 15, 2021 •

edited

Loading