From e7e650adac4965cef786d93e547f981aa290c83e Mon Sep 17 00:00:00 2001 From: Josh Kasten Date: Wed, 16 Aug 2023 15:47:50 +0000 Subject: [PATCH 1/8] interduce const for outcome table names This makes changing the table name easier in future commits. --- src/shared/helpers/OutcomesHelper.ts | 4 ++-- src/shared/services/Database.ts | 21 ++++++++++--------- test/support/tester/OutcomeTestHelper.ts | 4 ++-- test/unit/public-sdk-apis/sendOutcome.ts | 8 +++---- .../unit/public-sdk-apis/sendUniqueOutcome.ts | 12 +++++------ 5 files changed, 25 insertions(+), 24 deletions(-) diff --git a/src/shared/helpers/OutcomesHelper.ts b/src/shared/helpers/OutcomesHelper.ts index 222cee910..f32ca5bcd 100644 --- a/src/shared/helpers/OutcomesHelper.ts +++ b/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) { diff --git a/src/shared/services/Database.ts b/src/shared/services/Database.ts index 0877222f8..4ffff4787 100644 --- a/src/shared/services/Database.ts +++ b/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 = "NotificationClicked"; +export const TABLE_OUTCOMES_NOTIFICATION_RECEIVED = "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 { - const notifications = await this.getAll("NotificationClicked"); + const notifications = await this.getAll(TABLE_OUTCOMES_NOTIFICATION_CLICKED); return notifications.map(notification => NotificationClickedForOutcomesSerializer.fromDatabase(notification)); } async putNotificationClickedForOutcomes(appId: string, event: NotificationClickEventInternal): Promise { await this.put( - "NotificationClicked", + TABLE_OUTCOMES_NOTIFICATION_CLICKED, NotificationClickedForOutcomesSerializer.toDatabase(appId, event) ); } @@ -476,17 +477,17 @@ export default class Database { } async removeAllNotificationClickedForOutcomes(): Promise { - await this.remove("NotificationClicked"); + await this.remove(TABLE_OUTCOMES_NOTIFICATION_CLICKED); } async getAllNotificationReceivedForOutcomes(): Promise { - const notifications = await this.getAll("NotificationReceived"); + const notifications = await this.getAll(TABLE_OUTCOMES_NOTIFICATION_RECEIVED); return notifications.map(notification => NotificationReceivedForOutcomesSerializer.fromDatabase(notification)); } async putNotificationReceivedForOutcomes(appId: string, notification: IOSNotification): Promise { 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") ]); } diff --git a/test/support/tester/OutcomeTestHelper.ts b/test/support/tester/OutcomeTestHelper.ts index 1fdcac180..9b0769a25 100644 --- a/test/support/tester/OutcomeTestHelper.ts +++ b/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; diff --git a/test/unit/public-sdk-apis/sendOutcome.ts b/test/unit/public-sdk-apis/sendOutcome.ts index 838e3e484..16990a999 100644 --- a/test/unit/public-sdk-apis/sendOutcome.ts +++ b/test/unit/public-sdk-apis/sendOutcome.ts @@ -5,7 +5,7 @@ import { OutcomeRequestData } from "../../../src/page/models/OutcomeRequestData" import { DeliveryPlatformKind } from "../../../src/shared/models/DeliveryPlatformKind"; import { SubscriptionStateKind } from "../../../src/shared/models/SubscriptionStateKind"; import Log from "../../../src/shared/libraries/Log"; -import Database from "../../../src/shared/services/Database"; +import Database, { TABLE_OUTCOMES_NOTIFICATION_CLICKED } from "../../../src/shared/services/Database"; import timemachine from "timemachine"; import OutcomeTestHelper from '../../support/tester/OutcomeTestHelper'; import OneSignalApiShared from "../../../src/shared/api/OneSignalApiShared"; @@ -111,7 +111,7 @@ test("when outcome is unattributed and feature enabled and has weight it sends a test("when outcome is direct and feature enabled it sends an api call", async t => { const notificationClicked = OutcomeTestHelper.generateNotification(); - await Database.put("NotificationClicked", notificationClicked); + await Database.put(TABLE_OUTCOMES_NOTIFICATION_CLICKED, notificationClicked); const apiSpy = sinonSandbox.stub(OneSignalApiShared, "sendOutcome").resolves(); sinonSandbox.stub(OneSignal, "privateIsPushNotificationsEnabled").resolves(true); sinonSandbox.stub(MainHelper, "getCurrentNotificationType").resolves(SubscriptionStateKind.Subscribed); @@ -134,7 +134,7 @@ test("when outcome is direct and feature disabled there are no api calls", async OneSignal.config!.userConfig.outcomes!.unattributed.enabled = false; const notificationClicked = OutcomeTestHelper.generateNotification(); - await Database.put("NotificationClicked", notificationClicked); + await Database.put(TABLE_OUTCOMES_NOTIFICATION_CLICKED, notificationClicked); const apiSpy = sinonSandbox.stub(OneSignalApiShared, "sendOutcome").resolves(); sinonSandbox.stub(OneSignal, "privateIsPushNotificationsEnabled").resolves(true); sinonSandbox.stub(MainHelper, "getCurrentNotificationType").resolves(SubscriptionStateKind.Subscribed); @@ -145,7 +145,7 @@ test("when outcome is direct and feature disabled there are no api calls", async test("when outcome is direct and feature enabled and has weight it sends an api call", async t => { const notificationClicked = OutcomeTestHelper.generateNotification(); - await Database.put("NotificationClicked", notificationClicked); + await Database.put(TABLE_OUTCOMES_NOTIFICATION_CLICKED, notificationClicked); const apiSpy = sinonSandbox.stub(OneSignalApiShared, "sendOutcome").resolves(); sinonSandbox.stub(OneSignal, "privateIsPushNotificationsEnabled").resolves(true); sinonSandbox.stub(MainHelper, "getCurrentNotificationType").resolves(SubscriptionStateKind.Subscribed); diff --git a/test/unit/public-sdk-apis/sendUniqueOutcome.ts b/test/unit/public-sdk-apis/sendUniqueOutcome.ts index 47a328d9f..cc96b15b1 100644 --- a/test/unit/public-sdk-apis/sendUniqueOutcome.ts +++ b/test/unit/public-sdk-apis/sendUniqueOutcome.ts @@ -6,7 +6,7 @@ import { OutcomeRequestData } from "../../../src/page/models/OutcomeRequestData" import { DeliveryPlatformKind } from "../../../src/shared/models/DeliveryPlatformKind"; import { SubscriptionStateKind } from "../../../src/shared/models/SubscriptionStateKind"; import Log from "../../../src/shared/libraries/Log"; -import Database from "../../../src/shared/services/Database"; +import Database, { TABLE_OUTCOMES_NOTIFICATION_CLICKED, TABLE_OUTCOMES_NOTIFICATION_RECEIVED } from "../../../src/shared/services/Database"; import timemachine from "timemachine"; import { SentUniqueOutcome } from '../../../src/shared/models/Outcomes'; import OutcomeTestHelper from '../../support/tester/OutcomeTestHelper'; @@ -87,7 +87,7 @@ test("when outcome is unattributed and feature disabled there are no api calls", test("when outcome is direct and feature enabled it sends an api call", async t => { const notificationClicked = OutcomeTestHelper.generateNotification(); - await Database.put("NotificationClicked", notificationClicked); + await Database.put(TABLE_OUTCOMES_NOTIFICATION_CLICKED, notificationClicked); const apiSpy = sinonSandbox.stub(OneSignalApiShared, "sendOutcome").resolves(); sinonSandbox.stub(OneSignal, "privateIsPushNotificationsEnabled").resolves(true); sinonSandbox.stub(MainHelper, "getCurrentNotificationType").resolves(SubscriptionStateKind.Subscribed); @@ -109,7 +109,7 @@ test("when outcome is direct and feature disabled there are no api calls", async OneSignal.config!.userConfig.outcomes!.unattributed.enabled = false; const notificationClicked = OutcomeTestHelper.generateNotification(); - await Database.put("NotificationClicked", notificationClicked); + await Database.put(TABLE_OUTCOMES_NOTIFICATION_CLICKED, notificationClicked); const apiSpy = sinonSandbox.stub(OneSignalApiShared, "sendOutcome").resolves(); sinonSandbox.stub(OneSignal, "privateIsPushNotificationsEnabled").resolves(true); sinonSandbox.stub(MainHelper, "getCurrentNotificationType").resolves(SubscriptionStateKind.Subscribed); @@ -155,7 +155,7 @@ test("when outcome is indirect and feature disabled there are no api calls", asy test("when direct outcome is sent twice, there is only one api call", async t => { const notificationClicked = OutcomeTestHelper.generateNotification(); - await Database.put("NotificationClicked", notificationClicked); + await Database.put(TABLE_OUTCOMES_NOTIFICATION_CLICKED, notificationClicked); const apiSpy = sinonSandbox.stub(OneSignalApiShared, "sendOutcome").resolves(); const logSpy = sinonSandbox.stub(Log, "warn"); sinonSandbox.stub(OneSignal, "privateIsPushNotificationsEnabled").resolves(true); @@ -228,11 +228,11 @@ test("attribution of an outcome with multiple notifications happens correctly", const notificationArr = []; let notificationReceived = OutcomeTestHelper.generateNotification(); notificationArr.push(notificationReceived.notificationId); - await Database.put("NotificationReceived", notificationReceived); + await Database.put(TABLE_OUTCOMES_NOTIFICATION_RECEIVED, notificationReceived); await OneSignal.sendUniqueOutcome(OUTCOME_NAME); notificationReceived = OutcomeTestHelper.generateNotification(); notificationArr.push(notificationReceived.notificationId); - await Database.put("NotificationReceived", notificationReceived); + await Database.put(TABLE_OUTCOMES_NOTIFICATION_RECEIVED, notificationReceived); await OneSignal.sendUniqueOutcome(OUTCOME_NAME); const sentOutcome = await Database.get("SentUniqueOutcome", OUTCOME_NAME); From 5fb9c323c4cb1dfc71b394a8294702ea88be7e39 Mon Sep 17 00:00:00 2001 From: Josh Kasten Date: Wed, 16 Aug 2023 15:56:48 +0000 Subject: [PATCH 2/8] revert change to db v2 keyPath for notif clicks This was a change done in a beta version that shouldn't have as you must make a migration if you want to make a schema change. Added comment when the code was changed and the two different states the indexedDB scehma could be in now. --- src/shared/services/IndexedDb.ts | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/shared/services/IndexedDb.ts b/src/shared/services/IndexedDb.ts index 4b22608e9..0c44e912c 100644 --- a/src/shared/services/IndexedDb.ts +++ b/src/shared/services/IndexedDb.ts @@ -111,7 +111,11 @@ export default class IndexedDb { if (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.createObjectStore("NotificationClicked", { keyPath: "notificationId" }); } if (event.oldVersion < 3) { db.createObjectStore("SentUniqueOutcome", { keyPath: "outcomeName" }); From 3e76861ef3dfaea6393d0d757772ac4129328ed6 Mon Sep 17 00:00:00 2001 From: Josh Kasten Date: Wed, 16 Aug 2023 16:07:47 +0000 Subject: [PATCH 3/8] fix keyPath issue with a new table & migrate data Bumping indexDB to v5 and added a migration. This commit address the two states NotificationClicked's keyPath can be in due an older migration being changed when it shouldn't have. See the previous commit for more details. We are doing the following indexedDB table renames in this commit: * NotificationClicked -> Outcomes.NotificationClicked * NotificationReceived -> Outcomes.NotificationReceived IndexedDB does not allow changing the keyPath to correct it so instead we must create a new table. Only "NotificationClicked" needs to be fixed however we are also renaming "NotificationReceived " and prefixing them with "Outcomes." so they are consistent. Comments in code explain in detail the effected versions and what was wrong with them. --- src/shared/services/Database.ts | 4 +- src/shared/services/IndexedDb.ts | 82 +++++++++++++++++++++++++++++++- 2 files changed, 82 insertions(+), 4 deletions(-) diff --git a/src/shared/services/Database.ts b/src/shared/services/Database.ts index 4ffff4787..7599f70c9 100644 --- a/src/shared/services/Database.ts +++ b/src/shared/services/Database.ts @@ -42,8 +42,8 @@ interface DatabaseResult { * "NotificationOpened" = Pending Notification Click events that haven't fired yet */ -export const TABLE_OUTCOMES_NOTIFICATION_CLICKED = "NotificationClicked"; -export const TABLE_OUTCOMES_NOTIFICATION_RECEIVED = "NotificationReceived"; +export const TABLE_OUTCOMES_NOTIFICATION_CLICKED = "Outcomes.NotificationClicked"; +export const TABLE_OUTCOMES_NOTIFICATION_RECEIVED = "Outcomes.NotificationReceived"; export type OneSignalDbTable = "Options" | "Ids" | "NotificationOpened" | "Sessions" | "NotificationOpened" | typeof TABLE_OUTCOMES_NOTIFICATION_RECEIVED | typeof TABLE_OUTCOMES_NOTIFICATION_CLICKED | "SentUniqueOutcome" | ModelName; diff --git a/src/shared/services/IndexedDb.ts b/src/shared/services/IndexedDb.ts index 0c44e912c..cc232e466 100644 --- a/src/shared/services/IndexedDb.ts +++ b/src/shared/services/IndexedDb.ts @@ -4,7 +4,7 @@ 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 { @@ -102,7 +102,12 @@ 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; + 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; if (event.oldVersion < 1) { db.createObjectStore("Ids", { keyPath: "type" }); db.createObjectStore("NotificationOpened", { keyPath: "url" }); @@ -115,6 +120,7 @@ export default class IndexedDb { // "{ 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 create to trigger a migration to fix this bug. db.createObjectStore("NotificationClicked", { keyPath: "notificationId" }); } if (event.oldVersion < 3) { @@ -128,6 +134,10 @@ export default class IndexedDb { db.createObjectStore(ModelName.EmailSubscriptions, { keyPath: "modelId" }); } if (event.oldVersion < 5) { + this.migrateOutcomesNotificationClickedTableForV5(db, transaction); + this.migrateOutcomesNotificationReceivedTableForV5(db, transaction); + } + if (event.oldVersion < 6) { // Make sure to update the database version at the top of the file } // Wrap in conditional for tests @@ -136,6 +146,74 @@ 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 = (event: any) => { // Using any here as the TypeScript definition is wrong + const cursorResult = event.target.result as IDBCursorWithValue; + if (!cursorResult) { + // Delete old table once we have gone through all records + db.deleteObjectStore(oldTableName); + return; + } + const oldValue = cursorResult.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, + }); + cursorResult.continue(); + }; + cursor.onerror = () => { throw 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 = (event: any) => { // Using any here as the TypeScript definition is wrong + const cursorResult = event.target.result as IDBCursorWithValue; + if (!cursorResult) { + // Delete old table once we have gone through all records + db.deleteObjectStore(oldTableName); + return; + } + transaction + .objectStore(newTableName) + .put(cursorResult.value); + cursorResult.continue(); + }; + cursor.onerror = () => { throw 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). From 4a5ccbe9f79c81e02d541ffb1ca767e96b2a7351 Mon Sep 17 00:00:00 2001 From: Josh Kasten Date: Wed, 16 Aug 2023 16:11:10 +0000 Subject: [PATCH 4/8] add this biding to indexedDB's open event handlers These should have "this" binded to them so they can access class level properties. Became necessary since the introduction of this.migrateOutcomesNotificationClickedTableForV5(). --- src/shared/services/IndexedDb.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/shared/services/IndexedDb.ts b/src/shared/services/IndexedDb.ts index cc232e466..2ff15d5b5 100644 --- a/src/shared/services/IndexedDb.ts +++ b/src/shared/services/IndexedDb.ts @@ -29,9 +29,9 @@ export default class IndexedDb { 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; From 18ef7c21af761c424a71e8213a765f81dcae8bc2 Mon Sep 17 00:00:00 2001 From: Josh Kasten Date: Wed, 16 Aug 2023 19:05:04 +0000 Subject: [PATCH 5/8] migrate exisiting indexedDb.ts tests to jest --- __test__/unit/services/indexedDb.test.ts | 23 +++++++++++++++++++++++ test/unit/services/indexedDb.ts | 19 ------------------- 2 files changed, 23 insertions(+), 19 deletions(-) create mode 100644 __test__/unit/services/indexedDb.test.ts delete mode 100644 test/unit/services/indexedDb.ts diff --git a/__test__/unit/services/indexedDb.test.ts b/__test__/unit/services/indexedDb.test.ts new file mode 100644 index 000000000..205c6c815 --- /dev/null +++ b/__test__/unit/services/indexedDb.test.ts @@ -0,0 +1,23 @@ +// import "../../../test/support/polyfills/polyfills"; +import IndexedDb from "../../../src/shared/services/IndexedDb"; +import Random from "../../support/utils/Random"; + +require("fake-indexeddb/auto"); + +describe('Options access', () => { + + test('should get and set value', async () => { + const db = new IndexedDb(Random.getRandomString(10)); + 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 = new IndexedDb(Random.getRandomString(10)); + await db.put("Options", { key: 'optionsKey', value: 'optionsValue' }); + await db.remove("Options", 'optionsKey'); + const retrievedValue = await db.get("Options", 'optionsKey'); + expect(retrievedValue).toBeUndefined(); + }); +}); diff --git a/test/unit/services/indexedDb.ts b/test/unit/services/indexedDb.ts deleted file mode 100644 index 4b38bf545..000000000 --- a/test/unit/services/indexedDb.ts +++ /dev/null @@ -1,19 +0,0 @@ -import "../../support/polyfills/polyfills"; -import test from "ava"; -import IndexedDb from "../../../src/shared/services/IndexedDb"; -import Random from "../../support/tester/Random"; - -test(`should get and set value`, async t => { - const db = new IndexedDb(Random.getRandomString(10)); - await db.put("Options", { key: 'optionsKey', value: 'optionsValue' }); - const retrievedValue = await db.get("Options", 'optionsKey'); - t.deepEqual(retrievedValue, { key: 'optionsKey', value: 'optionsValue' }); -}); - -test(`should remove value`, async t => { - const db = new IndexedDb(Random.getRandomString(10)); - await db.put("Options", { key: 'optionsKey', value: 'optionsValue' }); - await db.remove("Options", 'optionsKey'); - const retrievedValue = await db.get("Options", 'optionsKey'); - t.is(retrievedValue, undefined); -}); \ No newline at end of file From bea1270a9735600f1bdec03edfc32b74e8ecceaf Mon Sep 17 00:00:00 2001 From: Josh Kasten Date: Thu, 17 Aug 2023 00:26:45 +0000 Subject: [PATCH 6/8] add tests for new db v5 migrations had to make a few changs to IndexedDb.ts class to make it testable. --- __test__/unit/services/indexedDb.test.ts | 90 ++++++++++++++++++++++-- src/shared/services/IndexedDb.ts | 29 +++++--- 2 files changed, 106 insertions(+), 13 deletions(-) diff --git a/__test__/unit/services/indexedDb.test.ts b/__test__/unit/services/indexedDb.test.ts index 205c6c815..07854977a 100644 --- a/__test__/unit/services/indexedDb.test.ts +++ b/__test__/unit/services/indexedDb.test.ts @@ -1,23 +1,105 @@ -// import "../../../test/support/polyfills/polyfills"; import IndexedDb from "../../../src/shared/services/IndexedDb"; import Random from "../../support/utils/Random"; require("fake-indexeddb/auto"); -describe('Options access', () => { +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 = new IndexedDb(Random.getRandomString(10)); + 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 = new IndexedDb(Random.getRandomString(10)); + 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((resolve) => { + openDbRequest.onupgradeneeded = function (event: any) { + const target = event.target as IDBOpenDBRequest; + const db = target.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}]); + }); + }); +}); diff --git a/src/shared/services/IndexedDb.ts b/src/shared/services/IndexedDb.ts index 2ff15d5b5..8eed1f86a 100644 --- a/src/shared/services/IndexedDb.ts +++ b/src/shared/services/IndexedDb.ts @@ -7,12 +7,14 @@ import Log from '../libraries/Log'; const DATABASE_VERSION = 5; export default class IndexedDb { - public emitter: Emitter; private database: IDBDatabase | undefined; private openLock: Promise | undefined; - constructor(private databaseName: string) { + constructor( + private readonly databaseName: string, + private readonly dbVersion = DATABASE_VERSION, + ) { this.emitter = new Emitter(); } @@ -21,7 +23,7 @@ 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 @@ -41,6 +43,14 @@ export default class IndexedDb { }); } + public async close(): Promise { + // 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 { if (!this.openLock) { this.openLock = this.open(this.databaseName); @@ -108,12 +118,13 @@ export default class IndexedDb { throw Error("Can't migrate DB without a transaction"); } const db = target.result; - if (event.oldVersion < 1) { + 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" }); // NOTE: 160000.beta4 to 160000 releases modified this line below as @@ -123,21 +134,21 @@ export default class IndexedDb { // DB v5 was create 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 (event.oldVersion < 6) { + if (newDbVersion >= 6 && event.oldVersion < 6) { // Make sure to update the database version at the top of the file } // Wrap in conditional for tests From f47f09029f9b2a69d2b560cfe5af71dfbbfd50a5 Mon Sep 17 00:00:00 2001 From: Josh Kasten Date: Thu, 17 Aug 2023 02:36:53 +0000 Subject: [PATCH 7/8] cleaned up any usage with indexedDb calls --- __test__/unit/services/indexedDb.test.ts | 5 ++--- src/shared/services/IndexedDb.ts | 18 ++++++++---------- 2 files changed, 10 insertions(+), 13 deletions(-) diff --git a/__test__/unit/services/indexedDb.test.ts b/__test__/unit/services/indexedDb.test.ts index 07854977a..bc82168ab 100644 --- a/__test__/unit/services/indexedDb.test.ts +++ b/__test__/unit/services/indexedDb.test.ts @@ -78,9 +78,8 @@ describe('migrations', () => { openDbRequest.onsuccess = resolve; }); const dbUpgradePromise = new Promise((resolve) => { - openDbRequest.onupgradeneeded = function (event: any) { - const target = event.target as IDBOpenDBRequest; - const db = target.result; + openDbRequest.onupgradeneeded = () => { + const db = openDbRequest.result; db.createObjectStore("NotificationClicked", { keyPath: "notification.id" }); db.createObjectStore("NotificationReceived", { keyPath: "notificationId" }); resolve(); diff --git a/src/shared/services/IndexedDb.ts b/src/shared/services/IndexedDb.ts index 8eed1f86a..5cba04074 100644 --- a/src/shared/services/IndexedDb.ts +++ b/src/shared/services/IndexedDb.ts @@ -176,14 +176,13 @@ export default class IndexedDb { const oldTableName = "NotificationClicked" const cursor = transaction.objectStore(oldTableName).openCursor(); - cursor.onsuccess = (event: any) => { // Using any here as the TypeScript definition is wrong - const cursorResult = event.target.result as IDBCursorWithValue; - if (!cursorResult) { + cursor.onsuccess = () => { + if (!cursor.result) { // Delete old table once we have gone through all records db.deleteObjectStore(oldTableName); return; } - const oldValue = cursorResult.value; + const oldValue = cursor.result.value; transaction .objectStore(newTableName) .put({ @@ -192,7 +191,7 @@ export default class IndexedDb { appId: oldValue.appId, timestamp: oldValue.timestamp, }); - cursorResult.continue(); + cursor.result.continue(); }; cursor.onerror = () => { throw cursor.error; }; } @@ -210,17 +209,16 @@ export default class IndexedDb { const oldTableName = "NotificationReceived" const cursor = transaction.objectStore(oldTableName).openCursor(); - cursor.onsuccess = (event: any) => { // Using any here as the TypeScript definition is wrong - const cursorResult = event.target.result as IDBCursorWithValue; - if (!cursorResult) { + cursor.onsuccess = () => { + if (!cursor.result) { // Delete old table once we have gone through all records db.deleteObjectStore(oldTableName); return; } transaction .objectStore(newTableName) - .put(cursorResult.value); - cursorResult.continue(); + .put(cursor.result.value); + cursor.result.continue(); }; cursor.onerror = () => { throw cursor.error; }; } From 38e14316a0d009fab435586619e65f4f8bdc03a2 Mon Sep 17 00:00:00 2001 From: Josh Kasten Date: Thu, 17 Aug 2023 02:39:06 +0000 Subject: [PATCH 8/8] log error instead of throwing on failed migration --- src/shared/services/IndexedDb.ts | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/src/shared/services/IndexedDb.ts b/src/shared/services/IndexedDb.ts index 5cba04074..db42ba367 100644 --- a/src/shared/services/IndexedDb.ts +++ b/src/shared/services/IndexedDb.ts @@ -131,7 +131,7 @@ export default class IndexedDb { // "{ 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 create to trigger a migration to fix this bug. + // DB v5 was created to trigger a migration to fix this bug. db.createObjectStore("NotificationClicked", { keyPath: "notificationId" }); } if (newDbVersion >= 3 && event.oldVersion < 3) { @@ -193,7 +193,12 @@ export default class IndexedDb { }); cursor.result.continue(); }; - cursor.onerror = () => { throw cursor.error; }; + 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" @@ -220,7 +225,12 @@ export default class IndexedDb { .put(cursor.result.value); cursor.result.continue(); }; - cursor.onerror = () => { throw cursor.error; }; + 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); + }; } /**