fix(core): Don't revert irreversibble migrations (#9105)

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);
+		return await main(connectionOptions, this.logger, Connection);
-
-		this.connection = new Connection(connectionOptions);
-		await this.connection.initialize();
-		if (dbType === 'postgresdb') await setSchema(this.connection);
-		await this.connection.undoLastMigration();
-		await this.connection.destroy();
 	}
 
 	async catch(error: Error) {

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, {
+	if (up) {
-		async up(this: BaseMigration, queryRunner: QueryRunner) {
+		Object.assign(migration.prototype, {
-			logMigrationStart(migration.name);
+			async up(this: BaseMigration, queryRunner: QueryRunner) {
-			const context = createContext(queryRunner, migration);
+				logMigrationStart(migration.name);
-			if (this.transaction === false) {
+				const context = createContext(queryRunner, migration);
-				await runDisablingForeignKeys(this, context, up);
+				if (this.transaction === false) {
-			} else {
+					await runDisablingForeignKeys(this, context, up);
-				await up.call(this, context);
+				} else {
-			}
+					await up.call(this, context);
-			logMigrationEnd(migration.name);
+				}
-		},
+				logMigrationEnd(migration.name);
-		async down(this: BaseMigration, queryRunner: QueryRunner) {
+			},
-			if (down) {
+		});
+	} 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);
 				}
-			}
+			},
-		},
+		});
-	});
+	}
 };

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<DataSource>({ 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<DataSource>();
+	// @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<DataSource>({ 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<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();
+});