Merge pull request #1087 from OneSignal/fix/outcomes_indexeddb_schema

[Fix] Outcomes IndexedDB schema

__test__/unit/services/indexedDb.test.ts

@@ -0,0 +1,104 @@
+import IndexedDb from "../../../src/shared/services/IndexedDb";
+import Random from "../../support/utils/Random";
+
+require("fake-indexeddb/auto");
+
+function newOSIndexedDb(
+  dbName = Random.getRandomString(10),
+  dbVersion = Number.MAX_SAFE_INTEGER,
+  ): IndexedDb {
+  return new IndexedDb(
+    dbName,
+    dbVersion,
+  );
+}
+
+describe('Options access', () => {
+  test('should get and set value', async () => {
+    const db = newOSIndexedDb();
+    await db.put("Options", { key: 'optionsKey', value: 'optionsValue' });
+    const retrievedValue = await db.get("Options", 'optionsKey');
+    expect(retrievedValue).toEqual({ key: 'optionsKey', value: 'optionsValue' });
+  });
+
+  test('should remove value', async () => {
+    const db = newOSIndexedDb();
+    await db.put("Options", { key: 'optionsKey', value: 'optionsValue' });
+    await db.remove("Options", 'optionsKey');
+    const retrievedValue = await db.get("Options", 'optionsKey');
+    expect(retrievedValue).toBeUndefined();
+  });
+});
+
+describe('migrations', () => {
+  describe('v5', () => {
+    test('can to write to new Outcomes.NotificationClicked table', async () => {
+      const db = newOSIndexedDb("testDbv5", 5);
+      const result = await db.put("Outcomes.NotificationClicked", { notificationId: "1" } );
+      expect(result).toEqual({ notificationId: "1" });
+    });
+
+    test('can write to new Outcomes.NotificationReceived table', async () => {
+      const db = newOSIndexedDb("testDbv5", 5);
+      const result = await db.put("Outcomes.NotificationReceived", { notificationId: "1" } );
+      expect(result).toEqual({ notificationId: "1" });
+    });
+
+    // Tests NotificationClicked records migrate over from a v15 SDK version
+    test('migrates notificationId type records into Outcomes.NotificationClicked', async () => {
+      const dbName = "testDbV4upgradeToV5" + Random.getRandomString(10);
+      const db = newOSIndexedDb(dbName, 4);
+      await db.put("NotificationClicked", { notificationId: "1" });
+      db.close();
+
+      const db2 = newOSIndexedDb(dbName, 5);
+      const result = await db2.getAll("Outcomes.NotificationClicked");
+      expect(result).toEqual([{appId: undefined, notificationId: "1", timestamp: undefined}]);
+    });
+
+    // Tests NotificationReceived records migrate over from a v15 SDK version
+    test('migrates notificationId type records into Outcomes.NotificationReceived', async () => {
+      const dbName = "testDbV4upgradeToV5" + Random.getRandomString(10);
+      const db = newOSIndexedDb(dbName, 4);
+      await db.put("NotificationReceived", { notificationId: "1" });
+      db.close();
+
+      const db2 = newOSIndexedDb(dbName, 5);
+      const result = await db2.getAll("Outcomes.NotificationReceived");
+      expect(result).toEqual([{appId: undefined, notificationId: "1", timestamp: undefined}]);
+    });
+
+    // Tests records coming from a broken SDK (160000.beta4 to 160000) and upgrading to fixed v5 db
+    test('migrates notification.id type records into Outcomes.NotificationClicked', async () => {
+      const dbName = "testDbV4upgradeToV5" + Random.getRandomString(10);
+
+      // 1. Put the db's schema into the broken v4 state that SDK v16000000 had
+      const openDbRequest = indexedDB.open(dbName, 4);
+      const dbOpenPromise = new Promise((resolve) => {
+        openDbRequest.onsuccess = resolve;
+      });
+      const dbUpgradePromise = new Promise<void>((resolve) => {
+        openDbRequest.onupgradeneeded = () => {
+          const db = openDbRequest.result;
+          db.createObjectStore("NotificationClicked", { keyPath: "notification.id" });
+          db.createObjectStore("NotificationReceived", { keyPath: "notificationId" });
+          resolve();
+        }
+      });
+      await Promise.all([dbOpenPromise, dbUpgradePromise]);
+
+      // 2. Put a record into the DB with the old schema
+      openDbRequest.result
+        .transaction(["NotificationClicked"], 'readwrite')
+        .objectStore("NotificationClicked")
+        .put({ notification: { id: "1" } });
+      openDbRequest.result.close();
+
+      // 3. Open the DB with the OneSignal IndexedDb class
+      const db2 = newOSIndexedDb(dbName, 5);
+      const result = await db2.getAll("Outcomes.NotificationClicked");
+      // 4. Expect the that data is brought over to the new table.
+      expect(result).toEqual([{appId: undefined, notificationId: "1", timestamp: undefined}]);
+    });
+  });
+});

src/shared/helpers/OutcomesHelper.ts

@@ -1,6 +1,6 @@
 import { OutcomesConfig, OutcomeAttribution, OutcomeAttributionType, SentUniqueOutcome } from '../models/Outcomes';
 import { OutcomesNotificationReceived } from "../models/OutcomesNotificationEvents";
-import Database from "../services/Database";
+import Database, { TABLE_OUTCOMES_NOTIFICATION_RECEIVED } from "../services/Database";
 import Log from '../libraries/Log';
 import { Utils } from "../../shared/context/Utils";
 import { logMethodCall, awaitOneSignalInitAndSupported } from '../utils/utils';
@@ -235,7 +235,7 @@ export default class OutcomesHelper {
         const notificationIdsToDelete = allReceivedNotificationSorted
           .filter(notif => matchingNotificationIds.indexOf(notif.notificationId) === -1)
           .map(notif => notif.notificationId);
-        notificationIdsToDelete.forEach(id => Database.remove("NotificationReceived", id));
+        notificationIdsToDelete.forEach(id => Database.remove(TABLE_OUTCOMES_NOTIFICATION_RECEIVED, id));
         Log.debug(`\t${notificationIdsToDelete.length} received notifications will be deleted.`);
 
         if (matchingNotificationIds.length > 0) {

src/shared/services/Database.ts

@@ -40,12 +40,13 @@ interface DatabaseResult {
 
 /**
  * "NotificationOpened" = Pending Notification Click events that haven't fired yet
- * "NotificationClicked" = Outcomes only, notifications part of it's session
- * "NotificationReceived" = Outcomes only, notifications part of it's session
  */
 
+export const TABLE_OUTCOMES_NOTIFICATION_CLICKED = "Outcomes.NotificationClicked";
+export const TABLE_OUTCOMES_NOTIFICATION_RECEIVED = "Outcomes.NotificationReceived";
+
 export type OneSignalDbTable = "Options" | "Ids" | "NotificationOpened" | "Sessions" |
-  "NotificationOpened" | "NotificationReceived" | "NotificationClicked" | "SentUniqueOutcome" |  ModelName;
+  "NotificationOpened" | typeof TABLE_OUTCOMES_NOTIFICATION_RECEIVED | typeof TABLE_OUTCOMES_NOTIFICATION_CLICKED | "SentUniqueOutcome" |  ModelName;
 
 export default class Database {
 
@@ -443,13 +444,13 @@ export default class Database {
   }
 
   async getAllNotificationClickedForOutcomes(): Promise<OutcomesNotificationClicked[]> {
-    const notifications = await this.getAll<NotificationReceivedForOutcomesSchema>("NotificationClicked");
+    const notifications = await this.getAll<NotificationReceivedForOutcomesSchema>(TABLE_OUTCOMES_NOTIFICATION_CLICKED);
     return notifications.map(notification => NotificationClickedForOutcomesSerializer.fromDatabase(notification));
   }
 
   async putNotificationClickedForOutcomes(appId: string, event: NotificationClickEventInternal): Promise<void> {
     await this.put(
-      "NotificationClicked",
+      TABLE_OUTCOMES_NOTIFICATION_CLICKED,
       NotificationClickedForOutcomesSerializer.toDatabase(appId, event)
     );
   }
@@ -476,17 +477,17 @@ export default class Database {
   }
 
   async removeAllNotificationClickedForOutcomes(): Promise<void> {
-    await this.remove("NotificationClicked");
+    await this.remove(TABLE_OUTCOMES_NOTIFICATION_CLICKED);
   }
 
   async getAllNotificationReceivedForOutcomes(): Promise<OutcomesNotificationReceived[]> {
-    const notifications = await this.getAll<NotificationReceivedForOutcomesSchema>("NotificationReceived");
+    const notifications = await this.getAll<NotificationReceivedForOutcomesSchema>(TABLE_OUTCOMES_NOTIFICATION_RECEIVED);
     return notifications.map(notification => NotificationReceivedForOutcomesSerializer.fromDatabase(notification));
   }
 
   async putNotificationReceivedForOutcomes(appId: string, notification: IOSNotification): Promise<void> {
     await this.put(
-      "NotificationReceived",
+      TABLE_OUTCOMES_NOTIFICATION_RECEIVED,
       NotificationReceivedForOutcomesSerializer.toDatabase(appId, notification, new Date().getTime())
     );
   }
@@ -510,8 +511,8 @@ export default class Database {
       Database.singletonInstance.remove("Ids"),
       Database.singletonInstance.remove("NotificationOpened"),
       Database.singletonInstance.remove("Options"),
-      Database.singletonInstance.remove("NotificationReceived"),
-      Database.singletonInstance.remove("NotificationClicked"),
+      Database.singletonInstance.remove(TABLE_OUTCOMES_NOTIFICATION_RECEIVED),
+      Database.singletonInstance.remove(TABLE_OUTCOMES_NOTIFICATION_CLICKED),
       Database.singletonInstance.remove("SentUniqueOutcome")
     ]);
   }

src/shared/services/IndexedDb.ts

@@ -4,15 +4,17 @@ import Utils from '../context/Utils';
 import Emitter from '../libraries/Emitter';
 import Log from '../libraries/Log';
 
-const DATABASE_VERSION = 4;
+const DATABASE_VERSION = 5;
 
 export default class IndexedDb {
-
   public emitter: Emitter;
   private database: IDBDatabase | undefined;
   private openLock: Promise<IDBDatabase> | undefined;
 
-  constructor(private databaseName: string) {
+  constructor(
+    private readonly databaseName: string,
+    private readonly dbVersion = DATABASE_VERSION,
+  ) {
     this.emitter = new Emitter();
   }
 
@@ -21,17 +23,17 @@ export default class IndexedDb {
       let request: IDBOpenDBRequest | undefined = undefined;
       try {
         // Open algorithm: https://www.w3.org/TR/IndexedDB/#h-opening
-        request = indexedDB.open(databaseName, DATABASE_VERSION);
+        request = indexedDB.open(databaseName, this.dbVersion);
       } catch (e) {
         // Errors should be thrown on the request.onerror event, but just in case Firefox throws additional errors
         // for profile schema too high
       }
       if (!request) {
         return null;
       }
-      request.onerror = this.onDatabaseOpenError;
-      request.onblocked = this.onDatabaseOpenBlocked;
-      request.onupgradeneeded = this.onDatabaseUpgradeNeeded;
+      request.onerror = this.onDatabaseOpenError.bind(this);
+      request.onblocked = this.onDatabaseOpenBlocked.bind(this);
+      request.onupgradeneeded = this.onDatabaseUpgradeNeeded.bind(this);
       request.onsuccess = () => {
         this.database = request.result;
         this.database.onerror = this.onDatabaseError;
@@ -41,6 +43,14 @@ export default class IndexedDb {
     });
   }
 
+  public async close(): Promise<void> {
+    // TODO:CLEANUP: Seems we have always had two DB connections open
+    // one could be delete to clean this up.
+    const dbLock = await this.ensureDatabaseOpen();
+    dbLock.close();
+    this.database?.close();
+  }
+
   private async ensureDatabaseOpen(): Promise<IDBDatabase> {
     if (!this.openLock) {
       this.openLock = this.open(this.databaseName);
@@ -102,28 +112,43 @@ export default class IndexedDb {
    */
   private onDatabaseUpgradeNeeded(event: IDBVersionChangeEvent): void {
     Log.debug('IndexedDb: Database is being rebuilt or upgraded (upgradeneeded event).');
-    const db = (event.target as IDBOpenDBRequest).result;
-    if (event.oldVersion < 1) {
+    const target = event.target as IDBOpenDBRequest;
+    const transaction = target.transaction;
+    if (!transaction) {
+      throw Error("Can't migrate DB without a transaction");
+    }
+    const db = target.result;
+    const newDbVersion = event.newVersion || Number.MAX_SAFE_INTEGER;
+    if (newDbVersion >= 1 && event.oldVersion < 1) {
       db.createObjectStore("Ids", { keyPath: "type" });
       db.createObjectStore("NotificationOpened", { keyPath: "url" });
       db.createObjectStore("Options", { keyPath: "key" });
     }
-    if (event.oldVersion < 2) {
+    if (newDbVersion >= 2 && event.oldVersion < 2) {
       db.createObjectStore("Sessions", { keyPath: "sessionKey" });
       db.createObjectStore("NotificationReceived", { keyPath: "notificationId" });
-      db.createObjectStore("NotificationClicked", { keyPath: "notification.id" });
+      // NOTE: 160000.beta4 to 160000 releases modified this line below as
+      // "{ keyPath: "notification.id" }". This resulted in DB v4 either
+      // having "notificationId" or "notification.id" depending if the visitor
+      // was new while this version was live.
+      // DB v5 was created to trigger a migration to fix this bug.
+      db.createObjectStore("NotificationClicked", { keyPath: "notificationId" });
     }
-    if (event.oldVersion < 3) {
+    if (newDbVersion >= 3 && event.oldVersion < 3) {
       db.createObjectStore("SentUniqueOutcome", { keyPath: "outcomeName" });
     }
-    if (event.oldVersion < 4) {
+    if (newDbVersion >= 4 && event.oldVersion < 4) {
       db.createObjectStore(ModelName.Identity, { keyPath: "modelId" });
       db.createObjectStore(ModelName.Properties, { keyPath: "modelId" });
       db.createObjectStore(ModelName.PushSubscriptions, { keyPath: "modelId" });
       db.createObjectStore(ModelName.SmsSubscriptions, { keyPath: "modelId" });
       db.createObjectStore(ModelName.EmailSubscriptions, { keyPath: "modelId" });
     }
-    if (event.oldVersion < 5) {
+    if (newDbVersion >= 5 && event.oldVersion < 5) {
+      this.migrateOutcomesNotificationClickedTableForV5(db, transaction);
+      this.migrateOutcomesNotificationReceivedTableForV5(db, transaction);
+    }
+    if (newDbVersion >= 6 && event.oldVersion < 6) {
       // Make sure to update the database version at the top of the file
     }
     // Wrap in conditional for tests
@@ -132,6 +157,82 @@ export default class IndexedDb {
     }
   }
 
+  // Table rename "NotificationClicked" -> "Outcomes.NotificationClicked"
+  // and migrate existing records.
+  // Motivation: This is done to correct the keyPath, you can't change it
+  // so a new table must be created.
+  // Background: Table was created with wrong keyPath of "notification.id"
+  // for new visitors for versions 160000.beta4 to 160000. Writes were
+  // attempted as "notificationId" in released 160000 however they may
+  // have failed if the visitor was new when those releases were in the wild.
+  // However those new on 160000.beta4 to 160000.beta8 will have records
+  // saved as "notification.id" that will be converted here.
+  private migrateOutcomesNotificationClickedTableForV5(
+    db: IDBDatabase,
+    transaction: IDBTransaction,
+  ) {
+    const newTableName = "Outcomes.NotificationClicked";
+    db.createObjectStore(newTableName, { keyPath: "notificationId" });
+
+    const oldTableName = "NotificationClicked"
+    const cursor = transaction.objectStore(oldTableName).openCursor();
+    cursor.onsuccess = () => {
+      if (!cursor.result) {
+        // Delete old table once we have gone through all records
+        db.deleteObjectStore(oldTableName);
+        return;
+      }
+      const oldValue = cursor.result.value;
+      transaction
+        .objectStore(newTableName)
+        .put({
+          // notification.id was possible from 160000.beta4 to 160000.beta8
+          notificationId: oldValue.notificationId || oldValue.notification.id,
+          appId: oldValue.appId,
+          timestamp: oldValue.timestamp,
+        });
+      cursor.result.continue();
+    };
+    cursor.onerror = () => {
+      // If there is an error getting old records nothing we can do but
+      // move on. Old table will stay around so an attempt could be made
+      // later.
+      console.error("Could not migrate NotificationClicked records", cursor.error);
+    };
+  }
+
+  // Table rename "NotificationReceived" -> "Outcomes.NotificationReceived"
+  // and migrate existing records.
+  // Motivation: Consistency of using pre-fix "Outcomes." like we have for
+  // the "Outcomes.NotificationClicked" table.
+  private migrateOutcomesNotificationReceivedTableForV5(
+    db: IDBDatabase,
+    transaction: IDBTransaction,
+  ) {
+    const newTableName = "Outcomes.NotificationReceived";
+    db.createObjectStore(newTableName, { keyPath: "notificationId" });
+
+    const oldTableName = "NotificationReceived"
+    const cursor = transaction.objectStore(oldTableName).openCursor();
+    cursor.onsuccess = () => {
+      if (!cursor.result) {
+        // Delete old table once we have gone through all records
+        db.deleteObjectStore(oldTableName);
+        return;
+      }
+      transaction
+        .objectStore(newTableName)
+        .put(cursor.result.value);
+        cursor.result.continue();
+    };
+    cursor.onerror = () => {
+      // If there is an error getting old records nothing we can do but
+      // move on. Old table will stay around so an attempt could be made
+      // later.
+      console.error("Could not migrate NotificationReceived records", cursor.error);
+    };
+  }
+
   /**
    * Asynchronously retrieves the value of the key at the table (if key is specified), or the entire table
    * (if key is not specified).

test/support/tester/OutcomeTestHelper.ts

@@ -1,5 +1,5 @@
 import Random from '../../support/tester/Random';
-import Database from '../../../src/shared/services/Database';
+import Database, { TABLE_OUTCOMES_NOTIFICATION_RECEIVED } from '../../../src/shared/services/Database';
 import { initializeNewSession, Session } from '../../../src/shared/models/Session';
 import { OutcomesNotificationClicked, OutcomesNotificationReceived } from "src/shared/models/OutcomesNotificationEvents";
 
@@ -24,7 +24,7 @@ export default class OutcomeTestHelper {
       if (notificationReceived.timestamp >= maxTimestamp && receivedNotificationIdsWithinTimeframe.length < limit) {
         receivedNotificationIdsWithinTimeframe.push(notificationReceived.notificationId);
       }
-      await Database.put("NotificationReceived", notificationReceived);
+      await Database.put(TABLE_OUTCOMES_NOTIFICATION_RECEIVED, notificationReceived);
     }
 
     return receivedNotificationIdsWithinTimeframe;