From 7daa417dc868a105fc1e42fd37ccd1246ba0ac51 Mon Sep 17 00:00:00 2001 From: Damien Elmes Date: Sun, 17 May 2020 19:07:15 +1000 Subject: [PATCH] fix renaming corner cases and decks.update() - .update() should update a single deck and preserve usn by default, as that's what existing code expects - decks are automatically renamed when they conflict with an existing name --- pylib/anki/decks.py | 11 +-- pylib/tests/test_decks.py | 25 ++---- rslib/src/backend/mod.rs | 9 ++- rslib/src/dbcheck.rs | 3 +- rslib/src/decks/mod.rs | 161 ++++++++++++++++++++++++-------------- rslib/src/decks/tree.rs | 4 +- 6 files changed, 126 insertions(+), 87 deletions(-) diff --git a/pylib/anki/decks.py b/pylib/anki/decks.py index 991a8aab9..8dc6c13b0 100644 --- a/pylib/anki/decks.py +++ b/pylib/anki/decks.py @@ -75,7 +75,7 @@ class DeckManager: self.update_config(g) return else: - self.update(g) + self.update(g, preserve_usn=False) # legacy def flush(self): @@ -97,7 +97,7 @@ class DeckManager: deck = self.new_deck_legacy(bool(type)) deck["name"] = name - self.update(deck) + self.update(deck, preserve_usn=False) # fixme hooks.deck_added(deck) @@ -205,7 +205,7 @@ class DeckManager: return self.get_legacy(id) return None - def update(self, g: Dict[str, Any], preserve_usn=False) -> None: + def update(self, g: Dict[str, Any], preserve_usn=True) -> None: "Add or update an existing deck. Used for syncing and merging." try: self.col.backend.add_or_update_deck_legacy(g, preserve_usn) @@ -217,12 +217,9 @@ class DeckManager: def rename(self, g: Dict[str, Any], newName: str) -> None: "Rename deck prefix to NAME if not exists. Updates children." g["name"] = newName - self.update(g) + self.update(g, preserve_usn=False) return - # fixme: ensure rename of b in a::b::c generates new b - # fixme: renaming may have altered active did order - def renameForDragAndDrop(self, draggedDeckDid: int, ontoDeckDid: Any) -> None: draggedDeck = self.get(draggedDeckDid) draggedDeckName = draggedDeck["name"] diff --git a/pylib/tests/test_decks.py b/pylib/tests/test_decks.py index 0236e74e5..cdd74fb6a 100644 --- a/pylib/tests/test_decks.py +++ b/pylib/tests/test_decks.py @@ -72,8 +72,10 @@ def test_rename(): assert "hello::world" not in names # create another deck id = d.decks.id("tmp") - # we can't rename it if it conflicts - assertException(Exception, lambda: d.decks.rename(d.decks.get(id), "foo")) + # automatically adjusted if a duplicate name + d.decks.rename(d.decks.get(id), "FOO") + names = [n.name for n in d.decks.all_names_and_ids()] + assert "FOO+" in names # when renaming, the children should be renamed too d.decks.id("one::two::three") id = d.decks.id("one") @@ -88,11 +90,6 @@ def test_rename(): child = d.decks.get(childId) assertException(DeckRenameError, lambda: d.decks.rename(child, "filtered::child")) assertException(DeckRenameError, lambda: d.decks.rename(child, "FILTERED::child")) - # changing case - parentId = d.decks.id("PARENT") - d.decks.id("PARENT::CHILD") - assertException(DeckRenameError, lambda: d.decks.rename(child, "PARENT::CHILD")) - assertException(DeckRenameError, lambda: d.decks.rename(child, "PARENT::child")) def test_renameForDragAndDrop(): @@ -137,18 +134,10 @@ def test_renameForDragAndDrop(): d.decks.renameForDragAndDrop(chinese_did, None) assert deckNames() == ["Chinese", "Chinese::HSK", "Languages"] - # can't drack a deck where sibling have same name - new_hsk_did = d.decks.id("HSK") - assertException( - DeckRenameError, lambda: d.decks.renameForDragAndDrop(new_hsk_did, chinese_did) - ) - d.decks.rem(new_hsk_did) - - # can't drack a deck where sibling have same name different case + # decks are renamed if necessary new_hsk_did = d.decks.id("hsk") - assertException( - DeckRenameError, lambda: d.decks.renameForDragAndDrop(new_hsk_did, chinese_did) - ) + d.decks.renameForDragAndDrop(new_hsk_did, chinese_did) + assert deckNames() == ["Chinese", "Chinese::HSK", "Chinese::hsk+", "Languages"] d.decks.rem(new_hsk_did) # '' is a convenient alias for the top level DID diff --git a/rslib/src/backend/mod.rs b/rslib/src/backend/mod.rs index 22345697d..010dbfb15 100644 --- a/rslib/src/backend/mod.rs +++ b/rslib/src/backend/mod.rs @@ -1046,7 +1046,14 @@ impl Backend { self.with_col(|col| { let schema11: DeckSchema11 = serde_json::from_slice(&input.deck)?; let mut deck: Deck = schema11.into(); - col.add_or_update_deck(&mut deck, input.preserve_usn_and_mtime)?; + if input.preserve_usn_and_mtime { + col.transact(None, |col| { + let usn = col.usn()?; + col.add_or_update_single_deck(&mut deck, usn) + })?; + } else { + col.add_or_update_deck(&mut deck)?; + } Ok(deck.id.0) }) } diff --git a/rslib/src/dbcheck.rs b/rslib/src/dbcheck.rs index a0772b1c7..50e6a15d0 100644 --- a/rslib/src/dbcheck.rs +++ b/rslib/src/dbcheck.rs @@ -154,8 +154,9 @@ impl Collection { } fn check_missing_deck_ids(&mut self, out: &mut CheckDatabaseOutput) -> Result<()> { + let usn = self.usn()?; for did in self.storage.missing_decks()? { - self.recover_missing_deck(did)?; + self.recover_missing_deck(did, usn)?; out.decks_missing += 1; } Ok(()) diff --git a/rslib/src/decks/mod.rs b/rslib/src/decks/mod.rs index e943f23ba..d4325deed 100644 --- a/rslib/src/decks/mod.rs +++ b/rslib/src/decks/mod.rs @@ -89,16 +89,6 @@ impl Deck { self.name.replace("\x1f", "::") } - pub(crate) fn prepare_for_update(&mut self) { - // fixme - we currently only do this when converting from human; should be done in pub methods instead - - // if self.name.contains(invalid_char_for_deck_component) { - // self.name = self.name.replace(invalid_char_for_deck_component, ""); - // } - // ensure_string_in_nfc(&mut self.name); - } - - // fixme: unify with prepare for update pub(crate) fn set_modified(&mut self, usn: Usn) { self.mtime_secs = TimestampSecs::now(); self.usn = usn; @@ -115,11 +105,11 @@ impl Deck { } } -// fixme: need to bump usn on upgrade if we rename fn invalid_char_for_deck_component(c: char) -> bool { c.is_ascii_control() || c == '"' } +// fixme: need to bump usn on upgrade if we rename fn normalized_deck_name_component(comp: &str) -> Cow { let mut out = normalize_to_nfc(comp); if out.contains(invalid_char_for_deck_component) { @@ -127,15 +117,29 @@ fn normalized_deck_name_component(comp: &str) -> Cow { } let trimmed = out.trim(); if trimmed.is_empty() { - "blank".into() + "blank".to_string().into() } else if trimmed.len() != out.len() { - // fixme: trimming leading/trailing spaces may break old clients if we don't bump mod trimmed.to_string().into() } else { out } } +fn normalize_native_name(name: &str) -> Cow { + if name + .split('\x1f') + .any(|comp| matches!(normalized_deck_name_component(comp), Cow::Owned(_))) + { + name.split('\x1f') + .map(normalized_deck_name_component) + .collect::() + .into() + } else { + // no changes required + name.into() + } +} + pub(crate) fn human_deck_name_to_native(name: &str) -> String { let mut out = String::with_capacity(name.len()); for comp in name.split("::") { @@ -146,7 +150,6 @@ pub(crate) fn human_deck_name_to_native(name: &str) -> String { } impl Collection { - // fixme: this cache may belong in CardGenContext? pub(crate) fn get_deck(&mut self, did: DeckID) -> Result>> { if let Some(deck) = self.state.deck_cache.get(&did) { return Ok(Some(deck.clone())); @@ -192,43 +195,52 @@ impl Collection { self.storage.deck_is_empty(DeckID(1)) } - pub(crate) fn add_or_update_deck(&mut self, deck: &mut Deck, preserve_usn: bool) -> Result<()> { - // fixme: vet cache clearing + /// Normalize deck name and rename if not unique. Bumps mtime and usn if + /// deck was modified. + fn prepare_deck_for_update(&mut self, deck: &mut Deck, usn: Usn) -> Result<()> { + if let Cow::Owned(name) = normalize_native_name(&deck.name) { + deck.name = name; + deck.set_modified(usn); + } + self.ensure_deck_name_unique(deck, usn) + } + + /// Add or update an existing deck modified by the user. May add parents, + /// or rename children as required. + pub(crate) fn add_or_update_deck(&mut self, deck: &mut Deck) -> Result<()> { self.state.deck_cache.clear(); self.transact(None, |col| { let usn = col.usn()?; - deck.prepare_for_update(); - - // fixme: bail - assert!(!deck.name.contains("::")); - - // fixme: check deck name is not duplicate - // handle blank deck name, etc - - if !preserve_usn { - deck.set_modified(usn); - } + deck.set_modified(usn); if deck.id.0 == 0 { + col.prepare_deck_for_update(deck, usn)?; col.match_or_create_parents(deck)?; col.storage.add_deck(deck) - } else { - if let Some(existing_deck) = col.storage.get_deck(deck.id)? { - if existing_deck.name != deck.name { - return col.update_renamed_deck(existing_deck, deck, usn); - } + } else if let Some(existing_deck) = col.storage.get_deck(deck.id)? { + if existing_deck.name != deck.name { + col.update_renamed_deck(existing_deck, deck, usn) } else { - // fixme: this should only happen in the syncing case, and we should - // ensure there are no missing parents at the end of the sync + col.add_or_update_single_deck(deck, usn) } - col.storage.update_deck(deck) + } else { + Err(AnkiError::invalid_input("updating non-existent deck")) } }) } - fn ensure_deck_name_unique(&self, deck: &mut Deck) -> Result<()> { + /// Add/update a single deck when syncing/importing. Ensures name is unique + /// & normalized, but does not check parents/children or update mtime + /// (unless the name was changed). Caller must set up transaction. + pub(crate) fn add_or_update_single_deck(&mut self, deck: &mut Deck, usn: Usn) -> Result<()> { + self.state.deck_cache.clear(); + self.prepare_deck_for_update(deck, usn)?; + self.storage.update_deck(deck) + } + + fn ensure_deck_name_unique(&self, deck: &mut Deck, usn: Usn) -> Result<()> { loop { match self.storage.get_deck_id(&deck.name)? { Some(did) if did == deck.id => { @@ -238,20 +250,18 @@ impl Collection { _ => (), } deck.name += "+"; + deck.set_modified(usn); } Ok(()) } - pub(crate) fn recover_missing_deck(&mut self, did: DeckID) -> Result<()> { + pub(crate) fn recover_missing_deck(&mut self, did: DeckID, usn: Usn) -> Result<()> { let mut deck = Deck::new_normal(); deck.id = did; deck.name = format!("recovered{}", did); - self.ensure_deck_name_unique(&mut deck)?; - deck.prepare_for_update(); - self.storage.update_deck(&deck)?; - - Ok(()) + deck.set_modified(usn); + self.add_or_update_single_deck(&mut deck, usn) } pub fn get_or_create_normal_deck(&mut self, human_name: &str) -> Result { @@ -261,26 +271,23 @@ impl Collection { } else { let mut deck = Deck::new_normal(); deck.name = native_name; - self.add_or_update_deck(&mut deck, false)?; + self.add_or_update_deck(&mut deck)?; Ok(deck) } } fn update_renamed_deck(&mut self, existing: Deck, updated: &mut Deck, usn: Usn) -> Result<()> { - // new name should not conflict with a different deck - if let Some(other_did) = self.storage.get_deck_id(&updated.name)? { - if other_did != updated.id { - // fixme: this could break when syncing - return Err(AnkiError::Existing); - } - } - + // match closest parent name self.match_or_create_parents(updated)?; - self.storage.update_deck(updated)?; - self.rename_child_decks(&existing, &updated.name, usn) + // rename children + self.rename_child_decks(&existing, &updated.name, usn)?; + // update deck + self.add_or_update_single_deck(updated, usn)?; + // after updating, we need to ensure all grandparents exist, which may not be the case + // in the parent->child case + self.create_missing_parents(&updated.name) } - // fixme: make sure this handles foo::bar and FOO::baz fn rename_child_decks(&mut self, old: &Deck, new_name: &str, usn: Usn) -> Result<()> { let children = self.storage.child_decks(old)?; let old_component_count = old.name.matches('\x1f').count() + 1; @@ -372,6 +379,9 @@ impl Collection { } pub fn remove_deck_and_child_decks(&mut self, did: DeckID) -> Result<()> { + // fixme: vet cache clearing + self.state.deck_cache.clear(); + self.transact(None, |col| { let usn = col.usn()?; @@ -400,9 +410,8 @@ impl Collection { let mut deck = deck.to_owned(); // fixme: separate key deck.name = self.i18n.tr(TR::DeckConfigDefaultName).into(); - self.ensure_deck_name_unique(&mut deck)?; deck.set_modified(usn); - self.storage.update_deck(&deck)?; + self.add_or_update_single_deck(&mut deck, usn)?; } else { self.storage.remove_deck(deck.id)?; self.storage.add_deck_grave(deck.id, usn)?; @@ -525,12 +534,48 @@ mod test { let _ = col.get_or_create_normal_deck("foo::bar::baz")?; let mut top_deck = col.get_or_create_normal_deck("foo")?; top_deck.name = "other".into(); - col.add_or_update_deck(&mut top_deck, false)?; + col.add_or_update_deck(&mut top_deck)?; assert_eq!( sorted_names(&col), vec!["Default", "other", "other::bar", "other::bar::baz"] ); + // should do the right thing in the middle of the tree as well + let mut middle = col.get_or_create_normal_deck("other::bar")?; + middle.name = "quux\x1ffoo".into(); + col.add_or_update_deck(&mut middle)?; + assert_eq!( + sorted_names(&col), + vec!["Default", "other", "quux", "quux::foo", "quux::foo::baz"] + ); + + // add another child + let _ = col.get_or_create_normal_deck("quux::foo::baz2"); + + // quux::foo -> quux::foo::baz::four + // means quux::foo::baz2 should be quux::foo::baz::four::baz2 + // and a new quux::foo should have been created + middle.name = "quux\x1ffoo\x1fbaz\x1ffour".into(); + col.add_or_update_deck(&mut middle)?; + assert_eq!( + sorted_names(&col), + vec![ + "Default", + "other", + "quux", + "quux::foo", + "quux::foo::baz", + "quux::foo::baz::four", + "quux::foo::baz::four::baz", + "quux::foo::baz::four::baz2" + ] + ); + + // should handle name conflicts + middle.name = "other".into(); + col.add_or_update_deck(&mut middle)?; + assert_eq!(middle.name, "other+"); + Ok(()) } @@ -542,7 +587,7 @@ mod test { let mut default = col.get_or_create_normal_deck("default")?; default.name = "one\x1ftwo".into(); - col.add_or_update_deck(&mut default, false)?; + col.add_or_update_deck(&mut default)?; // create a non-default deck confusingly named "default" let _fake_default = col.get_or_create_normal_deck("default")?; diff --git a/rslib/src/decks/tree.rs b/rslib/src/decks/tree.rs index 206c197eb..acf4750f5 100644 --- a/rslib/src/decks/tree.rs +++ b/rslib/src/decks/tree.rs @@ -335,9 +335,9 @@ mod test { // simulate answering a card child_deck.common.new_studied = 1; - col.add_or_update_deck(&mut child_deck, false)?; + col.add_or_update_deck(&mut child_deck)?; parent_deck.common.new_studied = 1; - col.add_or_update_deck(&mut parent_deck, false)?; + col.add_or_update_deck(&mut parent_deck)?; // with the default limit of 20, there should still be 4 due let tree = col.deck_tree(true, None)?;