[ENG-1176] Append (x) to the end of files instead of overwriting (#1425)

* append ` (x)` to files when duplicating, renaming or pasting instead of overwriting

* cleanup commented code

* fix renames

* rustfmt

* remove unused item

* Small tech debts and some nitpicks

* Bug with appending number on duplicates

* A bug on my new impl

---------

Co-authored-by: Ericson Fogo Soares <ericson.ds999@gmail.com>

core/src/api/files.rs

@@ -428,9 +428,10 @@ pub(crate) fn mount() -> AlphaRouter<Ctx> {
 						Ok(_) => {
 							return Err(rspc::Error::new(
 								ErrorCode::Conflict,
-								"File already exists".to_string(),
-							))
+								"Renaming would overwrite a file".to_string(),
+							));
 						}
+
 						Err(e) => {
 							if e.kind() != std::io::ErrorKind::NotFound {
 								return Err(rspc::Error::with_cause(
@@ -439,19 +440,19 @@ pub(crate) fn mount() -> AlphaRouter<Ctx> {
 									e,
 								));
 							}
+
+							fs::rename(location_path.join(&iso_file_path), new_file_full_path)
+								.await
+								.map_err(|e| {
+									rspc::Error::with_cause(
+										ErrorCode::InternalServerError,
+										"Failed to rename file".to_string(),
+										e,
+									)
+								})?;
 						}
 					}
 
-					fs::rename(location_path.join(&iso_file_path), new_file_full_path)
-						.await
-						.map_err(|e| {
-							rspc::Error::with_cause(
-								ErrorCode::Conflict,
-								"Failed to rename file".to_string(),
-								e,
-							)
-						})?;
-
 					Ok(())
 				}
 

core/src/object/fs/copy.rs

@@ -13,7 +13,7 @@ use crate::{
 	},
 };
 
-use std::{hash::Hash, path::PathBuf};
+use std::{ffi::OsStr, hash::Hash, path::PathBuf};
 
 use serde::{Deserialize, Serialize};
 use serde_json::json;
@@ -22,8 +22,9 @@ use tokio::{fs, io};
 use tracing::{trace, warn};
 
 use super::{
-	construct_target_filename, error::FileSystemJobsError, fetch_source_and_target_location_paths,
-	get_file_data_from_isolated_file_path, get_many_files_datas, FileData,
+	append_digit_to_filename, construct_target_filename, error::FileSystemJobsError,
+	fetch_source_and_target_location_paths, get_file_data_from_isolated_file_path,
+	get_many_files_datas, FileData,
 };
 
 #[derive(Serialize, Deserialize, Debug, Clone)]
@@ -37,7 +38,6 @@ pub struct FileCopierJobInit {
 	pub target_location_id: location::id::Type,
 	pub sources_file_path_ids: Vec<file_path::id::Type>,
 	pub target_location_relative_directory_path: PathBuf,
-	pub target_file_name_suffix: Option<String>,
 }
 
 #[derive(Serialize, Deserialize, Debug)]
@@ -80,10 +80,7 @@ impl StatefulJob for FileCopierJobInit {
 					&init.target_location_relative_directory_path,
 				);
 
-				full_target_path.push(construct_target_filename(
-					&file_data,
-					&init.target_file_name_suffix,
-				)?);
+				full_target_path.push(construct_target_filename(&file_data)?);
 
 				Ok::<_, MissingFieldError>(FileCopierJobStep {
 					source_file_data: file_data,
@@ -173,17 +170,65 @@ impl StatefulJob for FileCopierJobInit {
 			}
 
 			Ok(more_steps.into())
-		} else if &source_file_data.full_path == target_full_path {
-			// File is already here, do nothing
-			Ok(().into())
 		} else {
 			match fs::metadata(target_full_path).await {
 				Ok(_) => {
-					// only skip as it could be half way through a huge directory copy and run into an issue
-					warn!(
-						"Skipping {} as it would be overwritten",
-						target_full_path.display()
-					);
+					let new_file_name =
+						target_full_path
+							.file_stem()
+							.ok_or(JobError::JobDataNotFound(
+								"No stem on file path, but it's supposed to be a file".to_string(),
+							))?;
+
+					let new_file_full_path_without_suffix = target_full_path.parent().map_or_else(
+						|| {
+							Err(JobError::JobDataNotFound(
+								"No parent for file path, which is supposed to be directory"
+									.to_string(),
+							))
+						},
+						|x| Ok(x.to_path_buf()),
+					)?;
+
+					for i in 1..u32::MAX {
+						let mut new_file_full_path_candidate =
+							new_file_full_path_without_suffix.clone();
+
+						append_digit_to_filename(
+							&mut new_file_full_path_candidate,
+							new_file_name.to_str().ok_or(JobError::JobDataNotFound(
+								"Unable to convert file name to &str".to_string(),
+							))?,
+							target_full_path.extension().and_then(OsStr::to_str),
+							i,
+						);
+
+						match fs::metadata(&new_file_full_path_candidate).await {
+							Ok(_) => {
+								// This candidate already exists, so we try the next one
+								continue;
+							}
+							Err(e) if e.kind() == io::ErrorKind::NotFound => {
+								fs::copy(
+									&source_file_data.full_path,
+									&new_file_full_path_candidate,
+								)
+								.await
+								// Using the ? here because we don't want to increase the completed task
+								// count in case of file system errors
+								.map_err(|e| {
+									FileIOError::from((new_file_full_path_candidate, e))
+								})?;
+
+								break;
+							}
+							Err(e) => {
+								return Err(
+									FileIOError::from((new_file_full_path_candidate, e)).into()
+								)
+							}
+						}
+					}
 
 					Ok(JobRunErrors(vec![FileSystemJobsError::WouldOverwrite(
 						target_full_path.clone().into_boxed_path(),

core/src/object/fs/cut.rs

@@ -84,7 +84,7 @@ impl StatefulJob for FileCutterJobInit {
 	) -> Result<JobStepOutput<Self::Step, Self::RunMetadata>, JobError> {
 		let full_output = data
 			.full_target_directory_path
-			.join(construct_target_filename(file_data, &None)?);
+			.join(construct_target_filename(file_data)?);
 
 		if file_data.full_path == full_output {
 			// File is already here, do nothing

core/src/object/fs/mod.rs

@@ -9,6 +9,8 @@ use crate::{
 
 use std::path::{Path, PathBuf};
 
+use once_cell::sync::Lazy;
+use regex::Regex;
 use serde::{Deserialize, Serialize};
 
 pub mod create;
@@ -25,6 +27,9 @@ pub mod error;
 
 use error::FileSystemJobsError;
 
+static DUPLICATE_PATTERN: Lazy<Regex> =
+	Lazy::new(|| Regex::new(r" \(\d+\)").expect("Failed to compile hardcoded regex"));
+
 // pub const BYTES_EXT: &str = ".bytes";
 
 #[derive(Serialize, Deserialize, Debug, Clone, Eq, PartialEq)]
@@ -137,41 +142,44 @@ pub async fn fetch_source_and_target_location_paths(
 	}
 }
 
-fn construct_target_filename(
-	source_file_data: &FileData,
-	target_file_name_suffix: &Option<String>,
-) -> Result<String, MissingFieldError> {
+fn construct_target_filename(source_file_data: &FileData) -> Result<String, MissingFieldError> {
 	// extension wizardry for cloning and such
 	// if no suffix has been selected, just use the file name
 	// if a suffix is provided and it's a directory, use the directory name + suffix
 	// if a suffix is provided and it's a file, use the (file name + suffix).extension
 
-	Ok(if let Some(ref suffix) = target_file_name_suffix {
-		if maybe_missing(source_file_data.file_path.is_dir, "file_path.is_dir")?
+	Ok(
+		if *maybe_missing(&source_file_data.file_path.is_dir, "file_path.is_dir")?
 			|| source_file_data.file_path.extension.is_none()
 			|| source_file_data.file_path.extension == Some(String::new())
 		{
-			format!(
-				"{}{suffix}",
-				maybe_missing(&source_file_data.file_path.name, "file_path.name")?
-			)
+			maybe_missing(&source_file_data.file_path.name, "file_path.name")?.clone()
 		} else {
 			format!(
-				"{}{suffix}.{}",
+				"{}.{}",
 				maybe_missing(&source_file_data.file_path.name, "file_path.name")?,
-				maybe_missing(&source_file_data.file_path.extension, "file_path.extension")?,
+				maybe_missing(&source_file_data.file_path.extension, "file_path.extension")?
 			)
-		}
-	} else if *maybe_missing(&source_file_data.file_path.is_dir, "file_path.is_dir")?
-		|| source_file_data.file_path.extension.is_none()
-		|| source_file_data.file_path.extension == Some(String::new())
-	{
-		maybe_missing(&source_file_data.file_path.name, "file_path.name")?.clone()
-	} else {
-		format!(
-			"{}.{}",
-			maybe_missing(&source_file_data.file_path.name, "file_path.name")?,
-			maybe_missing(&source_file_data.file_path.extension, "file_path.extension")?
-		)
-	})
+		},
+	)
+}
+
+pub fn append_digit_to_filename(
+	final_path: &mut PathBuf,
+	file_name: &str,
+	ext: Option<&str>,
+	current_int: u32,
+) {
+	let new_file_name = if let Some(found) = DUPLICATE_PATTERN.find_iter(file_name).last() {
+		&file_name[..found.start()]
+	} else {
+		file_name
+	}
+	.to_string();
+
+	if let Some(ext) = ext {
+		final_path.push(format!("{} ({current_int}).{}", new_file_name, ext));
+	} else {
+		final_path.push(format!("{new_file_name} ({current_int})"));
+	}
 }

interface/app/$libraryId/Explorer/ContextMenu/FilePath/CutCopyItems.tsx

@@ -64,8 +64,7 @@ export const CutCopyItems = new ConditionalItem({
 								source_location_id: locationId,
 								sources_file_path_ids: selectedFilePaths.map((p) => p.id),
 								target_location_id: locationId,
-								target_location_relative_directory_path: path ?? '/',
-								target_file_name_suffix: ' copy'
+								target_location_relative_directory_path: path ?? '/'
 							});
 						} catch (error) {
 							toast.error({

interface/app/$libraryId/Explorer/ParentContextMenu.tsx

@@ -47,8 +47,7 @@ export default (props: PropsWithChildren) => {
 										source_location_id: sourceLocationId,
 										sources_file_path_ids: [...sourcePathIds],
 										target_location_id: parent.location.id,
-										target_location_relative_directory_path: path,
-										target_file_name_suffix: sameLocation ? ' copy' : null
+										target_location_relative_directory_path: path
 									});
 								} else if (sameLocation) {
 									toast.error('File already exists in this location');

packages/client/src/core.ts

@@ -159,7 +159,7 @@ export type ExplorerLayout = "grid" | "list" | "media"
 
 export type ExplorerSettings<TOrder> = { layoutMode: ExplorerLayout | null; gridItemSize: number | null; mediaColumns: number | null; mediaAspectSquare: boolean | null; mediaViewWithDescendants: boolean | null; openOnDoubleClick: DoubleClickAction | null; showBytesInGridView: boolean | null; colVisibility: { [key: string]: boolean } | null; colSizes: { [key: string]: number } | null; order?: TOrder | null; showHiddenFiles?: boolean }
 
-export type FileCopierJobInit = { source_location_id: number; target_location_id: number; sources_file_path_ids: number[]; target_location_relative_directory_path: string; target_file_name_suffix: string | null }
+export type FileCopierJobInit = { source_location_id: number; target_location_id: number; sources_file_path_ids: number[]; target_location_relative_directory_path: string }
 
 export type FileCutterJobInit = { source_location_id: number; target_location_id: number; sources_file_path_ids: number[]; target_location_relative_directory_path: string }