Merge pull request #1562 from firebase/wilhuff/cherry-picks

Prepare Firestore release 0.12.6

Author: wilhuff

Firestore/CHANGELOG.md

@@ -1,5 +1,11 @@
 # Unreleased
 
+# v0.12.6
+- [fixed] Fixed an issue where queries returned fewer results than they should,
+  caused by documents that were cached as deleted when they should not have
+  been (#1548). Some cache data is cleared and so clients may use extra
+  bandwidth the first time they launch with this version of the SDK.
+
 # v0.12.5
 - [changed] Internal improvements.
 

Firestore/Example/Tests/Local/FSTLevelDBMigrationsTests.mm

@@ -18,22 +18,29 @@
 
 #include <memory>
 #include <string>
+#include <vector>
 
 #import "Firestore/Protos/objc/firestore/local/Target.pbobjc.h"
 #import "Firestore/Source/Local/FSTLevelDB.h"
 #import "Firestore/Source/Local/FSTLevelDBKey.h"
 #import "Firestore/Source/Local/FSTLevelDBMigrations.h"
+#import "Firestore/Source/Local/FSTLevelDBMutationQueue.h"
 #import "Firestore/Source/Local/FSTLevelDBQueryCache.h"
 
 #include "Firestore/core/src/firebase/firestore/util/ordered_code.h"
+#include "Firestore/core/src/firebase/firestore/util/status.h"
+#include "Firestore/core/test/firebase/firestore/testutil/testutil.h"
+#include "absl/strings/match.h"
 #include "leveldb/db.h"
 
 #import "Firestore/Example/Tests/Local/FSTPersistenceTestHelpers.h"
 
 NS_ASSUME_NONNULL_BEGIN
 
+using firebase::firestore::FirestoreErrorCode;
 using firebase::firestore::local::LevelDbTransaction;
 using firebase::firestore::util::OrderedCode;
+using firebase::firestore::testutil::Key;
 using leveldb::DB;
 using leveldb::Options;
 using leveldb::Status;
@@ -64,54 +71,136 @@ - (void)tearDown {
 - (void)testAddsTargetGlobal {
   FSTPBTargetGlobal *metadata = [FSTLevelDBQueryCache readTargetMetadataFromDB:_db.get()];
   XCTAssertNil(metadata, @"Not expecting metadata yet, we should have an empty db");
-  LevelDbTransaction transaction(_db.get(), "testAddsTargetGlobal");
-  [FSTLevelDBMigrations runMigrationsWithTransaction:&transaction];
-  transaction.Commit();
+  [FSTLevelDBMigrations runMigrationsWithDatabase:_db.get()];
+
   metadata = [FSTLevelDBQueryCache readTargetMetadataFromDB:_db.get()];
   XCTAssertNotNil(metadata, @"Migrations should have added the metadata");
 }
 
 - (void)testSetsVersionNumber {
-  LevelDbTransaction transaction(_db.get(), "testSetsVersionNumber");
-  FSTLevelDBSchemaVersion initial =
-      [FSTLevelDBMigrations schemaVersionWithTransaction:&transaction];
-  XCTAssertEqual(0, initial, "No version should be equivalent to 0");
-
-  // Pick an arbitrary high migration number and migrate to it.
-  [FSTLevelDBMigrations runMigrationsWithTransaction:&transaction];
-  FSTLevelDBSchemaVersion actual = [FSTLevelDBMigrations schemaVersionWithTransaction:&transaction];
-  XCTAssertGreaterThan(actual, 0, @"Expected to migrate to a schema version > 0");
+  {
+    LevelDbTransaction transaction(_db.get(), "testSetsVersionNumber before");
+    FSTLevelDBSchemaVersion initial =
+        [FSTLevelDBMigrations schemaVersionWithTransaction:&transaction];
+    XCTAssertEqual(0, initial, "No version should be equivalent to 0");
+  }
+
+  {
+    // Pick an arbitrary high migration number and migrate to it.
+    [FSTLevelDBMigrations runMigrationsWithDatabase:_db.get()];
+
+    LevelDbTransaction transaction(_db.get(), "testSetsVersionNumber after");
+    FSTLevelDBSchemaVersion actual =
+        [FSTLevelDBMigrations schemaVersionWithTransaction:&transaction];
+    XCTAssertGreaterThan(actual, 0, @"Expected to migrate to a schema version > 0");
+  }
 }
 
-- (void)testCountsQueries {
-  NSUInteger expected = 50;
+#define ASSERT_NOT_FOUND(transaction, key)                \
+  do {                                                    \
+    std::string unused_result;                            \
+    Status status = transaction.Get(key, &unused_result); \
+    XCTAssertTrue(status.IsNotFound());                   \
+  } while (0)
+
+#define ASSERT_FOUND(transaction, key)                    \
+  do {                                                    \
+    std::string unused_result;                            \
+    Status status = transaction.Get(key, &unused_result); \
+    XCTAssertTrue(status.ok());                           \
+  } while (0)
+
+- (void)testDropsTheQueryCache {
+  NSString *userID = @"user";
+  FSTBatchID batchID = 1;
+  FSTTargetID targetID = 2;
+
+  FSTDocumentKey *key1 = Key("documents/1");
+  FSTDocumentKey *key2 = Key("documents/2");
+
+  std::string targetKeys[] = {
+      [FSTLevelDBTargetKey keyWithTargetID:targetID],
+      [FSTLevelDBTargetDocumentKey keyWithTargetID:targetID documentKey:key1],
+      [FSTLevelDBTargetDocumentKey keyWithTargetID:targetID documentKey:key2],
+      [FSTLevelDBDocumentTargetKey keyWithDocumentKey:key1 targetID:targetID],
+      [FSTLevelDBDocumentTargetKey keyWithDocumentKey:key2 targetID:targetID],
+      [FSTLevelDBQueryTargetKey keyWithCanonicalID:"foo.bar.baz" targetID:targetID],
+  };
+
+  // Keys that should not be modified by the dropping the query cache
+  std::string preservedKeys[] = {
+      [self dummyKeyForTable:"targetA"],
+      [FSTLevelDBMutationQueueKey keyWithUserID:userID],
+      [FSTLevelDBMutationKey keyWithUserID:userID batchID:batchID],
+  };
+
+  [FSTLevelDBMigrations runMigrationsWithDatabase:_db.get() upToVersion:2];
   {
     // Setup some targets to be counted in the migration.
-    LevelDbTransaction transaction(_db.get(), "testCountsQueries setup");
-    for (int i = 0; i < expected; i++) {
-      std::string key = [FSTLevelDBTargetKey keyWithTargetID:i];
-      transaction.Put(key, "dummy");
+    LevelDbTransaction transaction(_db.get(), "testDropsTheQueryCache setup");
+    for (const std::string &key : targetKeys) {
+      transaction.Put(key, "target");
+    }
+    for (const std::string &key : preservedKeys) {
+      transaction.Put(key, "preserved");
     }
-    // Add a dummy entry after the targets to make sure the iteration is correctly bounded.
-    // Use a table that would sort logically right after that table 'target'.
-    std::string dummyKey;
-    // Magic number that indicates a table name follows. Needed to mimic the prefix to the target
-    // table.
-    OrderedCode::WriteSignedNumIncreasing(&dummyKey, 5);
-    OrderedCode::WriteString(&dummyKey, "targetA");
-    transaction.Put(dummyKey, "dummy");
     transaction.Commit();
   }
 
+  [FSTLevelDBMigrations runMigrationsWithDatabase:_db.get() upToVersion:3];
   {
-    LevelDbTransaction transaction(_db.get(), "testCountsQueries");
-    [FSTLevelDBMigrations runMigrationsWithTransaction:&transaction];
-    transaction.Commit();
+    LevelDbTransaction transaction(_db.get(), "testDropsTheQueryCache");
+    for (const std::string &key : targetKeys) {
+      ASSERT_NOT_FOUND(transaction, key);
+    }
+    for (const std::string &key : preservedKeys) {
+      ASSERT_FOUND(transaction, key);
+    }
+
     FSTPBTargetGlobal *metadata = [FSTLevelDBQueryCache readTargetMetadataFromDB:_db.get()];
-    XCTAssertEqual(expected, metadata.targetCount, @"Failed to count all of the targets we added");
+    XCTAssertNotNil(metadata, @"Metadata should have been added");
+    XCTAssertEqual(metadata.targetCount, 0);
   }
 }
 
+- (void)testDropsTheQueryCacheWithThousandsOfEntries {
+  [FSTLevelDBMigrations runMigrationsWithDatabase:_db.get() upToVersion:2];
+  {
+    // Setup some targets to be destroyed.
+    LevelDbTransaction transaction(_db.get(), "testDropsTheQueryCacheWithThousandsOfEntries setup");
+    for (int i = 0; i < 10000; ++i) {
+      transaction.Put([FSTLevelDBTargetKey keyWithTargetID:i], "");
+    }
+    transaction.Commit();
+  }
+
+  [FSTLevelDBMigrations runMigrationsWithDatabase:_db.get() upToVersion:3];
+  {
+    LevelDbTransaction transaction(_db.get(), "Verify");
+    std::string prefix = [FSTLevelDBTargetKey keyPrefix];
+
+    auto it = transaction.NewIterator();
+    std::vector<std::string> found_keys;
+    for (it->Seek(prefix); it->Valid() && absl::StartsWith(it->key(), prefix); it->Next()) {
+      found_keys.push_back(std::string{it->key()});
+    }
+
+    XCTAssertEqual(found_keys, std::vector<std::string>{});
+  }
+}
+
+/**
+ * Creates the name of a dummy entry to make sure the iteration is correctly bounded.
+ */
+- (std::string)dummyKeyForTable:(const char *)tableName {
+  std::string dummyKey;
+  // Magic number that indicates a table name follows. Needed to mimic the prefix to the target
+  // table.
+  OrderedCode::WriteSignedNumIncreasing(&dummyKey, 5);
+  OrderedCode::WriteString(&dummyKey, tableName);
+  return dummyKey;
+}
+
 @end
 
 NS_ASSUME_NONNULL_END

Firestore/Example/Tests/SpecTests/FSTMockDatastore.mm

@@ -138,7 +138,7 @@ - (void)failStreamWithError:(NSError *)error {
 
 #pragma mark - Helper methods.
 
-- (void)writeWatchChange:(FSTWatchChange *)change snapshotVersion:(const SnapshotVersion &)snap {
+- (void)writeWatchChange:(FSTWatchChange *)change snapshotVersion:(SnapshotVersion)snap {
   if ([change isKindOfClass:[FSTWatchTargetChange class]]) {
     FSTWatchTargetChange *targetChange = (FSTWatchTargetChange *)change;
     if (targetChange.cause) {
@@ -152,6 +152,11 @@ - (void)writeWatchChange:(FSTWatchChange *)change snapshotVersion:(const Snapsho
         [self.activeTargets removeObjectForKey:targetID];
       }
     }
+    if ([targetChange.targetIDs count] != 0) {
+      // If the list of target IDs is not empty, we reset the snapshot version to NONE as
+      // done in `FSTSerializerBeta.versionFromListenResponse:`.
+      snap = SnapshotVersion::None();
+    }
   }
   [self.delegate watchStreamDidChange:change snapshotVersion:snap];
 }

Firestore/Example/Tests/SpecTests/FSTSpecTests.mm

@@ -194,25 +194,25 @@ - (void)doDelete:(NSString *)key {
   [self.driver writeUserMutation:FSTTestDeleteMutation(key)];
 }
 
-- (void)doWatchAck:(NSArray<NSNumber *> *)ackedTargets snapshot:(NSNumber *)watchSnapshot {
+- (void)doWatchAck:(NSArray<NSNumber *> *)ackedTargets {
   FSTWatchTargetChange *change =
       [FSTWatchTargetChange changeWithState:FSTWatchTargetChangeStateAdded
                                   targetIDs:ackedTargets
                                       cause:nil];
-  [self.driver receiveWatchChange:change snapshotVersion:[self parseVersion:watchSnapshot]];
+  [self.driver receiveWatchChange:change snapshotVersion:SnapshotVersion::None()];
 }
 
-- (void)doWatchCurrent:(NSArray<id> *)currentSpec snapshot:(NSNumber *)watchSnapshot {
+- (void)doWatchCurrent:(NSArray<id> *)currentSpec {
   NSArray<NSNumber *> *currentTargets = currentSpec[0];
   NSData *resumeToken = [currentSpec[1] dataUsingEncoding:NSUTF8StringEncoding];
   FSTWatchTargetChange *change =
       [FSTWatchTargetChange changeWithState:FSTWatchTargetChangeStateCurrent
                                   targetIDs:currentTargets
                                 resumeToken:resumeToken];
-  [self.driver receiveWatchChange:change snapshotVersion:[self parseVersion:watchSnapshot]];
+  [self.driver receiveWatchChange:change snapshotVersion:SnapshotVersion::None()];
 }
 
-- (void)doWatchRemove:(NSDictionary *)watchRemoveSpec snapshot:(NSNumber *)watchSnapshot {
+- (void)doWatchRemove:(NSDictionary *)watchRemoveSpec {
   NSError *error = nil;
   NSDictionary *cause = watchRemoveSpec[@"cause"];
   if (cause) {
@@ -226,19 +226,16 @@ - (void)doWatchRemove:(NSDictionary *)watchRemoveSpec snapshot:(NSNumber *)watch
       [FSTWatchTargetChange changeWithState:FSTWatchTargetChangeStateRemoved
                                   targetIDs:watchRemoveSpec[@"targetIds"]
                                       cause:error];
-  [self.driver receiveWatchChange:change snapshotVersion:[self parseVersion:watchSnapshot]];
+  [self.driver receiveWatchChange:change snapshotVersion:SnapshotVersion::None()];
   // Unlike web, the FSTMockDatastore detects a watch removal with cause and will remove active
   // targets
 }
 
-- (void)doWatchEntity:(NSDictionary *)watchEntity snapshot:(NSNumber *_Nullable)watchSnapshot {
+- (void)doWatchEntity:(NSDictionary *)watchEntity {
   if (watchEntity[@"docs"]) {
     HARD_ASSERT(!watchEntity[@"doc"], "Exactly one of |doc| or |docs| needs to be set.");
-    int count = 0;
     NSArray *docs = watchEntity[@"docs"];
     for (NSDictionary *doc in docs) {
-      count++;
-      bool isLast = (count == docs.count);
       NSMutableDictionary *watchSpec = [NSMutableDictionary dictionary];
       watchSpec[@"doc"] = doc;
       if (watchEntity[@"targets"]) {
@@ -247,11 +244,7 @@ - (void)doWatchEntity:(NSDictionary *)watchEntity snapshot:(NSNumber *_Nullable)
       if (watchEntity[@"removedTargets"]) {
         watchSpec[@"removedTargets"] = watchEntity[@"removedTargets"];
       }
-      NSNumber *_Nullable version = nil;
-      if (isLast) {
-        version = watchSnapshot;
-      }
-      [self doWatchEntity:watchSpec snapshot:version];
+      [self doWatchEntity:watchSpec];
     }
   } else if (watchEntity[@"doc"]) {
     NSArray *docSpec = watchEntity[@"doc"];
@@ -270,21 +263,21 @@ - (void)doWatchEntity:(NSDictionary *)watchEntity snapshot:(NSNumber *_Nullable)
                                                 removedTargetIDs:watchEntity[@"removedTargets"]
                                                      documentKey:doc.key
                                                         document:doc];
-    [self.driver receiveWatchChange:change snapshotVersion:[self parseVersion:watchSnapshot]];
+    [self.driver receiveWatchChange:change snapshotVersion:SnapshotVersion::None()];
   } else if (watchEntity[@"key"]) {
     FSTDocumentKey *docKey = FSTTestDocKey(watchEntity[@"key"]);
     FSTWatchChange *change =
         [[FSTDocumentWatchChange alloc] initWithUpdatedTargetIDs:@[]
                                                 removedTargetIDs:watchEntity[@"removedTargets"]
                                                      documentKey:docKey
                                                         document:nil];
-    [self.driver receiveWatchChange:change snapshotVersion:[self parseVersion:watchSnapshot]];
+    [self.driver receiveWatchChange:change snapshotVersion:SnapshotVersion::None()];
   } else {
     HARD_FAIL("Either key, doc or docs must be set.");
   }
 }
 
-- (void)doWatchFilter:(NSArray *)watchFilter snapshot:(NSNumber *_Nullable)watchSnapshot {
+- (void)doWatchFilter:(NSArray *)watchFilter {
   NSArray<NSNumber *> *targets = watchFilter[0];
   HARD_ASSERT(targets.count == 1, "ExistenceFilters currently support exactly one target only.");
 
@@ -294,15 +287,29 @@ - (void)doWatchFilter:(NSArray *)watchFilter snapshot:(NSNumber *_Nullable)watch
   FSTExistenceFilter *filter = [FSTExistenceFilter filterWithCount:keyCount];
   FSTExistenceFilterWatchChange *change =
       [FSTExistenceFilterWatchChange changeWithFilter:filter targetID:targets[0].intValue];
-  [self.driver receiveWatchChange:change snapshotVersion:[self parseVersion:watchSnapshot]];
+  [self.driver receiveWatchChange:change snapshotVersion:SnapshotVersion::None()];
 }
 
-- (void)doWatchReset:(NSArray<NSNumber *> *)watchReset snapshot:(NSNumber *_Nullable)watchSnapshot {
+- (void)doWatchReset:(NSArray<NSNumber *> *)watchReset {
   FSTWatchTargetChange *change =
       [FSTWatchTargetChange changeWithState:FSTWatchTargetChangeStateReset
                                   targetIDs:watchReset
                                       cause:nil];
-  [self.driver receiveWatchChange:change snapshotVersion:[self parseVersion:watchSnapshot]];
+  [self.driver receiveWatchChange:change snapshotVersion:SnapshotVersion::None()];
+}
+
+- (void)doWatchSnapshot:(NSDictionary *)watchSnapshot {
+  // The client will only respond to watchSnapshots if they are on a target change with an empty
+  // set of target IDs.
+  NSArray<NSNumber *> *targetIDs =
+      watchSnapshot[@"targetIds"] ? watchSnapshot[@"targetIds"] : [NSArray array];
+  NSData *resumeToken = [watchSnapshot[@"resumeToken"] dataUsingEncoding:NSUTF8StringEncoding];
+  FSTWatchTargetChange *change =
+      [FSTWatchTargetChange changeWithState:FSTWatchTargetChangeStateNoChange
+                                  targetIDs:targetIDs
+                                resumeToken:resumeToken];
+  [self.driver receiveWatchChange:change
+                  snapshotVersion:[self parseVersion:watchSnapshot[@"version"]]];
 }
 
 - (void)doWatchStreamClose:(NSDictionary *)closeSpec {
@@ -415,17 +422,19 @@ - (void)doStep:(NSDictionary *)step {
   } else if (step[@"userDelete"]) {
     [self doDelete:step[@"userDelete"]];
   } else if (step[@"watchAck"]) {
-    [self doWatchAck:step[@"watchAck"] snapshot:step[@"watchSnapshot"]];
+    [self doWatchAck:step[@"watchAck"]];
   } else if (step[@"watchCurrent"]) {
-    [self doWatchCurrent:step[@"watchCurrent"] snapshot:step[@"watchSnapshot"]];
+    [self doWatchCurrent:step[@"watchCurrent"]];
   } else if (step[@"watchRemove"]) {
-    [self doWatchRemove:step[@"watchRemove"] snapshot:step[@"watchSnapshot"]];
+    [self doWatchRemove:step[@"watchRemove"]];
   } else if (step[@"watchEntity"]) {
-    [self doWatchEntity:step[@"watchEntity"] snapshot:step[@"watchSnapshot"]];
+    [self doWatchEntity:step[@"watchEntity"]];
   } else if (step[@"watchFilter"]) {
-    [self doWatchFilter:step[@"watchFilter"] snapshot:step[@"watchSnapshot"]];
+    [self doWatchFilter:step[@"watchFilter"]];
   } else if (step[@"watchReset"]) {
-    [self doWatchReset:step[@"watchReset"] snapshot:step[@"watchSnapshot"]];
+    [self doWatchReset:step[@"watchReset"]];
+  } else if (step[@"watchSnapshot"]) {
+    [self doWatchSnapshot:step[@"watchSnapshot"]];
   } else if (step[@"watchStreamClose"]) {
     [self doWatchStreamClose:step[@"watchStreamClose"]];
   } else if (step[@"watchProto"]) {

Firestore/Example/Tests/SpecTests/json/collection_spec_test.json

@@ -56,8 +56,12 @@
             2
           ],
           "resume-token-1001"
-        ],
-        "watchSnapshot": 1001,
+        ]
+      },
+      {
+        "watchSnapshot": {
+          "version": 1001
+        },
         "expect": [
           {
             "query": {