Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Walkthrough매칭 요청 API(v3) 추가: POST 엔드포인트 및 서비스/밸리데이션/레포지토리/DTO/에러 코드 추가, 그룹멤버 생성 및 리더 알람 생성 흐름 도입, 알람 타입에 Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Controller as GroupV3Controller
participant Service as GroupRequestService
participant RepoG as GroupRepository
participant RepoV as GroupV3Repository
participant RepoM as GroupMemberRepository
participant Alarm as AlarmService
participant DB as Database
Client->>Controller: POST /v3/users/match-request/{matchId}
Controller->>Service: createRequest(userId, matchId)
Service->>RepoG: findGroupWithLock(matchId)
RepoG-->>Service: GroupValidationRes
Service->>RepoV: getValidation(userId, matchId)
RepoV->>DB: exists 쿼리 (중복/보류)
DB-->>RepoV: RequestValidationRes
RepoV-->>Service: RequestValidationRes
Service->>Service: validateCreateRequest(...)
alt validation ok
Service->>RepoM: createGroupMemberV3(userId, matchId)
RepoM->>DB: INSERT group_member
Service->>DB: UPDATE leader member 상태 -> NEW_REQUEST
Service->>Alarm: createAlarm(leaderId, AlarmType.NEW_REQUEST or PENDING)
Alarm->>DB: INSERT alarm
Service-->>Controller: success response
Controller-->>Client: 200 OK
else validation fail
Service-->>Controller: throw BusinessException
Controller-->>Client: Error response
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/main/java/at/mateball/domain/group/api/dto/RequestValidationRes.java (1)
2-7: 이 타입은api.dto보다 내부 계층 패키지에 두는 편이 좋겠습니다.컨트롤러 응답이 아니라 repository projection/서비스 내부 검증용이라서, 현재 위치는 infrastructure가 API 패키지에 의존하게 만듭니다.
infrastructure.dto나query쪽으로 옮기고Ressuffix도 내부 용도에 맞게 정리하는 편이 경계가 더 명확합니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/at/mateball/domain/group/api/dto/RequestValidationRes.java` around lines 2 - 7, 이 타입 RequestValidationRes(isDuplicatedRequest, hasPendingRequest)은 API 응답용이 아니라 내부 검증/프로젝션용이므로 패키지를 API 계층에서 분리하세요: 파일을 at.mateball.domain.group.api.dto에서 내부 계층 패키지(예: at.mateball.domain.group.infrastructure.dto 또는 at.mateball.domain.group.query)로 이동하고 타입명을 Res 접미사 대신 RequestValidationResult(또는 RequestValidationInfo 등 내부 용도에 맞는 명칭)로 변경한 뒤, 관련된 import/참조(Repository, Service, Query projection 등)를 모두 수정하여 API 계층이 내부 패키지에 의존하지 않도록 정리하세요.
🤖 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/at/mateball/domain/group/api/controller/GroupV3Controller.java`:
- Around line 62-72: The createRequest method in GroupV3Controller is returning
ResponseEntity.ok(...) which yields HTTP 200 despite using SuccessCode.CREATED;
change the response to return a 201 status by replacing ResponseEntity.ok(...)
with
ResponseEntity.status(HttpStatus.CREATED).body(MateballResponse.successWithNoData(SuccessCode.CREATED))
(and add the HttpStatus import) so the HTTP status matches SuccessCode.CREATED.
In `@src/main/java/at/mateball/domain/group/core/service/GroupV3Service.java`:
- Around line 217-227: The createRequest method currently checks invariants
(validateRequest) before inserting, which allows a race where two concurrent
requests both pass validation and create duplicate pending members; fix by
performing the validation-and-insert atomically inside a transaction with a
proper DB-level lock or constraint: load the target Group with a pessimistic
lock (e.g., findByIdForUpdate / repository method that does SELECT ... FOR
UPDATE) or add a unique DB constraint on (group_id, user_id, status=pending) and
catch the DB exception, then perform validateRequest and
groupMemberRepository.createGroupMember within the same transactional boundary
(same method), and handle duplicate-key/lock failures to return the appropriate
BusinessException; update usages of groupRepository.findById, validateRequest,
groupMemberRepository.createGroupMember, and
groupMemberRepository.updateMemberStatus accordingly.
- Around line 230-250: In validateRequest, move validations that can be decided
from the provided group before calling groupV3RepositoryCustom.getValidation:
first call validate(group.getGameInformation().getGameDate()), then check if
group.getLeader().getId().equals(userId) and throw
BusinessException(CANNOT_REQUEST_OWN_MATCH), then check if group.getStatus() ==
GroupStatus.COMPLETED.getValue() and throw
BusinessException(ALREADY_FINISHED_MATCH); only after these group-only checks
call groupV3RepositoryCustom.getValidation(userId, group.getId()) and then
handle RequestValidationRes.isDuplicatedRequest()/hasPendingRequest() as before.
---
Nitpick comments:
In `@src/main/java/at/mateball/domain/group/api/dto/RequestValidationRes.java`:
- Around line 2-7: 이 타입 RequestValidationRes(isDuplicatedRequest,
hasPendingRequest)은 API 응답용이 아니라 내부 검증/프로젝션용이므로 패키지를 API 계층에서 분리하세요: 파일을
at.mateball.domain.group.api.dto에서 내부 계층 패키지(예:
at.mateball.domain.group.infrastructure.dto 또는 at.mateball.domain.group.query)로
이동하고 타입명을 Res 접미사 대신 RequestValidationResult(또는 RequestValidationInfo 등 내부 용도에
맞는 명칭)로 변경한 뒤, 관련된 import/참조(Repository, Service, Query projection 등)를 모두 수정하여
API 계층이 내부 패키지에 의존하지 않도록 정리하세요.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 873e778f-96e5-4e88-a041-131bbae1bd8b
📒 Files selected for processing (7)
src/main/java/at/mateball/domain/alarm/common/AlarmType.javasrc/main/java/at/mateball/domain/group/api/controller/GroupV3Controller.javasrc/main/java/at/mateball/domain/group/api/dto/RequestValidationRes.javasrc/main/java/at/mateball/domain/group/core/service/GroupV3Service.javasrc/main/java/at/mateball/domain/group/infrastructure/repository/GroupV3RepositoryCustom.javasrc/main/java/at/mateball/domain/group/infrastructure/repository/GroupV3RepositoryImpl.javasrc/main/java/at/mateball/exception/code/BusinessErrorCode.java
src/main/java/at/mateball/domain/group/api/controller/GroupV3Controller.java
Show resolved
Hide resolved
src/main/java/at/mateball/domain/group/core/service/GroupV3Service.java
Outdated
Show resolved
Hide resolved
src/main/java/at/mateball/domain/group/core/service/GroupV3Service.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/main/java/at/mateball/domain/group/core/service/GroupV3Service.java (1)
216-225:⚠️ Potential issue | 🔴 Critical동시 요청에서 중복 requester가 생성될 수 있습니다.
Line 222~225는 “검증 후 삽입”이 분리되어 있어, 동시 요청 두 건이 동시에 통과하면 둘 다 생성될 수 있습니다. 이 제약은 서비스 로직만으로는 부족하고 DB 레벨 원자성(락/유니크 제약)으로 보장해야 합니다.
🔧 제안 수정
`@Transactional` public void createRequest(Long userId, Long groupId) { - Group group = groupRepository.findById(groupId) + Group group = groupRepository.findByIdForUpdate(groupId) .orElseThrow(() -> new BusinessException(BusinessErrorCode.GROUP_NOT_FOUND)); validateRequest(userId, group); - groupMemberRepository.createGroupMember(userId, groupId); + try { + groupMemberRepository.createGroupMember(userId, groupId); + } catch (DataIntegrityViolationException e) { + throw new BusinessException(BusinessErrorCode.DUPLICATED_MATCH_REQUEST); + } groupMemberRepository.updateMemberStatus(group.getLeader().getId(), group.getId(), GroupMemberStatus.NEW_REQUEST.getValue()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/at/mateball/domain/group/core/service/GroupV3Service.java` around lines 216 - 225, createRequest currently does "validateRequest" then "groupMemberRepository.createGroupMember", which allows duplicate requesters under concurrent calls; enforce atomicity by adding a DB-level constraint (unique on group_id + user_id in the group member table) and make createRequest resilient to constraint violations: keep the validateRequest call but ensure the persistence layer or createGroupMember handles unique constraint errors (e.g., catch DataIntegrityViolationException / ConstraintViolationException) and translate them to BusinessException(GROUP_MEMBER_ALREADY_EXISTS) or similar; alternatively acquire a row lock on the Group (SELECT ... FOR UPDATE) before calling groupMemberRepository.createGroupMember to serialize concurrent requests — update createRequest and groupMemberRepository.createGroupMember to implement one of these DB-level solutions and handle the resulting exception/path accordingly (also adjust groupMemberRepository.updateMemberStatus call flow to only run after successful insert).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/main/java/at/mateball/domain/group/core/service/GroupV3Service.java`:
- Around line 216-225: createRequest currently does "validateRequest" then
"groupMemberRepository.createGroupMember", which allows duplicate requesters
under concurrent calls; enforce atomicity by adding a DB-level constraint
(unique on group_id + user_id in the group member table) and make createRequest
resilient to constraint violations: keep the validateRequest call but ensure the
persistence layer or createGroupMember handles unique constraint errors (e.g.,
catch DataIntegrityViolationException / ConstraintViolationException) and
translate them to BusinessException(GROUP_MEMBER_ALREADY_EXISTS) or similar;
alternatively acquire a row lock on the Group (SELECT ... FOR UPDATE) before
calling groupMemberRepository.createGroupMember to serialize concurrent requests
— update createRequest and groupMemberRepository.createGroupMember to implement
one of these DB-level solutions and handle the resulting exception/path
accordingly (also adjust groupMemberRepository.updateMemberStatus call flow to
only run after successful insert).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a48dec59-c507-4fb9-aa5b-5a1497b89a75
📒 Files selected for processing (1)
src/main/java/at/mateball/domain/group/core/service/GroupV3Service.java
YangDaJin0104
requested changes
Mar 19, 2026
Contributor
YangDaJin0104
left a comment
YangDaJin0104
left a comment
There was a problem hiding this comment.
확실히 의도와 코드가 간결해 져서 보기 편하내요,,,ㅎ
| } No newline at end of file | ||
|
|
||
| @Transactional | ||
| public void createRequest(Long userId, Long groupId) { |
Contributor
There was a problem hiding this comment.
isDuplicatedRequest == false -> hasPendingRequest == false를 본 뒤에 createGroupMemeber() 실행이라 여기도 동시성 이슈가 생길수있으니 이젠 락을 걸어야할 차례가 왔소
| alarmService.createAlarm(group.getLeader().getId(), AlarmType.NEW_REQUEST, group.getId()); | ||
| } | ||
|
|
||
| private void validateRequest(Long userId, Group group) { |
Contributor
There was a problem hiding this comment.
이 부분이 조회 시점 기준 검증이라 원자성이 부족해서 아래 insert/update까지 한 트랜잭션 안에서 불변식을 보장하지 않으니, DB 수준에서 보호해줄 필요가있을거같습니당
| groupMemberRepository.createGroupMember(userId, groupId); | ||
| groupMemberRepository.updateMemberStatus(group.getLeader().getId(), group.getId(), GroupMemberStatus.NEW_REQUEST.getValue()); | ||
|
|
||
| alarmService.createAlarm(group.getLeader().getId(), AlarmType.NEW_REQUEST, group.getId()); |
Contributor
There was a problem hiding this comment.
알림 생성 실패 시 매칭 요청 생성도 록백이 되는건지? 아니면 핵심은 요청 생성이고, 알림은 부가 기능인지. 만약 이렇게 된다면 알림 저장 실패도 전체 록백으로 될 수 있는데 괜찮은지 확인 부탁드립니둉
src/main/java/at/mateball/domain/group/core/service/GroupV3Service.java
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
createRequest()는 검증, 요청 row 생성, 리더 상태 변경, 알림 생성까지 한 번에 오케스트레이션하고 있어서 흐름은 잘 보입니다. 다만 이후 승인/거절/실패 같은 상태 전이가 더 늘어나면 서비스가 비대해질 수 있어소 장기적으로는 request 도메인 정책을 조금 더 캡슐화해도 좋을 거 같습니당
Collaborator
Author
There was a problem hiding this comment.
매칭 생성/요청/수락/거절 api 만들고 매칭 관련 상위 서비스를 만들어도 좋을 것 같네요.///// -> 이 뜻으로 말씀한거 맞지요??? 말이 고급져서 약간 헷갈려요
Contributor
There was a problem hiding this comment.
넵 맞습니다아
지금 createRequest()는 검증, 요청 row 생성, 리더 상태 변경, 알림 생성까지 한 메서드에서 오케스트레이션하고 있어서 현재는 흐름이 잘 보이고 좋습니다만,,,
이후에 수락/거절/실패 처리까지 비슷한 패턴으로 계속 붙기 시작하면 GroupV3Service가 점점 커질 수 있어서. 그 시점에 매칭 요청/수락/거절 관련 흐름만 따로 묶는 상위 서비스나 도메인 정책 클래스로 분리해도 좋겠다는 의미였슴다
Collaborator
Author
There was a problem hiding this comment.
확인이요~! 이 리뷰를 반영해서 GroupRequestService 를 새롭게 추가했습니다, 일단 #239 에서 먼저 반영하고, 코드 합친다음에 이사시킬게요!!
hyeeum
force-pushed
the
feat/#239-create-request-v3-migration
branch
from
a0333ed to
2e304cb
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/at/mateball/domain/groupmember/core/GroupMember.java (1)
50-69:⚠️ Potential issue | 🟡 Minor4-파라미터 생성자에서 isLeader 필드가 설정되지 않아 데이터 불일치가 발생합니다.
기존 4-파라미터 생성자(50-56줄)는
isLeader를 설정하지 않아null로 남으며, 이를 통해 생성된 리더(GroupExecutor, 39줄)의isLeader값이null이 됩니다. 반면 새로운 5-파라미터 생성자(58-65줄)와member()팩토리 메서드(67-69줄)는isLeader를 명시적으로 설정합니다.GroupV3RepositoryImpl의 쿼리에서
.isLeader.isTrue()및.isLeader.isFalse()로 필터링하므로,null값을 가진 레코드와 불일치할 수 있습니다.4-파라미터 생성자 사용처를 모두 5-파라미터 생성자로 변경하거나, 4-파라미터 생성자를 제거하여 일관성을 유지하기 바랍니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/at/mateball/domain/groupmember/core/GroupMember.java` around lines 50 - 69, The 4-parameter GroupMember constructor leaves isLeader null causing mismatches with queries in GroupV3RepositoryImpl; update the 4-arg constructor in class GroupMember (the one taking User, Group, Boolean isParticipant, int status) to explicitly set this.isLeader = false (or remove the 4-arg ctor and replace its call sites to use the 5-arg constructor), ensuring all places that create GroupMember (including where GroupExecutor is constructed and the member() factory) produce non-null boolean values so .isLeader.isTrue()/isFalse() filters work correctly.
🧹 Nitpick comments (3)
src/main/java/at/mateball/domain/group/core/repository/querydsl/GroupRepositoryImpl.java (1)
294-307:@Override어노테이션이 누락되었습니다.이 클래스의 다른 메서드들과 일관성을 위해
@Override어노테이션을 추가해주세요.🔧 수정 제안
+ `@Override` public GroupValidationRes findValidateGroupData(Long groupId) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/at/mateball/domain/group/core/repository/querydsl/GroupRepositoryImpl.java` around lines 294 - 307, Add the missing `@Override` annotation to the findValidateGroupData method in GroupRepositoryImpl: locate the public GroupValidationRes findValidateGroupData(Long groupId) method (uses QGroup, queryFactory and constructs GroupValidationRes) and place `@Override` immediately above its signature to match other overridden methods in the class.src/main/java/at/mateball/domain/groupmember/core/repository/querydsl/GroupMemberRepositoryImpl.java (1)
844-854:@Override어노테이션이 누락되었습니다.인터페이스
GroupMemberRepositoryCustom에 선언된 메서드를 구현하므로@Override어노테이션을 추가해주세요.🔧 수정 제안
- - `@Override` public void createGroupMemberV3(Long userId, Long matchId) { User user = entityManager.getReference(User.class, userId); Group group = entityManager.getReference(Group.class, matchId); - GroupMember groupMember = GroupMember.member(user,group,GroupMemberStatus.AWAITING_APPROVAL.getValue()); + GroupMember groupMember = GroupMember.member(user, group, GroupMemberStatus.AWAITING_APPROVAL.getValue()); entityManager.persist(groupMember); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/at/mateball/domain/groupmember/core/repository/querydsl/GroupMemberRepositoryImpl.java` around lines 844 - 854, The implementation of createGroupMemberV3 in GroupMemberRepositoryImpl is missing the `@Override` annotation for the method declared in GroupMemberRepositoryCustom; add the `@Override` annotation above the createGroupMemberV3 method in GroupMemberRepositoryImpl to indicate it implements the interface method (locate the createGroupMemberV3 method in GroupMemberRepositoryImpl and annotate it with `@Override`).src/main/java/at/mateball/domain/group/infrastructure/repository/GroupV3RepositoryImpl.java (1)
328-356:@Override어노테이션 누락 및 null 안전성 검토 필요.
인터페이스 메서드 구현에
@Override어노테이션을 추가해주세요.이 메서드는
fetchOne()을 사용하여 그룹이 존재하지 않으면 null을 반환합니다.GroupV3Service.validateRequest에서findValidateGroupData로 그룹 존재를 먼저 확인하지만, 두 쿼리 사이에 그룹이 삭제되는 경쟁 조건이 발생하면 NPE가 발생할 수 있습니다.🔧 수정 제안
+ `@Override` public RequestValidationRes getValidation(Long userId, Long groupId) {
GroupV3Service.validateRequest에서 null 체크를 추가하거나, 반환 타입을Optional<RequestValidationRes>로 변경하는 것을 고려해주세요.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/at/mateball/domain/group/infrastructure/repository/GroupV3RepositoryImpl.java` around lines 328 - 356, Add the missing `@Override` on getValidation in GroupV3RepositoryImpl and make the method null-safe: either change its signature to return Optional<RequestValidationRes> (so getValidation uses fetchOne() and wraps result with Optional.ofNullable) or keep the current signature but ensure callers (e.g., GroupV3Service.validateRequest and its findValidateGroupData flow) perform a null check after calling getValidation to avoid NPE in the race where the group was deleted; adjust all usages accordingly (references: getValidation, GroupV3RepositoryImpl, RequestValidationRes, GroupV3Service.validateRequest, findValidateGroupData, fetchOne).
🤖 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/at/mateball/domain/group/core/service/GroupV3Service.java`:
- Around line 331-339: getValidation() can return null if the group was deleted
between findValidateGroupData() and the validation query, causing a potential
NPE when calling data.isDuplicatedRequest(); fix by null-checking the
RequestValidationRes returned from groupV3RepositoryCustom.getValidation(userId,
groupId) in GroupV3Service and if null throw a BusinessException (e.g.,
BusinessErrorCode.GROUP_NOT_FOUND) or re-run group existence validation, then
proceed to check data.isDuplicatedRequest() / data.hasPendingRequest().
---
Outside diff comments:
In `@src/main/java/at/mateball/domain/groupmember/core/GroupMember.java`:
- Around line 50-69: The 4-parameter GroupMember constructor leaves isLeader
null causing mismatches with queries in GroupV3RepositoryImpl; update the 4-arg
constructor in class GroupMember (the one taking User, Group, Boolean
isParticipant, int status) to explicitly set this.isLeader = false (or remove
the 4-arg ctor and replace its call sites to use the 5-arg constructor),
ensuring all places that create GroupMember (including where GroupExecutor is
constructed and the member() factory) produce non-null boolean values so
.isLeader.isTrue()/isFalse() filters work correctly.
---
Nitpick comments:
In
`@src/main/java/at/mateball/domain/group/core/repository/querydsl/GroupRepositoryImpl.java`:
- Around line 294-307: Add the missing `@Override` annotation to the
findValidateGroupData method in GroupRepositoryImpl: locate the public
GroupValidationRes findValidateGroupData(Long groupId) method (uses QGroup,
queryFactory and constructs GroupValidationRes) and place `@Override` immediately
above its signature to match other overridden methods in the class.
In
`@src/main/java/at/mateball/domain/group/infrastructure/repository/GroupV3RepositoryImpl.java`:
- Around line 328-356: Add the missing `@Override` on getValidation in
GroupV3RepositoryImpl and make the method null-safe: either change its signature
to return Optional<RequestValidationRes> (so getValidation uses fetchOne() and
wraps result with Optional.ofNullable) or keep the current signature but ensure
callers (e.g., GroupV3Service.validateRequest and its findValidateGroupData
flow) perform a null check after calling getValidation to avoid NPE in the race
where the group was deleted; adjust all usages accordingly (references:
getValidation, GroupV3RepositoryImpl, RequestValidationRes,
GroupV3Service.validateRequest, findValidateGroupData, fetchOne).
In
`@src/main/java/at/mateball/domain/groupmember/core/repository/querydsl/GroupMemberRepositoryImpl.java`:
- Around line 844-854: The implementation of createGroupMemberV3 in
GroupMemberRepositoryImpl is missing the `@Override` annotation for the method
declared in GroupMemberRepositoryCustom; add the `@Override` annotation above the
createGroupMemberV3 method in GroupMemberRepositoryImpl to indicate it
implements the interface method (locate the createGroupMemberV3 method in
GroupMemberRepositoryImpl and annotate it with `@Override`).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0e4607c7-3bb8-43bb-adf0-78514ad1206c
📒 Files selected for processing (13)
src/main/java/at/mateball/domain/alarm/common/AlarmType.javasrc/main/java/at/mateball/domain/group/api/controller/GroupV3Controller.javasrc/main/java/at/mateball/domain/group/api/dto/GroupValidationRes.javasrc/main/java/at/mateball/domain/group/api/dto/RequestValidationRes.javasrc/main/java/at/mateball/domain/group/core/repository/querydsl/GroupRepositoryCustom.javasrc/main/java/at/mateball/domain/group/core/repository/querydsl/GroupRepositoryImpl.javasrc/main/java/at/mateball/domain/group/core/service/GroupV3Service.javasrc/main/java/at/mateball/domain/group/infrastructure/repository/GroupV3RepositoryCustom.javasrc/main/java/at/mateball/domain/group/infrastructure/repository/GroupV3RepositoryImpl.javasrc/main/java/at/mateball/domain/groupmember/core/GroupMember.javasrc/main/java/at/mateball/domain/groupmember/core/repository/querydsl/GroupMemberRepositoryCustom.javasrc/main/java/at/mateball/domain/groupmember/core/repository/querydsl/GroupMemberRepositoryImpl.javasrc/main/java/at/mateball/exception/code/BusinessErrorCode.java
✅ Files skipped from review due to trivial changes (3)
- src/main/java/at/mateball/domain/alarm/common/AlarmType.java
- src/main/java/at/mateball/domain/group/api/dto/GroupValidationRes.java
- src/main/java/at/mateball/domain/group/api/dto/RequestValidationRes.java
🚧 Files skipped from review as they are similar to previous changes (3)
- src/main/java/at/mateball/domain/group/infrastructure/repository/GroupV3RepositoryCustom.java
- src/main/java/at/mateball/exception/code/BusinessErrorCode.java
- src/main/java/at/mateball/domain/group/api/controller/GroupV3Controller.java
src/main/java/at/mateball/domain/group/core/service/GroupV3Service.java
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
넵 맞습니다아
지금 createRequest()는 검증, 요청 row 생성, 리더 상태 변경, 알림 생성까지 한 메서드에서 오케스트레이션하고 있어서 현재는 흐름이 잘 보이고 좋습니다만,,,
이후에 수락/거절/실패 처리까지 비슷한 패턴으로 계속 붙기 시작하면 GroupV3Service가 점점 커질 수 있어서. 그 시점에 매칭 요청/수락/거절 관련 흐름만 따로 묶는 상위 서비스나 도메인 정책 클래스로 분리해도 좋겠다는 의미였슴다
Comment on lines
+337
to
+339
| if (data.hasPendingRequest()) { | ||
| throw new BusinessException(BusinessErrorCode.ALREADY_HAS_PENDING_REQUEST); | ||
| } |
Contributor
There was a problem hiding this comment.
unique constraint로 같은 사용자의 중복 요청은 막았는데, 이 메서드는 조회 기반 검증이라 서로 다른 두 사용자가 동시에 같은 match에 요청하는 경우까지는 완전하게 못 막을거같아서 보완하면 더 좋을거같아용!
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/main/java/at/mateball/domain/group/core/repository/GroupRepository.java (1)
25-27: LGTM! 비관적 락 구현이 적절합니다.동시 요청 시 race condition을 방지하기 위한
PESSIMISTIC_WRITE락이 올바르게 적용되었습니다. 이는 과거 리뷰에서 지적된 동시성 이슈를 해결합니다.운영 환경에서 락 대기 시간이 길어질 경우를 대비하여
@QueryHints로 타임아웃 설정을 고려해볼 수 있습니다.♻️ 선택적 개선안: 락 타임아웃 설정
+import jakarta.persistence.QueryHint; +import org.springframework.data.jpa.repository.QueryHints; `@Lock`(LockModeType.PESSIMISTIC_WRITE) +@QueryHints({`@QueryHint`(name = "jakarta.persistence.lock.timeout", value = "3000")}) `@Query`("select g from Group g where g.id = :groupId") Optional<Group> findGroupWithLock(Long groupId);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/at/mateball/domain/group/core/repository/GroupRepository.java` around lines 25 - 27, Add a pessimistic lock timeout hint to the existing findGroupWithLock method to avoid indefinite waits under contention: update the repository method (findGroupWithLock in GroupRepository) to include `@QueryHints` with a javax.persistence.lock.timeout or "javax.persistence.query.timeout" value (depending on JPA provider) and a suitable millisecond timeout value so that the PESSIMISTIC_WRITE lock fails fast instead of blocking indefinitely; ensure the hint is applied alongside `@Lock` and `@Query` annotations and adjust tests/handlers to expect a lock timeout exception where appropriate.src/main/java/at/mateball/domain/group/api/dto/GroupValidationRes.java (1)
12-18: 지연 로딩 연관관계 접근에 대한 고려사항
from()메서드에서group.getLeader().getId()와group.getGameInformation().getGameDate()를 호출하는데, 두 연관관계 모두FetchType.LAZY로 설정되어 있습니다 (src/main/java/at/mateball/domain/group/core/Group.java:20-26참조).현재
GroupRequestService.createRequest()가@Transactional내에서 호출하므로LazyInitializationException은 발생하지 않습니다. 다만 요청당 2개의 추가 쿼리가 발생합니다.단일 요청 처리에서는 큰 문제가 아니지만, 향후 성능 최적화가 필요할 경우 fetch join을 사용하는
findGroupWithLockFetchJoin()메서드로 대체할 수 있습니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/at/mateball/domain/group/api/dto/GroupValidationRes.java` around lines 12 - 18, GroupValidationRes.from에서 group.getLeader().getId()와 group.getGameInformation().getGameDate()가 LAZY 연관관계를 직접 접근해 요청당 추가 쿼리가 발생하므로, 성능 최적화 시 GroupRequestService.createRequest에서 호출하는 코드 경로를 변경해 fetch join을 사용한 조회로 대체하세요; 구체적으로 현재 사용하는 엔티티 조회를 findGroupWithLockFetchJoin 같은 fetch-join 기반 메서드로 바꿔 필요한 연관(leader, gameInformation)을 한 번에 로드하거나, 호출 전 트랜잭션 내부에서 명시적으로 초기화해 추가 쿼리를 제거하도록 수정하세요.
🤖 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/at/mateball/domain/groupRequest/GroupRequestService.java`:
- Around line 35-36: The call to groupV3RepositoryCustom.getValidation(userId,
groupId) can return null and must be checked before calling
groupRequestValidator.validateCreateRequest; update GroupRequestService to
verify the returned RequestValidationRes (e.g., validationData) is not null and
handle the null case (throw a suitable exception or return an error) instead of
passing it to groupRequestValidator.validateCreateRequest(userId,
validatedGroup, validationData); ensure you reference getValidation,
RequestValidationRes, groupV3RepositoryCustom,
groupRequestValidator.validateCreateRequest, userId, groupId and validatedGroup
when locating where to add the null check and error handling.
---
Nitpick comments:
In `@src/main/java/at/mateball/domain/group/api/dto/GroupValidationRes.java`:
- Around line 12-18: GroupValidationRes.from에서 group.getLeader().getId()와
group.getGameInformation().getGameDate()가 LAZY 연관관계를 직접 접근해 요청당 추가 쿼리가 발생하므로, 성능
최적화 시 GroupRequestService.createRequest에서 호출하는 코드 경로를 변경해 fetch join을 사용한 조회로
대체하세요; 구체적으로 현재 사용하는 엔티티 조회를 findGroupWithLockFetchJoin 같은 fetch-join 기반 메서드로 바꿔
필요한 연관(leader, gameInformation)을 한 번에 로드하거나, 호출 전 트랜잭션 내부에서 명시적으로 초기화해 추가 쿼리를
제거하도록 수정하세요.
In `@src/main/java/at/mateball/domain/group/core/repository/GroupRepository.java`:
- Around line 25-27: Add a pessimistic lock timeout hint to the existing
findGroupWithLock method to avoid indefinite waits under contention: update the
repository method (findGroupWithLock in GroupRepository) to include `@QueryHints`
with a javax.persistence.lock.timeout or "javax.persistence.query.timeout" value
(depending on JPA provider) and a suitable millisecond timeout value so that the
PESSIMISTIC_WRITE lock fails fast instead of blocking indefinitely; ensure the
hint is applied alongside `@Lock` and `@Query` annotations and adjust tests/handlers
to expect a lock timeout exception where appropriate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 254d6112-6ff5-43bc-b9c1-1261cd77ace9
📒 Files selected for processing (5)
src/main/java/at/mateball/domain/group/api/dto/GroupValidationRes.javasrc/main/java/at/mateball/domain/group/core/repository/GroupRepository.javasrc/main/java/at/mateball/domain/group/core/service/GroupV3Service.javasrc/main/java/at/mateball/domain/groupRequest/GroupRequestService.javasrc/main/java/at/mateball/domain/groupRequest/GroupRequestValidator.java
Comment on lines
+35
to
+36
| RequestValidationRes validationData = groupV3RepositoryCustom.getValidation(userId, groupId); | ||
| groupRequestValidator.validateCreateRequest(userId, validatedGroup, validationData); |
There was a problem hiding this comment.
getValidation() 결과에 대한 null 체크가 누락되었습니다.
groupV3RepositoryCustom.getValidation()은 그룹이 존재하지 않으면 null을 반환할 수 있습니다. 비관적 락으로 그룹을 조회한 후이지만, 극단적인 경우 null이 반환되면 Line 36에서 validateCreateRequest() 호출 시 NPE가 발생할 수 있습니다.
🛡️ 수정 제안
GroupValidationRes validatedGroup = GroupValidationRes.from(group);
RequestValidationRes validationData = groupV3RepositoryCustom.getValidation(userId, groupId);
+
+if (validationData == null) {
+ throw new BusinessException(BusinessErrorCode.GROUP_NOT_FOUND);
+}
+
groupRequestValidator.validateCreateRequest(userId, validatedGroup, validationData);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/java/at/mateball/domain/groupRequest/GroupRequestService.java`
around lines 35 - 36, The call to groupV3RepositoryCustom.getValidation(userId,
groupId) can return null and must be checked before calling
groupRequestValidator.validateCreateRequest; update GroupRequestService to
verify the returned RequestValidationRes (e.g., validationData) is not null and
handle the null case (throw a suitable exception or return an error) instead of
passing it to groupRequestValidator.validateCreateRequest(userId,
validatedGroup, validationData); ensure you reference getValidation,
RequestValidationRes, groupV3RepositoryCustom,
groupRequestValidator.validateCreateRequest, userId, groupId and validatedGroup
when locating where to add the null check and error handling.
Contributor
YangDaJin0104
left a comment
YangDaJin0104
left a comment
There was a problem hiding this comment.
동시성 문제 해결한 부분 확인했습니당! 생성 예외처리와 방어 로직은 보안하면 더 좋을거같습니당
|
|
||
| GroupMember groupMember = GroupMember.member(user,group,GroupMemberStatus.AWAITING_APPROVAL.getValue()); | ||
|
|
||
| entityManager.persist(groupMember); |
Contributor
There was a problem hiding this comment.
여기서 DB unique constraint 실패를 비지니스 예외로 바꿔줘야 서버 500 에러가 안터집니당!
Collaborator
Author
There was a problem hiding this comment.
#238 에 새롭게 추가된 ConstraintExceptionTranslator 가 ok 싸인받고 병합된다면,translator 를 사용해서 예외처리를 하겠습니다!
hyeeum
force-pushed
the
feat/#239-create-request-v3-migration
branch
from
dade148 to
73d37a7
Compare
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.
📌 이슈 번호
closed #239
✅ 어떻게 이슈를 해결했나요?
❤️ To 다진 / To 헤음
Summary by CodeRabbit
새로운 기능
예외 처리 / 개선
데이터 무결성
알림