Merge d9a59e2 into 41890a0

Author: milaGGL

firebase-firestore/CHANGELOG.md

@@ -1,5 +1,5 @@
 # Unreleased
-
+- [feature] Implemented an optimization in the local cache synchronization logic that reduces the number of billed document reads when documents were deleted on the server while the client was not actively listening to the query (e.g. while the client was offline). (GitHub [#4982](//github.com/firebase/firebase-android-sdk/pull/4982){: .external})
 
 # 24.6.0
 * [fixed] Fixed stack overflow caused by deeply nested server timestamps.

firebase-firestore/src/androidTest/java/com/google/firebase/firestore/QueryTest.java

@@ -38,6 +38,7 @@
 import com.google.android.gms.tasks.Task;
 import com.google.common.collect.Lists;
 import com.google.firebase.firestore.Query.Direction;
+import com.google.firebase.firestore.remote.TestingHooksUtil.ExistenceFilterBloomFilterInfo;
 import com.google.firebase.firestore.remote.TestingHooksUtil.ExistenceFilterMismatchInfo;
 import com.google.firebase.firestore.testutil.EventAccumulator;
 import com.google.firebase.firestore.testutil.IntegrationTestUtil;
@@ -1037,101 +1038,135 @@ public void testMultipleUpdatesWhileOffline() {
   }
 
   @Test
-  public void resumingAQueryShouldUseExistenceFilterToDetectDeletes() throws Exception {
+  public void resumingAQueryShouldUseBloomFilterToAvoidFullRequery() throws Exception {
     // Prepare the names and contents of the 100 documents to create.
     Map<String, Map<String, Object>> testData = new HashMap<>();
     for (int i = 0; i < 100; i++) {
       testData.put("doc" + (1000 + i), map("key", 42));
     }
 
-    // Create 100 documents in a new collection.
-    CollectionReference collection = testCollectionWithDocs(testData);
-
-    // Run a query to populate the local cache with the 100 documents and a resume token.
-    List<DocumentReference> createdDocuments = new ArrayList<>();
-    {
-      QuerySnapshot querySnapshot = waitFor(collection.get());
-      assertWithMessage("querySnapshot1").that(querySnapshot.size()).isEqualTo(100);
-      for (DocumentSnapshot documentSnapshot : querySnapshot.getDocuments()) {
-        createdDocuments.add(documentSnapshot.getReference());
+    // Each iteration of the "while" loop below runs a single iteration of the test. The test will
+    // be run multiple times only if a bloom filter false positive occurs.
+    int attemptNumber = 0;
+    while (true) {
+      attemptNumber++;
+
+      // Create 100 documents in a new collection.
+      CollectionReference collection = testCollectionWithDocs(testData);
+
+      // Run a query to populate the local cache with the 100 documents and a resume token.
+      List<DocumentReference> createdDocuments = new ArrayList<>();
+      {
+        QuerySnapshot querySnapshot = waitFor(collection.get());
+        assertWithMessage("querySnapshot1").that(querySnapshot.size()).isEqualTo(100);
+        for (DocumentSnapshot documentSnapshot : querySnapshot.getDocuments()) {
+          createdDocuments.add(documentSnapshot.getReference());
+        }
+      }
+      assertWithMessage("createdDocuments").that(createdDocuments).hasSize(100);
+
+      // Delete 50 of the 100 documents. Do this in a transaction, rather than
+      // DocumentReference.delete(), to avoid affecting the local cache.
+      HashSet<String> deletedDocumentIds = new HashSet<>();
+      waitFor(
+          collection
+              .getFirestore()
+              .runTransaction(
+                  transaction -> {
+                    for (int i = 0; i < createdDocuments.size(); i += 2) {
+                      DocumentReference documentToDelete = createdDocuments.get(i);
+                      transaction.delete(documentToDelete);
+                      deletedDocumentIds.add(documentToDelete.getId());
+                    }
+                    return null;
+                  }));
+      assertWithMessage("deletedDocumentIds").that(deletedDocumentIds).hasSize(50);
+
+      // Wait for 10 seconds, during which Watch will stop tracking the query and will send an
+      // existence filter rather than "delete" events when the query is resumed.
+      Thread.sleep(10000);
+
+      // Resume the query and save the resulting snapshot for verification. Use some internal
+      // testing hooks to "capture" the existence filter mismatches to verify that Watch sent a
+      // bloom filter, and it was used to avert a full requery.
+      AtomicReference<QuerySnapshot> snapshot2Ref = new AtomicReference<>();
+      ArrayList<ExistenceFilterMismatchInfo> existenceFilterMismatches =
+          captureExistenceFilterMismatches(
+              () -> {
+                QuerySnapshot querySnapshot = waitFor(collection.get());
+                snapshot2Ref.set(querySnapshot);
+              });
+      QuerySnapshot snapshot2 = snapshot2Ref.get();
+
+      // Verify that the snapshot from the resumed query contains the expected documents; that is,
+      // that it contains the 50 documents that were _not_ deleted.
+      // TODO(b/270731363): Remove the "if" condition below once the Firestore Emulator is fixed to
+      // send an existence filter. At the time of writing, the Firestore emulator fails to send an
+      // existence filter, resulting in the client including the deleted documents in the snapshot
+      // of the resumed query.
+      if (!(isRunningAgainstEmulator() && snapshot2.size() == 100)) {
+        HashSet<String> actualDocumentIds = new HashSet<>();
+        for (DocumentSnapshot documentSnapshot : snapshot2.getDocuments()) {
+          actualDocumentIds.add(documentSnapshot.getId());
+        }
+        HashSet<String> expectedDocumentIds = new HashSet<>();
+        for (DocumentReference documentRef : createdDocuments) {
+          if (!deletedDocumentIds.contains(documentRef.getId())) {
+            expectedDocumentIds.add(documentRef.getId());
+          }
+        }
+        assertWithMessage("snapshot2.docs")
+            .that(actualDocumentIds)
+            .containsExactlyElementsIn(expectedDocumentIds);
       }
-    }
-    assertWithMessage("createdDocuments").that(createdDocuments).hasSize(100);
 
-    // Delete 50 of the 100 documents. Do this in a transaction, rather than
-    // DocumentReference.delete(), to avoid affecting the local cache.
-    HashSet<String> deletedDocumentIds = new HashSet<>();
-    waitFor(
-        collection
-            .getFirestore()
-            .runTransaction(
-                transaction -> {
-                  for (int i = 0; i < createdDocuments.size(); i += 2) {
-                    DocumentReference documentToDelete = createdDocuments.get(i);
-                    transaction.delete(documentToDelete);
-                    deletedDocumentIds.add(documentToDelete.getId());
-                  }
-                  return null;
-                }));
-    assertWithMessage("deletedDocumentIds").that(deletedDocumentIds).hasSize(50);
-
-    // Wait for 10 seconds, during which Watch will stop tracking the query and will send an
-    // existence filter rather than "delete" events when the query is resumed.
-    Thread.sleep(10000);
-
-    // Resume the query and save the resulting snapshot for verification. Use some internal testing
-    // hooks to "capture" the existence filter mismatches to verify them.
-    AtomicReference<QuerySnapshot> snapshot2Ref = new AtomicReference<>();
-    ArrayList<ExistenceFilterMismatchInfo> existenceFilterMismatches =
-        captureExistenceFilterMismatches(
-            () -> {
-              QuerySnapshot querySnapshot = waitFor(collection.get());
-              snapshot2Ref.set(querySnapshot);
-            });
-    QuerySnapshot snapshot2 = snapshot2Ref.get();
-
-    // Verify that the snapshot from the resumed query contains the expected documents; that is,
-    // that it contains the 50 documents that were _not_ deleted.
-    // TODO(b/270731363): Remove the "if" condition below once the Firestore Emulator is fixed to
-    // send an existence filter. At the time of writing, the Firestore emulator fails to send an
-    // existence filter, resulting in the client including the deleted documents in the snapshot
-    // of the resumed query.
-    if (!(isRunningAgainstEmulator() && snapshot2.size() == 100)) {
-      HashSet<String> actualDocumentIds = new HashSet<>();
-      for (DocumentSnapshot documentSnapshot : snapshot2.getDocuments()) {
-        actualDocumentIds.add(documentSnapshot.getId());
+      // Skip the verification of the existence filter mismatch when testing against the Firestore
+      // emulator because the Firestore emulator does not include the `unchanged_names` bloom filter
+      // when it sends ExistenceFilter messages. Some day the emulator _may_ implement this logic,
+      // at which time this short-circuit can be removed.
+      if (isRunningAgainstEmulator()) {
+        return;
       }
-      HashSet<String> expectedDocumentIds = new HashSet<>();
-      for (DocumentReference documentRef : createdDocuments) {
-        if (!deletedDocumentIds.contains(documentRef.getId())) {
-          expectedDocumentIds.add(documentRef.getId());
-        }
+
+      // Verify that Watch sent an existence filter with the correct counts when the query was
+      // resumed.
+      assertWithMessage("Watch should have sent exactly 1 existence filter")
+          .that(existenceFilterMismatches)
+          .hasSize(1);
+      ExistenceFilterMismatchInfo existenceFilterMismatchInfo = existenceFilterMismatches.get(0);
+      assertWithMessage("localCacheCount")
+          .that(existenceFilterMismatchInfo.localCacheCount())
+          .isEqualTo(100);
+      assertWithMessage("existenceFilterCount")
+          .that(existenceFilterMismatchInfo.existenceFilterCount())
+          .isEqualTo(50);
+
+      // Verify that Watch sent a valid bloom filter.
+      ExistenceFilterBloomFilterInfo bloomFilter = existenceFilterMismatchInfo.bloomFilter();
+      assertWithMessage("The bloom filter specified in the existence filter")
+          .that(bloomFilter)
+          .isNotNull();
+      assertWithMessage("hashCount").that(bloomFilter.hashCount()).isGreaterThan(0);
+      assertWithMessage("bitmapLength").that(bloomFilter.bitmapLength()).isGreaterThan(0);
+      assertWithMessage("padding").that(bloomFilter.padding()).isGreaterThan(0);
+      assertWithMessage("padding").that(bloomFilter.padding()).isLessThan(8);
+
+      // Verify that the bloom filter was successfully used to avert a full requery. If a false
+      // positive occurred then retry the entire test. Although statistically rare, false positives
+      // are expected to happen occasionally. When a false positive _does_ happen, just retry the
+      // test with a different set of documents. If that retry _also_ experiences a false positive,
+      // then fail the test because that is so improbable that something must have gone wrong.
+      if (attemptNumber == 1 && !bloomFilter.applied()) {
+        continue;
       }
-      assertWithMessage("snapshot2.docs")
-          .that(actualDocumentIds)
-          .containsExactlyElementsIn(expectedDocumentIds);
-    }
 
-    // Skip the verification of the existence filter mismatch when testing against the Firestore
-    // emulator because the Firestore emulator fails to to send an existence filter at all.
-    // TODO(b/270731363): Enable the verification of the existence filter mismatch once the
-    // Firestore emulator is fixed to send an existence filter.
-    if (isRunningAgainstEmulator()) {
-      return;
-    }
+      assertWithMessage("bloom filter successfully applied with attemptNumber=" + attemptNumber)
+          .that(bloomFilter.applied())
+          .isTrue();
 
-    // Verify that Watch sent an existence filter with the correct counts when the query was
-    // resumed.
-    assertWithMessage("Watch should have sent exactly 1 existence filter")
-        .that(existenceFilterMismatches)
-        .hasSize(1);
-    ExistenceFilterMismatchInfo existenceFilterMismatchInfo = existenceFilterMismatches.get(0);
-    assertWithMessage("localCacheCount")
-        .that(existenceFilterMismatchInfo.localCacheCount())
-        .isEqualTo(100);
-    assertWithMessage("existenceFilterCount")
-        .that(existenceFilterMismatchInfo.existenceFilterCount())
-        .isEqualTo(50);
+      // Break out of the test loop now that the test passes.
+      break;
+    }
   }
 
   @Test

firebase-firestore/src/androidTest/java/com/google/firebase/firestore/remote/TestingHooksUtil.java

@@ -15,6 +15,7 @@
 package com.google.firebase.firestore.remote;
 
 import androidx.annotation.NonNull;
+import androidx.annotation.Nullable;
 import com.google.firebase.firestore.ListenerRegistration;
 import java.util.ArrayList;
 
@@ -79,5 +80,37 @@ public int localCacheCount() {
     public int existenceFilterCount() {
       return info.existenceFilterCount();
     }
+
+    @Nullable
+    public ExistenceFilterBloomFilterInfo bloomFilter() {
+      TestingHooks.ExistenceFilterBloomFilterInfo bloomFilterInfo = info.bloomFilter();
+      return bloomFilterInfo == null ? null : new ExistenceFilterBloomFilterInfo(bloomFilterInfo);
+    }
+  }
+
+  /** @see TestingHooks.ExistenceFilterBloomFilterInfo */
+  public static final class ExistenceFilterBloomFilterInfo {
+
+    private final TestingHooks.ExistenceFilterBloomFilterInfo info;
+
+    ExistenceFilterBloomFilterInfo(@NonNull TestingHooks.ExistenceFilterBloomFilterInfo info) {
+      this.info = info;
+    }
+
+    public boolean applied() {
+      return info.applied();
+    }
+
+    public int hashCount() {
+      return info.hashCount();
+    }
+
+    public int bitmapLength() {
+      return info.bitmapLength();
+    }
+
+    public int padding() {
+      return info.padding();
+    }
   }
 }

firebase-firestore/src/main/java/com/google/firebase/firestore/core/SyncEngine.java

@@ -205,13 +205,14 @@ public int listen(Query query) {
     hardAssert(!queryViewsByQuery.containsKey(query), "We already listen to query: %s", query);
 
     TargetData targetData = localStore.allocateTarget(query.toTarget());
-    remoteStore.listen(targetData);
 
     ViewSnapshot viewSnapshot =
         initializeViewAndComputeSnapshot(
             query, targetData.getTargetId(), targetData.getResumeToken());
     syncEngineListener.onViewSnapshots(Collections.singletonList(viewSnapshot));
 
+    remoteStore.listen(targetData);
+
     return targetData.getTargetId();
   }
 
@@ -430,7 +431,7 @@ public void handleRejectedListen(int targetId, Status error) {
           new RemoteEvent(
               SnapshotVersion.NONE,
               /* targetChanges= */ Collections.emptyMap(),
-              /* targetMismatches= */ Collections.emptySet(),
+              /* targetMismatches= */ Collections.emptyMap(),
               documentUpdates,
               limboDocuments);
       handleRemoteEvent(event);

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

@@ -260,7 +260,8 @@ TargetData decodeTargetData(com.google.firebase.firestore.proto.Target targetPro
         QueryPurpose.LISTEN,
         version,
         lastLimboFreeSnapshotVersion,
-        resumeToken);
+        resumeToken,
+        null);
   }
 
   public com.google.firestore.bundle.BundledQuery encodeBundledQuery(BundledQuery bundledQuery) {

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

@@ -425,7 +425,7 @@ public ImmutableSortedMap<DocumentKey, Document> applyRemoteEvent(RemoteEvent re
             targetCache.addMatchingKeys(change.getAddedDocuments(), targetId);
 
             TargetData newTargetData = oldTargetData.withSequenceNumber(sequenceNumber);
-            if (remoteEvent.getTargetMismatches().contains(targetId)) {
+            if (remoteEvent.getTargetMismatches().containsKey(targetId)) {
               newTargetData =
                   newTargetData
                       .withResumeToken(ByteString.EMPTY, SnapshotVersion.NONE)

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

@@ -22,6 +22,11 @@ public enum QueryPurpose {
   /** The query was used to refill a query after an existence filter mismatch. */
   EXISTENCE_FILTER_MISMATCH,
 
+  /**
+   * The query target was used if the query is the result of a false positive in the bloom filter.
+   */
+  EXISTENCE_FILTER_MISMATCH_BLOOM,
+
   /** The query was used to resolve a limbo document. */
   LIMBO_RESOLUTION,
 }