Conversation
…into feat/add-member-role
Walkthrough이 PR은 그룹 멤버십 권한 시스템을 enum 기반에서 데이터베이스 관리형 역할로 마이그레이션합니다. RoleEntity와 PermissionEntity를 도입하고, GroupMemberEntity를 refactor하여 groupRoleId를 사용하도록 변경하며, 여러 서비스에 소유권 검증 로직을 추가합니다. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant CreateGroupService
participant GroupRepository
participant GroupRoleRepositoryAdapter
participant RoleRepository
participant PermissionRepository
participant GroupMemberRepositoryAdapter
participant GroupMemberRepository
User->>CreateGroupService: 그룹 생성 요청
CreateGroupService->>GroupRepository: 그룹 저장
GroupRepository-->>CreateGroupService: 그룹ID
CreateGroupService->>GroupRoleRepositoryAdapter: create(groupId)
GroupRoleRepositoryAdapter->>RoleRepository: OWNER/HEAD_MANAGER/MANAGER/MEMBER 4개 역할 생성
RoleRepository-->>GroupRoleRepositoryAdapter: 역할ID들
GroupRoleRepositoryAdapter->>PermissionRepository: 각 역할별 권한 생성
PermissionRepository-->>GroupRoleRepositoryAdapter: 권한ID들
GroupRoleRepositoryAdapter-->>CreateGroupService: ownerRoleId
CreateGroupService->>GroupMemberRepositoryAdapter: save(groupId, userId, ownerRoleId)
GroupMemberRepositoryAdapter->>GroupMemberRepository: 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 소유자 확인됨
GroupRoleRepositoryAdapter-->>ChangeGroupService: true
ChangeGroupService->>GroupRepository: 그룹 수정
GroupRepository-->>ChangeGroupService: 수정 완료
ChangeGroupService-->>User: 수정 성공
else 소유자 아님
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)
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 무결성 중 어느 것을 우선시할지 팀과 함께 검토하는 것을 권장합니다.
| @Table(name = "group_roles", | ||
| uniqueConstraints = @UniqueConstraint( | ||
| name = "uk_group_roles_group_name", | ||
| columnNames = {"group_id", "role"} | ||
| ) | ||
| ) |
There was a problem hiding this comment.
유니크 제약 조건의 컬럼 이름이 실제 컬럼 이름과 불일치합니다.
Line 22의 columnNames = {"group_id", "role"}에서 "role"을 참조하지만, Line 37의 @Column(name = "group_role")에 의해 실제 DB 컬럼 이름은 "group_role"입니다. DDL 생성 시 오류가 발생하거나 제약 조건이 의도대로 동작하지 않습니다.
또한 제약 조건 이름 uk_group_roles_group_name에서 group_name은 실제 제약 대상과 관련이 없어 혼동됩니다.
🐛 수정 제안
`@Table`(name = "group_roles",
uniqueConstraints = `@UniqueConstraint`(
- name = "uk_group_roles_group_name",
- columnNames = {"group_id", "role"}
+ name = "uk_group_roles_group_role",
+ columnNames = {"group_id", "group_role"}
)
)Also applies to: 36-38
🤖 Prompt for AI Agents
In `@src/main/java/flipnote/group/adapter/out/entity/RoleEntity.java` around lines
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.
| import org.springframework.transaction.annotation.Transactional; | ||
|
|
||
| import flipnote.group.adapter.out.entity.GroupEntity; | ||
| import flipnote.group.adapter.out.persistence.GroupRoleRepositoryAdapter; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, locate and read the ChangeGroupService.java file
find . -name "ChangeGroupService.java" -type fRepository: FlipNoteTeam/FlipNote-Group
Length of output: 145
🏁 Script executed:
#!/bin/bash
# Find GroupRoleRepositoryPort and GroupRoleRepositoryAdapter files
find . -name "GroupRoleRepository*.java" -type fRepository: FlipNoteTeam/FlipNote-Group
Length of output: 325
🏁 Script executed:
#!/bin/bash
# Look for the application service directory structure
find . -path "*/flipnote/group/application/service/*" -name "*.java" -type fRepository: FlipNoteTeam/FlipNote-Group
Length of output: 368
🏁 Script executed:
#!/bin/bash
# Read ChangeGroupService.java to see imports and field declarations
head -30 ./src/main/java/flipnote/group/application/service/ChangeGroupService.javaRepository: FlipNoteTeam/FlipNote-Group
Length of output: 1138
🏁 Script executed:
#!/bin/bash
# Read GroupRoleRepositoryPort to verify the interface and methods
cat -n ./src/main/java/flipnote/group/application/port/out/GroupRoleRepositoryPort.javaRepository: FlipNoteTeam/FlipNote-Group
Length of output: 393
🏁 Script executed:
#!/bin/bash
# Read CreateGroupService to compare the correct pattern
head -30 ./src/main/java/flipnote/group/application/service/CreateGroupService.javaRepository: FlipNoteTeam/FlipNote-Group
Length of output: 1134
🏁 Script executed:
#!/bin/bash
# Check for all adapter imports in the service layer
rg "import flipnote.group.adapter" ./src/main/java/flipnote/group/application/service/ -nRepository: FlipNoteTeam/FlipNote-Group
Length of output: 908
Application 서비스 계층에서 포트 인터페이스 대신 구체적인 어댑터를 직접 의존하고 있습니다.
ChangeGroupService가 GroupRoleRepositoryAdapter(구현체)를 직접 참조하고 있어 헥사고날 아키텍처의 의존성 역전 원칙을 위반합니다. 동일한 기능을 수행하는 CreateGroupService는 GroupRoleRepositoryPort 포트 인터페이스를 올바르게 사용하고 있습니다.
다음과 같이 변경하십시오:
♻️ 포트 인터페이스 사용으로 변경
-import flipnote.group.adapter.out.persistence.GroupRoleRepositoryAdapter;
+import flipnote.group.application.port.out.GroupRoleRepositoryPort;- private final GroupRoleRepositoryAdapter groupRoleRepository;
+ private final GroupRoleRepositoryPort groupRoleRepository;🤖 Prompt for AI Agents
In `@src/main/java/flipnote/group/application/service/ChangeGroupService.java` at
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.
| @@ -18,6 +18,7 @@ | |||
| public class ChangeGroupService implements ChangeGroupUseCase { | |||
|
|
|||
| private final GroupRepository jpaGroupRepository; | |||
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.
| public void deleteGroup(DeleteGroupCommand cmd) { | ||
|
|
||
| //오너 인지 확인 | ||
| boolean isOwner = groupRoleRepository.checkRole(cmd.userId(), cmd.groupId(), GroupMemberRole.OWNER); | ||
|
|
||
| //오너가 아닐 경우 에러 발생 | ||
| if(!isOwner) { | ||
| throw new IllegalArgumentException("not owner"); | ||
| } | ||
|
|
||
| groupRepository.delete(cmd.groupId()); |
There was a problem hiding this comment.
그룹 존재 여부 확인이 누락되었습니다.
ChangeGroupService에서는 그룹 존재 여부를 먼저 확인한 후 소유자 검증을 수행하지만, DeleteGroupService에서는 이 검증이 빠져 있습니다. 존재하지 않는 그룹에 대해 삭제를 시도하면 checkRole에서 NPE가 발생하거나 (앞서 GroupRoleRepository 리뷰 참조), "not owner"라는 부정확한 에러 메시지가 반환될 수 있습니다.
🐛 제안
`@Override`
`@Transactional`
public void deleteGroup(DeleteGroupCommand cmd) {
+ // 그룹 존재 여부 확인
+ if (!groupRepository.existsById(cmd.groupId())) {
+ throw new IllegalArgumentException("group not Exists");
+ }
//오너 인지 확인
boolean isOwner = groupRoleRepository.checkRole(cmd.userId(), cmd.groupId(), GroupMemberRole.OWNER);📝 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.
| public void deleteGroup(DeleteGroupCommand cmd) { | |
| //오너 인지 확인 | |
| boolean isOwner = groupRoleRepository.checkRole(cmd.userId(), cmd.groupId(), GroupMemberRole.OWNER); | |
| //오너가 아닐 경우 에러 발생 | |
| if(!isOwner) { | |
| throw new IllegalArgumentException("not owner"); | |
| } | |
| groupRepository.delete(cmd.groupId()); | |
| public void deleteGroup(DeleteGroupCommand cmd) { | |
| // 그룹 존재 여부 확인 | |
| if (!groupRepository.existsById(cmd.groupId())) { | |
| throw new IllegalArgumentException("group not Exists"); | |
| } | |
| //오너 인지 확인 | |
| boolean isOwner = groupRoleRepository.checkRole(cmd.userId(), cmd.groupId(), GroupMemberRole.OWNER); | |
| //오너가 아닐 경우 에러 발생 | |
| if(!isOwner) { | |
| throw new IllegalArgumentException("not owner"); | |
| } | |
| groupRepository.delete(cmd.groupId()); |
🤖 Prompt for AI Agents
In `@src/main/java/flipnote/group/application/service/DeleteGroupService.java`
around lines 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()).
| // 유저가 그룹 내에 존재하는지 확인 | ||
| boolean isMember = groupMemberRepository.existsUserInGroup(cmd.groupId(), cmd.userId()); | ||
|
|
||
| if(!isMember) { | ||
| throw new IllegalArgumentException("user not in Group"); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find build configuration files and check for Spring Security dependency
echo "=== Checking for build configuration files ==="
find . -type f \( -name "build.gradle*" -o -name "pom.xml" -o -name "settings.gradle*" \) | head -20
echo -e "\n=== Checking for Spring Security dependency ==="
if [ -f "build.gradle" ] || [ -f "build.gradle.kts" ]; then
fd -e gradle -e kts | xargs grep -i "spring-security\|springframework.security" 2>/dev/null | head -20
fi
if [ -f "pom.xml" ]; then
grep -i "spring-security" pom.xml 2>/dev/null | head -20
fiRepository: FlipNoteTeam/FlipNote-Group
Length of output: 199
🏁 Script executed:
#!/bin/bash
# Find existing exception classes in the codebase
echo "=== Searching for existing custom Exception classes ==="
fd -e java | xargs grep -l "^public class.*Exception" | head -20
echo -e "\n=== Checking for exception handlers/GlobalExceptionHandler ==="
fd -e java | xargs grep -l "ExceptionHandler\|ControllerAdvice\|RestControllerAdvice" | head -20Repository: FlipNoteTeam/FlipNote-Group
Length of output: 190
🏁 Script executed:
#!/bin/bash
# Verify the same issue in other services mentioned
echo "=== Checking ChangeGroupService for IllegalArgumentException usage ==="
fd -e java -p "ChangeGroupService" | xargs grep -A2 -B2 "IllegalArgumentException\|not owner" 2>/dev/null
echo -e "\n=== Checking DeleteGroupService for similar patterns ==="
fd -e java -p "DeleteGroupService" | xargs grep -A2 -B2 "IllegalArgumentException" 2>/dev/nullRepository: FlipNoteTeam/FlipNote-Group
Length of output: 515
🏁 Script executed:
#!/bin/bash
# Check the actual exception handling configuration
echo "=== Looking for exception response mapping configuration ==="
rg -t java "(HttpStatus|status|statusCode)" --type=java | grep -i "exception\|error\|bad\|forbidden" | head -20Repository: FlipNoteTeam/FlipNote-Group
Length of output: 131
🏁 Script executed:
#!/bin/bash
# Find all controller classes and see how they handle exceptions
echo "=== Looking for Controller classes ==="
fd -e java | xargs grep -l "@Controller\|@RestController" | head -10
# Check for any exception handling in those controllers
echo -e "\n=== Checking for exception handling in controllers ==="
fd -e java | xargs grep -l "@ExceptionHandler" 2>/dev/null
# Look for any try-catch blocks or exception handling patterns
echo -e "\n=== Looking for exception handling patterns ==="
rg -t java "@ExceptionHandler|@ResponseStatus|ResponseEntity" | head -20Repository: FlipNoteTeam/FlipNote-Group
Length of output: 1274
🏁 Script executed:
#!/bin/bash
# Check the actual FindGroupService implementation more carefully
echo "=== Full FindGroupService content ==="
fd -e java -p "FindGroupService" | xargs cat
# Check ChangeGroupService for comparison
echo -e "\n=== ChangeGroupService context ==="
fd -e java -p "ChangeGroupService" | xargs head -50Repository: FlipNoteTeam/FlipNote-Group
Length of output: 2934
🏁 Script executed:
#!/bin/bash
# Search for build.gradle content to confirm Spring Security
echo "=== build.gradle content ==="
cat build.gradle | head -100Repository: FlipNoteTeam/FlipNote-Group
Length of output: 2495
🏁 Script executed:
#!/bin/bash
# Check GroupController implementation
echo "=== GroupController full content ==="
cat src/main/java/flipnote/group/adapter/in/web/GroupController.javaRepository: FlipNoteTeam/FlipNote-Group
Length of output: 4236
🏁 Script executed:
#!/bin/bash
# Search for any exception handling configuration or shared infrastructure
echo "=== Looking for config directory and exception handling ==="
find . -type d -name "config" | head -10
echo -e "\n=== Searching for any exception handlers in the project ==="
rg -t java "throws|catch|try" | grep -i "exception\|error" | head -20
echo -e "\n=== Looking for common exception patterns ==="
rg -t java "RuntimeException|Exception.*extends" | head -20Repository: FlipNoteTeam/FlipNote-Group
Length of output: 394
🏁 Script executed:
#!/bin/bash
# Check if there are other microservices or shared modules
echo "=== Project structure overview ==="
ls -la
echo -e "\n=== Looking for settings.gradle to understand module structure ==="
cat settings.gradleRepository: FlipNoteTeam/FlipNote-Group
Length of output: 901
권한 검증 실패에 IllegalArgumentException을 사용하는 것은 부적절합니다.
"user not in Group" 상황은 잘못된 입력이 아닌 접근 권한 부족입니다. IllegalArgumentException은 HTTP 400 Bad Request로 매핑되지만, 이 경우 HTTP 403 Forbidden이 적절합니다.
이 패턴은 ChangeGroupService (Line 40: "not owner")와 DeleteGroupService에서도 동일하게 발견됩니다.
권장사항: 다음과 같이 커스텀 예외를 정의하고 사용하세요:
커스텀 예외 정의 및 적용
// 새로운 예외 클래스
public class GroupAccessDeniedException extends RuntimeException {
public GroupAccessDeniedException(String message) {
super(message);
}
}- if(!isMember) {
- throw new IllegalArgumentException("user not in Group");
- }
+ if(!isMember) {
+ throw new GroupAccessDeniedException("user not in Group");
+ }추가로 이 예외를 HTTP 403으로 매핑하는 @ExceptionHandler를 작성하세요.
🤖 Prompt for AI Agents
In `@src/main/java/flipnote/group/application/service/FindGroupService.java`
around lines 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.
| import flipnote.group.domain.model.member.GroupMemberRole; | ||
|
|
||
| public interface GroupRoleRepository extends JpaRepository<RoleEntity, Long> { | ||
| RoleEntity findByGroupIdAndRole(Long groupId, GroupMemberRole groupMemberRole); |
There was a problem hiding this comment.
findByGroupIdAndRole가 null을 반환할 수 있어 NPE 위험이 있습니다.
Spring Data JPA에서 엔티티를 직접 반환하는 경우 결과가 없으면 null이 반환됩니다. GroupRoleRepositoryAdapter.checkRole(line 86~88)에서 roleEntity.getId()를 바로 호출하므로, 잘못된 groupId/role 조합이 전달되면 NullPointerException이 발생합니다.
Optional<RoleEntity>로 반환하고 호출부에서 안전하게 처리하세요.
🐛 제안
- RoleEntity findByGroupIdAndRole(Long groupId, GroupMemberRole groupMemberRole);
+ Optional<RoleEntity> findByGroupIdAndRole(Long groupId, GroupMemberRole groupMemberRole);GroupRoleRepositoryAdapter.checkRole에서도 다음과 같이 수정:
`@Override`
public boolean checkRole(Long userId, Long groupId, GroupMemberRole groupMemberRole) {
return groupRoleRepository.findByGroupIdAndRole(groupId, groupMemberRole)
.map(roleEntity -> groupMemberRepository.existsByUserIdAndGroupRoleId(userId, roleEntity.getId()))
.orElse(false);
}📝 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.
| RoleEntity findByGroupIdAndRole(Long groupId, GroupMemberRole groupMemberRole); | |
| Optional<RoleEntity> findByGroupIdAndRole(Long groupId, GroupMemberRole groupMemberRole); |
🤖 Prompt for AI Agents
In
`@src/main/java/flipnote/group/infrastructure/persistence/jpa/GroupRoleRepository.java`
at 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.
1 participant
Summary by CodeRabbit
릴리스 노트
Refactor
Removed