Single transaction (#14)

* style: use default instead equal with option value

* chore: add non_exhaustive for enums

* chore: fix clippy warnings

* refac!: single transaction by default

BREAKING CHANGES: we remove single transaction flag from cli

migra/src/errors.rs

@@ -12,6 +12,7 @@ pub type MigraResult<T> = Result<T, Error>;
 
 /// Migra error
 #[derive(Debug)]
+#[non_exhaustive]
 pub enum Error {
     /// Represents database errors.
     Db(DbError),
@@ -54,6 +55,7 @@ impl Error {
 
 /// All kinds of errors with witch this crate works.
 #[derive(Debug)]
+#[non_exhaustive]
 pub enum DbKind {
     /// Failed to database connection.
     DatabaseConnection,

migra/src/migration.rs

@@ -29,9 +29,6 @@ impl Migration {
 ///
 /// Can be presented as a list of all migrations, a list of pending migrations
 /// or a list of applied migrations, depending on the implementation.
-///
-///
-///
 #[derive(Debug, Clone, Default, PartialEq, Eq)]
 pub struct List {
     inner: Vec<Migration>,
@@ -219,16 +216,16 @@ mod tests {
     fn contains_migration() {
         let list = List::from(vec![FIRST_MIGRATION]);
 
-        assert_eq!(list.contains(&Migration::new(FIRST_MIGRATION)), true);
+        assert!(list.contains(&Migration::new(FIRST_MIGRATION)));
-        assert_eq!(list.contains(&Migration::new(SECOND_MIGRATION)), false);
+        assert!(!list.contains(&Migration::new(SECOND_MIGRATION)));
     }
 
     #[test]
     fn contains_migration_name() {
         let list = List::from(vec![FIRST_MIGRATION]);
 
-        assert_eq!(list.contains_name(FIRST_MIGRATION), true);
+        assert!(list.contains_name(FIRST_MIGRATION));
-        assert_eq!(list.contains_name(SECOND_MIGRATION), false);
+        assert!(!list.contains_name(SECOND_MIGRATION));
     }
 
     #[test]

migra_cli/src/commands/apply.rs

@@ -20,22 +20,12 @@ pub(crate) fn apply_sql(app: &App, cmd_opts: &ApplyCommandOpt) -> migra::StdResu
         .map(std::fs::read_to_string)
         .collect::<Result<Vec<_>, _>>()?;
 
-    database::should_run_in_transaction(
+    database::run_in_transaction(&mut client, |client| {
-        &mut client,
+        file_contents
-        cmd_opts.transaction_opts.single_transaction,
+            .iter()
-        |client| {
+            .try_for_each(|content| client.apply_sql(content))
-            file_contents
+            .map_err(From::from)
-                .iter()
+    })?;
-                .try_for_each(|content| {
-                    database::should_run_in_transaction(
-                        client,
-                        !cmd_opts.transaction_opts.single_transaction,
-                        |client| client.apply_sql(content),
-                    )
-                })
-                .map_err(From::from)
-        },
-    )?;
 
     Ok(())
 }

migra_cli/src/commands/downgrade.rs

@@ -32,27 +32,19 @@ pub(crate) fn rollback_applied_migrations(
         })
         .collect::<Result<Vec<_>, _>>()?;
 
-    database::should_run_in_transaction(
+    database::run_in_transaction(&mut client, |client| {
-        &mut client,
+        migrations_with_content
-        opts.transaction_opts.single_transaction,
+            .iter()
-        |client| {
+            .try_for_each(|(migration_name, content)| {
-            migrations_with_content
+                if all_migrations.contains_name(migration_name) {
-                .iter()
+                    println!("downgrade {}...", migration_name);
-                .try_for_each(|(migration_name, content)| {
+                    client.run_downgrade_migration(migration_name, &content)
-                    if all_migrations.contains_name(migration_name) {
+                } else {
-                        println!("downgrade {}...", migration_name);
+                    Ok(())
-                        database::should_run_in_transaction(
+                }
-                            client,
+            })
-                            !opts.transaction_opts.single_transaction,
+            .map_err(From::from)
-                            |client| client.run_downgrade_migration(migration_name, &content),
+    })?;
-                        )
-                    } else {
-                        Ok(())
-                    }
-                })
-                .map_err(From::from)
-        },
-    )?;
 
     Ok(())
 }

migra_cli/src/commands/upgrade.rs

@@ -52,23 +52,15 @@ pub(crate) fn upgrade_pending_migrations(
         })
         .collect::<Result<Vec<_>, _>>()?;
 
-    database::should_run_in_transaction(
+    database::run_in_transaction(&mut client, |client| {
-        &mut client,
+        migrations_with_content
-        opts.transaction_opts.single_transaction,
+            .iter()
-        |client| {
+            .try_for_each(|(migration_name, content)| {
-            migrations_with_content
+                println!("upgrade {}...", migration_name);
-                .iter()
+                client.run_upgrade_migration(migration_name, &content)
-                .try_for_each(|(migration_name, content)| {
+            })
-                    println!("upgrade {}...", migration_name);
+            .map_err(From::from)
-                    database::should_run_in_transaction(
+    })?;
-                        client,
-                        !opts.transaction_opts.single_transaction,
-                        |client| client.run_upgrade_migration(migration_name, &content),
-                    )
-                })
-                .map_err(From::from)
-        },
-    )?;
 
     Ok(())
 }

migra_cli/src/config.rs

@@ -50,7 +50,7 @@ fn is_sqlite_database_file(filename: &str) -> bool {
         .rsplit('.')
         .next()
         .map(|ext| ext.eq_ignore_ascii_case("db"))
-        == Some(true)
+        .unwrap_or_default()
 }
 
 fn default_database_connection_env() -> String {

migra_cli/src/database.rs

@@ -53,18 +53,3 @@ where
         .and_then(|res| client.commit_transaction().and(Ok(res)))
         .or_else(|err| client.rollback_transaction().and(Err(err)))
 }
-
-pub fn should_run_in_transaction<TrxFnMut>(
-    client: &mut AnyClient,
-    is_needed: bool,
-    trx_fn: TrxFnMut,
-) -> migra::Result<()>
-where
-    TrxFnMut: FnOnce(&mut AnyClient) -> migra::Result<()>,
-{
-    if is_needed {
-        run_in_transaction(client, trx_fn)
-    } else {
-        trx_fn(client)
-    }
-}

migra_cli/src/opts.rs

@@ -32,19 +32,10 @@ pub(crate) enum Command {
     Completions(CompletionsShell),
 }
 
-#[derive(Debug, StructOpt, Clone)]
-pub(crate) struct TransactionOpts {
-    #[structopt(long = "single-transaction")]
-    pub single_transaction: bool,
-}
-
 #[derive(Debug, StructOpt, Clone)]
 pub(crate) struct ApplyCommandOpt {
     #[structopt(parse(from_os_str), required = true)]
     pub file_paths: Vec<PathBuf>,
-
-    #[structopt(flatten)]
-    pub transaction_opts: TransactionOpts,
 }
 
 #[derive(Debug, StructOpt, Clone)]
@@ -64,9 +55,6 @@ pub(crate) struct UpgradeCommandOpt {
     /// How many existing migrations do we have to update.
     #[structopt(long = "number", short = "n")]
     pub migrations_number: Option<usize>,
-
-    #[structopt(flatten)]
-    pub transaction_opts: TransactionOpts,
 }
 
 #[derive(Debug, StructOpt, Clone)]
@@ -78,9 +66,6 @@ pub(crate) struct DowngradeCommandOpt {
     /// Rolls back all applied migrations. Ignores --number option.
     #[structopt(long = "all")]
     pub all_migrations: bool,
-
-    #[structopt(flatten)]
-    pub transaction_opts: TransactionOpts,
 }
 
 #[derive(Debug, StructOpt, Clone)]

migra_cli/tests/commands.rs

@@ -485,62 +485,6 @@ mod upgrade {
         Ok(())
     }
 
-    #[test]
-    fn partial_applied_invalid_migrations() -> TestResult {
-        fn inner<ValidateFn>(database_name: &'static str, validate: ValidateFn) -> TestResult
-        where
-            ValidateFn: Fn() -> TestResult,
-        {
-            let manifest_path = database_manifest_path(database_name);
-
-            Command::cargo_bin("migra")?
-                .arg("-c")
-                .arg(&manifest_path)
-                .arg("up")
-                .assert()
-                .failure();
-
-            validate()?;
-
-            Command::cargo_bin("migra")?
-                .arg("-c")
-                .arg(&manifest_path)
-                .arg("down")
-                .assert()
-                .success();
-
-            Ok(())
-        }
-
-        #[cfg(feature = "postgres")]
-        inner("postgres_invalid", || {
-            let mut conn = client_postgres::Client::connect(POSTGRES_URL, client_postgres::NoTls)?;
-            let articles_res = conn.query("SELECT a.id FROM articles AS a", &[]);
-            let persons_res = conn.query("SELECT p.id FROM persons AS p", &[]);
-
-            assert!(articles_res.is_ok());
-            assert!(persons_res.is_err());
-
-            Ok(())
-        })?;
-
-        #[cfg(feature = "sqlite")]
-        remove_sqlite_db().and_then(|_| {
-            inner("sqlite_invalid", || {
-                let conn = client_rusqlite::Connection::open(SQLITE_URL)?;
-                let articles_res = conn.execute_batch("SELECT a.id FROM articles AS a");
-                let persons_res = conn.execute_batch("SELECT p.id FROM persons AS p");
-
-                assert!(articles_res.is_ok());
-                assert!(persons_res.is_err());
-
-                Ok(())
-            })
-        })?;
-
-        Ok(())
-    }
-
     #[test]
     fn cannot_applied_invalid_migrations_in_single_transaction() -> TestResult {
         fn inner<ValidateFn>(database_name: &'static str, validate: ValidateFn) -> TestResult
@@ -588,6 +532,8 @@ mod upgrade {
             })
         })?;
 
+        // mysql doesn't support DDL in transaction 🤷
+
         Ok(())
     }
 }