Add new arguments to the revert subcommand

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::<i64>().map_err(|_| {
-        "Cannot parse <REVERT_NUMBER>. The input must be an integer or 'all'.".to_string()
-    })?;
-
-    if number < 1 {
-        return Err("<REVERT_NUMBER> must be at least equal to 1.".to_string());
-    }
-
-    Ok(())
-}

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<dyn Error>> {
             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::<i64>() {
-                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::<u64>().unwrap()
+                    )
+                )?;
             }
 
             regenerate_schema_if_file_specified(matches)?;

diesel_cli/src/validators/mod.rs

@@ -0,0 +1 @@
+pub(crate) mod num;

diesel_cli/src/validators/num.rs

@@ -0,0 +1,41 @@
+pub fn is_positive_int(val: String) -> Result<(), String> {
+    match val.parse::<u64>() {
+        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())
+        )
+    }
+}

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 <REVERT_NUMBER>': <REVERT_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 <REVERT_NUMBER>': Cannot parse <REVERT_NUMBER>. The input must be an integer or 'all'.\n",
+        result.stderr() == "error: Invalid value for '--number <REVERT_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"));

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<HashSet<String>>;
     fn latest_run_migration_version(&self) -> QueryResult<Option<String>>;
-    fn latest_run_migration_versions(&self, number: i64) -> QueryResult<Vec<String>>;
+    fn latest_run_migration_versions(&self, number: u64) -> QueryResult<Vec<String>>;
     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<Vec<String>> {
+    fn latest_run_migration_versions(&self, number: u64) -> QueryResult<Vec<String>> {
         __diesel_schema_migrations
             .select(version)
             .order(version.desc())
-            .limit(number)
+            .limit(number as i64)
             .load(self)
     }
 

diesel_migrations/migrations_internals/src/lib.rs

@@ -207,31 +207,24 @@ pub fn revert_all_migrations_in_directory<Conn>(
 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: &Conn,
     path: &Path,
-    number: i64,
+    number: u64,
 ) -> Result<Vec<String>, 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)