Better handling of multiple migrations (#1747)

* add migration tests that catch the bug

* make multiple migrations more robust

* clean up migrations ordering

* update comment [ci skip]

* remove explicit iter

util/src/migration/mod.rs

@@ -180,12 +180,11 @@ impl Manager {
 
 	/// Adds new migration rules.
 	pub fn add_migration<T>(&mut self, migration: T) -> Result<(), Error> where T: Migration {
-		let version_match = match self.migrations.last() {
-			Some(last) => last.version() + 1 == migration.version(),
+		let is_new = match self.migrations.last() {
+			Some(last) => migration.version() > last.version(),
 			None => true,
 		};
-
-		match version_match {
+		match is_new {
 			true => Ok(self.migrations.push(Box::new(migration))),
 			false => Err(Error::CannotAddMigration),
 		}
@@ -195,7 +194,8 @@ impl Manager {
 	/// and producing a path where the final migration lives.
 	pub fn execute(&mut self, old_path: &Path, version: u32) -> Result<PathBuf, Error> {
 		let config = self.config.clone();
-		let migrations = try!(self.migrations_from(version).ok_or(Error::MigrationImpossible));
+		let mut migrations = self.migrations_from(version);
+		if migrations.is_empty() { return Err(Error::MigrationImpossible) };
 		let db_config = DatabaseConfig {
 			max_open_files: 64,
 			cache_size: None,
@@ -235,10 +235,9 @@ impl Manager {
 		}
 	}
 
-	fn migrations_from(&mut self, version: u32) -> Option<&mut [Box<Migration>]> {
-		// index of the first required migration
-		let position = self.migrations.iter().position(|m| m.version() == version + 1);
-		position.map(move |p| &mut self.migrations[p..])
+	/// Find all needed migrations.
+	fn migrations_from(&mut self, version: u32) -> Vec<&mut Box<Migration>> {
+		self.migrations.iter_mut().filter(|m| m.version() > version).collect()
 	}
 }
 

util/src/migration/tests.rs

@@ -109,6 +109,18 @@ fn no_migration_needed() {
 	manager.execute(&db_path, 1).unwrap();
 }
 
+#[test]
+#[should_panic]
+fn wrong_adding_order() {
+	let dir = RandomTempPath::create_dir();
+	let db_path = db_path(dir.as_path());
+	let mut manager = Manager::new(Config::default());
+	make_db(&db_path, map![vec![] => vec![], vec![1] => vec![1]]);
+
+	manager.add_migration(Migration1).unwrap();
+	manager.add_migration(Migration0).unwrap();
+}
+
 #[test]
 fn multiple_migrations() {
 	let dir = RandomTempPath::create_dir();
@@ -139,6 +151,34 @@ fn second_migration() {
 	verify_migration(&end_path, expected);
 }
 
+#[test]
+fn first_and_noop_migration() {
+	let dir = RandomTempPath::create_dir();
+	let db_path = db_path(dir.as_path());
+	let mut manager = Manager::new(Config::default());
+	make_db(&db_path, map![vec![] => vec![], vec![1] => vec![1]]);
+	let expected = map![vec![0x11] => vec![0x22], vec![1, 0x11] => vec![1, 0x22]];
+
+	manager.add_migration(Migration0).unwrap();
+	let end_path = manager.execute(&db_path, 0).unwrap();
+
+	verify_migration(&end_path, expected);
+}
+
+#[test]
+fn noop_and_second_migration() {
+	let dir = RandomTempPath::create_dir();
+	let db_path = db_path(dir.as_path());
+	let mut manager = Manager::new(Config::default());
+	make_db(&db_path, map![vec![] => vec![], vec![1] => vec![1]]);
+	let expected = map![vec![] => vec![], vec![1] => vec![]];
+
+	manager.add_migration(Migration1).unwrap();
+	let end_path = manager.execute(&db_path, 0).unwrap();
+
+	verify_migration(&end_path, expected);
+}
+
 #[test]
 fn is_migration_needed() {
 	let mut manager = Manager::new(Config::default());