MOSU-240 refactor: 반복적인 분당 요청에 대해 제한 시간 증가 및 하루 차단된 사용자들 DB로 영속화#245
MOSU-240 refactor: 반복적인 분당 요청에 대해 제한 시간 증가 및 하루 차단된 사용자들 DB로 영속화#245
Conversation
|
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. WalkthroughThis update introduces a multi-level IP rate limiting system with escalating time penalties, persistent blocked IP history, and batch archiving of severe offenders. It adds new domain classes, JPA entities, repositories, and cache configurations, while refactoring the filter logic to utilize these new structures and support penalty escalation and tracking. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant IpRateLimitingFilter
participant ipRequestCountsCache
participant blockedIpCache
participant blockedHistoryCache
Client->>IpRateLimitingFilter: HTTP request
IpRateLimitingFilter->>blockedIpCache: Check if IP is blocked
alt Blocked
IpRateLimitingFilter-->>Client: Throw RateLimitException
else Not Blocked
IpRateLimitingFilter->>ipRequestCountsCache: Retrieve/increment RequestCounter
IpRateLimitingFilter->>IpRateLimitingFilter: Check if over max requests
alt Over Limit
IpRateLimitingFilter->>blockedHistoryCache: Retrieve/update BlockedIpHistory
IpRateLimitingFilter->>blockedIpCache: Update with new BlockedIp (escalated penalty)
IpRateLimitingFilter-->>Client: Throw RateLimitException
else Under Limit
IpRateLimitingFilter-->>Client: Allow request
end
end
sequenceDiagram
participant BlockedIpBatchAchiever
participant blockedHistoryCache
participant BlockedIpHistoryLogJpaJpaRepository
BlockedIpBatchAchiever->>blockedHistoryCache: Get all BlockedIpHistory
BlockedIpBatchAchiever->>BlockedIpBatchAchiever: Filter by LEVEL_5 penalty
BlockedIpBatchAchiever->>BlockedIpHistoryLogJpaJpaRepository: saveAllUsingBatch(batch)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (6)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (4)
🔇 Additional comments (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 @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 refactored the IP rate limiting functionality to enhance its robustness and effectiveness. The primary goal was to implement a progressive blocking strategy, where repeated violations by an IP address lead to escalating timeout periods. This change also includes enabling the rate limiting filter by default and improving the management of blocked IPs and their request counts through dedicated caching mechanisms. The overall impact is a more resilient system that can better handle abusive or repetitive requests.
Highlights
- Default Activation of IP Rate Limiting: The IP rate limiting filter is now enabled by default, ensuring that all incoming requests are subject to rate limiting checks.
- Introduction of Progressive IP Blocking: The rate limiting mechanism has been upgraded to implement progressive blocking. Instead of a flat block, repeated violations by an IP will now result in increasingly longer blocking durations, managed by a new
TimePenaltyenum. - Enhanced IP Blocking Management with Caching: Two new Caffeine caches,
blockedHistoryCacheandblockedIpCache, have been integrated to efficiently manage and track the blocking status and history of IPs. This allows for in-memory persistence of blocked IPs and their penalty levels for defined durations. - Refactoring of Request Counter Logic: The internal
RequestCounterlogic has been refactored into a dedicated DTO, improving code organization and maintainability within the rate limiting filter.
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 sophisticated rate-limiting mechanism with progressive time penalties for repeated offenders, which is a great enhancement for service protection. The implementation correctly uses Caffeine caches for tracking request counts and blocked IPs. However, there's a critical issue where the request that triggers a block is not immediately rejected, but processed successfully. Only subsequent requests are blocked. I've provided a suggestion to fix this. Additionally, there are several minor improvements suggested for code quality, such as removing unused code, fixing a log message typo, and improving encapsulation in DTOs.
src/main/java/life/mosu/mosuserver/global/filter/IpRateLimitingFilter.java
Show resolved
Hide resolved
src/main/java/life/mosu/mosuserver/global/config/IpRateLimitingProperties.java
Outdated
Show resolved
Hide resolved
src/main/java/life/mosu/mosuserver/global/filter/TimePenalty.java
Outdated
Show resolved
Hide resolved
| public class BlockedHistory { | ||
| private String ip; | ||
|
|
||
| @Enumerated(EnumType.STRING) |
There was a problem hiding this comment.
The @Enumerated(EnumType.STRING) annotation is part of the Jakarta Persistence API (JPA) and is used for mapping enums to database columns. Since this BlockedHistory class is a DTO used within a Caffeine cache and not a JPA entity, this annotation is unnecessary and misleading. It should be removed.
src/main/java/life/mosu/mosuserver/global/filter/dto/BlockedIp.java
Outdated
Show resolved
Hide resolved
src/main/java/life/mosu/mosuserver/global/filter/dto/RequestCounter.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: 8
🧹 Nitpick comments (10)
src/main/java/life/mosu/mosuserver/global/config/IpRateLimitingProperties.java (2)
22-22: Consider adding validation for the timePenalty field.The field addition supports the multi-level penalty system correctly. Consider adding
@NotNullvalidation if this field is required for proper system operation.
22-22: Consider adding validation for the TimePenalty property.The
timePenaltyproperty lacks validation constraints, unlike other properties in this configuration class. Consider adding appropriate validation if this property is required for the rate limiting functionality to work correctly.+@NotNull private TimePenalty timePenalty;src/main/java/life/mosu/mosuserver/global/filter/dto/BlockedIp.java (3)
8-20: Improve encapsulation and clean up formatting.The class logic is correct, but consider these improvements:
- Field visibility: The
penaltyLevelfield should beprivatefor proper encapsulation- Formatting: Remove the unnecessary empty line
Apply this diff:
@Getter public class BlockedIp { - TimePenalty penaltyLevel; + private final TimePenalty penaltyLevel; public BlockedIp(TimePenalty penaltyLevel) { this.penaltyLevel = penaltyLevel; } public Duration getTtl(){ return penaltyLevel.getDuration(); } - - }
9-9: Improve field visibility.The
penaltyLevelfield should beprivatefor better encapsulation, since Lombok's@Getterwill generate the appropriate getter method.- TimePenalty penaltyLevel; + private final TimePenalty penaltyLevel;
19-20: Remove unnecessary empty lines.- -src/main/java/life/mosu/mosuserver/global/filter/TimePenalty.java (2)
4-4: Remove unused import.The
TimeUnitimport is not used in this class.Apply this diff:
import java.time.Duration; -import java.util.concurrent.TimeUnit; import lombok.Getter;
4-4: Remove unused import.The
java.util.concurrent.TimeUnitimport is not used in this class.-import java.util.concurrent.TimeUnit;src/main/java/life/mosu/mosuserver/global/filter/IpRateLimitingFilter.java (1)
82-91: Consider alternative synchronization approachUsing cached objects as synchronization monitors can be problematic if cache eviction occurs during synchronization. Consider using a dedicated lock structure or atomic operations.
Alternative approach using atomic operations:
RequestCounter counter = ipRequestCountsCache.get(ip, k -> new RequestCounter()); int currentCount = counter.incrementAndGet(); // Assuming RequestCounter uses AtomicInteger if (currentCount >= ipRateLimitingProperties.getMaxRequestsPerMinute()) { log.warn("차단된 IP: {}, 요청 횟수: {}", ip, currentCount); handleBlockedIp(ip); }src/main/java/life/mosu/mosuserver/global/filter/dto/BlockedHistory.java (2)
9-11: Consider constructor design consistency.Having both
@RequiredArgsConstructor(which would create a constructor requiring all final fields) and a custom constructor can be confusing. Since you have a custom constructor that sets defaults, consider removing the Lombok annotation.-@RequiredArgsConstructor public class BlockedHistory {
12-16: Improve field visibility.Make fields
privatefor better encapsulation, since@Getterwill generate appropriate accessor methods.- private String ip; + private final String ip; @Enumerated(EnumType.STRING) - private TimePenalty penaltyLevel; - private long blockedAt; + private TimePenalty penaltyLevel; + private long blockedAt;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/main/java/life/mosu/mosuserver/global/config/IpRateLimitingProperties.java(2 hunks)src/main/java/life/mosu/mosuserver/global/filter/IpRateLimitingFilter.java(4 hunks)src/main/java/life/mosu/mosuserver/global/filter/TimePenalty.java(1 hunks)src/main/java/life/mosu/mosuserver/global/filter/dto/BlockedHistory.java(1 hunks)src/main/java/life/mosu/mosuserver/global/filter/dto/BlockedIp.java(1 hunks)src/main/java/life/mosu/mosuserver/global/filter/dto/RequestCounter.java(1 hunks)
🔇 Additional comments (11)
src/main/java/life/mosu/mosuserver/global/config/IpRateLimitingProperties.java (3)
4-4: LGTM!The import is correctly added to support the new TimePenalty field.
16-16: Verify the impact of enabling rate limiting by default.The change enables IP rate limiting by default, which represents a significant behavioral change that could affect existing deployments.
16-16: Consider the impact of enabling rate limiting by default.Enabling rate limiting by default (
enabled = true) could affect existing users who weren't previously subject to these restrictions. Ensure this change has been communicated and tested thoroughly in staging environments.src/main/java/life/mosu/mosuserver/global/filter/TimePenalty.java (4)
10-16: LGTM! Well-designed penalty escalation system.The penalty levels provide a good progressive escalation from 1 minute to 1 day, which aligns with common rate limiting practices.
21-26: LGTM! Safe and correct progression logic.The method properly handles the boundary condition and safely progresses through penalty levels using ordinal positioning.
11-16: LGTM! Well-designed penalty progression.The penalty levels provide a sensible escalation from 1 minute to 1 day, which should effectively deter abuse while not being overly punitive for occasional violations.
21-26: Robust nextLevel() implementation.The method correctly handles the edge case of staying at LEVEL_5 and uses ordinal-based progression for clean escalation logic.
src/main/java/life/mosu/mosuserver/global/filter/dto/BlockedHistory.java (4)
23-27: LGTM! Proper initialization with default penalty level.The constructor correctly initializes new blocked history entries with the lowest penalty level and current timestamp.
18-21: LGTM! Correct penalty escalation logic.The method properly updates both penalty level and timestamp for penalty escalation tracking.
14-15: LGTM! Proper JPA enumeration handling.Using
@Enumerated(EnumType.STRING)is the correct approach for persisting the TimePenalty enum, ensuring database readability and compatibility.
18-21: Well-designed history update method.The
updateHistorymethod correctly updates both the penalty level and timestamp atomically, maintaining consistency in the blocking history.
| @Getter | ||
| @RequiredArgsConstructor | ||
| public class BlockedHistory { | ||
| private String ip; | ||
|
|
||
| @Enumerated(EnumType.STRING) | ||
| private TimePenalty penaltyLevel; | ||
| private long blockedAt; |
There was a problem hiding this comment.
Critical: Missing JPA entity configuration for database persistence.
The class uses @Enumerated annotation but lacks essential JPA annotations for database persistence. Based on the PR objective mentioning "DB로 영속화" (persist to DB), this class needs proper entity configuration.
Apply this diff to add required JPA annotations:
+import jakarta.persistence.Entity;
+import jakarta.persistence.Id;
+import jakarta.persistence.Table;
import jakarta.persistence.EnumType;
import jakarta.persistence.Enumerated;
import life.mosu.mosuserver.global.filter.TimePenalty;
import lombok.Getter;
import lombok.RequiredArgsConstructor;
+@Entity
+@Table(name = "blocked_history")
@Getter
@RequiredArgsConstructor
public class BlockedHistory {
+ @Id
private String ip;📝 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.
| @Getter | |
| @RequiredArgsConstructor | |
| public class BlockedHistory { | |
| private String ip; | |
| @Enumerated(EnumType.STRING) | |
| private TimePenalty penaltyLevel; | |
| private long blockedAt; | |
| import jakarta.persistence.Entity; | |
| import jakarta.persistence.Id; | |
| import jakarta.persistence.Table; | |
| import jakarta.persistence.EnumType; | |
| import jakarta.persistence.Enumerated; | |
| import life.mosu.mosuserver.global.filter.TimePenalty; | |
| import lombok.Getter; | |
| import lombok.RequiredArgsConstructor; | |
| @Entity | |
| @Table(name = "blocked_history") | |
| @Getter | |
| @RequiredArgsConstructor | |
| public class BlockedHistory { | |
| @Id | |
| private String ip; | |
| @Enumerated(EnumType.STRING) | |
| private TimePenalty penaltyLevel; | |
| private long blockedAt; | |
| } |
🤖 Prompt for AI Agents
In src/main/java/life/mosu/mosuserver/global/filter/dto/BlockedHistory.java
lines 9 to 16, the class is missing JPA entity annotations required for database
persistence. To fix this, annotate the class with @Entity and specify a table
name with @Table if needed. Also, add an @Id field with appropriate generation
strategy to uniquely identify each record. This will enable JPA to manage the
persistence of BlockedHistory instances properly.
| public class RequestCounter { | ||
| int count = 0; | ||
|
|
||
| public void increment() { | ||
| count++; | ||
| } | ||
| } |
There was a problem hiding this comment.
Critical: Thread safety issues in concurrent rate limiting.
This class has several issues that will cause race conditions in a concurrent environment:
- Field visibility: The
countfield should beprivatefor proper encapsulation - Race condition: The increment operation is not thread-safe, causing lost increments when multiple requests from the same IP arrive simultaneously
- Data integrity: Inaccurate counts will lead to incorrect rate limiting decisions
Apply this diff to fix the thread safety issues:
-import lombok.Getter;
+import lombok.Getter;
+import java.util.concurrent.atomic.AtomicInteger;
@Getter
public class RequestCounter {
- int count = 0;
+ private final AtomicInteger count = new AtomicInteger(0);
- public void increment() {
- count++;
- }
+ public void increment() {
+ count.incrementAndGet();
+ }
+
+ public int getCount() {
+ return count.get();
+ }
}📝 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.
| public class RequestCounter { | |
| int count = 0; | |
| public void increment() { | |
| count++; | |
| } | |
| } | |
| import lombok.Getter; | |
| import java.util.concurrent.atomic.AtomicInteger; | |
| @Getter | |
| public class RequestCounter { | |
| private final AtomicInteger count = new AtomicInteger(0); | |
| public void increment() { | |
| count.incrementAndGet(); | |
| } | |
| public int getCount() { | |
| return count.get(); | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/main/java/life/mosu/mosuserver/global/filter/dto/RequestCounter.java
between lines 6 and 12, make the count field private to ensure encapsulation and
replace the int count with an AtomicInteger to handle concurrent increments
safely. Modify the increment method to use AtomicInteger's thread-safe
incrementAndGet() or incrementAndGet method to avoid race conditions and
maintain accurate counts under concurrent access.
Critical: Thread safety issue in concurrent environment.
The RequestCounter class will be used in a web filter where multiple threads can access the same counter simultaneously. The current implementation has a race condition:
- The
count++operation is not atomic - Multiple threads can read and increment the same value simultaneously
- This will result in lost increments and inaccurate request counting
Replace with thread-safe implementation:
-import lombok.Getter;
+import lombok.Getter;
+import java.util.concurrent.atomic.AtomicInteger;
@Getter
public class RequestCounter {
- int count = 0;
+ private final AtomicInteger count = new AtomicInteger(0);
- public void increment() {
- count++;
- }
+ public void increment() {
+ count.incrementAndGet();
+ }
+
+ public int getCount() {
+ return count.get();
+ }📝 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.
| public class RequestCounter { | |
| int count = 0; | |
| public void increment() { | |
| count++; | |
| } | |
| } | |
| import lombok.Getter; | |
| import java.util.concurrent.atomic.AtomicInteger; | |
| @Getter | |
| public class RequestCounter { | |
| private final AtomicInteger count = new AtomicInteger(0); | |
| public void increment() { | |
| count.incrementAndGet(); | |
| } | |
| public int getCount() { | |
| return count.get(); | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/main/java/life/mosu/mosuserver/global/filter/dto/RequestCounter.java
lines 6 to 12, the current increment method is not thread-safe because count++
is not atomic, causing race conditions in concurrent access. To fix this,
replace the int count with an AtomicInteger and update the increment method to
use AtomicInteger's thread-safe incrementAndGet or increment method to ensure
accurate counting in a multithreaded environment.
| this.blockedHistoryCache = Caffeine.newBuilder() | ||
| .expireAfterWrite(1, TimeUnit.DAYS) | ||
| .recordStats() | ||
| .build(); |
There was a problem hiding this comment.
Missing database persistence for blocked users
The PR objectives mention "하루 차단된 사용자들 DB로 영속화" (persist daily blocked users to DB), but the implementation only uses in-memory caching. The blockedHistoryCache will lose all data on application restart.
Consider adding database persistence for the blocked history to meet the PR requirements.
Would you like me to help implement the database persistence layer for blocked users?
🤖 Prompt for AI Agents
In src/main/java/life/mosu/mosuserver/global/filter/IpRateLimitingFilter.java
around lines 42 to 45, the blockedHistoryCache is currently implemented as an
in-memory cache without database persistence, which causes loss of data on
application restart. To fix this, implement a persistence layer that saves
blocked user data to the database and modify the filter to read from and write
to this database in addition to or instead of the in-memory cache, ensuring
blocked user history is retained across restarts as per the PR objectives.
| this.blockedIpCache = Caffeine.newBuilder() | ||
| .expireAfter(new Expiry<String, BlockedIp>() { | ||
| @Override | ||
| public long expireAfterCreate(String key, BlockedIp value, long currentTime) { | ||
| return value.getTtl().toNanos(); | ||
| } | ||
|
|
||
| @Override | ||
| public long expireAfterUpdate(String key, BlockedIp value, long currentTime, long currentDuration) { | ||
| return currentDuration; | ||
| } | ||
|
|
||
| @Override | ||
| public long expireAfterRead(String key, BlockedIp value, long currentTime, long currentDuration) { | ||
| return currentDuration; | ||
| } | ||
| }) | ||
| .build(key -> null); | ||
| } |
There was a problem hiding this comment.
Critical: LoadingCache with null loader will cause NullPointerException
The blockedIpCache is defined as a LoadingCache but the loader function returns null (line 64). This will cause a NullPointerException when get() is called on a missing key. Since you're only using getIfPresent(), put(), and invalidate(), use a regular Cache instead.
Apply this fix:
- private final LoadingCache<String, BlockedIp> blockedIpCache;
+ private final Cache<String, BlockedIp> blockedIpCache;
...
- this.blockedIpCache = Caffeine.newBuilder()
+ this.blockedIpCache = Caffeine.newBuilder()
.expireAfter(new Expiry<String, BlockedIp>() {
@Override
public long expireAfterCreate(String key, BlockedIp value, long currentTime) {
return value.getTtl().toNanos();
}
@Override
public long expireAfterUpdate(String key, BlockedIp value, long currentTime, long currentDuration) {
- return currentDuration;
+ return value.getTtl().toNanos();
}
@Override
public long expireAfterRead(String key, BlockedIp value, long currentTime, long currentDuration) {
return currentDuration;
}
})
- .build(key -> null);
+ .build();🤖 Prompt for AI Agents
In src/main/java/life/mosu/mosuserver/global/filter/IpRateLimitingFilter.java
lines 47 to 65, the blockedIpCache is defined as a LoadingCache with a loader
that returns null, causing a NullPointerException on get() calls. To fix this,
change the cache type to a regular Cache instead of LoadingCache and remove the
loader function, since only getIfPresent(), put(), and invalidate() are used.
| if (!ipRateLimitingProperties.isEnabled()) { | ||
| log.info("IpRateLimitingFilter disabled"); | ||
| filterChain.doFilter(request, response); | ||
| return; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reduce log level to avoid spam
Logging "IpRateLimitingFilter disabled" at INFO level on every request will spam the logs when the filter is disabled.
if (!ipRateLimitingProperties.isEnabled()) {
- log.info("IpRateLimitingFilter disabled");
+ log.trace("IpRateLimitingFilter disabled");
filterChain.doFilter(request, response);
return;
}📝 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.
| if (!ipRateLimitingProperties.isEnabled()) { | |
| log.info("IpRateLimitingFilter disabled"); | |
| filterChain.doFilter(request, response); | |
| return; | |
| } | |
| if (!ipRateLimitingProperties.isEnabled()) { | |
| log.trace("IpRateLimitingFilter disabled"); | |
| filterChain.doFilter(request, response); | |
| return; | |
| } |
🤖 Prompt for AI Agents
In src/main/java/life/mosu/mosuserver/global/filter/IpRateLimitingFilter.java
around lines 72 to 76, the log message "IpRateLimitingFilter disabled" is
currently at INFO level, which can spam logs when the filter is disabled. Change
the log level from INFO to DEBUG or TRACE to reduce log noise while still
allowing the message to be visible during detailed debugging sessions.
| private void handleBlockedIp(String ip) { | ||
| BlockedHistory history = blockedHistoryCache.get(ip, k -> new BlockedHistory(ip)); | ||
| TimePenalty nextPenaltyLevel = history.getPenaltyLevel().nextLevel(); | ||
| history.updateHistory(nextPenaltyLevel); | ||
|
|
||
| private static class RequestCounter { | ||
| blockedIpCache.invalidate(ip); | ||
| blockedIpCache.put(ip, new BlockedIp(nextPenaltyLevel)); | ||
| log.warn("IP 차단: {}, 차단 레벨: {})", ip, nextPenaltyLevel); | ||
|
|
||
| int count = 0; | ||
| } |
There was a problem hiding this comment.
Fix typo and simplify cache operation
- There's a mismatched closing parenthesis in the log message
- The
invalidate()call beforeput()is unnecessary asput()will overwrite any existing entry
private void handleBlockedIp(String ip) {
BlockedHistory history = blockedHistoryCache.get(ip, k -> new BlockedHistory(ip));
TimePenalty nextPenaltyLevel = history.getPenaltyLevel().nextLevel();
history.updateHistory(nextPenaltyLevel);
- blockedIpCache.invalidate(ip);
blockedIpCache.put(ip, new BlockedIp(nextPenaltyLevel));
- log.warn("IP 차단: {}, 차단 레벨: {})", ip, nextPenaltyLevel);
+ log.warn("IP 차단: {}, 차단 레벨: {}", ip, nextPenaltyLevel);
}📝 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 void handleBlockedIp(String ip) { | |
| BlockedHistory history = blockedHistoryCache.get(ip, k -> new BlockedHistory(ip)); | |
| TimePenalty nextPenaltyLevel = history.getPenaltyLevel().nextLevel(); | |
| history.updateHistory(nextPenaltyLevel); | |
| private static class RequestCounter { | |
| blockedIpCache.invalidate(ip); | |
| blockedIpCache.put(ip, new BlockedIp(nextPenaltyLevel)); | |
| log.warn("IP 차단: {}, 차단 레벨: {})", ip, nextPenaltyLevel); | |
| int count = 0; | |
| } | |
| private void handleBlockedIp(String ip) { | |
| BlockedHistory history = blockedHistoryCache.get(ip, k -> new BlockedHistory(ip)); | |
| TimePenalty nextPenaltyLevel = history.getPenaltyLevel().nextLevel(); | |
| history.updateHistory(nextPenaltyLevel); | |
| blockedIpCache.put(ip, new BlockedIp(nextPenaltyLevel)); | |
| log.warn("IP 차단: {}, 차단 레벨: {}", ip, nextPenaltyLevel); | |
| } |
🤖 Prompt for AI Agents
In src/main/java/life/mosu/mosuserver/global/filter/IpRateLimitingFilter.java
around lines 103 to 112, fix the typo in the log.warn message by removing the
extra closing parenthesis, and simplify the cache update by removing the
unnecessary blockedIpCache.invalidate(ip) call before blockedIpCache.put(ip, new
BlockedIp(nextPenaltyLevel)) since put() already overwrites existing entries.
| private void isAlreadyBlocked(String requestedIp) { | ||
| if(blockedIpCache.getIfPresent(requestedIp) != null){ | ||
| log.warn("이미 차단된 IP: {}", requestedIp); | ||
| throw new CustomRuntimeException(ErrorCode.TOO_MANY_REQUESTS); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Rename method to follow naming conventions
The method name isAlreadyBlocked suggests it returns a boolean, but it's void and throws an exception. This violates the principle of least surprise.
- private void isAlreadyBlocked(String requestedIp) {
+ private void checkIfBlocked(String requestedIp) {
if(blockedIpCache.getIfPresent(requestedIp) != null){
log.warn("이미 차단된 IP: {}", requestedIp);
throw new CustomRuntimeException(ErrorCode.TOO_MANY_REQUESTS);
}
}Also update the call site:
- isAlreadyBlocked(ip);
+ checkIfBlocked(ip);📝 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 void isAlreadyBlocked(String requestedIp) { | |
| if(blockedIpCache.getIfPresent(requestedIp) != null){ | |
| log.warn("이미 차단된 IP: {}", requestedIp); | |
| throw new CustomRuntimeException(ErrorCode.TOO_MANY_REQUESTS); | |
| } | |
| } | |
| // Rename the method to reflect that it performs a check (and may throw), not return a boolean | |
| private void checkIfBlocked(String requestedIp) { | |
| if (blockedIpCache.getIfPresent(requestedIp) != null) { | |
| log.warn("이미 차단된 IP: {}", requestedIp); | |
| throw new CustomRuntimeException(ErrorCode.TOO_MANY_REQUESTS); | |
| } | |
| } |
| private void isAlreadyBlocked(String requestedIp) { | |
| if(blockedIpCache.getIfPresent(requestedIp) != null){ | |
| log.warn("이미 차단된 IP: {}", requestedIp); | |
| throw new CustomRuntimeException(ErrorCode.TOO_MANY_REQUESTS); | |
| } | |
| } | |
| // Before: isAlreadyBlocked(ip); | |
| checkIfBlocked(ip); |
🤖 Prompt for AI Agents
In src/main/java/life/mosu/mosuserver/global/filter/IpRateLimitingFilter.java
around lines 114 to 119, rename the method isAlreadyBlocked to a name that does
not imply a boolean return, such as checkIfBlocked or validateBlockedIp, to
better reflect that it throws an exception instead of returning a value. Also
update all call sites of this method accordingly to use the new method name.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
src/main/java/life/mosu/mosuserver/domain/caffeine/repository/BlockedIpHistoryLogJpaJpaRepositoryImpl.java (1)
17-29: Well-implemented batch operation with minor enum handling suggestion.The batch implementation is solid with proper parameterization and reasonable batch size. Consider using
.name()instead ofString.valueOf()for enum conversion to be more explicit.Consider this minor improvement for enum handling:
- ps.setString(2, String.valueOf(entity.getPenaltyLevel())); + ps.setString(2, entity.getPenaltyLevel().name());src/main/java/life/mosu/mosuserver/application/caffeine/BlockedIpBatchAchiever.java (1)
21-21: Remove unused constant.The
DURATION_HOURS_STANDARDconstant is defined but never used in the class.Remove the unused constant:
- private final static Duration DURATION_HOURS_STANDARD = Duration.ofHours(1); private final static int BATCH_SIZE = 500;src/main/java/life/mosu/mosuserver/global/config/CaffeineCacheConfig.java (1)
34-54: Consider design improvements for the blocked IP cache.The custom expiry logic correctly implements dynamic TTLs based on penalty levels. However, consider these improvements:
LoadingCache with null loader: The cache loader returns null, which defeats the purpose of using
LoadingCache. Consider using a regularCacheinstead unless automatic loading is needed.Missing statistics recording: Unlike other caches, this one doesn't record statistics. Consider adding
.recordStats()for consistency and monitoring.Code readability: The anonymous
Expiryclass could be extracted to improve readability.Apply this diff to address the consistency and design issues:
@Bean - public LoadingCache<String, BlockedIp> blockedIpCache() { + public Cache<String, BlockedIp> blockedIpCache() { return Caffeine.newBuilder() .expireAfter(new Expiry<String, BlockedIp>() { @Override public long expireAfterCreate(String key, BlockedIp value, long currentTime) { return value.getTtl().toNanos(); } @Override public long expireAfterUpdate(String key, BlockedIp value, long currentTime, long currentDuration) { return currentDuration; } @Override public long expireAfterRead(String key, BlockedIp value, long currentTime, long currentDuration) { return currentDuration; } }) - .build(key -> null); + .recordStats() + .build(); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
src/main/java/life/mosu/mosuserver/application/caffeine/BlockedIpBatchAchiever.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/BlockedIpHistory.java(1 hunks)src/main/java/life/mosu/mosuserver/domain/caffeine/dto/RequestCounter.java(1 hunks)src/main/java/life/mosu/mosuserver/domain/caffeine/entity/BlockedIpHistoryLogJpaEntity.java(1 hunks)src/main/java/life/mosu/mosuserver/domain/caffeine/repository/BlockedIpHistoryLogJpaJpaRepository.java(1 hunks)src/main/java/life/mosu/mosuserver/domain/caffeine/repository/BlockedIpHistoryLogJpaJpaRepositoryImpl.java(1 hunks)src/main/java/life/mosu/mosuserver/domain/caffeine/repository/BlockedIpHistoryLogJpaRepositoryCustom.java(1 hunks)src/main/java/life/mosu/mosuserver/global/config/CaffeineCacheConfig.java(1 hunks)src/main/java/life/mosu/mosuserver/global/filter/IpRateLimitingFilter.java(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- src/main/java/life/mosu/mosuserver/domain/caffeine/repository/BlockedIpHistoryLogJpaJpaRepository.java
- src/main/java/life/mosu/mosuserver/domain/caffeine/entity/BlockedIpHistoryLogJpaEntity.java
- src/main/java/life/mosu/mosuserver/domain/caffeine/dto/BlockedIp.java
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/life/mosu/mosuserver/global/filter/IpRateLimitingFilter.java
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/main/java/life/mosu/mosuserver/domain/caffeine/dto/RequestCounter.java (1)
src/main/java/life/mosu/mosuserver/domain/caffeine/dto/BlockedIp.java (1)
Getter(7-20)
src/main/java/life/mosu/mosuserver/application/caffeine/BlockedIpBatchAchiever.java (1)
src/main/java/life/mosu/mosuserver/global/filter/IpRateLimitingFilter.java (1)
Slf4j(24-88)
🔇 Additional comments (7)
src/main/java/life/mosu/mosuserver/domain/caffeine/repository/BlockedIpHistoryLogJpaRepositoryCustom.java (1)
6-8: Clean interface design for batch operations.The interface is well-designed with a clear method signature for batch saving operations. The method name
saveAllUsingBatchclearly indicates its purpose.src/main/java/life/mosu/mosuserver/domain/caffeine/dto/BlockedIpHistory.java (1)
20-23: LGTM! Well-designed state update method.The
updateHistorymethod appropriately updates both the penalty level and timestamp, maintaining consistency in the blocked IP history state.src/main/java/life/mosu/mosuserver/application/caffeine/BlockedIpBatchAchiever.java (2)
28-53: Excellent batch processing implementation with proper error handling.The archive method is well-designed with:
- Correct filtering for LEVEL_5 penalty level
- Appropriate batch size processing
- Proper error handling that continues processing other batches on failure
- Good logging for debugging and monitoring
60-66: Clean conversion method from domain to entity.The helper method properly converts domain objects to JPA entities with all required fields.
src/main/java/life/mosu/mosuserver/global/config/CaffeineCacheConfig.java (3)
1-13: LGTM!Package declaration and imports are well-organized and all imports are used in the configuration.
17-23: LGTM!The request counts cache is properly configured with appropriate expiration policy and statistics recording for IP rate limiting.
25-31: LGTM!The blocked history cache correctly implements the 1-day persistence requirement with appropriate expiration policy and statistics recording.
| @Slf4j | ||
| @Component | ||
| @RequiredArgsConstructor | ||
| public class BlockedIpBatchAchiever implements DomainArchiver { |
There was a problem hiding this comment.
Fix class name typo: 'Achiever' should be 'Archiver'.
The class name has a typo. It should be BlockedIpBatchArchiver instead of BlockedIpBatchAchiever to properly reflect its archiving functionality.
Rename the class:
-public class BlockedIpBatchAchiever implements DomainArchiver {
+public class BlockedIpBatchArchiver implements DomainArchiver {📝 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.
| public class BlockedIpBatchAchiever implements DomainArchiver { | |
| public class BlockedIpBatchArchiver implements DomainArchiver { |
🤖 Prompt for AI Agents
In
src/main/java/life/mosu/mosuserver/application/caffeine/BlockedIpBatchAchiever.java
at line 19, rename the class from BlockedIpBatchAchiever to
BlockedIpBatchArchiver to correct the typo and accurately represent its
archiving role. Also, update the filename to BlockedIpBatchArchiver.java to
match the class name.
| import jakarta.persistence.EnumType; | ||
| import jakarta.persistence.Enumerated; |
There was a problem hiding this comment.
Remove JPA annotations from DTO class.
JPA annotations like @Enumerated should not be present in DTO classes. These annotations belong in JPA entity classes, not domain DTOs.
Apply this diff to remove the inappropriate JPA imports:
-import jakarta.persistence.EnumType;
-import jakarta.persistence.Enumerated;
import java.time.LocalDateTime;📝 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.
| import jakarta.persistence.EnumType; | |
| import jakarta.persistence.Enumerated; | |
| import java.time.LocalDateTime; |
🤖 Prompt for AI Agents
In src/main/java/life/mosu/mosuserver/domain/caffeine/dto/BlockedIpHistory.java
around lines 3 to 4, remove the JPA imports jakarta.persistence.EnumType and
jakarta.persistence.Enumerated as JPA annotations should not be used in DTO
classes. These annotations belong only in JPA entity classes, so delete these
import statements to keep the DTO clean and free of persistence-specific
annotations.
| @Getter | ||
| @RequiredArgsConstructor | ||
| public class BlockedIpHistory { | ||
| private String ip; | ||
|
|
||
| @Enumerated(EnumType.STRING) | ||
| private TimePenalty penaltyLevel; | ||
|
|
||
| private LocalDateTime blockedAt; |
There was a problem hiding this comment.
Remove JPA annotation and improve field design.
The @Enumerated annotation is inappropriate for a DTO class and should be removed. Consider making the ip field final since it shouldn't change after construction.
Apply this diff:
@Getter
@RequiredArgsConstructor
public class BlockedIpHistory {
- private String ip;
+ private final String ip;
- @Enumerated(EnumType.STRING)
private TimePenalty penaltyLevel;
private LocalDateTime blockedAt;📝 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.
| @Getter | |
| @RequiredArgsConstructor | |
| public class BlockedIpHistory { | |
| private String ip; | |
| @Enumerated(EnumType.STRING) | |
| private TimePenalty penaltyLevel; | |
| private LocalDateTime blockedAt; | |
| @Getter | |
| @RequiredArgsConstructor | |
| public class BlockedIpHistory { | |
| private final String ip; | |
| private TimePenalty penaltyLevel; | |
| private LocalDateTime blockedAt; | |
| } |
🤖 Prompt for AI Agents
In src/main/java/life/mosu/mosuserver/domain/caffeine/dto/BlockedIpHistory.java
between lines 10 and 18, remove the @Enumerated(EnumType.STRING) annotation from
the penaltyLevel field because it is a DTO and should not have JPA annotations.
Also, make the ip field final to reflect that it should be immutable after
construction.
| public class RequestCounter { | ||
| int count = 0; | ||
|
|
||
| public void increment() { | ||
| count++; | ||
| } | ||
| } |
There was a problem hiding this comment.
Thread safety issue: Use AtomicInteger for concurrent access.
The RequestCounter class is used in a concurrent environment (IP rate limiting filter) but lacks proper thread safety. While the filter uses synchronized(counter) blocks, the count field should be atomic or volatile to ensure visibility across threads.
Apply this diff to fix the thread safety issue:
-import lombok.Getter;
+import lombok.Getter;
+import java.util.concurrent.atomic.AtomicInteger;
@Getter
public class RequestCounter {
- int count = 0;
+ private final AtomicInteger count = new AtomicInteger(0);
public void increment() {
- count++;
+ count.incrementAndGet();
+ }
+
+ public int getCount() {
+ return count.get();
}
}📝 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.
| public class RequestCounter { | |
| int count = 0; | |
| public void increment() { | |
| count++; | |
| } | |
| } | |
| import lombok.Getter; | |
| import java.util.concurrent.atomic.AtomicInteger; | |
| @Getter | |
| public class RequestCounter { | |
| private final AtomicInteger count = new AtomicInteger(0); | |
| public void increment() { | |
| count.incrementAndGet(); | |
| } | |
| public int getCount() { | |
| return count.get(); | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/main/java/life/mosu/mosuserver/domain/caffeine/dto/RequestCounter.java
lines 6 to 12, the count field is not thread-safe for concurrent access. Replace
the int count with an AtomicInteger and update the increment method to use
AtomicInteger's incrementAndGet or getAndIncrement to ensure atomicity and
visibility across threads without relying on external synchronization.
1 participant
✨ 구현한 기능
📢 논의하고 싶은 내용
🎸 기타
Summary by CodeRabbit
New Features
Bug Fixes
Chores