Merge be2052b into 671c879

Author: milaGGL

Firestore/CHANGELOG.md

@@ -1,3 +1,9 @@
+# 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). (#INSERT_PR_NUMBER_HERE)
+
 # 10.11.0
 - [feature] Expose MultiDb API for public preview. (#10465)
 - [fixed] Fixed a compilation warning related to integer casting. (#11332)

Firestore/Example/Firestore.xcodeproj/project.pbxproj

@@ -1192,7 +1192,10 @@ - (void)testOrderByEquality {
                      matchesResult:@[ @"doc6", @"doc3" ]];
 }
 
-- (void)testResumingAQueryShouldUseExistenceFilterToDetectDeletes {
+- (void)testResumingAQueryShouldUseBloomFilterToAvoidFullRequery {
+  using firebase::firestore::testutil::CaptureExistenceFilterMismatches;
+  using firebase::firestore::util::TestingHooks;
+
   // Set this test to stop when the first failure occurs because some test assertion failures make
   // the rest of the test not applicable or will even crash.
   [self setContinueAfterFailure:NO];
@@ -1204,100 +1207,135 @@ - (void)testResumingAQueryShouldUseExistenceFilterToDetectDeletes {
     [testDocs setValue:@{@"key" : @42} forKey:[NSString stringWithFormat:@"doc%@", @(1000 + i)]];
   }
 
-  // Create 100 documents in a new collection.
-  FIRCollectionReference *collRef = [self collectionRefWithDocuments:testDocs];
-
-  // Run a query to populate the local cache with the 100 documents and a resume token.
-  FIRQuerySnapshot *querySnapshot1 = [self readDocumentSetForRef:collRef
-                                                          source:FIRFirestoreSourceDefault];
-  XCTAssertEqual(querySnapshot1.count, 100, @"querySnapshot1.count has an unexpected value");
-  NSArray<FIRDocumentReference *> *createdDocuments =
-      FIRDocumentReferenceArrayFromQuerySnapshot(querySnapshot1);
-
-  // Delete 50 of the 100 documents. Do this in a transaction, rather than
-  // [FIRDocumentReference deleteDocument], to avoid affecting the local cache.
-  NSSet<NSString *> *deletedDocumentIds;
-  {
-    NSMutableArray<NSString *> *deletedDocumentIdsAccumulator = [[NSMutableArray alloc] init];
-    XCTestExpectation *expectation = [self expectationWithDescription:@"DeleteTransaction"];
-    [collRef.firestore
-        runTransactionWithBlock:^id _Nullable(FIRTransaction *transaction, NSError **) {
-          for (decltype(createdDocuments.count) i = 0; i < createdDocuments.count; i += 2) {
-            FIRDocumentReference *documentToDelete = createdDocuments[i];
-            [transaction deleteDocument:documentToDelete];
-            [deletedDocumentIdsAccumulator addObject:documentToDelete.documentID];
-          }
-          return @"document deletion successful";
-        }
-        completion:^(id, NSError *) {
-          [expectation fulfill];
-        }];
-    [self awaitExpectation:expectation];
-    deletedDocumentIds = [NSSet setWithArray:deletedDocumentIdsAccumulator];
-  }
-  XCTAssertEqual(deletedDocumentIds.count, 50u, @"deletedDocumentIds has the wrong size");
-
-  // 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.
-  [NSThread sleepForTimeInterval:10.0f];
-
-  // Resume the query and save the resulting snapshot for verification.
-  // Use some internal testing hooks to "capture" the existence filter mismatches to verify them.
-  FIRQuerySnapshot *querySnapshot2;
-  std::vector<firebase::firestore::util::TestingHooks::ExistenceFilterMismatchInfo>
-      existence_filter_mismatches =
-          firebase::firestore::testutil::CaptureExistenceFilterMismatches([&] {
-            querySnapshot2 = [self readDocumentSetForRef:collRef source:FIRFirestoreSourceDefault];
-          });
-
-  // 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 (!([FSTIntegrationTestCase isRunningAgainstEmulator] && querySnapshot2.count == 100)) {
-    NSSet<NSString *> *actualDocumentIds =
-        [NSSet setWithArray:FIRQuerySnapshotGetIDs(querySnapshot2)];
-    NSSet<NSString *> *expectedDocumentIds;
+  // 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.
+    FIRCollectionReference *collRef = [self collectionRefWithDocuments:testDocs];
+
+    // Run a query to populate the local cache with the 100 documents and a resume token.
+    FIRQuerySnapshot *querySnapshot1 = [self readDocumentSetForRef:collRef
+                                                            source:FIRFirestoreSourceDefault];
+    XCTAssertEqual(querySnapshot1.count, 100, @"querySnapshot1.count has an unexpected value");
+    NSArray<FIRDocumentReference *> *createdDocuments =
+        FIRDocumentReferenceArrayFromQuerySnapshot(querySnapshot1);
+
+    // Delete 50 of the 100 documents. Do this in a transaction, rather than
+    // [FIRDocumentReference deleteDocument], to avoid affecting the local cache.
+    NSSet<NSString *> *deletedDocumentIds;
     {
-      NSMutableArray<NSString *> *expectedDocumentIdsAccumulator = [[NSMutableArray alloc] init];
-      for (FIRDocumentReference *documentRef in createdDocuments) {
-        if (![deletedDocumentIds containsObject:documentRef.documentID]) {
-          [expectedDocumentIdsAccumulator addObject:documentRef.documentID];
+      NSMutableArray<NSString *> *deletedDocumentIdsAccumulator = [[NSMutableArray alloc] init];
+      XCTestExpectation *expectation = [self expectationWithDescription:@"DeleteTransaction"];
+      [collRef.firestore
+          runTransactionWithBlock:^id _Nullable(FIRTransaction *transaction, NSError **) {
+            for (decltype(createdDocuments.count) i = 0; i < createdDocuments.count; i += 2) {
+              FIRDocumentReference *documentToDelete = createdDocuments[i];
+              [transaction deleteDocument:documentToDelete];
+              [deletedDocumentIdsAccumulator addObject:documentToDelete.documentID];
+            }
+            return @"document deletion successful";
+          }
+          completion:^(id, NSError *) {
+            [expectation fulfill];
+          }];
+      [self awaitExpectation:expectation];
+      deletedDocumentIds = [NSSet setWithArray:deletedDocumentIdsAccumulator];
+    }
+    XCTAssertEqual(deletedDocumentIds.count, 50u, @"deletedDocumentIds has the wrong size");
+
+    // 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.
+    [NSThread sleepForTimeInterval:10.0f];
+
+    // 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.
+    FIRQuerySnapshot *querySnapshot2;
+    std::vector<TestingHooks::ExistenceFilterMismatchInfo> existence_filter_mismatches =
+        CaptureExistenceFilterMismatches([&] {
+          querySnapshot2 = [self readDocumentSetForRef:collRef source:FIRFirestoreSourceDefault];
+        });
+
+    // 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 (!([FSTIntegrationTestCase isRunningAgainstEmulator] && querySnapshot2.count == 100)) {
+      NSSet<NSString *> *actualDocumentIds =
+          [NSSet setWithArray:FIRQuerySnapshotGetIDs(querySnapshot2)];
+      NSSet<NSString *> *expectedDocumentIds;
+      {
+        NSMutableArray<NSString *> *expectedDocumentIdsAccumulator = [[NSMutableArray alloc] init];
+        for (FIRDocumentReference *documentRef in createdDocuments) {
+          if (![deletedDocumentIds containsObject:documentRef.documentID]) {
+            [expectedDocumentIdsAccumulator addObject:documentRef.documentID];
+          }
         }
+        expectedDocumentIds = [NSSet setWithArray:expectedDocumentIdsAccumulator];
+      }
+      if (![actualDocumentIds isEqualToSet:expectedDocumentIds]) {
+        NSArray<NSString *> *unexpectedDocumentIds =
+            SortedStringsNotIn(actualDocumentIds, expectedDocumentIds);
+        NSArray<NSString *> *missingDocumentIds =
+            SortedStringsNotIn(expectedDocumentIds, actualDocumentIds);
+        XCTFail(@"querySnapshot2 contained %lu documents (expected %lu): "
+                @"%lu unexpected and %lu missing; "
+                @"unexpected documents: %@; missing documents: %@",
+                (unsigned long)actualDocumentIds.count, (unsigned long)expectedDocumentIds.count,
+                (unsigned long)unexpectedDocumentIds.count, (unsigned long)missingDocumentIds.count,
+                [unexpectedDocumentIds componentsJoinedByString:@", "],
+                [missingDocumentIds componentsJoinedByString:@", "]);
       }
-      expectedDocumentIds = [NSSet setWithArray:expectedDocumentIdsAccumulator];
     }
-    if (![actualDocumentIds isEqualToSet:expectedDocumentIds]) {
-      NSArray<NSString *> *unexpectedDocumentIds =
-          SortedStringsNotIn(actualDocumentIds, expectedDocumentIds);
-      NSArray<NSString *> *missingDocumentIds =
-          SortedStringsNotIn(expectedDocumentIds, actualDocumentIds);
-      XCTFail(@"querySnapshot2 contained %lu documents (expected %lu): "
-              @"%lu unexpected and %lu missing; "
-              @"unexpected documents: %@; missing documents: %@",
-              (unsigned long)actualDocumentIds.count, (unsigned long)expectedDocumentIds.count,
-              (unsigned long)unexpectedDocumentIds.count, (unsigned long)missingDocumentIds.count,
-              [unexpectedDocumentIds componentsJoinedByString:@", "],
-              [missingDocumentIds componentsJoinedByString:@", "]);
+
+    // 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 ([FSTIntegrationTestCase isRunningAgainstEmulator]) {
+      return;
     }
-  }
 
-  // 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 ([FSTIntegrationTestCase isRunningAgainstEmulator]) {
-    return;
-  }
+    // Verify that Watch sent an existence filter with the correct counts when the query was
+    // resumed.
+    XCTAssertEqual(existence_filter_mismatches.size(), size_t{1},
+                   @"Watch should have sent exactly 1 existence filter");
+    const TestingHooks::ExistenceFilterMismatchInfo &existenceFilterMismatchInfo =
+        existence_filter_mismatches[0];
+    XCTAssertEqual(existenceFilterMismatchInfo.local_cache_count, 100);
+    XCTAssertEqual(existenceFilterMismatchInfo.existence_filter_count, 50);
+
+    // Verify that Watch sent a valid bloom filter.
+    const absl::optional<TestingHooks::BloomFilterInfo> &bloom_filter =
+        existence_filter_mismatches[0].bloom_filter;
+    XCTAssertTrue(bloom_filter.has_value(),
+                  "Watch should have included a bloom filter in the existence filter");
+    XCTAssertGreaterThan(bloom_filter->hash_count, 0);
+    XCTAssertGreaterThan(bloom_filter->bitmap_length, 0);
+    XCTAssertGreaterThan(bloom_filter->padding, 0);
+    XCTAssertLessThan(bloom_filter->padding, 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 && !bloom_filter->applied) {
+      continue;
+    }
+
+    XCTAssertTrue(bloom_filter->applied,
+                  @"The bloom filter should have been successfully applied with attemptNumber=%@",
+                  @(attemptNumber));
 
-  // Verify that Watch sent an existence filter with the correct counts when the query was resumed.
-  XCTAssertEqual(static_cast<int>(existence_filter_mismatches.size()), 1);
-  firebase::firestore::util::TestingHooks::ExistenceFilterMismatchInfo &info =
-      existence_filter_mismatches[0];
-  XCTAssertEqual(info.local_cache_count, 100);
-  XCTAssertEqual(info.existence_filter_count, 50);
+    // Break out of the test loop now that the test passes.
+    break;
+  }
 }
 
 @end

Firestore/Example/Tests/Integration/API/FIRQueryTests.mm

@@ -115,6 +115,11 @@ void WatchQuery(const TargetData& query) override {
     // Snapshot version is ignored on the wire
     TargetData sentTargetData =
         query.WithResumeToken(query.resume_token(), SnapshotVersion::None());
+
+    if (query.expected_count().has_value()) {
+      sentTargetData = sentTargetData.WithExpectedCount(query.expected_count().value());
+    }
+
     datastore_->IncrementWatchStreamRequests();
     active_targets_[query.target_id()] = sentTargetData;
   }

Firestore/Example/Tests/SpecTests/FSTMockDatastore.mm

@@ -56,6 +56,7 @@
 #include "Firestore/core/src/model/types.h"
 #include "Firestore/core/src/nanopb/message.h"
 #include "Firestore/core/src/nanopb/nanopb_util.h"
+#include "Firestore/core/src/remote/bloom_filter.h"
 #include "Firestore/core/src/remote/existence_filter.h"
 #include "Firestore/core/src/remote/serializer.h"
 #include "Firestore/core/src/remote/watch_change.h"
@@ -71,6 +72,7 @@
 #include "Firestore/core/src/util/to_string.h"
 #include "Firestore/core/test/unit/testutil/testutil.h"
 #include "absl/memory/memory.h"
+#include "absl/strings/escaping.h"
 #include "absl/types/optional.h"
 
 namespace objc = firebase::firestore::objc;
@@ -98,6 +100,8 @@
 using firebase::firestore::nanopb::ByteString;
 using firebase::firestore::nanopb::MakeByteString;
 using firebase::firestore::nanopb::Message;
+using firebase::firestore::remote::BloomFilter;
+using firebase::firestore::remote::BloomFilterParameters;
 using firebase::firestore::remote::DocumentWatchChange;
 using firebase::firestore::remote::ExistenceFilter;
 using firebase::firestore::remote::ExistenceFilterWatchChange;
@@ -115,6 +119,7 @@
 using firebase::firestore::util::MakeStringPtr;
 using firebase::firestore::util::Path;
 using firebase::firestore::util::Status;
+using firebase::firestore::util::StatusOr;
 using firebase::firestore::util::TimerId;
 using firebase::firestore::util::ToString;
 using firebase::firestore::util::WrapCompare;
@@ -327,13 +332,36 @@ - (SnapshotVersion)parseVersion:(NSNumber *_Nullable)version {
   return Version(version.longLongValue);
 }
 
+- (absl::optional<BloomFilterParameters>)parseBloomFilterParameter:
+    (NSDictionary *_Nullable)bloomFilterProto {
+  if (bloomFilterProto == nil) {
+    return absl::nullopt;
+  }
+  NSDictionary *bitsData = bloomFilterProto[@"bits"];
+
+  // Decode base64 string into uint8_t vector. If bitmap is not specified in proto, use default
+  // empty string.
+  NSString *bitmapEncoded = bitsData[@"bitmap"];
+  std::string bitmapDecoded;
+  absl::Base64Unescape([bitmapEncoded cStringUsingEncoding:NSASCIIStringEncoding], &bitmapDecoded);
+  ByteString bitmap(bitmapDecoded);
+
+  // If not specified in proto, default padding and hashCount to 0.
+  int32_t padding = [bitsData[@"padding"] intValue];
+  int32_t hashCount = [bloomFilterProto[@"hashCount"] intValue];
+  return BloomFilterParameters{std::move(bitmap), padding, hashCount};
+}
+
 - (QueryPurpose)parseQueryPurpose:(NSString *)value {
   if ([value isEqualToString:@"TargetPurposeListen"]) {
     return QueryPurpose::Listen;
   }
   if ([value isEqualToString:@"TargetPurposeExistenceFilterMismatch"]) {
     return QueryPurpose::ExistenceFilterMismatch;
   }
+  if ([value isEqualToString:@"TargetPurposeExistenceFilterMismatchBloom"]) {
+    return QueryPurpose::ExistenceFilterMismatchBloom;
+  }
   if ([value isEqualToString:@"TargetPurposeLimboResolution"]) {
     return QueryPurpose::LimboResolution;
   }
@@ -484,8 +512,11 @@ - (void)doWatchFilter:(NSDictionary *)watchFilter {
   NSArray<NSNumber *> *targets = watchFilter[@"targetIds"];
   HARD_ASSERT(targets.count == 1, "ExistenceFilters currently support exactly one target only.");
 
-  ExistenceFilter filter{static_cast<int>(keys.count)};
-  ExistenceFilterWatchChange change{filter, targets[0].intValue};
+  absl::optional<BloomFilterParameters> bloomFilterParameters =
+      [self parseBloomFilterParameter:watchFilter[@"bloomFilter"]];
+
+  ExistenceFilter filter{static_cast<int>(keys.count), std::move(bloomFilterParameters)};
+  ExistenceFilterWatchChange change{std::move(filter), targets[0].intValue};
   [self.driver receiveWatchChange:change snapshotVersion:SnapshotVersion::None()];
 }
 
@@ -702,8 +733,18 @@ - (void)validateEvent:(FSTQueryEvent *)actual matches:(NSDictionary *)expected {
     }
 
     XCTAssertEqual(actual.viewSnapshot.value().document_changes().size(), expectedChanges.size());
-    for (size_t i = 0; i != expectedChanges.size(); ++i) {
-      XCTAssertTrue((actual.viewSnapshot.value().document_changes()[i] == expectedChanges[i]));
+
+    auto comparator = [](const DocumentViewChange &lhs, const DocumentViewChange &rhs) {
+      return lhs.document()->key() < rhs.document()->key();
+    };
+
+    std::vector<DocumentViewChange> expectedChangesSorted = expectedChanges;
+    std::sort(expectedChangesSorted.begin(), expectedChangesSorted.end(), comparator);
+    std::vector<DocumentViewChange> actualChangesSorted =
+        actual.viewSnapshot.value().document_changes();
+    std::sort(actualChangesSorted.begin(), actualChangesSorted.end(), comparator);
+    for (size_t i = 0; i != expectedChangesSorted.size(); ++i) {
+      XCTAssertTrue((actualChangesSorted[i] == expectedChangesSorted[i]));
     }
 
     BOOL expectedHasPendingWrites =
@@ -806,6 +847,10 @@ - (void)validateExpectedState:(nullable NSDictionary *)expectedState {
                 target_data = target_data.WithResumeToken(
                     ByteString(), [self parseVersion:queryData[@"readTime"]]);
               }
+
+              if ([queryData objectForKey:@"expectedCount"] != nil) {
+                target_data = target_data.WithExpectedCount([queryData[@"expectedCount"] intValue]);
+              }
               queries.push_back(std::move(target_data));
             }
             expectedActiveTargets[targetID] = std::move(queries);
@@ -924,7 +969,13 @@ - (void)validateActiveTargets {
     XCTAssertEqual(actual.target_id(), targetData.target_id());
     XCTAssertEqual(actual.snapshot_version(), targetData.snapshot_version());
     XCTAssertEqual(actual.resume_token(), targetData.resume_token());
-
+    if (targetData.expected_count().has_value()) {
+      if (!actual.expected_count().has_value()) {
+        XCTFail(@"Actual target data doesn't have an expected_count.");
+      } else {
+        XCTAssertEqual(actual.expected_count().value(), targetData.expected_count().value());
+      }
+    }
     actualTargets.erase(targetID);
   }