refactor(config): remove redundant Spring Boot config and fix injection patterns#314
Conversation
…on patterns - Delete TransactionConfig (Spring Boot auto-configures JpaTransactionManager) - Delete MultipartConfig (replaced with spring.servlet.multipart.* properties) - Delete BeanConfig (RestTemplate moved to RestClientConfig, WS stats to WebSocketConfig) - Remove duplicate @EnableScheduling from TaskSchedulerConfig - Fix @ConfigurationProperties: remove @component from AppProperties, change @configuration to @ConfigurationProperties on BookmarkProperties - Move RestTemplate bean to RestClientConfig - Add WS stats suppression to WebSocketConfig via ApplicationReadyEvent - Remove @Autowired from KepubConversionService, GoogleParser, HardcoverBookSearchService (single-constructor injection) - Remove redundant two-arg constructor from GoogleParser - Fix CbxReaderService pre-existing compile error (transferEntryTo)
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📜 Recent 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)
🧰 Additional context used📓 Path-based instructions (1)booklore-api/src/**/*.java📄 CodeRabbit inference engine (AGENTS.md)
Files:
🔀 Multi-repo context grimmory-tools/grimmory-docsgrimmory-tools/grimmory-docs
🔇 Additional comments (1)
📝 WalkthroughWalkthroughConsolidated Spring configuration: multiple config classes removed or relocated, property classes un-annotated and enabled via Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
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 |
![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 (3)
booklore-api/src/main/java/org/booklore/service/reader/CbxReaderService.java (1)
115-115: Remove stalemetadataparameter fromreadEntryDimension.After the switch at Line 128 to
archiveService.transferEntryTo(...), theCachedArchiveMetadata metadataargument is unused. Keeping it now is misleading and adds unnecessary coupling.Proposed cleanup
- CbxPageDimension dim = readEntryDimension(cbxPath, entryName, metadata, i + 1); + CbxPageDimension dim = readEntryDimension(cbxPath, entryName, i + 1); - private CbxPageDimension readEntryDimension(Path cbxPath, String entryName, CachedArchiveMetadata metadata, int pageNumber) { + private CbxPageDimension readEntryDimension(Path cbxPath, String entryName, int pageNumber) { try { ByteArrayOutputStream baos = new ByteArrayOutputStream(); archiveService.transferEntryTo(cbxPath, entryName, baos);Also applies to: 125-128
🤖 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/reader/CbxReaderService.java` at line 115, Remove the now-unused CachedArchiveMetadata parameter from the readEntryDimension method signature and all its call sites in CbxReaderService: update the method declaration of readEntryDimension (returning CbxPageDimension) to drop the metadata argument and change calls like the one using CbxPageDimension dim = readEntryDimension(cbxPath, entryName, metadata, i + 1); to pass only the remaining parameters; also review other occurrences (e.g., around the archiveService.transferEntryTo transition) and adjust any delegation or overloads so transferEntryTo and related logic no longer rely on or forward the stale metadata parameter.booklore-api/src/main/resources/application.yaml (1)
46-49: Multipart migration looks correct; consider a small threshold instead of 0.Enabling multipart and migrating the 1024MB limits from the deleted
MultipartConfigbean is the right approach for Spring Boot's declarative configuration.However,
file-size-threshold: 0means every upload—including tiny cover images or metadata files—is immediately written to a temp file on disk. A small threshold (e.g.,2KBor10KB) keeps small payloads in memory, avoiding unnecessary disk I/O while still protecting against memory pressure from large book uploads.💡 Optional: set a small threshold to reduce I/O for tiny uploads
max-file-size: 1024MB max-request-size: 1024MB - file-size-threshold: 0 + file-size-threshold: 2KB🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@booklore-api/src/main/resources/application.yaml` around lines 46 - 49, The multipart configuration currently sets file-size-threshold: 0 which forces all uploads to be written to disk; change the value to a small non-zero threshold (e.g., file-size-threshold: 2KB or 10KB) in the application.yaml so small uploads remain in memory while large uploads still go to temp files; update the multipart section (keys enabled, max-file-size, max-request-size, file-size-threshold) to reflect the chosen threshold to reduce unnecessary disk I/O.booklore-api/src/main/java/org/booklore/config/WebSocketConfig.java (1)
35-39: LGTM! The migration from@PostConstructin the deletedBeanConfigto@EventListener(ApplicationReadyEvent.class)is appropriate.Using
ApplicationReadyEventensures the WebSocket infrastructure is fully initialized before accessing the stats bean, which is a robust approach.Consider using
TimeUnitfor improved readability:♻️ Optional: Use TimeUnit for clarity
+import java.util.concurrent.TimeUnit;`@EventListener`(ApplicationReadyEvent.class) public void suppressStatsLogging(ApplicationReadyEvent event) { var stats = event.getApplicationContext().getBean(WebSocketMessageBrokerStats.class); - stats.setLoggingPeriod(30 * 24 * 60 * 60 * 1000L); // 30 days + stats.setLoggingPeriod(TimeUnit.DAYS.toMillis(30)); }🤖 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/config/WebSocketConfig.java` around lines 35 - 39, Replace the magic millisecond literal in suppressStatsLogging by computing the duration with TimeUnit to improve readability: when obtaining the WebSocketMessageBrokerStats bean in suppressStatsLogging, compute the 30-day value via TimeUnit.DAYS.toMillis(30) and pass that to stats.setLoggingPeriod(...) instead of 30 * 24 * 60 * 60 * 1000L.
🤖 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/config/RestClientConfig.java`:
- Around line 22-27: The restTemplate bean in RestClientConfig creates ambiguity
with existing oidcRestTemplate and noRedirectRestTemplate beans; mark the
restTemplate `@Bean` method (restTemplate(HttpClient)) as `@Primary` so unqualified
RestTemplate injections resolve to it and avoid NoUniqueBeanDefinitionException
in the future; update the restTemplate method to add the `@Primary` annotation
(and import org.springframework.context.annotation.Primary if missing) so Spring
will prefer this bean when multiple RestTemplate candidates exist.
---
Nitpick comments:
In `@booklore-api/src/main/java/org/booklore/config/WebSocketConfig.java`:
- Around line 35-39: Replace the magic millisecond literal in
suppressStatsLogging by computing the duration with TimeUnit to improve
readability: when obtaining the WebSocketMessageBrokerStats bean in
suppressStatsLogging, compute the 30-day value via TimeUnit.DAYS.toMillis(30)
and pass that to stats.setLoggingPeriod(...) instead of 30 * 24 * 60 * 60 *
1000L.
In
`@booklore-api/src/main/java/org/booklore/service/reader/CbxReaderService.java`:
- Line 115: Remove the now-unused CachedArchiveMetadata parameter from the
readEntryDimension method signature and all its call sites in CbxReaderService:
update the method declaration of readEntryDimension (returning CbxPageDimension)
to drop the metadata argument and change calls like the one using
CbxPageDimension dim = readEntryDimension(cbxPath, entryName, metadata, i + 1);
to pass only the remaining parameters; also review other occurrences (e.g.,
around the archiveService.transferEntryTo transition) and adjust any delegation
or overloads so transferEntryTo and related logic no longer rely on or forward
the stale metadata parameter.
In `@booklore-api/src/main/resources/application.yaml`:
- Around line 46-49: The multipart configuration currently sets
file-size-threshold: 0 which forces all uploads to be written to disk; change
the value to a small non-zero threshold (e.g., file-size-threshold: 2KB or 10KB)
in the application.yaml so small uploads remain in memory while large uploads
still go to temp files; update the multipart section (keys enabled,
max-file-size, max-request-size, file-size-threshold) to reflect the chosen
threshold to reduce unnecessary disk I/O.
🪄 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: 277feda2-c82c-490b-9bc7-78656b9dc929
📒 Files selected for processing (14)
booklore-api/src/main/java/org/booklore/BookloreApplication.javabooklore-api/src/main/java/org/booklore/config/AppProperties.javabooklore-api/src/main/java/org/booklore/config/BeanConfig.javabooklore-api/src/main/java/org/booklore/config/BookmarkProperties.javabooklore-api/src/main/java/org/booklore/config/MultipartConfig.javabooklore-api/src/main/java/org/booklore/config/RestClientConfig.javabooklore-api/src/main/java/org/booklore/config/TaskSchedulerConfig.javabooklore-api/src/main/java/org/booklore/config/TransactionConfig.javabooklore-api/src/main/java/org/booklore/config/WebSocketConfig.javabooklore-api/src/main/java/org/booklore/service/kobo/KepubConversionService.javabooklore-api/src/main/java/org/booklore/service/metadata/parser/GoogleParser.javabooklore-api/src/main/java/org/booklore/service/metadata/parser/hardcover/HardcoverBookSearchService.javabooklore-api/src/main/java/org/booklore/service/reader/CbxReaderService.javabooklore-api/src/main/resources/application.yaml
💤 Files with no reviewable changes (8)
- booklore-api/src/main/java/org/booklore/config/TaskSchedulerConfig.java
- booklore-api/src/main/java/org/booklore/config/AppProperties.java
- booklore-api/src/main/java/org/booklore/config/BookmarkProperties.java
- booklore-api/src/main/java/org/booklore/config/TransactionConfig.java
- booklore-api/src/main/java/org/booklore/service/metadata/parser/hardcover/HardcoverBookSearchService.java
- booklore-api/src/main/java/org/booklore/config/MultipartConfig.java
- booklore-api/src/main/java/org/booklore/config/BeanConfig.java
- booklore-api/src/main/java/org/booklore/service/metadata/parser/GoogleParser.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 / Frontend Tests
- GitHub Check: Test Suite / Backend Tests
- GitHub Check: Analyze (java-kotlin)
- GitHub Check: Analyze (javascript-typescript)
🧰 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/RestClientConfig.javabooklore-api/src/main/java/org/booklore/config/WebSocketConfig.javabooklore-api/src/main/java/org/booklore/service/reader/CbxReaderService.javabooklore-api/src/main/java/org/booklore/service/kobo/KepubConversionService.javabooklore-api/src/main/java/org/booklore/BookloreApplication.java
🧠 Learnings (2)
📚 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/main/java/org/booklore/service/kobo/KepubConversionService.javabooklore-api/src/main/java/org/booklore/BookloreApplication.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/main/java/org/booklore/BookloreApplication.java
🔇 Additional comments (3)
booklore-api/src/main/java/org/booklore/service/kobo/KepubConversionService.java (1)
4-4: LGTM! Proper use of Lombok constructor injection.The refactoring correctly replaces
@Autowiredfield injection with@RequiredArgsConstructorand afinalfield, aligning with the codebase's established dependency injection pattern. As per coding guidelines: "Prefer constructor injection via Lombok patterns already used in the codebase. Do not introduce@Autowiredfield injection".Also applies to: 14-14, 17-17
booklore-api/src/main/java/org/booklore/config/WebSocketConfig.java (1)
5-14: LGTM!The new imports are appropriate for the
ApplicationReadyEventlistener pattern used to configure WebSocket stats.booklore-api/src/main/java/org/booklore/BookloreApplication.java (1)
9-14: LGTM! Clean migration to centralized@EnableConfigurationProperties.Registering
AppPropertiesandBookmarkPropertiesvia@EnableConfigurationPropertiesinstead of@Component/@Configurationis the idiomatic Spring Boot pattern for@ConfigurationPropertiesclasses.Tests that depend on
AppPropertieseither already includeBookloreApplication.classin their@SpringBootTest(classes = ...)configuration or use focused unit tests with mocked dependencies, so no compatibility issues are expected from this change.
… settings in persistence configuration
Merge Develop into Master for Release
There was a problem hiding this comment.
As per our convo, DEFAULT_MAX_UPLOAD_SIZE_MB has always been capping upload request size to 1024mb. This changeset does not change that current behaviour.
#152 Surfaces this existing bug, and following this PR being merged should be picked up to address this hard coded inconsistency
2 participants
Description
Linked Issue: Fixes #
Changes
Summary by CodeRabbit
New Features
Chores