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

Add allowUnorderedMigrations option to Migrator #723

Merged
merged 10 commits into from
Jan 5, 2024

Conversation

tracehelms
Copy link
Contributor

@tracehelms tracehelms commented Oct 5, 2023

What is this?

This add an allowUnorderedMigrations option to MigratorProps. The options are true or false, with false being the default.

When true, it will allow new migrations to be run even if they are added alphabetically before ones that have already executed.

When false, it will give an error if your migration files are not alphabetically ordered in the same order that they were executed in the database.

Closes #697

Why?

If a team has multiple developers, two people can create migrations asynchronously on separate branches. With strict ordering, those migration files have to be merged in the right order (or renamed before merging) or else Kysely will throw an error when trying to migrate them.

Example:

- Developer A creates migration `2023-09-13-migration-A.ts`
- Developer B creates migration `2023-09-14-migration-B.ts`
- Both developers make a PR at roughly the same time
- Developer B merges their PR and deploys
- Developer A merges their PR
- Developer A deploys and tries to run migrations

Kysely will give an error for this with strict ordering. With permissive ordering, the unrun migration is executed.

Pending migrations vs. executed migrations

This also adds the concept of pending (unexecuted) migrations to Migrator. When migrating up, Migrator will run pending migrations in order for n steps. When migrating down, Migrator will consult the database for executed migrations and travel down that list n steps.

This doesn't change the current functionality for ensuring ordered migrations, but also allows out of order ones.

Some examples:
(* denotes current migration version)

New migration:

Migrations Already Ran:  0  1  2*
Migration Files:         0  1  2  3

Migrate up:              0  1  2  3*

Migrate down one:

Migrations Already Ran:  0  1  2*
Migration Files:         0  1  2  3

Migrate down:            0  1*

New out-of-order migration:

Migrations Already Ran:        0  1  2     4*
Migration Files:               0  1  2  3  4  5

Migrate up (strict):           error
Migrate up (permissive):       0  1  2  3  4*
Migrate up again (permissive): 0  1  2  3  4  5*

Migrate down with out-of-order migration:

Migrations Already Ran:    0  1  2     4*
Migration Files:           0  1  2  3  4  5

Migrate down (strict):     error
Migrate down (permissive): 0  1  2*

@vercel
Copy link

vercel bot commented Oct 5, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
kysely ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 2, 2024 5:53pm

@tracehelms tracehelms marked this pull request as ready for review October 5, 2023 22:06
@igalklebanov igalklebanov added enhancement New feature or request migrations Related to migrations api Related to library's API labels Oct 6, 2023
@igalklebanov
Copy link
Member

igalklebanov commented Oct 6, 2023

Hey 👋

Thanks! 💪

I feel like allowUnorderedMigrations seems clearer, or am I missing something?

@tracehelms
Copy link
Contributor Author

I feel like allowUnorderedMigrations seems clearer, or am I missing something?

That would definitely be clearer, good thinking. I'll update and post up the changes.

@tracehelms tracehelms changed the title Add migrationOrder option to Migrator to allow out of order migrations Add allowUnorderedMigrations option to Migrator Oct 6, 2023
@tracehelms
Copy link
Contributor Author

@igalklebanov Done, and I updated the docs / comments too 👍

@tracehelms
Copy link
Contributor Author

Hi @igalklebanov and @koskimas, I was just wondering if you've gotten a chance to look at this and what you think. Let me know if you've got any questions / requests 😄 Thanks!

@WyattMcMurry
Copy link

WyattMcMurry commented Oct 25, 2023

I think this is a really necessary change for coordinating across multiple teams, environments, and timezones.
Edit: just wanted to add that Kyesely rocks!

@danolife
Copy link

danolife commented Oct 26, 2023

Yes can you please release this already 😭
My production is broken because of unordered migrations, I don't want to update everything manually again!
Please @igalklebanov @koskimas 🙏

@tracehelms
Copy link
Contributor Author

This branch was out of date with master so I rebased and pushed up the changes 👍

@blbrown13
Copy link

This would be super helpful for our team's workflow! 😍 Hopefully it gets approved and merged soon (cc @igalklebanov @koskimas ) 🙏🏻

@avocadojesus
Copy link

This would be really useful for my team as well, coordinating migrations is very tricky for us and this would solve all of our problems. Other than this little snag, Kysely has been absolutely incredible to use, thanks so much to the maintainers for writing such an incredible library!!!

@koskimas
Copy link
Member

koskimas commented Dec 29, 2023

Now that I have a week off from work, I finally have some time for Kysely and could review this one.

@danolife We're not keeping these PRs waiting just to be mean or because we hate your team. We simply haven't had time.

@tracehelms This seems excellent and will definitely get merged! Could you rebase this one more time?

src/migration/migrator.ts Outdated Show resolved Hide resolved
@tracehelms
Copy link
Contributor Author

Hey @koskimas, thanks for reviewing! I'll get this rebased and patched up with your changes. 👍

@tracehelms
Copy link
Contributor Author

Ready for another look @koskimas 👍

Copy link
Member

@koskimas koskimas left a comment

Choose a reason for hiding this comment

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

Perfect!

@koskimas koskimas merged commit 6b1ab66 into kysely-org:master Jan 5, 2024
5 checks passed
@tracehelms
Copy link
Contributor Author

Awesome, thanks for reviewing and merging! 💯

@koskimas
Copy link
Member

koskimas commented Jan 6, 2024

This is now released in 0.27.2

thecodrr pushed a commit to thecodrr/kysely that referenced this pull request Sep 3, 2024
* naive version of allowing out of order migrations

* refactor migrator.ts to allow the concept of pending migrations

* fix test

* add more tests

* prettier

* more test cases

* add docs to getting started

* clean up

* change Migrator option to allowUnorderedMigrations

* add a secondary order by name for migrations
@loremru
Copy link

loremru commented Sep 14, 2024

Actually allowUnorderedMigrations doesn't work

image image image

@igalklebanov
Copy link
Member

igalklebanov commented Sep 14, 2024

@loremru this PR contains a unit test that proves it does. Thus makes me highly doubt your error has anything to do with this feature.

@loremru
Copy link

loremru commented Sep 16, 2024

@igalklebanov
I think #resolveMigrations should not sort my migrations if I set allowUnorderedMigrations to true.
When I removed this line, error disappears
image

@igalklebanov
Copy link
Member

igalklebanov commented Sep 21, 2024

@loremru you're misunderstanding the feature. It is about not throwing migrator-specific errors if a new migration file is added that is in the middle of the sorted ran migrations.

e.g.
PR 1 has 0003-a.js
PR 2 has 0003-b.js

If PR 2 gets merged first, running the migrations once PR 1 is merged would fail because:

[x] 0001
[x] 0002
[ ] 0003-a
[x] 0003-b

This feature makes it not fail.

In your case, the migrations are poorly named. You should at least pad left with 0s. This probably results in sql errors, not migrator errors as the historic order is wrong between migrations that access the same resource. e.g. alter before create.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api Related to library's API enhancement New feature or request migrations Related to migrations ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Out of Order Migrations and Possible Solutions
8 participants