Refactor/#165: 채팅 메시지 도메인 리팩토링 #171

hyunn522 · 2024-11-21T12:45:36Z

📝 요약

resolved Refactor: 채팅 메시지 도메인 리팩토링 #165

🔖 변경 사항

도메인 구조로 디렉토리 변경
chat 도메인에서 일급컬렉션 적용
Service layer 추상화 : Module Service와 Component Service 도입
- Module Service : Repository만을 의존하며 실제로 DB와 상호작용 및 null에 대한 간단한 예외처리를 거쳐 entity를 반환
- Component Service : Module Service만을 의존하며 Module Service로부터 얻은 entity를 이용한 비즈니스 로직만을 포함

매직넘버 상수화 및 가독성, 확장성 향상 : 불필요한 변수/주석 삭제, 각 메소드에 대해 SRP 준수
@Builder는 기본적으로 클래스 전체에 대해 동작하여 접근 제어자(private)를 따르지 않음
-> 외부에서 @Builder 사용을 막기 위해 AccessLevel 제한 설정

✅ 리뷰 요구사항

이슈명은 채팅 메시지 도메인이지만 수정하다보니 채팅방 도메인에도 수정이 많아졌네욤.. 다음부턴 주의하겠습니담

일급컬렉션 사용이 적절한지 봐주시면 좋을 것 같습니다.
현재 chat 도메인에 대해서만Module Service를 적용했는데, 해당 부분이 적절한지 봐주시면 좋을 것 같습니다.
피드백 반영하여 (현재 util로 작성되어있는) chat 도메인 외부에 대한 공통 Module Service도 추가하겠습니다.
chat 도메인에서 기존 Repository의 findBy{Entity} 형태를, findBy{EntityId} 형태로 가능한 메소드들에 한해 수정했습니다.
entity 자체를 넘겨줘도 JPA 내부적으로 id를 찾아서 비교하는 것으로 알고 있어, 오직 Repository의 메소드로 넘기기만을 위한
불필요한 DB 조회를 최소화하고자했습니다.
테스트는 추후 다른 브랜치에서 추가할 예정이라 일단 주석처리해놓았습니다.

📸 확인 방법 (선택)

📌 PR 진행 시 이러한 점들을 참고해 주세요

* P1 : 꼭 반영해 주세요 (Request Changes) - 이슈가 발생하거나 취약점이 발견되는 케이스 등
* P2 : 반영을 적극적으로 고려해 주시면 좋을 것 같아요 (Comment)
* P3 : 이런 방법도 있을 것 같아요~ 등의 사소한 의견입니다 (Chore)

정적 팩토리 메소드 내에서 비즈니스 로직을 수행하는 경우에는 create, 값을 매핑만 하는 경우에는 of로 네이밍

- repository find 시 status 조건 미적용돼있던 부분 수정 - 코드 포매팅

seongjunnoh

일급 컬렉션, 함수형 인터페이스, 람다, 스트림 등등 여러 자바 기술들을 적극 활용하셨네요!
덕분에 이것저것 많이 찾아보면서 재밌게 리뷰할 수 있었습니다.

처음에 서현님과 얘기하면서 component service와 module service를 분리해보자 라고 생각했을 때는 뭔가 component 서비스에는 repo 의존성이 없는 순수한 도메인 로직과 관련된 코드만이 존재하여 훨씬 깔끔해지겠다라고 생각했었는데,
여전히 의존성의 방향이 component 서비스 -> module 서비스 -> repo 이고, component 서비스에서도 영속성 계층의 모델인 jpa entity를 사용하니 비즈니스 로직만을 독립적으로 분리하였다고 보기 어려운거 같기는 하네요

뭔가 서비스 레이어를 sub 계층 2개로 나누었을때의 장/단점, 그리고 대안책에 대해서 좀 더 고민해봐야할 것 같습니다

저희가 하는 고민들과 관련된 블로그 링크 몇개 남깁니다!
https://coldpresso.tistory.com/24
https://jinalim-dev.tistory.com/80

seongjunnoh · 2024-11-22T07:24:29Z

src/main/java/space/space_spring/domain/chat/chatroom/model/ChatRoom.java

+        this.space = space;
+        this.name = name;
+        this.img = img;
+    }


여기서 @nullable 어노테이션을 이용한 인자 유효성 검증을 위한 코드를 왜 추가하셨는지 궁금합니다!
ChatRoomController에서 HTTP request를 받는 입력 모델에서의 유효성 검증과정이 있고, 이 유효성 검증을 통과한 입력 모델이 서비스단으로 전달되고, 이 모델의 정보를 바탕으로 엔티티를 생성하는 로직인거 같은데
제 생각으로는 엔티티에서도 검증을 위한 코드가 있어야할까? 싶긴합니다

그리고 추가적으로 컨트롤러의 입력모델에서는 @NotNull 인데 엔티티에서는 @nullable 인 것 같습니다. 확인부탁드려요~~

아 채팅방 생성 시 이미지의 필수 여부가 개발 과정에 변경되었던 것 같아요.
초기에는 이미지가 필수가 아닌 것으로 설계해서 엔티티에 @Nullable로 설정했는데,
이미지도 필수인 것으로 바뀌면서 반영이 안되었네요.
엔티티의 img 필드가 @Nullable임에 따라 생성자에도 해당 어노테이션을 추가한 거라,
엔티티의 img 필드를 @NotNull로 변경하면 해결될 것 같습니다~

그럼 혹시 해당 엔티티의 생성시 bean validation 을 어겼을 경우에 대한 후처리 로직도 있을까요??

흐름상 RequestDto에서 validation 진행 후 controller에서 이에 대한 bindingResult가 올바른지 확인함으로써
API 요청을 통한 엔티티 생성 시 validatioin 처리가 되어있습니다!
말씀하시는 내용이 엔티티 자체적으로 validation에 대한 처리가 있는지에 대한 것일까요?

넵 엔티티 자체적으로 validation이 있어야하지 않을까? 라는 생각으로 답글을 단 것이었는데
다시 생각해보니 앞에서 이미 걸러진 친구들이기도하고, 혹시 여기서 문제가 발생할 경우에는 서버 내부 코드에서의 문제가 맞으니, 따로 validation 처리를 하는 것보다는 500 error 를 발생시키는것이 맞는건가? 싶네여
(뭔가 이 부분도 다같이 얘기해보면 좋을 거 같긴 합니다)

그러면 @NotNull 어노테이션을 controller에서 검증을 거친 인자에 대해서 엔티티 내부에서 한 번 더 안전장치를 거신 거라고 생각하면 될까요??

음 의도했던 바는 안전장치라기보다는 ChatRoom 엔티티 자체의 특성을 반영한 설정을 한 것입니당
결론적으로 안전장치의 역할도 가능하구요!

다시 생각해보니 앞에서 이미 걸러진 친구들이기도하고, 혹시 여기서 문제가 발생할 경우에는 서버 내부 코드에서의 문제가 맞으니, 따로 validation 처리를 하는 것보다는 500 error 를 발생시키는것이 맞는건가? 싶네여

이 부분은 repository를 통해 DB에 ChatRoom 엔티티를 저장할 때 null인 경우 500 error를 발생시키는 걸 말씀하시는건가요??

넵 맞습니다

아하 이중으로 에러 처리를 해줘도 괜찮을 것 같아요 다음에 다른 분들이랑 다같이 얘기해봅시다!

seongjunnoh · 2024-11-22T16:26:44Z

src/main/java/space/space_spring/domain/chat/chatroom/model/ChatRooms.java

+            Long userId,
+            Function<ChatRoom, LastMessageInfoDto> lastMessageFinder,
+            BiFunction<Long, ChatRoom, Integer> unreadMessageCounter) {
+        return chatRooms.stream()


오오 함수형 인터페이스, 람다표현식, 스트림 까지 고급 자바 기술들의 집합체네요.
특히 함수를 매개변수로 받아들임으로써 마지막 메시지를 찾는 로직, 읽지 않은 메시지를 계산하는 로직을 현재 toChatRoomResponses 메서드의 주요로직인 엔티티 -> dto 변환로직과 분리할 수 있네요 !!

코드를 보고 개인적으로 자바8에 대한 공부가 많이 필요하다고 느꼈습니다 하하

음 제안드리고 싶은 사안이 있는데, 혹시 return type을 List 에서 DTOs 일급 컬렉션으로 변경하는건 어떤가요??
toChatRoomResponses 메서드 내부에서 일급 컬렉션에게 chatRoom 엔티티 중 dto 구성에 필요한 정보를 보내고, 일급 컬렉션은 필드로 가지고 있는 List 컬렉션에 이 정보들을 계속 추가하고, 최종적으로 구성된 List 컬렉션을 다시 toChatRoomResponses 메서드가 받으면 이걸 return 하는 플로우가 가능할 것 같습니다!

이러면 toChatRoomResponses 메서드의 책임을 ChatRooms 가 가지고 있는 CharRoom 엔티티들 중 필요한 정보를 뽑아낸다 라는 것으로 국한시키고, 뽑아낸 정보들을 반환하는 것에 대한 책임을 일급컬렉션으로 위임할 수 있지않을까 싶습니다.
또한 외부에서 해당 dto list 에 접근하는 로직이 추가될떄에도 일급 컬렉션에게 책임을 맡기면 되므로 유용할 것 같습니다!

seongjunnoh · 2024-11-22T16:27:38Z

src/main/java/space/space_spring/domain/chat/chatroom/model/UserChatRoom.java

+        this.chatRoom = chatRoom;
+        this.user = user;
+        this.lastReadTime = lastReadTime;
+    }


이 부분도 ChatRoom 과 동일하게 엔티티에 유효성 검증하는 코드를 작성하신 이유가 궁금합니다!

위와 같은 이슈입니다. 이 부분도 수정하겠습니다!

seongjunnoh · 2024-11-22T16:28:31Z

src/main/java/space/space_spring/domain/chat/chatroom/model/UserChatRooms.java

+
+    private UserChatRooms(List<UserChatRoom> userChatRooms) {
+        this.userChatRooms = Collections.unmodifiableList(userChatRooms);
+    }


오호 UserChatRooms 일급컬렉션 생성 시에 관련 UserChatRoom 엔티티의 list를 모두 인자로 주입해주고, list 컬렉션을 읽기 전용으로 제한하셨군요!
현 시점에서의 특정 채팅방의 모든 유저 정보 조회 api 를 구현하기 위해서는 UserChatRoom 엔티티들의 불변성 보장이 중요해보이므로 일급 컬렉션 생성 시에 읽기전용으로 제한을 거는것은 좋은 것 같습니다!

seongjunnoh · 2024-11-22T16:43:13Z

src/main/java/space/space_spring/domain/chat/chatroom/model/UserChatRooms.java

+        return userChatRooms.stream()
+                .map(UserChatRoom::getUser)
+                .collect(Collectors.toUnmodifiableList());
+    }


seongjunnoh · 2024-11-22T16:50:11Z

src/main/java/space/space_spring/domain/chat/chatroom/service/component/ChatRoomService.java

+@Slf4j
+@RequiredArgsConstructor
+public class ChatRoomService {
+


서비스 클래스에 @transactional(readOnly = true) 를 걸어놓고,
서비스 클래스 중 query가 아니라 command 에 해당하는 메서드에 한해서 @transactional 을 걸어놓는 방식은 어떤가요??

오 좋은 것 같네요!
그럼 하위 module service에서의 transaction 설정은 삭제해도 괜찮은 건가요?

음 하위 module service에 @transactional 어노테이션을 설정하더라도 이걸 호출한 상위 component service에서의 트랜잭션과 합쳐지는 것으로 알고있어서(default 설정에서는) 놔두시거나 제거하시거나 크게 상관은 없을 것 같습니다!

관련 블로그 링크입니닷
https://mangkyu.tistory.com/269

seongjunnoh · 2024-11-22T16:56:17Z

src/main/java/space/space_spring/domain/chat/chatroom/service/component/ChatRoomService.java

+                                                 String chatRoomImgUrl) {
+        User chatRoomOwner = userUtils.findUserByUserId(userId);
+        Space space = spaceUtils.findSpaceBySpaceId(spaceId);
+


앗 이 부분은 현재 develop 브랜치에 반영되어있는 UserRepository, SpaceRepository 와 return type에 대한 null 체크로 변경해주시면 될 듯 합니다!

아 그런데 module service 도입을 고려하신다고 했으니
흠 같이 좀 더 고민해보입시다

seongjunnoh · 2024-11-22T16:59:04Z

src/main/java/space/space_spring/domain/chat/chatting/model/ChatMessages.java

+        return chatMessages.stream()
+                .map(ChatMessageResponse::create)
+                .collect(Collectors.toList());
+    }


seongjunnoh · 2024-11-22T17:00:39Z

src/main/java/space/space_spring/domain/chat/chatting/service/component/ChattingService.java

+    public ChatMessageResponse sendChatMessage(Long senderId, ChatMessageRequest chatMessageRequest, Long chatRoomId)
+            throws IOException {
+        UserSpace senderInSpace = userSpaceUtils.isUserInSpace(senderId, chatMessageRequest.getSpaceId())
+                .orElseThrow(() -> new CustomException(USER_IS_NOT_IN_SPACE));


utils 여기도 있네요 하하

seongjunnoh · 2024-11-22T17:04:41Z

src/main/java/space/space_spring/domain/chat/chatting/service/component/ChattingService.java

+            }
+            default -> {
+            }
+        }


음 현재는 img, file, 일반 채팅(text) 에 대해서만 채팅타입을 다루고 있지만, 확장성을 고려하여 이 부분을 객체화 & enum 화 하여 chattingService 와 책임 분리를 하면 좋을 것 같습니다!

(하지만 레거시 기능이라고 하니 이러면 좀 더 좋을거같다~~ 라고만 생각하셔도 되지 않을까 싶습니다)

네 저도 같은 생각입니다ㅜㅜ
content를 그냥 Map으로 넣어놔서 저런 사태가 발생했는데.. 여유가 된다면 이부분도 나중에 리팩토링해보겠습니다!

hyunn522 added 16 commits November 7, 2024 21:43

refactor: chatroom 및 chatting 도메인 구조로 변경

c6f25fd

refactor: UserChatRoom 및 ChatMessage 디렉토리 위치 변경

88e15b7

refactor: chat domain의 model 생성 방식 변경

81d9097

정적 팩토리 메소드 내에서 비즈니스 로직을 수행하는 경우에는 create, 값을 매핑만 하는 경우에는 of로 네이밍

refactor: ChatService 메소드 분리

443a2af

refactor: 매직넘버 상수화 및 메소드명 변경

f2295bd

refactor: 불필요한 주석 삭제

f4e05e4

refactor: ChatMessage의 status 타입을 enum으로 변경

9be06d2

refactor: ChattingRepository의 메소드에 status 조건 추가

8d57a15

refactor: 가독성 및 확장성 향상을 위한 네이밍 및 로직 수정

1f239be

refactor: Chatting 도메인에서 일급컬렉션 래핑 적용

800e0ad

refactor: ChatRoom 도메인에서 일급컬렉션 래핑 적용

c1edef3

- repository find 시 status 조건 미적용돼있던 부분 수정 - 코드 포매팅

refactor: ChatRoom, Chatting 도메인에 module service 도입

2d64406

refactor: ChattingService에서 불필요한 변수 제거 및 가독성 향상

06a4167

refactor: ChatRoomService 메소드 분리

757d0db

refactor: Builder에 대한 AccessLevel 제한

234ab5f

refactor: 테스트 코드 임의로 주석 처리

de4ee39

hyunn522 self-assigned this Nov 21, 2024

refactor: 엔티티의 일급컬렉션 클래스 디렉토리 위치 변경

ebf1898

seongjunnoh reviewed Nov 22, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor/#165: 채팅 메시지 도메인 리팩토링 #171

Refactor/#165: 채팅 메시지 도메인 리팩토링 #171

hyunn522 commented Nov 21, 2024 •

edited

Loading

seongjunnoh left a comment

seongjunnoh Nov 22, 2024

hyunn522 Nov 27, 2024

seongjunnoh Nov 27, 2024

hyunn522 Nov 28, 2024

seongjunnoh Nov 28, 2024

hyunn522 Nov 29, 2024 •

edited

Loading

seongjunnoh Nov 29, 2024

hyunn522 Nov 30, 2024

seongjunnoh Nov 22, 2024

seongjunnoh Nov 22, 2024

hyunn522 Nov 27, 2024

seongjunnoh Nov 22, 2024

seongjunnoh Nov 22, 2024

seongjunnoh Nov 22, 2024

hyunn522 Nov 27, 2024

seongjunnoh Nov 27, 2024

seongjunnoh Nov 22, 2024

seongjunnoh Nov 22, 2024

seongjunnoh Nov 22, 2024

seongjunnoh Nov 22, 2024

hyunn522 Nov 27, 2024

Refactor/#165: 채팅 메시지 도메인 리팩토링 #171

Are you sure you want to change the base?

Refactor/#165: 채팅 메시지 도메인 리팩토링 #171

Conversation

hyunn522 commented Nov 21, 2024 • edited Loading

📝 요약

🔖 변경 사항

✅ 리뷰 요구사항

📸 확인 방법 (선택)

📌 PR 진행 시 이러한 점들을 참고해 주세요

seongjunnoh 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

hyunn522 Nov 29, 2024 • 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

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

hyunn522 commented Nov 21, 2024 •

edited

Loading

hyunn522 Nov 29, 2024 •

edited

Loading