fix(api): use ASIN when specified for retrieving Amazon / Audible metadata#369
Merged
balazs-szucs merged 1 commit intoApr 6, 2026
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughParsers now prefer a validated stored ASIN when present and only search when needed. ID extraction and validation were tightened, get*Ids(...) never returns null (returns empty lists on failures), and new helpers select a single top ID (or null) for metadata fetching. Changes
Sequence Diagram(s)sequenceDiagram
participant Book as Book
participant Parser as Parser (Amazon/Audible)
participant App as AppSettings
participant Remote as Remote Search
Book->>Parser: fetchTopMetadata(book, request)
Parser->>Book: read book.metadata.asin
Parser->>App: read provider domain/config
alt stored ASIN present & valid
Parser->>Parser: getExistingAsin() -> normalized ASIN
Parser->>Remote: fetch detail URL for ASIN
Remote-->>Parser: detail page
Parser-->>Book: return metadata
else stored ASIN missing/invalid
Parser->>Remote: buildQueryUrl(...) and perform search
Remote-->>Parser: search results (list or 503/empty)
Parser->>Parser: extract IDs (validate, skip null, dedupe)
Parser->>Parser: getTop... (first valid ID or null)
alt ID found
Parser->>Remote: fetch detail for found ID
Remote-->>Parser: detail page
Parser-->>Book: return metadata
else no ID
Parser-->>Book: return empty result
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
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 |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@booklore-api/src/main/java/org/booklore/service/metadata/parser/AmazonBookParser.java`:
- Around line 181-193: The getTopAmazonBookId method can NPE when
book.getMetadata() is null; update AmazonBookParser.getTopAmazonBookId to first
null-check book.getMetadata() before calling getAsin(), e.g. retrieve metadata
into a variable, verify metadata != null and metadata.getAsin() is non-empty,
then fall back to calling getAmazonBookIds(book, request) as currently
implemented; ensure you treat empty strings the same as null and return null if
no ID is found.
In
`@booklore-api/src/main/java/org/booklore/service/metadata/parser/AudibleParser.java`:
- Around line 210-223: In getTopAudibleId(Book book, FetchMetadataRequest
request) add a defensive null check for book.getMetadata() before calling
getAsin() (mirror the fix in AmazonBookParser): if metadata is null skip to
fetching IDs; ensure you reference getTopAudibleId and getAudibleIds when making
the change. Also remove the extra blank line before the method's closing brace
so there is no unnecessary empty line.
🪄 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: af184ad1-5945-4ba3-a4ea-7f66694271cf
📒 Files selected for processing (2)
booklore-api/src/main/java/org/booklore/service/metadata/parser/AmazonBookParser.javabooklore-api/src/main/java/org/booklore/service/metadata/parser/AudibleParser.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 (javascript-typescript)
- GitHub Check: Analyze (java-kotlin)
🧰 Additional context used
📓 Path-based instructions (1)
booklore-api/src/**/*.java
📄 CodeRabbit inference engine (AGENTS.md)
booklore-api/src/**/*.java: Use 4-space indentation in Java files and match surrounding Java style
Prefer constructor injection via Lombok patterns already used in the codebase. Do not introduce@Autowiredfield injection
Use MapStruct for entity/DTO mapping
Files:
booklore-api/src/main/java/org/booklore/service/metadata/parser/AmazonBookParser.javabooklore-api/src/main/java/org/booklore/service/metadata/parser/AudibleParser.java
🧠 Learnings (2)
📓 Common learnings
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.
Learnt from: pedronave
Repo: grimmory-tools/grimmory PR: 173
File: booklore-api/src/main/java/org/booklore/service/hardcover/HardcoverSyncService.java:220-225
Timestamp: 2026-03-24T18:46:10.869Z
Learning: In the Hardcover GraphQL API, empty strings (`""`) are not valid ISBN values and querying with `_eq: ""` returns no results. Therefore, binding a missing ISBN to `""` in `_or` edition filters in HardcoverSyncService.java is safe and will not accidentally match unrelated editions or books.
📚 Learning: 2026-03-24T18:46:47.249Z
Learnt from: pedronave
Repo: grimmory-tools/grimmory PR: 173
File: booklore-api/src/main/java/org/booklore/service/hardcover/HardcoverSyncService.java:188-199
Timestamp: 2026-03-24T18:46:47.249Z
Learning: In grimmory-tools/grimmory (HardcoverSyncService.java), when a `hardcoverBookId` is stored in book metadata but is malformed (non-numeric), the preferred behavior is to return null and skip the sync rather than falling back to ISBN-based lookup. The reasoning is that an explicitly set bookId represents deliberate user intent, and silently resolving via ISBN could sync progress to a different book than intended.
Applied to files:
booklore-api/src/main/java/org/booklore/service/metadata/parser/AmazonBookParser.javabooklore-api/src/main/java/org/booklore/service/metadata/parser/AudibleParser.java
🔇 Additional comments (6)
booklore-api/src/main/java/org/booklore/service/metadata/parser/AmazonBookParser.java (3)
98-104: LGTM!The refactored
fetchTopMetadatacorrectly delegates to the new helper method, keeping the method clean and focused.
107-111: LGTM!The switch from null-checking to
isEmpty()is cleaner and aligns with the updatedgetAmazonBookIdscontract.
195-207: LGTM!Returning
Collections.emptyList()instead ofnullin error paths is a good consistency improvement. This simplifies caller logic and prevents potential NPEs from null list handling.Also applies to: 238-246
booklore-api/src/main/java/org/booklore/service/metadata/parser/AudibleParser.java (3)
69-75: LGTM!Clean delegation to the new helper method, consistent with the AmazonBookParser refactoring.
78-82: LGTM!The switch to
isEmpty()check is consistent with the updatedgetAudibleIdscontract.
225-230: LGTM!Returning
Collections.emptyList()instead ofnullis consistent with the AmazonBookParser refactoring.
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
`@booklore-api/src/main/java/org/booklore/service/metadata/parser/AmazonBookParser.java`:
- Around line 188-190: The code in AmazonBookParser that selects the top ID from
getAmazonBookIds(book, request) can throw an NPE if the first element is null;
update the selection logic in the block around
amazonBookIds/getAmazonBookIds(...) so you either filter out nulls before
selecting or explicitly check that amazonBookIds.getFirst() != null (and not
empty) before returning it; reference the amazonBookIds variable and the
getAmazonBookIds(...) call and ensure the condition verifies non-null and
non-empty first candidate (or iterate to find the first non-null/non-empty
entry) to avoid the NPE.
🪄 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: 5eb7fb20-88b0-47a8-b80b-be77c8909e85
📒 Files selected for processing (2)
booklore-api/src/main/java/org/booklore/service/metadata/parser/AmazonBookParser.javabooklore-api/src/main/java/org/booklore/service/metadata/parser/AudibleParser.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). (2)
- GitHub Check: Test Suite / Backend Tests
- GitHub Check: Analyze (java-kotlin)
🧰 Additional context used
📓 Path-based instructions (1)
booklore-api/src/**/*.java
📄 CodeRabbit inference engine (AGENTS.md)
booklore-api/src/**/*.java: Use 4-space indentation in Java files and match surrounding Java style
Prefer constructor injection via Lombok patterns already used in the codebase. Do not introduce@Autowiredfield injection
Use MapStruct for entity/DTO mapping
Files:
booklore-api/src/main/java/org/booklore/service/metadata/parser/AudibleParser.javabooklore-api/src/main/java/org/booklore/service/metadata/parser/AmazonBookParser.java
🧠 Learnings (3)
📓 Common learnings
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.
📚 Learning: 2026-03-24T18:46:47.249Z
Learnt from: pedronave
Repo: grimmory-tools/grimmory PR: 173
File: booklore-api/src/main/java/org/booklore/service/hardcover/HardcoverSyncService.java:188-199
Timestamp: 2026-03-24T18:46:47.249Z
Learning: In grimmory-tools/grimmory (HardcoverSyncService.java), when a `hardcoverBookId` is stored in book metadata but is malformed (non-numeric), the preferred behavior is to return null and skip the sync rather than falling back to ISBN-based lookup. The reasoning is that an explicitly set bookId represents deliberate user intent, and silently resolving via ISBN could sync progress to a different book than intended.
Applied to files:
booklore-api/src/main/java/org/booklore/service/metadata/parser/AudibleParser.javabooklore-api/src/main/java/org/booklore/service/metadata/parser/AmazonBookParser.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:
booklore-api/src/main/java/org/booklore/service/metadata/parser/AmazonBookParser.java
🔇 Additional comments (3)
booklore-api/src/main/java/org/booklore/service/metadata/parser/AudibleParser.java (2)
70-74: Nice null/empty normalization for top + list flowsGood change: top metadata now resolves through one selector path, and
getAudibleIdsconsistently returns empty lists instead ofnull, simplifying caller safety.Also applies to: 80-81, 229-230
210-214:⚠️ Potential issue | 🟠 MajorPrioritize
audibleIdfield over genericasiningetTopAudibleId()Line 212 checks only
metadata.getAsin()but should prefer the Audible-specificmetadata.getAudibleId()field. Sinceasincan originate from non-Audible sources, using it as an Audible lookup key may skip search and sync progress to a different book. CheckaudibleIdfirst, then fall back toasinonly ifaudibleIdis not set.⛔ Skipped due to learnings
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.Learnt from: pedronave Repo: grimmory-tools/grimmory PR: 247 File: booklore-api/src/main/java/org/booklore/service/hardcover/HardcoverSyncService.java:156-159 Timestamp: 2026-04-01T17:50:06.817Z Learning: In grimmory-tools/grimmory (HardcoverSyncService.java), the Hardcover API already returns `user_book_reads` in a consistent, ordered fashion (ascending by recency), so `getLast()` reliably retrieves the most recent read entry. Adding an explicit `order_by` to the `user_book_reads` query is unnecessary.booklore-api/src/main/java/org/booklore/service/metadata/parser/AmazonBookParser.java (1)
108-110: Consistent empty-list returns are a solid reliability improvementReturning
Collections.emptyList()in no-query/no-results/anti-scraping branches removes null-handling ambiguity and keeps downstream flow predictable.Also applies to: 200-201, 208-209, 241-242
1 task
72c9cff to
2ef9742
Compare
imnotjames
commented
Apr 4, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
booklore-api/src/main/java/org/booklore/service/metadata/parser/AmazonBookParser.java (1)
144-149:⚠️ Potential issue | 🟠 MajorSkip invalid extracted IDs before choosing the top Amazon result.
extractAmazonBookId(item)can still contributenullor"", so this branch can either NPE ongetFirst().isEmpty()or give up on the first blank hit even when later ASINs are valid. Filtering to the first non-blank candidate here avoids breakingfetchTopMetadata(...)and also keeps the same bad IDs from flowing intofetchMetadata(...).Possible fix
- List<String> amazonBookIds = getAmazonBookIds(book, request); - if (!amazonBookIds.isEmpty() && !amazonBookIds.getFirst().isEmpty()) { - return amazonBookIds.getFirst(); - } - - return null; + return getAmazonBookIds(book, request).stream() + .filter(Objects::nonNull) + .map(String::trim) + .filter(id -> !id.isEmpty()) + .findFirst() + .orElse(null);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@booklore-api/src/main/java/org/booklore/service/metadata/parser/AmazonBookParser.java` around lines 144 - 149, The current branch returns amazonBookIds.getFirst() without filtering out null/empty entries causing a potential NPE or returning a blank ASIN; update the logic in the block that calls getAmazonBookIds(book, request) to filter the returned list for non-null, non-empty strings (e.g., remove items where extractAmazonBookId(...) produced null or ""), then pick the first valid candidate (and return null if none remain) so fetchTopMetadata(...) and fetchMetadata(...) receive only real ASINs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@booklore-api/src/main/java/org/booklore/service/metadata/parser/AmazonBookParser.java`:
- Around line 138-141: In AmazonBookParser where you read metadata via
book.getMetadata() and metadata.getAsin(), trim the retrieved storedBookId,
treat an all-whitespace value as absent (i.e., continue normal lookup), and if
the trimmed value is non-blank validate it (e.g., with a simple helper like
isValidHardcoverBookId or regex); return the trimmed ID only when valid, and
return null when the trimmed ID is non-blank but invalid so the sync is skipped
rather than falling back to ISBN-based lookup.
In
`@booklore-api/src/main/java/org/booklore/service/metadata/parser/AudibleParser.java`:
- Around line 109-112: The code currently treats any non-empty
metadata.getAsin() value as authoritative; change the logic in AudibleParser
where BookMetadata metadata = book.getMetadata() and storedBookId is derived so
that you first trim the value, treat a trimmed blank as absent, and validate the
trimmed ASIN format (e.g., 10-character ASIN pattern of alphanumerics) before
returning it; if the trimmed value is blank or fails validation, return null so
the sync is skipped rather than attempting a remote fetch with a malformed ID.
---
Duplicate comments:
In
`@booklore-api/src/main/java/org/booklore/service/metadata/parser/AmazonBookParser.java`:
- Around line 144-149: The current branch returns amazonBookIds.getFirst()
without filtering out null/empty entries causing a potential NPE or returning a
blank ASIN; update the logic in the block that calls getAmazonBookIds(book,
request) to filter the returned list for non-null, non-empty strings (e.g.,
remove items where extractAmazonBookId(...) produced null or ""), then pick the
first valid candidate (and return null if none remain) so fetchTopMetadata(...)
and fetchMetadata(...) receive only real ASINs.
🪄 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: d16256f3-79ee-4672-8ef8-8fa4ba92c8ad
📒 Files selected for processing (2)
booklore-api/src/main/java/org/booklore/service/metadata/parser/AmazonBookParser.javabooklore-api/src/main/java/org/booklore/service/metadata/parser/AudibleParser.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). (2)
- GitHub Check: Test Suite / Frontend Tests
- GitHub Check: Analyze (java-kotlin)
🧰 Additional context used
📓 Path-based instructions (1)
booklore-api/src/**/*.java
📄 CodeRabbit inference engine (AGENTS.md)
booklore-api/src/**/*.java: Use 4-space indentation in Java files and match surrounding Java style
Prefer constructor injection via Lombok patterns already used in the codebase. Do not introduce@Autowiredfield injection
Use MapStruct for entity/DTO mapping
Files:
booklore-api/src/main/java/org/booklore/service/metadata/parser/AudibleParser.javabooklore-api/src/main/java/org/booklore/service/metadata/parser/AmazonBookParser.java
🧠 Learnings (3)
📓 Common learnings
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.
📚 Learning: 2026-03-24T18:46:47.249Z
Learnt from: pedronave
Repo: grimmory-tools/grimmory PR: 173
File: booklore-api/src/main/java/org/booklore/service/hardcover/HardcoverSyncService.java:188-199
Timestamp: 2026-03-24T18:46:47.249Z
Learning: In grimmory-tools/grimmory (HardcoverSyncService.java), when a `hardcoverBookId` is stored in book metadata but is malformed (non-numeric), the preferred behavior is to return null and skip the sync rather than falling back to ISBN-based lookup. The reasoning is that an explicitly set bookId represents deliberate user intent, and silently resolving via ISBN could sync progress to a different book than intended.
Applied to files:
booklore-api/src/main/java/org/booklore/service/metadata/parser/AudibleParser.javabooklore-api/src/main/java/org/booklore/service/metadata/parser/AmazonBookParser.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:
booklore-api/src/main/java/org/booklore/service/metadata/parser/AmazonBookParser.java
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
booklore-api/src/main/java/org/booklore/service/metadata/parser/AmazonBookParser.java (1)
137-150:⚠️ Potential issue | 🟡 MinorTrim
storedBookIdbefore validation to handle whitespace-padded values.Line 140 checks
!storedBookId.isEmpty()but doesn't trim. A stored ASIN containing only whitespace (e.g.," ") would pass this check and be returned, potentially causing a failed lookup against/dp/%20%20. Trim first and treat blank as absent.🛡️ Proposed fix
private String getTopAmazonBookId(Book book, FetchMetadataRequest request) { BookMetadata metadata = book.getMetadata(); String storedBookId = metadata == null ? null : metadata.getAsin(); - if (storedBookId != null && !storedBookId.isEmpty()) { + if (storedBookId != null) { + storedBookId = storedBookId.trim(); + } + if (storedBookId != null && !storedBookId.isEmpty()) { return storedBookId; }Based on learnings: "when a
hardcoverBookIdis 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."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@booklore-api/src/main/java/org/booklore/service/metadata/parser/AmazonBookParser.java` around lines 137 - 150, In getTopAmazonBookId, trim the ASIN retrieved from BookMetadata (storedBookId = metadata.getAsin()) before validating; treat a trimmed-empty value as absent so it falls through to getAmazonBookIds instead of returning whitespace-only strings. Use the trimmedStoredBookId for the isEmpty check and eventual return (i.e., reference storedBookId / BookMetadata.getAsin and getAmazonBookIds) so malformed/whitespace ASINs are ignored and the method falls back to the next lookup.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@booklore-api/src/main/java/org/booklore/service/metadata/parser/AmazonBookParser.java`:
- Around line 137-150: In getTopAmazonBookId, trim the ASIN retrieved from
BookMetadata (storedBookId = metadata.getAsin()) before validating; treat a
trimmed-empty value as absent so it falls through to getAmazonBookIds instead of
returning whitespace-only strings. Use the trimmedStoredBookId for the isEmpty
check and eventual return (i.e., reference storedBookId / BookMetadata.getAsin
and getAmazonBookIds) so malformed/whitespace ASINs are ignored and the method
falls back to the next lookup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b34bd254-0a47-4176-ba40-b83383eea33b
📒 Files selected for processing (1)
booklore-api/src/main/java/org/booklore/service/metadata/parser/AmazonBookParser.java
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
booklore-api/src/**/*.java
📄 CodeRabbit inference engine (AGENTS.md)
booklore-api/src/**/*.java: Use 4-space indentation in Java files and match surrounding Java style
Prefer constructor injection via Lombok patterns already used in the codebase. Do not introduce@Autowiredfield injection
Use MapStruct for entity/DTO mapping
Files:
booklore-api/src/main/java/org/booklore/service/metadata/parser/AmazonBookParser.java
🧠 Learnings (6)
📓 Common learnings
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.
Learnt from: pedronave
Repo: grimmory-tools/grimmory PR: 173
File: booklore-api/src/main/java/org/booklore/service/hardcover/HardcoverSyncService.java:220-225
Timestamp: 2026-03-24T18:46:10.869Z
Learning: In the Hardcover GraphQL API, empty strings (`""`) are not valid ISBN values and querying with `_eq: ""` returns no results. Therefore, binding a missing ISBN to `""` in `_or` edition filters in HardcoverSyncService.java is safe and will not accidentally match unrelated editions or books.
📚 Learning: 2026-03-24T18:46:47.249Z
Learnt from: pedronave
Repo: grimmory-tools/grimmory PR: 173
File: booklore-api/src/main/java/org/booklore/service/hardcover/HardcoverSyncService.java:188-199
Timestamp: 2026-03-24T18:46:47.249Z
Learning: In grimmory-tools/grimmory (HardcoverSyncService.java), when a `hardcoverBookId` is stored in book metadata but is malformed (non-numeric), the preferred behavior is to return null and skip the sync rather than falling back to ISBN-based lookup. The reasoning is that an explicitly set bookId represents deliberate user intent, and silently resolving via ISBN could sync progress to a different book than intended.
Applied to files:
booklore-api/src/main/java/org/booklore/service/metadata/parser/AmazonBookParser.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:
booklore-api/src/main/java/org/booklore/service/metadata/parser/AmazonBookParser.java
📚 Learning: 2026-03-24T18:46:10.869Z
Learnt from: pedronave
Repo: grimmory-tools/grimmory PR: 173
File: booklore-api/src/main/java/org/booklore/service/hardcover/HardcoverSyncService.java:220-225
Timestamp: 2026-03-24T18:46:10.869Z
Learning: In the Hardcover GraphQL API, empty strings (`""`) are not valid ISBN values and querying with `_eq: ""` returns no results. Therefore, binding a missing ISBN to `""` in `_or` edition filters in HardcoverSyncService.java is safe and will not accidentally match unrelated editions or books.
Applied to files:
booklore-api/src/main/java/org/booklore/service/metadata/parser/AmazonBookParser.java
📚 Learning: 2026-03-24T18:58:08.199Z
Learnt from: pedronave
Repo: grimmory-tools/grimmory PR: 173
File: booklore-api/src/main/java/org/booklore/service/hardcover/HardcoverSyncService.java:291-294
Timestamp: 2026-03-24T18:58:08.199Z
Learning: In grimmory-tools/grimmory (HardcoverSyncService.java), when a Hardcover edition has no page count, falling back to the book-level page count (book.pages) is the preferred behavior. Skipping the sync entirely in this case is considered worse than using a slightly approximate total, because the page count is only a proxy for converting progress % to page numbers for the API. The Hardcover API allows setting page-based progress even when the edition lacks a page count, and progress displays in at least some UI views (e.g., the book detail dropdown).
Applied to files:
booklore-api/src/main/java/org/booklore/service/metadata/parser/AmazonBookParser.java
📚 Learning: 2026-03-25T19:09:09.638Z
Learnt from: imnotjames
Repo: grimmory-tools/grimmory PR: 189
File: booklore-api/src/main/java/org/booklore/service/kobo/KoboLibrarySyncService.java:113-116
Timestamp: 2026-03-25T19:09:09.638Z
Learning: In `booklore-api/src/main/java/org/booklore/service/kobo/KoboLibrarySyncService.java`, when deciding whether to forward the `Authorization` header to the upstream Kobo server proxy on the `/v1/library/sync` endpoint, a simple null check (`request.getHeader(HttpHeaders.AUTHORIZATION) != null`) is the correct guard. The header is either absent or present — if present it may or may not be a valid token, but validity is determined by the upstream Kobo server, not by Grimmory. Blank/whitespace intermediate states do not occur in practice, and pre-validating the token value before forwarding is not the responsibility of this service.
Applied to files:
booklore-api/src/main/java/org/booklore/service/metadata/parser/AmazonBookParser.java
🔇 Additional comments (3)
booklore-api/src/main/java/org/booklore/service/metadata/parser/AmazonBookParser.java (3)
98-104: LGTM!Clean refactoring that delegates ID selection to
getTopAmazonBookId. The early null return prevents unnecessary metadata fetch attempts.
107-111: LGTM!The transition from null-check to
isEmpty()is consistent with the updatedgetAmazonBookIdscontract that now returns an empty list instead of null.
152-207: LGTM!The changes properly address prior concerns:
- Null book IDs are now filtered out (lines 192-196) before being added to the list
- The method consistently returns an empty list instead of null on failure paths
- Partial results are preserved if an exception occurs after collecting some IDs
da17ee8 to
1e28f92
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
booklore-api/src/main/java/org/booklore/service/metadata/parser/AmazonBookParser.java (1)
155-163:⚠️ Potential issue | 🟠 MajorDon’t search past an explicit but invalid ASIN.
Line 161 starts a search whenever
getExistingAsin(book)returnsnull. After the validation bug is fixed, that still conflates “no ASIN stored” with “stored ASIN is malformed”, and the fallback search can retarget metadata to a different Amazon book—the behavior Issue#366is trying to stop. Based on learnings: "an explicitly set bookId represents deliberate user intent, and silently resolving via ISBN could sync progress to a different book."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@booklore-api/src/main/java/org/booklore/service/metadata/parser/AmazonBookParser.java` around lines 155 - 163, The current getTopAmazonBookId(Book, FetchMetadataRequest) falls back to searching when getExistingAsin(book) is null but still treats malformed stored ASINs the same as "no ASIN", allowing silent retargeting; change the logic so that when getExistingAsin(book) returns a non-null value you validate it (introduce or call an isValidAsin helper) and immediately return that stored ASIN regardless of validity (i.e., do NOT call getAmazonBookIds if an explicit ASIN exists), only invoking getAmazonBookIds when getExistingAsin(book) returns null; update getTopAmazonBookId, add or reuse an ASIN validation method, and ensure callers (e.g., code that relies on FetchMetadataRequest/Book) preserve the explicit ASIN rather than searching for alternatives.booklore-api/src/main/java/org/booklore/service/metadata/parser/AudibleParser.java (1)
126-134:⚠️ Potential issue | 🟠 MajorDon’t fall back to search for a malformed stored ASIN.
Line 132 starts an Audible search whenever
getExistingAsin(book)returnsnull. That still collapses “missing ASIN” and “invalid explicit ASIN” into the same path, which can resolve metadata for a different audiobook than the one the user intentionally pinned. Based on learnings: "an explicitly set bookId represents deliberate user intent, and silently resolving via ISBN could sync progress to a different book."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@booklore-api/src/main/java/org/booklore/service/metadata/parser/AudibleParser.java` around lines 126 - 134, getTopAudibleId currently falls back to searching whenever getExistingAsin(book) returns null, which merges “no ASIN set” and “user explicitly set an invalid/malformed ASIN”; update the logic so an explicitly set ASIN is honored and not replaced by a search. Change getExistingAsin to distinguish absence vs explicit value (e.g. return Optional<String> or add isExplicitAsinSet(Book)/getExplicitAsin(Book)), then in getTopAudibleId use that signal: if an explicit ASIN exists (even if malformed/empty) return it immediately; only call getAudibleIds when no explicit ASIN is present. Ensure you update any callers to use the new Optional/flag API and keep getAudibleIds unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@booklore-api/src/main/java/org/booklore/service/metadata/parser/AmazonBookParser.java`:
- Around line 55-56: ASIN_PATH_PATTERN in AmazonBookParser is using "/pd/..."
but elsewhere detail URLs are built as "/dp/{asin}", so update the pattern to
match the actual link shape (e.g. change ASIN_PATH_PATTERN to match "/dp/{asin}"
or make it robust for both "/dp/" and "/pd/"); ensure you only alter the static
Pattern ASIN_PATH_PATTERN so link-based ASIN extraction works when data-asin is
absent.
- Around line 147-152: The method currently calls
AmazonBookParser.ASIN_PATTERN.matcher(asin).group(0) without performing a match,
which throws IllegalStateException; update the method that uses
metadata.getAsin() to first normalize the asin (trim and upper-case if desired),
then create a Matcher from AmazonBookParser.ASIN_PATTERN and call
matcher.matches() (or matcher.find() if partial match is intended) before
calling group(0); only return the normalized ASIN when the match succeeds,
otherwise return null. Ensure you reference metadata.getAsin(),
AmazonBookParser.ASIN_PATTERN and the matcher variable when implementing this
check.
In
`@booklore-api/src/main/java/org/booklore/service/metadata/parser/AudibleParser.java`:
- Around line 118-123: The code calls
AudibleParser.ASIN_PATTERN.matcher(asin).group(0) without first checking for a
match, causing IllegalStateException for malformed ASINs; update the logic in
the AudibleParser method that handles stored ASINs (used by fetchTopMetadata())
to create a Matcher m = AudibleParser.ASIN_PATTERN.matcher(asin) and only return
m.group(0) if m.find() (or m.matches()) succeeds, otherwise return null to
preserve the user-intent behavior described (consistent with
HardcoverSyncService).
---
Duplicate comments:
In
`@booklore-api/src/main/java/org/booklore/service/metadata/parser/AmazonBookParser.java`:
- Around line 155-163: The current getTopAmazonBookId(Book,
FetchMetadataRequest) falls back to searching when getExistingAsin(book) is null
but still treats malformed stored ASINs the same as "no ASIN", allowing silent
retargeting; change the logic so that when getExistingAsin(book) returns a
non-null value you validate it (introduce or call an isValidAsin helper) and
immediately return that stored ASIN regardless of validity (i.e., do NOT call
getAmazonBookIds if an explicit ASIN exists), only invoking getAmazonBookIds
when getExistingAsin(book) returns null; update getTopAmazonBookId, add or reuse
an ASIN validation method, and ensure callers (e.g., code that relies on
FetchMetadataRequest/Book) preserve the explicit ASIN rather than searching for
alternatives.
In
`@booklore-api/src/main/java/org/booklore/service/metadata/parser/AudibleParser.java`:
- Around line 126-134: getTopAudibleId currently falls back to searching
whenever getExistingAsin(book) returns null, which merges “no ASIN set” and
“user explicitly set an invalid/malformed ASIN”; update the logic so an
explicitly set ASIN is honored and not replaced by a search. Change
getExistingAsin to distinguish absence vs explicit value (e.g. return
Optional<String> or add isExplicitAsinSet(Book)/getExplicitAsin(Book)), then in
getTopAudibleId use that signal: if an explicit ASIN exists (even if
malformed/empty) return it immediately; only call getAudibleIds when no explicit
ASIN is present. Ensure you update any callers to use the new Optional/flag API
and keep getAudibleIds 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: b0347cac-164a-4456-b6e6-fa0836565f6d
📒 Files selected for processing (2)
booklore-api/src/main/java/org/booklore/service/metadata/parser/AmazonBookParser.javabooklore-api/src/main/java/org/booklore/service/metadata/parser/AudibleParser.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 (1)
booklore-api/src/**/*.java
📄 CodeRabbit inference engine (AGENTS.md)
booklore-api/src/**/*.java: Use 4-space indentation in Java files and match surrounding Java style
Prefer constructor injection via Lombok patterns already used in the codebase. Do not introduce@Autowiredfield injection
Use MapStruct for entity/DTO mapping
Files:
booklore-api/src/main/java/org/booklore/service/metadata/parser/AudibleParser.javabooklore-api/src/main/java/org/booklore/service/metadata/parser/AmazonBookParser.java
🧠 Learnings (6)
📓 Common learnings
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.
📚 Learning: 2026-03-24T18:46:47.249Z
Learnt from: pedronave
Repo: grimmory-tools/grimmory PR: 173
File: booklore-api/src/main/java/org/booklore/service/hardcover/HardcoverSyncService.java:188-199
Timestamp: 2026-03-24T18:46:47.249Z
Learning: In grimmory-tools/grimmory (HardcoverSyncService.java), when a `hardcoverBookId` is stored in book metadata but is malformed (non-numeric), the preferred behavior is to return null and skip the sync rather than falling back to ISBN-based lookup. The reasoning is that an explicitly set bookId represents deliberate user intent, and silently resolving via ISBN could sync progress to a different book than intended.
Applied to files:
booklore-api/src/main/java/org/booklore/service/metadata/parser/AudibleParser.javabooklore-api/src/main/java/org/booklore/service/metadata/parser/AmazonBookParser.java
📚 Learning: 2026-03-24T18:46:10.869Z
Learnt from: pedronave
Repo: grimmory-tools/grimmory PR: 173
File: booklore-api/src/main/java/org/booklore/service/hardcover/HardcoverSyncService.java:220-225
Timestamp: 2026-03-24T18:46:10.869Z
Learning: In the Hardcover GraphQL API, empty strings (`""`) are not valid ISBN values and querying with `_eq: ""` returns no results. Therefore, binding a missing ISBN to `""` in `_or` edition filters in HardcoverSyncService.java is safe and will not accidentally match unrelated editions or books.
Applied to files:
booklore-api/src/main/java/org/booklore/service/metadata/parser/AudibleParser.javabooklore-api/src/main/java/org/booklore/service/metadata/parser/AmazonBookParser.java
📚 Learning: 2026-03-25T19:09:09.638Z
Learnt from: imnotjames
Repo: grimmory-tools/grimmory PR: 189
File: booklore-api/src/main/java/org/booklore/service/kobo/KoboLibrarySyncService.java:113-116
Timestamp: 2026-03-25T19:09:09.638Z
Learning: In `booklore-api/src/main/java/org/booklore/service/kobo/KoboLibrarySyncService.java`, when deciding whether to forward the `Authorization` header to the upstream Kobo server proxy on the `/v1/library/sync` endpoint, a simple null check (`request.getHeader(HttpHeaders.AUTHORIZATION) != null`) is the correct guard. The header is either absent or present — if present it may or may not be a valid token, but validity is determined by the upstream Kobo server, not by Grimmory. Blank/whitespace intermediate states do not occur in practice, and pre-validating the token value before forwarding is not the responsibility of this service.
Applied to files:
booklore-api/src/main/java/org/booklore/service/metadata/parser/AudibleParser.javabooklore-api/src/main/java/org/booklore/service/metadata/parser/AmazonBookParser.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:
booklore-api/src/main/java/org/booklore/service/metadata/parser/AmazonBookParser.java
📚 Learning: 2026-03-24T18:58:08.199Z
Learnt from: pedronave
Repo: grimmory-tools/grimmory PR: 173
File: booklore-api/src/main/java/org/booklore/service/hardcover/HardcoverSyncService.java:291-294
Timestamp: 2026-03-24T18:58:08.199Z
Learning: In grimmory-tools/grimmory (HardcoverSyncService.java), when a Hardcover edition has no page count, falling back to the book-level page count (book.pages) is the preferred behavior. Skipping the sync entirely in this case is considered worse than using a slightly approximate total, because the page count is only a proxy for converting progress % to page numbers for the API. The Hardcover API allows setting page-based progress even when the edition lacks a page count, and progress displays in at least some UI views (e.g., the book detail dropdown).
Applied to files:
booklore-api/src/main/java/org/booklore/service/metadata/parser/AmazonBookParser.java
Contributor
Author
|
Marked as draft for a bit. Going to add tests and handle edge cases. |
1e28f92 to
31b6a2d
Compare
Contributor
|
🧠 Learnings used✅ Actions performedReviews resumed. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
booklore-api/src/main/java/org/booklore/service/metadata/parser/AmazonBookParser.java (2)
54-54:⚠️ Potential issue | 🟠 MajorThis Amazon href matcher is backwards.
/dp/[^/]+/([A-Z0-9]{10})expects/dp/<slug>/<asin>, but Amazon product links are normally/.../dp/<asin>or/dp/<asin>. With the current early return inextractAmazonBookId(), a Paperback/Hardcover href that misses this regex drops the whole candidate instead of falling back todata-asin.Possible fix
- private static final Pattern ASIN_PATH_PATTERN = Pattern.compile("/dp/[^/]+/([A-Z0-9]{10})"); + private static final Pattern ASIN_PATH_PATTERN = Pattern.compile("/(?:[^/]+/)?dp/([A-Z0-9]{10})(?:[/?]|$)");Also applies to: 230-249, 252-260
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@booklore-api/src/main/java/org/booklore/service/metadata/parser/AmazonBookParser.java` at line 54, The ASIN_PATH_PATTERN is reversed and causes extractAmazonBookId() to prematurely discard candidates; update ASIN_PATH_PATTERN to match paths where the ASIN appears immediately after /dp (e.g., allow "/dp/<ASIN>" or paths with segments before /dp), then modify extractAmazonBookId() to first try extracting ASIN from the corrected ASIN_PATH_PATTERN and only if that fails fall back to reading the element's "data-asin" attribute (preserve existing data-asin logic). Ensure you reference and adjust ASIN_PATH_PATTERN and the extractAmazonBookId() method so Paperback/Hardcover hrefs that don't match the old pattern are not dropped and the data-asin fallback is used.
137-157:⚠️ Potential issue | 🟠 MajorDon't fall back to search when the stored ASIN is present but invalid.
This has the same user-intent problem as the Audible side:
find()extracts any 10-character substring from the stored value, andgetTopAmazonBookId()searches when no match is found. That means an explicitly entered but malformed ASIN can still resolve to a different book instead of stopping cleanly. Normalize the raw value, require a full ASIN match, and returnnullwhen a non-blank stored ASIN is invalid.Based on learnings: "when a
hardcoverBookIdis 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."Also applies to: 159-170
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@booklore-api/src/main/java/org/booklore/service/metadata/parser/AmazonBookParser.java` around lines 137 - 157, getExistingAsin currently tolerates malformed ASINs by using ASIN_PATTERN.matcher(asin).find(), which can extract a 10-char substring and allow fallback search; instead trim/normalize the raw stored value, treat blank as null, and require a full-match (use matches() or anchor the pattern) so that if a non-blank stored ASIN is invalid you return null rather than falling back to search. Update AmazonBookParser.getExistingAsin to normalize asin (e.g., asin = asin.trim()), return null on blank, and only return matcher.group(1) when the pattern fully matches; otherwise return null. Ensure the same change is applied to the analogous method referenced around lines 159-170.booklore-api/src/main/java/org/booklore/service/metadata/parser/AudibleParser.java (1)
109-129:⚠️ Potential issue | 🟠 MajorDon't treat a malformed stored ASIN as searchable input.
getExistingAsin()currently pulls the first[A-Z0-9]{10}substring viafind(), andgetTopAudibleId()falls back togetAudibleIds(...)when no match is found. That still lets an explicitly set but malformed ASIN resolve to a different audiobook, which is the mismatch this PR is trying to avoid. Normalize first, require a full 10-character ASIN match, and returnnullinstead of searching when a non-blank stored ASIN is invalid.Based on learnings: "when a
hardcoverBookIdis 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."Also applies to: 131-143
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@booklore-api/src/main/java/org/booklore/service/metadata/parser/AudibleParser.java` around lines 109 - 129, getExistingAsin currently uses ASIN_PATTERN.matcher(asin).find() which accepts a substring; change it to normalize the stored ASIN (trim, uppercase) and require a full 10-char ASIN match using matcher.matches() (or an anchored pattern) so malformed but non-blank metadata returns null; update the logic in getExistingAsin (and the analogous block around lines 131-143) so getTopAudibleId / getAudibleIds are not invoked when a stored ASIN is present but invalid, ensuring BookMetadata.getAsin() that is non-blank but malformed yields null instead of falling back to alternate lookups.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@booklore-api/src/main/java/org/booklore/service/metadata/parser/AudibleParser.java`:
- Line 160: In AudibleParser remove the stray System.out.println("FOUND LINK " +
href); call in the parsing logic and either delete it or replace it with the
configured logger (e.g., log.debug("FOUND LINK {}", href)) guarded by the
logger's debug level if needed; locate the print inside the AudibleParser
class/method that handles href extraction and swap to using the class logger or
remove the line entirely.
---
Duplicate comments:
In
`@booklore-api/src/main/java/org/booklore/service/metadata/parser/AmazonBookParser.java`:
- Line 54: The ASIN_PATH_PATTERN is reversed and causes extractAmazonBookId() to
prematurely discard candidates; update ASIN_PATH_PATTERN to match paths where
the ASIN appears immediately after /dp (e.g., allow "/dp/<ASIN>" or paths with
segments before /dp), then modify extractAmazonBookId() to first try extracting
ASIN from the corrected ASIN_PATH_PATTERN and only if that fails fall back to
reading the element's "data-asin" attribute (preserve existing data-asin logic).
Ensure you reference and adjust ASIN_PATH_PATTERN and the extractAmazonBookId()
method so Paperback/Hardcover hrefs that don't match the old pattern are not
dropped and the data-asin fallback is used.
- Around line 137-157: getExistingAsin currently tolerates malformed ASINs by
using ASIN_PATTERN.matcher(asin).find(), which can extract a 10-char substring
and allow fallback search; instead trim/normalize the raw stored value, treat
blank as null, and require a full-match (use matches() or anchor the pattern) so
that if a non-blank stored ASIN is invalid you return null rather than falling
back to search. Update AmazonBookParser.getExistingAsin to normalize asin (e.g.,
asin = asin.trim()), return null on blank, and only return matcher.group(1) when
the pattern fully matches; otherwise return null. Ensure the same change is
applied to the analogous method referenced around lines 159-170.
In
`@booklore-api/src/main/java/org/booklore/service/metadata/parser/AudibleParser.java`:
- Around line 109-129: getExistingAsin currently uses
ASIN_PATTERN.matcher(asin).find() which accepts a substring; change it to
normalize the stored ASIN (trim, uppercase) and require a full 10-char ASIN
match using matcher.matches() (or an anchored pattern) so malformed but
non-blank metadata returns null; update the logic in getExistingAsin (and the
analogous block around lines 131-143) so getTopAudibleId / getAudibleIds are not
invoked when a stored ASIN is present but invalid, ensuring
BookMetadata.getAsin() that is non-blank but malformed yields null instead of
falling back to alternate lookups.
🪄 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: e2b77b8a-772d-4ade-ad95-b2f5e8c1ec3a
📒 Files selected for processing (4)
booklore-api/src/main/java/org/booklore/service/metadata/parser/AmazonBookParser.javabooklore-api/src/main/java/org/booklore/service/metadata/parser/AudibleParser.javabooklore-api/src/test/java/org/booklore/service/metadata/parser/AmazonBookParserTest.javabooklore-api/src/test/java/org/booklore/service/metadata/parser/AudibleParserTest.java
📜 Review details
🧰 Additional context used
📓 Path-based instructions (2)
booklore-api/src/**/*.java
📄 CodeRabbit inference engine (AGENTS.md)
booklore-api/src/**/*.java: Use 4-space indentation in Java files and match surrounding Java style
Prefer constructor injection via Lombok patterns already used in the codebase. Do not introduce@Autowiredfield injection
Use MapStruct for entity/DTO mapping
Files:
booklore-api/src/main/java/org/booklore/service/metadata/parser/AudibleParser.javabooklore-api/src/main/java/org/booklore/service/metadata/parser/AmazonBookParser.javabooklore-api/src/test/java/org/booklore/service/metadata/parser/AudibleParserTest.javabooklore-api/src/test/java/org/booklore/service/metadata/parser/AmazonBookParserTest.java
booklore-api/src/test/**/*.java
📄 CodeRabbit inference engine (AGENTS.md)
Prefer focused unit tests; use
@SpringBootTestonly when the Spring context is required
Files:
booklore-api/src/test/java/org/booklore/service/metadata/parser/AudibleParserTest.javabooklore-api/src/test/java/org/booklore/service/metadata/parser/AmazonBookParserTest.java
🧠 Learnings (9)
📓 Common learnings
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.
📚 Learning: 2026-03-24T18:46:47.249Z
Learnt from: pedronave
Repo: grimmory-tools/grimmory PR: 173
File: booklore-api/src/main/java/org/booklore/service/hardcover/HardcoverSyncService.java:188-199
Timestamp: 2026-03-24T18:46:47.249Z
Learning: In grimmory-tools/grimmory (HardcoverSyncService.java), when a `hardcoverBookId` is stored in book metadata but is malformed (non-numeric), the preferred behavior is to return null and skip the sync rather than falling back to ISBN-based lookup. The reasoning is that an explicitly set bookId represents deliberate user intent, and silently resolving via ISBN could sync progress to a different book than intended.
Applied to files:
booklore-api/src/main/java/org/booklore/service/metadata/parser/AudibleParser.javabooklore-api/src/main/java/org/booklore/service/metadata/parser/AmazonBookParser.java
📚 Learning: 2026-04-04T15:36:55.394Z
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:55.394Z
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:
booklore-api/src/main/java/org/booklore/service/metadata/parser/AudibleParser.javabooklore-api/src/main/java/org/booklore/service/metadata/parser/AmazonBookParser.java
📚 Learning: 2026-03-24T18:46:10.869Z
Learnt from: pedronave
Repo: grimmory-tools/grimmory PR: 173
File: booklore-api/src/main/java/org/booklore/service/hardcover/HardcoverSyncService.java:220-225
Timestamp: 2026-03-24T18:46:10.869Z
Learning: In the Hardcover GraphQL API, empty strings (`""`) are not valid ISBN values and querying with `_eq: ""` returns no results. Therefore, binding a missing ISBN to `""` in `_or` edition filters in HardcoverSyncService.java is safe and will not accidentally match unrelated editions or books.
Applied to files:
booklore-api/src/main/java/org/booklore/service/metadata/parser/AudibleParser.javabooklore-api/src/main/java/org/booklore/service/metadata/parser/AmazonBookParser.java
📚 Learning: 2026-03-25T19:09:09.638Z
Learnt from: imnotjames
Repo: grimmory-tools/grimmory PR: 189
File: booklore-api/src/main/java/org/booklore/service/kobo/KoboLibrarySyncService.java:113-116
Timestamp: 2026-03-25T19:09:09.638Z
Learning: In `booklore-api/src/main/java/org/booklore/service/kobo/KoboLibrarySyncService.java`, when deciding whether to forward the `Authorization` header to the upstream Kobo server proxy on the `/v1/library/sync` endpoint, a simple null check (`request.getHeader(HttpHeaders.AUTHORIZATION) != null`) is the correct guard. The header is either absent or present — if present it may or may not be a valid token, but validity is determined by the upstream Kobo server, not by Grimmory. Blank/whitespace intermediate states do not occur in practice, and pre-validating the token value before forwarding is not the responsibility of this service.
Applied to files:
booklore-api/src/main/java/org/booklore/service/metadata/parser/AudibleParser.javabooklore-api/src/main/java/org/booklore/service/metadata/parser/AmazonBookParser.java
📚 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: 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:
booklore-api/src/main/java/org/booklore/service/metadata/parser/AudibleParser.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:
booklore-api/src/main/java/org/booklore/service/metadata/parser/AmazonBookParser.java
📚 Learning: 2026-03-24T18:58:08.199Z
Learnt from: pedronave
Repo: grimmory-tools/grimmory PR: 173
File: booklore-api/src/main/java/org/booklore/service/hardcover/HardcoverSyncService.java:291-294
Timestamp: 2026-03-24T18:58:08.199Z
Learning: In grimmory-tools/grimmory (HardcoverSyncService.java), when a Hardcover edition has no page count, falling back to the book-level page count (book.pages) is the preferred behavior. Skipping the sync entirely in this case is considered worse than using a slightly approximate total, because the page count is only a proxy for converting progress % to page numbers for the API. The Hardcover API allows setting page-based progress even when the edition lacks a page count, and progress displays in at least some UI views (e.g., the book detail dropdown).
Applied to files:
booklore-api/src/main/java/org/booklore/service/metadata/parser/AmazonBookParser.java
📚 Learning: 2026-03-26T01:46:48.863Z
Learnt from: CR
Repo: grimmory-tools/grimmory PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-26T01:46:48.863Z
Learning: Applies to booklore-api/src/test/**/*.java : Prefer focused unit tests; use `SpringBootTest` only when the Spring context is required
Applied to files:
booklore-api/src/test/java/org/booklore/service/metadata/parser/AmazonBookParserTest.java
Contributor
✅ Actions performedReviews resumed. |
96d3b53 to
832991e
Compare
832991e to
cfe53e1
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
booklore-api/src/test/java/org/booklore/service/metadata/parser/AmazonBookParserTest.java (1)
1-194: Good test coverage for the PR scope.The tests validate the key behaviors: ASIN preference, domain configuration, normalization, and fallback to search.
Consider adding tests for edge cases in future iterations:
- Empty string ASIN (
"")- Lowercase ASIN that should fail validation and trigger fallback
- ASIN with exactly 10 characters but invalid format (e.g., containing special characters within)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@booklore-api/src/test/java/org/booklore/service/metadata/parser/AmazonBookParserTest.java` around lines 1 - 194, Add three new unit tests in AmazonBookParserTest to cover edge ASIN cases: (1) fetchTopMetadata_emptyAsin_shouldFallback — use getBook("") and mockAmazonIDSearch(...) then assert amazonBookParser.fetchTopMetadata triggers Jsoup.connect("https://www.amazon.com/dp/SEARCHASIN"); (2) fetchTopMetadata_lowercaseAsin_shouldFallback — use getBook("examplesku") (lowercase) with a title and assert it falls back to search via mockAmazonIDSearch and mockJsoupConnect for SEARCHASIN; (3) fetchTopMetadata_invalid10CharAsin_shouldFallback — use getBook("ABC!@#1234") (10 chars containing special chars) and assert fallback to search; follow existing test patterns (use getBook, FetchMetadataRequest.builder(), mockAmazonIDSearch, mockJsoupConnect, and verify on Jsoup.connect) and reuse the same mocked AppSettingService and mockJsoup setup/teardown.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@booklore-api/src/test/java/org/booklore/service/metadata/parser/AmazonBookParserTest.java`:
- Around line 1-194: Add three new unit tests in AmazonBookParserTest to cover
edge ASIN cases: (1) fetchTopMetadata_emptyAsin_shouldFallback — use getBook("")
and mockAmazonIDSearch(...) then assert amazonBookParser.fetchTopMetadata
triggers Jsoup.connect("https://www.amazon.com/dp/SEARCHASIN"); (2)
fetchTopMetadata_lowercaseAsin_shouldFallback — use getBook("examplesku")
(lowercase) with a title and assert it falls back to search via
mockAmazonIDSearch and mockJsoupConnect for SEARCHASIN; (3)
fetchTopMetadata_invalid10CharAsin_shouldFallback — use getBook("ABC!@#1234")
(10 chars containing special chars) and assert fallback to search; follow
existing test patterns (use getBook, FetchMetadataRequest.builder(),
mockAmazonIDSearch, mockJsoupConnect, and verify on Jsoup.connect) and reuse the
same mocked AppSettingService and mockJsoup setup/teardown.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b5feb16b-f967-485a-bf2c-68a1c216d66e
📒 Files selected for processing (4)
booklore-api/src/main/java/org/booklore/service/metadata/parser/AmazonBookParser.javabooklore-api/src/main/java/org/booklore/service/metadata/parser/AudibleParser.javabooklore-api/src/test/java/org/booklore/service/metadata/parser/AmazonBookParserTest.javabooklore-api/src/test/java/org/booklore/service/metadata/parser/AudibleParserTest.java
🚧 Files skipped from review as they are similar to previous changes (2)
- booklore-api/src/main/java/org/booklore/service/metadata/parser/AudibleParser.java
- booklore-api/src/test/java/org/booklore/service/metadata/parser/AudibleParserTest.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: Analyze (java-kotlin)
- GitHub Check: Test Suite / Backend Tests
- GitHub Check: Test Suite / Frontend Tests
🧰 Additional context used
📓 Path-based instructions (2)
booklore-api/src/**/*.java
📄 CodeRabbit inference engine (AGENTS.md)
booklore-api/src/**/*.java: Use 4-space indentation in Java files and match surrounding Java style
Prefer constructor injection via Lombok patterns already used in the codebase. Do not introduce@Autowiredfield injection
Use MapStruct for entity/DTO mapping
Files:
booklore-api/src/main/java/org/booklore/service/metadata/parser/AmazonBookParser.javabooklore-api/src/test/java/org/booklore/service/metadata/parser/AmazonBookParserTest.java
booklore-api/src/test/**/*.java
📄 CodeRabbit inference engine (AGENTS.md)
Prefer focused unit tests; use
@SpringBootTestonly when the Spring context is required
Files:
booklore-api/src/test/java/org/booklore/service/metadata/parser/AmazonBookParserTest.java
🧠 Learnings (9)
📓 Common learnings
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.
📚 Learning: 2026-03-24T18:46:47.249Z
Learnt from: pedronave
Repo: grimmory-tools/grimmory PR: 173
File: booklore-api/src/main/java/org/booklore/service/hardcover/HardcoverSyncService.java:188-199
Timestamp: 2026-03-24T18:46:47.249Z
Learning: In grimmory-tools/grimmory (HardcoverSyncService.java), when a `hardcoverBookId` is stored in book metadata but is malformed (non-numeric), the preferred behavior is to return null and skip the sync rather than falling back to ISBN-based lookup. The reasoning is that an explicitly set bookId represents deliberate user intent, and silently resolving via ISBN could sync progress to a different book than intended.
Applied to files:
booklore-api/src/main/java/org/booklore/service/metadata/parser/AmazonBookParser.java
📚 Learning: 2026-04-04T15:36:55.394Z
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:55.394Z
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:
booklore-api/src/main/java/org/booklore/service/metadata/parser/AmazonBookParser.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:
booklore-api/src/main/java/org/booklore/service/metadata/parser/AmazonBookParser.java
📚 Learning: 2026-03-24T18:46:10.869Z
Learnt from: pedronave
Repo: grimmory-tools/grimmory PR: 173
File: booklore-api/src/main/java/org/booklore/service/hardcover/HardcoverSyncService.java:220-225
Timestamp: 2026-03-24T18:46:10.869Z
Learning: In the Hardcover GraphQL API, empty strings (`""`) are not valid ISBN values and querying with `_eq: ""` returns no results. Therefore, binding a missing ISBN to `""` in `_or` edition filters in HardcoverSyncService.java is safe and will not accidentally match unrelated editions or books.
Applied to files:
booklore-api/src/main/java/org/booklore/service/metadata/parser/AmazonBookParser.java
📚 Learning: 2026-03-25T19:09:09.638Z
Learnt from: imnotjames
Repo: grimmory-tools/grimmory PR: 189
File: booklore-api/src/main/java/org/booklore/service/kobo/KoboLibrarySyncService.java:113-116
Timestamp: 2026-03-25T19:09:09.638Z
Learning: In `booklore-api/src/main/java/org/booklore/service/kobo/KoboLibrarySyncService.java`, when deciding whether to forward the `Authorization` header to the upstream Kobo server proxy on the `/v1/library/sync` endpoint, a simple null check (`request.getHeader(HttpHeaders.AUTHORIZATION) != null`) is the correct guard. The header is either absent or present — if present it may or may not be a valid token, but validity is determined by the upstream Kobo server, not by Grimmory. Blank/whitespace intermediate states do not occur in practice, and pre-validating the token value before forwarding is not the responsibility of this service.
Applied to files:
booklore-api/src/main/java/org/booklore/service/metadata/parser/AmazonBookParser.java
📚 Learning: 2026-03-24T18:58:08.199Z
Learnt from: pedronave
Repo: grimmory-tools/grimmory PR: 173
File: booklore-api/src/main/java/org/booklore/service/hardcover/HardcoverSyncService.java:291-294
Timestamp: 2026-03-24T18:58:08.199Z
Learning: In grimmory-tools/grimmory (HardcoverSyncService.java), when a Hardcover edition has no page count, falling back to the book-level page count (book.pages) is the preferred behavior. Skipping the sync entirely in this case is considered worse than using a slightly approximate total, because the page count is only a proxy for converting progress % to page numbers for the API. The Hardcover API allows setting page-based progress even when the edition lacks a page count, and progress displays in at least some UI views (e.g., the book detail dropdown).
Applied to files:
booklore-api/src/main/java/org/booklore/service/metadata/parser/AmazonBookParser.java
📚 Learning: 2026-03-26T01:46:48.863Z
Learnt from: CR
Repo: grimmory-tools/grimmory PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-26T01:46:48.863Z
Learning: Applies to booklore-api/src/test/**/*.java : Prefer focused unit tests; use `SpringBootTest` only when the Spring context is required
Applied to files:
booklore-api/src/test/java/org/booklore/service/metadata/parser/AmazonBookParserTest.java
📚 Learning: 2026-03-26T01:46:48.863Z
Learnt from: CR
Repo: grimmory-tools/grimmory PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-26T01:46:48.863Z
Learning: Applies to booklore-api/src/**/*.java : Prefer constructor injection via Lombok patterns already used in the codebase. Do not introduce `Autowired` field injection
Applied to files:
booklore-api/src/test/java/org/booklore/service/metadata/parser/AmazonBookParserTest.java
🔀 Multi-repo context grimmory-tools/grimmory-docs
[::grimmory-tools/grimmory-docs::] docs/metadata/metadata-settings.md
- Mentions "Amazon ASIN" and Amazon provider configuration (region, cookie). Relevant: user-visible config/docs already surface ASIN as an identifier used by metadata providers.
[::grimmory-tools/grimmory-docs::] docs/metadata/metadata-fetch-configuration.md
- Mentions "Amazon ASIN, Amazon Rating, Amazon Review Count" in fetch configuration guidance.
[::grimmory-tools/grimmory-docs::] docs/metadata/amazon-cookie.md
- Amazon region/cookie guidance (hosts/domains). Relevant because parsers now prefer an explicit ASIN and domain selection is covered here.
[::grimmory-tools/grimmory-docs::] docs/metadata/metadata-center.md
- Lists Amazon ASIN and Audible ID among provider metadata fields.
[::grimmory-tools/grimmory-docs::] docs/metadata/sidecar-files.md
- Lists ASIN as an external identifier used in sidecar files.
[::grimmory-tools/grimmory-docs::] docs/magic-shelf.md
- References "asin" as a field selectable in UI filters.
Findings summary
- No code consumers, parser class names (AmazonBookParser/AudibleParser), or changed method names (getAmazonBookIds/getAudibleIds/getTopAmazonBookId/getTopAudibleId) were found in this repository. Searches returned only documentation that references ASINs, Amazon/Audible provider settings, and UI fields that expose ASIN-related data.
- Documentation items that mention ASIN, provider domains, or metadata-fetch behavior are potentially relevant to user-facing changes (the parsers now prefer stored ASINs and respect configured domains). Consider updating docs if you want to call out the new behavior (ASIN preference / search avoidance) or adjust examples that imply searches always occur.
🔇 Additional comments (17)
booklore-api/src/main/java/org/booklore/service/metadata/parser/AmazonBookParser.java (10)
36-36: LGTM!The ASIN pattern correctly matches 10 uppercase alphanumeric characters. Using a capturing group with
find()ingetExistingAsin()properly extracts valid ASINs from strings with surrounding garbage characters.
54-54: LGTM!The
/dp/path pattern now correctly matches Amazon product detail URLs.
98-104: LGTM!The updated
fetchTopMetadatanow correctly delegates togetTopAmazonBookId()for single-ID selection and gracefully handles the null case. The caller already filters null results per the relevant code snippet.
107-111: LGTM!The null-check is correctly replaced with
isEmpty()sincegetAmazonBookIds()now guarantees a non-null return.
137-157: LGTM!This method properly validates and normalizes stored ASINs:
- Guards against null at each level (
book,metadata,asin)- Uses
find()beforegroup(1)to avoidIllegalStateException- Returns null for malformed ASINs, respecting deliberate user intent rather than silently falling back
Based on learnings: "when a
hardcoverBookIdis 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."
159-171: LGTM!The method correctly implements the priority: stored ASIN first, then search fallback. The null-filtering in
getAmazonBookIds()(lines 214-216) ensuresgetFirst()won't return null when the list is non-empty.Minor note: The
!amazonBookIds.getFirst().isEmpty()check is defensive but technically redundant sinceextractAmazonBookId()only returns valid 10-character ASINs or null (which is filtered).
173-185: LGTM!Returning
Collections.emptyList()instead of null for all error/empty cases is a cleaner contract that simplifies caller code and eliminates potential NPEs.
213-217: LGTM!Properly filters null IDs and prevents duplicates in the result list.
239-250: LGTM!Using
matches()fordata-asinvalidation is appropriate since this attribute should contain a clean ASIN value. Falls back to null when invalid.
252-261: LGTM!Properly uses
find()beforegroup(1). The null/empty checks are defensive but harmless.booklore-api/src/test/java/org/booklore/service/metadata/parser/AmazonBookParserTest.java (7)
30-117: LGTM!Test class setup follows best practices:
- Uses
@ExtendWith(MockitoExtension.class)for focused unit testing (compliant with coding guidelines)- Properly manages
MockedStatic<Jsoup>lifecycle with@AfterEachcleanup- Well-organized helper methods reduce test boilerplate
119-129: LGTM!Good test validating the core PR objective: stored ASIN is used directly without performing a search.
131-142: LGTM!Validates that the configured Amazon domain is correctly used in URL construction.
144-154: LGTM!Tests ASIN normalization - the
find()pattern correctly extracts the valid ASIN from whitespace-padded input.
156-166: LGTM!Tests extraction of valid ASIN from a string containing invalid surrounding characters.
168-180: LGTM!Tests the fallback path: invalid stored ASIN triggers search, and the search result ASIN is used.
182-193: LGTM!Tests the fallback path when no ASIN is stored (null case).
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
booklore-api/src/test/java/org/booklore/service/metadata/parser/AmazonBookParserTest.java (2)
30-36: Consider adding a test for empty string ASIN.The current tests cover null ASIN and invalid (non-10-char) ASIN, but not the edge case of an empty string ASIN. Given that
getExistingAsin()checks for null but relies on the regex pattern to reject empty strings, an explicit test would document this behavior.💡 Example test case
`@Test` public void fetchTopMetadata_ignoresEmptyBookASINUsesLink() throws Exception { mockAmazonIDSearch("Example"); mockJsoupConnect("https://www.amazon.com/dp/SEARCHASIN", "<html />"); Book book = getBook(""); FetchMetadataRequest fetchMetadataRequest = FetchMetadataRequest.builder().title("Example").build(); amazonBookParser.fetchTopMetadata(book, fetchMetadataRequest); mockJsoup.verify(() -> Jsoup.connect("https://www.amazon.com/dp/SEARCHASIN")); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@booklore-api/src/test/java/org/booklore/service/metadata/parser/AmazonBookParserTest.java` around lines 30 - 36, Add a unit test in AmazonBookParserTest that verifies an empty-string ASIN is treated like an invalid ASIN and the parser falls back to searching by title/link: create a test (e.g., fetchTopMetadata_ignoresEmptyBookASINUsesLink) that uses getBook("") to produce a Book with an empty ASIN, stub mockAmazonIDSearch("Example") and mockJsoupConnect("https://www.amazon.com/dp/SEARCHASIN", "<html />"), call amazonBookParser.fetchTopMetadata(book, FetchMetadataRequest.builder().title("Example").build()), and assert via mockJsoup.verify(...) that Jsoup.connect("https://www.amazon.com/dp/SEARCHASIN") was invoked; this mirrors the null/invalid ASIN tests and documents behavior of getExistingAsin and fetchTopMetadata for empty strings.
91-106: Minor HTML typo in mock response.Line 97 has
<div div role=...>which appears to be a typo (duplicate "div"). While Jsoup parses this leniently, fixing it would make the test HTML clearer.🧹 Suggested fix
- <div div role="listitem" data-index> + <div role="listitem" data-index>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@booklore-api/src/test/java/org/booklore/service/metadata/parser/AmazonBookParserTest.java` around lines 91 - 106, In mockAmazonIDSearch, the mock HTML contains a typo "<div div role=...>" which should be a single "<div role=...>"—update the mocked response passed to mockJsoupConnect in the mockAmazonIDSearch method to remove the duplicated "div" token so the element is "<div role=\"listitem\" data-index>" to keep the test HTML clean and clearer for Jsoup parsing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@booklore-api/src/test/java/org/booklore/service/metadata/parser/AmazonBookParserTest.java`:
- Around line 30-36: Add a unit test in AmazonBookParserTest that verifies an
empty-string ASIN is treated like an invalid ASIN and the parser falls back to
searching by title/link: create a test (e.g.,
fetchTopMetadata_ignoresEmptyBookASINUsesLink) that uses getBook("") to produce
a Book with an empty ASIN, stub mockAmazonIDSearch("Example") and
mockJsoupConnect("https://www.amazon.com/dp/SEARCHASIN", "<html />"), call
amazonBookParser.fetchTopMetadata(book,
FetchMetadataRequest.builder().title("Example").build()), and assert via
mockJsoup.verify(...) that Jsoup.connect("https://www.amazon.com/dp/SEARCHASIN")
was invoked; this mirrors the null/invalid ASIN tests and documents behavior of
getExistingAsin and fetchTopMetadata for empty strings.
- Around line 91-106: In mockAmazonIDSearch, the mock HTML contains a typo "<div
div role=...>" which should be a single "<div role=...>"—update the mocked
response passed to mockJsoupConnect in the mockAmazonIDSearch method to remove
the duplicated "div" token so the element is "<div role=\"listitem\"
data-index>" to keep the test HTML clean and clearer for Jsoup parsing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 200ce667-8fa9-42eb-a941-d12c1731d808
📒 Files selected for processing (4)
booklore-api/src/main/java/org/booklore/service/metadata/parser/AmazonBookParser.javabooklore-api/src/main/java/org/booklore/service/metadata/parser/AudibleParser.javabooklore-api/src/test/java/org/booklore/service/metadata/parser/AmazonBookParserTest.javabooklore-api/src/test/java/org/booklore/service/metadata/parser/AudibleParserTest.java
🚧 Files skipped from review as they are similar to previous changes (2)
- booklore-api/src/main/java/org/booklore/service/metadata/parser/AudibleParser.java
- booklore-api/src/test/java/org/booklore/service/metadata/parser/AudibleParserTest.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)
booklore-api/src/**/*.java
📄 CodeRabbit inference engine (AGENTS.md)
booklore-api/src/**/*.java: Use 4-space indentation in Java files and match surrounding Java style
Prefer constructor injection via Lombok patterns already used in the codebase. Do not introduce@Autowiredfield injection
Use MapStruct for entity/DTO mapping
Files:
booklore-api/src/test/java/org/booklore/service/metadata/parser/AmazonBookParserTest.javabooklore-api/src/main/java/org/booklore/service/metadata/parser/AmazonBookParser.java
booklore-api/src/test/**/*.java
📄 CodeRabbit inference engine (AGENTS.md)
Prefer focused unit tests; use
@SpringBootTestonly when the Spring context is required
Files:
booklore-api/src/test/java/org/booklore/service/metadata/parser/AmazonBookParserTest.java
🧠 Learnings (8)
📓 Common learnings
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.
📚 Learning: 2026-03-26T01:46:48.863Z
Learnt from: CR
Repo: grimmory-tools/grimmory PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-26T01:46:48.863Z
Learning: Applies to booklore-api/src/test/**/*.java : Prefer focused unit tests; use `SpringBootTest` only when the Spring context is required
Applied to files:
booklore-api/src/test/java/org/booklore/service/metadata/parser/AmazonBookParserTest.java
📚 Learning: 2026-03-24T18:46:47.249Z
Learnt from: pedronave
Repo: grimmory-tools/grimmory PR: 173
File: booklore-api/src/main/java/org/booklore/service/hardcover/HardcoverSyncService.java:188-199
Timestamp: 2026-03-24T18:46:47.249Z
Learning: In grimmory-tools/grimmory (HardcoverSyncService.java), when a `hardcoverBookId` is stored in book metadata but is malformed (non-numeric), the preferred behavior is to return null and skip the sync rather than falling back to ISBN-based lookup. The reasoning is that an explicitly set bookId represents deliberate user intent, and silently resolving via ISBN could sync progress to a different book than intended.
Applied to files:
booklore-api/src/main/java/org/booklore/service/metadata/parser/AmazonBookParser.java
📚 Learning: 2026-04-04T15:36:55.394Z
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:55.394Z
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:
booklore-api/src/main/java/org/booklore/service/metadata/parser/AmazonBookParser.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:
booklore-api/src/main/java/org/booklore/service/metadata/parser/AmazonBookParser.java
📚 Learning: 2026-03-24T18:46:10.869Z
Learnt from: pedronave
Repo: grimmory-tools/grimmory PR: 173
File: booklore-api/src/main/java/org/booklore/service/hardcover/HardcoverSyncService.java:220-225
Timestamp: 2026-03-24T18:46:10.869Z
Learning: In the Hardcover GraphQL API, empty strings (`""`) are not valid ISBN values and querying with `_eq: ""` returns no results. Therefore, binding a missing ISBN to `""` in `_or` edition filters in HardcoverSyncService.java is safe and will not accidentally match unrelated editions or books.
Applied to files:
booklore-api/src/main/java/org/booklore/service/metadata/parser/AmazonBookParser.java
📚 Learning: 2026-03-24T18:58:08.199Z
Learnt from: pedronave
Repo: grimmory-tools/grimmory PR: 173
File: booklore-api/src/main/java/org/booklore/service/hardcover/HardcoverSyncService.java:291-294
Timestamp: 2026-03-24T18:58:08.199Z
Learning: In grimmory-tools/grimmory (HardcoverSyncService.java), when a Hardcover edition has no page count, falling back to the book-level page count (book.pages) is the preferred behavior. Skipping the sync entirely in this case is considered worse than using a slightly approximate total, because the page count is only a proxy for converting progress % to page numbers for the API. The Hardcover API allows setting page-based progress even when the edition lacks a page count, and progress displays in at least some UI views (e.g., the book detail dropdown).
Applied to files:
booklore-api/src/main/java/org/booklore/service/metadata/parser/AmazonBookParser.java
📚 Learning: 2026-03-25T19:09:09.638Z
Learnt from: imnotjames
Repo: grimmory-tools/grimmory PR: 189
File: booklore-api/src/main/java/org/booklore/service/kobo/KoboLibrarySyncService.java:113-116
Timestamp: 2026-03-25T19:09:09.638Z
Learning: In `booklore-api/src/main/java/org/booklore/service/kobo/KoboLibrarySyncService.java`, when deciding whether to forward the `Authorization` header to the upstream Kobo server proxy on the `/v1/library/sync` endpoint, a simple null check (`request.getHeader(HttpHeaders.AUTHORIZATION) != null`) is the correct guard. The header is either absent or present — if present it may or may not be a valid token, but validity is determined by the upstream Kobo server, not by Grimmory. Blank/whitespace intermediate states do not occur in practice, and pre-validating the token value before forwarding is not the responsibility of this service.
Applied to files:
booklore-api/src/main/java/org/booklore/service/metadata/parser/AmazonBookParser.java
🔀 Multi-repo context grimmory-tools/grimmory-docs
[::grimmory-tools/grimmory-docs::]
- docs/metadata/metadata-settings.md — documents Amazon provider config (Region, Cookie) and lists "Amazon ASIN" as a provider identifier and an enabled field. Confirms parsers should respect configured domain/cookie when constructing detail URLs.
- docs/metadata/amazon-cookie.md — describes cookie usage and troubleshooting for Amazon 503s/rate-limiting; relevant because parsers now return empty lists on 503 and use configured domain when forming detail URLs.
- docs/metadata/metadata-fetch-configuration.md — lists "Amazon ASIN" and "Audible ID" among provider-specific fields and explains provider priority/replace modes; relevant to how stored ASIN/Audible IDs will now be preferred during fetches.
- docs/metadata/metadata-center.md — shows the Metadata Center UI exposes provider identifiers (Amazon ASIN, Audible ID) and a "Fetch Metadata" flow; confirms user-visible surface where the behavioral change (using stored ASIN to avoid search) matters.
- docs/metadata/sidecar-files.md — sidecar JSON includes external identifiers (ASIN, Audible ID); parsers now preferring stored ASINs affects sidecar values and export/import behavior.
- docs/magic-shelf.md, docs/authors.md, docs/bookdrop-advanced.md, docs/library/duplicate-detection.md — multiple places reference ASIN/Audible identifiers (filtering, duplicate detection, templates); these rely on ASIN/Audible IDs being stable and consistent.
Observed absence:
- No code consumers or references to the changed class/method names (AmazonBookParser, AudibleParser, getAmazonBookIds, getTopAmazonBookId, getAudibleIds, getTopAudibleId) in this docs repository.
Implication:
- Documentation already exposes ASIN/Audible ID and Amazon/Audible domain/cookie settings. Consider updating user-facing docs to note the new behavior: stored ASIN/Audible ID will be used directly (avoiding searches) and that malformed IDs are validated/ignored (may cause fallback to search or no-op), so users should ensure ASIN/Audible IDs in sidecars/db are correct.
🔇 Additional comments (8)
booklore-api/src/main/java/org/booklore/service/metadata/parser/AmazonBookParser.java (6)
36-36: LGTM!The ASIN pattern correctly captures exactly 10 uppercase alphanumeric characters, which aligns with Amazon's ASIN format convention.
54-54: LGTM!The pattern correctly matches Amazon's
/dp/{ASIN}URL structure, addressing the previous/pd/vs/dp/mismatch issue.
98-104: LGTM!The simplified flow clearly expresses the intent: use a single validated ASIN when available, otherwise return null. This aligns with the PR objective to prefer stored ASINs.
137-171: LGTM!The ASIN extraction and validation logic properly handles all edge cases:
- Null-checks for book and metadata prevent NPEs
- Uses
matcher.find()beforegroup(1)(addressing the previous critical issue)- Returns null for malformed ASINs, which aligns with the learning that explicitly stored but invalid IDs should skip fallback to prevent syncing to unintended books
173-228: LGTM!Good defensive programming:
- Consistent non-null return (empty list on failures) simplifies caller code
- Null and duplicate filtering at lines 213-217 ensures only valid, unique ASINs are collected
230-261: LGTM!Proper ASIN extraction with validation:
- Uses
matches()for data-asin (must be exactly 10 chars)- Uses
find()for URL extraction (ASIN embedded in path)- Returns null for invalid/missing ASINs, ensuring downstream code only receives valid values
booklore-api/src/test/java/org/booklore/service/metadata/parser/AmazonBookParserTest.java (2)
120-194: LGTM!Good test coverage for the core behavioral changes:
- Validates stored ASIN is used directly when valid
- Validates domain configuration is respected
- Validates ASIN normalization (whitespace removal, character filtering via regex extraction)
- Validates fallback to search when ASIN is invalid or missing
The tests effectively document the expected behavior introduced by this PR. As per coding guidelines, this appropriately uses
@ExtendWith(MockitoExtension.class)for focused unit testing without requiring Spring context.
83-89: UsingParser.parse(html, "")is the correct and intended pattern for this test. TheParser.parse()method is a valid, documented Jsoup API. The test deliberately usesParser.parse()directly to construct the Document without introducing unnecessary mocking layers—ifJsoup.parse()were used instead, it would also need to be mocked. This pattern is used consistently inAudibleParserTest.javaas well, confirming it is intentional. No change is needed.> Likely an incorrect or invalid review comment.
zachyale
pushed a commit
to zachyale/grimmory
that referenced
this pull request
Apr 17, 2026
zachyale
pushed a commit
to zachyale/grimmory
that referenced
this pull request
Apr 17, 2026
zachyale
pushed a commit
that referenced
this pull request
Apr 17, 2026
zachyale
pushed a commit
that referenced
this pull request
Apr 22, 2026
7 tasks
dsmouse
pushed a commit
to dsmouse/grimmory
that referenced
this pull request
May 11, 2026
This was referenced May 26, 2026
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
Updates the
AmazonBookParserandAudibleParserto use existing ASIN when fetching top metadata - to match behavior ofGoodReadsParserLinked Issue: Fixes #366
Changes
AmazonBookParserto useListfor return type ofgetAmazonBookIdsAmazonBookParserAusibleParserAmazonBookParserAudibleParserSummary by CodeRabbit
Refactor
Bug Fixes
Tests