refactor(api): use spring content disposition helper for BookDownloadService#449
Conversation
📝 WalkthroughWalkthroughReplaced manual Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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 |
190f3c2 to
e7895c9
Compare
|
Forgot, needs tests. I'll add those in the morning |
e7895c9 to
fc9bdcb
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
booklore-api/src/main/java/org/booklore/service/book/BookDownloadService.java (1)
131-147: Add focused header-format tests for this helper before merge.This refactor changes the on-the-wire
Content-Dispositionformat. Please add tests for ASCII filenames, non-ASCII filenames, and zip-name paths to lock expected header behavior and prevent regressions.If you want, I can draft the exact JUnit test cases for
BookDownloadServiceheader assertions.🤖 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/BookDownloadService.java` around lines 131 - 147, Add focused JUnit tests covering the getContentDisposition(String)/getContentDisposition(File)/getContentDisposition(Path) helpers to lock the on-the-wire Content-Disposition format: create three tests that assert the returned header string for (1) a simple ASCII filename ("book.pdf"), (2) a non-ASCII filename (e.g., "файл.pdf") verifies the fallbackFilename (NON_ASCII_PATTERN replacement) appears as the filename parameter and the original UTF-8 filename appears in the filename* or encoded-UTF-8 form, and (3) a zip path/fileName scenario (Path with nested names or a .zip name) to ensure path-derived getContentDisposition(Path) returns the same expected header; use the BookDownloadService.getContentDisposition(...) overloads and assert exact ContentDisposition.builder output string to prevent regressions.
🤖 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/book/BookDownloadService.java`:
- Around line 139-145: In getContentDisposition, the call
.filename(fallbackFilename) is overwritten by the subsequent .filename(filename,
StandardCharsets.UTF_8) so the ASCII fallback never appears; either remove the
first .filename(fallbackFilename) and keep only .filename(filename,
StandardCharsets.UTF_8) for UTF‑8/RFC5987-only output, or remove the UTF‑8
overload and keep only .filename(fallbackFilename) (using NON_ASCII_PATTERN) to
provide an ASCII fallback for older clients; update the
ContentDisposition.builder(...) chain accordingly and delete the now-dead
fallbackFilename variable if you choose the UTF‑8-only route.
---
Nitpick comments:
In
`@booklore-api/src/main/java/org/booklore/service/book/BookDownloadService.java`:
- Around line 131-147: Add focused JUnit tests covering the
getContentDisposition(String)/getContentDisposition(File)/getContentDisposition(Path)
helpers to lock the on-the-wire Content-Disposition format: create three tests
that assert the returned header string for (1) a simple ASCII filename
("book.pdf"), (2) a non-ASCII filename (e.g., "файл.pdf") verifies the
fallbackFilename (NON_ASCII_PATTERN replacement) appears as the filename
parameter and the original UTF-8 filename appears in the filename* or
encoded-UTF-8 form, and (3) a zip path/fileName scenario (Path with nested names
or a .zip name) to ensure path-derived getContentDisposition(Path) returns the
same expected header; use the BookDownloadService.getContentDisposition(...)
overloads and assert exact ContentDisposition.builder output string to prevent
regressions.
🪄 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: 0504e62f-fc01-4170-bc81-63915d5f95a4
📒 Files selected for processing (1)
booklore-api/src/main/java/org/booklore/service/book/BookDownloadService.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: Test Suite / Backend Tests
- 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/service/book/BookDownloadService.java
🧠 Learnings (1)
📓 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.
🔇 Additional comments (1)
booklore-api/src/main/java/org/booklore/service/book/BookDownloadService.java (1)
81-82: Nice consolidation ofContent-Dispositionusage.Good call centralizing header generation through
getContentDisposition(...); it removes duplicated string assembly across endpoints and keeps behavior consistent.Also applies to: 120-121, 190-191, 303-304, 351-352
e53c019 to
a4e3db7
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
booklore-api/src/test/java/org/booklore/service/book/BookDownloadServiceTest.java (1)
72-87: Make the UTF-8 assertion less brittle to Spring serialization details.Asserting the entire header literal couples the test to Spring's
ContentDisposition#toString()formatting, which is not guaranteed to produce identical RFC 2047 encoded-words across patch/minor versions. Spring has modified encoding logic in patches (e.g., bug fix 6.2.8, RFC 2047 removal in 7.0.7), invalidating hardcoded expectations.Assert decoded filename and
filename*presence instead:Proposed refactor
+import org.springframework.http.ContentDisposition; +import static org.junit.jupiter.api.Assertions.assertTrue; ... - String expected = "attachment; filename=\"=?UTF-8?Q?=C9=87xample.epub?=\"; filename*=UTF-8''%C9%87xample.epub"; ... String actual = bookDownloadService.downloadBook(1L) .getHeaders() .getFirst("Content-Disposition"); - assertEquals(expected, actual); + ContentDisposition disposition = ContentDisposition.parse(actual); + assertEquals("attachment", disposition.getType()); + assertEquals("ɇxample.epub", disposition.getFilename()); + assertTrue(actual.contains("filename*="));🤖 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/BookDownloadServiceTest.java` around lines 72 - 87, The test downloadBook_includesContentDispositionUTF8 is brittle because it asserts the full Content-Disposition string; update it to extract the Content-Disposition header returned by bookDownloadService.downloadBook(1L), parse or decode the header to verify the decoded filename equals the original filename from getSampleBook("ɇxample.epub") and assert that a filename* parameter (percent-encoded UTF-8) is present; keep the existing setup (bookRepository mock, mockFiles) but replace the exact-string assertEquals with checks for the decoded filename match and presence of filename* to avoid coupling to Spring's toString() formatting.
🤖 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/test/java/org/booklore/service/book/BookDownloadServiceTest.java`:
- Around line 72-87: The test downloadBook_includesContentDispositionUTF8 is
brittle because it asserts the full Content-Disposition string; update it to
extract the Content-Disposition header returned by
bookDownloadService.downloadBook(1L), parse or decode the header to verify the
decoded filename equals the original filename from getSampleBook("ɇxample.epub")
and assert that a filename* parameter (percent-encoded UTF-8) is present; keep
the existing setup (bookRepository mock, mockFiles) but replace the exact-string
assertEquals with checks for the decoded filename match and presence of
filename* to avoid coupling to Spring's toString() formatting.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 57201605-427a-42ac-beb6-4303f5d137b9
📒 Files selected for processing (2)
booklore-api/src/main/java/org/booklore/service/book/BookDownloadService.javabooklore-api/src/test/java/org/booklore/service/book/BookDownloadServiceTest.java
🚧 Files skipped from review as they are similar to previous changes (1)
- booklore-api/src/main/java/org/booklore/service/book/BookDownloadService.java
📜 Review details
🧰 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/book/BookDownloadServiceTest.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/book/BookDownloadServiceTest.java
🧠 Learnings (4)
📓 Common learnings
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:42.597Z
Learning: In grimmory-tools/grimmory, Spring's `ContentDisposition.builder(...).filename(name, StandardCharsets.UTF_8).build().toString()` emits **both** a quoted-printable `filename=` parameter (e.g., `filename="=?UTF-8?Q?...?="`) and an RFC 5987 `filename*=` parameter in the header value. An explicit ASCII fallback computed via `NON_ASCII_PATTERN` before calling `.filename(fallbackFilename)` is therefore redundant when UTF-8 is specified. (Confirmed in BookDownloadService.java, PR `#449`.)
📚 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/book/BookDownloadServiceTest.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/book/BookDownloadServiceTest.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/book/BookDownloadServiceTest.java
🔇 Additional comments (3)
booklore-api/src/test/java/org/booklore/service/book/BookDownloadServiceTest.java (3)
27-53: Focused unit-test scaffolding is solid.Using
MockitoExtension+ direct mocks (without loading Spring context) is the right scope for this service-level behavior test.As per coding guidelines: "Prefer focused unit tests; use
@SpringBootTestonly when the Spring context is required".
55-70: Good coverage of the ASCII disposition path.This test cleanly validates the ASCII branch end-to-end at the response-header level.
89-109: Fixture helper is well-formed for this unit scope.The explicit back-reference setup keeps entity graph assumptions consistent with service usage.
a4e3db7 to
ec1b0c4
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/test/java/org/booklore/service/book/BookDownloadServiceTest.java`:
- Around line 62-63: The test stubs for Files.exists/Files.isDirectory in
BookDownloadServiceTest do not match the actual file path used by downloadBook
because fileSubPath("/subpath") produces an absolute segment and downloadBook
first calls FileUtils.requirePathWithinBase(...) before checking Files.*; update
the fixtures so the mocked path equals the resolved path used by production code
— either create the test path by passing the same base through
FileUtils.requirePathWithinBase (or by generating a relative fileSubPath instead
of an absolute one) and then use that exact Path in mockFiles.when(...) for
Files.exists and Files.isDirectory, ensuring all mocks (the ones around
mockFiles.when(...) and later stubs at the other locations) use the same
resolved Path value.
🪄 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: 6e31348d-eb9e-46c2-8b47-d9be05faad56
📒 Files selected for processing (2)
booklore-api/src/main/java/org/booklore/service/book/BookDownloadService.javabooklore-api/src/test/java/org/booklore/service/book/BookDownloadServiceTest.java
🚧 Files skipped from review as they are similar to previous changes (1)
- booklore-api/src/main/java/org/booklore/service/book/BookDownloadService.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 / Backend Tests
- GitHub Check: Test Suite / Frontend 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 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/book/BookDownloadServiceTest.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/book/BookDownloadServiceTest.java
🧠 Learnings (3)
📓 Common learnings
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:42.597Z
Learning: In grimmory-tools/grimmory, Spring's `ContentDisposition.builder(...).filename(name, StandardCharsets.UTF_8).build().toString()` emits **both** a quoted-printable `filename=` parameter (e.g., `filename="=?UTF-8?Q?...?="`) and an RFC 5987 `filename*=` parameter in the header value. An explicit ASCII fallback computed via `NON_ASCII_PATTERN` before calling `.filename(fallbackFilename)` is therefore redundant when UTF-8 is specified. (Confirmed in BookDownloadService.java, PR `#449`.)
📚 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/book/BookDownloadServiceTest.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/book/BookDownloadServiceTest.java
🔇 Additional comments (1)
booklore-api/src/test/java/org/booklore/service/book/BookDownloadServiceTest.java (1)
27-53: Good test setup scope (focused unit tests).Using
@ExtendWith(MockitoExtension.class)with mocked collaborators keeps these tests fast and isolated without unnecessary Spring context startup.As per coding guidelines: "Prefer focused unit tests; use
@SpringBootTestonly when the Spring context is required".
|
Good to go for review. |
Description
While working through the issue in #429, I found that the content disposition that was being generated didn't use the spring content disposition helper
added tests & did manual testing via the dev docker compose
Linked Issue: Related to #429
Changes
replaces our own content disposition generation with
org.springframework.http.ContentDispositionSummary by CodeRabbit
Bug Fixes
Tests