From 4326d59be9e7068c6c906db1c209530309fea2de Mon Sep 17 00:00:00 2001 From: Danny Martini Date: Thu, 11 Apr 2024 09:20:48 +0200 Subject: [PATCH] fix(core): Don't revert irreversibble migrations (#9105) --- packages/cli/src/commands/db/revert.ts | 41 ++++-- .../src/databases/utils/migrationHelpers.ts | 40 +++--- .../cli/test/unit/commands/db/revert.test.ts | 134 ++++++++++++++++++ 3 files changed, 191 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..f87076394a8aa 100644 --- a/packages/cli/src/databases/utils/migrationHelpers.ts +++ b/packages/cli/src/databases/utils/migrationHelpers.ts @@ -176,26 +176,34 @@ 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); + }, + }); + } 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, { + async down(this: BaseMigration, queryRunner: QueryRunner) { 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..d58e132607e5e --- /dev/null +++ b/packages/cli/test/unit/commands/db/revert.test.ts @@ -0,0 +1,134 @@ +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(); +}); + +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(); +});