-
Notifications
You must be signed in to change notification settings - Fork 7.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(core): Disallow orphan executions #7069
Conversation
Great PR! Please pay attention to the following items before merging: Files matching
Files matching
Files matching
Make sure to check off this list before asking for review. |
packages/cli/src/databases/migrations/sqlite/1693554410387-DisallowOrphanExecutions.ts
Outdated
Show resolved
Hide resolved
packages/cli/src/databases/migrations/sqlite/1693554410387-DisallowOrphanExecutions.ts
Outdated
Show resolved
Hide resolved
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #7069 +/- ##
=======================================
Coverage 32.11% 32.11%
=======================================
Files 3188 3188
Lines 195900 195924 +24
Branches 21364 21359 -5
=======================================
+ Hits 62915 62924 +9
- Misses 131953 131968 +15
Partials 1032 1032
☔ View full report in Codecov by Sentry. |
This reverts commit a377768.
* Ensure all executions point to a workflow. | ||
*/ | ||
async up({ queryRunner, escape }: MigrationContext) { | ||
const executionEntity = escape.tableName('execution_entity'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not combine the 3 migrations into a single one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do you mean? The syntax to modify a column differs between MySQL and Postgres, and sqlite requires recreating the table, and it looks like the DSL does not support this yet.
tableName: 'workflow_entity', | ||
columnName: 'id', | ||
onDelete: 'CASCADE', | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will require recreating the indexes as well I think
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please see 672d2d7
If we recreate the indices, it all looks good. See before and after. The only difference is the intended non-null If we recreate the column like below, typeorm creates a new table and fails when populating it. Note that the table name async up({
escape,
schemaBuilder: { column, addColumns, dropColumns },
runQuery,
}: MigrationContext) {
const executionEntity = escape.tableName('execution_entity');
const workflowId = escape.columnName('workflowId');
await runQuery(`DELETE FROM ${executionEntity} WHERE ${workflowId} IS NULL;`);
await addColumns('execution_entity', [column('temp_column').varchar(36).notNull]);
await runQuery(`UPDATE ${executionEntity} SET "temp_column" = ${workflowId};`);
await dropColumns('execution_entity', ['workflowId']);
await runQuery(`ALTER TABLE ${executionEntity} RENAME COLUMN "temp_column" TO ${workflowId};`);
}
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see comment
} | ||
} | ||
|
||
export class DropNotNull extends TableOperation { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tiny comment: Drop in the name suggests that the column is actually gone after this, I'd probably use RemoveNotNullAttribute or something clearer here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll leave this one to @netroy. I personally find it clear.
2 flaky tests on run #2082 ↗︎
Details:
24-ndv-paired-item.cy.ts • 1 flaky test
27-two-factor-authentication.cy.ts • 1 flaky test
This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. |
✅ All Cypress E2E specs passed |
✅ All Cypress E2E specs passed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's merge after the DB Tests pass.
✅ All Cypress E2E specs passed |
# [1.6.0](https://github.com/n8n-io/n8n/compare/n8n@1.5.1...n8n@1.6.0) (2023-09-06) ### Bug Fixes * **core:** Add support for in-transit encryption (TLS) on Redis connections ([#7047](#7047)) ([a910757](a910757)) * **core:** Disallow orphan executions ([#7069](#7069)) ([8a28e98](8a28e98)) * **core:** Split event bus controller into community and ee ([#7107](#7107)) ([011ee2e](011ee2e)) * **editor:** Standardize save text ([#7093](#7093)) ([58b3492](58b3492)) * Ensure all new executions are saved ([#7061](#7061)) ([b8e06d2](b8e06d2)) * Load remote resources even if expressions in non requried parameters resolve ([#6987](#6987)) ([8a8d4e8](8a8d4e8)) * **Postgres Node:** Connection pool of the database object has been destroyed ([#7074](#7074)) ([9dd5f0e](9dd5f0e)) * **Postgres Node:** Tunnel doesn't always close ([#7087](#7087)) ([58e55ba](58e55ba)) ### Features * **core:** Add list query middleware to credentials ([#7041](#7041)) ([fd78021](fd78021)) * **core:** Add support for floating licenses ([#7090](#7090)) ([e26553f](e26553f)) * **core:** Migration for soft deletions for executions ([#7088](#7088)) ([413e0bc](413e0bc)) * **HTTP Request Node:** Determine binary file name from content-disposition headers ([#7032](#7032)) ([273d091](273d091)) * **TheHive Node:** Overhaul ([#6457](#6457)) ([73e782e](73e782e)) Co-authored-by: netroy <netroy@users.noreply.github.com>
Got released with |
Until #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.Story: https://linear.app/n8n/issue/PAY-766/disallow-orphan-executions