feat(core): Print the name of the migration that cannot be reverted when using n8n db:revert (#9473)

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);
+	if (lastExecutedMigration === undefined) {
-
+		logger.error(
-	const connection = new DataSource(connectionOptions);
+			"Cancelled command. The database was never migrated. Are you sure you're connected to the right database?.",
-	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) {
+	const lastMigrationInstance = connection.migrations.find((m) => {
-		logger.error('The last migration was irreversible and cannot be reverted.');
+		// 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 (!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) {

packages/cli/src/databases/utils/migrationHelpers.ts

@@ -190,9 +190,7 @@ export const wrapMigration = (migration: Migration) => {
 			},
 		});
 	} else {
-		throw new ApplicationError(
+		throw new ApplicationError(`Migration "${migration.name}" is missing the method \`up\`.`);
-			'At least on migration is missing the method `up`. Make sure all migrations are valid.',
-		);
 	}
 	if (down) {
 		Object.assign(migration.prototype, {

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();
+	const migrations: Migration[] = [];
-	// @ts-expect-error property is readonly
+	const dataSource = mock<DataSource>({ migrations });
-	connectionOptions.migrations = [];
+	const migrationExecutor = mock<MigrationExecutor>();
-	const dataSource = mock<DataSource>({ migrations: [] });
+	migrationExecutor.getExecutedMigrations.mockResolvedValue([]);
 
 	//
 	// ACT
 	//
-	await main(connectionOptions, logger, function () {
+	await main(logger, dataSource, migrationExecutor);
-		return dataSource;
-	} as never);
 
 	//
 	// 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,33 +42,96 @@ 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 migrationsInCode = [new TestMigration()];
-	const migrations = [TestMigration];
+	const migrationsInDb: Migration[] = [{ id: 1, timestamp: Date.now(), name: 'TestMigration' }];
-	// @ts-expect-error property is readonly
+	const dataSource = mock<DataSource>({ migrations: migrationsInCode });
-	connectionOptions.migrations = migrations;
+
-	const dataSource = mock<DataSource>();
+	const migrationExecutor = mock<MigrationExecutor>();
-	// @ts-expect-error property is readonly, and I can't pass them the `mock`
+	migrationExecutor.getExecutedMigrations.mockResolvedValue(migrationsInDb);
-	// 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 () {
+	await main(logger, dataSource, migrationExecutor);
-		return dataSource;
+
-	} as never);
+	//
+	// ASSERT
+	//
+	expect(logger.error).toHaveBeenCalledTimes(1);
+	expect(logger.error).toBeCalledWith('Cancelled command. The last migration was irreversible.');
+	expect(dataSource.undoLastMigration).not.toHaveBeenCalled();
+	expect(dataSource.destroy).not.toHaveBeenCalled();
+});
+
+test('print migration name instead of class name in error message if the migration has a name', async () => {
+	//
+	// ARRANGE
+	//
+	class TestMigration implements IrreversibleMigration {
+		name = 'Migration Name';
+
+		async up() {}
+
+		down = undefined;
+	}
+
+	const migrationsInCode = [new TestMigration()];
+	const migrationsInDb: Migration[] = [{ id: 1, timestamp: Date.now(), name: 'Migration Name' }];
+	const dataSource = mock<DataSource>({ migrations: migrationsInCode });
+
+	const migrationExecutor = mock<MigrationExecutor>();
+	migrationExecutor.getExecutedMigrations.mockResolvedValue(migrationsInDb);
+
+	//
+	// ACT
+	//
+	await main(logger, dataSource, migrationExecutor);
 
 	//
 	// ASSERT
 	//
 	expect(logger.error).toHaveBeenCalledTimes(1);
 	expect(logger.error).toHaveBeenCalledWith(
-		'The last migration was irreversible and cannot be reverted.',
+		'Cancelled command. The last migration "Migration Name" was irreversible.',
+	);
+	expect(dataSource.undoLastMigration).not.toHaveBeenCalled();
+	expect(dataSource.destroy).not.toHaveBeenCalled();
+});
+
+test("don't revert the last migration if we cannot find the migration in the code", async () => {
+	//
+	// ARRANGE
+	//
+
+	const migrationsInDb: Migration[] = [{ id: 1, timestamp: Date.now(), name: 'TestMigration' }];
+	const dataSource = mock<DataSource>({ migrations: [] });
+
+	const migrationExecutor = mock<MigrationExecutor>();
+	migrationExecutor.getExecutedMigrations.mockResolvedValue(migrationsInDb);
+
+	//
+	// ACT
+	//
+	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();
@@ -79,22 +142,25 @@ test('revert the last migration if it has a down migration', async () => {
 	// ARRANGE
 	//
 	class TestMigration implements ReversibleMigration {
+		name = 'ReversibleMigration';
+
 		async up() {}
 
 		async down() {}
 	}
 
-	const connectionOptions = DbConfig.getConnectionOptions();
+	const migrationsInDb: Migration[] = [
-	// @ts-expect-error property is readonly
+		{ id: 1, timestamp: Date.now(), name: 'ReversibleMigration' },
-	connectionOptions.migrations = [TestMigration];
+	];
 	const dataSource = mock<DataSource>({ migrations: [new TestMigration()] });
 
+	const migrationExecutor = mock<MigrationExecutor>();
+	migrationExecutor.getExecutedMigrations.mockResolvedValue(migrationsInDb);
+
 	//
 	// ACT
 	//
-	await main(connectionOptions, logger, function () {
+	await main(logger, dataSource, migrationExecutor);
-		return dataSource;
-	} as never);
 
 	//
 	// ASSERT
@@ -103,32 +169,3 @@ 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<DataSource>({ 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();
-});

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);
+	});
+});