[feat/CK-242] 로드맵 관련 부분 의존성 리팩토링을 한다 #206

Ohjintaek · 2023-12-20T13:33:02Z

📌 작업 이슈 번호

✨ 작업 내용

로드맵과 관련된 부분을 패키지로 분리하고 의존성을 한 쪽으로 흐르도록 수정했습니다.
Member나 Auth, 공통 부분은 의존하도록 했고 GoalRoom을 의존하는 부분을 인터페이스를 활용하여 의존성 역전하는 것을 중점적으로 해 주었습니다. (RoadmapGoalRoomService를 만들고 구현체를 골룸 패키지에서 가지도록)

Member와 관련된 부분을 좀 깊게 의존하는 거 같긴 한데 로드맵 조회할 때 대부분의 경우 크리에이터인 Member를 필요로 해서 객체 참조를 하는 것으로 놔두었습니다. 아예 분리하고 로드맵이 memberId를 가지는 편이 낫다고 생각하시면 의견 주시면 감사하겠습니다.

💬 리뷰어에게 남길 멘트

(1) RoadmapGoalRoom의 경우 API 요청이 /roadmap/{roadmapId}/goal-rooms라서 RoadmapController에서 요청을 처리해 주지만 골룸에 관한 정보가 반드시 포함되어야 합니다. 골룸과 관련된 대부분의 의존성은 넘겨줬지만 GoalRoomStatus에 대한 의존성은 제거하지는 못했습니다. 일단 나중에 보기에 편하기 위해서 그대로 enum을 사용하는 거로 두긴 했지만 Dto 내부에서 String을 사용하는 것으로 바꾸면 의존성 제거를 할 수 있을 거 같은데 어떻게 생각하시나요?

(2) 로드맵 관련 도메인에서 사이클이 생기는 부분이 있는데 굳이 양방향 의존을 제거해야 하나하는 생각이 들어 없애진 않았습니다. 만약 제거한다면 Roadmap에서 콜렉션을 갖는 것을 제거할 거 같은데 해당 부분에 대해서 의견 주시면 감사하겠습니다. 아래는 의존성 사이클입니다.

🚀 요구사항 분석

github-actions · 2023-12-20T13:35:11Z

Unit Test Results

  78 files   78 suites 36s ⏱️
759 tests 759 ✔️ 0 💤 0 ❌
773 runs 773 ✔️ 0 💤 0 ❌

Results for commit ac71f6e.

♻️ This comment has been updated with latest results.

github-actions · 2023-12-22T06:16:06Z

🪄 Test Coverage Report

File	Coverage [99.3%]	🍏
RoadmapController.java	100%	🍏
RoadmapGoalRoomServiceImpl.java	100%	🍏
RoadmapReviewQueryRepositoryImpl.java	100%	🍏
GoalRoomQueryRepositoryImpl.java	100%	🍏
GoalRoomMemberQueryRepositoryImpl.java	100%	🍏
RoadmapOrderType.java	100%	🍏
RoadmapSearchCreatorNickname.java	100%	🍏
RoadmapSearchTagName.java	100%	🍏
RoadmapSearchTitle.java	100%	🍏
RoadmapScheduler.java	100%	🍏
RoadmapSaveArgumentResolverImpl.java	100%	🍏
RoadmapCreateService.java	100%	🍏
RoadmapDifficultyType.java	100%	🍏
RoadmapCreateEvent.java	100%	🍏
RoadmapQueryRepositoryImpl.java	99.6%	🍏
GoalRoomCreateService.java	99.24%	🍏
RoadmapReadService.java	98.99%	🍏
GoalRoomReadService.java	98.37%	🍏
RoadmapCreateEventListener.java	96.64%	🍏

Total Project Coverage	98.42%	🍏

miseongk

썬샷 리팩토링하시느라 고생하셨어요!!
깔끔하게 잘 해주셨네요 👍👍

로드맵 조회할 때 대부분의 경우 크리에이터인 Member를 필요로 해서 객체 참조를 하는 것으로 놔두었습니다. 아예 분리하고 로드맵이 memberId를 가지는 편이 낫다고 생각하시면 의견 주시면 감사하겠습니다.

저는 다른 컨텍스트는 무조건 의존을 끊고 id를 가지도록 했었는데요! 그렇게하니 조인을 할 수 없어 디비 접근을 여러번 할 수 밖에 없더라구요. 또 객체 참조를 끊는다고 해도 어짜피 결국 서비스 계층에서 로드맵이 멤버를 의존하는 것은 불가피한데, 도메인에서 의존을 끊어서 어떤 장점을 갖는지 갑자기 헷갈리네요 😅
하지만 로드맵에서 멤버를 직접 탐색하는 것을 끊어준다는 의미로 간접참조를 하는 것이면 저는 다 끊어주는 것도 좋다고 생각합니다! 이것은 저희가 규칙을 정하기 나름인 것 같아요. 다른 분들은 어떻게 생각하시나요?

(1) Dto 내부에서 String을 사용하는 것으로 바꾸면 의존성 제거를 할 수 있을 거 같은데 어떻게 생각하시나요?

DTO가 String을 갖도록 수정하면 될 것 같아요!

(2) 로드맵 관련 도메인에서 사이클이 생기는 부분이 있는데 굳이 양방향 의존을 제거해야 하나하는 생각이 들어 없애진 않았습니다. 만약 제거한다면 Roadmap에서 콜렉션을 갖는 것을 제거할 거 같은데 해당 부분에 대해서 의견 주시면 감사하겠습니다.

로드맵 관련 객체들 사이에서는 양방향 의존은 존재해도 괜찮을 것 같습니다!

그럼 좋은 연말 보내세요 🎄

backend/kirikiri/src/main/java/co/kirikiri/common/resolver/RoadmapSaveArgumentResolver.java

miseongk · 2023-12-26T12:17:31Z

backend/kirikiri/src/main/java/co/kirikiri/roadmap/persistence/RoadmapQueryRepositoryImpl.java

+import static co.kirikiri.domain.goalroom.QGoalRoom.goalRoom;
+import static co.kirikiri.domain.goalroom.QGoalRoomMember.goalRoomMember;
+import static co.kirikiri.domain.member.QMember.member;
+import static co.kirikiri.roadmap.domain.QRoadmap.roadmap;
+import static co.kirikiri.roadmap.domain.QRoadmapCategory.roadmapCategory;
+import static co.kirikiri.roadmap.domain.QRoadmapContent.roadmapContent;
+import static co.kirikiri.roadmap.domain.QRoadmapReview.roadmapReview;
+import static co.kirikiri.roadmap.domain.QRoadmapTag.roadmapTag;


혹시 save action 설정이 풀려있는지 확인해주실 수 있나요?? 정렬이 조금씩 바뀌어 있어서요!

엇 진짜 풀려있었는데 설정하고 해도 원복이 안 되네요...

backend/kirikiri/src/main/java/co/kirikiri/roadmap/service/RoadmapGoalRoomService.java

backend/kirikiri/src/main/java/co/kirikiri/roadmap/service/mapper/RoadmapMapper.java

miseongk · 2023-12-26T12:53:39Z

backend/kirikiri/src/main/java/co/kirikiri/service/goalroom/RoadmapGoalRoomServiceImpl.java

+        if (canDelete) {
+            goalRoomRepository.deleteAll(goalRooms);
+        }


이 부분은 메서드명과 반환값으로는 관련 골룸이 모두 삭제된다는 것을 알기 어려울 것 같아요!
그래서 그냥 원래대로 분리하는 것은 어떨까요?!

아 저도 리팩토링 때 canDeleteInGoalRoomsInRoadmap에서 boolean 값을 반환하는 동시에 GoalRoom들을 delete하는 게 문제라고 생각했었는데 역시 걸리네요

일단 저렇게 된 이유는 로드맵 패키지에 포함된 RoadmapScheduler에서 GoalRoom을 의존하는 상황을 없애려고 했기 때문입니다. 로드맵을 삭제했을 때 해당 로드맵에서 생성된 모든 골룸을 제거해주어야 하는데 이를 RoadmapGoalRoomServiceImpl로 넘겼더니 이렇게 됐네요. 일단 @onDelete 옵션을 활용해서 로드맵이 제거되었을 때 연관된 모든 골룸이 자동으로 삭제되도록 JPA를 활용하는 방식으로 수정해놓았습니다.

하지만 저희는 직접 의존을 끊어내는 작업을 하고 있으니까 추후에 JPA를 활용하는 방식 대신에 GoalRoom들의 정보를 가진 패키지에게 삭제를 요청하는 이벤트를 보내는 방식으로 다시 수정해 놓을게요 (GoalRoomScheduler에 TODO로 남겨주었습니다!!) 직접 의존을 제거하는 과정을 하면서도 왜 하는지에 대해 크게 와 닿지 않았는데 이 이슈를 해결하면서 어떤 의미가 있는지 좀 더 이해할 수 있었습니다

직접 의존을 제거하면 JPA의 특성을 제대로 활용하지 못할 수 있다.

직접 의존을 끊음으로써 큰 단위의 트랜잭션을 작게 가져갈 수 있다. (ex. 현재는 Roadmap을 제거할 때 GoalRoom과 관련된 삭제까지도 한 트랜잭션으로 묶이지만, 직접 의존 제거 후 이벤트 발행으로 트랜잭션 분리가 가능)

오 의존성을 제거하면서 로드맵 삭제 시 관련된 골룸이 모두 삭제되는 로직이 한 트랜잭션으로 묶여있었는데 이를 이벤트 발행으로 트랜잭션 분리를 할 수 있네요! 👍👍

backend/kirikiri/src/main/java/co/kirikiri/service/goalroom/RoadmapGoalRoomServiceImpl.java

backend/kirikiri/src/test/java/co/kirikiri/persistence/dto/RoadmapSearchTagNameTest.java

younghoondoodoom

안녕하세요 썬샷! 잘지내시나요? 이렇게라도 뵈니까 반갑네요🙌

패키지 분리하고 의존성 분리 하는게 정말 쉽지가 않네요.. 저는 비교적 간단한 부분을 담당해서 괜찮은데, 로드맵 관련 부분은 진짜 어질어질 하네요. 정말 고생하셨습니다👍

제 생각 간단하게 남겼으니까 참고 부탁드립니다! 참고로 도메인 의존성 관련된 부분은 질문에서 답변을 드린거 같아서 파일에 코멘트는 따로 남기지 않았습니다.

Member와 관련된 부분을 좀 깊게 의존하는 거 같긴 한데 로드맵 조회할 때 대부분의 경우 크리에이터인 Member를 필요로 해서 객체 참조를 하는 것으로 놔두었습니다. 아예 분리하고 로드맵이 memberId를 가지는 편이 낫다고 생각하시면 의견 주시면 감사하겠습니다.

저는 id를 통해 간접 참조 하는게 맞다고 봅니다! 객체 참조는 객체 간에 관계를 영구적으로 묶습니다. 그 말은 객체가 요구사항에 따라 상태가 바뀔때(생성, 변경, 삭제) 함께 바뀌어야한다는 것을 의미하는 것 같아요. 그 관점에서 봤을 때, Roadmap이 Member를 가지고 있으면 어색할 것 같아요!
조용호 님의 우아한객체지향에서 보면 연관된 모든 객체가 객체 참조를 통해 연관 관계를 맺고 있으면 생성 및 변경 시에 트랜잭션이 엄청 길어지는 문제가 발생할 수 있다고 해요! 비즈니스 요구사항이 더 복잡해질 수록 연관관계가 굉장히 많아지고 그게 한 트랜잭션에서 처리가 되니까요. 근데 id를 통해 간접 참조를 해놓으면 개념적으로 이미 분리가 되어있으니까 이벤트를 발행해서 처리하�는 방법들을 이용해서 트랜잭션을 쉽게 분리할 수 있을 것 같아요.

다만 트레이드 오프로 조회 시에 참조를 통해 접근을 하지 못하게 되어서 jpa를 사용하는 이점을 보지 못해서 불편한 단점이 있네요. 저도 이 부분에 관해서 많이 고민을 했는데 이미 많은 사람들이 고민을 통해서 몇가지 해결책을 만들어두었더라고요.
그 해결책 중 하나가 CQRS라는 개념이고 조회하는 도메인 모델과 명령(생성, 변경, 삭제)을 담당하는 객체를 나누어서 구현하는 방법이라고해요. 이렇게 되면 조회용 도메인 모델은 객체 참조를 하면서 JPA를 편하게 사용하면서 성능 최적화를 할 수 있고, 명령용 모델은 유지보수하기 좋게 관리할 수 있을 것 같아요. 근데 그만큼 관리 포인트가 많아지니까 저희 정도의 사이즈에서는 생각해봐야하긴 할 거 같아요. 그냥 똑같은 고민을 통해서 나온 방법론이 있다고만 말씀드리는 거에요!

저희는 이미 캐싱을 하고있기 때문에 쿼리가 분리되어서 나가도 성능 상 크게 상관 없을 것 같다고 생각해요. 썬샷은 어떤게 맞다고 생각하시나요??

(1) RoadmapGoalRoom의 경우 API 요청이 /roadmap/{roadmapId}/goal-rooms라서 RoadmapController에서 요청을 처리해 주지만 골룸에 관한 정보가 반드시 포함되어야 합니다. 골룸과 관련된 대부분의 의존성은 넘겨줬지만 GoalRoomStatus에 대한 의존성은 제거하지는 못했습니다. 일단 나중에 보기에 편하기 위해서 그대로 enum을 사용하는 거로 두긴 했지만 Dto 내부에서 String을 사용하는 것으로 바꾸면 의존성 제거를 할 수 있을 거 같은데 어떻게 생각하시나요?

response에서 도메인 객체를 사용하고 있네요! 제 생각에는 dto 내부에서 String을 사용해서 의존성을 끊는게 날 거 같아요! 그리고 이 api는 goalroom 쪽에 들어가는게 적절할 수도 있을 것 같은데 어떻게 생각하시나요?

(2) 로드맵 관련 도메인에서 사이클이 생기는 부분이 있는데 굳이 양방향 의존을 제거해야 하나하는 생각이 들어 없애진 않았습니다. 만약 제거한다면 Roadmap에서 콜렉션을 갖는 것을 제거할 거 같은데 해당 부분에 대해서 의견 주시면 감사하겠습니다. 아래는 의존성 사이클입니다.

A, B 객체가 양방향 의존관계가 되면 A 객체를 변경할때 B객체도 변경하는것이고, B 객체를 변경하면 A객체를 변경하는 개념이라고 합니다. 조회 시에는 양방향이 편하긴하겠지만 변경에 대한 유지보수성을 생각해보면 끊는게 낫다고 생각해요. 한 객체를 변경할때 거기에 영향을 받는 객체가 많아지고 모호해져서 응집도가 낮아질 것 같아요!

제 생각에는 조회는 생각하지 않고 일단 객체에 대한 생명주기를 기준으로 나누는게 좋을 것 같습니다! 다른 분들의 생각은 어떠신가요??

정말 모호하고 어려운 부분이라 의견이 충돌하는 부분이 많을것 같은데요. 텍스트로 주고 받기에는 너무 깊고 많은 내용이 오고갈거 같아서 추후에 시간 정해서 대화 해보시죠! 정말 고생하셨습니다🙇‍♂️

backend/kirikiri/src/main/java/co/kirikiri/common/resolver/RoadmapSaveArgumentResolver.java

backend/kirikiri/src/main/java/co/kirikiri/roadmap/service/RoadmapGoalRoomService.java

.../kirikiri/src/main/java/co/kirikiri/roadmap/service/dto/response/RoadmapForListResponse.java

backend/kirikiri/src/main/java/co/kirikiri/roadmap/service/dto/RoadmapDto.java

backend/kirikiri/src/main/java/co/kirikiri/roadmap/service/event/RoadmapCreateEvent.java

younghoondoodoom · 2023-12-28T03:58:44Z

backend/kirikiri/src/main/java/co/kirikiri/roadmap/service/RoadmapCreateEventListener.java

로드맵 이미지만 저장하는 로직이 들어있다보니 로드맵 패키지에 있는게 조금 어색한거 같아요. roadmap이랑 roadmapImage랑 나누는건 어떤가요? 둘의 생명주기가 다르다고 생각이 듭니다!

이건 꼭 변경했으면 좋은건 아니고 의견이 궁금합니다!

일단 Roadmp과 RoadmapContent, RoadmapReview는 생명주기가 다른 것 같아 의존관계를 끊어버렸는데 RoadmapContent와 RoadmapImage는 같이 움직인다고 생각해서 끊지는 않았습니다. 추가적으로 패키지 분리까지는 필요할까 싶어서 안 했는데 같이 얘기 나눠보면 좋을 거 같아요

저도 Roadmap / RoadmapContent, RoadmapNode, RoadmapImage / RoadmapReivew
이렇게 크게 세 부분으로 생명주기가 나뉜다고 생각해요!

저기서 패키지 분리를 하자면 RoadmapReview만 따로 패키지 분리를 할 수 있을 것 같아요.
근데 지금 저희 비즈니스 상 리뷰에 큰 비중을 두지 않아서 (추후에 추가될 가능성도 없고) 굳이 패키지를 나누지 않아도 괜찮아보이기도 해요!

네! 추후에 로드맵 사진을 따로 넣는다던가 하는 로직이 추가될 경우에 나누면 되겠네요👍

github-actions · 2024-01-14T14:59:41Z

🪄 Test Coverage Report

File	Coverage [97.89%]	🍏
RoadmapController.java	100%	🍏
MemberService.java	100%	🍏
JwtTokenProvider.java	100%	🍏
NaverOauthService.java	100%	🍏
RoadmapGoalRoomServiceImpl.java	100%	🍏
UUIDFilePathGenerator.java	100%	🍏
RandomNumberGenerator.java	100%	🍏
CacheKeyGenerator.java	100%	🍏
GoalRoomScheduler.java	100%	🍏
RoadmapReviewQueryRepositoryImpl.java	100%	🍏
GoalRoomPendingMemberQueryRepositoryImpl.java	100%	🍏
GoalRoomQueryRepositoryImpl.java	100%	🍏
GoalRoomMemberQueryRepositoryImpl.java	100%	🍏
CheckFeedQueryRepositoryImpl.java	100%	🍏
GoalRoomMemberJdbcRepositoryImpl.java	100%	🍏
RoadmapOrderType.java	100%	🍏
RoadmapSearchCreatorNickname.java	100%	🍏
RoadmapSearchTagName.java	100%	🍏
RoadmapSearchTitle.java	100%	🍏
RoadmapScheduler.java	100%	🍏
MemberQueryRepositoryImpl.java	100%	🍏
FileInformation.java	100%	🍏
RoadmapSaveArgumentResolverImpl.java	100%	🍏
RoadmapCreateService.java	100%	🍏
RoadmapCreateEventListener.java	100%	🍏
AmazonS3FileService.java	100%	🍏
NaverOauthNetworkService.java	100%	🍏
RoadmapDifficultyType.java	100%	🍏
GoalRoomController.java	100%	🍏
MemberController.java	100%	🍏
AuthController.java	100%	🍏
RefreshTokenRepositoryImpl.java	100%	🍏
RoadmapCreateEvent.java	100%	🍏
GoalRoomCreateService.java	98.52%	🍏
GoalRoomReadService.java	98.37%	🍏
RoadmapReadService.java	98.06%	🍏
AuthService.java	97.4%	🍏
RoadmapQueryRepositoryImpl.java	94.34%	🍏
CloudFrontService.java	93.75%	🍏
GenderType.java	90.38%	🍏
QuerydslRepositorySupporter.java	49.55%	❌

Total Project Coverage	97.93%	🍏

Ohjintaek · 2024-01-14T15:05:38Z

로드맵 도메인 내부에서 양방향 의존을 다 끊어버렸더니 수정할 게 생각보다 많았네요.
일단 Roadmap과 RoadmapReview, RoadmapContent는 이제 서로 전혀 모르는 상태입니다. 필요하다면 서비스 레이어에서 잘 조합해서 사용하구요. 그에 따라 일급 콜렉션인 RoadmapContents나 RoadmapReviews가 사용하지 않게 되었는데 일단 삭제는 하지 않았습니다.

추가적으로 현재 로드맵 삭제 시 하위 골룸들을 같이 삭제시켜줄 때 JPA를 활용하는 @onDelete를 사용해서 문제를 해결한 상태인데 나중에 밀리가 작업한 골룸 부분이 병합되고 나서는 이벤트 발행 방식으로 바꾸도록 하겠습니다 (지금 바꾸려면 제가 골룸부분 도메인 코드도 건드려야 해서, 일단 이렇게 해두었습니다)

코드 양적인 관점에서는 서비스 레이어에서 좀 더 코드를 많이 쳐야 되는 것 같긴한데, 연관관계를 끊다 보니 테스트 코드가 줄어들어 크게 상쇄되는 거 같긴합니다.

miseongk

의존성 분리를 한 단계 더 하시느라 코드 수정이 많았을텐데 고생하셨어요 썬샷!
제 의견을 남겨보았는데 확인 부탁드려요 🙇‍♀️
같이 이야기 해볼 부분이 조금 남아있는 것 같아 다시 한번 RC 할게요!
조금만 더 화이팅해봐요 👊

miseongk · 2024-01-18T07:40:36Z

backend/kirikiri/src/main/java/co/kirikiri/service/goalroom/RoadmapGoalRoomServiceImpl.java

+        if (canDelete) {
+            goalRoomRepository.deleteAll(goalRooms);
+        }


오 의존성을 제거하면서 로드맵 삭제 시 관련된 골룸이 모두 삭제되는 로직이 한 트랜잭션으로 묶여있었는데 이를 이벤트 발행으로 트랜잭션 분리를 할 수 있네요! 👍👍

miseongk · 2024-01-18T08:15:56Z

backend/kirikiri/src/main/java/co/kirikiri/roadmap/service/RoadmapCreateService.java

+    private RoadmapContent makeRoadmapContent(final RoadmapSaveDto roadmapSaveDto, final Long roadmapId, final RoadmapNodes roadmapNodes) {
+        final RoadmapContent roadmapContent = new RoadmapContent(roadmapSaveDto.content(), roadmapId);
+        roadmapContent.addNodes(roadmapNodes);
+        return roadmapContent;


RoadmapContent와 RoadmapNode의 생명주기가 같다면 RoadmapContent 생성 시 RoadmapNode도 함께 넣어 생성하면 좋을 것 같아요! 굳이 따로 addNodes를 해줄 필요가 없을 것 같아요.

miseongk · 2024-01-18T08:19:17Z

backend/kirikiri/src/main/java/co/kirikiri/roadmap/service/RoadmapCreateService.java

+        final RoadmapTags roadmapTags = makeRoadmapTags(roadmapSaveDto.tags());
        roadmap.addTags(roadmapTags);


RoadmapTag도 마찬가지로 로드맵 생성 시 생성자에 넣어 같이 생성해도 좋을 것 같아요!

좋습니다!! 😀
Roadmap과 RoadmapContent 모두 생성자에 넣도록 수정해주었습니다.

miseongk · 2024-01-18T08:20:38Z

backend/kirikiri/src/main/java/co/kirikiri/roadmap/service/RoadmapCreateService.java

-        return savedRoadmap.getId();
+        return roadmap.getId();


roadmap.getId()로 해줘도 문제없이 동작하겠지만 기존의 savedRoadmap.getId()가 조금 더 직관적인 것 같아요!

어, 이거 왜 통합 테스트에서 안 터졌을까요...
savedRoadmap.getId()로 수정해주었습니다!!!

miseongk · 2024-01-18T08:24:34Z

backend/kirikiri/src/main/java/co/kirikiri/roadmap/service/RoadmapCreateService.java

+        if (!roadmapGoalRoomService.hasGoalRooms(roadmapId)) {
            roadmapRepository.delete(roadmap);
+            roadmapContentRepository.deleteAllByRoadmapId(roadmapId);
+            roadmapReviewRepository.deleteAllByRoadmapId(roadmapId);


트랜잭션을 짧게 가져가기 위해서 이벤트를 발행해서 분리해도 좋을 것 같네요!

로드맵을 삭제하기 위해서 로드맵의 아이디를 FK로 가지는 Content나 Review들이 삭제되어야 돼서 한 트랜잭션 내에서 순서대로 진행되어야 할 거 같습니다. 일단 RoadmapReview는 로드맵이 사라지더라도 남도록 정책이 바뀔 가능성이 있는 것 같아 이벤트를 발행하도록 처리해주었습니다.
그와 별개로 156번째 라인이 가장 마지막에 시행되어야 DB에서 정합성 예외가 발생하지 않을텐데 제대로 실행되는 게 이상해서 확인해보니 코드 상에서 JPA가 Roadmap이 RoadmapContent나 RoadmapReview와 연관 관계를 갖지 않도록 수정했더니 테스트 DB 스키마가 기존과 달라지는 문제가 있네요. 일단 해당 코드는 순서를 수정해주었습니다.

miseongk · 2024-01-18T08:29:00Z

backend/kirikiri/src/test/java/co/kirikiri/roadmap/persistence/RoadmapRepositoryTest.java

-        final Roadmap gameRoadmap2 = 노드_정보를_포함한_로드맵을_생성한다("게임 로드맵2", creator, gameCategory);
-        final Roadmap travelRoadmap = 노드_정보를_포함한_로드맵을_생성한다("여행 로드맵", creator, travelCategory);
-        노드_정보를_포함한_삭제된_로드맵을_저장한다("여행 로드맵2", creator, travelCategory);
+        final Roadmap gameRoadmap1 = 로드맵을_저장한다("게임 로드맵", creator, gameCategory);


테스트 수정하시느라 고생많으셨어요 😂👍

miseongk · 2024-01-18T08:29:46Z

backend/kirikiri/src/main/java/co/kirikiri/roadmap/domain/Roadmap.java

    public Roadmap(final String title, final String introduction, final int requiredPeriod,
                   final RoadmapDifficulty difficulty, final Long creatorId, final RoadmapCategory category) {
        this(null, title, introduction, requiredPeriod, difficulty, RoadmapStatus.CREATED, creatorId, category);


Roadmap 생성자 중에서 사용하지 않거나 테스트에서만 사용하는 생성자를 정리하면 좋을 것 같아요 😊

생성자 몇 개 없애버리니 기분이 좋네요

miseongk · 2024-01-18T08:34:02Z

backend/kirikiri/src/main/java/co/kirikiri/roadmap/service/RoadmapCreateEventListener.java

저도 Roadmap / RoadmapContent, RoadmapNode, RoadmapImage / RoadmapReivew
이렇게 크게 세 부분으로 생명주기가 나뉜다고 생각해요!

저기서 패키지 분리를 하자면 RoadmapReview만 따로 패키지 분리를 할 수 있을 것 같아요.
근데 지금 저희 비즈니스 상 리뷰에 큰 비중을 두지 않아서 (추후에 추가될 가능성도 없고) 굳이 패키지를 나누지 않아도 괜찮아보이기도 해요!

younghoondoodoom

안녕하세요 썬샷! 의존성 분리하느라 정말 고생하셨습니다👍
아주 간단한 변경 요청이 있으니까 확인 부탁드립니다!

younghoondoodoom · 2024-01-20T07:03:31Z

backend/kirikiri/src/main/java/co/kirikiri/roadmap/domain/RoadmapContents.java

이 클래스는 이제 없어져도 되겠네요!

younghoondoodoom · 2024-01-20T07:04:31Z

backend/kirikiri/src/main/java/co/kirikiri/roadmap/domain/RoadmapReviews.java

이 클래스도 이제 없어져도 될것 같아요!

둘 다 없애버렸습니다 😀

github-actions · 2024-01-21T12:25:40Z

🪄 Test Coverage Report

File	Coverage [97.89%]	🍏
RoadmapController.java	100%	🍏
MemberService.java	100%	🍏
JwtTokenProvider.java	100%	🍏
NaverOauthService.java	100%	🍏
RoadmapGoalRoomServiceImpl.java	100%	🍏
UUIDFilePathGenerator.java	100%	🍏
RandomNumberGenerator.java	100%	🍏
CacheKeyGenerator.java	100%	🍏
GoalRoomScheduler.java	100%	🍏
RoadmapReviewQueryRepositoryImpl.java	100%	🍏
GoalRoomPendingMemberQueryRepositoryImpl.java	100%	🍏
GoalRoomQueryRepositoryImpl.java	100%	🍏
GoalRoomMemberQueryRepositoryImpl.java	100%	🍏
CheckFeedQueryRepositoryImpl.java	100%	🍏
GoalRoomMemberJdbcRepositoryImpl.java	100%	🍏
RoadmapOrderType.java	100%	🍏
RoadmapSearchCreatorNickname.java	100%	🍏
RoadmapSearchTagName.java	100%	🍏
RoadmapSearchTitle.java	100%	🍏
RoadmapScheduler.java	100%	🍏
MemberQueryRepositoryImpl.java	100%	🍏
FileInformation.java	100%	🍏
RoadmapSaveArgumentResolverImpl.java	100%	🍏
RoadmapCreateService.java	100%	🍏
RoadmapCreateEventListener.java	100%	🍏
AmazonS3FileService.java	100%	🍏
NaverOauthNetworkService.java	100%	🍏
RoadmapDifficultyType.java	100%	🍏
GoalRoomController.java	100%	🍏
MemberController.java	100%	🍏
AuthController.java	100%	🍏
RefreshTokenRepositoryImpl.java	100%	🍏
RoadmapCreateEvent.java	100%	🍏
GoalRoomCreateService.java	98.52%	🍏
GoalRoomReadService.java	98.37%	🍏
RoadmapReadService.java	98.06%	🍏
AuthService.java	97.4%	🍏
RoadmapQueryRepositoryImpl.java	94.34%	🍏
CloudFrontService.java	93.75%	🍏
GenderType.java	90.38%	🍏
QuerydslRepositorySupporter.java	49.55%	❌

Total Project Coverage	97.92%	🍏

Ohjintaek · 2024-02-26T02:06:40Z

리뷰하신 부분들 반영 완료했습니다 😆
말씀드릴 만한 부분은 이벤트로 로드맵 삭제 시 생성과 마찬가지로 트랜잭션을 끊으면 좋을 것 같아 발행하려고 했는데 DB 제약 조건 상 로드맵 하위의 리뷰, 골룸, 콘텐츠 등이 전부 삭제가 되어야지만 로드맵이 삭제되서 한 트랜잭션으로 묶일 수 밖에 없었습니다. 그래도 로드맵이 삭제될 때 콘텐츠는 같이 삭제되지만 리뷰는 삭제되지 않고 남겨지도록 비즈니스 로직이 수정될 여지가 있다고 생각해서 리뷰 삭제 부분은 이벤트를 발행해주었습니다.

이와 관련해서 현업에서는 수정의 용이성과 혹시 모를 cascade 옵션으로 인한 예상치 못한 데이터 삭제를 방지하기 위해 (DB에서 무결성 검사를 해주는 이점보다 이게 더 크다고 생각하는 것 같습니다) 외래키를 잘 사용하지 않는다고 하더라구요. 트랜잭션 관리하기도 용이하고 추후 데이터베이스 스키마의 변경에도 더 잘 대응할 수 있을 것 같습니다. 무결성 관련해서는 어플리케이션 코드와 테스트로 방지를 하구요. 저희 프로젝트를 바꾸자는 의견은 아니고 현업에서 외래키를 사용하지 않는 이유에 대해서 실제로 느낄 수 있었던 부분이어서 말씀드립니다!!

miseongk

안녕하세요 썬샷! 정말 오랜만이네요.. ㅎㅎ
리팩토링을 몇달동안 이어가시느라 정말 고생많으셨어요!!
이제 머지해도 좋을 것 같아요 👍
얼른 머지하고 끝냅시다! 고생하셨어요!!! 🙇‍♀️

miseongk · 2024-02-26T06:53:47Z

...end/kirikiri/src/main/java/co/kirikiri/roadmap/service/RoadmapReviewDeleteEventListener.java

+
+    private void deleteRoadmapReviews(final Long roadmapId) {
+        roadmapReviewRepository.deleteAllByRoadmapId(roadmapId);
+        applicationEventPublisher.publishEvent(new RoadmapDeleteEvent(roadmapId, "Roadmap"));


이 코드는 왜 있는건가요?!
이벤트 리스너 내에 로드맵 삭제를 처리하는 메서드가 없는 것 같은데요..!

아 원래 로드맵 리뷰 제거와 로드맵 제거 간의 트랜잭션을 분리해주려고 하려고 의도했어서 저렇게 했었습니다.
근데 외래키 제약조건 때문에 반드시 로드맵 리뷰가 모두 삭제된 뒤에 로드맵이 제거되어야 해서 의미가 없어진 부분이네요
삭제해주었습니다 😅

miseongk · 2024-02-26T07:04:05Z

backend/kirikiri/src/main/java/co/kirikiri/roadmap/service/RoadmapCreateService.java

+            applicationEventPublisher.publishEvent(new RoadmapDeleteEvent(roadmap.getId(), "Review"));
            roadmapContentRepository.deleteAllByRoadmapId(roadmapId);
-            roadmapReviewRepository.deleteAllByRoadmapId(roadmapId);
            roadmapRepository.delete(roadmap);


리뷰 삭제하는 부분을 트랜잭션에서 제거하기 위해서 @EventListener를 쓰신거죠?!
리뷰가 로드맵을 외래키로 갖고 있어서 만약 이벤트 리스너에서 리뷰를 삭제하는 로직을 수행하지 못하고 에러가 났을때, 트랜잭션으로 묶여있지 않아서 로드맵 삭제하는 코드를 실행할 때 에러가 날 수도 있을 것 같아요.
썬샷이 말씀해주신 외래키 때문에 트랜잭션을 분리할 수 없다는게 맞군요..!
외래키 제약조건을 없애거나 soft delete를 하는 방식이 더 좋을수도 있겠네요! (이렇게 하자는건 아닙니다)

우선은 이대로도 좋고 썬샷이 조금 더 옳다고 판단되는 방식으로 해주시면 좋을 것 같아요 🙂

github-actions · 2024-03-27T14:24:27Z

🪄 Test Coverage Report

File	Coverage [97.88%]	🍏
RoadmapController.java	100%	🍏
GoalRoomQueryRepositoryImpl.java	100%	🍏
GoalRoomMemberQueryRepositoryImpl.java	100%	🍏
GoalRoomMemberJdbcRepositoryImpl.java	100%	🍏
RoadmapOrderType.java	100%	🍏
RoadmapSearchCreatorNickname.java	100%	🍏
RoadmapSearchTagName.java	100%	🍏
RoadmapSearchTitle.java	100%	🍏
AmazonS3FileService.java	100%	🍏
RoadmapSaveArgumentResolverImpl.java	100%	🍏
RoadmapCreateService.java	100%	🍏
RoadmapReviewDeleteEventListener.java	100%	🍏
RoadmapCreateEventListener.java	100%	🍏
RoadmapDifficultyType.java	100%	🍏
GoalRoomController.java	100%	🍏
RoadmapCreateEvent.java	100%	🍏
RoadmapDeleteEvent.java	100%	🍏
MemberService.java	100%	🍏
RoadmapGoalRoomServiceImpl.java	100%	🍏
UUIDFilePathGenerator.java	100%	🍏
RandomNumberGenerator.java	100%	🍏
CacheKeyGenerator.java	100%	🍏
GoalRoomScheduler.java	100%	🍏
RoadmapReviewQueryRepositoryImpl.java	100%	🍏
AuthController.java	100%	🍏
RoadmapScheduler.java	100%	🍏
NaverOauthService.java	100%	🍏
JwtTokenProvider.java	100%	🍏
NaverOauthNetworkService.java	100%	🍏
MemberController.java	100%	🍏
RefreshTokenRepositoryImpl.java	100%	🍏
GoalRoomCreateService.java	98.52%	🍏
GoalRoomReadService.java	98.37%	🍏
RoadmapReadService.java	98.06%	🍏
AuthService.java	97.4%	🍏
RoadmapQueryRepositoryImpl.java	94.34%	🍏
CloudFrontService.java	93.75%	🍏
QuerydslRepositorySupporter.java	49.55%	❌

Total Project Coverage	97.95%	🍏

github-actions · 2024-03-27T14:43:41Z

🪄 Test Coverage Report

File	Coverage [97.87%]	🍏
RoadmapController.java	100%	🍏
GoalRoomQueryRepositoryImpl.java	100%	🍏
GoalRoomMemberQueryRepositoryImpl.java	100%	🍏
GoalRoomMemberJdbcRepositoryImpl.java	100%	🍏
RoadmapOrderType.java	100%	🍏
RoadmapSearchCreatorNickname.java	100%	🍏
RoadmapSearchTagName.java	100%	🍏
RoadmapSearchTitle.java	100%	🍏
AmazonS3FileService.java	100%	🍏
RoadmapSaveArgumentResolverImpl.java	100%	🍏
RoadmapCreateService.java	100%	🍏
RoadmapReviewDeleteEventListener.java	100%	🍏
RoadmapCreateEventListener.java	100%	🍏
RoadmapDifficultyType.java	100%	🍏
GoalRoomController.java	100%	🍏
RoadmapCreateEvent.java	100%	🍏
RoadmapDeleteEvent.java	100%	🍏
MemberService.java	100%	🍏
RoadmapGoalRoomServiceImpl.java	100%	🍏
UUIDFilePathGenerator.java	100%	🍏
RandomNumberGenerator.java	100%	🍏
CacheKeyGenerator.java	100%	🍏
GoalRoomScheduler.java	100%	🍏
RoadmapReviewQueryRepositoryImpl.java	100%	🍏
AuthController.java	100%	🍏
RoadmapScheduler.java	100%	🍏
NaverOauthService.java	100%	🍏
JwtTokenProvider.java	100%	🍏
NaverOauthNetworkService.java	100%	🍏
MemberController.java	100%	🍏
RefreshTokenRepositoryImpl.java	100%	🍏
GoalRoomCreateService.java	98.52%	🍏
GoalRoomReadService.java	98.37%	🍏
RoadmapReadService.java	98.06%	🍏
AuthService.java	97.4%	🍏
RoadmapQueryRepositoryImpl.java	94.34%	🍏
CloudFrontService.java	93.75%	🍏
QuerydslRepositorySupporter.java	49.55%	❌

Total Project Coverage	97.94%	🍏

Ohjintaek added 4 commits December 20, 2023 02:43

refactor: roadmap 관련 코드 패키지 분리

d72b469

refactor: RoadmapCreateService에서 goalRoom 의존성 제거

2ea6239

refactor: RoadmapReadService에서 goalRoom 의존성 제거

91f128b

refactor: RoadmapScheduler에서 goalRoom 의존성 제거 및 테스트코드 패키지 분리

7085619

Ohjintaek added BE 👨‍👩‍👧‍👦 BackEnd refactor🧹 refactor labels Dec 20, 2023

Ohjintaek requested review from miseongk and younghoondoodoom December 20, 2023 13:33

Ohjintaek self-assigned this Dec 20, 2023

Ohjintaek added 4 commits December 22, 2023 14:06

refactor: Roadmap에서 발생한 예외는 RoaadmapException을 던지도록 수정

3c0ba17

refactor: RoadmapSaveArgumentResolver 의존성 역전

354f9b6

test: 테스트 코드 수정

c8fc8f4

chore: build.gradle 수정 (jacoco 테스트 커버리지)

a307525

miseongk requested changes Dec 27, 2023

View reviewed changes

younghoondoodoom requested changes Dec 28, 2023

View reviewed changes

Ohjintaek added 8 commits January 12, 2024 23:51

refactor: roadmap과 roadmapReview에서 member 직접 참조를 삭제

7ce0184

chore: ScrollResponseMapper common으로 이관

b63f1b7

refactor: 로드맵 삭제 시 하위 골룸 삭제 방식 변경 (OnDelete 옵션 사용)

a0f588b

chore: 패키지 경로 수정

3d6cd17

refactor: dto 패키지 분리

df672b0

refactor: Event 객체에서 도메인 직접 전달 제거

7a1b1f3

refactor: RoadmapNode에서 RoadmapContent 의존 제거

3c5fc04

refactor: Roadmap과 RoadmapContent, RoadmapReview간 의존 관계 제거

9cf3c2a

younghoondoodoom requested review from miseongk and younghoondoodoom January 15, 2024 05:09

miseongk requested changes Jan 18, 2024

View reviewed changes

younghoondoodoom approved these changes Jan 20, 2024

View reviewed changes

Ohjintaek added 2 commits January 21, 2024 16:24

refactor: Roadmap 생성 시 태그, RoadmapContent 생성 시 노드들을 필수로 넣도록 수정

bdccbbc

test: RoadmapNodes 테스트코드 추가

5cf8113

Ohjintaek added 3 commits February 15, 2024 22:45

refactor: 불필요한 클래스 삭제

a0472bc

fix: 데이터베이스 무결성 오류 수정

732673f

refactor: 로드맵 삭제 시 연관된 리뷰 삭제를 이벤트로 발행

fc761d5

Ohjintaek requested a review from miseongk February 26, 2024 01:56

miseongk approved these changes Feb 26, 2024

View reviewed changes

chore: merge conflict 해결

ac71f6e

refactor: 불필요한 이벤트 호출 제거

3a7e610

Ohjintaek merged commit 9152846 into develop-backend Mar 27, 2024
1 check passed

		final RoadmapTags roadmapTags = makeRoadmapTags(roadmapSaveDto.tags());
		roadmap.addTags(roadmapTags);

[feat/CK-242] 로드맵 관련 부분 의존성 리팩토링을 한다 #206

[feat/CK-242] 로드맵 관련 부분 의존성 리팩토링을 한다 #206

Conversation

Ohjintaek commented Dec 20, 2023

📌 작업 이슈 번호

✨ 작업 내용

💬 리뷰어에게 남길 멘트

🚀 요구사항 분석

github-actions bot commented Dec 20, 2023 • edited Loading

Unit Test Results

github-actions bot commented Dec 22, 2023

🪄 Test Coverage Report

miseongk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ohjintaek Jan 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

younghoondoodoom left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jan 14, 2024

🪄 Test Coverage Report

Ohjintaek commented Jan 14, 2024

miseongk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

younghoondoodoom left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jan 21, 2024

🪄 Test Coverage Report

Ohjintaek commented Feb 26, 2024

miseongk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Mar 27, 2024

🪄 Test Coverage Report

github-actions bot commented Mar 27, 2024

🪄 Test Coverage Report

github-actions bot commented Dec 20, 2023 •

edited

Loading

Ohjintaek Jan 13, 2024 •

edited

Loading

younghoondoodoom left a comment •

edited

Loading