Fix deck importing edge cases

Test those edge cases, and add some global test helpers.

rslib/src/decks/addupdate.rs

@@ -148,7 +148,7 @@ impl Collection {
         Ok(())
     }
 
-    fn first_existing_parent(
+    pub(crate) fn first_existing_parent(
         &self,
         machine_name: &str,
         recursion_level: usize,

rslib/src/import_export/package/apkg/import/decks.rs

@@ -1,23 +1,17 @@
 // Copyright: Ankitects Pty Ltd and contributors
 // License: GNU AGPL, version 3 or later; http://www.gnu.org/licenses/agpl.html
 
-use std::{
+use std::{collections::HashMap, mem};
-    collections::{HashMap, HashSet},
-    mem,
-};
 
 use super::Context;
-use crate::{
+use crate::{decks::NormalDeck, prelude::*};
-    decks::{immediate_parent_name, NormalDeck},
-    prelude::*,
-};
 
 struct DeckContext<'d> {
     target_col: &'d mut Collection,
     usn: Usn,
-    seen_parents: HashSet<String>,
     renamed_parents: Vec<(String, String)>,
     imported_decks: HashMap<DeckId, DeckId>,
+    unique_suffix: String,
 }
 
 impl<'d> DeckContext<'d> {
@@ -25,9 +19,9 @@ impl<'d> DeckContext<'d> {
         Self {
             target_col,
             usn,
-            seen_parents: HashSet::new(),
             renamed_parents: Vec::new(),
             imported_decks: HashMap::new(),
+            unique_suffix: TimestampSecs::now().to_string(),
         }
     }
 }
@@ -54,27 +48,30 @@ impl DeckContext<'_> {
         // ensure parents are seen before children
         decks.sort_unstable_by(|d1, d2| d1.name.as_native_str().cmp(d2.name.as_native_str()));
         for deck in &mut decks {
-            // TODO: handle inconsistent capitalisation
-            self.ensure_parents_exist(deck.name.as_native_str())?;
-            self.maybe_reparent(deck);
             self.import_deck(deck)?;
         }
         Ok(())
     }
 
-    fn ensure_parents_exist(&mut self, name: &str) -> Result<()> {
+    fn import_deck(&mut self, deck: &mut Deck) -> Result<()> {
-        if let Some(parent) = immediate_parent_name(name) {
+        self.maybe_reparent(deck);
-            if !self.seen_parents.contains(parent) {
+        if let Some(original) = self.get_deck_by_name(deck)? {
-                self.ensure_parents_exist(parent)?;
+            if original.is_filtered() {
-                self.seen_parents.insert(parent.to_string());
+                self.uniquify_name(deck);
-                if let Some(new_parent) = self.reparented_name(parent) {
+                self.add_deck(deck)
-                    self.ensure_deck_exists(&new_parent)?;
+            } else {
-                } else {
+                self.update_deck(deck, original)
-                    self.ensure_deck_exists(parent)?;
-                };
             }
+        } else {
+            self.ensure_valid_first_existing_parent(deck)?;
+            self.add_deck(deck)
+        }
+    }
+
+    fn maybe_reparent(&self, deck: &mut Deck) {
+        if let Some(new_name) = self.reparented_name(deck.name.as_native_str()) {
+            deck.name = NativeDeckName::from_native_str(new_name);
         }
-        Ok(())
     }
 
     fn reparented_name(&self, name: &str) -> Option<String> {
@@ -86,58 +83,19 @@ impl DeckContext<'_> {
             })
     }
 
-    fn ensure_deck_exists(&mut self, name: &str) -> Result<()> {
-        if let Some(deck) = self.target_col.storage.get_deck_by_name(name)? {
-            if deck.is_filtered() {
-                self.add_default_deck(name, true)?;
-            }
-        } else {
-            self.add_default_deck(name, false)?;
-        };
-        Ok(())
-    }
-
-    fn add_default_deck(&mut self, name: &str, uniquify: bool) -> Result<()> {
-        let mut deck = Deck::new_normal();
-        deck.name = NativeDeckName::from_native_str(name);
-        if uniquify {
-            self.uniquify_name(&mut deck);
-        }
-        self.target_col.add_deck_inner(&mut deck, self.usn)
-    }
-
-    fn uniquify_name(&mut self, deck: &mut Deck) {
-        let old_parent = format!("{}\x1f", deck.name.as_native_str());
-        deck.uniquify_name();
-        let new_parent = format!("{}\x1f", deck.name.as_native_str());
-        self.renamed_parents.push((old_parent, new_parent));
-    }
-
-    fn maybe_reparent(&self, deck: &mut Deck) {
-        if let Some(new_name) = self.reparented_name(deck.name.as_native_str()) {
-            deck.name = NativeDeckName::from_native_str(new_name);
-        }
-    }
-
-    fn import_deck(&mut self, deck: &mut Deck) -> Result<()> {
-        if let Some(original) = self.get_deck_by_name(deck)? {
-            if original.is_filtered() {
-                self.uniquify_name(deck);
-                self.add_deck(deck)
-            } else {
-                self.update_deck(deck, original)
-            }
-        } else {
-            self.add_deck(deck)
-        }
-    }
-
     fn get_deck_by_name(&mut self, deck: &Deck) -> Result<Option<Deck>> {
         self.target_col
             .storage
             .get_deck_by_name(deck.name.as_native_str())
     }
 
+    fn uniquify_name(&mut self, deck: &mut Deck) {
+        let old_parent = format!("{}\x1f", deck.name.as_native_str());
+        deck.uniquify_name(&self.unique_suffix);
+        let new_parent = format!("{}\x1f", deck.name.as_native_str());
+        self.renamed_parents.push((old_parent, new_parent));
+    }
+
     fn add_deck(&mut self, deck: &mut Deck) -> Result<()> {
         let old_id = mem::take(&mut deck.id);
         self.target_col.add_deck_inner(deck, self.usn)?;
@@ -153,11 +111,31 @@ impl DeckContext<'_> {
         self.target_col
             .update_deck_inner(&mut new_deck, original, self.usn)
     }
+
+    fn ensure_valid_first_existing_parent(&mut self, deck: &mut Deck) -> Result<()> {
+        if let Some(ancestor) = self
+            .target_col
+            .first_existing_parent(deck.name.as_native_str(), 0)?
+        {
+            if ancestor.is_filtered() {
+                self.add_unique_default_deck(ancestor.name.as_native_str())?;
+                self.maybe_reparent(deck);
+            }
+        }
+        Ok(())
+    }
+
+    fn add_unique_default_deck(&mut self, name: &str) -> Result<()> {
+        let mut deck = Deck::new_normal();
+        deck.name = NativeDeckName::from_native_str(name);
+        self.uniquify_name(&mut deck);
+        self.target_col.add_deck_inner(&mut deck, self.usn)
+    }
 }
 
 impl Deck {
-    fn uniquify_name(&mut self) {
+    fn uniquify_name(&mut self, suffix: &str) {
-        let new_name = format!("{} {}", self.name.as_native_str(), TimestampSecs::now());
+        let new_name = format!("{} {}", self.name.as_native_str(), suffix);
         self.name = NativeDeckName::from_native_str(new_name);
     }
 }
@@ -171,3 +149,48 @@ impl NormalDeck {
         }
     }
 }
+
+#[cfg(test)]
+mod test {
+    use std::collections::HashSet;
+
+    use super::*;
+    use crate::{collection::open_test_collection, tests::new_deck_with_machine_name};
+
+    #[test]
+    fn parents() {
+        let mut col = open_test_collection();
+
+        col.add_deck_with_machine_name("filtered", true);
+        col.add_deck_with_machine_name("PARENT", false);
+
+        let mut ctx = DeckContext::new(&mut col, Usn(1));
+        ctx.unique_suffix = "★".to_string();
+
+        let imports = vec![
+            new_deck_with_machine_name("unknown parent\x1fchild", false),
+            new_deck_with_machine_name("filtered\x1fchild", false),
+            new_deck_with_machine_name("parent\x1fchild", false),
+            new_deck_with_machine_name("new parent\x1fchild", false),
+            new_deck_with_machine_name("NEW PARENT", false),
+        ];
+        ctx.import_decks(imports).unwrap();
+        let existing_decks: HashSet<_> = ctx
+            .target_col
+            .get_all_deck_names(true)
+            .unwrap()
+            .into_iter()
+            .map(|(_, name)| name)
+            .collect();
+
+        // missing parents get created
+        assert!(existing_decks.contains("unknown parent"));
+        // ... and uniquified if their existing counterparts are filtered
+        assert!(existing_decks.contains("filtered ★"));
+        assert!(existing_decks.contains("filtered ★::child"));
+        // the case of existing parents is matched
+        assert!(existing_decks.contains("PARENT::child"));
+        // the case of imported parents is matched, regardless of pass order
+        assert!(existing_decks.contains("NEW PARENT::child"));
+    }
+}

rslib/src/tests.rs

@@ -0,0 +1,53 @@
+// Copyright: Ankitects Pty Ltd and contributors
+// License: GNU AGPL, version 3 or later; http://www.gnu.org/licenses/agpl.html
+
+#![cfg(test)]
+
+use tempfile::{tempdir, TempDir};
+
+use crate::{collection::CollectionBuilder, media::MediaManager, prelude::*};
+
+pub(crate) fn open_fs_test_collection(name: &str) -> (Collection, TempDir) {
+    let tempdir = tempdir().unwrap();
+    let dir = tempdir.path();
+    let media_folder = dir.join(format!("{name}.media"));
+    std::fs::create_dir(&media_folder).unwrap();
+    let col = CollectionBuilder::new(dir.join(format!("{name}.anki2")))
+        .set_media_paths(media_folder, dir.join(format!("{name}.mdb")))
+        .build()
+        .unwrap();
+    (col, tempdir)
+}
+
+impl Collection {
+    pub(crate) fn add_media(&self, media: &[(&str, &[u8])]) {
+        let mgr = MediaManager::new(&self.media_folder, &self.media_db).unwrap();
+        let mut ctx = mgr.dbctx();
+        for (name, data) in media {
+            mgr.add_file(&mut ctx, name, data).unwrap();
+        }
+    }
+
+    pub(crate) fn new_note(&mut self, notetype: &str) -> Note {
+        self.get_notetype_by_name(notetype)
+            .unwrap()
+            .unwrap()
+            .new_note()
+    }
+
+    pub(crate) fn add_deck_with_machine_name(&mut self, name: &str, filtered: bool) -> Deck {
+        let mut deck = new_deck_with_machine_name(name, filtered);
+        self.add_deck_inner(&mut deck, Usn(1)).unwrap();
+        deck
+    }
+}
+
+pub(crate) fn new_deck_with_machine_name(name: &str, filtered: bool) -> Deck {
+    let mut deck = if filtered {
+        Deck::new_filtered()
+    } else {
+        Deck::new_normal()
+    };
+    deck.name = NativeDeckName::from_native_str(name);
+    deck
+}