[FIX] 방 관리 기능 개선 및 버그 수정 (+ 채팅 refactor)#93
Open
ydking0911 wants to merge 18 commits intodevelopfrom
Hidden character warning
The head ref may contain hidden characters: "fix/#91/\ubc29-\uad00\ub9ac-\uae30\ub2a5-\uac1c\uc120-\ubc0f-\ubc84\uadf8-\uc218\uc815"
Open
Conversation
Walkthrough룸 삭제와 룸메이트 강제퇴장 흐름을 이벤트 기반으로 전환하고, 채팅 관련 대량 삭제 메서드·웹소켓 알림 인프라(재시도·복구)·핸드셰이크·권한 검증을 추가하였습니다. 단위·통합 테스트가 다수 추가되어 동작을 검증합니다. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant API as HTTP API
participant UseCase as DeleteRoomUseCase
participant Repo as Repository
participant EventBus as ApplicationEventPublisher
participant Listener as RoomDeletedEventListener
participant Notifier as ChatWebSocketNotificationEventListener
participant WSService as ChatWebSocketSendService
Client->>API: DELETE /api/rooms/{roomNo}
API->>UseCase: execute(requesterNo, roomNo)
UseCase->>Repo: findByIdForUpdate(roomNo) (lock)
UseCase->>UseCase: validate host / completed / members
UseCase->>Repo: soft-delete + bulk deletes (requests, rules, likes)
UseCase->>UseCase: roomService.flush()
UseCase->>EventBus: publish RoomDeletedEvent(roomNo)
EventBus->>Listener: handle(RoomDeletedEvent) [BEFORE_COMMIT]
Listener->>Repo: findAllByRoomNo(roomNo)
loop per chatRoom
Listener->>Repo: deleteAllByChatRoom(chatRoom)
Listener->>Repo: deleteByChatRoomNo(chatRoomNo)
Listener->>EventBus: collect UserNotifyTask / BroadcastTask
end
Listener->>EventBus: publish ChatWebSocketNotificationEvent(...)
EventBus->>Notifier: handle(...) [AFTER_COMMIT async]
Notifier->>WSService: notifyUser / broadcast
WSService-->>Client: websocket delivery (with retry)
sequenceDiagram
participant Client as WebSocket Client
participant Server as WebSocket Server
participant Handler as JwtPrincipalHandshakeHandler
participant Interceptor as ChatRoomAuthorizationInterceptor
Client->>Server: CONNECT (handshake attributes)
Server->>Handler: determineUser(attributes)
Handler->>Server: return StompPrincipal(userNo) or null
Server->>Interceptor: preSend(CONNECT)
Interceptor->>Interceptor: session.userNo 읽어 Principal 주입
Client->>Server: SEND /app/chat-room/{id}/send
Server->>Interceptor: preSend(SEND)
Interceptor->>Interceptor: destination 및 session.userNo 검증 -> 통과/예외
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
Pull request overview
방 관리(방 삭제/강퇴) 기능을 추가·보강하면서, 삭제/강퇴 시 채팅 도메인 정리와 개인 알림(/queue/notification) 전달을 가능하게 하고 트랜잭션 정합성 이슈를 완화하는 변경입니다.
Changes:
- 방 삭제 API(
DELETE /api/rooms/{roomNo}) 및 관련 cascade 정리/이벤트 기반 채팅방 정리 추가 - 강퇴/방 삭제 시 WebSocket 개인 알림(
/queue/notification) 라우팅을 위한 Principal 설정 로직 추가 - soft-delete/벌크 삭제/flush 적용 및 관련 리스너/리포지토리 리팩터링 + 테스트 보강(유닛/통합)
Reviewed changes
Copilot reviewed 37 out of 37 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/test/java/com/project/dorumdorum/global/security/JwtPrincipalHandshakeHandlerTest.java | handshake attributes 기반 Principal 결정 로직 단위 테스트 추가 |
| src/test/java/com/project/dorumdorum/global/security/ChatRoomAuthorizationInterceptorTest.java | CONNECT/SEND 프레임 처리(Principal 등록/세션 검증) 단위 테스트 추가 |
| src/test/java/com/project/dorumdorum/domain/roommate/unit/infra/repository/RoommateRepositoryImplTest.java | findMyRoommates 결과 없음 케이스 테스트 추가 |
| src/test/java/com/project/dorumdorum/domain/room/unit/usecase/KickRoommateUseCaseTest.java | 강퇴 시 flush 호출 검증 추가 |
| src/test/java/com/project/dorumdorum/domain/room/unit/usecase/DeleteRoomUseCaseTest.java | 방 삭제 유스케이스 단위 테스트 신규 추가 |
| src/test/java/com/project/dorumdorum/domain/room/integration/KickRoommatePersistenceIntegrationTest.java | 강퇴 후 DB 반영 + 개인 알림 전송 통합 테스트 추가 |
| src/test/java/com/project/dorumdorum/domain/room/integration/DeleteRoomPersistenceIntegrationTest.java | 방 soft delete 및 연관(요청/규칙/채팅) 정리 통합 테스트 추가 |
| src/test/java/com/project/dorumdorum/domain/chat/unit/usecase/RoommateKickedEventListenerTest.java | 강퇴 리스너 Optional 조회/개인 알림 전송 포함 테스트 보강 |
| src/test/java/com/project/dorumdorum/domain/chat/unit/usecase/RoomDeletedEventListenerTest.java | 방 삭제 리스너(브로드캐스트/개인알림/삭제 순서) 단위 테스트 신규 추가 |
| src/main/java/com/project/dorumdorum/global/security/StompPrincipal.java | STOMP user destination 라우팅용 Principal 구현 추가 |
| src/main/java/com/project/dorumdorum/global/security/JwtPrincipalHandshakeHandler.java | handshake attributes의 userNo를 Principal로 등록 |
| src/main/java/com/project/dorumdorum/global/security/ChatRoomAuthorizationInterceptor.java | CONNECT 시 Principal 등록 + SEND 시 세션 userNo 검증 로직 추가 |
| src/main/java/com/project/dorumdorum/global/exception/code/status/RoomErrorStatus.java | 방 삭제 관련 에러 코드 추가(ROOM017/ROOM018) |
| src/main/java/com/project/dorumdorum/global/config/WebSocketConfig.java | HandshakeHandler 등록 및 broker 설정 보강 |
| src/main/java/com/project/dorumdorum/domain/roommate/infra/repository/RoommateRepositoryImpl.java | findMyRoommates에서 soft-deleted 룸/룸메이트 필터링 추가 |
| src/main/java/com/project/dorumdorum/domain/roommate/domain/service/RoommateService.java | leaveRoom 하드 삭제 유지(주석 추가) 및 flush 제거 |
| src/main/java/com/project/dorumdorum/domain/room/ui/spec/DeleteRoomApiSpec.java | 방 삭제 API 스펙(스웨거) 추가 |
| src/main/java/com/project/dorumdorum/domain/room/ui/DeleteRoomController.java | 방 삭제 컨트롤러 추가 |
| src/main/java/com/project/dorumdorum/domain/room/domain/service/RoomService.java | findByIdForUpdate 예외 코드 정정 + flush 메서드 추가 |
| src/main/java/com/project/dorumdorum/domain/room/domain/service/RoomRequestService.java | 방 기준 RoomRequest 벌크 삭제 메서드 추가 |
| src/main/java/com/project/dorumdorum/domain/room/domain/repository/RoomRequestRepository.java | RoomRequest 벌크 DELETE JPQL로 교체 |
| src/main/java/com/project/dorumdorum/domain/room/domain/repository/RoomRepository.java | for-update 조회에 deletedAt is null 조건 추가 |
| src/main/java/com/project/dorumdorum/domain/room/domain/repository/RoomLikeRepository.java | RoomLike 벌크 DELETE JPQL로 교체 |
| src/main/java/com/project/dorumdorum/domain/room/application/usecase/KickRoommateUseCase.java | 강퇴 처리 중 flush 추가 |
| src/main/java/com/project/dorumdorum/domain/room/application/usecase/DeleteRoomUseCase.java | 방 삭제 유스케이스 신규 추가(soft delete + cascade + 이벤트 발행) |
| src/main/java/com/project/dorumdorum/domain/room/application/event/RoomDeletedEvent.java | 방 삭제 이벤트 신규 추가 |
| src/main/java/com/project/dorumdorum/domain/checklist/domain/service/RoomRuleService.java | roomNo 기준 RoomRule 삭제 메서드 추가 |
| src/main/java/com/project/dorumdorum/domain/checklist/domain/repository/RoomRuleRepository.java | RoomRule 벌크 DELETE JPQL 추가 |
| src/main/java/com/project/dorumdorum/domain/chat/domain/service/ChatRoomService.java | 채팅방 벌크 삭제(deleteByChatRoomNo) 및 roomNo로 조회 추가 |
| src/main/java/com/project/dorumdorum/domain/chat/domain/service/ChatRoomMemberService.java | 멤버 Optional 조회 및 채팅방 기준 벌크 삭제 API 추가 |
| src/main/java/com/project/dorumdorum/domain/chat/domain/repository/ChatRoomRepository.java | roomNo 기준 조회 + chatRoomNo 기준 벌크 DELETE 추가 |
| src/main/java/com/project/dorumdorum/domain/chat/domain/repository/ChatRoomMemberRepository.java | chatRoom 기준 벌크 DELETE 추가 |
| src/main/java/com/project/dorumdorum/domain/chat/domain/repository/ChatMessageRepository.java | unreadCount 벌크 UPDATE에 flushAutomatically 추가 |
| src/main/java/com/project/dorumdorum/domain/chat/domain/entity/ChatRoom.java | room_no 인덱스 추가 |
| src/main/java/com/project/dorumdorum/domain/chat/application/event/RoommateKickedEventListener.java | 멤버 조회 단일화(Optional) + 개인 알림 전송 추가 |
| src/main/java/com/project/dorumdorum/domain/chat/application/event/RoomDeletedEventListener.java | 방 삭제 시 채팅방 브로드캐스트/개인알림 및 벌크 삭제 처리 추가 |
| src/main/java/com/project/dorumdorum/domain/chat/application/dto/response/NotificationMessage.java | 방 삭제/강퇴 개인 알림 메시지 DTO 추가 |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| .setHandshakeHandler(jwtPrincipalHandshakeHandler) | ||
| .addInterceptors(jwtHandshakeInterceptor) | ||
| .setAllowedOriginPatterns("*") | ||
| .withSockJS(); // HTTP long-polling으로 폴백 시 HTTP 요청으로 처리됌 -> 연결 수 제한 로직 별도 고려 필요 |
There was a problem hiding this comment.
WebSocket long-polling 주석의 맞춤법이 잘못되었습니다(“처리됌”). 코드 주석이므로 “처리됨” 등으로 수정해 주세요.
Suggested change
| .withSockJS(); // HTTP long-polling으로 폴백 시 HTTP 요청으로 처리됌 -> 연결 수 제한 로직 별도 고려 필요 | |
| .withSockJS(); // HTTP long-polling으로 폴백 시 HTTP 요청으로 처리됨 -> 연결 수 제한 로직 별도 고려 필요 |
| * 방 삭제 | ||
| * - 방장 권한 검증 | ||
| * - 방장 외 룸메이트 존재 시 삭제 불가 | ||
| * - RoomRequest, RoomLike cascade 삭제 후 방 소프트 삭제 |
There was a problem hiding this comment.
Javadoc에 ‘RoomRequest, RoomLike cascade 삭제 후 방 소프트 삭제’라고 되어 있지만, 실제 구현은 room.delete()로 소프트 삭제를 먼저 수행한 뒤 연관 데이터 삭제를 진행합니다. 문서와 구현 순서가 불일치하므로 Javadoc을 실제 동작에 맞게 수정해 주세요(또는 구현 순서를 의도대로 조정).
Suggested change
| * - RoomRequest, RoomLike cascade 삭제 후 방 소프트 삭제 | |
| * - 방 소프트 삭제 후 방장 퇴실 처리 및 flush 수행 | |
| * - RoomRequest, RoomRule, RoomLike 연관 데이터 삭제 |
Comment on lines
+54
to
+56
| room.delete(); | ||
| roommateService.leaveRoom(requesterNo, roomNo); | ||
| roomService.flush(); |
There was a problem hiding this comment.
PR 설명에서는 room.delete() 직후 roomService.flush()로 deletedAt을 먼저 DB에 반영한 뒤 leaveRoom()을 수행하는 것으로 되어 있는데, 현재 코드는 leaveRoom() 이후에 flush()가 호출됩니다. 의도한 트랜잭션 정합성/재진입 차단 목적이 flush 선행을 전제한 것이라면 구현 순서를 맞추고, 반대로 현재 순서가 의도라면 PR 설명(및 관련 주석)을 현재 동작에 맞게 업데이트해 주세요.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/test/java/com/project/dorumdorum/domain/room/unit/usecase/KickRoommateUseCaseTest.java (1)
44-105: 🧹 Nitpick | 🔵 Trivial성공/예외 경로 모두에서
flush()호출 여부를 검증한 점이 좋습니다 👍성공 경로에서
room.minusCurrentMate()이후roomService.flush()가 호출되는 것과, 각 예외 경로(방 없음/대상 미존재/비호스트/자기 강퇴)에서flush()가 절대 호출되지 않는 것을 명시적으로 검증하여 회귀를 막기에 충분합니다.한 가지 선택 사항으로, Mockito
InOrder를 활용하여leaveRoom → minusCurrentMate → flush → publishEvent순서까지 함께 검증하면 트랜잭션 내 순서 보장이 더욱 견고해질 수 있습니다. 필수 사항은 아닙니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/com/project/dorumdorum/domain/room/unit/usecase/KickRoommateUseCaseTest.java` around lines 44 - 105, Add an InOrder verification for the success test to assert the exact sequence leaveRoom → minusCurrentMate → roomService.flush → publishEvent: create an InOrder over roommateService, room, roomService, eventPublisher and verify in order that roommateService.leaveRoom(...) is called, then room.minusCurrentMate(), then roomService.flush(), then eventPublisher.publishEvent(...) (using the same arguments/assertions as the existing verify calls); leave all existing exception-path tests unchanged.src/main/java/com/project/dorumdorum/domain/chat/domain/repository/ChatRoomRepository.java (1)
16-28:⚠️ Potential issue | 🟠 Major좋은 코드입니다! 다만 관련 리포지토리에서 수정이 필요합니다.
ChatRoomRepository의deleteByChatRoomNo메서드는@Modifying(clearAutomatically=true, flushAutomatically=true)설정이 잘 되어 있어요. 다만 확인 결과,ChatMessageRepository.deleteByChatRoomNo메서드는 여전히@Modifying(clearAutomatically = true)만 적용되어 있고flushAutomatically가 빠져 있어요. 일관성을 위해ChatMessageRepository의deleteByChatRoomNo메서드도 다음과 같이 수정해 주세요:`@Modifying`(clearAutomatically = true, flushAutomatically = true) `@Query`("DELETE FROM ChatMessage m WHERE m.chatRoom.chatRoomNo = :chatRoomNo") void deleteByChatRoomNo(`@Param`("chatRoomNo") String chatRoomNo);이렇게 하면 메시지, 멤버, 채팅방 삭제 순서에서 모든 벌크 쿼리가 동일한 설정으로 작동해서 데이터 일관성을 더 잘 보장할 수 있습니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/project/dorumdorum/domain/chat/domain/repository/ChatRoomRepository.java` around lines 16 - 28, ChatMessageRepository의 deleteByChatRoomNo 메서드가 현재 `@Modifying`(clearAutomatically = true)만 적용되어 있어 ChatRoomRepository의 deleteByChatRoomNo와 일관성이 없으니, 메서드 deleteByChatRoomNo(`@Param`("chatRoomNo") String chatRoomNo) 선언을 찾아 `@Modifying에` flushAutomatically = true를 추가하고 기존 JPQL("DELETE FROM ChatMessage m WHERE m.chatRoom.chatRoomNo = :chatRoomNo") 및 `@Query` 어노테이션은 그대로 유지하여 bulk 삭제 시 자동 플러시 동작이 ChatRoomRepository와 동일하도록 수정하세요.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/main/java/com/project/dorumdorum/domain/chat/application/dto/response/NotificationMessage.java`:
- Around line 3-15: Change the NotificationMessage record to use a
NotificationType enum instead of a raw String for the type to avoid brittle
string literals: create an enum NotificationType (e.g., ROOM_DELETED,
KICKED_FROM_ROOM) and, if you must keep the exact string contract with the
frontend, annotate an accessor in the enum with `@JsonValue` to serialize to the
original literal; update the record signature (NotificationMessage) to accept
NotificationType and modify the static factory methods roomDeleted and kicked to
pass the corresponding NotificationType constants.
In
`@src/main/java/com/project/dorumdorum/domain/chat/application/event/RoomDeletedEventListener.java`:
- Around line 47-58: The current RoomDeletedEventListener sends WebSocket
notifications (broadcastDeletionSafely and notifyUserSafely) before performing
DB deletes (chatMessageService.deleteAllByChatRoom,
chatRoomMemberService.deleteAllByChatRoom, chatRoomService.deleteByChatRoomNo),
which can leak notifications on rollback; change this so DB deletes execute
inside the transaction and notification sending runs only AFTER commit.
Concretely, stop calling broadcastDeletionSafely and notifyUserSafely directly
in RoomDeletedEventListener; instead capture the minimal notification payload
(chatRoomNo, recipientUserNo(s)) and either (a) register a
TransactionSynchronization.afterCommit callback to dispatch those payloads, or
(b) publish a new ChatRoomDeletedNotificationEvent/outbox entry from
RoomDeletedEventListener and handle sending in an AFTER_COMMIT listener (e.g.,
ChatRoomDeletedNotificationHandler) that calls
broadcastDeletionSafely/notifyUserSafely. Ensure
ChatRoomDeletedNotificationEvent contains chatRoomNo and recipient IDs and that
only the after-commit handler invokes the WebSocket methods.
In
`@src/main/java/com/project/dorumdorum/domain/chat/application/event/RoommateKickedEventListener.java`:
- Around line 43-68: The listener RoommateKickedEventListener currently performs
DB state changes (chatMessageService.decreaseUnreadCount,
chatRoomMemberService.leave, chatMessageService.save) and sends external
notifications (broadcastSafely, notifyUserSafely) inside a
`@TransactionalEventListener`(phase = TransactionPhase.BEFORE_COMMIT) via
processKick(...), which can cause out-of-sync external notifications if the
transaction later rolls back; split processKick into two parts: (1) perform and
persist state changes and assemble the payload (message and ChatMessageResponse)
during BEFORE_COMMIT, and (2) defer the external sends (calls to broadcastSafely
and notifyUserSafely) to AFTER_COMMIT using
TransactionSynchronizationManager.registerSynchronization(... afterCommit ...)
or move sending into a separate `@TransactionalEventListener`(phase =
AFTER_COMMIT) that receives the prepared payload, ensuring only committed
changes trigger broadcasts; alternatively, document the intentional trade-off in
the class javadoc if you choose to keep current behavior.
- Around line 57-61: The system message builds content as displayName + "가
퇴장했습니다." which causes Korean postposition errors and may misrepresent a kick as
a normal leave; in RoommateKickedEventListener replace the concatenation with a
grammatically neutral honorific form like displayName + "님이 퇴장했습니다." or, if the
event is indeed a forced removal, use displayName + "님이 강퇴되었습니다."; alternatively
implement a small helper (e.g., chooseSubjectParticle(String name) or
formatSystemMessage(String displayName, boolean forced)) and use that helper
where displayName is computed (after userService.findById) before calling
chatMessageService.save so the message is both grammatically correct and
semantically accurate.
In
`@src/main/java/com/project/dorumdorum/domain/chat/domain/entity/ChatRoom.java`:
- Around line 17-21: Add the missing index DDL for idx_chat_room_room_no to
schema.sql: inspect existing UNIQUE INDEXes uk_chat_room_group and
uk_chat_room_direct on the chat_room.room_no column and decide if a separate
non-unique index is needed for query performance; if you determine it is
required, add a CREATE INDEX IF NOT EXISTS idx_chat_room_room_no ON chat_room
(room_no); statement to schema.sql so the index defined by the `@Index`(name =
"idx_chat_room_room_no", columnList = "room_no") annotation is actually created
in the database.
In
`@src/main/java/com/project/dorumdorum/domain/room/application/usecase/DeleteRoomUseCase.java`:
- Around line 50-55: The deletion currently trusts room.getCurrentMateCount()
which can be out of sync; update DeleteRoomUseCase to verify using actual
Roommate rows by calling roommateService.findByRoom(room) (or add a new
roommateService.existsNonOwnerInRoom(room) method) and throw
RestApiException(ROOM_HAS_MEMBERS) if any roommate other than the owner is
returned, only then call room.delete() and
roommateService.leaveRoom(requesterNo, roomNo).
In
`@src/main/java/com/project/dorumdorum/global/security/JwtPrincipalHandshakeHandler.java`:
- Around line 20-23: In JwtPrincipalHandshakeHandler, avoid directly casting
attributes.get("userNo") to String; instead fetch it as an Object (e.g.,
userNoObj from attributes.get("userNo")), check for null and then normalize to a
String using an instanceof check or String.valueOf(...) so a Long or other types
won't throw ClassCastException, and pass that normalized String into new
StompPrincipal(userNo).
In
`@src/test/java/com/project/dorumdorum/domain/room/integration/DeleteRoomPersistenceIntegrationTest.java`:
- Around line 81-100: In DeleteRoomPersistenceIntegrationTest, stop setting
userNo in the User.builder() calls so Tsid can auto-generate IDs; instead
persist users via userRepository.save(User.builder()...).capture the returned
User (e.g., User host = userRepository.save(...), User applicant =
userRepository.save(...)) and use host.getUserNo() and applicant.getUserNo() for
subsequent operations; apply the same change pattern to
KickRoommatePersistenceIntegrationTest where userNo is currently being set
manually.
In
`@src/test/java/com/project/dorumdorum/domain/room/integration/KickRoommatePersistenceIntegrationTest.java`:
- Around line 78-97: Remove explicit setting of User#userNo in the test and
instead call userRepository.save(...) without userNo, capture the returned User
(e.g., hostUser and memberUser) and use hostUser.getUserNo() and
memberUser.getUserNo() when creating Room/Roommate entities; update references
in KickRoommatePersistenceIntegrationTest to use these returned IDs so Hibernate
treats them as managed entities rather than detached ones.
- Around line 99-108: 현재 roomRepository.save(...)로 영속화된 뒤 detached 상태에서
room.plusCurrentMate()를 호출해 변경이 DB에 반영되지 않습니다; 해결하려면 room.plusCurrentMate() 호출을
save 이전으로 옮기거나 plusCurrentMate() 실행 후 다시 roomRepository.save(room)로 변경을 저장하도록
수정하세요 (참조: Room class, roomRepository.save, room.plusCurrentMate,
roomRepository.findById).
In
`@src/test/java/com/project/dorumdorum/domain/room/unit/usecase/DeleteRoomUseCaseTest.java`:
- Around line 43-44: The test currently only checks exception type via
assertThatThrownBy(() -> useCase.execute("member1",
"r1")).isInstanceOf(RestApiException.class); — update each failing-case
assertion (the three occurrences covering NO_PERMISSION_ON_ROOM,
CANNOT_DELETE_COMPLETED_ROOM, ROOM_HAS_MEMBERS) to also assert the specific
error code on the thrown RestApiException (e.g., capture the thrown exception
from useCase.execute and assert its errorCode / getCode() equals the expected
enum/constant like NO_PERMISSION_ON_ROOM). Locate the assertions around
useCase.execute in DeleteRoomUseCaseTest and replace or extend them to both
assert the exception type and assert the exact error code value returned by the
RestApiException.
In
`@src/test/java/com/project/dorumdorum/domain/roommate/unit/infra/repository/RoommateRepositoryImplTest.java`:
- Around line 55-66: Add an integration test for
RoommateRepositoryImpl.findMyRoommates that persists real Roommate and Room
entities (not mocks) with mixed deletedAt values and verifies only non-deleted
records are returned; specifically create some entities with
myRoommate.deletedAt, roommate.deletedAt, and roommate.room.deletedAt set and
some null, call findMyRoommates("u1"), and assert the returned
List<MyRoommateResponse> includes only entries whose associated deletedAt fields
are null to ensure the deletedAt.isNull() filters are enforced.
In
`@src/test/java/com/project/dorumdorum/global/security/JwtPrincipalHandshakeHandlerTest.java`:
- Around line 16-36: Add a new unit test in JwtPrincipalHandshakeHandlerTest
that verifies the fallback path of JwtPrincipalHandshakeHandler.determineUser
when attributes do NOT contain "userNo": create an empty attributes map (or one
missing "userNo"), call ExposedJwtPrincipalHandshakeHandler.determine(request,
handler, attributes) and assert it does not throw and returns either null or the
expected fallback Principal (matching the current implementation behavior);
ensure the test covers both empty-map and null-value scenarios to prevent NPEs
by invoking determine with mock(ServerHttpRequest.class) and
mock(WebSocketHandler.class) similarly to
determineUser_WithUserNo_ReturnsStompPrincipal.
---
Outside diff comments:
In
`@src/main/java/com/project/dorumdorum/domain/chat/domain/repository/ChatRoomRepository.java`:
- Around line 16-28: ChatMessageRepository의 deleteByChatRoomNo 메서드가 현재
`@Modifying`(clearAutomatically = true)만 적용되어 있어 ChatRoomRepository의
deleteByChatRoomNo와 일관성이 없으니, 메서드 deleteByChatRoomNo(`@Param`("chatRoomNo") String
chatRoomNo) 선언을 찾아 `@Modifying에` flushAutomatically = true를 추가하고 기존 JPQL("DELETE
FROM ChatMessage m WHERE m.chatRoom.chatRoomNo = :chatRoomNo") 및 `@Query` 어노테이션은
그대로 유지하여 bulk 삭제 시 자동 플러시 동작이 ChatRoomRepository와 동일하도록 수정하세요.
In
`@src/test/java/com/project/dorumdorum/domain/room/unit/usecase/KickRoommateUseCaseTest.java`:
- Around line 44-105: Add an InOrder verification for the success test to assert
the exact sequence leaveRoom → minusCurrentMate → roomService.flush →
publishEvent: create an InOrder over roommateService, room, roomService,
eventPublisher and verify in order that roommateService.leaveRoom(...) is
called, then room.minusCurrentMate(), then roomService.flush(), then
eventPublisher.publishEvent(...) (using the same arguments/assertions as the
existing verify calls); leave all existing exception-path tests unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b9822f6c-67ad-4b37-b5c3-837b4bbe7cff
📒 Files selected for processing (37)
src/main/java/com/project/dorumdorum/domain/chat/application/dto/response/NotificationMessage.javasrc/main/java/com/project/dorumdorum/domain/chat/application/event/RoomDeletedEventListener.javasrc/main/java/com/project/dorumdorum/domain/chat/application/event/RoommateKickedEventListener.javasrc/main/java/com/project/dorumdorum/domain/chat/domain/entity/ChatRoom.javasrc/main/java/com/project/dorumdorum/domain/chat/domain/repository/ChatMessageRepository.javasrc/main/java/com/project/dorumdorum/domain/chat/domain/repository/ChatRoomMemberRepository.javasrc/main/java/com/project/dorumdorum/domain/chat/domain/repository/ChatRoomRepository.javasrc/main/java/com/project/dorumdorum/domain/chat/domain/service/ChatRoomMemberService.javasrc/main/java/com/project/dorumdorum/domain/chat/domain/service/ChatRoomService.javasrc/main/java/com/project/dorumdorum/domain/checklist/domain/repository/RoomRuleRepository.javasrc/main/java/com/project/dorumdorum/domain/checklist/domain/service/RoomRuleService.javasrc/main/java/com/project/dorumdorum/domain/room/application/event/RoomDeletedEvent.javasrc/main/java/com/project/dorumdorum/domain/room/application/usecase/DeleteRoomUseCase.javasrc/main/java/com/project/dorumdorum/domain/room/application/usecase/KickRoommateUseCase.javasrc/main/java/com/project/dorumdorum/domain/room/domain/repository/RoomLikeRepository.javasrc/main/java/com/project/dorumdorum/domain/room/domain/repository/RoomRepository.javasrc/main/java/com/project/dorumdorum/domain/room/domain/repository/RoomRequestRepository.javasrc/main/java/com/project/dorumdorum/domain/room/domain/service/RoomRequestService.javasrc/main/java/com/project/dorumdorum/domain/room/domain/service/RoomService.javasrc/main/java/com/project/dorumdorum/domain/room/ui/DeleteRoomController.javasrc/main/java/com/project/dorumdorum/domain/room/ui/spec/DeleteRoomApiSpec.javasrc/main/java/com/project/dorumdorum/domain/roommate/domain/service/RoommateService.javasrc/main/java/com/project/dorumdorum/domain/roommate/infra/repository/RoommateRepositoryImpl.javasrc/main/java/com/project/dorumdorum/global/config/WebSocketConfig.javasrc/main/java/com/project/dorumdorum/global/exception/code/status/RoomErrorStatus.javasrc/main/java/com/project/dorumdorum/global/security/ChatRoomAuthorizationInterceptor.javasrc/main/java/com/project/dorumdorum/global/security/JwtPrincipalHandshakeHandler.javasrc/main/java/com/project/dorumdorum/global/security/StompPrincipal.javasrc/test/java/com/project/dorumdorum/domain/chat/unit/usecase/RoomDeletedEventListenerTest.javasrc/test/java/com/project/dorumdorum/domain/chat/unit/usecase/RoommateKickedEventListenerTest.javasrc/test/java/com/project/dorumdorum/domain/room/integration/DeleteRoomPersistenceIntegrationTest.javasrc/test/java/com/project/dorumdorum/domain/room/integration/KickRoommatePersistenceIntegrationTest.javasrc/test/java/com/project/dorumdorum/domain/room/unit/usecase/DeleteRoomUseCaseTest.javasrc/test/java/com/project/dorumdorum/domain/room/unit/usecase/KickRoommateUseCaseTest.javasrc/test/java/com/project/dorumdorum/domain/roommate/unit/infra/repository/RoommateRepositoryImplTest.javasrc/test/java/com/project/dorumdorum/global/security/ChatRoomAuthorizationInterceptorTest.javasrc/test/java/com/project/dorumdorum/global/security/JwtPrincipalHandshakeHandlerTest.java
Comment on lines
+47
to
+58
| // 채팅방 열려 있는 유저에게 브로드캐스트 (/topic/) | ||
| broadcastDeletionSafely(chatRoom, notification); | ||
|
|
||
| // DIRECT 채팅방은 지원자가 다른 화면에 있어도 받을 수 있도록 개인 큐로 추가 전송 (/queue/) | ||
| if (ChatRoomType.DIRECT.equals(chatRoom.getChatRoomType()) | ||
| && chatRoom.getApplicantUserNo() != null) { | ||
| notifyUserSafely(chatRoom.getApplicantUserNo(), notification); | ||
| } | ||
|
|
||
| chatMessageService.deleteAllByChatRoom(chatRoom.getChatRoomNo()); | ||
| chatRoomMemberService.deleteAllByChatRoom(chatRoom); | ||
| chatRoomService.deleteByChatRoomNo(chatRoom.getChatRoomNo()); |
There was a problem hiding this comment.
삭제 알림은 커밋 이후에 보내도록 분리해 주세요.
Line 48과 Line 53에서 WebSocket 알림을 먼저 보낸 뒤 Line 56-58의 DB 삭제가 실패하면, 트랜잭션은 롤백되지만 이미 발송된 “방 삭제” 알림은 되돌릴 수 없습니다. 채팅 데이터 정리는 BEFORE_COMMIT에 두더라도, 사용자 알림은 삭제 대상 chatRoomNo/수신자 정보를 별도 이벤트나 outbox에 담아 AFTER_COMMIT에서 전송하도록 분리하는 편이 안전합니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/main/java/com/project/dorumdorum/domain/chat/application/event/RoomDeletedEventListener.java`
around lines 47 - 58, The current RoomDeletedEventListener sends WebSocket
notifications (broadcastDeletionSafely and notifyUserSafely) before performing
DB deletes (chatMessageService.deleteAllByChatRoom,
chatRoomMemberService.deleteAllByChatRoom, chatRoomService.deleteByChatRoomNo),
which can leak notifications on rollback; change this so DB deletes execute
inside the transaction and notification sending runs only AFTER commit.
Concretely, stop calling broadcastDeletionSafely and notifyUserSafely directly
in RoomDeletedEventListener; instead capture the minimal notification payload
(chatRoomNo, recipientUserNo(s)) and either (a) register a
TransactionSynchronization.afterCommit callback to dispatch those payloads, or
(b) publish a new ChatRoomDeletedNotificationEvent/outbox entry from
RoomDeletedEventListener and handle sending in an AFTER_COMMIT listener (e.g.,
ChatRoomDeletedNotificationHandler) that calls
broadcastDeletionSafely/notifyUserSafely. Ensure
ChatRoomDeletedNotificationEvent contains chatRoomNo and recipient IDs and that
only the after-commit handler invokes the WebSocket methods.
Comment on lines
43
to
68
| @TransactionalEventListener(phase = TransactionPhase.BEFORE_COMMIT) | ||
| public void handle(RoommateKickedEvent event) { | ||
| chatRoomService.findByRoomNo(event.roomNo()).ifPresent(chatRoom -> { | ||
| if (chatRoomMemberService.isMember(chatRoom, event.kickedUserNo())) { | ||
| ChatRoomMember member = chatRoomMemberService.findByChatRoomAndUserNo(chatRoom, event.kickedUserNo()); | ||
| LocalDateTime fromTime = member.getLastReadAt() != null | ||
| ? member.getLastReadAt() | ||
| : member.getJoinedAt(); | ||
| chatMessageService.decreaseUnreadCount(chatRoom.getChatRoomNo(), fromTime, event.kickedUserNo()); | ||
| chatRoomMemberService.leave(member); | ||
| User kicked = userService.findById(event.kickedUserNo()); | ||
| String displayName = (kicked.getNickname() != null && !kicked.getNickname().isBlank()) | ||
| ? kicked.getNickname() : kicked.getName(); | ||
| String content = displayName + "가 퇴장했습니다."; | ||
| ChatMessage message = chatMessageService.save(chatRoom, "SYSTEM", content, MessageType.SYSTEM, 0); | ||
| ChatMessageResponse response = new ChatMessageResponse( | ||
| message.getMessageNo(), chatRoom.getChatRoomNo(), | ||
| "SYSTEM", null, content, MessageType.SYSTEM.name(), message.getCreatedAt(), message.getUnreadCount()); | ||
| broadcastSafely(chatRoom, response); | ||
| } | ||
| }); | ||
| chatRoomService.findByRoomNo(event.roomNo()).ifPresent(chatRoom -> | ||
| chatRoomMemberService.findOptionalByChatRoomAndUserNo(chatRoom, event.kickedUserNo()) | ||
| .ifPresent(member -> processKick(chatRoom, member, event)) | ||
| ); | ||
| } | ||
|
|
||
| private void processKick(ChatRoom chatRoom, ChatRoomMember member, RoommateKickedEvent event) { | ||
| LocalDateTime fromTime = member.getLastReadAt() != null | ||
| ? member.getLastReadAt() | ||
| : member.getJoinedAt(); | ||
| chatMessageService.decreaseUnreadCount(chatRoom.getChatRoomNo(), fromTime, event.kickedUserNo()); | ||
| chatRoomMemberService.leave(member); | ||
| User kicked = userService.findById(event.kickedUserNo()); | ||
| String displayName = (kicked.getNickname() != null && !kicked.getNickname().isBlank()) | ||
| ? kicked.getNickname() : kicked.getName(); | ||
| String content = displayName + "가 퇴장했습니다."; | ||
| ChatMessage message = chatMessageService.save(chatRoom, "SYSTEM", content, MessageType.SYSTEM, 0); | ||
| ChatMessageResponse response = new ChatMessageResponse( | ||
| message.getMessageNo(), chatRoom.getChatRoomNo(), | ||
| "SYSTEM", null, content, MessageType.SYSTEM.name(), message.getCreatedAt(), message.getUnreadCount()); | ||
| broadcastSafely(chatRoom, response); | ||
| notifyUserSafely(event.kickedUserNo(), | ||
| NotificationMessage.kicked(event.roomNo(), chatRoom.getChatRoomNo())); | ||
| } |
There was a problem hiding this comment.
BEFORE_COMMIT 단계에서의 WebSocket 알림 순서에 대한 검토 요청드립니다.
현재 processKick(...)은 BEFORE_COMMIT 단계에서 DB 변경(decreaseUnreadCount, leave, 시스템 메시지 save)과 브로드캐스트/개인 알림 전송을 함께 수행하고 있습니다. broadcastSafely/notifyUserSafely가 예외를 삼켜 트랜잭션에 영향은 주지 않지만, 반대 방향의 문제가 남습니다:
- 알림은 이미 나갔는데 이후 동일 트랜잭션(부모
KickRoommateUseCase)의 다른 작업이 실패해 롤백되면, 강퇴당하지 않은 사용자에게 "강퇴됨" 알림이 전달된 상태가 됩니다. 시스템 메시지(ChatMessage)도 구독자들은 이미 받은 상태인데 DB에는 존재하지 않게 됩니다.
DB 정합성은 BEFORE_COMMIT에 유지하되, 외부로 나가는 브로드캐스트/개인 알림만 AFTER_COMMIT 시점으로 분리하는 것이 더 안전해 보입니다. 예: processKick을 "상태 변경 + 페이로드 준비"와 "외부 전송"으로 쪼개고 후자를 TransactionSynchronizationManager.registerSynchronization(... afterCommit ...) 또는 별도 AFTER_COMMIT 리스너로 실행하는 방식이요. 의도적으로 BEFORE_COMMIT에서 같이 내보내는 설계라면, 클래스 주석에 "커밋 실패 시 오발송 가능" 트레이드오프도 함께 명시해 주시면 좋겠습니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/main/java/com/project/dorumdorum/domain/chat/application/event/RoommateKickedEventListener.java`
around lines 43 - 68, The listener RoommateKickedEventListener currently
performs DB state changes (chatMessageService.decreaseUnreadCount,
chatRoomMemberService.leave, chatMessageService.save) and sends external
notifications (broadcastSafely, notifyUserSafely) inside a
`@TransactionalEventListener`(phase = TransactionPhase.BEFORE_COMMIT) via
processKick(...), which can cause out-of-sync external notifications if the
transaction later rolls back; split processKick into two parts: (1) perform and
persist state changes and assemble the payload (message and ChatMessageResponse)
during BEFORE_COMMIT, and (2) defer the external sends (calls to broadcastSafely
and notifyUserSafely) to AFTER_COMMIT using
TransactionSynchronizationManager.registerSynchronization(... afterCommit ...)
or move sending into a separate `@TransactionalEventListener`(phase =
AFTER_COMMIT) that receives the prepared payload, ensuring only committed
changes trigger broadcasts; alternatively, document the intentional trade-off in
the class javadoc if you choose to keep current behavior.
Comment on lines
+78
to
+97
| userRepository.save(User.builder() | ||
| .userNo(HOST_NO) | ||
| .email("kick-host@gachon.ac.kr") | ||
| .password("pw") | ||
| .role(Role.USER) | ||
| .studentNo("202400101") | ||
| .name("Host") | ||
| .nickname("HostNick") | ||
| .gender(Gender.MALE) | ||
| .build()); | ||
| userRepository.save(User.builder() | ||
| .userNo(MEMBER_NO) | ||
| .email("kick-member@gachon.ac.kr") | ||
| .password("pw") | ||
| .role(Role.USER) | ||
| .studentNo("202400102") | ||
| .name("Member") | ||
| .nickname("MemberNick") | ||
| .gender(Gender.MALE) | ||
| .build()); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: User 엔티티의 ID 생성 전략과 테스트의 pre-assigned ID 사용 지점을 확인합니다.
rg -n -C4 '@Id|@GeneratedValue|class User|String userNo|implements Persistable|userNo\\(' src/main/java src/test/javaRepository: DorumDorum/BE
Length of output: 261
🏁 Script executed:
#!/bin/bash
# Find User entity file
echo "=== Finding User entity ==="
fd -type f -name "User.java" src/main/javaRepository: DorumDorum/BE
Length of output: 272
🏁 Script executed:
#!/bin/bash
# Search for `@GeneratedValue`, `@Version`, and ID-related annotations in main source
echo "=== Searching for ID generation strategy ==="
rg -n '@Id|@GeneratedValue|@Version' src/main/java --type java -A 2Repository: DorumDorum/BE
Length of output: 4716
🏁 Script executed:
#!/bin/bash
# Search for userNo field and pre-assigned ID usage in test
echo "=== Checking pre-assigned ID in test ==="
rg -n 'userNo\(|HOST_NO|MEMBER_NO' src/test/java/com/project/dorumdorum/domain/room/integration/KickRoommatePersistenceIntegrationTest.java -B 2 -A 2Repository: DorumDorum/BE
Length of output: 2807
🏁 Script executed:
#!/bin/bash
# Check for `@Version` annotation in entities that might cause optimistic locking
echo "=== Checking for `@Version` annotations ==="
rg -n '@Version' src/main/java --type-list java -A 2Repository: DorumDorum/BE
Length of output: 505
🏁 Script executed:
#!/bin/bash
# Check User entity full definition including `@Version`
cat -n src/main/java/com/project/dorumdorum/domain/user/domain/entity/User.java | head -50Repository: DorumDorum/BE
Length of output: 1595
🏁 Script executed:
#!/bin/bash
# Check Room entity for `@Version`
cat -n src/main/java/com/project/dorumdorum/domain/room/domain/entity/Room.java | head -50Repository: DorumDorum/BE
Length of output: 1940
🏁 Script executed:
#!/bin/bash
# Check BaseEntity for `@Version` annotation
find src/main/java -name "BaseEntity.java" -exec cat -n {} \;Repository: DorumDorum/BE
Length of output: 1156
자동 생성되는 User ID를 테스트에서 미리 설정하지 말아 주세요.
TSID로 자동 생성되도록 설정된 User#userNo를 테스트에서 미리 "kick-host-001", "kick-member-001"로 고정하면, Spring Data JPA가 이를 detached entity로 판단해 merge 경로를 실행하게 됩니다. 실제 DB에 해당 ID를 가진 row가 없으므로 Hibernate가 stale object로 판단하고 저장 시점에 예외를 던집니다.
대신 ID를 설정하지 않고 save 후 반환된 user 객체의 getUserNo()를 활용해 주세요:
수정 예시
User hostUser = userRepository.save(User.builder()
.email("kick-host@gachon.ac.kr")
.password("pw")
.role(Role.USER)
.studentNo("202400101")
.name("Host")
.nickname("HostNick")
.gender(Gender.MALE)
.build());
String hostUserNo = hostUser.getUserNo();
User memberUser = userRepository.save(User.builder()
.email("kick-member@gachon.ac.kr")
.password("pw")
.role(Role.USER)
.studentNo("202400102")
.name("Member")
.nickname("MemberNick")
.gender(Gender.MALE)
.build());
String memberUserNo = memberUser.getUserNo();
// 이후 Room, Roommate 등에서 hostUserNo, memberUserNo 사용🧰 Tools
🪛 GitHub Actions: cicd
[error] 78-78: KickRoommate 영속성 통합 테스트 실패: org.springframework.orm.ObjectOptimisticLockingFailureException caused by org.hibernate.StaleObjectStateException.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/test/java/com/project/dorumdorum/domain/room/integration/KickRoommatePersistenceIntegrationTest.java`
around lines 78 - 97, Remove explicit setting of User#userNo in the test and
instead call userRepository.save(...) without userNo, capture the returned User
(e.g., hostUser and memberUser) and use hostUser.getUserNo() and
memberUser.getUserNo() when creating Room/Roommate entities; update references
in KickRoommatePersistenceIntegrationTest to use these returned IDs so Hibernate
treats them as managed entities rather than detached ones.
Comment on lines
+55
to
+66
| @Test | ||
| @DisplayName("룸메이트가 없으면 빈 리스트 반환") | ||
| @SuppressWarnings({"unchecked", "rawtypes"}) | ||
| void findMyRoommates_WhenNoResults_ReturnsEmptyList() { | ||
| when(queryFactory.select(org.mockito.ArgumentMatchers.<Expression<?>>any())).thenReturn(query); | ||
| when(query.fetch()).thenReturn(List.of()); | ||
|
|
||
| RoommateRepositoryImpl repository = new RoommateRepositoryImpl(queryFactory); | ||
| List<MyRoommateResponse> result = repository.findMyRoommates("u1"); | ||
|
|
||
| assertThat(result).isEmpty(); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: findMyRoommates 관련 테스트가 deletedAt/soft-delete 케이스를 검증하는지 확인합니다.
rg -n -C3 'findMyRoommates|deletedAt|delete\(\)|isDeleted' src/test src/mainRepository: DorumDorum/BE
Length of output: 18577
삭제 필터 회귀를 방지하기 위해 통합 테스트를 추가해 보세요.
현재 테스트는 mock을 사용하여 fetch() 결과를 직접 반환하기 때문에, 실제 findMyRoommates() 구현의 deletedAt.isNull() 조건들(myRoommate.deletedAt.isNull(), roommate.deletedAt.isNull(), roommate.room.deletedAt.isNull())이 제대로 작동하는지 검증할 수 없어요. 삭제된 룸메이트/방 데이터를 섞어서 저장한 통합 테스트를 추가하면 이런 필터들이 올바르게 작동하는지 확실히 확인할 수 있습니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/test/java/com/project/dorumdorum/domain/roommate/unit/infra/repository/RoommateRepositoryImplTest.java`
around lines 55 - 66, Add an integration test for
RoommateRepositoryImpl.findMyRoommates that persists real Roommate and Room
entities (not mocks) with mixed deletedAt values and verifies only non-deleted
records are returned; specifically create some entities with
myRoommate.deletedAt, roommate.deletedAt, and roommate.room.deletedAt set and
some null, call findMyRoommates("u1"), and assert the returned
List<MyRoommateResponse> includes only entries whose associated deletedAt fields
are null to ensure the deletedAt.isNull() filters are enforced.
Comment on lines
+16
to
+36
| class JwtPrincipalHandshakeHandlerTest { | ||
|
|
||
| private final ExposedJwtPrincipalHandshakeHandler handshakeHandler = new ExposedJwtPrincipalHandshakeHandler(); | ||
|
|
||
| @Test | ||
| @DisplayName("handshake attributes에 userNo가 있으면 StompPrincipal을 반환한다") | ||
| void determineUser_WithUserNo_ReturnsStompPrincipal() { | ||
| Map<String, Object> attributes = new HashMap<>(); | ||
| attributes.put("userNo", "user-1"); | ||
|
|
||
| Principal principal = handshakeHandler.determine(mock(ServerHttpRequest.class), mock(WebSocketHandler.class), attributes); | ||
|
|
||
| assertThat(principal).isNotNull(); | ||
| assertThat(principal.getName()).isEqualTo("user-1"); | ||
| } | ||
|
|
||
| private static final class ExposedJwtPrincipalHandshakeHandler extends JwtPrincipalHandshakeHandler { | ||
| Principal determine(ServerHttpRequest request, WebSocketHandler handler, Map<String, Object> attributes) { | ||
| return super.determineUser(request, handler, attributes); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
LGTM!
protected determineUser를 테스트하기 위해 서브클래스로 노출하시는 방식이 깔끔합니다. 다만 userNo가 없는 경우 super.determineUser로 폴백하는 경로에 대한 테스트도 추가해 두시면 회귀 방지에 도움이 될 것 같습니다 (예: attributes가 비어 있을 때 NPE 없이 동작하는지 등).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/test/java/com/project/dorumdorum/global/security/JwtPrincipalHandshakeHandlerTest.java`
around lines 16 - 36, Add a new unit test in JwtPrincipalHandshakeHandlerTest
that verifies the fallback path of JwtPrincipalHandshakeHandler.determineUser
when attributes do NOT contain "userNo": create an empty attributes map (or one
missing "userNo"), call ExposedJwtPrincipalHandshakeHandler.determine(request,
handler, attributes) and assert it does not throw and returns either null or the
expected fallback Principal (matching the current implementation behavior);
ensure the test covers both empty-map and null-value scenarios to prevent NPEs
by invoking determine with mock(ServerHttpRequest.class) and
mock(WebSocketHandler.class) similarly to
determineUser_WithUserNo_ReturnsStompPrincipal.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 42 out of 42 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+44
to
+46
| NotificationMessage notification = NotificationMessage.roomDeleted(event.roomNo(), chatRoom.getChatRoomNo()); | ||
| broadcasts.add(new ChatWebSocketNotificationEvent.BroadcastTask(chatRoom.getChatRoomNo(), notification)); | ||
|
|
There was a problem hiding this comment.
RoomDeletedEventListener가 /topic/chat-room/{chatRoomNo}에 NotificationMessage를 브로드캐스트하고 있는데, 기존 코드 경로(예: SendGroupChatMessageUseCase/JoinChatRoomUseCase/LeaveChatRoomUseCase)는 동일 destination에 ChatMessageResponse만 전송하고 있습니다. 동일 destination에서 서로 다른 payload 스키마를 섞으면 클라이언트 역직렬화/핸들링이 깨질 가능성이 커서, (1) 별도 destination(/topic/notification 등)으로 분리하거나 (2) 공통 envelope(type 필드 포함)로 통일하거나 (3) 삭제 알림도 SYSTEM ChatMessageResponse 형태로 맞추는 쪽을 권장합니다.
Comment on lines
+41
to
+42
| CREATE INDEX IF NOT EXISTS idx_chat_room_room_no | ||
| ON chat_room (room_no); |
There was a problem hiding this comment.
application-dev/prod 설정이 ddl-auto=validate이고 spring.sql.init이 비활성화되어 있어서(테스트 프로필만 schema.sql 로드), 여기 schema.sql에 추가한 idx_chat_room_room_no 인덱스는 운영/개발 DB에는 자동 반영되지 않습니다. 운영 성능 개선이 목적이라면 별도 마이그레이션(예: Flyway/Liquibase 또는 배포용 DDL 스크립트/Runbook)으로 적용 경로를 마련하는 게 필요합니다.
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (1)
src/main/java/com/project/dorumdorum/domain/room/application/usecase/DeleteRoomUseCase.java (1)
51-53:⚠️ Potential issue | 🟠 Major[이전 리뷰 재지적]
currentMateCount대신 실제 Roommate row 기준으로 검증해 주세요.이 PR 자체가
current_mate_count/remaining동기화 버그를 타깃으로 삼고 있는 만큼, 삭제 가능 여부를 캐시된 카운트에만 의존하는 것은 위험합니다. 카운트가 실제Roommate테이블과 어긋난 상태에서는 방장 외 룸메이트가 남아 있는데도 방이 소프트 삭제되고, 해당 룸메이트는findMyRoommates의deletedAt IS NULL필터에 의해 갑자기 방을 잃어버리는 상황이 발생할 수 있습니다.개선 방향으로는
RoommateService에 "방장 외 룸메이트 존재 여부"를 직접 확인하는 쿼리 메서드(예:existsNonHostRoommate(roomNo))를 추가해 실제 row 기준으로 검증하시는 것을 제안드립니다.♻️ 제안 수정 예시
- if (room.getCurrentMateCount() > 1) { - throw new RestApiException(ROOM_HAS_MEMBERS); - } + if (roommateService.existsNonHostRoommate(roomNo)) { + throw new RestApiException(ROOM_HAS_MEMBERS); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/project/dorumdorum/domain/room/application/usecase/DeleteRoomUseCase.java` around lines 51 - 53, 현재 DeleteRoomUseCase에서 room.getCurrentMateCount()로 삭제 가능 여부를 판단하는 대신 실제 Roommate 테이블 기준으로 검증하도록 변경하세요: RoommateService에 existsNonHostRoommate(roomNo) 같은 메서드를 추가해 deletedAt IS NULL이면서 host가 아닌 룸메이트가 존재하는지 쿼리하고, DeleteRoomUseCase에서 room.getCurrentMateCount() 체크를 제거한 뒤 해당 서비스 메서드가 true면 기존처럼 throw new RestApiException(ROOM_HAS_MEMBERS); 하도록 교체하세요 (참조: DeleteRoomUseCase, RoommateService, existsNonHostRoommate, ROOM_HAS_MEMBERS).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/main/java/com/project/dorumdorum/domain/chat/application/event/ChatWebSocketNotificationEventListener.java`:
- Around line 16-23: The handle method in ChatWebSocketNotificationEventListener
should defend against one task's exception aborting the rest: add `@Slf4j` to the
class and wrap each broadcast and notifyUser invocation inside its own try/catch
so exceptions from chatWebSocketSendService.broadcast(...) or .notifyUser(...)
are caught, logged (include the task identifiers and exception), and swallowed
so the loop continues; also verify recoverSend signature and `@EnableRetry`
configuration remain correct but do not rely on `@Retryable` recovery to prevent
skipping subsequent tasks.
In
`@src/main/java/com/project/dorumdorum/domain/chat/infra/websocket/ChatWebSocketSendService.java`:
- Around line 18-32: The retry scope is too broad and the single generic
`@Recover` may become ambiguous; for methods broadcast and notifyUser narrow
retries to messaging delivery/network exceptions (e.g., retryFor =
MessageDeliveryException.class or MessagingException.class) and exclude
non-retriable errors with noRetryFor = {MessageConversionException.class,
IllegalArgumentException.class}, and replace the single recoverSend with two
explicit recover methods (e.g., recoverBroadcast(Exception e, String chatRoomNo,
Object payload) and recoverNotify(Exception e, String userNo, Object payload))
so each `@Retryable` (broadcast, notifyUser) has a matching `@Recover` signature.
In `@src/main/java/com/project/dorumdorum/global/config/WebSocketConfig.java`:
- Around line 25-29: The WebSocket config currently allows all origins via
setAllowedOriginPatterns("*") which is unsafe for cookie-based auth: inject a
config property (e.g. String[] allowedWebSocketOrigins or property key
app.websocket.allowed-origins) into WebSocketConfig and replace
setAllowedOriginPatterns("*") with
setAllowedOriginPatterns(allowedWebSocketOrigins) (or call setAllowedOrigins if
you prefer exact matches), keeping the rest of
registry.addEndpoint("/ws").setHandshakeHandler(jwtPrincipalHandshakeHandler).addInterceptors(jwtHandshakeInterceptor).withSockJS()
unchanged so JwtHandshakeInterceptor continues to authenticate from the
accessToken cookie but only for configured frontend domains.
In `@src/main/resources/schema.sql`:
- Around line 41-42: You added a new index idx_chat_room_room_no on
chat_room(room_no) which may be redundant with existing
uk_chat_room_direct(room_no, applicant_user_no) and the partial unique index
uk_chat_room_group(room_no) WHERE chat_room_type = 'GROUP'; run EXPLAIN ANALYZE
for the queries used by findAllByRoomNo(...) to confirm whether the
single-column index actually changes the planner choice or performance, and if
it doesn't, remove idx_chat_room_room_no to avoid extra write overhead —
otherwise document why the standalone index is required (e.g., to cover queries
across both DIRECT and GROUP when the partial index cannot) and keep it.
In
`@src/test/java/com/project/dorumdorum/domain/chat/unit/usecase/RoommateKickedEventListenerTest.java`:
- Around line 146-170: The test currently only asserts broadcasts size; enhance
it to assert the BroadcastTask's chatRoomNo equals "room-1" and that its payload
is a ChatMessageResponse (or the expected message wrapper) with
MessageType.SYSTEM and the expected system message content; locate the published
event captured from listener.handle(new
RoommateKickedEvent("room-1","user-kicked")), get event.broadcasts().get(0) (the
BroadcastTask) and add assertions on its chatRoomNo and payload type/fields to
ensure the broadcast payload is correct.
- Around line 111-127: In the test method handle_WhenNicknameNull_UsesName
remove the unused local variable by either calling givenMember(chatRoom,
"user-kicked") without assigning it or reusing the returned ChatRoomMember
(member) in an assertion (e.g., verify a leave(member) call) — locate the
givenMember(...) call and the local variable member and either drop the
assignment or add the proper verification that uses member so the local variable
is not unused.
In
`@src/test/java/com/project/dorumdorum/domain/room/integration/DeleteRoomPersistenceIntegrationTest.java`:
- Around line 179-186: 현재 테스트가 roomRequestRepository.findAll(),
chatRoomMemberRepository.findAll(), chatMessageRepository.findAll()로 전역 조회해
비어있음을 단정하고 있으니, DeleteRoomPersistenceIntegrationTest의 검증 범위를 삭제 대상 room에 한정하도록
바꿔주세요: roomRequestRepository.findByRoomNo(room.getRoomNo()) 또는
findAllByRoomNo(room.getRoomNo()) 같은 room-스코프 조회 메서드를 사용해 해당 room의 요청이 비어있는지
확인하고, chatRoomMemberRepository 및 chatMessageRepository도 각각
findByRoomNo/findAllByRoomNo( room.getRoomNo() ) 형태로 변경해 해당 room의 멤버와 메시지만 검사하도록
수정하세요.
---
Duplicate comments:
In
`@src/main/java/com/project/dorumdorum/domain/room/application/usecase/DeleteRoomUseCase.java`:
- Around line 51-53: 현재 DeleteRoomUseCase에서 room.getCurrentMateCount()로 삭제 가능
여부를 판단하는 대신 실제 Roommate 테이블 기준으로 검증하도록 변경하세요: RoommateService에
existsNonHostRoommate(roomNo) 같은 메서드를 추가해 deletedAt IS NULL이면서 host가 아닌 룸메이트가
존재하는지 쿼리하고, DeleteRoomUseCase에서 room.getCurrentMateCount() 체크를 제거한 뒤 해당 서비스 메서드가
true면 기존처럼 throw new RestApiException(ROOM_HAS_MEMBERS); 하도록 교체하세요 (참조:
DeleteRoomUseCase, RoommateService, existsNonHostRoommate, ROOM_HAS_MEMBERS).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7c2f7be2-d8ca-40c8-8bc4-5d53fedad235
📒 Files selected for processing (16)
src/main/java/com/project/dorumdorum/domain/chat/application/dto/response/NotificationMessage.javasrc/main/java/com/project/dorumdorum/domain/chat/application/dto/response/NotificationType.javasrc/main/java/com/project/dorumdorum/domain/chat/application/event/ChatWebSocketNotificationEvent.javasrc/main/java/com/project/dorumdorum/domain/chat/application/event/ChatWebSocketNotificationEventListener.javasrc/main/java/com/project/dorumdorum/domain/chat/application/event/RoomDeletedEventListener.javasrc/main/java/com/project/dorumdorum/domain/chat/application/event/RoommateKickedEventListener.javasrc/main/java/com/project/dorumdorum/domain/chat/infra/websocket/ChatWebSocketSendService.javasrc/main/java/com/project/dorumdorum/domain/room/application/usecase/DeleteRoomUseCase.javasrc/main/java/com/project/dorumdorum/global/config/WebSocketConfig.javasrc/main/resources/schema.sqlsrc/test/java/com/project/dorumdorum/domain/chat/unit/usecase/RoomDeletedEventListenerTest.javasrc/test/java/com/project/dorumdorum/domain/chat/unit/usecase/RoommateKickedEventListenerTest.javasrc/test/java/com/project/dorumdorum/domain/room/integration/DeleteRoomPersistenceIntegrationTest.javasrc/test/java/com/project/dorumdorum/domain/room/integration/KickRoommatePersistenceIntegrationTest.javasrc/test/java/com/project/dorumdorum/domain/room/unit/usecase/DeleteRoomUseCaseTest.javasrc/test/java/com/project/dorumdorum/global/security/JwtPrincipalHandshakeHandlerTest.java
Comment on lines
+16
to
+23
| @Async("notificationExecutor") | ||
| @TransactionalEventListener(phase = TransactionPhase.AFTER_COMMIT, fallbackExecution = false) | ||
| public void handle(ChatWebSocketNotificationEvent event) { | ||
| event.broadcasts().forEach(task -> | ||
| chatWebSocketSendService.broadcast(task.chatRoomNo(), task.payload())); | ||
| event.userNotifications().forEach(task -> | ||
| chatWebSocketSendService.notifyUser(task.userNo(), task.payload())); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
부분 실패 시 후속 태스크가 누락될 수 있는 점 확인 부탁드립니다.
broadcasts/userNotifications를 순차적으로 forEach로 돌리는 중, ChatWebSocketSendService.broadcast(...)가 @Retryable 3회 시도를 모두 소진해 최종 예외를 던지면 @Recover가 호출되어 정상 복구되므로 보통은 문제가 없습니다. 다만 recoverSend(...)의 시그니처가 매개변수 순서와 정확히 매칭되지 않아 복구가 실패하는 경우(또는 @EnableRetry 미설정 등)에는 예외가 forEach 바깥으로 전파되어 나머지 태스크 전송이 모두 누락될 수 있습니다.
방어적으로 각 태스크 전송을 try/catch로 감싸서 개별 실패가 후속 태스크에 영향을 주지 않도록 하면 조금 더 견고해집니다.
♻️ 제안 수정안
- event.broadcasts().forEach(task ->
- chatWebSocketSendService.broadcast(task.chatRoomNo(), task.payload()));
- event.userNotifications().forEach(task ->
- chatWebSocketSendService.notifyUser(task.userNo(), task.payload()));
+ event.broadcasts().forEach(task -> {
+ try {
+ chatWebSocketSendService.broadcast(task.chatRoomNo(), task.payload());
+ } catch (Exception e) {
+ log.warn("[Chat] broadcast 전파 실패. chatRoomNo={}", task.chatRoomNo(), e);
+ }
+ });
+ event.userNotifications().forEach(task -> {
+ try {
+ chatWebSocketSendService.notifyUser(task.userNo(), task.payload());
+ } catch (Exception e) {
+ log.warn("[Chat] notifyUser 전파 실패. userNo={}", task.userNo(), e);
+ }
+ });(로깅을 위해 @Slf4j도 함께 추가해주세요.)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/main/java/com/project/dorumdorum/domain/chat/application/event/ChatWebSocketNotificationEventListener.java`
around lines 16 - 23, The handle method in
ChatWebSocketNotificationEventListener should defend against one task's
exception aborting the rest: add `@Slf4j` to the class and wrap each broadcast and
notifyUser invocation inside its own try/catch so exceptions from
chatWebSocketSendService.broadcast(...) or .notifyUser(...) are caught, logged
(include the task identifiers and exception), and swallowed so the loop
continues; also verify recoverSend signature and `@EnableRetry` configuration
remain correct but do not rely on `@Retryable` recovery to prevent skipping
subsequent tasks.
Comment on lines
+18
to
+32
| @Retryable(retryFor = Exception.class, maxAttempts = 3, backoff = @Backoff(delay = 1000, multiplier = 2)) | ||
| public void broadcast(String chatRoomNo, Object payload) { | ||
| messagingTemplate.convertAndSend("/topic/chat-room/" + chatRoomNo, payload); | ||
| } | ||
|
|
||
| @Retryable(retryFor = Exception.class, maxAttempts = 3, backoff = @Backoff(delay = 1000, multiplier = 2)) | ||
| public void notifyUser(String userNo, Object payload) { | ||
| messagingTemplate.convertAndSendToUser(userNo, "/queue/notification", payload); | ||
| } | ||
|
|
||
| // broadcast / notifyUser 모두 (String, Object) 시그니처이므로 단일 recover로 처리 | ||
| @Recover | ||
| public void recoverSend(Exception e, String recipient, Object payload) { | ||
| log.warn("[Chat] WebSocket 알림 최종 실패. recipient={}", recipient, e); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# `@EnableRetry` 설정 여부 확인
rg -nP '@EnableRetry\b' --type=javaRepository: DorumDorum/BE
Length of output: 136
🏁 Script executed:
# DorumdorumApplication.java에서 `@EnableRetry` 설정 확인
cat -n src/main/java/com/project/dorumdorum/DorumdorumApplication.java | head -30Repository: DorumDorum/BE
Length of output: 983
🏁 Script executed:
# ChatWebSocketSendService.java 전체 내용 확인
cat -n src/main/java/com/project/dorumdorum/domain/chat/infra/websocket/ChatWebSocketSendService.javaRepository: DorumDorum/BE
Length of output: 1613
🏁 Script executed:
# Spring Retry가 지원하는 예외 타입과 messaging 관련 예외 검색
rg -i 'messageconversion|messagedelivery|exception' --type=java src/main/java/com/project/dorumdorum/domain/chat/ -A 2Repository: DorumDorum/BE
Length of output: 13024
재시도 정책 범위와 @Recover 메서드 매칭 방식을 개선해 보세요.
다행히 @EnableRetry는 DorumdorumApplication에 이미 설정되어 있어서 @Retryable/@Recover가 정상 동작할 거예요. 그 대신 다른 두 가지 개선 사항을 제안드립니다:
-
재시도 정책 범위 최적화:
retryFor = Exception.class로 모든 예외를 재시도하고 있는데, 직렬화 오류(MessageConversionException)나IllegalArgumentException같은 비즈니스 예외는 재시도해도 성공할 수 없어요. 네트워크 계층 예외(MessageDeliveryException등)만 선별하거나noRetryFor에 비재시도 예외를 명시하면 불필요한 재시도 대기(최대 3초)를 줄일 수 있습니다. -
@Recover메서드의 명확성: 현재recoverSend(Exception, String, Object)시그니처가 두 메서드와 잘 매칭되지만, 향후 다른 시그니처의@Retryable메서드가 추가될 때@Recover매칭이 모호해질 수 있어요. 각@Retryable메서드마다 명시적인@Recover메서드를 두거나 메서드명으로 직접 지정하면(recover속성) 더 안전할 거 같아요.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/main/java/com/project/dorumdorum/domain/chat/infra/websocket/ChatWebSocketSendService.java`
around lines 18 - 32, The retry scope is too broad and the single generic
`@Recover` may become ambiguous; for methods broadcast and notifyUser narrow
retries to messaging delivery/network exceptions (e.g., retryFor =
MessageDeliveryException.class or MessagingException.class) and exclude
non-retriable errors with noRetryFor = {MessageConversionException.class,
IllegalArgumentException.class}, and replace the single recoverSend with two
explicit recover methods (e.g., recoverBroadcast(Exception e, String chatRoomNo,
Object payload) and recoverNotify(Exception e, String userNo, Object payload))
so each `@Retryable` (broadcast, notifyUser) has a matching `@Recover` signature.
Comment on lines
25
to
+29
| registry.addEndpoint("/ws") | ||
| .setHandshakeHandler(jwtPrincipalHandshakeHandler) | ||
| .addInterceptors(jwtHandshakeInterceptor) | ||
| .setAllowedOriginPatterns("*") | ||
| .withSockJS(); // HTTP long-polling으로 폴백 시 HTTP 요청으로 처리됌 -> 연결 수 제한 로직 별도 고려 필요 | ||
| .withSockJS(); // HTTP long-polling으로 폴백 시 HTTP 요청으로 처리됨 -> 연결 수 제한 로직 별도 고려 필요 |
There was a problem hiding this comment.
쿠키 기반 WebSocket 인증에서는 허용 origin을 제한해 주세요.
JwtHandshakeInterceptor가 accessToken 쿠키로 인증하고, Line 26에서 해당 사용자를 STOMP Principal로 연결하고 있습니다. 이 상태에서 setAllowedOriginPatterns("*")를 유지하면 배포 환경의 SameSite/CORS 설정에 따라 악성 origin이 사용자의 쿠키로 /user/queue 개인 알림 구독을 시도할 수 있습니다. 프론트엔드 도메인을 설정값으로 주입해 명시적으로 제한해 주세요.
🛡️ 개선 예시
- .setAllowedOriginPatterns("*")
+ .setAllowedOriginPatterns(allowedWebSocketOrigins)`@Value`("${app.websocket.allowed-origins}")
private String[] allowedWebSocketOrigins;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/java/com/project/dorumdorum/global/config/WebSocketConfig.java`
around lines 25 - 29, The WebSocket config currently allows all origins via
setAllowedOriginPatterns("*") which is unsafe for cookie-based auth: inject a
config property (e.g. String[] allowedWebSocketOrigins or property key
app.websocket.allowed-origins) into WebSocketConfig and replace
setAllowedOriginPatterns("*") with
setAllowedOriginPatterns(allowedWebSocketOrigins) (or call setAllowedOrigins if
you prefer exact matches), keeping the rest of
registry.addEndpoint("/ws").setHandshakeHandler(jwtPrincipalHandshakeHandler).addInterceptors(jwtHandshakeInterceptor).withSockJS()
unchanged so JwtHandshakeInterceptor continues to authenticate from the
accessToken cookie but only for configured frontend domains.
Comment on lines
+41
to
+42
| CREATE INDEX IF NOT EXISTS idx_chat_room_room_no | ||
| ON chat_room (room_no); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
기존 인덱스와의 중복 여부 확인 부탁드립니다.
chat_room 테이블에는 이미 uk_chat_room_direct (room_no, applicant_user_no)와 부분 유니크 인덱스 uk_chat_room_group (room_no) WHERE chat_room_type = 'GROUP'이 존재합니다. room_no 단일 컬럼 조회 시 uk_chat_room_direct의 선두 컬럼으로도 커버가 가능하기 때문에, 새로 추가하신 idx_chat_room_room_no는 쿼리 플래너 선택 보장 목적 외에는 중복 여지가 있습니다.
findAllByRoomNo(...)가 DIRECT와 GROUP을 모두 조회하기 위해 전체 커버리지가 필요하다면 현재 인덱스도 타당합니다만, 실제로 EXPLAIN ANALYZE로 플랜을 한 번 확인해보시고 불필요하면 제거를 검토해주세요. 인덱스가 많을수록 쓰기 비용이 증가하기 때문에요.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/resources/schema.sql` around lines 41 - 42, You added a new index
idx_chat_room_room_no on chat_room(room_no) which may be redundant with existing
uk_chat_room_direct(room_no, applicant_user_no) and the partial unique index
uk_chat_room_group(room_no) WHERE chat_room_type = 'GROUP'; run EXPLAIN ANALYZE
for the queries used by findAllByRoomNo(...) to confirm whether the
single-column index actually changes the planner choice or performance, and if
it doesn't, remove idx_chat_room_room_no to avoid extra write overhead —
otherwise document why the standalone index is required (e.g., to cover queries
across both DIRECT and GROUP when the partial index cannot) and keep it.
Comment on lines
111
to
127
| void handle_WhenNicknameNull_UsesName() { | ||
| ChatRoomMember member = ChatRoomMember.builder().chatRoom(chatRoom).userNo("user-kicked").build(); | ||
| ChatRoomMember member = givenMember(chatRoom, "user-kicked"); | ||
| User mockUser = mock(User.class); | ||
| ChatMessage mockMessage = mock(ChatMessage.class); | ||
| when(chatRoomService.findByRoomNo("room-1")).thenReturn(Optional.of(chatRoom)); | ||
| when(chatRoomMemberService.isMember(chatRoom, "user-kicked")).thenReturn(true); | ||
| when(chatRoomMemberService.findByChatRoomAndUserNo(chatRoom, "user-kicked")).thenReturn(member); | ||
| when(userService.findById("user-kicked")).thenReturn(mockUser); | ||
| when(mockUser.getNickname()).thenReturn(null); | ||
| when(mockUser.getName()).thenReturn("홍길동"); | ||
| when(chatMessageService.save(any(), eq("SYSTEM"), contains("홍길동"), eq(MessageType.SYSTEM), eq(0))).thenReturn(mockMessage); | ||
| when(chatMessageService.save(any(), eq("SYSTEM"), contains("홍길동님이"), eq(MessageType.SYSTEM), eq(0))) | ||
| .thenReturn(mockMessage); | ||
| when(mockMessage.getMessageNo()).thenReturn("msg-1"); | ||
| when(mockMessage.getCreatedAt()).thenReturn(LocalDateTime.now()); | ||
|
|
||
| listener.handle(new RoommateKickedEvent("room-1", "user-kicked")); | ||
|
|
||
| verify(chatMessageService).save(eq(chatRoom), eq("SYSTEM"), contains("홍길동"), eq(MessageType.SYSTEM), eq(0)); | ||
| verify(chatMessageService).save(eq(chatRoom), eq("SYSTEM"), contains("홍길동님이"), eq(MessageType.SYSTEM), eq(0)); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
사용하지 않는 member 지역 변수가 있습니다.
Line 112에서 givenMember(...)의 반환값을 member에 담고 있지만, 이 테스트에서는 leave(member) 검증이 없어 변수가 사용되지 않습니다. givenMember(...)만 호출하거나 @SuppressWarnings("unused") 없이 깔끔하게 정리하시면 좋겠습니다.
♻️ 제안 수정안
- ChatRoomMember member = givenMember(chatRoom, "user-kicked");
+ givenMember(chatRoom, "user-kicked");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/test/java/com/project/dorumdorum/domain/chat/unit/usecase/RoommateKickedEventListenerTest.java`
around lines 111 - 127, In the test method handle_WhenNicknameNull_UsesName
remove the unused local variable by either calling givenMember(chatRoom,
"user-kicked") without assigning it or reusing the returned ChatRoomMember
(member) in an assertion (e.g., verify a leave(member) call) — locate the
givenMember(...) call and the local variable member and either drop the
assignment or add the proper verification that uses member so the local variable
is not unused.
Comment on lines
146
to
170
| @Test | ||
| @DisplayName("WebSocket 브로드캐스트 실패 시 예외를 무시하고 정상 종료") | ||
| void handle_WhenWebSocketFails_LogsAndContinues() { | ||
| ChatRoomMember member = ChatRoomMember.builder().chatRoom(chatRoom).userNo("user-kicked").build(); | ||
| @DisplayName("강퇴 처리 후 이벤트에 브로드캐스트 + 강퇴 사용자 개인 알림 태스크가 담긴다") | ||
| void handle_WhenMemberKicked_PublishesEventWithBroadcastAndUserNotification() { | ||
| givenMember(chatRoom, "user-kicked"); | ||
| User mockUser = mock(User.class); | ||
| ChatMessage mockMessage = mock(ChatMessage.class); | ||
| when(chatRoomService.findByRoomNo("room-1")).thenReturn(Optional.of(chatRoom)); | ||
| when(chatRoomMemberService.isMember(chatRoom, "user-kicked")).thenReturn(true); | ||
| when(chatRoomMemberService.findByChatRoomAndUserNo(chatRoom, "user-kicked")).thenReturn(member); | ||
| when(userService.findById("user-kicked")).thenReturn(mockUser); | ||
| when(mockUser.getNickname()).thenReturn("nick"); | ||
| when(chatMessageService.save(any(), any(), any(), any(), anyInt())).thenReturn(mockMessage); | ||
| when(mockMessage.getMessageNo()).thenReturn("msg-1"); | ||
| when(mockMessage.getCreatedAt()).thenReturn(LocalDateTime.now()); | ||
| doThrow(new RuntimeException("WebSocket error")).when(messagingTemplate).convertAndSend(anyString(), any(Object.class)); | ||
| when(mockUser.getNickname()).thenReturn("kickedNick"); | ||
| givenSystemMessage(); | ||
|
|
||
| listener.handle(new RoommateKickedEvent("room-1", "user-kicked")); | ||
|
|
||
| verify(chatRoomMemberService).leave(member); | ||
| verify(chatMessageService).save(any(), any(), any(), any(), anyInt()); | ||
| ArgumentCaptor<ChatWebSocketNotificationEvent> captor = | ||
| ArgumentCaptor.forClass(ChatWebSocketNotificationEvent.class); | ||
| verify(eventPublisher).publishEvent(captor.capture()); | ||
| ChatWebSocketNotificationEvent event = captor.getValue(); | ||
|
|
||
| assertThat(event.broadcasts()).hasSize(1); | ||
| assertThat(event.userNotifications()).hasSize(1); | ||
| assertThat(event.userNotifications().get(0).userNo()).isEqualTo("user-kicked"); | ||
|
|
||
| NotificationMessage notification = (NotificationMessage) event.userNotifications().get(0).payload(); | ||
| assertThat(notification.type()).isEqualTo(NotificationType.KICKED_FROM_ROOM); | ||
| assertThat(notification.roomNo()).isEqualTo("room-1"); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
브로드캐스트 페이로드 검증 보강 제안드립니다.
개인 알림(userNotifications)에 대해서는 NotificationType/roomNo까지 꼼꼼히 검증하시는 반면, broadcasts는 크기만 확인(Line 163)하고 있습니다. 프로덕션 코드에서 브로드캐스트 payload에 ChatMessageResponse와 chatRoomNo를 정확히 넣어주는 부분이 회귀 없이 유지되도록, BroadcastTask의 chatRoomNo와 payload 내용(MessageType.SYSTEM 등)도 함께 assert 해두시면 안전망이 더 촘촘해집니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/test/java/com/project/dorumdorum/domain/chat/unit/usecase/RoommateKickedEventListenerTest.java`
around lines 146 - 170, The test currently only asserts broadcasts size; enhance
it to assert the BroadcastTask's chatRoomNo equals "room-1" and that its payload
is a ChatMessageResponse (or the expected message wrapper) with
MessageType.SYSTEM and the expected system message content; locate the published
event captured from listener.handle(new
RoommateKickedEvent("room-1","user-kicked")), get event.broadcasts().get(0) (the
BroadcastTask) and add assertions on its chatRoomNo and payload type/fields to
ensure the broadcast payload is correct.
Comment on lines
+179
to
+186
| assertThat(persistedRoom.getDeletedAt()).isNotNull(); | ||
| assertThat(roommateRepository.findByUserNo(HOST_NO)).isEmpty(); | ||
| assertThat(roomRequestRepository.findAll()).isEmpty(); | ||
| assertThat(roomRuleRepository.findByRoomNo(room.getRoomNo())).isEmpty(); | ||
| assertThat(chatRoomRepository.findAllByRoomNo(room.getRoomNo())).isEmpty(); | ||
| assertThat(chatRoomMemberRepository.findAll()).isEmpty(); | ||
| assertThat(chatMessageRepository.findAll()).isEmpty(); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
검증 범위를 해당 room으로 좁혀 주시면 더 안전합니다.
roomRequestRepository.findAll(), chatRoomMemberRepository.findAll(), chatMessageRepository.findAll()처럼 전역 조회로 emptiness를 단정하면, 같은 SpringBootTest 컨텍스트를 공유하는 다른 테스트에서 데이터가 일부 누수되거나 future에 테스트가 추가될 때 예기치 않게 실패할 수 있습니다. 이미 roomRuleRepository.findByRoomNo(...), chatRoomRepository.findAllByRoomNo(...)처럼 scope를 좁혀 검증하는 패턴을 쓰고 있으니, 나머지도 같은 방식으로 통일해 주시면 테스트 격리 측면에서 더 견고합니다.
♻️ 제안 수정 예시
- assertThat(roomRequestRepository.findAll()).isEmpty();
+ assertThat(roomRequestRepository.findAllByRoom(persistedRoom)).isEmpty();
assertThat(roomRuleRepository.findByRoomNo(room.getRoomNo())).isEmpty();
assertThat(chatRoomRepository.findAllByRoomNo(room.getRoomNo())).isEmpty();
- assertThat(chatRoomMemberRepository.findAll()).isEmpty();
- assertThat(chatMessageRepository.findAll()).isEmpty();
+ // chatRoom이 이미 제거된 뒤에는 room 기반으로 member/message를 조회할 수 없으므로,
+ // 삭제 대상 chatRoomNo 목록을 setUp에서 기억해 두고 그 chatRoomNo들로만 비어 있는지 확인🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/test/java/com/project/dorumdorum/domain/room/integration/DeleteRoomPersistenceIntegrationTest.java`
around lines 179 - 186, 현재 테스트가 roomRequestRepository.findAll(),
chatRoomMemberRepository.findAll(), chatMessageRepository.findAll()로 전역 조회해
비어있음을 단정하고 있으니, DeleteRoomPersistenceIntegrationTest의 검증 범위를 삭제 대상 room에 한정하도록
바꿔주세요: roomRequestRepository.findByRoomNo(room.getRoomNo()) 또는
findAllByRoomNo(room.getRoomNo()) 같은 room-스코프 조회 메서드를 사용해 해당 room의 요청이 비어있는지
확인하고, chatRoomMemberRepository 및 chatMessageRepository도 각각
findByRoomNo/findAllByRoomNo( room.getRoomNo() ) 형태로 변경해 해당 room의 멤버와 메시지만 검사하도록
수정하세요.
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/test/java/com/project/dorumdorum/domain/chat/unit/usecase/ChatWebSocketNotificationEventListenerTest.java`:
- Around line 25-40: Add two new unit tests in
ChatWebSocketNotificationEventListenerTest to cover the missing scenarios: one
that constructs a ChatWebSocketNotificationEvent with empty userNotifications
and broadcasts and asserts via inOrder (or Mockito.verifyZeroInteractions) that
chatWebSocketSendService.notifyUser and .broadcast are not called; and another
that creates events with multiple ChatWebSocketNotificationEvent.UserNotifyTask
and multiple BroadcastTask entries, calls listener.handle(event), and then uses
InOrder on chatWebSocketSendService to verify that notifyUser is invoked in the
original list order for each user task and broadcast is invoked in original list
order for each broadcast task (use the same notifyUser/broadcast method names
from the test to identify calls).
In
`@src/test/java/com/project/dorumdorum/domain/chat/unit/usecase/RoomDeletedEventListenerTest.java`:
- Around line 29-158: Test handle_NoMembers_DoesNotPublishEvent currently only
asserts that no event is published; update that test to also verify the deletion
contract still runs by adding verifications that
chatMessageService.deleteAllByChatRoom(...) is called with the
chatRoom.chatRoomNo (e.g., "cr-1"),
chatRoomMemberService.deleteAllByChatRoom(room) is called, and
chatRoomService.deleteByChatRoomNo(...) is called, while keeping the
verify(eventPublisher, never()).publishEvent(any()) assertion; locate and modify
the method handle_NoMembers_DoesNotPublishEvent and use the existing ChatRoom
instance and mocked services (chatMessageService, chatRoomMemberService,
chatRoomService, eventPublisher) to add those verify(...) calls.
In
`@src/test/java/com/project/dorumdorum/domain/room/integration/DeleteRoomPersistenceIntegrationTest.java`:
- Around line 192-194: The Awaitility assertion relying on the async
notification (ChatWebSocketNotificationEventListener executed via
notificationExecutor) should specify an explicit timeout to avoid CI flakiness:
update the await().untilAsserted(...) call that verifies
messagingTemplate.convertAndSendToUser(..., times(3)) to include atMost with a
reasonable Duration (e.g., atMost(Duration.ofSeconds(10))) so the intent and
failure window are explicit.
In
`@src/test/java/com/project/dorumdorum/domain/room/integration/KickRoommatePersistenceIntegrationTest.java`:
- Around line 169-173: The test relies on Awaitility's default timeout and
unspecified invocation counts; update the await() usage in
KickRoommatePersistenceIntegrationTest to supply an explicit timeout (e.g.,
await().atMost(...).untilAsserted(...)) and make the Mockito verifications
explicit by checking exact invocation counts for
messagingTemplate.convertAndSend(...) and
messagingTemplate.convertAndSendToUser(...), e.g., verify(messagingTemplate,
times(1)) for each call (add import for org.mockito.Mockito.times); target the
existing symbols await().untilAsserted, messagingTemplate, convertAndSend,
convertAndSendToUser, and MEMBER_NO when applying the change.
- Around line 144-145: Move the updateLastReadAt call so the ChatRoomMember is
initialized with its lastReadAt before the first persist: call
memberChat.updateLastReadAt(...) prior to
chatRoomMemberRepository.save(memberChat) in
KickRoommatePersistenceIntegrationTest so the entity is saved once with the
intended state (remove the subsequent second save to avoid double
commits/merges).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 80eb21df-1c30-40c6-971b-d07f70b269ed
📒 Files selected for processing (6)
src/main/java/com/project/dorumdorum/domain/chat/application/event/ChatWebSocketNotificationEventListener.javasrc/main/java/com/project/dorumdorum/domain/chat/application/event/RoomDeletedEventListener.javasrc/test/java/com/project/dorumdorum/domain/chat/unit/usecase/ChatWebSocketNotificationEventListenerTest.javasrc/test/java/com/project/dorumdorum/domain/chat/unit/usecase/RoomDeletedEventListenerTest.javasrc/test/java/com/project/dorumdorum/domain/room/integration/DeleteRoomPersistenceIntegrationTest.javasrc/test/java/com/project/dorumdorum/domain/room/integration/KickRoommatePersistenceIntegrationTest.java
Comment on lines
+25
to
+40
| @Test | ||
| @DisplayName("개인 알림을 브로드캐스트보다 먼저 보낸다") | ||
| void handle_SendsUserNotificationBeforeBroadcast() { | ||
| Object userPayload = new Object(); | ||
| Object broadcastPayload = new Object(); | ||
| ChatWebSocketNotificationEvent event = new ChatWebSocketNotificationEvent( | ||
| List.of(new ChatWebSocketNotificationEvent.BroadcastTask("chat-room-1", broadcastPayload)), | ||
| List.of(new ChatWebSocketNotificationEvent.UserNotifyTask("user-1", userPayload)) | ||
| ); | ||
|
|
||
| listener.handle(event); | ||
|
|
||
| InOrder inOrder = inOrder(chatWebSocketSendService); | ||
| inOrder.verify(chatWebSocketSendService).notifyUser("user-1", userPayload); | ||
| inOrder.verify(chatWebSocketSendService).broadcast("chat-room-1", broadcastPayload); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
순서 검증 테스트가 깔끔하게 잘 작성되어 있습니다.
InOrder로 notifyUser → broadcast 호출 순서를 명확히 검증하고 있어 프로덕션 리스너의 계약을 잘 보호해 줍니다. 추가로 여유가 되실 때는 다음과 같은 시나리오도 함께 커버하시면 리팩터링 저항성이 더 올라갈 것 같습니다(필수는 아닙니다).
- 빈
userNotifications/broadcasts에서notifyUser/broadcast가 전혀 호출되지 않는지. - 각 리스트에 2개 이상의 태스크가 들어 있을 때 리스트 내부 순서가 보존되는지(
inOrder.verify(...)연속 호출).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/test/java/com/project/dorumdorum/domain/chat/unit/usecase/ChatWebSocketNotificationEventListenerTest.java`
around lines 25 - 40, Add two new unit tests in
ChatWebSocketNotificationEventListenerTest to cover the missing scenarios: one
that constructs a ChatWebSocketNotificationEvent with empty userNotifications
and broadcasts and asserts via inOrder (or Mockito.verifyZeroInteractions) that
chatWebSocketSendService.notifyUser and .broadcast are not called; and another
that creates events with multiple ChatWebSocketNotificationEvent.UserNotifyTask
and multiple BroadcastTask entries, calls listener.handle(event), and then uses
InOrder on chatWebSocketSendService to verify that notifyUser is invoked in the
original list order for each user task and broadcast is invoked in original list
order for each broadcast task (use the same notifyUser/broadcast method names
from the test to identify calls).
Comment on lines
+29
to
+158
| @ExtendWith(MockitoExtension.class) | ||
| @DisplayName("RoomDeletedEventListener Unit Tests") | ||
| class RoomDeletedEventListenerTest { | ||
|
|
||
| @Mock private ChatRoomService chatRoomService; | ||
| @Mock private ChatRoomMemberService chatRoomMemberService; | ||
| @Mock private ChatMessageService chatMessageService; | ||
| @Mock private ApplicationEventPublisher eventPublisher; | ||
| @InjectMocks private RoomDeletedEventListener listener; | ||
|
|
||
| private ChatRoomMember memberOf(ChatRoom chatRoom, String userNo) { | ||
| return ChatRoomMember.builder().chatRoom(chatRoom).userNo(userNo).build(); | ||
| } | ||
|
|
||
| @Test | ||
| @DisplayName("채팅방 멤버 전원에게 /queue/notification 개인 알림이 발행된다") | ||
| void handle_NotifiesAllMembersViaUserNotification() { | ||
| ChatRoom room = ChatRoom.builder().chatRoomNo("cr-1").roomNo("r1") | ||
| .chatRoomType(ChatRoomType.GROUP).build(); | ||
| when(chatRoomService.findAllByRoomNo("r1")).thenReturn(List.of(room)); | ||
| when(chatRoomMemberService.findByChatRoom(room)) | ||
| .thenReturn(List.of(memberOf(room, "user-host"))); | ||
|
|
||
| listener.handle(new RoomDeletedEvent("r1")); | ||
|
|
||
| ArgumentCaptor<ChatWebSocketNotificationEvent> captor = | ||
| ArgumentCaptor.forClass(ChatWebSocketNotificationEvent.class); | ||
| verify(eventPublisher).publishEvent(captor.capture()); | ||
| ChatWebSocketNotificationEvent event = captor.getValue(); | ||
|
|
||
| assertThat(event.broadcasts()).isEmpty(); | ||
| assertThat(event.userNotifications()).hasSize(1); | ||
| assertThat(event.userNotifications().get(0).userNo()).isEqualTo("user-host"); | ||
|
|
||
| NotificationMessage notification = (NotificationMessage) event.userNotifications().get(0).payload(); | ||
| assertThat(notification.type()).isEqualTo(NotificationType.ROOM_DELETED); | ||
| assertThat(notification.roomNo()).isEqualTo("r1"); | ||
| assertThat(notification.chatRoomNo()).isEqualTo("cr-1"); | ||
| } | ||
|
|
||
| @Test | ||
| @DisplayName("DIRECT 채팅방 — 멤버 전원(host + applicant)에게 개인 알림") | ||
| void handle_DirectChatRoom_NotifiesBothMembers() { | ||
| ChatRoom directRoom = ChatRoom.builder().chatRoomNo("cr-2").roomNo("r1") | ||
| .chatRoomType(ChatRoomType.DIRECT).applicantUserNo("applicant-1").build(); | ||
| when(chatRoomService.findAllByRoomNo("r1")).thenReturn(List.of(directRoom)); | ||
| when(chatRoomMemberService.findByChatRoom(directRoom)) | ||
| .thenReturn(List.of(memberOf(directRoom, "user-host"), memberOf(directRoom, "applicant-1"))); | ||
|
|
||
| listener.handle(new RoomDeletedEvent("r1")); | ||
|
|
||
| ArgumentCaptor<ChatWebSocketNotificationEvent> captor = | ||
| ArgumentCaptor.forClass(ChatWebSocketNotificationEvent.class); | ||
| verify(eventPublisher).publishEvent(captor.capture()); | ||
| ChatWebSocketNotificationEvent event = captor.getValue(); | ||
|
|
||
| assertThat(event.broadcasts()).isEmpty(); | ||
| assertThat(event.userNotifications()).hasSize(2); | ||
| assertThat(event.userNotifications()) | ||
| .extracting(ChatWebSocketNotificationEvent.UserNotifyTask::userNo) | ||
| .containsExactlyInAnyOrder("user-host", "applicant-1"); | ||
| } | ||
|
|
||
| @Test | ||
| @DisplayName("멤버가 없는 채팅방이면 이벤트를 발행하지 않는다") | ||
| void handle_NoMembers_DoesNotPublishEvent() { | ||
| ChatRoom room = ChatRoom.builder().chatRoomNo("cr-1").roomNo("r1") | ||
| .chatRoomType(ChatRoomType.GROUP).build(); | ||
| when(chatRoomService.findAllByRoomNo("r1")).thenReturn(List.of(room)); | ||
| when(chatRoomMemberService.findByChatRoom(room)).thenReturn(List.of()); | ||
|
|
||
| listener.handle(new RoomDeletedEvent("r1")); | ||
|
|
||
| verify(eventPublisher, never()).publishEvent(any()); | ||
| } | ||
|
|
||
| @Test | ||
| @DisplayName("채팅방이 없으면 DB 삭제 및 이벤트 발행 없이 종료") | ||
| void handle_NoChatRooms_DoesNothing() { | ||
| when(chatRoomService.findAllByRoomNo("r1")).thenReturn(List.of()); | ||
|
|
||
| listener.handle(new RoomDeletedEvent("r1")); | ||
|
|
||
| verify(chatMessageService, never()).deleteAllByChatRoom(any()); | ||
| verify(chatRoomMemberService, never()).deleteAllByChatRoom(any()); | ||
| verify(eventPublisher, never()).publishEvent(any()); | ||
| } | ||
|
|
||
| @Test | ||
| @DisplayName("메시지·멤버·채팅방 순서로 삭제된다") | ||
| void handle_DeletesInOrder() { | ||
| ChatRoom room = ChatRoom.builder().chatRoomNo("cr-1").roomNo("r1") | ||
| .chatRoomType(ChatRoomType.GROUP).build(); | ||
| when(chatRoomService.findAllByRoomNo("r1")).thenReturn(List.of(room)); | ||
| when(chatRoomMemberService.findByChatRoom(room)) | ||
| .thenReturn(List.of(memberOf(room, "user-host"))); | ||
|
|
||
| listener.handle(new RoomDeletedEvent("r1")); | ||
|
|
||
| InOrder inOrder = inOrder(chatMessageService, chatRoomMemberService, chatRoomService); | ||
| inOrder.verify(chatMessageService).deleteAllByChatRoom("cr-1"); | ||
| inOrder.verify(chatRoomMemberService).deleteAllByChatRoom(room); | ||
| inOrder.verify(chatRoomService).deleteByChatRoomNo("cr-1"); | ||
| } | ||
|
|
||
| @Test | ||
| @DisplayName("복수 채팅방이 있으면 각 채팅방 멤버 모두 알림 수신") | ||
| void handle_MultipleRooms_AllMembersNotified() { | ||
| ChatRoom groupRoom = ChatRoom.builder().chatRoomNo("cr-1").roomNo("r1") | ||
| .chatRoomType(ChatRoomType.GROUP).build(); | ||
| ChatRoom directRoom = ChatRoom.builder().chatRoomNo("cr-2").roomNo("r1") | ||
| .chatRoomType(ChatRoomType.DIRECT).build(); | ||
| when(chatRoomService.findAllByRoomNo("r1")).thenReturn(List.of(groupRoom, directRoom)); | ||
| when(chatRoomMemberService.findByChatRoom(groupRoom)) | ||
| .thenReturn(List.of(memberOf(groupRoom, "user-host"))); | ||
| when(chatRoomMemberService.findByChatRoom(directRoom)) | ||
| .thenReturn(List.of(memberOf(directRoom, "user-host"), memberOf(directRoom, "applicant-1"))); | ||
|
|
||
| listener.handle(new RoomDeletedEvent("r1")); | ||
|
|
||
| verify(chatRoomService).deleteByChatRoomNo("cr-1"); | ||
| verify(chatRoomService).deleteByChatRoomNo("cr-2"); | ||
|
|
||
| ArgumentCaptor<ChatWebSocketNotificationEvent> captor = | ||
| ArgumentCaptor.forClass(ChatWebSocketNotificationEvent.class); | ||
| verify(eventPublisher).publishEvent(captor.capture()); | ||
| ChatWebSocketNotificationEvent event = captor.getValue(); | ||
| assertThat(event.broadcasts()).isEmpty(); | ||
| assertThat(event.userNotifications()).hasSize(3); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
단위 테스트 커버리지와 어서션이 아주 꼼꼼합니다.
GROUP/DIRECT 분기, 멤버 없음, 채팅방 없음, 삭제 순서, 다중 채팅방 집계까지 리스너의 주요 경로를 고르게 방어해 주고 있어 리팩터링 시 회귀를 잘 잡아줄 것 같습니다. 페이로드 타입과 NotificationType.ROOM_DELETED / roomNo / chatRoomNo 필드까지 확인해 준 점도 좋습니다.
사소한(선택) 제안만 하나 드리자면, handle_NoMembers_DoesNotPublishEvent에서는 "이벤트가 발행되지 않는다"만 검증하고 있는데, 실제로는 이 분기에서도 chatMessageService.deleteAllByChatRoom(...) 등 삭제 호출은 여전히 일어나야 하는 계약이므로, 다음과 같이 한 줄을 추가해 두면 해당 계약이 명시적으로 보호됩니다.
♻️ 제안(선택)
listener.handle(new RoomDeletedEvent("r1"));
+ verify(chatMessageService).deleteAllByChatRoom("cr-1");
+ verify(chatRoomMemberService).deleteAllByChatRoom(room);
+ verify(chatRoomService).deleteByChatRoomNo("cr-1");
verify(eventPublisher, never()).publishEvent(any());📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| @ExtendWith(MockitoExtension.class) | |
| @DisplayName("RoomDeletedEventListener Unit Tests") | |
| class RoomDeletedEventListenerTest { | |
| @Mock private ChatRoomService chatRoomService; | |
| @Mock private ChatRoomMemberService chatRoomMemberService; | |
| @Mock private ChatMessageService chatMessageService; | |
| @Mock private ApplicationEventPublisher eventPublisher; | |
| @InjectMocks private RoomDeletedEventListener listener; | |
| private ChatRoomMember memberOf(ChatRoom chatRoom, String userNo) { | |
| return ChatRoomMember.builder().chatRoom(chatRoom).userNo(userNo).build(); | |
| } | |
| @Test | |
| @DisplayName("채팅방 멤버 전원에게 /queue/notification 개인 알림이 발행된다") | |
| void handle_NotifiesAllMembersViaUserNotification() { | |
| ChatRoom room = ChatRoom.builder().chatRoomNo("cr-1").roomNo("r1") | |
| .chatRoomType(ChatRoomType.GROUP).build(); | |
| when(chatRoomService.findAllByRoomNo("r1")).thenReturn(List.of(room)); | |
| when(chatRoomMemberService.findByChatRoom(room)) | |
| .thenReturn(List.of(memberOf(room, "user-host"))); | |
| listener.handle(new RoomDeletedEvent("r1")); | |
| ArgumentCaptor<ChatWebSocketNotificationEvent> captor = | |
| ArgumentCaptor.forClass(ChatWebSocketNotificationEvent.class); | |
| verify(eventPublisher).publishEvent(captor.capture()); | |
| ChatWebSocketNotificationEvent event = captor.getValue(); | |
| assertThat(event.broadcasts()).isEmpty(); | |
| assertThat(event.userNotifications()).hasSize(1); | |
| assertThat(event.userNotifications().get(0).userNo()).isEqualTo("user-host"); | |
| NotificationMessage notification = (NotificationMessage) event.userNotifications().get(0).payload(); | |
| assertThat(notification.type()).isEqualTo(NotificationType.ROOM_DELETED); | |
| assertThat(notification.roomNo()).isEqualTo("r1"); | |
| assertThat(notification.chatRoomNo()).isEqualTo("cr-1"); | |
| } | |
| @Test | |
| @DisplayName("DIRECT 채팅방 — 멤버 전원(host + applicant)에게 개인 알림") | |
| void handle_DirectChatRoom_NotifiesBothMembers() { | |
| ChatRoom directRoom = ChatRoom.builder().chatRoomNo("cr-2").roomNo("r1") | |
| .chatRoomType(ChatRoomType.DIRECT).applicantUserNo("applicant-1").build(); | |
| when(chatRoomService.findAllByRoomNo("r1")).thenReturn(List.of(directRoom)); | |
| when(chatRoomMemberService.findByChatRoom(directRoom)) | |
| .thenReturn(List.of(memberOf(directRoom, "user-host"), memberOf(directRoom, "applicant-1"))); | |
| listener.handle(new RoomDeletedEvent("r1")); | |
| ArgumentCaptor<ChatWebSocketNotificationEvent> captor = | |
| ArgumentCaptor.forClass(ChatWebSocketNotificationEvent.class); | |
| verify(eventPublisher).publishEvent(captor.capture()); | |
| ChatWebSocketNotificationEvent event = captor.getValue(); | |
| assertThat(event.broadcasts()).isEmpty(); | |
| assertThat(event.userNotifications()).hasSize(2); | |
| assertThat(event.userNotifications()) | |
| .extracting(ChatWebSocketNotificationEvent.UserNotifyTask::userNo) | |
| .containsExactlyInAnyOrder("user-host", "applicant-1"); | |
| } | |
| @Test | |
| @DisplayName("멤버가 없는 채팅방이면 이벤트를 발행하지 않는다") | |
| void handle_NoMembers_DoesNotPublishEvent() { | |
| ChatRoom room = ChatRoom.builder().chatRoomNo("cr-1").roomNo("r1") | |
| .chatRoomType(ChatRoomType.GROUP).build(); | |
| when(chatRoomService.findAllByRoomNo("r1")).thenReturn(List.of(room)); | |
| when(chatRoomMemberService.findByChatRoom(room)).thenReturn(List.of()); | |
| listener.handle(new RoomDeletedEvent("r1")); | |
| verify(eventPublisher, never()).publishEvent(any()); | |
| } | |
| @Test | |
| @DisplayName("채팅방이 없으면 DB 삭제 및 이벤트 발행 없이 종료") | |
| void handle_NoChatRooms_DoesNothing() { | |
| when(chatRoomService.findAllByRoomNo("r1")).thenReturn(List.of()); | |
| listener.handle(new RoomDeletedEvent("r1")); | |
| verify(chatMessageService, never()).deleteAllByChatRoom(any()); | |
| verify(chatRoomMemberService, never()).deleteAllByChatRoom(any()); | |
| verify(eventPublisher, never()).publishEvent(any()); | |
| } | |
| @Test | |
| @DisplayName("메시지·멤버·채팅방 순서로 삭제된다") | |
| void handle_DeletesInOrder() { | |
| ChatRoom room = ChatRoom.builder().chatRoomNo("cr-1").roomNo("r1") | |
| .chatRoomType(ChatRoomType.GROUP).build(); | |
| when(chatRoomService.findAllByRoomNo("r1")).thenReturn(List.of(room)); | |
| when(chatRoomMemberService.findByChatRoom(room)) | |
| .thenReturn(List.of(memberOf(room, "user-host"))); | |
| listener.handle(new RoomDeletedEvent("r1")); | |
| InOrder inOrder = inOrder(chatMessageService, chatRoomMemberService, chatRoomService); | |
| inOrder.verify(chatMessageService).deleteAllByChatRoom("cr-1"); | |
| inOrder.verify(chatRoomMemberService).deleteAllByChatRoom(room); | |
| inOrder.verify(chatRoomService).deleteByChatRoomNo("cr-1"); | |
| } | |
| @Test | |
| @DisplayName("복수 채팅방이 있으면 각 채팅방 멤버 모두 알림 수신") | |
| void handle_MultipleRooms_AllMembersNotified() { | |
| ChatRoom groupRoom = ChatRoom.builder().chatRoomNo("cr-1").roomNo("r1") | |
| .chatRoomType(ChatRoomType.GROUP).build(); | |
| ChatRoom directRoom = ChatRoom.builder().chatRoomNo("cr-2").roomNo("r1") | |
| .chatRoomType(ChatRoomType.DIRECT).build(); | |
| when(chatRoomService.findAllByRoomNo("r1")).thenReturn(List.of(groupRoom, directRoom)); | |
| when(chatRoomMemberService.findByChatRoom(groupRoom)) | |
| .thenReturn(List.of(memberOf(groupRoom, "user-host"))); | |
| when(chatRoomMemberService.findByChatRoom(directRoom)) | |
| .thenReturn(List.of(memberOf(directRoom, "user-host"), memberOf(directRoom, "applicant-1"))); | |
| listener.handle(new RoomDeletedEvent("r1")); | |
| verify(chatRoomService).deleteByChatRoomNo("cr-1"); | |
| verify(chatRoomService).deleteByChatRoomNo("cr-2"); | |
| ArgumentCaptor<ChatWebSocketNotificationEvent> captor = | |
| ArgumentCaptor.forClass(ChatWebSocketNotificationEvent.class); | |
| verify(eventPublisher).publishEvent(captor.capture()); | |
| ChatWebSocketNotificationEvent event = captor.getValue(); | |
| assertThat(event.broadcasts()).isEmpty(); | |
| assertThat(event.userNotifications()).hasSize(3); | |
| } | |
| `@Test` | |
| `@DisplayName`("멤버가 없는 채팅방이면 이벤트를 발행하지 않는다") | |
| void handle_NoMembers_DoesNotPublishEvent() { | |
| ChatRoom room = ChatRoom.builder().chatRoomNo("cr-1").roomNo("r1") | |
| .chatRoomType(ChatRoomType.GROUP).build(); | |
| when(chatRoomService.findAllByRoomNo("r1")).thenReturn(List.of(room)); | |
| when(chatRoomMemberService.findByChatRoom(room)).thenReturn(List.of()); | |
| listener.handle(new RoomDeletedEvent("r1")); | |
| verify(chatMessageService).deleteAllByChatRoom("cr-1"); | |
| verify(chatRoomMemberService).deleteAllByChatRoom(room); | |
| verify(chatRoomService).deleteByChatRoomNo("cr-1"); | |
| verify(eventPublisher, never()).publishEvent(any()); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/test/java/com/project/dorumdorum/domain/chat/unit/usecase/RoomDeletedEventListenerTest.java`
around lines 29 - 158, Test handle_NoMembers_DoesNotPublishEvent currently only
asserts that no event is published; update that test to also verify the deletion
contract still runs by adding verifications that
chatMessageService.deleteAllByChatRoom(...) is called with the
chatRoom.chatRoomNo (e.g., "cr-1"),
chatRoomMemberService.deleteAllByChatRoom(room) is called, and
chatRoomService.deleteByChatRoomNo(...) is called, while keeping the
verify(eventPublisher, never()).publishEvent(any()) assertion; locate and modify
the method handle_NoMembers_DoesNotPublishEvent and use the existing ChatRoom
instance and mocked services (chatMessageService, chatRoomMemberService,
chatRoomService, eventPublisher) to add those verify(...) calls.
Comment on lines
+192
to
+194
| await().untilAsserted(() -> | ||
| verify(messagingTemplate, times(3)) | ||
| .convertAndSendToUser(any(), eq("/queue/notification"), any())); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Awaitility 타임아웃을 명시해 두시면 CI flakiness 방어에 도움이 될 것 같습니다.
ChatWebSocketNotificationEventListener가 notificationExecutor로 @Async 실행되기 때문에 해당 알림 검증 구간은 환경에 따라 지연될 수 있습니다. 기본 10초에 의존하기보다는 명시적인 atMost(...)를 두시면 의도도 잘 드러나고 실패 시 진단이 수월합니다. 검증하는 3회는 DIRECT(HOST+APPLICANT) + GROUP(HOST) = 3이라는 설정과도 정확히 맞아 보여 숫자는 적절해 보입니다 👍.
♻️ 제안 수정안
- await().untilAsserted(() ->
- verify(messagingTemplate, times(3))
- .convertAndSendToUser(any(), eq("/queue/notification"), any()));
+ await().atMost(java.time.Duration.ofSeconds(5)).untilAsserted(() ->
+ verify(messagingTemplate, times(3))
+ .convertAndSendToUser(any(), eq("/queue/notification"), any()));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/test/java/com/project/dorumdorum/domain/room/integration/DeleteRoomPersistenceIntegrationTest.java`
around lines 192 - 194, The Awaitility assertion relying on the async
notification (ChatWebSocketNotificationEventListener executed via
notificationExecutor) should specify an explicit timeout to avoid CI flakiness:
update the await().untilAsserted(...) call that verifies
messagingTemplate.convertAndSendToUser(..., times(3)) to include atMost with a
reasonable Duration (e.g., atMost(Duration.ofSeconds(10))) so the intent and
failure window are explicit.
Comment on lines
+144
to
+145
| memberChat.updateLastReadAt(LocalDateTime.now().minusMinutes(1)); | ||
| chatRoomMemberRepository.save(memberChat); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
memberChat.updateLastReadAt(...) 타이밍은 초기 save 이전으로 옮기시면 의도가 더 명확해질 것 같습니다.
현재는 chatRoomMemberRepository.save(...)로 한 번 저장한 뒤 updateLastReadAt 호출 후 다시 save해서 merge 경로로 업데이트하는데, 테스트 메서드가 @Transactional이 아니라 두 번의 독립적인 커밋이 발생합니다. 의미상 "처음부터 lastReadAt을 가진 상태로 멤버를 저장"하는 것이 더 자연스럽고 불필요한 두 번째 save도 줄일 수 있습니다.
♻️ 제안 수정안
- chatRoomMemberRepository.save(ChatRoomMember.builder().chatRoom(chatRoom).userNo(HOST_NO).build());
- ChatRoomMember memberChat = chatRoomMemberRepository.save(
- ChatRoomMember.builder().chatRoom(chatRoom).userNo(MEMBER_NO).build()
- );
+ chatRoomMemberRepository.save(ChatRoomMember.builder().chatRoom(chatRoom).userNo(HOST_NO).build());
+ ChatRoomMember memberChat = ChatRoomMember.builder().chatRoom(chatRoom).userNo(MEMBER_NO).build();
+ memberChat.updateLastReadAt(LocalDateTime.now().minusMinutes(1));
+ chatRoomMemberRepository.save(memberChat);
@@
- memberChat.updateLastReadAt(LocalDateTime.now().minusMinutes(1));
- chatRoomMemberRepository.save(memberChat);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/test/java/com/project/dorumdorum/domain/room/integration/KickRoommatePersistenceIntegrationTest.java`
around lines 144 - 145, Move the updateLastReadAt call so the ChatRoomMember is
initialized with its lastReadAt before the first persist: call
memberChat.updateLastReadAt(...) prior to
chatRoomMemberRepository.save(memberChat) in
KickRoommatePersistenceIntegrationTest so the entity is saved once with the
intended state (remove the subsequent second save to avoid double
commits/merges).
Comment on lines
+169
to
+173
| await().untilAsserted(() -> { | ||
| verify(messagingTemplate).convertAndSend( | ||
| eq("/topic/chat-room/" + chatRoom.getChatRoomNo()), any(Object.class)); | ||
| verify(messagingTemplate).convertAndSendToUser(eq(MEMBER_NO), eq("/queue/notification"), any()); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Awaitility 타임아웃과 호출 횟수 검증을 조금 더 명시적으로 두시면 좋겠습니다.
await().untilAsserted(...)가 기본 타임아웃(10초)에 의존하고 있고, verify(messagingTemplate).convertAndSend(...) / convertAndSendToUser(...)는 호출 횟수를 지정하지 않아 기본값인 "적어도 1회"로 해석됩니다. 이벤트 흐름(RoommateKickedEvent → ChatWebSocketNotificationEvent → @Async("notificationExecutor"))이 CI에서 간헐적으로 느려질 가능성을 고려해 타임아웃과 회차를 명시해 두시면 flakiness/의도 전달 측면에서 더 견고해질 것 같습니다.
♻️ 제안 수정안
- await().untilAsserted(() -> {
- verify(messagingTemplate).convertAndSend(
- eq("/topic/chat-room/" + chatRoom.getChatRoomNo()), any(Object.class));
- verify(messagingTemplate).convertAndSendToUser(eq(MEMBER_NO), eq("/queue/notification"), any());
- });
+ await().atMost(java.time.Duration.ofSeconds(5)).untilAsserted(() -> {
+ verify(messagingTemplate, times(1)).convertAndSend(
+ eq("/topic/chat-room/" + chatRoom.getChatRoomNo()), any(Object.class));
+ verify(messagingTemplate, times(1))
+ .convertAndSendToUser(eq(MEMBER_NO), eq("/queue/notification"), any());
+ });(org.mockito.Mockito.times import 추가가 필요합니다.)
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| await().untilAsserted(() -> { | |
| verify(messagingTemplate).convertAndSend( | |
| eq("/topic/chat-room/" + chatRoom.getChatRoomNo()), any(Object.class)); | |
| verify(messagingTemplate).convertAndSendToUser(eq(MEMBER_NO), eq("/queue/notification"), any()); | |
| }); | |
| await().atMost(java.time.Duration.ofSeconds(5)).untilAsserted(() -> { | |
| verify(messagingTemplate, times(1)).convertAndSend( | |
| eq("/topic/chat-room/" + chatRoom.getChatRoomNo()), any(Object.class)); | |
| verify(messagingTemplate, times(1)) | |
| .convertAndSendToUser(eq(MEMBER_NO), eq("/queue/notification"), any()); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/test/java/com/project/dorumdorum/domain/room/integration/KickRoommatePersistenceIntegrationTest.java`
around lines 169 - 173, The test relies on Awaitility's default timeout and
unspecified invocation counts; update the await() usage in
KickRoommatePersistenceIntegrationTest to supply an explicit timeout (e.g.,
await().atMost(...).untilAsserted(...)) and make the Mockito verifications
explicit by checking exact invocation counts for
messagingTemplate.convertAndSend(...) and
messagingTemplate.convertAndSendToUser(...), e.g., verify(messagingTemplate,
times(1)) for each call (add import for org.mockito.Mockito.times); target the
existing symbols await().untilAsserted, messagingTemplate, convertAndSend,
convertAndSendToUser, and MEMBER_NO when applying the change.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/main/java/com/project/dorumdorum/domain/checklist/application/mapper/UserChecklistMapper.java (1)
20-24: 🧹 Nitpick | 🔵 Trivial
RoomRuleMapper와 동일하게, 스테일@Mapping어노테이션 정리를 제안드립니다.
updateUserChecklist가default메서드로 바뀌면서 바로 위의@Mapping(target = "userChecklistNo", ignore = true)등 4개 어노테이션은 MapStruct가 더 이상 참고하지 않습니다. 가독성과 오해 방지를 위해 제거하시는 편이 좋겠습니다.♻️ 제안 변경
- `@Mapping`(target = "userChecklistNo", ignore = true) - `@Mapping`(target = "userNo", ignore = true) - `@Mapping`(target = "createdAt", ignore = true) - `@Mapping`(target = "updatedAt", ignore = true) default void updateUserChecklist(UpdateUserChecklistRequest request, `@MappingTarget` UserChecklist checklist) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/project/dorumdorum/domain/checklist/application/mapper/UserChecklistMapper.java` around lines 20 - 24, The four stale `@Mapping` annotations above the default method updateUserChecklist (specifically the `@Mapping` entries for "userChecklistNo", "userNo", "createdAt", "updatedAt") are ignored by MapStruct because updateUserChecklist is now a default method; remove those four `@Mapping` annotations to avoid confusion and mirror the cleanup done for RoomRuleMapper, leaving the default method signature public/default void updateUserChecklist(UpdateUserChecklistRequest request, `@MappingTarget` UserChecklist checklist) intact.src/main/java/com/project/dorumdorum/domain/checklist/application/mapper/RoomRuleMapper.java (1)
21-25: 🧹 Nitpick | 🔵 Trivial잔여
@Mapping어노테이션 정리를 제안드립니다.
updateRoomRule이default메서드로 전환되면서 본문에서 직접roomRule.updateChecklist(...)를 호출하도록 바뀌었기 때문에, 바로 위에 남아있는@Mapping(target = "roomRuleNo", ignore = true)등 4개의 어노테이션은 더 이상 MapStruct가 처리하지 않아 사실상 죽은 메타데이터가 됩니다. 오해의 소지가 있으니 제거하시는 편이 명확할 것 같습니다.♻️ 제안 변경
- `@Mapping`(target = "roomRuleNo", ignore = true) - `@Mapping`(target = "room", ignore = true) - `@Mapping`(target = "createdAt", ignore = true) - `@Mapping`(target = "updatedAt", ignore = true) default void updateRoomRule(UpdateRoomRuleRequest request, `@MappingTarget` RoomRule roomRule) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/project/dorumdorum/domain/checklist/application/mapper/RoomRuleMapper.java` around lines 21 - 25, The four `@Mapping` annotations above the default method updateRoomRule in RoomRuleMapper (targets: roomRuleNo, room, createdAt, updatedAt) are now dead metadata because updateRoomRule is implemented as a default method that calls roomRule.updateChecklist(...) itself; remove those `@Mapping` annotations from the class so there is no misleading MapStruct metadata left, and verify no other map methods depend on those specific ignores.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/main/java/com/project/dorumdorum/domain/checklist/domain/entity/ChecklistBase.java`:
- Around line 126-178: The updateChecklist method accepts 25 raw parameters
which increases coupling and risks null assignment to non-nullable columns;
refactor by changing updateChecklist to accept a single value object (e.g.,
ChecklistUpdateCommand) instead of many params and, inside updateChecklist,
defensively validate required fields with Objects.requireNonNull (for fields
like bedtime, wakeUp, returnHome etc.) so the entity enforces non-null
invariants even if controller-level DTOs such as
UpdateRoomRuleRequest/UpdateUserChecklistRequest are bypassed.
---
Outside diff comments:
In
`@src/main/java/com/project/dorumdorum/domain/checklist/application/mapper/RoomRuleMapper.java`:
- Around line 21-25: The four `@Mapping` annotations above the default method
updateRoomRule in RoomRuleMapper (targets: roomRuleNo, room, createdAt,
updatedAt) are now dead metadata because updateRoomRule is implemented as a
default method that calls roomRule.updateChecklist(...) itself; remove those
`@Mapping` annotations from the class so there is no misleading MapStruct metadata
left, and verify no other map methods depend on those specific ignores.
In
`@src/main/java/com/project/dorumdorum/domain/checklist/application/mapper/UserChecklistMapper.java`:
- Around line 20-24: The four stale `@Mapping` annotations above the default
method updateUserChecklist (specifically the `@Mapping` entries for
"userChecklistNo", "userNo", "createdAt", "updatedAt") are ignored by MapStruct
because updateUserChecklist is now a default method; remove those four `@Mapping`
annotations to avoid confusion and mirror the cleanup done for RoomRuleMapper,
leaving the default method signature public/default void
updateUserChecklist(UpdateUserChecklistRequest request, `@MappingTarget`
UserChecklist checklist) intact.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: ASSERTIVE
Plan: Pro
Run ID: cf1b261a-4238-4bc7-be44-28275856af33
📒 Files selected for processing (6)
src/main/java/com/project/dorumdorum/domain/checklist/application/mapper/RoomRuleMapper.javasrc/main/java/com/project/dorumdorum/domain/checklist/application/mapper/UserChecklistMapper.javasrc/main/java/com/project/dorumdorum/domain/checklist/domain/entity/ChecklistBase.javasrc/main/java/com/project/dorumdorum/domain/room/infra/repository/RoomRepositoryImpl.javasrc/test/java/com/project/dorumdorum/domain/checklist/unit/mapper/RoomRuleMapperTest.javasrc/test/java/com/project/dorumdorum/domain/checklist/unit/mapper/UserChecklistMapperTest.java
Comment on lines
+126
to
+178
| public void updateChecklist( | ||
| String bedtime, | ||
| String wakeUp, | ||
| ReturnHomeType returnHome, | ||
| String returnHomeTime, | ||
| CleaningType cleaning, | ||
| PhoneCallType phoneCall, | ||
| SleepLightType sleepLight, | ||
| SleepHabitType sleepHabit, | ||
| SnoringType snoring, | ||
| ShowerTimeType showerTime, | ||
| EatingType eating, | ||
| LightsOutType lightsOut, | ||
| String lightsOutTime, | ||
| HomeVisitType homeVisit, | ||
| SmokingType smoking, | ||
| RefrigeratorType refrigerator, | ||
| String hairDryer, | ||
| AlarmType alarm, | ||
| EarphoneType earphone, | ||
| KeyskinType keyskin, | ||
| HeatType heat, | ||
| ColdType cold, | ||
| StudyType study, | ||
| TrashCanType trashCan, | ||
| String otherNotes | ||
| ) { | ||
| this.bedtime = bedtime; | ||
| this.wakeUp = wakeUp; | ||
| this.returnHome = returnHome; | ||
| this.returnHomeTime = returnHomeTime; | ||
| this.cleaning = cleaning; | ||
| this.phoneCall = phoneCall; | ||
| this.sleepLight = sleepLight; | ||
| this.sleepHabit = sleepHabit; | ||
| this.snoring = snoring; | ||
| this.showerTime = showerTime; | ||
| this.eating = eating; | ||
| this.lightsOut = lightsOut; | ||
| this.lightsOutTime = lightsOutTime; | ||
| this.homeVisit = homeVisit; | ||
| this.smoking = smoking; | ||
| this.refrigerator = refrigerator; | ||
| this.hairDryer = hairDryer; | ||
| this.alarm = alarm; | ||
| this.earphone = earphone; | ||
| this.keyskin = keyskin; | ||
| this.heat = heat; | ||
| this.cold = cold; | ||
| this.study = study; | ||
| this.trashCan = trashCan; | ||
| this.otherNotes = otherNotes; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
대량 업데이트 메서드 도입에 대한 의견 드립니다.
필드별 setter 남발을 피하고 한 번의 호출로 일괄 업데이트하도록 한 의도는 좋습니다. 다만 두 가지 정도 검토해 보시면 좋을 것 같습니다.
-
엔티티 API로서의 결합도: 25개 파라미터를 받는 메서드는 시그니처 변경 비용이 크고, 사실상 DTO의 형태를 그대로 엔티티에 주입하는 구조입니다. 매퍼 내부에서만 호출된다면 이 정도로 충분하지만, 장기적으로는 전용 값 객체(예:
ChecklistUpdateCommand)를 받는 형태로 바꾸면 유지보수가 더 쉬워질 수 있습니다. -
null 허용성:
bedtime,wakeUp,returnHome등은 컬럼 단에서nullable = false인데, 이 메서드는 null 체크 없이 그대로 대입합니다. 현재는UpdateRoomRuleRequest/UpdateUserChecklistRequest의@NotBlank/@NotNull검증으로 커버되고 있지만, 엔티티 자체에서도 필수 필드에 대해서는 방어적 검증(Objects.requireNonNull등)을 두면 컨트롤러 단 검증을 우회하는 경로에서 불변식을 보호할 수 있습니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/main/java/com/project/dorumdorum/domain/checklist/domain/entity/ChecklistBase.java`
around lines 126 - 178, The updateChecklist method accepts 25 raw parameters
which increases coupling and risks null assignment to non-nullable columns;
refactor by changing updateChecklist to accept a single value object (e.g.,
ChecklistUpdateCommand) instead of many params and, inside updateChecklist,
defensively validate required fields with Objects.requireNonNull (for fields
like bedtime, wakeUp, returnHome etc.) so the entity enforces non-null
invariants even if controller-level DTOs such as
UpdateRoomRuleRequest/UpdateUserChecklistRequest are bypassed.
Contributor
Author
There was a problem hiding this comment.
이거 안그래도 좀 걸렸었는데 mapper에 대해서 리팩토링하는건 어떻게 생각해요?
- mapper: DTO 변환 + update 위임
- entity: 자기 상태 변경 책임 보유
- base entity: 중복 필드 적용 헬퍼만 제공
이렇게 수정하려고 하는데 의견 남겨주세요~
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
📝 Pull Request Template
📌 제목
fix/#91 방 관리 기능 개선 및 버그 수정 (방 삭제, 강퇴, 개인 알림, 트랜잭션 정합성)
📢 요약
이슈 #91에서 요청된 방 관리 관련 버그 수정 및 신규 기능을 구현했습니다.
버그 수정
findMyRoommates쿼리에deletedAt IS NULL필터 누락으로 삭제된 룸메이트/방이 조회되는 문제 수정신규 기능
DELETE /api/rooms/{roomNo})/queue/notification개인 알림 전송트랜잭션 정합성 수정
DeleteRoomUseCase,KickRoommateUseCase에roomService.flush()추가ChatMessageRepository@Modifying에flushAutomatically = true추가RoomDeletedEventListener채팅방 삭제를 벌크 JPQL로 교체 (deleteByChatRoomNo)RoommateKickedEventListener이중 조회(isMember + find) → 단일findOptional로 교체🔗 연관 이슈: Resolves #91
🚀 PR 유형
📜 상세 설명
1. 방 삭제 기능 (
DeleteRoomUseCase)방 삭제 시 아래 순서로 처리합니다.
room.delete() → Room 소프트 삭제 (deletedAt 설정)
roomService.flush() → DB 반영 (이후 findByRoomNoForUpdate의 deletedAt IS NULL 조건으로 재진입 차단)
leaveRoom() → 방장 Roommate 하드 삭제
deleteAllByRoom() → RoomRequest, RoomRule, RoomLike 벌크 삭제
publishEvent() → RoomDeletedEvent 발행 → 채팅방 cascade 삭제 (BEFORE_COMMIT 리스너)
비즈니스 규칙
ROOM005NO_PERMISSION_ON_ROOMROOM018CANNOT_DELETE_COMPLETED_ROOMROOM017ROOM_HAS_MEMBERS2. WebSocket 개인 알림 (
/queue/notification)기존
/topic/브로드캐스트는 채팅방을 열고 있는 사용자에게만 전달됩니다. 채팅방 밖에 있는 사용자도 강퇴·방 삭제 알림을 받을 수 있도록/queue/notification개인 알림을 추가했습니다./topic/chat-room/{chatRoomNo}/queue/notificationconvertAndSendToUser()라우팅을 위해 WebSocket Handshake 시점에StompPrincipal을 등록하는JwtPrincipalHandshakeHandler를 추가했습니다.NotificationMessage { type, roomNo, chatRoomNo }
type: "ROOM_DELETED" | "KICKED_FROM_ROOM"
3. 트랜잭션 정합성 수정
flush()추가 위치DeleteRoomUseCase:room.delete()직후 → bulk DELETE 전 소프트 삭제 상태 DB 반영KickRoommateUseCase:room.minusCurrentMate()직후 → BEFORE_COMMIT 리스너가 최신 인원 수 조회 가능ChatMessageRepository:@Modifying(flushAutomatically = true)→ unreadCount 집계 오류 방지RoommateKickedEventListener이중 조회 제거RoomDeletedEventListener벌크 DELETE 교체chatRoomService.delete(entity)(단건 엔티티 삭제)를chatRoomService.deleteByChatRoomNo()(JPQL 벌크 DELETE)로 교체했습니다. BEFORE_COMMIT 컨텍스트에서 1차 캐시와 bulk DELETE가 혼재하면 캐시 불일치가 발생할 수 있어 일관성을 위해 통일했습니다.✅ PR 체크리스트
📜 기타
Summary by CodeRabbit
새로운 기능
버그 수정
테스트