-
Notifications
You must be signed in to change notification settings - Fork 5
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
[Feature] - 좋아요 기능 구현 #336
Conversation
Test Results 31 files 31 suites 7s ⏱️ Results for commit f5a3d18. ♻️ This comment has been updated with latest results. |
String requestURI = request.getRequestURI(); | ||
String token = request.getHeader(HttpHeaders.AUTHORIZATION); | ||
|
||
return isInWhiteList(method, requestURI) && isTokenBlank(token); |
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.
화이트 리스트에 포함된 요청이더라도, 액세스 토큰이 존재한다면 JwtAuthFilter
를 거치도록 구현하였습니다.
여행기 상세 조회 시에만 따로 헤더를 체크하는 것 보다, 전체적인 통일성을 주는 것이 낫겠다고 생각했습니다.
하지만 이 방식의 문제점은 유효하지 않은 토큰을 보낼 때 문제가 됩니다.
이전에 클로버가 구현한 방식대로 58번 라인의 try-catch에서 화이트 리스트에 포함되는지 확인하는 것이 좋을까요?
아니면 유효하지 않은 토큰을 보낸 것 자체를 잘못된 요청으로 봐야 할까요?
전체적으로 의견 주시면 감사하겠습니다.
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.
여행기 상세 조회 시에는 헤더를 체크하지 않는 것으로 알고 있는데 여행 계획을 말씀하신 거겠죠?
그리고 이전의 구현에서 58번 라인의 try-catch에서 화이트 리스트에 포함되어 있는지 확인하는 코드가 없는 것으로 확인했는데 어떤 것을 말씀하시는 건지 잘 모르겠습니다.
58번 라인
@Override
protected void doFilterInternal(HttpServletRequest request, HttpServletResponse response, FilterChain filterChain)
throws ServletException, IOException {
String token = request.getHeader(HttpHeaders.AUTHORIZATION);
if (token == null || token.isBlank()) {
sendUnauthorizedResponse(response, "로그인을 해주세요.");
return;
}
token = token.split("Bearer|bearer")[1];
try {
String memberId = tokenProvider.decodeAccessToken(token);
request.setAttribute(MEMBER_ID_ATTRIBUTE, memberId);
filterChain.doFilter(request, response);
} catch (Exception e) {
sendUnauthorizedResponse(response, e.getMessage());
}
}
화이트리스트 체크는 shouldNotFilter
에서만 진행하고 있었고 이러한 구현에 위화감이 없다는 생각이었는데 shouldNotFilter
의 조건을 화이트리스트 체크와 isTokenBlank(token)
으로 바꾼 이유 좀 더 자세히 들을 수 있을까요?
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.
Authorization
헤더가 있다면 WhiteList에 있더라도 검증을 시도하는 것으로 이해했는데 맞나용?
좋아요 여부 조회 때문에 요렇게 구현하신 것 같은데 일단 최선의 방법 같습니다.
근데 이것도 저번에 말했던 비로그인해도 문제 없어야 되는 동작에서 토큰 만료 오류가 발생한다
는 문제점이 해결될 것 같진 않은데 요 부분은 어떻게 해결하실 생각이신가용? 의견이 궁금합니다
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.
여행기 상세 조회 시에는 헤더를 체크하지 않는 것으로 알고 있는데 여행 계획을 말씀하신 거겠죠?
여행기 상세 조회 시, 사용자가 게시글에 좋아요 여부를 같이 줘야 합니다! 그래야 빨간색 하트로 채울지 말지 결정할 수 있습니닷!!
따라서 로그인 한 경우에는 헤더를 체크해서 사용자 정보를 가져와야 하는데, 그러면서도 로그인 하지 않은 사용자도 정상 접속 할 수 있어야 합니다.
따라서 위와 같은 방식으로 구현하게 되었습니다..!
그리고 이전의 구현에서 58번 라인의 try-catch에서 화이트 리스트에 포함되어 있는지 확인하는 코드가 없는 것으로 확인했는데 어떤 것을 말씀하시는 건지 잘 모르겠습니다.
요건 제가 설명을 잘 못한것 같네요 ㅠㅠ 현재 그렇게 구현되어 있다는 것이 아니고, 그렇게 구현하면 어떨지 여쭈어 보는 것이었습니닷!
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.
이 부분만 해결 되면 approve 주세욥 @Libienz !!
비로그인해도 문제 없어야 되는 동작에서 토큰 만료 오류가 발생한다는 문제점이 해결될 것 같진 않은데 요 부분은 어떻게 해결하실 생각이신가용? 의견이 궁금합니다
조금 고민해 보았는데, 인증과 인가에 대한 역할을 분리하는 것은 어떻게 생각하시나요?
즉 2개의 필터를 운용하는 것이지요!
제가 생각하는 각 필터의 역할은 다음과 같습니다.
인증에 대한 필터는 모든 요청에 대해 액세스 토큰으로부터 멤버를 추출하는 역할만 합니다.
만약 토큰이 잘못되면 에러를 응답시키는 것이 아니라, request에 attribute를 넣지 않고 다음으로 넘어갑니다.
즉, 사용자의 신원을 확인하는 인증에 대한 책임만 가집니다.
그러면 그 다음에 인가에 대한 필터가 적용 되어서 request에 attribute가 있는지 확인합니다.
그리고 이에 따라 접근을 허용할지 말지 결정합니다. 즉 인가 필터에서 화이트 리스트를 다룹니다.
이렇게 하면 나중에 admin과 같은 추가 role이 들어와도 인가에 대한 필터에서 처리하면 됩니다.
저희가 저번에 대화를 나누면서도 모호했던 것이 화이트 리스트 자체가 인증이 아닌가?
였습니다.
따라서 이를 분리하면 어떨까 생각되었습니다.
객체지향 관점에서도 인증/인가에 대한 역할을 분리하는 것이 좋다고 생각되는데 의견 주시면 감사하겠습니다.
private final MemberService memberService; | ||
|
||
@Transactional | ||
public TravelogueResponse createTravelogue(MemberAuth member, TravelogueRequest request) { | ||
Member author = memberService.getById(member.memberId()); | ||
Travelogue travelogue = travelogueService.createTravelogue(author, request); | ||
List<TagResponse> tags = travelogueTagService.createTravelogueTags(travelogue, request.tags()); | ||
return TravelogueResponse.of(travelogue, createDays(request.days(), travelogue), tags); | ||
TravelogueLikeResponse like = travelogueLikeService.findLikeByTravelogueAndLiker(travelogue, author); |
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.
여행기를 생성할 때에는 좋아요 수가 0이고, 좋아요 여부가 false 입니다.
현재는 DB를 찔러서 확인하고 있지만, 다음과 같이 상수로 빼는 것이 더 나을까요?
private static final TravelogueLikeResponse INITIAL_LIKE = new TravelogueLikeResponse(false, 0L);
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.
개인적으로는 DB 찌르는 현재 구현이 신뢰성있다고 생각합니다!
후에 데이터가 꼬일 가능성이 없을 것 같긴 하지만 이 정도 쿼리 아낀다고 성능향상이 많이 있을 것 같지도 않아서요..!
개인적인 의견 드려봅니다 🙇🏻♂️
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.
생성 시에는 어차피 좋아요 수 0, 여부 false가 고정값이라 상수로 빼두는 것도 괜찮은 구현 같습니다
DB 찌르는 것도 리비 말처럼 신뢰성 있을 것 같네요!
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.
낙낙 수고하셨습니다 👍
질문 남겨주시니 리뷰하기 한결 수월했네요
궁금한 부분이 있어 우선은 코멘트로 리뷰 드립니다..!
확인하시고 리뷰요청 다시 주세요!
좋아요 기능이 빠르게 머지되어야 하는 것으로 알고 있는데 빠른 리뷰 원하시면 멘션 한 번 주세요!
다음 리뷰 바로 남겨보도록 하겠습니다~
String requestURI = request.getRequestURI(); | ||
String token = request.getHeader(HttpHeaders.AUTHORIZATION); | ||
|
||
return isInWhiteList(method, requestURI) && isTokenBlank(token); |
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.
여행기 상세 조회 시에는 헤더를 체크하지 않는 것으로 알고 있는데 여행 계획을 말씀하신 거겠죠?
그리고 이전의 구현에서 58번 라인의 try-catch에서 화이트 리스트에 포함되어 있는지 확인하는 코드가 없는 것으로 확인했는데 어떤 것을 말씀하시는 건지 잘 모르겠습니다.
58번 라인
@Override
protected void doFilterInternal(HttpServletRequest request, HttpServletResponse response, FilterChain filterChain)
throws ServletException, IOException {
String token = request.getHeader(HttpHeaders.AUTHORIZATION);
if (token == null || token.isBlank()) {
sendUnauthorizedResponse(response, "로그인을 해주세요.");
return;
}
token = token.split("Bearer|bearer")[1];
try {
String memberId = tokenProvider.decodeAccessToken(token);
request.setAttribute(MEMBER_ID_ATTRIBUTE, memberId);
filterChain.doFilter(request, response);
} catch (Exception e) {
sendUnauthorizedResponse(response, e.getMessage());
}
}
화이트리스트 체크는 shouldNotFilter
에서만 진행하고 있었고 이러한 구현에 위화감이 없다는 생각이었는데 shouldNotFilter
의 조건을 화이트리스트 체크와 isTokenBlank(token)
으로 바꾼 이유 좀 더 자세히 들을 수 있을까요?
public class TravelogueLike { | ||
|
||
@Id | ||
@GeneratedValue(strategy = GenerationType.IDENTITY) | ||
private Long id; | ||
|
||
@JoinColumn(name = "TRAVELOGUE_ID", nullable = false) | ||
@ManyToOne(fetch = FetchType.LAZY) | ||
private Travelogue travelogue; | ||
|
||
@JoinColumn(name = "LIKER_ID", nullable = false) | ||
@ManyToOne(fetch = FetchType.LAZY) | ||
private Member liker; | ||
|
||
public TravelogueLike(Travelogue travelogue, Member liker) { | ||
this(null, travelogue, liker); | ||
} | ||
} |
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.
다대다 확인!
private final MemberService memberService; | ||
|
||
@Transactional | ||
public TravelogueResponse createTravelogue(MemberAuth member, TravelogueRequest request) { | ||
Member author = memberService.getById(member.memberId()); | ||
Travelogue travelogue = travelogueService.createTravelogue(author, request); | ||
List<TagResponse> tags = travelogueTagService.createTravelogueTags(travelogue, request.tags()); | ||
return TravelogueResponse.of(travelogue, createDays(request.days(), travelogue), tags); | ||
TravelogueLikeResponse like = travelogueLikeService.findLikeByTravelogueAndLiker(travelogue, author); |
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.
개인적으로는 DB 찌르는 현재 구현이 신뢰성있다고 생각합니다!
후에 데이터가 꼬일 가능성이 없을 것 같긴 하지만 이 정도 쿼리 아낀다고 성능향상이 많이 있을 것 같지도 않아서요..!
개인적인 의견 드려봅니다 🙇🏻♂️
@DisplayName("여행기를 좋아요 할 때 로그인 되어 있지 않으면 예외가 발생한다.") | ||
@Test | ||
void likeTravelogueWithNotLoginThrowException() { | ||
Member author = testHelper.initKakaoMemberTestData(); | ||
testHelper.initTravelogueTestData(author); | ||
|
||
RestAssured.given().log().all() | ||
.when().post("/api/v1/travelogues/1/like") | ||
.then().log().all() | ||
.statusCode(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.
JwtAuthFilter에서 발생하는 예외를 테스트하는 부분이군요?
메시지 검증이 필요할 것 같은데 누락한 이유가 있을까요?
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.
오호 다른 테스트 코드 복붙해 온거라 세심하게 살펴보지 못했네욥.
복붙해온 코드까지 메시지 검증 추가할게욥!!
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.
일단 빠른 merge 위해서 approve 드렸는데 코멘트 몇 개 달았으니 확인해주세요!
pr 메시지에서 고민의 흔적이 많이 보입니다 ^_^,,,
수고하셨습니다리 낙낙
String requestURI = request.getRequestURI(); | ||
String token = request.getHeader(HttpHeaders.AUTHORIZATION); | ||
|
||
return isInWhiteList(method, requestURI) && isTokenBlank(token); |
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.
Authorization
헤더가 있다면 WhiteList에 있더라도 검증을 시도하는 것으로 이해했는데 맞나용?
좋아요 여부 조회 때문에 요렇게 구현하신 것 같은데 일단 최선의 방법 같습니다.
근데 이것도 저번에 말했던 비로그인해도 문제 없어야 되는 동작에서 토큰 만료 오류가 발생한다
는 문제점이 해결될 것 같진 않은데 요 부분은 어떻게 해결하실 생각이신가용? 의견이 궁금합니다
@DeleteMapping("/{id}/like") | ||
public ResponseEntity<TravelogueLikeResponse> dislikeTravelogue(@PathVariable Long id, @Valid MemberAuth member) { | ||
return ResponseEntity.ok() | ||
.body(travelogueFacadeService.unlikeTravelogue(id, member)); |
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.
그냥 의견) 별거 아니긴한데 dislike, unlike중에 통일되면 좋을 것 같네요!
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.
와우 dislike로 하다가 unlike가 더 나은거 같아 중간에 바꿨는데 미처 바꾸지 못했던 부분이 있었군요!!
바로 수정하겠습니닷
@Getter | ||
@NoArgsConstructor(access = AccessLevel.PROTECTED) | ||
@AllArgsConstructor(access = AccessLevel.PRIVATE) | ||
@Table(uniqueConstraints = {@UniqueConstraint(columnNames = {"TRAVELOGUE_ID", "LIKER_ID"})}) |
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.
질문) @JoinColumn
에도 unique=true
줄 수 있던데 여기다 따로 빼두신 이유가 있나요? 둘이 다른 역할을 하는건지 잘 몰라서 여쭤봅니당
만약 동일한 역할이라면 갠적으로 nullable=false 설정하는 것처럼 해당 컬럼 관련 설정은 컬럼에 붙어있는게 좋을 것 같습니다. 그래야 한 눈에 확인하기 쉬울 것 같아서요!
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.
@JoinColumn
에 주면 해당 컬럼 하나에 대해서만 unique가 걸립니다!
하지만 여행기 좋아요는 (travelogue_id, liker_id) 이 조합이 unique 해야 해서 위와 같이 구현했습니다!
즉, 복합 컬럼에 대해 unique를 걸고 싶어서 따로 뺐습니다!
|
||
public interface TravelogueLikeRepository extends JpaRepository<TravelogueLike, Long> { | ||
|
||
Long countByTravelogue(Travelogue travelogue); |
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.
아직 성능 논의할 단계가 아니긴한데 Like가 많아질수록 count가 자주 일어날 것 같기도합니다. 저희는 작성보다 조회가 훨씬 많이 일어나는 서비스라고 생각해서 Travelogue
에 likeCount
를 직접 주는 방식으로 관리하면 어떨까 싶은데 어떻게 생각하시나요?
요 부분은 다 같이 한 번 얘기해보시죠!
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.
코드 보니 좋아요와 관련된 모든 로직에서 countBy
가 일어나고 있네요! 한 번 고민해보시져
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.
확실히 고민해 볼만한 부분은 맞는 것 같습니다!
실제로 페이스북에서는 게시글 테이블 자체에 좋아요 숫자가 따로 존재한다고 하네요.
조회가 훨씬 많은 부분이고, 좋아요 숫자는 정확성이 엄청 높아야 하는 부분은 아니니까요!!
하지만 클로버가 말씀하신대로 아직 성능을 논의할 단계가 아니라는 점이 계속 고민을 하게 만드는 것 같습니다 ㅠㅠ
동시성도 아예 고민이 안되는 것은 아닌데, 이것도 또 고민하기에는 이른 거 같고 ㅎㅎ 어렵네요!
월요일에 다같이 한번 논의해 보시죠!
까먹지 않게 노션 백엔드 탭에 투두로 적어놓았습니닷
private final MemberService memberService; | ||
|
||
@Transactional | ||
public TravelogueResponse createTravelogue(MemberAuth member, TravelogueRequest request) { | ||
Member author = memberService.getById(member.memberId()); | ||
Travelogue travelogue = travelogueService.createTravelogue(author, request); | ||
List<TagResponse> tags = travelogueTagService.createTravelogueTags(travelogue, request.tags()); | ||
return TravelogueResponse.of(travelogue, createDays(request.days(), travelogue), tags); | ||
TravelogueLikeResponse like = travelogueLikeService.findLikeByTravelogueAndLiker(travelogue, author); |
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.
생성 시에는 어차피 좋아요 수 0, 여부 false가 고정값이라 상수로 빼두는 것도 괜찮은 구현 같습니다
DB 찌르는 것도 리비 말처럼 신뢰성 있을 것 같네요!
|
||
// then | ||
assertThat(response) | ||
.isEqualTo(new TravelogueLikeResponse(false, 1L)); |
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.
별거 아니긴한데 .
하나만 있으면 개행 안하셔도 될 것 같습니다!
그게 다른 코드와 컨벤션이 맞을 것 같네용
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.
와우,, 바로 고치겠슴다!!
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.
변경된 사항 확인했습니다 낙낙 👍
수고하셨어유!!
* feat: 여행기 좋아요 기능 구현 * style: swagger 메시지 수정 * feat: `@Transactional` 추가 * feat: 여행기와 좋아요를 누른 사용자에 대해 unique 제약 조건 추가 * fix: 컬럼명에 id 누락된 부분 추가 * feat: 여행기 좋아요 취소 기능 구현 * feat: 액세스 토큰이 존재하는 경우, 화이트 리스트의 요청도 `JwtAuthFilter`를 거치도록 변경 * feat: 여행기 상세 조회 시 좋아요 수, 좋아요 여부도 같이 응답하도록 변경 * feat: 메인 페이지에서 여행기 조회 시 좋아요 개수도 같이 응답하도록 변경 * refactor: JwtAuthFilter 메소드 분리 * docs: Swagger 응답 description 수정 * style: 클래스 첫 빈 줄 추가 * refactor: 좋아요 취소에 대한 단어를 전체적으로 `unlike`로 통일 * test: 401 예외 확인 테스트에서 메시지도 검증도 추가 * style: `.`이 하나만 존재할 때 줄바꿈 하지 않도록 컨벤션에 맞게 수정
✅ 작업 내용
🙈 참고 사항
isLiked
와likeCount
를 포함하기로 했습니다. 다만, 좋아요 숫자의 정확성에 대해서는 추가적인 논의가 필요합니다. 이 부분은 전체 회의에서 더 깊이 있게 다루면 좋을 것 같습니다. 자세한 내용은 떼껄룩 문서를 참고해 주세요.