캐시 원자 단위 의존성 주입 Resolver 추가 및 동적 Bean 등록#260
Conversation
…zer in spring.factories
…heAtomicOperator beans
…on in ExamQuotaCacheManager
…o refactor/enhanced-redis
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Caution Review failedThe pull request is closed. WalkthroughThis change refactors the injection and registration of Redis Lua scripts and atomic cache operators in the exam quota management subsystem. It replaces direct bean injection with map-based and domain-based registrations, introduces dynamic bean registration for Lua scripts and atomic operators, and removes the old static configuration class. Changes
Sequence Diagram(s)sequenceDiagram
participant SpringContext
participant LuaScriptsFunctionalRegistrar
participant AtomicOperatorAutoRegistrar
participant ApplicationBeans
SpringContext->>LuaScriptsFunctionalRegistrar: initialize(context)
LuaScriptsFunctionalRegistrar->>SpringContext: Register <domain>LuaScripts beans
SpringContext->>ApplicationBeans: Instantiate beans (AtomicExamQuotaIncrementOperator, etc.)
SpringContext->>AtomicOperatorAutoRegistrar: afterSingletonsInstantiated()
AtomicOperatorAutoRegistrar->>SpringContext: Register <domain>CacheAtomicOperatorMap beans
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes found. Possibly related PRs
Poem
Note 🔌 MCP (Model Context Protocol) integration is now available in Early Access!Pro users can now connect to remote MCP servers under the Integrations page to get reviews and chat conversations that understand additional development context. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @polyglot-k, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
I've refactored the application's approach to managing Redis Lua scripts and cache atomic operators, specifically for exam quota operations. The core change involves transitioning from static, manual configuration to dynamic, domain-based registration. This significantly enhances the system's scalability and maintainability by automating the discovery and registration of new scripts and operators, thereby reducing boilerplate code and potential configuration errors. This change simplifies the process of adding new functionalities related to exam quotas.
Highlights
- Dynamic Lua Script Registration: I've introduced
LuaScriptsFunctionalRegistrarto automatically discover and register all Lua scripts located under thescripts/directory. These scripts are now grouped by domain and exposed as beans (e.g.,examLuaScripts), replacing the previous manual bean definitions for each script. - Refactored Script Usage in Operators: The
AtomicExamQuotaIncrementOperatorandAtomicExamQuotaDecrementOperatorclasses have been updated. They now receive a map of scripts (examLuaScripts) and select the appropriate script by name, rather than relying on individual script beans. - Automated Atomic Operator Registration: I've added
AtomicOperatorAutoRegistrarto automatically group and register allCacheAtomicOperatorbeans by domain, exposing them as maps (e.g.,examQuotaCacheAtomicOperatorMap). This removes the need for manual mapping in configuration. - Configuration Cleanup: The
ExamQuotaAtomicOperationConfigclass, which previously handled static script and operator bean definitions, has been removed as its functionality is now superseded by the dynamic registration mechanisms. - Dependency Injection Updates: The
ExamQuotaCacheManagerhas been updated to inject the new dynamically registered operator map bean (@Qualifier("examQuotaCacheAtomicOperatorMap")) and includes@Lazyto prevent potential circular dependencies during bean initialization.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments or fill out our survey to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request introduces a significant improvement by refactoring the registration of Lua scripts and cache atomic operators from a static configuration to a dynamic, domain-based approach. The use of LuaScriptsFunctionalRegistrar and AtomicOperatorAutoRegistrar to automatically discover and register these components is a solid architectural change that enhances maintainability and scalability. The code is generally well-structured. My review includes suggestions to improve the robustness of the script loading mechanism, add fail-fast checks for script resolution to prevent potential runtime NullPointerExceptions, and enhance documentation for better clarity.
...java/life/mosu/mosuserver/infra/persistence/redis/support/LuaScriptsFunctionalRegistrar.java
Outdated
Show resolved
Hide resolved
src/main/java/life/mosu/mosuserver/application/exam/cache/AtomicExamQuotaDecrementOperator.java
Outdated
Show resolved
Hide resolved
src/main/java/life/mosu/mosuserver/application/exam/cache/AtomicExamQuotaIncrementOperator.java
Outdated
Show resolved
Hide resolved
...java/life/mosu/mosuserver/infra/persistence/redis/support/LuaScriptsFunctionalRegistrar.java
Show resolved
Hide resolved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (1)
src/main/java/life/mosu/mosuserver/application/exam/cache/ExamQuotaCacheManager.java (1)
103-108: Unchecked cast toVoidCacheAtomicOperatormay blow up
cacheAtomicOperatorMap.get(operationType.key())returns aCacheAtomicOperator, but the code blindly casts toVoidCacheAtomicOperator.
If a non-void operator is ever registered under the same key, this will fail at runtime.Guard the cast:
CacheAtomicOperator<String, Long> op = cacheAtomicOperatorMap.get(operationType.key()); if (!(op instanceof VoidCacheAtomicOperator<?> vOp)) { log.error("Operator {} is not VoidCacheAtomicOperator", operationType.key()); throw new CustomRuntimeException(ErrorCode.CACHE_OPERATION_NOT_SUPPORTED); } VoidCacheAtomicOperator<String, Long> operator = vOp;
🧹 Nitpick comments (2)
src/main/java/life/mosu/mosuserver/infra/persistence/redis/support/LuaScriptsFunctionalRegistrar.java (1)
62-71: Expose an unmodifiable, parameterised Map beanRegistering with
Map.classloses generic info and returns a mutable instance:- context.registerBean(beanName, Map.class, () -> scripts); + context.registerBean(beanName, + new ParameterizedTypeReference<Map<String, DefaultRedisScript<Long>>>() {}, + () -> Collections.unmodifiableMap(scripts));• Preserves type safety for injection points
• Prevents accidental mutation after context start-upsrc/main/java/life/mosu/mosuserver/application/exam/cache/AtomicExamQuotaIncrementOperator.java (1)
15-15: Remove unused @slf4j or add minimal logging@slf4j is currently unused. Either remove it or log failures for easier diagnostics.
Example usage:
// in constructor (after resolving the script) log.debug("Loaded Lua script 'incrementQuota' for examQuota"); // in catch block (just before mapping to CustomRuntimeException) log.warn("Increment quota script failed for key {}: {}", key, msg, e);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/main/java/life/mosu/mosuserver/application/exam/cache/AtomicExamQuotaDecrementOperator.java(2 hunks)src/main/java/life/mosu/mosuserver/application/exam/cache/AtomicExamQuotaIncrementOperator.java(2 hunks)src/main/java/life/mosu/mosuserver/application/exam/cache/ExamQuotaCacheManager.java(2 hunks)src/main/java/life/mosu/mosuserver/global/config/ExamQuotaAtomicOperationConfig.java(0 hunks)src/main/java/life/mosu/mosuserver/infra/config/AtomicOperatorAutoRegistrar.java(1 hunks)src/main/java/life/mosu/mosuserver/infra/persistence/redis/support/LuaScriptsFunctionalRegistrar.java(1 hunks)src/main/resources/META-INF/spring.factories(1 hunks)
💤 Files with no reviewable changes (1)
- src/main/java/life/mosu/mosuserver/global/config/ExamQuotaAtomicOperationConfig.java
🔇 Additional comments (6)
src/main/java/life/mosu/mosuserver/infra/persistence/redis/support/LuaScriptsFunctionalRegistrar.java (1)
77-88:toCamelCaseignores hyphens and consecutive underscoresIf future script files contain
-or__, the current algorithm produces unexpected names (e.g.,foo-bar.lua→foo-bar).
Either document the naming convention strictly (snake_caseonly) or extend the helper to normalise additional delimiters.src/main/resources/META-INF/spring.factories (1)
1-2: Confirm no otherApplicationContextInitializerentries are required
spring.factoriesallows multiple initialisers separated by commas.
If another library also registers one, this single-line property will silently override it.
Double-check merged resources in the final JAR (e.g., via thespring.factoriesMaven/Gradle merger plugin).src/main/java/life/mosu/mosuserver/application/exam/cache/AtomicExamQuotaIncrementOperator.java (4)
23-27: Good move to domain-grouped, map-based script injectionSwitching to a domain-scoped script map reduces wiring boilerplate and aligns with the new dynamic registrar.
4-4: Imports/field refactor looks correctImporting Map and keeping DefaultRedisScript for the typed result are appropriate for the new lookup approach.
Also applies to: 19-19
25-27: examLuaScripts bean and “incrementQuota” key are correctly registeredThe
LuaScriptsFunctionalRegistrarbuilds a bean namedexamLuaScripts(domain “exam” + “LuaScripts”) and convertsincrement_quota.luato the keyincrementQuota. The qualifier and lookup inAtomicExamQuotaIncrementOperatoralign with what the registrar produces—no changes required.
45-48: ✅ Java toolchain targets Java 21, so List.of is supported
The Gradle build config uses a Java toolchain withlanguageVersion = JavaLanguageVersion.of(21), which is well beyond Java 9. No change needed forList.of.
src/main/java/life/mosu/mosuserver/application/exam/cache/AtomicExamQuotaDecrementOperator.java
Show resolved
Hide resolved
src/main/java/life/mosu/mosuserver/application/exam/cache/AtomicExamQuotaIncrementOperator.java
Outdated
Show resolved
Hide resolved
| Map<String, CacheAtomicOperator> mapValue = operators.stream() | ||
| .collect(Collectors.toMap(CacheAtomicOperator::getActionName, | ||
| Function.identity())); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Duplicate getActionName() values explode the context
Collectors.toMap with no merge function throws IllegalStateException when two operators in the same domain share an action name.
Either enforce uniqueness with a clear error or supply a merge strategy:
.collect(Collectors.toMap(
CacheAtomicOperator::getActionName,
Function.identity(),
(a, b) -> {
throw new IllegalStateException("Duplicate actionName: " + a.getActionName());
}));Explicit handling yields a descriptive startup failure instead of cryptic bean-creation errors.
🤖 Prompt for AI Agents
In
src/main/java/life/mosu/mosuserver/infra/config/AtomicOperatorAutoRegistrar.java
around lines 38 to 41, the use of Collectors.toMap without a merge function
causes an IllegalStateException if duplicate getActionName() values exist. Fix
this by adding a merge function that throws an IllegalStateException with a
clear message indicating the duplicate actionName, ensuring descriptive failure
on startup instead of cryptic errors.
| PathMatchingResourcePatternResolver resolver = new PathMatchingResourcePatternResolver(); | ||
| Resource[] resources = resolver.getResources(SCRIPT_BASE_PATH + "**/*.lua"); | ||
|
|
||
| Map<String, Map<String, DefaultRedisScript<Long>>> domainScriptsMap = new HashMap<>(); | ||
|
|
||
| for (Resource resource : resources) { | ||
| String path = resource.getURL().getPath(); | ||
| int idx = path.indexOf(SCRIPT_PATH_PREFIX); | ||
| if (idx < 0) { | ||
| continue; | ||
| } | ||
|
|
||
| String relativePath = path.substring(idx + SCRIPT_PATH_PREFIX.length()); | ||
| String[] parts = relativePath.split("/"); | ||
| if (parts.length < 2) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Path parsing brittle when running from a shaded JAR or on Windows
resource.getURL().getPath() returns strings such as
jar:file:/app.jar!/BOOT-INF/classes!/scripts/exam/increment.lua.
The current indexOf("/scripts/") + split("/") logic still works for the happy path but silently skips any resource whose URL does not contain /scripts/ (e.g., VFS, Windows back-slashes).
Consider using Spring’s ResourceUtils.toURI(resource) plus Paths.get(uri) to obtain platform-independent segments, or rely on resource.getFilename() and FilenameUtils.getPathNoEndSeparator() (Apache Commons) to extract the domain.
This avoids edge-case misses and makes unit-testing simpler.
🤖 Prompt for AI Agents
In
src/main/java/life/mosu/mosuserver/infra/persistence/redis/support/LuaScriptsFunctionalRegistrar.java
between lines 27 and 41, the current method of parsing resource paths using
resource.getURL().getPath() and string operations is brittle and fails on shaded
JARs or Windows due to different URL formats and path separators. Refactor the
code to use Spring's ResourceUtils.toURI(resource) combined with Paths.get(uri)
to get platform-independent path segments, or alternatively use
resource.getFilename() with Apache Commons FilenameUtils.getPathNoEndSeparator()
to reliably extract the domain and script path. This will ensure consistent path
parsing across environments and improve testability.
1 participant
✨ 연결 된 이슈
This pull request refactors how Lua scripts and cache atomic operators are registered and injected for exam quota operations. The main improvements are the introduction of dynamic, domain-based registration for Lua scripts and atomic operators, replacing the previous static configuration. This change enhances scalability, maintainability, and reduces boilerplate when adding new scripts or operators.
Key changes:
Dynamic Lua Script Registration
LuaScriptsFunctionalRegistrarto automatically discover and register all Lua scripts under thescripts/directory, grouping them by domain and exposing them as beans (e.g.,examLuaScripts). This replaces manual bean definitions for each script. [1] [2]AtomicExamQuotaIncrementOperatorandAtomicExamQuotaDecrementOperatorto receive a map of scripts (examLuaScripts) and select the appropriate script by name, instead of using individual script beans. [1] [2] [3] [4]Dynamic Atomic Operator Registration
AtomicOperatorAutoRegistrarto automatically group and register allCacheAtomicOperatorbeans by domain, exposing them as maps (e.g.,examQuotaCacheAtomicOperatorMap). This eliminates the need for manual mapping in configuration.Configuration Cleanup
ExamQuotaAtomicOperationConfigclass, which previously handled static script and operator bean definitions.Dependency Injection Updates
ExamQuotaCacheManagerto inject the new dynamically registered operator map bean (@Qualifier("examQuotaCacheAtomicOperatorMap")) and added@Lazyto avoid circular dependencies. [1] [2]These changes make it easier to add new Lua scripts or atomic operators for other domains and reduce the risk of bean naming conflicts or manual registration errors.
Summary by CodeRabbit
Refactor
Chores