Remove Mutation tombstones (#116)

Author: schmidt-sebastian

firebase-firestore/src/main/java/com/google/firebase/firestore/local/MemoryMutationQueue.java

@@ -29,6 +29,7 @@
 import com.google.firebase.firestore.util.Util;
 import com.google.protobuf.ByteString;
 import java.util.ArrayList;
+import java.util.Collections;
 import java.util.Iterator;
 import java.util.List;
 import javax.annotation.Nullable;
@@ -113,6 +114,7 @@ public void acknowledgeBatch(MutationBatch batch, ByteString streamToken) {
         batchId > highestAcknowledgedBatchId, "Mutation batchIds must be acknowledged in order");
 
     int batchIndex = indexOfExistingBatchId(batchId, "acknowledged");
+    hardAssert(batchIndex == 0, "Can only acknowledge the first batch in the mutation queue");
 
     // Verify that the batch in the queue is the one to be acknowledged.
     MutationBatch check = queue.get(batchIndex);
@@ -121,7 +123,6 @@ public void acknowledgeBatch(MutationBatch batch, ByteString streamToken) {
         "Queue ordering failure: expected batch %d, got batch %d",
         batchId,
         check.getBatchId());
-    hardAssert(!check.isTombstone(), "Can't acknowledge a previously removed batch");
 
     highestAcknowledgedBatchId = batchId;
     lastStreamToken = checkNotNull(streamToken);
@@ -173,14 +174,12 @@ public MutationBatch lookupMutationBatch(int batchId) {
 
     MutationBatch batch = queue.get(index);
     hardAssert(batch.getBatchId() == batchId, "If found batch must match");
-    return batch.isTombstone() ? null : batch;
+    return batch;
   }
 
   @Nullable
   @Override
   public MutationBatch getNextMutationBatchAfterBatchId(int batchId) {
-    int size = queue.size();
-
     // All batches with batchId <= highestAcknowledgedBatchId have been acknowledged so the
     // first unacknowledged batch after batchId will have a batchId larger than both of these
     // values.
@@ -189,21 +188,12 @@ public MutationBatch getNextMutationBatchAfterBatchId(int batchId) {
     // The requested batchId may still be out of range so normalize it to the start of the queue.
     int rawIndex = indexOfBatchId(nextBatchId);
     int index = rawIndex < 0 ? 0 : rawIndex;
-
-    // Finally return the first non-tombstone batch.
-    for (; index < size; index++) {
-      MutationBatch batch = queue.get(index);
-      if (!batch.isTombstone()) {
-        return batch;
-      }
-    }
-
-    return null;
+    return queue.size() > index ? queue.get(index) : null;
   }
 
   @Override
   public List<MutationBatch> getAllMutationBatches() {
-    return getAllLiveMutationBatchesBeforeIndex(queue.size());
+    return Collections.unmodifiableList(queue);
   }
 
   @Override
@@ -305,27 +295,9 @@ public void removeMutationBatch(MutationBatch batch) {
     // Find the position of the first batch for removal. This need not be the first entry in the
     // queue.
     int batchIndex = indexOfExistingBatchId(batch.getBatchId(), "removed");
-    hardAssert(
-        queue.get(batchIndex).getBatchId() == batch.getBatchId(),
-        "Removed batches must exist in the queue");
-
-    // Only actually remove batches if removing at the front of the queue. Previously rejected
-    // batches may have left tombstones in the queue, so expand the removal range to include any
-    // tombstones.
-    if (batchIndex == 0) {
-      int endIndex = 1;
-      for (; endIndex < queue.size(); endIndex++) {
-        MutationBatch currentBatch = queue.get(endIndex);
-        if (!currentBatch.isTombstone()) {
-          break;
-        }
-      }
-
-      queue.subList(batchIndex, endIndex).clear();
+    hardAssert(batchIndex == 0, "Can only remove the first entry of the mutation queue");
 
-    } else {
-      queue.set(batchIndex, queue.get(batchIndex).toTombstone());
-    }
+    queue.remove(0);
 
     // Remove entries from the index too.
     ImmutableSortedSet<DocumentReference> references = batchesByDocumentKey;
@@ -364,24 +336,6 @@ boolean containsKey(DocumentKey key) {
 
   // Helpers
 
-  /**
-   * A private helper that collects all the mutation batches in the queue up to but not including
-   * the given endIndex. All tombstones in the queue are excluded.
-   */
-  private List<MutationBatch> getAllLiveMutationBatchesBeforeIndex(int endIndex) {
-    List<MutationBatch> result = new ArrayList<>(endIndex);
-
-    for (int i = 0; i < endIndex; i++) {
-      MutationBatch batch = queue.get(i);
-
-      if (!batch.isTombstone()) {
-        result.add(batch);
-      }
-    }
-
-    return result;
-  }
-
   /**
    * Finds the index of the given batchId in the mutation queue. This operation is O(1).
    *

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

@@ -19,7 +19,6 @@
 import com.google.firebase.Timestamp;
 import com.google.firebase.firestore.model.DocumentKey;
 import com.google.firebase.firestore.model.MaybeDocument;
-import java.util.Collections;
 import java.util.HashSet;
 import java.util.List;
 import java.util.Set;
@@ -45,6 +44,7 @@ public final class MutationBatch {
   private final List<Mutation> mutations;
 
   public MutationBatch(int batchId, Timestamp localWriteTime, List<Mutation> mutations) {
+    hardAssert(!mutations.isEmpty(), "Cannot create an empty mutation batch");
     this.batchId = batchId;
     this.localWriteTime = localWriteTime;
     this.mutations = mutations;
@@ -164,22 +164,6 @@ public Timestamp getLocalWriteTime() {
     return localWriteTime;
   }
 
-  /**
-   * Returns true if this mutation batch has already been removed from the mutation queue.
-   *
-   * <p>Note that not all implementations of the MutationQueue necessarily use tombstones as a part
-   * of their implementation and generally speaking no code outside the mutation queues should
-   * really care about this.
-   */
-  public boolean isTombstone() {
-    return mutations.isEmpty();
-  }
-
-  /** Converts this batch to a tombstone. */
-  public MutationBatch toTombstone() {
-    return new MutationBatch(batchId, localWriteTime, Collections.emptyList());
-  }
-
   public List<Mutation> getMutations() {
     return mutations;
   }

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

@@ -94,10 +94,10 @@ public void testCountBatches() {
     assertEquals(2, batchCount());
     assertFalse(mutationQueue.isEmpty());
 
-    removeMutationBatches(batch2);
+    removeMutationBatches(batch1);
     assertEquals(1, batchCount());
 
-    removeMutationBatches(batch1);
+    removeMutationBatches(batch2);
     assertEquals(0, batchCount());
     assertTrue(mutationQueue.isEmpty());
   }
@@ -123,7 +123,7 @@ public void testLookupMutationBatch() {
     assertNull(notFound);
 
     List<MutationBatch> batches = createBatches(10);
-    List<MutationBatch> removed = makeHoles(asList(2, 6, 7), batches);
+    List<MutationBatch> removed = removeFirstBatches(3, batches);
 
     // After removing, a batch should not be found
     for (MutationBatch batch : removed) {
@@ -146,10 +146,7 @@ public void testLookupMutationBatch() {
   @Test
   public void testNextMutationBatchAfterBatchId() {
     List<MutationBatch> batches = createBatches(10);
-
-    // This is an array of successors assuming the removals below will happen:
-    List<MutationBatch> afters = asList(batches.get(3), batches.get(8), batches.get(8));
-    List<MutationBatch> removed = makeHoles(asList(2, 6, 7), batches);
+    List<MutationBatch> removed = removeFirstBatches(3, batches);
 
     for (int i = 0; i < batches.size() - 1; i++) {
       MutationBatch current = batches.get(i);
@@ -161,7 +158,7 @@ public void testNextMutationBatchAfterBatchId() {
 
     for (int i = 0; i < removed.size(); i++) {
       MutationBatch current = removed.get(i);
-      MutationBatch next = afters.get(i);
+      MutationBatch next = batches.get(0);
       MutationBatch found = mutationQueue.getNextMutationBatchAfterBatchId(current.getBatchId());
       assertNotNull(found);
       assertEquals(next.getBatchId(), found.getBatchId());
@@ -367,17 +364,17 @@ public void testRemoveMutationBatches() {
     assertEquals(batches, found);
     assertEquals(6, found.size());
 
-    removeMutationBatches(batches.remove(batches.size() - 1));
+    removeMutationBatches(batches.remove(0));
     assertEquals(5, batchCount());
 
     found = mutationQueue.getAllMutationBatches();
     assertEquals(batches, found);
     assertEquals(5, found.size());
 
-    removeMutationBatches(batches.remove(3));
+    removeMutationBatches(batches.remove(0));
     assertEquals(4, batchCount());
 
-    removeMutationBatches(batches.remove(1));
+    removeMutationBatches(batches.remove(0));
     assertEquals(3, batchCount());
 
     found = mutationQueue.getAllMutationBatches();
@@ -460,21 +457,18 @@ private int batchCount() {
   }
 
   /**
-   * Removes entries from from the given <tt>batches</tt> and returns them.
+   * Removes the first n entries from the given <tt>batches</tt> and returns them.
    *
-   * @param holes An list of indexes in the batches list; in increasing order. Indexes are relative
-   *     to the original state of the batches list, not any intermediate state that might occur.
+   * @param n The number of batches to remove..
    * @param batches The list to mutate, removing entries from it.
    * @return A new list containing all the entries that were removed from @a batches.
    */
-  private List<MutationBatch> makeHoles(List<Integer> holes, List<MutationBatch> batches) {
+  private List<MutationBatch> removeFirstBatches(int n, List<MutationBatch> batches) {
     List<MutationBatch> removed = new ArrayList<>();
-    for (int i = 0; i < holes.size(); i++) {
-      int index = holes.get(i) - i;
-      MutationBatch batch = batches.get(index);
+    for (int i = 0; i < n; i++) {
+      MutationBatch batch = batches.get(0);
       removeMutationBatches(batch);
-
-      batches.remove(index);
+      batches.remove(0);
       removed.add(batch);
     }
     return removed;