안 읽은 메시지 카운팅 기능 구현 #709

swonny · 2023-10-17T16:01:39Z

📄 작업 내용 요약

안 읽은 메시지 카운팅 기능 구현

🙋🏻 리뷰 시 주의 깊게 확인해야 하는 코드

일과 시간 중 설명드렸듯이 쿼리문이 조금 복잡합니다.
채팅방을 생성할 때 채팅방에 참여하는 판매자와 구매자에 대한 메시지 조회 로그(ReadMessageLog)를 생성합니다.
메시지 조회 시 메시지 조회 로그에 마지막 조회 메시지를 업데이트합니다.
메시지 조회와 메시지 조회 로그를 업데이트 하는 로직,
그리고 채팅방 생성 시 메시지 조회 로그를 초기화 하는 로직은 별개의 비즈니스 로직이라고 생각했습니다.
따라서 메시지 로그 저장 및 관리하는 로직은 이벤트로 분리했고, 이벤트를 처리하는 서비스 계층을 생성했습니다.

📎 Issue 번호

closed 읽지 않은 메세지 카운팅 기능 #677

apptie

고생하셨습니다
필수가 두 개 있어서 RC로 하겠습니다

apptie · 2023-10-17T22:42:00Z

...nd/ddang/src/main/java/com/ddang/ddang/chat/application/LastReadMessageLogEventListener.java

+
+@Component
+@RequiredArgsConstructor
+@Slf4j


필수

해당 클래스에서는 로그를 찍지 않아 @Slf4j는 삭제해야 할 것 같습니다

apptie · 2023-10-18T01:00:53Z

backend/ddang/src/main/java/com/ddang/ddang/chat/application/ChatRoomService.java

+ final Optional<ChatRoom> findChatRoom = chatRoomRepository.findChatRoomByAuctionId(findAuction.getId());
+ if (findChatRoom.isPresent()) {
+ return findChatRoom.get().getId();
+ }
+
+ return createChatRoom(findUser, findAuction);
+ }


필수

Suggested change

final Optional<ChatRoom> findChatRoom = chatRoomRepository.findChatRoomByAuctionId(findAuction.getId());

if (findChatRoom.isPresent()) {

return findChatRoom.get().getId();

}

return createChatRoom(findUser, findAuction);

}

return chatRoomRepository.findChatRoomByAuctionId(findAuction.getId())

.map(ChatRoom::getId)

.orElseGet(() -> createChatRoom(findUser, findAuction));

apptie · 2023-10-18T01:01:22Z

backend/ddang/src/main/java/com/ddang/ddang/chat/application/MessageService.java

@@ -77,6 +78,11 @@ public List<ReadMessageDto> readAllByLastMessageId(final ReadMessageRequest requ
 request.lastMessageId()
 );

+ if (readMessages.size() > 0) {
+ final Message lastReadMessage = readMessages.get(readMessages.size() - 1);
+ messageLogEventPublisher.publishEvent(new UpdateReadMessageLogEvent(reader, chatRoom, lastReadMessage));


선택

개행..?

apptie · 2023-10-18T01:01:57Z

...ang/src/main/java/com/ddang/ddang/chat/infrastructure/persistence/JpaChatRoomRepository.java

+ JOIN FETCH c.buyer
+ JOIN FETCH c.auction.seller


칭찬

👍

apptie · 2023-10-18T01:02:49Z

...dang/ddang/chat/infrastructure/persistence/QuerydslChatRoomAndMessageAndImageRepository.java

+ queryFactory.select(new QChatRoomAndMessageAndImageQueryProjectionDto(
+ chatRoom,
+ message,
+ auctionImage,
+ countUnreadMessages(userId)


칭찬

믿겠습니다....메리...

# Conflicts: # backend/ddang/src/main/resources/static/docs/docs.html

apptie

제가 남긴 리뷰가 적용된 것으로 보여 Approve 하도록 하겠습니다

kwonyj1022

필수가 있어서 rc 남기겠습니다.

kwonyj1022 · 2023-10-31T14:05:13Z

backend/ddang/src/main/java/com/ddang/ddang/chat/application/LastReadMessageLogService.java

+ public void update(final UpdateReadMessageLogEvent createReadMessageLogEvent) {
+ final User reader = createReadMessageLogEvent.reader();
+ final ChatRoom chatRoom = createReadMessageLogEvent.chatRoom();
+ final ReadMessageLog messageLog = readMessageLogRepository.findBy(reader.getId(), chatRoom.getId())
+ .orElseThrow(() ->
+ new ReadMessageLogNotFoundException(
+ "메시지 조회 로그가 존재하지 않습니다."
+ ));
+ messageLog.updateLastReadMessage(createReadMessageLogEvent.lastReadMessage().getId());
+ }


필수

변수명 바꿔주시면 좋을 것 같습니다.

Suggested change

public void update(final UpdateReadMessageLogEvent createReadMessageLogEvent) {

final User reader = createReadMessageLogEvent.reader();

final ChatRoom chatRoom = createReadMessageLogEvent.chatRoom();

final ReadMessageLog messageLog = readMessageLogRepository.findBy(reader.getId(), chatRoom.getId())

.orElseThrow(() ->

new ReadMessageLogNotFoundException(

"메시지 조회 로그가 존재하지 않습니다."

));

messageLog.updateLastReadMessage(createReadMessageLogEvent.lastReadMessage().getId());

}

public void update(final UpdateReadMessageLogEvent updateReadMessageLogEvent) {

final User reader = updateReadMessageLogEvent.reader();

final ChatRoom chatRoom = updateReadMessageLogEvent.chatRoom();

final ReadMessageLog messageLog = readMessageLogRepository.findBy(reader.getId(), chatRoom.getId())

.orElseThrow(() ->

new ReadMessageLogNotFoundException(

"메시지 조회 로그가 존재하지 않습니다."

));

messageLog.updateLastReadMessage(updateReadMessageLogEvent.lastReadMessage().getId());

}

kwonyj1022 · 2023-10-31T14:11:56Z

backend/ddang/src/main/java/com/ddang/ddang/chat/application/ChatRoomService.java

- return chatRoomRepository.findChatRoomIdByAuctionId(findAuction.getId())
- .orElseGet(() ->
- persistChatRoom(findUser, findAuction).getId()
- );
+ return chatRoomRepository.findChatRoomByAuctionId(findAuction.getId())
+ .map(ChatRoom::getId)
+ .orElseGet(() -> createChatRoom(findUser, findAuction));


질문

id만 반환하는데 findChatRoomId~ 에서 findChatRoom~ 으로 메서드를 바꾸고 getId를 하는 걸로 바꾼 이유가 궁금합니다.

다시 고민해봤는데,
메시지 읽음 처리를 위한 과정에서 ChatRoom을 조회할 때 auction.seller와 buyer를 join해야된다고 생각했던 것 같습니다
그래서 ChatRoomId만이 아닌 ChatRoom에 Seller와 Auction과 buyer가 모두 존재하는 chatRoom 객체를 생성하기 위한 메서드를 작성했다가, 나중에 리팩토링 하는 과정에서 ChatRoom 조회 시 join하지 않고도 기존에 조회했던 findUser와 findAuction을 사용하는 방식으로 바꾼 잔재인 것 같습니다 ㅎㅎ..

아무튼 findChatRoomByAuctionId는 더이상 필요하지 않은 메서드라 판단해 메서드도 지워주었습니다

kwonyj1022 · 2023-10-31T14:17:26Z

...nd/ddang/src/main/java/com/ddang/ddang/chat/application/LastReadMessageLogEventListener.java

+@Component
+@RequiredArgsConstructor
+public class LastReadMessageLogEventListener {
+
+ private final LastReadMessageLogService lastReadMessageLogService;
+
+ @EventListener
+ @Transactional
+ public void create(final CreateReadMessageLogEvent createReadMessageLogEvent) {
+ lastReadMessageLogService.create(createReadMessageLogEvent);
+ }


질문

이벤트 리스너가 서비스를 가지고 서비스 메서드를 호출하도록 한 이유가 궁금합니다. 이벤트 리스너 안에 서비스 코드를 그냥 넣으면 안되나요?
또 현재 해당 이벤트를 발행하는 ChatRoomService의 메서드에도 @Transactional이 붙어있고 이 이벤트 리스너에도 @Transactional이 붙어있고, 이벤트 리스너가 실행하는 LastReadMessageLogService의 메서드에도 @Transactional이 붙어있는데 이러면 어떻게 되나요? 의도하신 상황인건가요?

lastReadMessagLogService 하위 메서드에서 @Transactional을 붙여준 이유는 readOnly가 false인 트랜잭션을 열어주어야 하기 때문입니다. 그리고 이벤트 리스너에서 그리고 이벤트리스너에서 @Transactional을 붙여준 이유는 원래는 트랜잭션 분리를 하기 위함이었습니다.

그래서 @Transactional에 propagation으로 새로운 트랜잭션을 열어주고 @TransactionalEventListener를 사용해 기존 트랜잭션이 커밋된 이후에 메시지 로그가 저장되도록 변경해주었습니다.

질문

그렇다면 해당 클래스에서는 @TransactionalEventListener로 변경되야 하는 데 아직 수정되지 않은 것이 맞을까요?
추가적으로 LastReadMessageLogService에 붙어있는 propagation 설정이 있는 @Transactional을 여기에 붙이지 않고 LastReadMessageLogService에 붙여야 하는 이유도 궁금합니다.

그렇다면 해당 클래스에서는 @TransactionalEventListener로 변경되야 하는 데 아직 수정되지 않은 것이 맞을까요?

네 맞습니다. 이 부분은 말씀해주신 것처럼 @TransactionalEventListener를 붙여야하는 상황입니다! 그래서 리팩토링을 진행하면서 해당 어노테이션을 붙여주었습니다.

LastReadMessageLogService에 붙어있는 propagation 설정이 있는 @transactional을 여기에 붙이지 않고 LastReadMessageLogService에 붙여야 하는 이유도 궁금합니다.

지금 LastReadMessageLogService에는 propagation 설정이 안 되어 있는 것 같은데 아닌가요?! 아무튼 새로운 트랜잭션을 열어주는 propagation이 붙은 @Transactional 어노테이션은 리팩토링 과정에서 이벤트 리스너에 달아주었습니다. 하나의 이벤트에서 여러 서비스를 호출하게 될 확장성을 고려했을 때 새로운 트랜잭션을 열어주기 위해 하나의 이벤트 리스너에서 호출하는 모든 메서드 하위에 propagation 설정을 하는 것보다 이벤트 리스너에 달아주는 것이 더 코드 변경을 최소화 할 수 있다고 생각했기 때문입니다!

이벤트 리스너가 서비스를 가지고 서비스 메서드를 호출하도록 한 이유가 궁금합니다. 이벤트 리스너 안에 서비스 코드를 그냥 넣으면 안되나요?

이벤트 리스너의 역할을 명확히 해주기 위해서였습니다.
이벤트 리스너의 역할을 특정 사건이 일어났을 때 수행되어야 하는 로직을 매핑해주고 이벤트를 관리하는 역할이라고 생각했습니다.
이와 관련해 이벤트리스너를 호출한 로직과 트랜잭션을 끊어주고, 이벤트리스너에서 수행하고자 하는 메서드에서 예외가 발생했을 때 해당 예외를 잡아주는 역할도 하고 있습니다. 그렇기 떄문에 propagation이 포함된 @Transactional도 이벤트 리스너에 추가하게 되었습니다.
이런 작업들은 이벤트 발행과 관련된 로직을 수행할 때 필요한 작업들이라고 생각했고, 이외에 LastReadMessageLogService의 메서드가 하는 역할까지 이벤트리스너로 합쳐진다면 이벤트 리스너의 역할이 너무 커질 것 같다고 생각했습니다.

마찬가지로 이벤트 리스너의 역할까지 LastReadMessageLogService에서 담당한다면 추후에 이벤트 리스너가 아닌 다른 곳에서 사용할 때 예외가 발생해야하는 상황에서 예외가 발생하지 않아 오히려 헷갈릴 수도 있다고 생각했습니다.

그런데 이 질문을 받고 주변 크루들한테 물어봤는데, 엔초와 비슷하게 이벤트 리스너에서만 사용될 서비스이기 때문에 이벤트 리스너에서 수행하도록 하게 한다는 의견도 있더라구요. 제가 설명한 것과 같이 설명했을 때 납득은 됐지만 설득은 아직 안됐다고... 했었는데 엔초의 의견도 궁금합니다.

kwonyj1022 · 2023-10-31T14:28:37Z

backend/ddang/src/test/java/com/ddang/ddang/chat/application/ChatRoomServiceTest.java

+ @Autowired
+ MessageService messageService;


필수

삭제 부탁드립니다.

반영했습니다

JJ503

메리 정말 고생 많으셨습니다..!!
수정 사항은 아니고 디스커션을 봤지만, 제가 아직 이벤트를 완전히 이해하지 못해서인지 궁금한 점이 있어 코멘트 남겨두었습니다.
해당 pr이 머지되기 전 확실히 이해해두는 것이 좋을 것 같아 일단 rc 후 답변해주시면 바로 approve로 변경할 수 있도록 하겠습니다.

JJ503 · 2023-11-01T15:07:28Z

backend/ddang/src/main/java/com/ddang/ddang/chat/application/MessageService.java

+ @Transactional
 public List<ReadMessageDto> readAllByLastMessageId(final ReadMessageRequest request) {


질문

해당 메서드에서 @Transactional을 사용한 이유가 궁금합니다.
현재 메서드에서는 이벤트와 관련된 부분 외에는 쓰기 작업은 일어나지 않는 것으로 알고 있습니다.
그리고 이벤트에서는 트랜잭션이 분리되어 있기에 readOnly=true가 적용되지 않을 것이라 안일하게 추측하고 있는데, 제 추측이 잘못된 것인지도 궁금합니다!

해당 작업을 진행할 때는 messageService와 이벤트 작업이 하나의 트랜잭션으로 수행되어야 한다고 생각했습니다. 그래서 이벤트 리스너에서 레포지토리 저장 로직에서 쓰기 작업이 발생하기 때문에 여기에서 새롭게@Transactional 어노테이션을 사용했던 것 같아요!
그런데 말씀하신 것처럼 이 부분은 트랜잭션을 분리하면서 필요가 없어져서 제거해주었습니다.

JJ503 · 2023-11-01T15:15:58Z

...nd/ddang/src/main/java/com/ddang/ddang/chat/application/LastReadMessageLogEventListener.java

+@Component
+@RequiredArgsConstructor
+public class LastReadMessageLogEventListener {
+
+ private final LastReadMessageLogService lastReadMessageLogService;
+
+ @EventListener
+ @Transactional
+ public void create(final CreateReadMessageLogEvent createReadMessageLogEvent) {
+ lastReadMessageLogService.create(createReadMessageLogEvent);
+ }


질문

그렇다면 해당 클래스에서는 @TransactionalEventListener로 변경되야 하는 데 아직 수정되지 않은 것이 맞을까요?
추가적으로 LastReadMessageLogService에 붙어있는 propagation 설정이 있는 @Transactional을 여기에 붙이지 않고 LastReadMessageLogService에 붙여야 하는 이유도 궁금합니다.

JJ503 · 2023-11-01T15:24:00Z

backend/ddang/src/test/java/com/ddang/ddang/chat/application/LastReadMessageLogServiceTest.java

+
+ @Test
+ void 메시지_로그를_생성한다() {
+ // given & when


질문

보통 이런 경우 given을 뺐던 것 같은데 함께 적어준 이유가 궁금합니다!

create를 호출하는 부분이 given에 해당한다고 생각해서 이렇게 했던 것 같습니다. given이 있는 게 부자연스러운가요?!

그렇다면 그냥 given과 when으로 나눠도 되지 않나요?
create가 given이자 when인 것인지 given인 건지 헷갈리네요..!

JJ503 · 2023-11-01T15:26:02Z

backend/ddang/src/test/java/com/ddang/ddang/chat/application/fixture/MessageServiceFixture.java

+ 발신자 = User.builder()
 .name("발신자")
 .profileImage(new ProfileImage("upload.png", "store.png"))


선택

fixture는 마음대로 하기로 하긴 했지만, 그냥 본 김에 자동 정렬해도 좋을 것 같아 코멘트만 달아둡니다!

# Conflicts: # backend/ddang/src/main/resources/static/docs/docs.html

JJ503

메리 질문에 대한 답변들 잘 봤습니다!
그런데 flyway 스크립트 관련해 필수 수정 사항이 있어 rc 유지합니다.
수정되면 바로 approve로 수정하겠습니다~!

JJ503 · 2023-11-05T05:38:38Z

backend/ddang/src/main/resources/db/migration/V21__create_read_message_log_tables.sql

+ primary key (id)
+);
+
+alter table read_message_log add constraint fk_read_message_log_chat_room foreign key (chat_room_id) references chatRoom (id);


필수

flyway 스크립트 검증 로직이 드디어 제 역할을 해주었네요!!!! 😀
chatRoom을 chat_room으로 수정해줘야 할 것 같아요!

으악 너무너무너무너무 감사합니다 제이미 😭

JJ503

수정사항과 답변들 잘 확인했습니다!
고생 많으셨어요 메리~~

swonny added 18 commits October 17, 2023 00:21

feat: 읽은 메시지 로그 도메인 생성

96b54d0

feat: 읽은 메시지 로그 저장 레포지토리 생성

aabc0ef

refactor: 컬럼명 변경

de9e5e9

feat: 메시지 조회 시 마지막으로 읽은 메시지 로그 저장

c773023

feat: 채팅방 목록 조회 시 읽지 않은 메시지 개수를 포함하는 기능 추가

ce34598

feat: 채팅방 생성 시 참여자에 대한 메시지 로그 생성하는 기능 추가

d90c03c

feat: 경매 아이디로 채팅방 조회하는 기능 추가

3f8d431

feat: 채팅방 목록 조회 시 반환값에 안 읽은 메시지 개수 추가

e0ef4e8

refactor: 메시지 로그 조회 네이밍 변경

5e22986

feat: 어노테이션 추가

b14e7ab

feat: 로그 찾지 못한 경우에 대한 커스텀 예외 추가

049fa04

refactor: 로그 생성 로직 이벤트로 분리

0e72381

refactor: 불필요한 메서드 삭제

0b1e25c

test: 생략한 테스트 추가

066570b

refactor: 채팅방과 메시지 조회 로그cascade type 지정

b4d03e3

refactor: 불필요한 파라미터 삭제

e7993c6

Merge remote-tracking branch 'origin/develop-be' into feature/677

6710648

feat: flyway 스크립트 작성

824d5c2

swonny added backend 백엔드와 관련된 이슈나 PR에 사용 feature 기능 추가 시 labels Oct 17, 2023

swonny added this to the 최종 데모데이 milestone Oct 17, 2023

swonny requested review from apptie, JJ503 and kwonyj1022 October 17, 2023 16:01

swonny self-assigned this Oct 17, 2023

apptie requested changes Oct 18, 2023

View reviewed changes

swonny added 4 commits October 31, 2023 16:52

refactor: 불필요한 어노테이션 삭제

a1e3309

refactor: 개행 추가 및 분기문 스트림으로 대체

fbaf37d

feat: 안 읽은 메시지 개수 컨트롤러 업데이트

35e3b8a

Merge remote-tracking branch 'origin/develop-be' into feature/677

8cc690a

# Conflicts: # backend/ddang/src/main/resources/static/docs/docs.html

apptie approved these changes Oct 31, 2023

View reviewed changes

kwonyj1022 requested changes Oct 31, 2023

View reviewed changes

swonny added 3 commits November 1, 2023 16:00

refactor: 불필요한 필드 삭제

83aa31c

refactor: 불필요한 join이 발생하는 쿼리 삭제

688d8e2

refactor: 불필요한 import문 삭제

591ccd5

JJ503 requested changes Nov 1, 2023

View reviewed changes

swonny added 6 commits November 3, 2023 20:25

refactor: 메시지 전송과 읽음 저장 트랜잭션 분리

3019893

refactor: 불필요한 어노테이션 삭제, 자동 정렬

3bec36d

test: 예외 케이스 테스트 추가

7102287

Merge remote-tracking branch 'origin/develop-be' into feature/677

fb80954

# Conflicts: # backend/ddang/src/main/resources/static/docs/docs.html

refactor: 업데이트에 적절한 변수명으로 변경

dbad4b3

refactor: 레포지토리 저장 시 여러 개 저장하는 메서드 생성

461d34d

JJ503 reviewed Nov 5, 2023

View reviewed changes

chore: 잘못된 flyway 스크림트 수정

c299d27

JJ503 approved these changes Nov 5, 2023

View reviewed changes

kwonyj1022 approved these changes Nov 6, 2023

View reviewed changes

swonny merged commit 4d4c3f7 into develop-be Nov 6, 2023
2 checks passed

swonny deleted the feature/677 branch November 6, 2023 04:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

안 읽은 메시지 카운팅 기능 구현 #709

안 읽은 메시지 카운팅 기능 구현 #709

swonny commented Oct 17, 2023

apptie left a comment

apptie Oct 17, 2023

apptie Oct 18, 2023

apptie Oct 18, 2023

apptie Oct 18, 2023

apptie Oct 18, 2023

apptie left a comment

kwonyj1022 left a comment

kwonyj1022 Oct 31, 2023

kwonyj1022 Oct 31, 2023

swonny Nov 1, 2023

kwonyj1022 Oct 31, 2023

swonny Nov 1, 2023 •

edited

Loading

JJ503 Nov 1, 2023

swonny Nov 3, 2023 •

edited

Loading

swonny Nov 4, 2023

kwonyj1022 Oct 31, 2023

swonny Nov 1, 2023

JJ503 left a comment •

edited

Loading

JJ503 Nov 1, 2023

swonny Nov 3, 2023 •

edited

Loading

JJ503 Nov 1, 2023

JJ503 Nov 1, 2023

swonny Nov 3, 2023

JJ503 Nov 5, 2023

JJ503 Nov 1, 2023

JJ503 left a comment

JJ503 Nov 5, 2023

swonny Nov 5, 2023

JJ503 left a comment

		@Transactional
		public List<ReadMessageDto> readAllByLastMessageId(final ReadMessageRequest request) {

안 읽은 메시지 카운팅 기능 구현 #709

안 읽은 메시지 카운팅 기능 구현 #709

Conversation

swonny commented Oct 17, 2023

📄 작업 내용 요약

🙋🏻 리뷰 시 주의 깊게 확인해야 하는 코드

📎 Issue 번호

apptie 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

칭찬

apptie left a comment

Choose a reason for hiding this comment

kwonyj1022 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

질문

swonny Nov 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

질문

swonny Nov 3, 2023 • 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

JJ503 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

질문

swonny Nov 3, 2023 • 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

선택

JJ503 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

필수

Choose a reason for hiding this comment

JJ503 left a comment

Choose a reason for hiding this comment

swonny Nov 1, 2023 •

edited

Loading

swonny Nov 3, 2023 •

edited

Loading

JJ503 left a comment •

edited

Loading

swonny Nov 3, 2023 •

edited

Loading