-
Notifications
You must be signed in to change notification settings - Fork 1
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: 현재 로그인한 멤버 조회하는 유틸리티 구현 #41
Conversation
private Long getCurrentMemberIdFromSecurityContext() { | ||
Authentication authentication = SecurityContextHolder.getContext().getAuthentication(); | ||
try { | ||
return Long.parseLong(authentication.getName()); |
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.
아래 한 줄에서 발생할 수 있는 예외들은 어떤 것들이 있을까요?
return Long.parseLong(authentication.getName());
따로 검증하지 않고, Exception으로 한번에 처리하는 이유는 예상하지 못한 예외에 대비하기 위함일까요?
아니면, 여러 예외 상황을 한번에 처리하기 위함일까요?
제가 시큐리티를 잘 몰라서 그런지 따로 검증하지 않으니,
저 한줄에서 어떤 예외가 발생할 수 있는지 궁금한데, 추측하기 어렵네요.
(예외도 Exception으로 되어 있어서)
제가 생각나는 경우는 아래 두가지 정도인데, 재현님이 생각하는 경우는 다르거나 더 있을 것 같아요
읽는 사람도 어떤 예외가 발생할 수도 있는지 예상해볼 수 있도록,
먼저 검증해보는 것은 어떤지 의견 여쭈어 봅니다.
- Authentication이 null인 경우
- Parse의 실패 (언제 발생할지는 모르겠음)
예시
Authentication authentication = SecurityContextHolder.getContext().getAuthentication();
validateAuthenticationNotNull(authentication); // 여기서 null임을 검증함으로써, authentication 객체가 null일 수도 있음을 명시
try {
return Long.parseLong(authentication.getName());
} catch(NumberFormatException 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.
-
Authentication
이 null인 경우 -> 서비스 레이어에서는 내부적으로MemberUtil
을 사용하는데, 이 서비스 레이어를 테스트할 때 실수로SecurityContext
에Authentication
을 set 해주지 않은 경우 NPE가 발생할 수도 있습니다. -
파싱 실패하는 경우 ->
AnonymouseAuthentication
이 set 된 경우getName()
이memberId
가 아닌anonymousUser
라는 String을 리턴하기 때문에 파싱에 실패합니다.
말씀하신대로 두 가지 케이스르 분리할 수 있을 것 같습니다. 좋은 의견 감사해요~
} | ||
} | ||
|
||
public Long getCurrentMemberId() { |
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.
취향 차이일 수 있는데,
결국 이 클래스를 볼 때, 어떤 일을 할 수 있는 클래스인지 파악하기 위해
가장 먼저 보게 되는건 아무래도 외부에서 이 클래스에게 행동을 요청하는 public 메서드 들인것 같아요
.
그래서 내부적으로 쓰이는 private 메서드 보다는 외부에서 사용할 수 있는 public 메서드가 위에 있는건 어떨까요?
사실 저희 코드 보면서 항상 속으로만 생각하던 건데 의견 나눠보고 싶어 글 남깁니다!
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.
getCurrentMemberId()
와 getCurrentMemberIdFromSecurityContext
를 굳이 분리할 필요가 없을 것 같아 하나로 합쳤습니다. 참고 바랍니다~
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.
진호님께서 언급해주신 내용만 해결되면 저는 lgtm입니다!
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.
LGTM!
🌱 관련 이슈
📌 작업 내용 및 특이사항
SecurityContext
로부터 읽어오는 유틸리티를 구현했습니다.getCurrentMember
를 통해 엔티티로 조회해와서 사용하고, id 값만 필요한 경우getCurrentMemberId
로 사용하면 됩니다.📝 참고사항
📚 기타