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.

Sign up for GitHub

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: 현재 로그인한 멤버 조회하는 유틸리티 구현 #41

Merged
merged 2 commits into from
Feb 11, 2024
Merged

feat: 현재 로그인한 멤버 조회하는 유틸리티 구현 #41

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9fe1a7b
feat: 멤버 유틸리티 구현
uwoobeat Feb 10, 2024
92a9b5b
refactor: 예외 케이스 분리 및 불필요한 메서드 제거
uwoobeat Feb 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions src/main/java/com/gdschongik/gdsc/global/exception/ErrorCode.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,16 +9,18 @@
public enum ErrorCode {
INTERNAL_SERVER_ERROR(HttpStatus.INTERNAL_SERVER_ERROR, "서버 에러입니다."),

// Jwt
// Auth
INVALID_JWT_TOKEN(HttpStatus.UNAUTHORIZED, "유효하지 않은 JWT 토큰입니다."),
EXPIRED_JWT_TOKEN(HttpStatus.UNAUTHORIZED, "만료된 JWT 토큰입니다."),
AUTH_NOT_FOUND(HttpStatus.INTERNAL_SERVER_ERROR, "시큐리티 인증 정보를 찾을 수 없습니다."),

// Parameter
INVALID_QUERY_PARAMETER(HttpStatus.BAD_REQUEST, "잘못된 쿼리 파라미터입니다."),

// Member
MEMBER_NOT_FOUND(HttpStatus.NOT_FOUND, "존재하지 않는 회원입니다."),
MEMBER_DELETED(HttpStatus.CONFLICT, "탈퇴한 회원입니다.");
MEMBER_DELETED(HttpStatus.CONFLICT, "탈퇴한 회원입니다."),
;

private final HttpStatus status;
private final String message;
Expand Down
36 changes: 36 additions & 0 deletions src/main/java/com/gdschongik/gdsc/global/util/MemberUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
package com.gdschongik.gdsc.global.util;

import com.gdschongik.gdsc.domain.member.dao.MemberRepository;
import com.gdschongik.gdsc.domain.member.domain.Member;
import com.gdschongik.gdsc.global.exception.CustomException;
import com.gdschongik.gdsc.global.exception.ErrorCode;
import lombok.RequiredArgsConstructor;
import org.springframework.security.core.Authentication;
import org.springframework.security.core.context.SecurityContextHolder;
import org.springframework.stereotype.Component;

@Component
@RequiredArgsConstructor
public class MemberUtil {

private final MemberRepository memberRepository;

private Long getCurrentMemberIdFromSecurityContext() {
Authentication authentication = SecurityContextHolder.getContext().getAuthentication();
try {
return Long.parseLong(authentication.getName());
Copy link
Member

Choose a reason for hiding this comment

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

아래 한 줄에서 발생할 수 있는 예외들은 어떤 것들이 있을까요?

return Long.parseLong(authentication.getName());

따로 검증하지 않고, Exception으로 한번에 처리하는 이유는 예상하지 못한 예외에 대비하기 위함일까요?
아니면, 여러 예외 상황을 한번에 처리하기 위함일까요?

제가 시큐리티를 잘 몰라서 그런지 따로 검증하지 않으니,
저 한줄에서 어떤 예외가 발생할 수 있는지 궁금한데, 추측하기 어렵네요.
(예외도 Exception으로 되어 있어서)

제가 생각나는 경우는 아래 두가지 정도인데, 재현님이 생각하는 경우는 다르거나 더 있을 것 같아요
읽는 사람도 어떤 예외가 발생할 수도 있는지 예상해볼 수 있도록,
먼저 검증해보는 것은 어떤지 의견 여쭈어 봅니다.

  1. Authentication이 null인 경우
  2. Parse의 실패 (언제 발생할지는 모르겠음)

예시

Authentication authentication = SecurityContextHolder.getContext().getAuthentication();
validateAuthenticationNotNull(authentication); // 여기서 null임을 검증함으로써, authentication 객체가 null일 수도 있음을 명시

try {
    return Long.parseLong(authentication.getName());
} catch(NumberFormatException exception) {
    // 예외 발생
}

Copy link
Member Author

Choose a reason for hiding this comment

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

  1. Authentication이 null인 경우 -> 서비스 레이어에서는 내부적으로 MemberUtil 을 사용하는데, 이 서비스 레이어를 테스트할 때 실수로 SecurityContext에 Authentication 을 set 해주지 않은 경우 NPE가 발생할 수도 있습니다.

  2. 파싱 실패하는 경우 -> AnonymouseAuthentication 이 set 된 경우 getName() 이 memberId 가 아닌 anonymousUser 라는 String을 리턴하기 때문에 파싱에 실패합니다.

말씀하신대로 두 가지 케이스르 분리할 수 있을 것 같습니다. 좋은 의견 감사해요~

} catch (Exception e) {
throw new CustomException(ErrorCode.AUTH_NOT_FOUND);
}
}

public Long getCurrentMemberId() {
Copy link
Member

Choose a reason for hiding this comment

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

취향 차이일 수 있는데,
결국 이 클래스를 볼 때, 어떤 일을 할 수 있는 클래스인지 파악하기 위해
가장 먼저 보게 되는건 아무래도 외부에서 이 클래스에게 행동을 요청하는 public 메서드 들인것 같아요
.
그래서 내부적으로 쓰이는 private 메서드 보다는 외부에서 사용할 수 있는 public 메서드가 위에 있는건 어떨까요?

사실 저희 코드 보면서 항상 속으로만 생각하던 건데 의견 나눠보고 싶어 글 남깁니다!

Copy link
Member Author

Choose a reason for hiding this comment

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

getCurrentMemberId() 와 getCurrentMemberIdFromSecurityContext 를 굳이 분리할 필요가 없을 것 같아 하나로 합쳤습니다. 참고 바랍니다~

return getCurrentMemberIdFromSecurityContext();
}

public Member getCurrentMember() {
return memberRepository
.findById(getCurrentMemberIdFromSecurityContext())
.orElseThrow(() -> new CustomException(ErrorCode.MEMBER_NOT_FOUND));
}
}
Loading