Delete index entries for deleted documents

Author: schmidt-sebastian

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

@@ -502,6 +502,7 @@ private DocumentChangeResult populateDocumentChanges(
       @Nullable Map<DocumentKey, SnapshotVersion> documentVersions,
       SnapshotVersion globalVersion) {
     Map<DocumentKey, MutableDocument> changedDocs = new HashMap<>();
+    List<DocumentKey> removedDocs = new ArrayList<>();
     Set<DocumentKey> conditionChanged = new HashSet<>();
 
     // Each loop iteration only affects its "own" doc, so it's safe to get all the remote
@@ -525,7 +526,7 @@ private DocumentChangeResult populateDocumentChanges(
       if (doc.isNoDocument() && doc.getVersion().equals(SnapshotVersion.NONE)) {
         // NoDocuments with SnapshotVersion.NONE are used in manufactured events. We remove
         // these documents from cache since we lost access.
-        remoteDocuments.remove(doc.getKey());
+        removedDocs.add(doc.getKey());
         changedDocs.put(key, doc);
       } else if (!existingDoc.isValidDocument()
           || doc.getVersion().compareTo(existingDoc.getVersion()) > 0
@@ -545,6 +546,7 @@ private DocumentChangeResult populateDocumentChanges(
             doc.getVersion());
       }
     }
+    remoteDocuments.removeAll(removedDocs);
     return new DocumentChangeResult(changedDocs, conditionChanged);
   }
 

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

@@ -16,7 +16,9 @@
 
 import com.google.firebase.firestore.core.ListenSequence;
 import com.google.firebase.firestore.model.DocumentKey;
+import java.util.ArrayList;
 import java.util.HashSet;
+import java.util.List;
 import java.util.Set;
 
 /** Provides eager garbage collection for MemoryPersistence. */
@@ -72,11 +74,13 @@ public void onTransactionStarted() {
   @Override
   public void onTransactionCommitted() {
     MemoryRemoteDocumentCache remoteDocuments = persistence.getRemoteDocumentCache();
+    List<DocumentKey> docsToRemove = new ArrayList<>();
     for (DocumentKey key : orphanedDocuments) {
       if (!isReferenced(key)) {
-        remoteDocuments.remove(key);
+        docsToRemove.add(key);
       }
     }
+    remoteDocuments.removeAll(docsToRemove);
     orphanedDocuments = null;
   }
 

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

@@ -21,7 +21,9 @@
 import com.google.firebase.firestore.model.DocumentKey;
 import com.google.firebase.firestore.model.MutableDocument;
 import com.google.firebase.firestore.util.Consumer;
+import java.util.ArrayList;
 import java.util.HashMap;
+import java.util.List;
 import java.util.Map;
 
 /** Provides LRU garbage collection functionality for MemoryPersistence. */
@@ -115,17 +117,17 @@ public int removeTargets(long upperBound, SparseArray<?> activeTargetIds) {
 
   @Override
   public int removeOrphanedDocuments(long upperBound) {
-    int count = 0;
     MemoryRemoteDocumentCache cache = persistence.getRemoteDocumentCache();
+    List<DocumentKey> docsToRemove = new ArrayList<>();
     for (MutableDocument doc : cache.getDocuments()) {
       DocumentKey key = doc.getKey();
       if (!isPinned(key, upperBound)) {
-        cache.remove(key);
+        docsToRemove.add(key);
         orphanedSequenceNumbers.remove(key);
-        count++;
       }
     }
-    return count;
+    cache.removeAll(docsToRemove);
+    return docsToRemove.size();
   }
 
   @Override

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

@@ -20,13 +20,17 @@
 import androidx.annotation.NonNull;
 import com.google.firebase.database.collection.ImmutableSortedMap;
 import com.google.firebase.firestore.core.Query;
+import com.google.firebase.firestore.model.Document;
 import com.google.firebase.firestore.model.DocumentKey;
 import com.google.firebase.firestore.model.FieldIndex.IndexOffset;
 import com.google.firebase.firestore.model.MutableDocument;
 import com.google.firebase.firestore.model.ResourcePath;
 import com.google.firebase.firestore.model.SnapshotVersion;
+import java.util.ArrayList;
+import java.util.Collection;
 import java.util.HashMap;
 import java.util.Iterator;
+import java.util.List;
 import java.util.Map;
 
 /** In-memory cache of remote documents. */
@@ -62,8 +66,15 @@ public void add(MutableDocument document, SnapshotVersion readTime) {
   }
 
   @Override
-  public void remove(DocumentKey key) {
-    docs = docs.remove(key);
+  public void removeAll(Collection<DocumentKey> keys) {
+    hardAssert(indexManager != null, "setIndexManager() not called");
+
+    List<Document> deletedDocs = new ArrayList<>();
+    for (DocumentKey key : keys) {
+      docs = docs.remove(key);
+      deletedDocs.add(MutableDocument.newNoDocument(key, SnapshotVersion.NONE));
+    }
+    indexManager.updateIndexEntries(deletedDocs);
   }
 
   @Override

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

@@ -20,6 +20,7 @@
 import com.google.firebase.firestore.model.FieldIndex;
 import com.google.firebase.firestore.model.MutableDocument;
 import com.google.firebase.firestore.model.SnapshotVersion;
+import java.util.Collection;
 import java.util.Map;
 
 /**
@@ -45,8 +46,8 @@ interface RemoteDocumentCache {
    */
   void add(MutableDocument document, SnapshotVersion readTime);
 
-  /** Removes the cached entry for the given key (no-op if no entry exists). */
-  void remove(DocumentKey documentKey);
+  /** Removes the cached entries for the given keys (no-op if no entry exists). */
+  void removeAll(Collection<DocumentKey> keys);
 
   /**
    * Looks up an entry in the cache.

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

@@ -21,6 +21,8 @@
 import com.google.firebase.firestore.model.DocumentKey;
 import com.google.firebase.firestore.model.ResourcePath;
 import com.google.firebase.firestore.util.Consumer;
+import java.util.ArrayList;
+import java.util.List;
 
 /** Provides LRU functionality for SQLite persistence. */
 class SQLiteLruReferenceDelegate implements ReferenceDelegate, LruDelegate {
@@ -159,6 +161,8 @@ public int removeOrphanedDocuments(long upperBound) {
 
     boolean resultsRemaining = true;
 
+    List<DocumentKey> docsToRemove = new ArrayList<>();
+
     while (resultsRemaining) {
       int rowsProccessed =
           persistence
@@ -171,14 +175,15 @@ public int removeOrphanedDocuments(long upperBound) {
                     DocumentKey key = DocumentKey.fromPath(path);
                     if (!isPinned(key)) {
                       count[0]++;
-                      persistence.getRemoteDocumentCache().remove(key);
+                      docsToRemove.add(key);
                       removeSentinel(key);
                     }
                   });
 
       resultsRemaining = (rowsProccessed == REMOVE_ORPHANED_DOCUMENTS_BATCH_SIZE);
     }
 
+    persistence.getRemoteDocumentCache().removeAll(docsToRemove);
     return count[0];
   }
 

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

@@ -16,6 +16,7 @@
 
 import static com.google.firebase.firestore.util.Assert.fail;
 import static com.google.firebase.firestore.util.Assert.hardAssert;
+import static com.google.firebase.firestore.util.Util.repeatSequence;
 
 import android.content.Context;
 import android.database.Cursor;
@@ -671,23 +672,27 @@ boolean hasMoreSubqueries() {
       return argsIter.hasNext();
     }
 
-    /** Performs the next subquery and returns a {@link Query} object for method chaining. */
-    Query performNextSubquery() {
-      ++subqueriesPerformed;
-
+    private Object[] getNextSubqueryArgs() {
       List<Object> subqueryArgs = new ArrayList<>(argsHead);
-      StringBuilder placeholdersBuilder = new StringBuilder();
       for (int i = 0; argsIter.hasNext() && i < LIMIT - argsHead.size(); i++) {
-        if (i > 0) {
-          placeholdersBuilder.append(", ");
-        }
-        placeholdersBuilder.append("?");
-
         subqueryArgs.add(argsIter.next());
       }
-      String placeholders = placeholdersBuilder.toString();
+      return subqueryArgs.toArray();
+    }
 
-      return db.query(head + placeholders + tail).binding(subqueryArgs.toArray());
+    /** Performs the next subquery and returns a {@link Query} object for method chaining. */
+    Query performNextSubquery() {
+      ++subqueriesPerformed;
+      Object[] subqueryArgs = getNextSubqueryArgs();
+      return db.query(head + repeatSequence("?", subqueryArgs.length, ", ") + tail)
+          .binding(subqueryArgs);
+    }
+
+    /** Executes the next subquery. */
+    void executeNextSubquery() {
+      ++subqueriesPerformed;
+      Object[] subqueryArgs = getNextSubqueryArgs();
+      db.execute(head + repeatSequence("?", subqueryArgs.length, ", ") + tail, subqueryArgs);
     }
 
     /** How many subqueries were performed. */

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

@@ -20,6 +20,7 @@
 import com.google.firebase.Timestamp;
 import com.google.firebase.database.collection.ImmutableSortedMap;
 import com.google.firebase.firestore.core.Query;
+import com.google.firebase.firestore.model.Document;
 import com.google.firebase.firestore.model.DocumentCollections;
 import com.google.firebase.firestore.model.DocumentKey;
 import com.google.firebase.firestore.model.FieldIndex;
@@ -30,6 +31,7 @@
 import com.google.protobuf.InvalidProtocolBufferException;
 import com.google.protobuf.MessageLite;
 import java.util.ArrayList;
+import java.util.Collection;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
@@ -75,10 +77,24 @@ public void add(MutableDocument document, SnapshotVersion readTime) {
   }
 
   @Override
-  public void remove(DocumentKey documentKey) {
-    String path = EncodedPath.encode(documentKey.getPath());
+  public void removeAll(Collection<DocumentKey> keys) {
+    if (keys.isEmpty()) return;
+    List<Object> encodedPaths = new ArrayList<>();
+    List<Document> deletedDocs = new ArrayList<>();
+
+    for (DocumentKey key : keys) {
+      encodedPaths.add(EncodedPath.encode(key.getPath()));
+      deletedDocs.add(MutableDocument.newNoDocument(key, SnapshotVersion.NONE));
+    }
+
+    SQLitePersistence.LongQuery longQuery =
+        new SQLitePersistence.LongQuery(
+            db, "DELETE FROM remote_documents WHERE path IN (", encodedPaths, ")");
+    while (longQuery.hasMoreSubqueries()) {
+      longQuery.executeNextSubquery();
+    }
 
-    db.execute("DELETE FROM remote_documents WHERE path = ?", path);
+    indexManager.updateIndexEntries(deletedDocs);
   }
 
   @Override

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

@@ -27,6 +27,7 @@
 import com.google.firebase.firestore.model.mutation.Mutation;
 import com.google.firebase.firestore.model.mutation.MutationBatch;
 import com.google.protobuf.ByteString;
+import java.util.Collection;
 import java.util.List;
 import java.util.Map;
 
@@ -128,11 +129,10 @@ public void add(MutableDocument document, SnapshotVersion readTime) {
       }
 
       @Override
-      public void remove(DocumentKey documentKey) {
-        subject.remove(documentKey);
+      public void removeAll(Collection<DocumentKey> keys) {
+        subject.removeAll(keys);
       }
 
-      @Nullable
       @Override
       public MutableDocument get(DocumentKey documentKey) {
         MutableDocument result = subject.get(documentKey);

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

@@ -72,6 +72,7 @@ public void setUp() {
     persistence.getReferenceDelegate().setInMemoryPins(new ReferenceSet());
     IndexManager indexManager = persistence.getIndexManager(User.UNAUTHENTICATED);
     indexManager.start();
+    persistence.getRemoteDocumentCache().setIndexManager(indexManager);
     mutationQueue = persistence.getMutationQueue(User.UNAUTHENTICATED, indexManager);
     mutationQueue.start();
   }