refactor(api): use Path for FileUtils.getBookFullPath#209
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughFileUtils.getBookFullPath(...) now returns java.nio.file.Path; callers were updated to consume Path directly or convert via .toFile() / .toString() where needed. Tests and static mocks were updated to return and assert Path values. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/test/java/org/booklore/service/book/BookServiceTest.java (1)
478-485:⚠️ Potential issue | 🔴 CriticalFix stale String stub for
getBookFullPathinstreamBookContent_delegatesToStreamingService.The method
FileUtils.getBookFullPath(BookEntity)returnsPath, notString. The current stub returns a String literal which is type-unsafe and inconsistent with other stubs in this test file (lines 128, 156, 332, 353 all correctly usePath.of(...)). This will cause type mismatch in Mockito stubbing.Suggested patch
try (MockedStatic<FileUtils> fileUtilsMock = mockStatic(FileUtils.class)) { - fileUtilsMock.when(() -> FileUtils.getBookFullPath(entity)).thenReturn("/tmp/library/book.epub"); + Path fullPath = Path.of("/tmp/library/book.epub"); + fileUtilsMock.when(() -> FileUtils.getBookFullPath(entity)).thenReturn(fullPath); bookService.streamBookContent(15L, null, request, response); verify(bookRepository).findByIdWithBookFiles(15L); - verify(fileStreamingService).streamWithRangeSupport(eq(Paths.get("/tmp/library/book.epub")), eq("application/epub+zip"), eq(request), eq(response)); + verify(fileStreamingService).streamWithRangeSupport(eq(fullPath), eq("application/epub+zip"), eq(request), eq(response)); }🤖 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/book/BookServiceTest.java` around lines 478 - 485, The stub for FileUtils.getBookFullPath in the test streamBookContent_delegatesToStreamingService is returning a String but the method returns a Path; update the mock to return a Path (use Path.of(...) or Paths.get(...)) instead of "/tmp/library/book.epub" so the mocked static FileUtils.getBookFullPath(entity) matches the expected return type and the verify call to fileStreamingService.streamWithRangeSupport still uses Paths.get("/tmp/library/book.epub") for comparison.
🧹 Nitpick comments (1)
booklore-api/src/main/java/org/booklore/service/book/BookService.java (1)
341-341: Prefer keepingfilePathasPathend-to-end in both methods.Current flow converts
Path -> String -> File/Path. KeepingPaththroughout is cleaner and better aligned with this PR’s objective.Refactor sketch
- String filePath; + Path filePath; if (bookType != null) { BookFileType requestedType = BookFileType.valueOf(bookType.toUpperCase()); BookFileEntity bookFile = bookEntity.getBookFiles().stream() .filter(bf -> bf.getBookType() == requestedType) .findFirst() .orElseThrow(() -> ApiError.FILE_NOT_FOUND.createException("No file of type " + bookType + " found for book")); - filePath = bookFile.getFullFilePath().toString(); + filePath = bookFile.getFullFilePath(); } else { - filePath = FileUtils.getBookFullPath(bookEntity).toString(); + filePath = FileUtils.getBookFullPath(bookEntity); } - File file = new File(filePath); + File file = filePath.toFile(); if (!file.exists()) { - throw ApiError.FILE_NOT_FOUND.createException(filePath); + throw ApiError.FILE_NOT_FOUND.createException(filePath.toString()); } @@ - String filePath; + Path filePath; if (bookType != null) { BookFileType requestedType = BookFileType.valueOf(bookType.toUpperCase()); BookFileEntity bookFile = bookEntity.getBookFiles().stream() .filter(bf -> bf.getBookType() == requestedType) .findFirst() .orElseThrow(() -> ApiError.FILE_NOT_FOUND.createException("No file of type " + bookType + " found for book")); - filePath = bookFile.getFullFilePath().toString(); + filePath = bookFile.getFullFilePath(); } else { - filePath = FileUtils.getBookFullPath(bookEntity).toString(); + filePath = FileUtils.getBookFullPath(bookEntity); } - - Path path = Paths.get(filePath); + Path path = filePath;Also applies to: 365-365
🤖 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` at line 341, The code converts the Path returned by FileUtils.getBookFullPath(bookEntity) to a String (filePath) and back to File/Path later; change filePath to be a Path (e.g., Path filePath = FileUtils.getBookFullPath(bookEntity)) and propagate Path usage end-to-end in the surrounding methods instead of calling toString(), updating any downstream calls that currently accept String to accept Path (or perform a single, localized conversion at the I/O boundary such as filePath.toFile() only where APIs require File); adjust method signatures or call sites as needed so both occurrences that use FileUtils.getBookFullPath(bookEntity) (the current line and the other at the later occurrence) keep a Path rather than a String.
🤖 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/util/FileUtils.java`:
- Around line 32-38: The method FileUtils.getBookFullPath must guard against
nullable path parts: check bookFile.getFileSubPath() for null before calling
Path.of(...) and supply an empty string or omit that segment when building the
Path, and ensure you return null or Optional<Path> consistently; likewise,
callers that call Files.exists(...) on getBookFullPath() (e.g., the code around
lines 53-55) must first check that getBookFullPath(...) did not return null
before calling Files.exists. Update FileUtils.getBookFullPath to handle null
bookFile.getFileSubPath (and null bookEntity.getLibraryPath) safely and update
any Files.exists(...) call sites to null-check the returned Path (or change the
method to return Optional<Path> and adapt callers).
---
Outside diff comments:
In `@booklore-api/src/test/java/org/booklore/service/book/BookServiceTest.java`:
- Around line 478-485: The stub for FileUtils.getBookFullPath in the test
streamBookContent_delegatesToStreamingService is returning a String but the
method returns a Path; update the mock to return a Path (use Path.of(...) or
Paths.get(...)) instead of "/tmp/library/book.epub" so the mocked static
FileUtils.getBookFullPath(entity) matches the expected return type and the
verify call to fileStreamingService.streamWithRangeSupport still uses
Paths.get("/tmp/library/book.epub") for comparison.
---
Nitpick comments:
In `@booklore-api/src/main/java/org/booklore/service/book/BookService.java`:
- Line 341: The code converts the Path returned by
FileUtils.getBookFullPath(bookEntity) to a String (filePath) and back to
File/Path later; change filePath to be a Path (e.g., Path filePath =
FileUtils.getBookFullPath(bookEntity)) and propagate Path usage end-to-end in
the surrounding methods instead of calling toString(), updating any downstream
calls that currently accept String to accept Path (or perform a single,
localized conversion at the I/O boundary such as filePath.toFile() only where
APIs require File); adjust method signatures or call sites as needed so both
occurrences that use FileUtils.getBookFullPath(bookEntity) (the current line and
the other at the later occurrence) keep a Path rather than a String.
🪄 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: 8e33ab1e-bf36-483d-82ba-17ae35ea652c
📒 Files selected for processing (21)
booklore-api/src/main/java/org/booklore/service/book/BookDownloadService.javabooklore-api/src/main/java/org/booklore/service/book/BookService.javabooklore-api/src/main/java/org/booklore/service/email/SendEmailV2Service.javabooklore-api/src/main/java/org/booklore/service/fileprocessor/AudiobookProcessor.javabooklore-api/src/main/java/org/booklore/service/fileprocessor/Azw3Processor.javabooklore-api/src/main/java/org/booklore/service/fileprocessor/CbxProcessor.javabooklore-api/src/main/java/org/booklore/service/fileprocessor/EpubProcessor.javabooklore-api/src/main/java/org/booklore/service/fileprocessor/Fb2Processor.javabooklore-api/src/main/java/org/booklore/service/fileprocessor/MobiProcessor.javabooklore-api/src/main/java/org/booklore/service/fileprocessor/PdfProcessor.javabooklore-api/src/main/java/org/booklore/service/metadata/BookMetadataService.javabooklore-api/src/main/java/org/booklore/service/reader/CbxReaderService.javabooklore-api/src/main/java/org/booklore/service/reader/EpubReaderService.javabooklore-api/src/main/java/org/booklore/service/reader/PdfReaderService.javabooklore-api/src/main/java/org/booklore/util/FileUtils.javabooklore-api/src/test/java/org/booklore/service/Rar5FallbackIntegrationTest.javabooklore-api/src/test/java/org/booklore/service/book/BookServiceTest.javabooklore-api/src/test/java/org/booklore/service/email/SendEmailV2ServiceTest.javabooklore-api/src/test/java/org/booklore/service/reader/CbxReaderServiceTest.javabooklore-api/src/test/java/org/booklore/service/reader/EpubReaderServiceTest.javabooklore-api/src/test/java/org/booklore/util/FileUtilsTest.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/main/java/org/booklore/service/reader/EpubReaderService.javabooklore-api/src/main/java/org/booklore/service/metadata/BookMetadataService.javabooklore-api/src/main/java/org/booklore/service/book/BookService.javabooklore-api/src/main/java/org/booklore/service/fileprocessor/MobiProcessor.javabooklore-api/src/main/java/org/booklore/service/fileprocessor/CbxProcessor.javabooklore-api/src/test/java/org/booklore/service/reader/CbxReaderServiceTest.javabooklore-api/src/main/java/org/booklore/util/FileUtils.javabooklore-api/src/test/java/org/booklore/service/book/BookServiceTest.javabooklore-api/src/main/java/org/booklore/service/reader/PdfReaderService.javabooklore-api/src/main/java/org/booklore/service/fileprocessor/Fb2Processor.javabooklore-api/src/test/java/org/booklore/service/email/SendEmailV2ServiceTest.javabooklore-api/src/main/java/org/booklore/service/fileprocessor/PdfProcessor.javabooklore-api/src/main/java/org/booklore/service/fileprocessor/Azw3Processor.javabooklore-api/src/main/java/org/booklore/service/fileprocessor/AudiobookProcessor.javabooklore-api/src/main/java/org/booklore/service/fileprocessor/EpubProcessor.javabooklore-api/src/test/java/org/booklore/service/Rar5FallbackIntegrationTest.javabooklore-api/src/test/java/org/booklore/service/reader/EpubReaderServiceTest.javabooklore-api/src/test/java/org/booklore/util/FileUtilsTest.javabooklore-api/src/main/java/org/booklore/service/email/SendEmailV2Service.javabooklore-api/src/main/java/org/booklore/service/reader/CbxReaderService.javabooklore-api/src/main/java/org/booklore/service/book/BookDownloadService.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/reader/CbxReaderServiceTest.javabooklore-api/src/test/java/org/booklore/service/book/BookServiceTest.javabooklore-api/src/test/java/org/booklore/service/email/SendEmailV2ServiceTest.javabooklore-api/src/test/java/org/booklore/service/Rar5FallbackIntegrationTest.javabooklore-api/src/test/java/org/booklore/service/reader/EpubReaderServiceTest.javabooklore-api/src/test/java/org/booklore/util/FileUtilsTest.java
🧠 Learnings (8)
📓 Common learnings
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.
📚 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/main/java/org/booklore/service/reader/EpubReaderService.javabooklore-api/src/main/java/org/booklore/service/metadata/BookMetadataService.javabooklore-api/src/main/java/org/booklore/service/book/BookService.javabooklore-api/src/main/java/org/booklore/service/fileprocessor/CbxProcessor.javabooklore-api/src/main/java/org/booklore/util/FileUtils.javabooklore-api/src/test/java/org/booklore/service/book/BookServiceTest.javabooklore-api/src/main/java/org/booklore/service/reader/PdfReaderService.javabooklore-api/src/test/java/org/booklore/service/email/SendEmailV2ServiceTest.javabooklore-api/src/main/java/org/booklore/service/fileprocessor/PdfProcessor.javabooklore-api/src/main/java/org/booklore/service/fileprocessor/AudiobookProcessor.javabooklore-api/src/test/java/org/booklore/service/Rar5FallbackIntegrationTest.javabooklore-api/src/test/java/org/booklore/service/reader/EpubReaderServiceTest.javabooklore-api/src/test/java/org/booklore/util/FileUtilsTest.javabooklore-api/src/main/java/org/booklore/service/reader/CbxReaderService.javabooklore-api/src/main/java/org/booklore/service/book/BookDownloadService.java
📚 Learning: 2026-03-26T01:46:48.847Z
Learnt from: CR
Repo: grimmory-tools/grimmory PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-26T01:46:48.847Z
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/reader/EpubReaderService.javabooklore-api/src/test/java/org/booklore/service/reader/EpubReaderServiceTest.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/book/BookService.java
📚 Learning: 2026-03-26T01:46:48.847Z
Learnt from: CR
Repo: grimmory-tools/grimmory PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-26T01:46:48.847Z
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/book/BookServiceTest.javabooklore-api/src/test/java/org/booklore/service/email/SendEmailV2ServiceTest.javabooklore-api/src/test/java/org/booklore/service/reader/EpubReaderServiceTest.java
📚 Learning: 2026-03-26T01:46:48.847Z
Learnt from: CR
Repo: grimmory-tools/grimmory PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-26T01:46:48.847Z
Learning: Applies to booklore-api/src/**/*.java : Use MapStruct for entity/DTO mapping
Applied to files:
booklore-api/src/test/java/org/booklore/service/book/BookServiceTest.java
📚 Learning: 2026-03-26T01:46:48.847Z
Learnt from: CR
Repo: grimmory-tools/grimmory PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-26T01:46:48.847Z
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/book/BookServiceTest.javabooklore-api/src/main/java/org/booklore/service/reader/CbxReaderService.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/reader/PdfReaderService.java
🔇 Additional comments (20)
booklore-api/src/main/java/org/booklore/util/FileUtils.java (1)
27-30: Good delegation after return-type migration.This keeps one path-construction source of truth and avoids duplicate conversion logic.
booklore-api/src/main/java/org/booklore/service/reader/CbxReaderService.java (1)
147-147: Path propagation is correct here.Directly returning
FileUtils.getBookFullPath(bookEntity)is consistent with the refactor and removes unnecessary wrapping.booklore-api/src/main/java/org/booklore/service/reader/PdfReaderService.java (1)
116-116: Looks good for the Path migration.This removes redundant conversion and keeps
getBookPath(...)type-consistent.booklore-api/src/main/java/org/booklore/service/metadata/BookMetadataService.java (1)
200-200: CorrectPath -> Fileconversion at extractor boundaries.Using
.toFile()here is the right adaptation for the updatedFileUtilsAPI.Also applies to: 210-210
booklore-api/src/main/java/org/booklore/service/fileprocessor/Fb2Processor.java (1)
74-74: Nice update for the new path contract.
.toFile()is the correct conversion point for the cover extractor input.booklore-api/src/main/java/org/booklore/service/fileprocessor/AudiobookProcessor.java (1)
163-163: Good migration in the non-folder path branch.This is the expected conversion for APIs that still operate on
File.booklore-api/src/main/java/org/booklore/service/fileprocessor/Azw3Processor.java (1)
75-75: This conversion change is correct.Using
.toFile()here cleanly matches the updatedFileUtilsreturn type.booklore-api/src/main/java/org/booklore/service/fileprocessor/EpubProcessor.java (1)
76-76: Path migration is correctly applied in cover extraction.Line 76 cleanly consumes the new
Pathreturn type and avoids redundant conversions.booklore-api/src/main/java/org/booklore/service/email/SendEmailV2Service.java (1)
99-99: Attachment path handling is correctly updated toPath.Line 99 correctly converts the returned
PathtoFileat the API boundary that requiresFile.booklore-api/src/main/java/org/booklore/service/fileprocessor/MobiProcessor.java (1)
75-75:PathtoFileconversion is correctly placed.Line 75 applies the refactor cleanly with no extra normalization or string round-tripping.
booklore-api/src/main/java/org/booklore/service/fileprocessor/PdfProcessor.java (1)
77-77: PDF call sites are correctly updated for the newPathcontract.Lines 77 and 107 adapt to
FileUtils.getBookFullPath(...)returningPathwithout introducing extra conversion steps.Also applies to: 107-107
booklore-api/src/main/java/org/booklore/service/fileprocessor/CbxProcessor.java (1)
69-69: CBX path refactor is consistently applied at all relevant call sites.These updates correctly convert
PathtoFileonly where required by downstream APIs.Also applies to: 92-92, 255-255
booklore-api/src/main/java/org/booklore/service/book/BookDownloadService.java (1)
62-62: Download path handling now correctly usesPathend-to-end.Line 62 and Line 261 apply the refactor cleanly and remove unnecessary String/path conversion.
Also applies to: 261-261
booklore-api/src/main/java/org/booklore/service/reader/EpubReaderService.java (1)
220-220: DirectPathreturn ingetBookPath(...)is the right simplification.Line 220 removes an unnecessary conversion layer and keeps return types consistent across both branches.
booklore-api/src/test/java/org/booklore/service/reader/EpubReaderServiceTest.java (1)
113-113: Tests are correctly aligned with thePathreturn-type refactor.All updated mocks now return
Path, matching production behavior and keeping these unit tests accurate.Also applies to: 152-152, 180-180, 215-215, 241-241, 267-267, 302-302, 328-328, 356-356, 378-378, 399-399, 427-427, 456-456, 497-497
booklore-api/src/test/java/org/booklore/service/email/SendEmailV2ServiceTest.java (1)
23-23: Path-based mock migration looks correct.The updated import and static mock return values are consistent with the new
FileUtils.getBookFullPath(...)Pathcontract.Also applies to: 126-126, 196-196, 229-229
booklore-api/src/test/java/org/booklore/service/Rar5FallbackIntegrationTest.java (1)
77-77: Good update toPathreturn stubs.Returning
cbrCopydirectly is the correct adaptation for thePath-returningFileUtils.getBookFullPath(...).Also applies to: 99-99
booklore-api/src/test/java/org/booklore/util/FileUtilsTest.java (1)
45-45: Test assertion is correctly migrated toPathsemantics.Using
Pathfor both actual and expected values is the right change for this refactor.Also applies to: 47-49
booklore-api/src/test/java/org/booklore/service/reader/CbxReaderServiceTest.java (1)
66-66: Consistent and correctPath-based stub updates across reader tests.All affected
FileUtils.getBookFullPath(...)mocks now match the new method return type.Also applies to: 97-97, 144-144, 183-183, 212-212, 235-235, 264-264, 295-295, 308-308
booklore-api/src/test/java/org/booklore/service/book/BookServiceTest.java (1)
128-128: These migrated stubs are correct.The updated
FileUtils.getBookFullPath(...)mocks now correctly returnPathin the touched tests.Also applies to: 156-156, 332-332, 353-353
|
@imnotjames please make sure to align this with PR #82 -- we need to avoid merge conflicts if we can :) 👍 |
Sure thing. I did a quick test merge and there's currently two conflicts we can't easily avoid -- at the Because the change moves us away from |
imnotjames
force-pushed
the
refactor/29/use-path-for-get-book-full-path
branch
2 times, most recently
from
39100c8 to
f6acfd3
Compare
imnotjames
force-pushed
the
refactor/29/use-path-for-get-book-full-path
branch
from
f6acfd3 to
b1ff4e1
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
booklore-api/src/main/java/org/booklore/util/FileUtils.java (1)
27-38: Complete the Path refactoring in BookService to eliminate unnecessary String round-trips.The
FileUtils.getBookFullPath()refactor to returnPathis correct, butBookServicestill converts these Path objects to String and reparses them back to File. This pattern appears at lines 339, 341, 363, and 365, leaving the cleanup incomplete.♻️ Suggested refactor in
BookService- String filePath; + Path filePath; if (bookType != null) { BookFileType requestedType = BookFileType.valueOf(bookType.toUpperCase()); BookFileEntity bookFile = bookEntity.getBookFiles().stream() .filter(bf -> bf.getBookType() == requestedType) .findFirst() .orElseThrow(() -> ApiError.FILE_NOT_FOUND.createException("No file of type " + bookType + " found for book")); - filePath = bookFile.getFullFilePath().toString(); + filePath = bookFile.getFullFilePath(); } else { - filePath = FileUtils.getBookFullPath(bookEntity).toString(); + filePath = FileUtils.getBookFullPath(bookEntity); } - File file = new File(filePath); + File file = filePath.toFile(); if (!file.exists()) { - throw ApiError.FILE_NOT_FOUND.createException(filePath); + throw ApiError.FILE_NOT_FOUND.createException(filePath.toString()); }🤖 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/util/FileUtils.java` around lines 27 - 38, BookService still converts Path results from FileUtils.getBookFullPath(...) to String and back to File, undoing the Path refactor; update the call sites (the usages around the previous lines 339, 341, 363, 365) to accept and work with java.nio.file.Path directly instead of doing toString() and new File(...). Replace any File-constructing logic with Path-based APIs (Files.exists, Files.delete, Paths.get.resolve, Path.resolve, etc.) or use File.toPath()/Path.toFile() only when an API strictly requires java.io.File, and update method signatures/variables in BookService accordingly so no unnecessary String round-trips remain.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@booklore-api/src/main/java/org/booklore/util/FileUtils.java`:
- Around line 27-38: BookService still converts Path results from
FileUtils.getBookFullPath(...) to String and back to File, undoing the Path
refactor; update the call sites (the usages around the previous lines 339, 341,
363, 365) to accept and work with java.nio.file.Path directly instead of doing
toString() and new File(...). Replace any File-constructing logic with
Path-based APIs (Files.exists, Files.delete, Paths.get.resolve, Path.resolve,
etc.) or use File.toPath()/Path.toFile() only when an API strictly requires
java.io.File, and update method signatures/variables in BookService accordingly
so no unnecessary String round-trips remain.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f05decf7-cd58-4a74-9d4d-8377615a7791
📒 Files selected for processing (21)
booklore-api/src/main/java/org/booklore/service/book/BookDownloadService.javabooklore-api/src/main/java/org/booklore/service/book/BookService.javabooklore-api/src/main/java/org/booklore/service/email/SendEmailV2Service.javabooklore-api/src/main/java/org/booklore/service/fileprocessor/AudiobookProcessor.javabooklore-api/src/main/java/org/booklore/service/fileprocessor/Azw3Processor.javabooklore-api/src/main/java/org/booklore/service/fileprocessor/CbxProcessor.javabooklore-api/src/main/java/org/booklore/service/fileprocessor/EpubProcessor.javabooklore-api/src/main/java/org/booklore/service/fileprocessor/Fb2Processor.javabooklore-api/src/main/java/org/booklore/service/fileprocessor/MobiProcessor.javabooklore-api/src/main/java/org/booklore/service/fileprocessor/PdfProcessor.javabooklore-api/src/main/java/org/booklore/service/metadata/BookMetadataService.javabooklore-api/src/main/java/org/booklore/service/reader/CbxReaderService.javabooklore-api/src/main/java/org/booklore/service/reader/EpubReaderService.javabooklore-api/src/main/java/org/booklore/service/reader/PdfReaderService.javabooklore-api/src/main/java/org/booklore/util/FileUtils.javabooklore-api/src/test/java/org/booklore/service/Rar5FallbackIntegrationTest.javabooklore-api/src/test/java/org/booklore/service/book/BookServiceTest.javabooklore-api/src/test/java/org/booklore/service/email/SendEmailV2ServiceTest.javabooklore-api/src/test/java/org/booklore/service/reader/CbxReaderServiceTest.javabooklore-api/src/test/java/org/booklore/service/reader/EpubReaderServiceTest.javabooklore-api/src/test/java/org/booklore/util/FileUtilsTest.java
✅ Files skipped from review due to trivial changes (7)
- booklore-api/src/main/java/org/booklore/service/reader/PdfReaderService.java
- booklore-api/src/main/java/org/booklore/service/fileprocessor/AudiobookProcessor.java
- booklore-api/src/main/java/org/booklore/service/fileprocessor/Fb2Processor.java
- booklore-api/src/main/java/org/booklore/service/book/BookService.java
- booklore-api/src/test/java/org/booklore/service/reader/CbxReaderServiceTest.java
- booklore-api/src/main/java/org/booklore/service/fileprocessor/MobiProcessor.java
- booklore-api/src/main/java/org/booklore/service/metadata/BookMetadataService.java
🚧 Files skipped from review as they are similar to previous changes (7)
- booklore-api/src/main/java/org/booklore/service/reader/CbxReaderService.java
- booklore-api/src/main/java/org/booklore/service/fileprocessor/PdfProcessor.java
- booklore-api/src/test/java/org/booklore/service/email/SendEmailV2ServiceTest.java
- booklore-api/src/test/java/org/booklore/service/reader/EpubReaderServiceTest.java
- booklore-api/src/main/java/org/booklore/service/book/BookDownloadService.java
- booklore-api/src/test/java/org/booklore/service/book/BookServiceTest.java
- booklore-api/src/main/java/org/booklore/service/email/SendEmailV2Service.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: 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 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/reader/EpubReaderService.javabooklore-api/src/test/java/org/booklore/service/Rar5FallbackIntegrationTest.javabooklore-api/src/main/java/org/booklore/service/fileprocessor/CbxProcessor.javabooklore-api/src/main/java/org/booklore/service/fileprocessor/Azw3Processor.javabooklore-api/src/test/java/org/booklore/util/FileUtilsTest.javabooklore-api/src/main/java/org/booklore/util/FileUtils.javabooklore-api/src/main/java/org/booklore/service/fileprocessor/EpubProcessor.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/Rar5FallbackIntegrationTest.javabooklore-api/src/test/java/org/booklore/util/FileUtilsTest.java
🧠 Learnings (6)
📓 Common learnings
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.
📚 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/main/java/org/booklore/service/reader/EpubReaderService.javabooklore-api/src/test/java/org/booklore/service/Rar5FallbackIntegrationTest.javabooklore-api/src/main/java/org/booklore/service/fileprocessor/CbxProcessor.javabooklore-api/src/test/java/org/booklore/util/FileUtilsTest.javabooklore-api/src/main/java/org/booklore/util/FileUtils.java
📚 Learning: 2026-03-26T03:22:21.096Z
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:21.096Z
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/service/reader/EpubReaderService.javabooklore-api/src/main/java/org/booklore/util/FileUtils.java
📚 Learning: 2026-03-26T01:46:48.847Z
Learnt from: CR
Repo: grimmory-tools/grimmory PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-26T01:46:48.847Z
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/Rar5FallbackIntegrationTest.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/util/FileUtils.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/util/FileUtils.java
🔇 Additional comments (7)
booklore-api/src/main/java/org/booklore/util/FileUtils.java (1)
53-55: Good cleanup of the file-size path flow.Passing the
Paththrough directly removes the extra parse step cleanly.booklore-api/src/main/java/org/booklore/service/reader/EpubReaderService.java (1)
209-220: Nice removal of theString -> Pathround-trip.Returning the
PathfromFileUtilsdirectly keepsgetBookPath(...)aligned with the new API.booklore-api/src/main/java/org/booklore/service/fileprocessor/Azw3Processor.java (1)
73-76: Good directPathtoFilehandoff.This keeps the extractor boundary on
Filewithout reintroducing an intermediate string.booklore-api/src/main/java/org/booklore/service/fileprocessor/EpubProcessor.java (1)
74-77: Good directPathtoFilehandoff.This keeps the cover extraction path aligned with the new utility contract.
booklore-api/src/test/java/org/booklore/util/FileUtilsTest.java (1)
37-49: Good test update for the new return type.Asserting on
Pathdirectly avoids platform-specific separator handling in the test.booklore-api/src/main/java/org/booklore/service/fileprocessor/CbxProcessor.java (1)
67-69: Nice propagation of the newPathcontract through CBX processing.All three call sites now convert straight to
Fileat the integration boundary instead of round-tripping throughString.Also applies to: 91-93, 253-255
booklore-api/src/test/java/org/booklore/service/Rar5FallbackIntegrationTest.java (1)
75-77: Good static-stub alignment with production.Returning a
Pathhere keeps the fallback tests exercising the same contract as the reader code.Also applies to: 97-99
balazs-szucs
left a comment
balazs-szucs
left a comment
There was a problem hiding this comment.
Make sense thanks.
Pathing is generally bit a of pain Java, I feel like, easy to get wrong still using string is a... questionable code. This is miles better! Thank you
3 participants
Description
This change passes around a
Pathobject instead of a string inFileUtils.getBookFullPath(). IngetBookFullPathwe convert the path to a string, only to (in most cases) convert right back to a path. The conversion opens up room for error, and it's a heck of a lot simpler to just pass the path back and forth.For example, even in
FileUtils.getBookFullPath, we convert backslashes and then in other locations, we ALSO have to convert backslashes.Pathhandles these under the hood natively.This was originally part of #113 but I split it out because it seems beneficial on its own & helps with testing and reviewing the nightcompress library.
Linked Issue: Related to #29
Changes
FileUtils.getBookFullPath()to return ajava.nio.file.Pathinstead of aStringPathSummary by CodeRabbit