refactor: 댓글 삭제 로직 변경 #694

joelonsw · 2021-11-09T14:55:07Z

🧪 테스트 전체를 실행해봤나요?
💯 테스트가 잘 통과했나요?
🏗️ 빌드는 성공했나요?
🧹 안쓰는 코드가 남아있지 않나요?
🎟️ PR에 대한 이슈는 등록됐나요? => 지금 이 PR은 필수적인게 아니라서 우선 Issue는 등록 안해봅니당,,,
🏷️ PR 라벨 등록 했나요?
🍠 행복한 고구마인가요?

작업 내용

작업 동기
- 저번에 JPA에서 힘들었던 점 얘기하면서, 댓글 삭제 로직 얘기했었는데요.
- "User가 영속성 컨텍스트에 올라가있고, Reply를 삭제할라고 보니까, 연관 관계가 다수 얽혀있어 Reply가 제대로 삭제가 안되서 힘들었다" 라는 경험담을 이야기 했는데요
- 그래서 JPA에서 힘들었던 점 떠올라서 코드보다가 이상하다... 🤔 싶어서 조금 고쳐봤습니다.

이상한 점

현재 피드 삭제로직

public void delete(User user, Long feedId) {
    Feed findFeed = findEntityById(feedId);
    if (findFeed.notSameAuthor(user)) {
        throw new UnauthorizedException(ErrorType.UNAUTHORIZED_DELETE_FEED);
    }
    applicationEventPublisher.publishEvent(new NotificationFeedDeleteEvent(findFeed));
    feedRepository.delete(findFeed);
}

현재 댓글 삭제로직

public void deleteComment(User user, Long commentId) {
    Comment findComment = findEntityById(commentId);
    findComment.checkAuthority(user, ErrorType.UNAUTHORIZED_DELETE_COMMENT);
    if (findComment.isParentComment()) {
        applicationEventPublisher.publishEvent(new NotificationCommentDeleteEvent(findComment));
    }
    user.deleteComment(findComment);
}

피드랑 댓글 사실 엔티티 관계는 비슷하잖아요. Like 관계 중계 테이블에, User 작성자 있고, List<Comment> 있고 등등
근데 피드는 feedRepository.delete()로 지워지고, 댓글은 user에서 직접 끊어줘야 하는게 이상했어요
사실 DB 관점에서 바라봤을 때, 그냥 Comment의 튜플 하나 지우는 것은 어렵지 않잖아요?
그래서 댓글 삭제 로직도 commentRepository.delete(findComment); 로 코드 변경하고 테스트 돌려봤습니다

그전에 또 하나 이상했던 것
- 또한 이상했던 것은, 서비스 단에서 파라미터로 넘겨받은 User는 ArgumentResolver에서 이미 추출된 User 객체인데,
  - (그니까 ArgumentResolver에서 호출하고, DB 트랜잭션 열어서 조회해서 가져온 친구인데)
- 서비스에서 새로운 트랜잭션이 열렸을때 User 객체가 영속성 컨텍스트의 관리 대상이 되는가 싶더라고요?
  - 같은 트랜잭션도 아닌데 말이죠... 이건 제가 JPA의 지식이 부족한 탓이 있는 것 같아요
    - 막 알아서 준영속에서 영속이 되는 건가... User 엔티티가 처리 로직중에 필요해지면...?
  - 우선 영속성 컨텍스트의 관리대상이 아마 되는것 같아요.
    - user.deleteComment() 로직 진행해서 더티체크하고 반영되는 것 보면...
터지는 테스트들
- 공교롭게도 터지는 테스트들 (2개) 은 Reply와 관련된 것들이였는데요.
- 복잡한 관계들이 얽히고 섥혀있어 제대로 delete 쿼리가 안나가더라고요
- 터지는 테스트들을 보니까 em.flush() 만 given절에서 해주고 있어서 em.clear() 까지 해줬습니다
  - 우리가 테스트코드 단에서 @Transaction을 붙여서 setUp 대상의 모든 객체들이 영속성 컨텍스트에 올라감
    - ReplyLike, List<Reply> 등 모든 잡다한 연관관계가 영속성 컨텍스트에서 관리됨
    - 아마 이래서 테스트 코드에서 제대로 삭제쿼리를 못보낸 거 아닐까 추측중 🤔
  - em.clear() 까지 해줘야 새로운 트랜잭션을 열어서 테스트코드 처리하지 않을까 생각했음
  - https://www.inflearn.com/questions/38169
- 테스트가 돌아갔고, orphanRemoval 도 굳이 필요없지 않을까 싶어서 지워봤는데 테스트 코드 잘 돌아감
- 어드민 관련 테스트도 터져서 살짝 손 봄
나의 결론
- 테스트코드에서 @Transactional을 붙인게 약간 적폐이지 않았나...

netlify · 2021-11-09T14:58:23Z

✔️ Deploy Preview for nolto canceled.

🔨 Explore the source changes: da07907

🔍 Inspect the deploy log: https://app.netlify.com/sites/nolto/deploys/618b49ceccdb2800074b544d

Gomding · 2021-11-09T15:48:08Z

그래서 댓글 삭제 로직도 commentRepository.delete(findComment); 로 코드 변경하고 테스트 돌려봤습니다

완전 괜찮습니다 👍 👍 👍 👍
예전에는 'user가 직접 자신의 댓글을 지운다~' 라는 목적으로 user.deleteComment(comment); 를 사용했는데
서비스의 역할에서 보면 영속화 된 데이터를 지운다라는 측면에서 봐야할거같아요.
영속화된 데이터를 지우는데 orphanRemoval 설정에 의존적인 느낌이라 좋지 않아 보이네요
orphanRemoval은 좀 더 해당 엔티티와 깊은 관계에 있는 객체를 지울 때 사용하는게 맞는거 같네요 🤔

또한 이상했던 것은, 서비스 단에서 파라미터로 넘겨받은 User는 ArgumentResolver에서 이미 추출된 User 객체인데,
(그니까 ArgumentResolver에서 호출하고, DB 트랜잭션 열어서 조회해서 가져온 친구인데).
서비스에서 새로운 트랜잭션이 열렸을때 User 객체가 영속성 컨텍스트의 관리 대상이 되는가 싶더라고요?
같은 트랜잭션도 아닌데 말이죠... 이건 제가 JPA의 지식이 부족한 탓이 있는 것 같아요.
막 알아서 준영속에서 영속이 되는 건가... User 엔티티가 처리 로직중에 필요해지면...?
우선 영속성 컨텍스트의 관리대상이 아마 되는것 같아요.
user.deleteComment() 로직 진행해서 더티체크하고 반영되는 것 보면...

요 부분은 OSIV를 키워드로 공부해보시면 궁금증이 해소될거에요
JPA - OSIV(Open Session In View)

터지는 테스트들

comment 삭제 로직 변경 이후에 테스트가 터졌다는 거로 이해하면 될까요?

여기도 em.clear() 상당히 좋아요 👍
예전에 em.clear()를 뺐는데 왜 뺐는지 기억이 안나네요.. 이유가 있었던거 같기도하고

일단 테스트가 실패하는 이유를 조금 분석하면 User 엔티티에 Comments @onetomany 연관관계 필드를 보면
CascadeType.ALL이 걸려있을거에요. 요게 commentRepository.delete(comment)를 해서 comment 엔티티 객체의 상태를 removed 상태로 만들었지만 User에 남아있는 comment 객체가 있어서 엔티티 상태가 persist로 다시 변경 되는 순서일거에요~

commentRepository.delete(comment) -> comment 상태가 removed 가 됨
영속성 컨텍스트가 flush됨
User에 CascadeType.ALL 이 걸린 comments 필드에 삭제했던 comment 엔티티 객체가 남아있음 -> comment 상태가 persist 됨
결과적으로 comment는 persist 상태이므로 삭제 쿼리가 발생하지 않음

em.clear() 까지 해줘야 새로운 트랜잭션을 열어서 테스트코드 처리하지 않을까 생각했음

이건 트랜잭션이 새로 열리는거 보단 영속성 컨텍스트 내부를 비워버리는 거로 알고있어요!
List 컬렉션에 list.clear() 하는것 처럼요

추가.

EntityManager의 clear()는 영속성 컨텍스트 내부를 초기화 하는데 정확히는 entity 전체를 detached 상태로 만든다고 하네요
트랜잭션이랑은 별개라고 생각하시면 됩니다!
javax.persistence.EntityManager 공식 문서

Gomding · 2021-11-09T15:53:43Z

이슈도 등록해서 다른분들 approve가 되면 반영해도 좋다고 생각합니다~

추가로 User의 comments @onetomany 관계에 걸린 cascade 설정을 CascadeType.REMOVE로 설정해주는것도 좋다고 생각해요~

궁금하시면 em.clear() 추가하신거 롤백하고 위에 설정 적용해서 돌려보면 테스트가 통과할거에요!

PR merge할 때는 em.clear()는 추가하신거로 반영해주세요!

bosl95 · 2021-11-09T16:02:06Z

오 제가 공유드렸던 부분 빠르게 수정해주셨군요 👍👍👍👍 감사합니다!

제가 정확히 기억하고 있는 부분이라 당시 왜 그렇게 작성했었는지 상황을 조금 설명을 드리자면,
당시에 repository를 쓰자는 입장이었어서 더 기억에 잘 남아있어요 ㅋㅋㅋ 그때는 객체를 객체답게 쓰자, JPA 사용 이유가 무엇인가? 라고 말씀해주셔서 일단 연관 관계를 통해 적용하자고 결론이 났었슴다 😂
물론 저는 repository를 쓰는 것에 동의합니다 !! 지금 코드 좋아요!

그리고 찰리 말씀대로 feed보단 user가 직접 지운다 라는 맥락으로 그때 제가 제안했었는데, 결국 feed에 댓글을 영속할 수도 없고 user를 댓글에 영속할 수 없다는 특성을 간과하지 않았나 싶네요. 😭
개인적인 저의 지금 결론은 persist와 orphanRemoval는 부모 - 자식 관계가 명확한 엔티티 (다른 부모가 있으면 X, 유일한 부모- 자식관계)에서 사용해야되지 않나 라는 생각입니다!
(사실 정확히 말하자면 최대한 개발자가 결과 예측가능한 선에서의 관계 매핑이 최고라는 생각.. ㅎㅎ)

em.clear()도 그때 저는 쓰자는 입장이었어서 😂😂 당시에는 어떠한 이유로 작성했던 코드인지 조금 설명드리자면
em.clear()를 하고 findById를 해서 DB에 있는 것을 조회해와야 명확한 테스트가 되지 않을까? 라고 이야기 했을 때
굳이 em.clear()를 해서 영속성 컨텍스트를 비워야하냐? 결국 save한 엔티티를 바로 사용하는 것이고 엔티티 안에 값이 잘 들어가있는데 꼭 findById로 조회해와야하는 것인가? 라는 입장이 다수였던 것으로 기억합니다!
그래서 아마 clear()들이 여기저기서 제거되었던 것으로 기억해요 ㅠㅠ
em.clear()를 반영해줘야하는 곳과 반영해주지 않아도 되는 곳에 대해서는 아직 명확하게 정의내리기는 조금 어렵네요 🤔🤔

저번에 아마찌가 언급했던 것처럼 테스트 코드에서 @Transactional을 붙여서 발생하는 문제점이 여기였었군요!!

https://javabom.tistory.com/103

~~블로그에 나와있는 내용대로 @AfterEach로 롤백해주는 방법도 괜찮아보이네용~~

링크 다시 읽어보고 있는데 @Transactional을 빼면 fetch join을 해야하는 그림이군여
프로덕션 코드에 영향을 줄 정도의 테스트라면, @DataJpaTest의 @Transcational을 쓰는게 더 적절하다는 생각이 드네여!

수고 많으셨습니다 !! 👏👏👏👏

+) 척박한 취준 속 마른 단비같은 PR이네요 덕분에 재밌게 고민해보다 갑니다 ㅎㅎ

Gomding

👍

joelonsw · 2021-11-09T17:04:03Z

오 피알 올리기 잘했네요 여러분의 성원 힘이납니다 💪💪💪
두분 다 너무 좋은 피드백 감사드립니다. 덕분에 TIL 풍년이군요

개인적인 저의 지금 결론은 persist와 orphanRemoval는 부모 - 자식 관계가 명확한 엔티티 (다른 부모가 있으면 X, 유일한 부모- 자식관계)에서 사용해야되지 않나 라는 생각입니다!

기준 너무 좋네요! 포모의 말을 들으니 아 진짜 그렇다! 라고 생각이드네요.
Comment 같이 부모가 User이기도하고, Feed 이기도 한 친구는 어떤 부모와도 라이프사이클을 함께하기 힘드니까요. 👍👍👍👍👍
그리고 개발자가 예측 가능한 선에서의 코드가 좋은 코드라는 말도 매우 공감합니다...
내가 쓴건데 왜 내가 헷갈리냐고... 😅
남겨주신 @Transactional 관련 글도 잘 읽었습니다 :)

찰신의 깔끔한 Reply가 안지워지고 있는 이유 설명과, em.clear()의 속뜻 풀이,
그리고 OpenSessionInView 포스팅 감사합니다 너무너무 좋네요 저 포스팅
OpenSessionInView를 전혀 모르고 있었다고 봐도 무방했네요 그전까지ㅋㅋㅋㅋㅋ
감사합니다 :)
User의 Comments는 Cascade.REMOVE로 변경해서 커밋하나 추가해뒀습니다 :)

NewWisdom

우왓 어제 기절했다 온 사이에 많은 코멘트들이 오갔었네요 ㅎㅎ ㅠㅠ
피알이랑 코멘트들 재밌게 잘 읽었습니다 진짜루...
덕분에 예전에 연관관계, 테스트 코드로 한창 같이 고민했던 부분들도 생각나고...
이 피알 하나만으로 많은 복습이 되었네요 짱짱짱 ㅎㅎㅎ

위엣서 얘기한대로, 과거의 우리는 user가 직접 자신의 댓글을 지워야 객체다운거라고 생각하며
맹목적으로 user에서 피드나 댓글 등을 삭제하게 구현했었던 것 같아요 ㅎㅎㅎ
음 근데 조금 지나고 돌이켜 생각해보면,

그 복잡한 연관관계와 영속화의 뇌절을 겪어가면서 user가 댓글을 삭제한다는 행위를 반영하는 것이 과연 좋은 코드였을까?
만약 인자로 user가 넘어오지 않는 상황(user 없이 삭제)에서는 ?
찰리 말대로 그냥 단순히 서비스 입장에서는 해당 영속 데이터를 지운다 이니

바뀐 코드 방식으로 통일하는게 더 좋아보이네요 ㅎㅎ

그리고 우리 테스트 코드에서 @Transcational을 사용할 수 밖에 없는 이유도,
(얘기는 나눴지만 히스토리 차원에서 남깁니다 ㅎㅎㅎ)
현재 모든 서비스 메서드에서 받고 있는 User가 이미 ArgumentResolver에서 영속화 된 상태로 인자로 받고 있는데,
서비스 테스트에서 통합테스트(@SpringBootTest)를 하고 있으니
실제 환경과 동일히 영속화 된 user를 넘겨주려면 @Transcational이 들어갈 수 밖에 없었네요 !!!
근본적으로는 @Transcational의 적폐보다는 ArgumentResolver에서 영속 User를 그대로 내보내고 쓰고 있어서 생기는 문제가 아니였을까... 싶네요 ㅎㅎㅎ
(소나큐브가 계속 리졸버에서 pojo를 넘기라는게 이 때문일지도 ??)

째앴든 ~ 좋은 피알 감사합니다 ㅎㅎ
피알보고 몇가지 코멘트 남겨보았는데 가볍게 확인만 부탁드려용
덕분에 많이 배워갑니다 👏

NewWisdom · 2021-11-10T02:01:43Z

backend/src/main/java/com/wooteco/nolto/feed/application/CommentService.java

@@ -57,7 +57,7 @@ public void deleteComment(User user, Long commentId) {
        if (findComment.isParentComment()) {
            applicationEventPublisher.publishEvent(new NotificationCommentDeleteEvent(findComment));
        }
-        user.deleteComment(findComment);


User.deleteComment() 해당 메서드는 이제 사용하는 곳이 없으니 제거되면 좋겠네요 !! ㅎㅎ
(전반적으로 User 안에 쓰이지 않고 있는 메서드는 제거되어도 좋을 것 같아요 ㅎㅎㅎ)

옷 이거 제거된 거 일거에용!! ㅎㅎ

아아 User 클래스 안에는 남아있귈랭!! ㅎㅎ

남아있는 메서드 말씀하시는 거 였군여~~ 제삼다 ㅎㅎ

반영했습니다~

NewWisdom · 2021-11-10T02:27:10Z

backend/src/main/java/com/wooteco/nolto/user/domain/User.java

-    @OneToMany(mappedBy = "author", cascade = CascadeType.ALL, orphanRemoval = true)
+    @OneToMany(mappedBy = "author", cascade = CascadeType.REMOVE)


오 이제 자식과의 관계를 끊어서 삭제하는 로직이 없으니 현재로써는 orphanRemoval 설정이 불필요하군요 !!!

서비스의 역할에서 보면 영속화 된 데이터를 지운다라는 측면에서 봐야할거같아요.
영속화된 데이터를 지우는데 orphanRemoval 설정에 의존적인 느낌이라 좋지 않아 보이네요

코멘트를 읽다가 찰리의 이 말도 꽤 인상 깊었습니다 ㅎㅎ
음 이번에 조엘 pr로 삭제 로직을 부모 객체와의 연관관계를 통해서가 아닌 대상을 직접 삭제하기로 하고
여기서 불필요한 orphanRemoval 설정을 제거해주었다면,
Feed에 있는 orphanRemoval 설정도 제거해줘서 통일성을 가져가도 좋을 것 같다는 생각이 드네요 !!
(보니까 실제로 해당 옵션이 아무런 작용도 하지 않고 있었네요 😅)

요것도 반영했습니다~

joelonsw · 2021-11-10T04:21:37Z

소나큐브가 계속 리졸버에서 pojo를 넘기라는게 이 때문일지도 ??

오호... 소나큐브 피드백이 약간 이제서야 이해되네요
레벨2 때 id만 추출해서 dto마냥 controller로 넘겨주고 service단에서 그 dto 풀어서 찐으로 유저 찾은 뒤에 로직 진행하도록 코드 작성하곤 했거든요.

User를 아규먼트리졸버에서 바로 넘겨주는게 한 스텝 줄이는 과정이라고 생각했었는데,
어쩌면 pojo로 넘겨주는 것이 각 레이어별로 고려할 사안을 딱딱 끊어주고, 명확하게 해주는 방식일 수 있겠다 싶네요.
(서비스에서 필요하면 그제서야 user 조회해오고 쓰세요 갬성)

생각할 수 있는 포인트 감사합니당

- User 쓰이지 않는 deleteComment 메서드 삭제

bosl95 · 2021-11-10T05:11:30Z

코멘트보니까 또 궁금해져서 요것저것 보다가 ㅎㅎ 제 의견 정리해서 올려봅니닷

ArgumentResolver에서 영속화된 엔티티인 User가 서비스 레이어까지 넘어오게 되었고, 그로 인해 문제 발생! 여기까지가 저희 흐름인 거 같아요.

여기서 OSIV로 인해 영속화된 엔티티가 서비스 레이어로 넘어오는 것이다, 따라서 영속화된 User를 반환하지 않고 서비스 레이어에서 다시 find하는 흐름까지 온 거 같아요!
결국 이말인즉슨 OSIV를 이용하지 않는 느낌이 되겠네요. 저는 여기서 OSIV를 사용하지 않는 것이 과연 좋을까? 라는 생각이 들었어요.
OSIV를 사용하고 함은 결국 빈번한 Connection을 방지하기 위해서라고 이해했거든요...(다른이유도 있겠지만!)
👉 서비스 레이어에서 매번 findById를 한다 = connection이 두배!

명확한 테스트를 하고 싶어서 프로덕션 코드를 수정하고 빈번한 Connection을 증가시키는 것보단, 테스트코드가 개선될 방향을 찾아봐야하지 않을까?하는 저의 의견인데요!

그래서 테스트코드에 Transcational을 사용하지 않고 어떻게 프로덕션과 유사하게 테스트를 할 수 있을까를 고민해보았지만... 아직 명확하게 이거다!하는 방법은 못찾았어요 😥
다만 repository.deleteAll()을 쓰되, 테스트 코드에서 Transactional이 꼭 필요한 애들만 사용해주는 것이 맞지 않을까 라는 생각이 들었습니다..
적어도 클래스단에 적어주는 것보다는 메서드에서 트랜잭션을 선언해주는 게 조금이나마 명확한 테스트를 할 수 있지 않을까라는 생각이네요!!

두줄요약 : 테스트코드에서 @Transactional 없애고 정확한 테스트하기는 어려우니, 최대한 프로덕션과 동일한 환경에서 테스트하기위해 @Transactional이 필요한 애들만 메서드 레벨에서 사용하는 것은 어떨까

refactor: 댓글 삭제 로직 변경

f939d51

joelonsw added 🔨 refactor 🚀 backend backend work labels Nov 9, 2021

joelonsw self-assigned this Nov 9, 2021

Gomding approved these changes Nov 9, 2021

View reviewed changes

refactor(User): comments cascade 옵션 변경

3418885

NewWisdom approved these changes Nov 10, 2021

View reviewed changes

refactor(Feed): orphanRemoval 코드 삭제

da07907

- User 쓰이지 않는 deleteComment 메서드 삭제

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: 댓글 삭제 로직 변경 #694

refactor: 댓글 삭제 로직 변경 #694

joelonsw commented Nov 9, 2021 •

edited

Loading

netlify bot commented Nov 9, 2021 •

edited

Loading

Gomding commented Nov 9, 2021 •

edited

Loading

Gomding commented Nov 9, 2021 •

edited

Loading

bosl95 commented Nov 9, 2021 •

edited

Loading

Gomding left a comment

joelonsw commented Nov 9, 2021

NewWisdom left a comment

NewWisdom Nov 10, 2021

bosl95 Nov 10, 2021

NewWisdom Nov 10, 2021

bosl95 Nov 10, 2021

joelonsw Nov 10, 2021

NewWisdom Nov 10, 2021

joelonsw Nov 10, 2021

joelonsw commented Nov 10, 2021

bosl95 commented Nov 10, 2021 •

edited

Loading

		@OneToMany(mappedBy = "author", cascade = CascadeType.ALL, orphanRemoval = true)
		@OneToMany(mappedBy = "author", cascade = CascadeType.REMOVE)

refactor: 댓글 삭제 로직 변경 #694

Are you sure you want to change the base?

refactor: 댓글 삭제 로직 변경 #694

Conversation

joelonsw commented Nov 9, 2021 • edited Loading

작업 내용

netlify bot commented Nov 9, 2021 • edited Loading

Gomding commented Nov 9, 2021 • edited Loading

추가.

Gomding commented Nov 9, 2021 • edited Loading

PR merge할 때는 em.clear()는 추가하신거로 반영해주세요!

bosl95 commented Nov 9, 2021 • edited Loading

Gomding left a comment

Choose a reason for hiding this comment

joelonsw commented Nov 9, 2021

NewWisdom left a comment

Choose a reason for hiding this comment

NewWisdom Nov 10, 2021

Choose a reason for hiding this comment

bosl95 Nov 10, 2021

Choose a reason for hiding this comment

NewWisdom Nov 10, 2021

Choose a reason for hiding this comment

bosl95 Nov 10, 2021

Choose a reason for hiding this comment

joelonsw Nov 10, 2021

Choose a reason for hiding this comment

NewWisdom Nov 10, 2021

Choose a reason for hiding this comment

joelonsw Nov 10, 2021

Choose a reason for hiding this comment

joelonsw commented Nov 10, 2021

bosl95 commented Nov 10, 2021 • edited Loading

joelonsw commented Nov 9, 2021 •

edited

Loading

netlify bot commented Nov 9, 2021 •

edited

Loading

Gomding commented Nov 9, 2021 •

edited

Loading

Gomding commented Nov 9, 2021 •

edited

Loading

bosl95 commented Nov 9, 2021 •

edited

Loading

bosl95 commented Nov 10, 2021 •

edited

Loading