Remove special handling in Mutations (#2993)

Author: schmidt-sebastian

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

@@ -19,7 +19,6 @@
 import com.google.firebase.Timestamp;
 import com.google.firebase.firestore.model.DocumentKey;
 import com.google.firebase.firestore.model.MutableDocument;
-import com.google.firebase.firestore.model.SnapshotVersion;
 
 /** Represents a Delete operation */
 public final class DeleteMutation extends Mutation {
@@ -72,7 +71,7 @@ public void applyToLocalView(MutableDocument document, Timestamp localWriteTime)
     verifyKeyMatches(document);
 
     if (getPrecondition().isValidFor(document)) {
-      document.convertToNoDocument(SnapshotVersion.NONE);
+      document.convertToNoDocument(document.getVersion()).setHasLocalMutations();
     }
   }
 }

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

@@ -22,7 +22,6 @@
 import com.google.firebase.firestore.model.FieldPath;
 import com.google.firebase.firestore.model.MutableDocument;
 import com.google.firebase.firestore.model.ObjectValue;
-import com.google.firebase.firestore.model.SnapshotVersion;
 import com.google.firestore.v1.Value;
 import java.util.ArrayList;
 import java.util.HashMap;
@@ -136,19 +135,6 @@ void verifyKeyMatches(MutableDocument document) {
         "Can only apply a mutation to a document with the same key");
   }
 
-  /**
-   * Returns the version from the given document for use as the result of a mutation. Mutations are
-   * defined to return the version of the base document only if it is an existing document. Deleted
-   * and unknown documents have a post-mutation version of {@code SnapshotVersion.NONE}.
-   */
-  static SnapshotVersion getPostMutationVersion(MutableDocument document) {
-    if (document.isFoundDocument()) {
-      return document.getVersion();
-    } else {
-      return SnapshotVersion.NONE;
-    }
-  }
-
   /**
    * Creates a list of "transform results" (a transform result is a field value representing the
    * result of applying a transform) for use after a mutation containing transforms has been

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

@@ -138,7 +138,7 @@ public void applyToLocalView(MutableDocument document, Timestamp localWriteTime)
     value.setAll(getPatch());
     value.setAll(transformResults);
     document
-        .convertToFoundDocument(getPostMutationVersion(document), document.getData())
+        .convertToFoundDocument(document.getVersion(), document.getData())
         .setHasLocalMutations();
   }
 

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

@@ -96,9 +96,7 @@ public void applyToLocalView(MutableDocument document, Timestamp localWriteTime)
     Map<FieldPath, Value> transformResults = localTransformResults(localWriteTime, document);
     ObjectValue localValue = value.clone();
     localValue.setAll(transformResults);
-    document
-        .convertToFoundDocument(getPostMutationVersion(document), localValue)
-        .setHasLocalMutations();
+    document.convertToFoundDocument(document.getVersion(), localValue).setHasLocalMutations();
   }
 
   /** Returns the object value to use when setting the document. */

firebase-firestore/src/test/java/com/google/firebase/firestore/local/LocalStoreTestCase.java

@@ -444,8 +444,9 @@ public void testHandlesDeletedDocumentThenSetMutationThenAck() {
     }
 
     writeMutation(setMutation("foo/bar", map("foo", "bar")));
-    assertChanged(doc("foo/bar", 0, map("foo", "bar")).setHasLocalMutations());
-    assertContains(doc("foo/bar", 0, map("foo", "bar")).setHasLocalMutations());
+    int expectedVersion = garbageCollectorIsEager() ? 0 : 2;
+    assertChanged(doc("foo/bar", expectedVersion, map("foo", "bar")).setHasLocalMutations());
+    assertContains(doc("foo/bar", expectedVersion, map("foo", "bar")).setHasLocalMutations());
 
     releaseTarget(targetId);
     acknowledgeMutation(3);
@@ -464,8 +465,8 @@ public void testHandlesSetMutationThenDeletedDocument() {
     assertChanged(doc("foo/bar", 0, map("foo", "bar")).setHasLocalMutations());
 
     applyRemoteEvent(updateRemoteEvent(deletedDoc("foo/bar", 2), asList(targetId), emptyList()));
-    assertChanged(doc("foo/bar", 0, map("foo", "bar")).setHasLocalMutations());
-    assertContains(doc("foo/bar", 0, map("foo", "bar")).setHasLocalMutations());
+    assertChanged(doc("foo/bar", 2, map("foo", "bar")).setHasLocalMutations());
+    assertContains(doc("foo/bar", 2, map("foo", "bar")).setHasLocalMutations());
   }
 
   @Test
@@ -566,7 +567,7 @@ public void testHandlesPatchMutationThenAckThenDocument() {
   public void testHandlesDeleteMutationThenAck() {
     writeMutation(deleteMutation("foo/bar"));
     assertRemoved("foo/bar");
-    assertContains(deletedDoc("foo/bar", 0));
+    assertContains(deletedDoc("foo/bar", 0).setHasLocalMutations());
 
     acknowledgeMutation(1);
     assertRemoved("foo/bar");
@@ -589,7 +590,7 @@ public void testHandlesDocumentThenDeleteMutationThenAck() {
 
     writeMutation(deleteMutation("foo/bar"));
     assertRemoved("foo/bar");
-    assertContains(deletedDoc("foo/bar", 0));
+    assertContains(deletedDoc("foo/bar", 1).setHasLocalMutations());
 
     // Remove the target so only the mutation is pinning the document.
     releaseTarget(targetId);
@@ -608,12 +609,12 @@ public void testHandlesDeleteMutationThenDocumentThenAck() {
     int targetId = allocateQuery(query);
     writeMutation(deleteMutation("foo/bar"));
     assertRemoved("foo/bar");
-    assertContains(deletedDoc("foo/bar", 0));
+    assertContains(deletedDoc("foo/bar", 0).setHasLocalMutations());
 
     applyRemoteEvent(
         updateRemoteEvent(doc("foo/bar", 1, map("it", "base")), asList(targetId), emptyList()));
     assertRemoved("foo/bar");
-    assertContains(deletedDoc("foo/bar", 0));
+    assertContains(deletedDoc("foo/bar", 1).setHasLocalMutations());
 
     releaseTarget(targetId);
     acknowledgeMutation(2);
@@ -731,11 +732,11 @@ public void testHandlesSetMutationsAndPatchMutationOfJustOneTogether() {
   public void testHandlesDeleteMutationThenPatchMutationThenAckThenAck() {
     writeMutation(deleteMutation("foo/bar"));
     assertRemoved("foo/bar");
-    assertContains(deletedDoc("foo/bar", 0));
+    assertContains(deletedDoc("foo/bar", 0).setHasLocalMutations());
 
     writeMutation(patchMutation("foo/bar", map("foo", "bar")));
     assertRemoved("foo/bar");
-    assertContains(deletedDoc("foo/bar", 0));
+    assertContains(deletedDoc("foo/bar", 0).setHasLocalMutations());
 
     acknowledgeMutation(2); // delete mutation
     assertRemoved("foo/bar");
@@ -798,17 +799,17 @@ public void testCollectsGarbageAfterAcknowledgedMutation() {
     writeMutation(deleteMutation("foo/baz"));
     assertContains(doc("foo/bar", 1, map("foo", "bar")).setHasLocalMutations());
     assertContains(doc("foo/bah", 0, map("foo", "bah")).setHasLocalMutations());
-    assertContains(deletedDoc("foo/baz", 0));
+    assertContains(deletedDoc("foo/baz", 0).setHasLocalMutations());
 
     acknowledgeMutation(3);
     assertNotContains("foo/bar");
     assertContains(doc("foo/bah", 0, map("foo", "bah")).setHasLocalMutations());
-    assertContains(deletedDoc("foo/baz", 0));
+    assertContains(deletedDoc("foo/baz", 0).setHasLocalMutations());
 
     acknowledgeMutation(4);
     assertNotContains("foo/bar");
     assertNotContains("foo/bah");
-    assertContains(deletedDoc("foo/baz", 0));
+    assertContains(deletedDoc("foo/baz", 0).setHasLocalMutations());
 
     acknowledgeMutation(5);
     assertNotContains("foo/bar");
@@ -831,17 +832,17 @@ public void testCollectsGarbageAfterRejectedMutation() {
     writeMutation(deleteMutation("foo/baz"));
     assertContains(doc("foo/bar", 1, map("foo", "bar")).setHasLocalMutations());
     assertContains(doc("foo/bah", 0, map("foo", "bah")).setHasLocalMutations());
-    assertContains(deletedDoc("foo/baz", 0));
+    assertContains(deletedDoc("foo/baz", 0).setHasLocalMutations());
 
     rejectMutation(); // patch mutation
     assertNotContains("foo/bar");
     assertContains(doc("foo/bah", 0, map("foo", "bah")).setHasLocalMutations());
-    assertContains(deletedDoc("foo/baz", 0));
+    assertContains(deletedDoc("foo/baz", 0).setHasLocalMutations());
 
     rejectMutation(); // set mutation
     assertNotContains("foo/bar");
     assertNotContains("foo/bah");
-    assertContains(deletedDoc("foo/baz", 0));
+    assertContains(deletedDoc("foo/baz", 0).setHasLocalMutations());
 
     rejectMutation(); // delete mutation
     assertNotContains("foo/bar");

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

@@ -526,7 +526,7 @@ public void testDeleteDeletes() {
 
     Mutation delete = deleteMutation("collection/key");
     delete.applyToLocalView(deletedDoc, Timestamp.now());
-    assertEquals(deletedDoc("collection/key", 0), deletedDoc);
+    assertEquals(deletedDoc("collection/key", 0).setHasLocalMutations(), deletedDoc);
   }
 
   @Test