refactor(api): move detect deleted/new books to LibraryFileHelper#555
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📜 Recent 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). (4)
🧰 Additional context used📓 Path-based instructions (1)booklore-api/src/**/*.java📄 CodeRabbit inference engine (AGENTS.md)
Files:
🧠 Learnings (12)📓 Common learnings📚 Learning: 2026-03-24T18:46:47.249ZApplied to files:
📚 Learning: 2026-04-04T15:36:56.558ZApplied to files:
📚 Learning: 2026-03-23T18:36:05.843ZApplied to files:
📚 Learning: 2026-04-02T09:25:48.330ZApplied to files:
📚 Learning: 2026-04-14T12:46:56.673ZApplied to files:
📚 Learning: 2026-04-02T09:25:48.330ZApplied to files:
📚 Learning: 2026-04-14T12:43:08.698ZApplied to files:
📚 Learning: 2026-04-01T17:50:06.817ZApplied to files:
📚 Learning: 2026-03-26T01:46:48.863ZApplied to files:
📚 Learning: 2026-04-10T08:15:37.436ZApplied to files:
📚 Learning: 2026-04-13T05:02:01.463ZApplied to files:
📝 WalkthroughWalkthroughRefactors library synchronization by moving file-change detection from LibraryProcessingService into LibraryFileHelper and adding three public detection methods to compare persisted BookEntity/BookFileEntity data with current LibraryFile entries using stable generated keys. Changes
Sequence Diagram(s)sequenceDiagram
participant Proc as LibraryProcessingService
participant Helper as LibraryFileHelper
participant FS as Filesystem/LibraryFileSet
participant DB as BookEntity/BookFileEntity
Proc->>FS: getLibraryFiles(library)
Proc->>DB: load existing Books and additional BookFileEntities
Proc->>Helper: detectDeletedBookIds(FS.files, DB.books)
Proc->>Helper: detectDeletedAdditionalFiles(FS.files, DB.additionalFiles)
Proc->>Helper: detectNewBookPaths(FS.files, DB.books, DB.additionalFiles)
Helper-->>Proc: lists of deleted IDs and new paths
Proc->>DB: apply create/delete updates based on helper results
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
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/library/LibraryProcessingService.java (1)
101-121:⚠️ Potential issue | 🟡 MinorReusing
allAdditionalFilesacrossentityManager.clear()— verify detached-state safety.
allAdditionalFilesis loaded at line 101 and reused at line 121, across theentityManager.clear()at line 115. This works today because:
detectDeletedAdditionalFilesat line 103 forces eager resolution offile.getBook().getLibraryPath().getId()viagenerateUniqueKey, so the relationships are materialized into the Java object graph before the clear.- Any IDs deleted at line 106 still appear in the stale list, but their corresponding files were also missing from disk — so they won't be in
filteredFilesand won't be incorrectly excluded from the "new files" computation.Both invariants are subtle and would break silently if, for example,
detectDeletedAdditionalFileswere changed to short-circuit without touching the lazy relations, or ifdeleteRemovedAdditionalFilesexpanded its scope. Consider either re-loadingallAdditionalFilesafter theentityManager.clear()(symmetrical to thebooksre-fetch on line 119) or adding a code comment documenting the dependency.🤖 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/library/LibraryProcessingService.java` around lines 101 - 121, The code uses allAdditionalFiles (loaded via bookAdditionalFileRepository.findByLibraryId) both before and after entityManager.clear(), creating a risk from detached entities; after clearing the EntityManager you should re-load the additional files to ensure attached/fresh state — replace the reuse of the original allAdditionalFiles by calling bookAdditionalFileRepository.findByLibraryId(libraryId) again after entityManager.clear() (similarly to how books is re-fetched), and then pass that fresh list into libraryFileHelper.detectNewBookPaths; alternatively, if you intentionally rely on the prior behavior, add a clear explanatory comment referencing detectDeletedAdditionalFiles, deleteRemovedAdditionalFiles, entityManager.clear(), and detectNewBookPaths to document the dependency.
🧹 Nitpick comments (4)
booklore-api/src/test/java/org/booklore/service/library/LibraryFileHelperTest.java (2)
31-32:@InjectMockson a no-dependency class is a no-op.
LibraryFileHelperhas no injected collaborators, so@InjectMocksis effectively equivalent tonew LibraryFileHelper(). Using@InjectMockshere signals to readers that there are mock dependencies when there aren't any. Consider either:
- Direct instantiation:
private final LibraryFileHelper libraryFileHelper = new LibraryFileHelper();, or- Leaving
@InjectMocksonly if dependencies are anticipated in a follow-up.Non-blocking, stylistic.
🤖 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/library/LibraryFileHelperTest.java` around lines 31 - 32, The test uses `@InjectMocks` on LibraryFileHelper which has no dependencies, so replace the misleading annotation with direct instantiation: remove `@InjectMocks` and initialize libraryFileHelper as a final new LibraryFileHelper() (or keep `@InjectMocks` only if you plan to add collaborators later); update the field declaration for LibraryFileHelper accordingly to reflect direct construction.
427-504: LGTM — thorough coverage of the new detection methods.Good mix of cases: fileless/deleted book exclusion, folder-based audiobook matching, missing-file detection, no-books / additional-files interactions, and
isBookFormatfiltering. Tests directly exercise the key-generation behavior via paired fixtures that shareLibraryPathEntity, which is exactly what the refactor was meant to enable.One minor note: the tests rely on
libraryPathIdbeingnullon both sides of the comparison to produce matching"null:subPath:fileName"keys. That works, but setting an explicit id (e.g.,.id(10L)as done intestGetAllLibraryFiles_IncludesAudiobookFolderFiles) would make the intent clearer and guard against regressions ifgenerateKeyever changes its null handling.Also applies to: 508-725
🤖 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/library/LibraryFileHelperTest.java` around lines 427 - 504, The tests rely on LibraryPathEntity.id being null which makes generated keys use "null:subPath:fileName"; set an explicit id on the shared LibraryPathEntity instances to make key matching deterministic and guard against future null-handling changes—update the LibraryPathEntity builders used in these tests (e.g., the instances referenced by LibraryFile/bookLibraryFolder/bookLibraryFile and BookEntity.libraryPath) to include .id(10L) (or another fixed long) so both sides of the comparisons use the same non-null libraryPathId.booklore-api/src/main/java/org/booklore/service/library/LibraryFileHelper.java (2)
54-54: PreferStream.toList()overCollectors.toList().On Java 25,
.toList()is the idiomatic terminal operation and returns an unmodifiable list, which is appropriate here since callers don't mutate the results. The existing helper at line 230 already usesCollectors.toList()— that's outside the scope of this PR, but the new additions are a good place to adopt the modern idiom.♻️ Proposed change
- .map(BookEntity::getId) - .collect(Collectors.toList()); + .map(BookEntity::getId) + .toList();(apply similarly at lines 66 and 83)
Based on learnings: "For this project (grimmory-tools/grimmory / booklore-api), prefer modern Java 25 idioms... Favor current constructs such as ...
Stream.toList()".Also applies to: 66-66, 83-83
🤖 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/library/LibraryFileHelper.java` at line 54, Replace the Stream terminal operation usages of ".collect(Collectors.toList())" in LibraryFileHelper with the modern Java 25 idiom ".toList()": find each occurrence of ".collect(Collectors.toList())" (three places in LibraryFileHelper) and change it to the stream's ".toList()" so the result is an unmodifiable list; ensure you update all three occurrences (the one shown and the other two similar spots) and keep the surrounding logic unchanged.
86-93: Dead "fileless" branch at current call sites.
generateUniqueKey(BookEntity)returns"fileless:<id>"whenprimaryFile == null, but the only caller (detectNewBookPathsat line 72) pre-filters books viabook.getBookFiles() != null && !book.getBookFiles().isEmpty(), sogetPrimaryBookFile()will never return null there. This is fine as a defensive helper, but the pre-filter indetectNewBookPathsis redundant given the null-safe helper — pick one path to avoid confusion for future callers. Not blocking.🤖 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/library/LibraryFileHelper.java` around lines 86 - 93, The generateUniqueKey(BookEntity) helper already returns a safe "fileless:<id>" key when book.getPrimaryBookFile() is null, so remove the redundant pre-filter in detectNewBookPaths that checks book.getBookFiles() != null && !book.getBookFiles().isEmpty(); instead let detectNewBookPaths call generateUniqueKey(book) for all books (using generateUniqueKey and book.getPrimaryBookFile() as the canonical null-safe path), so the helper remains defensive for other callers and the detector logic is simplified.
🤖 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/service/library/LibraryFileHelper.java`:
- Around line 39-55: detectDeletedBookIds currently compares Path objects via
LibraryFile.getFullPath() vs BookEntity.getFullFilePath(), which is inconsistent
with detectDeletedAdditionalFiles and detectNewBookPaths that use the
libraryPathId:subPath:fileName key; to fix, change detectDeletedBookIds to
compute the same key for LibraryFile and BookEntity (using libraryPathId,
subPath, fileName normalization rules used by detectNewBookPaths) and compare
sets of those keys instead of Path, ensuring null/empty subPath handling matches
the other methods so deletes/new-file detection cannot disagree.
---
Outside diff comments:
In
`@booklore-api/src/main/java/org/booklore/service/library/LibraryProcessingService.java`:
- Around line 101-121: The code uses allAdditionalFiles (loaded via
bookAdditionalFileRepository.findByLibraryId) both before and after
entityManager.clear(), creating a risk from detached entities; after clearing
the EntityManager you should re-load the additional files to ensure
attached/fresh state — replace the reuse of the original allAdditionalFiles by
calling bookAdditionalFileRepository.findByLibraryId(libraryId) again after
entityManager.clear() (similarly to how books is re-fetched), and then pass that
fresh list into libraryFileHelper.detectNewBookPaths; alternatively, if you
intentionally rely on the prior behavior, add a clear explanatory comment
referencing detectDeletedAdditionalFiles, deleteRemovedAdditionalFiles,
entityManager.clear(), and detectNewBookPaths to document the dependency.
---
Nitpick comments:
In
`@booklore-api/src/main/java/org/booklore/service/library/LibraryFileHelper.java`:
- Line 54: Replace the Stream terminal operation usages of
".collect(Collectors.toList())" in LibraryFileHelper with the modern Java 25
idiom ".toList()": find each occurrence of ".collect(Collectors.toList())"
(three places in LibraryFileHelper) and change it to the stream's ".toList()" so
the result is an unmodifiable list; ensure you update all three occurrences (the
one shown and the other two similar spots) and keep the surrounding logic
unchanged.
- Around line 86-93: The generateUniqueKey(BookEntity) helper already returns a
safe "fileless:<id>" key when book.getPrimaryBookFile() is null, so remove the
redundant pre-filter in detectNewBookPaths that checks book.getBookFiles() !=
null && !book.getBookFiles().isEmpty(); instead let detectNewBookPaths call
generateUniqueKey(book) for all books (using generateUniqueKey and
book.getPrimaryBookFile() as the canonical null-safe path), so the helper
remains defensive for other callers and the detector logic is simplified.
In
`@booklore-api/src/test/java/org/booklore/service/library/LibraryFileHelperTest.java`:
- Around line 31-32: The test uses `@InjectMocks` on LibraryFileHelper which has
no dependencies, so replace the misleading annotation with direct instantiation:
remove `@InjectMocks` and initialize libraryFileHelper as a final new
LibraryFileHelper() (or keep `@InjectMocks` only if you plan to add collaborators
later); update the field declaration for LibraryFileHelper accordingly to
reflect direct construction.
- Around line 427-504: The tests rely on LibraryPathEntity.id being null which
makes generated keys use "null:subPath:fileName"; set an explicit id on the
shared LibraryPathEntity instances to make key matching deterministic and guard
against future null-handling changes—update the LibraryPathEntity builders used
in these tests (e.g., the instances referenced by
LibraryFile/bookLibraryFolder/bookLibraryFile and BookEntity.libraryPath) to
include .id(10L) (or another fixed long) so both sides of the comparisons use
the same non-null libraryPathId.
🪄 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: 3f2e78ed-a004-45c4-a1fa-00e5b111709f
📒 Files selected for processing (4)
booklore-api/src/main/java/org/booklore/service/library/LibraryFileHelper.javabooklore-api/src/main/java/org/booklore/service/library/LibraryProcessingService.javabooklore-api/src/test/java/org/booklore/service/library/LibraryFileHelperTest.javabooklore-api/src/test/java/org/booklore/service/library/LibraryProcessingServiceTest.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). (1)
- GitHub Check: Test Suite / Backend Tests
🧰 Additional context used
📓 Path-based instructions (2)
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/test/java/org/booklore/service/library/LibraryProcessingServiceTest.javabooklore-api/src/main/java/org/booklore/service/library/LibraryProcessingService.javabooklore-api/src/main/java/org/booklore/service/library/LibraryFileHelper.javabooklore-api/src/test/java/org/booklore/service/library/LibraryFileHelperTest.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/service/library/LibraryProcessingServiceTest.javabooklore-api/src/test/java/org/booklore/service/library/LibraryFileHelperTest.java
🧠 Learnings (9)
📓 Common learnings
Learnt from: balazs-szucs
Repo: grimmory-tools/grimmory PR: 334
File: booklore-api/src/main/java/org/booklore/service/reader/EpubReaderService.java:402-407
Timestamp: 2026-04-02T09:25:48.330Z
Learning: In grimmory-tools/grimmory, before commenting on any file processing code (epub or pdf), always verify the current state of the relevant upstream grimmory libraries: `grimmory-tools/epub4j` (for epub) and `grimmory-tools/PDFium4j` (for pdf). These custom libraries may have different APIs, capabilities, and limitations compared to the third-party libraries they replace. Issues about streaming, buffering, or API surface may need to be filed/addressed in those upstream repos rather than in grimmory itself.
📚 Learning: 2026-03-23T18:36:05.843Z
Learnt from: imnotjames
Repo: grimmory-tools/grimmory PR: 146
File: booklore-api/src/main/java/org/booklore/util/FileService.java:151-168
Timestamp: 2026-03-23T18:36:05.843Z
Learning: In `booklore-api/src/main/java/org/booklore/util/FileService.java`, the `"bin"` search path in `getSystemSearchPath()` is intentionally left as a relative path (not derived from `appProperties.getPathConfig()`), so binary lookup resolves relative to the process working directory. Do not flag this as an issue.
Applied to files:
booklore-api/src/test/java/org/booklore/service/library/LibraryProcessingServiceTest.java
📚 Learning: 2026-04-10T08:15:37.436Z
Learnt from: imnotjames
Repo: grimmory-tools/grimmory PR: 449
File: booklore-api/src/main/java/org/booklore/service/book/BookDownloadService.java:139-145
Timestamp: 2026-04-10T08:15:37.436Z
Learning: When using Spring `ContentDisposition.builder(...).filename(name, StandardCharsets.UTF_8).build()` (i.e., explicitly providing UTF-8), the resulting header value should include both the quoted `filename="=?UTF-8?..."` and the RFC 5987 `filename*=` parameters. In this case, any extra ASCII fallback computation (e.g., deriving an ASCII `fallbackFilename` via `NON_ASCII_PATTERN` and calling `.filename(fallbackFilename)`) is likely redundant—prefer calling only `.filename(fallbackName?, StandardCharsets.UTF_8)` as appropriate and let Spring handle the UTF-8 header parameters. Verify by comparing the emitted header for `filename` and `filename*` before deciding to keep an ASCII fallback.
Applied to files:
booklore-api/src/test/java/org/booklore/service/library/LibraryProcessingServiceTest.javabooklore-api/src/main/java/org/booklore/service/library/LibraryProcessingService.javabooklore-api/src/main/java/org/booklore/service/library/LibraryFileHelper.javabooklore-api/src/test/java/org/booklore/service/library/LibraryFileHelperTest.java
📚 Learning: 2026-04-14T12:43:08.698Z
Learnt from: balazs-szucs
Repo: grimmory-tools/grimmory PR: 502
File: booklore-api/src/main/java/org/booklore/service/reader/ChapterCacheService.java:0-0
Timestamp: 2026-04-14T12:43:08.698Z
Learning: For this codebase (booklore-api), target Java 25 with `--enable-preview`, so `_` is intentionally used as an unnamed/ignored variable (e.g., lambda parameter or pattern variable) per Java’s preview feature JEP 456. Do not flag `_` in those contexts as an invalid/reserved identifier; only flag it if it’s used in a non-supported position (e.g., where an unnamed variable is not applicable for the Java preview rules).
Applied to files:
booklore-api/src/test/java/org/booklore/service/library/LibraryProcessingServiceTest.javabooklore-api/src/main/java/org/booklore/service/library/LibraryProcessingService.javabooklore-api/src/main/java/org/booklore/service/library/LibraryFileHelper.javabooklore-api/src/test/java/org/booklore/service/library/LibraryFileHelperTest.java
📚 Learning: 2026-04-13T05:02:01.463Z
Learnt from: imnotjames
Repo: grimmory-tools/grimmory PR: 486
File: booklore-api/src/main/java/org/booklore/service/oidc/OidcGroupMappingService.java:130-159
Timestamp: 2026-04-13T05:02:01.463Z
Learning: In grimmory-tools/grimmory, treat `permissionDemoUser`/`permission_demo_user` as a restricting “limited demo mode” flag: it must never be set to `true` as part of any “grant all permissions” or admin-grant flow. Specifically, in code paths that grant admin permissions (for example the OIDC `isAdmin` branch in `OidcGroupMappingService.applyPermissions`), ensure `permissionDemoUser` is intentionally excluded from the admin-permission block and cannot be turned on; enabling it for admin users is counterproductive.
Applied to files:
booklore-api/src/main/java/org/booklore/service/library/LibraryProcessingService.javabooklore-api/src/main/java/org/booklore/service/library/LibraryFileHelper.java
📚 Learning: 2026-04-14T12:46:56.673Z
Learnt from: balazs-szucs
Repo: grimmory-tools/grimmory PR: 502
File: booklore-api/src/main/java/org/booklore/service/reader/ChapterCacheService.java:0-0
Timestamp: 2026-04-14T12:46:56.673Z
Learning: For this project (grimmory-tools/grimmory / booklore-api), prefer modern Java 25 idioms and language/library features when reviewing or suggesting changes. Favor current constructs such as unnamed variables (e.g., `_`) where applicable, records, pattern matching for `switch`/`instanceof`, sealed classes, sequenced collections, `Stream.toList()`, and text blocks. Avoid proposing regressions to older/legacy equivalents unless there’s a concrete compatibility constraint (e.g., a required older runtime/library behavior).
Applied to files:
booklore-api/src/main/java/org/booklore/service/library/LibraryProcessingService.javabooklore-api/src/main/java/org/booklore/service/library/LibraryFileHelper.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/**/*Entity.java : Keep JPA entities on the `*Entity` suffix
Applied to files:
booklore-api/src/main/java/org/booklore/service/library/LibraryFileHelper.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/test/java/org/booklore/service/library/LibraryFileHelperTest.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/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/service/library/LibraryFileHelperTest.java
🔇 Additional comments (2)
booklore-api/src/test/java/org/booklore/service/library/LibraryProcessingServiceTest.java (1)
116-116: LGTM — delegation stubs are consistent with the new helper API.The
any(), any(), any()andany(), any()matchers correctly mirrorLibraryFileHelper.detectNewBookPaths/detectDeletedAdditionalFilessignatures, and the stubbed return values drive the downstream grouping/deletion assertions as intended.Also applies to: 212-212, 255-255, 376-376
booklore-api/src/main/java/org/booklore/service/library/LibraryProcessingService.java (1)
60-62: LGTM — clean delegation toLibraryFileHelper.The in-class detection/key-generation logic has been replaced with helper calls without changing the call order or side effects. Good move for testability.
Also applies to: 103-108
3d3d3ca to
c6ad437
Compare
Description
this moves private behaviors out of the
LibraryProcessingServiceand into theLibraryFileHelper. This is done to make them public, create explicit tests for the behavior, and provide an more straightforward API for validating fixes against.This makes no changes to behavior and adds new tests for code that was previously untested.
This is the refactor to enable an easier review of #540
Linked Issue: Related to #446
Changes
detectDeletedBookIdstoLibraryFileHelperdetectNewBookPathstoLibraryFileHelperdetectDeletedAdditionalFilestoLibraryFileHelperSummary by CodeRabbit
Refactor
User-facing Fixes
Tests