From 2cdaffa059941748dd8ed63b1427dbff3c03e7cd Mon Sep 17 00:00:00 2001 From: Danny Martini Date: Tue, 9 Apr 2024 20:55:42 +0200 Subject: [PATCH 1/3] don't revert irreversible migrations --- packages/cli/src/commands/db/revert.ts | 41 +++++-- .../src/databases/utils/migrationHelpers.ts | 37 +++--- .../cli/test/unit/commands/db/revert.test.ts | 105 ++++++++++++++++++ 3 files changed, 159 insertions(+), 24 deletions(-) create mode 100644 packages/cli/test/unit/commands/db/revert.test.ts diff --git a/packages/cli/src/commands/db/revert.ts b/packages/cli/src/commands/db/revert.ts index 689598e819b4d..4ac9a06733ccf 100644 --- a/packages/cli/src/commands/db/revert.ts +++ b/packages/cli/src/commands/db/revert.ts @@ -9,6 +9,38 @@ import type { Migration } from '@db/types'; import { wrapMigration } from '@db/utils/migrationHelpers'; import config from '@/config'; +// This function is extracted to make it easier to unit test it. +// Mocking turned into a mess due to this command using typeorm and the db +// config directly and customizing and monkey patching parts. +export async function main( + connectionOptions: ConnectionOptions, + logger: Logger, + DataSource: typeof Connection, +) { + const dbType = config.getEnv('database.type'); + + (connectionOptions.migrations as Migration[]).forEach(wrapMigration); + + const connection = new DataSource(connectionOptions); + await connection.initialize(); + if (dbType === 'postgresdb') await setSchema(connection); + + const lastMigration = connection.migrations.at(-1); + + if (lastMigration === undefined) { + logger.error('There is no migration to reverse.'); + return; + } + + if (!lastMigration.down) { + logger.error('The last migration was irreversible and cannot be reverted.'); + return; + } + + await connection.undoLastMigration(); + await connection.destroy(); +} + export class DbRevertMigrationCommand extends Command { static description = 'Revert last database migration'; @@ -27,7 +59,6 @@ export class DbRevertMigrationCommand extends Command { } async run() { - const dbType = config.getEnv('database.type'); const connectionOptions: ConnectionOptions = { ...getConnectionOptions(), subscribers: [], @@ -37,13 +68,7 @@ export class DbRevertMigrationCommand extends Command { logging: ['query', 'error', 'schema'], }; - (connectionOptions.migrations as Migration[]).forEach(wrapMigration); - - this.connection = new Connection(connectionOptions); - await this.connection.initialize(); - if (dbType === 'postgresdb') await setSchema(this.connection); - await this.connection.undoLastMigration(); - await this.connection.destroy(); + return await main(connectionOptions, this.logger, Connection); } async catch(error: Error) { diff --git a/packages/cli/src/databases/utils/migrationHelpers.ts b/packages/cli/src/databases/utils/migrationHelpers.ts index 6909e765fbeb7..c5a24f6b8b867 100644 --- a/packages/cli/src/databases/utils/migrationHelpers.ts +++ b/packages/cli/src/databases/utils/migrationHelpers.ts @@ -176,26 +176,31 @@ const createContext = (queryRunner: QueryRunner, migration: Migration): Migratio export const wrapMigration = (migration: Migration) => { const { up, down } = migration.prototype; - Object.assign(migration.prototype, { - async up(this: BaseMigration, queryRunner: QueryRunner) { - logMigrationStart(migration.name); - const context = createContext(queryRunner, migration); - if (this.transaction === false) { - await runDisablingForeignKeys(this, context, up); - } else { - await up.call(this, context); - } - logMigrationEnd(migration.name); - }, - async down(this: BaseMigration, queryRunner: QueryRunner) { - if (down) { + if (up) { + Object.assign(migration.prototype, { + async up(this: BaseMigration, queryRunner: QueryRunner) { + logMigrationStart(migration.name); + const context = createContext(queryRunner, migration); + if (this.transaction === false) { + await runDisablingForeignKeys(this, context, up); + } else { + await up.call(this, context); + } + logMigrationEnd(migration.name); + }, + }); + } + if (down) { + Object.assign(migration.prototype, { + async down(this: BaseMigration, queryRunner: QueryRunner) { + console.log('down'); const context = createContext(queryRunner, migration); if (this.transaction === false) { await runDisablingForeignKeys(this, context, down); } else { await down.call(this, context); } - } - }, - }); + }, + }); + } }; diff --git a/packages/cli/test/unit/commands/db/revert.test.ts b/packages/cli/test/unit/commands/db/revert.test.ts new file mode 100644 index 0000000000000..131dfe2b03da7 --- /dev/null +++ b/packages/cli/test/unit/commands/db/revert.test.ts @@ -0,0 +1,105 @@ +import { main } from '@/commands/db/revert'; +import { mockInstance } from '../../../shared/mocking'; +import { Logger } from '@/Logger'; +import * as DbConfig from '@db/config'; +import type { IrreversibleMigration, ReversibleMigration } from '@/databases/types'; +import type { DataSource } from '@n8n/typeorm'; +import { mock } from 'jest-mock-extended'; + +const logger = mockInstance(Logger); + +afterEach(() => { + jest.resetAllMocks(); +}); + +test("don't revert migrations if there is no migration", async () => { + // + // ARRANGE + // + const connectionOptions = DbConfig.getConnectionOptions(); + // @ts-expect-error property is readonly + connectionOptions.migrations = []; + const dataSource = mock({ migrations: [] }); + + // + // ACT + // + await main(connectionOptions, logger, function () { + return dataSource; + } as never); + + // + // ASSERT + // + expect(logger.error).toHaveBeenCalledTimes(1); + expect(logger.error).toHaveBeenCalledWith('There is no migration to reverse.'); + expect(dataSource.undoLastMigration).not.toHaveBeenCalled(); + expect(dataSource.destroy).not.toHaveBeenCalled(); +}); + +test("don't revert the last migration if it had no down migration", async () => { + // + // ARRANGE + // + class TestMigration implements IrreversibleMigration { + async up() {} + } + + const connectionOptions = DbConfig.getConnectionOptions(); + const migrations = [TestMigration]; + // @ts-expect-error property is readonly + connectionOptions.migrations = migrations; + const dataSource = mock(); + // @ts-expect-error property is readonly, and I can't pass them the `mock` + // because `mock` will mock the down method and thus defeat the purpose + // of this test, because the tested code will assume that the migration has a + // down method. + dataSource.migrations = migrations.map((M) => new M()); + + // + // ACT + // + await main(connectionOptions, logger, function () { + return dataSource; + } as never); + + // + // ASSERT + // + expect(logger.error).toHaveBeenCalledTimes(1); + expect(logger.error).toHaveBeenCalledWith( + 'The last migration was irreversible and cannot be reverted.', + ); + expect(dataSource.undoLastMigration).not.toHaveBeenCalled(); + expect(dataSource.destroy).not.toHaveBeenCalled(); +}); + +test('revert the last migration if it has a down migration', async () => { + // + // ARRANGE + // + class TestMigration implements ReversibleMigration { + async up() {} + + async down() {} + } + + const connectionOptions = DbConfig.getConnectionOptions(); + // @ts-expect-error property is readonly + connectionOptions.migrations = [TestMigration]; + const dataSource = mock({ migrations: [new TestMigration()] }); + + // + // ACT + // + await main(connectionOptions, logger, function () { + return dataSource; + } as never); + + // + // ASSERT + // + expect(logger.error).not.toHaveBeenCalled(); + expect(dataSource.undoLastMigration).toHaveBeenCalled(); + expect(dataSource.destroy).toHaveBeenCalled(); +}); From e37993ac784aaa08e2de2b3ea44bc9d85b76d974 Mon Sep 17 00:00:00 2001 From: Danny Martini Date: Thu, 11 Apr 2024 06:46:46 +0200 Subject: [PATCH 2/3] remove stray console.log --- packages/cli/src/databases/utils/migrationHelpers.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/cli/src/databases/utils/migrationHelpers.ts b/packages/cli/src/databases/utils/migrationHelpers.ts index c5a24f6b8b867..d4a2a6abe65c3 100644 --- a/packages/cli/src/databases/utils/migrationHelpers.ts +++ b/packages/cli/src/databases/utils/migrationHelpers.ts @@ -193,7 +193,6 @@ export const wrapMigration = (migration: Migration) => { if (down) { Object.assign(migration.prototype, { async down(this: BaseMigration, queryRunner: QueryRunner) { - console.log('down'); const context = createContext(queryRunner, migration); if (this.transaction === false) { await runDisablingForeignKeys(this, context, down); From 44782e465b4dcf744f0f273ceef5da8679fceacf Mon Sep 17 00:00:00 2001 From: Danny Martini Date: Thu, 11 Apr 2024 06:47:08 +0200 Subject: [PATCH 3/3] throw if a migration is missing the `up` method --- .../src/databases/utils/migrationHelpers.ts | 4 +++ .../cli/test/unit/commands/db/revert.test.ts | 29 +++++++++++++++++++ 2 files changed, 33 insertions(+) diff --git a/packages/cli/src/databases/utils/migrationHelpers.ts b/packages/cli/src/databases/utils/migrationHelpers.ts index d4a2a6abe65c3..f87076394a8aa 100644 --- a/packages/cli/src/databases/utils/migrationHelpers.ts +++ b/packages/cli/src/databases/utils/migrationHelpers.ts @@ -189,6 +189,10 @@ export const wrapMigration = (migration: Migration) => { logMigrationEnd(migration.name); }, }); + } else { + throw new ApplicationError( + 'At least on migration is missing the method `up`. Make sure all migrations are valid.', + ); } if (down) { Object.assign(migration.prototype, { diff --git a/packages/cli/test/unit/commands/db/revert.test.ts b/packages/cli/test/unit/commands/db/revert.test.ts index 131dfe2b03da7..d58e132607e5e 100644 --- a/packages/cli/test/unit/commands/db/revert.test.ts +++ b/packages/cli/test/unit/commands/db/revert.test.ts @@ -103,3 +103,32 @@ test('revert the last migration if it has a down migration', async () => { expect(dataSource.undoLastMigration).toHaveBeenCalled(); expect(dataSource.destroy).toHaveBeenCalled(); }); + +test('throw if a migration is invalid, e.g. has no `up` method', async () => { + // + // ARRANGE + // + class TestMigration {} + + const connectionOptions = DbConfig.getConnectionOptions(); + // @ts-expect-error property is readonly + connectionOptions.migrations = [TestMigration]; + const dataSource = mock({ migrations: [new TestMigration()] }); + + // + // ACT + // + await expect( + main(connectionOptions, logger, function () { + return dataSource; + } as never), + ).rejects.toThrowError( + 'At least on migration is missing the method `up`. Make sure all migrations are valid.', + ); + + // + // ASSERT + // + expect(dataSource.undoLastMigration).not.toHaveBeenCalled(); + expect(dataSource.destroy).not.toHaveBeenCalled(); +});