From 9def7a729b52cd6b4698c47e190e9e2bd7894da5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E0=A4=95=E0=A4=BE=E0=A4=B0=E0=A4=A4=E0=A5=8B=E0=A4=AB?= =?UTF-8?q?=E0=A5=8D=E0=A4=AB=E0=A5=87=E0=A4=B2=E0=A4=B8=E0=A5=8D=E0=A4=95?= =?UTF-8?q?=E0=A5=8D=E0=A4=B0=E0=A4=BF=E0=A4=AA=E0=A5=8D=E0=A4=9F=E2=84=A2?= Date: Wed, 5 Jul 2023 11:26:49 +0200 Subject: [PATCH] fix(core): Remove typeorm patches, but still enforce transactions on every migration (#6594) * revert(core): Remove typeorm patches, but still enforce transactions on every migration This reverts #6519 * always re-enable foreign keys, and explicitly rollback transaction --- package.json | 3 +- .../sqlite/1652367743993-AddUserSettings.ts | 2 + .../sqlite/1652905585850-AddAPIKeyColumn.ts | 2 + ...268682475-DeleteExecutionsWithWorkflows.ts | 2 + ...690000000002-MigrateIntegerKeysToString.ts | 13 ++-- packages/cli/src/databases/types.ts | 12 ++-- .../src/databases/utils/migrationHelpers.ts | 62 +++++++++++++------ patches/typeorm@0.3.12.patch | 31 ---------- pnpm-lock.yaml | 8 +-- 9 files changed, 69 insertions(+), 66 deletions(-) delete mode 100644 patches/typeorm@0.3.12.patch diff --git a/package.json b/package.json index 5d6c77cf6c..4435306ea8 100644 --- a/package.json +++ b/package.json @@ -95,8 +95,7 @@ "element-ui@2.15.12": "patches/element-ui@2.15.12.patch", "typedi@0.10.0": "patches/typedi@0.10.0.patch", "@sentry/cli@2.17.0": "patches/@sentry__cli@2.17.0.patch", - "pkce-challenge@3.0.0": "patches/pkce-challenge@3.0.0.patch", - "typeorm@0.3.12": "patches/typeorm@0.3.12.patch" + "pkce-challenge@3.0.0": "patches/pkce-challenge@3.0.0.patch" } } } diff --git a/packages/cli/src/databases/migrations/sqlite/1652367743993-AddUserSettings.ts b/packages/cli/src/databases/migrations/sqlite/1652367743993-AddUserSettings.ts index 82efcf0043..ee73395478 100644 --- a/packages/cli/src/databases/migrations/sqlite/1652367743993-AddUserSettings.ts +++ b/packages/cli/src/databases/migrations/sqlite/1652367743993-AddUserSettings.ts @@ -1,6 +1,8 @@ import type { MigrationContext, ReversibleMigration } from '@db/types'; export class AddUserSettings1652367743993 implements ReversibleMigration { + transaction = false as const; + async up({ queryRunner, tablePrefix }: MigrationContext) { await queryRunner.query( `CREATE TABLE "temporary_user" ("id" varchar PRIMARY KEY NOT NULL, "email" varchar(255), "firstName" varchar(32), "lastName" varchar(32), "password" varchar, "resetPasswordToken" varchar, "resetPasswordTokenExpiration" integer DEFAULT NULL, "personalizationAnswers" text, "createdAt" datetime(3) NOT NULL DEFAULT (STRFTIME('%Y-%m-%d %H:%M:%f', 'NOW')), "updatedAt" datetime(3) NOT NULL DEFAULT (STRFTIME('%Y-%m-%d %H:%M:%f', 'NOW')), "globalRoleId" integer NOT NULL, "settings" text, CONSTRAINT "FK_${tablePrefix}f0609be844f9200ff4365b1bb3d" FOREIGN KEY ("globalRoleId") REFERENCES "${tablePrefix}role" ("id") ON DELETE NO ACTION ON UPDATE NO ACTION)`, diff --git a/packages/cli/src/databases/migrations/sqlite/1652905585850-AddAPIKeyColumn.ts b/packages/cli/src/databases/migrations/sqlite/1652905585850-AddAPIKeyColumn.ts index fa319b9808..74440331bb 100644 --- a/packages/cli/src/databases/migrations/sqlite/1652905585850-AddAPIKeyColumn.ts +++ b/packages/cli/src/databases/migrations/sqlite/1652905585850-AddAPIKeyColumn.ts @@ -1,6 +1,8 @@ import type { MigrationContext, ReversibleMigration } from '@db/types'; export class AddAPIKeyColumn1652905585850 implements ReversibleMigration { + transaction = false as const; + async up({ queryRunner, tablePrefix }: MigrationContext) { await queryRunner.query( `CREATE TABLE "temporary_user" ("id" varchar PRIMARY KEY NOT NULL, "email" varchar(255), "firstName" varchar(32), "lastName" varchar(32), "password" varchar, "resetPasswordToken" varchar, "resetPasswordTokenExpiration" integer DEFAULT NULL, "personalizationAnswers" text, "createdAt" datetime(3) NOT NULL DEFAULT (STRFTIME('%Y-%m-%d %H:%M:%f', 'NOW')), "updatedAt" datetime(3) NOT NULL DEFAULT (STRFTIME('%Y-%m-%d %H:%M:%f', 'NOW')), "globalRoleId" integer NOT NULL, "settings" text, "apiKey" varchar, CONSTRAINT "FK_${tablePrefix}f0609be844f9200ff4365b1bb3d" FOREIGN KEY ("globalRoleId") REFERENCES "${tablePrefix}role" ("id") ON DELETE NO ACTION ON UPDATE NO ACTION)`, diff --git a/packages/cli/src/databases/migrations/sqlite/1673268682475-DeleteExecutionsWithWorkflows.ts b/packages/cli/src/databases/migrations/sqlite/1673268682475-DeleteExecutionsWithWorkflows.ts index 250c747ef4..cfd670bc62 100644 --- a/packages/cli/src/databases/migrations/sqlite/1673268682475-DeleteExecutionsWithWorkflows.ts +++ b/packages/cli/src/databases/migrations/sqlite/1673268682475-DeleteExecutionsWithWorkflows.ts @@ -1,6 +1,8 @@ import type { MigrationContext, ReversibleMigration } from '@db/types'; export class DeleteExecutionsWithWorkflows1673268682475 implements ReversibleMigration { + transaction = false as const; + async up({ queryRunner, tablePrefix }: MigrationContext) { const workflowIds = (await queryRunner.query(` SELECT id FROM "${tablePrefix}workflow_entity" diff --git a/packages/cli/src/databases/migrations/sqlite/1690000000002-MigrateIntegerKeysToString.ts b/packages/cli/src/databases/migrations/sqlite/1690000000002-MigrateIntegerKeysToString.ts index 50cd5c57b2..80e8dcaed6 100644 --- a/packages/cli/src/databases/migrations/sqlite/1690000000002-MigrateIntegerKeysToString.ts +++ b/packages/cli/src/databases/migrations/sqlite/1690000000002-MigrateIntegerKeysToString.ts @@ -1,18 +1,21 @@ import type { MigrationContext, IrreversibleMigration } from '@db/types'; export class MigrateIntegerKeysToString1690000000002 implements IrreversibleMigration { + transaction = false as const; + async up({ queryRunner, tablePrefix }: MigrationContext) { await queryRunner.query(` -CREATE TABLE "${tablePrefix}TMP_workflow_entity" ("id" varchar(36) PRIMARY KEY NOT NULL, "name" varchar(128) NOT NULL, "active" boolean NOT NULL, "nodes" text, "connections" text NOT NULL, "createdAt" datetime(3) NOT NULL DEFAULT (STRFTIME('%Y-%m-%d %H:%M:%f', 'NOW')), "updatedAt" datetime(3) NOT NULL DEFAULT (STRFTIME('%Y-%m-%d %H:%M:%f', 'NOW')), "settings" text, "staticData" text, "pinData" text, "versionId" varchar(36), "triggerCount" integer NOT NULL DEFAULT 0);`); + CREATE TABLE "${tablePrefix}TMP_workflow_entity" ("id" varchar(36) PRIMARY KEY NOT NULL, "name" varchar(128) NOT NULL, "active" boolean NOT NULL, "nodes" text, "connections" text NOT NULL, "createdAt" datetime(3) NOT NULL DEFAULT (STRFTIME('%Y-%m-%d %H:%M:%f', 'NOW')), "updatedAt" datetime(3) NOT NULL DEFAULT (STRFTIME('%Y-%m-%d %H:%M:%f', 'NOW')), "settings" text, "staticData" text, "pinData" text, "versionId" varchar(36), "triggerCount" integer NOT NULL DEFAULT 0);`); await queryRunner.query( `INSERT INTO "${tablePrefix}TMP_workflow_entity" (id, name, active, nodes, connections, createdAt, updatedAt, settings, staticData, pinData, triggerCount, versionId) SELECT id, name, active, nodes, connections, createdAt, updatedAt, settings, staticData, pinData, triggerCount, versionId FROM "${tablePrefix}workflow_entity";`, ); await queryRunner.query(`DROP TABLE "${tablePrefix}workflow_entity";`); - await queryRunner.query(`ALTER TABLE "${tablePrefix}TMP_workflow_entity" RENAME TO "${tablePrefix}workflow_entity"; -`); + await queryRunner.query( + `ALTER TABLE "${tablePrefix}TMP_workflow_entity" RENAME TO "${tablePrefix}workflow_entity"`, + ); await queryRunner.query(` -CREATE TABLE "${tablePrefix}TMP_tag_entity" ("id" varchar(36) PRIMARY KEY NOT NULL, "name" varchar(24) NOT NULL, "createdAt" datetime(3) NOT NULL DEFAULT (STRFTIME('%Y-%m-%d %H:%M:%f', 'NOW')), "updatedAt" datetime(3) NOT NULL DEFAULT (STRFTIME('%Y-%m-%d %H:%M:%f', 'NOW')));`); + CREATE TABLE "${tablePrefix}TMP_tag_entity" ("id" varchar(36) PRIMARY KEY NOT NULL, "name" varchar(24) NOT NULL, "createdAt" datetime(3) NOT NULL DEFAULT (STRFTIME('%Y-%m-%d %H:%M:%f', 'NOW')), "updatedAt" datetime(3) NOT NULL DEFAULT (STRFTIME('%Y-%m-%d %H:%M:%f', 'NOW')));`); await queryRunner.query( `INSERT INTO "${tablePrefix}TMP_tag_entity" SELECT * FROM "${tablePrefix}tag_entity";`, ); @@ -22,7 +25,7 @@ CREATE TABLE "${tablePrefix}TMP_tag_entity" ("id" varchar(36) PRIMARY KEY NOT NU ); await queryRunner.query(` -CREATE TABLE "${tablePrefix}TMP_workflows_tags" ("workflowId" varchar(36) NOT NULL, "tagId" integer NOT NULL, CONSTRAINT "FK_${tablePrefix}workflows_tags_workflow_entity" FOREIGN KEY ("workflowId") REFERENCES "${tablePrefix}workflow_entity" ("id") ON DELETE CASCADE ON UPDATE NO ACTION, CONSTRAINT "FK_${tablePrefix}workflows_tags_tag_entity" FOREIGN KEY ("tagId") REFERENCES "${tablePrefix}tag_entity" ("id") ON DELETE CASCADE ON UPDATE NO ACTION, PRIMARY KEY ("workflowId", "tagId"));`); + CREATE TABLE "${tablePrefix}TMP_workflows_tags" ("workflowId" varchar(36) NOT NULL, "tagId" integer NOT NULL, CONSTRAINT "FK_${tablePrefix}workflows_tags_workflow_entity" FOREIGN KEY ("workflowId") REFERENCES "${tablePrefix}workflow_entity" ("id") ON DELETE CASCADE ON UPDATE NO ACTION, CONSTRAINT "FK_${tablePrefix}workflows_tags_tag_entity" FOREIGN KEY ("tagId") REFERENCES "${tablePrefix}tag_entity" ("id") ON DELETE CASCADE ON UPDATE NO ACTION, PRIMARY KEY ("workflowId", "tagId"));`); await queryRunner.query( `INSERT INTO "${tablePrefix}TMP_workflows_tags" SELECT * FROM "${tablePrefix}workflows_tags";`, ); diff --git a/packages/cli/src/databases/types.ts b/packages/cli/src/databases/types.ts index 19d4813163..ada0268756 100644 --- a/packages/cli/src/databases/types.ts +++ b/packages/cli/src/databases/types.ts @@ -12,15 +12,19 @@ export interface MigrationContext { migrationName: string; } -type MigrationFn = (ctx: MigrationContext) => Promise; +export type MigrationFn = (ctx: MigrationContext) => Promise; -export interface ReversibleMigration { +export interface BaseMigration { up: MigrationFn; + down?: MigrationFn | never; + transaction?: false; +} + +export interface ReversibleMigration extends BaseMigration { down: MigrationFn; } -export interface IrreversibleMigration { - up: MigrationFn; +export interface IrreversibleMigration extends BaseMigration { down?: never; } diff --git a/packages/cli/src/databases/utils/migrationHelpers.ts b/packages/cli/src/databases/utils/migrationHelpers.ts index a1f8bf223f..5c1a91763e 100644 --- a/packages/cli/src/databases/utils/migrationHelpers.ts +++ b/packages/cli/src/databases/utils/migrationHelpers.ts @@ -1,11 +1,10 @@ -/* eslint-disable no-await-in-loop */ import { readFileSync, rmSync } from 'fs'; import { UserSettings } from 'n8n-core'; import type { QueryRunner } from 'typeorm/query-runner/QueryRunner'; import config from '@/config'; import { getLogger } from '@/Logger'; import { inTest } from '@/constants'; -import type { Migration, MigrationContext } from '@db/types'; +import type { BaseMigration, Migration, MigrationContext, MigrationFn } from '@db/types'; const logger = getLogger(); @@ -39,30 +38,47 @@ export function loadSurveyFromDisk(): string | null { } } -let logFinishTimeout: NodeJS.Timeout; +let runningMigrations = false; -export function logMigrationStart(migrationName: string, disableLogging = inTest): void { - if (disableLogging) return; +function logMigrationStart(migrationName: string): void { + if (inTest) return; - if (!logFinishTimeout) { + if (!runningMigrations) { logger.warn('Migrations in progress, please do NOT stop the process.'); + runningMigrations = true; } logger.debug(`Starting migration ${migrationName}`); - - clearTimeout(logFinishTimeout); } -export function logMigrationEnd(migrationName: string, disableLogging = inTest): void { - if (disableLogging) return; +function logMigrationEnd(migrationName: string): void { + if (inTest) return; logger.debug(`Finished migration ${migrationName}`); - - logFinishTimeout = setTimeout(() => { - logger.warn('Migrations finished.'); - }, 100); } +const runDisablingForeignKeys = async ( + migration: BaseMigration, + context: MigrationContext, + fn: MigrationFn, +) => { + const { dbType, queryRunner } = context; + if (dbType !== 'sqlite') throw new Error('Disabling transactions only available in sqlite'); + await queryRunner.query('PRAGMA foreign_keys=OFF'); + await queryRunner.startTransaction(); + try { + await fn.call(migration, context); + await queryRunner.commitTransaction(); + } catch (e) { + try { + await queryRunner.rollbackTransaction(); + } catch {} + throw e; + } finally { + await queryRunner.query('PRAGMA foreign_keys=ON'); + } +}; + export const wrapMigration = (migration: Migration) => { const dbType = config.getEnv('database.type'); const dbName = config.getEnv(`database.${dbType === 'mariadb' ? 'mysqldb' : dbType}.database`); @@ -78,13 +94,23 @@ export const wrapMigration = (migration: Migration) => { const { up, down } = migration.prototype; Object.assign(migration.prototype, { - async up(queryRunner: QueryRunner) { + async up(this: BaseMigration, queryRunner: QueryRunner) { logMigrationStart(migrationName); - await up.call(this, { queryRunner, ...context }); + if (!this.transaction) { + await runDisablingForeignKeys(this, { queryRunner, ...context }, up); + } else { + await up.call(this, { queryRunner, ...context }); + } logMigrationEnd(migrationName); }, - async down(queryRunner: QueryRunner) { - await down?.call(this, { queryRunner, ...context }); + async down(this: BaseMigration, queryRunner: QueryRunner) { + if (down) { + if (!this.transaction) { + await runDisablingForeignKeys(this, { queryRunner, ...context }, up); + } else { + await down.call(this, { queryRunner, ...context }); + } + } }, }); }; diff --git a/patches/typeorm@0.3.12.patch b/patches/typeorm@0.3.12.patch deleted file mode 100644 index 8b09c80421..0000000000 --- a/patches/typeorm@0.3.12.patch +++ /dev/null @@ -1,31 +0,0 @@ -diff --git a/migration/MigrationExecutor.js b/migration/MigrationExecutor.js -index 5d37b9cf9ca2505242f05160f05ff683e00c1e5d..4a768819f86b8f176bd3b826a649afe54ab39598 100644 ---- a/migration/MigrationExecutor.js -+++ b/migration/MigrationExecutor.js -@@ -216,15 +216,17 @@ class MigrationExecutor { - // nothing else needs to be done, continue to next migration - continue; - } -+ await queryRunner.beforeMigration(); - if (migration.transaction && !queryRunner.isTransactionActive) { - await queryRunner.startTransaction(); - transactionStartedByUs = true; - } - await migration - .instance.up(queryRunner) -- .catch((error) => { -+ .catch(async (error) => { - // informative log about migration failure - this.connection.logger.logMigration(`Migration "${migration.name}" failed, error: ${error === null || error === void 0 ? void 0 : error.message}`); -+ await queryRunner.afterMigration(queryRunner); - throw error; - }) - .then(async () => { -@@ -233,6 +235,7 @@ class MigrationExecutor { - // commit transaction if we started it - if (migration.transaction && transactionStartedByUs) - await queryRunner.commitTransaction(); -+ await queryRunner.afterMigration(queryRunner); - }) - .then(() => { - // informative log about migration success \ No newline at end of file diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index edd69c2452..cb4a966acd 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -38,9 +38,6 @@ patchedDependencies: typedi@0.10.0: hash: 62r6bc2crgimafeyruodhqlgo4 path: patches/typedi@0.10.0.patch - typeorm@0.3.12: - hash: yav7zi22hnry26k2lwg6jcumde - path: patches/typeorm@0.3.12.patch importers: @@ -442,7 +439,7 @@ importers: version: 0.10.0(patch_hash=62r6bc2crgimafeyruodhqlgo4) typeorm: specifier: ^0.3.12 - version: 0.3.12(patch_hash=yav7zi22hnry26k2lwg6jcumde)(ioredis@5.2.4)(mysql2@2.3.3)(pg@8.8.0)(sqlite3@5.1.6) + version: 0.3.12(ioredis@5.2.4)(mysql2@2.3.3)(pg@8.8.0)(sqlite3@5.1.6) uuid: specifier: ^8.3.2 version: 8.3.2 @@ -21388,7 +21385,7 @@ packages: dev: false patched: true - /typeorm@0.3.12(patch_hash=yav7zi22hnry26k2lwg6jcumde)(ioredis@5.2.4)(mysql2@2.3.3)(pg@8.8.0)(sqlite3@5.1.6): + /typeorm@0.3.12(ioredis@5.2.4)(mysql2@2.3.3)(pg@8.8.0)(sqlite3@5.1.6): resolution: {integrity: sha512-sYSxBmCf1nJLLTcYtwqZ+lQIRtLPyUoO93rHTOKk9vJCyT4UfRtU7oRsJvfvKP3nnZTD1hzz2SEy2zwPEN6OyA==} engines: {node: '>= 12.9.0'} hasBin: true @@ -21470,7 +21467,6 @@ packages: transitivePeerDependencies: - supports-color dev: false - patched: true /typescript@5.1.3: resolution: {integrity: sha512-XH627E9vkeqhlZFQuL+UsyAXEnibT0kWR2FWONlr4sTjvxyJYnyefgrkyECLzM5NenmKzRAy2rR/OlYLA1HkZw==}