feat: 채팅 기능 구현 #408
feat: 채팅 기능 구현 #408
Conversation
- n + 1 해결 필요
Gyuhyeok99
requested review from
lsy1307,
nayonsoso,
whqtker and
wibaek
as code owners
Walkthrough
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes
Suggested reviewers
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
♻️ Duplicate comments (1)
src/test/java/com/example/solidconnection/chat/fixture/ChatReadStatusFixtureBuilder.java (1)
17-19: ChatRoomFixtureBuilder와 동일한 상태 관리 문제앞서
ChatRoomFixtureBuilder에서 지적한 것과 같은 문제가 있습니다. 새로운 인스턴스 생성 시 현재 상태가 복사되지 않습니다.
🧹 Nitpick comments (13)
src/main/java/com/example/solidconnection/chat/domain/ChatParticipant.java (1)
38-44: 양방향 연관관계 생성자 구현이 잘 되어 있습니다! 🎯
- 필드 초기화: siteUserId와 chatRoom 필드가 올바르게 설정됩니다
- 양방향 연관관계: chatRoom이 null이 아닐 때 자동으로 participants 컬렉션에 추가됩니다
- 널 안전성: chatRoom null 체크로 NPE를 방지합니다
다만 몇 가지 개선사항을 제안드립니다:
public ChatParticipant(long siteUserId, ChatRoom chatRoom) { + if (siteUserId <= 0) { + throw new IllegalArgumentException("siteUserId must be positive"); + } this.siteUserId = siteUserId; this.chatRoom = chatRoom; if (chatRoom != null) { chatRoom.getChatParticipants().add(this); } }src/main/java/com/example/solidconnection/chat/dto/ChatRoomListResponse.java (1)
5-12: 깔끔한 DTO 레코드 구현입니다! 📦
- 불변 구조: record를 활용한 깔끔한 불변 객체 설계
- 팩토리 메서드:
of()메서드로 명확한 생성 의도 표현- 명확한 네이밍:
ChatRoomListResponse로 용도가 분명함현재 구현이 완벽하게 작동하지만, 향후 확장성을 고려한다면:
public static ChatRoomListResponse of(List<ChatRoomResponse> chatRooms) { + if (chatRooms == null) { + return new ChatRoomListResponse(List.of()); + } return new ChatRoomListResponse(chatRooms); }src/main/java/com/example/solidconnection/chat/domain/ChatAttachment.java (1)
36-44: 일관된 양방향 연관관계 패턴으로 잘 구현되었습니다! 🔗
- 필드 초기화: 모든 매개변수가 올바르게 필드에 할당됩니다
- 양방향 연관관계: chatMessage와의 연관관계를 양쪽에서 유지합니다
- 널 안전성: chatMessage null 체크로 안전성을 확보합니다
- 일관성: ChatParticipant와 동일한 패턴을 따릅니다
데이터 무결성을 위한 개선 제안:
public ChatAttachment(boolean isImage, String url, String thumbnailUrl, ChatMessage chatMessage) { + if (url == null || url.trim().isEmpty()) { + throw new IllegalArgumentException("url cannot be null or empty"); + } this.isImage = isImage; this.url = url; this.thumbnailUrl = thumbnailUrl; this.chatMessage = chatMessage; if (chatMessage != null) { chatMessage.getChatAttachments().add(this); } }src/main/java/com/example/solidconnection/chat/dto/ChatAttachmentResponse.java (1)
1-17: DTO 레코드 구현이 깔끔합니다! 📎
- 레코드 활용: 불변 DTO로서 적절한 Java Record 사용
- 필드 구성: 첨부파일 정보의 핵심 요소들을 포함
- 타임존 처리: ZonedDateTime 사용으로 시간대 정보 보존
다만 정적 팩토리 메서드가 단순히 생성자를 호출하는 것 같은데, Record의 기본 생성자로도 충분할 것 같습니다. 특별한 로직이 필요하지 않다면 생략을 고려해보세요.
src/main/java/com/example/solidconnection/chat/dto/ChatRoomResponse.java (1)
5-22: 채팅방 응답 DTO가 깔끔하게 설계되었습니다
- 불변 레코드 구조: 데이터 안정성 보장
- 정적 팩토리 메서드: 편리한 인스턴스 생성
- 적절한 데이터 타입: ZonedDateTime 사용으로 시간대 정보 포함
필드명 일관성을 위해 다음과 같은 수정을 고려해보세요:
- long unReadCount + long unreadCount
unReadCount보다는unreadCount가 일반적인 camelCase 네이밍 컨벤션에 더 부합합니다.src/main/java/com/example/solidconnection/chat/repository/ChatReadStatusRepository.java (1)
17-17: 메서드 시그니처 검토파라미터 타입이 primitive
long으로 되어 있어 null 체크가 불가능합니다. 유효성 검증이 필요한 경우를 고려해보세요.src/test/java/com/example/solidconnection/chat/fixture/ChatRoomFixtureBuilder.java (1)
14-14: 기본값 설정 고려
isGroup필드에 기본값이 설정되지 않아false가 기본값으로 사용됩니다. 명시적으로 기본값을 설정하는 것을 고려해보세요.- private boolean isGroup; + private boolean isGroup = false;src/test/java/com/example/solidconnection/chat/fixture/ChatParticipantFixtureBuilder.java (1)
18-20: 빌더 인스턴스 생성 패턴 개선 필요
chatParticipant()메서드가 매번 새로운 빌더 인스턴스를 생성하는데, 이미 Spring DI로 주입받은 빌더를 사용하는 구조에서는 불필요해 보입니다.다음과 같이 정적 팩토리 메서드로 변경하거나 제거하는 것을 고려해보세요:
- public ChatParticipantFixtureBuilder chatParticipant() { - return new ChatParticipantFixtureBuilder(chatParticipantRepository); - } + public static ChatParticipantFixtureBuilder chatParticipant() { + // 정적 컨텍스트에서는 repository 주입이 어려우므로 + // 이 메서드를 제거하고 직접 빌더를 주입받아 사용하는 것을 권장 + throw new UnsupportedOperationException("Use injected builder instance instead"); + }src/test/java/com/example/solidconnection/chat/fixture/ChatAttachmentFixtureBuilder.java (1)
20-22: 빌더 인스턴스 생성 패턴 일관성 필요
ChatParticipantFixtureBuilder와 동일한 패턴을 사용하고 있습니다. 전체 테스트 픽스처 빌더에서 일관된 접근 방식을 적용하는 것이 좋겠습니다.src/test/java/com/example/solidconnection/chat/service/ChatServiceTest.java (2)
210-210: 매직 넘버 상수 위치 개선 제안
NO_NEXT_PAGE_NUMBER상수가 테스트 클래스 내부에 정의되어 있는데, 이는 실제 비즈니스 로직에서도 사용되는 값으로 보입니다.
SliceResponse클래스나 공통 상수 클래스로 이동하는 것을 고려해보세요:// SliceResponse.java public record SliceResponse<T>(...) { public static final int NO_NEXT_PAGE = -1; // ... }
280-282: 테스트 데이터 생성 가독성 개선반복문을 통한 테스트 데이터 생성이 동작은 하지만, 의도를 더 명확하게 표현할 수 있습니다.
- for (int i = 1; i <= 25; i++) { - chatMessageFixture.메시지("메시지" + i, (i % 2 == 0) ? user.getId() : mentor1.getId(), chatRoom); - } + // 페이징 테스트를 위해 25개의 메시지 생성 (사용자와 멘토가 번갈아가며) + for (int i = 1; i <= 25; i++) { + long senderId = (i % 2 == 0) ? user.getId() : mentor1.getId(); + chatMessageFixture.메시지("메시지" + i, senderId, chatRoom); + }src/main/java/com/example/solidconnection/chat/repository/ChatRoomRepository.java (1)
24-36: 복잡한 서브쿼리 성능 검토 필요읽지 않은 메시지 카운트 쿼리에서 참여자 ID를 찾기 위한 서브쿼리가 매 row마다 실행될 수 있습니다.
성능이 문제가 된다면, 두 단계로 나누어 처리하는 것을 고려해보세요:
- 먼저 사용자의 participant ID를 조회
- 해당 ID로 읽지 않은 메시지 카운트 조회
// 또는 Native Query를 사용하여 더 효율적인 쿼리 작성 고려 @Query(value = """ SELECT COUNT(*) FROM chat_message cm WHERE cm.chat_room_id = :chatRoomId AND cm.sender_id != :userId AND cm.created_at > COALESCE( (SELECT crs.updated_at FROM chat_read_status crs JOIN chat_participant cp ON crs.chat_participant_id = cp.id WHERE cp.chat_room_id = :chatRoomId AND cp.site_user_id = :userId), '1970-01-01' ) """, nativeQuery = true)src/main/java/com/example/solidconnection/chat/service/ChatService.java (1)
122-130: 중복 쿼리 최적화 가능현재 구현에서 참가자 확인을 위해 두 번의 쿼리가 실행됩니다:
validateChatRoomParticipant에서existsByChatRoomIdAndSiteUserId호출- 바로 다음에
findByChatRoomIdAndSiteUserId호출다음과 같이 최적화할 수 있습니다:
-public void markChatMessagesAsRead(long siteUserId, long roomId) { - validateChatRoomParticipant(siteUserId, roomId); - - ChatParticipant participant = chatParticipantRepository.findByChatRoomIdAndSiteUserId(roomId, siteUserId) - .orElseThrow(() -> new CustomException(CHAT_PARTICIPANT_NOT_FOUND)); - - chatReadStatusRepository.upsertReadStatus(roomId, participant.getId()); -} +public void markChatMessagesAsRead(long siteUserId, long roomId) { + ChatParticipant participant = chatParticipantRepository.findByChatRoomIdAndSiteUserId(roomId, siteUserId) + .orElseThrow(() -> new CustomException(CHAT_ROOM_ACCESS_DENIED)); + + chatReadStatusRepository.upsertReadStatus(roomId, participant.getId()); +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (32)
src/main/java/com/example/solidconnection/chat/controller/ChatController.java(1 hunks)src/main/java/com/example/solidconnection/chat/domain/ChatAttachment.java(1 hunks)src/main/java/com/example/solidconnection/chat/domain/ChatMessage.java(1 hunks)src/main/java/com/example/solidconnection/chat/domain/ChatParticipant.java(1 hunks)src/main/java/com/example/solidconnection/chat/domain/ChatReadStatus.java(1 hunks)src/main/java/com/example/solidconnection/chat/domain/ChatRoom.java(2 hunks)src/main/java/com/example/solidconnection/chat/dto/ChatAttachmentResponse.java(1 hunks)src/main/java/com/example/solidconnection/chat/dto/ChatMessageResponse.java(1 hunks)src/main/java/com/example/solidconnection/chat/dto/ChatParticipantResponse.java(1 hunks)src/main/java/com/example/solidconnection/chat/dto/ChatRoomListResponse.java(1 hunks)src/main/java/com/example/solidconnection/chat/dto/ChatRoomResponse.java(1 hunks)src/main/java/com/example/solidconnection/chat/repository/ChatAttachmentRepository.java(1 hunks)src/main/java/com/example/solidconnection/chat/repository/ChatMessageRepository.java(1 hunks)src/main/java/com/example/solidconnection/chat/repository/ChatParticipantRepository.java(1 hunks)src/main/java/com/example/solidconnection/chat/repository/ChatReadStatusRepository.java(1 hunks)src/main/java/com/example/solidconnection/chat/repository/ChatRoomRepository.java(1 hunks)src/main/java/com/example/solidconnection/chat/service/ChatService.java(1 hunks)src/main/java/com/example/solidconnection/common/exception/ErrorCode.java(2 hunks)src/main/java/com/example/solidconnection/mentor/service/MentoringCommandService.java(5 hunks)src/test/java/com/example/solidconnection/chat/fixture/ChatAttachmentFixture.java(1 hunks)src/test/java/com/example/solidconnection/chat/fixture/ChatAttachmentFixtureBuilder.java(1 hunks)src/test/java/com/example/solidconnection/chat/fixture/ChatMessageFixture.java(1 hunks)src/test/java/com/example/solidconnection/chat/fixture/ChatMessageFixtureBuilder.java(1 hunks)src/test/java/com/example/solidconnection/chat/fixture/ChatParticipantFixture.java(1 hunks)src/test/java/com/example/solidconnection/chat/fixture/ChatParticipantFixtureBuilder.java(1 hunks)src/test/java/com/example/solidconnection/chat/fixture/ChatReadStatusFixture.java(1 hunks)src/test/java/com/example/solidconnection/chat/fixture/ChatReadStatusFixtureBuilder.java(1 hunks)src/test/java/com/example/solidconnection/chat/fixture/ChatRoomFixture.java(1 hunks)src/test/java/com/example/solidconnection/chat/fixture/ChatRoomFixtureBuilder.java(1 hunks)src/test/java/com/example/solidconnection/chat/repository/ChatReadStatusRepositoryForTest.java(1 hunks)src/test/java/com/example/solidconnection/chat/service/ChatServiceTest.java(1 hunks)src/test/java/com/example/solidconnection/mentor/service/MentoringCommandServiceTest.java(5 hunks)
🔇 Additional comments (27)
src/main/java/com/example/solidconnection/chat/domain/ChatReadStatus.java (1)
36-39: 1. 생성자 구현이 깔끔하게 잘 되어 있습니다채팅방 ID와 참여자 ID를 받아서 필드를 직접 초기화하는 간단하고 명확한 구현입니다. 유니크 제약조건과 함께 데이터 무결성을 보장하는 좋은 설계네요.
src/main/java/com/example/solidconnection/chat/repository/ChatAttachmentRepository.java (1)
6-8: 1. 표준적인 JPA 리포지토리 인터페이스 구현ChatAttachment 엔티티를 위한 기본적인 CRUD 기능을 제공하는 표준적인 구현입니다. Spring Data JPA 관례를 잘 따르고 있어요.
src/main/java/com/example/solidconnection/chat/domain/ChatMessage.java (2)
37-37: 1. 컬렉션 불변성 보장을 위한 final 키워드 적용chatAttachments 필드를 final로 선언하여 재할당을 방지하고 불변성을 보장하는 좋은 접근법입니다.
39-46: 2. 양방향 연관관계를 올바르게 구현한 생성자다음과 같은 좋은 패턴들이 적용되었습니다:
- 필수 필드들(content, senderId, chatRoom)을 파라미터로 받아 초기화
- chatRoom이 null이 아닐 때만 양방향 연관관계를 설정하여 NPE 방지
- ChatRoom의 chatMessages 컬렉션에 현재 메시지를 추가하여 일관성 유지JPA 엔티티의 양방향 연관관계 관리를 위한 표준적인 구현입니다.
src/test/java/com/example/solidconnection/chat/repository/ChatReadStatusRepositoryForTest.java (1)
7-10: 1. 테스트용 리포지토리 인터페이스가 잘 구현되었습니다다음과 같은 좋은 점들이 있습니다:
- Spring Data JPA 쿼리 메서드 명명 규칙을 정확히 따름
- Optional 반환 타입으로 null 안정성 제공
- 엔티티의 유니크 제약조건 필드들과 일치하는 메서드 시그니처테스트에서 읽기 상태 조회를 위한 적절한 구현이네요.
src/main/java/com/example/solidconnection/chat/domain/ChatRoom.java (4)
15-15: 1. Hibernate BatchSize 어노테이션 임포트 추가N+1 쿼리 문제 해결을 위한 배치 페칭 최적화 준비를 위한 임포트입니다.
30-30: 3. 컬렉션 불변성 보장을 위한 final 키워드 적용chatParticipants와 chatMessages 컬렉션을 final로 선언하여 재할당을 방지하고 불변성을 보장하는 좋은 설계입니다. 엔티티의 상태 일관성을 유지하는 데 도움이 됩니다.
Also applies to: 33-33
35-37: 4. 그룹 채팅 구분을 위한 생성자 추가isGroup 파라미터를 받는 생성자를 추가하여 채팅방 생성 시점에 그룹 채팅 여부를 설정할 수 있게 한 깔끔한 구현입니다. 일대일 채팅과 그룹 채팅을 명확히 구분할 수 있는 좋은 접근법이네요.
29-30: BatchSize 적용 확인 및 추가 검증 요청File: src/main/java/com/example/solidconnection/chat/domain/ChatRoom.java
Lines: 29–30@BatchSize(size = 10) private final List<ChatParticipant> chatParticipants = new ArrayList<>();
@batchsize 적용 확인
ChatRoom.chatParticipants 컬렉션에 @batchsize(size = 10)이 정상 반영된 것을 확인했습니다.남아 있는 N+1 문제 지점
getChatRooms 내부의chatMessageRepository.findLatestMessageByChatRoomId호출이 여전히 각 방마다 추가 쿼리를 발생시킬 수 있습니다.실제 성능 검증 방법
운영 또는 테스트 환경에서 SQL 로그(예:spring.jpa.show-sql=true)를 활성화하여 getChatRooms 호출 시 chat_participant와 chat_message 테이블에 대해 배치 페치 쿼리가 실행되는지 확인해 보세요.배치 사이즈 재조정 검토
실제 트래픽 패턴과 SQL 로그 결과를 토대로 size 10이 최적인지 검토하고, 필요하다면 적절히 조정하시길 권장드립니다.src/test/java/com/example/solidconnection/chat/fixture/ChatReadStatusFixture.java (1)
7-19: 테스트 픽스처 구현이 프로젝트 패턴과 일치합니다! 🧪
- 일관된 구조:
@TestComponent와 빌더 패턴을 활용한 표준 구조- 가독성: 한글 메서드명으로 테스트 코드의 가독성 향상
- 책임 분리: 실제 객체 생성은 빌더에 위임하여 단일 책임 원칙 준수
- 편의성:
읽음상태()메서드로 테스트 데이터 생성이 간편함현재 구현이 완벽하며, 다른 채팅 도메인 픽스처들과 일관성을 유지하고 있습니다.
src/main/java/com/example/solidconnection/chat/dto/ChatParticipantResponse.java (1)
3-12: 채팅 참가자 정보를 위한 깔끔한 DTO 설계입니다! 👤
- 명확한 구조: partnerId, nickname, profileUrl로 참가자 정보를 명확히 표현
- 불변성: record를 통한 안전한 불변 객체 구현
- 팩토리 메서드:
of()메서드로 명시적인 객체 생성- 일관성: 프로젝트의 다른 Response DTO들과 동일한 패턴
현재 구현이 요구사항을 완벽히 충족하며, ChatRoomResponse에서 참가자 정보 전달 역할을 잘 수행할 것으로 보입니다.
src/test/java/com/example/solidconnection/chat/fixture/ChatParticipantFixture.java (1)
1-20: 테스트 픽스처 구현이 깔끔합니다! 👍
- 패턴 준수: Builder 패턴을 통한 깔끔한 객체 생성
- 의존성 주입: @requiredargsconstructor를 활용한 적절한 DI 구현
- 네이밍: 도메인 언어와 일치하는 메서드명 사용
구현이 간결하고 명확하여 테스트 코드에서 사용하기 편리할 것 같습니다.
src/test/java/com/example/solidconnection/chat/fixture/ChatRoomFixture.java (1)
1-18: 일관성 있는 픽스처 패턴 적용! 🎯
- 일관성: 다른 픽스처 클래스와 동일한 구조 유지
- 파라미터 설계: isGroup 플래그를 통한 유연한 채팅방 타입 설정
- 빌더 활용: 적절한 책임 분리로 가독성 향상
채팅방 생성 로직이 명확하고 테스트에서 활용하기 좋은 구조입니다.
src/test/java/com/example/solidconnection/chat/fixture/ChatMessageFixture.java (1)
1-21: 메시지 픽스처의 파라미터 설계가 우수합니다! 💬
- 파라미터 구성: 메시지 생성에 필요한 핵심 요소들을 적절히 포함
- 타입 안정성: long senderId, ChatRoom 엔티티 등 명확한 타입 사용
- 패턴 일관성: 기존 픽스처들과 동일한 구조 유지
테스트 시나리오에서 다양한 메시지 생성이 용이할 것 같습니다.
src/test/java/com/example/solidconnection/chat/fixture/ChatMessageFixtureBuilder.java (2)
23-41: 나머지 빌더 구현은 훌륭합니다! ✨
- 플루언트 인터페이스: 각 setter 메서드가 올바르게 this 반환
- 엔티티 생성: create() 메서드의 생성자 호출과 저장 로직이 적절
- 타입 안정성: 각 필드의 타입이 명확하게 정의됨
빌더의 핵심 기능들이 잘 구현되어 있습니다.
19-21: 빌더 패턴 개선이 필요합니다! 🔧
- 문제점: 새 인스턴스 생성 시 현재 상태가 복사되지 않음
- 영향: 빌더 재사용 시 예상치 못한 동작 가능성
- 개선 방안: 현재 인스턴스 반환 또는 상태 초기화 로직 추가
다음과 같이 수정하는 것을 권장합니다:
public ChatMessageFixtureBuilder chatMessage() { - return new ChatMessageFixtureBuilder(chatMessageRepository); + this.content = null; + this.senderId = 0L; + this.chatRoom = null; + return this; }또는 팩토리 메서드로 분리:
+public static ChatMessageFixtureBuilder newChatMessage(ChatMessageRepository repository) { + return new ChatMessageFixtureBuilder(repository); +} + public ChatMessageFixtureBuilder chatMessage() { - return new ChatMessageFixtureBuilder(chatMessageRepository); + return this; }Likely an incorrect or invalid review comment.
src/main/java/com/example/solidconnection/chat/repository/ChatParticipantRepository.java (1)
7-12: 깔끔한 레포지토리 설계입니다!
- 채팅 참여자 검증을 위한
existsByChatRoomIdAndSiteUserId메서드- 참여자 조회를 위한
findByChatRoomIdAndSiteUserId메서드두 메서드 모두 Spring Data JPA 네이밍 컨벤션을 잘 따르고 있으며, 기본 타입 long 사용으로 일관성도 유지되고 있습니다.
src/main/java/com/example/solidconnection/mentor/service/MentoringCommandService.java (1)
35-36: 채팅 관련 의존성 추가가 적절합니다멘토링 승인 시 채팅방 생성을 위한 레포지토리 의존성들이 추가되었습니다.
src/main/java/com/example/solidconnection/common/exception/ErrorCode.java (2)
48-49: 채팅 관련 데이터 조회 에러 코드가 적절히 추가되었습니다
CHAT_PARTNER_NOT_FOUND: 채팅 상대를 찾을 수 없을 때 (400)CHAT_PARTICIPANT_NOT_FOUND: 채팅 참여자를 찾을 수 없을 때 (404)기존 "data not found" 섹션에 일관성 있게 배치되었습니다.
120-122: 채팅 도메인 전용 에러 코드 섹션이 잘 구성되었습니다
INVALID_CHAT_ROOM_STATE: 잘못된 채팅방 상태 (400)CHAT_ROOM_ACCESS_DENIED: 채팅방 접근 거부 (403)새로운 "chat" 섹션으로 분류하여 도메인별 에러 관리가 체계적으로 이루어지고 있습니다.
src/main/java/com/example/solidconnection/chat/dto/ChatMessageResponse.java (1)
6-18: 채팅 메시지 응답 DTO가 잘 구성되었습니다
- 완전한 메시지 정보: 메시지 ID, 내용, 발신자, 생성 시간, 첨부파일 포함
- 타입 안전성: 불변 레코드로 데이터 무결성 보장
- 첨부파일 지원:
List<ChatAttachmentResponse>로 다중 첨부파일 처리- 일관된 팩토리 패턴: 다른 DTO들과 동일한
of메서드 제공전체적으로 채팅 기능에 필요한 모든 정보를 적절히 담고 있습니다.
src/test/java/com/example/solidconnection/chat/fixture/ChatRoomFixtureBuilder.java (1)
16-18: 빌더 패턴 구현에서 상태 관리 문제가 있습니다.
chatRoom()메서드가 새로운 인스턴스를 생성하지만 현재 상태를 복사하지 않아 예상과 다른 동작을 할 수 있습니다.다음과 같이 수정하는 것을 권장합니다:
- public ChatRoomFixtureBuilder chatRoom() { - return new ChatRoomFixtureBuilder(chatRoomRepository); - } + public static ChatRoomFixtureBuilder chatRoom(ChatRoomRepository chatRoomRepository) { + return new ChatRoomFixtureBuilder(chatRoomRepository); + }또는 현재 상태를 복사하는 로직을 추가해야 합니다.
Likely an incorrect or invalid review comment.
src/main/java/com/example/solidconnection/chat/repository/ChatMessageRepository.java (1)
13-19: N+1 문제 해결을 위한 좋은 접근 방식입니다.
LEFT JOIN FETCH를 사용하여 채팅 첨부파일과 함께 메시지를 한 번에 로딩하는 것은 N+1 쿼리 문제를 효과적으로 해결합니다.Slice를 사용한 페이징 처리도 성능상 유리합니다.src/test/java/com/example/solidconnection/mentor/service/MentoringCommandServiceTest.java (2)
126-149: 멘토링-채팅방 연동 테스트 구현이 잘 되어 있습니다.
- 승인 전후의 상태를 명확히 검증하는 구조가 좋습니다.
Optional을 활용한 존재 여부 확인이 적절합니다.- 채팅방 속성과 참가자 목록을 모두 검증하는 포괄적인 테스트입니다.
173-189: 거절 시나리오 테스트도 적절히 구현되었습니다.멘토링 거절 시 채팅방이 생성되지 않는다는 것을 명확히 검증하고 있어 비즈니스 로직의 정확성을 보장합니다.
src/test/java/com/example/solidconnection/chat/fixture/ChatAttachmentFixture.java (1)
1-30: 테스트 픽스처 구현이 깔끔합니다!빌더 패턴을 활용한 테스트 데이터 생성 방식이 명확하고, 한글 메서드명으로 가독성을 높인 점이 좋습니다.
src/main/java/com/example/solidconnection/chat/repository/ChatRoomRepository.java (1)
38-55: 일대일 채팅방 조회 쿼리가 잘 구현되었습니다EXISTS 절을 사용하여 참여자를 확인하고, 정확히 2명인지 검증하는 로직이 명확합니다. LEFT JOIN FETCH로 참여자 정보를 미리 로딩하는 것도 좋은 접근입니다.
src/main/java/com/example/solidconnection/chat/controller/ChatController.java
Show resolved
Hide resolved
src/main/java/com/example/solidconnection/chat/repository/ChatMessageRepository.java
Outdated
Show resolved
Hide resolved
src/main/java/com/example/solidconnection/chat/repository/ChatReadStatusRepository.java
Outdated
Show resolved
Hide resolved
src/main/java/com/example/solidconnection/chat/repository/ChatRoomRepository.java
Show resolved
Hide resolved
|
|
||
| @RequiredArgsConstructor | ||
| @Service | ||
| public class ChatService { |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
채팅방 생성 시 동시성 제어 확인 필요
PR 설명에서 멘토링 승인 시 채팅방이 생성되는데, 더블클릭 등으로 인한 중복 생성 문제를 언급하셨습니다. 이와 관련하여 다음 사항들을 확인해 주세요:
- 멘토링 서비스의 동시성 제어: 채팅방 생성 로직에 적절한 동시성 제어가 있는지 확인
- 유니크 제약 조건: 동일한 참가자 간 중복 채팅방 생성을 방지하는 DB 제약 조건 확인
- 멱등성 보장: 동일한 요청이 여러 번 와도 하나의 채팅방만 생성되도록 보장
🏁 Script executed:
#!/bin/bash
# 멘토링 서비스에서 채팅방 생성 로직 확인
ast-grep --pattern 'class MentoringCommandService {
$$$
}'
# ChatRoom 엔티티의 유니크 제약 조건 확인
fd -e java "ChatRoom" --exec grep -A 10 -B 10 "@Table\|@UniqueConstraint\|@Index" {}
# 채팅방 생성 관련 메서드 찾기
rg -A 10 "createChatRoom|ChatRoom.*new|new.*ChatRoom" --type javaLength of output: 6414
채팅방 동시성 제어 강화 제안
멘토링 승인 시 동시 요청으로 인한 중복 채팅방 생성 이슈가 감지되었습니다.
다음 위치를 확인해 주세요:
- createChatRoomIfNotExists 메서드 (src/main/java/com/example/solidconnection/mentor/service/MentoringCommandService.java)
- ChatRoom 엔티티 (src/main/java/com/example/solidconnection/chat/domain/ChatRoom.java)
이 문제를 해결하기 위한 개선 방안을 제안합니다:
- 서비스 레벨 락 적용
@Lock(PESSIMISTIC_WRITE)또는LockModeType.PESSIMISTIC_WRITE활용
- 트랜잭션 전파 및 격리도 조정
@Transactional(propagation = Propagation.REQUIRES_NEW)등 검토
- DB 유니크 제약 추가
@Table(uniqueConstraints=@UniqueConstraint(columnNames={"mentor_user_id","mentee_user_id","is_group"}))설정
- 멱등성 검증 테스트 작성
- 동시 요청 시 중복 생성 방지 여부를 확인하는 테스트 케이스 작성
🤖 Prompt for AI Agents
In src/main/java/com/example/solidconnection/chat/service/ChatService.java at
line 36, enhance concurrency control to prevent duplicate chat room creation
during simultaneous mentoring approvals. Apply a pessimistic lock using
@Lock(PESSIMISTIC_WRITE) or LockModeType.PESSIMISTIC_WRITE in the
createChatRoomIfNotExists method in MentoringCommandService.java. Adjust
transaction propagation and isolation by using @Transactional with
Propagation.REQUIRES_NEW. Add a unique constraint on the ChatRoom entity for
mentor_user_id, mentee_user_id, and is_group columns. Finally, implement
idempotency tests to verify that concurrent requests do not create duplicate
chat rooms.
| // todo : 예외 처리 관련 고려 필요 | ||
| createChatRoomIfNotExists(mentor.getSiteUserId(), mentoring.getMenteeId()); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
동시성 문제와 예외 처리에 대한 검토가 필요합니다
PR 설명에서 언급된 대로 멘토가 승인을 더블클릭할 경우 중복 채팅방 생성 문제가 발생할 수 있습니다.
다음 사항들을 고려해보세요:
- 동시성 제어:
createChatRoomIfNotExists메서드에 분산 락 또는 데이터베이스 제약조건 추가 - 예외 처리: 채팅방 생성 실패 시 멘토링 상태 롤백 고려
- 트랜잭션 분리: 채팅방 생성을 별도 트랜잭션으로 분리하여 독립적 실패 처리
동시성 문제 검증을 위해 다음 스크립트를 실행해보겠습니다:
🏁 Script executed:
#!/bin/bash
# 채팅방 중복 생성을 방지하는 데이터베이스 제약조건이 있는지 확인
rg -A 10 -B 5 "UNIQUE.*participants|participants.*UNIQUE" --type sql
rg -A 10 -B 5 "@Table.*uniqueConstraints|uniqueConstraints.*@Table" Length of output: 13161
동시성 제어 및 예외 처리 강화 필요
- 동시성 제어
현재 ChatRoom 엔티티에 멘토-멘티 조합의 DB 레벨 유니크 제약조건이 없습니다.
분산 락(Redis Lock 등) 또는 낙관적 락(@Version) 도입을 검토해주세요. - 예외 처리 강화
채팅방 생성 실패 시 멘토링 승인 상태를 롤백하거나 보상 로직(compensation)을 구현해야 합니다. - 트랜잭션 분리
채팅방 생성 로직을 별도 트랜잭션으로 분리해 독립적으로 실패를 처리할 수 있도록 설계해주세요.
- // todo : 예외 처리 관련 고려 필요
- createChatRoomIfNotExists(mentor.getSiteUserId(), mentoring.getMenteeId());
+ // TODO: 동시성 제어 및 예외 처리 로직 추가 필요
+ createChatRoomIfNotExists(mentor.getSiteUserId(), mentoring.getMenteeId());Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
src/main/java/com/example/solidconnection/mentor/service/MentoringCommandService.java
around lines 60-61, enhance concurrency control by adding a DB-level unique
constraint on the mentor-mentee combination or implement a distributed lock
(e.g., Redis Lock) or optimistic locking with @Version. Strengthen exception
handling by rolling back mentoring approval status or adding compensation logic
if chat room creation fails. Separate the chat room creation logic into its own
transaction to allow independent failure handling.
| private void createChatRoomIfNotExists(long mentorUserId, long menteeUserId) { | ||
| Optional<ChatRoom> existingChatRoom = chatRoomRepository.findOneOnOneChatRoomByParticipants(mentorUserId, menteeUserId); | ||
| if (existingChatRoom.isPresent()) { | ||
| return; | ||
| } | ||
|
|
||
| ChatRoom newChatRoom = new ChatRoom(false); | ||
| ChatRoom savedChatRoom = chatRoomRepository.save(newChatRoom); | ||
|
|
||
| ChatParticipant mentorParticipant = new ChatParticipant(mentorUserId, savedChatRoom); | ||
| ChatParticipant menteeParticipant = new ChatParticipant(menteeUserId, savedChatRoom); | ||
|
|
||
| chatParticipantRepository.saveAll(List.of(mentorParticipant, menteeParticipant)); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
채팅방 생성 로직의 개선이 필요합니다
현재 구현에서 몇 가지 개선점이 있습니다:
- 동시성 문제: 두 사용자 간 채팅방 존재 여부 확인과 생성 사이에 레이스 컨디션 발생 가능
- 예외 처리 부재: 채팅방 생성 실패 시 멘토링 상태 불일치 가능성
- 트랜잭션 경계: 채팅방 생성 실패가 전체 멘토링 승인을 롤백시킬 수 있음
다음과 같은 개선을 제안합니다:
-private void createChatRoomIfNotExists(long mentorUserId, long menteeUserId) {
- Optional<ChatRoom> existingChatRoom = chatRoomRepository.findOneOnOneChatRoomByParticipants(mentorUserId, menteeUserId);
- if (existingChatRoom.isPresent()) {
- return;
- }
-
- ChatRoom newChatRoom = new ChatRoom(false);
- ChatRoom savedChatRoom = chatRoomRepository.save(newChatRoom);
-
- ChatParticipant mentorParticipant = new ChatParticipant(mentorUserId, savedChatRoom);
- ChatParticipant menteeParticipant = new ChatParticipant(menteeUserId, savedChatRoom);
-
- chatParticipantRepository.saveAll(List.of(mentorParticipant, menteeParticipant));
-}
+@Transactional(propagation = Propagation.REQUIRES_NEW)
+private void createChatRoomIfNotExists(long mentorUserId, long menteeUserId) {
+ try {
+ Optional<ChatRoom> existingChatRoom = chatRoomRepository.findOneOnOneChatRoomByParticipants(mentorUserId, menteeUserId);
+ if (existingChatRoom.isPresent()) {
+ return;
+ }
+
+ ChatRoom newChatRoom = new ChatRoom(false);
+ ChatRoom savedChatRoom = chatRoomRepository.save(newChatRoom);
+
+ ChatParticipant mentorParticipant = new ChatParticipant(mentorUserId, savedChatRoom);
+ ChatParticipant menteeParticipant = new ChatParticipant(menteeUserId, savedChatRoom);
+
+ chatParticipantRepository.saveAll(List.of(mentorParticipant, menteeParticipant));
+ } catch (Exception e) {
+ log.warn("채팅방 생성 실패: mentorUserId={}, menteeUserId={}", mentorUserId, menteeUserId, e);
+ // 채팅방 생성 실패가 멘토링 승인을 방해하지 않도록 함
+ }
+}🤖 Prompt for AI Agents
In
src/main/java/com/example/solidconnection/mentor/service/MentoringCommandService.java
around lines 73 to 86, the createChatRoomIfNotExists method has concurrency
risks, lacks exception handling, and does not manage transaction boundaries
properly. To fix this, wrap the entire method in a transactional context to
ensure atomicity, add synchronization or use database-level locking to prevent
race conditions when checking and creating chat rooms, and implement proper
exception handling to manage failures gracefully and avoid inconsistent
mentoring states.
src/test/java/com/example/solidconnection/chat/fixture/ChatReadStatusFixtureBuilder.java
Show resolved
Hide resolved
src/test/java/com/example/solidconnection/mentor/service/MentoringCommandServiceTest.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
reformat code 적용하긴했는데 잘 작동된건가요?
제가 리포맷한 결과랑 같아서 잘 작동된 것 같습니다 !
채팅방 생성 관련해서 예외처리를 어떻게 해야할지 고민이 되어 우선 딱히 해놓지 않았습니다. 멘토링이 승인될 때 채팅방이 생성되는 건 맞는 거 같은데 동시성 문제같은 게 발생할 수도 있을 거 같긴합니다.. 예를들어 멘토가 승인 시 더블클릭해버리면 채팅방이 두 개 생성될 수도 있지 않을까 싶네요 747c635
이건 많이 어렵네요 ... 기획에서 결정되었는지는 모르겠는데 저희가 동일한 참여자로 구성된 채팅방이 여러 개 존재해도 되지 않나요 ? 멘토링 관련 일대일 채팅방이라면 mentoring_id 를 FK로 갖고 유니크 제약 조건 잘 먹이면 해결할 수 있을 거 같긴 한데, 그룹 채팅방은 잘 모르겠네요.
일단 회의 때 이야기한 대로 bruno보면서 최대한 구현해보았는데 실수한 부분이 있다면 말씀해주세요!
다른 점은 없는 것 같습니다.
궁금한 점 & 개선하면 좋을 거 같은 점 코멘트 남겼습니다 ~ 정말 고생하셨습니다 !
src/main/java/com/example/solidconnection/chat/service/ChatService.java
Outdated
Show resolved
Hide resolved
| } | ||
| } | ||
|
|
||
| private void createChatRoomIfNotExists(long mentorUserId, long menteeUserId) { |
There was a problem hiding this comment.
createChatRoomIfNotExists 메서드는 구조 상 어쩔 수 없이 MentoringCommandService 에 위치하지, 수행하는 로직을 보면 ChatService 가 더 어울리는 것 같습니다 ! 따라서 채팅방 생성 관련 메서드를 ChatService로 이동시키고, 스프링 이벤트를 사용하는 것이 더 좋을 것 같습니다.
또한 멘토링 수락은 성공했으나 채팅방 생성 단계에서 실패하면 멘토링 수락 여부까지 전부 롤백됩니다. 만약 위 내용을 반영한다면 채팅방 생성 또한 별도의 트랜잭션으로 관리할 수 있기에 유연성(?) 측면에서 더 좋아질 것 같습니다.
There was a problem hiding this comment.
이건 별도의 채팅방 개설이 필요한 거 같아서 해당 로직은 우선 지워놓고 다른 pr에서 작업하겠습니다!
만약 멘토링 승인과 함께 채팅방이 개설되는 것이었다면 성혁님 말씀대로 구현하는 게 훨씬 좋았을 거 같네요 😊 8f698cb
There was a problem hiding this comment.
규혁님 정말 수고 많으셨습니다!! 작업량 자체도 많았고, 생각할거리도 많았네요.
JPQL 들을 보면서 참 고생이 많으셨겠다는게 느껴졌습니다🥹
그런 의미에서 이번 PR을 “N+1이 발생하더라도 일단 완성”하신게 현명한 선택 같습니다.
최적의 코드를 한번에 작성하기에는 생각할만한 포인트가 참 많았던 것 같습니다. ㅎㅎ
그리고 테스트코드는.. 정말 이정도면 달인이 되신 것 같습니다!! 너무 술술 읽혔습니다 👍👍👍👍
멘토링이 승인될 때 채팅방이 생성되는 건 맞는 거 같은데 동시성 문제같은 게 발생할 수도 있을 거 같긴합니다.. 예를들어 멘토가 승인 시 더블클릭해버리면 채팅방이 두 개 생성될 수도 있지 않을까 싶네요
저도 가장 간단한 방법으로는 unique key 가 떠올랐는데,.. 지금은 chat_room 이 metoring_id 를 가지고 있지 않은 구조이군요 🤔
아예 “멘토링 승인”으로 만들어진 채팅방이라는 것을 의미하도록
chat_room 이 metoring_id를 가지고 있어도 괜찮을 것 같다는 생각입니다.
그럼 성혁님이 말씀하신 “동일한 참여자로 구성된 채팅방이 여러 개 존재해도 되지 않나요?” 도 허용될 수 있을 것 같고요.
| @PathVariable("room-id") Long roomId) { | ||
| chatService.markChatMessagesAsRead(siteUserId, roomId); |
There was a problem hiding this comment.
| @PathVariable("room-id") Long roomId) { | |
| chatService.markChatMessagesAsRead(siteUserId, roomId); | |
| @PathVariable("room-id") Long roomId | |
| ) { | |
| chatService.markChatMessagesAsRead(siteUserId, roomId); |
[컨벤션]
| @PageableDefault(size = 20, sort = "createdAt", direction = Sort.Direction.DESC) Pageable pageable) { | ||
| SliceResponse<ChatMessageResponse> response = chatService.getChatMessages(siteUserId, roomId, pageable); |
There was a problem hiding this comment.
| @PageableDefault(size = 20, sort = "createdAt", direction = Sort.Direction.DESC) Pageable pageable) { | |
| SliceResponse<ChatMessageResponse> response = chatService.getChatMessages(siteUserId, roomId, pageable); | |
| @PageableDefault(size = 20, sort = "createdAt", direction = Sort.Direction.DESC) Pageable pageable | |
| ) { | |
| SliceResponse<ChatMessageResponse> response = chatService.getChatMessages(siteUserId, roomId, pageable); |
| @Modifying | ||
| @Query(value = """ | ||
| INSERT INTO chat_read_status (chat_room_id, chat_participant_id, created_at, updated_at) | ||
| VALUES (:chatRoomId, :chatParticipantId, NOW(6), NOW(6)) | ||
| ON DUPLICATE KEY UPDATE updated_at = NOW(6) | ||
| """, nativeQuery = true) | ||
| void upsertReadStatus(@Param("chatRoomId") long chatRoomId, @Param("chatParticipantId") long chatParticipantId); |
There was a problem hiding this comment.
오 없으면 insert, 있으면 update 하는 것을 통칭 upsert 라고 하는군요!
규혁님 덕분에 하나 배웠습니다 😊👍
그런데 제 생각에도 JPA의 save() 로도 동일하게 작동할 것 같아요~
BaseEntity에 @DynamicUpdate도 있으니 updated_at 도 save() 시에 자동 갱신될 것이고요!
네이티브 쿼리로 이것을 구현해야 하는 이유가 따로 있을까요?
There was a problem hiding this comment.
한 번 제가 save 로 잘 되는지 확인해봤는데요, 추가적인 로직 없이 단순히 upsertReadStatus 메서드를 save 를 사용하도록 수정하였습니다.
@Transactional
public void markChatMessagesAsRead(long siteUserId, long roomId) {
validateChatRoomParticipant(siteUserId, roomId);
ChatParticipant participant = chatParticipantRepository.findByChatRoomIdAndSiteUserId(roomId, siteUserId)
.orElseThrow(() -> new CustomException(CHAT_PARTICIPANT_NOT_FOUND));
ChatReadStatus chatReadStatus = chatReadStatusRepository.findByChatRoomIdAndChatParticipantId(roomId, participant.getId())
.orElseGet(() -> new ChatReadStatus(roomId, participant.getId()));
chatReadStatusRepository.save(chatReadStatus);
// chatReadStatusRepository.upsertReadStatus(roomId, participant.getId());
}(맞게 작성했겠죠 ?)
기존_읽음_상태가_있으면_updatedAt을_갱신한다 테스트가 실패합니다.
java.lang.AssertionError:
Expecting actual:
2025-07-29T16:19:01.636855Z (java.time.ZonedDateTime)
to be strictly after:
2025-07-29T16:19:01.636855Z (java.time.ZonedDateTime)
로그를 보니 update_at 이 갱신되지 않은 것 같습니다. 조회한 엔티티를 변경하지 않았으므로 더티 체킹이 동작하지 않고, @DynamicUpdate 가 동작하지 않은 것 같습니다(@PreUpdate 도 마찬가지).
그렇다고 update_at 필드에 대한 setter를 구현하여 더티 체킹이 동작하도록 하자니, 아마 이걸 의도하신 건 아닌 것 같습니다.
그래서 제 생각은 ... 작성된 네이티브 쿼리 사용해도 좋을 것 같습니다 ..! 더 좋은 방법이 떠오르지 않습니다 ㅠㅠ
이건 별개의 의견인데, @Modifying 어노테이션 옵션에 clearAutomatically 와 flushAutomatically 가 있더라고요(기본 값 false). 벌크 연산을 수행하는 경우 가장 큰 문제가 DB와 영속성 컨텍스트 간 불일치인데, 두 옵션 모두 true 로 설정하면 불일치 문제를 원천적으로 방지할 수 있다고 생각합니다 ! 저는 이 메서드뿐만 아니라 모든 벌크 연산 메서드에 두 옵션 모두 true 로 설정하도록 하는 것이 좋아 보이는데, 이 의견에 대해 어떻게 생각하시는지 궁금합니다 ..!
There was a problem hiding this comment.
조회한 엔티티를 변경하지 않았으므로 더티 체킹이 동작하지 않고, @DynamicUpdate 가 동작하지 않은 것 같습니다
아하 그렇겠네요~
어떤 값을 수정하지 않았으므로 그게 당연한 동작이겠네요.! 확인해주셔서 감사합니다🙇🏻♀️
There was a problem hiding this comment.
기존_읽음_상태가_있으면_updatedAt을_갱신한다 테스트가 실패합니다.
이것때문에 네이티브 쿼리를 작성하게 된 게 맞습니다!
처음에 채팅 관련 논의를 회의 때 할 때는 알아서 해줄 것이라고 생각했지만.. 제가 구현을 하다보니 동작을 안하는 걸 확인하고 이 방식으로 구현을 하게 되었었습니다!
유의미한 테스트 코드를 짠 거 같네요 ㅎㅎ
There was a problem hiding this comment.
이건 별개의 의견인데, @Modifying 어노테이션 옵션에 clearAutomatically 와 flushAutomatically 가 있더라고요(기본 값 false). 벌크 연산을 수행하는 경우 가장 큰 문제가 DB와 영속성 컨텍스트 간 불일치인데, 두 옵션 모두 true 로 설정하면 불일치 문제를 원천적으로 방지할 수 있다고 생각합니다 !
오.. 벌크연산의 주의할점만 해당 사항으로 알고 있었는데 이걸 방지할 수 있다는 점은 처음알았습니다! 이 pr에서 몰랐던 내용들을 참 많이 알아가네요.. 감사합니다 ㅎㅎ
전 성능이 조금 떨어지더라도 이러한 이슈는 방지하는 게 좋다고 생각해서 두 옵션 모두 true로 해도 좋은 거 같습니다!
There was a problem hiding this comment.
여러분이 말씀하시는 "벌크 연산"이 영속성 컨텍스트를 거치지 않고 DB에 직접 write 하는 쿼리를 말씀하시는게 맞나요?
벌크(여러개)로 연산한다는 뜻이 아니라요!
(저는 후자만 알고 있었어서 여쭤봅니다😳)
There was a problem hiding this comment.
오호~ 저는 '하나만 다루는데 왜 계속 벌크라고 하시지?' 싶었네요 ㅋㅋㅋ
찾아보고 나서 다른 의미도 있다는 것을 알게되었습니다😁
이 메서드뿐만 아니라 모든 벌크 연산 메서드에 두 옵션 모두 true 로 설정하도록 하는 것
저도 찬성합니다!
There was a problem hiding this comment.
저도 한 2년 전에.. 김영한님 강의 들을 때 공부하고 잊고 있던 개념인데 성혁님 덕에 살아났습니다 ㅎㅎ
d9ad86b 반영하였습니다!
| WHERE cm.chatRoom = cr | ||
| ) DESC NULLS LAST | ||
| """) | ||
| List<ChatRoom> findOneOnOneChatRoomsByUserId(@Param("userId") long userId); |
There was a problem hiding this comment.
이 조회는 Slice 로 페이지네이션 응답을 할 필요는 없나요?
There was a problem hiding this comment.
아하! 회의때는 채팅방이 많이 개설되지 않을 것이라고 이야기를 해서 굳이 Slice를 하지 말자! 고 이야기가 나왔었는데 이 부분도 Slice를 적용할까요?
There was a problem hiding this comment.
아하~ 일리가 있네요, 페이지네이션 반영 안하셔도 됩니다!👍
| @Query(""" | ||
| SELECT cm FROM ChatMessage cm | ||
| LEFT JOIN FETCH cm.chatAttachments | ||
| WHERE cm.chatRoom.id = :roomId | ||
| ORDER BY cm.createdAt DESC | ||
| """) | ||
| Slice<ChatMessage> findByRoomIdWithPaging(@Param("roomId") long roomId, Pageable pageable); |
There was a problem hiding this comment.
채팅 메세지를 최신순으로 정렬하여 응답한다면,
예를 들어
- 반갑습니다 (10:00)
- 저도 반갑습니다 (10:10)
- 날씨가 덥네요 (10:20)
라는 메세지가
- 날씨가 덥네요 (10:20)
- 저도 반갑습니다 (10:10)
- 반갑습니다 (10:00)
이런식으로 보이지 않을까요..?
There was a problem hiding this comment.
음 ... 저도 이 부분은 프론트의 영역이라고 생각합니다 ! 저희는 단순히 최신 순으로 정렬된 채팅을 전달해주면 된다고 생각합니다.
| @Query(""" | ||
| SELECT COUNT(cm) FROM ChatMessage cm | ||
| LEFT JOIN ChatReadStatus crs ON crs.chatRoomId = cm.chatRoom.id | ||
| AND crs.chatParticipantId = ( | ||
| SELECT cp.id FROM ChatParticipant cp | ||
| WHERE cp.chatRoom.id = :chatRoomId | ||
| AND cp.siteUserId = :userId | ||
| ) | ||
| WHERE cm.chatRoom.id = :chatRoomId | ||
| AND cm.senderId != :userId | ||
| AND (crs.updatedAt IS NULL OR cm.createdAt > crs.updatedAt) | ||
| """) | ||
| long countUnreadMessages(@Param("chatRoomId") long chatRoomId, @Param("userId") long userId); |
There was a problem hiding this comment.
다른 지식 없이 코드만 보는 입장에서는,
33번 라인의 AND cm.senderId != :userId 가 없어도 되지 않을까 싶어요!
왜냐하면 내가 메세지를 보냈다는 것은, 그 메세지를 당연히 읽었다는 것이니까요.
정리해서 다시 말하자면 이렇습니다!
AS-IS
- 내가 참여한 어떤 채팅방의 채팅 메세지들 중에서
- 내가 보내지 않은 채팅 메세지이면서
- 확인하지 않은 메세지의 수를 count
TO-BE
"내가 보낸 메세지"는 이미 내가 확인했을 것이므로, 두번째 조건이 없어도 될 것 같다.
정책상 제가 모르는 부분이 있다면 말씀해주세요 🙇🏻♀️
There was a problem hiding this comment.
아하 좋은 이야기 감사합니다!
AND cm.senderId != :userId가 없어도 잘 동작할 거 같습니다!
그런데 일반적으로 "미읽음 메시지 수"는 다른 사람이 보낸 메시지 중 읽지 않은 것만 카운트한다고 생각하여 비즈니스 로직상 있는게 나을 거 같다고 생각했는데 그냥 지울까요?
리뷰 작성하면서 AND cm.senderId != :userId 필터링 조건이 있으면 성능이 더 좋은가?라는 생각도 들긴 했는데 이건 테스트를 해보지 않는 이상 잘 모르겠어서 확신이 안드네요 😅
There was a problem hiding this comment.
일반적으로 "미읽음 메시지 수"는 다른 사람이 보낸 메시지 중 읽지 않은 것만 카운트한다고 생각하여 비즈니스 로직상 있는게 나을 거 같다고 생각했는데 그냥 지울까요?
다시 생각해보니 스키마 배경지식 없이, 일반적으로 생각한다면 다른 사람이 보낸 것만 카운트하는게 맞겠네요😅
| // todo : n + 1 문제 해결 필요! | ||
| List<ChatRoom> chatRooms = chatRoomRepository.findOneOnOneChatRoomsByUserId(siteUserId); | ||
| List<ChatRoomResponse> chatRoomInfos = chatRooms.stream() | ||
| .map(chatRoom -> toChatRoomResponse(chatRoom, siteUserId)) | ||
| .toList(); | ||
| return ChatRoomListResponse.of(chatRoomInfos); |
There was a problem hiding this comment.
오.. 이건 ktx 타고 구르면서 봐도 N+1이 발생할만하겠군요 🚅
제가 떠오르는 아이디어는, List<ChatRoomResponse> chatRoomInfos 의 각각의 원소들에 대해 select 문을 실행하지 않고, IN 절을 사용하여 관련된 모든 것들을 가지고 온 다음 id 별로 분류하는 방법도 생각납니다! 실제 저희 코드에서 그런 기법을 여러번 쓰기도 했고요~
There was a problem hiding this comment.
ㅎㅎ... 이 pr작성하는 날 수많은 JPQL에 치이면서... 과부하가 와버렸었습니다.. 우선 기능이 급하니... 머지되고 바로 수정 pr 작성하겠습니다... 😂
오.. 이건 ktx 타고 구르면서 봐도 N+1이 발생할만하겠군요 🚅
너무 인상적인 표현이네요 ㅋㅋㅋㅋ...
| ChatParticipant partnerParticipant = findPartner(chatRoom, siteUserId); | ||
|
|
||
| SiteUser siteUser = siteUserRepository.findById(partnerParticipant.getSiteUserId()) | ||
| .orElseThrow(() -> new CustomException(USER_NOT_FOUND)); | ||
| ChatParticipantResponse partner = ChatParticipantResponse.of(siteUser.getId(), siteUser.getNickname(), siteUser.getProfileImageUrl()); |
There was a problem hiding this comment.
이 부분에서 떠오르는 성능 개선 방법으로는,
"멘토링 수락으로 만들어진 채팅은 상대가 바뀌지 않을 것이니 캐싱으로 해결할 수도 있을 것 같다"는 점입니다~
상대가 닉네임이나 프로필 이미지를 바꾸면 업데이트하는 식으로요.
우선 이 PR은 구현이 목표이니, 당장 반영하지 않으셔도 되고 의견만 제시해봅니다 🙋🏻♀️
There was a problem hiding this comment.
n+1문제 해결과 함께 해당사항도 반영해보겠습니다!!
- isGroup과 findFirst를 활용하여 중간 리스트 생성 제거
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/test/java/com/example/solidconnection/chat/service/ChatServiceTest.java (1)
1-366: 전반적으로 우수한 테스트 커버리지입니다ChatService의 주요 기능들이 체계적으로 테스트되고 있습니다. 다만, PR 설명에서 언급된 몇 가지 이슈들에 대한 추가 테스트를 고려해보시면 좋겠습니다:
- N+1 쿼리 문제: 대량의 채팅방을 조회할 때의 성능 테스트
- 동시성 문제: 멘토가 승인을 중복 클릭했을 때 채팅방 중복 생성 방지 테스트
이러한 엣지 케이스에 대한 테스트 코드 작성이 필요하시다면 도움을 드릴 수 있습니다. 새로운 이슈로 등록하시겠습니까?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/main/java/com/example/solidconnection/chat/controller/ChatController.java(1 hunks)src/main/java/com/example/solidconnection/chat/repository/ChatRoomRepository.java(1 hunks)src/main/java/com/example/solidconnection/chat/service/ChatService.java(1 hunks)src/main/java/com/example/solidconnection/common/exception/ErrorCode.java(2 hunks)src/test/java/com/example/solidconnection/chat/service/ChatServiceTest.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/main/java/com/example/solidconnection/common/exception/ErrorCode.java
- src/main/java/com/example/solidconnection/chat/repository/ChatRoomRepository.java
- src/main/java/com/example/solidconnection/chat/service/ChatService.java
- src/main/java/com/example/solidconnection/chat/controller/ChatController.java
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (7)
src/test/java/com/example/solidconnection/chat/service/ChatServiceTest.java (7)
37-74: 테스트 구조가 잘 설계되었습니다! 👍테스트 픽스처를 활용한 데이터 설정과 의존성 주입이 깔끔하게 구현되었습니다. 테스트 컨테이너를 사용한 통합 테스트 환경 구성도 적절합니다.
137-146: 채팅 상대방 검증 로직이 잘 구현되었습니다1:1 채팅방에서 상대방이 없는 비정상적인 상태를 검증하는 중요한 테스트입니다. 데이터 무결성 보장에 도움이 됩니다.
149-190: 읽지 않은 메시지 카운팅 테스트가 체계적입니다주요 시나리오를 모두 커버하고 있습니다:
- 읽음 상태가 없을 때의 전체 카운팅
- 읽음 상태 이후 메시지만 카운팅
테스트 설정도 BeforeEach를 활용해 중복을 제거했습니다.
195-196: 상수 정의가 명확합니다
NO_NEXT_PAGE_NUMBER상수를 사용해 매직 넘버를 제거한 점이 좋습니다. 코드 가독성과 유지보수성이 향상됩니다.
265-267: 페이징 테스트 데이터 생성 로직이 효과적입니다25개의 메시지를 생성하여 페이징을 테스트하는 방식이 적절합니다. 발신자를 번갈아가며 설정하는 것도 현실적인 시나리오입니다.
283-300: 접근 권한 검증 테스트가 철저합니다보안 관련 중요한 시나리오들을 잘 다루고 있습니다:
- 참여자가 아닌 사용자의 접근 차단
- 존재하지 않는 채팅방 접근 차단
이러한 테스트는 채팅 기능의 보안성을 보장하는 데 필수적입니다.
316-351: 읽음 처리 테스트가 정교하게 구현되었습니다특히 좋은 점들:
- 최초 읽음 상태 생성과 기존 상태 업데이트를 구분하여 테스트
updatedAt타임스탬프 갱신을 검증하여 시간 추적 기능 확인- 테스트 전용 레포지토리를 사용한 검증으로 테스트 격리성 확보
src/test/java/com/example/solidconnection/chat/service/ChatServiceTest.java
Outdated
Show resolved
Hide resolved
- 코드래빗 의견 반영
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/test/java/com/example/solidconnection/chat/service/ChatServiceTest.java (1)
110-111: 과거 리뷰 코멘트가 이미 반영되었습니다110번 라인의 괄호 불일치 문제가 이미 수정되어 올바르게 닫혀있음을 확인했습니다.
🧹 Nitpick comments (1)
src/test/java/com/example/solidconnection/chat/service/ChatServiceTest.java (1)
76-147: N+1 쿼리 문제에 대한 성능 테스트 추가를 고려해보세요PR 설명에서
getChatRooms함수에 N+1 쿼리 문제가 있다고 언급하셨는데, 이를 검증하는 테스트가 없습니다.다음과 같은 항목들을 테스트에 추가하면 좋을 것 같습니다:
- 많은 수의 채팅방이 있을 때 쿼리 실행 횟수 검증
- 성능 개선 후 비교를 위한 벤치마크 테스트
N+1 쿼리 문제를 검증하는 테스트 코드를 작성해드릴까요?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/test/java/com/example/solidconnection/chat/service/ChatServiceTest.java(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (2)
src/test/java/com/example/solidconnection/chat/service/ChatServiceTest.java (2)
278-278: 페이지 번호 검증 로직을 확인해주세요현재
firstResponse.nextPageNumber()가 2를 반환하는 것을 기대하고 있는데, 일반적으로 페이지 번호는 0부터 시작하므로 다음 페이지는 1이어야 합니다.서비스 구현에서 페이지 번호를 1부터 시작하도록 설정했는지 확인이 필요합니다.
303-365: 채팅방 생성 시 동시성 문제에 대한 테스트 추가를 고려해보세요PR 설명에서 멘토가 승인 버튼을 두 번 클릭할 경우 중복 채팅방이 생성될 수 있다는 동시성 문제를 언급하셨습니다.
이러한 동시성 시나리오를 검증하는 테스트 추가를 권장합니다:
- 동시에 여러 스레드에서 채팅방 생성 시도
- 중복 생성 방지 로직 검증
동시성 문제를 검증하는 멀티스레드 테스트를 작성해드릴까요?
⛔ Skipped due to learnings
Learnt from: nayonsoso PR: solid-connection/solid-connect-server#375 File: src/main/java/com/example/solidconnection/mentor/service/MentorMyPageService.java:47-53 Timestamp: 2025-07-05T17:54:42.475Z Learning: MentorMyPageService에서 PUT 메서드 구현 시 전체 채널을 새로 생성하여 교체하는 방식을 사용하는 것이 PUT의 의미론적 특성과 일치하며, 트랜잭션 로킹 관점에서도 합리적인 접근이다.
whqtker
left a comment
whqtker
left a comment
There was a problem hiding this comment.
고생 많이 하셨습니다 ㅠㅠ 👏
개인적인 의견도 좋게 봐 주셔서 감사합니다. 변경사항 적용하시고 커밋 해시 달아주셔서 빠르게 확인할 수 있었습니다. 저도 다음에는 달아야겠네요
제가 작성한 리뷰에 대해서 마무리가 된 듯 하여 approve 하겠습니다 !
3 participants
관련 이슈
작업 내용
채팅 관련 기능을 추가하였습니다!
특이 사항
리뷰 요구사항 (선택)
일단 회의 때 이야기한 대로 bruno보면서 최대한 구현해보았는데 실수한 부분이 있다면 말씀해주세요!
reformat code 적용하긴했는데 잘 작동된건가요?
필요한 것만 구현한 거 같은데 자꾸 pr이 커지네요....