fix(hibernate): improve transaction management and optimize entity fetching#176
fix(hibernate): improve transaction management and optimize entity fetching#176balazs-szucs wants to merge 17 commits intogrimmory-tools:developfrom
Conversation
📝 WalkthroughWalkthroughAdded class-level read-only transactions to many services, introduced paginated/batched repository APIs and projection for embeddings, replaced SUBSELECT fetch with batch loading and deferred LOBs, added embeddings-based recommendations and batched persistence, moved some post-import side effects into a REQUIRES_NEW transaction, and updated tests accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant Rec as BookRecommendationService
participant Repo as BookRepository
participant Vec as BookVectorService
participant Qry as BookQueryService
Note over Rec,Repo: Embeddings-based recommendation flow
Rec->>Repo: findAllEmbeddingsForRecommendation(bookId)
Repo-->>Rec: List<BookEmbeddingProjection>
alt target has embedding
Rec->>Vec: deserialize target embedding
Vec-->>Rec: target vector
Rec->>Vec: compute cosine similarities for candidates
Vec-->>Rec: similarity scores
Rec->>Repo: findAllById(selectedIds)
Repo-->>Rec: List<BookEntity>
Rec->>Qry: enforce author caps & map to BookRecommendation
Qry-->>Rec: List<BookRecommendation>
else fallback
Rec->>Repo: find candidate entities (entity-based)
Repo-->>Rec: candidate entities
Rec->>Qry: load full entities & build recommendations
Qry-->>Rec: List<BookRecommendation>
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
booklore-api/src/main/java/org/booklore/service/upload/FileUploadService.java (1)
63-95:⚠️ Potential issue | 🟠 MajorRemove
@TransactionalfromuploadFileto avoid wrapping filesystem mutations in a DB transaction.Line 63 wraps
transferTo(...), file move, and metadata extraction in a Spring transaction boundary. If a runtime exception occurs aftermoveFileToFinalLocation(...)completes or during transaction commit, the database transaction rolls back while the file remains in its final location, causing DB/filesystem divergence.The audit log does not require the parent transaction—
AuditService.log()usesauditLogRepository.save(), which creates its own implicit transaction and silently handles failures via try-catch.Recommended approach
- `@Transactional` public void uploadFile(MultipartFile file, long libraryId, long pathId) { // ... validation, file transfer, move ... moveFileToFinalLocation(tempPath, finalPath); log.info("File uploaded to final location: {}", finalPath); - auditService.log(AuditAction.BOOK_UPLOADED, "Library", libraryId, "Uploaded file: " + originalFileName); + recordUploadAudit(libraryId, originalFileName); } + + `@Transactional` + protected void recordUploadAudit(long libraryId, String originalFileName) { + auditService.log(AuditAction.BOOK_UPLOADED, "Library", libraryId, "Uploaded file: " + originalFileName); + }This moves the audit log into its own transaction, keeping filesystem mutations outside transactional boundaries.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@booklore-api/src/main/java/org/booklore/service/upload/FileUploadService.java` around lines 63 - 95, Remove the method-level `@Transactional` on uploadFile so filesystem operations (file.transferTo, moveFileToFinalLocation, metadata extraction) are not executed inside a DB transaction; keep calling auditService.log as-is (it already handles its own persistence/transaction), and ensure uploadFile only handles filesystem work and throws/errors as before without a surrounding transactional boundary.
♻️ Duplicate comments (3)
booklore-api/src/main/java/org/booklore/service/book/BookQueryService.java (1)
47-54:⚠️ Potential issue | 🟠 MajorPagination totals may be inaccurate after content filtering.
contentRestrictionService.applyRestrictions()can remove books from the page content, but the returnedPageImplstill usespage.getTotalElements()from the original unfiltered query. This causes:
- Total count may include restricted books the user can't see
- Page may contain fewer items than
pageable.getPageSize()- Client may request pages beyond actual filtered content
If content restrictions are expected to be rare or minimal, this might be acceptable. Otherwise, consider applying restrictions at the query level or adjusting the total count.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@booklore-api/src/main/java/org/booklore/service/book/BookQueryService.java` around lines 47 - 54, getAllBooksByLibraryIdsPaged currently pages via bookRepository.findAllWithMetadataByLibraryIdsPage then filters the page content with contentRestrictionService.applyRestrictions, but still returns a PageImpl with the original page.getTotalElements(), producing incorrect totals; fix by ensuring the returned PageImpl uses a total that reflects the filtered results (either apply restrictions at query time in bookRepository.findAllWithMetadataByLibraryIdsPage or compute the filtered total before returning: e.g., run the same restriction logic against the full matching set or query for a filtered count, then build PageImpl with the filtered list and that corrected total), and keep mapping to DTOs via mapBookToDto as before so the content matches the adjusted total.booklore-api/src/test/java/org/booklore/service/HibernateRegressionTest.java (1)
912-923:⚠️ Potential issue | 🟠 MajorThis regression check still runs with an active session and can miss lazy-init failures.
findAllFullBooksBatch_initializesLazyCollectionsasserts immediately after repository fetch, so lazy relations may still resolve through the open persistence context. Detach/clear before assertions (or exercise the service boundary) to validate detached access.Suggested fix
List<BookEntity> batch = bookRepository.findAllFullBooksBatch( PageRequest.of(0, 100)); + entityManager.clear(); // force detached-state assertions assertThat(batch).isNotEmpty(); BookEntity loaded = batch.stream() .filter(b -> b.getId().equals(book.getId())) .findFirst() .orElseThrow(); assertThat(loaded.getMetadata().getCategories()).hasSize(1); assertThat(loaded.getMetadata().getTitle()).isEqualTo("Batch Test Book");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@booklore-api/src/test/java/org/booklore/service/HibernateRegressionTest.java` around lines 912 - 923, The test currently asserts on entities while the persistence context is still open, hiding lazy-init issues; after calling bookRepository.findAllFullBooksBatch(...) in HibernateRegressionTest (the findAllFullBooksBatch test), clear/detach the persistence context before asserting (e.g., call EntityManager.clear() or detach the loaded BookEntity) so that accessing lazy collections happens on a detached entity and will surface lazy-init failures; reference the findAllFullBooksBatch call and the BookEntity loaded variable when applying the detach/clear.booklore-api/src/main/java/org/booklore/service/bookdrop/BookDropService.java (1)
476-499:⚠️ Potential issue | 🟠 MajorStill missing a rollback/compensation path after the import finishes.
Line 476 starts a separate post-import phase after the initial import has already completed. If anything in this block, or the commit immediately after it, fails,
performFileMove()returns failure and deletes the target file, but the imported book and file records remain committed. Line 498 also publishesfileProcessResult.getBook(), so the event payload is still the pre-enrichment snapshot. Please either make import + enrichment atomic, or add compensating DB cleanup before returning failure and emitBOOK_ADDonly after the post-processing transaction commits from the finalized entity/DTO.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@booklore-api/src/main/java/org/booklore/service/bookdrop/BookDropService.java` around lines 476 - 499, The post-import block using TransactionTemplate (newTx) that calls metadataRefreshService.updateBookMetadata, koboAutoShelfService.autoAddBookToKoboShelves and notificationService.sendMessage(Topic.BOOK_ADD, fileProcessResult.getBook()) can leave committed book/file records if it fails; either make the import + enrichment atomic or add compensating cleanup and emit the event only after enrichment commit. Fix by one of two approaches: (A) move the initial import and this enrichment into a single transactional flow so the repository save and the newTx work happen in one commit, or (B) on failure inside newTx catch/finally perform compensating DB cleanup that deletes the created BookEntity and associated BookFile records (using bookRepository and any file storage cleanup used by performFileMove()), and only call notificationService.sendMessage after successfully committing the newTx using the freshly loaded BookEntity/DTO (not fileProcessResult.getBook()). Ensure the code references the TransactionTemplate/newTx block, MetadataUpdateContext, metadataRefreshService.updateBookMetadata, koboAutoShelfService.autoAddBookToKoboShelves, and notificationService.sendMessage so reviewers can locate and apply the change.
🧹 Nitpick comments (2)
booklore-api/src/test/java/org/booklore/service/komga/KomgaServiceTest.java (1)
210-225: Mock setup ordering may cause flaky behavior.The
komgaMapper.getUnknownSeriesName()stub (line 225) is configured after the repository stubs that depend on its return value (lines 220-223). While Mockito typically handles this, it's clearer to set up the mapper stub first for readability and to match the execution order.🔧 Suggested reordering
`@Test` void shouldGetAllSeriesOptimized() { // Given: Mock the optimized repository method List<String> seriesNames = List.of("Series A", "Series B", "Series C"); + when(komgaMapper.getUnknownSeriesName()).thenReturn("Unknown Series"); when(bookRepository.findDistinctSeriesNamesGroupedByLibraryId(anyLong(), anyString())) .thenReturn(seriesNames); // Mock books for the first page (Series A and Series B only) List<BookEntity> seriesABooks = List.of(seriesBooks.get(0), seriesBooks.get(1)); List<BookEntity> seriesBBooks = List.of(seriesBooks.get(2), seriesBooks.get(3)); when(bookRepository.findBooksBySeriesNameGroupedByLibraryId("Series A", 1L, "Unknown Series")) .thenReturn(seriesABooks); when(bookRepository.findBooksBySeriesNameGroupedByLibraryId("Series B", 1L, "Unknown Series")) .thenReturn(seriesBBooks); - when(komgaMapper.getUnknownSeriesName()).thenReturn("Unknown Series"); when(komgaMapper.toKomgaSeriesDto(eq("Series A"), anyLong(), any()))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@booklore-api/src/test/java/org/booklore/service/komga/KomgaServiceTest.java` around lines 210 - 225, In the shouldGetAllSeriesOptimized() test, move the komgaMapper.getUnknownSeriesName() stub before any repository stubs that reference its return value (e.g., bookRepository.findBooksBySeriesNameGroupedByLibraryId(...) and bookRepository.findDistinctSeriesNamesGroupedByLibraryId(...)) so the mapper value is defined first; update the setup order so komgaMapper.getUnknownSeriesName() is stubbed before the two findBooksBySeriesNameGroupedByLibraryId(...) when(...) calls to avoid potential ordering confusion and make the test clearer.booklore-api/src/test/java/org/booklore/service/HibernateRegressionTest.java (1)
40-41: Use@DataJpaTestinstead of@SpringBootTestto focus this test on the data layer.This test validates repositories and entity mappings without depending on app-layer beans. The mocked
TaskCronServiceandFlywayare never invoked—they exist only to silence missing dependency warnings. Replace@SpringBootTest(classes = BookloreApplication.class)with@DataJpaTestto load only JPA-related components, improving test speed and clarity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@booklore-api/src/test/java/org/booklore/service/HibernateRegressionTest.java` around lines 40 - 41, Replace the class-level `@SpringBootTest`(classes = BookloreApplication.class) on HibernateRegressionTest with `@DataJpaTest` so the test loads only JPA components; remove or stop configuring mocks for app-layer beans like TaskCronService and Flyway (they won’t be needed under `@DataJpaTest`), ensure you import org.springframework.boot.test.autoconfigure.orm.jpa.DataJpaTest, and keep or remove `@Transactional` as desired since `@DataJpaTest` is transactional by default.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@booklore-api/src/main/java/org/booklore/model/entity/BookLoreUserEntity.java`:
- Around line 99-107: Equals/hashCode must use Hibernate.getClass to avoid
proxy/class mismatches and compute hashCode from the identifier rather than
getClass(); in BookLoreUserEntity replace the instanceof check with a class
equality using Hibernate.getClass(this) vs Hibernate.getClass(o) (e.g., if (o ==
null || Hibernate.getClass(this) != Hibernate.getClass(o)) return false), cast
to BookLoreUserEntity and compare id as before, and change hashCode() to return
id == null ? 0 : id.hashCode(); apply same pattern to the other entities
(TagEntity, MoodEntity, CategoryEntity, LibraryEntity, AuthorEntity) to keep
behavior consistent.
In
`@booklore-api/src/test/java/org/booklore/service/HibernateRegressionTest.java`:
- Around line 736-739: The test in HibernateRegressionTest currently asserts
that loaded.getMetadata().getSearchText() is not null even though searchText
isn't initialized in the fixture; update the test to avoid brittle default-value
assumptions by either initializing searchText on the test fixture (where
BookEntity / Metadata are created) or replacing the null-check assertion with a
non-throwing access assertion (e.g., assert that invoking
loaded.getMetadata().getSearchText() does not throw) so the test verifies
lazy-field accessibility rather than a specific default value.
- Around line 63-68: The test class HibernateRegressionTest uses field injection
for LibraryRepository and BookRepository; replace both `@Autowired` fields with
private final fields (libraryRepository, bookRepository), annotate the class
with Lombok's `@RequiredArgsConstructor`, remove the `@Autowired` annotations, and
update imports so Lombok's annotation is present; ensure constructor injection
semantics are preserved by keeping the field names and types unchanged so Spring
can autowire via the generated constructor.
---
Outside diff comments:
In
`@booklore-api/src/main/java/org/booklore/service/upload/FileUploadService.java`:
- Around line 63-95: Remove the method-level `@Transactional` on uploadFile so
filesystem operations (file.transferTo, moveFileToFinalLocation, metadata
extraction) are not executed inside a DB transaction; keep calling
auditService.log as-is (it already handles its own persistence/transaction), and
ensure uploadFile only handles filesystem work and throws/errors as before
without a surrounding transactional boundary.
---
Duplicate comments:
In `@booklore-api/src/main/java/org/booklore/service/book/BookQueryService.java`:
- Around line 47-54: getAllBooksByLibraryIdsPaged currently pages via
bookRepository.findAllWithMetadataByLibraryIdsPage then filters the page content
with contentRestrictionService.applyRestrictions, but still returns a PageImpl
with the original page.getTotalElements(), producing incorrect totals; fix by
ensuring the returned PageImpl uses a total that reflects the filtered results
(either apply restrictions at query time in
bookRepository.findAllWithMetadataByLibraryIdsPage or compute the filtered total
before returning: e.g., run the same restriction logic against the full matching
set or query for a filtered count, then build PageImpl with the filtered list
and that corrected total), and keep mapping to DTOs via mapBookToDto as before
so the content matches the adjusted total.
In
`@booklore-api/src/main/java/org/booklore/service/bookdrop/BookDropService.java`:
- Around line 476-499: The post-import block using TransactionTemplate (newTx)
that calls metadataRefreshService.updateBookMetadata,
koboAutoShelfService.autoAddBookToKoboShelves and
notificationService.sendMessage(Topic.BOOK_ADD, fileProcessResult.getBook()) can
leave committed book/file records if it fails; either make the import +
enrichment atomic or add compensating cleanup and emit the event only after
enrichment commit. Fix by one of two approaches: (A) move the initial import and
this enrichment into a single transactional flow so the repository save and the
newTx work happen in one commit, or (B) on failure inside newTx catch/finally
perform compensating DB cleanup that deletes the created BookEntity and
associated BookFile records (using bookRepository and any file storage cleanup
used by performFileMove()), and only call notificationService.sendMessage after
successfully committing the newTx using the freshly loaded BookEntity/DTO (not
fileProcessResult.getBook()). Ensure the code references the
TransactionTemplate/newTx block, MetadataUpdateContext,
metadataRefreshService.updateBookMetadata,
koboAutoShelfService.autoAddBookToKoboShelves, and
notificationService.sendMessage so reviewers can locate and apply the change.
In
`@booklore-api/src/test/java/org/booklore/service/HibernateRegressionTest.java`:
- Around line 912-923: The test currently asserts on entities while the
persistence context is still open, hiding lazy-init issues; after calling
bookRepository.findAllFullBooksBatch(...) in HibernateRegressionTest (the
findAllFullBooksBatch test), clear/detach the persistence context before
asserting (e.g., call EntityManager.clear() or detach the loaded BookEntity) so
that accessing lazy collections happens on a detached entity and will surface
lazy-init failures; reference the findAllFullBooksBatch call and the BookEntity
loaded variable when applying the detach/clear.
---
Nitpick comments:
In
`@booklore-api/src/test/java/org/booklore/service/HibernateRegressionTest.java`:
- Around line 40-41: Replace the class-level `@SpringBootTest`(classes =
BookloreApplication.class) on HibernateRegressionTest with `@DataJpaTest` so the
test loads only JPA components; remove or stop configuring mocks for app-layer
beans like TaskCronService and Flyway (they won’t be needed under `@DataJpaTest`),
ensure you import
org.springframework.boot.test.autoconfigure.orm.jpa.DataJpaTest, and keep or
remove `@Transactional` as desired since `@DataJpaTest` is transactional by default.
In `@booklore-api/src/test/java/org/booklore/service/komga/KomgaServiceTest.java`:
- Around line 210-225: In the shouldGetAllSeriesOptimized() test, move the
komgaMapper.getUnknownSeriesName() stub before any repository stubs that
reference its return value (e.g.,
bookRepository.findBooksBySeriesNameGroupedByLibraryId(...) and
bookRepository.findDistinctSeriesNamesGroupedByLibraryId(...)) so the mapper
value is defined first; update the setup order so
komgaMapper.getUnknownSeriesName() is stubbed before the two
findBooksBySeriesNameGroupedByLibraryId(...) when(...) calls to avoid potential
ordering confusion and make the test clearer.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1df1b782-0316-4a94-92b7-583054a611d6
📒 Files selected for processing (62)
booklore-api/build.gradle.ktsbooklore-api/src/main/java/org/booklore/app/service/AppBookService.javabooklore-api/src/main/java/org/booklore/app/service/AppSeriesService.javabooklore-api/src/main/java/org/booklore/config/security/service/LogoutService.javabooklore-api/src/main/java/org/booklore/controller/BookController.javabooklore-api/src/main/java/org/booklore/controller/MetadataController.javabooklore-api/src/main/java/org/booklore/model/entity/AppSettingEntity.javabooklore-api/src/main/java/org/booklore/model/entity/AuthorEntity.javabooklore-api/src/main/java/org/booklore/model/entity/BookEntity.javabooklore-api/src/main/java/org/booklore/model/entity/BookLoreUserEntity.javabooklore-api/src/main/java/org/booklore/model/entity/BookMetadataEntity.javabooklore-api/src/main/java/org/booklore/model/entity/BookdropFileEntity.javabooklore-api/src/main/java/org/booklore/model/entity/CategoryEntity.javabooklore-api/src/main/java/org/booklore/model/entity/ComicMetadataEntity.javabooklore-api/src/main/java/org/booklore/model/entity/KoboLibrarySnapshotEntity.javabooklore-api/src/main/java/org/booklore/model/entity/LibraryEntity.javabooklore-api/src/main/java/org/booklore/model/entity/MetadataFetchProposalEntity.javabooklore-api/src/main/java/org/booklore/model/entity/MoodEntity.javabooklore-api/src/main/java/org/booklore/model/entity/PdfAnnotationEntity.javabooklore-api/src/main/java/org/booklore/model/entity/TagEntity.javabooklore-api/src/main/java/org/booklore/model/entity/TaskHistoryEntity.javabooklore-api/src/main/java/org/booklore/repository/BookRepository.javabooklore-api/src/main/java/org/booklore/repository/projection/BookEmbeddingProjection.javabooklore-api/src/main/java/org/booklore/service/MagicShelfService.javabooklore-api/src/main/java/org/booklore/service/ReadingSessionService.javabooklore-api/src/main/java/org/booklore/service/book/BookQueryService.javabooklore-api/src/main/java/org/booklore/service/book/BookService.javabooklore-api/src/main/java/org/booklore/service/book/BookUpdateService.javabooklore-api/src/main/java/org/booklore/service/book/NotebookService.javabooklore-api/src/main/java/org/booklore/service/bookdrop/BookDropService.javabooklore-api/src/main/java/org/booklore/service/komga/KomgaService.javabooklore-api/src/main/java/org/booklore/service/library/BookRestorationService.javabooklore-api/src/main/java/org/booklore/service/library/LibraryProcessingService.javabooklore-api/src/main/java/org/booklore/service/library/LibraryService.javabooklore-api/src/main/java/org/booklore/service/metadata/BookMetadataService.javabooklore-api/src/main/java/org/booklore/service/metadata/MetadataTaskService.javabooklore-api/src/main/java/org/booklore/service/progress/ReadingProgressService.javabooklore-api/src/main/java/org/booklore/service/recommender/BookRecommendationService.javabooklore-api/src/main/java/org/booklore/service/restriction/ContentRestrictionService.javabooklore-api/src/main/java/org/booklore/service/upload/FileUploadService.javabooklore-api/src/main/java/org/booklore/service/watcher/BookFilePersistenceService.javabooklore-api/src/main/java/org/booklore/service/watcher/LibraryFileEventProcessor.javabooklore-api/src/main/java/org/booklore/task/tasks/BookRecommendationUpdaterTask.javabooklore-api/src/main/resources/application.yamlbooklore-api/src/test/java/org/booklore/config/security/service/LogoutServiceTest.javabooklore-api/src/test/java/org/booklore/repository/BookOpdsRepositoryDataJpaTest.javabooklore-api/src/test/java/org/booklore/service/BookRuleEvaluatorEdgeCasesTest.javabooklore-api/src/test/java/org/booklore/service/BookRuleEvaluatorFieldCoverageTest.javabooklore-api/src/test/java/org/booklore/service/BookRuleEvaluatorNumericFieldsTest.javabooklore-api/src/test/java/org/booklore/service/BookRuleEvaluatorServiceIntegrationTest.javabooklore-api/src/test/java/org/booklore/service/HibernateRegressionTest.javabooklore-api/src/test/java/org/booklore/service/book/BookCreatorServiceTest.javabooklore-api/src/test/java/org/booklore/service/book/BookFileAttachmentServiceTest.javabooklore-api/src/test/java/org/booklore/service/book/BookFileDetachmentServiceTest.javabooklore-api/src/test/java/org/booklore/service/bookdrop/BookDropServiceTest.javabooklore-api/src/test/java/org/booklore/service/kobo/KoboLibrarySnapshotServiceTest.javabooklore-api/src/test/java/org/booklore/service/komga/KomgaServiceTest.javabooklore-api/src/test/java/org/booklore/service/library/LibraryProcessingServiceRegressionTest.javabooklore-api/src/test/java/org/booklore/service/library/LibraryProcessingServiceTest.javabooklore-api/src/test/java/org/booklore/service/watcher/LibraryFileEventProcessorTest.javabooklore-api/src/test/java/org/booklore/task/tasks/BookRecommendationUpdaterTaskTest.javabooklore-api/src/test/resources/application-test.yml
💤 Files with no reviewable changes (2)
- booklore-api/src/main/java/org/booklore/controller/MetadataController.java
- booklore-api/src/main/java/org/booklore/service/metadata/BookMetadataService.java
✅ Files skipped from review due to trivial changes (8)
- booklore-api/src/test/resources/application-test.yml
- booklore-api/src/main/java/org/booklore/repository/projection/BookEmbeddingProjection.java
- booklore-api/src/main/java/org/booklore/model/entity/CategoryEntity.java
- booklore-api/src/test/java/org/booklore/service/book/BookFileAttachmentServiceTest.java
- booklore-api/src/test/java/org/booklore/service/BookRuleEvaluatorEdgeCasesTest.java
- booklore-api/src/test/java/org/booklore/service/kobo/KoboLibrarySnapshotServiceTest.java
- booklore-api/src/main/java/org/booklore/service/ReadingSessionService.java
- booklore-api/src/main/java/org/booklore/service/recommender/BookRecommendationService.java
🚧 Files skipped from review as they are similar to previous changes (30)
- booklore-api/src/test/java/org/booklore/service/watcher/LibraryFileEventProcessorTest.java
- booklore-api/src/main/java/org/booklore/service/book/BookUpdateService.java
- booklore-api/src/main/java/org/booklore/model/entity/AppSettingEntity.java
- booklore-api/src/main/java/org/booklore/model/entity/BookEntity.java
- booklore-api/src/main/java/org/booklore/config/security/service/LogoutService.java
- booklore-api/src/main/java/org/booklore/model/entity/TaskHistoryEntity.java
- booklore-api/src/main/java/org/booklore/model/entity/TagEntity.java
- booklore-api/src/main/java/org/booklore/service/watcher/BookFilePersistenceService.java
- booklore-api/src/main/java/org/booklore/service/watcher/LibraryFileEventProcessor.java
- booklore-api/src/main/java/org/booklore/service/progress/ReadingProgressService.java
- booklore-api/src/main/java/org/booklore/service/MagicShelfService.java
- booklore-api/src/main/java/org/booklore/service/restriction/ContentRestrictionService.java
- booklore-api/src/main/java/org/booklore/model/entity/MetadataFetchProposalEntity.java
- booklore-api/src/main/java/org/booklore/model/entity/PdfAnnotationEntity.java
- booklore-api/src/main/java/org/booklore/service/library/BookRestorationService.java
- booklore-api/src/test/java/org/booklore/task/tasks/BookRecommendationUpdaterTaskTest.java
- booklore-api/src/main/java/org/booklore/app/service/AppBookService.java
- booklore-api/src/main/java/org/booklore/model/entity/AuthorEntity.java
- booklore-api/src/test/java/org/booklore/service/book/BookFileDetachmentServiceTest.java
- booklore-api/src/test/java/org/booklore/service/BookRuleEvaluatorServiceIntegrationTest.java
- booklore-api/src/main/java/org/booklore/service/book/NotebookService.java
- booklore-api/src/test/java/org/booklore/service/bookdrop/BookDropServiceTest.java
- booklore-api/src/main/java/org/booklore/model/entity/BookdropFileEntity.java
- booklore-api/src/main/java/org/booklore/task/tasks/BookRecommendationUpdaterTask.java
- booklore-api/src/main/java/org/booklore/model/entity/LibraryEntity.java
- booklore-api/src/main/java/org/booklore/service/komga/KomgaService.java
- booklore-api/src/main/java/org/booklore/service/library/LibraryService.java
- booklore-api/src/main/java/org/booklore/model/entity/KoboLibrarySnapshotEntity.java
- booklore-api/src/main/java/org/booklore/service/metadata/MetadataTaskService.java
- booklore-api/src/test/java/org/booklore/service/book/BookCreatorServiceTest.java
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Test Suite / Backend Tests
- GitHub Check: Analyze (java-kotlin)
🧰 Additional context used
📓 Path-based instructions (2)
booklore-api/src/**/*.java
📄 CodeRabbit inference engine (AGENTS.md)
booklore-api/src/**/*.java: Use 4-space indentation and match surrounding Java style
Prefer constructor injection via Lombok patterns already used in the codebase. Do not introduce@Autowiredfield injection
Use MapStruct for entity/DTO mapping
Keep JPA entities on the *Entity suffix
Files:
booklore-api/src/test/java/org/booklore/config/security/service/LogoutServiceTest.javabooklore-api/src/main/java/org/booklore/service/upload/FileUploadService.javabooklore-api/src/main/java/org/booklore/model/entity/BookLoreUserEntity.javabooklore-api/src/main/java/org/booklore/app/service/AppSeriesService.javabooklore-api/src/test/java/org/booklore/service/library/LibraryProcessingServiceRegressionTest.javabooklore-api/src/main/java/org/booklore/controller/BookController.javabooklore-api/src/main/java/org/booklore/model/entity/ComicMetadataEntity.javabooklore-api/src/main/java/org/booklore/model/entity/BookMetadataEntity.javabooklore-api/src/main/java/org/booklore/service/bookdrop/BookDropService.javabooklore-api/src/test/java/org/booklore/repository/BookOpdsRepositoryDataJpaTest.javabooklore-api/src/test/java/org/booklore/service/BookRuleEvaluatorFieldCoverageTest.javabooklore-api/src/test/java/org/booklore/service/BookRuleEvaluatorNumericFieldsTest.javabooklore-api/src/main/java/org/booklore/service/book/BookService.javabooklore-api/src/main/java/org/booklore/model/entity/MoodEntity.javabooklore-api/src/main/java/org/booklore/service/library/LibraryProcessingService.javabooklore-api/src/test/java/org/booklore/service/library/LibraryProcessingServiceTest.javabooklore-api/src/main/java/org/booklore/service/book/BookQueryService.javabooklore-api/src/main/java/org/booklore/repository/BookRepository.javabooklore-api/src/test/java/org/booklore/service/HibernateRegressionTest.javabooklore-api/src/test/java/org/booklore/service/komga/KomgaServiceTest.java
booklore-api/src/test/**/*.java
📄 CodeRabbit inference engine (AGENTS.md)
Prefer focused unit tests; use
@SpringBootTestonly when the Spring context is required
Files:
booklore-api/src/test/java/org/booklore/config/security/service/LogoutServiceTest.javabooklore-api/src/test/java/org/booklore/service/library/LibraryProcessingServiceRegressionTest.javabooklore-api/src/test/java/org/booklore/repository/BookOpdsRepositoryDataJpaTest.javabooklore-api/src/test/java/org/booklore/service/BookRuleEvaluatorFieldCoverageTest.javabooklore-api/src/test/java/org/booklore/service/BookRuleEvaluatorNumericFieldsTest.javabooklore-api/src/test/java/org/booklore/service/library/LibraryProcessingServiceTest.javabooklore-api/src/test/java/org/booklore/service/HibernateRegressionTest.javabooklore-api/src/test/java/org/booklore/service/komga/KomgaServiceTest.java
🧠 Learnings (7)
📚 Learning: 2026-03-25T16:46:06.366Z
Learnt from: CR
Repo: grimmory-tools/grimmory PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-25T16:46:06.366Z
Learning: Applies to booklore-api/src/**/*.java : Keep JPA entities on the *Entity suffix
Applied to files:
booklore-api/src/main/java/org/booklore/model/entity/BookLoreUserEntity.javabooklore-api/src/main/java/org/booklore/app/service/AppSeriesService.javabooklore-api/src/main/java/org/booklore/model/entity/ComicMetadataEntity.javabooklore-api/src/main/java/org/booklore/model/entity/BookMetadataEntity.javabooklore-api/src/test/java/org/booklore/repository/BookOpdsRepositoryDataJpaTest.javabooklore-api/src/test/java/org/booklore/service/BookRuleEvaluatorFieldCoverageTest.javabooklore-api/src/main/java/org/booklore/model/entity/MoodEntity.javabooklore-api/src/test/java/org/booklore/service/HibernateRegressionTest.java
📚 Learning: 2026-03-25T16:46:06.366Z
Learnt from: CR
Repo: grimmory-tools/grimmory PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-25T16:46:06.366Z
Learning: Applies to booklore-api/src/**/*.java : Use MapStruct for entity/DTO mapping
Applied to files:
booklore-api/src/main/java/org/booklore/model/entity/BookMetadataEntity.javabooklore-api/src/test/java/org/booklore/repository/BookOpdsRepositoryDataJpaTest.javabooklore-api/src/main/java/org/booklore/service/book/BookService.javabooklore-api/src/main/java/org/booklore/service/book/BookQueryService.javabooklore-api/src/test/java/org/booklore/service/HibernateRegressionTest.javabooklore-api/src/test/java/org/booklore/service/komga/KomgaServiceTest.java
📚 Learning: 2026-03-24T18:46:47.249Z
Learnt from: pedronave
Repo: grimmory-tools/grimmory PR: 173
File: booklore-api/src/main/java/org/booklore/service/hardcover/HardcoverSyncService.java:188-199
Timestamp: 2026-03-24T18:46:47.249Z
Learning: In grimmory-tools/grimmory (HardcoverSyncService.java), when a `hardcoverBookId` is stored in book metadata but is malformed (non-numeric), the preferred behavior is to return null and skip the sync rather than falling back to ISBN-based lookup. The reasoning is that an explicitly set bookId represents deliberate user intent, and silently resolving via ISBN could sync progress to a different book than intended.
Applied to files:
booklore-api/src/main/java/org/booklore/service/bookdrop/BookDropService.java
📚 Learning: 2026-03-25T16:46:06.366Z
Learnt from: CR
Repo: grimmory-tools/grimmory PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-25T16:46:06.366Z
Learning: Applies to booklore-api/src/test/**/*.java : Prefer focused unit tests; use SpringBootTest only when the Spring context is required
Applied to files:
booklore-api/src/test/java/org/booklore/repository/BookOpdsRepositoryDataJpaTest.javabooklore-api/src/test/java/org/booklore/service/BookRuleEvaluatorFieldCoverageTest.javabooklore-api/src/test/java/org/booklore/service/BookRuleEvaluatorNumericFieldsTest.javabooklore-api/src/test/java/org/booklore/service/library/LibraryProcessingServiceTest.javabooklore-api/src/test/java/org/booklore/service/HibernateRegressionTest.javabooklore-api/src/test/java/org/booklore/service/komga/KomgaServiceTest.java
📚 Learning: 2026-03-25T16:46:06.366Z
Learnt from: CR
Repo: grimmory-tools/grimmory PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-25T16:46:06.366Z
Learning: Applies to booklore-api/src/**/*.java : Prefer constructor injection via Lombok patterns already used in the codebase. Do not introduce Autowired field injection
Applied to files:
booklore-api/src/test/java/org/booklore/repository/BookOpdsRepositoryDataJpaTest.javabooklore-api/src/test/java/org/booklore/service/BookRuleEvaluatorFieldCoverageTest.javabooklore-api/src/test/java/org/booklore/service/BookRuleEvaluatorNumericFieldsTest.javabooklore-api/src/test/java/org/booklore/service/HibernateRegressionTest.java
📚 Learning: 2026-03-24T18:58:08.199Z
Learnt from: pedronave
Repo: grimmory-tools/grimmory PR: 173
File: booklore-api/src/main/java/org/booklore/service/hardcover/HardcoverSyncService.java:291-294
Timestamp: 2026-03-24T18:58:08.199Z
Learning: In grimmory-tools/grimmory (HardcoverSyncService.java), when a Hardcover edition has no page count, falling back to the book-level page count (book.pages) is the preferred behavior. Skipping the sync entirely in this case is considered worse than using a slightly approximate total, because the page count is only a proxy for converting progress % to page numbers for the API. The Hardcover API allows setting page-based progress even when the edition lacks a page count, and progress displays in at least some UI views (e.g., the book detail dropdown).
Applied to files:
booklore-api/src/main/java/org/booklore/service/book/BookService.javabooklore-api/src/main/java/org/booklore/service/book/BookQueryService.java
📚 Learning: 2026-03-25T21:02:55.427Z
Learnt from: imnotjames
Repo: grimmory-tools/grimmory PR: 195
File: booklore-api/src/main/java/org/booklore/config/logging/RequestLoggingFilterConfig.java:15-19
Timestamp: 2026-03-25T21:02:55.427Z
Learning: In `booklore-api/src/main/java/org/booklore/config/logging/RequestLoggingFilterConfig.java` and `booklore-api/src/main/java/org/booklore/config/logging/filter/RequestLoggingFilter.java`, logging all request headers and payloads at DEBUG level is intentional and accepted behavior in this project. The previous `LoggingFilter` also logged all headers. No header redaction or payload scrubbing is required — the DEBUG log level is considered sufficient access control. The new filter is not profile-gated (unlike the old `Profile("dev")` filter) but relies on `logger.isDebugEnabled()` for gating.
Applied to files:
booklore-api/src/main/resources/application.yaml
🔇 Additional comments (34)
booklore-api/src/test/java/org/booklore/config/security/service/LogoutServiceTest.java (1)
152-152: Bulk-save verification looks correct.This assertion is aligned with the service’s batched token persistence path and keeps the test intent clear.
booklore-api/src/main/java/org/booklore/model/entity/BookLoreUserEntity.java (1)
10-10:Objectsimport is correct and needed.Used by
Objects.equals(...)inequals, so this addition looks good.booklore-api/build.gradle.kts (2)
141-146: LGTM — consider documenting the heap size rationale.The 2GB heap allocation is reasonable for JPA integration tests with lazy loading verification, especially when combined with the bytecode enhancement agent (
-XX:+EnableDynamicAgentLoading). If this was added to address OOM during tests, a brief comment documenting why would help future maintainers.
134-139: No changes required—Hibernate enhancement configuration is properly implemented with correct transactional boundaries.The concerns raised in the original review have been addressed in the codebase:
Bidirectional association synchronization: Service code correctly sets both sides of relationships. For example, in
LibraryService.java(line 142), newLibraryPathEntityobjects are built with.library(library)back-reference before being added to the collection. TheHibernateRegressionTest.LibraryPathBackReferenceclass explicitly validates this pattern works correctly.Transactional coverage for lazy loading:
LibraryServicehas class-level@Transactional(readOnly = true)covering all methods, with explicit@Transactionaldecorators on write operations. Similar patterns apply across other services. The absence ofhibernate.open-in-viewis intentional—it enforces that lazy fields are accessed only within active transaction boundaries, which is the correct approach.Regression testing: The
HibernateRegressionTestsuite includes multiple@Nestedtest classes specifically validating entity relationships (LibraryPathBackReference), lazy loading (CollectionInitialization), and lazy field access patterns (MetadataDescriptionAccess) after the Hibernate refactoring.booklore-api/src/main/java/org/booklore/service/book/BookService.java (2)
50-51: Class-level@Transactionalholds DB connections during file I/O operations.The class-level
@Transactional(readOnly = true)wraps file-serving methods likedownloadBook,downloadAllBookFiles,getBookContent, andstreamBookContent. These methods spend most of their time in filesystem/HTTP I/O, so this keeps a Hibernate session and DB connection open for the entire transfer duration rather than just the initial lookup.Consider removing the class-level annotation and adding
@Transactional(readOnly = true)only to the specific database-access methods that need it.
104-133: LGTM! New paginated endpoint follows existing patterns.The
getBookDTOsPagedmethod correctly:
- Performs authorization checks (admin vs. user library scope)
- Batch-loads progress data to avoid N+1 queries
- Filters shelves by user ID
- Returns a properly mapped
Page<Book>booklore-api/src/main/java/org/booklore/model/entity/ComicMetadataEntity.java (1)
82-107: LGTM! Good optimization from SUBSELECT to BatchSize.Replacing
@Fetch(FetchMode.SUBSELECT)with@BatchSize(size = 20)on the collection associations is a sensible optimization. SUBSELECT can load all collections for all loaded parent entities in one query (memory-heavy for large datasets), while BatchSize provides more predictable memory usage by loading collections in configurable batches.booklore-api/src/test/java/org/booklore/service/BookRuleEvaluatorNumericFieldsTest.java (1)
49-60: LGTM! Test configuration aligns with application-test.yml baseline.The addition of
provider_disables_autocommit=falseis correct for H2-based tests. This differs from production'struesetting intentionally—H2 doesn't benefit from the HikariCP optimization, andfalseensures proper test transaction management with@Transactionalrollback semantics.booklore-api/src/test/java/org/booklore/service/BookRuleEvaluatorFieldCoverageTest.java (1)
56-67: LGTM! Consistent with other test files in this PR.The configuration changes mirror those in the sibling test classes, maintaining consistency across the test suite.
booklore-api/src/main/resources/application.yaml (2)
60-68: Good HikariCP and OSIV optimizations.
auto-commit: falsepaired withprovider_disables_autocommit: trueeliminates unnecessary JDBC roundtrips for autocommit status checks.open-in-view: falseis a best practice that prevents accidental lazy loading in controller/view layers, forcing proper eager fetching in the service layer.
79-79: Good safety net for pagination queries.Enabling
fail_on_pagination_over_collection_fetch: truewill cause Hibernate to fail fast when attempting in-memory pagination over collection fetches (which silently loads all rows then paginates in memory). This helps catch performance issues during development rather than in production.Ensure all paginated repository queries use only
@EntityGraphwith ToOne paths or rely on@BatchSizefor collections. The repository changes in this PR appear to follow this pattern correctly.booklore-api/src/main/java/org/booklore/controller/BookController.java (1)
77-83: LGTM! New paginated endpoint follows existing authorization patterns.The endpoint correctly:
- Uses Spring Data's
Pageableargument resolver (enabled via@EnableSpringDataWebSupport)- Respects the global
max-page-size: 100limit configured inapplication.yaml- Delegates authorization to the service layer (consistent with the existing
GET /api/v1/booksendpoint)- Hides the
Pageableparameter from OpenAPI docs (standard practice since pagination params are auto-documented)booklore-api/src/test/java/org/booklore/repository/BookOpdsRepositoryDataJpaTest.java (1)
48-65: LGTM! Consistent test configuration updates.The changes align with the pattern applied across all test files in this PR.
booklore-api/src/main/java/org/booklore/repository/BookRepository.java (5)
69-72: LGTM! Correctly narrowed EntityGraph for list queries.Limiting
@EntityGraphto ToOne paths (metadata,metadata.comicMetadata,library) while relying on@BatchSizefor collections prevents Cartesian products and aligns with thefail_on_pagination_over_collection_fetch: trueconfiguration.
96-107: LGTM! New rescan and soft-delete queries.
findAllByLibraryIdForRescan: Appropriately fetches onlybookFilesandlibraryPathneeded for rescan logic.findDeletedByLibraryIdWithFiles: Correctly scopes to soft-deleted books with necessary file paths for restoration.
336-351: LGTM! Cleaner series matching with parameterized condition.The change from file-based fallback to
(m.seriesName IS NULL AND :seriesName = :unknownSeriesName)is more efficient and easier to understand. It correctly matches books without a series name only when the requested series name equals the "Unknown Series" placeholder.
431-491: LGTM! Pagination queries follow ToOne-only EntityGraph pattern.All new paginated methods (
findAllWithMetadataPaged,findAllWithMetadataByLibraryIdPaged,findAllWithMetadataPage,findAllWithMetadataByLibraryIdsPage,findAllFullBooksBatch) correctly use ToOne-only paths in their@EntityGraphdefinitions, avoiding Cartesian products and ensuring compatibility withfail_on_pagination_over_collection_fetch: true.
497-505: LGTM! Lightweight projection for recommendation lookups.The
findAllEmbeddingsForRecommendationmethod uses a projection interface to fetch only the required fields (bookId,embeddingVector,seriesName), which is significantly more efficient than loading full entities when pre-computed embeddings are available.booklore-api/src/main/java/org/booklore/model/entity/BookMetadataEntity.java (2)
303-314: Verify Hibernate bytecode enhancement is configured for lazy basic fields.
@Basic(fetch = FetchType.LAZY)with@LazyGrouprequires Hibernate bytecode enhancement withenableLazyInitialization = true. Without this, the lazy annotation will be silently ignored and these fields will still load eagerly.#!/bin/bash # Check if Hibernate enhancement includes enableLazyInitialization fd -HI 'build\.gradle\.kts' | xargs grep -A 10 'hibernate' | grep -E '(enableLazyInitialization|enableAssociationManagement|enhancement)'
379-417: LGTM - Batch loading replaces subselect fetching.Replacing
@Fetch(FetchMode.SUBSELECT)with@BatchSize(size = 20)and adding@Builder.Defaultinitializers is a sound optimization. Batch loading provides more predictable query patterns and the non-null defaults prevent NPEs when building entities.booklore-api/src/main/java/org/booklore/app/service/AppSeriesService.java (1)
175-182: LGTM - Query optimization aligns with batch loading strategy.The query now fetches only the To-One
metadataassociation eagerly. TheauthorsandbookFilescollections accessed later (lines 207-212, 220-228) will be batch-loaded via@BatchSize(size = 20)onBookMetadataEntity.authorsandBookEntity.bookFiles. This reduces the initial query complexity while still avoiding N+1 when iterating over multiple books.booklore-api/src/test/java/org/booklore/service/komga/KomgaServiceTest.java (1)
105-109: LGTM - Test stubs align with refactored service flow.The stubs correctly match the new repository method signatures for series-grouped queries. The
getUnknownSeriesName()mock returns a consistent value, which aligns with how the production code uses it across multiple calls.booklore-api/src/main/java/org/booklore/service/library/LibraryProcessingService.java (3)
52-71: LGTM - Repository-based book fetching improves memory efficiency.Moving from
libraryEntity.getBookEntities()tobookRepository.findAllByLibraryIdForRescan(libraryId)avoids eagerly loading all books through the entity relationship, which is especially beneficial for large libraries.
73-132: LGTM - Proper entity refresh after persistence context clear.The pattern of calling
entityManager.clear()followed by re-fetching both the library (viafindByIdWithPaths) and books (viafindAllByLibraryIdForRescan) ensures fresh state for the subsequentdetectNewBookPathscall. This correctly handles detached entity scenarios.
148-164: LGTM - Static method improves testability.Making
detectDeletedBookIdsaprotected staticmethod that accepts aList<BookEntity>directly allows for easier unit testing without requiring entity relationships to be set up.booklore-api/src/main/java/org/booklore/service/book/BookQueryService.java (3)
56-65: LGTM - Explicit initialization prevents lazy loading issues outside transaction.Calling
Hibernate.initialize()onauthorsandcategoriesensures these collections are loaded within the transaction boundary, making the entities safe to use after the transaction completes.
88-100: LGTM - Conditional update avoids unnecessary writes.The
Objects.equalscheck before settingembeddingVectorprevents dirty-checking overhead and unnecessary database updates when embeddings haven't changed.
102-116: LGTM - Batch processing with reasonable chunk size.Processing recommendations in batches with
findAllByIdandsaveAllper batch prevents loading the entire dataset into memory and allows Hibernate to flush periodically.booklore-api/src/main/java/org/booklore/model/entity/MoodEntity.java (1)
27-30: LGTM - Consistent batch size configuration.Adding
@BatchSize(size = 20)to the inverse side of the many-to-many relationship maintains consistency with the owning side inBookMetadataEntity.moodsand other collections in this PR.booklore-api/src/test/java/org/booklore/service/library/LibraryProcessingServiceRegressionTest.java (2)
56-70: LGTM - Constructor updated to match production service.The test correctly injects the new
BookRepositorydependency, maintaining alignment with the productionLibraryProcessingServiceconstructor.
95-96: LGTM - Stubs updated for refactored repository methods.The test now stubs
findByIdWithPathsandfindAllByLibraryIdForRescanto match the production rescan flow that separates library path loading from book fetching.booklore-api/src/test/java/org/booklore/service/library/LibraryProcessingServiceTest.java (2)
59-73: LGTM - Test setup correctly updated for new dependency.The constructor injection of
BookRepositorymock aligns with the production service changes, maintaining proper test isolation.
567-584: LGTM - Verification confirms proper entity refresh pattern.The
times(2)verification for bothfindByIdWithPathsandfindAllByLibraryIdForRescancorrectly validates that the service re-fetches state afterentityManager.clear(), ensuring no stale entity references are used.booklore-api/src/main/java/org/booklore/service/bookdrop/BookDropService.java (1)
43-45: Constructor-injected transaction manager matches the existing service pattern.Adding
PlatformTransactionManageras afinaldependency keeps this service on the Lombok constructor-injection style and avoids field injection. As per coding guidelines: "Prefer constructor injection via Lombok patterns already used in the codebase. Do not introduce@Autowiredfield injection".Also applies to: 81-81
booklore-api/src/main/java/org/booklore/model/entity/BookLoreUserEntity.java
Outdated
Show resolved
Hide resolved
booklore-api/src/test/java/org/booklore/service/HibernateRegressionTest.java
Show resolved
Hide resolved
booklore-api/src/test/java/org/booklore/service/HibernateRegressionTest.java
Show resolved
Hide resolved
…serEntity.java Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
# Conflicts: # booklore-api/src/test/java/org/booklore/service/book/BookCreatorServiceTest.java
# Conflicts: # booklore-api/src/main/java/org/booklore/service/book/BookService.java # frontend/src/app/features/book/service/book.service.ts
|
@coderabbitai: review |
|
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (1)
booklore-api/src/main/java/org/booklore/service/book/BookQueryService.java (1)
47-53:⚠️ Potential issue | 🟠 MajorPost-filtering breaks page totals and offsets.
applyRestrictions(...)can remove books on Line 49, but Line 53 still reuses the unfilteredpage.getTotalElements(). That makestotalElements/totalPagesinaccurate, and because the filtering happens after the offset is chosen, clients can still get sparse or empty pages. This needs query-level restriction handling, or an equivalent restricted count/slice calculation before building thePageImpl.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@booklore-api/src/main/java/org/booklore/service/book/BookQueryService.java` around lines 47 - 53, The current getAllBooksByLibraryIdsPaged applies contentRestrictionService.applyRestrictions after executing bookRepository.findAllWithMetadataByLibraryIdsPage, causing incorrect totals and sparse pages; change it to compute restriction-aware pagination: use bookRepository to fetch only ids for the libraryIds (e.g., findIdsByLibraryIds), apply contentRestrictionService.applyRestrictions to those entities/ids to get the filtered id set, calculate totalFiltered = filteredIds.size(), compute the sublist of IDs for the requested Pageable (offset/limit), then fetch the full BookEntity page for that sublist (e.g., findAllWithMetadataByIds) and map them with mapBookToDto before returning new PageImpl<>(dtos, pageable, totalFiltered); ensure getAllBooksByLibraryIdsPaged, bookRepository.findAllWithMetadataByLibraryIdsPage, contentRestrictionService.applyRestrictions, and mapBookToDto are used/updated accordingly.
🧹 Nitpick comments (2)
booklore-api/src/main/java/org/booklore/config/security/oidc/BackchannelLogoutService.java (1)
87-96: Consider batching session and token saves.The loop calls
save()individually for each session (line 89) and each refresh token (line 95). Given the PR objectives mention optimizing token revocation withsaveAll, consider collecting entities and persisting in batch:♻️ Suggested batched approach
+ List<RefreshTokenEntity> tokensToRevoke = new ArrayList<>(); for (OidcSessionEntity session : sessions) { session.setRevoked(true); - oidcSessionRepository.save(session); var user = session.getUser(); refreshTokenRepository.findAllByUserAndRevokedFalse(user).forEach(token -> { token.setRevoked(true); token.setRevocationDate(Instant.now()); - refreshTokenRepository.save(token); + tokensToRevoke.add(token); }); // ... notifications and audit ... } + oidcSessionRepository.saveAll(sessions); + refreshTokenRepository.saveAll(tokensToRevoke);,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@booklore-api/src/main/java/org/booklore/config/security/oidc/BackchannelLogoutService.java` around lines 87 - 96, In BackchannelLogoutService (the loop iterating over OidcSessionEntity sessions), avoid per-entity saves by collecting modified OidcSessionEntity objects into a list and calling oidcSessionRepository.saveAll(collectedSessions) once, and similarly collect modified RefreshToken entities (after setting revoked=true and revocationDate=Instant.now()) into a list to call refreshTokenRepository.saveAll(collectedTokens) once; ensure you still set session.setRevoked(true) and token.setRevoked(true)/setRevocationDate(...) before batching, and keep the method transactional so both batched saves occur atomically.booklore-api/src/main/java/org/booklore/service/book/BookService.java (1)
123-131: Consider extracting common enrichment logic.The book enrichment and shelf filtering logic (lines 123-131) duplicates code from
getBookDTOs(lines 92-100). A private helper method could reduce this duplication:private void enrichBooksForUser(Collection<Book> books, Long userId, Map<Long, UserBookProgressEntity> progressMap, Map<Long, UserBookFileProgressEntity> fileProgressMap, boolean setShelvesNullWhenEmpty) { books.forEach(book -> { readingProgressService.enrichBookWithProgress( book, progressMap.get(book.getId()), fileProgressMap.get(book.getId())); Set<Shelf> filtered = filterShelvesByUserId(book.getShelves(), userId); book.setShelves(setShelvesNullWhenEmpty && filtered != null && filtered.isEmpty() ? null : filtered); }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@booklore-api/src/main/java/org/booklore/service/book/BookService.java` around lines 123 - 131, Duplicate enrichment and shelf-filtering logic in the bookPage loop and getBookDTOs should be extracted into a private helper (e.g., enrichBooksForUser) that accepts Collection<Book> books, Long userId, Map<Long,UserBookProgressEntity> progressMap, Map<Long,UserBookFileProgressEntity> fileProgressMap, and a boolean setShelvesNullWhenEmpty; move the readingProgressService.enrichBookWithProgress(...) call and the filterShelvesByUserId(...) + book.setShelves(...) logic into that helper and replace both original blocks (the bookPage.forEach and the getBookDTOs loop) with calls to enrichBooksForUser to eliminate duplication while preserving the current null/empty shelf behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@booklore-api/src/main/java/org/booklore/config/security/oidc/BackchannelLogoutService.java`:
- Around line 32-35: The PROCESSED_JTIS Caffeine cache in
BackchannelLogoutService currently has maximumSize(1_000), which can be
exhausted and allow JTI replay; change the cache configuration in
BackchannelLogoutService.PROCESSED_JTIS to use a larger maximumSize (e.g.,
10_000) or implement rate limiting on the /backchannel-logout endpoint (see
OidcAuthController) to prevent cache eviction attacks—locate the PROCESSED_JTIS
declaration and either update maximumSize(10_000) or wire in a rate limiter on
OidcAuthController.backchannelLogout to throttle requests and protect the cache.
In
`@booklore-api/src/main/java/org/booklore/config/security/service/AuthRateLimitService.java`:
- Around line 23-29: The rate-limit cache in AuthRateLimitService is too small
(maximumSize(1000)) and can be evicted by distributed attacks; update the
attemptCache configuration in the AuthRateLimitService constructor to use a
larger capacity (e.g., maximumSize(10000)) or replace the in-memory Caffeine
cache with a robust external store (e.g., Redis with TTL) for tracking keys like
the "login:ip:" / "login:user:" and "refresh:" entries so counters are not lost
under churn; modify the attemptCache initialization accordingly (or inject a
Redis-backed store) and ensure other methods that read/write attemptCache still
operate against the new store.
In
`@booklore-api/src/main/java/org/booklore/repository/UserBookProgressRepository.java`:
- Around line 220-227: The JPQL IN :libraryIds will fail for null/empty; update
handling so getContinueReading()/getContinueListening() guard libraryIds from
AppBookService.getAccessibleLibraryIds(): if libraryIds is empty return an empty
result immediately, and if libraryIds is null allow "no filter" by changing the
repository queries (findTopContinueReadingBookIds and
findTopContinueListeningBookIds) to use a null-safe predicate like "AND
(:libraryIds IS NULL OR b.library.id IN :libraryIds)" so null means no library
restriction while empty collection still yields no results.
- Around line 212-223: The query in findTopContinueReadingBookIds and
findTopContinueListeningBookIds uses SELECT DISTINCT with ORDER BY
ubp.lastReadTime while joining b.bookFiles, which causes PostgreSQL errors and
inefficiency; replace the JOIN b.bookFiles bf + DISTINCT with an EXISTS subquery
that checks for a matching BookFile (e.g., EXISTS (SELECT 1 FROM BookFileEntity
bf WHERE bf.book = b AND bf.isBookFormat = true AND bf.bookType <>
org.booklore.model.enums.BookFileType.AUDIOBOOK)), remove DISTINCT from the
select, keep SELECT ubp.book.id and the ORDER BY ubp.lastReadTime DESC, and
preserve the other filters (ubp.user.id, b.deleted, b.library.id IN :libraryIds,
ubp.lastReadTime IS NOT NULL) so the result is deduplicated correctly and orders
by lastReadTime.
In `@booklore-api/src/main/java/org/booklore/service/book/BookQueryService.java`:
- Around line 102-106: Reject non-positive batchSize at the start of
saveRecommendationsInBatches by validating the batchSize parameter (and
optionally recommendations) before entering the loop; if batchSize <= 0, throw
an IllegalArgumentException with a clear message so the for-loop that uses
batchSize can never hang or misbehave.
In `@frontend/src/app/features/book/service/book.service.ts`:
- Line 105: The staleTime of 30_000 combined with the current cache patch in
updateBookShelves risks showing stale list entries; update updateBookShelves to
invalidate or refetch the BOOKS_QUERY_KEY list after the mutation completes (or
on mutation success) instead of relying solely on the patch — e.g., perform your
optimistic cache patch as now but call
queryClient.invalidateQueries(BOOKS_QUERY_KEY) (or trigger a refetch) on success
so backend-side changes beyond the patched fields are reflected immediately.
---
Duplicate comments:
In `@booklore-api/src/main/java/org/booklore/service/book/BookQueryService.java`:
- Around line 47-53: The current getAllBooksByLibraryIdsPaged applies
contentRestrictionService.applyRestrictions after executing
bookRepository.findAllWithMetadataByLibraryIdsPage, causing incorrect totals and
sparse pages; change it to compute restriction-aware pagination: use
bookRepository to fetch only ids for the libraryIds (e.g., findIdsByLibraryIds),
apply contentRestrictionService.applyRestrictions to those entities/ids to get
the filtered id set, calculate totalFiltered = filteredIds.size(), compute the
sublist of IDs for the requested Pageable (offset/limit), then fetch the full
BookEntity page for that sublist (e.g., findAllWithMetadataByIds) and map them
with mapBookToDto before returning new PageImpl<>(dtos, pageable,
totalFiltered); ensure getAllBooksByLibraryIdsPaged,
bookRepository.findAllWithMetadataByLibraryIdsPage,
contentRestrictionService.applyRestrictions, and mapBookToDto are used/updated
accordingly.
---
Nitpick comments:
In
`@booklore-api/src/main/java/org/booklore/config/security/oidc/BackchannelLogoutService.java`:
- Around line 87-96: In BackchannelLogoutService (the loop iterating over
OidcSessionEntity sessions), avoid per-entity saves by collecting modified
OidcSessionEntity objects into a list and calling
oidcSessionRepository.saveAll(collectedSessions) once, and similarly collect
modified RefreshToken entities (after setting revoked=true and
revocationDate=Instant.now()) into a list to call
refreshTokenRepository.saveAll(collectedTokens) once; ensure you still set
session.setRevoked(true) and token.setRevoked(true)/setRevocationDate(...)
before batching, and keep the method transactional so both batched saves occur
atomically.
In `@booklore-api/src/main/java/org/booklore/service/book/BookService.java`:
- Around line 123-131: Duplicate enrichment and shelf-filtering logic in the
bookPage loop and getBookDTOs should be extracted into a private helper (e.g.,
enrichBooksForUser) that accepts Collection<Book> books, Long userId,
Map<Long,UserBookProgressEntity> progressMap,
Map<Long,UserBookFileProgressEntity> fileProgressMap, and a boolean
setShelvesNullWhenEmpty; move the
readingProgressService.enrichBookWithProgress(...) call and the
filterShelvesByUserId(...) + book.setShelves(...) logic into that helper and
replace both original blocks (the bookPage.forEach and the getBookDTOs loop)
with calls to enrichBooksForUser to eliminate duplication while preserving the
current null/empty shelf behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b6df7c7a-0e25-4ea2-8ce0-a5bd0126c9ab
📒 Files selected for processing (20)
Dockerfilebooklore-api/build.gradle.ktsbooklore-api/src/main/java/org/booklore/app/service/AppBookService.javabooklore-api/src/main/java/org/booklore/config/security/oidc/BackchannelLogoutService.javabooklore-api/src/main/java/org/booklore/config/security/oidc/OidcStateService.javabooklore-api/src/main/java/org/booklore/config/security/service/AuthRateLimitService.javabooklore-api/src/main/java/org/booklore/config/security/service/LoginRateLimitService.javabooklore-api/src/main/java/org/booklore/controller/BookController.javabooklore-api/src/main/java/org/booklore/mapper/v2/BookMapperV2.javabooklore-api/src/main/java/org/booklore/model/entity/BookLoreUserEntity.javabooklore-api/src/main/java/org/booklore/repository/UserBookProgressRepository.javabooklore-api/src/main/java/org/booklore/service/IconService.javabooklore-api/src/main/java/org/booklore/service/book/BookQueryService.javabooklore-api/src/main/java/org/booklore/service/book/BookService.javabooklore-api/src/main/java/org/booklore/service/metadata/BookMetadataService.javabooklore-api/src/main/java/org/booklore/util/koreader/EpubCfiService.javabooklore-api/src/main/resources/application.yamlbooklore-api/src/test/java/org/booklore/service/book/BookCreatorServiceTest.javadev.docker-compose.ymlfrontend/src/app/features/book/service/book.service.ts
💤 Files with no reviewable changes (1)
- booklore-api/src/main/java/org/booklore/service/metadata/BookMetadataService.java
✅ Files skipped from review due to trivial changes (2)
- booklore-api/src/main/java/org/booklore/config/security/oidc/OidcStateService.java
- booklore-api/src/main/java/org/booklore/config/security/service/LoginRateLimitService.java
🚧 Files skipped from review as they are similar to previous changes (6)
- booklore-api/build.gradle.kts
- booklore-api/src/main/java/org/booklore/controller/BookController.java
- booklore-api/src/main/java/org/booklore/model/entity/BookLoreUserEntity.java
- booklore-api/src/main/java/org/booklore/app/service/AppBookService.java
- booklore-api/src/test/java/org/booklore/service/book/BookCreatorServiceTest.java
- booklore-api/src/main/resources/application.yaml
📜 Review details
🧰 Additional context used
📓 Path-based instructions (3)
booklore-api/src/**/*.java
📄 CodeRabbit inference engine (AGENTS.md)
booklore-api/src/**/*.java: Use 4-space indentation in Java files and match surrounding Java style
Prefer constructor injection via Lombok patterns already used in the codebase. Do not introduce@Autowiredfield injection
Use MapStruct for entity/DTO mapping
Files:
booklore-api/src/main/java/org/booklore/service/IconService.javabooklore-api/src/main/java/org/booklore/util/koreader/EpubCfiService.javabooklore-api/src/main/java/org/booklore/config/security/oidc/BackchannelLogoutService.javabooklore-api/src/main/java/org/booklore/config/security/service/AuthRateLimitService.javabooklore-api/src/main/java/org/booklore/mapper/v2/BookMapperV2.javabooklore-api/src/main/java/org/booklore/repository/UserBookProgressRepository.javabooklore-api/src/main/java/org/booklore/service/book/BookService.javabooklore-api/src/main/java/org/booklore/service/book/BookQueryService.java
frontend/src/**/*.{ts,tsx,html,scss}
📄 CodeRabbit inference engine (AGENTS.md)
Use 2-space indentation in TypeScript, HTML, and SCSS files
Files:
frontend/src/app/features/book/service/book.service.ts
frontend/src/app/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
frontend/src/app/**/*.{ts,tsx}: Preferinject()over constructor injection
Followfrontend/eslint.config.js: component selectors useapp-*, directive selectors useapp*, andanyis disallowed
Files:
frontend/src/app/features/book/service/book.service.ts
🧠 Learnings (3)
📚 Learning: 2026-03-26T01:46:48.863Z
Learnt from: CR
Repo: grimmory-tools/grimmory PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-26T01:46:48.863Z
Learning: Applies to booklore-api/src/**/*.java : Use MapStruct for entity/DTO mapping
Applied to files:
booklore-api/src/main/java/org/booklore/mapper/v2/BookMapperV2.javabooklore-api/src/main/java/org/booklore/service/book/BookService.javabooklore-api/src/main/java/org/booklore/service/book/BookQueryService.java
📚 Learning: 2026-03-24T18:58:08.199Z
Learnt from: pedronave
Repo: grimmory-tools/grimmory PR: 173
File: booklore-api/src/main/java/org/booklore/service/hardcover/HardcoverSyncService.java:291-294
Timestamp: 2026-03-24T18:58:08.199Z
Learning: In grimmory-tools/grimmory (HardcoverSyncService.java), when a Hardcover edition has no page count, falling back to the book-level page count (book.pages) is the preferred behavior. Skipping the sync entirely in this case is considered worse than using a slightly approximate total, because the page count is only a proxy for converting progress % to page numbers for the API. The Hardcover API allows setting page-based progress even when the edition lacks a page count, and progress displays in at least some UI views (e.g., the book detail dropdown).
Applied to files:
booklore-api/src/main/java/org/booklore/service/book/BookService.javabooklore-api/src/main/java/org/booklore/service/book/BookQueryService.java
📚 Learning: 2026-03-26T01:46:48.863Z
Learnt from: CR
Repo: grimmory-tools/grimmory PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-26T01:46:48.863Z
Learning: Applies to booklore-api/src/**/*.java : Prefer constructor injection via Lombok patterns already used in the codebase. Do not introduce `Autowired` field injection
Applied to files:
booklore-api/src/main/java/org/booklore/service/book/BookService.java
🔇 Additional comments (9)
booklore-api/src/main/java/org/booklore/service/IconService.java (1)
41-41: Looks good — cache size reduction is consistent with current eviction flow.Line 41 integrates cleanly with
updateCache(...)and does not introduce a functional regression.dev.docker-compose.yml (1)
20-20:JAVA_TOOL_OPTIONSparity is preserved.This override includes the full JVM flag set expected by the runtime configuration, including the newly added tuning flags.
Dockerfile (1)
71-78: Runtime JVM tuning additions look good.The extended
JAVA_TOOL_OPTIONSblock is coherent and keeps-XX:+ExitOnOutOfMemoryErrorwhile adding the intended memory/compiler/Shenandoah tuning parameters.booklore-api/src/main/java/org/booklore/service/book/BookService.java (4)
50-51: Class-level transaction still holds connections during file I/O.This concern was raised in a previous review and remains unaddressed. Methods like
downloadBook,downloadAllBookFiles,getBookContent, andstreamBookContentperform long-running file/HTTP I/O operations but will now hold a Hibernate session and DB connection for the entire transfer duration.Consider moving
@Transactional(readOnly = true)to individual database-access methods instead, leaving file-serving methods non-transactional.
30-31: LGTM!Standard Spring Data imports for pagination support.
105-134: LGTM! Authorization checks are correctly implemented.The method properly:
- Authenticates the user via
authenticationService.getAuthenticatedUser()- Restricts non-admin users to only their assigned libraries
- Enriches books with user-specific progress data
- Filters shelves to the current user
The pattern mirrors the existing
getBookDTOsmethod appropriately.
260-263: LGTM!Correctly overrides the class-level
readOnly = truefor this write operation.booklore-api/src/main/java/org/booklore/repository/UserBookProgressRepository.java (1)
9-18: No issues in the added imports.Added
PageableandCollectionare consistent with the new repository method signatures.booklore-api/src/main/java/org/booklore/mapper/v2/BookMapperV2.java (1)
31-37: Keep the description inclusion rule in one place.Leaving
descriptionout ofmapMetadata(...)and lettingBookQueryService.mapBookToDto(...)opt back in only when requested keeps that policy centralized instead of splitting it across two mappings.
booklore-api/src/main/java/org/booklore/config/security/oidc/BackchannelLogoutService.java
Show resolved
Hide resolved
booklore-api/src/main/java/org/booklore/config/security/service/AuthRateLimitService.java
Show resolved
Hide resolved
| SELECT DISTINCT ubp.book.id FROM UserBookProgressEntity ubp | ||
| JOIN ubp.book b | ||
| JOIN b.bookFiles bf | ||
| WHERE ubp.user.id = :userId | ||
| AND ubp.readStatus IN (org.booklore.model.enums.ReadStatus.READING, org.booklore.model.enums.ReadStatus.RE_READING) | ||
| AND (b.deleted IS NULL OR b.deleted = false) | ||
| AND bf.isBookFormat = true | ||
| AND bf.bookType <> org.booklore.model.enums.BookFileType.AUDIOBOOK | ||
| AND b.library.id IN :libraryIds | ||
| AND ubp.lastReadTime IS NOT NULL | ||
| ORDER BY ubp.lastReadTime DESC | ||
| """) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In PostgreSQL, is SELECT DISTINCT a FROM ... ORDER BY bvalid whenb is not in the select list?
💡 Result:
No. In PostgreSQL, SELECT DISTINCT a ... ORDER BY b is not valid if b is not part of the SELECT DISTINCT target list; PostgreSQL raises:
ERROR: for SELECT DISTINCT, ORDER BY expressions must appear in select list[1]
Reason: after DISTINCT there may be multiple possible b values for the same a, so ordering the distinct a rows by b is not well-defined [1].
(Without DISTINCT, PostgreSQL does allow ORDER BY on columns not in the output list [2]. If you need “pick one row per a ordered by b”, use DISTINCT ON (a) ... ORDER BY a, b (leftmost ORDER BY must match DISTINCT ON) [2].)
Sources: [1], [2]
🏁 Script executed:
find . -name "UserBookProgressRepository.java" -type fRepository: grimmory-tools/grimmory
Length of output: 151
🏁 Script executed:
wc -l ./booklore-api/src/main/java/org/booklore/repository/UserBookProgressRepository.javaRepository: grimmory-tools/grimmory
Length of output: 155
🏁 Script executed:
sed -n '205,250p' ./booklore-api/src/main/java/org/booklore/repository/UserBookProgressRepository.javaRepository: grimmory-tools/grimmory
Length of output: 2066
Refactor SELECT DISTINCT with ORDER BY non-selected column to use EXISTS subquery.
Both findTopContinueReadingBookIds and findTopContinueListeningBookIds use SELECT DISTINCT ubp.book.id ... ORDER BY ubp.lastReadTime DESC where ubp.lastReadTime is not in the select list. PostgreSQL explicitly forbids this pattern—it requires all ORDER BY expressions to appear in the SELECT list when using DISTINCT. Additionally, the JOIN b.bookFiles bf creates a many-to-one relationship requiring DISTINCT to deduplicate, which is inefficient.
Replace the join with an EXISTS subquery on bookFiles to avoid the cartesian product and remove the need for DISTINCT:
Refactor sketch (apply to both methods)
- `@Query`("""
- SELECT DISTINCT ubp.book.id FROM UserBookProgressEntity ubp
- JOIN ubp.book b
- JOIN b.bookFiles bf
+ `@Query`("""
+ SELECT ubp.book.id FROM UserBookProgressEntity ubp
+ JOIN ubp.book b
WHERE ubp.user.id = :userId
AND ubp.readStatus IN (org.booklore.model.enums.ReadStatus.READING, org.booklore.model.enums.ReadStatus.RE_READING)
AND (b.deleted IS NULL OR b.deleted = false)
- AND bf.isBookFormat = true
- AND bf.bookType <> org.booklore.model.enums.BookFileType.AUDIOBOOK
+ AND EXISTS (
+ SELECT 1 FROM b.bookFiles bf
+ WHERE bf.isBookFormat = true
+ AND bf.bookType <> org.booklore.model.enums.BookFileType.AUDIOBOOK
+ )
AND b.library.id IN :libraryIds
AND ubp.lastReadTime IS NOT NULL
ORDER BY ubp.lastReadTime DESC
""")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| SELECT DISTINCT ubp.book.id FROM UserBookProgressEntity ubp | |
| JOIN ubp.book b | |
| JOIN b.bookFiles bf | |
| WHERE ubp.user.id = :userId | |
| AND ubp.readStatus IN (org.booklore.model.enums.ReadStatus.READING, org.booklore.model.enums.ReadStatus.RE_READING) | |
| AND (b.deleted IS NULL OR b.deleted = false) | |
| AND bf.isBookFormat = true | |
| AND bf.bookType <> org.booklore.model.enums.BookFileType.AUDIOBOOK | |
| AND b.library.id IN :libraryIds | |
| AND ubp.lastReadTime IS NOT NULL | |
| ORDER BY ubp.lastReadTime DESC | |
| """) | |
| SELECT ubp.book.id FROM UserBookProgressEntity ubp | |
| JOIN ubp.book b | |
| WHERE ubp.user.id = :userId | |
| AND ubp.readStatus IN (org.booklore.model.enums.ReadStatus.READING, org.booklore.model.enums.ReadStatus.RE_READING) | |
| AND (b.deleted IS NULL OR b.deleted = false) | |
| AND EXISTS ( | |
| SELECT 1 FROM b.bookFiles bf | |
| WHERE bf.isBookFormat = true | |
| AND bf.bookType <> org.booklore.model.enums.BookFileType.AUDIOBOOK | |
| ) | |
| AND b.library.id IN :libraryIds | |
| AND ubp.lastReadTime IS NOT NULL | |
| ORDER BY ubp.lastReadTime DESC | |
| """) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@booklore-api/src/main/java/org/booklore/repository/UserBookProgressRepository.java`
around lines 212 - 223, The query in findTopContinueReadingBookIds and
findTopContinueListeningBookIds uses SELECT DISTINCT with ORDER BY
ubp.lastReadTime while joining b.bookFiles, which causes PostgreSQL errors and
inefficiency; replace the JOIN b.bookFiles bf + DISTINCT with an EXISTS subquery
that checks for a matching BookFile (e.g., EXISTS (SELECT 1 FROM BookFileEntity
bf WHERE bf.book = b AND bf.isBookFormat = true AND bf.bookType <>
org.booklore.model.enums.BookFileType.AUDIOBOOK)), remove DISTINCT from the
select, keep SELECT ubp.book.id and the ORDER BY ubp.lastReadTime DESC, and
preserve the other filters (ubp.user.id, b.deleted, b.library.id IN :libraryIds,
ubp.lastReadTime IS NOT NULL) so the result is deduplicated correctly and orders
by lastReadTime.
There was a problem hiding this comment.
We're currently using MySQL (maria). is this advice still true?
There was a problem hiding this comment.
@imajes, great question! The answer is nuanced for MariaDB specifically:
MySQL (5.7+, 8.0+): Yes, the advice fully applies. MySQL raises error 3065 with message "Expression #1 of ORDER BY clause is not in SELECT list" when ONLY_FULL_GROUP_BY or DISTINCT is on and a query orders by a column that is not returned in the SELECT list. When DISTINCT removes duplicates, MySQL must sort by the exact same columns; any additional column in ORDER BY throws the error.
MariaDB: MariaDB appears to be more permissive than MySQL here. One real-world report noted "I have another test environment with MariaDB, didn't have to use any special sql_mode" when encountering this exact error on MySQL. So MariaDB may not raise the error 3065 outright.
However, even on MariaDB, there are two remaining concerns:
-
Semantic correctness / non-deterministic ordering:
SELECT DISTINCT ubp.book.idcombined withORDER BY ubp.lastReadTime DESCis logically ambiguous. A singlebook.idcan correspond to multiple rows with differentlastReadTimevalues (from theJOIN b.bookFiles bf). The database will pick an arbitrarylastReadTimeto sort by — meaning the ordering of results is unpredictable. -
Performance: The
JOIN b.bookFiles bfmultiplies rows (one per book file), requiringDISTINCTto deduplicate them. This is less efficient than anEXISTSsubquery, which stops at the first matching file.
So while MariaDB may not error, the query can still return incorrectly ordered results. The EXISTS refactor is still the right fix — it makes the intent explicit and avoids both concerns:
`@Query`("""
SELECT ubp.book.id FROM UserBookProgressEntity ubp
JOIN ubp.book b
WHERE ubp.user.id = :userId
AND ubp.readStatus IN (org.booklore.model.enums.ReadStatus.READING, org.booklore.model.enums.ReadStatus.RE_READING)
AND (b.deleted IS NULL OR b.deleted = false)
AND EXISTS (
SELECT 1 FROM b.bookFiles bf
WHERE bf.isBookFormat = true
AND bf.bookType <> org.booklore.model.enums.BookFileType.AUDIOBOOK
)
AND b.library.id IN :libraryIds
AND ubp.lastReadTime IS NOT NULL
ORDER BY ubp.lastReadTime DESC
""")Without DISTINCT, ubp.book.id is unique per UserBookProgressEntity row (one progress row per user+book), so no deduplication is needed and ORDER BY ubp.lastReadTime is unambiguous.
There was a problem hiding this comment.
The suggested fix is a barely readable query, and this is a... selfhosted book app. The amount performance overhead this creates is negligible. The performance concern is negligible at this scale. The ordering concern doesn't apply here because lastReadTime is on ubp, not derived from bf, the JOIN-duplicated rows have the same value, so ordering is deterministic in practice
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| AND b.library.id IN :libraryIds | ||
| AND ubp.lastReadTime IS NOT NULL | ||
| ORDER BY ubp.lastReadTime DESC | ||
| """) | ||
| List<Long> findTopContinueReadingBookIds( | ||
| @Param("userId") Long userId, | ||
| @Param("libraryIds") Collection<Long> libraryIds, | ||
| Pageable pageable); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Verify call sites and arguments passed to repository methods:"
rg -nP --type=java -C3 '\bfindTopContinue(Read|Listen)ingBookIds\s*\('
echo
echo "Verify null/empty branches in getAccessibleLibraryIds:"
rg -nP --type=java -C8 '\bgetAccessibleLibraryIds\s*\('Repository: grimmory-tools/grimmory
Length of output: 37588
🏁 Script executed:
cat -n booklore-api/src/main/java/org/booklore/repository/UserBookProgressRepository.java | sed -n '210,260p'Repository: grimmory-tools/grimmory
Length of output: 2134
🏁 Script executed:
rg -nA5 'inLibraries.*accessibleLibraryIds' booklore-api/src/main/java/org/booklore/app/service/AppBookService.javaRepository: grimmory-tools/grimmory
Length of output: 1661
🏁 Script executed:
rg -nA10 'static.*inLibraries' booklore-api/src/main/java/org/booklore/app/service/AppBookSpecification.javaRepository: grimmory-tools/grimmory
Length of output: 184
🏁 Script executed:
find booklore-api -name '*AppBookSpecification*' -o -name '*BookSpecification*'Repository: grimmory-tools/grimmory
Length of output: 150
🏁 Script executed:
rg -nA10 'inLibraries' booklore-api/src/main/java/org/booklore/app/specification/AppBookSpecification.javaRepository: grimmory-tools/grimmory
Length of output: 534
🏁 Script executed:
cat -n booklore-api/src/main/java/org/booklore/app/specification/AppBookSpecification.java | head -30Repository: grimmory-tools/grimmory
Length of output: 1202
Guard libraryIds against null/empty before passing to findTopContinueReadingBookIds and findTopContinueListeningBookIds.
Both repository methods at lines 220 and 241 use IN :libraryIds in their JPQL queries without handling null (admin users) or empty collections (users with no assigned libraries). AppBookService.getAccessibleLibraryIds() returns null or Collections.emptySet(), which will fail at bind/execute time. Other queries in the service safely guard this with AppBookSpecification.inLibraries(), which returns a no-op condition for null/empty—apply the same pattern here by checking before the repository call.
Affects: AppBookService.getContinueReading() (line 148) and AppBookService.getContinueListening() (line 171).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@booklore-api/src/main/java/org/booklore/repository/UserBookProgressRepository.java`
around lines 220 - 227, The JPQL IN :libraryIds will fail for null/empty; update
handling so getContinueReading()/getContinueListening() guard libraryIds from
AppBookService.getAccessibleLibraryIds(): if libraryIds is empty return an empty
result immediately, and if libraryIds is null allow "no filter" by changing the
repository queries (findTopContinueReadingBookIds and
findTopContinueListeningBookIds) to use a null-safe predicate like "AND
(:libraryIds IS NULL OR b.library.id IN :libraryIds)" so null means no library
restriction while empty collection still yields no results.
| @Transactional | ||
| public void saveRecommendationsInBatches(Map<Long, Set<BookRecommendationLite>> recommendations, int batchSize) { | ||
| List<Long> bookIds = new ArrayList<>(recommendations.keySet()); | ||
| for (int i = 0; i < bookIds.size(); i += batchSize) { | ||
| List<Long> batchIds = bookIds.subList(i, Math.min(i + batchSize, bookIds.size())); |
There was a problem hiding this comment.
Reject non-positive batch sizes before entering the loop.
Line 105 never advances when batchSize <= 0, so a bad caller can hang the recommendation update loop. Validate the argument up front.
🛠️ Proposed fix
`@Transactional`
public void saveRecommendationsInBatches(Map<Long, Set<BookRecommendationLite>> recommendations, int batchSize) {
+ if (batchSize <= 0) {
+ throw new IllegalArgumentException("batchSize must be greater than 0");
+ }
List<Long> bookIds = new ArrayList<>(recommendations.keySet());
for (int i = 0; i < bookIds.size(); i += batchSize) {
List<Long> batchIds = bookIds.subList(i, Math.min(i + batchSize, bookIds.size()));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@booklore-api/src/main/java/org/booklore/service/book/BookQueryService.java`
around lines 102 - 106, Reject non-positive batchSize at the start of
saveRecommendationsInBatches by validating the batchSize parameter (and
optionally recommendations) before entering the loop; if batchSize <= 0, throw
an IllegalArgumentException with a clear message so the for-loop that uses
batchSize can never hang or misbehave.
| queryKey: BOOKS_QUERY_KEY, | ||
| queryFn: () => lastValueFrom(this.http.get<Book[]>(this.url, {params: {stripForListView: false}})) | ||
| queryFn: () => lastValueFrom(this.http.get<Book[]>(this.url, {params: {stripForListView: false}})), | ||
| staleTime: 30_000, |
There was a problem hiding this comment.
Line 105 creates a 30s stale-data window unless list invalidation is added after shelf mutations.
With staleTime: 30_000, BOOKS_QUERY_KEY stays fresh, but updateBookShelves() currently patches cache without invalidating the list query. If backend applies additional changes beyond patched fields, users can see stale list data for up to 30 seconds.
Suggested fix (outside this line range, in BookService.updateBookShelves)
updateBookShelves(bookIds: Set<number | undefined>, shelvesToAssign: Set<number | null | undefined>, shelvesToUnassign: Set<number | null | undefined>): Observable<Book[]> {
- return this.bookPatchService.updateBookShelves(bookIds, shelvesToAssign, shelvesToUnassign);
+ return this.bookPatchService.updateBookShelves(bookIds, shelvesToAssign, shelvesToUnassign).pipe(
+ tap(() => invalidateBooksQuery(this.queryClient))
+ );
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/app/features/book/service/book.service.ts` at line 105, The
staleTime of 30_000 combined with the current cache patch in updateBookShelves
risks showing stale list entries; update updateBookShelves to invalidate or
refetch the BOOKS_QUERY_KEY list after the mutation completes (or on mutation
success) instead of relying solely on the patch — e.g., perform your optimistic
cache patch as now but call queryClient.invalidateQueries(BOOKS_QUERY_KEY) (or
trigger a refetch) on success so backend-side changes beyond the patched fields
are reflected immediately.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
booklore-api/src/main/java/org/booklore/repository/UserBookProgressRepository.java (1)
212-244:⚠️ Potential issue | 🟠 MajorRefactor both “top continue” queries to avoid
DISTINCToverJOIN b.bookFiles.Line 212 and Line 233 still rely on
SELECT DISTINCT ubp.book.id ... ORDER BY ubp.lastReadTime DESCafter joiningb.bookFiles, which remains a fragile pattern and was already flagged earlier.Suggested JPQL rewrite (apply to both methods)
- SELECT DISTINCT ubp.book.id FROM UserBookProgressEntity ubp + SELECT ubp.book.id FROM UserBookProgressEntity ubp JOIN ubp.book b - JOIN b.bookFiles bf WHERE ubp.user.id = :userId AND ubp.readStatus IN (org.booklore.model.enums.ReadStatus.READING, org.booklore.model.enums.ReadStatus.RE_READING) AND (b.deleted IS NULL OR b.deleted = false) - AND bf.isBookFormat = true - AND bf.bookType <> org.booklore.model.enums.BookFileType.AUDIOBOOK + AND EXISTS ( + SELECT 1 FROM b.bookFiles bf + WHERE bf.isBookFormat = true + AND bf.bookType <> org.booklore.model.enums.BookFileType.AUDIOBOOK + ) AND b.library.id IN :libraryIds AND ubp.lastReadTime IS NOT NULL ORDER BY ubp.lastReadTime DESC- SELECT DISTINCT ubp.book.id FROM UserBookProgressEntity ubp + SELECT ubp.book.id FROM UserBookProgressEntity ubp JOIN ubp.book b - JOIN b.bookFiles bf WHERE ubp.user.id = :userId AND ubp.readStatus IN (org.booklore.model.enums.ReadStatus.READING, org.booklore.model.enums.ReadStatus.RE_READING) AND (b.deleted IS NULL OR b.deleted = false) - AND bf.isBookFormat = true - AND bf.bookType = org.booklore.model.enums.BookFileType.AUDIOBOOK + AND EXISTS ( + SELECT 1 FROM b.bookFiles bf + WHERE bf.isBookFormat = true + AND bf.bookType = org.booklore.model.enums.BookFileType.AUDIOBOOK + ) AND b.library.id IN :libraryIds AND ubp.lastReadTime IS NOT NULL ORDER BY ubp.lastReadTime DESCIn current MariaDB and MySQL versions, is `SELECT DISTINCT a ... ORDER BY b` valid when `b` is not part of the select list?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@booklore-api/src/main/java/org/booklore/repository/UserBookProgressRepository.java` around lines 212 - 244, Both continue-reading queries (the `@Query` for findTopContinueReadingBookIds and the similar audiobook query) currently join b.bookFiles and use SELECT DISTINCT ubp.book.id which causes duplicate/ordering issues; replace the JOIN with either (a) a GROUP BY ubp.book.id and ORDER BY MAX(ubp.lastReadTime) DESC so duplicates from bookFiles are collapsed, or (b) remove the JOIN entirely and use an EXISTS subquery against BookFile (e.g. WHERE EXISTS (SELECT bf FROM BookFileEntity bf WHERE bf.book = b AND bf.isBookFormat = true AND bf.bookType [= or <>] org.booklore.model.enums.BookFileType.AUDIOBOOK)), keeping the same predicates for readStatus, libraryIds and lastReadTime; apply the same rewrite to both the findTopContinueReadingBookIds query and the audiobook variant so ORDER BY uses the aggregated/max lastReadTime or is unaffected by multiple bookFiles.booklore-api/src/main/java/org/booklore/app/service/AppBookService.java (1)
148-149:⚠️ Potential issue | 🟠 MajorGuard library scope before invoking repository methods with
IN :libraryIds.Line 148 and Line 171 pass
accessibleLibraryIdsdirectly, but this value can benull(admin) or empty (no assigned libraries). That creates a runtime risk for the repositoryIN :libraryIdsqueries.Service-side branching sketch
int maxItems = validateLimit(limit, 10); + if (accessibleLibraryIds != null && accessibleLibraryIds.isEmpty()) { + return Collections.emptyList(); + } - List<Long> topIds = userBookProgressRepository.findTopContinueReadingBookIds( - userId, accessibleLibraryIds, PageRequest.of(0, maxItems)); + Pageable page = PageRequest.of(0, maxItems); + List<Long> topIds = (accessibleLibraryIds == null) + ? userBookProgressRepository.findTopContinueReadingBookIdsAllLibraries(userId, page) + : userBookProgressRepository.findTopContinueReadingBookIds(userId, accessibleLibraryIds, page);int maxItems = validateLimit(limit, 10); + if (accessibleLibraryIds != null && accessibleLibraryIds.isEmpty()) { + return Collections.emptyList(); + } - List<Long> topIds = userBookProgressRepository.findTopContinueListeningBookIds( - userId, accessibleLibraryIds, PageRequest.of(0, maxItems)); + Pageable page = PageRequest.of(0, maxItems); + List<Long> topIds = (accessibleLibraryIds == null) + ? userBookProgressRepository.findTopContinueListeningBookIdsAllLibraries(userId, page) + : userBookProgressRepository.findTopContinueListeningBookIds(userId, accessibleLibraryIds, page);#!/bin/bash set -euo pipefail # Verify null/empty contract in service scope resolution: rg -nP --type=java 'private Set<Long> getAccessibleLibraryIds|return null;|Collections\.emptySet\(\)' \ booklore-api/src/main/java/org/booklore/app/service/AppBookService.java # Verify repository callsites that pass the scope directly: rg -nP --type=java 'findTopContinue(Read|Listen)ingBookIds\s*\(' \ booklore-api/src/main/java/org/booklore/app/service/AppBookService.java # Verify repository queries still require IN :libraryIds: rg -nP --type=java 'b\.library\.id IN :libraryIds' \ booklore-api/src/main/java/org/booklore/repository/UserBookProgressRepository.javaAlso applies to: 171-172
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@booklore-api/src/main/java/org/booklore/app/service/AppBookService.java` around lines 148 - 149, The call to userBookProgressRepository.findTopContinueReadingBookIds (and the similar findTopContinueListeningBookIds) passes accessibleLibraryIds directly which can be null (admin) or empty (no libs) and will break the "IN :libraryIds" JPQL; before invoking these repository methods check accessibleLibraryIds: if accessibleLibraryIds is empty return Collections.emptyList() (or an empty result) immediately, and if accessibleLibraryIds is null use the repository variant that does not filter by library (or add a dedicated call like findTopContinueReadingBookIdsForAdmin(userId, PageRequest)) ; update the code paths around topIds (and the analogous listeningIds call) to branch accordingly so you never invoke the IN :libraryIds query with a null/empty collection.booklore-api/src/main/java/org/booklore/service/book/BookQueryService.java (2)
102-106:⚠️ Potential issue | 🟠 MajorReject non-positive
batchSizebefore entering the loop.Line 105 never advances when
batchSize == 0, and negative values make thesubListbounds invalid. Guard it upfront so a bad config can't hang this job.🛠️ Proposed fix
`@Transactional` public void saveRecommendationsInBatches(Map<Long, Set<BookRecommendationLite>> recommendations, int batchSize) { + if (batchSize <= 0) { + throw new IllegalArgumentException("batchSize must be greater than 0"); + } List<Long> bookIds = new ArrayList<>(recommendations.keySet()); for (int i = 0; i < bookIds.size(); i += batchSize) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@booklore-api/src/main/java/org/booklore/service/book/BookQueryService.java` around lines 102 - 106, In saveRecommendationsInBatches(Map<Long, Set<BookRecommendationLite>> recommendations, int batchSize) validate batchSize before the loop (e.g., if batchSize <= 0) and fail fast by throwing an IllegalArgumentException with a clear message (or returning early if that matches project conventions); this prevents an infinite loop when batchSize == 0 and invalid subList bounds for negative sizes — add the check at the top of the method before creating bookIds and entering the for-loop.
47-53:⚠️ Potential issue | 🟠 Major
totalElementsis still based on the unfiltered page.Lines 49-53 apply
contentRestrictionServiceafter the repository page is sliced, but Line 53 keepspage.getTotalElements(). If restrictions drop any books, clients will get short/empty pages with totals that still count hidden rows. This needs query-side restriction or a recomputed total.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@booklore-api/src/main/java/org/booklore/service/book/BookQueryService.java` around lines 47 - 53, getAllBooksByLibraryIdsPaged uses bookRepository.findAllWithMetadataByLibraryIdsPage then filters the page content via contentRestrictionService.applyRestrictions, but still constructs the PageImpl with page.getTotalElements() (which counts unfiltered rows). Fix by ensuring the total reflects the filtered result: either push the restriction into the query (update findAllWithMetadataByLibraryIdsPage to apply the same contentRestriction predicates and return a correct count) or, if immediate fix is required in getAllBooksByLibraryIdsPaged, compute a correct total from the restriction (e.g., run a restricted-count query or recompute totalElements from the restricted set when you can safely do so) and pass that recomputed total into the PageImpl instead of page.getTotalElements(); reference methods: getAllBooksByLibraryIdsPaged, bookRepository.findAllWithMetadataByLibraryIdsPage, contentRestrictionService.applyRestrictions, and PageImpl constructor.
🧹 Nitpick comments (1)
booklore-api/build.gradle.kts (1)
143-143: Prefer a configurable test heap instead of a fixed2g.A hard-coded value can be brittle on smaller CI/local runners. Making it property-driven keeps this optimization while improving portability.
♻️ Suggested change
tasks.named<Test>("test") { useJUnitPlatform() - maxHeapSize = "2g" + maxHeapSize = providers.gradleProperty("testMaxHeap") + .orElse("2g") + .get() jvmArgs("-XX:+EnableDynamicAgentLoading") finalizedBy(tasks.named("jacocoTestReport")) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@booklore-api/build.gradle.kts` at line 143, Replace the hard-coded test JVM heap ("maxHeapSize = \"2g\"") with a property-driven value so CI/local runners can override it; read a Gradle/project property (e.g. testHeapSize or via system property) with a sensible default like "2g" and assign that value to maxHeapSize in the test task configuration (referencing the existing maxHeapSize setting) so callers can set -PtestHeapSize or an env-driven property to adjust the heap without editing the build file.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@booklore-api/src/main/java/org/booklore/config/security/service/LoginRateLimitService.java`:
- Around line 23-29: LoginRateLimitService is dead code (annotated `@Service` but
never used); either remove it or integrate it—if you want to remove it, delete
the LoginRateLimitService class and references (constructor AuditService field,
attemptCache) and any unused imports to reduce clutter; if you intend to
keep/use it, wire it into the existing authentication flow (replace or add
alongside AuthRateLimitService in AuthenticationService where rate limiting is
applied), and update the cache configuration in LoginRateLimitService (increase
maximumSize from 1000 to ~10000 or swap attemptCache for an external store like
Redis) and ensure AuditService is correctly invoked where attempts are recorded.
In `@booklore-api/src/main/java/org/booklore/controller/BookController.java`:
- Around line 79-83: The OpenAPI doc currently hides pagination params by
annotating the Pageable parameter with `@Parameter`(hidden = true) in the
getBooksPaged method; replace that with the springdoc-supported annotation
`@ParameterObject` on the Pageable parameter so page/size/sort are exposed in the
generated contract (update the import to
org.springdoc.core.annotations.ParameterObject if missing and remove the
`@Parameter`(hidden = true) usage). Ensure the signature of getBooksPaged(Pageable
pageable) keeps the same parameter name and type but uses `@ParameterObject`
Pageable pageable so documentation shows page, size and sort.
---
Duplicate comments:
In `@booklore-api/src/main/java/org/booklore/app/service/AppBookService.java`:
- Around line 148-149: The call to
userBookProgressRepository.findTopContinueReadingBookIds (and the similar
findTopContinueListeningBookIds) passes accessibleLibraryIds directly which can
be null (admin) or empty (no libs) and will break the "IN :libraryIds" JPQL;
before invoking these repository methods check accessibleLibraryIds: if
accessibleLibraryIds is empty return Collections.emptyList() (or an empty
result) immediately, and if accessibleLibraryIds is null use the repository
variant that does not filter by library (or add a dedicated call like
findTopContinueReadingBookIdsForAdmin(userId, PageRequest)) ; update the code
paths around topIds (and the analogous listeningIds call) to branch accordingly
so you never invoke the IN :libraryIds query with a null/empty collection.
In
`@booklore-api/src/main/java/org/booklore/repository/UserBookProgressRepository.java`:
- Around line 212-244: Both continue-reading queries (the `@Query` for
findTopContinueReadingBookIds and the similar audiobook query) currently join
b.bookFiles and use SELECT DISTINCT ubp.book.id which causes duplicate/ordering
issues; replace the JOIN with either (a) a GROUP BY ubp.book.id and ORDER BY
MAX(ubp.lastReadTime) DESC so duplicates from bookFiles are collapsed, or (b)
remove the JOIN entirely and use an EXISTS subquery against BookFile (e.g. WHERE
EXISTS (SELECT bf FROM BookFileEntity bf WHERE bf.book = b AND bf.isBookFormat =
true AND bf.bookType [= or <>]
org.booklore.model.enums.BookFileType.AUDIOBOOK)), keeping the same predicates
for readStatus, libraryIds and lastReadTime; apply the same rewrite to both the
findTopContinueReadingBookIds query and the audiobook variant so ORDER BY uses
the aggregated/max lastReadTime or is unaffected by multiple bookFiles.
In `@booklore-api/src/main/java/org/booklore/service/book/BookQueryService.java`:
- Around line 102-106: In saveRecommendationsInBatches(Map<Long,
Set<BookRecommendationLite>> recommendations, int batchSize) validate batchSize
before the loop (e.g., if batchSize <= 0) and fail fast by throwing an
IllegalArgumentException with a clear message (or returning early if that
matches project conventions); this prevents an infinite loop when batchSize == 0
and invalid subList bounds for negative sizes — add the check at the top of the
method before creating bookIds and entering the for-loop.
- Around line 47-53: getAllBooksByLibraryIdsPaged uses
bookRepository.findAllWithMetadataByLibraryIdsPage then filters the page content
via contentRestrictionService.applyRestrictions, but still constructs the
PageImpl with page.getTotalElements() (which counts unfiltered rows). Fix by
ensuring the total reflects the filtered result: either push the restriction
into the query (update findAllWithMetadataByLibraryIdsPage to apply the same
contentRestriction predicates and return a correct count) or, if immediate fix
is required in getAllBooksByLibraryIdsPaged, compute a correct total from the
restriction (e.g., run a restricted-count query or recompute totalElements from
the restricted set when you can safely do so) and pass that recomputed total
into the PageImpl instead of page.getTotalElements(); reference methods:
getAllBooksByLibraryIdsPaged,
bookRepository.findAllWithMetadataByLibraryIdsPage,
contentRestrictionService.applyRestrictions, and PageImpl constructor.
---
Nitpick comments:
In `@booklore-api/build.gradle.kts`:
- Line 143: Replace the hard-coded test JVM heap ("maxHeapSize = \"2g\"") with a
property-driven value so CI/local runners can override it; read a Gradle/project
property (e.g. testHeapSize or via system property) with a sensible default like
"2g" and assign that value to maxHeapSize in the test task configuration
(referencing the existing maxHeapSize setting) so callers can set -PtestHeapSize
or an env-driven property to adjust the heap without editing the build file.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8bd5c23d-9ebc-4c4e-9c8f-470e7cca307c
📒 Files selected for processing (20)
Dockerfilebooklore-api/build.gradle.ktsbooklore-api/src/main/java/org/booklore/app/service/AppBookService.javabooklore-api/src/main/java/org/booklore/config/security/oidc/BackchannelLogoutService.javabooklore-api/src/main/java/org/booklore/config/security/oidc/OidcStateService.javabooklore-api/src/main/java/org/booklore/config/security/service/AuthRateLimitService.javabooklore-api/src/main/java/org/booklore/config/security/service/LoginRateLimitService.javabooklore-api/src/main/java/org/booklore/controller/BookController.javabooklore-api/src/main/java/org/booklore/mapper/v2/BookMapperV2.javabooklore-api/src/main/java/org/booklore/model/entity/BookLoreUserEntity.javabooklore-api/src/main/java/org/booklore/repository/UserBookProgressRepository.javabooklore-api/src/main/java/org/booklore/service/IconService.javabooklore-api/src/main/java/org/booklore/service/book/BookQueryService.javabooklore-api/src/main/java/org/booklore/service/book/BookService.javabooklore-api/src/main/java/org/booklore/service/metadata/BookMetadataService.javabooklore-api/src/main/java/org/booklore/util/koreader/EpubCfiService.javabooklore-api/src/main/resources/application.yamlbooklore-api/src/test/java/org/booklore/service/book/BookCreatorServiceTest.javadev.docker-compose.ymlfrontend/src/app/features/book/service/book.service.ts
💤 Files with no reviewable changes (1)
- booklore-api/src/main/java/org/booklore/service/metadata/BookMetadataService.java
✅ Files skipped from review due to trivial changes (3)
- booklore-api/src/main/java/org/booklore/service/IconService.java
- booklore-api/src/main/java/org/booklore/config/security/oidc/OidcStateService.java
- Dockerfile
🚧 Files skipped from review as they are similar to previous changes (2)
- booklore-api/src/test/java/org/booklore/service/book/BookCreatorServiceTest.java
- booklore-api/src/main/java/org/booklore/service/book/BookService.java
📜 Review details
🧰 Additional context used
📓 Path-based instructions (4)
booklore-api/src/**/*.java
📄 CodeRabbit inference engine (AGENTS.md)
booklore-api/src/**/*.java: Use 4-space indentation in Java files and match surrounding Java style
Prefer constructor injection via Lombok patterns already used in the codebase. Do not introduce@Autowiredfield injection
Use MapStruct for entity/DTO mapping
Files:
booklore-api/src/main/java/org/booklore/config/security/service/LoginRateLimitService.javabooklore-api/src/main/java/org/booklore/config/security/service/AuthRateLimitService.javabooklore-api/src/main/java/org/booklore/mapper/v2/BookMapperV2.javabooklore-api/src/main/java/org/booklore/util/koreader/EpubCfiService.javabooklore-api/src/main/java/org/booklore/config/security/oidc/BackchannelLogoutService.javabooklore-api/src/main/java/org/booklore/controller/BookController.javabooklore-api/src/main/java/org/booklore/model/entity/BookLoreUserEntity.javabooklore-api/src/main/java/org/booklore/app/service/AppBookService.javabooklore-api/src/main/java/org/booklore/service/book/BookQueryService.javabooklore-api/src/main/java/org/booklore/repository/UserBookProgressRepository.java
frontend/src/**/*.{ts,tsx,html,scss}
📄 CodeRabbit inference engine (AGENTS.md)
Use 2-space indentation in TypeScript, HTML, and SCSS files
Files:
frontend/src/app/features/book/service/book.service.ts
frontend/src/app/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
frontend/src/app/**/*.{ts,tsx}: Preferinject()over constructor injection
Followfrontend/eslint.config.js: component selectors useapp-*, directive selectors useapp*, andanyis disallowed
Files:
frontend/src/app/features/book/service/book.service.ts
booklore-api/src/**/*Entity.java
📄 CodeRabbit inference engine (AGENTS.md)
Keep JPA entities on the
*Entitysuffix
Files:
booklore-api/src/main/java/org/booklore/model/entity/BookLoreUserEntity.java
🧠 Learnings (9)
📚 Learning: 2026-03-26T01:46:48.863Z
Learnt from: CR
Repo: grimmory-tools/grimmory PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-26T01:46:48.863Z
Learning: Applies to booklore-api/src/**/*.java : Use MapStruct for entity/DTO mapping
Applied to files:
booklore-api/src/main/java/org/booklore/mapper/v2/BookMapperV2.javabooklore-api/src/main/java/org/booklore/model/entity/BookLoreUserEntity.javabooklore-api/src/main/java/org/booklore/app/service/AppBookService.javabooklore-api/src/main/java/org/booklore/service/book/BookQueryService.java
📚 Learning: 2026-03-31T06:22:25.311Z
Learnt from: imnotjames
Repo: grimmory-tools/grimmory PR: 113
File: booklore-api/build.gradle.kts:89-90
Timestamp: 2026-03-31T06:22:25.311Z
Learning: When reviewing build logic or CI checks that call the JitPack build status API, do not treat a response like {"status":"none"} as evidence that a dependency is unavailable or that a build has failed. JitPack typically builds packages on-demand the first time they’re requested (e.g., via Gradle); "none" means the package isn’t pre-built/cached yet. Only raise a build failure concern when the status explicitly indicates an error/failure (e.g., failed/cancelled), or when dependent resolution actually fails.
Applied to files:
booklore-api/build.gradle.kts
📚 Learning: 2026-03-26T01:46:48.863Z
Learnt from: CR
Repo: grimmory-tools/grimmory PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-26T01:46:48.863Z
Learning: Applies to booklore-api/src/**/*Entity.java : Keep JPA entities on the `*Entity` suffix
Applied to files:
booklore-api/src/main/java/org/booklore/model/entity/BookLoreUserEntity.javabooklore-api/src/main/java/org/booklore/app/service/AppBookService.java
📚 Learning: 2026-03-26T01:46:48.863Z
Learnt from: CR
Repo: grimmory-tools/grimmory PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-26T01:46:48.863Z
Learning: Applies to booklore-api/src/**/*.java : Prefer constructor injection via Lombok patterns already used in the codebase. Do not introduce `Autowired` field injection
Applied to files:
booklore-api/src/main/java/org/booklore/model/entity/BookLoreUserEntity.javabooklore-api/src/main/java/org/booklore/app/service/AppBookService.java
📚 Learning: 2026-03-24T18:58:08.199Z
Learnt from: pedronave
Repo: grimmory-tools/grimmory PR: 173
File: booklore-api/src/main/java/org/booklore/service/hardcover/HardcoverSyncService.java:291-294
Timestamp: 2026-03-24T18:58:08.199Z
Learning: In grimmory-tools/grimmory (HardcoverSyncService.java), when a Hardcover edition has no page count, falling back to the book-level page count (book.pages) is the preferred behavior. Skipping the sync entirely in this case is considered worse than using a slightly approximate total, because the page count is only a proxy for converting progress % to page numbers for the API. The Hardcover API allows setting page-based progress even when the edition lacks a page count, and progress displays in at least some UI views (e.g., the book detail dropdown).
Applied to files:
booklore-api/src/main/java/org/booklore/app/service/AppBookService.javabooklore-api/src/main/java/org/booklore/service/book/BookQueryService.java
📚 Learning: 2026-03-25T21:02:57.527Z
Learnt from: imnotjames
Repo: grimmory-tools/grimmory PR: 195
File: booklore-api/src/main/java/org/booklore/config/logging/RequestLoggingFilterConfig.java:15-19
Timestamp: 2026-03-25T21:02:57.527Z
Learning: In `booklore-api/src/main/java/org/booklore/config/logging/RequestLoggingFilterConfig.java` and `booklore-api/src/main/java/org/booklore/config/logging/filter/RequestLoggingFilter.java`, logging all request headers and payloads at DEBUG level is intentional and accepted behavior in this project. The previous `LoggingFilter` also logged all headers. No header redaction or payload scrubbing is required — the DEBUG log level is considered sufficient access control. The new filter is not profile-gated (unlike the old `Profile("dev")` filter) but relies on `logger.isDebugEnabled()` for gating.
Applied to files:
booklore-api/src/main/resources/application.yaml
📚 Learning: 2026-03-24T18:46:47.249Z
Learnt from: pedronave
Repo: grimmory-tools/grimmory PR: 173
File: booklore-api/src/main/java/org/booklore/service/hardcover/HardcoverSyncService.java:188-199
Timestamp: 2026-03-24T18:46:47.249Z
Learning: In grimmory-tools/grimmory (HardcoverSyncService.java), when a `hardcoverBookId` is stored in book metadata but is malformed (non-numeric), the preferred behavior is to return null and skip the sync rather than falling back to ISBN-based lookup. The reasoning is that an explicitly set bookId represents deliberate user intent, and silently resolving via ISBN could sync progress to a different book than intended.
Applied to files:
booklore-api/src/main/java/org/booklore/repository/UserBookProgressRepository.java
📚 Learning: 2026-03-25T19:09:09.638Z
Learnt from: imnotjames
Repo: grimmory-tools/grimmory PR: 189
File: booklore-api/src/main/java/org/booklore/service/kobo/KoboLibrarySyncService.java:113-116
Timestamp: 2026-03-25T19:09:09.638Z
Learning: In `booklore-api/src/main/java/org/booklore/service/kobo/KoboLibrarySyncService.java`, when deciding whether to forward the `Authorization` header to the upstream Kobo server proxy on the `/v1/library/sync` endpoint, a simple null check (`request.getHeader(HttpHeaders.AUTHORIZATION) != null`) is the correct guard. The header is either absent or present — if present it may or may not be a valid token, but validity is determined by the upstream Kobo server, not by Grimmory. Blank/whitespace intermediate states do not occur in practice, and pre-validating the token value before forwarding is not the responsibility of this service.
Applied to files:
booklore-api/src/main/java/org/booklore/repository/UserBookProgressRepository.java
📚 Learning: 2026-03-26T03:22:24.500Z
Learnt from: imnotjames
Repo: grimmory-tools/grimmory PR: 209
File: booklore-api/src/main/java/org/booklore/util/FileUtils.java:32-38
Timestamp: 2026-03-26T03:22:24.500Z
Learning: In `grimmory-tools/grimmory`, `BookFileEntity.fileSubPath` (in `booklore-api/src/main/java/org/booklore/model/entity/BookFileEntity.java`) is declared with `Column(name = "file_sub_path", length = 512, nullable = false)`, meaning it is never null. Do not flag null-guard concerns for this field.
Applied to files:
booklore-api/src/main/java/org/booklore/repository/UserBookProgressRepository.java
🔇 Additional comments (8)
booklore-api/src/main/java/org/booklore/util/koreader/EpubCfiService.java (1)
24-25: Cache tuning update looks consistent and safe in this scope.Reducing size/TTL here is a clear performance-memory tradeoff, and this change is internally consistent with the existing cache lifecycle and invalidation flow.
booklore-api/src/main/java/org/booklore/model/entity/BookLoreUserEntity.java (1)
98-110: Correct Hibernate-safeequals/hashCodeimplementation.The implementation properly addresses proxy handling by using
Hibernate.getClass()for both class comparison and hash code generation. The ID-based equality with null check ensures correct behavior for both transient and persistent entities.booklore-api/build.gradle.kts (1)
136-137: Verify behavior after disabling Hibernate association management.Turning off
enableAssociationManagementremoves enhancer-driven bidirectional synchronization. Please confirm entity add/remove paths now fully maintain both sides manually to avoid inconsistent persistence state.frontend/src/app/features/book/service/book.service.ts (1)
104-105: Re-check cache invalidation with the newstaleTime(already flagged previously).Line 105 introduces a 30s freshness window. Please verify
BookPatchService.updateBookShelves(...)invalidates/refetchesBOOKS_QUERY_KEYon success; otherwise shelf mutations may remain stale until freshness expires.#!/bin/bash set -euo pipefail PATCH_FILE="$(fd -i 'book-patch.service.ts' | head -n1)" BOOK_FILE="$(fd -i 'book.service.ts' | rg 'frontend/src/app/features/book/service/book.service.ts' | head -n1)" echo "Inspecting: ${PATCH_FILE}" rg -n -C3 'class BookPatchService|updateBookShelves\s*\(|invalidateBooksQuery|invalidateQueries|setQueryData|BOOKS_QUERY_KEY' "${PATCH_FILE}" echo echo "Inspecting: ${BOOK_FILE}" rg -n -C3 'getBooksQueryOptions|staleTime|updateBookShelves\s*\(' "${BOOK_FILE}"Expected verification result: a clear invalidation/refetch path for
BOOKS_QUERY_KEYin the shelf-update mutation flow.booklore-api/src/main/java/org/booklore/config/security/service/AuthRateLimitService.java (1)
23-29: Cache size reduction weakens brute-force protection.Reducing
maximumSizefrom 10,000 to 1,000 while tracking both IP-based and username-based keys (2 keys per login attempt) allows distributed attackers to exhaust the cache and evict legitimate rate-limit counters, effectively bypassing brute-force protection.booklore-api/src/main/java/org/booklore/config/security/oidc/BackchannelLogoutService.java (1)
32-35: Reduced replay-protection cache enables potential token replay attacks.Reducing
maximumSizeto 1,000 while the/backchannel-logoutendpoint (perOidcAuthController.java:125-134) lacks rate limiting allows attackers to flood the endpoint with bogus tokens, evict legitimate JTIs, and replay previously-processed logout tokens.dev.docker-compose.yml (1)
20-20: JVM tuning flags look reasonable; Shenandoah GC is available in the Gradle image.The flags mirror the production Dockerfile settings, which is good for dev/prod parity. A few notes:
JAVA_TOOL_OPTIONSapplies to all Java processes in the container, including the Gradle daemon and the Spring Boot app it spawns viabootRun. This is likely the intended behavior but worth noting.The
gradle:9.3.1-jdk25-alpineimage uses Eclipse Temurin JDK 25, which includes Shenandoah GC as a production feature (JEP 521). This differs from the productioneclipse-temurin:25-jre-alpinebase image (JDK vs. JRE), but both support Shenandoah.booklore-api/src/main/java/org/booklore/app/service/AppBookService.java (1)
41-41: Good move: class-level read-only transaction default is clean and consistent.This reduces annotation repetition while keeping explicit write transactions possible on mutating methods.
| public LoginRateLimitService(AuditService auditService) { | ||
| this.auditService = auditService; | ||
| this.attemptCache = Caffeine.newBuilder() | ||
| .maximumSize(1000) | ||
| .expireAfterWrite(Duration.ofMinutes(15)) | ||
| .build(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Find usages of LoginRateLimitService vs AuthRateLimitService to understand if both are actively used
echo "=== LoginRateLimitService usages ==="
rg -n --type java 'LoginRateLimitService' -g '!LoginRateLimitService.java'
echo ""
echo "=== AuthRateLimitService usages ==="
rg -n --type java 'AuthRateLimitService' -g '!AuthRateLimitService.java'Repository: grimmory-tools/grimmory
Length of output: 603
🏁 Script executed:
# First, let's check the full implementation of LoginRateLimitService
cat booklore-api/src/main/java/org/booklore/config/security/service/LoginRateLimitService.java
echo ""
echo "=== Checking for Spring bean definitions or configurations ==="
rg -n 'LoginRateLimitService' --type java --type xml
echo ""
echo "=== Checking test files that might use LoginRateLimitService ==="
rg -n 'LoginRateLimitService' booklore-api/src/testRepository: grimmory-tools/grimmory
Length of output: 1980
LoginRateLimitService is unused dead code and should either be integrated or removed.
This service is annotated with @Service but has zero usages anywhere in the codebase. Only AuthRateLimitService is actively wired into AuthenticationService.
Before addressing the cache size concern, clarify the intent:
- If this service is planned for future use in a separate login flow, integrate it and then evaluate the cache configuration (1000 IPs with 15-minute expiry could be insufficient under distributed attacks).
- If it's not needed, remove it to eliminate confusion and maintenance burden.
If proceeding with this service, consider a larger cache size (e.g., 10,000) or external storage like Redis for production.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@booklore-api/src/main/java/org/booklore/config/security/service/LoginRateLimitService.java`
around lines 23 - 29, LoginRateLimitService is dead code (annotated `@Service` but
never used); either remove it or integrate it—if you want to remove it, delete
the LoginRateLimitService class and references (constructor AuditService field,
attemptCache) and any unused imports to reduce clutter; if you intend to
keep/use it, wire it into the existing authentication flow (replace or add
alongside AuthRateLimitService in AuthenticationService where rate limiting is
applied), and update the cache configuration in LoginRateLimitService (increase
maximumSize from 1000 to ~10000 or swap attemptCache for an external store like
Redis) and ensure AuditService is correctly invoked where attempts are recorded.
There was a problem hiding this comment.
@balazs-szucs, understood — I'll defer to your judgment on this one.
(∩ˇ_ˇ)⊃━━☆
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
booklore-api/src/main/java/org/booklore/controller/BookController.java (1)
82-83:⚠️ Potential issue | 🟡 MinorExpose pageable query params in OpenAPI docs.
Line 83 hides
Pageablewith@Parameter(hidden = true), sopage,size, andsortmay not appear in generated API docs for this endpoint. Use@ParameterObjectinstead.Suggested change
import io.swagger.v3.oas.annotations.Operation; import io.swagger.v3.oas.annotations.Parameter; import io.swagger.v3.oas.annotations.tags.Tag; import io.swagger.v3.oas.annotations.responses.ApiResponse; import io.swagger.v3.oas.annotations.responses.ApiResponses; +import org.springdoc.core.annotations.ParameterObject; @@ `@GetMapping`("/page") public ResponseEntity<Page<Book>> getBooksPaged( - `@Parameter`(hidden = true) Pageable pageable) { + `@ParameterObject` Pageable pageable) { return ResponseEntity.ok(bookService.getBookDTOsPaged(pageable)); }For springdoc-openapi 3.0.2, what is the recommended way to document Spring Data Pageable query parameters in OpenAPI: `@Parameter`(hidden = true) Pageable or `@ParameterObject` Pageable?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@booklore-api/src/main/java/org/booklore/controller/BookController.java` around lines 82 - 83, The getBooksPaged method currently hides the Pageable parameter with `@Parameter`(hidden = true), which prevents page/size/sort from appearing in OpenAPI; replace that annotation with `@ParameterObject` (from org.springdoc.api.annotations.ParameterObject) on the Pageable parameter so Springdoc will expose the page, size and sort query params in the generated docs and keep the method signature public ResponseEntity<Page<Book>> getBooksPaged(`@ParameterObject` Pageable pageable).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@booklore-api/src/main/resources/application.yaml`:
- Around line 79-80: Remove the deprecated and unused Hibernate property by
deleting the plan_parameter_metadata_max_size entry and its comment from the
configuration block; keep only the active plan_cache_max_size setting
(hibernate.query.plan_cache_max_size) since
hibernate.query.plan_parameter_metadata_max_size is deprecated in Hibernate 6+
and has no effect.
- Around line 36-37: The Tomcat thread pool properties
(server.tomcat.threads.max and server.tomcat.threads.min-spare) are ineffective
when virtual threads are enabled via spring.threads.virtual.enabled; remove the
entries "max: 10" and "min-spare: 2" from application.yaml (the
server.tomcat.threads block) so only the virtual thread configuration remains
and avoid confusion about concurrency limits tied to server.tomcat.threads.*
---
Duplicate comments:
In `@booklore-api/src/main/java/org/booklore/controller/BookController.java`:
- Around line 82-83: The getBooksPaged method currently hides the Pageable
parameter with `@Parameter`(hidden = true), which prevents page/size/sort from
appearing in OpenAPI; replace that annotation with `@ParameterObject` (from
org.springdoc.api.annotations.ParameterObject) on the Pageable parameter so
Springdoc will expose the page, size and sort query params in the generated docs
and keep the method signature public ResponseEntity<Page<Book>>
getBooksPaged(`@ParameterObject` Pageable pageable).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b215e92e-885f-4077-a793-34b2276f3c83
📒 Files selected for processing (7)
Dockerfilebooklore-api/build.gradle.ktsbooklore-api/src/main/java/org/booklore/controller/BookController.javabooklore-api/src/main/java/org/booklore/service/book/BookService.javabooklore-api/src/main/java/org/booklore/service/book/BookUpdateService.javabooklore-api/src/main/resources/application.yamldev.docker-compose.yml
✅ Files skipped from review due to trivial changes (1)
- booklore-api/build.gradle.kts
🚧 Files skipped from review as they are similar to previous changes (4)
- dev.docker-compose.yml
- booklore-api/src/main/java/org/booklore/service/book/BookUpdateService.java
- Dockerfile
- booklore-api/src/main/java/org/booklore/service/book/BookService.java
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Test Suite / Frontend Tests
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (java-kotlin)
🧰 Additional context used
📓 Path-based instructions (1)
booklore-api/src/**/*.java
📄 CodeRabbit inference engine (AGENTS.md)
booklore-api/src/**/*.java: Use 4-space indentation in Java files and match surrounding Java style
Prefer constructor injection via Lombok patterns already used in the codebase. Do not introduce@Autowiredfield injection
Use MapStruct for entity/DTO mapping
Files:
booklore-api/src/main/java/org/booklore/controller/BookController.java
🧠 Learnings (1)
📚 Learning: 2026-03-25T21:02:57.527Z
Learnt from: imnotjames
Repo: grimmory-tools/grimmory PR: 195
File: booklore-api/src/main/java/org/booklore/config/logging/RequestLoggingFilterConfig.java:15-19
Timestamp: 2026-03-25T21:02:57.527Z
Learning: In `booklore-api/src/main/java/org/booklore/config/logging/RequestLoggingFilterConfig.java` and `booklore-api/src/main/java/org/booklore/config/logging/filter/RequestLoggingFilter.java`, logging all request headers and payloads at DEBUG level is intentional and accepted behavior in this project. The previous `LoggingFilter` also logged all headers. No header redaction or payload scrubbing is required — the DEBUG log level is considered sufficient access control. The new filter is not profile-gated (unlike the old `Profile("dev")` filter) but relies on `logger.isDebugEnabled()` for gating.
Applied to files:
booklore-api/src/main/resources/application.yaml
🔇 Additional comments (4)
booklore-api/src/main/java/org/booklore/controller/BookController.java (1)
42-43: Imports for pagination types look good.
PageandPageableare correctly added for the new paginated endpoint.booklore-api/src/main/resources/application.yaml (3)
38-38: Reasonable header-size bump.This fits the remote-auth and forwarded-header shape of the rest of the config.
62-63: Nice to see the transaction-boundary toggles land together.Bundling the autocommit change,
open-in-view: false, and the related Hibernate connection/slow-query settings makes the PR’s service-layer transaction changes much easier to reason about.Also applies to: 70-70, 90-93
82-85: Nice guardrail for the new pageable reads.Turning this failure on next to the more conservative batch size is a good fit for the new
/pageflow in this PR.
Description
Linked Issue: Fixes #
Changes
This pull request introduces several improvements and optimizations across the codebase, focusing on entity mapping efficiency, transactional consistency, and controller enhancements. The main changes include improved lazy loading and batch fetching for JPA entities, transactional annotation adjustments for service classes, a new paginated endpoint in the book controller, and some code cleanups for better performance and maintainability.
Entity mapping and performance improvements:
Switched several entity collection fields (e.g.,
bookMetadataEntityListinAuthorEntity,shelvesanduserBookProgressinBookEntity) from lists to sets and initialized them with default empty collections. Added@BatchSizeand@LazyGroupannotations to optimize lazy loading and batch fetching, and made certain large fields (likeembeddingVectorandsearchTextinBookMetadataEntity) lazy-loaded for performance.Added or improved
equalsandhashCodeimplementations forBookLoreUserEntityto ensure correct entity comparison, using Hibernate's class-based equality.Service and transactional consistency:
Moved the
@Transactional(readOnly = true)annotation from individual methods to the class level inAppBookServiceto ensure consistent transactional behavior for all service methods.Added
@Transactionalto thelogoutmethod inLogoutServiceand optimized token revocation by collecting tokens and saving them in bulk withsaveAllinstead of saving each individually.Controller and API enhancements:
/pageinBookControllerto support retrieving books with pagination and sorting using Spring Data'sPageable.Build and configuration improvements:
build.gradle.ktsto enable lazy initialization and increased the test task's heap size for better test performance.Summary by CodeRabbit
New Features
Performance Improvements
Bug Fixes