db-migrate / node-db-migrate

Database migration framework for node
Other
2.32k stars 360 forks source link

No order is guaranteed if migrations have the same date #646

Open abolognino opened 5 years ago

abolognino commented 5 years ago

I'm submitting a...

Current behavior

Migrations on the same date (i.e. same year, month, day, hour, minute and second) aren't applied with any discernible criteria:

image

Expected behavior

I would expect the alphabetical order to be respected.

Minimal reproduction of the problem with instructions

create a bunch of migrations (if you have a machine fast enough to create multiple migrations on the same second, otherwise simply rename them)

for i in `seq 1 100`; do npx db_migrate create x$i --sql-file --env dev; done

apply them:

npx db_migrate up --env dev

watch them be in a randomish order

What is the motivation / use case for changing the behavior?

Avoid the confusion of users and breakage when auto-generating migrations

Environment


db-migrate version: 0.11.6
db-migrate driver with versions: pg 1.0.0

Additional information:
- Node version: v10.15.3
- Platform:  Linux (Ubuntu 18.04.3 LTS)
--- Want to back this issue? **[Post a bounty on it!](https://www.bountysource.com/issues/79518314-no-order-is-guaranteed-if-migrations-have-the-same-date?utm_campaign=plugin&utm_content=tracker%2F73887&utm_medium=issues&utm_source=github)** We accept bounties via [Bountysource](https://www.bountysource.com/?utm_campaign=plugin&utm_content=tracker%2F73887&utm_medium=issues&utm_source=github).
stale[bot] commented 5 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

wzrdtales commented 5 years ago

This is not a bug, but how migrations work indeed. So the naming of migrations within db-migrate are actually target for plugins, since we gathered already use cases like semver versioning as an alternative sorting approach. So plugging a plugin into this will be the solution in the long run (this needs to be implemented yet though), "fixing" this issue which is very uncommon will however never happen. In this case it is how it works and changing it would mean a breaking change, but this shouldn't be that big of a problem since this issue never happens unless you programatically create migrations, or the unlikely case happens that two users at the same time create a migration individually.

I will let you know o/c as soon as the ability to configure different sorting schemes by plugins becomes available.

abolognino commented 5 years ago

@wzrdtales right now there is no deterministic sorting, that's the main issue.

Any order would be acceptable, but migrations with the same timestamp right now are applied in a seemingly random order. This is just bad.

This issue occured literally the first time I've ever used node-db-migrate in a project.

The problem is the filteredCompleted function in db-migrate-shared, changing the implementation from

var sortFn = function(a, b) {

    a = a.name.slice(0, a.name.indexOf('-'));
    b = b.name.slice(0, b.name.indexOf('-'));

    if(!isNaN(a)) {
        return a - b;
    }

    return a.localeCompare(b);
};

to

var sortFn = function(a, b) {
    return a.name.localeCompare(b.name);
};

fixes the issue in my usecase (I've no idea if this breaks something else). I suggest you either fix the filteredCompleted function to use the full name of the migration and not only the timestamp OR return an error if more than a migration exists with the same timestamp.

stale[bot] commented 4 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

wzrdtales commented 4 years ago

returning an error if there is more than one migration to be executed with the same timestamp sounds fair enough, you're right in terms of missing information towards the user about something bad is happening.

wzrdtales commented 4 years ago

A note to your suggested change: I can't just change something like that, since it would be a breaking change, so it will not be touched, but as stated before, new strategies are on the plugin ToDo.