Make migration methods and error handling less surprising

Signed-off-by: Victor Porof <victor.porof@gmail.com>

src/migrator.rs

@@ -18,6 +18,19 @@
 //!
 //! The destination environment should be empty of data, otherwise an error is returned.
 //!
+//! There are 3 versions of the migration methods:
+//! * `migrate_<src>_to_<dst>`, where `<src>` and `<dst>` are the source and destination
+//!   environment types. You're responsive with opening both these environments, handling
+//!   all errors, and performing any cleanup if necessary.
+//! * `open_and_migrate_<src>_to_<dst>`, which is similar the the above, but automatically
+//!   attempts to open the source environment and delete all of its supporting files if
+//!   there's no other environment open at that path. You're still responsible with
+//!   handling all errors.
+//! * `easy_migrate_<src>_to_<dst>` which is similar to the above, but ignores the
+//!   migration and doesn't delete any files if the source environment is invalid
+//!   (corrupted), unavailable (path not found or incompatible with configuration), or
+//!   empty (database has no records).
+//!
 //! The tool currently has these limitations:
 //!
 //! 1. It doesn't support migration from environments created with
@@ -86,10 +99,10 @@ macro_rules! fn_migrator {
         }
     };
 
-    ($migrate:tt, $name:tt, $builder:tt, $src_env:ty, $dst_env:ty) => {
-        /// Same as the other migration methods, but automatically attempts to open the source
-        /// environment, ignores it if it's invalid, if it doesn't exist or if it's empty.
-        /// Finally, attempts to delete all of its supporting files.
+    (open $migrate:tt, $name:tt, $builder:tt, $src_env:ty, $dst_env:ty) => {
+        /// Same as the non `open_*` migration method, but automatically attempts to open the
+        /// source environment. Finally, deletes all of its supporting files if there's no other
+        /// environment open at that path.
         pub fn $name<F, D>(path: &std::path::Path, build: F, dst_env: D) -> Result<(), MigrateError>
         where
             F: FnOnce(crate::backend::$builder) -> crate::backend::$builder,
@@ -102,17 +115,8 @@ macro_rules! fn_migrator {
             builder.set_max_dbs(crate::env::DEFAULT_MAX_DBS);
             builder = build(builder);
 
-            let src_env = match manager.get_or_create_from_builder(path, builder, Rkv::from_builder::<$builder>) {
-                Err(crate::StoreError::FileInvalid) => return Ok(()),
-                Err(crate::StoreError::IoError(ref e)) if e.kind() == std::io::ErrorKind::NotFound => return Ok(()),
-                Err(crate::StoreError::UnsuitableEnvironmentPath(_)) => return Ok(()),
-                result => result,
-            }?;
-
-            match Migrator::$migrate(src_env.read()?, dst_env) {
-                Err(crate::MigrateError::SourceEmpty) => return Ok(()),
-                result => result,
-            }?;
+            let src_env = manager.get_or_create_from_builder(path, builder, Rkv::from_builder::<$builder>)?;
+            Migrator::$migrate(src_env.read()?, dst_env)?;
 
             drop(src_env);
             manager.try_close_and_delete(path)?;
@@ -120,6 +124,25 @@ macro_rules! fn_migrator {
             Ok(())
         }
     };
+
+    (easy $migrate:tt, $name:tt, $src_env:ty, $dst_env:ty) => {
+        /// Same as the `open_*` migration method, but ignores the migration and doesn't delete
+        /// any files if the source environment is invalid (corrupted), unavailable, or empty.
+        pub fn $name<D>(path: &std::path::Path, dst_env: D) -> Result<(), MigrateError>
+        where
+            D: std::ops::Deref<Target = Rkv<$dst_env>>,
+        {
+            match Migrator::$migrate(path, |builder| builder, dst_env) {
+                Err(crate::MigrateError::StoreError(crate::StoreError::FileInvalid)) => Ok(()),
+                Err(crate::MigrateError::StoreError(crate::StoreError::IoError(_))) => Ok(()),
+                Err(crate::MigrateError::StoreError(crate::StoreError::UnsuitableEnvironmentPath(_))) => Ok(()),
+                Err(crate::MigrateError::SourceEmpty) => Ok(()),
+                result => result,
+            }?;
+
+            Ok(())
+        }
+    };
 }
 
 macro_rules! fns_migrator {
@@ -132,7 +155,8 @@ macro_rules! fns_migrator {
     ($name:tt, $src:tt, $dst:tt) => {
         paste::item! {
             fn_migrator!($name, [<$src:camel Environment>], [<$dst:camel Environment>]);
-            fn_migrator!($name, [<auto_ $name>], [<$src:camel>], [<$src:camel Environment>], [<$dst:camel Environment>]);
+            fn_migrator!(open $name, [<open_and_ $name>], [<$src:camel>], [<$src:camel Environment>], [<$dst:camel Environment>]);
+            fn_migrator!(easy [<open_and_ $name>], [<easy_ $name>], [<$src:camel Environment>], [<$dst:camel Environment>]);
         }
     };
 }

tests/env-migration.rs

@@ -66,10 +66,10 @@ fn test_simple_migrator_lmdb_to_safe() {
         assert_eq!(store.get(&reader, "bar").expect("read"), Some(Value::Bool(true)));
         assert_eq!(store.get(&reader, "baz").expect("read"), Some(Value::Str("héllo, yöu")));
     }
-    // Easy migrate.
+    // Open and migrate.
     {
         let dst_env = Rkv::new::<SafeMode>(root.path()).expect("new succeeded");
-        Migrator::auto_migrate_lmdb_to_safe_mode(root.path(), |builder| builder, &dst_env).expect("migrated");
+        Migrator::open_and_migrate_lmdb_to_safe_mode(root.path(), |builder| builder, &dst_env).expect("migrated");
     }
     // Verify that the database was indeed migrated.
     {
@@ -117,10 +117,10 @@ fn test_simple_migrator_safe_to_lmdb() {
         assert_eq!(store.get(&reader, "bar").expect("read"), Some(Value::Bool(true)));
         assert_eq!(store.get(&reader, "baz").expect("read"), Some(Value::Str("héllo, yöu")));
     }
-    // Easy migrate.
+    // Open and migrate.
     {
         let dst_env = Rkv::new::<Lmdb>(root.path()).expect("new succeeded");
-        Migrator::auto_migrate_safe_mode_to_lmdb(root.path(), |builder| builder, &dst_env).expect("migrated");
+        Migrator::open_and_migrate_safe_mode_to_lmdb(root.path(), |builder| builder, &dst_env).expect("migrated");
     }
     // Verify that the database was indeed migrated.
     {
@@ -150,15 +150,15 @@ fn test_migrator_round_trip() {
         populate_store!(&src_env);
         src_env.sync(true).expect("synced");
     }
-    // Easy migrate.
+    // Open and migrate.
     {
         let dst_env = Rkv::new::<SafeMode>(root.path()).expect("new succeeded");
-        Migrator::auto_migrate_lmdb_to_safe_mode(root.path(), |builder| builder, &dst_env).expect("migrated");
+        Migrator::open_and_migrate_lmdb_to_safe_mode(root.path(), |builder| builder, &dst_env).expect("migrated");
     }
-    // Easy migrate back.
+    // Open and migrate back.
     {
         let dst_env = Rkv::new::<Lmdb>(root.path()).expect("new succeeded");
-        Migrator::auto_migrate_safe_mode_to_lmdb(root.path(), |builder| builder, &dst_env).expect("migrated");
+        Migrator::open_and_migrate_safe_mode_to_lmdb(root.path(), |builder| builder, &dst_env).expect("migrated");
     }
     // Verify that the database was indeed migrated twice.
     {
@@ -188,8 +188,10 @@ fn test_migrator_no_dir_1() {
     let root = Builder::new().prefix("test_migrator_no_dir").tempdir().expect("tempdir");
     fs::create_dir_all(root.path()).expect("dir created");
 
+    // This won't fail with IoError even though the path is a bogus path, because this
+    // is the "easy mode" migration which automatically handles (ignores) this error.
     let dst_env = Rkv::new::<SafeMode>(root.path()).expect("new succeeded");
-    Migrator::auto_migrate_lmdb_to_safe_mode(Path::new("bogus"), |builder| builder, &dst_env).expect("migrated");
+    Migrator::easy_migrate_lmdb_to_safe_mode(Path::new("bogus"), &dst_env).expect("migrated");
 
     let mut datamdb = root.path().to_path_buf();
     let mut lockmdb = root.path().to_path_buf();
@@ -207,8 +209,10 @@ fn test_migrator_no_dir_2() {
     let root = Builder::new().prefix("test_migrator_no_dir").tempdir().expect("tempdir");
     fs::create_dir_all(root.path()).expect("dir created");
 
+    // This won't fail with IoError even though the path is a bogus path, because this
+    // is the "easy mode" migration which automatically handles (ignores) this error.
     let dst_env = Rkv::new::<Lmdb>(root.path()).expect("new succeeded");
-    Migrator::auto_migrate_safe_mode_to_lmdb(Path::new("bogus"), |builder| builder, &dst_env).expect("migrated");
+    Migrator::easy_migrate_safe_mode_to_lmdb(Path::new("bogus"), &dst_env).expect("migrated");
 
     let mut datamdb = root.path().to_path_buf();
     let mut lockmdb = root.path().to_path_buf();
@@ -232,7 +236,7 @@ fn test_migrator_invalid_1() {
     // This won't fail with FileInvalid even though the database is a bogus file, because this
     // is the "easy mode" migration which automatically handles (ignores) this error.
     let dst_env = Rkv::new::<SafeMode>(root.path()).expect("new succeeded");
-    Migrator::auto_migrate_lmdb_to_safe_mode(root.path(), |builder| builder, &dst_env).expect("migrated");
+    Migrator::easy_migrate_lmdb_to_safe_mode(root.path(), &dst_env).expect("migrated");
 
     let mut datamdb = root.path().to_path_buf();
     let mut lockmdb = root.path().to_path_buf();
@@ -256,7 +260,7 @@ fn test_migrator_invalid_2() {
     // This won't fail with FileInvalid even though the database is a bogus file, because this
     // is the "easy mode" migration which automatically handles (ignores) this error.
     let dst_env = Rkv::new::<Lmdb>(root.path()).expect("new succeeded");
-    Migrator::auto_migrate_safe_mode_to_lmdb(root.path(), |builder| builder, &dst_env).expect("migrated");
+    Migrator::easy_migrate_safe_mode_to_lmdb(root.path(), &dst_env).expect("migrated");
 
     let mut datamdb = root.path().to_path_buf();
     let mut lockmdb = root.path().to_path_buf();