Ensure that we create an empty TargetGlobal row. (#1029)

Ensure the v3 migration unconditionally creates the TargetGlobal row. Remove the no-longer-necessary v2 schema migration.

Author: mikelehen

packages/firestore/src/local/indexeddb_schema.ts

@@ -29,7 +29,8 @@ import { SnapshotVersion } from '../core/snapshot_version';
  * Schema Version for the Web client:
  * 1. Initial version including Mutation Queue, Query Cache, and Remote Document
  *    Cache
- * 2. Added targetCount to targetGlobal row.
+ * 2. Used to ensure a targetGlobal object exists and add targetCount to it. No
+ *    longer required because migration 3 unconditionally clears it.
  * 3. Dropped and re-created Query Cache to deal with cache corruption related
  *    to limbo resolution. Addresses
  *    https://github.com/firebase/firebase-ios-sdk/issues/1548
@@ -61,21 +62,19 @@ export function createOrUpgradeDb(
     createRemoteDocumentCache(db);
   }
 
-  let p = PersistencePromise.resolve();
-  if (fromVersion < 2 && toVersion >= 2) {
-    p = ensureTargetGlobalExists(txn).next(targetGlobal =>
-      saveTargetCount(txn, targetGlobal)
-    );
-  }
+  // Migration 2 to populate the targetGlobal object no longer needed since
+  // migration 3 unconditionally clears it.
 
-  p = p.next(() => {
+  let p = PersistencePromise.resolve();
+  if (fromVersion < 3 && toVersion >= 3) {
     // Brand new clients don't need to drop and recreate--only clients that
     // potentially have corrupt data.
-    if (fromVersion !== 0 && fromVersion < 3 && toVersion >= 3) {
+    if (fromVersion !== 0) {
       dropQueryCache(db);
       createQueryCache(db);
     }
-  });
+    p = p.next(() => writeEmptyTargetGlobalEntry(txn));
+  }
 
   return p;
 }
@@ -524,48 +523,23 @@ function dropQueryCache(db: IDBDatabase): void {
 }
 
 /**
- * Counts the number of targets persisted and adds that value to the target
- * global singleton.
- */
-function saveTargetCount(
-  txn: SimpleDbTransaction,
-  metadata: DbTargetGlobal
-): PersistencePromise<void> {
-  const globalStore = txn.store<DbTargetGlobalKey, DbTargetGlobal>(
-    DbTargetGlobal.store
-  );
-  const targetStore = txn.store<DbTargetKey, DbTarget>(DbTarget.store);
-  return targetStore.count().next(count => {
-    metadata.targetCount = count;
-    return globalStore.put(DbTargetGlobal.key, metadata);
-  });
-}
-
-/**
- * Ensures that the target global singleton row exists by adding it if it's
- * missing.
+ * Creates the target global singleton row.
  *
  * @param {IDBTransaction} txn The version upgrade transaction for indexeddb
  */
-function ensureTargetGlobalExists(
+function writeEmptyTargetGlobalEntry(
   txn: SimpleDbTransaction
-): PersistencePromise<DbTargetGlobal> {
+): PersistencePromise<void> {
   const globalStore = txn.store<DbTargetGlobalKey, DbTargetGlobal>(
     DbTargetGlobal.store
   );
-  return globalStore.get(DbTargetGlobal.key).next(metadata => {
-    if (metadata != null) {
-      return PersistencePromise.resolve(metadata);
-    } else {
-      metadata = new DbTargetGlobal(
-        /*highestTargetId=*/ 0,
-        /*lastListenSequenceNumber=*/ 0,
-        SnapshotVersion.MIN.toTimestamp(),
-        /*targetCount=*/ 0
-      );
-      return globalStore.put(DbTargetGlobal.key, metadata).next(() => metadata);
-    }
-  });
+  const metadata = new DbTargetGlobal(
+    /*highestTargetId=*/ 0,
+    /*lastListenSequenceNumber=*/ 0,
+    SnapshotVersion.MIN.toTimestamp(),
+    /*targetCount=*/ 0
+  );
+  return globalStore.put(DbTargetGlobal.key, metadata);
 }
 
 /**

packages/firestore/test/unit/local/indexeddb_schema.test.ts

@@ -24,10 +24,11 @@ import {
   DbTarget,
   DbTargetGlobal,
   DbTargetGlobalKey,
-  DbTargetKey
+  DbTargetKey,
+  DbTimestamp
 } from '../../../src/local/indexeddb_schema';
 import { SimpleDb, SimpleDbTransaction } from '../../../src/local/simple_db';
-import { PersistencePromise } from '../../../src/local/persistence_promise';
+import { SnapshotVersion } from '../../../src/core/snapshot_version';
 
 const INDEXEDDB_TEST_DATABASE = 'schemaTest';
 
@@ -71,17 +72,6 @@ function getAllObjectStores(db: IDBDatabase): string[] {
   return objectStores;
 }
 
-function getTargetCount(db: IDBDatabase): Promise<number> {
-  const sdb = new SimpleDb(db);
-  return sdb
-    .runTransaction('readonly', [DbTargetGlobal.store], txn =>
-      txn
-        .store<DbTargetGlobalKey, DbTargetGlobal>(DbTargetGlobal.store)
-        .get(DbTargetGlobal.key)
-    )
-    .then(metadata => metadata.targetCount);
-}
-
 describe('IndexedDbSchema: createOrUpgradeDb', () => {
   if (!IndexedDbPersistence.isAvailable()) {
     console.warn('No IndexedDB. Skipping createOrUpgradeDb() tests.');
@@ -101,58 +91,35 @@ describe('IndexedDbSchema: createOrUpgradeDb', () => {
     });
   });
 
-  it('can install schema version 2', () => {
-    return withDb(2, db => {
-      expect(db.version).to.equal(2);
-      // We should have all of the stores, we should have the target global row
-      // and we should not have any targets counted, because there are none.
-      expect(getAllObjectStores(db)).to.have.members(ALL_STORES);
-      // Check the target count. We haven't added any targets, so we expect 0.
-      return getTargetCount(db).then(targetCount => {
-        expect(targetCount).to.equal(0);
-      });
-    });
-  });
-
-  it('can upgrade from schema version 1 to 2', () => {
-    const expectedTargetCount = 5;
-    return withDb(1, db => {
-      const sdb = new SimpleDb(db);
-      // Now that we have all of the stores, add some targets so the next
-      // migration can count them.
-      return sdb.runTransaction('readwrite', [DbTarget.store], txn => {
-        const store = txn.store(DbTarget.store);
-        let p = PersistencePromise.resolve();
-        for (let i = 0; i < expectedTargetCount; i++) {
-          p = p.next(() => store.put({ targetId: i }));
-        }
-        return p;
-      });
-    }).then(() =>
-      withDb(2, db => {
-        expect(db.version).to.equal(2);
-        expect(getAllObjectStores(db)).to.have.members(ALL_STORES);
-        return getTargetCount(db).then(targetCount => {
-          expect(targetCount).to.equal(expectedTargetCount);
-        });
-      })
-    );
-  });
-
   it('drops the query cache from 2 to 3', () => {
     const userId = 'user';
     const batchId = 1;
     const targetId = 2;
 
     const expectedMutation = new DbMutationBatch(userId, batchId, 1000, []);
+    const dummyTargetGlobal = new DbTargetGlobal(
+      /*highestTargetId=*/ 1,
+      /*highestListenSequencNumber=*/ 1,
+      /*lastRemoteSnapshotVersion=*/ new DbTimestamp(1, 1),
+      /*targetCount=*/ 1
+    );
+    const resetTargetGlobal = new DbTargetGlobal(
+      /*highestTargetId=*/ 0,
+      /*highestListenSequencNumber=*/ 0,
+      /*lastRemoteSnapshotVersion=*/ SnapshotVersion.MIN.toTimestamp(),
+      /*targetCount=*/ 0
+    );
 
     return withDb(2, db => {
       const sdb = new SimpleDb(db);
       return sdb.runTransaction(
         'readwrite',
-        [DbTarget.store, DbMutationBatch.store],
+        [DbTarget.store, DbTargetGlobal.store, DbMutationBatch.store],
         txn => {
           const targets = txn.store<DbTargetKey, DbTarget>(DbTarget.store);
+          const targetGlobal = txn.store<DbTargetGlobalKey, DbTargetGlobal>(
+            DbTargetGlobal.store
+          );
           const mutations = txn.store<DbMutationBatchKey, DbMutationBatch>(
             DbMutationBatch.store
           );
@@ -161,6 +128,9 @@ describe('IndexedDbSchema: createOrUpgradeDb', () => {
             targets
               // tslint:disable-next-line:no-any
               .put({ targetId, canonicalId: 'foo' } as any)
+              .next(() =>
+                targetGlobal.put(DbTargetGlobal.key, dummyTargetGlobal)
+              )
               .next(() => mutations.put(expectedMutation))
           );
         }
@@ -173,9 +143,12 @@ describe('IndexedDbSchema: createOrUpgradeDb', () => {
         const sdb = new SimpleDb(db);
         return sdb.runTransaction(
           'readwrite',
-          [DbTarget.store, DbMutationBatch.store],
+          [DbTarget.store, DbTargetGlobal.store, DbMutationBatch.store],
           txn => {
             const targets = txn.store<DbTargetKey, DbTarget>(DbTarget.store);
+            const targetGlobal = txn.store<DbTargetGlobalKey, DbTargetGlobal>(
+              DbTargetGlobal.store
+            );
             const mutations = txn.store<DbMutationBatchKey, DbMutationBatch>(
               DbMutationBatch.store
             );
@@ -186,6 +159,14 @@ describe('IndexedDbSchema: createOrUpgradeDb', () => {
                 // The target should have been dropped
                 expect(target).to.be.null;
               })
+              .next(() => targetGlobal.get(DbTargetGlobal.key))
+              .next(targetGlobalEntry => {
+                // Target Global should exist but be cleared.
+                // HACK: round-trip through JSON to clear types, like IndexedDb
+                // does.
+                const expected = JSON.parse(JSON.stringify(resetTargetGlobal));
+                expect(targetGlobalEntry).to.deep.equal(expected);
+              })
               .next(() => mutations.get([userId, batchId]))
               .next(mutation => {
                 // Mutations should be unaffected.