Skip to content

Commit

Permalink
feat(core): Print the name of the migration that cannot be reverted w…
Browse files Browse the repository at this point in the history 
…hen using `n8n db:revert` (n8n-io#9473)
  • Loading branch information
@despairblue
despairblue authored May 23, 2024
1 parent 9367907 commit 3b93aae
Show file tree
Hide file tree
Showing 4 changed files with 199 additions and 71 deletions.
59 changes: 43 additions & 16 deletions packages/cli/src/commands/db/revert.ts
View file
Original file line number Diff line number Diff line change
@@ -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';
Expand All @@ -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;
}

Expand Down Expand Up @@ -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) {
Expand Down
4 changes: 1 addition & 3 deletions packages/cli/src/databases/utils/migrationHelpers.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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, {
Expand Down
141 changes: 89 additions & 52 deletions packages/cli/test/unit/commands/db/revert.test.ts
View file
Original file line number Diff line number Diff line change
@@ -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);
Expand All @@ -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<DataSource>({ migrations: [] });
const migrations: Migration[] = [];
const dataSource = mock<DataSource>({ migrations });
const migrationExecutor = mock<MigrationExecutor>();
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();
});
Expand All @@ -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<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());
const migrationsInCode = [new TestMigration()];
const migrationsInDb: Migration[] = [{ id: 1, timestamp: Date.now(), name: 'TestMigration' }];
const dataSource = mock<DataSource>({ migrations: migrationsInCode });

const migrationExecutor = mock<MigrationExecutor>();
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<DataSource>({ migrations: [new TestMigration()] });
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(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<DataSource>({ migrations: [new TestMigration()] });
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 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<DataSource>({ migrations: [new TestMigration()] });

const migrationExecutor = mock<MigrationExecutor>();
migrationExecutor.getExecutedMigrations.mockResolvedValue(migrationsInDb);

//
// ACT
//
await main(logger, dataSource, migrationExecutor);

//
// ASSERT
//
expect(logger.error).not.toHaveBeenCalled();
expect(dataSource.undoLastMigration).toHaveBeenCalled();
expect(dataSource.destroy).toHaveBeenCalled();
});
66 changes: 66 additions & 0 deletions packages/cli/test/unit/databases/utils/migrationHelpers.test.ts
View file
Original file line number Diff line number Diff line change
@@ -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);
});
});

0 comments on commit 3b93aae

Please sign in to comment.