Make easy migrator more reslient against situations where old files don't get deleted

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

src/migrator.rs

@@ -28,8 +28,8 @@
 //!   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).
+//!   (corrupted), unavailable (path not accessible or incompatible with configuration),
+//!   or empty (database has no records).
 //!
 //! The tool currently has these limitations:
 //!
@@ -100,9 +100,9 @@ macro_rules! fn_migrator {
     };
 
     (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.
+        /// Same as the the `migrate_x_to_y` migration method above, 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 and the migration succeeded.
         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,
@@ -126,8 +126,13 @@ macro_rules! fn_migrator {
     };
 
     (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.
+        /// Same as the `open_and_migrate_x_to_y` migration method above, but ignores the
+        /// migration and doesn't delete any files if the following conditions apply:
+        /// - Source environment is invalid (corrupted), unavailable, or empty.
+        /// - Destination environment is not empty.
+        /// Use this instead of the other migration methods if:
+        /// - You're not concerned by throwing away old data and starting fresh with a new store.
+        /// - You'll never want to overwrite data in the new store from the old store.
         pub fn $name<D>(path: &std::path::Path, dst_env: D) -> Result<(), MigrateError>
         where
             D: std::ops::Deref<Target = Rkv<$dst_env>>,
@@ -137,6 +142,7 @@ macro_rules! fn_migrator {
                 Err(crate::MigrateError::StoreError(crate::StoreError::IoError(_))) => Ok(()),
                 Err(crate::MigrateError::StoreError(crate::StoreError::UnsuitableEnvironmentPath(_))) => Ok(()),
                 Err(crate::MigrateError::SourceEmpty) => Ok(()),
+                Err(crate::MigrateError::DestinationNotEmpty) => Ok(()),
                 result => result,
             }?;
 

tests/env-migration.rs

@@ -18,8 +18,11 @@ use tempfile::Builder;
 use rkv::{
     backend::{
         Lmdb,
+        LmdbEnvironment,
         SafeMode,
+        SafeModeEnvironment,
     },
+    Manager,
     Migrator,
     Rkv,
     StoreOptions,
@@ -38,8 +41,8 @@ macro_rules! populate_store {
 }
 
 #[test]
-fn test_simple_migrator_lmdb_to_safe() {
-    let root = Builder::new().prefix("test_simple_migrator_lmdb_to_safe").tempdir().expect("tempdir");
+fn test_open_migrator_lmdb_to_safe() {
+    let root = Builder::new().prefix("test_open_migrator_lmdb_to_safe").tempdir().expect("tempdir");
     fs::create_dir_all(root.path()).expect("dir created");
 
     // Populate source environment and persist to disk.
@@ -92,8 +95,8 @@ fn test_simple_migrator_lmdb_to_safe() {
 }
 
 #[test]
-fn test_simple_migrator_safe_to_lmdb() {
-    let root = Builder::new().prefix("test_simple_migrator_safe_to_lmdb").tempdir().expect("tempdir");
+fn test_open_migrator_safe_to_lmdb() {
+    let root = Builder::new().prefix("test_open_migrator_safe_to_lmdb").tempdir().expect("tempdir");
     fs::create_dir_all(root.path()).expect("dir created");
 
     // Populate source environment and persist to disk.
@@ -140,8 +143,8 @@ fn test_simple_migrator_safe_to_lmdb() {
 }
 
 #[test]
-fn test_migrator_round_trip() {
-    let root = Builder::new().prefix("test_simple_migrator_lmdb_to_safe").tempdir().expect("tempdir");
+fn test_open_migrator_round_trip() {
+    let root = Builder::new().prefix("test_open_migrator_lmdb_to_safe").tempdir().expect("tempdir");
     fs::create_dir_all(root.path()).expect("dir created");
 
     // Populate source environment and persist to disk.
@@ -184,8 +187,8 @@ fn test_migrator_round_trip() {
 }
 
 #[test]
-fn test_migrator_no_dir_1() {
-    let root = Builder::new().prefix("test_migrator_no_dir").tempdir().expect("tempdir");
+fn test_easy_migrator_no_dir_1() {
+    let root = Builder::new().prefix("test_easy_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
@@ -205,8 +208,8 @@ fn test_migrator_no_dir_1() {
 }
 
 #[test]
-fn test_migrator_no_dir_2() {
-    let root = Builder::new().prefix("test_migrator_no_dir").tempdir().expect("tempdir");
+fn test_easy_migrator_no_dir_2() {
+    let root = Builder::new().prefix("test_easy_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
@@ -226,8 +229,8 @@ fn test_migrator_no_dir_2() {
 }
 
 #[test]
-fn test_migrator_invalid_1() {
-    let root = Builder::new().prefix("test_migrator_invalid").tempdir().expect("tempdir");
+fn test_easy_migrator_invalid_1() {
+    let root = Builder::new().prefix("test_easy_migrator_invalid").tempdir().expect("tempdir");
     fs::create_dir_all(root.path()).expect("dir created");
 
     let dbfile = root.path().join("data.mdb");
@@ -250,8 +253,8 @@ fn test_migrator_invalid_1() {
 }
 
 #[test]
-fn test_migrator_invalid_2() {
-    let root = Builder::new().prefix("test_migrator_invalid").tempdir().expect("tempdir");
+fn test_easy_migrator_invalid_2() {
+    let root = Builder::new().prefix("test_easy_migrator_invalid").tempdir().expect("tempdir");
     fs::create_dir_all(root.path()).expect("dir created");
 
     let dbfile = root.path().join("data.safe.bin");
@@ -354,3 +357,113 @@ fn test_migrator_safe_to_lmdb_3() {
     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")));
 }
+
+#[test]
+fn test_easy_migrator_failed_migration_1() {
+    let root = Builder::new().prefix("test_easy_migrator_failed_migration_1").tempdir().expect("tempdir");
+    fs::create_dir_all(root.path()).expect("dir created");
+
+    let dbfile = root.path().join("data.mdb");
+    fs::write(&dbfile, "bogus").expect("bogus dbfile created");
+
+    // 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::easy_migrate_lmdb_to_safe_mode(root.path(), &dst_env).expect("migrated");
+
+    // Populate destination environment and persist to disk.
+    populate_store!(&dst_env);
+    dst_env.sync(true).expect("synced");
+
+    // Delete bogus file and create a valid source environment in its place.
+    fs::remove_file(&dbfile).expect("bogus dbfile removed");
+    let src_env = Rkv::new::<Lmdb>(root.path()).expect("new succeeded");
+    populate_store!(&src_env);
+    src_env.sync(true).expect("synced");
+
+    // Attempt to migrate again. This should *NOT* fail with DestinationNotEmpty.
+    Migrator::easy_migrate_lmdb_to_safe_mode(root.path(), &dst_env).expect("migrated");
+}
+
+#[test]
+fn test_easy_migrator_failed_migration_2() {
+    let root = Builder::new().prefix("test_easy_migrator_failed_migration_2").tempdir().expect("tempdir");
+    fs::create_dir_all(root.path()).expect("dir created");
+
+    let dbfile = root.path().join("data.safe.bin");
+    fs::write(&dbfile, "bogus").expect("bogus dbfile created");
+
+    // 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::easy_migrate_safe_mode_to_lmdb(root.path(), &dst_env).expect("migrated");
+
+    // Populate destination environment and persist to disk.
+    populate_store!(&dst_env);
+    dst_env.sync(true).expect("synced");
+
+    // Delete bogus file and create a valid source environment in its place.
+    fs::remove_file(&dbfile).expect("bogus dbfile removed");
+    let src_env = Rkv::new::<SafeMode>(root.path()).expect("new succeeded");
+    populate_store!(&src_env);
+    src_env.sync(true).expect("synced");
+
+    // Attempt to migrate again. This should *NOT* fail with DestinationNotEmpty.
+    Migrator::easy_migrate_safe_mode_to_lmdb(root.path(), &dst_env).expect("migrated");
+}
+
+fn test_easy_migrator_from_manager_failed_migration_1() {
+    let root = Builder::new().prefix("test_easy_migrator_from_manager_failed_migration_1").tempdir().expect("tempdir");
+    fs::create_dir_all(root.path()).expect("dir created");
+
+    {
+        let mut src_manager = Manager::<LmdbEnvironment>::singleton().write().unwrap();
+        let created_src_arc = src_manager.get_or_create(root.path(), Rkv::new::<Lmdb>).unwrap();
+        let src_env = created_src_arc.read().unwrap();
+        populate_store!(&src_env);
+        src_env.sync(true).expect("synced");
+    }
+    {
+        let mut dst_manager = Manager::<SafeModeEnvironment>::singleton().write().unwrap();
+        let created_dst_arc_1 = dst_manager.get_or_create(root.path(), Rkv::new::<SafeMode>).unwrap();
+        let dst_env_1 = created_dst_arc_1.read().unwrap();
+        populate_store!(&dst_env_1);
+    }
+
+    // Attempt to migrate again in a new env. This should *NOT* fail with DestinationNotEmpty.
+    let dst_manager = Manager::<SafeModeEnvironment>::singleton().read().unwrap();
+    let created_dst_arc_2 = dst_manager.get(root.path()).unwrap().unwrap();
+    let dst_env_2 = created_dst_arc_2.read().unwrap();
+    Migrator::easy_migrate_lmdb_to_safe_mode(root.path(), dst_env_2).expect("migrated");
+}
+
+fn test_easy_migrator_from_manager_failed_migration_2() {
+    let root = Builder::new().prefix("test_easy_migrator_from_manager_failed_migration_2").tempdir().expect("tempdir");
+    fs::create_dir_all(root.path()).expect("dir created");
+
+    {
+        let mut src_manager = Manager::<SafeModeEnvironment>::singleton().write().unwrap();
+        let created_src_arc = src_manager.get_or_create(root.path(), Rkv::new::<SafeMode>).unwrap();
+        let src_env = created_src_arc.read().unwrap();
+        populate_store!(&src_env);
+        src_env.sync(true).expect("synced");
+    }
+    {
+        let mut dst_manager = Manager::<LmdbEnvironment>::singleton().write().unwrap();
+        let created_dst_arc_1 = dst_manager.get_or_create(root.path(), Rkv::new::<Lmdb>).unwrap();
+        let dst_env_1 = created_dst_arc_1.read().unwrap();
+        populate_store!(&dst_env_1);
+    }
+
+    // Attempt to migrate again in a new env. This should *NOT* fail with DestinationNotEmpty.
+    let dst_manager = Manager::<LmdbEnvironment>::singleton().read().unwrap();
+    let created_dst_arc_2 = dst_manager.get(root.path()).unwrap().unwrap();
+    let dst_env_2 = created_dst_arc_2.read().unwrap();
+    Migrator::easy_migrate_safe_mode_to_lmdb(root.path(), dst_env_2).expect("migrated");
+}
+
+#[test]
+fn test_easy_migrator_from_manager_failed_migration() {
+    test_easy_migrator_from_manager_failed_migration_1();
+    test_easy_migrator_from_manager_failed_migration_2();
+}