migration up or down to the specific version #759
Conversation
|
@velasco300 hello! Thank you for contribution! I think we can merge this after: #706. |
|
Thank you |
There was a problem hiding this comment.
Hey @velasco300, thanks for contributions! Sorry for the delay.
I found a problem... let say I have following three migrations:
pub use sea_orm_migration::prelude::*;
mod m20220120_000001_create_post_table;
mod m20220120_000002_aaa;
mod m20220120_000003_bbb;
pub struct Migrator;
#[async_trait::async_trait]
impl MigratorTrait for Migrator {
fn migrations() -> Vec<Box<dyn MigrationTrait>> {
vec![
Box::new(m20220120_000001_create_post_table::Migration),
Box::new(m20220120_000002_aaa::Migration),
Box::new(m20220120_000003_bbb::Migration),
]
}
}Then, I apply all migration at once with command which works as expected:
$ DATABASE_URL="postgres://root:root@localhost/active_enum_tests" cargo r -- migrate up -d ../examples/tonic_example/migration
Applying all pending migrations
Applying migration 'm20220120_000001_create_post_table'
Migration 'm20220120_000001_create_post_table' has been applied
Applying migration 'm20220120_000002_aaa'
Migration 'm20220120_000002_aaa' has been applied
Applying migration 'm20220120_000003_bbb'
Migration 'm20220120_000003_bbb' has been applied
But then if I run migrate up -V with a target migration, it perform rollback instead of "migrate up":
$ DATABASE_URL="postgres://root:root@localhost/active_enum_tests" cargo r -- migrate up -d ../examples/tonic_example/migration -V m20220120_000002_aaa
Rolling back 1 applied migrations
Rolling back migration 'm20220120_000003_bbb'
Migration 'm20220120_000003_bbb' has been rollbacked
It's counter intuitive and probably destructive. I would suggest migrate up -V and migrate down -V will only go in its desired direction but not bidirectional (apply / rollback migrations dpending on the given target migration)
Btw... it would be appreciated if there are test cases added for the new methods: change_to_version and get_steps.
|
@billy1624 Thank you for your feedback. At first What I thought was that I knew clearly that I wanted to change to a certain version, but I had to understand and handle more details. For example, in shell scripts, You have to do like this: when > version then do something when < version then do something when unknown then do something, such API are not easy to use. I'll fix it later |
|
Appreciated :) |
|
@billy1624 I fixed it, in your case( sea-orm-cli migrate up -V m20220120_000001_create_post_table ),In my case( sea-orm-cli migrate up -V m20220120_000001_create_post_table -f ) |
sea-orm-migration/src/migrator.rs
Outdated
| MigrationStatus::Pending => { | ||
| for migration in migrations { | ||
| if migration.status == MigrationStatus::Pending { | ||
| index += 1; | ||
| } |
There was a problem hiding this comment.
I think we should use get_pending_migrations method to get a list of pending migration. To keep the behaviour consistent with the up method.
There was a problem hiding this comment.
If this is the case ( #755 ）, Migration V1: Applied，Migration V2: Pending，Migration V3: Applied, Migration V4: Applied, get_pending_migrations method has not V3
sea-orm-migration/src/migrator.rs
Outdated
| MigrationStatus::Applied => { | ||
| for migration in migrations.into_iter().rev() { | ||
| if migration.status == MigrationStatus::Applied { | ||
| index += 1; | ||
| } | ||
| if migration.migration.name() == version { | ||
| if migration.status == MigrationStatus::Pending { | ||
| index += 1; | ||
| } | ||
| matched = true; | ||
| break; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Same here. Using get_applied_migrations to get the list of applied migrations. I think the logic where migration.status == MigrationStatus::Pending still increment the index by 1 is faulty. As the down method is counting on applied migration only and all pending migrations are ignored.
Consider the case where we have three migrations:
- Migration 1: Applied
- Migration 2: Pending
- Migration 3: Applied
We should rollback 2 migrations instead of 3
There was a problem hiding this comment.
Down to Migration 1, after that Migration 1 is Applied or Pending, In practical application, I think Applied is more reasonable，if Pending, I don't know what the current status is，What about your suggestion？
|
It would be appreciated if we have some test cases around |
|
We have upgraded |
|
Ah I don't have a clear thought / comment for this for now |
|
upgraded |
|
@billy1624 @tyt2y3 I think the PR is done if you want to review it. |
| /// Apply or Rollback migrations to version | ||
| async fn change_to_version(db: &DbConn, version: &str) -> Result<(), DbErr> { | ||
| Self::up_to_version(db, version).await?; | ||
| Self::down_to_version(db, version).await | ||
| } |
There was a problem hiding this comment.
I think this shouldn't perform up then down. Instead, it should go in one direction only
There was a problem hiding this comment.
Hey @velasco300, thanks for contributing!
I think it's better for this feature to be built on top of MigratorTrait::up and MigratorTrait::down. So, basically, it take a migration file name ("migration version"). Then, decide the direction up or down and count the number of "steps" to migrate N version up or down to reach the target "migration version".
Also, I think it'd be best if we can tell user what migrations will be applied / rollbacked and ask for user confirmation before executing.
For example, in Yii, it will print something like...
$ ./yii migrate/to 220627_000000
Yii Migration Tool (based on Yii v2.0.40)
Total 5 migrations to be reverted:
m220812_000000_xxx_migration_file_name_xxx
m220811_000001_xxx_migration_file_name_xxx
m220811_000000_xxx_migration_file_name_xxx
m220802_000001_xxx_migration_file_name_xxx
m220802_000000_xxx_migration_file_name_xxx
Revert the above migrations? (yes|no) [no]:no <- ask for confirmation from user
What's your thoughts? @velasco300
example:
Enter at the command line: sea-orm-cli migrate up -V m20220120_000001_create_post_table