Skip to content
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): Add missing primary key to execution annotation tags table #11168

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

burivuhster
Copy link
Contributor

@burivuhster burivuhster commented Oct 8, 2024

Summary

This PR adds missing composite primary index to the table execution_annotation_tags.
It uses typeorm method for MySQL and PostgreSQL, but since sqlite does not support adding primary keys to existing tables, it creates a new table and copy existing data.

Related Linear tickets, Github issues, and Community forum posts

Review / Merge checklist

  • PR title and summary are descriptive. (conventions)
  • Docs updated or follow-up ticket created.
  • Tests included.
  • PR Labeled with release/backport (if the PR is an urgent fix that needs to be backported)

@n8n-assistant n8n-assistant bot added core Enhancement outside /nodes-base and /editor-ui n8n team Authored by the n8n team labels Oct 8, 2024
@burivuhster burivuhster marked this pull request as ready for review October 8, 2024 16:00
@burivuhster burivuhster requested a review from a team as a code owner October 8, 2024 16:00
@csuermann
Copy link
Contributor

csuermann commented Oct 8, 2024

Since this PR adds another migration on top of the original migration that introduced the table (1724753530828-CreateExecutionAnnotationTables), will this PR actually fix the migration blocker for users that don't have access to the sql_require_primary_key setting in MySQL?

@burivuhster burivuhster changed the title fix(core): Add missing primary key to execution annotation tags table (no-changelog) fix(core): Add missing primary key to execution annotation tags table Oct 9, 2024
@burivuhster burivuhster force-pushed the ai-384-community-issue-unable-to-create-or-change-a-table-without-a branch from 97387ae to 30c8dcf Compare October 9, 2024 09:11
@burivuhster
Copy link
Contributor Author

@csuermann you are right, adding another migration will not help those who can not disable sql_require_primary_key temporarily. Here is the proposed solution:

  1. We edit an initial migration to add PK
  2. We also add new migration, which conditionally adds PK if the table does not have it

For the majority of users, who already have the initial migration applied, the new migration will add missing PK.
For those who will install n8n after this PR merged: the table will be created with PK in initial migration, the new migration will have no effect.

What do you think?

@csuermann
Copy link
Contributor

That should solve it! Thanks!

tomi
tomi previously approved these changes Oct 10, 2024
Copy link
Contributor

@tomi tomi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good from my side. I assume you tested both these scenarios:

  1. migrating to an empty DB
  2. migrating to a DB which has all except the new migration

Comment on lines 40 to 45
await dropIndex(`${annotationTagMappingsTableName}_tmp`, ['tagId'], {
customIndexName: 'IDX_a3697779b366e131b2bbdae297',
});
await dropIndex(`${annotationTagMappingsTableName}_tmp`, ['annotationId'], {
customIndexName: 'IDX_c1519757391996eb06064f0e7c',
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to drop these if the tmp table gets dropped at the end? Does that also drop these indexes?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, otherwise it fails on creating new table, as indexes with these names are already exist.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could give the new indexes custom names to prevent this as well.
I had to do this here as well:

// INFO: The PK names that TypeORM creates are predictable and thus it
// will create a PK name which already exists in the current
// execution_metadata table. That's why we have to randomize the PK name
// here.
column('id').int.notNull.primaryWithName(nanoid()).autoGenerate,

@burivuhster burivuhster force-pushed the ai-384-community-issue-unable-to-create-or-change-a-table-without-a branch from b5ac53d to ea49d5e Compare October 11, 2024 15:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Enhancement outside /nodes-base and /editor-ui n8n team Authored by the n8n team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants