-
Notifications
You must be signed in to change notification settings - Fork 28
[4주차] RateLimit 기능 구현 #73
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: peppone-choi
Are you sure you want to change the base?
Conversation
|
@peppone-choi 님, 안녕하세요. 리뷰 시작하겠습니다. |
| "if current == 1 then\n" + | ||
| " redis.call('expire', KEYS[1], ARGV[1])\n" + | ||
| "end\n" + | ||
| "return current"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LuaScript 가 string 으로 하드코딩 되어 있어서, 재사용성이 떨어질 수 있습니다.
나중에 시간이 되신다면 파일로 분리해서 작성해보셔도 좋을 것 같습니다 👍
| String requestCountKey = "rate_limit:count:" + ipAddress; | ||
|
|
||
| // 2. 현재 요청 수 증가 (Lua 스크립트 사용하여 원자적 연산) | ||
| Long currentCount = incrementCounter(requestCountKey, 60); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MOVIE_REQUEST_MAX_SECONDS 라는 상수로 분리해도 좋을 것 같아요 :)
|
|
||
| private void blockIp(String ipAddress) { | ||
| // IP 차단 정보를 Redis에 저장 | ||
| String blockKey = "ip_block:" + ipAddress; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
key 를 만드는 로직은 여러곳에서 사용되니, 메서드/ util 을 통해서 분리해주셔도 좋을 것 같아요 :)
| if (isBlocked(ipAddress)) { | ||
| LocalDateTime blockExpires = LocalDateTime.now().plusHours(1); | ||
| blockService.block(ipAddress, blockExpires); | ||
| ForbiddenException exception = new ForbiddenException(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
단순히 ip block 이 된 이유가 too_many_requests (429) 일텐데 따로 분리하셔서, ForbiddenException 으로 처리하신 이유가 있으셨나요?
| public void addInterceptors(InterceptorRegistry registry) { | ||
| registry.addInterceptor(ipBlockInterceptor) | ||
| .addPathPatterns("/api/v1/**"); | ||
| registry.addInterceptor(movieRateLimitInterceptor) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
interceptor 를 통해 관심사를 분리해주신 것이 좋았습니다 👍
aop 가 아닌 interceptor 로 구현하신 이유가 있으셨나요?
| } | ||
|
|
||
| private boolean checkRateLimiter(String ipAddress) { | ||
| String rateLimiterKey = "rate_limit:limiter:" + ipAddress; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
같은 시간대 영화 예매를 제한하는 케이스를 대응하려면 screeningId 가 있어야할 것 같아 보입니다.
해당 로직은 ip 기준으로만 block 이 되어 있네요 .
| NotFoundException exception = new NotFoundException(); | ||
| exception.setErrorCode(NotFoundErrorCode.RESOURCE_NOT_FOUND); | ||
| exception.setDetail("해당하는 유저를 찾을 수 없습니다."); | ||
| return exception; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
예외 객체는 보통 한 번 생성되면 내부 상태가 변하지 않도록 설계하는 게 바람직할 것 같습니다.
생성자를 사용하거나, 정적 팩토리 메서드를 사용하면 어떨까요?
|
@peppone-choi 님, 고생하셨습니다 👍 좋았던 점
아쉬웠던 점
|
📝작업 내용
🔒해결하려는 문제 혹은 고민이 되었던 부분을 남겨주세요.(문제 수 만큼 복사해서 사용할 것)
🔑해당 문제를 어떻게 해결했나요? 그리고 왜 그렇게 생각했는지 이유를 남겨주세요.(자세하게 남겨주실 수록 좋습니다.)
💬리뷰 요구사항(선택)