From 917ad75b92686543934425039f454235c7dc2006 Mon Sep 17 00:00:00 2001 From: theredfish Date: Sun, 11 Oct 2020 01:10:59 +0200 Subject: [PATCH] Add new arguments to the revert subcommand --- diesel_cli/src/cli.rs | 37 +++++------ diesel_cli/src/main.rs | 43 +++++++------ diesel_cli/src/validators/mod.rs | 1 + diesel_cli/src/validators/num.rs | 41 +++++++++++++ diesel_cli/tests/migration_revert.rs | 61 +++++++++++++------ .../migrations_internals/src/connection.rs | 6 +- .../migrations_internals/src/lib.rs | 22 ++----- 7 files changed, 133 insertions(+), 78 deletions(-) create mode 100644 diesel_cli/src/validators/mod.rs create mode 100644 diesel_cli/src/validators/num.rs diff --git a/diesel_cli/src/cli.rs b/diesel_cli/src/cli.rs index f365cf555acc..31880a43e122 100644 --- a/diesel_cli/src/cli.rs +++ b/diesel_cli/src/cli.rs @@ -1,3 +1,4 @@ +use crate::validators::num::*; use clap::{App, AppSettings, Arg, Shell, SubCommand}; pub fn build_cli() -> App<'static, 'static> { @@ -20,20 +21,30 @@ pub fn build_cli() -> App<'static, 'static> { .subcommand(SubCommand::with_name("run").about("Runs all pending migrations")) .subcommand( SubCommand::with_name("revert") - .about("Reverts the latest run migration") + .about("Reverts the specified migrations") + .arg( + Arg::with_name("REVERT_ALL") + .long("all") + .short("a") + .help("Reverts previously run migration files.") + .takes_value(false) + .conflicts_with("REVERT_NUMBER"), + ) .arg( Arg::with_name("REVERT_NUMBER") .long("number") .short("n") - .help("Reverts the last migration files") + .help("Reverts the last `n` migration files") .long_help( "When this option is specified the last `n` migration files \ will be reverted. By default revert the last one.", ) + // TODO : when upgrading to clap 3.0 add default_value("1"). + // Then update code in main.rs for the revert subcommand. + // See https://github.com/clap-rs/clap/issues/1605 .takes_value(true) - .required(true) - .default_value("1") - .validator(migration_revert_number_validator), + .validator(is_positive_int) + .conflicts_with("REVERT_ALL"), ), ) .subcommand( @@ -234,19 +245,3 @@ fn migration_dir_arg<'a, 'b>() -> Arg<'a, 'b> { .takes_value(true) .global(true) } - -fn migration_revert_number_validator(val: String) -> Result<(), String> { - if val == "all" { - return Ok(()); - } - - let number = val.parse::().map_err(|_| { - "Cannot parse . The input must be an integer or 'all'.".to_string() - })?; - - if number < 1 { - return Err(" must be at least equal to 1.".to_string()); - } - - Ok(()) -} diff --git a/diesel_cli/src/main.rs b/diesel_cli/src/main.rs index 857173b79981..c60d95208aff 100644 --- a/diesel_cli/src/main.rs +++ b/diesel_cli/src/main.rs @@ -24,6 +24,7 @@ mod infer_schema_internals; mod print_schema; #[cfg(any(feature = "postgres", feature = "mysql"))] mod query_helper; +mod validators; use chrono::*; use clap::{ArgMatches, Shell}; @@ -75,24 +76,30 @@ fn run_migration_command(matches: &ArgMatches) -> Result<(), Box> { let database_url = database::database_url(matches); let dir = migrations_dir(matches).unwrap_or_else(handle_error); - // We can unwrap since the argument is validated from the cli module. - // The value is required and is a number greater than 0 or "all". - let number = args.value_of("REVERT_NUMBER").unwrap(); - - // Test if we have an integer or "all" - match number.parse::() { - Ok(number) => { - call_with_conn!( - database_url, - migrations::revert_latest_migrations_in_directory(&dir, number) - )?; - } - _ => { - call_with_conn!( - database_url, - migrations::revert_all_migrations_in_directory(&dir) - )?; - } + if args.is_present("REVERT_ALL") { + call_with_conn!( + database_url, + migrations::revert_all_migrations_in_directory(&dir) + )?; + } else { + // TODO : remove this logic when upgrading to clap 3.0. + // We handle the default_value here instead of doing it + // in the cli. This is because arguments with default + // values conflict even if not used. + // See https://github.com/clap-rs/clap/issues/1605 + let number = match args.value_of("REVERT_NUMBER") { + None => "1", + Some(number) => number, + }; + + call_with_conn!( + database_url, + migrations::revert_latest_migrations_in_directory( + &dir, + // We can unwrap since the argument is validated from the cli module. + number.parse::().unwrap() + ) + )?; } regenerate_schema_if_file_specified(matches)?; diff --git a/diesel_cli/src/validators/mod.rs b/diesel_cli/src/validators/mod.rs new file mode 100644 index 000000000000..bed07382f7cc --- /dev/null +++ b/diesel_cli/src/validators/mod.rs @@ -0,0 +1 @@ +pub(crate) mod num; diff --git a/diesel_cli/src/validators/num.rs b/diesel_cli/src/validators/num.rs new file mode 100644 index 000000000000..99e79418263a --- /dev/null +++ b/diesel_cli/src/validators/num.rs @@ -0,0 +1,41 @@ +pub fn is_positive_int(val: String) -> Result<(), String> { + match val.parse::() { + Ok(val) if val > 0 => Ok(()), + // If the the value is <= 0 or can't be parsed + _ => Err(format!("{} isn't a positive integer.", val)), + } +} + +#[cfg(test)] +mod tests { + use super::is_positive_int; + + #[test] + fn is_positive_int_should_parse_a_positive_integer_from_input_string() { + assert_eq!(is_positive_int("1".to_string()), Ok(())) + } + + #[test] + fn is_positive_int_should_throw_an_error_with_zero() { + assert_eq!( + is_positive_int("0".to_string()), + Err("0 isn't a positive integer.".to_string()) + ) + } + + #[test] + fn is_positive_int_should_throw_an_error_with_negative_integer() { + assert_eq!( + is_positive_int("-5".to_string()), + Err("-5 isn't a positive integer.".to_string()) + ) + } + + #[test] + fn is_positive_int_should_throw_an_error_with_float() { + assert_eq!( + is_positive_int("5.2".to_string()), + Err("5.2 isn't a positive integer.".to_string()) + ) + } +} diff --git a/diesel_cli/tests/migration_revert.rs b/diesel_cli/tests/migration_revert.rs index ea1dd8b5be46..fd19e4f57fed 100644 --- a/diesel_cli/tests/migration_revert.rs +++ b/diesel_cli/tests/migration_revert.rs @@ -216,17 +216,11 @@ fn migration_revert_all_runs_the_migrations_down() { assert!(db.table_exists("contracts")); assert!(db.table_exists("bills")); - // Reverts all migrations. The migrations `contracts`, `bills`, `customers` and - // `00000000000000_diesel_initial_setup` should be reverted. - let result = p - .command("migration") - .arg("revert") - .arg("-n") - .arg("all") - .run(); + let result = p.command("migration").arg("revert").arg("--all").run(); assert!(result.is_success(), "Result was unsuccessful {:?}", result); + #[cfg(feature = "postgres")] assert!( result.stdout() == "Rolling back migration 2017-09-12-210424_create_bills\n\ @@ -237,20 +231,40 @@ fn migration_revert_all_runs_the_migrations_down() { result.stdout() ); + // MySQL and SQLite databases don't call the default migration 00000000000000_diesel_initial_setup. + #[cfg(feature = "mysql,sqlite")] + assert!( + result.stdout() + == "Rolling back migration 2017-09-12-210424_create_bills\n\ + Rolling back migration 2017-09-03-210424_create_contracts\n\ + Rolling back migration 2017-08-31-210424_create_customers\n", + "Unexpected stdout : {}", + result.stdout() + ); + assert!(!db.table_exists("customers")); assert!(!db.table_exists("contracts")); assert!(!db.table_exists("bills")); } #[test] -fn migration_revert_with_zero_should_throw_an_error() { - let p = project("migration_revert_with_zero_should_throw_an_error") +fn migration_revert_with_zero_should_not_revert_any_migration() { + let p = project("migration_revert_with_zero_should_not_revert_any_migration") .folder("migrations") .build(); + let db = database(&p.database_url()); + + p.create_migration( + "2017-08-31-210424_create_customers", + "CREATE TABLE customers ( id INTEGER PRIMARY KEY )", + "DROP TABLE customers", + ); // Make sure the project is setup p.command("setup").run(); + assert!(db.table_exists("customers")); + // Should not revert any migration. let result = p .command("migration") @@ -260,16 +274,11 @@ fn migration_revert_with_zero_should_throw_an_error() { .run(); assert!(!result.is_success(), "Result was unsuccessful {:?}", result); - - assert!( - result.stderr() == "error: Invalid value for '--number ': must be at least equal to 1.\n", - "Unexpected stderr : {}", - result.stderr() - ); + assert!(result.stdout() == ""); } #[test] -fn migration_revert_with_an_invalid_input_should_throw_an_error() { +fn migration_revert_n_with_a_string_should_throw_an_error() { let p = project("migration_revert_with_an_invalid_input_should_throw_an_error") .folder("migrations") .build(); @@ -288,15 +297,15 @@ fn migration_revert_with_an_invalid_input_should_throw_an_error() { assert!(!result.is_success(), "Result was unsuccessful {:?}", result); assert!( - result.stderr() == "error: Invalid value for '--number ': Cannot parse . The input must be an integer or 'all'.\n", + result.stderr() == "error: Invalid value for '--number ': infinite isn't a positive integer.\n", "Unexpected stderr : {}", result.stderr() ); } #[test] -fn migration_revert_with_more_than_the_number_of_migrations_should_run_all_the_migrations_down() { - let p = project("migration_revert_with_more_than_the_number_of_migrations_should_run_all_the_migrations_down") +fn migration_revert_with_more_than_max_should_revert_all() { + let p = project("migration_revert_with_more_than_max_should_revert_all") .folder("migrations") .build(); let db = database(&p.database_url()); @@ -337,6 +346,7 @@ fn migration_revert_with_more_than_the_number_of_migrations_should_run_all_the_m assert!(result.is_success(), "Result was unsuccessful {:?}", result); + #[cfg(feature = "postgres")] assert!( result.stdout() == "Rolling back migration 2017-09-12-210424_create_bills\n\ @@ -347,6 +357,17 @@ fn migration_revert_with_more_than_the_number_of_migrations_should_run_all_the_m result.stdout() ); + // MySQL and SQLite databases don't call the default migration 00000000000000_diesel_initial_setup. + #[cfg(feature = "mysql,sqlite")] + assert!( + result.stdout() + == "Rolling back migration 2017-09-12-210424_create_bills\n\ + Rolling back migration 2017-09-03-210424_create_contracts\n\ + Rolling back migration 2017-08-31-210424_create_customers\n", + "Unexpected stdout : {}", + result.stdout() + ); + assert!(!db.table_exists("customers")); assert!(!db.table_exists("contracts")); assert!(!db.table_exists("bills")); diff --git a/diesel_migrations/migrations_internals/src/connection.rs b/diesel_migrations/migrations_internals/src/connection.rs index d1cef23177eb..20f679d80d83 100644 --- a/diesel_migrations/migrations_internals/src/connection.rs +++ b/diesel_migrations/migrations_internals/src/connection.rs @@ -19,7 +19,7 @@ use super::schema::__diesel_schema_migrations::dsl::*; pub trait MigrationConnection: diesel::migration::MigrationConnection { fn previously_run_migration_versions(&self) -> QueryResult>; fn latest_run_migration_version(&self) -> QueryResult>; - fn latest_run_migration_versions(&self, number: i64) -> QueryResult>; + fn latest_run_migration_versions(&self, number: u64) -> QueryResult>; fn insert_new_migration(&self, version: &str) -> QueryResult<()>; } @@ -54,11 +54,11 @@ where __diesel_schema_migrations.select(max(version)).first(self) } - fn latest_run_migration_versions(&self, number: i64) -> QueryResult> { + fn latest_run_migration_versions(&self, number: u64) -> QueryResult> { __diesel_schema_migrations .select(version) .order(version.desc()) - .limit(number) + .limit(number as i64) .load(self) } diff --git a/diesel_migrations/migrations_internals/src/lib.rs b/diesel_migrations/migrations_internals/src/lib.rs index 600565e4a172..e0e003da2766 100644 --- a/diesel_migrations/migrations_internals/src/lib.rs +++ b/diesel_migrations/migrations_internals/src/lib.rs @@ -207,31 +207,24 @@ pub fn revert_all_migrations_in_directory( where Conn: MigrationConnection, { - let number = migrations_in_directory(path)?.len() as i64; + let number = migrations_in_directory(path)?.len() as u64; revert_latest_migrations_in_directory(conn, path, number) } -// TODO : see if we need a function named revert_latest_migrations -// to call this one. pub fn revert_latest_migrations_in_directory( conn: &Conn, path: &Path, - number: i64, + number: u64, ) -> Result, RunMigrationsError> where Conn: MigrationConnection, { setup_database(conn)?; - let latest_migration_versions = match conn.latest_run_migration_versions(number) { - Ok(migration_versions) => migration_versions, - Err(_) => { - return Err(RunMigrationsError::MigrationError( - MigrationError::NoMigrationRun, - )) - } - }; + let latest_migration_versions = conn + .latest_run_migration_versions(number) + .map_err(|_| RunMigrationsError::MigrationError(MigrationError::NoMigrationRun))?; if latest_migration_versions.is_empty() { return Err(RunMigrationsError::MigrationError( @@ -240,10 +233,7 @@ where } for migration_version in &latest_migration_versions { - // TODO see how to handle the errors... even if these errors - // are displayed on stdout. - revert_migration_with_version(conn, path, &migration_version, &mut stdout()) - .unwrap_or_else(|_| ()); + revert_migration_with_version(conn, path, &migration_version, &mut stdout())?; } Ok(latest_migration_versions)