Extend migration framework (#1546)

* partially done alternate migration scheme

* finish altering migration framework

* migrate tests to new migration framework

* address comments

* remove superfluous newline
[ci skip]

* TempIdx -> TempIndex
[ci skip]

ethcore/src/migrations/extras/v6.rs

@@ -1,4 +1,4 @@
-use util::migration::Migration;
+use util::migration::SimpleMigration;
 
 /// This migration reduces the sizes of keys and moves `ExtrasIndex` byte from back to the front.
 pub struct ToV6;
@@ -17,7 +17,7 @@ impl ToV6 {
 	}
 }
 
-impl Migration for ToV6 {
+impl SimpleMigration for ToV6 {
 	fn version(&self) -> u32 {
 		6
 	}

parity/migration.rs

@@ -19,8 +19,7 @@ use std::fs::File;
 use std::io::{Read, Write, Error as IoError, ErrorKind};
 use std::path::PathBuf;
 use std::fmt::{Display, Formatter, Error as FmtError};
-use util::migration::{Manager as MigrationManager, Config as MigrationConfig, MigrationIterator};
+use util::migration::{Manager as MigrationManager, Config as MigrationConfig, Error as MigrationError};
-use util::kvdb::{Database, DatabaseConfig, CompactionProfile};
 use ethcore::migrations;
 
 /// Database is assumed to be at default version, when no version file is found.
@@ -65,6 +64,15 @@ impl From<IoError> for Error {
 	}
 }
 
+impl From<MigrationError> for Error {
+	fn from(err: MigrationError) -> Self {
+		match err {
+			MigrationError::Io(e) => Error::Io(e),
+			_ => Error::MigrationFailed,
+		}
+	}
+}
+
 /// Returns the version file path.
 fn version_file_path(path: &PathBuf) -> PathBuf {
 	let mut file_path = path.clone();
@@ -109,14 +117,6 @@ fn extras_database_path(path: &PathBuf) -> PathBuf {
 	extras_path
 }
 
-/// Temporary database path used for migration.
-fn temp_database_path(path: &PathBuf) -> PathBuf {
-	let mut temp_path = path.clone();
-	temp_path.pop();
-	temp_path.push("temp_migration");
-	temp_path
-}
-
 /// Database backup
 fn backup_database_path(path: &PathBuf) -> PathBuf {
 	let mut backup_path = path.clone();
@@ -146,44 +146,27 @@ fn extras_database_migrations() -> Result<MigrationManager, Error> {
 }
 
 /// Migrates database at given position with given migration rules.
-fn migrate_database(version: u32, path: PathBuf, migrations: MigrationManager) -> Result<(), Error> {
+fn migrate_database(version: u32, db_path: PathBuf, migrations: MigrationManager) -> Result<(), Error> {
 	// check if migration is needed
 	if !migrations.is_needed(version) {
 		return Ok(())
 	}
 
-	let temp_path = temp_database_path(&path);
+	let backup_path = backup_database_path(&db_path);
-	let backup_path = backup_database_path(&path);
+	// remove the backup dir if it exists
-	// remote the dir if it exists
-	let _ = fs::remove_dir_all(&temp_path);
 	let _ = fs::remove_dir_all(&backup_path);
 
-	{
-		let db_config = DatabaseConfig {
-			prefix_size: None,
-			max_open_files: 64,
-			cache_size: None,
-			compaction: CompactionProfile::default(),
-		};
-
-		// open old database
-		let old = try!(Database::open(&db_config, path.to_str().unwrap()).map_err(|_| Error::MigrationFailed));
-
-		// create new database
-		let mut temp = try!(Database::open(&db_config, temp_path.to_str().unwrap()).map_err(|_| Error::MigrationFailed));
-
 	// migrate old database to the new one
-		try!(migrations.execute(MigrationIterator::from(old.iter()), version, &mut temp).map_err(|_| Error::MigrationFailed));
+	let temp_path = try!(migrations.execute(&db_path, version));
-	}
 
 	// create backup
-	try!(fs::rename(&path, &backup_path));
+	try!(fs::rename(&db_path, &backup_path));
 
 	// replace the old database with the new one
-	if let Err(err) = fs::rename(&temp_path, &path) {
+	if let Err(err) = fs::rename(&temp_path, &db_path) {
 		// if something went wrong, bring back backup
-		try!(fs::rename(&backup_path, path));
+		try!(fs::rename(&backup_path, &db_path));
-		return Err(From::from(err));
+		return Err(err.into());
 	}
 
 	// remove backup

util/src/migration/db_impl.rs

@@ -1,55 +0,0 @@
-// Copyright 2015, 2016 Ethcore (UK) Ltd.
-// This file is part of Parity.
-
-// Parity is free software: you can redistribute it and/or modify
-// it under the terms of the GNU General Public License as published by
-// the Free Software Foundation, either version 3 of the License, or
-// (at your option) any later version.
-
-// Parity is distributed in the hope that it will be useful,
-// but WITHOUT ANY WARRANTY; without even the implied warranty of
-// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
-// GNU General Public License for more details.
-
-// You should have received a copy of the GNU General Public License
-// along with Parity.  If not, see <http://www.gnu.org/licenses/>.
-
-//! `kvdb::Database` as `migration::Destination`
-
-use std::collections::BTreeMap;
-use kvdb::{Database, DatabaseIterator, DBTransaction};
-use migration::{Destination, Error};
-
-/// Database iterator with `Item` complient with migration `Manager` interface.
-pub struct MigrationIterator {
-	iter: DatabaseIterator,
-}
-
-impl From<DatabaseIterator> for MigrationIterator {
-	fn from(iter: DatabaseIterator) -> Self {
-		MigrationIterator {
-			iter: iter
-		}
-	}
-}
-
-impl Iterator for MigrationIterator {
-	type Item = (Vec<u8>, Vec<u8>);
-
-	fn next(&mut self) -> Option<Self::Item> {
-		self.iter.next().map(|(k, v)| (k.to_vec(), v.to_vec()))
-	}
-}
-
-impl Destination for Database {
-	fn commit(&mut self, batch: BTreeMap<Vec<u8>, Vec<u8>>) -> Result<(), Error> {
-		let transaction = DBTransaction::new();
-
-		for keypair in &batch {
-			try!(transaction.put(&keypair.0, &keypair.1).map_err(Error::Custom))
-		}
-
-		self.write(transaction).map_err(Error::Custom)
-	}
-}
-

util/src/migration/manager.rs

@@ -1,117 +0,0 @@
-// Copyright 2015, 2016 Ethcore (UK) Ltd.
-// This file is part of Parity.
-
-// Parity is free software: you can redistribute it and/or modify
-// it under the terms of the GNU General Public License as published by
-// the Free Software Foundation, either version 3 of the License, or
-// (at your option) any later version.
-
-// Parity is distributed in the hope that it will be useful,
-// but WITHOUT ANY WARRANTY; without even the implied warranty of
-// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
-// GNU General Public License for more details.
-
-// You should have received a copy of the GNU General Public License
-// along with Parity.  If not, see <http://www.gnu.org/licenses/>.
-
-//! Migration manager
-
-use std::collections::BTreeMap;
-use migration::{Migration, Destination};
-
-/// Migration error.
-#[derive(Debug)]
-pub enum Error {
-	/// Error returned when it is impossible to add new migration rules.
-	CannotAddMigration,
-	/// Error returned when migration from specific version can not be performed.
-	MigrationImpossible,
-	/// Custom error.
-	Custom(String),
-}
-
-/// Migration config.
-pub struct Config {
-	/// Defines how many elements should be migrated at once.
-	pub batch_size: usize,
-}
-
-impl Default for Config {
-	fn default() -> Self {
-		Config {
-			batch_size: 1024,
-		}
-	}
-}
-
-/// Manages database migration.
-pub struct Manager {
-	config: Config,
-	migrations: Vec<Box<Migration>>,
-}
-
-impl Manager {
-	/// Creates new migration manager with given configuration.
-	pub fn new(config: Config) -> Self {
-		Manager {
-			config: config,
-			migrations: vec![]
-		}
-	}
-
-	/// 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(),
-			None => true,
-		};
-
-		match version_match {
-			true => Ok(self.migrations.push(Box::new(migration))),
-			false => Err(Error::CannotAddMigration),
-		}
-	}
-
-	/// Performs migration to destination.
-	pub fn execute<D>(&self, db_iter: D, version: u32, destination: &mut Destination) -> Result<(), Error> where
-		D: Iterator<Item = (Vec<u8>, Vec<u8>)> {
-
-		let migrations = try!(self.migrations_from(version).ok_or(Error::MigrationImpossible));
-
-		let mut batch: BTreeMap<Vec<u8>, Vec<u8>> = BTreeMap::new();
-
-		for keypair in db_iter {
-			let migrated = migrations.iter().fold(Some(keypair), |migrated, migration| {
-				migrated.and_then(|(key, value)| migration.simple_migrate(key, value))
-			});
-
-			if let Some((key, value)) = migrated {
-				batch.insert(key, value);
-			}
-
-			if batch.len() == self.config.batch_size {
-				try!(destination.commit(batch));
-				batch = BTreeMap::new();
-			}
-		}
-
-		try!(destination.commit(batch));
-
-		Ok(())
-	}
-
-	/// Returns true if migration is needed.
-	pub fn is_needed(&self, version: u32) -> bool {
-		match self.migrations.last() {
-			Some(last) => version < last.version(),
-			None => false,
-		}
-	}
-
-	fn migrations_from(&self, version: u32) -> Option<&[Box<Migration>]> {
-		// index of the first required migration
-		let position = self.migrations.iter().position(|m| m.version() == version + 1);
-		position.map(|p| &self.migrations[p..])
-	}
-}
-

util/src/migration/mod.rs

@@ -15,19 +15,58 @@
 // along with Parity.  If not, see <http://www.gnu.org/licenses/>.
 
 //! DB Migration module.
-
-mod db_impl;
-mod manager;
-
 #[cfg(test)]
 mod tests;
 
-pub use self::manager::{Error, Config, Manager};
-pub use self::db_impl::MigrationIterator;
 use std::collections::BTreeMap;
+use std::fs;
+use std::path::{Path, PathBuf};
 
-/// Single migration.
+use ::kvdb::{CompactionProfile, Database, DatabaseConfig, DBTransaction};
+
+/// Migration config.
+pub struct Config {
+	/// Defines how many elements should be migrated at once.
+	pub batch_size: usize,
+}
+
+impl Default for Config {
+	fn default() -> Self {
+		Config {
+			batch_size: 1024,
+		}
+	}
+}
+
+/// Migration error.
+#[derive(Debug)]
+pub enum Error {
+	/// Error returned when it is impossible to add new migration rules.
+	CannotAddMigration,
+	/// Error returned when migration from specific version can not be performed.
+	MigrationImpossible,
+	/// Io Error.
+	Io(::std::io::Error),
+	/// Custom error.
+	Custom(String),
+}
+
+impl From<::std::io::Error> for Error {
+	fn from(e: ::std::io::Error) -> Self {
+		Error::Io(e)
+	}
+}
+
+/// A generalized migration from the given db to a destination db.
 pub trait Migration: 'static {
+	/// Version of the database after the migration.
+	fn version(&self) -> u32;
+	/// Migrate a source to a destination.
+	fn migrate(&self, source: &Database, config: &Config, destination: &mut Database) -> Result<(), Error>;
+}
+
+/// A simple migration over key-value pairs.
+pub trait SimpleMigration: 'static {
 	/// Version of database after the migration.
 	fn version(&self) -> u32;
 	/// Should migrate existing object to new database.
@@ -35,8 +74,152 @@ pub trait Migration: 'static {
 	fn simple_migrate(&self, key: Vec<u8>, value: Vec<u8>) -> Option<(Vec<u8>, Vec<u8>)>;
 }
 
-/// Migration destination.
+impl<T: SimpleMigration> Migration for T {
-pub trait Destination {
+	fn version(&self) -> u32 { SimpleMigration::version(self) }
-	/// Called on destination to commit batch of migrated entries.
+
-	fn commit(&mut self, batch: BTreeMap<Vec<u8>, Vec<u8>>) -> Result<(), Error>;
+	fn migrate(&self, source: &Database, config: &Config, dest: &mut Database) -> Result<(), Error> {
+		let mut batch: BTreeMap<Vec<u8>, Vec<u8>> = BTreeMap::new();
+
+		for (key, value) in source.iter() {
+
+			if let Some((key, value)) = self.simple_migrate(key.to_vec(), value.to_vec()) {
+				batch.insert(key, value);
 			}
+
+			if batch.len() == config.batch_size {
+				try!(commit_batch(dest, &batch));
+				batch.clear();
+			}
+		}
+
+		if batch.len() != 0 {
+			try!(commit_batch(dest, &batch));
+		}
+
+		Ok(())
+	}
+}
+
+/// Commit a batch of writes to a database.
+pub fn commit_batch(db: &mut Database, batch: &BTreeMap<Vec<u8>, Vec<u8>>) -> Result<(), Error> {
+	let transaction = DBTransaction::new();
+
+	for keypair in batch {
+		try!(transaction.put(&keypair.0, &keypair.1).map_err(Error::Custom));
+	}
+
+	db.write(transaction).map_err(Error::Custom)
+}
+
+/// Get the path where all databases reside.
+fn database_path(path: &Path) -> PathBuf {
+	let mut temp_path = path.to_owned();
+	temp_path.pop();
+	temp_path
+}
+
+enum TempIndex {
+	One,
+	Two,
+}
+
+impl TempIndex {
+	fn swap(&mut self) {
+		match *self {
+			TempIndex::One => *self = TempIndex::Two,
+			TempIndex::Two => *self = TempIndex::One,
+		}
+	}
+
+	// given the path to the old database, get the path of this one.
+	fn path(&self, db_root: &Path) -> PathBuf {
+		let mut buf = db_root.to_owned();
+
+		match *self {
+			TempIndex::One => buf.push("temp_migration_1"),
+			TempIndex::Two => buf.push("temp_migration_2"),
+		};
+
+		buf
+	}
+}
+
+/// Manages database migration.
+pub struct Manager {
+	config: Config,
+	migrations: Vec<Box<Migration>>,
+}
+
+impl Manager {
+	/// Creates new migration manager with given configuration.
+	pub fn new(config: Config) -> Self {
+		Manager {
+			config: config,
+			migrations: vec![],
+		}
+	}
+
+	/// 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(),
+			None => true,
+		};
+
+		match version_match {
+			true => Ok(self.migrations.push(Box::new(migration))),
+			false => Err(Error::CannotAddMigration),
+		}
+	}
+
+	/// Performs migration in order, starting with a source path, migrating between two temporary databases,
+	/// and producing a path where the final migration lives.
+	pub fn execute(&self, old_path: &Path, version: u32) -> Result<PathBuf, Error> {
+		let migrations = try!(self.migrations_from(version).ok_or(Error::MigrationImpossible));
+		let db_config = DatabaseConfig {
+			prefix_size: None,
+			max_open_files: 64,
+			cache_size: None,
+			compaction: CompactionProfile::default(),
+		};
+
+		let db_root = database_path(old_path);
+		let mut temp_idx = TempIndex::One;
+		let mut temp_path = temp_idx.path(&db_root);
+
+		// start with the old db.
+		let old_path_str = try!(old_path.to_str().ok_or(Error::MigrationImpossible));
+		let mut cur_db = try!(Database::open(&db_config, old_path_str).map_err(|s| Error::Custom(s)));
+		for migration in migrations {
+			// open the target temporary database.
+			temp_path = temp_idx.path(&db_root);
+			let temp_path_str = try!(temp_path.to_str().ok_or(Error::MigrationImpossible));
+			let mut new_db = try!(Database::open(&db_config, temp_path_str).map_err(|s| Error::Custom(s)));
+
+			// perform the migration from cur_db to new_db.
+			try!(migration.migrate(&cur_db, &self.config, &mut new_db));
+			// next iteration, we will migrate from this db into the other temp.
+			cur_db = new_db;
+			temp_idx.swap();
+
+			// remove the other temporary migration database.
+			let _ = fs::remove_dir_all(temp_idx.path(&db_root));
+		}
+		Ok(temp_path)
+	}
+
+	/// Returns true if migration is needed.
+	pub fn is_needed(&self, version: u32) -> bool {
+		match self.migrations.last() {
+			Some(last) => version < last.version(),
+			None => false,
+		}
+	}
+
+	fn migrations_from(&self, version: u32) -> Option<&[Box<Migration>]> {
+		// index of the first required migration
+		let position = self.migrations.iter().position(|m| m.version() == version + 1);
+		position.map(|p| &self.migrations[p..])
+	}
+}
+

util/src/migration/tests.rs

@@ -14,19 +14,50 @@
 // You should have received a copy of the GNU General Public License
 // along with Parity.  If not, see <http://www.gnu.org/licenses/>.
 
-use std::collections::BTreeMap;
+//! Tests for migrations.
-use migration::{Error, Destination, Migration, Manager, Config};
+//! A random temp directory is created. A database is created within it, and migrations
+//! are performed in temp sub-directories.
 
-impl Destination for BTreeMap<Vec<u8>, Vec<u8>> {
+use common::*;
-	fn commit(&mut self, batch: BTreeMap<Vec<u8>, Vec<u8>>) -> Result<(), Error> {
+use migration::{Config, SimpleMigration, Manager};
-		self.extend(batch);
+use kvdb::{Database, DBTransaction};
-		Ok(())
+
+use devtools::RandomTempPath;
+use std::path::PathBuf;
+
+fn db_path(path: &Path) -> PathBuf {
+	let mut p = path.to_owned();
+	p.push("db");
+	p
+}
+
+// initialize a database at the given directory with the given values.
+fn make_db(path: &Path, pairs: BTreeMap<Vec<u8>, Vec<u8>>) {
+	let db = Database::open_default(path.to_str().unwrap()).expect("failed to open temp database");
+	{
+		let transaction = DBTransaction::new();
+		for (k, v) in pairs {
+			transaction.put(&k, &v).expect("failed to add pair to transaction");
+		}
+
+		db.write(transaction).expect("failed to write db transaction");
+	}
+}
+
+// helper for verifying a migrated database.
+fn verify_migration(path: &Path, pairs: BTreeMap<Vec<u8>, Vec<u8>>) {
+	let db = Database::open_default(path.to_str().unwrap()).unwrap();
+
+	for (k, v) in pairs {
+		let x = db.get(&k).unwrap().unwrap();
+
+		assert_eq!(&x[..], &v[..]);
 	}
 }
 
 struct Migration0;
 
-impl Migration for Migration0 {
+impl SimpleMigration for Migration0 {
 	fn version(&self) -> u32 {
 		1
 	}
@@ -42,7 +73,7 @@ impl Migration for Migration0 {
 
 struct Migration1;
 
-impl Migration for Migration1 {
+impl SimpleMigration for Migration1 {
 	fn version(&self) -> u32 {
 		2
 	}
@@ -54,68 +85,58 @@ impl Migration for Migration1 {
 
 #[test]
 fn one_simple_migration() {
+	let dir = RandomTempPath::create_dir();
+	let db_path = db_path(dir.as_path());
 	let mut manager = Manager::new(Config::default());
-	let keys = vec![vec![], vec![1u8]];
+	make_db(&db_path, map![vec![] => vec![], vec![1] => vec![1]]);
-	let values = vec![vec![], vec![1u8]];
+	let expected = map![vec![0x11] => vec![0x22], vec![1, 0x11] => vec![1, 0x22]];
-	let db = keys.into_iter().zip(values.into_iter());
 
-	let expected_keys = vec![vec![0x11u8], vec![1, 0x11]];
-	let expected_values = vec![vec![0x22u8], vec![1, 0x22]];
-	let expected_db = expected_keys.into_iter().zip(expected_values.into_iter()).collect::<BTreeMap<_, _>>();
-
-	let mut result = BTreeMap::new();
 	manager.add_migration(Migration0).unwrap();
-	manager.execute(db, 0, &mut result).unwrap();
+	let end_path = manager.execute(&db_path, 0).unwrap();
-	assert_eq!(expected_db, result);
+
+	verify_migration(&end_path, expected);
 }
 
 #[test]
 #[should_panic]
 fn no_migration_needed() {
+	let dir = RandomTempPath::create_dir();
+	let db_path = db_path(dir.as_path());
 	let mut manager = Manager::new(Config::default());
-	let keys = vec![vec![], vec![1u8]];
+	make_db(&db_path, map![vec![] => vec![], vec![1] => vec![1]]);
-	let values = vec![vec![], vec![1u8]];
-	let db = keys.into_iter().zip(values.into_iter());
 
-	let mut result = BTreeMap::new();
 	manager.add_migration(Migration0).unwrap();
-	manager.execute(db, 1, &mut result).unwrap();
+	manager.execute(&db_path, 1).unwrap();
 }
 
 #[test]
 fn multiple_migrations() {
+	let dir = RandomTempPath::create_dir();
+	let db_path = db_path(dir.as_path());
 	let mut manager = Manager::new(Config::default());
-	let keys = vec![vec![], vec![1u8]];
+	make_db(&db_path, map![vec![] => vec![], vec![1] => vec![1]]);
-	let values = vec![vec![], vec![1u8]];
+	let expected = map![vec![0x11] => vec![], vec![1, 0x11] => vec![]];
-	let db = keys.into_iter().zip(values.into_iter());
 
-	let expected_keys = vec![vec![0x11u8], vec![1, 0x11]];
-	let expected_values = vec![vec![], vec![]];
-	let expected_db = expected_keys.into_iter().zip(expected_values.into_iter()).collect::<BTreeMap<_, _>>();
-
-	let mut result = BTreeMap::new();
 	manager.add_migration(Migration0).unwrap();
 	manager.add_migration(Migration1).unwrap();
-	manager.execute(db, 0, &mut result).unwrap();
+	let end_path = manager.execute(&db_path, 0).unwrap();
-	assert_eq!(expected_db, result);
+
+	verify_migration(&end_path, expected);
 }
 
 #[test]
 fn second_migration() {
+	let dir = RandomTempPath::create_dir();
+	let db_path = db_path(dir.as_path());
 	let mut manager = Manager::new(Config::default());
-	let keys = vec![vec![], vec![1u8]];
+	make_db(&db_path, map![vec![] => vec![], vec![1] => vec![1]]);
-	let values = vec![vec![], vec![1u8]];
+	let expected = map![vec![] => vec![], vec![1] => vec![]];
-	let db = keys.into_iter().zip(values.into_iter());
 
-	let expected_keys = vec![vec![], vec![1u8]];
-	let expected_values = vec![vec![], vec![]];
-	let expected_db = expected_keys.into_iter().zip(expected_values.into_iter()).collect::<BTreeMap<_, _>>();
-
-	let mut result = BTreeMap::new();
 	manager.add_migration(Migration0).unwrap();
 	manager.add_migration(Migration1).unwrap();
-	manager.execute(db, 1, &mut result).unwrap();
+	let end_path = manager.execute(&db_path, 1).unwrap();
-	assert_eq!(expected_db, result);
+
+	verify_migration(&end_path, expected);
 }
 
 #[test]