Conversation
…ications if not configured
|
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 introduces inquiry update functionality to the application. It adds a new DTO for update requests, a controller endpoint for updating inquiries, and corresponding service and entity methods to handle updates. Additionally, it modifies notification logic for Discord integration and reorganizes configuration files for different environments. Several infrastructure improvements include caching enhancements, atomic cache operations, OAuth user persistence refactoring, and thread-safe IP rate limiting with comprehensive tests. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Controller
participant Service
participant Entity
participant AttachmentService
User->>Controller: HTTP PUT /inquiry/{postId} (with InquiryUpdateRequest)
Controller->>Service: updateInquiry(user, request, postId)
Service->>Entity: update(title, content, author)
Service->>AttachmentService: updateInquiryAttachments(...)
Service->>Service: save updated inquiry
Service->>Controller: return success
Controller->>User: HTTP 200 OK (inquiry updated)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
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 (5)
✨ 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 @jbh010204, 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 implemented a new feature allowing users to update their inquiries and significantly refactored our application's configuration management. This change introduces environment-specific YAML files for better separation of concerns and enhances the robustness of our Discord notification system by gracefully handling unconfigured webhook URLs.
Highlights
- Inquiry Management Enhancement: I've added a new PUT /api/v1/inquiry/{postId} endpoint, enabling users to modify their existing inquiries, including the title, content, and associated attachments. This is supported by new service logic and an update method in the InquiryJpaEntity.
- Environment-Specific Configuration: I've refactored our application's configuration by introducing application-local.yml, application-prod.yml, and application-test.yml. This allows for distinct settings (like logging levels, external service URLs) for different deployment environments, making the application more adaptable.
- Dynamic Profile Activation: To support the new environment-specific configurations, I've updated application.yml to dynamically activate the appropriate Spring profile using the SPRING_PROFILES_ACTIVE environment variable, simplifying deployment setup.
- Robust Discord Notifications: I've improved the DiscordNotifier to prevent errors when the Discord webhook URL is not configured. Now, if the discord.base-url is not explicitly set (or defaults to a placeholder), the system will log a debug message and skip sending notifications, avoiding exceptions in environments where Discord integration isn't needed.
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 separates application configuration into different profiles for different environments and adds functionality to update inquiries. My review focuses on ensuring the new features are robust and correct. I've pointed out a potential bug in the Discord notification logic due to an incorrect check, a missing transactional annotation on a service method which could lead to data inconsistency, and a validation mismatch in a DTO. Applying these suggestions will improve the reliability and correctness of the code.
src/main/java/life/mosu/mosuserver/application/inquiry/InquiryService.java
Outdated
Show resolved
Hide resolved
src/main/java/life/mosu/mosuserver/infra/notify/DiscordNotifier.java
Outdated
Show resolved
Hide resolved
src/main/java/life/mosu/mosuserver/infra/notify/DiscordNotifier.java
Outdated
Show resolved
Hide resolved
src/main/java/life/mosu/mosuserver/presentation/inquiry/dto/InquiryUpdateRequest.java
Outdated
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
🧹 Nitpick comments (5)
src/main/resources/application-test.yml (1)
5-6: TRACE root level may bloat test logs
TRACEgenerates very noisy output and can quickly grow the file, slowing CI and masking useful info. Consider downgrading toDEBUGor narrowing categories unless you truly need full trace data.src/main/resources/application-local.yml (1)
9-9: Add newline at end of file.Static analysis tools indicate that the file should end with a newline character for better YAML compliance.
discord: - base-url: "" + base-url: "" +src/main/resources/application-prod.yml (1)
14-14: Add newline at end of file.Static analysis tools indicate that the file should end with a newline character for better YAML compliance.
discord: - base-url: ${DISCORD_URL} + base-url: ${DISCORD_URL} +src/main/java/life/mosu/mosuserver/infra/notify/DiscordNotifier.java (1)
24-25: Consider using a safer default URL.Using
"http://default.com"as a default could potentially make unintended network calls to a real domain. Consider using a localhost URL or a more explicit placeholder.- @Value("${discord.base-url:http://default.com}") + @Value("${discord.base-url:http://localhost:0/disabled}")This makes it clearer that Discord notifications are disabled and avoids potential network calls to external domains.
src/main/java/life/mosu/mosuserver/presentation/inquiry/dto/InquiryUpdateRequest.java (1)
17-17: Consider adding validation constraint for attachments.The
attachmentsfield lacks validation constraints. Consider adding appropriate validation to prevent potential issues with excessive attachment counts or null handling.- List<FileRequest> attachments + @Valid List<FileRequest> attachments
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/main/java/life/mosu/mosuserver/application/inquiry/InquiryService.java(3 hunks)src/main/java/life/mosu/mosuserver/domain/inquiry/entity/InquiryJpaEntity.java(1 hunks)src/main/java/life/mosu/mosuserver/infra/notify/DiscordNotifier.java(2 hunks)src/main/java/life/mosu/mosuserver/presentation/inquiry/InquiryController.java(3 hunks)src/main/java/life/mosu/mosuserver/presentation/inquiry/dto/InquiryUpdateRequest.java(1 hunks)src/main/resources/application-local.yml(1 hunks)src/main/resources/application-prod.yml(1 hunks)src/main/resources/application-test.yml(1 hunks)src/main/resources/application.yml(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/main/java/life/mosu/mosuserver/domain/inquiry/entity/InquiryJpaEntity.java (1)
src/main/java/life/mosu/mosuserver/domain/inquiryAnswer/entity/InquiryAnswerJpaEntity.java (1)
update(59-63)
🪛 YAMLlint (1.37.1)
src/main/resources/application-local.yml
[error] 9-9: no new line character at the end of file
(new-line-at-end-of-file)
src/main/resources/application-prod.yml
[error] 14-14: no new line character at the end of file
(new-line-at-end-of-file)
🔇 Additional comments (7)
src/main/resources/application-local.yml (1)
1-9: LGTM! Good local development configuration.The TRACE logging level and empty Discord URL are appropriate for local development environment, preventing unwanted notifications while providing detailed debugging information.
src/main/resources/application-prod.yml (1)
1-14: LGTM! Excellent production configuration practices.Good use of environment variables for sensitive data like
TOSS_SECRET_KEYandDISCORD_URL, and appropriate INFO logging level for production.src/main/resources/application.yml (1)
74-75: Excellent architectural improvement with profile-based configuration.Adding profile support via
SPRING_PROFILES_ACTIVEenvironment variable enables clean separation of environment-specific configurations into dedicated files. This follows Spring Boot best practices and improves maintainability.src/main/java/life/mosu/mosuserver/infra/notify/DiscordNotifier.java (2)
4-4: Good addition for null safety checks.Adding the
Objectsimport supports the null safety check in thesendmethod.
29-32: Excellent defensive programming approach.The early return with debug logging when Discord webhook URL is not configured prevents unnecessary network calls and provides clear feedback about the configuration state. This aligns well with the environment-specific configuration changes.
src/main/java/life/mosu/mosuserver/presentation/inquiry/InquiryController.java (1)
45-53: LGTM! Well-implemented update endpoint.The update endpoint follows established patterns in the controller with proper authentication, authorization, validation, and response handling. The use of PUT method is appropriate for update operations.
src/main/java/life/mosu/mosuserver/application/inquiry/InquiryService.java (1)
100-106: LGTM! Solid business logic implementation.The method correctly implements the update flow with proper permission checking, entity updates, and attachment handling. The logic follows established patterns in the service.
src/main/java/life/mosu/mosuserver/application/inquiry/InquiryService.java
Outdated
Show resolved
Hide resolved
src/main/java/life/mosu/mosuserver/domain/inquiry/entity/InquiryJpaEntity.java
Show resolved
Hide resolved
src/main/java/life/mosu/mosuserver/presentation/inquiry/dto/InquiryUpdateRequest.java
Outdated
Show resolved
Hide resolved
…zer in spring.factories
…heAtomicOperator beans
…on in ExamQuotaCacheManager
…o refactor/enhanced-redis
캐시 원자 단위 의존성 주입 Resolver 추가 및 동적 Bean 등록
…image build steps
- blockedIpCache에 putIfAbsent, computeIfPresent 적용으로 경합 상황에서의 중복 차단 방지 - compute를 사용해 BlockedIpHistory를 원자적으로 업데이트하도록 변경 - 초기 차단 상태(LEVEL_0) 지속 시간을 0초에서 10초로 변경하여 안정성 확보
MOSU-258 refactor: IP 차단 요청시 발생할 수 있는 동시성 경합 상황 테스트 및 코드 개선
MOSU-264 refactor: Dirty Checking을 위한 Transaction 분리
MOSU-267 refactor: 도메인 캐싱 적용
… into feat/mosu-249
… streamline configuration
…ations if not configured
There was a problem hiding this comment.
Actionable comments posted: 19
🔭 Outside diff range comments (4)
src/main/java/life/mosu/mosuserver/global/config/CaffeineFilterCacheConfig.java (1)
35-56: Fix: LoadingCache with a null loader will NPE — use a manual Cache insteadCaffeine prohibits null values/loader returns. Returning null from the CacheLoader will cause a NullPointerException on load. Since you’re managing entries explicitly and using Expiry, prefer a manual Cache and build without a loader.
Apply:
import com.github.benmanes.caffeine.cache.Cache; import com.github.benmanes.caffeine.cache.Caffeine; import com.github.benmanes.caffeine.cache.Expiry; -import com.github.benmanes.caffeine.cache.LoadingCache; @@ - public LoadingCache<String, BlockedIp> blockedIpCache() { - return Caffeine.newBuilder() + public Cache<String, BlockedIp> blockedIpCache() { + return Caffeine.newBuilder() .expireAfter(new Expiry<String, BlockedIp>() { @@ }) - .build(key -> null); + .build(); }src/main/java/life/mosu/mosuserver/application/oauth/OAuthUserService.java (1)
81-83: Fix potential NPE when loggingresponse.
responsecan be null;response.toString()will NPE.- log.info(response.toString()); + log.debug("Kakao service_terms response: {}", response); return response != null ? response : Collections.emptyMap();Also consider logging only selected fields (or at
debug) to avoid verbose logs and accidental leakage.src/main/java/life/mosu/mosuserver/application/exam/cache/AtomicExamQuotaDecrementOperator.java (1)
36-49: Avoid parsing exception strings from Lua for control flowRelying on substrings in exception messages is fragile and i18n-unfriendly. Prefer:
- Return explicit status codes from Lua (e.g., 0=ok, 1=not-found, 2=already-zero) and branch on result.
- Or set distinct error codes in Redis replies that you interpret here.
Happy to draft a Lua/Java pair using return codes.
src/main/java/life/mosu/mosuserver/application/exam/cache/AtomicExamQuotaIncrementOperator.java (1)
47-63: Avoid exception-message parsing; prefer Lua return codesUsing substrings of e.getMessage() for branching is brittle. Return deterministic codes from the Lua script and branch on result instead (e.g., null -> execution failure, 1 -> not found, 2 -> exceeded).
If helpful, I can provide a Lua/Java pair illustrating this.
♻️ Duplicate comments (1)
src/main/resources/application-test.yml (1)
2-4: logging.file.name overrides logging.file.path — consolidate to a single propertySpring Boot prioritizes logging.file.name; with both set, path is ignored and logs go to app.log in CWD. Consolidate:
-logging: - file: - path: ./logs - name: app.log +logging: + file: + name: ./logs/app.logOr keep only path to use default spring.log.
🧹 Nitpick comments (36)
src/main/resources/application-prod.yml (1)
27-27: Fix linter: add trailing newline at EOFYAMLlint flags “no new line character at the end of file.”
- Add a newline after Line 27.
src/main/resources/application-local.yml (3)
3-3: Avoid TRACE in local by default; allow override via env
TRACE can leak sensitive data (headers, tokens) and is noisy. Prefer DEBUG and let developers opt-in to TRACE via an env var.- root: TRACE + root: ${LOG_LEVEL:DEBUG}
6-7: Nit: consider naming “webhook-url” instead of “base-url”
If this is a single Discord webhook endpoint, “webhook-url” is clearer than “base-url”. If you genuinely need a base + paths, keep as-is.
1-16: YAML lint: add trailing newline
Static analysis flagged missing newline at EOF. Add one to satisfy linters and POSIX tools.Reference: YAMLlint new-line-at-end-of-file.
src/main/resources/application-test.yml (1)
27-27: Add trailing newline at EOFYAMLlint flags missing newline at end of file. Add a newline to satisfy linters/CI.
.github/workflows/docker-depoly.yaml (2)
25-35: Harden secret writes and ensure deterministic formattingUse printf to robustly handle multi-line secrets and set restrictive file permissions. Current echo usage is workable but less robust.
Suggested change:
-echo "${{ secrets.ENV_BASE }}" > .env -echo "${{ secrets.ENV_BASE }}" > .env.blue -echo "${{ secrets.ENV_BASE }}" > .env.green - -echo "${{ secrets.ENV }}" >> .env -echo "${{ secrets.ENV }}" >> .env.blue -echo "${{ secrets.ENV }}" >> .env.green - -echo "APP_IMAGE_VERSION=${{ github.sha }}" >> .env -echo "APP_IMAGE_VERSION=${{ github.sha }}" >> .env.blue -echo "APP_IMAGE_VERSION=${{ github.sha }}" >> .env.green +umask 077 +printf '%s\n' "${{ secrets.ENV_BASE }}" > .env +printf '%s\n' "${{ secrets.ENV_BASE }}" > .env.blue +printf '%s\n' "${{ secrets.ENV_BASE }}" > .env.green + +printf '%s\n' "${{ secrets.ENV }}" >> .env +printf '%s\n' "${{ secrets.ENV }}" >> .env.blue +printf '%s\n' "${{ secrets.ENV }}" >> .env.green + +printf '%s\n' "APP_IMAGE_VERSION=${{ github.sha }}" >> .env +printf '%s\n' "APP_IMAGE_VERSION=${{ github.sha }}" >> .env.blue +printf '%s\n' "APP_IMAGE_VERSION=${{ github.sha }}" >> .env.green
36-36: Remove trailing whitespace to satisfy YAML lintThere’s trailing space on Line 36. Delete it to fix YAMLlint error.
.github/workflows/self-depoly.yaml (3)
31-36: Shallow clone and cleanup for external JARs; avoid lingering PAT usageReduce surface area and speed up by shallow cloning and cleaning up the temp dir.
Apply:
- git clone https://x-access-token:${{ secrets.GH_PAT }}@github.com/mosu-dev/mosu-kmc-jar.git temp-jar + git clone --depth=1 https://x-access-token:${{ secrets.GH_PAT }}@github.com/mosu-dev/mosu-kmc-jar.git temp-jar cp temp-jar/*.jar libs/ + rm -rf temp-jar
55-55: Trim trailing spaces to satisfy lintersYAMLlint flagged trailing spaces on these lines. Remove them.
Also applies to: 59-59, 63-63, 67-67
68-72: Order of log message vs. actions (nit)Print “Stopping all containers...” before stopping, so logs reflect intent before action.
- sudo docker stop $(sudo docker ps -aq) || true - sudo docker rm $(sudo docker ps -aq) || true - echo "Stopping all containers..." + echo "Stopping all containers..." + sudo docker stop $(sudo docker ps -aq) || true + sudo docker rm $(sudo docker ps -aq) || truesrc/main/java/life/mosu/mosuserver/domain/caffeine/dto/BlockedIp.java (1)
16-22: Good addition: default initializer and TTL access
init()cleanly standardizes the default penalty (LEVEL_0), and usingvalue.getTtl()for expiry integration is correct.Optional: consider making this value object final (class-level) and adding a null guard in the constructor to enforce immutability and invariants.
src/main/java/life/mosu/mosuserver/application/caffeine/UpdatableCacheManager.java (1)
5-8: Clarify atomic semantics and return a signal/value from putIfAbsentVoid return gives no signal whether an insert occurred or what value is stored. For clearer and safer concurrency, consider returning either the existing value or a boolean. Also, accepting a Supplier avoids constructing the value eagerly.
Suggested interface refinement (optional):
package life.mosu.mosuserver.application.caffeine; import org.springframework.cache.Cache; +import java.util.function.Supplier; -public interface UpdatableCacheManager { - - void putIfAbsent(Cache cache, String key, Object value); -} +public interface UpdatableCacheManager { + /** + * Atomically ensures the key exists. If absent, computes and inserts the value once. + * Must not be implemented as a non-atomic get-then-put. + * @return existing value if present, otherwise the newly inserted value + */ + <V> V computeIfAbsent(Cache cache, String key, Supplier<V> valueSupplier); +}If changing the method name is undesired, an alternative is:
- boolean putIfAbsent(Cache cache, String key, Object value)
src/main/java/life/mosu/mosuserver/global/config/CaffeineFilterCacheConfig.java (1)
19-22: Prevent unbounded growth: set maximumSize for cachesBoth
ipRequestCountsCacheandblockedHistoryCachehave no size bounds. With only time-based expiry, a hot-key or bursty traffic pattern can cause memory growth.Add
.maximumSize(...)(ideally sourced from configuration) to both builders. Example:
.maximumSize(properties.getRequestCountsCacheMaxSize())for request counts.maximumSize(…)for blocked history (use a constant or property)Also applies to: 27-31
src/main/java/life/mosu/mosuserver/application/caffeine/LocalCacheConfig.java (1)
23-33: Consider capacity bounds and configurability per cacheThese caches are created with time-based expiry only. Without
.maximumSize(...), they can grow until expiry, risking memory pressure. Since you already centralize cache metadata inCacheGroup, consider adding a per-group capacity and wiring it here, e.g.:
- Add
getMaximumSize()toCacheGroup- Configure builder with
.maximumSize(g.getMaximumSize())If not feasible now, at least add a conservative default
.maximumSize(...).src/main/java/life/mosu/mosuserver/global/filter/IpRateLimitingFilter.java (3)
57-61: Close race window: re-check block state before invoking downstream chainBetween the initial
isAlreadyBlocked(ip)check andfilterChain.doFilter, another thread can block the IP, letting this request slip through. Re-check just before the chain to harden behavior under high contention.@@ - log.debug("IP: {}, 요청 횟수 증가 후: {}", ip, after); - log.debug("Cache stats: {}", ipRequestCountsCache.stats()); - - filterChain.doFilter(request, response); + log.debug("IP: {}, 요청 횟수 증가 후: {}", ip, after); + log.debug("Cache stats: {}", ipRequestCountsCache.stats()); + // Re-check to avoid pass-through after a concurrent block + BlockedIp blocked = blockedIpCache.getIfPresent(ip); + if (blocked != null) { + log.warn("요청 직전 차단 감지 - IP: {}, 차단 레벨: {}", ip, blocked.getPenaltyLevel()); + throw new CustomRuntimeException(ErrorCode.TOO_MANY_REQUESTS); + } + filterChain.doFilter(request, response);
70-79: Log block creation with resulting penalty levelCurrently, only the “already blocked” case logs a warning. When a new block is created, add a log including the computed penalty for observability.
@@ - TimePenalty level = blockedHistoryCache.asMap().compute(ip, (k, history) -> { + TimePenalty level = blockedHistoryCache.asMap().compute(ip, (k, history) -> { BlockedIpHistory h = (history == null) ? new BlockedIpHistory(ip) : history; TimePenalty next = h.getPenaltyLevel().nextLevel(); h.updateHistory(next); return h; }).getPenaltyLevel(); blockedIpCache.asMap().computeIfPresent(ip, (k, v) -> new BlockedIp(level)); + log.warn("새로 차단된 IP: {}, 차단 레벨: {}", ip, level);
82-88: Method naming: isAlreadyBlocked(...) throws, not returns
isAlreadyBlockedperforms a guard and throws. Consider renaming toensureNotBlockedorassertNotBlockedto better reflect behavior.src/main/java/life/mosu/mosuserver/application/caffeine/LocalCacheManager.java (2)
35-37: Return an unmodifiable set for cacheNamesExpose an unmodifiable
cacheNamesto prevent accidental mutation by callers.- this.cacheNames = cacheNamesSet; + this.cacheNames = Collections.unmodifiableSet(cacheNamesSet);
50-56: API ergonomics: prefer cacheName over Cache instance
putIfAbsent(Cache cache, String key, Object value)requires aCacheinstance only to re-lookup by name. Simplify toputIfAbsent(String cacheName, Object key, Object value)to decouple callers from specific instances.Example:
- public void putIfAbsent(Cache cache, String key, Object value) { - Cache localCache = getCache(cache.getName()); + public void putIfAbsent(String cacheName, Object key, Object value) { + Cache localCache = getCache(cacheName); if (localCache != null) { localCache.putIfAbsent(key, value); } }src/main/java/life/mosu/mosuserver/application/notice/NoticeService.java (2)
33-74: Separate caches for list and detail to avoid cross-type pollutionBoth paginated lists and detail objects share the same cache ("notice"). Keys differ, but mixing value types in one cache can complicate monitoring and future migrations (e.g., external cache serializers).
- @CacheEvict(cacheNames = "notice", allEntries = true) + @CacheEvict(cacheNames = {"notice:list", "notice:detail"}, allEntries = true) @@ - @Cacheable(cacheNames = "notice", - key = "'page=' + #page + ',size=' + #size") + @Cacheable(cacheNames = "notice:list", + key = "'page=' + #page + ',size=' + #size + ',sort=id'") @@ - @Cacheable(cacheNames = "notice", key = "#noticeId") + @Cacheable(cacheNames = "notice:detail", key = "#noticeId") @@ - @CacheEvict(cacheNames = "notice", allEntries = true) + @CacheEvict(cacheNames = {"notice:list", "notice:detail"}, allEntries = true) @@ - @CacheEvict(cacheNames = "notice", allEntries = true) + @CacheEvict(cacheNames = {"notice:list", "notice:detail"}, allEntries = true)
40-45: Future-proof cache key with sort expressionKey currently uses only
pageandsize. If sort changes later, entries can be incorrectly reused. Since you sort by"id", include it in the key (as in the diff above) or compute from aSortargument if it becomes dynamic.src/test/java/life/mosu/mosuserver/global/filter/IpRateLimitingFilterMultiUserIsolationTest.java (2)
84-156: Test name vs assertion mismatchThe test name states that the chain is invoked, but the test does not assert chain invocations (chain is a no-op). Either track and assert chain calls or rename the test to reflect what is asserted (counters under limit).
Option A (assert chain calls): introduce an
AtomicIntegerand increment in the chain; assert equals touserCount * requestsPerUser.Option B (rename): e.g.,
임계치_이하에서는_카운터만_정확히_증가한다.
353-387: Mark heavy concurrency tests for selective executionHigh-load tests can be flaky/slow in CI. Consider annotating with
@Tag("stress")or reducing thread counts, and gating execution by profile.src/main/resources/application.yml (2)
7-9: Add missing newline at EOF to satisfy YAML linting.YAMLlint flags “no new line character at the end of file”. Add a trailing newline.
3-6: Consider making imported external configs optional (or explicitly classpath-scoped).If
security-config.ymlorswagger-config.ymlare absent in certain environments, startup will fail. Consider marking them optional and/or scoping to classpath.Example:
- - security-config.yml - - swagger-config.yml + - optional:classpath:security-config.yml + - optional:classpath:swagger-config.ymlsrc/test/java/life/mosu/mosuserver/application/oauth/OAuthUserServiceTest.java (1)
13-43: Good baseline test; add positive/edge cases to strengthen coverage.Recommend adding:
- Case where
terms_03exists withagreed = true.- Case where
terms_03exists withagreed = false.- Case where
service_termskey is missing or not a list.Minor: use typed generics in casts to reduce unchecked warnings.
src/main/java/life/mosu/mosuserver/application/oauth/OAuthUserService.java (2)
49-49: Tidy log message (readability).Prefer a clearer template and consistent spacing.
-log.info("동의 여부{}", agreedToMarketing); +log.info("동의 여부: {}", agreedToMarketing);If this runs in prod frequently, consider
debuglevel to reduce noise.
73-80: Add client-side timeout to avoid indefinite blocking.
block()without a timeout can hang the login flow if Kakao stalls.Map<String, Object> response = webClient.get() .uri(url) .header("Authorization", "Bearer " + accessToken) .retrieve() .bodyToMono(new ParameterizedTypeReference<Map<String, Object>>() { }) + .timeout(java.time.Duration.ofSeconds(5)) .block();Alternatively, configure timeouts on the
WebClientbean (connect/read) and keep this call clean.src/main/java/life/mosu/mosuserver/application/exam/cache/ExamQuotaCacheManager.java (2)
103-109: Improve diagnostics when operator missingWhen operator == null, include available keys to speed up triage.
- if (operator == null) { - log.error("Unsupported cache operation key: {}", operationType.key()); + if (operator == null) { + log.error("Unsupported cache operation key: {}. Available keys: {}", + operationType.key(), cacheAtomicOperatorMap.keySet()); throw new CustomRuntimeException(ErrorCode.CACHE_OPERATION_NOT_SUPPORTED); }
116-119: Preserve failure cause on fallback failureIf recoverAndRetry fails, consider including the cause to aid observability (assuming CustomRuntimeException supports a cause). If not, add an error log with the original exception.
- if (!recoverAndRetry(examId, operator, operationType.key())) { - throw new CustomRuntimeException(ErrorCode.CACHE_UPDATE_FAIL); - } + if (!recoverAndRetry(examId, operator, operationType.key())) { + // Option A: preserve cause + throw new CustomRuntimeException(ErrorCode.CACHE_UPDATE_FAIL/*, e */); + // Option B: if no ctor with cause exists, ensure we log the original error context above. + }src/main/java/life/mosu/mosuserver/application/exam/cache/AtomicExamQuotaDecrementOperator.java (1)
29-31: Prefer IllegalStateException over NPE for missing scriptNPE from requireNonNull is less expressive for configuration errors.
- this.decrementScript = Objects.requireNonNull(examLuaScripts.get("decrementQuota"), - "Redis script 'decrementQuota' not found"); + DefaultRedisScript<Long> script = examLuaScripts.get("decrementQuota"); + if (script == null) { + throw new IllegalStateException("Redis script 'decrementQuota' not found in 'examLuaScripts'"); + } + this.decrementScript = script;src/main/java/life/mosu/mosuserver/infra/persistence/redis/support/LuaScriptsFunctionalRegistrar.java (2)
62-71: Optionally register beans with generic type metadataRegistering as raw Map works, but you can keep type info to improve autowiring diagnostics:
// Example (outside diff): use ResolvableType var type = ResolvableType.forClassWithGenerics(Map.class, String.class, DefaultRedisScript.class); context.registerBean(beanName, type, () -> scripts);Not mandatory, but improves clarity.
72-75: Fail-fast logging with contextConsider logging the number of scripts discovered and domains processed before throwing to aid troubleshooting.
Small nit; current message is fine if upstream logs capture cause.
src/main/java/life/mosu/mosuserver/application/exam/cache/AtomicExamQuotaIncrementOperator.java (2)
9-9: Remove unused @slf4j or add logs@slf4j is present but unused. Either remove it or add targeted logs (e.g., when Redis returns null).
-import lombok.extern.slf4j.Slf4j; @@ -@Slf4jAlso applies to: 16-16
30-32: Prefer IllegalStateException over NPE for missing scriptSame rationale as decrement operator.
- this.incrementScript = Objects.requireNonNull(examLuaScripts.get("incrementQuota"), - "Redis script 'incrementQuota' not found"); + DefaultRedisScript<Long> script = examLuaScripts.get("incrementQuota"); + if (script == null) { + throw new IllegalStateException("Redis script 'incrementQuota' not found in 'examLuaScripts'"); + } + this.incrementScript = script;src/main/java/life/mosu/mosuserver/infra/config/AtomicOperatorAutoRegistrar.java (1)
38-41: Handle duplicate action names deterministicallytoMap without a merge function will throw an IllegalStateException on duplicates with a generic message. Consider an explicit merge to improve diagnostics.
- .collect(Collectors.toMap(CacheAtomicOperator::getActionName, - Function.identity())); + .collect(Collectors.toMap( + CacheAtomicOperator::getActionName, + Function.identity(), + (a, b) -> { throw new IllegalStateException("Duplicate operator for action: " + a.getActionName()); } + ));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (40)
.github/workflows/docker-depoly.yaml(1 hunks).github/workflows/self-depoly.yaml(1 hunks)build.gradle(1 hunks)src/main/java/life/mosu/mosuserver/application/caffeine/LocalCacheConfig.java(1 hunks)src/main/java/life/mosu/mosuserver/application/caffeine/LocalCacheManager.java(1 hunks)src/main/java/life/mosu/mosuserver/application/caffeine/UpdatableCacheManager.java(1 hunks)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/application/inquiry/InquiryService.java(2 hunks)src/main/java/life/mosu/mosuserver/application/notice/NoticeService.java(3 hunks)src/main/java/life/mosu/mosuserver/application/oauth/OAuthUserPersistenceProcessor.java(1 hunks)src/main/java/life/mosu/mosuserver/application/oauth/OAuthUserService.java(3 hunks)src/main/java/life/mosu/mosuserver/domain/caffeine/CacheGroup.java(1 hunks)src/main/java/life/mosu/mosuserver/domain/caffeine/CacheType.java(1 hunks)src/main/java/life/mosu/mosuserver/domain/caffeine/dto/BlockedIp.java(1 hunks)src/main/java/life/mosu/mosuserver/domain/caffeine/dto/RequestCounter.java(1 hunks)src/main/java/life/mosu/mosuserver/domain/file/File.java(1 hunks)src/main/java/life/mosu/mosuserver/domain/user/entity/UserJpaEntity.java(1 hunks)src/main/java/life/mosu/mosuserver/global/annotation/PhoneNumberPattern.java(0 hunks)src/main/java/life/mosu/mosuserver/global/config/CaffeineFilterCacheConfig.java(2 hunks)src/main/java/life/mosu/mosuserver/global/config/ExamQuotaAtomicOperationConfig.java(0 hunks)src/main/java/life/mosu/mosuserver/global/config/SecurityConfig.java(3 hunks)src/main/java/life/mosu/mosuserver/global/config/WebMvcConfig.java(2 hunks)src/main/java/life/mosu/mosuserver/global/filter/IpRateLimitingFilter.java(2 hunks)src/main/java/life/mosu/mosuserver/global/filter/TimePenalty.java(1 hunks)src/main/java/life/mosu/mosuserver/global/filter/Whitelist.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/java/life/mosu/mosuserver/presentation/auth/dto/request/SignUpAccountRequest.java(1 hunks)src/main/resources/META-INF/spring.factories(1 hunks)src/main/resources/application-base.yml(1 hunks)src/main/resources/application-local.yml(1 hunks)src/main/resources/application-prod.yml(1 hunks)src/main/resources/application-test.yml(1 hunks)src/main/resources/application.yml(1 hunks)src/main/resources/db/data/data.sql(1 hunks)src/main/resources/security-config.yml(1 hunks)src/test/java/life/mosu/mosuserver/application/oauth/OAuthUserServiceTest.java(1 hunks)src/test/java/life/mosu/mosuserver/global/filter/IpRateLimitingFilterMultiUserIsolationTest.java(1 hunks)
💤 Files with no reviewable changes (3)
- src/main/java/life/mosu/mosuserver/global/filter/Whitelist.java
- src/main/java/life/mosu/mosuserver/global/annotation/PhoneNumberPattern.java
- src/main/java/life/mosu/mosuserver/global/config/ExamQuotaAtomicOperationConfig.java
✅ Files skipped from review due to trivial changes (9)
- src/main/resources/security-config.yml
- src/main/java/life/mosu/mosuserver/global/filter/TimePenalty.java
- src/main/resources/META-INF/spring.factories
- src/main/java/life/mosu/mosuserver/global/config/WebMvcConfig.java
- src/main/java/life/mosu/mosuserver/domain/file/File.java
- src/main/resources/db/data/data.sql
- src/main/java/life/mosu/mosuserver/domain/caffeine/CacheType.java
- src/main/resources/application-base.yml
- src/main/java/life/mosu/mosuserver/domain/caffeine/CacheGroup.java
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/life/mosu/mosuserver/application/inquiry/InquiryService.java
🧰 Additional context used
🧬 Code Graph Analysis (4)
src/main/java/life/mosu/mosuserver/global/config/CaffeineFilterCacheConfig.java (1)
src/main/java/life/mosu/mosuserver/domain/caffeine/dto/RequestCounter.java (1)
RequestCounter(5-16)
src/main/java/life/mosu/mosuserver/application/exam/cache/AtomicExamQuotaIncrementOperator.java (1)
src/main/java/life/mosu/mosuserver/application/exam/cache/AtomicExamQuotaDecrementOperator.java (1)
Component(14-60)
src/test/java/life/mosu/mosuserver/global/filter/IpRateLimitingFilterMultiUserIsolationTest.java (1)
src/main/java/life/mosu/mosuserver/domain/caffeine/dto/RequestCounter.java (1)
RequestCounter(5-16)
src/main/java/life/mosu/mosuserver/global/filter/IpRateLimitingFilter.java (1)
src/main/java/life/mosu/mosuserver/domain/caffeine/dto/RequestCounter.java (1)
RequestCounter(5-16)
🪛 YAMLlint (1.37.1)
.github/workflows/docker-depoly.yaml
[error] 36-36: trailing spaces
(trailing-spaces)
src/main/resources/application-test.yml
[error] 27-27: no new line character at the end of file
(new-line-at-end-of-file)
src/main/resources/application-local.yml
[error] 16-16: no new line character at the end of file
(new-line-at-end-of-file)
.github/workflows/self-depoly.yaml
[error] 55-55: trailing spaces
(trailing-spaces)
[error] 59-59: trailing spaces
(trailing-spaces)
[error] 63-63: trailing spaces
(trailing-spaces)
[error] 67-67: trailing spaces
(trailing-spaces)
src/main/resources/application.yml
[error] 9-9: no new line character at the end of file
(new-line-at-end-of-file)
src/main/resources/application-prod.yml
[error] 27-27: no new line character at the end of file
(new-line-at-end-of-file)
🪛 actionlint (1.7.7)
.github/workflows/self-depoly.yaml
13-13: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
16-16: the runner of "actions/setup-java@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
22-22: the runner of "actions/cache@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
48-48: string should not be empty
(syntax-check)
🔇 Additional comments (14)
src/main/resources/application-prod.yml (1)
1-4: Verify log directory exists at runtimeIf ./logs doesn’t exist, some logging backends won’t create the directory and logging fails. Ensure your container/entrypoint pre-creates it or switch to a single logging.file.name path.
- Confirm entrypoint creates ./logs (e.g., mkdir -p ./logs).
- Alternatively:
- logging.file.name: ./logs/app.log (single property)
src/main/resources/application-local.yml (1)
10-13: Confirmed safe: missing ALIMTALK vars won’t prevent startup
Thealimtalkproperties are bound via @ConfigurationProperties in NotifyProperties (no strict @value), so Spring will inject null for any unset values rather than fail on startup. No defaults or YAML changes are required.src/main/java/life/mosu/mosuserver/global/config/SecurityConfig.java (1)
34-43: Import reordering: OKReordering stays consistent with Spring/Security and project packages; no functional impact.
src/main/java/life/mosu/mosuserver/global/config/CaffeineFilterCacheConfig.java (1)
18-23: Ignore missing import – same packageBoth IpRateLimitingProperties and CaffeineFilterCacheConfig reside in life.mosu.mosuserver.global.config, so Java allows using the class without an import. The code compiles as-is.
Likely an incorrect or invalid review comment.
src/main/java/life/mosu/mosuserver/domain/caffeine/dto/RequestCounter.java (1)
7-15: Thread-safe increment looks goodUsing AtomicInteger with
incrementAndGet()andgetCount()is appropriate and removes the need for external synchronization.src/main/java/life/mosu/mosuserver/global/filter/IpRateLimitingFilter.java (1)
70-76: No import needed for TimePenalty in IpRateLimitingFilterTimePenalty is declared in src/main/java/life/mosu/mosuserver/global/filter/TimePenalty.java—the same package as IpRateLimitingFilter—so it doesn’t require an explicit import. You can safely ignore the suggestion to add one.
Likely an incorrect or invalid review comment.
src/main/java/life/mosu/mosuserver/presentation/auth/dto/request/SignUpAccountRequest.java (1)
60-60: Ensure null-safety forserviceTermRequestAdd a null‐check or validation annotation to prevent a potential NPE when calling
serviceTermRequest.agreedToMarketing():• In SignUpAccountRequest.java, annotate the
serviceTermRequestfield with@NotNull(from javax.validation) or handle null defensively in the mapper:.agreedToMarketing( request.getServiceTermRequest() != null && request.getServiceTermRequest().agreedToMarketing() )• Verify that your validation layer enforces
serviceTermRequestis present before mapping.
• Also consider sourcing TOS/privacy consent flags fromserviceTermRequestrather than hard-codingtrueto keep behavior consistent.src/main/java/life/mosu/mosuserver/domain/user/entity/UserJpaEntity.java (1)
96-104: LGTM — update method now includes marketing consent.Change is cohesive with OAuth persistence updates. JPA dirty checking under a transaction will persist these field updates.
src/main/java/life/mosu/mosuserver/application/oauth/OAuthUserPersistenceProcessor.java (2)
22-30: LGTM — update path relies on JPA dirty checking within @transactional.The managed entity will be updated without explicit save, which is fine here.
33-44: Verify defaulting consents and role on first-time OAuth users.You default TOS/Privacy to
trueand role toROLE_PENDING. Confirm this matches business rules for OAuth onboarding and is consistent with manual signup.src/main/java/life/mosu/mosuserver/application/oauth/OAuthUserService.java (2)
23-23: Good separation of concerns; confirm transactional boundary on persistence.Delegating to
OAuthUserPersistenceProcessoris a clean cut. Please confirmprocess(...)executes within a proper transactional boundary to avoid partial writes or lazy-initialization issues during OAuth login.
60-60: Verify null-safety ofoAuthUserPersistenceProcessor.process(userInfo)Before adding a null‐check here, confirm whether the
process(...)method is guaranteed to return a non-nullUserJpaEntity. If it can ever return null, you should fail fast at this boundary—either withObjects.requireNonNull(...)or by throwing anOAuth2AuthenticationExceptionwith a clear error.• Audit
src/main/java/life/mosu/mosuserver/application/oauth/OAuthUserPersistenceProcessor.javato see ifprocess(...)can return null (check its implementation or any nullability annotations).
• If it may return null, update the service accordingly:- final UserJpaEntity oAuthUser = oAuthUserPersistenceProcessor.process(userInfo); + final UserJpaEntity oAuthUser = + java.util.Objects.requireNonNull( + oAuthUserPersistenceProcessor.process(userInfo), + "OAuthUserPersistenceProcessor.process() returned null for userInfo=" + userInfo);Or replace the null check with an
OAuth2AuthenticationExceptioncarrying a meaningfulOAuth2Error.src/main/java/life/mosu/mosuserver/application/exam/cache/AtomicExamQuotaDecrementOperator.java (1)
24-27: Verify script-map bean presence and wiringThis relies on a bean named "examLuaScripts" registered by LuaScriptsFunctionalRegistrar. Please ensure:
- scripts are under src/main/resources/scripts/exam/*.lua
- filenames follow snake_case to map to camelCase keys (e.g., decrement_quota.lua -> "decrementQuota")
- spring.factories registers the initializer
I can provide a quick repo scan to confirm.
src/main/java/life/mosu/mosuserver/infra/persistence/redis/support/LuaScriptsFunctionalRegistrar.java (1)
24-71: Initializer Registration Confirmed
- Verified that
LuaScriptsFunctionalRegistraris registered insrc/main/resources/META-INF/spring.factoriesunderApplicationContextInitializer.- Confirmed Lua scripts are present at
src/main/resources/scripts/**.lua.No further changes required.
| - name: Checkout source | ||
| uses: actions/checkout@v3 | ||
|
|
||
| - name: Set up JDK 21 | ||
| uses: actions/setup-java@v3 | ||
| with: | ||
| java-version: '21' | ||
| distribution: 'temurin' | ||
|
|
There was a problem hiding this comment.
Update deprecated action versions (checkout/setup-java/cache)
actionlint flags these actions as too old. Upgrade to v4 for better support and security fixes.
Apply:
-uses: actions/checkout@v3
+uses: actions/checkout@v4
...
-uses: actions/setup-java@v3
+uses: actions/setup-java@v4
...
-uses: actions/cache@v3
+uses: actions/cache@v4Also applies to: 21-30
🧰 Tools
🪛 actionlint (1.7.7)
13-13: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
16-16: the runner of "actions/setup-java@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🤖 Prompt for AI Agents
In .github/workflows/self-depoly.yaml around lines 12 to 30, the actions
'actions/checkout' and 'actions/setup-java' are using deprecated versions v3.
Update these actions to use version v4 by changing 'actions/checkout@v3' to
'actions/checkout@v4' and 'actions/setup-java@v3' to 'actions/setup-java@v4' to
ensure better support and security fixes.
| working-directory: | ||
| - name: Push Docker image |
There was a problem hiding this comment.
Fix invalid empty 'working-directory' key
The empty value breaks workflow parsing (“string should not be empty”). Either set it explicitly or remove it.
Apply:
- working-directory:
+ # working-directory: .📝 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.
| working-directory: | |
| - name: Push Docker image | |
| # working-directory: . | |
| - name: Push Docker image |
🧰 Tools
🪛 actionlint (1.7.7)
48-48: string should not be empty
(syntax-check)
🤖 Prompt for AI Agents
In .github/workflows/self-depoly.yaml at lines 48 to 49, the 'working-directory'
key is empty, causing a workflow parsing error. Remove the 'working-directory'
key entirely or set it to a valid non-empty string to fix the invalid empty key
issue.
| echo "${{ secrets.ENV_BASE }}" > .env | ||
| echo "${{ secrets.ENV_BASE }}" > .env.blue | ||
| echo "${{ secrets.ENV_BASE }}" > .env.green | ||
|
|
||
| echo "${{ secrets.ENV_TEST }}" >> .env | ||
| echo "${{ secrets.ENV_TEST }}" >> .env.blue | ||
| echo "${{ secrets.ENV_TEST }}" >> .env.green | ||
|
|
||
| echo "APP_IMAGE_VERSION=${{ github.sha }}" >> .env | ||
| echo "APP_IMAGE_VERSION=${{ github.sha }}" >> .env.blue | ||
| echo "APP_IMAGE_VERSION=${{ github.sha }}" >> .env.green | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Safer multi-line secret writes and file permissions
Use printf and a restrictive umask to prevent permission leaks. Also keeps newline handling deterministic.
Apply:
- echo "${{ secrets.ENV_BASE }}" > .env
- echo "${{ secrets.ENV_BASE }}" > .env.blue
- echo "${{ secrets.ENV_BASE }}" > .env.green
-
- echo "${{ secrets.ENV_TEST }}" >> .env
- echo "${{ secrets.ENV_TEST }}" >> .env.blue
- echo "${{ secrets.ENV_TEST }}" >> .env.green
-
- echo "APP_IMAGE_VERSION=${{ github.sha }}" >> .env
- echo "APP_IMAGE_VERSION=${{ github.sha }}" >> .env.blue
- echo "APP_IMAGE_VERSION=${{ github.sha }}" >> .env.green
+ umask 077
+ printf '%s\n' "${{ secrets.ENV_BASE }}" > .env
+ printf '%s\n' "${{ secrets.ENV_BASE }}" > .env.blue
+ printf '%s\n' "${{ secrets.ENV_BASE }}" > .env.green
+
+ printf '%s\n' "${{ secrets.ENV_TEST }}" >> .env
+ printf '%s\n' "${{ secrets.ENV_TEST }}" >> .env.blue
+ printf '%s\n' "${{ secrets.ENV_TEST }}" >> .env.green
+
+ printf '%s\n' "APP_IMAGE_VERSION=${{ github.sha }}" >> .env
+ printf '%s\n' "APP_IMAGE_VERSION=${{ github.sha }}" >> .env.blue
+ printf '%s\n' "APP_IMAGE_VERSION=${{ github.sha }}" >> .env.green📝 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.
| echo "${{ secrets.ENV_BASE }}" > .env | |
| echo "${{ secrets.ENV_BASE }}" > .env.blue | |
| echo "${{ secrets.ENV_BASE }}" > .env.green | |
| echo "${{ secrets.ENV_TEST }}" >> .env | |
| echo "${{ secrets.ENV_TEST }}" >> .env.blue | |
| echo "${{ secrets.ENV_TEST }}" >> .env.green | |
| echo "APP_IMAGE_VERSION=${{ github.sha }}" >> .env | |
| echo "APP_IMAGE_VERSION=${{ github.sha }}" >> .env.blue | |
| echo "APP_IMAGE_VERSION=${{ github.sha }}" >> .env.green | |
| umask 077 | |
| printf '%s\n' "${{ secrets.ENV_BASE }}" > .env | |
| printf '%s\n' "${{ secrets.ENV_BASE }}" > .env.blue | |
| printf '%s\n' "${{ secrets.ENV_BASE }}" > .env.green | |
| printf '%s\n' "${{ secrets.ENV_TEST }}" >> .env | |
| printf '%s\n' "${{ secrets.ENV_TEST }}" >> .env.blue | |
| printf '%s\n' "${{ secrets.ENV_TEST }}" >> .env.green | |
| printf '%s\n' "APP_IMAGE_VERSION=${{ github.sha }}" >> .env | |
| printf '%s\n' "APP_IMAGE_VERSION=${{ github.sha }}" >> .env.blue | |
| printf '%s\n' "APP_IMAGE_VERSION=${{ github.sha }}" >> .env.green |
🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 59-59: trailing spaces
(trailing-spaces)
[error] 63-63: trailing spaces
(trailing-spaces)
[error] 67-67: trailing spaces
(trailing-spaces)
🤖 Prompt for AI Agents
In .github/workflows/self-depoly.yaml around lines 56 to 67, the current use of
echo to write multi-line secrets to .env files can cause permission leaks and
inconsistent newline handling. Replace echo commands with printf for
deterministic newlines and set a restrictive umask before writing to ensure the
created files have secure permissions. This will prevent accidental exposure of
sensitive environment variables.
| // Lombok | ||
| compileOnly 'org.projectlombok:lombok' | ||
| annotationProcessor 'org.projectlombok:lombok' | ||
| testCompileOnly 'org.projectlombok:lombok' | ||
| testImplementation 'org.projectlombok:lombok' | ||
| testAnnotationProcessor 'org.projectlombok:lombok' |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid pulling Lombok onto the test runtime classpath
testImplementation 'org.projectlombok:lombok' is unnecessary and brings Lombok onto the test runtime classpath. Lombok should be compile-only with an annotation processor. You already have testCompileOnly and testAnnotationProcessor.
Apply this diff:
// Lombok
compileOnly 'org.projectlombok:lombok'
annotationProcessor 'org.projectlombok:lombok'
testCompileOnly 'org.projectlombok:lombok'
-testImplementation 'org.projectlombok:lombok'
testAnnotationProcessor 'org.projectlombok:lombok'📝 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.
| // Lombok | |
| compileOnly 'org.projectlombok:lombok' | |
| annotationProcessor 'org.projectlombok:lombok' | |
| testCompileOnly 'org.projectlombok:lombok' | |
| testImplementation 'org.projectlombok:lombok' | |
| testAnnotationProcessor 'org.projectlombok:lombok' | |
| // Lombok | |
| compileOnly 'org.projectlombok:lombok' | |
| annotationProcessor 'org.projectlombok:lombok' | |
| testCompileOnly 'org.projectlombok:lombok' | |
| testAnnotationProcessor 'org.projectlombok:lombok' |
🤖 Prompt for AI Agents
In build.gradle around lines 36 to 41, remove the line `testImplementation
'org.projectlombok:lombok'` to avoid including Lombok on the test runtime
classpath. Keep only `testCompileOnly` and `testAnnotationProcessor` for Lombok
in the test scope to ensure it is used only at compile time and annotation
processing.
| private Map<String, Cache> cacheMap = new ConcurrentHashMap<>(); | ||
| private volatile Set<String> cacheNames = Collections.emptySet(); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Potential publication visibility issue for cacheMap
cacheMap is reassigned in @PostConstruct but is not volatile or final. Although Spring typically publishes beans safely, marking it volatile avoids subtle visibility issues.
- private Map<String, Cache> cacheMap = new ConcurrentHashMap<>();
+ private volatile Map<String, Cache> cacheMap = new ConcurrentHashMap<>();📝 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.
| private Map<String, Cache> cacheMap = new ConcurrentHashMap<>(); | |
| private volatile Set<String> cacheNames = Collections.emptySet(); | |
| private volatile Map<String, Cache> cacheMap = new ConcurrentHashMap<>(); | |
| private volatile Set<String> cacheNames = Collections.emptySet(); |
🤖 Prompt for AI Agents
In
src/main/java/life/mosu/mosuserver/application/caffeine/LocalCacheManager.java
around lines 18 to 20, the cacheMap field is reassigned after construction but
is not declared volatile or final, which may cause visibility issues in
concurrent contexts. To fix this, declare cacheMap as volatile to ensure
visibility of its updates across threads, preventing subtle concurrency bugs.
| exposure: | ||
| include: "*" |
There was a problem hiding this comment.
Don’t expose all Actuator endpoints in production
Exposing "*" is risky. Limit to only what you need (e.g., health/info/prometheus) and hide health details.
management:
endpoints:
web:
exposure:
- include: "*"
+ include: "health,info,prometheus"
+ endpoint:
+ health:
+ show-details: "never"🤖 Prompt for AI Agents
In src/main/resources/application-prod.yml at lines 11 to 12, the Actuator
endpoints are currently exposed with include: "*", which is insecure for
production. Modify the configuration to expose only specific endpoints such as
health, info, and prometheus, and configure health details to be hidden. Replace
the wildcard with a limited list of endpoints to reduce the attack surface.
| discord: | ||
| base-url: ${DISCORD_URL} | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make Discord URL optional with a safe default
The PR goal mentions handling missing Discord URL gracefully. Without a default, unresolved placeholders can still break startup depending on how properties are bound. Use an empty-string default so the app can boot and your code can decide to skip notify.
discord:
- base-url: ${DISCORD_URL}
+ base-url: ${DISCORD_URL:}🤖 Prompt for AI Agents
In src/main/resources/application-prod.yml around lines 16 to 18, the discord
base-url property uses a placeholder without a default value, which can cause
startup failures if DISCORD_URL is not set. Modify the property to provide an
empty-string default by changing the placeholder to use the syntax
${DISCORD_URL:}, ensuring the application can start safely even if the
environment variable is missing.
| management: | ||
| endpoints: | ||
| web: | ||
| exposure: | ||
| include: "*" |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Exposing all actuator endpoints is fine for tests; ensure prod/stage don’t use "*"
Confirm non-test profiles don’t expose all endpoints. Prod should typically include only health,info (or be disabled behind auth).
🏁 Script executed:
#!/bin/bash
# Find profile-specific YAMLs and inspect actuator exposure settings
fd -a '^application(-.+)?\.ya?ml$' | sort
echo "---- Actuator exposure occurrences ----"
rg -n -A2 -i $'^\\s*management:\\n\\s*endpoints:\\n\\s*web:\\n\\s*exposure:\\n\\s*include:' | sed 's/^/ /'
echo "---- application-prod*.yml quick check ----"
fd -a '^application-prod.*\.ya?ml$' | xargs -r rg -n -A2 -i 'management:|endpoints:|web:|exposure:|include:'Length of output: 1106
Restrict actuator endpoints in production
Production (application-prod.yml) is currently exposing all actuator endpoints (include: “*”), which is fine for tests but risky in production. Please limit exposure to only the endpoints you need (e.g., health, info) or secure them behind authentication.
• File to update: src/main/resources/application-prod.yml (around lines 8–12)
• Change from:
management:
endpoints:
web:
exposure:
include: "*"To something like:
management:
endpoints:
web:
exposure:
- include: "*"
+ include:
+ - health
+ - info🤖 Prompt for AI Agents
In src/main/resources/application-prod.yml around lines 8 to 12, the actuator
endpoints are currently exposed with include: "*", which is insecure for
production. Update the configuration to restrict exposure by replacing the
wildcard with a limited list of necessary endpoints such as "health" and "info".
This will limit the endpoints exposed in production and improve security.
| discord: | ||
| base-url: "" | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Empty discord.base-url may not be treated as “missing” — prefer env placeholder or null
If the code checks “missing” via null/absence, an empty string can bypass that and still be “configured”. Safer options:
-discord:
- base-url: ""
+discord:
+ base-url: ${DISCORD_BASE_URL:}Or remove the property entirely in test to simulate missing config. Verify the code treats blank values as absent (e.g., StringUtils.hasText).
Run to locate consumers and checks:
🏁 Script executed:
#!/bin/bash
# Find usages of discord base-url in Java/Kotlin/Properties/YAML
rg -n -A2 -i 'discord\\.?base-url|DISCORD_BASE_URL|discord.*baseUrl|baseUrl.*discord' --glob '!**/build/**' --glob '!**/target/**'Length of output: 132
🏁 Script executed:
#!/bin/bash
echo "=== Searching for 'discord' references in Java/Kotlin source files ==="
rg -n 'discord' -tjava -tkt --glob '!**/build/**' --glob '!**/target/**' || echo "No 'discord' references found"
echo
echo "=== Searching for @ConfigurationProperties annotations ==="
rg -n '@ConfigurationProperties' -tjava -tkt --glob '!**/build/**' --glob '!**/target/**' || echo "No @ConfigurationProperties annotations found"Length of output: 684
🏁 Script executed:
#!/bin/bash
echo "=== Searching for 'discord' references across codebase ==="
rg -n 'discord' --glob '*.java' --glob '*.kt' --glob '*.yml' --glob '*.yaml' --glob '*.properties' --glob '!**/build/**' --glob '!**/target/**' -A2 || echo "No 'discord' references found"
echo
echo "=== Searching specifically for 'base-url' usages ==="
rg -n 'base-url' --glob '*.java' --glob '*.kt' --glob '*.yml' --glob '*.yaml' --glob '*.properties' --glob '!**/build/**' --glob '!**/target/**' -A2 || echo "No 'base-url' references found"
echo
echo "=== Searching for @Value injections of discord ==="
rg -n '@Value.*discord' --glob '*.java' --glob '*.kt' --glob '!**/build/**' --glob '!**/target/**' -A2 || echo "No @Value injections for discord found"Length of output: 7266
🏁 Script executed:
#!/bin/bash
echo "=== Extracting DiscordNotifier implementation ==="
rg -n 'class DiscordNotifier' -A50 -B5 src/main/java/life/mosu/mosuserver/infra/notify/DiscordNotifier.javaLength of output: 1597
Remove empty discord.base-url override in test config to ensure missing-config fallback works
In src/main/resources/application-test.yml, an explicit empty string (base-url: "") will be treated as a configured value (not missing), causing the notifier to attempt posting to "" rather than skipping. Instead, let Spring fall back to the default URL (http://default.com) that the code uses to detect “not set”:
• File: src/main/resources/application-test.yml
Lines 16–18
Suggested changes:
discord:
- base-url: ""
+ # Remove the empty override so the default (http://default.com) is used,
+ # triggering the `== "http://default.com"` check and skipping sends in tests.Or, if you prefer an explicit placeholder:
discord:
base-url: ${DISCORD_BASE_URL:http://default.com}Either option ensures that blank values don’t bypass the “missing” check and trigger real HTTP calls during tests.
🤖 Prompt for AI Agents
In src/main/resources/application-test.yml at lines 16 to 18, remove the empty
string assignment for discord.base-url to avoid it being treated as a configured
value. Instead, either delete the base-url line entirely to allow Spring to use
the default fallback or replace it with a placeholder like
${DISCORD_BASE_URL:http://default.com} so that missing or blank values correctly
trigger the fallback logic and prevent unintended HTTP calls during tests.
| // 초기 레벨이 LEVEL_0이라고 가정하고, 상승 후 LEVEL_1을 기대 | ||
| log.info("history: {}", history); | ||
| assertThat(history.getPenaltyLevel()).isEqualTo(TimePenalty.LEVEL_1); | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Missing import for TimePenalty in test
TimePenalty.LEVEL_1 is referenced but not imported. Add the import to avoid compilation errors.
Run to verify import presence:
Apply:
@@
import life.mosu.mosuserver.domain.caffeine.dto.RequestCounter;
+import life.mosu.mosuserver.domain.caffeine.dto.TimePenalty;🏁 Script executed:
#!/bin/bash
fd -a "IpRateLimitingFilterMultiUserIsolationTest.java" | xargs -I{} rg -n "import .*TimePenalty|\\bTimePenalty\\b" {}Length of output: 202
Add missing TimePenalty import in the test
The reference to TimePenalty.LEVEL_1 in IpRateLimitingFilterMultiUserIsolationTest.java isn’t covered by any existing import, causing compilation failures. Add the import alongside RequestCounter:
File: src/test/java/life/mosu/mosuserver/global/filter/IpRateLimitingFilterMultiUserIsolationTest.java
@@ (imports)
import life.mosu.mosuserver.domain.caffeine.dto.RequestCounter;
+import life.mosu.mosuserver.domain.caffeine.dto.TimePenalty;📝 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.
| // 초기 레벨이 LEVEL_0이라고 가정하고, 상승 후 LEVEL_1을 기대 | |
| log.info("history: {}", history); | |
| assertThat(history.getPenaltyLevel()).isEqualTo(TimePenalty.LEVEL_1); | |
| @@ (imports) | |
| import life.mosu.mosuserver.domain.caffeine.dto.RequestCounter; | |
| +import life.mosu.mosuserver.domain.caffeine.dto.TimePenalty; |
🤖 Prompt for AI Agents
In
src/test/java/life/mosu/mosuserver/global/filter/IpRateLimitingFilterMultiUserIsolationTest.java
around lines 487 to 490, the code references TimePenalty.LEVEL_1 without
importing TimePenalty, causing compilation errors. Fix this by adding an import
statement for TimePenalty at the top of the file alongside the existing import
for RequestCounter.
4 participants
✨ 구현한 기능
📢 논의하고 싶은 내용
🎸 기타
Summary by CodeRabbit
New Features
Bug Fixes
Chores