Skip to content
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

[BE] feat: 중복 요청 방지 기능 구현 #781

Merged
merged 29 commits into from
Oct 15, 2024

Conversation

nayonsoso
Copy link
Contributor

@nayonsoso nayonsoso commented Oct 7, 2024


🚀 어떤 기능을 구현했나요 ?

  • 중복 요청 방지 기능을 구현했습니다.

🔥 어떻게 해결했나요 ?

  • 커밋 따라서 쭉쭉 읽으시면 이해가 편하실거예요.
  • 그리고 테스트 코드도 쭉 읽으면 완전 이해 완료하실 수 있습니다💡
  • 요청을 받아서, 중복 로직 방지 로직을 수행하는 곳은 "인터셉터"입니다.
  • 요청 거부 전략으로는 POST 요청에 대해서,
  • ‼️1초 동안 동일한 URI, public IP, User-Agent 의 조합으로 3번 초과 요청이 오면‼️
  • 그 이후부터 429 응답을 내려줍니다.

image
image

⬆️ 위 사진은 Jmeter (반복 요청을 보낼 수 있는 도구) 를 사용해서 1초동안 여러번의 요청을 보내본 결과입니다.

📝 어떤 부분에 집중해서 리뷰해야 할까요?

  • 중복 요청 거부 로직에 대해서, 이해가 어려운 부분들 있다면 바로 질문해주세요



🔮 미래 계획

1) dev-ec2 에 redis 환경 만들어주기

필요한 것은 "작동하는 redis" 와 "java application 과 redis 의 연결 설정" 인데요,
전자는 docker 를 사용해서 이미 갖춰두었고,
이 PR 머지하기 전에 application-dev.yml 에 아래 코드 추가할 예정입니다.

redis:
  host: localhost
  port: 6379

2) prod-ec2-a / prod-ec2-b 에 redis 환경 만들어주기

redis 만을 위한 ec2 를 만들어준 다음, (역시나 도커로 편하게 redis 다운 &. 실행시킬 생각입니다)
application-prod.yml 에 아래 코드를 추가해줍니다.

redis:
  host: {그 ec2의 private ip}
  port: 6379

image

보안그룹은 이미 만들어져있는 project-cache 를 새로 만들 ec2 에 달아주면 될 것 같습니다


로컬에서 실행하는 방법

Redis 를 다운받고, 실행한다. (Redis 다운받는 법은 인터넷에 많음)
아래 명령어를 입력했을 때, 버전이 나오면 다운로드 된 것이다.

redis-server --version

이제 실행한다.

brew services start redis

⚠️ 한가지 이슈 : RedisTemplate 을 빈으로 등록했기 때문에,
redis 를 실행하지 않은 상태에서 local Application을 실행하면 오류가 뜨며 실행되지 않는다🙀

@nayonsoso nayonsoso self-assigned this Oct 7, 2024
@nayonsoso nayonsoso changed the title [BE] 중복 요청 방지 기능 구현 [BE] feat: 중복 요청 방지 기능 구현 Oct 7, 2024
Copy link

github-actions bot commented Oct 7, 2024

Test Results

148 tests  +3   145 ✅ +3   4s ⏱️ ±0s
 56 suites +1     3 💤 ±0 
 56 files   +1     0 ❌ ±0 

Results for commit f1a9d38. ± Comparison against base commit 8c3c5ae.

♻️ This comment has been updated with latest results.

@donghoony donghoony added this to the 6차 스프린트: 최종장 milestone Oct 7, 2024
backend/src/main/resources/application.yml Outdated Show resolved Hide resolved
backend/src/main/java/reviewme/config/RedisConfig.java Outdated Show resolved Hide resolved
@@ -50,6 +51,11 @@ public ProblemDetail handleDataConsistencyException(DataInconsistencyException e
return ProblemDetail.forStatusAndDetail(HttpStatus.INTERNAL_SERVER_ERROR, ex.getErrorMessage());
}

@ExceptionHandler(TooManyDuplicateRequestException.class)
Copy link
Contributor

@donghoony donghoony Oct 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(네이밍) Duplicate는 없어도 될 듯합니다! TooManyRequestException으로도 충분하다고 생각해요

Copy link
Contributor Author

@nayonsoso nayonsoso Oct 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TooManyRequestException 은 '요청 자체가 너무 많아서' 서버가 처리할 수 없다는 뉘앙스인 것 같아요,
이 예외가 발생하는 상황은 그런게 아니니까 구분하기 위해서라도 Duplicate를 붙이고 싶어요!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

다른 단어를 선택하는 건 어떨까요? duplicate는 정말 완벽히 같은 복제본의 느낌인데, 본문이 다르더라도 많은 요청이 막힐 수 있는 부분이라서요. 🤔

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ManyDuplicate의 의미가 중복돼 클래스 이름이 어색해서 남긴 리뷰였습니다~

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

본문이 다르더라도 많은 요청이 막힐 수 있는 부분이라서요.
Many와 Duplicate의 의미가 중복돼 클래스 이름이 어색해서 남긴 리뷰였습니다~

아하 그럴수도 있겠네요..! 이해됩니다 ㅎㅎ

TooManyRequestException로 하겠습니다~
찾아보니 429 자체에 "특정 사용자"라는 뉘앙스가 있기도 하네요

image

Copy link
Contributor

@skylar1220 skylar1220 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

산따최!!
어려워보이는데 구현해냈고나🚀🚀

Comment on lines 31 to 39
Object value = redisTemplate.opsForValue().get(key);
if (value == null) {
redisTemplate.opsForValue().set(key, 1, DURATION);
return true;
}

if (!(value instanceof Integer)) {
throw new RequestFrequencyNonNumericException(value);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

찾아보니 있으면 기존 value에 1 증가 시켜주는 일을 해주는 redisTemplate.opsForValue().increment(key) 라는 메서드가 있는 것 같은데 직접 구현한 이유가 있을까요?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이거 좋네요 👍🏻

Copy link
Contributor Author

@nayonsoso nayonsoso Oct 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이렇게 한 배경은...

redisTemplate.opsForValue().increment(key) 는 value 에 해당하는 값이 numeric 이 아니면 예외가 발생해요. 그런데 저는 RedisTemplate 의 value 는 확장성을 위해서 Integer 나 Long 이 아니라 Object 여야 한다고 생각해요!

였지만 커비의 제안으로 RedisTemplate<String, Long> 로 형식을 바꾸게 됨으로써 저 메서드를 사용하도록 바꿨습니다🫡

private final RedisTemplate<String, Object> redisTemplate;

@Override
public boolean preHandle(HttpServletRequest request, HttpServletResponse response, Object handler) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

value로는 무조건 숫자가 오는 것 같은데 int 나 long이 아닌 object를 사용한 이유가 궁금해요~

Copy link
Contributor

@Kimprodp Kimprodp Oct 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@skylar1220 object는 override 메서드라 못바꾸지 않나요?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

앗 설명이 부족했네요
RedisTemplate<String, Object> redisTemplate; 부분을 말합니다!

Copy link
Contributor Author

@nayonsoso nayonsoso Oct 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

레디스의 '사용 확장성'을 생각했어요!
레디스에 반드시 Integer 이나 Long 과 같은 값을 담을게 아니라,
"세션의 정보"와 같은 것들도 담을 수 있게 될 것 같아요.
(무중단 배포로 sticky session 을 사용하지 않게 되면 곧 우리가 해야 할 일이기도 하고요😅)

그런데 지금 숫자로 value 를 한정하면, 나중에 다른 형태의 값을 저장할 때 기존 기능에 영향을 주게 돼요.
그래서 Object 를 저장하게 해준 것입니다!

Copy link
Contributor Author

@nayonsoso nayonsoso Oct 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

추가))

저는 어플리케이션에 하나의 RedisTemplate 을 추가할 수 있을거라 생각했는데, 알고보니 아니었네요🫨 세션같은 정보를 저장할때는 그에 맞는 형식의 RedisTemplate 을 추가하도록 하고, 중복 요청정보를 저장할 때는 RedisTemplate<String, Long> 로 저장하도록 하겠습니다!

그리고 여러개의 RedisTemplate을 사용하더라도 저장을 할 때는 하나의 레디스에 key 와 value 모두 String 으로 직렬화해서 사용해서 상관 없다고 합니다😊 커비가 짚어준 덕분에 더 효율적으로 코드를 작성하게 되었어요!!!! 커비 최고최고😍

@Test
void 특정_POST_요청이_처음이_아니며_최대_빈도보다_클_경우_예외를_발생시킨다() {
// given
int maxFrequency = 3;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

3부터 예외를 반환하니 maxFrequency를 경계값인 2로 하는 건 어떨까요?

Copy link
Contributor Author

@nayonsoso nayonsoso Oct 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

3번째 요청까지는 받고, 4번째 요청부터 리젝합니다!
그래서 maxFrequency = 3인 현채 코드가 경계값이라 생각해요

Comment on lines 31 to 39
Object value = redisTemplate.opsForValue().get(key);
if (value == null) {
redisTemplate.opsForValue().set(key, 1, DURATION);
return true;
}

if (!(value instanceof Integer)) {
throw new RequestFrequencyNonNumericException(value);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이거 좋네요 👍🏻

Copy link
Contributor

@Kimprodp Kimprodp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

산초 커밋과 테스트를 읽으니 깔끔하게 이해가 잘 되네요👍👍
고생했습니다 코멘트만 확인 부탁해요~

private final RedisTemplate<String, Object> redisTemplate;

@Override
public boolean preHandle(HttpServletRequest request, HttpServletResponse response, Object handler) {
Copy link
Contributor

@Kimprodp Kimprodp Oct 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@skylar1220 object는 override 메서드라 못바꾸지 않나요?

backend/src/main/java/reviewme/config/WebConfig.java Outdated Show resolved Hide resolved
Comment on lines 41 to 46

# todo: 이게 맞는걸까..?
redis:
host: localhost
port: 6379
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이 설정은 test/application.yml 에 들어갑니다.
딱히 의미 있는 설정은 아니지만,
스프링 부트 테스트를 할 때 "빈등록" 성공을 위해서 작성해주었습니다.
(어플리케이션에서 yml 로부터 값을 읽어와서 빈을 등록하니깐요~)
조금 찝찝하긴 한데 이게 최선이라 생각해서 이렇게 해놨어요😅

Copy link
Contributor

@donghoony donghoony Oct 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(반영하라는 건 아니예요~) 테스트용 configuration에 Properties를 새로 만든 Template을 등록할 수도 있습니다

Copy link
Contributor

@Kimprodp Kimprodp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

신초 고생했읍니다. 빨리 적용하고 중복요청 보내보고 싶어요 👍👍

Copy link
Contributor

@donghoony donghoony left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

따닥 미워

Copy link
Contributor

@skylar1220 skylar1220 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

어렵다.. 산초 최고🚀🚀🚀🚀

- RedisTemplate<String, Object> -> RedisTemplate<String, Long>
- 명시적으로 null 인 경우 1로 초기화했던 이전 코드와 달리, setIfAbsent를 통해서 값이 초기화하는 지금은 '1로 초기화되었는지' 검증하기가 어렵다.
- increment 가 증가한 결과를 바로 반환한다. 이를 사용하도록 수정했다.
- RedisTemplate과 ValueOperations를 모킹한다.
- 구체적인 인자를 지정하지 않은 stub 문을 수정한다.
- 테드 의견 반영
@nayonsoso nayonsoso force-pushed the be/feature/duplicated-request-refuse branch from 0b48f39 to cf7e628 Compare October 15, 2024 07:27
@nayonsoso nayonsoso merged commit 2bd0197 into develop Oct 15, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

[BE] 따닥 방지 및 악의적인 중복 요청 방지
4 participants