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

[SCRUM-166] 현재 사용자 인증 및 인가에 있어 admin 역할을 추가 #45

Merged
merged 27 commits into from
Sep 2, 2024

Conversation

sebbbin
Copy link
Member

@sebbbin sebbbin commented Aug 28, 2024

🍃 Pull Requests

⛳️ 작업한 스크럼

  • SCRUM - #166

👷 작업한 내용

해당 적혀있는 1,2,3업무에 대한 코드 작성을 완료하였습니다.
각 스탭별로 나눠서 커밋했습니다.
admin 개발 업무
-> 테스트 코드를 작성해서 테스트 해보면 좋을까요?

🚨 참고 사항

  • 파일 위치 고민
    AdminInterceptor에 위치를 고민하다가
  1. auth -> interceptor (생성) -> AuthInterceptor,AdminInterceptor 두개 파일 넣기
  2. (현재) auth -> authentication -> AuthInterceptor / user -> authentication -> AdminInterceptor
    현재 2번인데 1번 어떤가요

(수빈) 개인적으로는 AdminInterceptor도 auth 폴더 안에 있는 것이 맞다고 생각합니다. 관련 의견 주시면 반영하겠습니다.

  • AdminInterceptor에서 excludePathPatterns을 추가할게 있을까요? (url 관련 추가를 완료했습니다)

  • 테스트 코드까지 코드의 간단한 부분은 수정하였는데 현재 gradle error가 나는 부분은 건들지 못했습니다 .. ;-;..-> 테스트 코드를 수정하고 현재 다 통과하는 것을 확인하였는데, github actions를 통과하지 못하는 상태인 것 같습니다.


👷 작업한 내용

  • 좋아요/북마크 중복 문제와 관련해서 동시성 관련한 테스트를 작성하고, synchronized 키워드를 활용해 해당 테스트를 통과할 수 있도록 코드를 작성했습니다.

🚨 참고 사항

  • 로컬로나 실제 api 서버로나 확인해보았을 때는 중복 처리가 제대로 작동하고 있었습니다. 만약 문제 발생의 원인이 동시성 관련 문제가 아니라면 다시 문제를 살펴보아야 할 것 같습니다.
  • synchronized 키워드로 문제를 해결하기는 했지만, 후에 서버를 확장해서 분산 서버의 형태가 된다면, 동기화 처리가 제대로 되지 못합니다. 혹시 redis를 활용한 분산락 처리까지 고려하는 것이 좋을지 궁금합니다.
  • 물론 어플리케이션 단에서 좋아요/북마크 중복 처리를 막을 수 있지만 데이터베이스 단에서 트리거를 활용하면 중복 데이터가 생기는 문제를 추가로 방지할 수 있습니다. 데이터베이스에 트리거를 추가하는 것에 대해서는 어떻게 생각하시는지 궁금합니다.
CREATE TRIGGER before_insert_bookmark
BEFORE INSERT ON lyrics.bookmarks
FOR EACH ROW
BEGIN
  IF (NEW.deleted_at IS NULL AND
      EXISTS (SELECT 1 FROM lyrics.bookmarks WHERE user_id = NEW.user_id AND note_id = NEW.note_id AND deleted_at IS NULL)) THEN
    SIGNAL SQLSTATE '45000' SET MESSAGE_TEXT = 'Duplicate entry for active bookmark';
  END IF;
END;

Copy link
Collaborator

@jinkonu jinkonu left a comment

Choose a reason for hiding this comment

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

말씀드린 부분을 깔끔하게 구현하신 것 같습니다!!
고생 많으셨고, 리뷰드린 부분에 대해 참고부탁드려요~~~
화이팅

@@ -28,7 +28,7 @@ public class BookmarkCommandService {
private final UserQueryRepository userQueryRepository;
private final NoteQueryRepository noteQueryRepository;

public Bookmark create(Long noteId, Long userId) {
public synchronized Bookmark create(Long noteId, Long userId) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

간단명쾌한 답이네요!!

Comment on lines +16 to +50
@Component
@RequiredArgsConstructor
public class AdminInterceptor implements HandlerInterceptor {

private final AuthContext authContext;
private final JwtExtractor jwtExtractor;
private final TokenExtractor tokenExtractor;

@Override
public boolean preHandle(HttpServletRequest request, HttpServletResponse response, Object handler) throws Exception {
if (isExcluded((request))) {
return true;
}

setAuthContext(request);
if (Role.ADMIN.equals(authContext.getRole())) {
return true;
}

response.setStatus(HttpServletResponse.SC_FORBIDDEN);
return false;
}
private boolean isExcluded(HttpServletRequest request) {
return request.getRequestURI().matches("/api/v1/artists") &&
request.getMethod().equalsIgnoreCase(HttpMethod.GET.name()) ||
request.getRequestURI().matches("/api/v1/artists/\\d+") &&
request.getMethod().equalsIgnoreCase(HttpMethod.GET.name());
}

private void setAuthContext(HttpServletRequest request) {
String authorization = request.getHeader(HttpHeaders.AUTHORIZATION);
JwtClaim claim = jwtExtractor.parseJwtClaim(tokenExtractor.extractToken(authorization));
authContext.setRole(claim.role());
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

굿!! 잘짜신 것 같아요

Comment on lines +38 to +40
.addPathPatterns("/api/v1/artists/**")
.addPathPatterns("/api/v1/notifications/public")
.excludePathPatterns("/api/v1/artists/search");
Copy link
Collaborator

Choose a reason for hiding this comment

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

이렇게 되는 경우에 /api/v1/artists/search가 인터셉터 범위에서 제외되는 것을 확인했는지 궁금합니다!

Copy link
Contributor

Choose a reason for hiding this comment

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

네 로컬에서 통해 확인해보았습니다! 사진과 같이 아티스트 추가는 막히는 대신에, 아티스트 검색은 제대로 작동합니다~
image
image

AtomicInteger duplicateErrorCount = new AtomicInteger(0);

//when
for (int i = 0; i < threadCount; i++) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

for 반복문은 말그대로 반복해서 코드를 실행하지 않나요?
바로 밑의 execute를 동시에 실행하지 않을 수도 있을 것 같은데 테스트는 정상적으로 통과하는지 궁금합니다...

저는 이 부분이 의심되어서 챗지피티에 문의하니까 아래와 같은 코드를 주더라고요!! 참고 부탁드립니다..!

@Test
void 북마크가_동시다발적으로_저장될_떄도_중복_북마크가_생기지_않아야_한다() {
    //given
    int threadCount = 100;
    ExecutorService executorService = Executors.newFixedThreadPool(threadCount);
    CountDownLatch readyLatch = new CountDownLatch(threadCount);
    CountDownLatch startLatch = new CountDownLatch(1);
    CountDownLatch doneLatch = new CountDownLatch(threadCount);
    AtomicInteger duplicateErrorCount = new AtomicInteger(0);

    //when
    for (int i = 0; i < threadCount; i++) {
        executorService.execute(() -> {
            readyLatch.countDown(); // 스레드가 준비 상태임을 알림
            try {
                startLatch.await(); // 모든 스레드가 준비될 때까지 대기
                sut.create(note.getId(), user.getId());
            } catch (BookmarkAlreadyExistsException | NonUniqueResultException e) {
                duplicateErrorCount.getAndIncrement();
            } catch (InterruptedException e) {
                Thread.currentThread().interrupt();
            } finally {
                doneLatch.countDown();
            }
        });
    }

    try {
        readyLatch.await(); // 모든 스레드가 준비될 때까지 대기
        startLatch.countDown(); // 모든 스레드가 동시에 실행 시작
        doneLatch.await(); // 모든 스레드의 실행이 끝날 때까지 대기
    } catch (InterruptedException e) {
        Thread.currentThread().interrupt();
        throw new RuntimeException(e);
    }

    //then
    assertAll(
        () -> assertThat(bookmarkQueryRepository.findByNoteIdAndUserId(note.getId(), user.getId())).isPresent(),
        () -> assertThat(duplicateErrorCount.get()).isEqualTo(threadCount - 1)
    );
}

Copy link
Contributor

Choose a reason for hiding this comment

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

오 제가 참고한 동시성 테스트에서는 주로 for문이나 repeat문을 썼었는데 확실히 한번에 execute하면 동시성 테스트를 더 엄격하게 할 수 있을 것 같아요! 감사합니다~~

Copy link
Collaborator

Choose a reason for hiding this comment

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

👍👍

@elive7 elive7 merged commit f5d7f2b into dev Sep 2, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants