Merged
Conversation
- AlertDeduplicationService를 도입하여 짧은 시간 내 발생하는 동일 알림의 중복 전송 방지 - EventListener를 통해 애플리케이션 시작 및 종료 시 Discord 알림 전송 로직 추가 - 테스트 코드 추가
[FEAT] 동시성 제어 보강 및 상태 정합성 개선
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (16)
📝 WalkthroughWalkthroughRefactors chat-room transactional listeners and join/leave flows, adds STOMP-based per-chat-room presence tracking, strengthens DB uniqueness via schema and entity constraints, maps DataIntegrityViolationException to domain codes, introduces Discord alerting/deduplication and Logback integration, expands locking helpers, and adds tests and Javadocs. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant STOMP
participant Listener as ChatRoomPresenceListener
participant PresenceRepo as UserPresenceRepository
participant SseRegistry as SseEmitterRegistry
rect rgba(200,150,255,0.5)
Client->>STOMP: SessionSubscribeEvent(destination=/topic/chat-room/{roomId})
STOMP->>Listener: handleSubscribe(event)
Listener->>Listener: extract sessionId, userNo, chatRoomNo
Listener->>PresenceRepo: setInChatroom(userNo, chatRoomNo)
PresenceRepo-->>PresenceRepo: persist IN_CHATROOM with TTL
Listener->>Listener: store session in in-memory map
end
rect rgba(255,150,150,0.5)
Client->>STOMP: SessionDisconnectEvent
STOMP->>Listener: handleDisconnect(event)
Listener->>Listener: remove session -> resolve userNo
Listener->>SseRegistry: hasConnection(userNo)
alt has active SSE
SseRegistry-->>Listener: true
Listener->>PresenceRepo: setOnline(userNo)
else no SSE
SseRegistry-->>Listener: false
Listener->>PresenceRepo: setOffline(userNo)
end
PresenceRepo-->>PresenceRepo: update presence / TTL as needed
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
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)
47-58:⚠️ Potential issue | 🟡 MinorIncorrect exception type in room-not-found test.
The test name indicates it should test the "room not found" scenario, but the stub throws
NO_PERMISSION_ON_ROOMinstead of_NOT_FOUND. According toRoomService.findByIdForUpdate, the method throwsRestApiException(_NOT_FOUND)when the room doesn't exist.🐛 Proposed fix
`@Test` `@DisplayName`("존재하지 않는 방 ID로 요청 시 RestApiException 발생") void execute_WhenRoomNotFound_ThrowsException() { - when(roomService.findByIdForUpdate("nonexistent")).thenThrow(new RestApiException(NO_PERMISSION_ON_ROOM)); + when(roomService.findByIdForUpdate("nonexistent")).thenThrow(new RestApiException(_NOT_FOUND)); assertThatThrownBy(() -> useCase.execute("host", "nonexistent", "member1")) .isInstanceOf(RestApiException.class);🤖 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 47 - 58, The test KickRoommateUseCaseTest.execute_WhenRoomNotFound_ThrowsException currently stubs roomService.findByIdForUpdate to throw RestApiException(NO_PERMISSION_ON_ROOM); change that to throw the not-found error instead—e.g., new RestApiException(ROOM_NOT_FOUND) (or the actual _NOT_FOUND constant used by RoomService.findByIdForUpdate) so the test matches the "room not found" scenario; keep the rest of the assertions (no roommateService or eventPublisher interactions) unchanged.src/main/java/com/project/dorumdorum/domain/chat/application/usecase/GetOrCreateDirectChatRoomUseCase.java (1)
30-45:⚠️ Potential issue | 🔴 CriticalDon't authorize from
applicantUserNoalone.Line 34 trusts the caller-supplied
applicantUserNoinstead of a persisted room-application relation. Any user can pass their own ID there, and the host can pass an arbitrary user ID, to create/access a "direct applicant chat" that has no real application behind it. Validate that(roomNo, applicantUserNo)actually exists before returning or creating the chat room.🤖 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/usecase/GetOrCreateDirectChatRoomUseCase.java` around lines 30 - 45, The code in GetOrCreateDirectChatRoomUseCase.execute currently trusts the caller-supplied applicantUserNo; fix it by verifying a persisted room-application relationship before returning/creating a direct chat. After retrieving Room via roomService.findById and before calling chatRoomService.findDirectChatRoom or chatRoomService.createDirectChatRoom, call the domain service/repository that represents applications (e.g., roomApplicationService.existsByRoomNoAndApplicantUserNo or roomApplicationRepository.findByRoomNoAndApplicantUserNo) to ensure (roomNo, applicantUserNo) is a real application; if it does not exist, throw the appropriate RestApiException (e.g., NOT_CHAT_ROOM_MEMBER or a new NOT_ROOM_APPLICANT error). Keep the existing host/caller check but rely on the persisted relation to authorize host-initiated access to an applicant’s chat.
🧹 Nitpick comments (8)
src/main/java/com/project/dorumdorum/domain/notice/application/usecase/LoadNoticesUseCase.java (1)
21-25: Good addition of method documentation.The JavaDoc accurately describes the method's behavior—fetching notices in descending write-date order and mapping to response DTOs.
📝 Optional: Consider standard JavaDoc format
For better IDE/tooling integration, you might use standard JavaDoc tags:
/** * 공지사항 목록 조회 - * - 작성일 내림차순으로 공지사항을 조회 - * - 응답 DTO 목록으로 변환해 반환 + * + * 작성일 내림차순으로 공지사항을 조회하고 응답 DTO 목록으로 변환합니다. + * + * `@return` 작성일 내림차순으로 정렬된 공지사항 응답 목록 */🤖 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/notice/application/usecase/LoadNoticesUseCase.java` around lines 21 - 25, The existing block comment for the notices retrieval should be converted into a proper JavaDoc for the LoadNoticesUseCase method that fetches notices (the method that returns the list of NoticeResponse DTOs); replace the current multi-line comment with a standard JavaDoc starting with /** and include appropriate tags such as `@return` describing the returned List<NoticeResponse> and `@implNote` or `@see` if relevant, and ensure the description still states that notices are fetched in descending write-date order and mapped to response DTOs.src/test/java/com/project/dorumdorum/domain/notification/infra/sse/ChatRoomPresenceListenerTest.java (1)
57-79: Consider parameterizing the disconnect branch tests.Both disconnect tests differ mainly by
hasConnectionreturn value and expected repository method. A parameterized test would reduce duplication and keep intent tighter.🤖 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/notification/infra/sse/ChatRoomPresenceListenerTest.java` around lines 57 - 79, Tests in ChatRoomPresenceListenerTest duplicate the disconnect scenarios; replace the two tests with a single parameterized test that calls listener.handleSubscribe(...) then sets sseEmitterRegistry.hasConnection("user-1") to true/false (via `@ValueSource` or `@CsvSource`) and invokes listener.handleDisconnect(...), asserting userPresenceRepository.setOnline("user-1") when hasConnection is true and setOffline("user-1") when false, and verifying the opposite method was never called; keep references to listener.handleSubscribe, sseEmitterRegistry.hasConnection, listener.handleDisconnect, and userPresenceRepository.setOnline/setOffline to locate and update the existing test logic.src/main/java/com/project/dorumdorum/domain/room/application/usecase/DecideApplicationRequestUseCase.java (1)
86-86: TODO comment indicates pending notification feature.The
rejectmethod has a TODO for notifying the applicant upon rejection, similar to theRoommateAcceptedEventpublished inapprove.Would you like me to help implement the rejection notification event, or open a new issue to track this task?
🤖 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/DecideApplicationRequestUseCase.java` at line 86, The TODO in DecideApplicationRequestUseCase.reject means we need to publish a rejection event to notify the applicant similar to how approve publishes RoommateAcceptedEvent; update the reject method to create and publish a RoommateRejectedEvent (or equivalent) containing the roomRequest id and applicant userNo (roomRequest.getUserNo()) via the same event publisher used in approve, and ensure any new event class (RoommateRejectedEvent) mirrors the accepted event's payload and handlers so notification delivery is triggered.src/main/java/com/project/dorumdorum/global/exception/code/status/UserErrorStatus.java (1)
13-15: Inconsistent error code prefixes inUserErrorStatusentries.The new
DUPLICATE_SIGN_UP_INFOcorrectly uses theUSER001code prefix. However,DUPLICATE_EMAILandEMAIL_NOT_FOUNDboth useROOM002—a room-related error code that doesn't belong in user error statuses. These should useUSERprefixed codes.Fixing this requires updating the test in
ExceptionAdviceTest.java(line 87) that currently assertsROOM002:♻️ Suggested fix for error code consistency
-DUPLICATE_EMAIL(HttpStatus.BAD_REQUEST, "ROOM002", "이미 가입된 이메일입니다."), +DUPLICATE_EMAIL(HttpStatus.BAD_REQUEST, "USER002", "이미 가입된 이메일입니다."), DUPLICATE_SIGN_UP_INFO(HttpStatus.BAD_REQUEST, "USER001", "이미 가입된 회원 정보입니다."), -EMAIL_NOT_FOUND(HttpStatus.BAD_REQUEST, "ROOM002", "가입되지 않은 이메일입니다."), +EMAIL_NOT_FOUND(HttpStatus.BAD_REQUEST, "USER003", "가입되지 않은 이메일입니다."),Update test assertion accordingly.
🤖 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/exception/code/status/UserErrorStatus.java` around lines 13 - 15, Update the inconsistent error code prefixes for the user-related statuses: change DUPLICATE_EMAIL and EMAIL_NOT_FOUND to use USER-prefixed codes (e.g., align with DUPLICATE_SIGN_UP_INFO's "USER001" style) so all user errors share the USER namespace, and then update the test assertion in ExceptionAdviceTest that expects "ROOM002" (currently at the assertion around the duplicated email check) to assert the new USER-prefixed code instead; ensure the enum constants (DUPLICATE_EMAIL, EMAIL_NOT_FOUND) and the test expectation are kept consistent.src/test/resources/application-test.yml (1)
17-17: Considerddl-auto: validatewhenschema.sqlis authoritative in tests.With
create-drop, Hibernate can mask drift thatvalidatewould catch early against your curated DDL.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/resources/application-test.yml` at line 17, Tests are currently using Hibernate DDL mode that can hide schema drift from the authoritative schema.sql; update the test config to use the Hibernate property spring.jpa.hibernate.ddl-auto: validate (instead of create-drop) so Hibernate validates the entity mappings against your curated schema.sql at startup; ensure the YAML entry for spring.jpa.hibernate.ddl-auto is added/updated in the same test config where defer-datasource-initialization and schema.sql are configured so validation runs after the SQL schema is applied.src/main/java/com/project/dorumdorum/domain/chat/domain/entity/ChatRoom.java (1)
17-21: Make the partial-index rollout part of the release checklist.After removing the entity-level uniqueness metadata, duplicate room prevention lives only in the DB partial indexes. Please ensure those indexes are already applied in production before this ships.
🤖 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/entity/ChatRoom.java` around lines 17 - 21, Add a release-checklist step to ensure DB partial indexes that enforce duplicate-room prevention are applied in production before deploying the change that removed entity-level uniqueness metadata; specifically verify the partial index definitions referenced by the ChatRoom entity (e.g., index name idx_chat_room_last_message_at and any partial unique indexes that replaced entity constraints) exist and are active, and include steps to apply them (migration script, rollback plan) and to validate their presence (query pg_catalog/information_schema or run a quick INSERT test) as part of the release procedure.src/test/java/com/project/dorumdorum/domain/checklist/unit/usecase/UpdateRoomRuleUseCaseTest.java (1)
114-117: Consider simplifying the error code assertion.The chained
.getCode().getCode()calls are confusing and fragile. If theErrorCodestructure changes, this will break silently. Consider asserting against the enum directly or using a clearer comparison.assertThatThrownBy(() -> useCase.execute("u1", "r1", invalidRequest)) .isInstanceOf(RestApiException.class) - .extracting("errorCode.code") - .isEqualTo(INVALID_ROOM_CAPACITY.getCode().getCode()); + .extracting("errorCode") + .isEqualTo(INVALID_ROOM_CAPACITY);🤖 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/checklist/unit/usecase/UpdateRoomRuleUseCaseTest.java` around lines 114 - 117, The test currently asserts the error code using fragile chained calls in UpdateRoomRuleUseCaseTest; instead, change the assertion to compare the ErrorCode enum (INVALID_ROOM_CAPACITY) or the RestApiException's ErrorCode field directly: catch the exception from useCase.execute("u1", "r1", invalidRequest) as a RestApiException and assert that its getErrorCode() (or equivalent getter) equals INVALID_ROOM_CAPACITY, or use AssertJ's extracting to pull the errorCode object and compare it to the enum, avoiding .getCode().getCode() chains.src/main/java/com/project/dorumdorum/domain/user/application/usecase/SignUpUseCase.java (1)
21-26: Javadoc comment is incomplete.The comment mentions "중복 이메일 가입을 차단" (blocking duplicate email registration), but the code now also blocks duplicate student numbers. Consider updating the comment to reflect both checks for accuracy.
/** * 회원가입 처리 * - 비밀번호 확인 여부를 검증 - * - 중복 이메일 가입을 차단 + * - 중복 이메일 또는 학번 가입을 차단 * - 사용자를 저장하고 생성된 사용자 번호를 반환 */🤖 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/user/application/usecase/SignUpUseCase.java` around lines 21 - 26, The Javadoc on SignUpUseCase is outdated: update the class/method comment for the 회원가입 처리 to list both duplicate-email and duplicate-student-number checks (in addition to the existing password confirmation and user save/return behavior). Mention the specific validations performed (비밀번호 확인 검증, 이메일 중복 차단, 학번 중복 차단) and that the user is persisted and the created user ID is returned so the documentation matches the implementation in SignUpUseCase.
🤖 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/usecase/LeaveChatRoomUseCase.java`:
- Around line 96-98: The message construction currently always appends "가
퇴장했습니다." which is grammatically wrong for names ending with a consonant; update
LeaveChatRoomUseCase to compute displayName (from
leavingUser.getNickname()/getName()) then decide the correct subject particle by
adding a private helper method (e.g., endsWithConsonant(String)) that checks the
last Hangul syllable's jongseong, and use it to choose between "이" and "가" when
setting the content variable so it becomes either displayName + "이 퇴장했습니다." or
displayName + "가 퇴장했습니다.".
In
`@src/main/java/com/project/dorumdorum/domain/notification/infra/repository/RedisUserPresenceRepository.java`:
- Around line 30-39: refreshPresence currently does a non-atomic read-then-write
(redisTemplate.opsForValue().get(...) then set(...)), which can race with
setInChatroom and overwrite IN_CHATROOM with ONLINE; change refreshPresence to
perform the read-and-conditional-write atomically (e.g., use
redisTemplate.execute/RedisCallback with a small Lua script or Redis
compare-and-set pattern) so the logic "if current value == OFFLINE then set to
ONLINE and apply TTL, else leave unchanged" runs as a single Redis command;
reference the refreshPresence method, redisTemplate.opsForValue().get / set
calls, and the UserPresence.fromRedisValue / UserPresence.online /
toRedisValue/TTL so you update the existing method to use an atomic Redis
operation that preserves IN_CHATROOM.
In
`@src/main/java/com/project/dorumdorum/domain/notification/infra/sse/ChatRoomPresenceListener.java`:
- Around line 52-63: When removing the ChatRoomSession for sessionId, current
logic flips the user's presence based only on sseEmitterRegistry.hasConnection
and the single removed session; instead, before calling
userPresenceRepository.setOnline/setOffline, check whether there are any
remaining chat-room sessions for the same user (e.g., scan chatRoomSessions
values for entries with session.userNo() or use a per-user session count) and
only consider the user offline if there are no other chat-room sessions AND no
SSE connections (sseEmitterRegistry.hasConnection(session.userNo()) is false);
update the disconnect flow in ChatRoomPresenceListener to preserve IN_CHATROOM
when other sessions for the same user remain and only call
userPresenceRepository.setOffline when both checks indicate no remaining
activity for that user.
In `@src/main/java/com/project/dorumdorum/domain/room/domain/entity/Room.java`:
- Around line 99-101: The isValidCapacity method in Room currently autounboxes
the Integer parameter and can NPE; update Room.isValidCapacity(Integer capacity)
to first guard against null (e.g., return false or handle per domain rule when
capacity is null) before comparing to currentMateCount, ensuring the comparison
uses safe null-aware logic referencing currentMateCount and capacity to avoid
runtime NPEs.
In `@src/main/java/com/project/dorumdorum/global/exception/ExceptionAdvice.java`:
- Around line 51-58: The DATA_INTEGRITY_ERROR_CODES map in ExceptionAdvice is
missing five unique-constraint keys—add entries for "uk_room_like_user_room",
"uk_device_user_device", "uk_chat_room_direct", "uk_chat_room_group", and
"uk_room_rule_room_no" mapping them to the appropriate domain BaseCodeInterface
constants (e.g., DUPLICATE_ROOM_LIKE, DUPLICATE_DEVICE,
DUPLICATE_CHAT_ROOM_DIRECT, DUPLICATE_CHAT_ROOM_GROUP, DUPLICATE_ROOM_RULE);
update the DATA_INTEGRITY_ERROR_CODES Map.of declaration to include these
mappings and, if those BaseCodeInterface constants do not yet exist, define them
in the domain error codes enum/class so the existing SQLState check using
DUPLICATE_KEY_SQL_STATE returns the correct user-facing error instead of falling
through to the 500 handler.
In `@src/main/resources/application-dev.yml`:
- Line 13: The app now uses Hibernate's ddl-auto: validate, so Spring won't
create tables; either enable execution of schema.sql in the dev profile by
adding spring.sql.init.mode: always to application-dev.yml, or move all CREATE
TABLE DDL into schema.sql (it currently only contains indexes/constraints), or
add a migration tool (Flyway/Liquibase) and provide proper migrations; update
the dev configuration or schema file accordingly so the database schema exists
before Hibernate validation runs.
In `@src/main/resources/schema.sql`:
- Around line 19-28: The schema change adds partial unique indexes
uk_chat_room_group and uk_chat_room_direct on chat_room (room_no) and (room_no,
applicant_user_no) filtered by chat_room_type, but existing duplicate rows will
cause index creation to fail; add a pre-deployment migration/script that queries
chat_room for duplicates (grouped by room_no where chat_room_type='GROUP', and
grouped by room_no, applicant_user_no where chat_room_type='DIRECT'), fail the
deployment if any duplicates are found, and provide safe remediation steps
(report offending ids, or deduplicate/merge/remove duplicates) before running
the index creation; ensure this check is run as part of the deployment pipeline
and document the remediation action taken.
In
`@src/test/java/com/project/dorumdorum/domain/notification/infra/sse/ChatRoomPresenceListenerTest.java`:
- Around line 69-79: The test may pass spuriously because unstubbed booleans
default to false; ensure the listener actually checked the SSE registry for the
specific user by verifying sseEmitterRegistry.hasConnection("user-1") was
invoked. Update the test method handleDisconnect_WhenNoSseConnection_SetsOffline
to include a verify(sseEmitterRegistry).hasConnection("user-1") (or equivalent
assertion) after invoking listener.handleDisconnect and before asserting
userPresenceRepository.setOffline/setOnline, so you confirm the correct argument
was checked.
In
`@src/test/java/com/project/dorumdorum/global/exception/ExceptionAdviceTest.java`:
- Around line 110-127: Add a second unit test (or extend the existing test
handleDataIntegrityViolationException_WhenDuplicateUserSignUp_ReturnsBadRequest)
that constructs a ConstraintViolationException with the same SQL state but with
the constraint name "uk_user_student_no" and wraps it in a
DataIntegrityViolationException, then call
exceptionAdvice.handleDataIntegrityViolationException(exception) and assert the
response status is 400 and response.getBody().getCode() equals "USER001" (and
verify systemAlertPublisher never publishes); this ensures the code path
handling "uk_user_student_no" (alongside "uk_user_email") maps to
DUPLICATE_SIGN_UP_INFO.
---
Outside diff comments:
In
`@src/main/java/com/project/dorumdorum/domain/chat/application/usecase/GetOrCreateDirectChatRoomUseCase.java`:
- Around line 30-45: The code in GetOrCreateDirectChatRoomUseCase.execute
currently trusts the caller-supplied applicantUserNo; fix it by verifying a
persisted room-application relationship before returning/creating a direct chat.
After retrieving Room via roomService.findById and before calling
chatRoomService.findDirectChatRoom or chatRoomService.createDirectChatRoom, call
the domain service/repository that represents applications (e.g.,
roomApplicationService.existsByRoomNoAndApplicantUserNo or
roomApplicationRepository.findByRoomNoAndApplicantUserNo) to ensure (roomNo,
applicantUserNo) is a real application; if it does not exist, throw the
appropriate RestApiException (e.g., NOT_CHAT_ROOM_MEMBER or a new
NOT_ROOM_APPLICANT error). Keep the existing host/caller check but rely on the
persisted relation to authorize host-initiated access to an applicant’s chat.
In
`@src/test/java/com/project/dorumdorum/domain/room/unit/usecase/KickRoommateUseCaseTest.java`:
- Around line 47-58: The test
KickRoommateUseCaseTest.execute_WhenRoomNotFound_ThrowsException currently stubs
roomService.findByIdForUpdate to throw RestApiException(NO_PERMISSION_ON_ROOM);
change that to throw the not-found error instead—e.g., new
RestApiException(ROOM_NOT_FOUND) (or the actual _NOT_FOUND constant used by
RoomService.findByIdForUpdate) so the test matches the "room not found"
scenario; keep the rest of the assertions (no roommateService or eventPublisher
interactions) unchanged.
---
Nitpick comments:
In
`@src/main/java/com/project/dorumdorum/domain/chat/domain/entity/ChatRoom.java`:
- Around line 17-21: Add a release-checklist step to ensure DB partial indexes
that enforce duplicate-room prevention are applied in production before
deploying the change that removed entity-level uniqueness metadata; specifically
verify the partial index definitions referenced by the ChatRoom entity (e.g.,
index name idx_chat_room_last_message_at and any partial unique indexes that
replaced entity constraints) exist and are active, and include steps to apply
them (migration script, rollback plan) and to validate their presence (query
pg_catalog/information_schema or run a quick INSERT test) as part of the release
procedure.
In
`@src/main/java/com/project/dorumdorum/domain/notice/application/usecase/LoadNoticesUseCase.java`:
- Around line 21-25: The existing block comment for the notices retrieval should
be converted into a proper JavaDoc for the LoadNoticesUseCase method that
fetches notices (the method that returns the list of NoticeResponse DTOs);
replace the current multi-line comment with a standard JavaDoc starting with /**
and include appropriate tags such as `@return` describing the returned
List<NoticeResponse> and `@implNote` or `@see` if relevant, and ensure the
description still states that notices are fetched in descending write-date order
and mapped to response DTOs.
In
`@src/main/java/com/project/dorumdorum/domain/room/application/usecase/DecideApplicationRequestUseCase.java`:
- Line 86: The TODO in DecideApplicationRequestUseCase.reject means we need to
publish a rejection event to notify the applicant similar to how approve
publishes RoommateAcceptedEvent; update the reject method to create and publish
a RoommateRejectedEvent (or equivalent) containing the roomRequest id and
applicant userNo (roomRequest.getUserNo()) via the same event publisher used in
approve, and ensure any new event class (RoommateRejectedEvent) mirrors the
accepted event's payload and handlers so notification delivery is triggered.
In
`@src/main/java/com/project/dorumdorum/domain/user/application/usecase/SignUpUseCase.java`:
- Around line 21-26: The Javadoc on SignUpUseCase is outdated: update the
class/method comment for the 회원가입 처리 to list both duplicate-email and
duplicate-student-number checks (in addition to the existing password
confirmation and user save/return behavior). Mention the specific validations
performed (비밀번호 확인 검증, 이메일 중복 차단, 학번 중복 차단) and that the user is persisted and
the created user ID is returned so the documentation matches the implementation
in SignUpUseCase.
In
`@src/main/java/com/project/dorumdorum/global/exception/code/status/UserErrorStatus.java`:
- Around line 13-15: Update the inconsistent error code prefixes for the
user-related statuses: change DUPLICATE_EMAIL and EMAIL_NOT_FOUND to use
USER-prefixed codes (e.g., align with DUPLICATE_SIGN_UP_INFO's "USER001" style)
so all user errors share the USER namespace, and then update the test assertion
in ExceptionAdviceTest that expects "ROOM002" (currently at the assertion around
the duplicated email check) to assert the new USER-prefixed code instead; ensure
the enum constants (DUPLICATE_EMAIL, EMAIL_NOT_FOUND) and the test expectation
are kept consistent.
In
`@src/test/java/com/project/dorumdorum/domain/checklist/unit/usecase/UpdateRoomRuleUseCaseTest.java`:
- Around line 114-117: The test currently asserts the error code using fragile
chained calls in UpdateRoomRuleUseCaseTest; instead, change the assertion to
compare the ErrorCode enum (INVALID_ROOM_CAPACITY) or the RestApiException's
ErrorCode field directly: catch the exception from useCase.execute("u1", "r1",
invalidRequest) as a RestApiException and assert that its getErrorCode() (or
equivalent getter) equals INVALID_ROOM_CAPACITY, or use AssertJ's extracting to
pull the errorCode object and compare it to the enum, avoiding
.getCode().getCode() chains.
In
`@src/test/java/com/project/dorumdorum/domain/notification/infra/sse/ChatRoomPresenceListenerTest.java`:
- Around line 57-79: Tests in ChatRoomPresenceListenerTest duplicate the
disconnect scenarios; replace the two tests with a single parameterized test
that calls listener.handleSubscribe(...) then sets
sseEmitterRegistry.hasConnection("user-1") to true/false (via `@ValueSource` or
`@CsvSource`) and invokes listener.handleDisconnect(...), asserting
userPresenceRepository.setOnline("user-1") when hasConnection is true and
setOffline("user-1") when false, and verifying the opposite method was never
called; keep references to listener.handleSubscribe,
sseEmitterRegistry.hasConnection, listener.handleDisconnect, and
userPresenceRepository.setOnline/setOffline to locate and update the existing
test logic.
In `@src/test/resources/application-test.yml`:
- Line 17: Tests are currently using Hibernate DDL mode that can hide schema
drift from the authoritative schema.sql; update the test config to use the
Hibernate property spring.jpa.hibernate.ddl-auto: validate (instead of
create-drop) so Hibernate validates the entity mappings against your curated
schema.sql at startup; ensure the YAML entry for spring.jpa.hibernate.ddl-auto
is added/updated in the same test config where defer-datasource-initialization
and schema.sql are configured so validation runs after the SQL schema is
applied.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: dae9b1d2-2eaf-4207-9de0-696b9879f9ba
📒 Files selected for processing (82)
src/main/java/com/project/dorumdorum/domain/calendar/application/usecase/LoadCalendarEventsUseCase.javasrc/main/java/com/project/dorumdorum/domain/chat/application/usecase/GetChatRoomMembersUseCase.javasrc/main/java/com/project/dorumdorum/domain/chat/application/usecase/GetOrCreateDirectChatRoomUseCase.javasrc/main/java/com/project/dorumdorum/domain/chat/application/usecase/JoinChatRoomUseCase.javasrc/main/java/com/project/dorumdorum/domain/chat/application/usecase/LeaveChatRoomUseCase.javasrc/main/java/com/project/dorumdorum/domain/chat/application/usecase/LoadChatMessagesUseCase.javasrc/main/java/com/project/dorumdorum/domain/chat/application/usecase/LoadMyChatRoomsUseCase.javasrc/main/java/com/project/dorumdorum/domain/chat/application/usecase/MarkChatRoomReadUseCase.javasrc/main/java/com/project/dorumdorum/domain/chat/application/usecase/SendGroupChatMessageUseCase.javasrc/main/java/com/project/dorumdorum/domain/chat/domain/entity/ChatRoom.javasrc/main/java/com/project/dorumdorum/domain/chat/domain/repository/ChatRoomMemberRepository.javasrc/main/java/com/project/dorumdorum/domain/chat/domain/service/ChatRoomMemberService.javasrc/main/java/com/project/dorumdorum/domain/checklist/application/usecase/CreateUserChecklistUseCase.javasrc/main/java/com/project/dorumdorum/domain/checklist/application/usecase/LoadMyRoomRuleUseCase.javasrc/main/java/com/project/dorumdorum/domain/checklist/application/usecase/LoadUserChecklistUseCase.javasrc/main/java/com/project/dorumdorum/domain/checklist/application/usecase/UpdateRoomRuleUseCase.javasrc/main/java/com/project/dorumdorum/domain/checklist/application/usecase/UpdateUserChecklistUseCase.javasrc/main/java/com/project/dorumdorum/domain/notice/application/usecase/LoadNoticesUseCase.javasrc/main/java/com/project/dorumdorum/domain/notification/application/usecase/LoadMyNotificationsUseCase.javasrc/main/java/com/project/dorumdorum/domain/notification/application/usecase/MarkAsReadNotificationUseCase.javasrc/main/java/com/project/dorumdorum/domain/notification/application/usecase/RegisterDeviceTokenUseCase.javasrc/main/java/com/project/dorumdorum/domain/notification/application/usecase/SendTestNotificationUseCase.javasrc/main/java/com/project/dorumdorum/domain/notification/domain/entity/Device.javasrc/main/java/com/project/dorumdorum/domain/notification/domain/repository/UserPresenceRepository.javasrc/main/java/com/project/dorumdorum/domain/notification/infra/repository/RedisUserPresenceRepository.javasrc/main/java/com/project/dorumdorum/domain/notification/infra/sse/ChatRoomPresenceListener.javasrc/main/java/com/project/dorumdorum/domain/notification/infra/sse/SseEmitterRegistry.javasrc/main/java/com/project/dorumdorum/domain/notification/infra/sse/SseHeartbeatScheduler.javasrc/main/java/com/project/dorumdorum/domain/room/application/usecase/ApplyRoomUseCase.javasrc/main/java/com/project/dorumdorum/domain/room/application/usecase/CancelRoomApplicationUseCase.javasrc/main/java/com/project/dorumdorum/domain/room/application/usecase/CheckMyRoomUseCase.javasrc/main/java/com/project/dorumdorum/domain/room/application/usecase/ConfirmRoomAssignmentUseCase.javasrc/main/java/com/project/dorumdorum/domain/room/application/usecase/CreateRoomUseCase.javasrc/main/java/com/project/dorumdorum/domain/room/application/usecase/DecideApplicationRequestUseCase.javasrc/main/java/com/project/dorumdorum/domain/room/application/usecase/FindRoomsUseCase.javasrc/main/java/com/project/dorumdorum/domain/room/application/usecase/KickRoommateUseCase.javasrc/main/java/com/project/dorumdorum/domain/room/application/usecase/LoadMyAppliedRoomsUseCase.javasrc/main/java/com/project/dorumdorum/domain/room/application/usecase/LoadMyLikedRoomsUseCase.javasrc/main/java/com/project/dorumdorum/domain/room/application/usecase/LoadMyRoomsUseCase.javasrc/main/java/com/project/dorumdorum/domain/room/application/usecase/LoadRoomApplicationsUseCase.javasrc/main/java/com/project/dorumdorum/domain/room/application/usecase/RoomLikeUseCase.javasrc/main/java/com/project/dorumdorum/domain/room/application/usecase/UpdateRoomTitleUseCase.javasrc/main/java/com/project/dorumdorum/domain/room/domain/entity/Room.javasrc/main/java/com/project/dorumdorum/domain/room/domain/entity/RoomLike.javasrc/main/java/com/project/dorumdorum/domain/room/domain/entity/RoomRequest.javasrc/main/java/com/project/dorumdorum/domain/room/domain/repository/RoomRepository.javasrc/main/java/com/project/dorumdorum/domain/room/domain/service/RoomLikeService.javasrc/main/java/com/project/dorumdorum/domain/room/domain/service/RoomService.javasrc/main/java/com/project/dorumdorum/domain/roommate/application/usecase/LoadMyRoommatesUseCase.javasrc/main/java/com/project/dorumdorum/domain/roommate/domain/entity/Roommate.javasrc/main/java/com/project/dorumdorum/domain/roommate/domain/service/RoommateService.javasrc/main/java/com/project/dorumdorum/domain/user/application/usecase/LoadUserProfileUseCase.javasrc/main/java/com/project/dorumdorum/domain/user/application/usecase/LoginUseCase.javasrc/main/java/com/project/dorumdorum/domain/user/application/usecase/LogoutUseCase.javasrc/main/java/com/project/dorumdorum/domain/user/application/usecase/SendVerificationEmailUseCase.javasrc/main/java/com/project/dorumdorum/domain/user/application/usecase/SignUpUseCase.javasrc/main/java/com/project/dorumdorum/domain/user/application/usecase/TokenReissueUseCase.javasrc/main/java/com/project/dorumdorum/domain/user/application/usecase/UpdateUserProfileUseCase.javasrc/main/java/com/project/dorumdorum/domain/user/application/usecase/VerifyEmailUseCase.javasrc/main/java/com/project/dorumdorum/domain/user/domain/entity/User.javasrc/main/java/com/project/dorumdorum/domain/user/domain/repository/UserRepository.javasrc/main/java/com/project/dorumdorum/domain/user/domain/service/UserService.javasrc/main/java/com/project/dorumdorum/global/exception/ExceptionAdvice.javasrc/main/java/com/project/dorumdorum/global/exception/code/status/RoomErrorStatus.javasrc/main/java/com/project/dorumdorum/global/exception/code/status/UserErrorStatus.javasrc/main/resources/application-dev.ymlsrc/main/resources/schema.sqlsrc/test/java/com/project/dorumdorum/domain/chat/integration/ChatRoomSchemaInitializationTest.javasrc/test/java/com/project/dorumdorum/domain/chat/unit/service/ChatRoomMemberServiceTest.javasrc/test/java/com/project/dorumdorum/domain/chat/unit/usecase/LeaveChatRoomUseCaseTest.javasrc/test/java/com/project/dorumdorum/domain/chat/unit/usecase/MarkChatRoomReadUseCaseTest.javasrc/test/java/com/project/dorumdorum/domain/checklist/unit/usecase/UpdateRoomRuleUseCaseTest.javasrc/test/java/com/project/dorumdorum/domain/notification/infra/repository/RedisUserPresenceRepositoryTest.javasrc/test/java/com/project/dorumdorum/domain/notification/infra/sse/ChatRoomPresenceListenerTest.javasrc/test/java/com/project/dorumdorum/domain/notification/infra/sse/SseEmitterRegistryTest.javasrc/test/java/com/project/dorumdorum/domain/notification/infra/sse/SseHeartbeatSchedulerTest.javasrc/test/java/com/project/dorumdorum/domain/room/unit/entity/RoomTest.javasrc/test/java/com/project/dorumdorum/domain/room/unit/usecase/ConfirmRoomAssignmentUseCaseTest.javasrc/test/java/com/project/dorumdorum/domain/room/unit/usecase/DecideApplicationRequestUseCaseTest.javasrc/test/java/com/project/dorumdorum/domain/room/unit/usecase/KickRoommateUseCaseTest.javasrc/test/java/com/project/dorumdorum/global/exception/ExceptionAdviceTest.javasrc/test/resources/application-test.yml
💤 Files with no reviewable changes (2)
- src/main/java/com/project/dorumdorum/domain/roommate/domain/service/RoommateService.java
- src/main/java/com/project/dorumdorum/domain/room/domain/service/RoomLikeService.java
Comment on lines
+96
to
+98
| String displayName = (leavingUser.getNickname() != null && !leavingUser.getNickname().isBlank()) | ||
| ? leavingUser.getNickname() : leavingUser.getName(); | ||
| String content = displayName + "가 퇴장했습니다."; |
There was a problem hiding this comment.
Korean grammar: subject particle should vary based on final consonant.
The hardcoded "가 퇴장했습니다" is grammatically incorrect when the display name ends with a consonant (받침). Korean uses "이" after consonants and "가" after vowels.
Proposed fix for correct Korean grammar
String displayName = (leavingUser.getNickname() != null && !leavingUser.getNickname().isBlank())
? leavingUser.getNickname() : leavingUser.getName();
- String content = displayName + "가 퇴장했습니다.";
+ String particle = endsWithConsonant(displayName) ? "이" : "가";
+ String content = displayName + particle + " 퇴장했습니다.";You would need a helper method to check if the last character has a final consonant (받침):
private boolean endsWithConsonant(String text) {
if (text == null || text.isEmpty()) return false;
char lastChar = text.charAt(text.length() - 1);
if (lastChar < '가' || lastChar > '힣') return false;
return (lastChar - '가') % 28 != 0;
}🤖 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/usecase/LeaveChatRoomUseCase.java`
around lines 96 - 98, The message construction currently always appends "가
퇴장했습니다." which is grammatically wrong for names ending with a consonant; update
LeaveChatRoomUseCase to compute displayName (from
leavingUser.getNickname()/getName()) then decide the correct subject particle by
adding a private helper method (e.g., endsWithConsonant(String)) that checks the
last Hangul syllable's jongseong, and use it to choose between "이" and "가" when
setting the content variable so it becomes either displayName + "이 퇴장했습니다." or
displayName + "가 퇴장했습니다.".
Comment on lines
+30
to
39
| public void refreshPresence(String userNo) { | ||
| String key = KEY_PREFIX + userNo; | ||
| String current = redisTemplate.opsForValue().get(key); | ||
| UserPresence presence = UserPresence.fromRedisValue(current); | ||
|
|
||
| UserPresence newPresence = presence.kind() == UserPresence.PresenceKind.IN_CHATROOM | ||
| ? presence | ||
| : UserPresence.online(); | ||
| UserPresence newPresence = presence.kind() == UserPresence.PresenceKind.OFFLINE | ||
| ? UserPresence.online() | ||
| : presence; | ||
|
|
||
| redisTemplate.opsForValue().set(key, newPresence.toRedisValue(), TTL); |
There was a problem hiding this comment.
Make refreshPresence atomic to avoid clobbering chat-room presence.
At Line 32 and Line 39, the read-then-write flow can race with setInChatroom(...), causing IN_CHATROOM to be overwritten by ONLINE.
Suggested fix (avoid read→write overwrite window)
`@Override`
public void refreshPresence(String userNo) {
String key = KEY_PREFIX + userNo;
- String current = redisTemplate.opsForValue().get(key);
- UserPresence presence = UserPresence.fromRedisValue(current);
-
- UserPresence newPresence = presence.kind() == UserPresence.PresenceKind.OFFLINE
- ? UserPresence.online()
- : presence;
-
- redisTemplate.opsForValue().set(key, newPresence.toRedisValue(), TTL);
+ Boolean existed = redisTemplate.expire(key, TTL);
+ if (Boolean.TRUE.equals(existed)) {
+ return; // keep current presence, only refresh TTL
+ }
+ redisTemplate.opsForValue().setIfAbsent(
+ key,
+ UserPresence.online().toRedisValue(),
+ TTL
+ );
}🤖 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/notification/infra/repository/RedisUserPresenceRepository.java`
around lines 30 - 39, refreshPresence currently does a non-atomic
read-then-write (redisTemplate.opsForValue().get(...) then set(...)), which can
race with setInChatroom and overwrite IN_CHATROOM with ONLINE; change
refreshPresence to perform the read-and-conditional-write atomically (e.g., use
redisTemplate.execute/RedisCallback with a small Lua script or Redis
compare-and-set pattern) so the logic "if current value == OFFLINE then set to
ONLINE and apply TTL, else leave unchanged" runs as a single Redis command;
reference the refreshPresence method, redisTemplate.opsForValue().get / set
calls, and the UserPresence.fromRedisValue / UserPresence.online /
toRedisValue/TTL so you update the existing method to use an atomic Redis
operation that preserves IN_CHATROOM.
Comment on lines
+52
to
+63
| ChatRoomSession session = chatRoomSessions.remove(sessionId); | ||
| if (session == null) { | ||
| return; | ||
| } | ||
|
|
||
| if (sseEmitterRegistry.hasConnection(session.userNo())) { | ||
| userPresenceRepository.setOnline(session.userNo()); | ||
| return; | ||
| } | ||
|
|
||
| userPresenceRepository.setOffline(session.userNo()); | ||
| } |
There was a problem hiding this comment.
Disconnect flow ignores remaining chat sessions for the same user.
At Line 57–Line 63, a single session disconnect can set user ONLINE/OFFLINE even when other chat-room sessions are still active, which incorrectly drops IN_CHATROOM.
Suggested fix (preserve chat-room state when another session exists)
ChatRoomSession session = chatRoomSessions.remove(sessionId);
if (session == null) {
return;
}
+ChatRoomSession remaining = chatRoomSessions.values().stream()
+ .filter(s -> s.userNo().equals(session.userNo()))
+ .findFirst()
+ .orElse(null);
+if (remaining != null) {
+ userPresenceRepository.setInChatroom(session.userNo(), remaining.chatRoomNo());
+ return;
+}
+
if (sseEmitterRegistry.hasConnection(session.userNo())) {
userPresenceRepository.setOnline(session.userNo());
return;
}
userPresenceRepository.setOffline(session.userNo());🤖 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/notification/infra/sse/ChatRoomPresenceListener.java`
around lines 52 - 63, When removing the ChatRoomSession for sessionId, current
logic flips the user's presence based only on sseEmitterRegistry.hasConnection
and the single removed session; instead, before calling
userPresenceRepository.setOnline/setOffline, check whether there are any
remaining chat-room sessions for the same user (e.g., scan chatRoomSessions
values for entries with session.userNo() or use a per-user session count) and
only consider the user offline if there are no other chat-room sessions AND no
SSE connections (sseEmitterRegistry.hasConnection(session.userNo()) is false);
update the disconnect flow in ChatRoomPresenceListener to preserve IN_CHATROOM
when other sessions for the same user remain and only call
userPresenceRepository.setOffline when both checks indicate no remaining
activity for that user.
Comment on lines
+99
to
+101
| public boolean isValidCapacity(Integer capacity) { | ||
| return capacity >= currentMateCount; | ||
| } |
There was a problem hiding this comment.
Guard null input in capacity validation to prevent runtime NPE.
Line 100 autounboxes Integer capacity; a null value will throw before domain-level handling.
Proposed fix
public boolean isValidCapacity(Integer capacity) {
- return capacity >= currentMateCount;
+ return capacity != null && capacity >= currentMateCount;
}📝 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
| public boolean isValidCapacity(Integer capacity) { | |
| return capacity >= currentMateCount; | |
| } | |
| public boolean isValidCapacity(Integer capacity) { | |
| return capacity != null && capacity >= currentMateCount; | |
| } |
🤖 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/domain/entity/Room.java`
around lines 99 - 101, The isValidCapacity method in Room currently autounboxes
the Integer parameter and can NPE; update Room.isValidCapacity(Integer capacity)
to first guard against null (e.g., return false or handle per domain rule when
capacity is null) before comparing to currentMateCount, ensuring the comparison
uses safe null-aware logic referencing currentMateCount and capacity to avoid
runtime NPEs.
Comment on lines
+51
to
+58
| private static final String DUPLICATE_KEY_SQL_STATE = "23505"; | ||
| private static final Map<String, BaseCodeInterface> DATA_INTEGRITY_ERROR_CODES = Map.of( | ||
| "uk_room_request_user_room_direction", DUPLICATE_JOIN_REQUEST, | ||
| "uk_roommate_user_no", ALREADY_JOINED_USER, | ||
| "uk_user_email", DUPLICATE_SIGN_UP_INFO, | ||
| "uk_user_student_no", DUPLICATE_SIGN_UP_INFO, | ||
| "uk_chat_room_member_room_user", ALREADY_CHAT_ROOM_MEMBER | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Datasource / dialect =="
fd -HI -i 'application\.ya?ml$' . \
| xargs -r rg -n -C2 'datasource:|url:|driver-class-name:|database-platform:|dialect:'
fd -HI -i 'application\.properties$' . \
| xargs -r rg -n -C2 'spring\.datasource\.url|spring\.datasource\.driver-class-name|spring\.jpa\.database-platform|hibernate\.dialect'
fd -HI -i 'build\.gradle(\.kts)?$' . \
| xargs -r rg -n -C2 'postgresql|mysql|mariadb|h2'
fd -HI -i '^pom\.xml$' . \
| xargs -r rg -n -C2 'postgresql|mysql|mariadb|h2'
echo
echo "== Named unique constraints =="
rg -nP --iglob '*.{java,sql}' '\buk_[a-z0-9_]+\b'
echo
echo "== ExceptionAdvice mapping =="
rg -n -C3 'DATA_INTEGRITY_ERROR_CODES|DUPLICATE_KEY_SQL_STATE' 'src/main/java/com/project/dorumdorum/global/exception/ExceptionAdvice.java'Repository: DorumDorum/BE
Length of output: 6010
Complete the constraint mapping for all user-facing duplicate-key scenarios.
The resolver correctly targets PostgreSQL's SQLState 23505, but DATA_INTEGRITY_ERROR_CODES omits 5 unique constraints defined in the schema:
uk_room_like_user_roomuk_device_user_deviceuk_chat_room_directuk_chat_room_groupuk_room_rule_room_no
Duplicate inserts on any of these constraints will return null and fall through to the generic 500 handler. If these constraints guard user-facing operations, add them to the map with appropriate domain error codes.
🤖 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/exception/ExceptionAdvice.java`
around lines 51 - 58, The DATA_INTEGRITY_ERROR_CODES map in ExceptionAdvice is
missing five unique-constraint keys—add entries for "uk_room_like_user_room",
"uk_device_user_device", "uk_chat_room_direct", "uk_chat_room_group", and
"uk_room_rule_room_no" mapping them to the appropriate domain BaseCodeInterface
constants (e.g., DUPLICATE_ROOM_LIKE, DUPLICATE_DEVICE,
DUPLICATE_CHAT_ROOM_DIRECT, DUPLICATE_CHAT_ROOM_GROUP, DUPLICATE_ROOM_RULE);
update the DATA_INTEGRITY_ERROR_CODES Map.of declaration to include these
mappings and, if those BaseCodeInterface constants do not yet exist, define them
in the domain error codes enum/class so the existing SQLState check using
DUPLICATE_KEY_SQL_STATE returns the correct user-facing error instead of falling
through to the 500 handler.
| jpa: | ||
| hibernate: | ||
| ddl-auto: update | ||
| ddl-auto: validate |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if schema.sql initialization is configured for dev profile
echo "=== Checking application-dev.yml for sql init config ==="
cat src/main/resources/application-dev.yml | grep -A5 "sql:" || echo "No sql init config found in dev profile"
echo ""
echo "=== Checking application.yml for default sql init config ==="
cat src/main/resources/application.yml 2>/dev/null | grep -A5 "sql:" || echo "No sql init config in default profile or file not found"
echo ""
echo "=== Checking for migration tools (Flyway/Liquibase) ==="
fd -e xml -e gradle -e kts | xargs grep -l "flyway\|liquibase" 2>/dev/null || echo "No migration tool config found"Repository: DorumDorum/BE
Length of output: 649
🏁 Script executed:
#!/bin/bash
# Get the full content of application-dev.yml to see complete JPA config
echo "=== Full application-dev.yml content ==="
cat src/main/resources/application-dev.yml
echo ""
echo "=== Checking for schema.sql file ==="
find src/main/resources -name "schema.sql" -o -name "data.sql" 2>/dev/null | head -20
echo ""
echo "=== Checking for db/migration directory (Flyway default) ==="
ls -la src/main/resources/db/migration 2>/dev/null || echo "No db/migration directory found"
echo ""
echo "=== Checking for V*.sql files (Flyway versioned migrations) ==="
find src/main/resources -name "V*.sql" 2>/dev/null | head -10
echo ""
echo "=== Checking pom.xml for spring.sql.init config or Flyway ==="
grep -i "sql.init\|flyway\|liquibase" src/main/resources/application*.yml 2>/dev/null || echo "Not found in yml files"Repository: DorumDorum/BE
Length of output: 1305
🏁 Script executed:
#!/bin/bash
# Check Spring Boot version
echo "=== Spring Boot version from pom.xml ==="
grep -A2 "spring-boot-starter" pom.xml | head -10 || echo "pom.xml check failed"
echo ""
echo "=== Checking schema.sql content (first 20 lines) ==="
head -20 src/main/resources/schema.sql
echo ""
echo "=== Checking schema.sql file size ==="
wc -l src/main/resources/schema.sql
echo ""
echo "=== Checking for application-test.yml ==="
cat src/main/resources/application-test.yml 2>/dev/null || echo "No application-test.yml found"Repository: DorumDorum/BE
Length of output: 982
🏁 Script executed:
#!/bin/bash
# Check for gradle build files
echo "=== Looking for build files ==="
ls -la build.gradle* 2>/dev/null || echo "No gradle build files found in root"
echo ""
echo "=== Full schema.sql content ==="
cat src/main/resources/schema.sql
echo ""
echo "=== Search for CREATE TABLE statements ==="
find src/main/resources -name "*.sql" -exec grep -l "CREATE TABLE" {} \;
echo ""
echo "=== Check if there are any other SQL files ==="
find src/main/resources -name "*.sql" | sortRepository: DorumDorum/BE
Length of output: 1188
This configuration change requires spring.sql.init.mode to be set or schema.sql to be updated with table creation DDL.
Changing ddl-auto from update to validate means Hibernate will no longer create database tables from entity definitions. The schema.sql file only contains indexes and constraints (28 lines with no CREATE TABLE statements), so tables will never be created. Spring Boot's default spring.sql.init.mode is embedded (only for H2, Derby), which means schema.sql will NOT execute against PostgreSQL.
Without additional configuration, the application will fail to start in the dev environment with a schema validation error.
Add one of the following:
- Set
spring.sql.init.mode: alwaysin application-dev.yml to enable schema.sql execution, or - Migrate table definitions from entity DDL to schema.sql and keep full schema creation there, or
- Implement a migration tool (Flyway/Liquibase)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/resources/application-dev.yml` at line 13, The app now uses
Hibernate's ddl-auto: validate, so Spring won't create tables; either enable
execution of schema.sql in the dev profile by adding spring.sql.init.mode:
always to application-dev.yml, or move all CREATE TABLE DDL into schema.sql (it
currently only contains indexes/constraints), or add a migration tool
(Flyway/Liquibase) and provide proper migrations; update the dev configuration
or schema file accordingly so the database schema exists before Hibernate
validation runs.
Comment on lines
+19
to
+28
| ALTER TABLE IF EXISTS chat_room | ||
| DROP CONSTRAINT IF EXISTS uk_chat_room_direct; | ||
|
|
||
| CREATE UNIQUE INDEX IF NOT EXISTS uk_chat_room_group | ||
| ON chat_room (room_no) | ||
| WHERE chat_room_type = 'GROUP'; | ||
|
|
||
| CREATE UNIQUE INDEX IF NOT EXISTS uk_chat_room_direct | ||
| ON chat_room (room_no, applicant_user_no) | ||
| WHERE chat_room_type = 'DIRECT'; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if there are any comments or migration scripts that handle existing duplicate data
rg -n "duplicate|migration|cleanup" --type sqlRepository: DorumDorum/BE
Length of output: 39
🏁 Script executed:
# First, read the schema.sql file to understand its context
head -50 src/main/resources/schema.sql | cat -nRepository: DorumDorum/BE
Length of output: 1135
🏁 Script executed:
# Check if Flyway or other migration tools are configured
find . -type f -name "flyway*.conf" -o -name "liquibase*.conf" -o -name "V*.sql" -o -name "U*.sql" 2>/dev/null | head -20Repository: DorumDorum/BE
Length of output: 39
🏁 Script executed:
# Check for migration-related configuration in pom.xml or application properties
rg -n "flyway|liquibase|migration" --type xml --type properties | head -20Repository: DorumDorum/BE
Length of output: 91
🏁 Script executed:
# Look for other SQL files that might handle migrations or setup
fd -e sql | head -30Repository: DorumDorum/BE
Length of output: 86
🏁 Script executed:
# Check how schema.sql is used - Spring Boot config
rg -n "schema\.sql" --type yaml --type properties --type java | head -20Repository: DorumDorum/BE
Length of output: 91
🏁 Script executed:
# Look for application configuration files
fd -type f \( -name "application*.yml" -o -name "application*.yaml" -o -name "application*.properties" \)Repository: DorumDorum/BE
Length of output: 227
🏁 Script executed:
# Check the full schema.sql to see if chat_room table definition is included
wc -l src/main/resources/schema.sqlRepository: DorumDorum/BE
Length of output: 89
🏁 Script executed:
# Check for any comments or notes about schema initialization
grep -n "chat_room\|migration\|duplicate\|deployment" src/main/resources/schema.sqlRepository: DorumDorum/BE
Length of output: 416
🏁 Script executed:
# Read the entire schema.sql file to understand full context
cat -n src/main/resources/schema.sqlRepository: DorumDorum/BE
Length of output: 1135
🏁 Script executed:
# Look for Spring Boot application configuration
find . -type f \( -name "application.yml" -o -name "application.yaml" \) | head -5Repository: DorumDorum/BE
Length of output: 93
🏁 Script executed:
# Check if there's a pom.xml to understand project setup
head -100 pom.xml 2>/dev/null | cat -nRepository: DorumDorum/BE
Length of output: 39
🏁 Script executed:
# Search for chat_room table definition in the schema
grep -n "CREATE TABLE.*chat_room" src/main/resources/schema.sqlRepository: DorumDorum/BE
Length of output: 39
🏁 Script executed:
# Read the application.yml to understand schema initialization
cat -n src/main/resources/application.ymlRepository: DorumDorum/BE
Length of output: 5877
🏁 Script executed:
# Search for chat_room table creation in all SQL files
grep -r "CREATE TABLE.*chat_room" src/Repository: DorumDorum/BE
Length of output: 39
🏁 Script executed:
# Check if there's another schema file or if chat_room is defined differently
find . -type f -name "*.sql" -o -name "*.java" | xargs grep -l "chat_room" 2>/dev/null | head -10Repository: DorumDorum/BE
Length of output: 694
🏁 Script executed:
# Look for entity definitions or JPA annotations that might create the table
find . -path ./target -prune -o -type f -name "*.java" -print | xargs grep -l "class ChatRoom" 2>/dev/nullRepository: DorumDorum/BE
Length of output: 1349
🏁 Script executed:
# Read the ChatRoom entity definition
cat -n src/main/java/com/project/dorumdorum/domain/chat/domain/entity/ChatRoom.javaRepository: DorumDorum/BE
Length of output: 2011
🏁 Script executed:
# Check the ChatRoomSchemaInitializationTest
cat -n src/test/java/com/project/dorumdorum/domain/chat/integration/ChatRoomSchemaInitializationTest.javaRepository: DorumDorum/BE
Length of output: 2449
🏁 Script executed:
# Search for chat_room table creation in all Java files (might be in entity definition)
grep -r "chat_room\|@Table" src/main/java/com/project/dorumdorum/domain/chat/domain/entity/ChatRoom.javaRepository: DorumDorum/BE
Length of output: 159
🏁 Script executed:
# Check if there's a separate schema file that creates the chat_room table
find src -name "*.sql" -exec grep -l "CREATE TABLE.*chat_room\|create table.*chat_room" {} \;Repository: DorumDorum/BE
Length of output: 39
🏁 Script executed:
# Check for profile-specific application configuration
find src/main/resources -name "application*.yml" -o -name "application*.yaml"Repository: DorumDorum/BE
Length of output: 170
🏁 Script executed:
# Look for Hibernate DDL configuration in all configs
grep -r "ddl-auto\|generate-ddl\|hibernate.ddl" src/main/resources/Repository: DorumDorum/BE
Length of output: 185
🏁 Script executed:
# Check if there's a database migration tool (Flyway) or evolutions directory
find . -type d -name "db" -o -name "migration" -o -name "evolutions" 2>/dev/null | grep -v targetRepository: DorumDorum/BE
Length of output: 39
🏁 Script executed:
# Look for README or documentation about schema setup
find . -maxdepth 3 -name "README*" -o -name "SETUP*" -o -name "DATABASE*" 2>/dev/nullRepository: DorumDorum/BE
Length of output: 39
🏁 Script executed:
# Read dev and prod configurations
cat -n src/main/resources/application-dev.ymlRepository: DorumDorum/BE
Length of output: 1231
🏁 Script executed:
# Read prod configuration
cat -n src/main/resources/application-prod.ymlRepository: DorumDorum/BE
Length of output: 1427
🏁 Script executed:
# Check if there are any data migration scripts or pre-deployment checks
find . -type f \( -name "*.sh" -o -name "*.sql" \) | grep -E "migration|setup|deploy|init" | head -20Repository: DorumDorum/BE
Length of output: 39
🏁 Script executed:
# Look for any documentation in README or wiki
find . -maxdepth 2 -type f -name "README*" | xargs cat 2>/dev/null | head -100Repository: DorumDorum/BE
Length of output: 39
Verify no existing duplicate data before production deployment.
The CREATE UNIQUE INDEX IF NOT EXISTS will succeed if the index already exists, but will fail if the underlying data contains duplicates. Before deploying to production on an existing database with data, ensure there are no violations of these partial constraints (no duplicate room_no for GROUP chats, and no duplicate room_no + applicant_user_no for DIRECT chats). No pre-deployment cleanup scripts were found in the codebase.
🤖 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 19 - 28, The schema change adds
partial unique indexes uk_chat_room_group and uk_chat_room_direct on chat_room
(room_no) and (room_no, applicant_user_no) filtered by chat_room_type, but
existing duplicate rows will cause index creation to fail; add a pre-deployment
migration/script that queries chat_room for duplicates (grouped by room_no where
chat_room_type='GROUP', and grouped by room_no, applicant_user_no where
chat_room_type='DIRECT'), fail the deployment if any duplicates are found, and
provide safe remediation steps (report offending ids, or
deduplicate/merge/remove duplicates) before running the index creation; ensure
this check is run as part of the deployment pipeline and document the
remediation action taken.
Comment on lines
+69
to
+79
| @Test | ||
| @DisplayName("채팅방 구독 세션이 disconnect 되고 SSE 연결도 없으면 OFFLINE으로 전환한다") | ||
| void handleDisconnect_WhenNoSseConnection_SetsOffline() { | ||
| listener.handleSubscribe(new SessionSubscribeEvent(this, createSubscribeMessage("session-1", "user-1", "/topic/chat-room/cr-1"))); | ||
| when(sseEmitterRegistry.hasConnection("user-1")).thenReturn(false); | ||
|
|
||
| listener.handleDisconnect(new SessionDisconnectEvent(this, createDisconnectMessage("session-1"), "session-1", CloseStatus.NORMAL)); | ||
|
|
||
| verify(userPresenceRepository).setOffline("user-1"); | ||
| verify(userPresenceRepository, never()).setOnline("user-1"); | ||
| } |
There was a problem hiding this comment.
Assert hasConnection("user-1") to prevent a false-positive offline test.
In the offline case, the test can still pass if the listener resolves the wrong user/session, because unstubbed boolean calls default to false. Add an explicit verification of the hasConnection argument.
✅ Suggested test-hardening diff
@@
void handleDisconnect_WhenSseConnected_SetsOnline() {
listener.handleSubscribe(new SessionSubscribeEvent(this, createSubscribeMessage("session-1", "user-1", "/topic/chat-room/cr-1")));
when(sseEmitterRegistry.hasConnection("user-1")).thenReturn(true);
listener.handleDisconnect(new SessionDisconnectEvent(this, createDisconnectMessage("session-1"), "session-1", CloseStatus.NORMAL));
+ verify(sseEmitterRegistry).hasConnection("user-1");
verify(userPresenceRepository).setOnline("user-1");
verify(userPresenceRepository, never()).setOffline("user-1");
}
@@
void handleDisconnect_WhenNoSseConnection_SetsOffline() {
listener.handleSubscribe(new SessionSubscribeEvent(this, createSubscribeMessage("session-1", "user-1", "/topic/chat-room/cr-1")));
when(sseEmitterRegistry.hasConnection("user-1")).thenReturn(false);
listener.handleDisconnect(new SessionDisconnectEvent(this, createDisconnectMessage("session-1"), "session-1", CloseStatus.NORMAL));
+ verify(sseEmitterRegistry).hasConnection("user-1");
verify(userPresenceRepository).setOffline("user-1");
verify(userPresenceRepository, never()).setOnline("user-1");
}🤖 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/notification/infra/sse/ChatRoomPresenceListenerTest.java`
around lines 69 - 79, The test may pass spuriously because unstubbed booleans
default to false; ensure the listener actually checked the SSE registry for the
specific user by verifying sseEmitterRegistry.hasConnection("user-1") was
invoked. Update the test method handleDisconnect_WhenNoSseConnection_SetsOffline
to include a verify(sseEmitterRegistry).hasConnection("user-1") (or equivalent
assertion) after invoking listener.handleDisconnect and before asserting
userPresenceRepository.setOffline/setOnline, so you confirm the correct argument
was checked.
Comment on lines
+110
to
+127
| @Test | ||
| @DisplayName("user unique 제약 위반은 DUPLICATE_SIGN_UP_INFO로 변환한다") | ||
| void handleDataIntegrityViolationException_WhenDuplicateUserSignUp_ReturnsBadRequest() { | ||
| SQLException sqlException = new SQLException("duplicate key", "23505"); | ||
| ConstraintViolationException cause = new ConstraintViolationException( | ||
| "duplicate", | ||
| sqlException, | ||
| "uk_user_email" | ||
| ); | ||
| DataIntegrityViolationException exception = new DataIntegrityViolationException("duplicate", cause); | ||
|
|
||
| ResponseEntity<ErrorResponse> response = exceptionAdvice.handleDataIntegrityViolationException(exception); | ||
|
|
||
| assertThat(response.getStatusCode().value()).isEqualTo(400); | ||
| assertThat(response.getBody()).isNotNull(); | ||
| assertThat(response.getBody().getCode()).isEqualTo("USER001"); | ||
| verify(systemAlertPublisher, never()).publish(any(), any(), any(), any()); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how ExceptionAdvice handles the student_no constraint
ast-grep --pattern 'uk_user_student_no'
rg -n "uk_user_student_no|student_no" --type java -C3Repository: DorumDorum/BE
Length of output: 1716
Add test case for uk_user_student_no constraint to ensure both email and student number duplicates map to the same error code.
The ExceptionAdvice already handles uk_user_student_no and maps it to DUPLICATE_SIGN_UP_INFO (same as uk_user_email). However, this test only verifies the email constraint. For complete coverage, add a second test case (or extend this one) to confirm uk_user_student_no also returns status code 400 with error code USER001.
🤖 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/exception/ExceptionAdviceTest.java`
around lines 110 - 127, Add a second unit test (or extend the existing test
handleDataIntegrityViolationException_WhenDuplicateUserSignUp_ReturnsBadRequest)
that constructs a ConstraintViolationException with the same SQL state but with
the constraint name "uk_user_student_no" and wraps it in a
DataIntegrityViolationException, then call
exceptionAdvice.handleDataIntegrityViolationException(exception) and assert the
response status is 400 and response.getBody().getCode() equals "USER001" (and
verify systemAlertPublisher never publishes); this ensures the code path
handling "uk_user_student_no" (alongside "uk_user_email") maps to
DUPLICATE_SIGN_UP_INFO.
…oderabbitai-설정 [FEAT] Discord 시스템 알림 연동 및 CodeRabbit AI 설정
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/com/project/dorumdorum/domain/chat/application/event/RoommateKickedEventListener.java (1)
39-45:⚠️ Potential issue | 🟠 MajorUse one locked lookup for the kicked member.
Line 42 checks membership and Line 44 loads the row in two separate queries. If another transaction removes that
ChatRoomMemberbetween them, this listener throws and rolls backKickRoommateUseCase. A single locked fetch here—findByChatRoomNoAndUserNoForUpdate(...)looks like the right primitive—would keep this path idempotent under that race.🤖 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 39 - 45, The listener in RoommateKickedEventListener::handle currently does a membership check via chatRoomMemberService.isMember(...) and then separately loads the member via chatRoomMemberService.findByChatRoomAndUserNo(...), allowing a race where the member row can be removed between calls; change this to a single locked fetch (use chatRoomMemberService.findByChatRoomNoAndUserNoForUpdate(...) or equivalent) to load the ChatRoomMember under a SELECT ... FOR UPDATE and, if present, pass that locked entity to chatRoomMemberService.leave(...) so the operation is atomic and race-safe.
🧹 Nitpick comments (5)
src/main/java/com/project/dorumdorum/global/alert/DiscordAlertSender.java (1)
53-56: Discord response is discarded without checking status.The HTTP response is discarded without checking if the webhook call succeeded. While the exception handling catches network errors, a 4xx/5xx response from Discord would be silently ignored.
Consider logging non-2xx responses:
Proposed enhancement
- httpClient.send(request, HttpResponse.BodyHandlers.discarding()); + HttpResponse<Void> response = httpClient.send(request, HttpResponse.BodyHandlers.discarding()); + if (response.statusCode() >= 400) { + log.warn("[Alert] Discord 응답 오류. status={}, title={}", response.statusCode(), alert.title()); + } } catch (Exception e) {🤖 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/alert/DiscordAlertSender.java` around lines 53 - 56, The code in DiscordAlertSender currently discards the HttpResponse from httpClient.send(request, HttpResponse.BodyHandlers.discarding()), so non-2xx Discord responses are ignored; change the send call to capture the response (use HttpResponse<String> with BodyHandlers.ofString()), check response.statusCode() after sending in the same try block, and if the status is not in the 200-299 range log a warning including alert.title(), the status code and response body to surface 4xx/5xx failures (retain the existing catch for network exceptions and include the exception in that log).src/test/java/com/project/dorumdorum/global/alert/DiscordAlertPropertiesTest.java (1)
39-57: Consider adding a test for blank (non-null) URL fallback behavior.The fallback tests cover
nullURLs well, butresolveWebhookUrlalso handles blank strings (" "). Consider adding a test to ensure blank strings also trigger fallback:`@Test` `@DisplayName`("errorWebhookUrl이 빈 문자열이면 webhookUrl로 fallback") void resolveWebhookUrl_BlankErrorUrl_FallbackToDefault() { DiscordAlertProperties props = new DiscordAlertProperties(); props.setWebhookUrl("http://default"); props.setErrorWebhookUrl(" "); assertThat(props.resolveWebhookUrl(AlertSeverity.CRITICAL)).isEqualTo("http://default"); },
🤖 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/alert/DiscordAlertPropertiesTest.java` around lines 39 - 57, Add a unit test to cover blank (non-null) webhook URL fallback: in DiscordAlertPropertiesTest create a new test (e.g., resolveWebhookUrl_BlankErrorUrl_FallbackToDefault) that instantiates DiscordAlertProperties, sets props.setWebhookUrl("http://default") and props.setErrorWebhookUrl(" "), then asserts props.resolveWebhookUrl(AlertSeverity.CRITICAL) equals "http://default"; this verifies DiscordAlertProperties.resolveWebhookUrl treats blank strings as unset and falls back to webhookUrl.src/main/java/com/project/dorumdorum/global/alert/AlertDeduplicationService.java (1)
26-31: Consider handling Redis failures gracefully.If Redis is unavailable,
setIfAbsentwill throw an exception that propagates to callers. Depending on your requirements, you may want to:
- Catch exceptions and default to "not duplicate" (allow sending)
- Or let it fail fast (current behavior)
The current fail-fast approach is reasonable if Redis availability is critical, but worth documenting the expected behavior.
🤖 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/alert/AlertDeduplicationService.java` around lines 26 - 31, The isDuplicate method currently lets Redis errors from redisTemplate.opsForValue().setIfAbsent propagate; wrap the call in a try/catch inside isDuplicate (referencing isDuplicate, buildKey, resolveTtl and redisTemplate.opsForValue().setIfAbsent) and on Redis failures either log the exception and return false (treat as "not duplicate" so alerts can proceed) or, if you prefer configurable fail-fast behavior, add a boolean config flag to choose between returning false on error versus rethrowing the exception; ensure the log includes the key and exception details for debugging.src/main/java/com/project/dorumdorum/global/alert/DiscordAlertEventListener.java (1)
17-27: Good defensive error handling for async event listener.The try/catch wrapper prevents exceptions from propagating out of the async handler, which is the correct pattern for
@TransactionalEventListenerwith@Async. Logging the alert title aids debugging.One minor consideration: the
@Asyncannotation doesn't specify an executor, so it will use Spring's default executor rather than thenotificationExecutorbean defined inAsyncConfig. If you want consistent thread pool behavior with other notification tasks, consider qualifying it:`@Async`("notificationExecutor"),
🤖 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/alert/DiscordAlertEventListener.java` around lines 17 - 27, The `@Async` on DiscordAlertEventListener.handle uses the default executor; update the annotation to use the notificationExecutor bean so async handling shares the intended thread pool: replace the plain `@Async` with a qualified one (e.g., `@Async`("notificationExecutor")) on the handle(SystemAlertEvent event) method to ensure it uses the AsyncConfig's notificationExecutor.src/test/java/com/project/dorumdorum/global/alert/AlertDeduplicationServiceTest.java (1)
65-89: Good TTL verification tests, consider adding WARN and INFO coverage.The TTL tests for CRITICAL (60s) and ERROR (300s) are well-implemented using
ArgumentCaptor. For completeness, consider adding similar tests for WARN (900s) and INFO (1800s) severities to ensure all TTL mappings are verified.,
🤖 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/alert/AlertDeduplicationServiceTest.java` around lines 65 - 89, Add two tests similar to the existing TTL tests to cover WARN and INFO severities: in AlertDeduplicationServiceTest create methods (e.g., isDuplicate_WarnSeverity_Uses900SecondTtl and isDuplicate_InfoSeverity_Uses1800SecondTtl) that mock redisTemplate.opsForValue() to return valueOps and stub valueOps.setIfAbsent(..., any(Duration.class)) to return true, call service.isDuplicate(alert(AlertSeverity.WARN, "warn")) and service.isDuplicate(alert(AlertSeverity.INFO, "info")), capture the Duration argument with ArgumentCaptor<Duration>, verify valueOps.setIfAbsent(anyString(), anyString(), captor.capture()), and assert that captor.getValue() equals Duration.ofSeconds(900) for WARN and Duration.ofSeconds(1800) for INFO.
🤖 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/RoommateKickedEventListener.java`:
- Around line 39-40: The WebSocket broadcast is currently sent inside
RoommateKickedEventListener.handle() as a BEFORE_COMMIT listener (invoking
broadcastSafely(...)), which can send frames before the transaction actually
commits; change the listener to perform only transactional DB work and move the
broadcast logic into an after-commit callback by calling
TransactionSynchronizationManager.registerSynchronization(...) and registering a
TransactionSynchronization that calls broadcastSafely(...) in afterCommit;
update both the handle method (and the similar block at lines 54-64) to register
the synchronization instead of sending the frame directly so broadcasts occur
only after successful commit.
In
`@src/main/java/com/project/dorumdorum/domain/chat/application/usecase/CreateChatRoomUseCase.java`:
- Around line 52-58: The catch in joinSafely currently swallows any
DataIntegrityViolationException; change it to verify the member exists before
suppressing: inside the catch for DataIntegrityViolationException (in
CreateChatRoomUseCase.joinSafely) call the membership check on
chatRoomMemberService (e.g., chatRoomMemberService.isMember or equivalent using
chatRoom.getChatRoomNo() and userNo) and only suppress/log the exception if that
check returns true (duplicate insert). If the check shows the member is still
absent, rethrow the exception (or wrap and throw a runtime/DataAccessException)
so unrelated constraint violations are not masked; keep the existing debug log
for the duplicate case.
In
`@src/main/java/com/project/dorumdorum/domain/chat/application/usecase/JoinChatRoomUseCase.java`:
- Around line 47-63: The catch blocks in JoinChatRoomUseCase that swallow
DataIntegrityViolationException need to re-check membership before suppressing
the error: after catching the exception from chatRoomMemberService.join (both
the host join using created/event.hostUserNo() and the accepted user join using
chatRoom/event.acceptedUserNo()), call chatRoomMemberService.isMember(...) to
verify the user actually exists in the room; if isMember returns true, keep the
debug log and continue, otherwise re-throw the caught
DataIntegrityViolationException so non-duplicate constraint failures surface
instead of silently failing.
- Line 42: The WebSocket broadcast (convertAndSend()) is currently triggered in
BEFORE_COMMIT (e.g. in JoinChatRoomUseCase and RoommateKickedEventListener) and
can be rolled back; change the notification to run only after the transaction
commits by registering a synchronization via
TransactionSynchronizationManager.registerSynchronization(...). Specifically,
keep your DB write logic in the existing BEFORE_COMMIT handler but replace the
direct convertAndSend() call with a TransactionSynchronization that implements
afterCommit() (or overrides afterCommit() on a TransactionSynchronization
implementation) and performs convertAndSend() there so broadcasts only run after
successful commit.
In
`@src/main/java/com/project/dorumdorum/global/alert/AlertDeduplicationService.java`:
- Around line 33-35: The current buildKey in AlertDeduplicationService uses
Math.abs(alert.title().hashCode()) which both can yield Integer.MIN_VALUE (stays
negative) and suffers collisions; replace this with a stable, non-negative
digest of the title (e.g., compute a SHA-256 or other strong hash of
alert.title() and use a truncated hex string) or at minimum convert the int hash
to an unsigned representation (Integer.toUnsignedLong(hash) or
Integer.toUnsignedString(hash)) before building the key; update buildKey to use
KEY_PREFIX + alert.severity() + ":" + <stable-nonnegative-title-digest> so keys
are consistently formatted and collisions are reduced.
In
`@src/main/java/com/project/dorumdorum/global/alert/ApplicationLifecycleAlertListener.java`:
- Around line 25-33: The shutdown alert in
ApplicationLifecycleAlertListener.onContextClosed may not be delivered because
dependent beans (AlertDeduplicationService/Redis, DiscordAlertSender/HTTP
client, async DiscordAlertEventListener) can be stopped before this event;
update the listener to ensure delivery by either making the shutdown flow run
before other beans stop or bypassing async/deduplication: implement
ApplicationLifecycleAlertListener as a SmartLifecycle with a high phase (start
false, isAutoStartup false, stop triggers publishing synchronously) so on stop
it publishes before lower-phase beans are stopped, or alternatively call
SystemAlertPublisher.publishShutdownSynchronously (create a sync publish method
that skips async event dispatch and AlertDeduplicationService) from
onContextClosed to send the alert directly to DiscordAlertSender; reference
ApplicationLifecycleAlertListener.onContextClosed, SystemAlertPublisher (add
publishShutdownSynchronously), DiscordAlertEventListener,
AlertDeduplicationService, and DiscordAlertSender when making the change.
In
`@src/main/java/com/project/dorumdorum/global/logging/DiscordLogbackAppender.java`:
- Around line 66-71: The dedup key is being reserved before calling the webhook
so failures leave the key in cache and suppress retries; modify the send flow in
DiscordLogbackAppender so that when you reserve/set the dedup key (the logic
around isDuplicate(...) and buildDedupKey(...)) you remove/unreserve that key if
the subsequent sendToDiscord/httpClient.send(...) throws or returns a
failure—wrap the webhook call in a try/catch (or try/finally) and on any
exception or non-success response explicitly delete the dedup cache entry before
rethrowing or logging; apply the same fix to the other duplicate-handling blocks
(the code around lines handling sendToDiscord/httpClient.send in the 82-95 and
115-127 ranges) to ensure failed sends do not remain locked out.
- Around line 98-100: The dedup key in buildDedupKey currently uses
shortLogger(event.getLoggerName()), which collapses distinct classes with the
same simple name; change it to use the full logger name so different packages
produce distinct keys (i.e., replace shortLogger(event.getLoggerName()) with
event.getLoggerName() in the return value of buildDedupKey), keeping the rest of
the key composition (event.getLevel().levelStr and the message hash from
event.getFormattedMessage()) unchanged.
---
Outside diff comments:
In
`@src/main/java/com/project/dorumdorum/domain/chat/application/event/RoommateKickedEventListener.java`:
- Around line 39-45: The listener in RoommateKickedEventListener::handle
currently does a membership check via chatRoomMemberService.isMember(...) and
then separately loads the member via
chatRoomMemberService.findByChatRoomAndUserNo(...), allowing a race where the
member row can be removed between calls; change this to a single locked fetch
(use chatRoomMemberService.findByChatRoomNoAndUserNoForUpdate(...) or
equivalent) to load the ChatRoomMember under a SELECT ... FOR UPDATE and, if
present, pass that locked entity to chatRoomMemberService.leave(...) so the
operation is atomic and race-safe.
---
Nitpick comments:
In
`@src/main/java/com/project/dorumdorum/global/alert/AlertDeduplicationService.java`:
- Around line 26-31: The isDuplicate method currently lets Redis errors from
redisTemplate.opsForValue().setIfAbsent propagate; wrap the call in a try/catch
inside isDuplicate (referencing isDuplicate, buildKey, resolveTtl and
redisTemplate.opsForValue().setIfAbsent) and on Redis failures either log the
exception and return false (treat as "not duplicate" so alerts can proceed) or,
if you prefer configurable fail-fast behavior, add a boolean config flag to
choose between returning false on error versus rethrowing the exception; ensure
the log includes the key and exception details for debugging.
In
`@src/main/java/com/project/dorumdorum/global/alert/DiscordAlertEventListener.java`:
- Around line 17-27: The `@Async` on DiscordAlertEventListener.handle uses the
default executor; update the annotation to use the notificationExecutor bean so
async handling shares the intended thread pool: replace the plain `@Async` with a
qualified one (e.g., `@Async`("notificationExecutor")) on the
handle(SystemAlertEvent event) method to ensure it uses the AsyncConfig's
notificationExecutor.
In `@src/main/java/com/project/dorumdorum/global/alert/DiscordAlertSender.java`:
- Around line 53-56: The code in DiscordAlertSender currently discards the
HttpResponse from httpClient.send(request,
HttpResponse.BodyHandlers.discarding()), so non-2xx Discord responses are
ignored; change the send call to capture the response (use HttpResponse<String>
with BodyHandlers.ofString()), check response.statusCode() after sending in the
same try block, and if the status is not in the 200-299 range log a warning
including alert.title(), the status code and response body to surface 4xx/5xx
failures (retain the existing catch for network exceptions and include the
exception in that log).
In
`@src/test/java/com/project/dorumdorum/global/alert/AlertDeduplicationServiceTest.java`:
- Around line 65-89: Add two tests similar to the existing TTL tests to cover
WARN and INFO severities: in AlertDeduplicationServiceTest create methods (e.g.,
isDuplicate_WarnSeverity_Uses900SecondTtl and
isDuplicate_InfoSeverity_Uses1800SecondTtl) that mock
redisTemplate.opsForValue() to return valueOps and stub
valueOps.setIfAbsent(..., any(Duration.class)) to return true, call
service.isDuplicate(alert(AlertSeverity.WARN, "warn")) and
service.isDuplicate(alert(AlertSeverity.INFO, "info")), capture the Duration
argument with ArgumentCaptor<Duration>, verify valueOps.setIfAbsent(anyString(),
anyString(), captor.capture()), and assert that captor.getValue() equals
Duration.ofSeconds(900) for WARN and Duration.ofSeconds(1800) for INFO.
In
`@src/test/java/com/project/dorumdorum/global/alert/DiscordAlertPropertiesTest.java`:
- Around line 39-57: Add a unit test to cover blank (non-null) webhook URL
fallback: in DiscordAlertPropertiesTest create a new test (e.g.,
resolveWebhookUrl_BlankErrorUrl_FallbackToDefault) that instantiates
DiscordAlertProperties, sets props.setWebhookUrl("http://default") and
props.setErrorWebhookUrl(" "), then asserts
props.resolveWebhookUrl(AlertSeverity.CRITICAL) equals "http://default"; this
verifies DiscordAlertProperties.resolveWebhookUrl treats blank strings as unset
and falls back to webhookUrl.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 72db14e4-c227-435b-a723-4416cee71214
📒 Files selected for processing (20)
.coderabbit.ymlsrc/main/java/com/project/dorumdorum/domain/chat/application/event/RoommateKickedEventListener.javasrc/main/java/com/project/dorumdorum/domain/chat/application/usecase/CreateChatRoomUseCase.javasrc/main/java/com/project/dorumdorum/domain/chat/application/usecase/JoinChatRoomUseCase.javasrc/main/java/com/project/dorumdorum/global/alert/AlertDeduplicationService.javasrc/main/java/com/project/dorumdorum/global/alert/ApplicationLifecycleAlertListener.javasrc/main/java/com/project/dorumdorum/global/alert/DiscordAlertEventListener.javasrc/main/java/com/project/dorumdorum/global/alert/DiscordAlertProperties.javasrc/main/java/com/project/dorumdorum/global/alert/DiscordAlertSender.javasrc/main/java/com/project/dorumdorum/global/logging/DiscordLogbackAppender.javasrc/main/java/com/project/dorumdorum/global/warmer/WarmupScheduler.javasrc/main/resources/application.ymlsrc/main/resources/logback-spring.xmlsrc/test/java/com/project/dorumdorum/domain/chat/integration/ChatTransactionAtomicityIntegrationTest.javasrc/test/java/com/project/dorumdorum/domain/chat/unit/usecase/CreateChatRoomUseCaseTest.javasrc/test/java/com/project/dorumdorum/domain/chat/unit/usecase/JoinChatRoomUseCaseTest.javasrc/test/java/com/project/dorumdorum/domain/chat/unit/usecase/RoommateKickedEventListenerTest.javasrc/test/java/com/project/dorumdorum/global/alert/AlertDeduplicationServiceTest.javasrc/test/java/com/project/dorumdorum/global/alert/DiscordAlertPropertiesTest.javasrc/test/java/com/project/dorumdorum/global/logging/DiscordLogbackAppenderTest.java
✅ Files skipped from review due to trivial changes (2)
- src/main/resources/application.yml
- .coderabbit.yml
Comment on lines
+39
to
40
| @TransactionalEventListener(phase = TransactionPhase.BEFORE_COMMIT) | ||
| public void handle(RoommateKickedEvent event) { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/main/java/com/project/dorumdorum/domain/chat/application/event/RoommateKickedEventListener.javaRepository: DorumDorum/BE
Length of output: 3864
🏁 Script executed:
rg -n "TransactionalEventListener|broadcastSafely|convertAndSend" --type java src/main/java/com/project/dorumdorum/domain/chat/Repository: DorumDorum/BE
Length of output: 2896
🏁 Script executed:
rg -B5 -A5 "TransactionSynchronizationManager|afterCommit" --type java src/main/java/com/project/dorumdorum/ | head -50Repository: DorumDorum/BE
Length of output: 39
🏁 Script executed:
cat -n src/main/java/com/project/dorumdorum/domain/chat/application/usecase/CreateDirectChatRoomUseCase.javaRepository: DorumDorum/BE
Length of output: 1612
🏁 Script executed:
rg -n "TransactionSynchronizationManager" --type java src/Repository: DorumDorum/BE
Length of output: 39
🏁 Script executed:
cat -n src/main/java/com/project/dorumdorum/domain/chat/application/usecase/JoinChatRoomUseCase.java | head -85Repository: DorumDorum/BE
Length of output: 4724
Move the WebSocket send to afterCommit.
broadcastSafely(...) is invoked from a BEFORE_COMMIT listener, so a later flush/commit failure can roll back the DB changes after clients already received the frame. Keep the DB work in this transaction, but register the broadcast for afterCommit using TransactionSynchronizationManager.registerSynchronization().
Suggested fix
+import org.springframework.transaction.support.TransactionSynchronization;
+import org.springframework.transaction.support.TransactionSynchronizationManager;
+
...
- broadcastSafely(chatRoom, response);
+ TransactionSynchronizationManager.registerSynchronization(new TransactionSynchronization() {
+ `@Override`
+ public void afterCommit() {
+ broadcastSafely(chatRoom, response);
+ }
+ });
...Also applies to: 54-64
🤖 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 39 - 40, The WebSocket broadcast is currently sent inside
RoommateKickedEventListener.handle() as a BEFORE_COMMIT listener (invoking
broadcastSafely(...)), which can send frames before the transaction actually
commits; change the listener to perform only transactional DB work and move the
broadcast logic into an after-commit callback by calling
TransactionSynchronizationManager.registerSynchronization(...) and registering a
TransactionSynchronization that calls broadcastSafely(...) in afterCommit;
update both the handle method (and the similar block at lines 54-64) to register
the synchronization instead of sending the frame directly so broadcasts occur
only after successful commit.
Comment on lines
+52
to
+58
| private void joinSafely(ChatRoom chatRoom, String userNo) { | ||
| try { | ||
| chatRoomMemberService.join(chatRoom, userNo); | ||
| } catch (DataIntegrityViolationException e) { | ||
| log.debug("[Chat] 채팅방 중복 입장 (동시 요청). chatRoomNo={}, userNo={}", | ||
| chatRoom.getChatRoomNo(), userNo); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find the file mentioned in the review
find . -name "CreateChatRoomUseCase.java" -type fRepository: DorumDorum/BE
Length of output: 154
🏁 Script executed:
# Also explore the general structure
find . -name "*.java" -path "*/domain/chat/*" -type f | head -20Repository: DorumDorum/BE
Length of output: 2001
🏁 Script executed:
# Search for ChatRoomMemberService to understand available methods
find . -name "*ChatRoomMemberService*" -type fRepository: DorumDorum/BE
Length of output: 244
🏁 Script executed:
# Read the CreateChatRoomUseCase.java file to verify the code
wc -l ./src/main/java/com/project/dorumdorum/domain/chat/application/usecase/CreateChatRoomUseCase.javaRepository: DorumDorum/BE
Length of output: 157
🏁 Script executed:
# Read lines around 52-58 to see the code in question
sed -n '45,65p' ./src/main/java/com/project/dorumdorum/domain/chat/application/usecase/CreateChatRoomUseCase.javaRepository: DorumDorum/BE
Length of output: 609
🏁 Script executed:
# Read the ChatRoomMemberService to see available methods
cat ./src/main/java/com/project/dorumdorum/domain/chat/domain/service/ChatRoomMemberService.javaRepository: DorumDorum/BE
Length of output: 2880
🏁 Script executed:
# Check for other usages of joinSafely to understand context
rg "joinSafely" -A 2 -B 2Repository: DorumDorum/BE
Length of output: 1362
🏁 Script executed:
# Check for other direct calls to chatRoomMemberService.join() that might have similar issues
rg "chatRoomMemberService\.join" -A 2 -B 2Repository: DorumDorum/BE
Length of output: 4402
Only ignore verified duplicate-member inserts.
The joinSafely() method catches DataIntegrityViolationException without verifying that the member was actually created. A race condition exists: between the pre-check !isMember() and the actual join() call, another thread can insert the member, causing this thread's insert to fail with a constraint violation. The exception is then silently suppressed, but the member creation never happens. Additionally, DataIntegrityViolationException can be thrown for other constraint violations beyond duplicate members, so this pattern masks unrelated failures.
Add a membership verification in the catch block to ensure the join actually succeeded before suppressing the exception:
Fix
private void joinSafely(ChatRoom chatRoom, String userNo) {
try {
chatRoomMemberService.join(chatRoom, userNo);
} catch (DataIntegrityViolationException e) {
+ if (!chatRoomMemberService.isMember(chatRoom, userNo)) {
+ throw e;
+ }
log.debug("[Chat] 채팅방 중복 입장 (동시 요청). chatRoomNo={}, userNo={}",
chatRoom.getChatRoomNo(), userNo);
}
}🤖 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/usecase/CreateChatRoomUseCase.java`
around lines 52 - 58, The catch in joinSafely currently swallows any
DataIntegrityViolationException; change it to verify the member exists before
suppressing: inside the catch for DataIntegrityViolationException (in
CreateChatRoomUseCase.joinSafely) call the membership check on
chatRoomMemberService (e.g., chatRoomMemberService.isMember or equivalent using
chatRoom.getChatRoomNo() and userNo) and only suppress/log the exception if that
check returns true (duplicate insert). If the check shows the member is still
absent, rethrow the exception (or wrap and throw a runtime/DataAccessException)
so unrelated constraint violations are not masked; keep the existing debug log
for the duplicate case.
| */ | ||
| @TransactionalEventListener(phase = TransactionPhase.AFTER_COMMIT) | ||
| @Transactional(propagation = Propagation.REQUIRES_NEW) | ||
| @TransactionalEventListener(phase = TransactionPhase.BEFORE_COMMIT) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "JoinChatRoomUseCase.java" 2>/dev/nullRepository: DorumDorum/BE
Length of output: 152
🏁 Script executed:
cat -n ./src/main/java/com/project/dorumdorum/domain/chat/application/usecase/JoinChatRoomUseCase.javaRepository: DorumDorum/BE
Length of output: 4724
🏁 Script executed:
rg "convertAndSend" --type java -B 3 -A 1Repository: DorumDorum/BE
Length of output: 8674
🏁 Script executed:
cat -n ./src/main/java/com/project/dorumdorum/domain/chat/application/event/RoommateKickedEventListener.javaRepository: DorumDorum/BE
Length of output: 3864
🏁 Script executed:
cat -n ./src/main/java/com/project/dorumdorum/domain/chat/application/usecase/SendGroupChatMessageUseCase.java | head -60Repository: DorumDorum/BE
Length of output: 3402
🏁 Script executed:
cat -n ./src/main/java/com/project/dorumdorum/domain/chat/application/usecase/SendGroupChatMessageUseCase.java | tail -20Repository: DorumDorum/BE
Length of output: 912
Defer the WebSocket send to afterCommit phase.
Both JoinChatRoomUseCase and RoommateKickedEventListener run in BEFORE_COMMIT, allowing convertAndSend() to publish notifications that could roll back with the transaction. Database writes belong here, but the WebSocket broadcast must execute only after the commit succeeds. Otherwise, subscribers are notified of changes that don't exist in the database.
Use TransactionSynchronizationManager.registerSynchronization() with an afterCommit() callback to defer the broadcast:
Possible fix
+import org.springframework.transaction.support.TransactionSynchronization;
+import org.springframework.transaction.support.TransactionSynchronizationManager;
+
...
- broadcastSafely(chatRoom, response);
+ TransactionSynchronizationManager.registerSynchronization(new TransactionSynchronization() {
+ `@Override`
+ public void afterCommit() {
+ broadcastSafely(chatRoom, response);
+ }
+ });
...🤖 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/usecase/JoinChatRoomUseCase.java`
at line 42, The WebSocket broadcast (convertAndSend()) is currently triggered in
BEFORE_COMMIT (e.g. in JoinChatRoomUseCase and RoommateKickedEventListener) and
can be rolled back; change the notification to run only after the transaction
commits by registering a synchronization via
TransactionSynchronizationManager.registerSynchronization(...). Specifically,
keep your DB write logic in the existing BEFORE_COMMIT handler but replace the
direct convertAndSend() call with a TransactionSynchronization that implements
afterCommit() (or overrides afterCommit() on a TransactionSynchronization
implementation) and performs convertAndSend() there so broadcasts only run after
successful commit.
Comment on lines
+47
to
+63
| try { | ||
| chatRoomMemberService.join(created, event.hostUserNo()); | ||
| } catch (DataIntegrityViolationException e) { | ||
| log.debug("[Chat] 방장 중복 입장 (동시 요청). roomNo={}, hostUserNo={}", | ||
| event.roomNo(), event.hostUserNo()); | ||
| } | ||
| return created; | ||
| }); | ||
|
|
||
| if (!chatRoomMemberService.isMember(chatRoom, event.acceptedUserNo())) { | ||
| chatRoomMemberService.join(chatRoom, event.acceptedUserNo()); | ||
| try { | ||
| chatRoomMemberService.join(chatRoom, event.acceptedUserNo()); | ||
| } catch (DataIntegrityViolationException e) { | ||
| log.debug("[Chat] 신규 멤버 중복 입장 (동시 요청). roomNo={}, userNo={}", | ||
| event.roomNo(), event.acceptedUserNo()); | ||
| return; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -type f -name "JoinChatRoomUseCase.java"Repository: DorumDorum/BE
Length of output: 227
🏁 Script executed:
cat -n src/main/java/com/project/dorumdorum/domain/chat/application/usecase/JoinChatRoomUseCase.javaRepository: DorumDorum/BE
Length of output: 4724
🏁 Script executed:
fd -type f -name "*ChatRoomMember*" | head -20Repository: DorumDorum/BE
Length of output: 227
🏁 Script executed:
rg "DataIntegrityViolationException" -A 5 -B 5 src/Repository: DorumDorum/BE
Length of output: 43750
Re-check membership after exception to confirm duplicate, not other constraint failure.
Lines 49 and 59 catch DataIntegrityViolationException without verifying the failure reason. This exception can be raised by any constraint violation (foreign keys, other unique constraints, etc.), not just duplicate-member inserts. If a non-duplicate constraint fails, the code silently continues while the user is not actually joined, causing data inconsistency. Add a membership check after the catch block to confirm the user was already a member before suppressing the exception; if still missing, re-throw it.
Suggested fix
try {
chatRoomMemberService.join(created, event.hostUserNo());
} catch (DataIntegrityViolationException e) {
+ if (!chatRoomMemberService.isMember(created, event.hostUserNo())) {
+ throw e;
+ }
log.debug("[Chat] 방장 중복 입장 (동시 요청). roomNo={}, hostUserNo={}",
event.roomNo(), event.hostUserNo());
}
...
try {
chatRoomMemberService.join(chatRoom, event.acceptedUserNo());
} catch (DataIntegrityViolationException e) {
+ if (!chatRoomMemberService.isMember(chatRoom, event.acceptedUserNo())) {
+ throw e;
+ }
log.debug("[Chat] 신규 멤버 중복 입장 (동시 요청). roomNo={}, userNo={}",
event.roomNo(), event.acceptedUserNo());
return;
}🤖 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/usecase/JoinChatRoomUseCase.java`
around lines 47 - 63, The catch blocks in JoinChatRoomUseCase that swallow
DataIntegrityViolationException need to re-check membership before suppressing
the error: after catching the exception from chatRoomMemberService.join (both
the host join using created/event.hostUserNo() and the accepted user join using
chatRoom/event.acceptedUserNo()), call chatRoomMemberService.isMember(...) to
verify the user actually exists in the room; if isMember returns true, keep the
debug log and continue, otherwise re-throw the caught
DataIntegrityViolationException so non-duplicate constraint failures surface
instead of silently failing.
Comment on lines
+33
to
+35
| private String buildKey(SystemAlert alert) { | ||
| return KEY_PREFIX + alert.severity() + ":" + Math.abs(alert.title().hashCode()); | ||
| } |
There was a problem hiding this comment.
Math.abs(hashCode()) has edge cases that could cause issues.
Two concerns with the current key building:
- Hash collisions: Different alert titles with the same hash will be incorrectly deduplicated.
- Integer.MIN_VALUE:
Math.abs(Integer.MIN_VALUE)returnsInteger.MIN_VALUE(still negative due to overflow), creating inconsistent key formats.
Consider using a more robust approach:
Proposed fix
private String buildKey(SystemAlert alert) {
- return KEY_PREFIX + alert.severity() + ":" + Math.abs(alert.title().hashCode());
+ return KEY_PREFIX + alert.severity() + ":" + Integer.toUnsignedString(alert.title().hashCode());
}Alternatively, if collision avoidance is important, consider hashing the full title string (e.g., SHA-256 truncated).
📝 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
| private String buildKey(SystemAlert alert) { | |
| return KEY_PREFIX + alert.severity() + ":" + Math.abs(alert.title().hashCode()); | |
| } | |
| private String buildKey(SystemAlert alert) { | |
| return KEY_PREFIX + alert.severity() + ":" + Integer.toUnsignedString(alert.title().hashCode()); | |
| } |
🤖 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/alert/AlertDeduplicationService.java`
around lines 33 - 35, The current buildKey in AlertDeduplicationService uses
Math.abs(alert.title().hashCode()) which both can yield Integer.MIN_VALUE (stays
negative) and suffers collisions; replace this with a stable, non-negative
digest of the title (e.g., compute a SHA-256 or other strong hash of
alert.title() and use a truncated hex string) or at minimum convert the int hash
to an unsigned representation (Integer.toUnsignedLong(hash) or
Integer.toUnsignedString(hash)) before building the key; update buildKey to use
KEY_PREFIX + alert.severity() + ":" + <stable-nonnegative-title-digest> so keys
are consistently formatted and collisions are reduced.
Comment on lines
+25
to
+33
| @EventListener(ContextClosedEvent.class) | ||
| public void onContextClosed() { | ||
| systemAlertPublisher.publish( | ||
| AlertSeverity.WARN, | ||
| AlertType.DEPLOYMENT, | ||
| "[배포] 서버 종료", | ||
| "dorumdorum 서버가 종료되고 있습니다." | ||
| ); | ||
| } |
There was a problem hiding this comment.
Shutdown alert may not be reliably delivered.
During ContextClosedEvent, dependent beans (Redis for deduplication, HTTP client for Discord) may already be closing or destroyed. The alert publishing chain requires:
SystemAlertPublisherto publish eventDiscordAlertEventListener(async) to handle eventAlertDeduplicationServiceto check RedisDiscordAlertSenderto make HTTP call
Without explicit lifecycle ordering (SmartLifecycle, @DependsOn, or @Order), this shutdown alert may fail silently or not be sent at all.
Consider either:
- Accepting this limitation (startup alert is the critical one)
- Using
SmartLifecyclewith a high phase to ensure this runs before other beans close - Making the shutdown alert synchronous and bypassing deduplication
🤖 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/alert/ApplicationLifecycleAlertListener.java`
around lines 25 - 33, The shutdown alert in
ApplicationLifecycleAlertListener.onContextClosed may not be delivered because
dependent beans (AlertDeduplicationService/Redis, DiscordAlertSender/HTTP
client, async DiscordAlertEventListener) can be stopped before this event;
update the listener to ensure delivery by either making the shutdown flow run
before other beans stop or bypassing async/deduplication: implement
ApplicationLifecycleAlertListener as a SmartLifecycle with a high phase (start
false, isAutoStartup false, stop triggers publishing synchronously) so on stop
it publishes before lower-phase beans are stopped, or alternatively call
SystemAlertPublisher.publishShutdownSynchronously (create a sync publish method
that skips async event dispatch and AlertDeduplicationService) from
onContextClosed to send the alert directly to DiscordAlertSender; reference
ApplicationLifecycleAlertListener.onContextClosed, SystemAlertPublisher (add
publishShutdownSynchronously), DiscordAlertEventListener,
AlertDeduplicationService, and DiscordAlertSender when making the change.
Comment on lines
+66
to
+71
| if (isDuplicate(event)) { | ||
| addInfo("[DiscordLogbackAppender] 중복 로그 건너뜀: " + buildDedupKey(event)); | ||
| return; | ||
| } | ||
|
|
||
| sendToDiscord(event); |
There was a problem hiding this comment.
Don't leave failed sends locked out in the dedup cache.
The key is reserved before the webhook call, and the failure path only logs the exception. If httpClient.send(...) throws, the same WARN/ERROR is suppressed for the full TTL even though nothing was delivered.
Possible fix
`@Override`
protected void append(ILoggingEvent event) {
if (webhookUrl == null || webhookUrl.isBlank()) return;
if (!event.getLevel().isGreaterOrEqual(Level.WARN)) return;
periodicCleanup();
- if (isDuplicate(event)) {
- addInfo("[DiscordLogbackAppender] 중복 로그 건너뜀: " + buildDedupKey(event));
+ String key = buildDedupKey(event);
+ if (isDuplicate(key, event.getLevel())) {
+ addInfo("[DiscordLogbackAppender] 중복 로그 건너뜀: " + key);
return;
}
- sendToDiscord(event);
+ sendToDiscord(event, key);
}
-private boolean isDuplicate(ILoggingEvent event) {
- String key = buildDedupKey(event);
- long now = System.currentTimeMillis();
- long ttl = event.getLevel() == Level.ERROR ? ERROR_DEDUP_MILLIS : WARN_DEDUP_MILLIS;
+private boolean isDuplicate(String key, Level level) {
+ long now = System.currentTimeMillis();
+ long ttl = level == Level.ERROR ? ERROR_DEDUP_MILLIS : WARN_DEDUP_MILLIS;
boolean[] isDup = {false};
dedupCache.compute(key, (k, existingExpiry) -> {
if (existingExpiry != null && existingExpiry > now) {
isDup[0] = true;
@@
});
return isDup[0];
}
-private void sendToDiscord(ILoggingEvent event) {
+private void sendToDiscord(ILoggingEvent event, String key) {
try {
String payload = buildEmbedPayload(event);
HttpRequest request = HttpRequest.newBuilder()
.uri(URI.create(webhookUrl))
.timeout(Duration.ofSeconds(5))
@@
.POST(HttpRequest.BodyPublishers.ofString(payload))
.build();
httpClient.send(request, HttpResponse.BodyHandlers.discarding());
} catch (Exception e) {
+ dedupCache.remove(key);
addWarn("[DiscordLogbackAppender] Discord 전송 실패: " + e.getMessage());
}
}Also applies to: 82-95, 115-127
🤖 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/logging/DiscordLogbackAppender.java`
around lines 66 - 71, The dedup key is being reserved before calling the webhook
so failures leave the key in cache and suppress retries; modify the send flow in
DiscordLogbackAppender so that when you reserve/set the dedup key (the logic
around isDuplicate(...) and buildDedupKey(...)) you remove/unreserve that key if
the subsequent sendToDiscord/httpClient.send(...) throws or returns a
failure—wrap the webhook call in a try/catch (or try/finally) and on any
exception or non-success response explicitly delete the dedup cache entry before
rethrowing or logging; apply the same fix to the other duplicate-handling blocks
(the code around lines handling sendToDiscord/httpClient.send in the 82-95 and
115-127 ranges) to ensure failed sends do not remain locked out.
Comment on lines
+98
to
+100
| private String buildDedupKey(ILoggingEvent event) { | ||
| int msgHash = Math.abs(event.getFormattedMessage().hashCode()); | ||
| return event.getLevel().levelStr + ":" + shortLogger(event.getLoggerName()) + ":" + msgHash; |
There was a problem hiding this comment.
Use the full logger name in the dedup key.
shortLogger() collapses different classes that share the same simple name into one bucket. If com.foo.RoomService and com.bar.RoomService emit the same WARN/ERROR message during the TTL window, one alert suppresses the other.
Possible fix
private String buildDedupKey(ILoggingEvent event) {
+ String loggerName = event.getLoggerName() == null ? "unknown" : event.getLoggerName();
int msgHash = Math.abs(event.getFormattedMessage().hashCode());
- return event.getLevel().levelStr + ":" + shortLogger(event.getLoggerName()) + ":" + msgHash;
+ return event.getLevel().levelStr + ":" + loggerName + ":" + msgHash;
}🤖 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/logging/DiscordLogbackAppender.java`
around lines 98 - 100, The dedup key in buildDedupKey currently uses
shortLogger(event.getLoggerName()), which collapses distinct classes with the
same simple name; change it to use the full logger name so different packages
produce distinct keys (i.e., replace shortLogger(event.getLoggerName()) with
event.getLoggerName() in the return value of buildDedupKey), keeping the rest of
the key composition (event.getLevel().levelStr and the message hash from
event.getFormattedMessage()) unchanged.
…트-집계-오류 [FIX] 그룹 채팅 퇴장/강퇴 unread 집계 보정 및 테스트 안정화
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
src/main/java/com/project/dorumdorum/domain/chat/application/usecase/JoinChatRoomUseCase.java (2)
47-52:⚠️ Potential issue | 🟠 MajorDon't swallow non-duplicate join failures here.
src/main/java/com/project/dorumdorum/domain/chat/application/usecase/CreateChatRoomUseCase.java:52-59uses the same catch pattern, and the provided context saysChatRoomMemberService.join(...)may still translate duplicates before this layer. If that is still true, these handlers never run; if it now bubblesDataIntegrityViolationException, suppressing it without anisMember(...)re-check can commit the room approval while the chat membership is still missing. Re-throw when the row is still absent.Also applies to: 57-63
🤖 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/usecase/JoinChatRoomUseCase.java` around lines 47 - 52, The catch for DataIntegrityViolationException in JoinChatRoomUseCase (and similarly in CreateChatRoomUseCase) currently swallows all failures from ChatRoomMemberService.join(roomNo, hostUserNo) which can hide non-duplicate errors; modify the handler to, upon catching DataIntegrityViolationException, call ChatRoomRepository.isMember(roomNo, hostUserNo) (or the existing isMember(...) method) to verify the row exists and only suppress the exception if isMember returns true—otherwise re-throw the exception so the caller can handle the failed membership creation and avoid committing room approval without a member row.
42-42:⚠️ Potential issue | 🟠 MajorPublish the join frame only after commit.
handle()now runs inBEFORE_COMMIT, sobroadcastSafely(...)can notify subscribers about a join that later rolls back on flush/commit failure. Keep the DB work here, but register the STOMP send inafterCommit().Also applies to: 72-80
🤖 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/usecase/JoinChatRoomUseCase.java` at line 42, The event handler currently annotated with `@TransactionalEventListener`(phase = TransactionPhase.BEFORE_COMMIT) publishes STOMP join frames too early; change the logic so database changes remain in handle() but the call to broadcastSafely(...) is deferred until after the transaction commits. Keep handle() and its DB operations as-is, remove or keep the BEFORE_COMMIT listener only for DB work, and register an after-commit action (e.g., TransactionSynchronizationManager.registerSynchronization(...) or use `@TransactionalEventListener`(phase = TransactionPhase.AFTER_COMMIT) / an afterCommit() callback) that invokes broadcastSafely(...). Ensure the after-commit callback references the same data produced by handle() (IDs or DTOs) and that the join-frame send is executed only inside that afterCommit/AFTER_COMMIT path.src/main/java/com/project/dorumdorum/domain/chat/application/event/RoommateKickedEventListener.java (1)
42-42:⚠️ Potential issue | 🟠 MajorPublish the kick notification only after commit.
This listener now runs in
BEFORE_COMMIT, sobroadcastSafely(...)can still send a leave frame for a transaction that later rolls back. Keep the unread-count/member removal in the transaction, but move the WebSocket publish toafterCommit().Also applies to: 57-60, 65-69
🤖 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` at line 42, The listener currently annotated with `@TransactionalEventListener`(phase = TransactionPhase.BEFORE_COMMIT) (RoommateKickedEventListener) publishes WebSocket frames too early; move the broadcastSafely(...) call into an after-commit hook so the leave frame is only sent when the DB transaction commits. Keep the unread-count and member removal logic inside the transactional handler (the existing method that updates unread counts / removes the member), but either change the event listener phase for sending to TransactionPhase.AFTER_COMMIT or split responsibilities: leave the DB mutations in the current `@TransactionalEventListener` handler and invoke broadcastSafely(...) from an afterCommit() callback (or a separate `@TransactionalEventListener`(phase = TransactionPhase.AFTER_COMMIT)). Ensure you reference the broadcastSafely(...) invocation and the RoommateKickedEventListener method when making the change.src/main/java/com/project/dorumdorum/domain/chat/application/usecase/LeaveChatRoomUseCase.java (1)
112-115:⚠️ Potential issue | 🟡 MinorUse the correct
이/가particle for the display name.
displayName + "가 퇴장했습니다."is only correct when the name ends without a final consonant. Choose이/가from the last Hangul syllable here; the same helper can be reused by the other system-message builders.🤖 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/usecase/LeaveChatRoomUseCase.java` around lines 112 - 115, The system message uses a hardcoded "가" suffix; update LeaveChatRoomUseCase to pick the correct Korean subject particle (이/가) for displayName by inspecting the last character of leavingUser.getNickname()/getName() (e.g. in the existing displayName logic) and appending either "이" or "가" accordingly before forming content; implement a small reusable helper (e.g., determineSubjectParticle(String name) or KoreanParticleUtils.getSubjectParticle) and call it when building the content passed to chatMessageService.save (so content becomes displayName + particle + " 퇴장했습니다."), and ensure the helper is reusable by other system-message builders like any other code that builds system messages.
🧹 Nitpick comments (2)
src/test/java/com/project/dorumdorum/domain/chat/unit/usecase/MarkChatRoomReadUseCaseTest.java (1)
44-44: Strengthen receipt payload assertions (current matcher is too broad)Using
any(ChatReadReceiptResponse.class)won’t catch incorrect payload values. Please capture and assertchatRoomNo,readerUserNo, andreadAtin both tests.Proposed diff
+import org.mockito.ArgumentCaptor; +import static org.junit.jupiter.api.Assertions.*; @@ - verify(messagingTemplate).convertAndSend(eq("/topic/chat-room/cr-1/read"), any(ChatReadReceiptResponse.class)); + ArgumentCaptor<ChatReadReceiptResponse> payloadCaptor = ArgumentCaptor.forClass(ChatReadReceiptResponse.class); + verify(messagingTemplate).convertAndSend(eq("/topic/chat-room/cr-1/read"), payloadCaptor.capture()); + ChatReadReceiptResponse payload = payloadCaptor.getValue(); + assertEquals("cr-1", payload.chatRoomNo()); + assertEquals("u1", payload.readerUserNo()); + assertNotNull(payload.readAt()); @@ - verify(messagingTemplate).convertAndSend(eq("/topic/chat-room/cr-1/read"), any(ChatReadReceiptResponse.class)); + ArgumentCaptor<ChatReadReceiptResponse> payloadCaptor = ArgumentCaptor.forClass(ChatReadReceiptResponse.class); + verify(messagingTemplate).convertAndSend(eq("/topic/chat-room/cr-1/read"), payloadCaptor.capture()); + ChatReadReceiptResponse payload = payloadCaptor.getValue(); + assertEquals("cr-1", payload.chatRoomNo()); + assertEquals("u1", payload.readerUserNo()); + assertNotNull(payload.readAt());Also applies to: 61-61
🤖 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/MarkChatRoomReadUseCaseTest.java` at line 44, In MarkChatRoomReadUseCaseTest, replace the broad any(ChatReadReceiptResponse.class) matcher on the messagingTemplate.convertAndSend verify call with an ArgumentCaptor<ChatReadReceiptResponse> (or use Mockito's argThat) to capture the actual ChatReadReceiptResponse payload and assert its chatRoomNo, readerUserNo, and that readAt is non-null (or matches expected timestamp) in both test cases; update the two verify calls referencing messagingTemplate.convertAndSend so the captured response is used for assertions to ensure the payload fields are correct.src/test/java/com/project/dorumdorum/testsupport/TestcontainersSupport.java (1)
38-44: Avoid catchingThrowablein the Docker probe.Catching
Throwablecan hide fatal JVM conditions like OutOfMemoryError or StackOverflowError. CatchExceptioninstead so only probe failures are treated as "Docker unavailable."♻️ Proposed change
private static boolean checkDockerAvailability() { try { DockerClientFactory.instance().client(); return true; - } catch (Throwable ignored) { + } catch (Exception ignored) { return false; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/com/project/dorumdorum/testsupport/TestcontainersSupport.java` around lines 38 - 44, In checkDockerAvailability(), avoid catching Throwable; change the catch clause to catch Exception so fatal JVM errors (e.g., OutOfMemoryError/StackOverflowError) aren't swallowed—leave the try block that calls DockerClientFactory.instance().client() intact and only handle non-fatal probe failures by catching Exception and returning false.
🤖 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/usecase/LeaveChatRoomUseCase.java`:
- Around line 51-56: memberCount is captured too early and can be stale for both
flows: re-read the current member count inside the serialized/locked region
before deciding branches or mutating state. In LeaveChatRoomUseCase, update
logic in the GROUP flow inside the lock taken by leaveGroupChatRoom(chatRoom,
member, userNo, memberCount) to re-query the current count (use the repository
method that counts members) and then decide whether to transfer host, remove
member, or delete room; for the DIRECT flow (leaveDirectChatRoom) perform a
fresh count within a protected transaction/serialization or use an
optimistic-lock retry to recompute memberCount before branching to avoid two
concurrent leaves both seeing >1 or both seeing 1. Ensure all subsequent actions
(host transfer, last-member deletion) use this freshly-read count.
- Around line 109-119: sendLeaveSystemMessage is calling
messagingTemplate.convertAndSend() inside the `@Transactional` execute() path
which risks broker failures rolling back DB changes or publishing before commit;
change it to persist the ChatMessage via chatMessageService.save(...) inside
sendLeaveSystemMessage, then register a transaction synchronization (e.g.,
TransactionSynchronizationManager.registerSynchronization(...) / afterCommit
callback) to perform messagingTemplate.convertAndSend("/topic/chat-room/" +
chatRoomNo, response) after the transaction commits, and in that afterCommit
handler catch/log/send-failure metrics so websocket send failures do not affect
DB transaction and are observable.
---
Duplicate comments:
In
`@src/main/java/com/project/dorumdorum/domain/chat/application/event/RoommateKickedEventListener.java`:
- Line 42: The listener currently annotated with
`@TransactionalEventListener`(phase = TransactionPhase.BEFORE_COMMIT)
(RoommateKickedEventListener) publishes WebSocket frames too early; move the
broadcastSafely(...) call into an after-commit hook so the leave frame is only
sent when the DB transaction commits. Keep the unread-count and member removal
logic inside the transactional handler (the existing method that updates unread
counts / removes the member), but either change the event listener phase for
sending to TransactionPhase.AFTER_COMMIT or split responsibilities: leave the DB
mutations in the current `@TransactionalEventListener` handler and invoke
broadcastSafely(...) from an afterCommit() callback (or a separate
`@TransactionalEventListener`(phase = TransactionPhase.AFTER_COMMIT)). Ensure you
reference the broadcastSafely(...) invocation and the
RoommateKickedEventListener method when making the change.
In
`@src/main/java/com/project/dorumdorum/domain/chat/application/usecase/JoinChatRoomUseCase.java`:
- Around line 47-52: The catch for DataIntegrityViolationException in
JoinChatRoomUseCase (and similarly in CreateChatRoomUseCase) currently swallows
all failures from ChatRoomMemberService.join(roomNo, hostUserNo) which can hide
non-duplicate errors; modify the handler to, upon catching
DataIntegrityViolationException, call ChatRoomRepository.isMember(roomNo,
hostUserNo) (or the existing isMember(...) method) to verify the row exists and
only suppress the exception if isMember returns true—otherwise re-throw the
exception so the caller can handle the failed membership creation and avoid
committing room approval without a member row.
- Line 42: The event handler currently annotated with
`@TransactionalEventListener`(phase = TransactionPhase.BEFORE_COMMIT) publishes
STOMP join frames too early; change the logic so database changes remain in
handle() but the call to broadcastSafely(...) is deferred until after the
transaction commits. Keep handle() and its DB operations as-is, remove or keep
the BEFORE_COMMIT listener only for DB work, and register an after-commit action
(e.g., TransactionSynchronizationManager.registerSynchronization(...) or use
`@TransactionalEventListener`(phase = TransactionPhase.AFTER_COMMIT) / an
afterCommit() callback) that invokes broadcastSafely(...). Ensure the
after-commit callback references the same data produced by handle() (IDs or
DTOs) and that the join-frame send is executed only inside that
afterCommit/AFTER_COMMIT path.
In
`@src/main/java/com/project/dorumdorum/domain/chat/application/usecase/LeaveChatRoomUseCase.java`:
- Around line 112-115: The system message uses a hardcoded "가" suffix; update
LeaveChatRoomUseCase to pick the correct Korean subject particle (이/가) for
displayName by inspecting the last character of
leavingUser.getNickname()/getName() (e.g. in the existing displayName logic) and
appending either "이" or "가" accordingly before forming content; implement a
small reusable helper (e.g., determineSubjectParticle(String name) or
KoreanParticleUtils.getSubjectParticle) and call it when building the content
passed to chatMessageService.save (so content becomes displayName + particle + "
퇴장했습니다."), and ensure the helper is reusable by other system-message builders
like any other code that builds system messages.
---
Nitpick comments:
In
`@src/test/java/com/project/dorumdorum/domain/chat/unit/usecase/MarkChatRoomReadUseCaseTest.java`:
- Line 44: In MarkChatRoomReadUseCaseTest, replace the broad
any(ChatReadReceiptResponse.class) matcher on the
messagingTemplate.convertAndSend verify call with an
ArgumentCaptor<ChatReadReceiptResponse> (or use Mockito's argThat) to capture
the actual ChatReadReceiptResponse payload and assert its chatRoomNo,
readerUserNo, and that readAt is non-null (or matches expected timestamp) in
both test cases; update the two verify calls referencing
messagingTemplate.convertAndSend so the captured response is used for assertions
to ensure the payload fields are correct.
In `@src/test/java/com/project/dorumdorum/testsupport/TestcontainersSupport.java`:
- Around line 38-44: In checkDockerAvailability(), avoid catching Throwable;
change the catch clause to catch Exception so fatal JVM errors (e.g.,
OutOfMemoryError/StackOverflowError) aren't swallowed—leave the try block that
calls DockerClientFactory.instance().client() intact and only handle non-fatal
probe failures by catching Exception and returning false.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3ec3c496-bcc6-41cb-922d-692ebd934ebf
📒 Files selected for processing (17)
.github/workflows/cicd.yml.gitignoresrc/main/java/com/project/dorumdorum/domain/chat/application/dto/response/ChatMessageResponse.javasrc/main/java/com/project/dorumdorum/domain/chat/application/dto/response/ChatReadReceiptResponse.javasrc/main/java/com/project/dorumdorum/domain/chat/application/event/RoommateKickedEventListener.javasrc/main/java/com/project/dorumdorum/domain/chat/application/usecase/JoinChatRoomUseCase.javasrc/main/java/com/project/dorumdorum/domain/chat/application/usecase/LeaveChatRoomUseCase.javasrc/main/java/com/project/dorumdorum/domain/chat/application/usecase/MarkChatRoomReadUseCase.javasrc/main/java/com/project/dorumdorum/domain/chat/application/usecase/SendGroupChatMessageUseCase.javasrc/main/java/com/project/dorumdorum/domain/roommate/domain/service/RoommateService.javasrc/main/resources/application-dev.ymlsrc/test/java/com/project/dorumdorum/domain/chat/integration/ChatRoomSchemaInitializationTest.javasrc/test/java/com/project/dorumdorum/domain/chat/integration/ChatTransactionAtomicityIntegrationTest.javasrc/test/java/com/project/dorumdorum/domain/chat/unit/usecase/LeaveChatRoomUseCaseTest.javasrc/test/java/com/project/dorumdorum/domain/chat/unit/usecase/MarkChatRoomReadUseCaseTest.javasrc/test/java/com/project/dorumdorum/domain/chat/unit/usecase/RoommateKickedEventListenerTest.javasrc/test/java/com/project/dorumdorum/testsupport/TestcontainersSupport.java
✅ Files skipped from review due to trivial changes (3)
- .gitignore
- src/main/java/com/project/dorumdorum/domain/chat/application/dto/response/ChatReadReceiptResponse.java
- src/test/java/com/project/dorumdorum/domain/chat/integration/ChatTransactionAtomicityIntegrationTest.java
🚧 Files skipped from review as they are similar to previous changes (4)
- src/main/resources/application-dev.yml
- src/main/java/com/project/dorumdorum/domain/chat/application/usecase/SendGroupChatMessageUseCase.java
- src/main/java/com/project/dorumdorum/domain/roommate/domain/service/RoommateService.java
- src/main/java/com/project/dorumdorum/domain/chat/application/usecase/MarkChatRoomReadUseCase.java
Comment on lines
+51
to
+56
| if (ChatRoomType.GROUP.equals(chatRoom.getChatRoomType())) { | ||
| leaveGroupChatRoom(chatRoom, member, userNo, memberCount); | ||
| return; | ||
| } | ||
|
|
||
| leaveDirectChatRoom(chatRoom, member, userNo, memberCount); |
There was a problem hiding this comment.
memberCount is stale for both leave flows.
The count is captured before any serialization. In GROUP the room lock is taken later, and in DIRECT there is no lock at all, so two concurrent leaves can still both take the memberCount > 1 branch, wrongly block the last host from leaving, or skip deleting the now-empty chat room. Re-read the count inside the serialized section before branching.
Also applies to: 59-107
🤖 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/usecase/LeaveChatRoomUseCase.java`
around lines 51 - 56, memberCount is captured too early and can be stale for
both flows: re-read the current member count inside the serialized/locked region
before deciding branches or mutating state. In LeaveChatRoomUseCase, update
logic in the GROUP flow inside the lock taken by leaveGroupChatRoom(chatRoom,
member, userNo, memberCount) to re-query the current count (use the repository
method that counts members) and then decide whether to transfer host, remove
member, or delete room; for the DIRECT flow (leaveDirectChatRoom) perform a
fresh count within a protected transaction/serialization or use an
optimistic-lock retry to recompute memberCount before branching to avoid two
concurrent leaves both seeing >1 or both seeing 1. Ensure all subsequent actions
(host transfer, last-member deletion) use this freshly-read count.
Comment on lines
+109
to
+119
| private void sendLeaveSystemMessage(ChatRoom chatRoom, String userNo) { | ||
| String chatRoomNo = chatRoom.getChatRoomNo(); | ||
| User leavingUser = userService.findById(userNo); | ||
| String displayName = (leavingUser.getNickname() != null && !leavingUser.getNickname().isBlank()) | ||
| ? leavingUser.getNickname() : leavingUser.getName(); | ||
| String content = displayName + "가 퇴장했습니다."; | ||
| ChatMessage message = chatMessageService.save(chatRoom, "SYSTEM", content, MessageType.SYSTEM, 0); | ||
| ChatMessageResponse response = new ChatMessageResponse( | ||
| message.getMessageNo(), chatRoomNo, | ||
| "SYSTEM", null, content, MessageType.SYSTEM.name(), message.getCreatedAt(), message.getUnreadCount()); | ||
| messagingTemplate.convertAndSend("/topic/chat-room/" + chatRoomNo, response); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let me locate the file and examine the full class context
find . -type f -name "LeaveChatRoomUseCase.java"Repository: DorumDorum/BE
Length of output: 153
🏁 Script executed:
# Also search for the execute method to understand the transactional context
rg -A 30 "class LeaveChatRoomUseCase" src/main/java/com/project/dorumdorum/domain/chat/application/usecase/LeaveChatRoomUseCase.javaRepository: DorumDorum/BE
Length of output: 1296
🏁 Script executed:
# Find all `@Transactional` annotations in the class
rg "@Transactional" src/main/java/com/project/dorumdorum/domain/chat/application/usecase/LeaveChatRoomUseCase.javaRepository: DorumDorum/BE
Length of output: 75
🏁 Script executed:
# Look for the execute method signature
rg -B 5 "public.*execute" src/main/java/com/project/dorumdorum/domain/chat/application/usecase/LeaveChatRoomUseCase.javaRepository: DorumDorum/BE
Length of output: 259
🏁 Script executed:
# Check the entire sendLeaveSystemMessage method and surrounding context
rg -B 5 -A 20 "private void sendLeaveSystemMessage" src/main/java/com/project/dorumdorum/domain/chat/application/usecase/LeaveChatRoomUseCase.javaRepository: DorumDorum/BE
Length of output: 973
Move the STOMP send out of the transaction.
execute() is @Transactional, so convertAndSend() can roll back a successful leave on broker errors and can also publish a leave frame before the DB commit succeeds. Persist the ChatMessage in the transaction, then register the WebSocket send for afterCommit() and handle/log failures there.
🤖 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/usecase/LeaveChatRoomUseCase.java`
around lines 109 - 119, sendLeaveSystemMessage is calling
messagingTemplate.convertAndSend() inside the `@Transactional` execute() path
which risks broker failures rolling back DB changes or publishing before commit;
change it to persist the ChatMessage via chatMessageService.save(...) inside
sendLeaveSystemMessage, then register a transaction synchronization (e.g.,
TransactionSynchronizationManager.registerSynchronization(...) / afterCommit
callback) to perform messagingTemplate.convertAndSend("/topic/chat-room/" +
chatRoomNo, response) after the transaction commits, and in that afterCommit
handler catch/log/send-failure metrics so websocket send failures do not affect
DB transaction and are observable.
7 tasks
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
2 participants
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
📌 제목
Summary by CodeRabbit
New Features
Bug Fixes
Chores