1
0
mirror of https://github.com/bitwarden/browser.git synced 2024-12-22 16:29:09 +01:00

[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`
This commit is contained in:
Justin Baur 2024-03-25 09:28:42 -05:00 committed by GitHub
parent 8639f494f3
commit 908d3d165e
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
7 changed files with 37 additions and 14 deletions

View File

@ -46,7 +46,7 @@ class WebMigrationHelper extends MigrationHelper {
storageService: WindowStorageService, storageService: WindowStorageService,
logService: LogService, logService: LogService,
) { ) {
super(currentVersion, storageService, logService); super(currentVersion, storageService, logService, "web-disk-local");
this.diskLocalStorageService = storageService; this.diskLocalStorageService = storageService;
} }

View File

@ -82,6 +82,7 @@ describe("MigrationBuilderService", () => {
startingStateVersion, startingStateVersion,
new FakeStorageService(startingState), new FakeStorageService(startingState),
mock(), mock(),
"general",
); );
await sut.build().migrate(helper); await sut.build().migrate(helper);

View File

@ -18,6 +18,7 @@ export class MigrationRunner {
await currentVersion(this.diskStorage, this.logService), await currentVersion(this.diskStorage, this.logService),
this.diskStorage, this.diskStorage,
this.logService, this.logService,
"general",
); );
if (migrationHelper.currentVersion < 0) { if (migrationHelper.currentVersion < 0) {

View File

@ -83,35 +83,35 @@ describe("MigrationBuilder", () => {
}); });
it("should migrate", async () => { 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"); const spy = jest.spyOn(migrator, "migrate");
await sut.migrate(helper); await sut.migrate(helper);
expect(spy).toBeCalledWith(helper); expect(spy).toBeCalledWith(helper);
}); });
it("should rollback", async () => { 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"); const spy = jest.spyOn(rollback_migrator, "rollback");
await sut.migrate(helper); await sut.migrate(helper);
expect(spy).toBeCalledWith(helper); expect(spy).toBeCalledWith(helper);
}); });
it("should update version on migrate", async () => { 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"); const spy = jest.spyOn(migrator, "updateVersion");
await sut.migrate(helper); await sut.migrate(helper);
expect(spy).toBeCalledWith(helper, "up"); expect(spy).toBeCalledWith(helper, "up");
}); });
it("should update version on rollback", async () => { 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"); const spy = jest.spyOn(rollback_migrator, "updateVersion");
await sut.migrate(helper); await sut.migrate(helper);
expect(spy).toBeCalledWith(helper, "down"); expect(spy).toBeCalledWith(helper, "down");
}); });
it("should not run the migrator if the current version does not match the from version", async () => { 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 migrate = jest.spyOn(migrator, "migrate");
const rollback = jest.spyOn(rollback_migrator, "rollback"); const rollback = jest.spyOn(rollback_migrator, "rollback");
await sut.migrate(helper); 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 () => { 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 migrate = jest.spyOn(migrator, "updateVersion");
const rollback = jest.spyOn(rollback_migrator, "updateVersion"); const rollback = jest.spyOn(rollback_migrator, "updateVersion");
await sut.migrate(helper); await sut.migrate(helper);
@ -130,7 +130,7 @@ describe("MigrationBuilder", () => {
}); });
it("should be able to call instance methods", async () => { 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); await sut.with(TestMigratorWithInstanceMethod, 0, 1).migrate(helper);
}); });
}); });

View File

@ -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 // eslint-disable-next-line import/no-restricted-paths -- Needed to generate unique strings for injection
import { Utils } from "../platform/misc/utils"; import { Utils } from "../platform/misc/utils";
import { MigrationHelper } from "./migration-helper"; import { MigrationHelper, MigrationHelperType } from "./migration-helper";
import { Migrator } from "./migrator"; import { Migrator } from "./migrator";
const exampleJSON = { const exampleJSON = {
@ -37,7 +37,7 @@ describe("RemoveLegacyEtmKeyMigrator", () => {
storage = mock(); storage = mock();
storage.get.mockImplementation((key) => (exampleJSON as any)[key]); storage.get.mockImplementation((key) => (exampleJSON as any)[key]);
sut = new MigrationHelper(0, storage, logService); sut = new MigrationHelper(0, storage, logService, "general");
}); });
describe("get", () => { describe("get", () => {
@ -150,6 +150,7 @@ describe("RemoveLegacyEtmKeyMigrator", () => {
export function mockMigrationHelper( export function mockMigrationHelper(
storageJson: any, storageJson: any,
stateVersion = 0, stateVersion = 0,
type: MigrationHelperType = "general",
): MockProxy<MigrationHelper> { ): MockProxy<MigrationHelper> {
const logService: MockProxy<LogService> = mock(); const logService: MockProxy<LogService> = mock();
const storage: MockProxy<AbstractStorageService> = mock(); const storage: MockProxy<AbstractStorageService> = mock();
@ -157,7 +158,7 @@ export function mockMigrationHelper(
storage.save.mockImplementation(async (key, value) => { storage.save.mockImplementation(async (key, value) => {
(storageJson as any)[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<MigrationHelper>(); const mockHelper = mock<MigrationHelper>();
mockHelper.get.mockImplementation((key) => helper.get(key)); mockHelper.get.mockImplementation((key) => helper.get(key));
@ -175,6 +176,9 @@ export function mockMigrationHelper(
helper.setToUser(userId, keyDefinition, value), helper.setToUser(userId, keyDefinition, value),
); );
mockHelper.getAccounts.mockImplementation(() => helper.getAccounts()); mockHelper.getAccounts.mockImplementation(() => helper.getAccounts());
mockHelper.type = helper.type;
return mockHelper; return mockHelper;
} }
@ -291,7 +295,7 @@ export async function runMigrator<
const allInjectedData = injectData(initalData, []); const allInjectedData = injectData(initalData, []);
const fakeStorageService = new FakeStorageService(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 // Run their migrations
if (direction === "rollback") { if (direction === "rollback") {

View File

@ -9,12 +9,29 @@ export type KeyDefinitionLike = {
key: string; key: string;
}; };
export type MigrationHelperType = "general" | "web-disk-local";
export class MigrationHelper { export class MigrationHelper {
constructor( constructor(
public currentVersion: number, public currentVersion: number,
private storageService: AbstractStorageService, private storageService: AbstractStorageService,
public logService: LogService, 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. * Gets a value from the storage service at the given key.

View File

@ -26,7 +26,7 @@ describe("migrator default methods", () => {
beforeEach(() => { beforeEach(() => {
storage = mock(); storage = mock();
logService = mock(); logService = mock();
helper = new MigrationHelper(0, storage, logService); helper = new MigrationHelper(0, storage, logService, "general");
sut = new TestMigrator(0, 1); sut = new TestMigrator(0, 1);
}); });