fix(trash)+refactor(file life cycle)

* fix issue with the empty trash (wasn't calling thumbnail clean up)
 * refactor file service life cycle (TrashService don't call directly ThumbnailService, but call the on_file_deleted() hook
 * remove unused mehod: _validate_user_ownership()
This commit is contained in:
Edouard Vanbelle
2026-05-21 23:56:14 +02:00
parent dd68d783e0
commit 76a85949e7
8 changed files with 123 additions and 85 deletions
@@ -0,0 +1,47 @@
use std::future::Future;
use std::pin::Pin;
use std::sync::Arc;
use crate::application::ports::file_lifecycle::FileDeletedHook;
/// Composite dispatcher for file lifecycle events.
///
/// Aggregates all `FileDeletedHook` implementations and fans out each event to
/// every registered handler. Services hold a single `Arc<dyn FileDeletedHook>`
/// pointing here — new handlers are added once, in DI, without touching the
/// services themselves.
pub struct FileLifecycleService {
deleted: Vec<Arc<dyn FileDeletedHook>>,
}
impl Default for FileLifecycleService {
fn default() -> Self {
Self::new()
}
}
impl FileLifecycleService {
pub fn new() -> Self {
Self {
deleted: Vec::new(),
}
}
pub fn with_deleted_hook(mut self, hook: Arc<dyn FileDeletedHook>) -> Self {
self.deleted.push(hook);
self
}
}
impl FileDeletedHook for FileLifecycleService {
fn on_file_deleted<'a>(
&'a self,
file_id: &'a str,
) -> Pin<Box<dyn Future<Output = ()> + Send + 'a>> {
Box::pin(async move {
for hook in &self.deleted {
hook.on_file_deleted(file_id).await;
}
})
}
}
@@ -29,8 +29,8 @@ pub struct FileManagementService {
trash_service: Option<Arc<TrashService>>, trash_service: Option<Arc<TrashService>>,
content_cache: Option<Arc<FileContentCache>>, content_cache: Option<Arc<FileContentCache>>,
authz: Arc<PgAclEngine>, authz: Arc<PgAclEngine>,
/// Hooks fired after a file is permanently deleted. /// Hook fired after a file is permanently deleted (typically the FileLifecycleService composite).
file_deleted_hooks: Vec<Arc<dyn FileDeletedHook>>, file_deleted_hook: Option<Arc<dyn FileDeletedHook>>,
} }
impl FileManagementService { impl FileManagementService {
@@ -52,13 +52,13 @@ impl FileManagementService {
trash_service, trash_service,
content_cache, content_cache,
authz, authz,
file_deleted_hooks: Vec::new(), file_deleted_hook: None,
} }
} }
/// Registers a hook to fire after a file is permanently deleted. /// Sets the lifecycle hook fired after a file is permanently deleted.
pub fn with_file_deleted_hook(mut self, hook: Arc<dyn FileDeletedHook>) -> Self { pub fn with_file_deleted_hook(mut self, hook: Arc<dyn FileDeletedHook>) -> Self {
self.file_deleted_hooks.push(hook); self.file_deleted_hook = Some(hook);
self self
} }
@@ -185,7 +185,7 @@ impl FileManagementService {
if let Some(cc) = &self.content_cache { if let Some(cc) = &self.content_cache {
cc.invalidate(id).await; cc.invalidate(id).await;
} }
for hook in &self.file_deleted_hooks { if let Some(hook) = &self.file_deleted_hook {
hook.on_file_deleted(id).await; hook.on_file_deleted(id).await;
} }
info!("File permanently deleted: {}", id); info!("File permanently deleted: {}", id);
+1
View File
@@ -6,6 +6,7 @@ pub mod calendar_service;
pub mod contact_service; pub mod contact_service;
pub mod device_auth_service; pub mod device_auth_service;
pub mod favorites_service; pub mod favorites_service;
pub mod file_lifecycle_service;
pub mod file_management_service; pub mod file_management_service;
pub mod file_retrieval_service; pub mod file_retrieval_service;
pub mod file_upload_service; pub mod file_upload_service;
+27 -67
View File
@@ -7,6 +7,7 @@ use crate::application::dtos::display_helpers::{
}; };
use crate::application::dtos::trash_dto::TrashedItemDto; use crate::application::dtos::trash_dto::TrashedItemDto;
use crate::application::ports::authorization_ports::AuthorizationEngine; use crate::application::ports::authorization_ports::AuthorizationEngine;
use crate::application::ports::file_lifecycle::FileDeletedHook;
use crate::application::ports::storage_ports::{FileReadPort, FileWritePort}; use crate::application::ports::storage_ports::{FileReadPort, FileWritePort};
use crate::application::ports::trash_ports::TrashUseCase; use crate::application::ports::trash_ports::TrashUseCase;
use crate::common::errors::{DomainError, ErrorKind, Result}; use crate::common::errors::{DomainError, ErrorKind, Result};
@@ -21,7 +22,6 @@ use crate::infrastructure::repositories::pg::trash_db_repository::TrashDbReposit
use crate::infrastructure::services::dedup_service::DedupService; use crate::infrastructure::services::dedup_service::DedupService;
use crate::infrastructure::services::file_content_cache::FileContentCache; use crate::infrastructure::services::file_content_cache::FileContentCache;
use crate::infrastructure::services::pg_acl_engine::PgAclEngine; use crate::infrastructure::services::pg_acl_engine::PgAclEngine;
use crate::infrastructure::services::thumbnail_service::ThumbnailService;
/** /**
* Application service for trash operations. * Application service for trash operations.
@@ -53,8 +53,8 @@ pub struct TrashService {
/// orphaned blob files and thumbnails that the PG trigger cannot reach. /// orphaned blob files and thumbnails that the PG trigger cannot reach.
dedup_service: Arc<DedupService>, dedup_service: Arc<DedupService>,
/// Thumbnail service for cleaning up thumbnails on permanent delete /// Hook fired after a file is permanently deleted (typically the FileLifecycleService composite).
thumbnail_service: Option<Arc<ThumbnailService>>, file_deleted_hook: Option<Arc<dyn FileDeletedHook>>,
/// Content cache — invalidated when files are permanently deleted from trash. /// Content cache — invalidated when files are permanently deleted from trash.
content_cache: Option<Arc<FileContentCache>>, content_cache: Option<Arc<FileContentCache>>,
@@ -75,7 +75,6 @@ impl TrashService {
folder_storage_port: Arc<FolderDbRepository>, folder_storage_port: Arc<FolderDbRepository>,
retention_days: u32, retention_days: u32,
dedup_service: Arc<DedupService>, dedup_service: Arc<DedupService>,
thumbnail_service: Option<Arc<ThumbnailService>>,
content_cache: Option<Arc<FileContentCache>>, content_cache: Option<Arc<FileContentCache>>,
authz: Arc<PgAclEngine>, authz: Arc<PgAclEngine>,
) -> Self { ) -> Self {
@@ -85,13 +84,19 @@ impl TrashService {
file_write_port, file_write_port,
folder_storage_port, folder_storage_port,
dedup_service, dedup_service,
thumbnail_service, file_deleted_hook: None,
content_cache, content_cache,
authz, authz,
retention_days, retention_days,
} }
} }
/// Sets the lifecycle hook fired after a file is permanently deleted.
pub fn with_file_deleted_hook(mut self, hook: Arc<dyn FileDeletedHook>) -> Self {
self.file_deleted_hook = Some(hook);
self
}
/// Converts a TrashedItem entity to a DTO /// Converts a TrashedItem entity to a DTO
fn to_dto(&self, item: TrashedItem) -> TrashedItemDto { fn to_dto(&self, item: TrashedItem) -> TrashedItemDto {
// Calculate days_until_deletion before moving item fields // Calculate days_until_deletion before moving item fields
@@ -130,46 +135,6 @@ impl TrashService {
icon_special_class, icon_special_class,
} }
} }
/// Validates that the given user owns the trashed item.
/// Returns an error if the item does not exist or belongs to a different user.
#[instrument(skip(self))]
async fn _validate_user_ownership(&self, item_id: &str, user_id: &str) -> Result<()> {
let item_uuid = Uuid::parse_str(item_id)
.map_err(|e| DomainError::validation_error(format!("Invalid item ID: {}", e)))?;
let user_uuid = Uuid::parse_str(user_id)
.map_err(|e| DomainError::validation_error(format!("Invalid user ID: {}", e)))?;
match self
.trash_repository
.get_trash_item(&item_uuid, &user_uuid)
.await?
{
Some(item) => {
if item.user_id() != user_uuid {
error!(
"User {} attempted to access trash item {} owned by {}",
user_id,
item_id,
item.user_id()
);
return Err(DomainError::access_denied(
"TrashItem",
"You do not have permission to access this trash item",
));
}
Ok(())
}
None => {
// Item not found for this user — treat as authorization error
// to avoid leaking existence information
Err(DomainError::not_found(
"TrashItem",
format!("{} (user: {})", item_id, user_id),
))
}
}
}
} }
impl TrashUseCase for TrashService { impl TrashUseCase for TrashService {
@@ -617,12 +582,8 @@ impl TrashUseCase for TrashService {
} }
} }
// Best-effort thumbnail cleanup — thumbnails are cache if let Some(hook) = &self.file_deleted_hook {
// artifacts, so failure must not block file deletion. hook.on_file_deleted(&file_id).await;
if let Some(thumb) = &self.thumbnail_service
&& let Err(e) = thumb.delete_thumbnails(&file_id).await
{
warn!("Failed to delete thumbnails for file {}: {}", file_id, e);
} }
} }
TrashedItemType::Folder => { TrashedItemType::Folder => {
@@ -714,18 +675,20 @@ impl TrashUseCase for TrashService {
async fn empty_trash(&self, user_id: Uuid) -> Result<()> { async fn empty_trash(&self, user_id: Uuid) -> Result<()> {
info!("Emptying trash for user {}", user_id); info!("Emptying trash for user {}", user_id);
// Collect trashed file IDs BEFORE bulk-deleting so we can clean up // Collect ALL trashed file IDs BEFORE bulk-deleting so hooks (thumbnail
// their thumbnails afterward. This is best-effort — if the query // cleanup, etc.) can run afterward. We use get_all_trashed_file_ids (not
// fails we still proceed with the bulk delete. // get_trash_items) because the trash_items view excludes files inside a
let trashed_file_ids: Vec<String> = if self.thumbnail_service.is_some() { // trashed folder — those files will still be deleted by clear_trash via
match self.trash_repository.get_trash_items(&user_id).await { // the folder CASCADE, but their hooks would otherwise be missed.
Ok(items) => items let trashed_file_ids: Vec<String> = if self.file_deleted_hook.is_some() {
.iter() match self
.filter(|i| matches!(i.item_type(), TrashedItemType::File)) .trash_repository
.map(|i| i.original_id().to_string()) .get_all_trashed_file_ids(&user_id)
.collect(), .await
{
Ok(ids) => ids,
Err(e) => { Err(e) => {
warn!("Could not list trashed items for thumbnail cleanup: {}", e); warn!("Could not list trashed files for hook cleanup: {}", e);
Vec::new() Vec::new()
} }
} }
@@ -758,12 +721,9 @@ impl TrashUseCase for TrashService {
} }
} }
// Best-effort thumbnail cleanup for all deleted files if let Some(hook) = &self.file_deleted_hook {
if let Some(thumb) = &self.thumbnail_service {
for file_id in &trashed_file_ids { for file_id in &trashed_file_ids {
if let Err(e) = thumb.delete_thumbnails(file_id).await { hook.on_file_deleted(file_id).await;
warn!("Failed to delete thumbnails for file {}: {}", file_id, e);
}
} }
} }
@@ -402,6 +402,11 @@ impl TrashRepository for MockTrashRepository {
Ok(()) Ok(())
} }
async fn get_all_trashed_file_ids(&self, _user_id: &Uuid) -> Result<Vec<String>> {
let files = self.trashed_files.lock().unwrap();
Ok(files.keys().cloned().collect())
}
async fn delete_expired_bulk(&self) -> Result<(u64, u64)> { async fn delete_expired_bulk(&self) -> Result<(u64, u64)> {
let mut items = self.trash_items.lock().unwrap(); let mut items = self.trash_items.lock().unwrap();
let now = Utc::now(); let now = Utc::now();
+21 -12
View File
@@ -44,6 +44,7 @@ use crate::infrastructure::services::trash_cleanup_service::TrashCleanupService;
use crate::application::services::app_password_service::AppPasswordService; use crate::application::services::app_password_service::AppPasswordService;
use crate::application::services::calendar_service::CalendarService; use crate::application::services::calendar_service::CalendarService;
use crate::application::services::device_auth_service::DeviceAuthService; use crate::application::services::device_auth_service::DeviceAuthService;
use crate::application::services::file_lifecycle_service::FileLifecycleService;
use crate::application::services::music_service::MusicService; use crate::application::services::music_service::MusicService;
use crate::application::services::storage_usage_service::StorageUsageService; use crate::application::services::storage_usage_service::StorageUsageService;
use crate::application::services::wopi_lock_service::WopiLockService; use crate::application::services::wopi_lock_service::WopiLockService;
@@ -274,10 +275,14 @@ impl AppServiceFactory {
"Core services initialized: path service, file content cache, thumbnails, chunked upload, image transcode, dedup (PRIMARY blob storage)" "Core services initialized: path service, file content cache, thumbnails, chunked upload, image transcode, dedup (PRIMARY blob storage)"
); );
let file_lifecycle =
Arc::new(FileLifecycleService::new().with_deleted_hook(thumbnail_service.clone()));
Ok(CoreServices { Ok(CoreServices {
path_service, path_service,
file_content_cache, file_content_cache,
thumbnail_service, thumbnail_service,
file_lifecycle,
chunked_upload_service, chunked_upload_service,
image_transcode_service, image_transcode_service,
dedup_service, dedup_service,
@@ -392,7 +397,7 @@ impl AppServiceFactory {
Some(core.file_content_cache.clone()), Some(core.file_content_cache.clone()),
authz.clone(), authz.clone(),
) )
.with_file_deleted_hook(core.thumbnail_service.clone()), .with_file_deleted_hook(core.file_lifecycle.clone()),
); );
let file_use_case_factory = Arc::new(AppFileUseCaseFactory::new( let file_use_case_factory = Arc::new(AppFileUseCaseFactory::new(
@@ -463,17 +468,19 @@ impl AppServiceFactory {
let trash_repo = repos.trash_repository.as_ref()?; let trash_repo = repos.trash_repository.as_ref()?;
// Wire ports directly to TrashService — no adapter layer needed // Wire ports directly to TrashService — no adapter layer needed
let service = Arc::new(TrashService::new( let service = Arc::new(
trash_repo.clone(), TrashService::new(
repos.file_read_repository.clone(), trash_repo.clone(),
repos.file_write_repository.clone(), repos.file_read_repository.clone(),
repos.folder_repository.clone(), repos.file_write_repository.clone(),
self.config.storage.trash_retention_days, repos.folder_repository.clone(),
core.dedup_service.clone(), self.config.storage.trash_retention_days,
Some(core.thumbnail_service.clone()), core.dedup_service.clone(),
Some(core.file_content_cache.clone()), Some(core.file_content_cache.clone()),
authz.clone(), authz.clone(),
)); )
.with_file_deleted_hook(core.file_lifecycle.clone()),
);
// Initialize cleanup service (bulk-deletes expired items in 2 SQL queries) // Initialize cleanup service (bulk-deletes expired items in 2 SQL queries)
let cleanup_service = TrashCleanupService::new( let cleanup_service = TrashCleanupService::new(
@@ -1021,6 +1028,8 @@ pub struct CoreServices {
pub path_service: Arc<PathService>, pub path_service: Arc<PathService>,
pub file_content_cache: Arc<FileContentCache>, pub file_content_cache: Arc<FileContentCache>,
pub thumbnail_service: Arc<ThumbnailService>, pub thumbnail_service: Arc<ThumbnailService>,
/// Composite lifecycle dispatcher — register new permanent-delete hooks here only.
pub file_lifecycle: Arc<FileLifecycleService>,
pub chunked_upload_service: Arc<ChunkedUploadService>, pub chunked_upload_service: Arc<ChunkedUploadService>,
pub image_transcode_service: Arc<ImageTranscodeService>, pub image_transcode_service: Arc<ImageTranscodeService>,
pub dedup_service: Arc<DedupService>, pub dedup_service: Arc<DedupService>,
@@ -11,6 +11,11 @@ pub trait TrashRepository: Send + Sync {
async fn delete_permanently(&self, id: &Uuid, user_id: &Uuid) -> Result<()>; async fn delete_permanently(&self, id: &Uuid, user_id: &Uuid) -> Result<()>;
async fn clear_trash(&self, user_id: &Uuid) -> Result<()>; async fn clear_trash(&self, user_id: &Uuid) -> Result<()>;
/// All trashed file IDs for this user, regardless of parent folder trash status.
/// Used by empty_trash for thumbnail cleanup — the view used by get_trash_items
/// excludes files inside trashed folders, which would miss their ext thumbnails.
async fn get_all_trashed_file_ids(&self, user_id: &Uuid) -> Result<Vec<String>>;
/// Bulk-delete all expired trash items (files + folders) in a single /// Bulk-delete all expired trash items (files + folders) in a single
/// transaction. Returns `(files_deleted, folders_deleted)`. /// transaction. Returns `(files_deleted, folders_deleted)`.
async fn delete_expired_bulk(&self) -> Result<(u64, u64)>; async fn delete_expired_bulk(&self) -> Result<(u64, u64)>;
@@ -157,6 +157,17 @@ impl TrashRepository for TrashDbRepository {
Ok(()) Ok(())
} }
async fn get_all_trashed_file_ids(&self, user_id: &Uuid) -> Result<Vec<String>> {
let rows = sqlx::query_scalar::<_, String>(
"SELECT id::text FROM storage.files WHERE user_id = $1 AND is_trashed = TRUE",
)
.bind(user_id)
.fetch_all(self.pool.as_ref())
.await
.map_err(|e| DomainError::internal_error("TrashDb", format!("all_trashed_files: {e}")))?;
Ok(rows)
}
async fn delete_expired_bulk(&self) -> Result<(u64, u64)> { async fn delete_expired_bulk(&self) -> Result<(u64, u64)> {
let cutoff = Utc::now() - chrono::Duration::days(self.retention_days); let cutoff = Utc::now() - chrono::Duration::days(self.retention_days);