fix(file-move): implement transaction management for file moves and rollback on failure (#2588)

Signed-off-by: Balázs Szücs <bszucs1209@gmail.com>

booklore-api/src/main/java/org/booklore/service/file/FileMoveService.java

@@ -1,5 +1,8 @@
 package org.booklore.service.file;
 
+import jakarta.persistence.EntityManager;
+import lombok.AllArgsConstructor;
+import lombok.extern.slf4j.Slf4j;
 import org.booklore.config.AppProperties;
 import org.booklore.mapper.BookMapper;
 import org.booklore.mapper.LibraryMapper;
@@ -15,11 +18,8 @@ import org.booklore.repository.BookRepository;
 import org.booklore.repository.LibraryRepository;
 import org.booklore.service.NotificationService;
 import org.booklore.service.monitoring.MonitoringRegistrationService;
-import jakarta.persistence.EntityManager;
-import lombok.AllArgsConstructor;
-import lombok.extern.slf4j.Slf4j;
 import org.springframework.stereotype.Service;
-import org.springframework.transaction.annotation.Transactional;
+import org.springframework.transaction.support.TransactionTemplate;
 
 import java.nio.file.Path;
 import java.nio.file.Paths;
@@ -42,9 +42,9 @@ public class FileMoveService {
     private final BookMapper bookMapper;
     private final NotificationService notificationService;
     private final EntityManager entityManager;
+    private final TransactionTemplate transactionTemplate;
 
 
-    @Transactional
     public void bulkMoveFiles(FileMoveRequest request) {
         List<FileMoveRequest.Move> moves = request.getMoves();
 
@@ -171,25 +171,42 @@ public class FileMoveService {
                 return;
             }
 
+            List<PlannedMove> committedMoves = new ArrayList<>();
+
             // Commit file moves FIRST before updating database
             for (PlannedMove planned : plannedMovesByBookFileId.values()) {
                 fileMoveHelper.commitMove(planned.temp(), planned.target());
+                committedMoves.add(planned);
             }
             plannedMovesByBookFileId.clear();
 
             // Only update database after all file commits succeed
-            for (var bookFile : bookEntity.getBookFiles()) {
-                String newFileName;
-                if (bookFile.isBook()) {
-                    Path targetPath = fileMoveHelper.generateNewFilePath(bookEntity, bookFile, libraryPathEntity, pattern);
-                    newFileName = targetPath.getFileName().toString();
-                } else {
-                    newFileName = bookFile.getFileName();
-                }
-                bookFileRepository.updateFileNameAndSubPath(bookFile.getId(), newFileName, newFileSubPath);
-            }
+            try {
+                transactionTemplate.executeWithoutResult(status -> {
+                    for (var bookFile : bookEntity.getBookFiles()) {
+                        String newFileName;
+                        if (bookFile.isBook()) {
+                            Path targetPath = fileMoveHelper.generateNewFilePath(bookEntity, bookFile, libraryPathEntity, pattern);
+                            newFileName = targetPath.getFileName().toString();
+                        } else {
+                            newFileName = bookFile.getFileName();
+                        }
+                        bookFileRepository.updateFileNameAndSubPath(bookFile.getId(), newFileName, newFileSubPath);
+                    }
 
-            bookRepository.updateLibrary(bookEntity.getId(), targetLibrary.getId(), libraryPathEntity);
+                    bookRepository.updateLibrary(bookEntity.getId(), targetLibrary.getId(), libraryPathEntity);
+                });
+            } catch (Exception e) {
+                log.error("Database update failed after files were moved. Attempting to rollback file moves for book ID {}", bookId, e);
+                for (PlannedMove committed : committedMoves) {
+                    try {
+                        fileMoveHelper.moveFile(committed.target(), committed.source());
+                    } catch (Exception rollbackEx) {
+                        log.error("Failed to rollback file move (Target -> Source) for book ID {}: {} -> {}", bookId, committed.target(), committed.source(), rollbackEx);
+                    }
+                }
+                throw e;
+            }
 
             Path libraryRoot = Paths.get(libraryPathEntity.getPath()).toAbsolutePath().normalize();
             for (Path sourceParent : sourceParentsToCleanup) {
@@ -211,7 +228,6 @@ public class FileMoveService {
         }
     }
 
-    @Transactional
     public FileMoveResult moveSingleFile(BookEntity bookEntity) {
         record PlannedMove(Path source, Path temp, Path target) {}
 
@@ -302,22 +318,40 @@ public class FileMoveService {
                 return FileMoveResult.builder().moved(false).build();
             }
 
+            List<PlannedMove> committedMoves = new ArrayList<>();
+
             // Commit all file moves FIRST before updating database
             for (PlannedMove planned : plannedMovesByBookFileId.values()) {
                 fileMoveHelper.commitMove(planned.temp(), planned.target());
+                committedMoves.add(planned);
             }
             plannedMovesByBookFileId.clear();
 
             // Update database for ALL BookFileEntity records (only after all commits succeed)
-            for (var bookFile : bookWithFiles.getBookFiles()) {
-                String newFileName;
-                if (bookFile.isBook()) {
-                    Path targetPath = fileMoveHelper.generateNewFilePath(bookWithFiles, bookFile, bookWithFiles.getLibraryPath(), pattern);
-                    newFileName = targetPath.getFileName().toString();
-                } else {
-                    newFileName = bookFile.getFileName();
+            BookEntity finalBookWithFiles = bookWithFiles;
+            try {
+                transactionTemplate.executeWithoutResult(status -> {
+                    for (var bookFile : finalBookWithFiles.getBookFiles()) {
+                        String newFileName;
+                        if (bookFile.isBook()) {
+                            Path targetPath = fileMoveHelper.generateNewFilePath(finalBookWithFiles, bookFile, finalBookWithFiles.getLibraryPath(), pattern);
+                            newFileName = targetPath.getFileName().toString();
+                        } else {
+                            newFileName = bookFile.getFileName();
+                        }
+                        bookFileRepository.updateFileNameAndSubPath(bookFile.getId(), newFileName, newFileSubPath);
+                    }
+                });
+            } catch (Exception e) {
+                log.error("Database update failed after files were moved. Attempting to rollback file moves for book ID {}", bookEntity.getId(), e);
+                for (PlannedMove committed : committedMoves) {
+                    try {
+                        fileMoveHelper.moveFile(committed.target(), committed.source());
+                    } catch (Exception rollbackEx) {
+                        log.error("Failed to rollback file move (Target -> Source) for book ID {}: {} -> {}", bookEntity.getId(), committed.target(), committed.source(), rollbackEx);
+                    }
                 }
-                bookFileRepository.updateFileNameAndSubPath(bookFile.getId(), newFileName, newFileSubPath);
+                throw e;
             }
 
             // Clean up empty parent directories

booklore-api/src/test/java/org/booklore/service/file/FileMoveServiceOrderingTest.java

@@ -1,5 +1,6 @@
 package org.booklore.service.file;
 
+import jakarta.persistence.EntityManager;
 import org.booklore.config.AppProperties;
 import org.booklore.mapper.BookMapper;
 import org.booklore.mapper.LibraryMapper;
@@ -15,7 +16,6 @@ import org.booklore.repository.BookRepository;
 import org.booklore.repository.LibraryRepository;
 import org.booklore.service.NotificationService;
 import org.booklore.service.monitoring.MonitoringRegistrationService;
-import jakarta.persistence.EntityManager;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.DisplayName;
 import org.junit.jupiter.api.Test;
@@ -25,6 +25,8 @@ import org.mockito.Mock;
 import org.mockito.junit.jupiter.MockitoExtension;
 import org.mockito.junit.jupiter.MockitoSettings;
 import org.mockito.quality.Strictness;
+import org.springframework.transaction.TransactionStatus;
+import org.springframework.transaction.support.TransactionTemplate;
 
 import java.io.IOException;
 import java.nio.file.Path;
@@ -33,6 +35,7 @@ import java.util.ArrayList;
 import java.util.Collections;
 import java.util.List;
 import java.util.Set;
+import java.util.function.Consumer;
 
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.mockito.ArgumentMatchers.*;
@@ -51,7 +54,9 @@ class FileMoveServiceOrderingTest {
     @Mock private LibraryMapper libraryMapper;
     @Mock private BookMapper bookMapper;
     @Mock private NotificationService notificationService;
+
     @Mock private EntityManager entityManager;
+    @Mock private TransactionTemplate transactionTemplate;
 
     private FileMoveService service;
     private LibraryEntity library;
@@ -61,9 +66,10 @@ class FileMoveServiceOrderingTest {
         TestableFileMoveService(AppProperties appProperties, BookRepository bookRepository, BookAdditionalFileRepository bookFileRepository,
                                 LibraryRepository libraryRepository, FileMoveHelper fileMoveHelper,
                                 MonitoringRegistrationService monitoringRegistrationService, LibraryMapper libraryMapper,
-                                BookMapper bookMapper, NotificationService notificationService, EntityManager entityManager) {
+                                BookMapper bookMapper, NotificationService notificationService, EntityManager entityManager,
+                                TransactionTemplate transactionTemplate) {
             super(appProperties, bookRepository, bookFileRepository, libraryRepository, fileMoveHelper,
-                    monitoringRegistrationService, libraryMapper, bookMapper, notificationService, entityManager);
+                    monitoringRegistrationService, libraryMapper, bookMapper, notificationService, entityManager, transactionTemplate);
         }
 
         @Override
@@ -75,8 +81,15 @@ class FileMoveServiceOrderingTest {
     @BeforeEach
     void setUp() {
         when(appProperties.getDiskType()).thenReturn("LOCAL");
+        
+        doAnswer(invocation -> {
+            Consumer<TransactionStatus> action = invocation.getArgument(0);
+            action.accept(mock(TransactionStatus.class));
+            return null;
+        }).when(transactionTemplate).executeWithoutResult(any());
+
         service = spy(new TestableFileMoveService(appProperties, bookRepository, bookFileRepository, libraryRepository,
-                fileMoveHelper, monitoringRegistrationService, libraryMapper, bookMapper, notificationService, entityManager));
+                fileMoveHelper, monitoringRegistrationService, libraryMapper, bookMapper, notificationService, entityManager, transactionTemplate));
 
         library = new LibraryEntity();
         library.setId(1L);
@@ -308,4 +321,31 @@ class FileMoveServiceOrderingTest {
         verify(fileMoveHelper).unregisterLibrary(1L);
         verify(monitoringRegistrationService).registerLibrary(dto);
     }
+    @Test
+    @DisplayName("moveSingleFile: rolls back committed files on database failure")
+    void filesRolledBackOnDbFailure() throws IOException {
+        BookEntity book = createBook();
+        Path target = Paths.get("/library/new/path/NewName.epub");
+
+        mockStandardBehavior(book, target);
+        
+        // Mock DB failure
+        doThrow(new RuntimeException("DB Error")).when(transactionTemplate).executeWithoutResult(any());
+        
+        when(monitoringRegistrationService.isLibraryMonitored(anyLong())).thenReturn(false);
+        when(monitoringRegistrationService.getPathsForLibraries(anySet())).thenReturn(Collections.emptySet());
+
+        try {
+            service.moveSingleFile(book);
+        } catch (RuntimeException e) {
+            // Expected
+        }
+
+        InOrder inOrder = inOrder(fileMoveHelper);
+        inOrder.verify(fileMoveHelper).moveFileWithBackup(any());
+        inOrder.verify(fileMoveHelper).commitMove(any(), any()); // Committed
+        
+        // Verify manual rollback (Target -> Source)
+        inOrder.verify(fileMoveHelper).moveFile(any(), any());
+    }
 }

booklore-api/src/test/java/org/booklore/service/file/FileMoveServiceTest.java

@@ -1,5 +1,6 @@
 package org.booklore.service.file;
 
+import jakarta.persistence.EntityManager;
 import org.booklore.config.AppProperties;
 import org.booklore.mapper.BookMapper;
 import org.booklore.mapper.LibraryMapper;
@@ -17,7 +18,6 @@ import org.booklore.repository.BookRepository;
 import org.booklore.repository.LibraryRepository;
 import org.booklore.service.NotificationService;
 import org.booklore.service.monitoring.MonitoringRegistrationService;
-import jakarta.persistence.EntityManager;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.DisplayName;
 import org.junit.jupiter.api.Nested;
@@ -31,11 +31,7 @@ import org.mockito.quality.Strictness;
 import java.io.IOException;
 import java.nio.file.Path;
 import java.nio.file.Paths;
-import java.util.ArrayList;
-import java.util.Collections;
-import java.util.List;
-import java.util.Optional;
-import java.util.Set;
+import java.util.*;
 
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.mockito.ArgumentMatchers.*;
@@ -55,6 +51,7 @@ class FileMoveServiceTest {
     @Mock private BookMapper bookMapper;
     @Mock private NotificationService notificationService;
     @Mock private EntityManager entityManager;
+    @Mock private org.springframework.transaction.support.TransactionTemplate transactionTemplate;
 
     private FileMoveService service;
     private LibraryEntity library;
@@ -64,9 +61,10 @@ class FileMoveServiceTest {
         TestableFileMoveService(AppProperties appProperties, BookRepository bookRepository, BookAdditionalFileRepository bookFileRepository,
                                 LibraryRepository libraryRepository, FileMoveHelper fileMoveHelper,
                                 MonitoringRegistrationService monitoringRegistrationService, LibraryMapper libraryMapper,
-                                BookMapper bookMapper, NotificationService notificationService, EntityManager entityManager) {
+                                BookMapper bookMapper, NotificationService notificationService, EntityManager entityManager,
+                                org.springframework.transaction.support.TransactionTemplate transactionTemplate) {
             super(appProperties, bookRepository, bookFileRepository, libraryRepository, fileMoveHelper,
-                    monitoringRegistrationService, libraryMapper, bookMapper, notificationService, entityManager);
+                    monitoringRegistrationService, libraryMapper, bookMapper, notificationService, entityManager, transactionTemplate);
         }
 
         @Override
@@ -78,8 +76,16 @@ class FileMoveServiceTest {
     @BeforeEach
     void setUp() {
         when(appProperties.getDiskType()).thenReturn("LOCAL");
+        
+        // Mock simple execution for transaction template
+        doAnswer(invocation -> {
+            java.util.function.Consumer<org.springframework.transaction.TransactionStatus> action = invocation.getArgument(0);
+            action.accept(null);
+            return null;
+        }).when(transactionTemplate).executeWithoutResult(any());
+
         service = spy(new TestableFileMoveService(appProperties, bookRepository, bookFileRepository, libraryRepository,
-                fileMoveHelper, monitoringRegistrationService, libraryMapper, bookMapper, notificationService, entityManager));
+                fileMoveHelper, monitoringRegistrationService, libraryMapper, bookMapper, notificationService, entityManager, transactionTemplate));
 
         library = new LibraryEntity();
         library.setId(1L);