From 1e22432233d7cc72494fc66d8499efbceaec64cb Mon Sep 17 00:00:00 2001 From: RumovZ Date: Fri, 15 Apr 2022 19:07:52 +0200 Subject: [PATCH] Fix deck importing edge cases Test those edge cases, and add some global test helpers. --- rslib/src/decks/addupdate.rs | 2 +- .../package/apkg/import/decks.rs | 167 ++++++++++-------- rslib/src/tests.rs | 53 ++++++ 3 files changed, 149 insertions(+), 73 deletions(-) create mode 100644 rslib/src/tests.rs diff --git a/rslib/src/decks/addupdate.rs b/rslib/src/decks/addupdate.rs index cf2975478..159c57abd 100644 --- a/rslib/src/decks/addupdate.rs +++ b/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, diff --git a/rslib/src/import_export/package/apkg/import/decks.rs b/rslib/src/import_export/package/apkg/import/decks.rs index 98138d5dd..35b035a54 100644 --- a/rslib/src/import_export/package/apkg/import/decks.rs +++ b/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::{ - collections::{HashMap, HashSet}, - mem, -}; +use std::{collections::HashMap, mem}; use super::Context; -use crate::{ - decks::{immediate_parent_name, NormalDeck}, - prelude::*, -}; +use crate::{decks::NormalDeck, prelude::*}; struct DeckContext<'d> { target_col: &'d mut Collection, usn: Usn, - seen_parents: HashSet, renamed_parents: Vec<(String, String)>, imported_decks: HashMap, + 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<()> { - if let Some(parent) = immediate_parent_name(name) { - if !self.seen_parents.contains(parent) { - self.ensure_parents_exist(parent)?; - self.seen_parents.insert(parent.to_string()); - if let Some(new_parent) = self.reparented_name(parent) { - self.ensure_deck_exists(&new_parent)?; - } else { - self.ensure_deck_exists(parent)?; - }; + fn import_deck(&mut self, deck: &mut Deck) -> Result<()> { + self.maybe_reparent(deck); + 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.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 { @@ -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> { 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) { - let new_name = format!("{} {}", self.name.as_native_str(), TimestampSecs::now()); + fn uniquify_name(&mut self, suffix: &str) { + 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")); + } +} diff --git a/rslib/src/tests.rs b/rslib/src/tests.rs new file mode 100644 index 000000000..5c1be1818 --- /dev/null +++ b/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 +}