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): Fix DB migrations for MySQL #13261

Merged
merged 10 commits into from
Feb 18, 2025

Conversation

burivuhster
Copy link
Contributor

@burivuhster burivuhster commented Feb 14, 2025

Summary

This PR address couple of DB migration issues, preventing n8n to start when using MySQL as a database

Column check constraint

Migration AddStatsColumnsToTestRun1736172058779 introduced some column check constraints, but referencing other columns in check condition of a column check constraint is not supported by MySQL (checked on 8.0.29, 8.0.41, 8.4.4).

Starting migration AddStatsColumnsToTestRun1736172058779
query failed: ALTER TABLE `test_run` ADD COLUMN `totalCases` INT CHECK(
				CASE
					WHEN status = 'new' THEN `totalCases` IS NULL
					WHEN status in ('cancelled', 'error') THEN `totalCases` IS NULL OR `totalCases` >= 0
					ELSE `totalCases` >= 0
				END
			)
error: Error: Column check constraint 'test_run_chk_1' references other column.

Solution

As those stats columns are soon to be removed anyway, created a separate migration file for MySQL under same migration name, without the check constraints. This change will not affect existing instances where the migration applied successfully, but only instances powered by MySQL, where this migration could not be applied.

Incomplete migration of PK type

The migration MigrateTestDefinitionKeyToString1731582748663 implementation for MySQL was incomplete. Although it run successfully, the queries for assigning new Primary Key and deleting temporary one was missing. This left the table test_definition in an inconsistent state (no PK), which prevented the next migration CreateTestMetricTable1732271325258 from running properly on MySQL 8.4.4:

Starting migration CreateTestMetricTable1732271325258
query failed: CREATE TABLE `test_metric` (`id` varchar(36) NOT NULL, `name` varchar(255) NOT NULL, `testDefinitionId` varchar(36) NOT NULL, `createdAt` datetime(3) NOT NULL DEFAULT CURRENT_TIMESTAMP(3), `updatedAt` datetime(3) NOT NULL DEFAULT CURRENT_TIMESTAMP(3), INDEX `IDX_3a4e9cf37111ac3270e2469b47` (`testDefinitionId`), CONSTRAINT `FK_3a4e9cf37111ac3270e2469b475` FOREIGN KEY (`testDefinitionId`) REFERENCES `test_definition` (`id`) ON DELETE CASCADE, PRIMARY KEY (`id`)) ENGINE=InnoDB
error: Error: Failed to add the foreign key constraint. Missing unique key for constraint 'FK_3a4e9cf37111ac3270e2469b475' in the referenced table 'test_definition'

Solution

Missing operations were added to the MigrateTestDefinitionKeyToString1731582748663 migration.
For those instances where the incomplete migration was already applied, the same operations were added to the subsequent migration CreateTestMetricTable1732271325258 conditionally.
Also FixTestDefinitionPrimaryKey1739873751194 migration was added to fix the primary column if it has not been fixed yet by the patched MigrateTestDefinitionKeyToString1731582748663 or CreateTestMetricTable1732271325258 migrations.

Added DB tests

To avoid issues like this in future, a new job for running tests using MySQL was added to a CI config.

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 Feb 14, 2025
Copy link

codecov bot commented Feb 17, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

📢 Thoughts on this report? Let us know!

@burivuhster burivuhster marked this pull request as ready for review February 17, 2025 10:55
@burivuhster burivuhster requested a review from a team as a code owner February 17, 2025 10:55
tomi
tomi previously approved these changes Feb 18, 2025
Copy link
Collaborator

@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.

Thank you for fixing this and adding the new targets to the DB tests ❤️

Copy link

cypress bot commented Feb 18, 2025

n8n    Run #9318

Run Properties:  status check passed Passed #9318  •  git commit 32db13585d: 🌳 🖥️ browsers:node18.12.0-chrome107 🤖 burivuhster 🗃️ e2e/*
Project n8n
Branch Review ai-644-failing-mysql-db-migration
Run status status check passed Passed #9318
Run duration 04m 27s
Commit git commit 32db13585d: 🌳 🖥️ browsers:node18.12.0-chrome107 🤖 burivuhster 🗃️ e2e/*
Committer Eugene Molodkin
View all properties for this run ↗︎

Test results
Tests that failed  Failures 0
Tests that were flaky  Flaky 1
Tests that did not run due to a developer annotating a test with .skip  Pending 5
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 436
View all changes introduced in this branch ↗︎

Copy link
Contributor

✅ All Cypress E2E specs passed

@burivuhster
Copy link
Contributor Author

@tomi I was running some tests locally and found there are still cases, where primary key fix might not be applied (when all previous migrations were applied "successfully" before patch). I added a separate conditional migration to account for that. Tested on mysql 8.0.29 and 8.4, mariadb 10.5, each for both applying all migrations at once (same as in CI pipeline), and applying migrations from master branch first, then switching to this branch and applying all the fixes.

@burivuhster burivuhster requested a review from tomi February 18, 2025 11:50
Copy link
Contributor

✅ All Cypress E2E specs passed

@burivuhster burivuhster merged commit d0968a1 into master Feb 18, 2025
47 checks passed
@burivuhster burivuhster deleted the ai-644-failing-mysql-db-migration branch February 18, 2025 12:16
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.

2 participants