fix(model): long attachment names keep their extension and stay removable (#130, #131)

crates/model/src/names.rs

@@ -69,17 +69,46 @@ pub fn collides_by_case(a: &str, b: &str) -> bool {
     a != b && a.to_lowercase() == b.to_lowercase()
 }
 
-/// A file name for an attachment: no path parts, only safe characters, never reserved.
+/// The longest an attachment's file name is made, in characters.
+pub const MAX_FILE_NAME: usize = 80;
+
+/// A file name for an attachment: no path parts, only safe characters, never reserved, and at
+/// most [`MAX_FILE_NAME`] characters. A name that is too long loses the end of its stem, not
+/// its extension, so `report.pdf` stays a PDF however long it is (issue #130). Applying it
+/// again changes nothing.
 pub fn safe_file_name(name: &str) -> String {
     let base = name.rsplit(['/', '\\']).next().unwrap_or(name);
     let cleaned: String = base.chars().map(|c| if c.is_alphanumeric() || c == '.' || c == '-' || c == '_' { c } else { '-' }).collect();
-    let trimmed: String = cleaned.trim_matches(|c| c == '.' || c == '-').chars().take(80).collect();
-    let mut out = if trimmed.is_empty() { "file".to_string() } else { trimmed };
+    let trimmed = cleaned.trim_matches(|c| c == '.' || c == '-');
+    let mut out = if trimmed.is_empty() { "file".to_string() } else { trimmed.to_string() };
     let stem = out.split('.').next().unwrap_or("").to_string();
     if is_reserved_name(&stem) {
         out = format!("file-{out}");
     }
-    out
+    let (stem, extension) = split_extension(&out);
+    fit_file_name(stem, extension, "")
+}
+
+/// `name` split before its extension (the dot included), when it has one worth keeping: a
+/// short run of letters and digits after the last dot.
+pub fn split_extension(name: &str) -> (&str, &str) {
+    match name.rfind('.') {
+        Some(dot) if dot > 0 && (2..=17).contains(&(name.len() - dot)) && name[dot + 1..].chars().all(|c| c.is_ascii_alphanumeric()) => {
+            name.split_at(dot)
+        }
+        _ => (name, ""),
+    }
+}
+
+/// `stem`, `suffix` and `extension` joined into a name of at most [`MAX_FILE_NAME`] characters,
+/// cutting the stem when they do not fit. The uniqueness suffix of a second copy, `-2`, goes
+/// in through here too, so the name it makes is still a safe name (issue #131).
+pub fn fit_file_name(stem: &str, extension: &str, suffix: &str) -> String {
+    let room = MAX_FILE_NAME.saturating_sub(extension.chars().count() + suffix.chars().count());
+    let stem: String = stem.chars().take(room).collect();
+    let stem = stem.trim_end_matches(['.', '-']);
+    let stem = if stem.is_empty() { "file" } else { stem };
+    format!("{stem}{suffix}{extension}")
 }
 
 /// A file in a page or container folder that looks like a sync conflict copy of
@@ -135,6 +164,40 @@ mod tests {
         assert_eq!(safe_file_name("..."), "file");
     }
 
+    /// Issue #130: a long name keeps its extension, and is never longer than the limit.
+    #[test]
+    fn a_long_attachment_name_keeps_its_extension() {
+        for extension in ["pdf", "png", "docx", "tar.gz"] {
+            let name = safe_file_name(&format!("{}.{extension}", "x".repeat(90)));
+            assert!(name.ends_with(&format!(".{}", extension.rsplit('.').next().expect("ext"))), "{name}");
+            assert_eq!(name.chars().count(), MAX_FILE_NAME, "{name}");
+        }
+        assert_eq!(safe_file_name(&format!("{}.txt", "x".repeat(76))), format!("{}.txt", "x".repeat(76)), "a name at the limit is kept");
+        assert_eq!(safe_file_name(&"x".repeat(90)), "x".repeat(80), "a name with no extension is cut as before");
+        assert_eq!(safe_file_name(&format!("{}.not an extension at all", "x".repeat(90))).chars().count(), MAX_FILE_NAME);
+        assert_eq!(safe_file_name(&format!("CON.{}.png", "y".repeat(90))).chars().count(), MAX_FILE_NAME, "a reserved stem made safe still fits");
+        for name in ["../evil name.txt", "CON.png", "...", &format!("{}.pdf", "z".repeat(200)), &format!("é{}.docx", "ü".repeat(100))] {
+            let once = safe_file_name(name);
+            assert_eq!(safe_file_name(&once), once, "applying it again changes nothing: {name}");
+        }
+    }
+
+    /// Issue #131: the suffix of a second copy fits inside the limit with the extension kept, so
+    /// the name made is a safe name in its own right.
+    #[test]
+    fn a_suffixed_name_fits_and_stays_safe() {
+        let original = format!("{}.txt", "x".repeat(76));
+        let (stem, extension) = split_extension(&original);
+        for n in [2, 10, 1234] {
+            let name = fit_file_name(stem, extension, &format!("-{n}"));
+            assert!(name.ends_with(&format!("-{n}.txt")) && name.chars().count() == MAX_FILE_NAME, "{name}");
+            assert_eq!(safe_file_name(&name), name);
+        }
+        assert_eq!(fit_file_name("pic", ".png", "-2"), "pic-2.png");
+        assert_eq!(split_extension("archive"), ("archive", ""));
+        assert_eq!(split_extension(".hidden"), (".hidden", ""));
+    }
+
     #[test]
     fn conflict_names() {
         assert!(!looks_like_conflict("index.html"));

crates/store/src/assets.rs

@@ -133,14 +133,13 @@ fn unique_name(dir: &Path, name: &str) -> String {
         Ok(rd) => rd.filter_map(|e| e.ok()).map(|e| e.file_name().to_string_lossy().to_lowercase()).collect(),
         Err(_) => HashSet::new(),
     };
-    let (stem, ext) = match name.rsplit_once('.') {
-        Some((s, e)) => (s.to_string(), format!(".{e}")),
-        None => (name.to_string(), String::new()),
-    };
+    // The suffix goes in before the extension and within the name's length limit, so the name
+    // made is one `remove` accepts as a safe name (issue #131).
+    let (stem, ext) = names::split_extension(name);
     let mut candidate = name.to_string();
     let mut n = 2;
     while taken.contains(&candidate.to_lowercase()) {
-        candidate = format!("{stem}-{n}{ext}");
+        candidate = names::fit_file_name(stem, ext, &format!("-{n}"));
         n += 1;
     }
     candidate
@@ -298,6 +297,29 @@ mod tests {
         assert_eq!(list(&page).len(), 1, "and nothing was deleted trying");
     }
 
+    /// Issues #130 and #131: a long name keeps its file type, and every copy made of it, however
+    /// many, has a name that can be taken out again.
+    #[test]
+    fn long_names_keep_their_type_and_every_copy_can_be_removed() {
+        let t = TempDir::new("assets-long-names");
+        let p = page(&t);
+        for extension in ["pdf", "png", "docx"] {
+            let link = attach_bytes(&p, &format!("{}.{extension}", "x".repeat(90)), b"owned file").expect("attach");
+            assert!(link.ends_with(&format!(".{extension}")), "{link}");
+        }
+        assert_eq!(media_kind(&attach_bytes(&p, &format!("{}.png", "y".repeat(90)), b"png").expect("attach")), MediaKind::Image);
+
+        let name = format!("{}.txt", "z".repeat(76));
+        let copies: Vec<String> = (0..12)
+            .map(|i| attach_bytes(&p, &name, format!("copy {i}").as_bytes()).expect("attach").trim_start_matches("assets/").to_string())
+            .collect();
+        assert_eq!(copies.iter().collect::<HashSet<_>>().len(), 12, "every copy has a name of its own");
+        for copy in &copies {
+            assert!(copy.ends_with(".txt") && copy.chars().count() <= names::MAX_FILE_NAME, "{copy}");
+            remove(&p, copy).unwrap_or_else(|e| panic!("{copy} can be removed: {e}"));
+        }
+    }
+
     /// Issue #103: a picture the user attached is not taken for the icon of the attachment its
     /// name extends. It is listed, and removing that attachment leaves it alone.
     #[test]