fix(repositories): add automatic flushing to delete methods in various repositories#791
fix(repositories): add automatic flushing to delete methods in various repositories#791balazs-szucs wants to merge 2 commits intogrimmory-tools:developfrom
Conversation
📝 WalkthroughWalkthroughSystematically converts derived delete and update repository methods across multiple repositories to explicit JPQL queries or updates, adds/adjusts Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
backend/src/main/java/org/booklore/repository/RefreshTokenRepository.java (1)
17-20: Consider matching refresh tokens by user ID instead of entity instance.The new bulk delete uses
t.user = :user, while the established user-delete pattern inUserContentRestrictionRepository.java:26-29usesr.user.id = :userId. Matching by ID avoids entity-parameter binding in a bulk DML query and keeps user-scoped deletes consistent.Optional consistency refactor
- `@Query`("DELETE FROM RefreshTokenEntity t WHERE t.user = :user") - void deleteByUser(`@Param`("user") BookLoreUserEntity user); + `@Query`("DELETE FROM RefreshTokenEntity t WHERE t.user.id = :userId") + void deleteByUserId(`@Param`("userId") Long userId);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/main/java/org/booklore/repository/RefreshTokenRepository.java` around lines 17 - 20, The deleteByUser bulk DML should match by the user's ID rather than binding an entity instance: change the RefreshTokenRepository method deleteByUser to accept a userId (e.g., Long userId) and update the `@Query` to "DELETE FROM RefreshTokenEntity t WHERE t.user.id = :userId" with the corresponding `@Param`("userId") to avoid entity-parameter binding and align with the user-scoped delete pattern used elsewhere.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@backend/src/main/java/org/booklore/repository/UserContentRestrictionRepository.java`:
- Around line 26-34: The bulk-delete methods in UserContentRestrictionRepository
(deleteByUserId and deleteByUserIdAndRestrictionType) use
clearAutomatically=true which clears the persistence context and detaches the
BookLoreUserEntity loaded in ContentRestrictionService.updateRestrictions; to
fix, either remove clearAutomatically from these `@Modifying` annotations so the
persistence context is not cleared, or (if you must keep clearAutomatically)
reload the user entity (e.g., re-fetch via UserRepository.findById or
entityManager.find) inside ContentRestrictionService.updateRestrictions
immediately after calling deleteByUserId before constructing new
UserContentRestrictionEntity instances; update the repository annotations or add
the user reload in the service accordingly.
---
Nitpick comments:
In `@backend/src/main/java/org/booklore/repository/RefreshTokenRepository.java`:
- Around line 17-20: The deleteByUser bulk DML should match by the user's ID
rather than binding an entity instance: change the RefreshTokenRepository method
deleteByUser to accept a userId (e.g., Long userId) and update the `@Query` to
"DELETE FROM RefreshTokenEntity t WHERE t.user.id = :userId" with the
corresponding `@Param`("userId") to avoid entity-parameter binding and align with
the user-scoped delete pattern used elsewhere.
🪄 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: 1991f872-6b4c-4178-8df2-35ef2eab63e0
📒 Files selected for processing (13)
backend/src/main/java/org/booklore/repository/AnnotationRepository.javabackend/src/main/java/org/booklore/repository/BookMetadataRepository.javabackend/src/main/java/org/booklore/repository/BookNoteV2Repository.javabackend/src/main/java/org/booklore/repository/BookdropFileRepository.javabackend/src/main/java/org/booklore/repository/ComicCreatorMappingRepository.javabackend/src/main/java/org/booklore/repository/EmailRecipientV2Repository.javabackend/src/main/java/org/booklore/repository/KoboDeletedBookProgressRepository.javabackend/src/main/java/org/booklore/repository/OidcSessionRepository.javabackend/src/main/java/org/booklore/repository/PdfAnnotationRepository.javabackend/src/main/java/org/booklore/repository/RefreshTokenRepository.javabackend/src/main/java/org/booklore/repository/UserContentRestrictionRepository.javabackend/src/main/java/org/booklore/repository/UserEmailProviderPreferenceRepository.javabackend/src/main/java/org/booklore/task/tasks/TempFetchedMetadataCleanupTask.java
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
backend/src/**/*.java
📄 CodeRabbit inference engine (AGENTS.md)
backend/src/**/*.java: Use 4-space indentation and match surrounding Java style in backend code
Prefer constructor injection via Lombok patterns already used in the codebase. Do not introduce@Autowiredfield injection in backend code
Use MapStruct for entity/DTO mapping in backend code
Keep JPA entities on the *Entity suffix in backend code
Files:
backend/src/main/java/org/booklore/repository/KoboDeletedBookProgressRepository.javabackend/src/main/java/org/booklore/repository/EmailRecipientV2Repository.javabackend/src/main/java/org/booklore/repository/BookMetadataRepository.javabackend/src/main/java/org/booklore/repository/BookdropFileRepository.javabackend/src/main/java/org/booklore/task/tasks/TempFetchedMetadataCleanupTask.javabackend/src/main/java/org/booklore/repository/ComicCreatorMappingRepository.javabackend/src/main/java/org/booklore/repository/RefreshTokenRepository.javabackend/src/main/java/org/booklore/repository/UserEmailProviderPreferenceRepository.javabackend/src/main/java/org/booklore/repository/PdfAnnotationRepository.javabackend/src/main/java/org/booklore/repository/BookNoteV2Repository.javabackend/src/main/java/org/booklore/repository/AnnotationRepository.javabackend/src/main/java/org/booklore/repository/OidcSessionRepository.javabackend/src/main/java/org/booklore/repository/UserContentRestrictionRepository.java
🧠 Learnings (2)
📚 Learning: 2026-04-10T08:15:37.436Z
Learnt from: imnotjames
Repo: grimmory-tools/grimmory PR: 449
File: booklore-api/src/main/java/org/booklore/service/book/BookDownloadService.java:139-145
Timestamp: 2026-04-10T08:15:37.436Z
Learning: When using Spring `ContentDisposition.builder(...).filename(name, StandardCharsets.UTF_8).build()` (i.e., explicitly providing UTF-8), the resulting header value should include both the quoted `filename="=?UTF-8?..."` and the RFC 5987 `filename*=` parameters. In this case, any extra ASCII fallback computation (e.g., deriving an ASCII `fallbackFilename` via `NON_ASCII_PATTERN` and calling `.filename(fallbackFilename)`) is likely redundant—prefer calling only `.filename(fallbackName?, StandardCharsets.UTF_8)` as appropriate and let Spring handle the UTF-8 header parameters. Verify by comparing the emitted header for `filename` and `filename*` before deciding to keep an ASCII fallback.
Applied to files:
backend/src/main/java/org/booklore/repository/KoboDeletedBookProgressRepository.javabackend/src/main/java/org/booklore/repository/EmailRecipientV2Repository.javabackend/src/main/java/org/booklore/repository/BookMetadataRepository.javabackend/src/main/java/org/booklore/repository/BookdropFileRepository.javabackend/src/main/java/org/booklore/task/tasks/TempFetchedMetadataCleanupTask.javabackend/src/main/java/org/booklore/repository/ComicCreatorMappingRepository.javabackend/src/main/java/org/booklore/repository/RefreshTokenRepository.javabackend/src/main/java/org/booklore/repository/UserEmailProviderPreferenceRepository.javabackend/src/main/java/org/booklore/repository/PdfAnnotationRepository.javabackend/src/main/java/org/booklore/repository/BookNoteV2Repository.javabackend/src/main/java/org/booklore/repository/AnnotationRepository.javabackend/src/main/java/org/booklore/repository/OidcSessionRepository.javabackend/src/main/java/org/booklore/repository/UserContentRestrictionRepository.java
📚 Learning: 2026-04-14T12:43:08.698Z
Learnt from: balazs-szucs
Repo: grimmory-tools/grimmory PR: 502
File: booklore-api/src/main/java/org/booklore/service/reader/ChapterCacheService.java:0-0
Timestamp: 2026-04-14T12:43:08.698Z
Learning: For this codebase (booklore-api), target Java 25 with `--enable-preview`, so `_` is intentionally used as an unnamed/ignored variable (e.g., lambda parameter or pattern variable) per Java’s preview feature JEP 456. Do not flag `_` in those contexts as an invalid/reserved identifier; only flag it if it’s used in a non-supported position (e.g., where an unnamed variable is not applicable for the Java preview rules).
Applied to files:
backend/src/main/java/org/booklore/repository/KoboDeletedBookProgressRepository.javabackend/src/main/java/org/booklore/repository/EmailRecipientV2Repository.javabackend/src/main/java/org/booklore/repository/BookMetadataRepository.javabackend/src/main/java/org/booklore/repository/BookdropFileRepository.javabackend/src/main/java/org/booklore/task/tasks/TempFetchedMetadataCleanupTask.javabackend/src/main/java/org/booklore/repository/ComicCreatorMappingRepository.javabackend/src/main/java/org/booklore/repository/RefreshTokenRepository.javabackend/src/main/java/org/booklore/repository/UserEmailProviderPreferenceRepository.javabackend/src/main/java/org/booklore/repository/PdfAnnotationRepository.javabackend/src/main/java/org/booklore/repository/BookNoteV2Repository.javabackend/src/main/java/org/booklore/repository/AnnotationRepository.javabackend/src/main/java/org/booklore/repository/OidcSessionRepository.javabackend/src/main/java/org/booklore/repository/UserContentRestrictionRepository.java
🔇 Additional comments (11)
backend/src/main/java/org/booklore/task/tasks/TempFetchedMetadataCleanupTask.java (1)
74-75: LGTM —count()preserves the intended pending-row count.Using the inherited repository
count()keeps the metadata calculation equivalent while removing the redundant custom count method.backend/src/main/java/org/booklore/repository/BookMetadataRepository.java (1)
18-26: LGTM — the bulk timestamp updates now use the intended flush/clear behavior.The annotation change is scoped to the two JPQL
UPDATEmethods and does not alter their predicates or signatures.backend/src/main/java/org/booklore/repository/KoboDeletedBookProgressRepository.java (1)
14-17: LGTM — this matches the new modifying-query pattern.The existing bulk
DELETEremains transactional and now flushes/clears consistently.backend/src/main/java/org/booklore/repository/BookdropFileRepository.java (1)
27-30: LGTM — the modifying annotation update is consistent here.No concerns with adding automatic flush/clear to this existing bulk delete.
backend/src/main/java/org/booklore/repository/EmailRecipientV2Repository.java (1)
20-23: LGTM — this update query now follows the same flush/clear policy.The behavior change is limited to persistence-context synchronization.
backend/src/main/java/org/booklore/repository/UserEmailProviderPreferenceRepository.java (1)
18-21: LGTM — this bulk delete matches the entity shape.Using
p.userId = :userIdis appropriate for the scalar user-id column and keeps the operation explicit.backend/src/main/java/org/booklore/repository/AnnotationRepository.java (1)
5-8: LGTM — the converted delete is explicit and correctly scoped.The bulk delete predicates on scalar
bookIdanduserId, with the expected transactional and modifying-query annotations.Also applies to: 35-38
backend/src/main/java/org/booklore/repository/ComicCreatorMappingRepository.java (1)
19-22: The bulk delete query using association path traversal should work as-is with Hibernate/JPA.Line 21 navigates
m.comicMetadata.bookIdin a JPQLDELETEquery. While the concern about provider restrictions in bulk DML is valid, this pattern is supported for ManyToOne associations (unlike collection associations). The current query is correctly using a standard JPA construct and should work across all JPA providers.However, if explicit defensive coding or cross-provider compatibility is a priority, a subquery approach is available as an alternative:
Optional subquery alternative
- `@Query`("DELETE FROM ComicCreatorMappingEntity m WHERE m.comicMetadata.bookId = :bookId") + `@Query`("DELETE FROM ComicCreatorMappingEntity m WHERE m.comicMetadata IN (SELECT cm FROM ComicMetadataEntity cm WHERE cm.bookId = :bookId)") void deleteByComicMetadataBookId(`@Param`("bookId") Long bookId);backend/src/main/java/org/booklore/repository/OidcSessionRepository.java (1)
26-34: LGTM — the bulk deletes are scoped correctly.The JPQL predicates match the derived method names, and the scheduled cleanup caller does not reuse managed
OidcSessionEntityinstances after these calls.backend/src/main/java/org/booklore/repository/PdfAnnotationRepository.java (1)
16-19: LGTM — explicit delete matches the caller’s predicate.The query deletes only annotations for the provided
bookIdanduserId, matchingPdfAnnotationService.deleteAnnotations.backend/src/main/java/org/booklore/repository/BookNoteV2Repository.java (1)
35-38: LGTM — the bulk delete is correctly parameterized.The JPQL entity and named parameters match the existing repository field usage.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@backend/src/main/java/org/booklore/service/restriction/ContentRestrictionService.java`:
- Around line 64-69: The current updateRestrictions method issues
restrictionRepository.deleteByUserId(userId) before validating the user, which
can cause an immediate flush of deletes and leaves validation relying on
rollback; change the ordering in ContentRestrictionService.updateRestrictions so
you first call userRepository.findById(userId).orElseThrow(...) to validate the
user exists, then perform restrictionRepository.deleteByUserId(userId), and only
after that proceed to persist the new ContentRestriction rows (keeping the
`@Transactional` on updateRestrictions). Ensure you reference the existing methods
updateRestrictions, deleteByUserId, and userRepository.findById when making the
change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5c08906d-21ad-4f36-8989-9248074bb591
📒 Files selected for processing (1)
backend/src/main/java/org/booklore/service/restriction/ContentRestrictionService.java
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
backend/src/**/*.java
📄 CodeRabbit inference engine (AGENTS.md)
backend/src/**/*.java: Use 4-space indentation and match surrounding Java style in backend code
Prefer constructor injection via Lombok patterns already used in the codebase. Do not introduce@Autowiredfield injection in backend code
Use MapStruct for entity/DTO mapping in backend code
Keep JPA entities on the *Entity suffix in backend code
Files:
backend/src/main/java/org/booklore/service/restriction/ContentRestrictionService.java
🧠 Learnings (3)
📚 Learning: 2026-04-01T17:50:06.817Z
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.
Applied to files:
backend/src/main/java/org/booklore/service/restriction/ContentRestrictionService.java
📚 Learning: 2026-04-10T08:15:37.436Z
Learnt from: imnotjames
Repo: grimmory-tools/grimmory PR: 449
File: booklore-api/src/main/java/org/booklore/service/book/BookDownloadService.java:139-145
Timestamp: 2026-04-10T08:15:37.436Z
Learning: When using Spring `ContentDisposition.builder(...).filename(name, StandardCharsets.UTF_8).build()` (i.e., explicitly providing UTF-8), the resulting header value should include both the quoted `filename="=?UTF-8?..."` and the RFC 5987 `filename*=` parameters. In this case, any extra ASCII fallback computation (e.g., deriving an ASCII `fallbackFilename` via `NON_ASCII_PATTERN` and calling `.filename(fallbackFilename)`) is likely redundant—prefer calling only `.filename(fallbackName?, StandardCharsets.UTF_8)` as appropriate and let Spring handle the UTF-8 header parameters. Verify by comparing the emitted header for `filename` and `filename*` before deciding to keep an ASCII fallback.
Applied to files:
backend/src/main/java/org/booklore/service/restriction/ContentRestrictionService.java
📚 Learning: 2026-04-14T12:43:08.698Z
Learnt from: balazs-szucs
Repo: grimmory-tools/grimmory PR: 502
File: booklore-api/src/main/java/org/booklore/service/reader/ChapterCacheService.java:0-0
Timestamp: 2026-04-14T12:43:08.698Z
Learning: For this codebase (booklore-api), target Java 25 with `--enable-preview`, so `_` is intentionally used as an unnamed/ignored variable (e.g., lambda parameter or pattern variable) per Java’s preview feature JEP 456. Do not flag `_` in those contexts as an invalid/reserved identifier; only flag it if it’s used in a non-supported position (e.g., where an unnamed variable is not applicable for the Java preview rules).
Applied to files:
backend/src/main/java/org/booklore/service/restriction/ContentRestrictionService.java
| @Transactional | ||
| public List<ContentRestriction> updateRestrictions(Long userId, List<ContentRestriction> restrictions) { | ||
| restrictionRepository.deleteByUserId(userId); | ||
|
|
||
| BookLoreUserEntity user = userRepository.findById(userId) | ||
| .orElseThrow(() -> ApiError.USER_NOT_FOUND.createException(userId)); |
There was a problem hiding this comment.
Validate the user before issuing the flushed bulk delete.
Line 66 now deletes restrictions before Lines 68-69 confirm the user exists. Since this PR makes delete queries flush immediately, avoid relying on rollback for a predictable validation failure; validate first, then delete and load the user for the new rows.
🛡️ Proposed ordering fix
`@Transactional`
public List<ContentRestriction> updateRestrictions(Long userId, List<ContentRestriction> restrictions) {
- restrictionRepository.deleteByUserId(userId);
-
+ if (!userRepository.existsById(userId)) {
+ throw ApiError.USER_NOT_FOUND.createException(userId);
+ }
+
+ restrictionRepository.deleteByUserId(userId);
BookLoreUserEntity user = userRepository.findById(userId)
.orElseThrow(() -> ApiError.USER_NOT_FOUND.createException(userId));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @Transactional | |
| public List<ContentRestriction> updateRestrictions(Long userId, List<ContentRestriction> restrictions) { | |
| restrictionRepository.deleteByUserId(userId); | |
| BookLoreUserEntity user = userRepository.findById(userId) | |
| .orElseThrow(() -> ApiError.USER_NOT_FOUND.createException(userId)); | |
| `@Transactional` | |
| public List<ContentRestriction> updateRestrictions(Long userId, List<ContentRestriction> restrictions) { | |
| if (!userRepository.existsById(userId)) { | |
| throw ApiError.USER_NOT_FOUND.createException(userId); | |
| } | |
| restrictionRepository.deleteByUserId(userId); | |
| BookLoreUserEntity user = userRepository.findById(userId) | |
| .orElseThrow(() -> ApiError.USER_NOT_FOUND.createException(userId)); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@backend/src/main/java/org/booklore/service/restriction/ContentRestrictionService.java`
around lines 64 - 69, The current updateRestrictions method issues
restrictionRepository.deleteByUserId(userId) before validating the user, which
can cause an immediate flush of deletes and leaves validation relying on
rollback; change the ordering in ContentRestrictionService.updateRestrictions so
you first call userRepository.findById(userId).orElseThrow(...) to validate the
user exists, then perform restrictionRepository.deleteByUserId(userId), and only
after that proceed to persist the new ContentRestriction rows (keeping the
`@Transactional` on updateRestrictions). Ensure you reference the existing methods
updateRestrictions, deleteByUserId, and userRepository.findById when making the
change.
1 participant
Description
Linked Issue: Fixes #
Changes
This pull request refactors a number of repository classes to improve the handling of modifying queries in Spring Data JPA. The main focus is on ensuring that changes to the database are immediately synchronized with the persistence context and database by enabling both
clearAutomaticallyandflushAutomaticallyin the@Modifyingannotation. Additionally, several repository methods that previously relied on Spring Data's derived query methods for deletes have been converted to explicit JPQL@Querymethods, often with added transactional annotations and parameter bindings for clarity and consistency.Repository method improvements:
clearAutomatically = true, flushAutomatically = trueto all@Modifyingannotations on update and delete methods across repositories, ensuring the persistence context is kept in sync after modifying queries.@Query,@Modifying, and@Transactionalfor greater control and clarity. This includes methods such asdeleteByBookIdAndUserId,deleteByUser,deleteByRevokedTrueAndCreatedAtBefore, and others.@Modifying,@Transactional,@Query, and@Paramwhere needed to support the new method implementations.Consistency and code clarity:
MetadataFetchJobRepository.Summary by CodeRabbit
Refactor
Bug Fixes