perf: make StoragePath::join and File builders consume self to avoid clones
StoragePath::join deep-cloned the whole Vec<String> (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
This commit is contained in:
+23
-51
@@ -388,45 +388,36 @@ impl File {
|
|||||||
// Methods to create new versions of the file (immutable)
|
// Methods to create new versions of the file (immutable)
|
||||||
|
|
||||||
/// Creates a new version of the file with updated name
|
/// Creates a new version of the file with updated name
|
||||||
pub fn with_name(&self, new_name: String) -> FileResult<Self> {
|
pub fn with_name(mut self, new_name: String) -> FileResult<Self> {
|
||||||
let new_name = normalize_storage_name(&new_name);
|
let new_name = normalize_storage_name(&new_name);
|
||||||
if let Err(reason) = validate_storage_name(&new_name) {
|
if let Err(reason) = validate_storage_name(&new_name) {
|
||||||
return Err(FileError::InvalidFileName(format!("{new_name}: {reason}")));
|
return Err(FileError::InvalidFileName(format!("{new_name}: {reason}")));
|
||||||
}
|
}
|
||||||
|
|
||||||
// Update path based on name
|
// Recompute the path from the unchanged parent + the new name.
|
||||||
let parent_path = self.storage_path.parent();
|
let new_storage_path = match self.storage_path.parent() {
|
||||||
let new_storage_path = match parent_path {
|
|
||||||
Some(parent) => parent.join(&new_name),
|
Some(parent) => parent.join(&new_name),
|
||||||
None => StoragePath::from_string(&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()
|
let now = std::time::SystemTime::now()
|
||||||
.duration_since(std::time::UNIX_EPOCH)
|
.duration_since(std::time::UNIX_EPOCH)
|
||||||
.unwrap_or_default()
|
.unwrap_or_default()
|
||||||
.as_secs();
|
.as_secs();
|
||||||
|
|
||||||
Ok(Self {
|
// Consume `self` and mutate in place — only the path, name and mtime
|
||||||
id: self.id.clone(),
|
// change; id / mime_type / folder_id / blob_hash are carried over
|
||||||
name: new_name,
|
// without the per-field clone the old `&self` builder paid.
|
||||||
storage_path: new_storage_path,
|
self.path_string = new_storage_path.to_string();
|
||||||
path_string: new_path_string,
|
self.storage_path = new_storage_path;
|
||||||
size: self.size,
|
self.name = new_name;
|
||||||
mime_type: self.mime_type.clone(),
|
self.modified_at = now;
|
||||||
folder_id: self.folder_id.clone(),
|
Ok(self)
|
||||||
created_at: self.created_at,
|
|
||||||
modified_at: now,
|
|
||||||
owner_id: self.owner_id,
|
|
||||||
blob_hash: self.blob_hash.clone(),
|
|
||||||
})
|
|
||||||
}
|
}
|
||||||
|
|
||||||
/// Creates a new version of the file with updated folder
|
/// Creates a new version of the file with updated folder
|
||||||
pub fn with_folder(
|
pub fn with_folder(
|
||||||
&self,
|
mut self,
|
||||||
folder_id: Option<String>,
|
folder_id: Option<String>,
|
||||||
folder_path: Option<StoragePath>,
|
folder_path: Option<StoragePath>,
|
||||||
) -> FileResult<Self> {
|
) -> FileResult<Self> {
|
||||||
@@ -436,49 +427,30 @@ impl File {
|
|||||||
None => StoragePath::from_string(&self.name), // Root
|
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()
|
let now = std::time::SystemTime::now()
|
||||||
.duration_since(std::time::UNIX_EPOCH)
|
.duration_since(std::time::UNIX_EPOCH)
|
||||||
.unwrap_or_default()
|
.unwrap_or_default()
|
||||||
.as_secs();
|
.as_secs();
|
||||||
|
|
||||||
Ok(Self {
|
// Consume `self`: only the path, folder_id and mtime change.
|
||||||
id: self.id.clone(),
|
self.path_string = new_storage_path.to_string();
|
||||||
name: self.name.clone(),
|
self.storage_path = new_storage_path;
|
||||||
storage_path: new_storage_path,
|
self.folder_id = folder_id;
|
||||||
path_string: new_path_string,
|
self.modified_at = now;
|
||||||
size: self.size,
|
Ok(self)
|
||||||
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(),
|
|
||||||
})
|
|
||||||
}
|
}
|
||||||
|
|
||||||
/// Creates a new version of the file with updated size
|
/// 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()
|
let now = std::time::SystemTime::now()
|
||||||
.duration_since(std::time::UNIX_EPOCH)
|
.duration_since(std::time::UNIX_EPOCH)
|
||||||
.unwrap_or_default()
|
.unwrap_or_default()
|
||||||
.as_secs();
|
.as_secs();
|
||||||
|
|
||||||
Self {
|
// Consume `self`: only size and mtime change — no per-field clone.
|
||||||
id: self.id.clone(),
|
self.size = new_size;
|
||||||
name: self.name.clone(),
|
self.modified_at = now;
|
||||||
storage_path: self.storage_path.clone(),
|
self
|
||||||
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(),
|
|
||||||
}
|
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -103,18 +103,16 @@ impl StoragePath {
|
|||||||
Self { segments }
|
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
|
/// Traversal segments (`.`, `..`) and segments containing `/` are
|
||||||
/// silently ignored to prevent path-traversal attacks.
|
/// silently ignored to prevent path-traversal attacks.
|
||||||
pub fn join(&self, segment: &str) -> Self {
|
pub fn join(mut self, segment: &str) -> Self {
|
||||||
let mut new_segments = self.segments.clone();
|
|
||||||
if Self::is_safe_segment(segment) {
|
if Self::is_safe_segment(segment) {
|
||||||
new_segments.push(segment.to_string());
|
self.segments.push(segment.to_string());
|
||||||
}
|
|
||||||
Self {
|
|
||||||
segments: new_segments,
|
|
||||||
}
|
}
|
||||||
|
self
|
||||||
}
|
}
|
||||||
|
|
||||||
/// Gets the file name (last segment)
|
/// Gets the file name (last segment)
|
||||||
|
|||||||
@@ -64,7 +64,9 @@ impl PathService {
|
|||||||
|
|
||||||
/// Creates a file path within a folder
|
/// Creates a file path within a folder
|
||||||
pub fn create_file_path(&self, folder_path: &StoragePath, file_name: &str) -> StoragePath {
|
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
|
/// Checks if a path is a direct child of another
|
||||||
|
|||||||
Reference in New Issue
Block a user