Conversation
…into feat/add-member-role
|
Caution Review failedThe pull request is closed. Walkthrough그룹 멤버 권한 모델을 enum 기반에서 DB 관리형 역할/권한 모델로 전환합니다. GroupMemberEntity가 role 대신 groupRoleId를 사용하도록 변경되고 RoleEntity·PermissionEntity와 관련 리포지토·포트·매퍼·서비스에서 역할 생성·검증 로직이 추가되었습니다. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant CreateGroupService
participant GroupRepository
participant GroupRoleRepositoryAdapter
participant RoleRepository
participant GroupRolePermissionRepository
participant GroupMemberRepositoryAdapter
participant GroupMemberRepository
User->>CreateGroupService: 그룹 생성 요청
CreateGroupService->>GroupRepository: 그룹 저장
GroupRepository-->>CreateGroupService: groupId
CreateGroupService->>GroupRoleRepositoryAdapter: create(groupId)
GroupRoleRepositoryAdapter->>RoleRepository: save RoleEntity(OWNER/HEAD_MANAGER/MANAGER/MEMBER)
RoleRepository-->>GroupRoleRepositoryAdapter: roleIds
GroupRoleRepositoryAdapter->>GroupRolePermissionRepository: save PermissionEntity들
GroupRolePermissionRepository-->>GroupRoleRepositoryAdapter: permissionIds
GroupRoleRepositoryAdapter-->>CreateGroupService: ownerRoleId
CreateGroupService->>GroupMemberRepositoryAdapter: save(groupId, userId, ownerRoleId)
GroupMemberRepositoryAdapter->>GroupMemberRepository: save GroupMemberEntity
GroupMemberRepository-->>GroupMemberRepositoryAdapter: 완료
GroupMemberRepositoryAdapter-->>CreateGroupService: 완료
CreateGroupService-->>User: 그룹 생성 완료
sequenceDiagram
actor User
participant ChangeGroupService
participant GroupRoleRepositoryAdapter
participant RoleRepository
participant GroupMemberRepository
participant GroupRepository
User->>ChangeGroupService: 그룹 수정 요청
ChangeGroupService->>GroupRoleRepositoryAdapter: checkRole(userId, groupId, OWNER)
GroupRoleRepositoryAdapter->>RoleRepository: findByGroupIdAndRole(groupId, OWNER)
RoleRepository-->>GroupRoleRepositoryAdapter: roleEntity
GroupRoleRepositoryAdapter->>GroupMemberRepository: existsByUserIdAndGroupRoleId(userId, roleId)
GroupMemberRepository-->>GroupRoleRepositoryAdapter: 존재 여부
alt owner
GroupRoleRepositoryAdapter-->>ChangeGroupService: true
ChangeGroupService->>GroupRepository: 그룹 수정 수행
GroupRepository-->>ChangeGroupService: 수정 완료
ChangeGroupService-->>User: 수정 성공
else not owner
GroupRoleRepositoryAdapter-->>ChangeGroupService: false
ChangeGroupService-->>User: IllegalArgumentException("not owner")
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/main/java/flipnote/group/adapter/out/persistence/mapper/GroupMemberMapper.java (1)
6-6:⚠️ Potential issue | 🟡 Minor사용되지 않는
GroupMemberRoleimport가 남아 있습니다.enum 기반 역할에서
Long roleId로 마이그레이션했으므로 이 import는 더 이상 필요하지 않습니다.EnumType,Enumeratedimport도 확인이 필요합니다.♻️ 수정 제안
-import flipnote.group.domain.model.member.GroupMemberRole;src/main/java/flipnote/group/adapter/out/entity/GroupMemberEntity.java (1)
4-8:⚠️ Potential issue | 🟡 Minor사용되지 않는 import 3개가 남아 있습니다.
enum 기반 역할에서
Long groupRoleId로 마이그레이션했으므로GroupMemberRole,EnumType,Enumeratedimport가 더 이상 필요하지 않습니다.♻️ 수정 제안
-import flipnote.group.domain.model.member.GroupMemberRole; import jakarta.persistence.Column; import jakarta.persistence.Entity; -import jakarta.persistence.EnumType; -import jakarta.persistence.Enumerated; import jakarta.persistence.GeneratedValue;
🤖 Fix all issues with AI agents
In `@src/main/java/flipnote/group/adapter/out/entity/RoleEntity.java`:
- Around line 19-24: RoleEntity에 선언된 `@Table의` uniqueConstraints에서 columnNames에
잘못된 컬럼명을 사용하고 제약 이름이 혼동을 일으킵니다; update the uniqueConstraints in the `@Table`
annotation to use the actual DB column name "group_role" instead of "role" and
rename the constraint from "uk_group_roles_group_name" to a clearer name like
"uk_group_roles_group_role" (or similar). Locate the `@Table`(...) on the
RoleEntity class and the `@Column`(name = "group_role") declaration to ensure the
columnNames array matches "group_role" and the constraint name reflects the
columns it covers.
In `@src/main/java/flipnote/group/application/service/ChangeGroupService.java`:
- Line 20: ChangeGroupService currently depends on the JPA interface field
jpaGroupRepository (type GroupRepository); refactor it to depend on the
abstraction GroupRepositoryPort instead: replace the field jpaGroupRepository
with a GroupRepositoryPort-typed field, update the constructor to accept
GroupRepositoryPort, and use that port in all methods of ChangeGroupService
(same access pattern as FindGroupService/CreateGroupService/DeleteGroupService)
so the service no longer directly references GroupRepository.
- Line 7: ChangeGroupService currently depends on the concrete
GroupRoleRepositoryAdapter which violates dependency inversion; replace that
concrete dependency with the port interface GroupRoleRepositoryPort by updating
the import, field type, constructor parameter (and any usages) from
GroupRoleRepositoryAdapter to GroupRoleRepositoryPort so the service depends on
the interface (keep the adapter only wired in at composition/root). Ensure all
references inside ChangeGroupService (fields, method signatures, and any casts)
use GroupRoleRepositoryPort and remove direct adapter imports.
In `@src/main/java/flipnote/group/application/service/DeleteGroupService.java`:
- Around line 22-32: DeleteGroupService.deleteGroup is missing a pre-check that
the group exists before calling groupRoleRepository.checkRole, which can cause
NPEs or misleading "not owner" errors; fix by first querying
groupRepository.existsById(cmd.groupId()) or fetching the group (using the same
existence check pattern as ChangeGroupService), throw a clear NotFoundException
(or similar) if absent, and only then call
groupRoleRepository.checkRole(cmd.userId(), cmd.groupId(),
GroupMemberRole.OWNER) before proceeding to
groupRepository.delete(cmd.groupId()).
In `@src/main/java/flipnote/group/application/service/FindGroupService.java`:
- Around line 32-37: Replace the use of IllegalArgumentException for access
checks with a custom runtime exception (e.g., GroupAccessDeniedException) and
throw it from the membership/ownership checks (in FindGroupService where
groupMemberRepository.existsUserInGroup(cmd.groupId(), cmd.userId()) is used,
and likewise in ChangeGroupService and DeleteGroupService where the "not owner"
check occurs); add a simple GroupAccessDeniedException(String message) class and
register an `@ExceptionHandler` (or controller advice) that maps
GroupAccessDeniedException to HTTP 403 Forbidden so these access failures
produce 403 responses instead of 400.
In
`@src/main/java/flipnote/group/infrastructure/persistence/jpa/GroupRoleRepository.java`:
- Line 9: The repository method findByGroupIdAndRole currently returns a
RoleEntity that can be null causing NPE in GroupRoleRepositoryAdapter.checkRole;
change its signature to return Optional<RoleEntity> and update the adapter
(method checkRole) to map the Optional to a boolean by calling
groupMemberRepository.existsByUserIdAndGroupRoleId(userId, roleEntity.getId())
when present and returning false when empty. Ensure to update imports/usages of
findByGroupIdAndRole and adjust any tests or callers accordingly.
🧹 Nitpick comments (10)
src/main/java/flipnote/group/adapter/out/entity/PermissionEntity.java (1)
18-51:groupRoleId를 bareLong이 아닌@ManyToOne관계로 매핑하는 것을 고려해 보세요.현재
groupRoleId를Long으로 직접 매핑하고 있어 JPA의 관계 탐색, cascade, orphan removal 등을 활용할 수 없습니다.RoleEntity에서도 동일한 패턴을 사용 중이므로 프로젝트 컨벤션으로 보이지만, 향후 권한 조회 시 join query가 복잡해질 수 있습니다.src/main/java/flipnote/group/application/service/FindGroupService.java (1)
29-42: 읽기 전용 서비스 메서드에@Transactional(readOnly = true)추가를 고려하세요.두 개의 repository 호출(
existsUserInGroup+findById)이 하나의 트랜잭션 컨텍스트에서 실행되지 않아, 멤버 확인 후 그룹 조회 사이에 데이터 불일치가 발생할 수 있습니다. 다른 서비스(ChangeGroupService,CreateGroupService)에는@Transactional이 적용되어 있어 일관성 측면에서도 권장합니다.♻️ `@Transactional`(readOnly = true) 추가
`@Override` + `@Transactional`(readOnly = true) public FindGroupResult findGroup(FindGroupCommand cmd) {src/main/java/flipnote/group/application/service/CreateGroupService.java (1)
38-43:groupRoleRepository.create(groupId)의 반환값 의미가 명확하지 않습니다.
create(groupId)가 모든 기본 역할을 생성하고 OWNER 역할의 ID를 반환한다면, 메서드명이 이를 명확히 표현하지 못합니다.createDefaultRoles(groupId)또는 반환 타입을 명시적으로 하는 것이 좋습니다.♻️ 메서드명 개선 제안
포트 인터페이스와 어댑터에서:
- Long create(Long groupId); + Long createDefaultRolesAndGetOwnerRoleId(Long groupId);또는 좀 더 간결하게:
- Long create(Long groupId); + Long createDefaultRoles(Long groupId);src/main/java/flipnote/group/infrastructure/persistence/jpa/GroupMemberRepository.java (1)
8-8: 파라미터 이름id가 모호합니다.Spring Data JPA 파생 쿼리 메서드에서
GroupRoleId를 매핑하므로 파라미터 이름도groupRoleId로 일치시키는 것이 가독성에 좋습니다.♻️ 제안
- boolean existsByUserIdAndGroupRoleId(Long userId, Long id); + boolean existsByUserIdAndGroupRoleId(Long userId, Long groupRoleId);src/main/java/flipnote/group/application/port/out/GroupRoleRepositoryPort.java (1)
5-9: 메서드 이름과 반환값의 의미가 명확하지 않습니다.
create(Long groupId)→ 그룹의 모든 기본 역할을 생성하고 OWNER의 roleId를 반환합니다. 메서드명과 반환값만으로는 이 동작을 추론하기 어렵습니다.createDefaultRoles같은 이름이 더 의도를 잘 전달합니다.checkRole→ boolean 반환 메서드는hasRole이 더 관용적입니다.♻️ 제안
public interface GroupRoleRepositoryPort { - Long create(Long groupId); + Long createDefaultRoles(Long groupId); - boolean checkRole(Long userId, Long groupId, GroupMemberRole groupMemberRole); + boolean hasRole(Long userId, Long groupId, GroupMemberRole groupMemberRole); }src/main/java/flipnote/group/adapter/out/persistence/GroupRoleRepositoryAdapter.java (2)
52-61: 인라인 전체 경로 참조 대신 import 문을 사용하세요.Line 58에서
java.util.stream.Collectors를 인라인으로 사용하고 있습니다. 파일 상단에 import 문으로 선언하는 것이 일관성 있고 가독성이 좋습니다.♻️ 제안
파일 상단 import 추가:
import java.util.stream.Collectors;.collect(java.util.stream.Collectors.toMap( + .collect(Collectors.toMap(
27-42: OWNER, HEAD_MANAGER, MANAGER의 기본 권한이 동일합니다.세 역할 모두
KICK과JOIN_REQUEST_MANAGE권한을 갖고 있어 역할 간 차별화가 없습니다. 의도된 것이라면 무시하셔도 되지만, 향후 역할별 권한을 다르게 설정할 계획이라면 TODO 주석을 남겨두는 것을 권장합니다.src/main/java/flipnote/group/adapter/out/persistence/mapper/GroupMemberMapper.java (1)
10-15:@Component가 불필요합니다.모든 메서드가
static이고 생성자가private이므로 Spring이 이 클래스를 빈으로 인스턴스화할 수 없습니다.@Component어노테이션을 제거하는 것이 좋습니다.♻️ 수정 제안
-@Component `@NoArgsConstructor`(access = AccessLevel.PRIVATE) public class GroupMemberMapper {src/main/java/flipnote/group/adapter/out/entity/GroupMemberEntity.java (2)
53-58: Javadoc에groupRoleId파라미터 설명이 누락되어 있습니다.
@param groupRoleId가 빠져 있고,@return설명도 비어 있습니다.♻️ 수정 제안
/** * 멤버 생성 * `@param` groupId * `@param` userId + * `@param` groupRoleId - * `@return` + * `@return` GroupMemberEntity */
43-44: 의도적 설계 선택인 것 같으나, DB 레벨 FK 제약 조건 추가를 검토해보세요.현재 코드는
@JoinColumn이나@ManyToOne없이Long타입으로 외래 키를 관리하고 있습니다. 같은 패턴이RoleEntity(groupId),PermissionEntity(groupRoleId),GroupEntity(groupId) 등 전체 엔티티에 일관되게 적용되어 있어 의도적 설계로 보입니다.다만 현재 설정에서는:
- JPA 레벨의 FK 제약이 없고
- DB 스키마 파일이 발견되지 않아 Hibernate auto-generated 스키마에도 명시적 FK 제약이 없을 가능성이 높습니다
애플리케이션 로직(예:
groupRoleRepository.findByGroupIdAndRole())으로 참조 정합성을 검증하고 있으나, DB 레벨에서도 FK 제약을 정의하면 외부 시스템이나 직접 쿼리를 통한 데이터 불일치를 더욱 견고하게 방지할 수 있습니다. 느슨한 결합과 DB 무결성 중 어느 것을 우선시할지 팀과 함께 검토하는 것을 권장합니다.
src/main/java/flipnote/group/application/service/ChangeGroupService.java
Show resolved
Hide resolved
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check GroupRepository usage in service layer files
rg "GroupRepository" --type=java -g '*/application/service/*' -nRepository: FlipNoteTeam/FlipNote-Group
Length of output: 219
🏁 Script executed:
#!/bin/bash
# Find and examine ChangeGroupService
fd "ChangeGroupService.java" --type fRepository: FlipNoteTeam/FlipNote-Group
Length of output: 143
🏁 Script executed:
#!/bin/bash
# Find GroupRepositoryPort to verify the abstraction exists
fd "GroupRepositoryPort" --type fRepository: FlipNoteTeam/FlipNote-Group
Length of output: 145
🏁 Script executed:
#!/bin/bash
# Check the structure of application/service directory
fd . --type f -g "*/application/service/*.java" | head -20Repository: FlipNoteTeam/FlipNote-Group
Length of output: 186
🏁 Script executed:
#!/bin/bash
# List service files
ls -la src/main/java/flipnote/group/application/service/Repository: FlipNoteTeam/FlipNote-Group
Length of output: 472
🏁 Script executed:
#!/bin/bash
# Examine ChangeGroupService
cat -n src/main/java/flipnote/group/application/service/ChangeGroupService.javaRepository: FlipNoteTeam/FlipNote-Group
Length of output: 1893
🏁 Script executed:
#!/bin/bash
# Examine GroupRepositoryPort
cat -n src/main/java/flipnote/group/application/port/out/GroupRepositoryPort.javaRepository: FlipNoteTeam/FlipNote-Group
Length of output: 376
🏁 Script executed:
#!/bin/bash
# Check FindGroupService
cat -n src/main/java/flipnote/group/application/service/FindGroupService.javaRepository: FlipNoteTeam/FlipNote-Group
Length of output: 1667
🏁 Script executed:
#!/bin/bash
# Check CreateGroupService to verify the architectural pattern
cat -n src/main/java/flipnote/group/application/service/CreateGroupService.javaRepository: FlipNoteTeam/FlipNote-Group
Length of output: 1865
🏁 Script executed:
#!/bin/bash
# Check DeleteGroupService as well
cat -n src/main/java/flipnote/group/application/service/DeleteGroupService.javaRepository: FlipNoteTeam/FlipNote-Group
Length of output: 1385
GroupRepositoryPort를 통해 접근하도록 리팩토링이 필요합니다.
ChangeGroupService가 JPA 인터페이스인 GroupRepository를 직접 참조하고 있는데, FindGroupService, CreateGroupService, DeleteGroupService에서는 모두 GroupRepositoryPort를 통해 접근하고 있습니다. 아키텍처 일관성을 위해 동일한 패턴으로 통일해야 합니다.
🤖 Prompt for AI Agents
In `@src/main/java/flipnote/group/application/service/ChangeGroupService.java` at
line 20, ChangeGroupService currently depends on the JPA interface field
jpaGroupRepository (type GroupRepository); refactor it to depend on the
abstraction GroupRepositoryPort instead: replace the field jpaGroupRepository
with a GroupRepositoryPort-typed field, update the constructor to accept
GroupRepositoryPort, and use that port in all methods of ChangeGroupService
(same access pattern as FindGroupService/CreateGroupService/DeleteGroupService)
so the service no longer directly references GroupRepository.
src/main/java/flipnote/group/application/service/DeleteGroupService.java
Show resolved
Hide resolved
src/main/java/flipnote/group/infrastructure/persistence/jpa/GroupRoleRepository.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
`@src/main/java/flipnote/group/adapter/out/persistence/GroupRoleRepositoryAdapter.java`:
- Around line 84-91: The checkRole method in GroupRoleRepositoryAdapter
currently throws an exception when groupRoleRepository.findByGroupIdAndRole(...)
is empty, which breaks the boolean-based control flow; change it so that if the
RoleEntity is not found the method returns false instead of throwing.
Concretely, locate checkRole and replace the orElseThrow flow with a safe lookup
that returns false when roleEntity is absent and only calls
groupMemberRepository.existsByUserIdAndGroupRoleId(userId, roleEntity.getId())
when present. Ensure the method signature and return semantics remain boolean
and no exception is thrown for a missing role.
- Around line 49-73: The create(Long groupId) method in
GroupRoleRepositoryAdapter performs multiple repository saves
(groupRoleRepository.save(...) and groupRolePermissionRepository.saveAll(...))
but lacks transactional protection; annotate the create method (or the
GroupRoleRepositoryAdapter class) with `@Transactional` (from
org.springframework.transaction.annotation.Transactional) so the role saves and
permission saves are executed in one transaction and will rollback together on
failure (optionally specify rollbackFor=Exception.class if desired); ensure
imports are added and existing callers are compatible with adding transactional
behavior.
🧹 Nitpick comments (2)
src/main/java/flipnote/group/application/service/ChangeGroupService.java (1)
37-43: 소유자 검증 로직 추가는 좋으나, 에러 처리 개선이 필요합니다.
IllegalArgumentException은 잘못된 인자에 대한 예외이므로, 권한 부족 상황에는 의미적으로 적합하지 않습니다. TODO 주석(Line 40)에서도 인지하고 계시므로, 전용 권한 예외(예:AccessDeniedException또는 커스텀 예외)로 조기 전환을 권장합니다.추가로,
checkRole이 내부적으로orElseThrow를 호출하므로, 존재하지 않는 역할에 대해IllegalArgumentException("not role")이 먼저 던져질 수 있습니다. 의미 있는 에러 메시지 구분이 필요합니다.권한 부족 전용 커스텀 예외 클래스를 생성해 드릴까요?
src/main/java/flipnote/group/adapter/out/persistence/GroupRoleRepositoryAdapter.java (1)
52-61: 스트림 생성 간소화 및 import 정리
Arrays.stream(new GroupMemberRole[]{...})보다Stream.of(...)가 간결하고,java.util.stream.Collectors는 fully qualified 대신 import 처리하는 것이 관례에 맞습니다.♻️ 수정 제안
파일 상단 import 추가:
import java.util.stream.Collectors; import java.util.stream.Stream;메서드 내 코드:
- Map<GroupMemberRole, Long> roleIdByRole = Arrays.stream(new GroupMemberRole[]{ - GroupMemberRole.OWNER, - GroupMemberRole.HEAD_MANAGER, - GroupMemberRole.MANAGER, - GroupMemberRole.MEMBER - }) - .collect(java.util.stream.Collectors.toMap( + Map<GroupMemberRole, Long> roleIdByRole = Stream.of( + GroupMemberRole.OWNER, + GroupMemberRole.HEAD_MANAGER, + GroupMemberRole.MANAGER, + GroupMemberRole.MEMBER + ) + .collect(Collectors.toMap( role -> role, role -> groupRoleRepository.save(RoleEntity.create(groupId, role)).getId() ));
src/main/java/flipnote/group/adapter/out/persistence/GroupRoleRepositoryAdapter.java
Show resolved
Hide resolved
src/main/java/flipnote/group/adapter/out/persistence/GroupRoleRepositoryAdapter.java
Show resolved
Hide resolved
1 participant
Summary by CodeRabbit
New Features
Refactor
Removed