refactor(service): replace manual caching with @Cacheable and JPQL with Criteria API#316
Conversation
…th Criteria API - Replace AppSettingService double-checked locking (volatile + ReentrantLock) with Spring @Cacheable/@CacheEvict on appSettings cache - Add appSettings cache name to CacheConfig - Replace NotificationService dynamic JPQL string concatenation with type-safe JPA Criteria API (CriteriaBuilder predicates) - Fix CbxReaderService pre-existing compile error (transferEntryTo)
📝 WalkthroughWalkthroughThis PR refactors the caching infrastructure by updating the Spring cache configuration to support both "publicSettings" and "appSettings" caches, modernizing query construction in Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
booklore-api/src/main/java/org/booklore/service/appsettings/AppSettingService.java (1)
45-48: Spring proxy self-invocation bypasses the cache.When
validateOidcForceOnlyMode()(line 84) callsgetAppSettings()internally, it invokes the method onthisrather than through the Spring proxy, so@Cacheablewon't apply. The cache is only effective for external callers.For internal calls to also benefit from caching, consider injecting a self-reference or extracting the cached method to a separate bean.
♻️ Option: Inject self-reference for proxy-aware internal calls
public class AppSettingService { private final AppProperties appProperties; private final SettingPersistenceHelper settingPersistenceHelper; private final AuthenticationService authenticationService; private final AuditService auditService; + private final AppSettingService self; - public AppSettingService(AppProperties appProperties, SettingPersistenceHelper settingPersistenceHelper, `@Lazy` AuthenticationService authenticationService, `@Lazy` AuditService auditService) { + public AppSettingService(AppProperties appProperties, SettingPersistenceHelper settingPersistenceHelper, `@Lazy` AuthenticationService authenticationService, `@Lazy` AuditService auditService, `@Lazy` AppSettingService self) { this.appProperties = appProperties; this.settingPersistenceHelper = settingPersistenceHelper; this.authenticationService = authenticationService; this.auditService = auditService; + this.self = self; }Then in
validateOidcForceOnlyMode():- AppSettings current = getAppSettings(); + AppSettings current = self.getAppSettings();🤖 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/appsettings/AppSettingService.java` around lines 45 - 48, The internal call to getAppSettings() from validateOidcForceOnlyMode() bypasses Spring caching because it's a self-invocation; to fix, obtain a proxy-aware reference and call through that or move the cached method to another bean: either inject a proxy self-reference (e.g., add a `@Lazy/`@Autowired AppSettingService selfProxy and replace calls to this.getAppSettings() in validateOidcForceOnlyMode() with selfProxy.getAppSettings()), or extract buildAppSettings()/getAppSettings() into a separate `@Component` (e.g., AppSettingsProvider) annotated with `@Cacheable` and call that provider from AppSettingService.validateOidcForceOnlyMode().
🤖 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/appsettings/AppSettingService.java`:
- Around line 45-48: The internal call to getAppSettings() from
validateOidcForceOnlyMode() bypasses Spring caching because it's a
self-invocation; to fix, obtain a proxy-aware reference and call through that or
move the cached method to another bean: either inject a proxy self-reference
(e.g., add a `@Lazy/`@Autowired AppSettingService selfProxy and replace calls to
this.getAppSettings() in validateOidcForceOnlyMode() with
selfProxy.getAppSettings()), or extract buildAppSettings()/getAppSettings() into
a separate `@Component` (e.g., AppSettingsProvider) annotated with `@Cacheable` and
call that provider from AppSettingService.validateOidcForceOnlyMode().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 73e0330f-d554-4f31-b7d0-9e23cd854742
📒 Files selected for processing (3)
booklore-api/src/main/java/org/booklore/config/CacheConfig.javabooklore-api/src/main/java/org/booklore/service/NotificationService.javabooklore-api/src/main/java/org/booklore/service/appsettings/AppSettingService.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/config/CacheConfig.javabooklore-api/src/main/java/org/booklore/service/NotificationService.javabooklore-api/src/main/java/org/booklore/service/appsettings/AppSettingService.java
🔇 Additional comments (4)
booklore-api/src/main/java/org/booklore/service/NotificationService.java (1)
71-78: No fix needed;permissionsis a@OneToOnerelationship.The original concern assumes that joining
permissionscould produce duplicate rows per user. However,BookLoreUserEntity.permissionsis annotated as@OneToOne, which guarantees exactly one related entity per user. The join will not fan out, so duplicate usernames cannot occur. The proposeddistinct(true)is unnecessary.> Likely an incorrect or invalid review comment.booklore-api/src/main/java/org/booklore/config/CacheConfig.java (1)
16-23: LGTM!Clean addition of the
appSettingscache. Both caches sharing the same configuration (24h TTL, max 100 entries) is appropriate for application settings data.booklore-api/src/main/java/org/booklore/service/appsettings/AppSettingService.java (2)
50-78: LGTM!The
@Cachingannotation correctly evicts both caches after successful updates. UsingallEntries = trueis appropriate here, and the defaultbeforeInvocation = falseensures the cache is only evicted after successful method completion, preserving consistency with the@Transactionalbehavior.
201-214: LGTM!Consistent cache eviction pattern matching
updateSetting(). Both caches are properly invalidated when settings are persisted.
Description
Linked Issue: Fixes #
Changes
Summary by CodeRabbit