Allow migrations to have attached metadata, avoid running in transaction

On PostgreSQL, certain commands cannot be run inside of a transaction,
such as `CREATE INDEX CONCURRENTLY` and `ALTER TYPE`. This causes
problems when attempting to use Diesel migrations, since we always run
these migrations inside a transaction.

To allow opting out of this, we introduce the concept of migration
metadata. I've opted to keep this incredibly general, rather than adding
a specific `should_run_in_migration` function, as there's been interest
expressed in having this capability to enable alternate migration
runners.

Since we need to retain object safety, we can't actually have any
defined structure for the metadata. The best we can do is either
something that is conceptually `Map<String, String>`, or an enum like
`serde_json::Value` or `toml::Value`. I really don't want to restrict
what can or cannot be in here, so I've opted for the more general
option.

Author: sgrif

CHANGELOG.md

@@ -23,6 +23,9 @@ for Rust libraries in [RFC #1105](https://github.com/rust-lang/rfcs/blob/master/
 * Added `sqlite-bundled` feature to `diesel_cli` to make installing on
   some platforms easier.
 
+* Migrations can now opt out of being wrapped in a transaction by adding
+  `metadata.toml` to their directory with `run_in_transaction = false`.
+
 ### Changed
 
 * `sql_function!` has been redesigned. The syntax is now `sql_function!(fn

diesel/src/migration/errors.rs

@@ -14,15 +14,22 @@ use result;
 pub enum MigrationError {
     /// The migration directory wasn't found
     MigrationDirectoryNotFound,
+
     /// Provided migration was in an unknown format
     UnknownMigrationFormat(PathBuf),
+
     /// General system IO error
     IoError(io::Error),
+
     /// Provided migration had an incompatible version number
     UnknownMigrationVersion(String),
+
     /// No migrations had to be/ could be run
     NoMigrationRun,
-    ///
+
+    /// The metadatafile for a given migration was invalid
+    InvalidMetadata(Box<Error>),
+
     #[doc(hidden)]
     __NonExhaustive,
 }
@@ -33,20 +40,41 @@ impl Error for MigrationError {
             MigrationError::MigrationDirectoryNotFound => {
                 "Unable to find migrations directory in this directory or any parent directories."
             }
+
             MigrationError::UnknownMigrationFormat(_) => {
                 "Invalid migration directory, the directory's name should be \
                  <timestamp>_<name_of_migration>, and it should only contain up.sql and down.sql."
             }
+
             MigrationError::IoError(ref error) => error.description(),
+
             MigrationError::UnknownMigrationVersion(_) => {
                 "Unable to find migration version to revert in the migrations directory."
             }
+
             MigrationError::NoMigrationRun => {
                 "No migrations have been run. Did you forget `diesel migration run`?"
             }
+
+            MigrationError::InvalidMetadata(_) => "Migration metadata was invalid",
+
             MigrationError::__NonExhaustive => unreachable!(),
         }
     }
+
+    fn cause(&self) -> Option<&Error> {
+        match *self {
+            MigrationError::IoError(ref err) => Some(err),
+
+            MigrationError::InvalidMetadata(ref err) => Some(&**err),
+
+            MigrationError::MigrationDirectoryNotFound
+            | MigrationError::UnknownMigrationFormat(_)
+            | MigrationError::UnknownMigrationVersion(_)
+            | MigrationError::NoMigrationRun
+            | MigrationError::__NonExhaustive => None,
+        }
+    }
 }
 
 impl fmt::Display for MigrationError {
@@ -83,11 +111,13 @@ impl From<io::Error> for MigrationError {
 pub enum RunMigrationsError {
     /// A general migration error occured
     MigrationError(MigrationError),
+
     /// The provided migration included an invalid query
     QueryError(result::Error),
+
     /// The provided migration was empty
     EmptyMigration,
-    ///
+
     #[doc(hidden)]
     __NonExhaustive,
 }

diesel/src/migration/mod.rs

@@ -5,19 +5,28 @@ pub use self::errors::{MigrationError, RunMigrationsError};
 
 use connection::SimpleConnection;
 use std::path::Path;
+use std::borrow::Cow;
 
 /// Represents a migration that interacts with diesel
 pub trait Migration {
     /// Get the migration version
     fn version(&self) -> &str;
+
     /// Apply this migration
     fn run(&self, conn: &SimpleConnection) -> Result<(), RunMigrationsError>;
+
     /// Revert this migration
     fn revert(&self, conn: &SimpleConnection) -> Result<(), RunMigrationsError>;
+
     /// Get the migration file path
     fn file_path(&self) -> Option<&Path> {
         None
     }
+
+    /// Get the metadata associated with this migration, if any
+    fn metadata(&self) -> Option<&Metadata> {
+        None
+    }
 }
 
 impl Migration for Box<Migration> {
@@ -32,9 +41,14 @@ impl Migration for Box<Migration> {
     fn revert(&self, conn: &SimpleConnection) -> Result<(), RunMigrationsError> {
         (&**self).revert(conn)
     }
+
     fn file_path(&self) -> Option<&Path> {
         (&**self).file_path()
     }
+
+    fn metadata(&self) -> Option<&Metadata> {
+        (&**self).metadata()
+    }
 }
 
 impl<'a> Migration for &'a Migration {
@@ -49,7 +63,27 @@ impl<'a> Migration for &'a Migration {
     fn revert(&self, conn: &SimpleConnection) -> Result<(), RunMigrationsError> {
         (&**self).revert(conn)
     }
+
     fn file_path(&self) -> Option<&Path> {
         (&**self).file_path()
     }
+
+    fn metadata(&self) -> Option<&Metadata> {
+        (&**self).metadata()
+    }
+}
+
+/// Represents metadata associated with a migration.
+///
+/// The format of a migration's metadata is dependent on the migration format
+/// being used.
+///
+/// For Diesel's built in SQL file migrations, metadata is stored in a file
+/// called `metadata.toml`. Diesel looks for a single key, `run_in_transaction`.
+/// By default, all migrations are run in a transaction on SQLite and
+/// PostgreSQL. This behavior can be disabled for a single migration by setting
+/// this to `false`.
+pub trait Metadata {
+    /// Get the metadata at the given key, if present
+    fn get(&self, key: &str) -> Option<Cow<str>>;
 }

diesel_cli/tests/migration_run.rs

@@ -346,3 +346,41 @@ file = "src/my_schema.rs"
     );
     assert!(p.has_file("src/my_schema.rs"));
 }
+
+#[test]
+#[cfg(feature = "postgres")]
+fn migrations_can_be_run_without_transactions() {
+    let p = project("migration_run_without_transaction").build();
+    let db = database(&p.database_url());
+
+    // Make sure the project is setup
+    p.command("setup").run();
+
+    p.create_migration(
+        "12345_create_users_table",
+        "CREATE TABLE users (id INTEGER PRIMARY KEY, name TEXT NOT NULL);",
+        "DROP TABLE users",
+    );
+
+    // CREATE INDEX CONCURRENTLY cannot be run inside a transaction.
+    // It also can't be run in a multiline statement, so it needs its own
+    // migration.
+    p.create_migration(
+        "12346_index_users_table",
+        "CREATE UNIQUE INDEX CONCURRENTLY idx ON users (name)",
+        "DROP INDEX idx",
+    );
+    p.add_migration_metadata("12346_index_users_table", "run_in_transaction = false");
+
+    assert!(!db.table_exists("users"));
+
+    let result = p.command("migration").arg("run").run();
+
+    assert!(result.is_success(), "Result was unsuccessful {:?}", result);
+    assert!(
+        result.stdout().contains("Running migration 12346"),
+        "Unexpected stdout {}",
+        result.stdout()
+    );
+    assert!(db.table_exists("users"));
+}

diesel_cli/tests/support/project_builder.rs

@@ -166,6 +166,17 @@ impl Project {
         let mut down_file = fs::File::create(&migration_path.join("down.sql")).unwrap();
         down_file.write_all(down.as_bytes()).unwrap();
     }
+
+    #[cfg(feature = "postgres")]
+    pub fn add_migration_metadata(&self, name: &str, metadata: &str) {
+        use std::io::Write;
+
+        let migration_path = self.directory.path().join("migrations").join(name);
+        let mut file = fs::File::create(&migration_path.join("metadata.toml"))
+            .expect("Could not create migration metadata file");
+        file.write_all(metadata.as_bytes())
+            .expect("Could not write to migration metadata file");
+    }
 }
 
 #[cfg(not(feature = "sqlite"))]

diesel_migrations/migrations_internals/Cargo.toml

@@ -10,6 +10,7 @@ homepage = "http://diesel.rs"
 clippy = { optional = true, version = "=0.0.195" }
 diesel = { version = "~1.2.0", default-features = false }
 barrel = { version = "<= 0.2.0", optional = true, features = ["diesel-filled"] }
+toml = "0.4.6"
 
 [dev-dependencies]
 tempdir = "0.3.4"

diesel_migrations/migrations_internals/src/lib.rs

@@ -76,6 +76,7 @@
 //! ```
 #[macro_use]
 extern crate diesel;
+extern crate toml;
 
 #[cfg(feature = "barrel")]
 extern crate barrel;
@@ -320,14 +321,33 @@ fn run_migration<Conn>(
 where
     Conn: MigrationConnection,
 {
-    conn.transaction(|| {
+    let mut run_migration = || {
         if migration.version() != "00000000000000" {
             try!(writeln!(output, "Running migration {}", name(&migration)));
         }
         try!(migration.run(conn));
         try!(conn.insert_new_migration(migration.version()));
         Ok(())
-    })
+    };
+
+    let run_in_transaction = migration
+        .metadata()
+        .and_then(|m| m.get("run_in_transaction"));
+    let run_in_transaction = match run_in_transaction.as_ref().map(|s| s.as_ref()) {
+        Some("true") => true,
+        Some("false") | None => false,
+        Some(_) => {
+            return Err(MigrationError::InvalidMetadata(
+                "run_in_transaction must be a boolean".into(),
+            ).into())
+        }
+    };
+
+    if run_in_transaction {
+        conn.transaction(run_migration)
+    } else {
+        run_migration().map_err(Into::into)
+    }
 }
 
 fn revert_migration<Conn: Connection>(

diesel_migrations/migrations_internals/src/migration.rs

@@ -1,8 +1,12 @@
 use diesel::connection::SimpleConnection;
 use diesel::migration::*;
 
-use std::path::{Path, PathBuf};
+use std::borrow::Cow;
 use std::fmt;
+use std::fs::File;
+use std::io::Read;
+use std::path::{Path, PathBuf};
+use toml;
 
 #[allow(missing_debug_implementations)]
 #[derive(Clone, Copy)]
@@ -38,7 +42,7 @@ pub fn migration_from(path: PathBuf) -> Result<Box<Migration>, MigrationError> {
 
     if valid_sql_migration_directory(&path) {
         let version = try!(version_from_path(&path));
-        Ok(Box::new(SqlFileMigration(path, version)))
+        SqlFileMigration::new(path, version).map(|m| Box::new(m) as _)
     } else {
         Err(MigrationError::UnknownMigrationFormat(path))
     }
@@ -82,26 +86,53 @@ pub fn version_from_path(path: &Path) -> Result<String, MigrationError> {
         .unwrap_or_else(|| Err(MigrationError::UnknownMigrationFormat(path.to_path_buf())))
 }
 
-use std::fs::File;
-use std::io::Read;
+struct SqlFileMigration {
+    directory: PathBuf,
+    version: String,
+    metadata: Option<TomlMetadata>,
+}
 
-struct SqlFileMigration(PathBuf, String);
+impl SqlFileMigration {
+    fn new(directory: PathBuf, version: String) -> Result<Self, MigrationError> {
+        let metadata_path = directory.join("metadata.toml");
+        let metadata = if metadata_path.exists() {
+            let mut buf = Vec::new();
+            let mut file = File::open(metadata_path)?;
+            file.read_to_end(&mut buf)?;
+            let value =
+                toml::from_slice(&buf).map_err(|e| MigrationError::InvalidMetadata(e.into()))?;
+            Some(TomlMetadata(value))
+        } else {
+            None
+        };
+
+        Ok(Self {
+            directory,
+            version,
+            metadata,
+        })
+    }
+}
 
 impl Migration for SqlFileMigration {
     fn file_path(&self) -> Option<&Path> {
-        Some(self.0.as_path())
+        Some(&self.directory)
     }
 
     fn version(&self) -> &str {
-        &self.1
+        &self.version
     }
 
     fn run(&self, conn: &SimpleConnection) -> Result<(), RunMigrationsError> {
-        run_sql_from_file(conn, &self.0.join("up.sql"))
+        run_sql_from_file(conn, &self.directory.join("up.sql"))
     }
 
     fn revert(&self, conn: &SimpleConnection) -> Result<(), RunMigrationsError> {
-        run_sql_from_file(conn, &self.0.join("down.sql"))
+        run_sql_from_file(conn, &self.directory.join("down.sql"))
+    }
+
+    fn metadata(&self) -> Option<&Metadata> {
+        self.metadata.as_ref().map(|m| m as _)
     }
 }
 
@@ -118,6 +149,18 @@ fn run_sql_from_file(conn: &SimpleConnection, path: &Path) -> Result<(), RunMigr
     Ok(())
 }
 
+struct TomlMetadata(toml::Value);
+
+impl Metadata for TomlMetadata {
+    fn get(&self, key: &str) -> Option<Cow<str>> {
+        self.0.get(key).map(|v| {
+            v.as_str()
+                .map(Into::into)
+                .unwrap_or_else(|| v.to_string().into())
+        })
+    }
+}
+
 #[cfg(test)]
 mod tests {
     extern crate tempdir;