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

[Feat] 차단한 유저에 대한 요청 필터링 #287

Merged
merged 5 commits into from
Apr 16, 2023
Merged

Conversation

heoboseong7
Copy link
Collaborator

관련 Issue

작업 내용

  • insightId의 작성자를 차단한 경우 요청 필터링
  • userId가 차단된 경우 요청 필터링

InsightId 작성자 관련

  • GET /api/v1/insight{insightId}
  • GET /api/v1/insight/author/{insightId}
  • GET /api/v1/comments/insights/{insightId}/count
  • GET /api/v1/comments/insights/{insightId}/preview
  • POST /api/v1/reaction
  • GET /api/v1/insight/{insightId}/challenge-record
  • GET /api/v1/insight/my-page/{userId}

요청 대상 userId 관련

  • GET /api/v1/drawer/{userId}
  • POST /api/v1/user/profile/follow/{userId}
  • GET /api/v1/user/priflie/achieved-title/{userId}
  • GET /api/v1/user/priflie/all-achieved-title/{userId}
  • GET /api/v1/user/profile/followee/{userId}
  • GET /api/v1/user/profile/follower/{userId}

@github-actions
Copy link

테스트통과 🤖

Copy link
Collaborator

@Youhoseong Youhoseong left a comment

Choose a reason for hiding this comment

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

일단 현 상황 코드대로 리뷰는했는데요!

ThreadLocal의 필요성을 잘 모르겠어요ㅎㅎ
캐싱? ThreadLocal을 쓰더라도 어차피 요청 1개당 차단 목록 조회하는건 필수적이지 않나 싶어서요~

생각을 들어보고 시퍼요~

Comment on lines 21 to 28
public BlockFilterAspect(ProfileQueryDomainService profileQueryDomainService, InsightQueryDomainService insightQueryDomainService) {
this.profileQueryDomainService = profileQueryDomainService;
this.insightQueryDomainService = insightQueryDomainService;
}

private final ProfileQueryDomainService profileQueryDomainService;
private final InsightQueryDomainService insightQueryDomainService;
private final ThreadLocal<Set<Long>> blockedUserIdsStore = new ThreadLocal<>();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
public BlockFilterAspect(ProfileQueryDomainService profileQueryDomainService, InsightQueryDomainService insightQueryDomainService) {
this.profileQueryDomainService = profileQueryDomainService;
this.insightQueryDomainService = insightQueryDomainService;
}
private final ProfileQueryDomainService profileQueryDomainService;
private final InsightQueryDomainService insightQueryDomainService;
private final ThreadLocal<Set<Long>> blockedUserIdsStore = new ThreadLocal<>();
private final ProfileQueryDomainService profileQueryDomainService;
private final InsightQueryDomainService insightQueryDomainService;
private final ThreadLocal<Set<Long>> blockedUserIdsStore = new ThreadLocal<>();
public BlockFilterAspect(ProfileQueryDomainService profileQueryDomainService, InsightQueryDomainService insightQueryDomainService) {
this.profileQueryDomainService = profileQueryDomainService;
this.insightQueryDomainService = insightQueryDomainService;
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

blockedUserIdRegistry는 어때요?

Comment on lines +73 to +75
if (blockedUserIds.contains(response)) {
throw new KeeweException(KeeweRtnConsts.ERR453);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
if (blockedUserIds.contains(response)) {
throw new KeeweException(KeeweRtnConsts.ERR453);
}
if (blockedUserIds.contains(response)) {
blockedUserIdsStore.remove();
throw new KeeweException(KeeweRtnConsts.ERR453);
}

Comment on lines +61 to 62
Set<Long> blockedUserIds = getBlockedUserIds();
responses.removeIf(response -> blockedUserIds.contains(response.getUserId()));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
Set<Long> blockedUserIds = getBlockedUserIds();
responses.removeIf(response -> blockedUserIds.contains(response.getUserId()));
Set<Long> blockedUserIds = getBlockedUserIds();
responses.removeIf(response -> blockedUserIds.contains(response.getUserId()));
blockedUserIdsStore.remove();

Comment on lines 66 to 69
private void validateInsightId(Long request) {
Long writerId = insightQueryDomainService.getByIdWithWriter(request).getWriter().getId();
validateUserId(writerId);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

저는 이거 실제로 insightId를 validation한다고 생각하진 않는데 어떠세요?

또 파라미터 이름 request를 좀 더 구체적으로 바꾸면 좋을거같네요

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

네. extract method 써서 만들었는데 코드 자체가 좀 이상하네요;;

@Youhoseong
Copy link
Collaborator

고생하셨어요 👍 케이스가 되게 많네요

@heoboseong7
Copy link
Collaborator Author

일단 현 상황 코드대로 리뷰는했는데요!

ThreadLocal의 필요성을 잘 모르겠어요ㅎㅎ 캐싱? ThreadLocal을 쓰더라도 어차피 요청 1개당 차단 목록 조회하는건 필수적이지 않나 싶어서요~

생각을 들어보고 시퍼요~

다시 보니 여러 번 조회할 일이 없겠네요 ㅎㅎ
blockedUserIds가 여러 번 사용되는 경우 조회를 1번만 하려는 목적이었어요
ThreadLocal 부분은 제거했습니다~

@github-actions
Copy link

테스트통과 🤖

Copy link
Collaborator

@Youhoseong Youhoseong left a comment

Choose a reason for hiding this comment

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

LGTM 👍🏽

@heoboseong7 heoboseong7 merged commit 3e38bee into main Apr 16, 2023
@heoboseong7 heoboseong7 deleted the issue-#285 branch April 16, 2023 12:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feat] 차단한 유저에 대한 요청 필터링
2 participants