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

migration up or down to the specific version #759

Open
wants to merge 15 commits into
base: master
Choose a base branch
from
15 changes: 12 additions & 3 deletions sea-orm-cli/src/commands.rs
Original file line number Diff line number Diff line change
Expand Up @@ -248,19 +248,22 @@ pub fn run_migrate_command(matches: &ArgMatches<'_>) -> Result<(), Box<dyn Error
update_migrator(&migration_name, migration_dir)?;
return Ok(());
}
let (subcommand, migration_dir, steps, verbose) = match migrate_subcommand {
let (subcommand, migration_dir, steps, version, force, verbose) = match migrate_subcommand {
// Catch all command with pattern `migrate xxx`
(subcommand, Some(args)) => {
let migration_dir = args.value_of("MIGRATION_DIR").unwrap();
let steps = args.value_of("NUM_MIGRATION");
let version = args.value_of("VERSION_MIGRATION");
let force = args.is_present("FORCE_MIGRATION");
let verbose = args.is_present("VERBOSE");
(subcommand, migration_dir, steps, verbose)
(subcommand, migration_dir, steps, version, force, verbose)
}
// Catch command `migrate`, this will be treated as `migrate up`
_ => {
let migration_dir = matches.value_of("MIGRATION_DIR").unwrap();
let verbose = matches.is_present("VERBOSE");
("up", migration_dir, None, verbose)
let force = matches.is_present("FORCE_MIGRATION");
("up", migration_dir, None, None, force, verbose)
}
};
// Construct the `--manifest-path`
Expand All @@ -280,6 +283,12 @@ pub fn run_migrate_command(matches: &ArgMatches<'_>) -> Result<(), Box<dyn Error
if let Some(steps) = steps {
args.extend(["-n", steps]);
}
if let Some(version) = version {
args.extend(["-V", version]);
}
if force {
args.push("-f");
}
if verbose {
args.push("-v");
}
Expand Down
30 changes: 29 additions & 1 deletion sea-orm-cli/src/migration.rs
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,22 @@ pub fn get_subcommands() -> Vec<App<'static, 'static>> {
.short("n")
.help("Number of pending migrations to be applied")
.takes_value(true),
)
.arg(
Arg::with_name("VERSION_MIGRATION")
.long("version")
.short("V")
.help("Version of pending migrations to be applied")
.takes_value(true),
)
.arg(
Arg::with_name("FORCE_MIGRATION")
.long("force")
.short("f")
.help("force version of pending migrations to be applied")
.takes_value(false),
),
SubCommand::with_name("down")
SubCommand::with_name("down")
.about("Rollback applied migrations")
.arg(
Arg::with_name("NUM_MIGRATION")
Expand All @@ -44,6 +58,20 @@ pub fn get_subcommands() -> Vec<App<'static, 'static>> {
.help("Number of pending migrations to be rolled back")
.takes_value(true)
.default_value("1"),
)
.arg(
Arg::with_name("VERSION_MIGRATION")
.long("version")
.short("V")
.help("Version of pending migrations to be rolled back")
.takes_value(true),
)
.arg(
Arg::with_name("FORCE_MIGRATION")
.long("force")
.short("f")
.help("force version of pending migrations to be rolled back")
.takes_value(false),
),
]
}
30 changes: 24 additions & 6 deletions sea-orm-migration/src/cli.rs
Original file line number Diff line number Diff line change
Expand Up @@ -60,14 +60,32 @@ where
("up", None) => M::up(db, None).await,
("down", None) => M::down(db, Some(1)).await,
("up", Some(args)) => {
let str = args.value_of("NUM_MIGRATION").unwrap_or_default();
let steps = str.parse().ok();
M::up(db, steps).await
let version = args.value_of("VERSION_MIGRATION");
if version.is_some() {
if args.is_present("FORCE_MIGRATION") {
M::change_to_version(db, version.unwrap()).await
} else {
M::up_to_version(db, version.unwrap()).await
}
} else {
let str = args.value_of("NUM_MIGRATION").unwrap_or_default();
let steps = str.parse().ok();
M::up(db, steps).await
}
}
("down", Some(args)) => {
let str = args.value_of("NUM_MIGRATION").unwrap();
let steps = str.parse().ok().unwrap_or(1);
M::down(db, Some(steps)).await
let version = args.value_of("VERSION_MIGRATION");
if version.is_some() {
if args.is_present("FORCE_MIGRATION") {
M::change_to_version(db, version.unwrap()).await
} else {
M::down_to_version(db, version.unwrap()).await
}
} else {
let str = args.value_of("NUM_MIGRATION").unwrap();
let steps = str.parse().ok().unwrap_or(1);
M::down(db, Some(steps)).await
}
}
_ => M::up(db, None).await,
}
Expand Down
64 changes: 64 additions & 0 deletions sea-orm-migration/src/migrator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -289,6 +289,70 @@ pub trait MigratorTrait: Send {

Ok(())
}

/// 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
}
Comment on lines +311 to +315
Copy link
Member

Choose a reason for hiding this comment

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

I think this shouldn't perform up then down. Instead, it should go in one direction only


/// Apply migrations to version
async fn up_to_version(db: &DbConn, version: &str) -> Result<(), DbErr> {
let steps = Self::get_steps(db, MigrationStatus::Pending, version).await?;
if steps > 0 {
Self::up(db, Some(steps)).await
} else {
Ok(())
}
}

/// Rollback migrations to version
async fn down_to_version(db: &DbConn, version: &str) -> Result<(), DbErr> {
let steps = Self::get_steps(db, MigrationStatus::Applied, version).await?;
if steps > 1 {
Self::down(db, Some(steps - 1)).await
} else {
Ok(())
}
}

async fn get_steps(db: &DbConn, status: MigrationStatus, version: &str) -> Result<u32, DbErr> {
let mut index = 0;
let mut matched = false;
let migrations = Self::get_migration_with_status(db).await?;
match status {
MigrationStatus::Pending => {
for migration in migrations {
if migration.status == MigrationStatus::Pending {
index += 1;
}
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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

if migration.migration.name() == version {
matched = true;
break;
}
}
}
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;
}
}
}
Copy link
Member

Choose a reason for hiding this comment

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

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:

  1. Migration 1: Applied
  2. Migration 2: Pending
  3. Migration 3: Applied
    We should rollback 2 migrations instead of 3

Copy link
Author

Choose a reason for hiding this comment

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

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?

}
if matched {
Ok(index)
} else {
Ok(0)
}
}
}

pub(crate) fn query_tables(db: &DbConn) -> SelectStatement {
Expand Down