From 908d3d165e869b6cf56a462cdfe42c3571dfcad4 Mon Sep 17 00:00:00 2001 From: Justin Baur <19896123+justindbaur@users.noreply.github.com> Date: Mon, 25 Mar 2024 09:28:42 -0500 Subject: [PATCH] [PM-6965] Add `type` Property to `MigrationHelper` (#8411) * Add `type` Property to `MigrationHelper` * Fix Tests * Make `type` parameter required * Fix mockHelper.type * Remove `readonly` from `type` --- .../src/app/platform/web-migration-runner.ts | 2 +- .../migration-builder.service.spec.ts | 1 + .../src/platform/services/migration-runner.ts | 1 + .../migration-builder.spec.ts | 14 +++++++------- .../state-migrations/migration-helper.spec.ts | 12 ++++++++---- .../src/state-migrations/migration-helper.ts | 19 ++++++++++++++++++- .../src/state-migrations/migrator.spec.ts | 2 +- 7 files changed, 37 insertions(+), 14 deletions(-) diff --git a/apps/web/src/app/platform/web-migration-runner.ts b/apps/web/src/app/platform/web-migration-runner.ts index e8f44f7f41..4bd1d2d4b5 100644 --- a/apps/web/src/app/platform/web-migration-runner.ts +++ b/apps/web/src/app/platform/web-migration-runner.ts @@ -46,7 +46,7 @@ class WebMigrationHelper extends MigrationHelper { storageService: WindowStorageService, logService: LogService, ) { - super(currentVersion, storageService, logService); + super(currentVersion, storageService, logService, "web-disk-local"); this.diskLocalStorageService = storageService; } diff --git a/libs/common/src/platform/services/migration-builder.service.spec.ts b/libs/common/src/platform/services/migration-builder.service.spec.ts index c06d4bf53c..1330ea07a4 100644 --- a/libs/common/src/platform/services/migration-builder.service.spec.ts +++ b/libs/common/src/platform/services/migration-builder.service.spec.ts @@ -82,6 +82,7 @@ describe("MigrationBuilderService", () => { startingStateVersion, new FakeStorageService(startingState), mock(), + "general", ); await sut.build().migrate(helper); diff --git a/libs/common/src/platform/services/migration-runner.ts b/libs/common/src/platform/services/migration-runner.ts index f900343424..006031f7e5 100644 --- a/libs/common/src/platform/services/migration-runner.ts +++ b/libs/common/src/platform/services/migration-runner.ts @@ -18,6 +18,7 @@ export class MigrationRunner { await currentVersion(this.diskStorage, this.logService), this.diskStorage, this.logService, + "general", ); if (migrationHelper.currentVersion < 0) { diff --git a/libs/common/src/state-migrations/migration-builder.spec.ts b/libs/common/src/state-migrations/migration-builder.spec.ts index f10d3b11a9..6a4ff8e6d4 100644 --- a/libs/common/src/state-migrations/migration-builder.spec.ts +++ b/libs/common/src/state-migrations/migration-builder.spec.ts @@ -83,35 +83,35 @@ describe("MigrationBuilder", () => { }); it("should migrate", async () => { - const helper = new MigrationHelper(0, mock(), mock()); + const helper = new MigrationHelper(0, mock(), mock(), "general"); const spy = jest.spyOn(migrator, "migrate"); await sut.migrate(helper); expect(spy).toBeCalledWith(helper); }); it("should rollback", async () => { - const helper = new MigrationHelper(1, mock(), mock()); + const helper = new MigrationHelper(1, mock(), mock(), "general"); const spy = jest.spyOn(rollback_migrator, "rollback"); await sut.migrate(helper); expect(spy).toBeCalledWith(helper); }); it("should update version on migrate", async () => { - const helper = new MigrationHelper(0, mock(), mock()); + const helper = new MigrationHelper(0, mock(), mock(), "general"); const spy = jest.spyOn(migrator, "updateVersion"); await sut.migrate(helper); expect(spy).toBeCalledWith(helper, "up"); }); it("should update version on rollback", async () => { - const helper = new MigrationHelper(1, mock(), mock()); + const helper = new MigrationHelper(1, mock(), mock(), "general"); const spy = jest.spyOn(rollback_migrator, "updateVersion"); await sut.migrate(helper); expect(spy).toBeCalledWith(helper, "down"); }); it("should not run the migrator if the current version does not match the from version", async () => { - const helper = new MigrationHelper(3, mock(), mock()); + const helper = new MigrationHelper(3, mock(), mock(), "general"); const migrate = jest.spyOn(migrator, "migrate"); const rollback = jest.spyOn(rollback_migrator, "rollback"); await sut.migrate(helper); @@ -120,7 +120,7 @@ describe("MigrationBuilder", () => { }); it("should not update version if the current version does not match the from version", async () => { - const helper = new MigrationHelper(3, mock(), mock()); + const helper = new MigrationHelper(3, mock(), mock(), "general"); const migrate = jest.spyOn(migrator, "updateVersion"); const rollback = jest.spyOn(rollback_migrator, "updateVersion"); await sut.migrate(helper); @@ -130,7 +130,7 @@ describe("MigrationBuilder", () => { }); it("should be able to call instance methods", async () => { - const helper = new MigrationHelper(0, mock(), mock()); + const helper = new MigrationHelper(0, mock(), mock(), "general"); await sut.with(TestMigratorWithInstanceMethod, 0, 1).migrate(helper); }); }); diff --git a/libs/common/src/state-migrations/migration-helper.spec.ts b/libs/common/src/state-migrations/migration-helper.spec.ts index e929877b63..f86cac8768 100644 --- a/libs/common/src/state-migrations/migration-helper.spec.ts +++ b/libs/common/src/state-migrations/migration-helper.spec.ts @@ -9,7 +9,7 @@ import { AbstractStorageService } from "../platform/abstractions/storage.service // eslint-disable-next-line import/no-restricted-paths -- Needed to generate unique strings for injection import { Utils } from "../platform/misc/utils"; -import { MigrationHelper } from "./migration-helper"; +import { MigrationHelper, MigrationHelperType } from "./migration-helper"; import { Migrator } from "./migrator"; const exampleJSON = { @@ -37,7 +37,7 @@ describe("RemoveLegacyEtmKeyMigrator", () => { storage = mock(); storage.get.mockImplementation((key) => (exampleJSON as any)[key]); - sut = new MigrationHelper(0, storage, logService); + sut = new MigrationHelper(0, storage, logService, "general"); }); describe("get", () => { @@ -150,6 +150,7 @@ describe("RemoveLegacyEtmKeyMigrator", () => { export function mockMigrationHelper( storageJson: any, stateVersion = 0, + type: MigrationHelperType = "general", ): MockProxy { const logService: MockProxy = mock(); const storage: MockProxy = mock(); @@ -157,7 +158,7 @@ export function mockMigrationHelper( storage.save.mockImplementation(async (key, value) => { (storageJson as any)[key] = value; }); - const helper = new MigrationHelper(stateVersion, storage, logService); + const helper = new MigrationHelper(stateVersion, storage, logService, type); const mockHelper = mock(); mockHelper.get.mockImplementation((key) => helper.get(key)); @@ -175,6 +176,9 @@ export function mockMigrationHelper( helper.setToUser(userId, keyDefinition, value), ); mockHelper.getAccounts.mockImplementation(() => helper.getAccounts()); + + mockHelper.type = helper.type; + return mockHelper; } @@ -291,7 +295,7 @@ export async function runMigrator< const allInjectedData = injectData(initalData, []); const fakeStorageService = new FakeStorageService(initalData); - const helper = new MigrationHelper(migrator.fromVersion, fakeStorageService, mock()); + const helper = new MigrationHelper(migrator.fromVersion, fakeStorageService, mock(), "general"); // Run their migrations if (direction === "rollback") { diff --git a/libs/common/src/state-migrations/migration-helper.ts b/libs/common/src/state-migrations/migration-helper.ts index 315a343e9e..5b8e4ff93e 100644 --- a/libs/common/src/state-migrations/migration-helper.ts +++ b/libs/common/src/state-migrations/migration-helper.ts @@ -9,12 +9,29 @@ export type KeyDefinitionLike = { key: string; }; +export type MigrationHelperType = "general" | "web-disk-local"; + export class MigrationHelper { constructor( public currentVersion: number, private storageService: AbstractStorageService, public logService: LogService, - ) {} + type: MigrationHelperType, + ) { + this.type = type; + } + + /** + * On some clients, migrations are ran multiple times without direct action from the migration writer. + * + * All clients will run through migrations at least once, this run is referred to as `"general"`. If a migration is + * ran more than that single time, they will get a unique name if that the write can make conditional logic based on which + * migration run this is. + * + * @remarks The preferrable way of writing migrations is ALWAYS to be defensive and reflect on the data you are given back. This + * should really only be used when reflecting on the data given isn't enough. + */ + type: MigrationHelperType; /** * Gets a value from the storage service at the given key. diff --git a/libs/common/src/state-migrations/migrator.spec.ts b/libs/common/src/state-migrations/migrator.spec.ts index 3abaa27727..d1189c25ea 100644 --- a/libs/common/src/state-migrations/migrator.spec.ts +++ b/libs/common/src/state-migrations/migrator.spec.ts @@ -26,7 +26,7 @@ describe("migrator default methods", () => { beforeEach(() => { storage = mock(); logService = mock(); - helper = new MigrationHelper(0, storage, logService); + helper = new MigrationHelper(0, storage, logService, "general"); sut = new TestMigrator(0, 1); });