feat: 좋아요 DELETE API 구현 #433
Conversation
dooboocookie
requested review from
kokodak,
zillionme and
chaewon121
and removed request for
kokodak,
zillionme and
chaewon121
There was a problem hiding this comment.
루카의 빠른 코드 작성! 수고하셨숩니다🫡
하지만 저는 항상 코드리뷰 하는 데에 오랜 시간이 걸리는 편이라, 최대한 빨리 작성해 보았습니다.
리뷰가 얼마 되지는 않지만 루카가 고민한 부분에 대해 같이 고민해 보았습니다. 학습적인 면도 리뷰로 달아놓았는데 같이 이야기 해봐도 좋을 것 같아용~
📉좋아요 0개 이하
이것은 예외일까요? 아닐까요?
예를 들어 이런 상황이 있다고 가정해볼게요.
통계 테이블이 다른 누군가의 의해서 실수로 조작돼서 좋아요를 누른사람의 수보다 좋아요 통계수가 더 적다고 가정해보겠습니다.
근데 그 사람들이 모두 좋아요 취소를 할 때 통계테이블에서 -1을 계속 하면 0개 이하로 내려갈 수 있습니다.
근데 0개에서 좋아요 취소를 누르는 사람은 예외 상황이라고 보는 것이 맞을까요? 아니면, 그냥 예외를 터트리지 않고 0개로 유지시키는것이 맞을까요?
지금은 0개로 유지를 시켜놓은 상태입니다.
테이블에서 likeCount의 컬럼의 제약조건은 항상 0이상이어야 한다고 생각합니다.
(루카의 코드 변경이 없을 것 같아요.)
이유 : 좋아요 개수가 음수라는 것은 불가능한 정의이기 때문입니다.
루카 코드를 보며 고민한 점.
원래 : delete 요청은 항상 같은 응답을 보내야 한다고 생각했습니다.
하지만 루카의 코드는 좋아요를 누른 적 없는 장소에 좋아요를 삭제하면 예외응답을 하는데요. (테이블에 해당하는 컬럼이 존재하지 않기 때문)
하지만 DELETE의 멱등성에 대해 찾아보니,DELETE의 멱등성의 기준은 상태코드가 아니라, 리소스의 상태(DELETE를 여러번 호출해도 응답 상태와 무관하게 리소스가 존재하지 않는 상태를 유지)라고 하네용.
| public void cancelLike(final CancelLikeCommand cancelLikeCommand) { | ||
| final Long playerId = cancelLikeCommand.playerId(); | ||
| final Player player = playerService.findPlayerById(playerId); | ||
|
|
||
| final Long placeId = cancelLikeCommand.placeId(); | ||
| final PlaceLike placeLike = placeLikeRepository.findByPlaceIdAndPlayerId(placeId, playerId) | ||
| .orElseThrow(() -> new PlaceLikeException(PlaceLikeExceptionType.NOT_EXIST)); | ||
|
|
||
| placeLike.validateOwner(player); | ||
| placeLikeRepository.delete(placeLike); |
There was a problem hiding this comment.
질문 1
Player player = playerService.findPlayerById(playerId);
플레이어의존재 여부는 확인하는데
placeId로 장소의 존재 여부는 왜 확인하지 않나요?
P4:
placeLike.validateOwner(player); 에 대해서는 필요없다고 생각합니다.
playerId에 해당하는 좋아요를 가져왔기 때문에 이중 검증을 할 필요가 있을까요?
gameId로 조회해오는 Game객체(validateOwner메서드 있음)와는 다른 상황이라고 생각합니다.
There was a problem hiding this comment.
Player player = playerService.findPlayerById(playerId);
플레이어의존재 여부는 확인하는데
placeId로 장소의 존재 여부는 왜 확인하지 않나요?
제가 너무 행위 주체를 Player라고 가정하고 로직을 처리했나봐요!!
그래서 꼭 Player를 조회해서 가져와야된다는 강박이 있었던 것 같아요.
그리고 그와 반대로 Place를 각자 조회하지 않는 부분은 의존성을 최소화하면서 로직 구현할려하다보니 그렇게 됏네요!!
제 생각엔 현재로선 player나 place가 여기에 있을 필요가 없을 것 같아요.
둘 다, 아이디 값으로 처리되도록 하겠습니다!!
placeLike.validateOwner(player); 에 대해서는 필요없다고 생각합니다.
playerId에 해당하는 좋아요를 가져왔기 때문에 이중 검증을 할 필요가 있을까요?
gameId로 조회해오는 Game객체(validateOwner메서드 있음)와는 다른 상황이라고 생각합니다.
제 생각에도 합당한 말씀이신것 같아요.
이는 위에서 이레가 설명해주신 이유에 의해서 제거하겠습니다.
| likeCount = LIKE_COUNT_DEFAULT_VALUE; | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
| public void subtractLike() { | |
| if(isLikeCountZero) { | |
| return; | |
| } | |
| likeCount--; | |
| } | |
| private boolean isLikeCountZero() { | |
| return likeCount = 0; | |
| } |
이 더 가독성이 좋지 않을까요?
There was a problem hiding this comment.
엇!
그렇네요.
이프문안에 유의미한 내용의 프라이빗 메서드로 구현해보겠습니다
| public class PlaceStatistics extends BaseEntity { | ||
|
|
There was a problem hiding this comment.
P5:
지난 번과 같은 코드에는 리뷰를 달 수 없어서요.
private Long likeCount;에 valdidation.@min(0)을 다는 것을 어떨까요?
물론 코드레벨(resetIfUnderZeroValue)에서 해당 제약조건을 지켜주는 것은 알고 있습니다.
하지만 생성자에서는 likeCount가 -1로 들어와도 예외가 발생하지 않기 때문입니다.
There was a problem hiding this comment.
제약조건을 거는 것도 좋은 것 좋은 방법 같습니다!!
저희는 어플리케이션 레벨에서는 0이하일 때는 그냥 0으로 유지시키기로 하기로 하였습니다.
만약 테이블의 제약조건을 거는 것이 목적이라면 위의 빈 밸리데이션 어노테이션 말고 @Column의 속성으로 제약조건을 거는 방법이 있을 것 같은데요.
일단 저희가 테이블에는 Fk관련된 제약조건 말고는 걸려있는게 없어서 일단은 두겠습니다!!
| Optional<PlaceStatistics> findByPlaceId(Long placeId); | ||
| } |
There was a problem hiding this comment.
P5:
PlaceStatisticsService에서 subtractLike()를 사용할 때, 동시성 문제가 발생할 수 있습니다.
사소한 문제이긴 하지만, 아래와 같은 두가지 해결책이 있을 것 같습니다.
- 비관적 락을 사용하는 방법(트랜잭션이 충돌이 발생한다고 가정하고 락을건다)
= selet ~ from ~ for update
| Optional<PlaceStatistics> findByPlaceId(Long placeId); | |
| } | |
| @Lock(LockModeType.PESSIMISTIC_WRITE) | |
| Optional<PlaceStatistics> findByPlaceId(Long placeId); | |
| } |
- 낙관적 락을 사용하는 방법
- 레포지토리에 특정 엔티티를 낙관적 락으로 관리하는 법
| Optional<PlaceStatistics> findByPlaceId(Long placeId); | |
| } | |
| @Lock(LockModeType.OPTIMISTIC) | |
| Optional<PlaceStatistics> findByPlaceId(Long placeId); | |
| } |
- 서비스에 버전 필드 추가를 통한 낙관적 락 사용
PlaceStatistics에 @Vesrsion int version이라는 필드를 추가하면, 엔티티를 조회한 시점의 버전과 수정하고난 후의 버전이 다르면 예외를 발생시킨다고 합니다.
There was a problem hiding this comment.
장소가 다양하기도 하고(사용자수도 적고), 각 장소에 비슷한 시간대에 도착해서 동일한 시간에 좋아요를 누를 확률은 0에 가까워서.. 락을 건다면 낙관적 락이 적절해보이는 것 같아요. 만약 동시성 이슈가 걸려서 예외가 터지더라도, 일정 횟수 재시도를 걸 수도 있겠네요. PESSIMISTIC_WRITE 로 건 비관적 락은 조회 성능에 영향을 끼칠 수도 있구요.
근데 동일한 시간에 좋아요를 누를 확률은 0에 가까워서 라는 전제 자체가 어쩌면 동시성 이슈가 발생하지 않는게 아닐까...? 하는 생각도 드네요 🤔
There was a problem hiding this comment.
일다 제 생각에도
저희가 좋아요 통계와 같은 도메인을 다루면서 동시성 고려를 하고 있다는 것은 좋으나,
일단 저희앱이 사용자가 많지 않은 상태이고 동시성을 이 PR안에서 다루는 것보다는 해당 이슈가 진짜로 발생했을 때(어? 이거 머피의 법칙인가) 혹은 그 문제가 우려되는 상황에서 대응하면 될 것 같습니다.
저도 리뷰 받고 많이 알아봤는데,
장소 좋아요같은 경우에는 일단 엄중한 결과를 필요로하는 도메인도 아니고, 성능적인 이점을 가져갈 필요도 있어 보이기 때문에 낙관적 락이 조금더 적절해보입니다.
일단은 적용은 하지 않겠습니다👍
| final PlaceStatistics placeStatistics = placeStatisticsBuilder.init() | ||
| .place(place) | ||
| .likeCount(beforeLikeCount) | ||
| .build(); |
| this.placeStatisticsBuilder = placeStatisticsBuilder; | ||
| this.placeStatisticsRepository = placeStatisticsRepository; | ||
| } | ||
|
|
There was a problem hiding this comment.
P5:
장소통계의 좋아요 수가 0인 경우의 테스트도 있으면 좋을 것 같아요!
There was a problem hiding this comment.
안녕하세요 루카
코드 잘 작성해주셨네요!! 자세한 PR도 감사합니다. 고생하셨습니당
몇가지 생각할 점과 피드백이 있어서 RC 요청 보냅니다 🐔
📉좋아요 0개 이하
저는 0개를 유지하는 코드가 적용됐다면 예외를 추가적으로 발생시킬 필요는 없어보여요. 정상적인 시나리오에서는 볼 일 없는 예외이기도 하고, 0개 유지가 된다는 가정하에 그렇게 크리티컬한 에러는 아닌 것 같아보여서 그렇습니당
⚙️좋아요 통계의 동기화
일단 트랜잭션 물려있으니까.. 이것도 정상적인 시나리오에서는 정합성이 깨질 일은 없겠지만.. 루카말대로 개발자의 실수로 깨질 수는 있겠네요. 다만 좋아요 수가 비즈니스적으로 엄밀함을 요구하는 기능은 아닌 것 같아서, 특정 주기(아마 긴 주기)마다 한번씩 DB 데이터 돌면서 정합성이 깨져있나 안깨져있나 확인하는 방법이 좋을 것 같아요!
| } | ||
|
|
||
| @DeleteMapping("/my") | ||
| public ResponseEntity<Void> deletePlaceLike(@Auth PlayerRequest playerRequest, |
There was a problem hiding this comment.
P4:
레벨2 당시 카일, 서브웨이에게 받았던 리뷰 중에 이런 말이 기억납니다.
Controller 와 Service 의 메서드명이 DB 친화적으로 되는 것은 지양해야한다
물론 저희가 지금까지 만들어온 메서드명은 다분히 DB 친화적이라 갑자기 이런 말을 하는게 이상할 수도 있지만..
루카가 PlaceLikeService 메서드명으로 cancelLike 를 지어주신 것이 굉장히 마음에 들어서 이런 리뷰를 한번 남겨봅니다 👨🚀 (cancel 이란 단어가 delete 보다 굉장히 비즈니스 친화적이라는 생각)
There was a problem hiding this comment.
Controller 와 Service 의 메서드명이 DB 친화적으로 되는 것은 지양해야한다
정말 좋은 말씀을 해주셨네요!!
큰 것을 하나 배웠네요.
저도 저것을 딱히 DB 친화적으로 지으려했던 것은 아닌데,
DELETE라는 이름이 나왔떤 이유는 API 메서드 때문이였던것 같아요.
제 생각에는 Service 레이어에서는 비즈니스적인 표현을 쓰는 것이 매우 적합해 보입니다!!
그치만 표현 영역에 있는 메서드 명도 비즈니스로직에 가까워야할까,
아니면 저희가 RestAPI로 표현하고있기 때문에 RESTAPI 스러운 이름이 되어야할까 고민이 되네요.
일단 이런 부분은 팀적으로 맞춰나가면 좋을 것 같습니다!!
코코닥의 의견 적극 반영해서 cancel로 변경하겠습니다!!
| Optional<PlaceLike> findByPlaceIdAndPlayerId(Long placeId, | ||
| Long playerId); |
| import com.now.naaga.player.presentation.dto.PlayerRequest; | ||
|
|
||
| public record CancelLikeCommand(Long playerId, | ||
| Long placeId) { |
| INACCESSIBLE_AUTHENTICATION( | ||
| 703, | ||
| HttpStatus.FORBIDDEN, | ||
| "접근 권한이 없는 좋아요/싫어요입니다."), | ||
|
|
||
| NOT_EXIST( | ||
| 704, | ||
| HttpStatus.NOT_FOUND, | ||
| "좋아요/싫어요가 존재하지 않습니다." | ||
| ), | ||
| ; | ||
|
|
| final PlaceStatistics placeStatistics = placeStatisticsRepository.findByPlaceId(placeId) | ||
| .orElseThrow(() -> new PlaceStatisticsException(PlaceStatisticsExceptionType.NOT_FOUND)); |
There was a problem hiding this comment.
P3:
이 코드를 보면서 생각났는데, 장소 통계를 장소 id 로 조회해올 때 없을 수도 있다는게 뭔가 부자연스러운 느낌? 이 들긴 하는데 팀원의 생각이 궁금하네요.
장소랑 장소 통계랑 OneToOne 이라서 장소가 생겨날 때 장소 통계도 같이 생겨나는것도 괜찮을 것 같은데 어떻게 생각하시나요? Lazy 하게 초기화하는게 나을려나
장소 통계를 Lazy 하게 가져갈거면, 아마 첫 장소 통계 데이터를 넣는 시점은 조회/수정이 일어날 때겠죠? 그러면 각 조회/수정 메서드에서 장소 통계를 placeId 로 조회해올 때 예외를 터뜨리면 안되겠네요. 실재하는 placeId 이고, 그것에 대한 장소 통계가 없었다면 새로 만들어줘야하기 때문에.. 다만 Lazy 하게 가게 된다면, 장소 통계 테이블에 있는 placeId 는 유니크해야하니까, 조회/수정 메서드에 있으면 데이터 넣고, 없으면 무시하고 하는 로직이 중복적으로 생길 것 같긴해요.
장소 통계를 장소 등록 시점에 바로 꽂아줄거면, placeId 로 장소 통계를 조회했을 때 장소 통계가 없을 시에 그것은 애초에 등록되지 않은 장소일테니 PlaceStatisticsException 보다는 그보다 더 본질적인 PlaceException 이 어울릴 것 같아요.
또한 장소 통계가 장소 등록시에 바로 꽂힌다면.. 장소랑 장소 통계랑 생명주기가 완전히 겹쳐버려서 패키지가 합쳐지는 게 나아보이네요.
아직은 팀적으로 결정난게 없어보여서, 일단 인지정도만 하고 있으면 될 것 같습니다.
There was a problem hiding this comment.
제 생각도 마찬가지입니다.
사실 저도 구현을 하면서 우려를 했던 점인데, Place의 생성로직을 고쳐야된다고 생각했습니다.
개인적인 입장은 여전히 Place 생성 시 생성해야된다고 생각중이여서, 이 이슈가 끝나면 바로 이슈를 파려고 했습니다.
저번에 코코닥의 PR에 대한 의견을 이어서, 이 코드가 머지되면 완벽한 코드는 아니고 버그를 유발할 수 있지만, 최대한 빠른 시일내에 이슈를 파고 수정해보면 어떨까요?
제 의견은 아까 말로 이야기했던 내용과 일치합니다.
- 장소 생성 시, 장소 통계를 생성한다.
- 장소 삭제 시, 장소 통계를 삭제한다.
- 이렇게 되면 장소로 장소 통계를 조회가 없으면 진짜 예외겠죠?
- 장소-장소 통계가 밀접한 도메인 제약조건을 갖고 생성 파괴 시점이 일치하므로 같은 도메인 패키지에서 관리한다.
There was a problem hiding this comment.
저번에 코코닥의 PR에 대한 의견을 이어서, 이 코드가 머지되면 완벽한 코드는 아니고 버그를 유발할 수 있지만, 최대한 빠른 시일내에 이슈를 파고 수정해보면 어떨까요?
아주 좋다고 생각합니다. 저 변경 사항을 이 PR 에 적용하기에는 PR의 SRP 원칙이 깨질 것 같아요! 이슈를 새롭게 파서 새로운 PR 로 보는걸로 하죠
제 의견은 아까 말로 이야기했던 내용과 일치합니다.
ㄱㄱ
| Optional<PlaceStatistics> findByPlaceId(Long placeId); | ||
| } |
There was a problem hiding this comment.
장소가 다양하기도 하고(사용자수도 적고), 각 장소에 비슷한 시간대에 도착해서 동일한 시간에 좋아요를 누를 확률은 0에 가까워서.. 락을 건다면 낙관적 락이 적절해보이는 것 같아요. 만약 동시성 이슈가 걸려서 예외가 터지더라도, 일정 횟수 재시도를 걸 수도 있겠네요. PESSIMISTIC_WRITE 로 건 비관적 락은 조회 성능에 영향을 끼칠 수도 있구요.
근데 동일한 시간에 좋아요를 누를 확률은 0에 가까워서 라는 전제 자체가 어쩌면 동시성 이슈가 발생하지 않는게 아닐까...? 하는 생각도 드네요 🤔
|
여러 의견 남겨주셔서 감사합니다. DELETE의 멱등성은 일단 현재 로직에서는 지켜지지 않습니다.
이레 말씀대로 자원 존재하지 않을 때 예외를 터트리지 않고 그냥 반환하면 1번에 대한 멱등성을 지킬 수 있습니다. 그래서 그렇게 적용하겠습니다!! 하지만 여전히 2번에 대한 멱등성은 지켜지지 않네요. 아마 저 작업은 PATCH 류의 작업을 DELETE 메서드에서 하기 때문이라고 생각합니다. |
| final PlaceStatistics placeStatistics = placeStatisticsRepository.findByPlaceId(placeId) | ||
| .orElseThrow(() -> new PlaceStatisticsException(PlaceStatisticsExceptionType.NOT_FOUND)); | ||
|
|
||
| placeStatistics.subtractLike(); | ||
| } | ||
| } |
There was a problem hiding this comment.
저도 장소가 생성될 때 장소 통계가 함께 생성되고,placeId로 장소 통계를 조회해왔을 때 예외가 발생하는 것이 맞다고 생각합니다.
코코닥의 말대로 Place 예외타입을 사용하는 게 맞을 것 같구요.
장소 통계 같은 경우, place가 삭제 될 때 함께 삭제되어야할 것 같은데 PlaceStatistics의 장소 필드에 @onDelete(action = OnDeleteAction.CASCADE)와 같은 옵션이 붙어야 하지 않을까요?
There was a problem hiding this comment.
위에서 언급했던대로, PlaceStatistics의 같이 생성되고 삭제되는 로직에 대해서는 추후에 따로 이슈 파서 진행하도록 하죠!!
|
다들 좋은 리뷰 감사합니다 여러분들의 좋은 리뷰 덕분에 |
📌 관련 이슈
🛠️ 작업 내용
🎯 리뷰 포인트
🔢프로세스
📄API
좋아요/싫어요 라는 리소스의 이름은
like로 표현합니다.위의 API는 다음과 같이 읽힐 수 있습니다.
장소들 중placeId장소의좋아요들 중내가 누른좋아요를 삭제한다그래서 Auth 헤더에 있는 토큰 정보의 playerId와 placeId로 좋아요를 검색해서 삭제합니다.
🧐오너 검증 필요성
보통 어떤 특정 도메인에 대해서 어떤 처리를 할 때,
그 도메인의 오너가 해당 플레이어가 맞는지 검증하곤 합니다.
하지만 이 삭제 로직에선
특정 Id의 좋아요를 삭제하는 것이아니라,특정 플레이어가 특정 플레이스에 좋아요를 누른 것을 찾아서 삭제합니다.
이미 이곳에서부터 플레이어를 검색조건으로 쓰고 있는데 validateOwner 검증이 필요할지 고민입니다.
이를 테스트하지 못했어요. 왜냐하면 터질 수 없는 예외기 때문이죠...ㅠ
⛓️의존성
플레이어장소,장소 좋아요,장소 통계의 의존관계를 갖습니다.좋아요는 누가 어디를 눌렀는지 알아야하므로장소와플레이어에게 의존성을 갖고통계는 어떤 통계인지 알아야하므로장소에게 의존성을 갖고장소는 누구에게 등록됐는지 알아야하므로플레이어에게 의존성을 갖습니다.저는 자연스럽다고 생각하시는데, 혹시 다른 의견이 있으실까봐 이해하시기 편하시라고 적어놨습니다.
📉좋아요 0개 이하
이것은 예외일까요? 아닐까요?
예를 들어 이런 상황이 있다고 가정해볼게요.
통계 테이블이 다른 누군가의 의해서 실수로 조작돼서 좋아요를 누른사람의 수보다 좋아요 통계수가 더 적다고 가정해보겠습니다.
근데 그 사람들이 모두 좋아요 취소를 할 때 통계테이블에서 -1을 계속 하면 0개 이하로 내려갈 수 있습니다.
근데 0개에서 좋아요 취소를 누르는 사람은 예외 상황이라고 보는 것이 맞을까요? 아니면, 그냥 예외를 터트리지 않고 0개로 유지시키는것이 맞을까요?
지금은 0개로 유지를 시켜놓은 상태입니다.
⚙️좋아요 통계의 동기화
현재는 트랜잭션에 의해서 좋아요 삭제될 때 통계에서 -1이 동시에 일어나도록 하고 있습니다.
근데 위에서 이야기한 것과 같은 데이터 정합성 깨지는일이 있을 수도 있을 것 같은데, 이를 어떻게 맞출 수 있을까요?(이것은 크게 고려할 필요는 없을 것 같긴 합니다)
변경 코드 수
이것은 예외도 다 정의하고 모든 클래스를 새로 만들고 테스트도 작성하다보니, 여러 라인의 변경이 생겼는데, 로직적으로 복잡한 내용은 없을 것입니다!
변경 상한선 못지킨 점 정말 죄송합니다.
코코닥이 디스커션에 올린 내용 다시한번 읽어볼게용
⏳ 작업 시간
추정 시간: 3시간
실제 시간: 5시간
삭제하는 로직이 생각보다 복잡하네요.