Skip to content

Commit

Permalink
fix(core): Don't revert irreversibble migrations (#9105)
Browse files Browse the repository at this point in the history
  • Loading branch information
despairblue authored and netroy committed Apr 11, 2024
1 parent 58f9e35 commit a1870b3
Show file tree
Hide file tree
Showing 3 changed files with 191 additions and 24 deletions.
41 changes: 33 additions & 8 deletions packages/cli/src/commands/db/revert.ts
Expand Up @@ -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';

Expand All @@ -27,7 +59,6 @@ export class DbRevertMigrationCommand extends Command {
}

async run() {
const dbType = config.getEnv('database.type');
const connectionOptions: ConnectionOptions = {
...getConnectionOptions(),
subscribers: [],
Expand All @@ -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) {
Expand Down
40 changes: 24 additions & 16 deletions packages/cli/src/databases/utils/migrationHelpers.ts
Expand Up @@ -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);
}
}
},
});
},
});
}
};
134 changes: 134 additions & 0 deletions 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();
});

0 comments on commit a1870b3

Please sign in to comment.