fix(api): use configurable upload limit for author photo / book cover#783
Conversation
📝 WalkthroughWalkthroughReplaces hardcoded 5MB upload limits with a configurable Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Service as AuthorMetadataService / BookCoverService
participant FileUtil as FileService
participant Settings as AppSettingService
participant Storage as ImageStore
Client->>Service: POST /upload (MultipartFile)
Service->>FileUtil: validateFile(file)
FileUtil->>Settings: getAppSettings()
Settings-->>FileUtil: AppSettings(maxFileUploadSizeInMb)
FileUtil-->>Service: validation result (ok / throw FILE_TOO_LARGE)
alt valid
Service->>Storage: decode & save image
Storage-->>Service: saved metadata
Service-->>Client: 200 OK
else invalid
Service-->>Client: error (FILE_TOO_LARGE / EMPTY_FILE)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
befd989 to
820c109
Compare
820c109 to
b808465
Compare
b808465 to
0c58ec0
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/src/test/java/org/booklore/util/FileServiceTest.java (1)
1034-1046:⚠️ Potential issue | 🟡 Minor
fileTooLarge_throwsRuntimeExceptionno longer stubs the upload size setting.Unlike the sibling tests in this nested class, this test does not stub
appSettingService.getAppSettings(). With the new production code,getMaxFileUploadSizeInMb()returnsnull→ the effective limit is0bytes, and the 6 MB payload is rejected by that zero-limit branch rather than exercising "6 MB exceeds a 5 MB configured limit" as the name implies. The assertion currently still passes only because the production error message is hard-coded to"5 MB".Stub
maxFileUploadSizeInMb(5)here too so the test actually validates the intended boundary.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/test/java/org/booklore/util/FileServiceTest.java` around lines 1034 - 1046, The test fileTooLarge_throwsRuntimeException is missing the app settings stub so getMaxFileUploadSizeInMb() returns null (effectively 0), causing the 6MB payload to be rejected by the zero-limit branch; fix by stubbing the app setting to a 5 MB limit before calling fileService.createThumbnailFromFile — e.g. invoke the same helper used in sibling tests (maxFileUploadSizeInMb(5)) or directly stub appSettingService.getAppSettings() so getMaxFileUploadSizeInMb() returns 5, then proceed with creating the 6MB MockMultipartFile and asserting the RuntimeException.
🧹 Nitpick comments (6)
backend/src/test/java/org/booklore/util/FileServiceTest.java (1)
973-1123: RepeatedappSettingServicestubbing could move into a nested@BeforeEach.Five tests in
CreateThumbnailFromFileTestseach inline the sameAppSettings.builder().maxFileUploadSizeInMb(5).build()stub. Lifting this to a@BeforeEachinsideCreateThumbnailFromFileTests(withlenient()since some tests don't reach validation) would reduce boilerplate and make intent clearer. Non-blocking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/test/java/org/booklore/util/FileServiceTest.java` around lines 973 - 1123, The repeated stubbing of appSettingService.getAppSettings() with AppSettings.builder().maxFileUploadSizeInMb(5).build() in the CreateThumbnailFromFileTests should be moved into a `@BeforeEach` inside that nested test class; add a `@BeforeEach` that calls when(appSettingService.getAppSettings()).thenReturn(...) (use Mockito.lenient() on the when(...) for tests that don't hit validation) so all tests in CreateThumbnailFromFileTests inherit the same maxFileUploadSizeInMb setting; update references in tests that asserted behavior to keep using fileService.createThumbnailFromFile(...) unchanged.backend/src/test/java/org/booklore/service/AuthorMetadataServiceTest.java (1)
45-69: Constructor wiring looks correct, but no new test exercisesvalidatePhoto.The new
appSettingServicemock is injected, but there is no test covering the new behavior inuploadAuthorPhoto— specifically the empty-file rejection and the oversize rejection paths added invalidatePhoto. Given this PR's main risk (see earlier comments on the0-default), a couple of focused tests here would meaningfully derisk regressions.Want me to draft
uploadAuthorPhoto_rejectsEmptyFileanduploadAuthorPhoto_rejectsFileLargerThanConfiguredLimittests mirroring the patterns inBookCoverServiceTest.FileValidation?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/test/java/org/booklore/service/AuthorMetadataServiceTest.java` around lines 45 - 69, Add two unit tests that exercise AuthorMetadataService.validatePhoto via uploadAuthorPhoto: create uploadAuthorPhoto_rejectsEmptyFile and uploadAuthorPhoto_rejectsFileLargerThanConfiguredLimit, using the existing appSettingService mock to return the configured max size (mirror BookCoverServiceTest.FileValidation patterns), construct an empty MultipartFile for the first test and a MultipartFile whose byte length exceeds the mocked max for the second, call AuthorMetadataService.uploadAuthorPhoto and assert the expected rejection (exception/validation error) is thrown; ensure you reuse the existing authenticationService lenient() setup and the AuthorMetadataService instance constructed in setUp so the tests target the new validatePhoto logic and the appSettingService wiring.backend/src/main/java/org/booklore/service/AuthorMetadataService.java (2)
246-246: Doc update needed:docs/authors.mdstill advertises a hard 5 MB limit.Per the linked-repositories context,
grimmory-tools/grimmory-docs→docs/authors.mdstates author photo uploads are capped at 5 MB. With this change, the cap is now whatever admins configure viamaxFileUploadSizeInMb(documented indocs/tools/global-preferences.md). Updatingdocs/authors.mdto reference the global setting would avoid user confusion once this PR ships.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/main/java/org/booklore/service/AuthorMetadataService.java` at line 246, Update docs/authors.md to remove the hardcoded "5 MB" photo upload limit and instead reference the configurable global setting maxFileUploadSizeInMb (as documented in docs/tools/global-preferences.md); mention that author photo validation now uses the application preference (see validatePhoto(file) in AuthorMetadataService) and provide instructions where admins can change the limit and how users will be informed of the current max size.
230-239: Author photo validation should include MIME type checks matchingBookCoverService.validateCoverFile.
validateCoverFilerejects non-JPEG/PNG uploads by detecting MIME type from the file content usingMimeDetector.detect(inputStream)before image processing. In contrast,validatePhotoonly validates empty and file size, allowing non-image payloads to reachFileService.readImage, where they surface as genericFILE_READ_ERRORrather than a clear validation error. Mirror the MIME check here for consistency and better error messaging, particularly since documentation claims "PNG or JPEG" support.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/main/java/org/booklore/service/AuthorMetadataService.java` around lines 230 - 239, The validatePhoto method in AuthorMetadataService currently only checks emptiness and size; update it to also detect the file MIME type using MimeDetector.detect(file.getInputStream()) (same approach as BookCoverService.validateCoverFile) and reject anything other than image/png or image/jpeg by throwing the appropriate ApiError (e.g., FILE_TYPE_NOT_ALLOWED or INVALID_INPUT) with a clear message so non-image payloads are rejected before reaching FileService.readImage; keep the existing size and empty-file checks and reuse the same allowed-type logic/symbols as in BookCoverService.validateCoverFile and MimeDetector.detect.backend/src/main/java/org/booklore/service/metadata/BookCoverService.java (1)
506-517: Duplicate helper across three services.This exact
getMaxFileUploadSizeMb()method (byte-for-byte) is now replicated inBookCoverService,FileService, andAuthorMetadataService. Extracting it — e.g. as a method onAppSettingService(getMaxFileUploadSizeMbOrDefault()) or a smallUploadSizeLimitProvider— would centralize the fallback policy and the warning log, and make any future change (e.g., treatingnullas unlimited) a one-line edit.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/main/java/org/booklore/service/metadata/BookCoverService.java` around lines 506 - 517, The getMaxFileUploadSizeMb() helper is duplicated verbatim in BookCoverService, FileService, and AuthorMetadataService; extract and centralize this logic by adding a single provider method (either add getMaxFileUploadSizeMbOrDefault() to AppSettingService or create a small UploadSizeLimitProvider) that fetches AppSettings, applies the null fallback and warning log, and returns a long; then replace the duplicated getMaxFileUploadSizeMb() implementations in BookCoverService, FileService, and AuthorMetadataService to call the new centralized method (refer to the existing method name getMaxFileUploadSizeMb(), AppSettingService, and the three services to locate spots to change).backend/src/main/java/org/booklore/util/FileService.java (1)
74-74:MAX_FILE_SIZE_BYTESis now dead code.
validateCoverFileno longer referencesMAX_FILE_SIZE_BYTES; the only remaining consumer of the size limit isgetMaxFileUploadSizeMb(). The constant can be removed to avoid confusion between the old hard limit and the new configured one.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/main/java/org/booklore/util/FileService.java` at line 74, Remove the now-unused MAX_FILE_SIZE_BYTES constant from FileService to avoid confusion: delete the declaration of MAX_FILE_SIZE_BYTES and any remaining references (the only legitimate size source should be getMaxFileUploadSizeMb()); ensure validateCoverFile and other methods rely on the configured max returned by getMaxFileUploadSizeMb() and that no other code expects the removed constant.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/src/main/java/org/booklore/service/metadata/BookCoverService.java`:
- Around line 506-517: getMaxFileUploadSizeMb currently returns 0 when
AppSettings.getMaxFileUploadSizeInMb() is null, which causes validateCoverFile
to reject all uploads; change getMaxFileUploadSizeMb to preserve the prior
sensible default instead of 0 — e.g., if maxFileUploadSizeMb is null, log a
warning that the setting is unset and return 5L (5 MB) so upload validation in
validateCoverFile continues to work; reference
AppSettings.getMaxFileUploadSizeInMb(), getMaxFileUploadSizeMb(), and
validateCoverFile when making the change and update the log message to reflect
the 5 MB fallback.
In `@backend/src/main/java/org/booklore/util/FileService.java`:
- Around line 216-220: The thrown IllegalArgumentException uses a hard-coded "5
MB" message while the limit is derived from getMaxFileUploadSizeMb(); update the
exception message in the FileService block that compares file.getSize() to
maxFileSize to interpolate the actual maxSizeMb value (or maxSizeMb + " MB") so
the error reflects the configured limit rather than a fixed 5 MB.
---
Outside diff comments:
In `@backend/src/test/java/org/booklore/util/FileServiceTest.java`:
- Around line 1034-1046: The test fileTooLarge_throwsRuntimeException is missing
the app settings stub so getMaxFileUploadSizeInMb() returns null (effectively
0), causing the 6MB payload to be rejected by the zero-limit branch; fix by
stubbing the app setting to a 5 MB limit before calling
fileService.createThumbnailFromFile — e.g. invoke the same helper used in
sibling tests (maxFileUploadSizeInMb(5)) or directly stub
appSettingService.getAppSettings() so getMaxFileUploadSizeInMb() returns 5, then
proceed with creating the 6MB MockMultipartFile and asserting the
RuntimeException.
---
Nitpick comments:
In `@backend/src/main/java/org/booklore/service/AuthorMetadataService.java`:
- Line 246: Update docs/authors.md to remove the hardcoded "5 MB" photo upload
limit and instead reference the configurable global setting
maxFileUploadSizeInMb (as documented in docs/tools/global-preferences.md);
mention that author photo validation now uses the application preference (see
validatePhoto(file) in AuthorMetadataService) and provide instructions where
admins can change the limit and how users will be informed of the current max
size.
- Around line 230-239: The validatePhoto method in AuthorMetadataService
currently only checks emptiness and size; update it to also detect the file MIME
type using MimeDetector.detect(file.getInputStream()) (same approach as
BookCoverService.validateCoverFile) and reject anything other than image/png or
image/jpeg by throwing the appropriate ApiError (e.g., FILE_TYPE_NOT_ALLOWED or
INVALID_INPUT) with a clear message so non-image payloads are rejected before
reaching FileService.readImage; keep the existing size and empty-file checks and
reuse the same allowed-type logic/symbols as in
BookCoverService.validateCoverFile and MimeDetector.detect.
In `@backend/src/main/java/org/booklore/service/metadata/BookCoverService.java`:
- Around line 506-517: The getMaxFileUploadSizeMb() helper is duplicated
verbatim in BookCoverService, FileService, and AuthorMetadataService; extract
and centralize this logic by adding a single provider method (either add
getMaxFileUploadSizeMbOrDefault() to AppSettingService or create a small
UploadSizeLimitProvider) that fetches AppSettings, applies the null fallback and
warning log, and returns a long; then replace the duplicated
getMaxFileUploadSizeMb() implementations in BookCoverService, FileService, and
AuthorMetadataService to call the new centralized method (refer to the existing
method name getMaxFileUploadSizeMb(), AppSettingService, and the three services
to locate spots to change).
In `@backend/src/main/java/org/booklore/util/FileService.java`:
- Line 74: Remove the now-unused MAX_FILE_SIZE_BYTES constant from FileService
to avoid confusion: delete the declaration of MAX_FILE_SIZE_BYTES and any
remaining references (the only legitimate size source should be
getMaxFileUploadSizeMb()); ensure validateCoverFile and other methods rely on
the configured max returned by getMaxFileUploadSizeMb() and that no other code
expects the removed constant.
In `@backend/src/test/java/org/booklore/service/AuthorMetadataServiceTest.java`:
- Around line 45-69: Add two unit tests that exercise
AuthorMetadataService.validatePhoto via uploadAuthorPhoto: create
uploadAuthorPhoto_rejectsEmptyFile and
uploadAuthorPhoto_rejectsFileLargerThanConfiguredLimit, using the existing
appSettingService mock to return the configured max size (mirror
BookCoverServiceTest.FileValidation patterns), construct an empty MultipartFile
for the first test and a MultipartFile whose byte length exceeds the mocked max
for the second, call AuthorMetadataService.uploadAuthorPhoto and assert the
expected rejection (exception/validation error) is thrown; ensure you reuse the
existing authenticationService lenient() setup and the AuthorMetadataService
instance constructed in setUp so the tests target the new validatePhoto logic
and the appSettingService wiring.
In `@backend/src/test/java/org/booklore/util/FileServiceTest.java`:
- Around line 973-1123: The repeated stubbing of
appSettingService.getAppSettings() with
AppSettings.builder().maxFileUploadSizeInMb(5).build() in the
CreateThumbnailFromFileTests should be moved into a `@BeforeEach` inside that
nested test class; add a `@BeforeEach` that calls
when(appSettingService.getAppSettings()).thenReturn(...) (use Mockito.lenient()
on the when(...) for tests that don't hit validation) so all tests in
CreateThumbnailFromFileTests inherit the same maxFileUploadSizeInMb setting;
update references in tests that asserted behavior to keep using
fileService.createThumbnailFromFile(...) unchanged.
🪄 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: 47f08b77-5d9d-488c-a06e-af0571b02f15
📒 Files selected for processing (6)
backend/src/main/java/org/booklore/service/AuthorMetadataService.javabackend/src/main/java/org/booklore/service/metadata/BookCoverService.javabackend/src/main/java/org/booklore/util/FileService.javabackend/src/test/java/org/booklore/service/AuthorMetadataServiceTest.javabackend/src/test/java/org/booklore/service/metadata/BookCoverServiceTest.javabackend/src/test/java/org/booklore/util/FileServiceTest.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: Packaging Smoke Test
🧰 Additional context used
📓 Path-based instructions (2)
backend/src/**/*.java
📄 CodeRabbit inference engine (AGENTS.md)
backend/src/**/*.java: Use 4-space indentation and match surrounding Java style in backend code
Prefer constructor injection via Lombok patterns already used in the codebase. Do not introduce@Autowiredfield injection in backend code
Use MapStruct for entity/DTO mapping in backend code
Keep JPA entities on the *Entity suffix in backend code
Files:
backend/src/test/java/org/booklore/service/AuthorMetadataServiceTest.javabackend/src/main/java/org/booklore/service/metadata/BookCoverService.javabackend/src/main/java/org/booklore/util/FileService.javabackend/src/main/java/org/booklore/service/AuthorMetadataService.javabackend/src/test/java/org/booklore/util/FileServiceTest.javabackend/src/test/java/org/booklore/service/metadata/BookCoverServiceTest.java
backend/src/test/**/*.java
📄 CodeRabbit inference engine (AGENTS.md)
Prefer focused unit tests; use
@SpringBootTestonly when the Spring context is required in backend code
Files:
backend/src/test/java/org/booklore/service/AuthorMetadataServiceTest.javabackend/src/test/java/org/booklore/util/FileServiceTest.javabackend/src/test/java/org/booklore/service/metadata/BookCoverServiceTest.java
🧠 Learnings (3)
📚 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:
backend/src/test/java/org/booklore/service/AuthorMetadataServiceTest.javabackend/src/main/java/org/booklore/service/metadata/BookCoverService.javabackend/src/main/java/org/booklore/util/FileService.javabackend/src/main/java/org/booklore/service/AuthorMetadataService.javabackend/src/test/java/org/booklore/util/FileServiceTest.javabackend/src/test/java/org/booklore/service/metadata/BookCoverServiceTest.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:
backend/src/test/java/org/booklore/service/AuthorMetadataServiceTest.javabackend/src/main/java/org/booklore/service/metadata/BookCoverService.javabackend/src/main/java/org/booklore/util/FileService.javabackend/src/main/java/org/booklore/service/AuthorMetadataService.javabackend/src/test/java/org/booklore/util/FileServiceTest.javabackend/src/test/java/org/booklore/service/metadata/BookCoverServiceTest.java
📚 Learning: 2026-04-04T15:36:56.558Z
Learnt from: balazs-szucs
Repo: grimmory-tools/grimmory PR: 372
File: booklore-api/src/main/java/org/booklore/app/service/AppBookService.java:357-359
Timestamp: 2026-04-04T15:36:56.558Z
Learning: In `booklore-api/src/main/java/org/booklore/app/service/AppBookService.java`, `shelfId` and `magicShelfId` are mutually exclusive navigation contexts in `getFilterOptions`. A request will never supply both at the same time, so the `else if (shelfId != null)` branch after the `magicBookIds != null` check is intentional and correct — there is no missing shelf-∩-magicShelf intersection case. Do not flag this pattern as a bug.
Applied to files:
backend/src/test/java/org/booklore/service/metadata/BookCoverServiceTest.java
🔀 Multi-repo context grimmory-tools/grimmory-docs
[::grimmory-tools/grimmory-docs::] docs/tools/global-preferences.md — documents "Max File Upload Size" (1000 MB) and notes the setting takes effect after restart. This is the user-facing setting the PR reads (maxFileUploadSizeInMb).
[::grimmory-tools/grimmory-docs::] docs/authors.md — states author photo upload limit "PNG or JPEG, max 5 MB". This contradicts the PR change to use the configurable maxFileUploadSizeInMb and should be updated to reflect the configurable limit or reference the global setting.
[::grimmory-tools/grimmory-docs::] docs/tools/custom-fonts.md and docs/reader-preferences.md — contain references to "Max file size: 5 MB" for custom fonts. These are not directly changed by the PR (which targets author photo and covers) but are places where hard-coded 5 MB limits appear in docs and may need review for consistency with configurable upload limits.
Conclusion: docs/tools/global-preferences.md already exposes the relevant setting used by the PR. User-facing documentation files (notably docs/authors.md) still claim a 5 MB hard limit and should be updated to avoid confusing users now that author photo/book cover limits are configurable.
🔇 Additional comments (3)
backend/src/main/java/org/booklore/util/FileService.java (1)
191-202: Same concern as inBookCoverService.getMaxFileUploadSizeMb:0default silently blocks all uploads, and this helper is duplicated across three services.See the comment on
BookCoverService.javalines 506–517 for the root-cause discussion and the suggested centralization intoAppSettingService. Once that is done, this method can be deleted.backend/src/main/java/org/booklore/service/AuthorMetadataService.java (1)
217-228: Same0-default/duplication issue as inBookCoverServiceandFileService.See the primary comment on
BookCoverService.javalines 506–517. Recommend centralizing this helper and choosing a safer fallback than0to avoid unintentionally blocking all author photo uploads when the setting is unset.backend/src/test/java/org/booklore/service/metadata/BookCoverServiceTest.java (1)
60-60: LGTM — test now aligns with the configurable size behavior.Hoisting
@Mock AppSettings appSettingsto the class level and stubbingappSettingService.getAppSettings()/appSettings.getMaxFileUploadSizeInMb()in the individual validation tests cleanly mirrors the production change, and renamingrejectsFileLargerThan5MB→rejectsFileLargerThanLimitmatches the new semantics.Also applies to: 398-505
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
backend/src/main/java/org/booklore/util/FileService.java (1)
74-74: Remove now-unusedMAX_FILE_SIZE_BYTESconstant.With
validateCoverFilenow deriving the limit fromgetMaxFileUploadSizeMb(), the static5L * 1024 * 1024constant at line 74 has no remaining references and should be removed to avoid future confusion about which limit is authoritative.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/main/java/org/booklore/util/FileService.java` at line 74, Remove the now-unused constant MAX_FILE_SIZE_BYTES from the FileService class: delete the declaration "private static final long MAX_FILE_SIZE_BYTES = 5L * 1024 * 1024;" so the class no longer contains a stale hard-coded limit; ensure no remaining references to MAX_FILE_SIZE_BYTES exist (validateCoverFile and getMaxFileUploadSizeMb are the authoritative places for file size limits) and run a compile to confirm removal.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/src/main/java/org/booklore/util/FileService.java`:
- Around line 191-202: Replace the silent defaulting logic in
getMaxFileUploadSizeMb(): when AppSettings.getMaxFileUploadSizeInMb() returns
null treat it as a fatal configuration error and throw an IllegalStateException
with a descriptive message (include context like the AppSettings instance or
that server default is 100 MB) instead of returning 0L; update the same behavior
in related helpers used by BookCoverService and AuthorMetadataService so null
indicates broken/corrupt configuration and surfaces a 5xx rather than a
FILE_TOO_LARGE 400.
---
Nitpick comments:
In `@backend/src/main/java/org/booklore/util/FileService.java`:
- Line 74: Remove the now-unused constant MAX_FILE_SIZE_BYTES from the
FileService class: delete the declaration "private static final long
MAX_FILE_SIZE_BYTES = 5L * 1024 * 1024;" so the class no longer contains a stale
hard-coded limit; ensure no remaining references to MAX_FILE_SIZE_BYTES exist
(validateCoverFile and getMaxFileUploadSizeMb are the authoritative places for
file size limits) and run a compile to confirm removal.
🪄 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: 9c8efc68-04d9-4a8d-9889-a3e698fc7edb
📒 Files selected for processing (2)
backend/src/main/java/org/booklore/util/FileService.javabackend/src/test/java/org/booklore/util/FileServiceTest.java
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/src/test/java/org/booklore/util/FileServiceTest.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). (5)
- GitHub Check: Test Suite / Frontend Tests
- GitHub Check: Test Suite / Backend Tests
- GitHub Check: Analyze (java-kotlin)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Frontend Lint Threshold Check
🧰 Additional context used
📓 Path-based instructions (1)
backend/src/**/*.java
📄 CodeRabbit inference engine (AGENTS.md)
backend/src/**/*.java: Use 4-space indentation and match surrounding Java style in backend code
Prefer constructor injection via Lombok patterns already used in the codebase. Do not introduce@Autowiredfield injection in backend code
Use MapStruct for entity/DTO mapping in backend code
Keep JPA entities on the *Entity suffix in backend code
Files:
backend/src/main/java/org/booklore/util/FileService.java
🧠 Learnings (4)
📓 Common learnings
Learnt from: imnotjames
Repo: grimmory-tools/grimmory PR: 783
File: backend/src/main/java/org/booklore/service/metadata/BookCoverService.java:506-517
Timestamp: 2026-04-24T05:15:32.360Z
Learning: In grimmory-tools/grimmory, `AppSettings.getMaxFileUploadSizeInMb()` has a server-side default of 100 MB. If it is null at runtime, that indicates a broken/corrupt configuration state — the preferred behavior is to throw an `IllegalStateException` (not silently default to 0 or 5 MB) so that the caller receives a clear server error rather than a misleading file-size rejection. This applies in `BookCoverService`, `FileService`, and `AuthorMetadataService` wherever `getMaxFileUploadSizeMb()` helpers are used.
📚 Learning: 2026-04-24T05:15:32.360Z
Learnt from: imnotjames
Repo: grimmory-tools/grimmory PR: 783
File: backend/src/main/java/org/booklore/service/metadata/BookCoverService.java:506-517
Timestamp: 2026-04-24T05:15:32.360Z
Learning: When using `AppSettings.getMaxFileUploadSizeInMb()` / `getMaxFileUploadSizeMb()` in `BookCoverService`, `FileService`, and `AuthorMetadataService`, do not silently substitute a fallback value (e.g., 0 or 5 MB) when the setting is `null` at runtime. Treat `null` as a broken/corrupt server configuration state and throw an `IllegalStateException` so the caller receives a clear server error instead of a misleading file-size rejection.
Applied to files:
backend/src/main/java/org/booklore/util/FileService.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:
backend/src/main/java/org/booklore/util/FileService.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:
backend/src/main/java/org/booklore/util/FileService.java
🔀 Multi-repo context grimmory-tools/grimmory-docs
Linked repositories findings
[::grimmory-tools/grimmory-docs::]
- docs/authors.md: author photo upload description still says "PNG or JPEG, max 5 MB" (line ~88). This contradicts the PR change to use configurable maxFileUploadSizeInMb and should be updated.
- docs/tools/custom-fonts.md: several places reference "Max file size: 5 MB" for custom fonts and a troubleshooting table entry "File size exceeds 5 MB". These are unrelated to cover/author-photo uploads but are other user-facing 5 MB references to consider for consistency.
- docs/reader-preferences.md: table lists "Maximum file size per font | 5 MB" (line ~127) — same as above.
- docs/tools/global-preferences.md: documents the server setting "Max File Upload Size" and default "1000 MB" (includes note that changes take effect after restart). This is the setting the PR reads (maxFileUploadSizeInMb) and confirms the docs location that should be referenced/linked from author/cover upload guidance.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
backend/src/main/java/org/booklore/service/metadata/BookCoverService.java (1)
506-517: Consider extracting this helper to avoid duplication across services.This method is a near-verbatim copy of
FileService.getMaxFileUploadSizeMb()(and per the PR's scope, a similar helper exists/will exist inAuthorMetadataService). Consider hoisting it intoAppSettingService(e.g.getMaxFileUploadSizeMbOrThrow()) so the null-check and error contract live in one place — any future change (fallback policy, log wording, error mapping) then applies everywhere automatically.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/main/java/org/booklore/service/metadata/BookCoverService.java` around lines 506 - 517, The getMaxFileUploadSizeMb method in BookCoverService is duplicated across services (e.g., FileService.getMaxFileUploadSizeMb and similar code in AuthorMetadataService); extract this logic into AppSettingService as a single helper (suggest name getMaxFileUploadSizeMbOrThrow) that performs the null check, logs and throws the ApiError, and returns a long; then update BookCoverService.getMaxFileUploadSizeMb (and FileService/AuthorMetadataService) to call AppSettingService.getMaxFileUploadSizeMbOrThrow(), removing the duplicated null-check/log/throw logic so behavior is centralized.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@backend/src/main/java/org/booklore/service/metadata/BookCoverService.java`:
- Around line 506-517: The getMaxFileUploadSizeMb method in BookCoverService is
duplicated across services (e.g., FileService.getMaxFileUploadSizeMb and similar
code in AuthorMetadataService); extract this logic into AppSettingService as a
single helper (suggest name getMaxFileUploadSizeMbOrThrow) that performs the
null check, logs and throws the ApiError, and returns a long; then update
BookCoverService.getMaxFileUploadSizeMb (and FileService/AuthorMetadataService)
to call AppSettingService.getMaxFileUploadSizeMbOrThrow(), removing the
duplicated null-check/log/throw logic so behavior is centralized.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e41e0638-a825-4e3d-a765-7affb908dd0e
📒 Files selected for processing (2)
backend/src/main/java/org/booklore/service/metadata/BookCoverService.javabackend/src/main/java/org/booklore/util/FileService.java
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/src/main/java/org/booklore/util/FileService.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). (5)
- GitHub Check: Test Suite / Frontend Tests
- GitHub Check: Test Suite / Backend Tests
- GitHub Check: Frontend Lint Threshold Check
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (java-kotlin)
🧰 Additional context used
📓 Path-based instructions (1)
backend/src/**/*.java
📄 CodeRabbit inference engine (AGENTS.md)
backend/src/**/*.java: Use 4-space indentation and match surrounding Java style in backend code
Prefer constructor injection via Lombok patterns already used in the codebase. Do not introduce@Autowiredfield injection in backend code
Use MapStruct for entity/DTO mapping in backend code
Keep JPA entities on the *Entity suffix in backend code
Files:
backend/src/main/java/org/booklore/service/metadata/BookCoverService.java
🧠 Learnings (11)
📓 Common learnings
Learnt from: imnotjames
Repo: grimmory-tools/grimmory PR: 783
File: backend/src/main/java/org/booklore/service/metadata/BookCoverService.java:506-517
Timestamp: 2026-04-24T05:15:32.360Z
Learning: In grimmory-tools/grimmory, `AppSettings.getMaxFileUploadSizeInMb()` has a server-side default of 100 MB. If it is null at runtime, that indicates a broken/corrupt configuration state — the preferred behavior is to throw an `IllegalStateException` (not silently default to 0 or 5 MB) so that the caller receives a clear server error rather than a misleading file-size rejection. This applies in `BookCoverService`, `FileService`, and `AuthorMetadataService` wherever `getMaxFileUploadSizeMb()` helpers are used.
📚 Learning: 2026-04-24T05:15:32.360Z
Learnt from: imnotjames
Repo: grimmory-tools/grimmory PR: 783
File: backend/src/main/java/org/booklore/service/metadata/BookCoverService.java:506-517
Timestamp: 2026-04-24T05:15:32.360Z
Learning: When using `AppSettings.getMaxFileUploadSizeInMb()` / `getMaxFileUploadSizeMb()` in `BookCoverService`, `FileService`, and `AuthorMetadataService`, do not silently substitute a fallback value (e.g., 0 or 5 MB) when the setting is `null` at runtime. Treat `null` as a broken/corrupt server configuration state and throw an `IllegalStateException` so the caller receives a clear server error instead of a misleading file-size rejection.
Applied to files:
backend/src/main/java/org/booklore/service/metadata/BookCoverService.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:
backend/src/main/java/org/booklore/service/metadata/BookCoverService.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:
backend/src/main/java/org/booklore/service/metadata/BookCoverService.java
📚 Learning: 2026-04-14T12:46:59.296Z
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:59.296Z
Learning: In grimmory-tools/grimmory (booklore-api), the team targets Java 25 with `--enable-preview` and actively aims to use the most modern Java conventions available. When reviewing or suggesting code for this project, always prefer modern Java 25 idioms and features (e.g., unnamed variables `_`, records, pattern matching for switch/instanceof, sealed classes, sequenced collections, `Stream.toList()`, text blocks, etc.) over older or more conservative patterns. Do not suggest reverting modern constructs to legacy alternatives.
Applied to files:
backend/src/main/java/org/booklore/service/metadata/BookCoverService.java
📚 Learning: 2026-04-19T01:05:27.934Z
Learnt from: imnotjames
Repo: grimmory-tools/grimmory PR: 507
File: booklore-api/src/main/java/org/booklore/service/kobo/KoboServerProxy.java:57-59
Timestamp: 2026-04-19T01:05:27.934Z
Learning: In `booklore-api/src/main/java/org/booklore/service/kobo/KoboServerProxy.java`, the `null → "{}"` body fallback in `executeProxyRequest()` (using `BodyPublishers.ofString("{}")` even for null bodies/GET requests) is pre-existing behavior that has not caused issues in practice. Do not suggest changing this to `BodyPublishers.noBody()` without evidence of an actual problem, as the behavior against the real Kobo upstream API is untested and unknown.
Applied to files:
backend/src/main/java/org/booklore/service/metadata/BookCoverService.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:
backend/src/main/java/org/booklore/service/metadata/BookCoverService.java
📚 Learning: 2026-04-23T14:49:01.089Z
Learnt from: balazs-szucs
Repo: grimmory-tools/grimmory PR: 790
File: backend/src/main/java/org/booklore/nativelib/NativeLibraries.java:71-77
Timestamp: 2026-04-23T14:49:01.089Z
Learning: In grimmory-tools/grimmory, the `catch (Throwable)` in `NativeLibraries.runProbe()` (`backend/src/main/java/org/booklore/nativelib/NativeLibraries.java`) is intentional design: any failure during native library probing (including fatal JVM errors) is treated as "library unavailable" rather than propagating. Do not flag this as an issue in future reviews.
Applied to files:
backend/src/main/java/org/booklore/service/metadata/BookCoverService.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:
backend/src/main/java/org/booklore/service/metadata/BookCoverService.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:
backend/src/main/java/org/booklore/service/metadata/BookCoverService.java
📚 Learning: 2026-04-04T14:02:30.887Z
Learnt from: balazs-szucs
Repo: grimmory-tools/grimmory PR: 372
File: Dockerfile:72-73
Timestamp: 2026-04-04T14:02:30.887Z
Learning: In the grimmory project (grimmory-tools/grimmory), enabling heap dumps by default in the Dockerfile via `-XX:+HeapDumpOnOutOfMemoryError` and `-XX:HeapDumpPath=/tmp/heapdump.hprof` in `JAVA_TOOL_OPTIONS` is the intentional team standard. Do not flag this as an issue in future reviews.
Applied to files:
backend/src/main/java/org/booklore/service/metadata/BookCoverService.java
🔀 Multi-repo context grimmory-tools/grimmory-docs
Linked repositories findings
[::grimmory-tools/grimmory-docs::]
- docs/authors.md — "Photo options" lists "Upload a file from your device (PNG or JPEG, max 5 MB)". This directly contradicts the PR which moves author photo limit to the configurable maxFileUploadSizeInMb and should be updated. (docs/authors.md, section "Photo options")
- docs/tools/custom-fonts.md — multiple places state "Max file size: 5 MB per font" and a troubleshooting row "File size exceeds 5 MB". These are other user-facing 5 MB references (fonts) that are not about covers/photos but may be reviewed for consistency. (docs/tools/custom-fonts.md, limits & troubleshooting)
- docs/reader-preferences.md — table shows "Maximum file size per font | 5 MB". (docs/reader-preferences.md, "Custom Font Library / Limits")
- docs/tools/global-preferences.md — documents the server setting "Max File Upload Size" with default "1000 MB" and notes that changes require a server restart. This is the config the PR reads (maxFileUploadSizeInMb) and should be referenced in UI/help text for uploads. (docs/tools/global-preferences.md, "Max File Upload Size" entry)
Summary: documentation contains hardcoded "5 MB" limits for author photos and fonts that conflict with the PR's change to use the configurable maxFileUploadSizeInMb; global-preferences documents the server setting used by the PR. These docs should be updated to avoid user confusion.
🔇 Additional comments (2)
backend/src/main/java/org/booklore/service/metadata/BookCoverService.java (2)
506-517: Null-handling aligns with the discussed approach.Throwing
ApiError.INTERNAL_SERVER_ERRORhere correctly treats an unsetmaxFileUploadSizeInMbas a broken server-config state (it has a 100 MB default inAppSettingService) rather than silently falling back to 0/5 MB and producing a misleading "File size exceeds the limit" response. Mirrors the helper inFileService.getMaxFileUploadSizeMb()for consistency.
519-527: Validation change looks correct.Conversion
maxSizeMb * 1024 * 1024is evaluated aslong(first operand islong), so no overflow risk for any realistic configured value, andFILE_TOO_LARGE's%dformat accepts thelongargument. Error message now correctly reflects the configured limit.
Description
Previously, the file upload limit for author photos and book covers defaulted to 5mb. However, some users (for some reason) want humongous book covers and requested to have this limit increased.
This change updates the author photo and book cover systems to use the server setting
maxFileUploadSizeInMbto determine the expected maximum file size for an upload.Linked Issue: Fixes #781
Changes
maxFileUploadSizeInMbsettingmaxFileUploadSizeInMbsettingSummary by CodeRabbit