From ba50a266ba825da8bcede917029d2ca9f4500b68 Mon Sep 17 00:00:00 2001 From: Brandon Thomas Date: Tue, 9 Jun 2026 09:31:23 -0400 Subject: [PATCH] wrap in transaction --- .../bulk_add_folder_media_files_handler.rs | 106 ++++++++++++------ .../bulk_remove_folder_media_files_handler.rs | 80 +++++++++---- 2 files changed, 133 insertions(+), 53 deletions(-) diff --git a/crates/service/web/storyteller_web/src/http_server/endpoints/folders/media_files/bulk_add_folder_media_files_handler.rs b/crates/service/web/storyteller_web/src/http_server/endpoints/folders/media_files/bulk_add_folder_media_files_handler.rs index 3f1b94f5c7..f5ce0f0d03 100644 --- a/crates/service/web/storyteller_web/src/http_server/endpoints/folders/media_files/bulk_add_folder_media_files_handler.rs +++ b/crates/service/web/storyteller_web/src/http_server/endpoints/folders/media_files/bulk_add_folder_media_files_handler.rs @@ -3,8 +3,10 @@ use std::sync::Arc; use actix_web::web::{Json, Path}; use actix_web::{web, HttpRequest}; -use log::warn; -use sqlx::Acquire; +use log::{error, warn}; +use sqlx::pool::PoolConnection; +use sqlx::{Acquire, MySql, Transaction}; +use tokens::tokens::media_files::MediaFileToken; use artcraft_api_defs::folders::media_files::{ BulkAddFolderMediaFilesRequest, BulkAddFolderMediaFilesSuccessResponse, @@ -92,39 +94,10 @@ pub async fn bulk_add_folder_media_files_handler( CommonWebError::from_error(err) })?; - // Insert + recompute the denormalized `maybe_last_media_file_token_1..4` - // columns inside a single transaction so they stay in sync with - // membership. Empty `accepted` is a no-op — no membership change means - // no recompute needed. + // Empty `accepted` is a no-op — no membership change means no + // recompute needed and no transaction to open. if !accepted.is_empty() { - let mut tx = conn.begin().await.map_err(|err| { - warn!("Failed to begin transaction: {:?}", err); - CommonWebError::from_error(err) - })?; - - bulk_insert_folder_media_files(BulkInsertFolderMediaFilesArgs { - folder_token: &path.folder_token, - media_file_tokens: &accepted, - mysql_executor: &mut *tx, - phantom: PhantomData, - }).await.map_err(|err| { - warn!("bulk_insert_folder_media_files failed: {:?}", err); - CommonWebError::from_error(err) - })?; - - recompute_folder_last_media_files(RecomputeFolderLastMediaFilesArgs { - folder_token: &path.folder_token, - mysql_executor: &mut *tx, - phantom: PhantomData, - }).await.map_err(|err| { - warn!("recompute_folder_last_media_files failed: {:?}", err); - CommonWebError::from_error(err) - })?; - - tx.commit().await.map_err(|err| { - warn!("Failed to commit bulk_add transaction: {:?}", err); - CommonWebError::from_error(err) - })?; + perform_atomic_add(&mut conn, &path.folder_token, &accepted).await?; } Ok(Json(BulkAddFolderMediaFilesSuccessResponse { @@ -132,3 +105,68 @@ pub async fn bulk_add_folder_media_files_handler( accepted_media_file_tokens: accepted, })) } + +/// Open a transaction, run the insert + recompute as one unit, and +/// commit on success. On any failure inside [`perform_add_work`] the +/// transaction is explicitly rolled back before the original error is +/// re-raised — sqlx would roll back on drop too, but doing it explicitly +/// makes the failure path obvious and surfaces any rollback error in +/// the log. +async fn perform_atomic_add( + conn: &mut PoolConnection, + folder_token: &FolderToken, + media_file_tokens: &[MediaFileToken], +) -> Result<(), CommonWebError> { + let mut tx = conn.begin().await.map_err(|err| { + warn!("Failed to begin transaction: {:?}", err); + CommonWebError::from_error(err) + })?; + + let work_result = perform_add_work(&mut tx, folder_token, media_file_tokens).await; + + match work_result { + Ok(()) => { + tx.commit().await.map_err(|err| { + warn!("Failed to commit bulk_add transaction: {:?}", err); + CommonWebError::from_error(err) + })?; + Ok(()) + } + Err(err) => { + if let Err(rollback_err) = tx.rollback().await { + error!( + "Rollback after bulk_add failure also failed: {:?} (original error: {:?})", + rollback_err, err, + ); + } + Err(err) + } + } +} + +async fn perform_add_work( + tx: &mut Transaction<'_, MySql>, + folder_token: &FolderToken, + media_file_tokens: &[MediaFileToken], +) -> Result<(), CommonWebError> { + bulk_insert_folder_media_files(BulkInsertFolderMediaFilesArgs { + folder_token, + media_file_tokens, + mysql_executor: &mut **tx, + phantom: PhantomData, + }).await.map_err(|err| { + warn!("bulk_insert_folder_media_files failed: {:?}", err); + CommonWebError::from_error(err) + })?; + + recompute_folder_last_media_files(RecomputeFolderLastMediaFilesArgs { + folder_token, + mysql_executor: &mut **tx, + phantom: PhantomData, + }).await.map_err(|err| { + warn!("recompute_folder_last_media_files failed: {:?}", err); + CommonWebError::from_error(err) + })?; + + Ok(()) +} diff --git a/crates/service/web/storyteller_web/src/http_server/endpoints/folders/media_files/bulk_remove_folder_media_files_handler.rs b/crates/service/web/storyteller_web/src/http_server/endpoints/folders/media_files/bulk_remove_folder_media_files_handler.rs index 3ce390715c..57b47c8103 100644 --- a/crates/service/web/storyteller_web/src/http_server/endpoints/folders/media_files/bulk_remove_folder_media_files_handler.rs +++ b/crates/service/web/storyteller_web/src/http_server/endpoints/folders/media_files/bulk_remove_folder_media_files_handler.rs @@ -3,7 +3,7 @@ use std::sync::Arc; use actix_web::web::{Json, Path}; use actix_web::{web, HttpRequest}; -use log::warn; +use log::{error, warn}; use artcraft_api_defs::folders::media_files::{ BulkRemoveFolderMediaFilesRequest, BulkRemoveFolderMediaFilesSuccessResponse, @@ -18,8 +18,10 @@ use mysql_queries::queries::folders::media_files::bulk_delete_folder_media_files use mysql_queries::queries::folders::media_files::recompute_folder_last_media_files::{ recompute_folder_last_media_files, RecomputeFolderLastMediaFilesArgs, }; -use sqlx::Acquire; +use sqlx::pool::PoolConnection; +use sqlx::{Acquire, MySql, Transaction}; use tokens::tokens::folders::FolderToken; +use tokens::tokens::media_files::MediaFileToken; use crate::http_server::common_responses::common_web_error::CommonWebError; use crate::http_server::web_utils::user_session::require_user_session_using_connection::require_user_session_using_connection; @@ -78,9 +80,7 @@ pub async fn bulk_remove_folder_media_files_handler( return Err(CommonWebError::NotFound); } - // Delete + recompute the denormalized `maybe_last_media_file_token_1..4` - // columns inside a single transaction so they stay in sync with - // membership. Empty input short-circuits with zero work. + // Empty input short-circuits with zero work — no transaction needed. if request.media_file_tokens.is_empty() { return Ok(Json(BulkRemoveFolderMediaFilesSuccessResponse { success: true, @@ -88,15 +88,65 @@ pub async fn bulk_remove_folder_media_files_handler( })); } + let removed_count = perform_atomic_remove( + &mut conn, + &path.folder_token, + &request.media_file_tokens, + ).await?; + + Ok(Json(BulkRemoveFolderMediaFilesSuccessResponse { + success: true, + removed_count, + })) +} + +/// Open a transaction, run the delete + recompute as one unit, and +/// commit on success. On any failure inside [`perform_remove_work`] the +/// transaction is explicitly rolled back before the original error is +/// re-raised — sqlx would roll back on drop too, but doing it explicitly +/// makes the failure path obvious and surfaces any rollback error in +/// the log. +async fn perform_atomic_remove( + conn: &mut PoolConnection, + folder_token: &FolderToken, + media_file_tokens: &[MediaFileToken], +) -> Result { let mut tx = conn.begin().await.map_err(|err| { warn!("Failed to begin transaction: {:?}", err); CommonWebError::from_error(err) })?; + let work_result = perform_remove_work(&mut tx, folder_token, media_file_tokens).await; + + match work_result { + Ok(removed_count) => { + tx.commit().await.map_err(|err| { + warn!("Failed to commit bulk_remove transaction: {:?}", err); + CommonWebError::from_error(err) + })?; + Ok(removed_count) + } + Err(err) => { + if let Err(rollback_err) = tx.rollback().await { + error!( + "Rollback after bulk_remove failure also failed: {:?} (original error: {:?})", + rollback_err, err, + ); + } + Err(err) + } + } +} + +async fn perform_remove_work( + tx: &mut Transaction<'_, MySql>, + folder_token: &FolderToken, + media_file_tokens: &[MediaFileToken], +) -> Result { let removed_count = bulk_delete_folder_media_files(BulkDeleteFolderMediaFilesArgs { - folder_token: &path.folder_token, - media_file_tokens: &request.media_file_tokens, - mysql_executor: &mut *tx, + folder_token, + media_file_tokens, + mysql_executor: &mut **tx, phantom: PhantomData, }).await.map_err(|err| { warn!("bulk_delete_folder_media_files failed: {:?}", err); @@ -104,21 +154,13 @@ pub async fn bulk_remove_folder_media_files_handler( })?; recompute_folder_last_media_files(RecomputeFolderLastMediaFilesArgs { - folder_token: &path.folder_token, - mysql_executor: &mut *tx, + folder_token, + mysql_executor: &mut **tx, phantom: PhantomData, }).await.map_err(|err| { warn!("recompute_folder_last_media_files failed: {:?}", err); CommonWebError::from_error(err) })?; - tx.commit().await.map_err(|err| { - warn!("Failed to commit bulk_remove transaction: {:?}", err); - CommonWebError::from_error(err) - })?; - - Ok(Json(BulkRemoveFolderMediaFilesSuccessResponse { - success: true, - removed_count, - })) + Ok(removed_count) }