From bf46c1ca102d0312d8a7c503b8b974616f5fe85d Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 9 Jun 2026 13:45:16 +0000 Subject: [PATCH] perf: make StoragePath::join and File builders consume self to avoid clones MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit StoragePath::join deep-cloned the whole Vec (every segment String) just to append one element. Take self by value and push in place. All callers pass owned values except PathService::create_file_path, which holds a borrow and now clones explicitly — the same copy the old &self join already made. File::with_name / with_folder / with_size took &self and rebuilt the struct, cloning every carried-over field (id, mime_type, folder_id, blob_hash, ...). Consume self and mutate only the fields that change. Behaviour is identical; the fallible builders now drop the input on Err, which is fine for these rename/move/resize transforms (all current callers replace the file). Impact is small in practice — with_folder/with_size have no callers and with_name is test-only, while the one hot join caller (create_file_path) must copy segments regardless — but the consuming form is the idiomatic one. Verified: cargo fmt + clippy --all-features --all-targets -D warnings clean; domain tests (path_service::, entities::file::) pass — 30 + 6. https://claude.ai/code/session_01UtfkS3nZF1vrF5jNAps6wV --- src/domain/entities/file.rs | 74 +++++++-------------- src/domain/services/path_service.rs | 12 ++-- src/infrastructure/services/path_service.rs | 4 +- 3 files changed, 31 insertions(+), 59 deletions(-) diff --git a/src/domain/entities/file.rs b/src/domain/entities/file.rs index f42147f9..0c379040 100644 --- a/src/domain/entities/file.rs +++ b/src/domain/entities/file.rs @@ -388,45 +388,36 @@ impl File { // Methods to create new versions of the file (immutable) /// Creates a new version of the file with updated name - pub fn with_name(&self, new_name: String) -> FileResult { + pub fn with_name(mut self, new_name: String) -> FileResult { let new_name = normalize_storage_name(&new_name); if let Err(reason) = validate_storage_name(&new_name) { return Err(FileError::InvalidFileName(format!("{new_name}: {reason}"))); } - // Update path based on name - let parent_path = self.storage_path.parent(); - let new_storage_path = match parent_path { + // Recompute the path from the unchanged parent + the new name. + let new_storage_path = match self.storage_path.parent() { Some(parent) => parent.join(&new_name), None => StoragePath::from_string(&new_name), }; - // Update string representation - let new_path_string = new_storage_path.to_string(); - let now = std::time::SystemTime::now() .duration_since(std::time::UNIX_EPOCH) .unwrap_or_default() .as_secs(); - Ok(Self { - id: self.id.clone(), - name: new_name, - storage_path: new_storage_path, - path_string: new_path_string, - size: self.size, - mime_type: self.mime_type.clone(), - folder_id: self.folder_id.clone(), - created_at: self.created_at, - modified_at: now, - owner_id: self.owner_id, - blob_hash: self.blob_hash.clone(), - }) + // Consume `self` and mutate in place — only the path, name and mtime + // change; id / mime_type / folder_id / blob_hash are carried over + // without the per-field clone the old `&self` builder paid. + self.path_string = new_storage_path.to_string(); + self.storage_path = new_storage_path; + self.name = new_name; + self.modified_at = now; + Ok(self) } /// Creates a new version of the file with updated folder pub fn with_folder( - &self, + mut self, folder_id: Option, folder_path: Option, ) -> FileResult { @@ -436,49 +427,30 @@ impl File { None => StoragePath::from_string(&self.name), // Root }; - // Update string representation - let new_path_string = new_storage_path.to_string(); - let now = std::time::SystemTime::now() .duration_since(std::time::UNIX_EPOCH) .unwrap_or_default() .as_secs(); - Ok(Self { - id: self.id.clone(), - name: self.name.clone(), - storage_path: new_storage_path, - path_string: new_path_string, - size: self.size, - mime_type: self.mime_type.clone(), - folder_id, - created_at: self.created_at, - modified_at: now, - owner_id: self.owner_id, - blob_hash: self.blob_hash.clone(), - }) + // Consume `self`: only the path, folder_id and mtime change. + self.path_string = new_storage_path.to_string(); + self.storage_path = new_storage_path; + self.folder_id = folder_id; + self.modified_at = now; + Ok(self) } /// Creates a new version of the file with updated size - pub fn with_size(&self, new_size: u64) -> Self { + pub fn with_size(mut self, new_size: u64) -> Self { let now = std::time::SystemTime::now() .duration_since(std::time::UNIX_EPOCH) .unwrap_or_default() .as_secs(); - Self { - id: self.id.clone(), - name: self.name.clone(), - storage_path: self.storage_path.clone(), - path_string: self.path_string.clone(), - size: new_size, - mime_type: self.mime_type.clone(), - folder_id: self.folder_id.clone(), - created_at: self.created_at, - modified_at: now, - owner_id: self.owner_id, - blob_hash: self.blob_hash.clone(), - } + // Consume `self`: only size and mtime change — no per-field clone. + self.size = new_size; + self.modified_at = now; + self } } diff --git a/src/domain/services/path_service.rs b/src/domain/services/path_service.rs index 70fa4b4c..e1ebbe71 100644 --- a/src/domain/services/path_service.rs +++ b/src/domain/services/path_service.rs @@ -103,18 +103,16 @@ impl StoragePath { Self { segments } } - /// Appends a segment to the path. + /// Appends a segment to the path, consuming `self` so the existing + /// segment buffer is reused instead of deep-cloned. /// /// Traversal segments (`.`, `..`) and segments containing `/` are /// silently ignored to prevent path-traversal attacks. - pub fn join(&self, segment: &str) -> Self { - let mut new_segments = self.segments.clone(); + pub fn join(mut self, segment: &str) -> Self { if Self::is_safe_segment(segment) { - new_segments.push(segment.to_string()); - } - Self { - segments: new_segments, + self.segments.push(segment.to_string()); } + self } /// Gets the file name (last segment) diff --git a/src/infrastructure/services/path_service.rs b/src/infrastructure/services/path_service.rs index 6c9189d2..dc60e341 100644 --- a/src/infrastructure/services/path_service.rs +++ b/src/infrastructure/services/path_service.rs @@ -64,7 +64,9 @@ impl PathService { /// Creates a file path within a folder pub fn create_file_path(&self, folder_path: &StoragePath, file_name: &str) -> StoragePath { - folder_path.join(file_name) + // `join` consumes its receiver to reuse the buffer; we only hold a + // borrow here, so clone first — the same copy the old `&self` join did. + folder_path.clone().join(file_name) } /// Checks if a path is a direct child of another