-
Notifications
You must be signed in to change notification settings - Fork 240
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
3단계 - 즐겨찾기 기능 구현 #355
base: hscom96
Are you sure you want to change the base?
3단계 - 즐겨찾기 기능 구현 #355
Conversation
- LoginMember 클래스 auth 패키지로 이동 및 User로 명칭 변경 - LoginMember 클래스 이동으로 순환이 제거되어 기존 UserDetail이 불필요하여 삭제
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.
안녕하세요 현수님! 😃
지난 피드백 반영과 즐겨찾기 기능 구현 잘해주셨네요 👍
예외 케이스에 대한 검증을 조금만 더 보완했으면 합니다!
관련 내용을 코멘트로 남겼으니 확인 부탁드리겠습니다
src/main/java/nextstep/auth/authorization/AuthenticationPrincipalArgumentResolver.java
Show resolved
Hide resolved
UserDetail loadUserByUsername(String email); | ||
User loadUserByUsername(String email); |
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.
UserDetail 인터페이스를 제거하고 User 클래스로 대체하기 보다
User 클래스가 UserDetail 인터페이스를 구현하는 형태는 어떨까요?
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.
네 먼저 말씀하신대로 다음 PR과 같이 구현 해봤는데 0e880a2
말씀하신 부분에서 궁금한점이 있습니다.
인터페이스 (ex. UserDetail) 사용의 효과중 하나로 의존성 역전을 위한것으로 이해했는데
제가 만든 프로젝트에서는 member 패키지에서 LoginMember가 삭제되고, User객체는 auth 패키지에만 있어서
기존처럼 User클래스가 UserDetail 인터페이스를 상속 안해줘도 member에서 auth패키지로 의존성이 한방향으로만 흐르고 있고
또한 User와 UserDetail이 같은 패키지 내부에 있는데
여기서 User가 UserDetail을 구현하는 장점이 어떤게 있을지 알 수 있을까요? 🤔
public class FavoritesController { | ||
private final FavoritesService favoritesService; | ||
|
||
@Secured(RoleType.ROLE_ADMIN) |
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.
요구사항
- 예외 케이스에 대한 검증도 포함하세요.
- 로그인이 필요한 API 요청 시 유효하지 않은 경우 401 응답 내려주기
관리자가 아닌 일반 회원, 비로그인 사용자에 따라 예외를 처리할 수 있게 변경해 보시면 좋을 것 같아요!
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.
기존 테스트에 일반 회원, 비로그인 사용자에 대한 예외 테스트를 보강했습니다 !!
e194e54
리뷰어님 안녕하세요!
드디어 마지막 단계 제출합니다 ㅎㅎ
지난 단계 꼼꼼히 리뷰해주셔서 감사합니다.
시간 나실때 천천히 리뷰 해주셔도 괜찮습니다. 😄