From 8a28e98ec811952163c58feaad608ec14ffc9243 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Iv=C3=A1n=20Ovejero?= Date: Mon, 4 Sep 2023 16:57:10 +0200 Subject: [PATCH] fix(core): Disallow orphan executions (#7069) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Until https://github.com/n8n-io/n8n/pull/7061 we had an edge case where a manual unsaved workflow when run creates an orphan execution, i.e. a saved execution not pointing to any workflow. This execution is only ever visible to the instance owner (even if triggered by a member), and is wrongly stored as unfinished and crashed. This PR enforces that the DB disallows any such executions from making it into the DB. This is needed also for the S3 client, which will include the `workflowId` in the path-like filename. --------- Co-authored-by: कारतोफ्फेलस्क्रिप्ट™ --- packages/cli/src/databases/dsl/Indices.ts | 6 +-- packages/cli/src/databases/dsl/Table.ts | 48 ++++++++++++++++++- packages/cli/src/databases/dsl/index.ts | 11 +++-- .../1693554410387-DisallowOrphanExecutions.ts | 22 +++++++++ .../src/databases/migrations/mysqldb/index.ts | 2 + .../databases/migrations/postgresdb/index.ts | 2 + .../src/databases/migrations/sqlite/index.ts | 2 + 7 files changed, 85 insertions(+), 8 deletions(-) create mode 100644 packages/cli/src/databases/migrations/common/1693554410387-DisallowOrphanExecutions.ts diff --git a/packages/cli/src/databases/dsl/Indices.ts b/packages/cli/src/databases/dsl/Indices.ts index d6b868e2aa71b..d07c6db558e1c 100644 --- a/packages/cli/src/databases/dsl/Indices.ts +++ b/packages/cli/src/databases/dsl/Indices.ts @@ -13,9 +13,9 @@ abstract class IndexOperation extends LazyPromise { } constructor( - protected tablePrefix: string, protected tableName: string, protected columnNames: string[], + protected tablePrefix: string, queryRunner: QueryRunner, protected customIndexName?: string, ) { @@ -27,14 +27,14 @@ abstract class IndexOperation extends LazyPromise { export class CreateIndex extends IndexOperation { constructor( - tablePrefix: string, tableName: string, columnNames: string[], protected isUnique: boolean, + tablePrefix: string, queryRunner: QueryRunner, customIndexName?: string, ) { - super(tablePrefix, tableName, columnNames, queryRunner, customIndexName); + super(tableName, columnNames, tablePrefix, queryRunner, customIndexName); } async execute(queryRunner: QueryRunner) { diff --git a/packages/cli/src/databases/dsl/Table.ts b/packages/cli/src/databases/dsl/Table.ts index b2735cadc9a61..d58defeb61817 100644 --- a/packages/cli/src/databases/dsl/Table.ts +++ b/packages/cli/src/databases/dsl/Table.ts @@ -11,8 +11,8 @@ abstract class TableOperation extends LazyPromise { protected prefix: string, queryRunner: QueryRunner, ) { - super((resolve) => { - void this.execute(queryRunner).then(resolve); + super((resolve, reject) => { + void this.execute(queryRunner).then(resolve).catch(reject); }); } } @@ -116,3 +116,47 @@ export class DropColumns extends TableOperation { return queryRunner.dropColumns(`${prefix}${tableName}`, columnNames); } } + +class ModifyNotNull extends TableOperation { + constructor( + tableName: string, + protected columnName: string, + protected isNullable: boolean, + prefix: string, + queryRunner: QueryRunner, + ) { + super(tableName, prefix, queryRunner); + } + + async execute(queryRunner: QueryRunner) { + const { tableName, prefix, columnName, isNullable } = this; + const table = await queryRunner.getTable(`${prefix}${tableName}`); + if (!table) throw new Error(`No table found with the name ${tableName}`); + const oldColumn = table.findColumnByName(columnName)!; + const newColumn = oldColumn.clone(); + newColumn.isNullable = isNullable; + return queryRunner.changeColumn(table, oldColumn, newColumn); + } +} + +export class AddNotNull extends ModifyNotNull { + constructor( + tableName: string, + protected columnName: string, + prefix: string, + queryRunner: QueryRunner, + ) { + super(tableName, columnName, false, prefix, queryRunner); + } +} + +export class DropNotNull extends ModifyNotNull { + constructor( + tableName: string, + protected columnName: string, + prefix: string, + queryRunner: QueryRunner, + ) { + super(tableName, columnName, true, prefix, queryRunner); + } +} diff --git a/packages/cli/src/databases/dsl/index.ts b/packages/cli/src/databases/dsl/index.ts index bd5a06a5910ee..6eb6df8bc97b5 100644 --- a/packages/cli/src/databases/dsl/index.ts +++ b/packages/cli/src/databases/dsl/index.ts @@ -1,6 +1,6 @@ import type { QueryRunner } from 'typeorm'; import { Column } from './Column'; -import { AddColumns, CreateTable, DropColumns, DropTable } from './Table'; +import { AddColumns, AddNotNull, CreateTable, DropColumns, DropNotNull, DropTable } from './Table'; import { CreateIndex, DropIndex } from './Indices'; export const createSchemaBuilder = (tablePrefix: string, queryRunner: QueryRunner) => ({ @@ -21,10 +21,15 @@ export const createSchemaBuilder = (tablePrefix: string, queryRunner: QueryRunne columnNames: string[], isUnique = false, customIndexName?: string, - ) => new CreateIndex(tablePrefix, tableName, columnNames, isUnique, queryRunner, customIndexName), + ) => new CreateIndex(tableName, columnNames, isUnique, tablePrefix, queryRunner, customIndexName), dropIndex: (tableName: string, columnNames: string[], customIndexName?: string) => - new DropIndex(tablePrefix, tableName, columnNames, queryRunner, customIndexName), + new DropIndex(tableName, columnNames, tablePrefix, queryRunner, customIndexName), + + addNotNull: (tableName: string, columnName: string) => + new AddNotNull(tableName, columnName, tablePrefix, queryRunner), + dropNotNull: (tableName: string, columnName: string) => + new DropNotNull(tableName, columnName, tablePrefix, queryRunner), /* eslint-enable */ }); diff --git a/packages/cli/src/databases/migrations/common/1693554410387-DisallowOrphanExecutions.ts b/packages/cli/src/databases/migrations/common/1693554410387-DisallowOrphanExecutions.ts new file mode 100644 index 0000000000000..c0f74ec0bb2f2 --- /dev/null +++ b/packages/cli/src/databases/migrations/common/1693554410387-DisallowOrphanExecutions.ts @@ -0,0 +1,22 @@ +import type { MigrationContext, ReversibleMigration } from '@db/types'; + +export class DisallowOrphanExecutions1693554410387 implements ReversibleMigration { + /** + * Ensure all executions point to a workflow. + */ + async up({ escape, schemaBuilder: { addNotNull }, runQuery }: MigrationContext) { + const executionEntity = escape.tableName('execution_entity'); + const workflowId = escape.columnName('workflowId'); + + await runQuery(`DELETE FROM ${executionEntity} WHERE ${workflowId} IS NULL;`); + + await addNotNull('execution_entity', 'workflowId'); + } + + /** + * Reversal excludes restoring deleted rows. + */ + async down({ schemaBuilder: { dropNotNull } }: MigrationContext) { + await dropNotNull('execution_entity', 'workflowId'); + } +} diff --git a/packages/cli/src/databases/migrations/mysqldb/index.ts b/packages/cli/src/databases/migrations/mysqldb/index.ts index b7aa3bd0e1e9d..b6255dc5f1b59 100644 --- a/packages/cli/src/databases/migrations/mysqldb/index.ts +++ b/packages/cli/src/databases/migrations/mysqldb/index.ts @@ -46,6 +46,7 @@ import { RemoveResetPasswordColumns1690000000030 } from '../common/1690000000030 import { CreateWorkflowNameIndex1691088862123 } from '../common/1691088862123-CreateWorkflowNameIndex'; import { AddMfaColumns1690000000030 } from './../common/1690000000040-AddMfaColumns'; import { CreateWorkflowHistoryTable1692967111175 } from '../common/1692967111175-CreateWorkflowHistoryTable'; +import { DisallowOrphanExecutions1693554410387 } from '../common/1693554410387-DisallowOrphanExecutions'; import { ExecutionSoftDelete1693491613982 } from '../common/1693491613982-ExecutionSoftDelete'; export const mysqlMigrations: Migration[] = [ @@ -96,5 +97,6 @@ export const mysqlMigrations: Migration[] = [ CreateWorkflowNameIndex1691088862123, AddMfaColumns1690000000030, CreateWorkflowHistoryTable1692967111175, + DisallowOrphanExecutions1693554410387, ExecutionSoftDelete1693491613982, ]; diff --git a/packages/cli/src/databases/migrations/postgresdb/index.ts b/packages/cli/src/databases/migrations/postgresdb/index.ts index 673cd03fe89f6..aa4906c6b2d6b 100644 --- a/packages/cli/src/databases/migrations/postgresdb/index.ts +++ b/packages/cli/src/databases/migrations/postgresdb/index.ts @@ -44,6 +44,7 @@ import { AddMissingPrimaryKeyOnExecutionData1690787606731 } from './169078760673 import { CreateWorkflowNameIndex1691088862123 } from '../common/1691088862123-CreateWorkflowNameIndex'; import { AddMfaColumns1690000000030 } from './../common/1690000000040-AddMfaColumns'; import { CreateWorkflowHistoryTable1692967111175 } from '../common/1692967111175-CreateWorkflowHistoryTable'; +import { DisallowOrphanExecutions1693554410387 } from '../common/1693554410387-DisallowOrphanExecutions'; import { ExecutionSoftDelete1693491613982 } from '../common/1693491613982-ExecutionSoftDelete'; export const postgresMigrations: Migration[] = [ @@ -92,5 +93,6 @@ export const postgresMigrations: Migration[] = [ CreateWorkflowNameIndex1691088862123, AddMfaColumns1690000000030, CreateWorkflowHistoryTable1692967111175, + DisallowOrphanExecutions1693554410387, ExecutionSoftDelete1693491613982, ]; diff --git a/packages/cli/src/databases/migrations/sqlite/index.ts b/packages/cli/src/databases/migrations/sqlite/index.ts index 325ffbe6bda79..d4c4e736b9362 100644 --- a/packages/cli/src/databases/migrations/sqlite/index.ts +++ b/packages/cli/src/databases/migrations/sqlite/index.ts @@ -43,6 +43,7 @@ import { RemoveResetPasswordColumns1690000000030 } from './1690000000030-RemoveR import { CreateWorkflowNameIndex1691088862123 } from '../common/1691088862123-CreateWorkflowNameIndex'; import { AddMfaColumns1690000000030 } from './1690000000040-AddMfaColumns'; import { CreateWorkflowHistoryTable1692967111175 } from '../common/1692967111175-CreateWorkflowHistoryTable'; +import { DisallowOrphanExecutions1693554410387 } from '../common/1693554410387-DisallowOrphanExecutions'; import { ExecutionSoftDelete1693491613982 } from './1693491613982-ExecutionSoftDelete'; const sqliteMigrations: Migration[] = [ @@ -90,6 +91,7 @@ const sqliteMigrations: Migration[] = [ CreateWorkflowNameIndex1691088862123, AddMfaColumns1690000000030, CreateWorkflowHistoryTable1692967111175, + DisallowOrphanExecutions1693554410387, ExecutionSoftDelete1693491613982, ];