refactor(metadata): replace Spring JSON processor with Jackson for leaner prod image#905
Merged
balazs-szucs merged 7 commits intoMay 3, 2026
Conversation
Contributor
|
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:
WalkthroughRemoves a Gradle implementation dependency and migrates JSON parsing in metadata code from Spring's JSONObject/JSONArray to Jackson's ObjectMapper/JsonNode, adding node-shape and null-safety checks, refactoring calibre user-metadata extraction, and adding a test for array-form moods. Changes
Sequence Diagram(s)(omitted — changes are internal parsing/refactors not involving a multi-component sequential flow) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
🚥 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
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
backend/src/main/java/org/booklore/service/metadata/extractor/EpubMetadataExtractor.java (1)
245-245: Variable naming.
jsonrootdeviates from camelCase used elsewhere in this file (e.g.valueNode,builderMeta). Rename tojsonRoot.🤖 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/extractor/EpubMetadataExtractor.java` at line 245, The local variable name jsonroot in EpubMetadataExtractor should be renamed to jsonRoot to follow camelCase conventions used elsewhere (e.g., valueNode, builderMeta); update the declaration and all usages within the method where OBJECT_MAPPER.readTree(content) is assigned to that variable so references (reads, traversals, checks) use jsonRoot consistently and compile without changing behavior.backend/src/main/java/org/booklore/service/metadata/parser/GoodReadsParser.java (2)
437-444: Defensive null-check onbookJsonto avoid relying on the broad catch.
apolloStateJson.get(key)can in principle returnnull; while the surroundingtry/catchwould convert any NPE into a logged error, it’s cleaner (and avoids a noisy stacktrace) to skip explicitly.♻️ Proposed refactor
if (key.contains("Book:kca:")) { JsonNode bookJson = apolloStateJson.get(key); - String title = bookJson.path("title").asText(null); + if (bookJson == null) continue; + String title = bookJson.path("title").asText(null); if (title != null && !title.isEmpty()) { return bookJson; } }🤖 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/parser/GoodReadsParser.java` around lines 437 - 444, In GoodReadsParser (backend/src/main/java/org/booklore/service/metadata/parser/GoodReadsParser.java) the snippet that reads JsonNode bookJson = apolloStateJson.get(key) should defensively check bookJson for null before calling bookJson.path(...) so we don't rely on the surrounding try/catch to hide a NullPointerException; modify the block that handles keys containing "Book:kca:" to skip/continue when bookJson == null (or otherwise handle the null) before evaluating title, referencing apolloStateJson, bookJson, key and the "Book:kca:" match.
424-432: SimplifygetJsonKeysusing the Jackson 3 collection API.
ObjectNode.propertyNames()already returns a collection of names; you can populate theLinkedHashSetdirectly instead of looping. Minor readability improvement.♻️ Proposed refactor
private LinkedHashSet<String> getJsonKeys(JsonNode apolloStateJson) { - LinkedHashSet<String> keySet = new LinkedHashSet<>(); - if (apolloStateJson instanceof ObjectNode objectNode) { - for (String fieldName : objectNode.propertyNames()) { - keySet.add(fieldName); - } - } - return keySet; + if (apolloStateJson instanceof ObjectNode objectNode) { + return new LinkedHashSet<>(objectNode.propertyNames()); + } + return new LinkedHashSet<>(); }Please confirm
ObjectNode.propertyNames()in Jackson 3 (tools.jackson.databind.node.ObjectNode) returns aCollection<String>rather than onlyIterable<String>; if it’s onlyIterable, the original loop should be kept.🤖 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/parser/GoodReadsParser.java` around lines 424 - 432, The getJsonKeys method can be simplified by using ObjectNode.propertyNames() directly to initialize the LinkedHashSet instead of iterating; update getJsonKeys(JsonNode apolloStateJson) to cast to ObjectNode (as currently) and then do something like new LinkedHashSet<>(objectNode.propertyNames()) if propertyNames() returns a Collection<String>; if propertyNames() is only an Iterable<String> in your Jackson version, keep the explicit loop (or use StreamSupport to convert the Iterable to a Collection) to preserve behavior. Ensure you reference getJsonKeys and ObjectNode.propertyNames() when making the change.
🤖 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/extractor/EpubMetadataExtractor.java`:
- Around line 244-252: The code currently calls OBJECT_MAPPER.readTree(content)
for calibre:user_metadata and calibre:user_metadata:`#pagecount` without local
try/catch, letting JacksonException abort the whole EpubMetadataExtractor run;
wrap each OBJECT_MAPPER.readTree(content) call in a small try/catch that catches
JacksonException (or IOException), logs a debug/warn with the field name and
content snippet, and returns/continues so only that field is skipped; ensure the
calibre:user_metadata:`#pagecount` branch still calls
safeParseInt(builderMeta::pageCount) only when parsing succeeded, and the
calibre:user_metadata branch only calls extractCalibreUserMetadata(parsedJson,
builderMeta, moods, tags) on a non-null JsonNode.
In
`@backend/src/main/java/org/booklore/service/metadata/parser/GoodReadsParser.java`:
- Around line 287-288: The fields followersCount and textReviewsCount in
GoodReadsParser are being set using userJson.has(...) combined with
path(...).asInt(), which treats explicit JSON null as 0; update the assignment
to only convert when the node is an integer (e.g., use
userJson.path("followersCount").canConvertToInt() or
userJson.path("followersCount").isInt() and similarly for "textReviewsCount")
and otherwise leave the Integer fields null; modify the logic around
followersCount and textReviewsCount assignments in GoodReadsParser so you check
the node type before calling asInt().
- Line 300: The code currently calls reviewJson.path("updatedAt").asLong() which
returns 0 for missing nodes and leads parseEpochMillis to produce epoch
1970-01-01; change the logic in GoodReadsParser to check the JsonNode for
presence before converting: obtain the node via reviewJson.path("updatedAt") (or
get("updatedAt")), call .isMissingNode() (or .isNull()) and only call .asLong()
and parseEpochMillis when the node exists; otherwise pass null to the .date(...)
setter. Reference symbols: GoodReadsParser, reviewJson.path("updatedAt"),
.isMissingNode(), and parseEpochMillis.
---
Nitpick comments:
In
`@backend/src/main/java/org/booklore/service/metadata/extractor/EpubMetadataExtractor.java`:
- Line 245: The local variable name jsonroot in EpubMetadataExtractor should be
renamed to jsonRoot to follow camelCase conventions used elsewhere (e.g.,
valueNode, builderMeta); update the declaration and all usages within the method
where OBJECT_MAPPER.readTree(content) is assigned to that variable so references
(reads, traversals, checks) use jsonRoot consistently and compile without
changing behavior.
In
`@backend/src/main/java/org/booklore/service/metadata/parser/GoodReadsParser.java`:
- Around line 437-444: In GoodReadsParser
(backend/src/main/java/org/booklore/service/metadata/parser/GoodReadsParser.java)
the snippet that reads JsonNode bookJson = apolloStateJson.get(key) should
defensively check bookJson for null before calling bookJson.path(...) so we
don't rely on the surrounding try/catch to hide a NullPointerException; modify
the block that handles keys containing "Book:kca:" to skip/continue when
bookJson == null (or otherwise handle the null) before evaluating title,
referencing apolloStateJson, bookJson, key and the "Book:kca:" match.
- Around line 424-432: The getJsonKeys method can be simplified by using
ObjectNode.propertyNames() directly to initialize the LinkedHashSet instead of
iterating; update getJsonKeys(JsonNode apolloStateJson) to cast to ObjectNode
(as currently) and then do something like new
LinkedHashSet<>(objectNode.propertyNames()) if propertyNames() returns a
Collection<String>; if propertyNames() is only an Iterable<String> in your
Jackson version, keep the explicit loop (or use StreamSupport to convert the
Iterable to a Collection) to preserve behavior. Ensure you reference getJsonKeys
and ObjectNode.propertyNames() when making the change.
🪄 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: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: f53a4bb8-5206-4f89-8ff8-688aefeb027e
📒 Files selected for processing (3)
backend/build.gradle.ktsbackend/src/main/java/org/booklore/service/metadata/extractor/EpubMetadataExtractor.javabackend/src/main/java/org/booklore/service/metadata/parser/GoodReadsParser.java
💤 Files with no reviewable changes (1)
- backend/build.gradle.kts
📜 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/service/metadata/extractor/EpubMetadataExtractor.javabackend/src/main/java/org/booklore/service/metadata/parser/GoodReadsParser.java
🧠 Learnings (5)
📚 Learning: 2026-04-02T09:25:37.417Z
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:37.417Z
Learning: When reviewing epub-related file processing code in this project, remember that grimmory-tools/grimmory uses the `org.grimmory.epub4j` library (from grimmory-tools/epub4j) and that it is a grimmory-maintained fork fundamentally different from `io.documentnode.epub4j`. Do not assume the same API behavior, limitations, or semantics as `io.documentnode.epub4j`. Before flagging issues or proposing changes, verify the current API/capabilities in the grimmory-tools/epub4j repository.
Applied to files:
backend/src/main/java/org/booklore/service/metadata/extractor/EpubMetadataExtractor.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/extractor/EpubMetadataExtractor.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/extractor/EpubMetadataExtractor.javabackend/src/main/java/org/booklore/service/metadata/parser/GoodReadsParser.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/extractor/EpubMetadataExtractor.javabackend/src/main/java/org/booklore/service/metadata/parser/GoodReadsParser.java
📚 Learning: 2026-04-02T09:25:48.330Z
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, epub processing uses `org.grimmory.epub4j` from https://github.com/grimmory-tools/epub4j. This is a grimmory-maintained fork and is fundamentally different from `io.documentnode.epub4j`. Do NOT generalize API knowledge, behavior, or limitations of `io.documentnode.epub4j` to the grimmory fork. Before raising review comments on epub-related file processing code, verify the current API and capabilities in the grimmory-tools/epub4j repository.
Applied to files:
backend/src/main/java/org/booklore/service/metadata/parser/GoodReadsParser.java
🔀 Multi-repo context grimmory-tools/grimmory-docs
grimmory-tools/grimmory-docs
-
docs/metadata/metadata-fetch-configuration.md — multiple references to GoodReads as a metadata provider and examples showing GoodReads preferred for descriptions/ratings/series data. [::grimmory-tools/grimmory-docs::docs/metadata/metadata-fetch-configuration.md:20,36,105,210,237,250]
-
README.md, docs/* — docs site contains many end-user references to metadata providers (GoodReads) and fields such as pageCount and ratings that the backend metadata parsers populate; no code-level consumers found in this repo. Examples: README.md top-level and multiple docs pages describe metadata flows and features driven by fetched metadata. [::grimmory-tools/grimmory-docs::README.md and various docs/*]
Observation
- The docs reference GoodReads and metadata fields (e.g., page count, ratings) that are parsed by backend classes updated in this PR, but there are no code consumers in this repository that depend on the specific JSON parsing implementation (JSONObject/JSONArray vs Jackson). Documentation text may need no change; no breaking cross-repo API/contract issues were discovered here.
🔇 Additional comments (1)
backend/src/main/java/org/booklore/service/metadata/extractor/EpubMetadataExtractor.java (1)
446-476: No changes needed — moods and tags are correctly extracted from Calibre user metadata. The#value#field is always a string in the JSON structure (e.g.,"[\"dark\", \"atmospheric\"]"), not an actual array.JsonNode.asText()on this TextNode returns the string value, whichparseJsonArrayOrCsv()correctly parses. Existing tests confirm the functionality works as intended.
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/src/main/java/org/booklore/service/metadata/parser/GoodReadsParser.java (1)
475-490:⚠️ Potential issue | 🟡 MinorMinor: empty/
"null"strings can leak into the genres set.When a
bookGenres[i]element is missing thegenreobject, orgenre.nameis absent/JSON null,genreJson.path("name").asText()returns""(or"null"for an explicitNullNode) and that value is added to the set. The pre-JacksonJSONObject.getStringwould have thrown and been caught, leaving the entry out. Filter blanks/"null"to preserve the previous behavior.🧹 Proposed fix
for (int i = 0; i < bookGenresJsonArray.size(); i++) { JsonNode genreJson = bookGenresJsonArray.get(i).path("genre"); - genres.add(genreJson.path("name").asText()); + String name = genreJson.path("name").asText(null); + if (StringUtils.isNotBlank(name) && !"null".equals(name)) { + genres.add(name); + } }🤖 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/parser/GoodReadsParser.java` around lines 475 - 490, The extractGenres method can add empty or literal "null" strings into the genres set when genre or genre.name is missing; update extractGenres to read the name via genreJson.path("name").asText(), then only add it to the genres Set<String> if the resulting name is non-empty (trim().length()>0) and not equalIgnoreCase("null"); keep using the existing variables (bookJson, bookGenresJsonArray, genreJson) and ensure you still return the genres set (not null) on normal flow, while preserving the existing exception handling.
🧹 Nitpick comments (2)
backend/src/main/java/org/booklore/service/metadata/parser/GoodReadsParser.java (2)
302-302: Optional: avoid traversingpath("updatedAt")twice.Tiny readability / micro-perf nit — extract once and reuse:
♻️ Suggested refactor
+ JsonNode updatedAtNode = reviewJson.path("updatedAt"); BookReview review = BookReview.builder() .metadataProvider(MetadataProvider.GoodReads) - .date(reviewJson.path("updatedAt").isIntegralNumber() ? parseEpochMillis(String.valueOf(reviewJson.path("updatedAt").asLong())) : null) + .date(updatedAtNode.isIntegralNumber() ? Instant.ofEpochMilli(updatedAtNode.asLong()) : null)Note: since the node is already known to be an integral number, you can call
Instant.ofEpochMillidirectly and skip theString.valueOf→Long.parseLonground-trip inparseEpochMillis.🤖 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/parser/GoodReadsParser.java` at line 302, In GoodReadsParser (method building the review .date(...)), avoid calling reviewJson.path("updatedAt") twice and the String→parse round-trip: extract JsonNode updatedAt = reviewJson.path("updatedAt"), test updatedAt.isIntegralNumber(), and then pass updatedAt.asLong() directly to parseEpochMillis (or to Instant.ofEpochMilli if you prefer) when calling .date(...); this replaces the current .date(reviewJson.path("updatedAt").isIntegralNumber() ? parseEpochMillis(String.valueOf(reviewJson.path("updatedAt").asLong())) : null) with a single, reused updatedAt reference and a direct long value.
51-51: UseJsonMapper.builder().build()for Jackson 3.x consistency.
new ObjectMapper()still works for default settings in Jackson 3.x, but the recommended idiom for thetools.jackson.*API is the format-specific builder (JsonMapper.builder().build()ornew JsonMapper()). Same applies toEpubMetadataExtractor#OBJECT_MAPPER. This is optional and can be deferred if minimizing diff scope is preferred.🤖 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/parser/GoodReadsParser.java` at line 51, Replace the ObjectMapper instantiation in GoodReadsParser by using Jackson 3.x's recommended API: replace the private static final ObjectMapper OBJECT_MAPPER = new ObjectMapper(); with creation via JsonMapper (e.g., JsonMapper.builder().build() or new JsonMapper()) and do the same consistent change for EpubMetadataExtractor#OBJECT_MAPPER so both classes use the JsonMapper builder idiom instead of new ObjectMapper().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In
`@backend/src/main/java/org/booklore/service/metadata/parser/GoodReadsParser.java`:
- Around line 475-490: The extractGenres method can add empty or literal "null"
strings into the genres set when genre or genre.name is missing; update
extractGenres to read the name via genreJson.path("name").asText(), then only
add it to the genres Set<String> if the resulting name is non-empty
(trim().length()>0) and not equalIgnoreCase("null"); keep using the existing
variables (bookJson, bookGenresJsonArray, genreJson) and ensure you still return
the genres set (not null) on normal flow, while preserving the existing
exception handling.
---
Nitpick comments:
In
`@backend/src/main/java/org/booklore/service/metadata/parser/GoodReadsParser.java`:
- Line 302: In GoodReadsParser (method building the review .date(...)), avoid
calling reviewJson.path("updatedAt") twice and the String→parse round-trip:
extract JsonNode updatedAt = reviewJson.path("updatedAt"), test
updatedAt.isIntegralNumber(), and then pass updatedAt.asLong() directly to
parseEpochMillis (or to Instant.ofEpochMilli if you prefer) when calling
.date(...); this replaces the current
.date(reviewJson.path("updatedAt").isIntegralNumber() ?
parseEpochMillis(String.valueOf(reviewJson.path("updatedAt").asLong())) : null)
with a single, reused updatedAt reference and a direct long value.
- Line 51: Replace the ObjectMapper instantiation in GoodReadsParser by using
Jackson 3.x's recommended API: replace the private static final ObjectMapper
OBJECT_MAPPER = new ObjectMapper(); with creation via JsonMapper (e.g.,
JsonMapper.builder().build() or new JsonMapper()) and do the same consistent
change for EpubMetadataExtractor#OBJECT_MAPPER so both classes use the
JsonMapper builder idiom instead of new ObjectMapper().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 3fa8d0a8-3ea2-4085-a587-98718ecbf01e
📒 Files selected for processing (2)
backend/src/main/java/org/booklore/service/metadata/extractor/EpubMetadataExtractor.javabackend/src/main/java/org/booklore/service/metadata/parser/GoodReadsParser.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). (4)
- GitHub Check: Test Suite / Backend Tests
- GitHub Check: Test Suite / Frontend Tests
- GitHub Check: Analyze (java-kotlin)
- GitHub Check: Analyze (javascript-typescript)
🧰 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/extractor/EpubMetadataExtractor.javabackend/src/main/java/org/booklore/service/metadata/parser/GoodReadsParser.java
🧠 Learnings (10)
📓 Common learnings
Learnt from: balazs-szucs
Repo: grimmory-tools/grimmory PR: 334
File: booklore-api/src/main/java/org/booklore/service/reader/EpubReaderService.java:402-407
Timestamp: 2026-04-02T09:25:48.330Z
Learning: In grimmory-tools/grimmory, before commenting on any file processing code (epub or pdf), always verify the current state of the relevant upstream grimmory libraries: `grimmory-tools/epub4j` (for epub) and `grimmory-tools/PDFium4j` (for pdf). These custom libraries may have different APIs, capabilities, and limitations compared to the third-party libraries they replace. Issues about streaming, buffering, or API surface may need to be filed/addressed in those upstream repos rather than in grimmory itself.
📚 Learning: 2026-04-02T09:25:37.417Z
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:37.417Z
Learning: When reviewing epub-related file processing code in this project, remember that grimmory-tools/grimmory uses the `org.grimmory.epub4j` library (from grimmory-tools/epub4j) and that it is a grimmory-maintained fork fundamentally different from `io.documentnode.epub4j`. Do not assume the same API behavior, limitations, or semantics as `io.documentnode.epub4j`. Before flagging issues or proposing changes, verify the current API/capabilities in the grimmory-tools/epub4j repository.
Applied to files:
backend/src/main/java/org/booklore/service/metadata/extractor/EpubMetadataExtractor.java
📚 Learning: 2026-04-02T09:25:48.330Z
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.
Applied to files:
backend/src/main/java/org/booklore/service/metadata/extractor/EpubMetadataExtractor.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/extractor/EpubMetadataExtractor.javabackend/src/main/java/org/booklore/service/metadata/parser/GoodReadsParser.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/extractor/EpubMetadataExtractor.java
📚 Learning: 2026-04-24T05:28:14.493Z
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:28:14.493Z
Learning: In grimmory-tools/grimmory, when `AppSettings.getMaxFileUploadSizeInMb()` returns null in `BookCoverService.getMaxFileUploadSizeMb()`, `FileService`, and `AuthorMetadataService`, the preferred behavior is to emit a warning log and return `0L` (which causes all uploads to be rejected via the size check), rather than throwing an exception. The rationale is that the application's error handler strips 5xx response detail anyway, so a throw gives the user no more actionable information than a size-check rejection, and an admin warning in the log is needed in either case. Do not suggest replacing this pattern with a thrown exception.
Applied to files:
backend/src/main/java/org/booklore/service/metadata/extractor/EpubMetadataExtractor.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/extractor/EpubMetadataExtractor.javabackend/src/main/java/org/booklore/service/metadata/parser/GoodReadsParser.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/extractor/EpubMetadataExtractor.javabackend/src/main/java/org/booklore/service/metadata/parser/GoodReadsParser.java
📚 Learning: 2026-04-02T09:25:48.330Z
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, epub processing uses `org.grimmory.epub4j` from https://github.com/grimmory-tools/epub4j. This is a grimmory-maintained fork and is fundamentally different from `io.documentnode.epub4j`. Do NOT generalize API knowledge, behavior, or limitations of `io.documentnode.epub4j` to the grimmory fork. Before raising review comments on epub-related file processing code, verify the current API and capabilities in the grimmory-tools/epub4j repository.
Applied to files:
backend/src/main/java/org/booklore/service/metadata/parser/GoodReadsParser.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/parser/GoodReadsParser.java
🔀 Multi-repo context grimmory-tools/grimmory-docs
Findings for grimmory-tools/grimmory-docs
- Docs reference metadata fields and providers parsed by the backend (GoodReads, pageCount, ratings, moods, tags, sidecar JSON examples), showing feature-level coupling but no code-level consumers or build/dependency ties to spring JSON or Jackson.
- docs/metadata/metadata-fetch-configuration.md — mentions GoodReads provider, Goodreads ID/Rating/Review Count and general metadata-provider behavior. [::grimmory-tools/grimmory-docs::docs/metadata/metadata-fetch-configuration.md:20,34,36,105,210,215,237,250,263]
- docs/magic-shelf.md — many references to pageCount and Goodreads-based rules/fields (e.g., pageCount usages, goodreadsRating, goodreadsReviewCount). [::grimmory-tools/grimmory-docs::docs/magic-shelf.md:11,37,66,104,105,205,447-448,1428,1436,1462,1486,1502,1510,1579-1580,1588,1618,1641,1692,1716,1753,1761]
- docs/metadata/sidecar-files.md — sidecar example contains "pageCount": 350 (shows expected JSON shape). [::grimmory-tools/grimmory-docs::docs/metadata/sidecar-files.md:30]
- README.md and other docs — high-level descriptions referencing metadata providers and UI features that depend on backend-parsed fields (ratings, pages, external IDs). [::grimmory-tools/grimmory-docs::README.md:1-120]
- No occurrences found of build files, Gradle config, or code that import or depend on org.springframework.boot.configurationprocessor.json or directly on Jackson in this repo; the repository is documentation-only and thus not impacted by the dependency change in backend/build.gradle.kts.
Conclusion: Documentation references metadata fields parsed by the modified backend code (GoodReads, pageCount, ratings, moods/tags, sidecar JSON), but there are no code-level consumers or dependency links in this repo that would break due to replacing Spring's JSON classes with Jackson. Reviewers should be aware docs reference the same fields and may remain accurate; no cross-repo code updates appear necessary here.
🔇 Additional comments (3)
backend/src/main/java/org/booklore/service/metadata/extractor/EpubMetadataExtractor.java (2)
244-260: LGTM — calibre payload errors are now field-local.Wrapping each
OBJECT_MAPPER.readTree(content)call in its owntry/catchcorrectly addresses the prior concern: a malformedcalibre:user_metadata/calibre:user_metadata:#pagecount`` blob is now logged at debug and skipped, and the rest of the metadata (title, authors, ISBN, etc.) continues to be extracted instead of the wholeBookMetadatabeing dropped.
454-484: Calibre user-metadata iteration looks correct on Jackson 3.x.Use of
instanceof ObjectNode,properties()forMap.Entry<String, JsonNode>traversal, the!fieldObj.isObject()shape guard, and thevalueNode.isNull()short-circuit are all idiomatic Jackson 3.x. The per-fieldtry/catchkeeps a single bad sub-field from polluting moods/tags/CALIBRE_FIELD_MAPPINGS extraction. ThefieldObj == nullcheck is technically redundant (Jackson'sproperties()never yields null values, onlyNullNode), but it's harmless.backend/src/main/java/org/booklore/service/metadata/parser/GoodReadsParser.java (1)
287-291: Prior follower/review-count andupdatedAtconcerns are addressed.
canConvertToInt()correctly guards against JSONnull/non-integer values being silently coerced to0forfollowersCount/textReviewsCount, and theisIntegralNumber()check onupdatedAtprevents the missing-node-defaults-to-epoch-1970 regression.Also applies to: 302-302
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/src/main/java/org/booklore/service/metadata/parser/GoodReadsParser.java (1)
468-483:⚠️ Potential issue | 🟡 MinorSkip blank genre names to avoid polluting the categories set.
genreJson.path("name").asText()returns""for missing or non-textual nodes; that empty string is then added unconditionally to thegenresset and ultimately surfaces as an empty-named category. Also worth guarding against a missingbookGenres[i]element returning a missing node before navigating togenre.♻️ Proposed fix
- JsonNode bookGenresJsonArray = bookJson.get("bookGenres"); - if (bookGenresJsonArray != null && bookGenresJsonArray.isArray()) { - for (int i = 0; i < bookGenresJsonArray.size(); i++) { - JsonNode genreJson = bookGenresJsonArray.get(i).path("genre"); - genres.add(genreJson.path("name").asText()); - } - } + JsonNode bookGenresJsonArray = bookJson.get("bookGenres"); + if (bookGenresJsonArray != null && bookGenresJsonArray.isArray()) { + for (JsonNode entry : bookGenresJsonArray) { + String name = entry.path("genre").path("name").asText(null); + if (name != null && !name.isBlank()) { + genres.add(name); + } + } + }🤖 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/parser/GoodReadsParser.java` around lines 468 - 483, In extractGenres, avoid adding empty or missing genre names and guard against missing bookGenres elements: when iterating bookGenresJsonArray in extractGenres(JsonNode bookJson), first ensure each element is non-missing, then obtain the genre node and its name via genreJson.path("name").asText(), trim it and only add to the genres Set if the resulting string is non-empty; also change the error path to return an empty Set (not null) after logging so callers always get a Set. Ensure you reference the existing extractGenres method and the local variables bookGenresJsonArray, genreJson, and genres when making these checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In
`@backend/src/main/java/org/booklore/service/metadata/parser/GoodReadsParser.java`:
- Around line 468-483: In extractGenres, avoid adding empty or missing genre
names and guard against missing bookGenres elements: when iterating
bookGenresJsonArray in extractGenres(JsonNode bookJson), first ensure each
element is non-missing, then obtain the genre node and its name via
genreJson.path("name").asText(), trim it and only add to the genres Set if the
resulting string is non-empty; also change the error path to return an empty Set
(not null) after logging so callers always get a Set. Ensure you reference the
existing extractGenres method and the local variables bookGenresJsonArray,
genreJson, and genres when making these checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: d199e87b-a326-41ff-a0a5-33702bce81f8
📒 Files selected for processing (2)
backend/src/main/java/org/booklore/service/metadata/extractor/EpubMetadataExtractor.javabackend/src/main/java/org/booklore/service/metadata/parser/GoodReadsParser.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 (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/extractor/EpubMetadataExtractor.javabackend/src/main/java/org/booklore/service/metadata/parser/GoodReadsParser.java
🧠 Learnings (10)
📓 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.
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.
📚 Learning: 2026-04-02T09:25:37.417Z
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:37.417Z
Learning: When reviewing epub-related file processing code in this project, remember that grimmory-tools/grimmory uses the `org.grimmory.epub4j` library (from grimmory-tools/epub4j) and that it is a grimmory-maintained fork fundamentally different from `io.documentnode.epub4j`. Do not assume the same API behavior, limitations, or semantics as `io.documentnode.epub4j`. Before flagging issues or proposing changes, verify the current API/capabilities in the grimmory-tools/epub4j repository.
Applied to files:
backend/src/main/java/org/booklore/service/metadata/extractor/EpubMetadataExtractor.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/extractor/EpubMetadataExtractor.javabackend/src/main/java/org/booklore/service/metadata/parser/GoodReadsParser.java
📚 Learning: 2026-04-02T09:25:48.330Z
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.
Applied to files:
backend/src/main/java/org/booklore/service/metadata/extractor/EpubMetadataExtractor.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/extractor/EpubMetadataExtractor.java
📚 Learning: 2026-04-24T05:28:14.493Z
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:28:14.493Z
Learning: In grimmory-tools/grimmory, when `AppSettings.getMaxFileUploadSizeInMb()` returns null in `BookCoverService.getMaxFileUploadSizeMb()`, `FileService`, and `AuthorMetadataService`, the preferred behavior is to emit a warning log and return `0L` (which causes all uploads to be rejected via the size check), rather than throwing an exception. The rationale is that the application's error handler strips 5xx response detail anyway, so a throw gives the user no more actionable information than a size-check rejection, and an admin warning in the log is needed in either case. Do not suggest replacing this pattern with a thrown exception.
Applied to files:
backend/src/main/java/org/booklore/service/metadata/extractor/EpubMetadataExtractor.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/extractor/EpubMetadataExtractor.javabackend/src/main/java/org/booklore/service/metadata/parser/GoodReadsParser.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/extractor/EpubMetadataExtractor.javabackend/src/main/java/org/booklore/service/metadata/parser/GoodReadsParser.java
📚 Learning: 2026-04-02T09:25:48.330Z
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, epub processing uses `org.grimmory.epub4j` from https://github.com/grimmory-tools/epub4j. This is a grimmory-maintained fork and is fundamentally different from `io.documentnode.epub4j`. Do NOT generalize API knowledge, behavior, or limitations of `io.documentnode.epub4j` to the grimmory fork. Before raising review comments on epub-related file processing code, verify the current API and capabilities in the grimmory-tools/epub4j repository.
Applied to files:
backend/src/main/java/org/booklore/service/metadata/parser/GoodReadsParser.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/parser/GoodReadsParser.java
🔀 Multi-repo context grimmory-tools/grimmory-docs
Linked repositories findings
grimmory-tools/grimmory-docs
- Docs reference the metadata fields and providers parsed/produced by the modified backend (Goodreads, pageCount, moods, tags, sidecar JSON format). These are feature-level consumers (UI/docs) of fields produced by EpubMetadataExtractor and GoodReadsParser, but there are no code-level integrations or build/dependency links to the backend JSON libraries.
- docs/metadata/sidecar-files.md: sidecar JSON example includes "pageCount": 350 [::grimmory-tools/grimmory-docs::docs/metadata/sidecar-files.md:30]
- docs/magic-shelf.md: many rules and examples reference pageCount, moods, tags, goodreadsRating, goodreadsReviewCount (multiple occurrences, e.g. lines ~11, 63–66, 104–105, 117–118, 447–448, 1436–1437) [::grimmory-tools/grimmory-docs::docs/magic-shelf.md:11,63-66,104-105,117-118,447-448,1436-1437]
- docs/metadata/metadata-fetch-configuration.md and docs/metadata/metadata-settings.md: describe GoodReads as a provider and list provider-specific fields (Goodreads ID/Rating/Review Count, moods, tags) [::grimmory-tools/grimmory-docs::docs/metadata/metadata-fetch-configuration.md:36,105][::grimmory-tools/grimmory-docs::docs/metadata/metadata-settings.md:62-64,113,136]
- docs/metadata/metadata-center.md and docs/view-preferences.md: reference display/use of Goodreads ratings/review counts and sidecar file behavior [::grimmory-tools/grimmory-docs::docs/metadata/metadata-center.md:21,147,197-199][::grimmory-tools/grimmory-docs::docs/view-preferences.md:55-56]
Interpretation:
- The PR's switch from Spring JSONObject/JSONArray to Jackson is an internal backend change; documentation references the same JSON field names and provider features and therefore do not require updates.
- There are no code consumers in this repo that import or rely on org.springframework.boot.configurationprocessor.json or on backend build/dependencies; docs-only coupling means no cross-repo code changes are needed.
🔇 Additional comments (3)
backend/src/main/java/org/booklore/service/metadata/parser/GoodReadsParser.java (1)
52-52: Migration looks solid; node-shape and null guards are well placed.
JsonMapper.builder().build()is the correct Jackson 3 entry point, andapolloStateJson.isMissingNode()early-exits avoid the historical NPE risk from the previous JSONObject API. ThecanConvertToInt()/isIntegralNumber()guards onfollowersCount/textReviewsCount/updatedAtcorrectly avoid the0/epoch-1970 traps from the prior iteration. Nice cleanup.Also applies to: 225-232, 276-307
backend/src/main/java/org/booklore/service/metadata/extractor/EpubMetadataExtractor.java (2)
245-261: Field-local error containment is correctly applied.Wrapping each
OBJECT_MAPPER.readTree(content)call in its own try/catch ensures that a malformedcalibre:user_metadata*payload now only forfeits that single field, instead of bubbling aJacksonExceptionto the outer handler and discarding the entireBookMetadata. Good follow-through on the earlier review.
464-473: This review comment is based on an incorrect assumption about the Calibre metadata format and should be dismissed.The review assumes
#value#is stored as a JSON array node (e.g.,"#value#": ["dark", "atmospheric"]), but the actual format—evident from the test fixtures—stores it as a JSON string containing array-like or CSV text (e.g.,"#value#": "["dark", "atmospheric"]"). WhenvalueNodeis aStringNode,asText().trim()correctly returns the string content ("["dark", "atmospheric"]"or"tag1, tag2, tag3"), which is then passed toparseJsonArrayOrCsvfor proper parsing. The tests demonstrate this flow works as intended: both the array-format and CSV-format strings are correctly parsed and values are extracted.There is no regression. The code is correct.
# Conflicts: # backend/src/main/java/org/booklore/service/metadata/extractor/EpubMetadataExtractor.java
Contributor
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
`@backend/src/main/java/org/booklore/service/metadata/extractor/EpubMetadataExtractor.java`:
- Around line 464-473: In EpubMetadataExtractor, the code uses
valueNode.asText() which returns empty for JSON arrays and causes moods/tags
(handled in extractSetField(value, moodsSet) and extractSetField(value,
tagsSet)) to be skipped; change the extraction to serialize arrays with
valueNode.toString() (or use toString() whenever the node is not textual) before
trimming and passing into extractSetField/parseJsonArrayOrCsv so array-valued
Calibre fields like "#moods" and "#extra_tags" are preserved.
🪄 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: Repository YAML (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: 68caca52-a979-482d-a4dc-f7660360c990
📒 Files selected for processing (2)
backend/build.gradle.ktsbackend/src/main/java/org/booklore/service/metadata/extractor/EpubMetadataExtractor.java
💤 Files with no reviewable changes (1)
- backend/build.gradle.kts
📜 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 (4)
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/extractor/EpubMetadataExtractor.java
**/*
⚙️ CodeRabbit configuration file
**/*: This project is being developed using current and future-facing technologies:
- Java 25 with --enable-preview (preview features are INTENTIONAL and encouraged)
- Spring Boot 4 (latest major version, check APIs accordingly)
- Jackson 3 (new package: tools.jackson.* instead of com.fasterxml.jackson.*)
- Hibernate 7.3.x (Jakarta Persistence 3.2, new APIs; avoid deprecated Hibernate 5/6 patterns)
- Angular 21 (signals-based reactivity, no NgModules unless legacy)
Grimmory Internal Tools
- epub4j and pdfium4j are our own internal tools developed by the Grimmory team.
- Always verify behavior and API changes against the upstream repositories:
- If you encounter issues with these libraries, check if a fix exists in the upstream grimmory-tools organization.
Metadata Standards and Compliance
- For all metadata writing and parsing logic, double-check against Dublin Core and ANSI standards to ensure perfect official compliance.
- We strictly follow the widespread and official XML-compliant methods for EPUB2, EPUB3, CBX, and PDF formats.
General Java and Spring rules
- ALWAYS prefer modern, idiomatic Java 25 constructs over legacy patterns.
- Preview features (--enable-preview) are enabled and intentional; do NOT flag them as risky unless there is a concrete runtime issue.
- Prefer: records, sealed classes/interfaces, pattern matching (switch expressions, instanceof), structured concurrency (StructuredTaskScope), scoped values, string templates, unnamed patterns/variables.
- Prefer virtual threads (Thread.ofVirtual(), Executors.newVirtualThreadPerTaskExecutor()) over platform threads for I/O-bound work.
- Prefer the new Sequenced Collections API (SequencedCollection, SequencedMap) where applicable.
- Prefer
varfor local variables when the type is obvious from context.- Use stream().toList() instead of stream().collect(Collectors.toList()) for imm...
Files:
backend/src/main/java/org/booklore/service/metadata/extractor/EpubMetadataExtractor.java
**/service/**/*.java
⚙️ CodeRabbit configuration file
**/service/**/*.java: Spring Framework 7 service layer review:
- Flag missing
@Transactionalon methods that perform multiple writes.- Prefer constructor injection over
@Autowiredfield injection. Use@AllArgsConstructor.- Use ApiError enum for throwing exceptions.
- Flag checked exceptions swallowed silently; must log or rethrow.
- Flag Thread.sleep(); prefer Duration-based overloads or ScheduledExecutorService.
- Prefer virtual threads (Thread.ofVirtual()) for I/O-bound operations.
- Flag mutable shared state in singleton beans.
Files:
backend/src/main/java/org/booklore/service/metadata/extractor/EpubMetadataExtractor.java
**/metadata/**/*.java
⚙️ CodeRabbit configuration file
**/metadata/**/*.java: Metadata and Sidecar review:
- Ensure perfect official compliance with Dublin Core and ANSI standards.
- Strictly follow official XML-compliant methods for EPUB2, EPUB3, CBX, and PDF formats.
- Sidecar files must be written in a way that is compatible with widespread e-reader standards.
- Avoid proprietary or non-standard XML tags unless absolutely necessary for internal tracking.
Files:
backend/src/main/java/org/booklore/service/metadata/extractor/EpubMetadataExtractor.java
🧠 Learnings (8)
📓 Common learnings
Learnt from: balazs-szucs
Repo: grimmory-tools/grimmory PR: 334
File: booklore-api/src/main/java/org/booklore/service/reader/EpubReaderService.java:402-407
Timestamp: 2026-04-02T09:25:48.330Z
Learning: In grimmory-tools/grimmory, before commenting on any file processing code (epub or pdf), always verify the current state of the relevant upstream grimmory libraries: `grimmory-tools/epub4j` (for epub) and `grimmory-tools/PDFium4j` (for pdf). These custom libraries may have different APIs, capabilities, and limitations compared to the third-party libraries they replace. Issues about streaming, buffering, or API surface may need to be filed/addressed in those upstream repos rather than in grimmory itself.
📚 Learning: 2026-04-02T09:25:37.417Z
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:37.417Z
Learning: When reviewing epub-related file processing code in this project, remember that grimmory-tools/grimmory uses the `org.grimmory.epub4j` library (from grimmory-tools/epub4j) and that it is a grimmory-maintained fork fundamentally different from `io.documentnode.epub4j`. Do not assume the same API behavior, limitations, or semantics as `io.documentnode.epub4j`. Before flagging issues or proposing changes, verify the current API/capabilities in the grimmory-tools/epub4j repository.
Applied to files:
backend/src/main/java/org/booklore/service/metadata/extractor/EpubMetadataExtractor.java
📚 Learning: 2026-04-02T09:25:48.330Z
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.
Applied to files:
backend/src/main/java/org/booklore/service/metadata/extractor/EpubMetadataExtractor.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/extractor/EpubMetadataExtractor.java
📚 Learning: 2026-04-23T14:49:03.756Z
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:03.756Z
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/extractor/EpubMetadataExtractor.java
📚 Learning: 2026-04-24T05:28:17.574Z
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:28:17.574Z
Learning: In grimmory-tools/grimmory, when `AppSettings.getMaxFileUploadSizeInMb()` returns null in `BookCoverService.getMaxFileUploadSizeMb()`, `FileService`, and `AuthorMetadataService`, the preferred behavior is to emit a warning log and return `0L` (which causes all uploads to be rejected via the size check), rather than throwing an exception. The rationale is that the application's error handler strips 5xx response detail anyway, so a throw gives the user no more actionable information than a size-check rejection, and an admin warning in the log is needed in either case. Do not suggest replacing this pattern with a thrown exception.
Applied to files:
backend/src/main/java/org/booklore/service/metadata/extractor/EpubMetadataExtractor.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/extractor/EpubMetadataExtractor.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/extractor/EpubMetadataExtractor.java
🔀 Multi-repo context grimmory-tools/grimmory-docs
Linked repositories findings
grimmory-tools/grimmory-docs
- Docs reference the metadata fields and provider-specific fields produced by the backend (pageCount, moods, tags, goodreadsRating, goodreadsReviewCount, goodreadsId, sidecar format). These are consumers at the feature/docs level (no code-level dependency on backend JSON libs). Examples:
- Sidecar example includes pageCount: [::grimmory-tools/grimmory-docs::docs/metadata/sidecar-files.md:1-40]
- Magic Shelves use pageCount, moods, tags, goodreadsRating, goodreadsReviewCount in rules/examples: [::grimmory-tools/grimmory-docs::docs/magic-shelf.md:11,63-66,104-105,117-118,447-448,1436-1437]
- Metadata settings mention Goodreads provider fields and sidecar behavior: [::grimmory-tools/grimmory-docs::docs/metadata/metadata-settings.md:62-64,113,136][::grimmory-tools/grimmory-docs::docs/metadata/metadata-fetch-configuration.md:186,194-215]
- No files reference org.springframework.boot.configurationprocessor.json, spring-boot-configuration-processor, or other Spring JSON processor classes; docs mention JSON generically and Jackson/JSON libraries are not referenced: search returned no matches for those strings. [::grimmory-tools/grimmory-docs::(search results)]
Conclusion: documentation depends on the same metadata field names and sidecar JSON shape that the backend code produces/consumes; the PR's internal replacement of Spring JSONObject/JSONArray with Jackson does not require doc changes and poses no cross-repo code-breaking risk for grimmory-docs (no code-level dependency found). [::grimmory-tools/grimmory-docs::docs/metadata/sidecar-files.md:1-40]
…pubMetadataExtractor
Contributor
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
`@backend/src/main/java/org/booklore/service/metadata/extractor/EpubMetadataExtractor.java`:
- Around line 52-53: The static OBJECT_MAPPER in EpubMetadataExtractor is
created via JsonMapper.builder().build() which is unnecessary for an
unconfigured mapper; replace its initialization to use JsonMapper.shared() (or
inject a Spring-managed ObjectMapper) so the class-level constant OBJECT_MAPPER
uses the shared singleton instance as per guidelines.
🪄 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: Repository YAML (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: 03f08e3b-f35c-45f9-aa1a-e722e29bbe8e
📒 Files selected for processing (2)
backend/src/main/java/org/booklore/service/metadata/extractor/EpubMetadataExtractor.javabackend/src/test/java/org/booklore/service/metadata/extractor/EpubMetadataExtractorTest.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 / Backend Tests
- GitHub Check: Test Suite / Frontend Tests
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (java-kotlin)
- GitHub Check: Frontend Lint Threshold Check
🧰 Additional context used
📓 Path-based instructions (6)
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/metadata/extractor/EpubMetadataExtractorTest.javabackend/src/main/java/org/booklore/service/metadata/extractor/EpubMetadataExtractor.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/metadata/extractor/EpubMetadataExtractorTest.java
**/*
⚙️ CodeRabbit configuration file
**/*: This project is being developed using current and future-facing technologies:
- Java 25 with --enable-preview (preview features are INTENTIONAL and encouraged)
- Spring Boot 4 (latest major version, check APIs accordingly)
- Jackson 3 (new package: tools.jackson.* instead of com.fasterxml.jackson.*)
- Hibernate 7.3.x (Jakarta Persistence 3.2, new APIs; avoid deprecated Hibernate 5/6 patterns)
- Angular 21 (signals-based reactivity, no NgModules unless legacy)
Grimmory Internal Tools
- epub4j and pdfium4j are our own internal tools developed by the Grimmory team.
- Always verify behavior and API changes against the upstream repositories:
- If you encounter issues with these libraries, check if a fix exists in the upstream grimmory-tools organization.
Metadata Standards and Compliance
- For all metadata writing and parsing logic, double-check against Dublin Core and ANSI standards to ensure perfect official compliance.
- We strictly follow the widespread and official XML-compliant methods for EPUB2, EPUB3, CBX, and PDF formats.
General Java and Spring rules
- ALWAYS prefer modern, idiomatic Java 25 constructs over legacy patterns.
- Preview features (--enable-preview) are enabled and intentional; do NOT flag them as risky unless there is a concrete runtime issue.
- Prefer: records, sealed classes/interfaces, pattern matching (switch expressions, instanceof), structured concurrency (StructuredTaskScope), scoped values, string templates, unnamed patterns/variables.
- Prefer virtual threads (Thread.ofVirtual(), Executors.newVirtualThreadPerTaskExecutor()) over platform threads for I/O-bound work.
- Prefer the new Sequenced Collections API (SequencedCollection, SequencedMap) where applicable.
- Prefer
varfor local variables when the type is obvious from context.- Use stream().toList() instead of stream().collect(Collectors.toList()) for imm...
Files:
backend/src/test/java/org/booklore/service/metadata/extractor/EpubMetadataExtractorTest.javabackend/src/main/java/org/booklore/service/metadata/extractor/EpubMetadataExtractor.java
**/service/**/*.java
⚙️ CodeRabbit configuration file
**/service/**/*.java: Spring Framework 7 service layer review:
- Flag missing
@Transactionalon methods that perform multiple writes.- Prefer constructor injection over
@Autowiredfield injection. Use@AllArgsConstructor.- Use ApiError enum for throwing exceptions.
- Flag checked exceptions swallowed silently; must log or rethrow.
- Flag Thread.sleep(); prefer Duration-based overloads or ScheduledExecutorService.
- Prefer virtual threads (Thread.ofVirtual()) for I/O-bound operations.
- Flag mutable shared state in singleton beans.
Files:
backend/src/test/java/org/booklore/service/metadata/extractor/EpubMetadataExtractorTest.javabackend/src/main/java/org/booklore/service/metadata/extractor/EpubMetadataExtractor.java
**/metadata/**/*.java
⚙️ CodeRabbit configuration file
**/metadata/**/*.java: Metadata and Sidecar review:
- Ensure perfect official compliance with Dublin Core and ANSI standards.
- Strictly follow official XML-compliant methods for EPUB2, EPUB3, CBX, and PDF formats.
- Sidecar files must be written in a way that is compatible with widespread e-reader standards.
- Avoid proprietary or non-standard XML tags unless absolutely necessary for internal tracking.
Files:
backend/src/test/java/org/booklore/service/metadata/extractor/EpubMetadataExtractorTest.javabackend/src/main/java/org/booklore/service/metadata/extractor/EpubMetadataExtractor.java
**/*Test.java
⚙️ CodeRabbit configuration file
**/*Test.java: Java test review:
- Prefer
@ExtendWith(SpringExtension.class) or@SpringBootTestfor integration tests.- Flag tests with no assertions.
- Flag Thread.sleep() in tests; use Awaitility or virtual-thread-friendly alternatives.
- Flag hardcoded ports or file paths.
- Flag missing edge case coverage: null, empty, boundary values.
- Prefer AssertJ over JUnit's built-in assertions for readability.
- Prefer
@Sqlor Testcontainers for database state; not hand-rolled setup/teardown.
Files:
backend/src/test/java/org/booklore/service/metadata/extractor/EpubMetadataExtractorTest.java
🧠 Learnings (8)
📚 Learning: 2026-04-02T09:25:37.417Z
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:37.417Z
Learning: When reviewing epub-related file processing code in this project, remember that grimmory-tools/grimmory uses the `org.grimmory.epub4j` library (from grimmory-tools/epub4j) and that it is a grimmory-maintained fork fundamentally different from `io.documentnode.epub4j`. Do not assume the same API behavior, limitations, or semantics as `io.documentnode.epub4j`. Before flagging issues or proposing changes, verify the current API/capabilities in the grimmory-tools/epub4j repository.
Applied to files:
backend/src/test/java/org/booklore/service/metadata/extractor/EpubMetadataExtractorTest.javabackend/src/main/java/org/booklore/service/metadata/extractor/EpubMetadataExtractor.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/test/java/org/booklore/service/metadata/extractor/EpubMetadataExtractorTest.javabackend/src/main/java/org/booklore/service/metadata/extractor/EpubMetadataExtractor.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/metadata/extractor/EpubMetadataExtractorTest.javabackend/src/main/java/org/booklore/service/metadata/extractor/EpubMetadataExtractor.java
📚 Learning: 2026-04-27T15:25:55.042Z
Learnt from: imnotjames
Repo: grimmory-tools/grimmory PR: 930
File: backend/src/test/java/org/booklore/service/metadata/parser/ComicvineBookParserTest.java:68-75
Timestamp: 2026-04-27T15:25:55.042Z
Learning: In this repository’s JUnit 5 test sources (e.g., under backend/src/test/java/), do not flag “bare” `assert` statements as a bug. The project test runner is configured to always execute tests with assertions enabled (e.g., `-ea`), so `assert` behavior is consistent. Continue to review for correctness, but don’t treat unguarded `assert` usage in test classes as a static-analysis issue.
Applied to files:
backend/src/test/java/org/booklore/service/metadata/extractor/EpubMetadataExtractorTest.java
📚 Learning: 2026-04-02T09:25:48.330Z
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.
Applied to files:
backend/src/main/java/org/booklore/service/metadata/extractor/EpubMetadataExtractor.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/extractor/EpubMetadataExtractor.java
📚 Learning: 2026-04-23T14:49:03.756Z
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:03.756Z
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/extractor/EpubMetadataExtractor.java
📚 Learning: 2026-04-24T05:28:17.574Z
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:28:17.574Z
Learning: In grimmory-tools/grimmory, when `AppSettings.getMaxFileUploadSizeInMb()` returns null in `BookCoverService.getMaxFileUploadSizeMb()`, `FileService`, and `AuthorMetadataService`, the preferred behavior is to emit a warning log and return `0L` (which causes all uploads to be rejected via the size check), rather than throwing an exception. The rationale is that the application's error handler strips 5xx response detail anyway, so a throw gives the user no more actionable information than a size-check rejection, and an admin warning in the log is needed in either case. Do not suggest replacing this pattern with a thrown exception.
Applied to files:
backend/src/main/java/org/booklore/service/metadata/extractor/EpubMetadataExtractor.java
🔀 Multi-repo context grimmory-tools/grimmory-docs
Findings for grimmory-tools/grimmory-docs
-
Docs reference backend metadata fields that the PR touches (pageCount, moods, tags, goodreadsRating, goodreadsReviewCount, goodreadsId, sidecar format). Examples:
- [::grimmory-tools/grimmory-docs::docs/metadata/sidecar-files.md:30] — sidecar example includes "pageCount": 350
- [::grimmory-tools/grimmory-docs::docs/magic-shelf.md:66,117,104-105,1436-1437] — documents
pageCount,moods,goodreadsRating,goodreadsReviewCount,goodreadsIdusage in Magic Shelves - [::grimmory-tools/grimmory-docs::docs/metadata/metadata-settings.md:185,📄 section] — sidecar JSON format and included fields list (moods, tags, provider IDs, ratings)
- [::grimmory-tools/grimmory-docs::docs/metadata/metadata-fetch-configuration.md:263] — notes provider-specific fields (moods, tags, ratings) are fetched only when provider is queried
-
No references to Spring's org.springframework.boot.configurationprocessor.json or spring-boot-configuration-processor were found in the repo (search returned no matches). [::grimmory-tools/grimmory-docs::(search results)]
Summary conclusion:
- The PR's internal replacement of Spring's JSONObject/JSONArray with Jackson is an implementation detail and does not require changes in grimmory-docs. Documentation consumes the same metadata field names/sidecar format produced by the backend; no cross-repo code-level breakage detected.
🔇 Additional comments (3)
backend/src/test/java/org/booklore/service/metadata/extractor/EpubMetadataExtractorTest.java (1)
705-716: LGTM!The test properly validates array-based mood extraction from calibre
user_metadata. It follows the existing test patterns, uses AssertJ assertions, and covers the new Jackson-based parsing behavior for array-valued#value#fields.backend/src/main/java/org/booklore/service/metadata/extractor/EpubMetadataExtractor.java (2)
3-6: LGTM!Correct Jackson 3 imports using the
tools.jackson.*package namespace.
246-260: LGTM!The error handling correctly isolates JSON parsing failures to prevent a single malformed Calibre metadata field from aborting the entire EPUB metadata extraction. Debug-level logging is appropriate for this recoverable scenario.
imnotjames
reviewed
May 3, 2026
imnotjames
reviewed
May 3, 2026
imnotjames
approved these changes
May 3, 2026
imnotjames
left a comment
Contributor
There was a problem hiding this comment.
One question and one stylistic choice that could improve code readability. Nothing egregious, once the comments are answered looks good to me.
7 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Linked Issue: Fixes #
Changes
This pull request refactors the codebase to replace the use of
org.springframework.boot.configurationprocessor.jsonwithtools.jackson.databind(Jackson) for JSON parsing in the metadata extraction and parsing services. This change improves maintainability, performance, and consistency in JSON handling. Additionally, the now-unnecessaryspring-boot-configuration-processordependency is removed from the build configuration.Dependency Updates:
spring-boot-configuration-processordependency frombackend/build.gradle.ktsas it is no longer needed for JSON parsing.Refactoring to Jackson for JSON Parsing:
JSONObjectandJSONArrayfromorg.springframework.boot.configurationprocessor.jsonwithJsonNodeand related classes from Jackson inEpubMetadataExtractor.javaandGoodReadsParser.java. This includes updating method signatures, parsing logic, and iteration over JSON properties.Summary by CodeRabbit
Chores
Refactor
Tests