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

feat: sort migrations before applying #111

Merged
merged 3 commits into from
Aug 13, 2024

Conversation

sf0rman
Copy link
Contributor

@sf0rman sf0rman commented May 22, 2024

Problem Description

Migration files are not ordered by timestamp when running up-command which can cause ordering issues.

For example
Migration1: createCollection
Migration2: change stuff within collection

If not ordered, migration-stuff attempts to run first and fails due to dependency on Migration1 (or collection existing)

ExecuteMigrationError: MongoServerError: ns does not exist: db.collection

Solution

When creating a migration file, we know that all class names end with a 13-digit timestamp (./lib/commands/new.ts:57) regardless of using a custom template or the default one.

We can therefore extract timestamp from className (last 13 digits of className) and sort migrations based on this before running migrations.

Copy link

@matheusmelchiades matheusmelchiades left a comment

Choose a reason for hiding this comment

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

LGTM

@matheusmelchiades
Copy link

matheusmelchiades commented Aug 12, 2024

Should check if order should be by file sort than className...
#114

Copy link
Owner

@mycodeself mycodeself left a comment

Choose a reason for hiding this comment

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

Hi @sf0rman, thank you very much for your input.

I have simply made a change to compare the className as a string with localeCompare when sorting the migrations, This way we are not dependent on the format of the className which may change.

I'll generate a fix version with these changes right now!

Thanks again!

Edit: I messed up it with the localCompare option, I have reverted to your original code in the version 1.6.2, it works perfectly. Thank you

lib/commands/up.ts Outdated Show resolved Hide resolved
lib/utils/timestamp.ts Show resolved Hide resolved
lib/commands/up.ts Outdated Show resolved Hide resolved
Use localeCompare instead of relying on the format of the className that could change.
@mycodeself mycodeself merged commit dab6f48 into mycodeself:master Aug 13, 2024
4 checks passed
@mycodeself
Copy link
Owner

🎉 This PR is included in version 1.6.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants