Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
| refreshTokenCommandService.deleteByTokenId(tokenId); | ||
|
|
||
| // 유저 최근 접속일 업데이트 | ||
| Events.publishEvent(new UserLoginEvent(userId)); |
There was a problem hiding this comment.
토큰 재발급과 유저의 최근 접속일 업데이트가 어떤 연관이 있을까요?
(유저가 이미 로그인되어도 토큰 재발급은 이루어질 수 있음)
oauth2 로그인에 성공했을 때 유저 최근 접속일을 자동으로 업데이트할 수 없을까요?
There was a problem hiding this comment.
oauth2 로그인에 성공했을 때 업데이트 - OAuth2AuthenticationSuccessHandler에서 처리
최근 접속일이 로그인이 아닌 토큰 재발급시에 이루어져야 한다고 생각했습니다
한 번 로그인 이후 재로그인 하는 일이 많이 없기 때문입니다
처음에는 그래서 토큰 재발급 시 + 로그인 시 최근 접속일이 업데이트 되도록 하였는데, TokenFilter 호출 시 업데이트 되도록 하면서 로그인 시 업데이트 하는 동작을 제거하였습니다
There was a problem hiding this comment.
유저가 어떤 동작을 안하고 프론트에서 그냥 토큰 재발급만 한 경우에도 유저가 최근 접속한 날짜가 변경되어야 하나요?
There was a problem hiding this comment.
말그대로 접속 날짜니까 그래야 한다고 생각했습니다
제거할까욤?!
There was a problem hiding this comment.
유저의 최근활동이랑 유저의 접속 날짜를 따로 저장하는걸까요?
토큰 재발급이 유저가 접속하는거로 생각하시는 이유가 궁금해요
| SecurityContextHolder.getContext().setAuthentication(authentication); | ||
|
|
||
| // 유저 최근 활동일 업데이트 | ||
| Events.publishEvent(new UserLoginEvent(userId)); |
There was a problem hiding this comment.
이거 토큰 필터에서 처리한 이유가 있나요?
success handler 가 마지막으로 로그인 절차를 밟는 곳이라 생각하는데 해당 필터에서 처리한 이유가 궁금해요
There was a problem hiding this comment.
엇 OAuth2AuthenticationSuccessHandler 말씀이신가요...?!
로그인이 아닌 최근 유저 동작 기준으로 업데이트를 하기 위해서 해당 필터에서 처리하였습니다
(유저가 동작 할 때 -> 이 유저가 최근 활동했구나 판단)
There was a problem hiding this comment.
아하 이거 이벤트 이름때문에...
본문에도 써두긴 했는데 이벤트 이름을 역시 Activity 정도로 변경해야 할까요
There was a problem hiding this comment.
사용자가 마지막으로 API 를 호출한 시각을 기록하기 위함이라고 생각하면 될까요?
There was a problem hiding this comment.
로그인뿐 아니라 다른 API를 호출했을 때 갱신되는 값이라면 수정하는게 좋아보여요
| return userRepository.create(user); | ||
| } | ||
|
|
||
| @Transactional |
There was a problem hiding this comment.
요거 클래스부분에 안붙이고 함수마다 붙이는 이유가 있나요~?
There was a problem hiding this comment.
loginUpdateAt 동작이 RequestNew를 사용해서 함수마다 붙이도록 변경했습니다
There was a problem hiding this comment.
updateUserLoginAt 에만 붙여도 되는거 아닌가요?
There was a problem hiding this comment.
기존 클래스에서 전체 클래스 위에 Transactional이 붙여져 있었어서 그대로 가져왔었는데, 필요 없다면 제거하겠습니다~!!
There was a problem hiding this comment.
전체 클래스 위에 있던 Transactional 를 제외하고 메서드마다 붙인 이유가 궁금했습니다!@
그냥 메서드별로 사용하셔도 됩니다~!
| public class UserLoginEvent extends Event{ | ||
| private UUID userId; | ||
|
|
||
| public UserLoginEvent(UUID userId) { | ||
| super(); | ||
| this.userId = userId; | ||
| } | ||
| } |
There was a problem hiding this comment.
이벤트로 처리한 이유가 궁금합니다!!
유저의 마지막 접속 기록을 저장하는데 이벤트로 따로 빼서 처리한 이유가 궁금해요.
There was a problem hiding this comment.
접속 기록이 사용자의 동작마다 업데이트 된다면 다양한 곳에서 발생할 수 있다고 판단하여 Event로 분리하였습니다 (접속 기록 - 사용자 마지막 활동 기록과 동일하게 보았던 것 같아요)
또한 접속 기록 업데이트가 핵심 비즈니스 로직이랑 분리되어야 한다고 생각하기도 했습니닷...! (트랜잭션 분리)
There was a problem hiding this comment.
지금은 사실 유저 접속 기록을 저장만 하는 것 같아서 AOP나 인터셉터에서 처리하는 방식으로 할 수 있을 것 같은데 추후에 유저 최근 접속 기록을 통해 처리할 기능(..알림?)이 있다고 생각하면 이벤트도 좋은거 같아요~ 👍
| return userRepository.create(user); | ||
| } | ||
|
|
||
| @Transactional |
There was a problem hiding this comment.
updateUserLoginAt 에만 붙여도 되는거 아닌가요?
| SecurityContextHolder.getContext().setAuthentication(authentication); | ||
|
|
||
| // 유저 최근 활동일 업데이트 | ||
| Events.publishEvent(new UserLoginEvent(userId)); |
There was a problem hiding this comment.
사용자가 마지막으로 API 를 호출한 시각을 기록하기 위함이라고 생각하면 될까요?
| public class UserLoginEvent extends Event{ | ||
| private UUID userId; | ||
|
|
||
| public UserLoginEvent(UUID userId) { | ||
| super(); | ||
| this.userId = userId; | ||
| } | ||
| } |
There was a problem hiding this comment.
지금은 사실 유저 접속 기록을 저장만 하는 것 같아서 AOP나 인터셉터에서 처리하는 방식으로 할 수 있을 것 같은데 추후에 유저 최근 접속 기록을 통해 처리할 기능(..알림?)이 있다고 생각하면 이벤트도 좋은거 같아요~ 👍
| try { | ||
| userCommandService.updateUserLoginAt(event.getUserId()); | ||
| } catch (Exception e) { | ||
| log.error("유저 로그인 시간 업데이트 실패 {}:", e.getMessage()); |
There was a problem hiding this comment.
실패하면 그냥 로그만 찍나요?
- "유저 로그인 시간 업데이트"가 아닌것 같아요
| refreshTokenCommandService.deleteByTokenId(tokenId); | ||
|
|
||
| // 유저 최근 접속일 업데이트 | ||
| Events.publishEvent(new UserLoginEvent(userId)); |
There was a problem hiding this comment.
유저가 어떤 동작을 안하고 프론트에서 그냥 토큰 재발급만 한 경우에도 유저가 최근 접속한 날짜가 변경되어야 하나요?
| SecurityContextHolder.getContext().setAuthentication(authentication); | ||
|
|
||
| // 유저 최근 활동일 업데이트 | ||
| Events.publishEvent(new UserLoginEvent(userId)); |
There was a problem hiding this comment.
로그인뿐 아니라 다른 API를 호출했을 때 갱신되는 값이라면 수정하는게 좋아보여요
📌 관련 이슈
#54
✨ 작업 개요
✅ 작업 상세 내용
🔍 고민 지점
음 처음에는 접속일이라 lastLoginAt을 사용했는데... 이벤트 동작 시점이 '토큰 필터 동작' 즉 API 요청 시점이라서 유저 마지막 활동일에 더 적합한 느낌이긴 하네욤 lastActivityAt으로 변경이 필요할까요?!