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

Attempt to make "require shares fields" migration safer/more reliable #19619

Merged
merged 7 commits into from
Sep 12, 2023

Conversation

paescuj
Copy link
Member

@paescuj paescuj commented Sep 7, 2023

After the last correction of the "require shares fields" migration in #19407, it seems that there are still MySQL instances that struggle with this migration (see #19399).

Unfortunately this could not be reproduced so far, so here's an attempt to make the migration a bit safer/more reliable:

  • Split into multiple ALTER TABLE statements, because depending on the engine it may not be supported to drop & recreate in the same statement.
  • Fetch and use the actual foreign key constraint name instead of static one.
  • Log foreign key constraints if it should come in an unexpected form.

Closes #19399 (we'll reopen if issue should still not be solved)

@changeset-bot
Copy link

changeset-bot bot commented Sep 7, 2023

🦋 Changeset detected

Latest commit: 4d37132

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
@directus/api Patch
directus Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@paescuj paescuj requested review from a team, rijkvanzanten, DanielBiegler and jaads and removed request for a team September 7, 2023 12:35
@paescuj paescuj changed the title Attempt to make "require shares fields" migration more safer/reliable Attempt to make "require shares fields" migration safer/more reliable Sep 7, 2023
@prikr
Copy link

prikr commented Sep 10, 2023

@paescuj, do you think #19615 is related to this?

@paescuj
Copy link
Member Author

paescuj commented Sep 11, 2023

@paescuj, do you think #19615 is related to this?

Well, it's the same error message, but not really related since it's happening in two different places and therefore requiring separate treatment.

paescuj and others added 2 commits September 11, 2023 10:53
Co-authored-by: Azri Kahar <42867097+azrikahar@users.noreply.github.com>
Copy link
Member

@br41nslug br41nslug left a comment

Choose a reason for hiding this comment

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

I am unable to repro the original issue but the code looks good

@paescuj paescuj merged commit 0c56c46 into main Sep 12, 2023
7 checks passed
@paescuj paescuj deleted the more-reliable-shares-migration branch September 12, 2023 19:10
@github-actions github-actions bot added this to the Next Release milestone Sep 12, 2023
br-rafaelbarros pushed a commit to personal-forks/directus-source that referenced this pull request Nov 7, 2023
…directus#19619)

Co-authored-by: Azri Kahar <42867097+azrikahar@users.noreply.github.com>
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 1, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Upgrading from Docker image 10.4.3 to 10.5.3 breaks the app on database migration
5 participants