tweaks to the parent matching behaviour

- move logic out of the storage layer - its job is only to read
and write data from the DB
- avoid the Result within a Result
- return the preferred case as an option, so we can avoid a copy
in the unchanged case
- return a Cow when normalizing, so we can avoid copying in the
unchanged case
- add tags directly in clear_unused_tags(), so we avoid doing
lookups for every tag in the tag list

rslib/src/storage/tag/mod.rs

@@ -2,8 +2,7 @@
 // License: GNU AGPL, version 3 or later; http://www.gnu.org/licenses/agpl.html
 
 use super::SqliteStorage;
-use crate::err::{AnkiError, Result};
+use crate::{err::Result, tags::Tag, types::Usn};
-use crate::{tags::Tag, types::Usn};
 
 use rusqlite::{params, Row, NO_PARAMS};
 use std::collections::HashMap;
@@ -16,10 +15,6 @@ fn row_to_tag(row: &Row) -> Result<Tag> {
     })
 }
 
-fn immediate_parent_name(tag_name: &str) -> Option<&str> {
-    tag_name.rsplitn(2, "::").nth(1)
-}
-
 impl SqliteStorage {
     /// All tags in the collection, in alphabetical order.
     pub(crate) fn all_tags(&self) -> Result<Vec<Tag>> {
@@ -61,43 +56,12 @@ impl SqliteStorage {
         Ok(())
     }
 
-    /// If parent tag(s) exist, rewrite name to match their case.
+    pub(crate) fn preferred_tag_case(&self, tag: &str) -> Result<Option<String>> {
-    fn match_parents(&self, tag: &str) -> Result<String> {
-        let child_split: Vec<_> = tag.split("::").collect();
-        let t = if let Some(parent_tag) = self.first_existing_parent(&tag)? {
-            let parent_count = parent_tag.matches("::").count() + 1;
-            format!(
-                "{}::{}",
-                parent_tag,
-                &child_split[parent_count..].join("::")
-            )
-        } else {
-            tag.into()
-        };
-
-        Ok(t)
-    }
-
-    fn first_existing_parent(&self, mut tag: &str) -> Result<Option<String>> {
-        while let Some(parent_name) = immediate_parent_name(tag) {
-            if let Some(parent_tag) = self.get_tag(parent_name)? {
-                return Ok(Some(parent_tag.name));
-            }
-            tag = parent_name;
-        }
-
-        Ok(None)
-    }
-
-    // Get stored tag name or the same passed name if it doesn't exist, rewritten to match parents case.
-    // Returns a tuple of the preferred name and a boolean indicating if the tag exists.
-    pub(crate) fn preferred_tag_case(&self, tag: &str) -> Result<(Result<String>, bool)> {
         self.db
             .prepare_cached("select tag from tags where tag = ?")?
-            .query_row(params![tag], |row| {
+            .query_and_then(params![tag], |row| row.get(0))?
-                Ok((self.match_parents(row.get_raw(0).as_str()?), true))
+            .next()
-            })
+            .transpose()
-            .or_else::<AnkiError, _>(|_| Ok((self.match_parents(tag), false)))
             .map_err(Into::into)
     }
 

rslib/src/tags.rs

@@ -86,19 +86,29 @@ fn normalized_tag_name_component(comp: &str) -> Cow<str> {
     }
 }
 
-fn normalize_tag_name(name: &str) -> String {
+fn normalize_tag_name(name: &str) -> Cow<str> {
-    let mut out = String::with_capacity(name.len());
+    if name
-    for comp in name.split("::") {
+        .split("::")
-        out.push_str(&normalized_tag_name_component(comp));
+        .any(|comp| matches!(normalized_tag_name_component(comp), Cow::Owned(_)))
-        out.push_str("::");
+    {
+        name.split("::")
+            .map(normalized_tag_name_component)
+            .collect::<String>()
+            .into()
+    } else {
+        // no changes required
+        name.into()
     }
-    out.trim_end_matches("::").into()
 }
 
-fn immediate_parent_name(tag_name: UniCase<&str>) -> Option<UniCase<&str>> {
+fn immediate_parent_name_unicase(tag_name: UniCase<&str>) -> Option<UniCase<&str>> {
     tag_name.rsplitn(2, '\x1f').nth(1).map(UniCase::new)
 }
 
+fn immediate_parent_name_str(tag_name: &str) -> Option<&str> {
+    tag_name.rsplitn(2, "::").nth(1)
+}
+
 /// For the given tag, check if immediate parent exists. If so, add
 /// tag and return.
 /// If the immediate parent is missing, check and add any missing parents.
@@ -109,7 +119,7 @@ fn add_tag_and_missing_parents<'a, 'b>(
     missing: &'a mut Vec<UniCase<&'b str>>,
     tag_name: UniCase<&'b str>,
 ) {
-    if let Some(parent) = immediate_parent_name(tag_name) {
+    if let Some(parent) = immediate_parent_name_unicase(tag_name) {
         if !all.contains(&parent) {
             missing.push(parent);
             add_tag_and_missing_parents(all, missing, parent);
@@ -214,24 +224,52 @@ impl Collection {
         Ok((tags, added))
     }
 
-    /// Register tag if it doesn't exist.
+    /// Adjust tag casing to match any existing parents, and register it if it's not already
-    /// Returns a tuple of the tag with its name normalized and a boolean indicating if it was added.
+    /// in the tags list. Returns a tuple of the tag with its name normalized, and a boolean
+    /// indicating if it was added.
     pub(crate) fn register_tag(&self, tag: Tag) -> Result<(Tag, bool)> {
         let normalized_name = normalize_tag_name(&tag.name);
-        let mut t = Tag {
-            name: normalized_name.clone(),
-            ..tag
-        };
         if normalized_name.is_empty() {
-            return Ok((t, false));
+            // this should not be possible
+            return Err(AnkiError::invalid_input("blank tag"));
         }
-        let (preferred, exists) = self.storage.preferred_tag_case(&normalized_name)?;
+        if let Some(out_tag) = self.storage.get_tag(&normalized_name)? {
-        t.name = preferred?;
+            // already registered
-        if !exists {
+            Ok((out_tag, false))
-            self.storage.register_tag(&t)?;
+        } else {
+            let name = self
+                .adjusted_case_for_parents(&normalized_name)?
+                .unwrap_or_else(|| normalized_name.into());
+            let out_tag = Tag { name, ..tag };
+            self.storage.register_tag(&out_tag)?;
+            Ok((out_tag, true))
+        }
+    }
+
+    /// If parent tag(s) exist and differ in case, return a rewritten tag.
+    fn adjusted_case_for_parents(&self, tag: &str) -> Result<Option<String>> {
+        if let Some(parent_tag) = self.first_existing_parent_tag(&tag)? {
+            let child_split: Vec<_> = tag.split("::").collect();
+            let parent_count = parent_tag.matches("::").count() + 1;
+            Ok(Some(format!(
+                "{}::{}",
+                parent_tag,
+                &child_split[parent_count..].join("::")
+            )))
+        } else {
+            Ok(None)
+        }
+    }
+
+    fn first_existing_parent_tag(&self, mut tag: &str) -> Result<Option<String>> {
+        while let Some(parent_name) = immediate_parent_name_str(tag) {
+            if let Some(parent_tag) = self.storage.preferred_tag_case(parent_name)? {
+                return Ok(Some(parent_tag));
+            }
+            tag = parent_name;
         }
 
-        Ok((t, !exists))
+        Ok(None)
     }
 
     pub fn clear_unused_tags(&self) -> Result<()> {
@@ -239,7 +277,8 @@ impl Collection {
         self.storage.clear_tags()?;
         let usn = self.usn()?;
         for name in self.storage.all_tags_in_notes()? {
-            self.register_tag(Tag {
+            let name = normalize_tag_name(&name).into();
+            self.storage.register_tag(&Tag {
                 collapsed: collapsed.contains(&name),
                 name,
                 usn,