From 1b4bdc2ddb889fc6ea2887ed72c96040b61efb01 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bal=C3=A1zs=20Sz=C3=BCcs?= <127139797+balazs-szucs@users.noreply.github.com> Date: Sat, 20 Dec 2025 22:39:09 +0100 Subject: [PATCH] fix(scans): prevent shelf associations from being cleared during metadata operations (#1947) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix: ensure book metadata is eagerly loaded for updates and rescans Signed-off-by: Balázs Szücs * Fix failing tests in LibraryRescanHelperTest and MetadataControllerTest - Update LibraryRescanHelper to handle null and deleted books gracefully during rescan. - Update LibraryRescanHelperTest to mock BookRepository correctly. - Update MetadataControllerTest to mock findAllWithMetadataByIds. --------- Signed-off-by: Balázs Szücs --- .../controller/MetadataController.java | 4 ++- .../service/library/LibraryRescanHelper.java | 13 +++++--- .../service/metadata/BookMetadataService.java | 4 ++- .../controller/MetadataControllerTest.java | 2 +- .../library/LibraryRescanHelperTest.java | 33 +++++++++---------- 5 files changed, 32 insertions(+), 24 deletions(-) diff --git a/booklore-api/src/main/java/com/adityachandel/booklore/controller/MetadataController.java b/booklore-api/src/main/java/com/adityachandel/booklore/controller/MetadataController.java index ad0c35d1..589d027f 100644 --- a/booklore-api/src/main/java/com/adityachandel/booklore/controller/MetadataController.java +++ b/booklore-api/src/main/java/com/adityachandel/booklore/controller/MetadataController.java @@ -64,7 +64,9 @@ public class MetadataController { @Parameter(description = "Metadata update wrapper") @RequestBody MetadataUpdateWrapper metadataUpdateWrapper, @Parameter(description = "ID of the book") @PathVariable long bookId, @Parameter(description = "Merge categories") @RequestParam(defaultValue = "true") boolean mergeCategories) { - BookEntity bookEntity = bookRepository.findById(bookId).orElseThrow(() -> ApiError.BOOK_NOT_FOUND.createException(bookId)); + BookEntity bookEntity = bookRepository.findAllWithMetadataByIds(java.util.Collections.singleton(bookId)).stream() + .findFirst() + .orElseThrow(() -> ApiError.BOOK_NOT_FOUND.createException(bookId)); MetadataUpdateContext context = MetadataUpdateContext.builder() .bookEntity(bookEntity) diff --git a/booklore-api/src/main/java/com/adityachandel/booklore/service/library/LibraryRescanHelper.java b/booklore-api/src/main/java/com/adityachandel/booklore/service/library/LibraryRescanHelper.java index 512d9798..742acbc5 100644 --- a/booklore-api/src/main/java/com/adityachandel/booklore/service/library/LibraryRescanHelper.java +++ b/booklore-api/src/main/java/com/adityachandel/booklore/service/library/LibraryRescanHelper.java @@ -9,6 +9,7 @@ import com.adityachandel.booklore.model.entity.LibraryEntity; import com.adityachandel.booklore.model.websocket.TaskProgressPayload; import com.adityachandel.booklore.model.websocket.Topic; import com.adityachandel.booklore.repository.LibraryRepository; +import com.adityachandel.booklore.repository.BookRepository; import com.adityachandel.booklore.service.NotificationService; import com.adityachandel.booklore.service.metadata.BookMetadataUpdater; import com.adityachandel.booklore.service.metadata.extractor.MetadataExtractorFactory; @@ -32,13 +33,15 @@ public class LibraryRescanHelper { private final BookMetadataUpdater bookMetadataUpdater; private final NotificationService notificationService; private final TaskCancellationManager cancellationManager; + private final BookRepository bookRepository; - public LibraryRescanHelper(LibraryRepository libraryRepository, MetadataExtractorFactory metadataExtractorFactory, @Lazy BookMetadataUpdater bookMetadataUpdater, NotificationService notificationService, TaskCancellationManager cancellationManager) { + public LibraryRescanHelper(LibraryRepository libraryRepository, MetadataExtractorFactory metadataExtractorFactory, @Lazy BookMetadataUpdater bookMetadataUpdater, NotificationService notificationService, TaskCancellationManager cancellationManager, BookRepository bookRepository) { this.libraryRepository = libraryRepository; this.metadataExtractorFactory = metadataExtractorFactory; this.bookMetadataUpdater = bookMetadataUpdater; this.notificationService = notificationService; this.cancellationManager = cancellationManager; + this.bookRepository = bookRepository; } @Transactional @@ -46,9 +49,7 @@ public class LibraryRescanHelper { LibraryEntity library = libraryRepository.findById(context.getLibraryId()).orElseThrow(() -> ApiError.LIBRARY_NOT_FOUND.createException(context.getLibraryId())); - List bookEntities = library.getBookEntities().stream() - .filter(b -> b != null && (b.getDeleted() == null || !b.getDeleted())) - .toList(); + List bookEntities = bookRepository.findAllWithMetadataByLibraryId(library.getId()); log.info("Found {} book(s) to process in library id={}", bookEntities.size(), library.getId()); @@ -58,6 +59,10 @@ public class LibraryRescanHelper { sendTaskProgressNotification(taskId, 0, String.format("Starting rescan for library: %s", library.getName()), TaskStatus.IN_PROGRESS); for (BookEntity bookEntity : bookEntities) { + if (bookEntity == null || (bookEntity.getDeleted() != null && bookEntity.getDeleted())) { + continue; + } + if (taskId != null && cancellationManager.isTaskCancelled(taskId)) { log.info("Library rescan for library {} was cancelled", library.getId()); sendTaskProgressNotification(taskId, (processedBooks * 100) / totalBooks, diff --git a/booklore-api/src/main/java/com/adityachandel/booklore/service/metadata/BookMetadataService.java b/booklore-api/src/main/java/com/adityachandel/booklore/service/metadata/BookMetadataService.java index 1ad87ef5..fa81873e 100644 --- a/booklore-api/src/main/java/com/adityachandel/booklore/service/metadata/BookMetadataService.java +++ b/booklore-api/src/main/java/com/adityachandel/booklore/service/metadata/BookMetadataService.java @@ -237,7 +237,9 @@ public class BookMetadataService { } private BookMetadata updateCover(Long bookId, BiConsumer writerAction) { - BookEntity bookEntity = bookRepository.findById(bookId).orElseThrow(() -> ApiError.BOOK_NOT_FOUND.createException(bookId)); + BookEntity bookEntity = bookRepository.findAllWithMetadataByIds(java.util.Collections.singleton(bookId)).stream() + .findFirst() + .orElseThrow(() -> ApiError.BOOK_NOT_FOUND.createException(bookId)); bookEntity.getMetadata().setCoverUpdatedOn(Instant.now()); MetadataPersistenceSettings settings = appSettingService.getAppSettings().getMetadataPersistenceSettings(); boolean saveToOriginalFile = settings.isSaveToOriginalFile(); diff --git a/booklore-api/src/test/java/com/adityachandel/booklore/controller/MetadataControllerTest.java b/booklore-api/src/test/java/com/adityachandel/booklore/controller/MetadataControllerTest.java index 466c3a7b..63e92477 100644 --- a/booklore-api/src/test/java/com/adityachandel/booklore/controller/MetadataControllerTest.java +++ b/booklore-api/src/test/java/com/adityachandel/booklore/controller/MetadataControllerTest.java @@ -53,7 +53,7 @@ class MetadataControllerTest { bookEntity.setId(bookId); bookEntity.setMetadata(new BookMetadataEntity()); - when(bookRepository.findById(bookId)).thenReturn(Optional.of(bookEntity)); + when(bookRepository.findAllWithMetadataByIds(java.util.Collections.singleton(bookId))).thenReturn(java.util.List.of(bookEntity)); when(bookMetadataMapper.toBookMetadata(any(), anyBoolean())).thenReturn(new BookMetadata()); metadataController.updateMetadata(wrapper, bookId, true); diff --git a/booklore-api/src/test/java/com/adityachandel/booklore/service/library/LibraryRescanHelperTest.java b/booklore-api/src/test/java/com/adityachandel/booklore/service/library/LibraryRescanHelperTest.java index 1aea2e2f..7f8aeaac 100644 --- a/booklore-api/src/test/java/com/adityachandel/booklore/service/library/LibraryRescanHelperTest.java +++ b/booklore-api/src/test/java/com/adityachandel/booklore/service/library/LibraryRescanHelperTest.java @@ -10,6 +10,7 @@ import com.adityachandel.booklore.model.enums.MetadataReplaceMode; import com.adityachandel.booklore.model.enums.TaskType; import com.adityachandel.booklore.model.websocket.TaskProgressPayload; import com.adityachandel.booklore.model.websocket.Topic; +import com.adityachandel.booklore.repository.BookRepository; import com.adityachandel.booklore.repository.LibraryRepository; import com.adityachandel.booklore.service.NotificationService; import com.adityachandel.booklore.service.metadata.BookMetadataUpdater; @@ -44,6 +45,7 @@ class LibraryRescanHelperTest { @Mock private BookMetadataUpdater bookMetadataUpdater; @Mock private NotificationService notificationService; @Mock private TaskCancellationManager cancellationManager; + @Mock private BookRepository bookRepository; @InjectMocks private LibraryRescanHelper libraryRescanHelper; @Captor private ArgumentCaptor payloadCaptor; @@ -88,8 +90,6 @@ class LibraryRescanHelperTest { void handleRescanOptions_shouldProcessAllBooks_whenLibraryHasBooks() { BookEntity book1 = createBookEntity(1L, "book1.epub", BookFileType.EPUB); BookEntity book2 = createBookEntity(2L, "book2.pdf", BookFileType.PDF); - library.getBookEntities().add(book1); - library.getBookEntities().add(book2); BookMetadata metadata1 = new BookMetadata(); metadata1.setTitle("Book 1"); @@ -97,6 +97,7 @@ class LibraryRescanHelperTest { metadata2.setTitle("Book 2"); when(libraryRepository.findById(1L)).thenReturn(Optional.of(library)); + when(bookRepository.findAllWithMetadataByLibraryId(1L)).thenReturn(List.of(book1, book2)); when(metadataExtractorFactory.extractMetadata(eq(BookFileType.EPUB), any(File.class))).thenReturn(metadata1); when(metadataExtractorFactory.extractMetadata(eq(BookFileType.PDF), any(File.class))).thenReturn(metadata2); when(cancellationManager.isTaskCancelled(taskId)).thenReturn(false); @@ -115,13 +116,11 @@ class LibraryRescanHelperTest { BookEntity book2 = createBookEntity(2L, "book2.pdf", BookFileType.PDF); book2.setDeleted(true); - library.getBookEntities().add(book1); - library.getBookEntities().add(book2); - BookMetadata metadata = new BookMetadata(); metadata.setTitle("Book 1"); when(libraryRepository.findById(1L)).thenReturn(Optional.of(library)); + when(bookRepository.findAllWithMetadataByLibraryId(1L)).thenReturn(List.of(book1, book2)); when(metadataExtractorFactory.extractMetadata(any(BookFileType.class), any(File.class))).thenReturn(metadata); when(cancellationManager.isTaskCancelled(taskId)).thenReturn(false); @@ -134,11 +133,13 @@ class LibraryRescanHelperTest { @Test void handleRescanOptions_shouldSkipNullBooks() { BookEntity book1 = createBookEntity(1L, "book1.epub", BookFileType.EPUB); - library.getBookEntities().add(book1); - library.getBookEntities().add(null); + java.util.List books = new ArrayList<>(); + books.add(book1); + books.add(null); BookMetadata metadata = new BookMetadata(); when(libraryRepository.findById(1L)).thenReturn(Optional.of(library)); + when(bookRepository.findAllWithMetadataByLibraryId(1L)).thenReturn(books); when(metadataExtractorFactory.extractMetadata(any(BookFileType.class), any(File.class))).thenReturn(metadata); when(cancellationManager.isTaskCancelled(taskId)).thenReturn(false); @@ -152,12 +153,11 @@ class LibraryRescanHelperTest { void handleRescanOptions_shouldContinue_whenMetadataExtractionReturnsNull() { BookEntity book1 = createBookEntity(1L, "book1.epub", BookFileType.EPUB); BookEntity book2 = createBookEntity(2L, "book2.pdf", BookFileType.PDF); - library.getBookEntities().add(book1); - library.getBookEntities().add(book2); BookMetadata metadata2 = new BookMetadata(); when(libraryRepository.findById(1L)).thenReturn(Optional.of(library)); + when(bookRepository.findAllWithMetadataByLibraryId(1L)).thenReturn(List.of(book1, book2)); when(metadataExtractorFactory.extractMetadata(eq(BookFileType.EPUB), any(File.class))).thenReturn(null); when(metadataExtractorFactory.extractMetadata(eq(BookFileType.PDF), any(File.class))).thenReturn(metadata2); when(cancellationManager.isTaskCancelled(taskId)).thenReturn(false); @@ -172,13 +172,12 @@ class LibraryRescanHelperTest { void handleRescanOptions_shouldContinue_whenMetadataUpdateThrowsException() { BookEntity book1 = createBookEntity(1L, "book1.epub", BookFileType.EPUB); BookEntity book2 = createBookEntity(2L, "book2.pdf", BookFileType.PDF); - library.getBookEntities().add(book1); - library.getBookEntities().add(book2); BookMetadata metadata1 = new BookMetadata(); BookMetadata metadata2 = new BookMetadata(); when(libraryRepository.findById(1L)).thenReturn(Optional.of(library)); + when(bookRepository.findAllWithMetadataByLibraryId(1L)).thenReturn(List.of(book1, book2)); when(metadataExtractorFactory.extractMetadata(any(BookFileType.class), any(File.class))) .thenReturn(metadata1, metadata2); when(cancellationManager.isTaskCancelled(taskId)).thenReturn(false); @@ -195,10 +194,9 @@ class LibraryRescanHelperTest { void handleRescanOptions_shouldCancel_whenTaskCancellationRequested() { BookEntity book1 = createBookEntity(1L, "book1.epub", BookFileType.EPUB); BookEntity book2 = createBookEntity(2L, "book2.pdf", BookFileType.PDF); - library.getBookEntities().add(book1); - library.getBookEntities().add(book2); when(libraryRepository.findById(1L)).thenReturn(Optional.of(library)); + when(bookRepository.findAllWithMetadataByLibraryId(1L)).thenReturn(List.of(book1, book2)); when(cancellationManager.isTaskCancelled(taskId)).thenReturn(false, true); libraryRescanHelper.handleRescanOptions(rescanContext, taskId); @@ -213,10 +211,10 @@ class LibraryRescanHelperTest { @Test void handleRescanOptions_shouldSendProgressNotifications() { BookEntity book1 = createBookEntity(1L, "book1.epub", BookFileType.EPUB); - library.getBookEntities().add(book1); BookMetadata metadata = new BookMetadata(); when(libraryRepository.findById(1L)).thenReturn(Optional.of(library)); + when(bookRepository.findAllWithMetadataByLibraryId(1L)).thenReturn(List.of(book1)); when(metadataExtractorFactory.extractMetadata(any(BookFileType.class), any(File.class))).thenReturn(metadata); when(cancellationManager.isTaskCancelled(taskId)).thenReturn(false); @@ -237,6 +235,7 @@ class LibraryRescanHelperTest { @Test void handleRescanOptions_shouldHandleEmptyLibrary() { when(libraryRepository.findById(1L)).thenReturn(Optional.of(library)); + when(bookRepository.findAllWithMetadataByLibraryId(1L)).thenReturn(new ArrayList<>()); when(cancellationManager.isTaskCancelled(taskId)).thenReturn(false); libraryRescanHelper.handleRescanOptions(rescanContext, taskId); @@ -252,12 +251,12 @@ class LibraryRescanHelperTest { @Test void handleRescanOptions_shouldSetCorrectMetadataUpdateContext() { BookEntity book = createBookEntity(1L, "book1.epub", BookFileType.EPUB); - library.getBookEntities().add(book); BookMetadata metadata = new BookMetadata(); metadata.setTitle("Test Book"); when(libraryRepository.findById(1L)).thenReturn(Optional.of(library)); + when(bookRepository.findAllWithMetadataByLibraryId(1L)).thenReturn(List.of(book)); when(metadataExtractorFactory.extractMetadata(any(BookFileType.class), any(File.class))).thenReturn(metadata); when(cancellationManager.isTaskCancelled(taskId)).thenReturn(false); @@ -276,10 +275,10 @@ class LibraryRescanHelperTest { @Test void handleRescanOptions_shouldHandleNullTaskId() { BookEntity book = createBookEntity(1L, "book1.epub", BookFileType.EPUB); - library.getBookEntities().add(book); BookMetadata metadata = new BookMetadata(); when(libraryRepository.findById(1L)).thenReturn(Optional.of(library)); + when(bookRepository.findAllWithMetadataByLibraryId(1L)).thenReturn(List.of(book)); when(metadataExtractorFactory.extractMetadata(any(BookFileType.class), any(File.class))).thenReturn(metadata); libraryRescanHelper.handleRescanOptions(rescanContext, null); @@ -294,10 +293,10 @@ class LibraryRescanHelperTest { @Test void handleRescanOptions_shouldContinue_whenNotificationFails() { BookEntity book = createBookEntity(1L, "book1.epub", BookFileType.EPUB); - library.getBookEntities().add(book); BookMetadata metadata = new BookMetadata(); when(libraryRepository.findById(1L)).thenReturn(Optional.of(library)); + when(bookRepository.findAllWithMetadataByLibraryId(1L)).thenReturn(List.of(book)); when(metadataExtractorFactory.extractMetadata(any(BookFileType.class), any(File.class))).thenReturn(metadata); when(cancellationManager.isTaskCancelled(taskId)).thenReturn(false); doThrow(new RuntimeException("Notification failed"))