Merged
Conversation
Walkthrough그룹 멤버 제거(kick) 기능을 추가했습니다. MemberController 라우팅이 변경 사항
시퀀스 다이어그램sequenceDiagram
participant Client
participant MemberController
participant KickMemberService
participant GroupRoleRepository
participant GroupMemberRepository
participant Database
Client->>MemberController: DELETE /v1/groups/{groupId}/members/{memberId}
MemberController->>KickMemberService: kickMember(KickMemberCommand)
KickMemberService->>GroupRoleRepository: checkPermission(userId, groupId, MEMBER_MANAGE)
GroupRoleRepository->>Database: findGroupMember(userId, groupId) / existsByGroupIdAndRole(...)
Database-->>GroupRoleRepository: GroupMember / boolean
alt 권한 없음
GroupRoleRepository-->>KickMemberService: false
KickMemberService-->>MemberController: IllegalArgumentException
MemberController-->>Client: 400/403 응답
else 권한 있음
KickMemberService->>GroupMemberRepository: checkMember(memberId)
GroupMemberRepository->>Database: existsById(memberId)
Database-->>GroupMemberRepository: boolean
alt 멤버 없음
GroupMemberRepository-->>KickMemberService: false
KickMemberService-->>MemberController: IllegalArgumentException
MemberController-->>Client: 400 응답
else 멤버 있음
KickMemberService->>GroupMemberRepository: deleteGroupMember(memberId)
GroupMemberRepository->>Database: findByIdForUpdate(groupId) / deleteById(memberId)
Database-->>GroupMemberRepository: 삭제 및 그룹 카운트 갱신
GroupMemberRepository-->>KickMemberService: void
KickMemberService-->>MemberController: void
MemberController-->>Client: 204 No Content
end
end
예상 코드 리뷰 소요 시간🎯 3 (Moderate) | ⏱️ ~25 minutes 관련 PR
시
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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: 4
🧹 Nitpick comments (2)
src/main/java/flipnote/group/application/service/KickMemberService.java (1)
25-27: 권한 오류에 적합한 예외 타입 사용 권장
IllegalArgumentException은 잘못된 인자를 나타내는 예외입니다. 권한 부족 상황에는AccessDeniedException또는 커스텀 예외(예:PermissionDeniedException)를 사용하는 것이 더 적합합니다. 이렇게 하면 예외 핸들러에서 적절한 HTTP 상태 코드(403 Forbidden)를 반환하기 쉬워집니다.♻️ 예외 타입 변경 제안
- throw new IllegalArgumentException("not exist permission"); + throw new AccessDeniedException("not exist permission");또는 커스텀 예외 사용:
throw new PermissionDeniedException("Member management permission required");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/flipnote/group/application/service/KickMemberService.java` around lines 25 - 27, Replace the use of IllegalArgumentException in KickMemberService where permission is checked with an appropriate access-denied exception: throw java.nio.file.AccessDeniedException or, preferably, a custom PermissionDeniedException (e.g., PermissionDeniedException("Member management permission required")) in the method that currently does the if(!hasPermission) check; update the import and exception type used in KickMemberService and ensure any controller/exception-handler maps the new exception to 403 Forbidden.src/main/java/flipnote/group/adapter/out/entity/GroupMemberEntity.java (1)
52-57: ID 수동 할당 허용에 대한 주의생성자에
id파라미터를 추가하셨습니다.@GeneratedValue(strategy = GenerationType.IDENTITY)가 설정되어 있으므로 ID는 데이터베이스에서 자동 생성됩니다.
create()팩토리 메서드에서는id를 전달하지 않으므로 정상 동작하지만, 빌더를 직접 사용하여id를 수동으로 설정하면 다음과 같은 문제가 발생할 수 있습니다:
- 새 엔티티 저장 시 ID 충돌
- 기존 엔티티 덮어쓰기
테스트 목적이라면 괜찮으나, 프로덕션 코드에서 빌더를 통한 ID 직접 설정은 피하시기 바랍니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/flipnote/group/adapter/out/entity/GroupMemberEntity.java` around lines 52 - 57, The constructor GroupMemberEntity(Long id, Long groupId, Long userId, RoleEntity role) allows manual id assignment which conflicts with the `@GeneratedValue` identity strategy; remove the id parameter from the constructor (and any builder pathway that exposes it) so entities are constructed with new GroupMemberEntity(groupId, userId, role) or keep a private/internal-only constructor for tests; update the builder/factory methods (create() and any Builder class) to not accept or set id and add a runtime guard in GroupMemberEntity (e.g., throw or ignore if id is passed externally) if you must keep an id-accepting constructor for tests.
🤖 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/flipnote/group/adapter/out/persistence/GroupMemberRepositoryAdapter.java`:
- Around line 91-98: deleteGroupMember currently removes a member but never
decrements the group's member count (save() calls plusCount() at Line 37),
causing aggregation drift; update deleteGroupMember to load the associated Group
entity (via the GroupMember -> group reference or GroupRepository), call the
counterpart decrement method (e.g., minusCount() or decrementCount()) on the
Group, persist the updated Group (GroupRepository.save or appropriate port) and
perform these operations inside the same transaction to preserve
consistency/concurrency guarantees; ensure you still validate member existence
(the existing orElseThrow) before updating counts.
In
`@src/main/java/flipnote/group/adapter/out/persistence/GroupRoleRepositoryAdapter.java`:
- Around line 106-113: The current checkPermission method ignores the requested
GroupPermission and only verifies the role exists; update it to validate that
the member's role actually grants the requested permission: retrieve the member
via groupMemberRepository.findByGroupIdAndUserId, obtain the member's role via
groupMember.getRole() (or role name via getRole()), and either check the role's
permission collection (e.g., role.getPermissions().contains(permission)) or
add/use a repository method such as
groupRoleRepository.existsByGroupIdAndRoleAndPermission(groupId, roleName,
permission) to return true only when the role grants that specific
GroupPermission.
In
`@src/main/java/flipnote/group/application/port/out/GroupMemberRepositoryPort.java`:
- Line 19: The deleteGroupMember port currently accepts only memberId
(deleteGroupMember) which can cause cross-group deletions; change the port
signature to require the group scope by adding a groupId parameter (e.g.,
deleteGroupMember(Long groupId, Long memberId)), update all implementations,
adapters, and callers of GroupMemberRepositoryPort.deleteGroupMember to pass and
validate groupId, and update related unit/integration tests and any
transactional/authorization checks to use both groupId and memberId when
deleting.
In `@src/main/java/flipnote/group/application/service/KickMemberService.java`:
- Around line 22-30: KickMemberService.kickMember must validate the target
before deletion: verify the target member with cmd.memberId() actually belongs
to cmd.groupId(), prevent the requester cmd.userId() from kicking themselves,
and enforce role hierarchy so a requester cannot kick a member with equal or
higher role. Update the method to (1) load the target membership/role for
cmd.memberId() in cmd.groupId() (e.g., via groupMemberRepository lookup), throw
if not found, (2) if cmd.userId().equals(cmd.memberId()) throw an
IllegalArgumentException, and (3) compare the requester's role (use
groupRoleRepository.checkPermission or fetch requester role) against the
target's role and reject the operation if requester role is not higher; only
call groupMemberRepository.deleteGroupMember(cmd.memberId()) after these checks.
---
Nitpick comments:
In `@src/main/java/flipnote/group/adapter/out/entity/GroupMemberEntity.java`:
- Around line 52-57: The constructor GroupMemberEntity(Long id, Long groupId,
Long userId, RoleEntity role) allows manual id assignment which conflicts with
the `@GeneratedValue` identity strategy; remove the id parameter from the
constructor (and any builder pathway that exposes it) so entities are
constructed with new GroupMemberEntity(groupId, userId, role) or keep a
private/internal-only constructor for tests; update the builder/factory methods
(create() and any Builder class) to not accept or set id and add a runtime guard
in GroupMemberEntity (e.g., throw or ignore if id is passed externally) if you
must keep an id-accepting constructor for tests.
In `@src/main/java/flipnote/group/application/service/KickMemberService.java`:
- Around line 25-27: Replace the use of IllegalArgumentException in
KickMemberService where permission is checked with an appropriate access-denied
exception: throw java.nio.file.AccessDeniedException or, preferably, a custom
PermissionDeniedException (e.g., PermissionDeniedException("Member management
permission required")) in the method that currently does the if(!hasPermission)
check; update the import and exception type used in KickMemberService and ensure
any controller/exception-handler maps the new exception to 403 Forbidden.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
src/main/java/flipnote/group/adapter/in/web/MemberController.javasrc/main/java/flipnote/group/adapter/in/web/PermissionController.javasrc/main/java/flipnote/group/adapter/out/entity/GroupMemberEntity.javasrc/main/java/flipnote/group/adapter/out/persistence/GroupMemberRepositoryAdapter.javasrc/main/java/flipnote/group/adapter/out/persistence/GroupRoleRepositoryAdapter.javasrc/main/java/flipnote/group/application/port/in/KickMemberUseCase.javasrc/main/java/flipnote/group/application/port/in/command/KickMemberCommand.javasrc/main/java/flipnote/group/application/port/out/GroupMemberRepositoryPort.javasrc/main/java/flipnote/group/application/service/KickMemberService.javasrc/main/java/flipnote/group/domain/model/member/MemberInfo.javasrc/main/java/flipnote/group/domain/model/permission/GroupPermission.javasrc/main/java/flipnote/group/infrastructure/persistence/jpa/GroupRoleRepository.java
💤 Files with no reviewable changes (1)
- src/main/java/flipnote/group/adapter/in/web/PermissionController.java
src/main/java/flipnote/group/adapter/out/persistence/GroupMemberRepositoryAdapter.java
Show resolved
Hide resolved
src/main/java/flipnote/group/adapter/out/persistence/GroupRoleRepositoryAdapter.java
Show resolved
Hide resolved
src/main/java/flipnote/group/application/port/out/GroupMemberRepositoryPort.java
Show resolved
Hide resolved
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/main/java/flipnote/group/application/port/out/GroupMemberRepositoryPort.java (1)
21-21: 파라미터 이름aLong은 의미가 불분명합니다.
memberId로 명명하여 코드 가독성을 높이세요.♻️ 제안 수정
- boolean checkMember(Long aLong); + boolean checkMember(Long memberId);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/flipnote/group/application/port/out/GroupMemberRepositoryPort.java` at line 21, The parameter name aLong in the interface method boolean checkMember(Long aLong) is unclear; rename the parameter to memberId to improve readability and update the method signature in GroupMemberRepositoryPort (and any implementing classes) from checkMember(Long aLong) to checkMember(Long memberId), and adjust all call sites and implementations to use the new parameter name.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@src/main/java/flipnote/group/application/port/out/GroupMemberRepositoryPort.java`:
- Line 21: The parameter name aLong in the interface method boolean
checkMember(Long aLong) is unclear; rename the parameter to memberId to improve
readability and update the method signature in GroupMemberRepositoryPort (and
any implementing classes) from checkMember(Long aLong) to checkMember(Long
memberId), and adjust all call sites and implementations to use the new
parameter name.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/main/java/flipnote/group/adapter/out/persistence/GroupMemberRepositoryAdapter.javasrc/main/java/flipnote/group/application/port/out/GroupMemberRepositoryPort.javasrc/main/java/flipnote/group/application/service/KickMemberService.java
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
1 participant
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.
Summary by CodeRabbit