diff --git a/packages/cli/src/commands/db/revert.ts b/packages/cli/src/commands/db/revert.ts index 4ac9a06733ccf..eac71cd6ab583 100644 --- a/packages/cli/src/commands/db/revert.ts +++ b/packages/cli/src/commands/db/revert.ts @@ -1,6 +1,6 @@ import { Command, Flags } from '@oclif/core'; import type { DataSourceOptions as ConnectionOptions } from '@n8n/typeorm'; -import { DataSource as Connection } from '@n8n/typeorm'; +import { MigrationExecutor, DataSource as Connection } from '@n8n/typeorm'; import { Container } from 'typedi'; import { Logger } from '@/Logger'; import { setSchema } from '@/Db'; @@ -13,27 +13,44 @@ import config from '@/config'; // 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, + connection: Connection, + migrationExecutor: MigrationExecutor, ) { - const dbType = config.getEnv('database.type'); + const executedMigrations = await migrationExecutor.getExecutedMigrations(); + const lastExecutedMigration = executedMigrations.at(0); - (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 (lastExecutedMigration === undefined) { + logger.error( + "Cancelled command. The database was never migrated. Are you sure you're connected to the right database?.", + ); + return; + } - if (lastMigration === undefined) { - logger.error('There is no migration to reverse.'); + const lastMigrationInstance = connection.migrations.find((m) => { + // Migration names are optional. If a migration has no name property + // TypeORM will default to the class name. + const name1 = m.name ?? m.constructor.name; + const name2 = lastExecutedMigration.name; + + return name1 === name2; + }); + + if (lastMigrationInstance === undefined) { + logger.error( + `The last migration that was executed is "${lastExecutedMigration.name}", but I could not find that migration's code in the currently installed version of n8n.`, + ); + logger.error( + 'This usually means that you downgraded n8n before running `n8n db:revert`. Please upgrade n8n again and run `n8n db:revert` and then downgrade again.', + ); return; } - if (!lastMigration.down) { - logger.error('The last migration was irreversible and cannot be reverted.'); + if (!lastMigrationInstance.down) { + const message = lastMigrationInstance.name + ? `Cancelled command. The last migration "${lastMigrationInstance.name}" was irreversible.` + : 'Cancelled command. The last migration was irreversible.'; + logger.error(message); return; } @@ -68,7 +85,17 @@ export class DbRevertMigrationCommand extends Command { logging: ['query', 'error', 'schema'], }; - return await main(connectionOptions, this.logger, Connection); + const connection = new Connection(connectionOptions); + await connection.initialize(); + + const dbType = config.getEnv('database.type'); + if (dbType === 'postgresdb') await setSchema(connection); + + const migrationExecutor = new MigrationExecutor(connection); + + (connectionOptions.migrations as Migration[]).forEach(wrapMigration); + + return await main(this.logger, connection, migrationExecutor); } async catch(error: Error) { diff --git a/packages/cli/src/databases/utils/migrationHelpers.ts b/packages/cli/src/databases/utils/migrationHelpers.ts index f87076394a8aa..1dc4b45ec32ef 100644 --- a/packages/cli/src/databases/utils/migrationHelpers.ts +++ b/packages/cli/src/databases/utils/migrationHelpers.ts @@ -190,9 +190,7 @@ export const wrapMigration = (migration: Migration) => { }, }); } else { - throw new ApplicationError( - 'At least on migration is missing the method `up`. Make sure all migrations are valid.', - ); + throw new ApplicationError(`Migration "${migration.name}" is missing the method \`up\`.`); } 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 d58e132607e5e..cea3f89253383 100644 --- a/packages/cli/test/unit/commands/db/revert.test.ts +++ b/packages/cli/test/unit/commands/db/revert.test.ts @@ -1,9 +1,9 @@ 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 type { Migration, MigrationExecutor } from '@n8n/typeorm'; +import { type DataSource } from '@n8n/typeorm'; import { mock } from 'jest-mock-extended'; const logger = mockInstance(Logger); @@ -16,23 +16,23 @@ 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: [] }); + const migrations: Migration[] = []; + const dataSource = mock({ migrations }); + const migrationExecutor = mock(); + migrationExecutor.getExecutedMigrations.mockResolvedValue([]); // // ACT // - await main(connectionOptions, logger, function () { - return dataSource; - } as never); + await main(logger, dataSource, migrationExecutor); // // ASSERT // expect(logger.error).toHaveBeenCalledTimes(1); - expect(logger.error).toHaveBeenCalledWith('There is no migration to reverse.'); + expect(logger.error).toHaveBeenCalledWith( + "Cancelled command. The database was never migrated. Are you sure you're connected to the right database?.", + ); expect(dataSource.undoLastMigration).not.toHaveBeenCalled(); expect(dataSource.destroy).not.toHaveBeenCalled(); }); @@ -42,93 +42,130 @@ test("don't revert the last migration if it had no down migration", async () => // ARRANGE // class TestMigration implements IrreversibleMigration { + name = undefined; + async up() {} + + down = undefined; } - 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()); + const migrationsInCode = [new TestMigration()]; + const migrationsInDb: Migration[] = [{ id: 1, timestamp: Date.now(), name: 'TestMigration' }]; + const dataSource = mock({ migrations: migrationsInCode }); + + const migrationExecutor = mock(); + migrationExecutor.getExecutedMigrations.mockResolvedValue(migrationsInDb); // // ACT // - await main(connectionOptions, logger, function () { - return dataSource; - } as never); + await main(logger, dataSource, migrationExecutor); // // ASSERT // expect(logger.error).toHaveBeenCalledTimes(1); - expect(logger.error).toHaveBeenCalledWith( - 'The last migration was irreversible and cannot be reverted.', - ); + expect(logger.error).toBeCalledWith('Cancelled command. The last migration was irreversible.'); expect(dataSource.undoLastMigration).not.toHaveBeenCalled(); expect(dataSource.destroy).not.toHaveBeenCalled(); }); -test('revert the last migration if it has a down migration', async () => { +test('print migration name instead of class name in error message if the migration has a name', async () => { // // ARRANGE // - class TestMigration implements ReversibleMigration { + class TestMigration implements IrreversibleMigration { + name = 'Migration Name'; + async up() {} - async down() {} + down = undefined; } - const connectionOptions = DbConfig.getConnectionOptions(); - // @ts-expect-error property is readonly - connectionOptions.migrations = [TestMigration]; - const dataSource = mock({ migrations: [new TestMigration()] }); + const migrationsInCode = [new TestMigration()]; + const migrationsInDb: Migration[] = [{ id: 1, timestamp: Date.now(), name: 'Migration Name' }]; + const dataSource = mock({ migrations: migrationsInCode }); + + const migrationExecutor = mock(); + migrationExecutor.getExecutedMigrations.mockResolvedValue(migrationsInDb); // // ACT // - await main(connectionOptions, logger, function () { - return dataSource; - } as never); + await main(logger, dataSource, migrationExecutor); // // ASSERT // - expect(logger.error).not.toHaveBeenCalled(); - expect(dataSource.undoLastMigration).toHaveBeenCalled(); - expect(dataSource.destroy).toHaveBeenCalled(); + expect(logger.error).toHaveBeenCalledTimes(1); + expect(logger.error).toHaveBeenCalledWith( + 'Cancelled command. The last migration "Migration Name" was irreversible.', + ); + expect(dataSource.undoLastMigration).not.toHaveBeenCalled(); + expect(dataSource.destroy).not.toHaveBeenCalled(); }); -test('throw if a migration is invalid, e.g. has no `up` method', async () => { +test("don't revert the last migration if we cannot find the migration in the code", async () => { // // ARRANGE // - class TestMigration {} - const connectionOptions = DbConfig.getConnectionOptions(); - // @ts-expect-error property is readonly - connectionOptions.migrations = [TestMigration]; - const dataSource = mock({ migrations: [new TestMigration()] }); + const migrationsInDb: Migration[] = [{ id: 1, timestamp: Date.now(), name: 'TestMigration' }]; + const dataSource = mock({ migrations: [] }); + + const migrationExecutor = mock(); + migrationExecutor.getExecutedMigrations.mockResolvedValue(migrationsInDb); // // 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.', - ); + await main(logger, dataSource, migrationExecutor); // // ASSERT // + expect(logger.error).toHaveBeenCalledTimes(2); + expect(logger.error).toHaveBeenNthCalledWith( + 1, + 'The last migration that was executed is "TestMigration", but I could not find that migration\'s code in the currently installed version of n8n.', + ); + expect(logger.error).toHaveBeenNthCalledWith( + 2, + 'This usually means that you downgraded n8n before running `n8n db:revert`. Please upgrade n8n again and run `n8n db:revert` and then downgrade again.', + ); 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 { + name = 'ReversibleMigration'; + + async up() {} + + async down() {} + } + + const migrationsInDb: Migration[] = [ + { id: 1, timestamp: Date.now(), name: 'ReversibleMigration' }, + ]; + const dataSource = mock({ migrations: [new TestMigration()] }); + + const migrationExecutor = mock(); + migrationExecutor.getExecutedMigrations.mockResolvedValue(migrationsInDb); + + // + // ACT + // + await main(logger, dataSource, migrationExecutor); + + // + // ASSERT + // + expect(logger.error).not.toHaveBeenCalled(); + expect(dataSource.undoLastMigration).toHaveBeenCalled(); + expect(dataSource.destroy).toHaveBeenCalled(); +}); diff --git a/packages/cli/test/unit/databases/utils/migrationHelpers.test.ts b/packages/cli/test/unit/databases/utils/migrationHelpers.test.ts new file mode 100644 index 0000000000000..17cded335dc70 --- /dev/null +++ b/packages/cli/test/unit/databases/utils/migrationHelpers.test.ts @@ -0,0 +1,66 @@ +import type { IrreversibleMigration, ReversibleMigration } from '@/databases/types'; +import { wrapMigration } from '@/databases/utils/migrationHelpers'; + +describe('migrationHelpers.wrapMigration', () => { + test('throws if passed a migration without up method', async () => { + // + // ARRANGE + // + class TestMigration {} + + // + // ACT & ASSERT + // + expect(() => wrapMigration(TestMigration as never)).toThrow( + 'Migration "TestMigration" is missing the method `up`.', + ); + }); + + test('wraps up method', async () => { + // + // ARRANGE + // + class TestMigration implements IrreversibleMigration { + async up() {} + } + const originalUp = jest.fn(); + TestMigration.prototype.up = originalUp; + + // + // ACT + // + wrapMigration(TestMigration); + await new TestMigration().up(); + + // + // ASSERT + // + expect(TestMigration.prototype.up).not.toBe(originalUp); + expect(originalUp).toHaveBeenCalledTimes(1); + }); + + test('wraps down method', async () => { + // + // ARRANGE + // + class TestMigration implements ReversibleMigration { + async up() {} + + async down() {} + } + const originalDown = jest.fn(); + TestMigration.prototype.down = originalDown; + + // + // ACT + // + wrapMigration(TestMigration); + await new TestMigration().down(); + + // + // ASSERT + // + expect(TestMigration.prototype.down).not.toBe(originalDown); + expect(originalDown).toHaveBeenCalledTimes(1); + }); +});