Repository: vincent-herlemont/native_db (language: rust)

Issue:
Migration fails when another model with a different id exists
The following change to the migrate test illustrates the issue.
With the additional model it throws an error:
```
Impossible to migrate the table 1_2_generate_my_primary_key because the table 1_1_generate_my_primary_key has data
```

```diff
diff --git a/tests/migrate/only_primary_key.rs b/tests/migrate/only_primary_key.rs
index 36695d4..33c83b6 100644
--- a/tests/migrate/only_primary_key.rs
+++ b/tests/migrate/only_primary_key.rs
@@ -51,13 +51,27 @@ impl ItemV2 {
     }
 }
 
+#[derive(Serialize, Deserialize, Eq, PartialEq, Debug, Clone)]
+#[native_model(id = 2, version = 1)]
+#[native_db]
+struct Item2 {
+    #[primary_key]
+    id: u32,
+}
+
 #[test]
 fn test_migrate() {
     let tf = TmpFs::new().unwrap();
     let mut builder = DatabaseBuilder::new();
     builder.define::<ItemV1>().unwrap();
+    builder.define::<Item2>().unwrap();
     let db = builder.create(tf.path("test").as_std_path()).unwrap();
 
+    let item_2 = Item2 { id: 1 };
+    let rw_txn = db.rw_transaction().unwrap();
+    rw_txn.insert(item_2).unwrap();
+    rw_txn.commit().unwrap();
+
     let item = ItemV1 {
         id: 1,
         name: "test".to_string(),
@@ -83,6 +97,7 @@ fn test_migrate() {
     let mut builder = DatabaseBuilder::new();
     builder.define::<ItemV1>().unwrap();
     builder.define::<ItemV2>().unwrap();
+    builder.define::<Item2>().unwrap();
     let db = builder.create(tf.path("test").as_std_path()).unwrap();
 
     let rw = db.rw_transaction().unwrap();
 ``` 


Discussion / hints:

Fix patch:
diff --git a/src/transaction/internal/rw_transaction.rs b/src/transaction/internal/rw_transaction.rs
index 04906a8..8b7d032 100644
--- a/src/transaction/internal/rw_transaction.rs
+++ b/src/transaction/internal/rw_transaction.rs
@@ -232,9 +232,14 @@ impl<'db> InternalRwTransaction<'db> {
             ));
         }
 
-        // Check which table have the data
-        let mut old_table_definition = None;
-        for new_primary_table_definition in self.primary_table_definitions.values() {
+        let mut old_table_definition: Option<&PrimaryTableDefinition> = None;
+        let model_table_definitions = self.primary_table_definitions.values().filter(|t| {
+            t.native_model_options.native_model_id
+                == new_table_definition.native_model_options.native_model_id
+        });
+
+        // Find the old model table with data
+        for new_primary_table_definition in model_table_definitions {
             // check if table exists, if the table does not exist continue
             if self
                 .redb_transaction
@@ -251,9 +256,10 @@ impl<'db> InternalRwTransaction<'db> {
             let len = table.len()?;
             if len > 0 && old_table_definition.is_some() {
                 panic!(
-                    "Impossible to migrate the table {} because the table {} has data",
+                    "Impossible to migrate the table {} because multiple old tables with data exist: {}, {}",
                     T::native_db_model().primary_key.unique_table_name,
-                    new_primary_table_definition.redb.name()
+                    new_primary_table_definition.redb.name(),
+                    old_table_definition.unwrap().redb.name()
                 );
             } else if table.len()? > 0 {
                 old_table_definition = Some(new_primary_table_definition);


Test patch:
diff --git a/tests/migrate/mod.rs b/tests/migrate/mod.rs
index 248136a..8ae129f 100644
--- a/tests/migrate/mod.rs
+++ b/tests/migrate/mod.rs
@@ -1,2 +1,4 @@
 mod only_primary_key;
+mod with_multiple_versions;
+mod with_other_model;
 mod with_secondary_keys;
diff --git a/tests/migrate/with_multiple_versions.rs b/tests/migrate/with_multiple_versions.rs
new file mode 100644
index 0000000..7963c87
--- /dev/null
+++ b/tests/migrate/with_multiple_versions.rs
@@ -0,0 +1,182 @@
+use native_db::*;
+use native_model::{native_model, Model};
+use serde::{Deserialize, Serialize};
+use shortcut_assert_fs::TmpFs;
+
+#[derive(Serialize, Deserialize, Eq, PartialEq, Debug)]
+#[native_model(id = 1, version = 1)]
+#[native_db]
+struct ItemV1 {
+    #[primary_key]
+    id: u32,
+    name: String,
+}
+
+#[derive(Serialize, Deserialize, Eq, PartialEq, Debug)]
+#[native_model(id = 1, version = 2, from = ItemV1)]
+#[native_db]
+struct ItemV2 {
+    #[primary_key]
+    id: u32,
+    name_v2: String,
+}
+
+#[derive(Serialize, Deserialize, Eq, PartialEq, Debug)]
+#[native_model(id = 1, version = 3, from = ItemV2)]
+#[native_db]
+struct ItemV3 {
+    #[primary_key]
+    id: u32,
+    name_v3: String,
+}
+
+impl From<ItemV1> for ItemV2 {
+    fn from(item: ItemV1) -> Self {
+        ItemV2 {
+            id: item.id,
+            name_v2: item.name,
+        }
+    }
+}
+
+impl From<ItemV2> for ItemV1 {
+    fn from(item: ItemV2) -> Self {
+        ItemV1 {
+            id: item.id,
+            name: item.name_v2,
+        }
+    }
+}
+
+impl From<ItemV2> for ItemV3 {
+    fn from(item: ItemV2) -> Self {
+        ItemV3 {
+            id: item.id,
+            name_v3: item.name_v2,
+        }
+    }
+}
+
+impl From<ItemV3> for ItemV2 {
+    fn from(item: ItemV3) -> Self {
+        ItemV2 {
+            id: item.id,
+            name_v2: item.name_v3,
+        }
+    }
+}
+
+#[test]
+fn test_skip_version() {
+    let tf = TmpFs::new().unwrap();
+    let mut builder = DatabaseBuilder::new();
+    builder.define::<ItemV1>().unwrap();
+    let db = builder.create(tf.path("test").as_std_path()).unwrap();
+
+    let item = ItemV1 {
+        id: 1,
+        name: "test".to_string(),
+    };
+
+    let rw_txn = db.rw_transaction().unwrap();
+    rw_txn.insert(item).unwrap();
+    rw_txn.commit().unwrap();
+
+    let r_txn = db.r_transaction().unwrap();
+
+    let item: ItemV1 = r_txn.get().primary(1).unwrap().unwrap();
+    assert_eq!(
+        item,
+        ItemV1 {
+            id: 1,
+            name: "test".to_string(),
+        }
+    );
+    drop(r_txn);
+    drop(db);
+
+    let mut builder = DatabaseBuilder::new();
+    builder.define::<ItemV1>().unwrap();
+    builder.define::<ItemV2>().unwrap();
+    builder.define::<ItemV3>().unwrap();
+    let db = builder.create(tf.path("test").as_std_path()).unwrap();
+
+    let rw = db.rw_transaction().unwrap();
+    rw.migrate::<ItemV3>().unwrap();
+    rw.commit().unwrap();
+
+    let r_txn = db.r_transaction().unwrap();
+    let item: ItemV3 = r_txn.get().primary(1).unwrap().unwrap();
+    assert_eq!(
+        item,
+        ItemV3 {
+            id: 1,
+            name_v3: "test".to_string(),
+        }
+    );
+
+    let stats = db.redb_stats().unwrap();
+    assert_eq!(stats.primary_tables.len(), 3);
+    assert_eq!(stats.primary_tables[0].name, "1_1_id");
+    assert_eq!(stats.primary_tables[0].n_entries, Some(0));
+    assert_eq!(stats.primary_tables[1].name, "1_2_id");
+    assert_eq!(stats.primary_tables[1].n_entries, Some(0));
+    assert_eq!(stats.primary_tables[2].name, "1_3_id");
+    assert_eq!(stats.primary_tables[2].n_entries, Some(1));
+    assert_eq!(stats.secondary_tables.len(), 0);
+}
+
+#[test]
+#[should_panic]
+fn test_skip_version_with_data_should_fail() {
+    let tf = TmpFs::new().unwrap();
+    let mut builder = DatabaseBuilder::new();
+    builder.define::<ItemV1>().unwrap();
+    builder.define::<ItemV2>().unwrap();
+    let db = builder.create(tf.path("test").as_std_path()).unwrap();
+
+    let item = ItemV1 {
+        id: 1,
+        name: "test".to_string(),
+    };
+    let item_v2 = ItemV2 {
+        id: 1,
+        name_v2: "test".to_string(),
+    };
+
+    let rw_txn = db.rw_transaction().unwrap();
+    rw_txn.insert(item).unwrap();
+    rw_txn.insert(item_v2).unwrap();
+    rw_txn.commit().unwrap();
+
+    let r_txn = db.r_transaction().unwrap();
+    let item: ItemV1 = r_txn.get().primary(1).unwrap().unwrap();
+    assert_eq!(
+        item,
+        ItemV1 {
+            id: 1,
+            name: "test".to_string(),
+        }
+    );
+
+    let item: ItemV2 = r_txn.get().primary(1).unwrap().unwrap();
+    assert_eq!(
+        item,
+        ItemV2 {
+            id: 1,
+            name_v2: "test".to_string(),
+        }
+    );
+
+    drop(r_txn);
+    drop(db);
+
+    let mut builder = DatabaseBuilder::new();
+    builder.define::<ItemV1>().unwrap();
+    builder.define::<ItemV2>().unwrap();
+    builder.define::<ItemV3>().unwrap();
+    let db = builder.create(tf.path("test").as_std_path()).unwrap();
+
+    let rw = db.rw_transaction().unwrap();
+    let _ = rw.migrate::<ItemV3>();
+}
diff --git a/tests/migrate/with_other_model.rs b/tests/migrate/with_other_model.rs
new file mode 100644
index 0000000..7f573be
--- /dev/null
+++ b/tests/migrate/with_other_model.rs
@@ -0,0 +1,127 @@
+use native_db::*;
+use native_model::{native_model, Model};
+use serde::{Deserialize, Serialize};
+use shortcut_assert_fs::TmpFs;
+
+#[derive(Serialize, Deserialize, Eq, PartialEq, Debug)]
+#[native_model(id = 1, version = 1)]
+#[native_db]
+struct ItemV1 {
+    #[primary_key]
+    id: u32,
+    name: String,
+}
+
+#[derive(Serialize, Deserialize, Eq, PartialEq, Debug)]
+#[native_model(id = 1, version = 2, from = ItemV1)]
+#[native_db]
+struct ItemV2 {
+    #[primary_key]
+    id: u32,
+    name_v2: String,
+}
+
+impl From<ItemV1> for ItemV2 {
+    fn from(item: ItemV1) -> Self {
+        ItemV2 {
+            id: item.id,
+            name_v2: item.name,
+        }
+    }
+}
+
+impl From<ItemV2> for ItemV1 {
+    fn from(item: ItemV2) -> Self {
+        ItemV1 {
+            id: item.id,
+            name: item.name_v2,
+        }
+    }
+}
+
+#[derive(Serialize, Deserialize, Eq, PartialEq, Debug)]
+#[native_model(id = 2, version = 1)]
+#[native_db]
+struct Item2 {
+    #[primary_key]
+    id: u32,
+    name2: String,
+}
+
+#[test]
+fn test_migrate() {
+    let tf = TmpFs::new().unwrap();
+    let mut builder = DatabaseBuilder::new();
+    builder.define::<ItemV1>().unwrap();
+    builder.define::<Item2>().unwrap();
+    let db = builder.create(tf.path("test").as_std_path()).unwrap();
+
+    let item_2 = Item2 {
+        id: 1,
+        name2: "test2".to_string(),
+    };
+    let rw_txn = db.rw_transaction().unwrap();
+    rw_txn.insert(item_2).unwrap();
+    rw_txn.commit().unwrap();
+
+    let item = ItemV1 {
+        id: 1,
+        name: "test".to_string(),
+    };
+
+    let rw_txn = db.rw_transaction().unwrap();
+    rw_txn.insert(item).unwrap();
+    rw_txn.commit().unwrap();
+
+    let r_txn = db.r_transaction().unwrap();
+
+    let item: ItemV1 = r_txn.get().primary(1).unwrap().unwrap();
+    assert_eq!(
+        item,
+        ItemV1 {
+            id: 1,
+            name: "test".to_string(),
+        }
+    );
+    drop(r_txn);
+    drop(db);
+
+    let mut builder = DatabaseBuilder::new();
+    builder.define::<ItemV1>().unwrap();
+    builder.define::<ItemV2>().unwrap();
+    builder.define::<Item2>().unwrap();
+    let db = builder.create(tf.path("test").as_std_path()).unwrap();
+
+    let rw = db.rw_transaction().unwrap();
+    rw.migrate::<ItemV2>().unwrap();
+    rw.commit().unwrap();
+
+    let r_txn = db.r_transaction().unwrap();
+    let item: ItemV2 = r_txn.get().primary(1).unwrap().unwrap();
+    assert_eq!(
+        item,
+        ItemV2 {
+            id: 1,
+            name_v2: "test".to_string(),
+        }
+    );
+
+    let item: Item2 = r_txn.get().primary(1).unwrap().unwrap();
+    assert_eq!(
+        item,
+        Item2 {
+            id: 1,
+            name2: "test2".to_string(),
+        }
+    );
+
+    let stats = db.redb_stats().unwrap();
+    assert_eq!(stats.primary_tables.len(), 3);
+    assert_eq!(stats.primary_tables[0].name, "1_1_id");
+    assert_eq!(stats.primary_tables[0].n_entries, Some(0));
+    assert_eq!(stats.primary_tables[1].name, "1_2_id");
+    assert_eq!(stats.primary_tables[1].n_entries, Some(1));
+    assert_eq!(stats.primary_tables[2].name, "2_1_id");
+    assert_eq!(stats.primary_tables[2].n_entries, Some(1));
+    assert_eq!(stats.secondary_tables.len(), 0);
+}