Extract & add test

backend/sync_server/src/server/update_document.rs

@@ -8,7 +8,6 @@ use axum_extra::{
 use axum_jsonschema::Json;
 use chrono::{DateTime, Utc};
 use log::info;
-use regex::Regex;
 use schemars::JsonSchema;
 use serde::Deserialize;
 use sync_lib::{base64_to_bytes, is_file_type_mergable, merge};
@@ -20,12 +19,9 @@ use super::{
     responses::DocumentUpdateResponse,
 };
 use crate::{
-    database::{
+    database::models::{DocumentId, StoredDocumentVersion, VaultId, VaultUpdateId},
-        self, Transaction,
-        models::{DocumentId, StoredDocumentVersion, VaultId, VaultUpdateId},
-    },
     errors::{SyncServerError, client_error, not_found_error, server_error},
-    utils::sanitize_path,
+    utils::{deduped_file_paths, sanitize_path},
 };
 
 // This is required for aide to infer the path parameter types and names
@@ -172,13 +168,21 @@ async fn internal_update_document(
     let new_relative_path = if parent_document.relative_path == latest_version.relative_path
         && latest_version.relative_path != sanitized_relative_path
     {
-        get_deduped_file_name(
+        let mut new_relative_path = Default::default();
-            &state.database,
+        for candidate in deduped_file_paths(&sanitized_relative_path) {
-            &vault_id,
+            if state
-            &mut transaction,
+                .database
-            &sanitized_relative_path,
+                .get_latest_document_by_path(&vault_id, &candidate, Some(&mut transaction))
-        )
+                .await
-        .await?
+                .map_err(server_error)?
+                .is_none()
+            {
+                new_relative_path = candidate;
+                break;
+            }
+        }
+
+        new_relative_path
     } else {
         latest_version.relative_path.clone()
     };
@@ -212,53 +216,3 @@ async fn internal_update_document(
         DocumentUpdateResponse::FastForwardUpdate(new_version.into())
     }))
 }
-
-// Only a single file can be on the same path, so we need to dedup the path
-// in case the client is trying to rename the file to an existing file's name
-// that it's unaware of.
-async fn get_deduped_file_name(
-    database: &database::Database,
-    vault_id: &VaultId,
-    transaction: &mut Transaction<'_>,
-    path: &str,
-) -> Result<String, SyncServerError> {
-    let parts = path.rsplitn(2, '.').collect::<Vec<_>>();
-    let mut reverse_parts = parts.into_iter().rev();
-    let (stem, extension) = match (reverse_parts.next(), reverse_parts.next()) {
-        (Some(stem), maybe_extension) => (
-            stem,
-            maybe_extension
-                .map(|ext| format!(".{ext}"))
-                .unwrap_or_default(),
-        ),
-        _ => unreachable!("Path must have at least one part"),
-    };
-
-    let regex = Regex::new(r" \((\d+)\)$").unwrap();
-    let start_number = regex
-        .captures(stem)
-        .and_then(|caps| caps.get(1))
-        .and_then(|m| m.as_str().parse::<u32>().ok())
-        .unwrap_or(0);
-
-    let clean_stem = regex.replace(stem, "").to_string();
-
-    for dedup_number in start_number.. {
-        let proposed_path = if dedup_number == 0 {
-            format!("{clean_stem}{extension}")
-        } else {
-            format!("{clean_stem} ({dedup_number}){extension}")
-        };
-
-        if database
-            .get_latest_document_by_path(vault_id, &proposed_path, Some(transaction))
-            .await
-            .map_err(server_error)?
-            .is_none()
-        {
-            return Ok(proposed_path.to_string());
-        }
-    }
-
-    unreachable!("Loop must always return a value");
-}

backend/sync_server/src/utils.rs

@@ -1,3 +1,5 @@
+use regex::Regex;
+
 /// Sanitize the document's path to allow all clients to create the same path in
 /// their filesystem. If we didn't do this server-side, client's would need to
 /// deal with mapping invalid names to valid ones and then back.
@@ -21,6 +23,45 @@ pub fn sanitize_path(path: &str) -> String {
         .join("/")
 }
 
+pub fn deduped_file_paths(path: &str) -> impl Iterator<Item = String> {
+    let mut path_parts = path.split('/').collect::<Vec<_>>();
+    let file_name = path_parts.pop().unwrap().to_owned();
+
+    let mut directory = path_parts.join("/");
+    if !directory.is_empty() {
+        directory.push('/');
+    }
+
+    let name_parts = file_name.rsplitn(2, '.').collect::<Vec<_>>();
+    let mut reverse_parts = name_parts.into_iter().rev();
+    let (stem, extension) = match (reverse_parts.next(), reverse_parts.next()) {
+        (Some(stem), maybe_extension) => (
+            stem.to_owned(),
+            maybe_extension
+                .map(|ext| format!(".{ext}"))
+                .unwrap_or_default(),
+        ),
+        _ => unreachable!("Path must have at least one part"),
+    };
+
+    let regex = Regex::new(r" \((\d+)\)$").unwrap();
+    let start_number = regex
+        .captures(&stem)
+        .and_then(|caps| caps.get(1))
+        .and_then(|m| m.as_str().parse::<u32>().ok())
+        .unwrap_or(0);
+
+    let clean_stem = regex.replace(&stem, "").to_string();
+
+    (start_number..).map(move |dedup_number| {
+        if dedup_number == 0 {
+            format!("{directory}{clean_stem}{extension}")
+        } else {
+            format!("{directory}{clean_stem} ({dedup_number}){extension}")
+        }
+    })
+}
+
 #[cfg(test)]
 mod test {
     use super::*;
@@ -30,4 +71,47 @@ mod test {
         assert_eq!(sanitize_path("/my/path/what?"), "/my/path/what");
         assert_eq!(sanitize_path("/my/path/\\\\:?"), "/my/path/_");
     }
+
+    #[test]
+    fn test_deduped_file_paths() {
+        let mut deduped = deduped_file_paths("file.txt");
+        assert_eq!(deduped.next(), Some("file.txt".to_owned()));
+        assert_eq!(deduped.next(), Some("file (1).txt".to_owned()));
+        assert_eq!(deduped.next(), Some("file (2).txt".to_owned()));
+
+        let mut deduped = deduped_file_paths("file");
+        assert_eq!(deduped.next(), Some("file".to_owned()));
+        assert_eq!(deduped.next(), Some("file (1)".to_owned()));
+        assert_eq!(deduped.next(), Some("file (2)".to_owned()));
+
+        let mut deduped = deduped_file_paths("file (51).md");
+        assert_eq!(deduped.next(), Some("file (51).md".to_owned()));
+        assert_eq!(deduped.next(), Some("file (52).md".to_owned()));
+        assert_eq!(deduped.next(), Some("file (53).md".to_owned()));
+
+        let mut deduped = deduped_file_paths("file (5)");
+        assert_eq!(deduped.next(), Some("file (5)".to_owned()));
+        assert_eq!(deduped.next(), Some("file (6)".to_owned()));
+        assert_eq!(deduped.next(), Some("file (7)".to_owned()));
+
+        let mut deduped = deduped_file_paths("my/path.with.dots/file (5).md");
+        assert_eq!(
+            deduped.next(),
+            Some("my/path.with.dots/file (5).md".to_owned())
+        );
+        assert_eq!(
+            deduped.next(),
+            Some("my/path.with.dots/file (6).md".to_owned())
+        );
+
+        let mut deduped = deduped_file_paths("my/path.with.dots/file (5)");
+        assert_eq!(
+            deduped.next(),
+            Some("my/path.with.dots/file (5)".to_owned())
+        );
+        assert_eq!(
+            deduped.next(),
+            Some("my/path.with.dots/file (6)".to_owned())
+        );
+    }
 }