Conversation
Test Results38 tests 38 ✅ 7s ⏱️ Results for commit cf73fa2. |
| @@ -72,4 +72,14 @@ public void updateFeedbackStatus() { | |||
| this.answerStatus = AnswerStatus.BEFORE; | |||
| } | |||
| } | |||
|
|
|||
| public void increaseLike() { | |||
There was a problem hiding this comment.
한번에 많은 사용자가 좋아요를 눌렀을 때, 정합성을 지킬 수 있을까요?
이에 대한 테스트를 해보시면 좋을 것 같습니다.
There was a problem hiding this comment.
한 번에 많은 사용자가 눌렀을 경우에는 정합성이 지켜지지 않을 것 같습니다...
원래는 Redis에 좋아요 요청을 캐싱한 뒤, 주기적으로 Batch 작업으로 데이터를 동기화하는 방식을 생각했습니다. 그러나 시간이 부족하여 간단한 CRUD 구현으로 대체했습니다.(나중에 개선할 예정입니다!)
낙관적/비관적 락도 고려했으나, 좋아요 데이터가 서비스의 핵심 데이터는 아니라고 판단했기 때문에 성능에 큰 영향을 주는 처리는 제외했습니다.
혹시 Redis로 캐싱하는 방법도 정합성을 지키는 방법으로 괜찮을까요?
| @Operation(summary = "좋아요 추가") | ||
| @PostMapping("/{feedbackId}") | ||
| public ResponseEntity<Void> addLike(@PathVariable("feedbackId") Long feedbackId, | ||
| @CurrentUser CustomUser2Member user) { |
There was a problem hiding this comment.
CustomUser2Member 클래스 명을 이렇게 지으신 이유가 있을까요?
There was a problem hiding this comment.
해당 클래스는 주하님이 작성하셔서 왜 그렇게 지으신지는 잘 모르겠습니다...
|
|
||
| @Operation(summary = "좋아요 추가") | ||
| @PostMapping("/{feedbackId}") | ||
| public ResponseEntity<Void> addLike(@PathVariable("feedbackId") Long feedbackId, |
There was a problem hiding this comment.
스프링에서 제공하는 기본 응답 클래스 대신, 공통 api 응답을 구현하는 것은 어떨까요?
There was a problem hiding this comment.
공통 api 응답 관련해서 프로젝트 초기에 이야기가 나왔습니다.
해당 주제에 대한 저의 의문은 Http 상태 코드로 해당 요청의 성공과 실패를 알 수 있고, 클라이언트는 결국 반환하는 데이터가 가장 중요할텐데 다른 부가적인 요소가 필요한가였습니다.
또한 에러 응답을 반환할 때도 너무 상세한 에러를 포함하면 보안상 위험한 것과 같이, 클라이언트가 꼭 필요한 정보와 적절한 Http 상태 코드만 반환하는 것이 좋다고 생각되어 공통 api 응답은 만들지 않았습니다.
네이버의 공통 응답 형식
{ "code" : "Success", "message" : "", "body" : {} }
대부분 위와 비슷한 공통 응답 형식을 가지던데, 공통 api 응답이 주는 이점에서 가장 큰 이유가 일관성을 유지해서 클라이언트의 편의성이 증가되는 것이 맞을까요?
| return ResponseEntity.status(HttpStatus.CREATED).build(); | ||
| } | ||
|
|
||
| @Operation(summary = "좋아요 유무 확인") |
There was a problem hiding this comment.
경로 명명 규칙
existsLike와 deleteLike의 경로는 각각 /exist/{feedbackId}와 /{feedbackId}인데, 경로의 일관성이 부족해 보입니다.
existsLike는 좋아요 유무를 확인하는 API이므로 /{feedbackId}/exists로 바꾸는 것이 더 일관성이 있을 수 있습니다. 마찬가지로 좋아요 삭제도 /feedback/{feedbackId}/like 같은 형태로 변경한다면 좀 더 명확해 질 것입니다.
There was a problem hiding this comment.
말씀해주신 경로가 확실히 더 명확해 보이네요!
앞으로 경로명도 찾아보고 참고하면서 일관성있게 만들어 보겠습니다!
| public class Like { | ||
|
|
||
| @Id | ||
| @GeneratedValue(strategy = GenerationType.IDENTITY) | ||
| private Long likeId; | ||
|
|
||
| @ManyToOne | ||
| @ManyToOne(fetch = FetchType.LAZY) |
There was a problem hiding this comment.
좋아요 기능은 특정 피드백에 누른 여부만 확인하면 된다고 판단했습니다. 그래서 불필요한 조인을 막고자 지연 로딩 설정을 했습니다!
|
|
||
| @Transactional | ||
| public void addLike(Long feedbackId, CustomUser2Member user) { | ||
| Feedback feedback = feedbackRepository.findById(feedbackId) |
There was a problem hiding this comment.
중복되는 코드가 많긴 하네요...
피드백과 멤버를 조회하는 코드는 따로 메서드로 만들어서 변경해보겠습니다!
| } | ||
|
|
||
| public LikeExistResponse existsLike(Long feedbackId, CustomUser2Member user) { | ||
| Feedback feedback = feedbackRepository.findById(feedbackId) |
There was a problem hiding this comment.
existsByMemberAndFeedback(member, feedback)를 호출하기 위해서 각각의 엔티티를 인자로 넣어주려고 조회했습니다...
지금 다시 생각해보니 불필요한 조회라고 생각이 되어서 외래키로 좋아요 데이터를 조회하는 방식으로 변경해보겠습니다!
| private final MemberRepository memberRepository; | ||
| private final FeedbackRepository feedbackRepository; | ||
|
|
||
| @Transactional |
There was a problem hiding this comment.
클래스에도 Transactional(readOnly=true), 메소드에도 transactional 어노테이션이 붙어있는데, 의도하신 걸까요?
There was a problem hiding this comment.
클래스의 기본 트랜잭션은 읽기 전용으로 설정했습니다.
그렇지만 insert, update, delete 쿼리를 사용하는 메서드에서는 트랜잭션 속성이 readOnly = false가 되게 명시적으로 설정했습니다.
LikeService는 메서드가 많지 않아서 큰 상관없은 없지만, 읽기 작업을 하는 메서드가 많고 쓰기 작업을 하는 작업을 하는 메서드가 적은 상황에서는 해당 방법이 성능과 가독성에도 좋다고 들어서 이렇게 사용하고 있습니다. (저희 프로젝트에서 비슷한 예시로 FeedbackService 클래스가 있습니다.)
혹시 제가 잘못 알고 있는 건가요...? 잘못된 지식이라면 얼른 바꿔보도록 하겠습니다!
#️⃣ 연관된 이슈
✅ 체크리스트
ex) [feat] ㅇㅇ 기능 추가📝 작업 내용
📸 스크린샷 (UI 변경 시 필수)
👀 리뷰어 가이드라인