fix(api): handle kobo proxy errors during kobo sync#217
Merged
balazs-szucs merged 4 commits intoMar 27, 2026
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdd a helper to map Kobo JSON responses into Entitlement objects, make proxied requests set Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant SyncService as KoboLibrarySyncService
participant Proxy as KoboServerProxy
participant Kobo
Client->>SyncService: POST /api/.../library/sync
SyncService->>Proxy: proxyCurrentRequest(..., includeBody=true)
Proxy->>Kobo: HTTP request (Accept: application/json, Content-Type: application/json)
Kobo-->>Proxy: HTTP response (JSON 200 or error/non-JSON)
Proxy-->>SyncService: ResponseEntity<JsonNode> or null on parse/error
alt Proxy response usable (JSON)
SyncService->>SyncService: getEntitlementsFromKoboStoreResponse(response)
SyncService->>SyncService: read X_KOBO_SYNC / X_KOBO_SYNCTOKEN headers
else Proxy threw / response unusable
SyncService->>SyncService: log warning/error, skip proxy-derived updates
end
SyncService-->>Client: continue sync flow (no fatal proxy error)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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
Comment |
d5e3714 to
a7d8d9a
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
booklore-api/src/main/java/org/booklore/service/kobo/KoboServerProxy.java (1)
108-109: Potential duplicateAcceptheader when original request includes one.
HttpHeaders.ACCEPTis inHEADERS_OUT_INCLUDE(line 44), so the forwarding loop (lines 111-117) will add the original request'sAcceptheader after this hardcoded one. SinceHttpRequest.Builder.header()appends rather than replaces, the outbound request may carry both values.Consider either:
- Removing
HttpHeaders.ACCEPTfromHEADERS_OUT_INCLUDEso only the hardcoded header is sent, or- Setting the
Acceptheader after the forwarding loop to ensure it takes precedence.Option 1: Exclude Accept from forwarding
private static final Set<String> HEADERS_OUT_EXCLUDE = Set.of( - KoboHeaders.X_KOBO_SYNCTOKEN + KoboHeaders.X_KOBO_SYNCTOKEN, + HttpHeaders.ACCEPT );🤖 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/kobo/KoboServerProxy.java` around lines 108 - 109, The hardcoded Accept header appended via HttpRequest.Builder.header(HttpHeaders.ACCEPT, "application/json") can be duplicated because HEADERS_OUT_INCLUDE contains HttpHeaders.ACCEPT and the forwarding loop copies original headers; to fix, either remove HttpHeaders.ACCEPT from the HEADERS_OUT_INCLUDE collection or move/set the Accept header after the forwarding loop so it replaces/overrides the forwarded value; update the code around HEADERS_OUT_INCLUDE usage and the HttpRequest.Builder.header(...) call in KoboServerProxy (the header-forwarding loop and the builder.accept/header call) accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@booklore-api/src/main/java/org/booklore/service/kobo/KoboServerProxy.java`:
- Around line 108-109: The hardcoded Accept header appended via
HttpRequest.Builder.header(HttpHeaders.ACCEPT, "application/json") can be
duplicated because HEADERS_OUT_INCLUDE contains HttpHeaders.ACCEPT and the
forwarding loop copies original headers; to fix, either remove
HttpHeaders.ACCEPT from the HEADERS_OUT_INCLUDE collection or move/set the
Accept header after the forwarding loop so it replaces/overrides the forwarded
value; update the code around HEADERS_OUT_INCLUDE usage and the
HttpRequest.Builder.header(...) call in KoboServerProxy (the header-forwarding
loop and the builder.accept/header call) accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4b537e3a-3add-4346-af37-d81b1a6c888d
📒 Files selected for processing (2)
booklore-api/src/main/java/org/booklore/service/kobo/KoboLibrarySyncService.javabooklore-api/src/main/java/org/booklore/service/kobo/KoboServerProxy.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/kobo/KoboServerProxy.javabooklore-api/src/main/java/org/booklore/service/kobo/KoboLibrarySyncService.java
🧠 Learnings (4)
📓 Common learnings
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.
📚 Learning: 2026-03-26T02:20:22.468Z
Learnt from: imnotjames
Repo: grimmory-tools/grimmory PR: 198
File: booklore-api/src/main/java/org/booklore/controller/KoboController.java:100-103
Timestamp: 2026-03-26T02:20:22.468Z
Learning: In `booklore-api/src/main/java/org/booklore/controller/KoboController.java`, the Kobo reader device already makes direct outbound connections to Kobo's CDN/servers for other requests (confirmed by imnotjames via testing). Therefore, returning a `307 TEMPORARY_REDIRECT` to `cdn.kobo.com` for non-`BL-` book cover thumbnails (instead of server-side proxying) does NOT introduce a new privacy or behavioral regression — the Kobo device's IP and User-Agent are already exposed to Kobo through other direct requests.
Applied to files:
booklore-api/src/main/java/org/booklore/service/kobo/KoboServerProxy.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/kobo/KoboServerProxy.javabooklore-api/src/main/java/org/booklore/service/kobo/KoboLibrarySyncService.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/kobo/KoboLibrarySyncService.java
🔇 Additional comments (2)
booklore-api/src/main/java/org/booklore/service/kobo/KoboLibrarySyncService.java (2)
42-65: Clean extraction of entitlement mapping logic.The helper method properly handles null body, iterates through the array, and returns an empty collection on failure. The logging for unknown entitlement types at line 54 is helpful for debugging.
138-151: Graceful error handling aligns with PR objectives.The try/catch wrapper correctly isolates the Kobo proxy call, allowing sync to complete with local entitlements when upstream fails. The warning-level log at line 150 is appropriate since this is a recoverable condition.
One minor note: if
tokenGenerator.fromBase64()(line 148) throws afterentitlements.addAll()(line 141) succeeds, the response will include Kobo entitlements but with a stalesyncToken. This is acceptable since the next sync will re-request from Kobo.
a7d8d9a to
22a9b24
Compare
5d6e4ce to
4320481
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
booklore-api/src/main/java/org/booklore/service/kobo/KoboLibrarySyncService.java (1)
45-61: Keep one bad entitlement from dropping the whole upstream payload.The outer
catchmakes this helper all-or-nothing: if onetreeToValue(...)fails, every entitlement already parsed from the response is discarded. Since this branch is now best-effort anyway, it would be safer to log and skip only the malformed item.Possible refactor
- .map(body -> { - try { - List<Entitlement> results = new ArrayList<>(); - if (body.isArray()) { - for (JsonNode node : body) { - if (node.has("NewEntitlement")) { - results.add(objectMapper.treeToValue(node, NewEntitlement.class)); - } else if (node.has("ChangedEntitlement")) { - results.add(objectMapper.treeToValue(node, ChangedEntitlement.class)); - } else { - log.warn("Unknown entitlement type in Kobo response: {}", node); - } - } - } - return results; - } catch (Exception e) { - log.error("Failed to map Kobo response to Entitlement objects", e); - return Collections.<Entitlement>emptyList(); - } - }) + .map(body -> { + List<Entitlement> results = new ArrayList<>(); + if (body.isArray()) { + for (JsonNode node : body) { + try { + if (node.has("NewEntitlement")) { + results.add(objectMapper.treeToValue(node, NewEntitlement.class)); + } else if (node.has("ChangedEntitlement")) { + results.add(objectMapper.treeToValue(node, ChangedEntitlement.class)); + } else { + log.warn("Unknown entitlement type in Kobo response: {}", node); + } + } catch (Exception e) { + log.warn("Skipping malformed entitlement from Kobo response: {}", node, e); + } + } + } + return results; + })🤖 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/kobo/KoboLibrarySyncService.java` around lines 45 - 61, The current mapping in KoboLibrarySyncService swallows all parsed entitlements if any single objectMapper.treeToValue(...) throws; change the logic so each JsonNode is parsed inside its own try/catch (around the objectMapper.treeToValue call) and on parse failure log the node and exception (e.g., log.warn or log.error with the node and e) and continue, adding only successfully parsed NewEntitlement or ChangedEntitlement instances to the results List<Entitlement>; keep or narrow any outer catch for truly unexpected failures but do not return Collections.emptyList() just because one item failed so that valid entitlements already parsed are preserved.
🤖 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/kobo/KoboLibrarySyncService.java`:
- Around line 143-149: The code updates shouldContinueSync before decoding the
sync token, allowing a failed token parse (tokenGenerator.fromBase64) to leave
shouldContinueSync set to "continue"; change the order and logic in the try
block in KoboLibrarySyncService so you first read and decode the
X_KOBO_SYNCTOKEN into syncToken (using tokenGenerator.fromBase64 and only
assigning to syncToken on success), and only after the token decode succeeds
assign shouldContinueSync from
koboStoreResponse.getHeaders().getFirst(KoboHeaders.X_KOBO_SYNC); ensure
exceptions from tokenGenerator.fromBase64 prevent mutating shouldContinueSync
and are caught as before.
In `@booklore-api/src/main/java/org/booklore/service/kobo/KoboServerProxy.java`:
- Around line 107-108: The Accept: application/json header is being forced for
all proxied requests in proxyCurrentRequest(), which breaks non-JSON endpoints;
change the code so the Accept header is only set to "application/json" when
proxying the specific sync/init routes (e.g., detect request.getRequestURI() or
path info contains "/api/v2/sync" or "/api/v2/sync/init") or when the incoming
request has no Accept header. Update proxyCurrentRequest() to conditionally add
.header(HttpHeaders.ACCEPT, "application/json") only for those sync/init paths
(leave other requests to forward the original Accept or omit it).
---
Nitpick comments:
In
`@booklore-api/src/main/java/org/booklore/service/kobo/KoboLibrarySyncService.java`:
- Around line 45-61: The current mapping in KoboLibrarySyncService swallows all
parsed entitlements if any single objectMapper.treeToValue(...) throws; change
the logic so each JsonNode is parsed inside its own try/catch (around the
objectMapper.treeToValue call) and on parse failure log the node and exception
(e.g., log.warn or log.error with the node and e) and continue, adding only
successfully parsed NewEntitlement or ChangedEntitlement instances to the
results List<Entitlement>; keep or narrow any outer catch for truly unexpected
failures but do not return Collections.emptyList() just because one item failed
so that valid entitlements already parsed are preserved.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 152a6d63-6c5c-4df2-bbbc-442bd5c3b986
📒 Files selected for processing (2)
booklore-api/src/main/java/org/booklore/service/kobo/KoboLibrarySyncService.javabooklore-api/src/main/java/org/booklore/service/kobo/KoboServerProxy.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/kobo/KoboServerProxy.javabooklore-api/src/main/java/org/booklore/service/kobo/KoboLibrarySyncService.java
🧠 Learnings (3)
📓 Common learnings
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.
📚 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/kobo/KoboServerProxy.javabooklore-api/src/main/java/org/booklore/service/kobo/KoboLibrarySyncService.java
📚 Learning: 2026-03-26T02:20:22.468Z
Learnt from: imnotjames
Repo: grimmory-tools/grimmory PR: 198
File: booklore-api/src/main/java/org/booklore/controller/KoboController.java:100-103
Timestamp: 2026-03-26T02:20:22.468Z
Learning: In `booklore-api/src/main/java/org/booklore/controller/KoboController.java`, the Kobo reader device already makes direct outbound connections to Kobo's CDN/servers for other requests (confirmed by imnotjames via testing). Therefore, returning a `307 TEMPORARY_REDIRECT` to `cdn.kobo.com` for non-`BL-` book cover thumbnails (instead of server-side proxying) does NOT introduce a new privacy or behavioral regression — the Kobo device's IP and User-Agent are already exposed to Kobo through other direct requests.
Applied to files:
booklore-api/src/main/java/org/booklore/service/kobo/KoboServerProxy.java
59ab0db to
a77f813
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
booklore-api/src/main/java/org/booklore/service/kobo/KoboLibrarySyncService.java (1)
151-155:⚠️ Potential issue | 🟠 MajorKeep upstream sync-header parsing non-fatal to avoid reintroducing 500s.
At Line 152,
tokenGenerator.fromBase64(upstreamKoboSyncTokenHeader)can throw and currently escapes, which can still abort sync despite the new best-effort proxy guard. This should be contained so proxy/header-parse issues remain non-fatal.💡 Suggested defensive fix
- if (upstreamKoboSyncTokenHeader != null) { - syncToken = tokenGenerator.fromBase64(upstreamKoboSyncTokenHeader); - } - - shouldContinueSync = "continue".equalsIgnoreCase(upstreamContinueSyncHeader); + try { + if (upstreamKoboSyncTokenHeader != null) { + BookloreSyncToken decoded = tokenGenerator.fromBase64(upstreamKoboSyncTokenHeader); + if (decoded != null) { + syncToken = decoded; + } + } + shouldContinueSync = "continue".equalsIgnoreCase(upstreamContinueSyncHeader); + } catch (Exception e) { + log.warn("Failed to parse upstream Kobo sync headers; continuing with local sync state", e); + }🤖 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/kobo/KoboLibrarySyncService.java` around lines 151 - 155, Parsing the upstream sync token with tokenGenerator.fromBase64(upstreamKoboSyncTokenHeader) can throw and currently lets exceptions escape; make header parsing non-fatal by wrapping the call in a try/catch that logs a debug/warn (including the header value or parse error) and leaves syncToken null/unchanged if parsing fails, and similarly defensively parse upstreamContinueSyncHeader into shouldContinueSync so header parse errors cannot abort the sync flow (use upstreamKoboSyncTokenHeader, tokenGenerator.fromBase64, syncToken, upstreamContinueSyncHeader, shouldContinueSync to locate the spots).
🤖 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/kobo/KoboLibrarySyncService.java`:
- Around line 151-155: Parsing the upstream sync token with
tokenGenerator.fromBase64(upstreamKoboSyncTokenHeader) can throw and currently
lets exceptions escape; make header parsing non-fatal by wrapping the call in a
try/catch that logs a debug/warn (including the header value or parse error) and
leaves syncToken null/unchanged if parsing fails, and similarly defensively
parse upstreamContinueSyncHeader into shouldContinueSync so header parse errors
cannot abort the sync flow (use upstreamKoboSyncTokenHeader,
tokenGenerator.fromBase64, syncToken, upstreamContinueSyncHeader,
shouldContinueSync to locate the spots).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c1bf0497-aedc-4429-a4f8-888810774a11
📒 Files selected for processing (2)
booklore-api/src/main/java/org/booklore/service/kobo/KoboLibrarySyncService.javabooklore-api/src/main/java/org/booklore/service/kobo/KoboServerProxy.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/kobo/KoboServerProxy.javabooklore-api/src/main/java/org/booklore/service/kobo/KoboLibrarySyncService.java
🧠 Learnings (6)
📓 Common learnings
Learnt from: imnotjames
Repo: grimmory-tools/grimmory PR: 217
File: booklore-api/src/main/java/org/booklore/service/kobo/KoboServerProxy.java:107-108
Timestamp: 2026-03-26T05:52:04.269Z
Learning: In `booklore-api/src/main/java/org/booklore/service/kobo/KoboServerProxy.java`, the `executeProxyRequest()` method unconditionally parses every proxy response body as JSON via `objectMapper.readTree(response.body())`. Therefore, setting `Accept: application/json` on all outbound proxy requests is intentional and consistent — non-JSON responses from Kobo would already cause a Jackson parse exception. The Accept header is forced to `application/json` to encourage Kobo's server to return JSON-formatted error responses (rather than HTML), preventing parse failures on error paths like during sync.
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.
📚 Learning: 2026-03-26T05:52:04.269Z
Learnt from: imnotjames
Repo: grimmory-tools/grimmory PR: 217
File: booklore-api/src/main/java/org/booklore/service/kobo/KoboServerProxy.java:107-108
Timestamp: 2026-03-26T05:52:04.269Z
Learning: In `booklore-api/src/main/java/org/booklore/service/kobo/KoboServerProxy.java`, the `executeProxyRequest()` method unconditionally parses every proxy response body as JSON via `objectMapper.readTree(response.body())`. Therefore, setting `Accept: application/json` on all outbound proxy requests is intentional and consistent — non-JSON responses from Kobo would already cause a Jackson parse exception. The Accept header is forced to `application/json` to encourage Kobo's server to return JSON-formatted error responses (rather than HTML), preventing parse failures on error paths like during sync.
Applied to files:
booklore-api/src/main/java/org/booklore/service/kobo/KoboServerProxy.javabooklore-api/src/main/java/org/booklore/service/kobo/KoboLibrarySyncService.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/kobo/KoboServerProxy.javabooklore-api/src/main/java/org/booklore/service/kobo/KoboLibrarySyncService.java
📚 Learning: 2026-03-26T02:20:22.468Z
Learnt from: imnotjames
Repo: grimmory-tools/grimmory PR: 198
File: booklore-api/src/main/java/org/booklore/controller/KoboController.java:100-103
Timestamp: 2026-03-26T02:20:22.468Z
Learning: In `booklore-api/src/main/java/org/booklore/controller/KoboController.java`, the Kobo reader device already makes direct outbound connections to Kobo's CDN/servers for other requests (confirmed by imnotjames via testing). Therefore, returning a `307 TEMPORARY_REDIRECT` to `cdn.kobo.com` for non-`BL-` book cover thumbnails (instead of server-side proxying) does NOT introduce a new privacy or behavioral regression — the Kobo device's IP and User-Agent are already exposed to Kobo through other direct requests.
Applied to files:
booklore-api/src/main/java/org/booklore/service/kobo/KoboServerProxy.javabooklore-api/src/main/java/org/booklore/service/kobo/KoboLibrarySyncService.java
📚 Learning: 2026-03-25T21:02:55.427Z
Learnt from: imnotjames
Repo: grimmory-tools/grimmory PR: 195
File: booklore-api/src/main/java/org/booklore/config/logging/RequestLoggingFilterConfig.java:15-19
Timestamp: 2026-03-25T21:02:55.427Z
Learning: In `booklore-api/src/main/java/org/booklore/config/logging/RequestLoggingFilterConfig.java` and `booklore-api/src/main/java/org/booklore/config/logging/filter/RequestLoggingFilter.java`, logging all request headers and payloads at DEBUG level is intentional and accepted behavior in this project. The previous `LoggingFilter` also logged all headers. No header redaction or payload scrubbing is required — the DEBUG log level is considered sufficient access control. The new filter is not profile-gated (unlike the old `Profile("dev")` filter) but relies on `logger.isDebugEnabled()` for gating.
Applied to files:
booklore-api/src/main/java/org/booklore/service/kobo/KoboServerProxy.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/kobo/KoboLibrarySyncService.java
🔇 Additional comments (2)
booklore-api/src/main/java/org/booklore/service/kobo/KoboServerProxy.java (1)
107-108: Forced JSON accept/content headers are consistent with proxy behavior.No issue here — this matches the proxy’s current JSON-only response parsing path and reduces HTML error-body parse failures during sync flows.
Based on learnings:
executeProxyRequest()already unconditionally parses proxy responses as JSON, so forcingAccept: application/jsonis intentional for this path.booklore-api/src/main/java/org/booklore/service/kobo/KoboLibrarySyncService.java (1)
42-65: Entitlement extraction helper is a good resiliency improvement.Null-safe body handling and controlled fallback on mapping errors are solid for non-fatal sync behavior.
when the kobo proxy fails, we no longer fail the sync entirely and instead just skip the kobo request
a77f813 to
a1ab627
Compare
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/kobo/KoboLibrarySyncService.java`:
- Around line 151-153: The upstream sync-token decode is unguarded: wrap the
tokenGenerator.fromBase64(upstreamKoboSyncTokenHeader) call in a try/catch
inside KoboLibrarySyncService so a malformed upstream token does not overwrite
or nullify the existing syncToken; only assign to syncToken if decoding
succeeds, otherwise log a warning including the raw header and error and leave
the current local syncToken unchanged so sync can continue.
🪄 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: 53ae274a-7312-4773-97a6-2f83292197dd
📒 Files selected for processing (2)
booklore-api/src/main/java/org/booklore/service/kobo/KoboLibrarySyncService.javabooklore-api/src/main/java/org/booklore/service/kobo/KoboServerProxy.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/kobo/KoboServerProxy.javabooklore-api/src/main/java/org/booklore/service/kobo/KoboLibrarySyncService.java
🧠 Learnings (6)
📓 Common learnings
Learnt from: imnotjames
Repo: grimmory-tools/grimmory PR: 217
File: booklore-api/src/main/java/org/booklore/service/kobo/KoboServerProxy.java:107-108
Timestamp: 2026-03-26T05:52:04.269Z
Learning: In `booklore-api/src/main/java/org/booklore/service/kobo/KoboServerProxy.java`, the `executeProxyRequest()` method unconditionally parses every proxy response body as JSON via `objectMapper.readTree(response.body())`. Therefore, setting `Accept: application/json` on all outbound proxy requests is intentional and consistent — non-JSON responses from Kobo would already cause a Jackson parse exception. The Accept header is forced to `application/json` to encourage Kobo's server to return JSON-formatted error responses (rather than HTML), preventing parse failures on error paths like during sync.
📚 Learning: 2026-03-26T05:52:04.269Z
Learnt from: imnotjames
Repo: grimmory-tools/grimmory PR: 217
File: booklore-api/src/main/java/org/booklore/service/kobo/KoboServerProxy.java:107-108
Timestamp: 2026-03-26T05:52:04.269Z
Learning: In `booklore-api/src/main/java/org/booklore/service/kobo/KoboServerProxy.java`, the `executeProxyRequest()` method unconditionally parses every proxy response body as JSON via `objectMapper.readTree(response.body())`. Therefore, setting `Accept: application/json` on all outbound proxy requests is intentional and consistent — non-JSON responses from Kobo would already cause a Jackson parse exception. The Accept header is forced to `application/json` to encourage Kobo's server to return JSON-formatted error responses (rather than HTML), preventing parse failures on error paths like during sync.
Applied to files:
booklore-api/src/main/java/org/booklore/service/kobo/KoboServerProxy.javabooklore-api/src/main/java/org/booklore/service/kobo/KoboLibrarySyncService.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/kobo/KoboServerProxy.javabooklore-api/src/main/java/org/booklore/service/kobo/KoboLibrarySyncService.java
📚 Learning: 2026-03-26T02:20:22.468Z
Learnt from: imnotjames
Repo: grimmory-tools/grimmory PR: 198
File: booklore-api/src/main/java/org/booklore/controller/KoboController.java:100-103
Timestamp: 2026-03-26T02:20:22.468Z
Learning: In `booklore-api/src/main/java/org/booklore/controller/KoboController.java`, the Kobo reader device already makes direct outbound connections to Kobo's CDN/servers for other requests (confirmed by imnotjames via testing). Therefore, returning a `307 TEMPORARY_REDIRECT` to `cdn.kobo.com` for non-`BL-` book cover thumbnails (instead of server-side proxying) does NOT introduce a new privacy or behavioral regression — the Kobo device's IP and User-Agent are already exposed to Kobo through other direct requests.
Applied to files:
booklore-api/src/main/java/org/booklore/service/kobo/KoboServerProxy.javabooklore-api/src/main/java/org/booklore/service/kobo/KoboLibrarySyncService.java
📚 Learning: 2026-03-25T21:02:55.427Z
Learnt from: imnotjames
Repo: grimmory-tools/grimmory PR: 195
File: booklore-api/src/main/java/org/booklore/config/logging/RequestLoggingFilterConfig.java:15-19
Timestamp: 2026-03-25T21:02:55.427Z
Learning: In `booklore-api/src/main/java/org/booklore/config/logging/RequestLoggingFilterConfig.java` and `booklore-api/src/main/java/org/booklore/config/logging/filter/RequestLoggingFilter.java`, logging all request headers and payloads at DEBUG level is intentional and accepted behavior in this project. The previous `LoggingFilter` also logged all headers. No header redaction or payload scrubbing is required — the DEBUG log level is considered sufficient access control. The new filter is not profile-gated (unlike the old `Profile("dev")` filter) but relies on `logger.isDebugEnabled()` for gating.
Applied to files:
booklore-api/src/main/java/org/booklore/service/kobo/KoboServerProxy.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/kobo/KoboLibrarySyncService.java
🔇 Additional comments (3)
booklore-api/src/main/java/org/booklore/service/kobo/KoboServerProxy.java (1)
107-108: LGTM: forced JSONAcceptis consistent with current proxy behavior.This change aligns with the proxy’s JSON-only response parsing path and helps avoid HTML error payloads breaking sync flows.
Based on learnings:
executeProxyRequest()already unconditionally parses proxy bodies as JSON, so forcingAccept: application/jsonis intentional to reduce non-JSON error responses.booklore-api/src/main/java/org/booklore/service/kobo/KoboLibrarySyncService.java (2)
42-65: Good resilience improvement in entitlement mapping.Returning an empty collection on mapping failure keeps sync non-fatal and matches the PR objective for graceful degradation.
138-143: Nice fallback behavior for upstream proxy failures.Catching proxy exceptions here prevents the sync route from failing hard when Kobo upstream is unavailable.
balazs-szucs
approved these changes
Mar 27, 2026
This was referenced Mar 30, 2026
This was referenced Apr 14, 2026
This was referenced Apr 14, 2026
zachyale
pushed a commit
to zachyale/grimmory
that referenced
this pull request
Apr 17, 2026
* fix(api): handle kobo proxy sync failures when the kobo proxy fails, we no longer fail the sync entirely and instead just skip the kobo request * fix(api): always `Accept` application/json * fix(api): handle edge case for an exception being impossibly raised * fix(api): update try/catch to be just proxy
zachyale
pushed a commit
to zachyale/grimmory
that referenced
this pull request
Apr 17, 2026
* fix(api): handle kobo proxy sync failures when the kobo proxy fails, we no longer fail the sync entirely and instead just skip the kobo request * fix(api): always `Accept` application/json * fix(api): handle edge case for an exception being impossibly raised * fix(api): update try/catch to be just proxy
zachyale
pushed a commit
that referenced
this pull request
Apr 17, 2026
* fix(api): handle kobo proxy sync failures when the kobo proxy fails, we no longer fail the sync entirely and instead just skip the kobo request * fix(api): always `Accept` application/json * fix(api): handle edge case for an exception being impossibly raised * fix(api): update try/catch to be just proxy
zachyale
pushed a commit
that referenced
this pull request
Apr 22, 2026
* fix(api): handle kobo proxy sync failures when the kobo proxy fails, we no longer fail the sync entirely and instead just skip the kobo request * fix(api): always `Accept` application/json * fix(api): handle edge case for an exception being impossibly raised * fix(api): update try/catch to be just proxy
This was referenced May 25, 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
Whenever Kobo servers error out the kobo sync fails to process with an HTTP 500. This is because Kobo servers respond with HTML. In the sync routes we try to combine the kobo response with our response whcih fails.
This corrects the issue by wrapping the sync code that makes a proxy request in a try / catch & handling JSON parse failures in the kobo proxy requests more gracefully.
Linked Issue: Fixes #216
Changes
Testing Strategy
Tested with a Kobo Clara BW to do standard sync operations & file downloading.
Summary by CodeRabbit
Bug Fixes
Refactor