Feat: [FN-117] 그룹 수정 API 추가 #34
Conversation
Walkthrough그룹 수정 PUT 엔드포인트(/v1/groups/{groupId}), DTO(GroupPutRequest/GroupPutResponse), 서비스(changeGroup)와 분산 락 기반 정책 서비스(GroupPolicyService), 엔티티 변경 메서드 및 관련 검증/에러코드와 단위 테스트를 추가. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client as 클라이언트
participant Controller as GroupController
participant Service as GroupService
participant Policy as GroupPolicyService
participant Repo as GroupRepository
Note over Policy: 분산 락(Redisson) + DB for update
Client->>Controller: PUT /v1/groups/{groupId}\nGroupPutRequest
Controller->>Service: changeGroup(auth, req, groupId)
Service->>Policy: changeGroup(groupId, req)
Policy->>Policy: acquire distributed lock\n(tryLock(...))
alt lock acquired
Policy->>Repo: findByIdForUpdate(groupId)
Repo-->>Policy: Group (row-locked)
Policy->>Policy: validateMaxMemberUpdatable(req.maxMember)
Policy->>Policy: group.changeGroup(req)
Policy-->>Service: updated Group
Policy->>Policy: release lock
else lock not acquired
Policy-->>Service: throw SERVICE_TEMPORARILY_UNAVAILABLE
end
Service-->>Controller: GroupPutResponse.from(group)
Controller-->>Client: 200 OK
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (13)
src/main/java/project/flipnote/group/exception/GroupErrorCode.java (1)
18-19: 에러 메시지 문구 수정 제안: 규칙을 더 명확하게 표현현재 메시지("그룹 내에 인원수보다 많게 수정해야합니다.")는 문법적으로 부자연스럽고, 실제 검증 규칙(현재 memberCount 이하로 낮출 수 없음)과의 매핑이 모호합니다. 사용자가 즉시 이해할 수 있도록 아래처럼 수정하는 것을 권장합니다.
- INVALID_MEMBER_COUNT(HttpStatus.BAD_REQUEST, "GROUP_008", "그룹 내에 인원수보다 많게 수정해야합니다."); + INVALID_MEMBER_COUNT(HttpStatus.BAD_REQUEST, "GROUP_008", "최대 인원 수는 현재 인원 수보다 크거나 같아야 합니다.");
- "현재 인원 수" 용어를 사용해 상태 의존 제약임을 분명히 합니다.
- 다른 코드 메시지들과 동일하게 존댓말/마침표 스타일을 유지합니다.
src/main/java/project/flipnote/group/entity/Group.java (2)
17-29: 불필요한 import 제거
jakarta.validation.Valid가 사용되지 않습니다. 컴파일러 경고 제거와 가독성을 위해 삭제하세요.-import jakarta.validation.Valid; import jakarta.validation.constraints.Max; import jakarta.validation.constraints.Min; import jakarta.validation.constraints.NotBlank; +import project.flipnote.group.model.GroupPutRequest;
103-112: 엔터티 변경 메서드 매핑은 적절함 + DTO 필드명과의 의미 정합성 확인
- 필드 할당 로직은 직관적이고 누락 없음 확인했습니다.
- DTO의
image를 엔터티imageUrl에 매핑하는 부분은 의미상 약간의 불일치가 있으니, 장기적으로 DTO 필드명을imageUrl로 통일하는 것을 고려해 주세요. 당장은 서비스/컨트롤러 검증이 있으므로 기능적 문제는 없습니다.src/main/java/project/flipnote/group/controller/GroupController.java (1)
9-9: 중복 import 제거
PathVariableimport가 중복되었습니다. 하나를 제거하세요.-import org.springframework.web.bind.annotation.PathVariable;src/main/java/project/flipnote/group/model/GroupPutRequest.java (2)
26-27: maxMember 요청단 유효성 강화를 위한 범위 제약 추가서비스 레이어에서 1~100 범위를 재검증하지만, 요청단 Bean Validation으로 조기 차단하면 불필요한 서비스 로직 진입을 줄일 수 있습니다.
- @NotNull - Integer maxMember, + @NotNull + @Min(1) + @Max(100) + Integer maxMember,상단 import도 추가해주세요.
+import jakarta.validation.constraints.Max; +import jakarta.validation.constraints.Min;
29-31: 필드명 일치 여부 검토 (선택 사항)DTO
image가 엔터티의imageUrl로 매핑됩니다. API 일관성을 위해 DTO 필드명을imageUrl로 변경하는 방안을 고려해 주세요. 변경 시 API 호환성 영향 범위를 반드시 점검해야 하므로, 버전 태깅 또는 점진적 마이그레이션을 권장합니다.src/main/java/project/flipnote/group/service/GroupService.java (2)
57-57: 주석 오타/맥락 수정여기는 "그룹 내 유저 조회" 섹션 직전인데 주석이
//그룹 생성으로 표기되어 있습니다. 혼동을 줄이기 위해 주석을 정정하세요.- //그룹 생성 + //그룹 내 유저 조회
171-176: 메서드 명세 명확화 제안: 의도를 드러내는 이름현재
validateUserCount는 "현재 memberCount > 요청 maxMember 인지"를 검증합니다. 의미가 더 잘 드러나도록 네이밍을 고려해 주세요.-private void validateUserCount(Group group, int maxMember) { +private void validateMaxMemberNotLowerThanCurrentCount(Group group, int maxMember) { if (group.getMemberCount() > maxMember) { throw new BizException(GroupErrorCode.INVALID_MEMBER_COUNT); } }호출부도 함께 변경 필요합니다.
src/main/java/project/flipnote/group/model/GroupPutResponse.java (2)
8-26: 응답에 groupId 포함 고려(클라이언트 식별/리다이렉트/추가 호출 제거에 유용)PUT 응답이 리소스 식별자(id)를 포함하면, 클라이언트가 별도 상태를 들고 있지 않아도 후속 동작(디테일 조회 링크 구성, 캐시 키 갱신 등)에 유리합니다. 현재 Detail 응답에도 id가 없다면 팀 컨벤션에 따르되, 가능하면 표준화 관점에서 id를 노출하는 쪽을 권장합니다. 아래처럼 최소 변경으로 필드 1개와 매핑 1줄만 추가하면 됩니다.
public record GroupPutResponse( + Long groupId, String name, Category category, String description, Boolean applicationRequired, Boolean publicVisible, Integer maxMember, String imageUrl, LocalDateTime createdAt, LocalDateTime modifiedAt ) { public static GroupPutResponse from(Group group) { return new GroupPutResponse( + group.getId(), group.getName(), group.getCategory(), group.getDescription(), group.getApplicationRequired(), group.getPublicVisible(), group.getMaxMember(), group.getImageUrl(), group.getCreatedAt(), group.getModifiedAt() ); } }Also applies to: 27-39
8-26: DTO 중복 제거 및 매핑 로직 공통화 제안현재
GroupPutResponse와GroupDetailResponse는
- 필드 정의(이름, 카테고리, 설명, 신청 필요 여부, 공개 여부, 최대 인원, 이미지 URL, 생성·수정 시각)
public static … from(Group group)매핑 로직
모두 1:1로 완전히 동일하게 중복되고 있습니다.유지보수 비용과 리팩터링 내성을 높이기 위해 아래 방안을 검토해 주세요:
공통 DTO 정의
src/main/java/project/flipnote/group/model/GroupResponse.java등으로 통합 Record 생성- 기존
GroupPutResponse,GroupDetailResponse는 제거하거나, 필요 시 어댑터 역할만 수행공통 매퍼 유틸리티 추출
src/main/java/project/flipnote/group/mapper/GroupMappers.java에public class GroupMappers { public static GroupResponse toResponse(Group group) { return new GroupResponse( group.getName(), group.getCategory(), group.getDescription(), group.isApplicationRequired(), group.isPublicVisible(), group.getMaxMember(), group.getImageUrl(), group.getCreatedAt(), group.getModifiedAt() ); } }- 각 DTO의
from(...)메서드는 위 유틸만 호출만약 엔드포인트별로 타입 구분이 꼭 필요하다면
GroupPutResponse.from(...)과GroupDetailResponse.from(...)에선처럼 공통 매퍼 호출만 남기고 중복 로직 제거public static GroupPutResponse from(Group group) { return GroupMappers.toResponse(group); }위 리팩터링으로 다음 위치의 중복을 제거할 수 있습니다:
src/main/java/project/flipnote/group/model/GroupPutResponse.java(static from 메서드)src/main/java/project/flipnote/group/model/GroupDetailResponse.java(static from 메서드)src/test/java/project/flipnote/group/service/GroupServiceTest.java (3)
346-377: 권한 실패 시 상태 불변성 및 부수 효과 없음을 명시적으로 검증예외 타입/코드 검증은 좋습니다. 여기에 엔터티 변경이 없었음을 단언하고, 영속 레이어에 쓰기가 발생하지 않았음을 검증하면 더 안전합니다.
BizException exception = assertThrows(BizException.class, () -> groupService.changeGroup(authPrinciple, req, group.getId())); assertEquals(GroupErrorCode.USER_NOT_PERMISSION, exception.getErrorCode()); + // 상태 불변성 + assertEquals(Category.IT, group.getCategory()); + assertEquals(100, group.getMaxMember()); + // 부수 효과 없음 + verify(groupRepository, never()).save(any());
312-344: 응답 DTO 검증 및 저장 호출 검증 추가 권장테스트에서 엔티티 변경만 검증하고 있으므로, 컨트롤러/클라이언트 계약을 보장하기 위해 응답 DTO 필드 매핑과 명시적 저장 전략 확인을 위한 저장 호출 여부 검증을 함께 추가해주세요.
- 파일:
src/test/java/project/flipnote/group/service/GroupServiceTest.java- 메서드:
그룹_수정_성공//when GroupPutResponse res = groupService.changeGroup(authPrinciple, req, group.getId()); //then + assertNotNull(res); + assertEquals(req.name(), res.name()); + assertEquals(req.category(), res.category()); + assertEquals(req.description(), res.description()); + assertEquals(req.applicationRequired(), res.applicationRequired()); + assertEquals(req.publicVisible(), res.publicVisible()); + assertEquals(req.maxMember(), res.maxMember()); + assertEquals(req.imageUrl(), res.imageUrl()); assertEquals(req.name(), group.getName()); assertEquals(req.category(), group.getCategory()); + // 저장 호출 여부 검증 (명시적 save 전략) + verify(groupRepository, times(1)).save(group);
379-411: 그룹 수정 실패 시 상태 불변성 및 부수 효과 검증 추가 제안현재 테스트에서는
INVALID_MEMBER_COUNT예외 코드 검증까지 잘 이루어지고 있습니다.
추가로, 예외 발생 시 그룹의maxMember값이 변경되지 않았음을 확인하고, 저장(save) 호출이 없었음을 검증하는 Assertion을 넣으면 좋겠습니다.BizException exception = assertThrows(BizException.class, () -> groupService.changeGroup(authPrinciple, req, group.getId())); assertEquals(GroupErrorCode.INVALID_MEMBER_COUNT, exception.getErrorCode()); + // 상태 불변성 검증: maxMember가 그대로 유지되어야 함 + assertEquals(100, group.getMaxMember()); + // 부수 효과 없음 검증: save 호출이 없어야 함 + verify(groupRepository, never()).save(any());또한, 경계값 테스트를 보강하면 안정성을 더욱 높일 수 있습니다. 별도 테스트로 다음 케이스를 추가해 보세요:
maxMember == 현재 memberCount(예: 100 → 허용되는지 여부 확인)maxMember < 1(0 또는 음수 입력 시INVALID_MAX_MEMBER예외)maxMember > 시스템 상한(101 이상 입력 시INVALID_MAX_MEMBER예외)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
src/main/java/project/flipnote/group/controller/GroupController.java(3 hunks)src/main/java/project/flipnote/group/entity/Group.java(3 hunks)src/main/java/project/flipnote/group/exception/GroupErrorCode.java(1 hunks)src/main/java/project/flipnote/group/model/GroupPutRequest.java(1 hunks)src/main/java/project/flipnote/group/model/GroupPutResponse.java(1 hunks)src/main/java/project/flipnote/group/service/GroupService.java(5 hunks)src/test/java/project/flipnote/group/service/GroupServiceTest.java(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (7)
src/main/java/project/flipnote/group/model/GroupPutRequest.java (2)
src/main/java/project/flipnote/group/model/GroupCreateRequest.java (1)
GroupCreateRequest(6-29)src/main/java/project/flipnote/group/model/GroupDetailResponse.java (1)
GroupDetailResponse(8-41)
src/main/java/project/flipnote/group/exception/GroupErrorCode.java (3)
src/main/java/project/flipnote/groupjoin/exception/GroupJoinErrorCode.java (2)
Getter(8-26)Override(22-25)src/main/java/project/flipnote/group/exception/GroupInvitationErrorCode.java (2)
Getter(9-25)Override(21-24)src/main/java/project/flipnote/cardset/exception/CardSetErrorCode.java (1)
Getter(9-23)
src/main/java/project/flipnote/group/controller/GroupController.java (3)
src/main/java/project/flipnote/groupjoin/controller/GroupJoinController.java (2)
RestController(14-76)PatchMapping(43-53)src/main/java/project/flipnote/user/controller/UserController.java (1)
PutMapping(36-43)src/main/java/project/flipnote/group/controller/GroupInvitationController.java (2)
RequiredArgsConstructor(23-63)PatchMapping(52-62)
src/main/java/project/flipnote/group/model/GroupPutResponse.java (1)
src/main/java/project/flipnote/group/model/GroupDetailResponse.java (1)
GroupDetailResponse(8-41)
src/main/java/project/flipnote/group/entity/Group.java (2)
src/main/java/project/flipnote/group/model/GroupCreateRequest.java (1)
GroupCreateRequest(6-29)src/main/java/project/flipnote/group/model/GroupDetailResponse.java (1)
GroupDetailResponse(8-41)
src/main/java/project/flipnote/group/service/GroupService.java (2)
src/main/java/project/flipnote/groupjoin/service/GroupJoinService.java (1)
Slf4j(41-286)src/main/java/project/flipnote/group/service/GroupMemberPolicyService.java (1)
RequiredArgsConstructor(22-69)
src/test/java/project/flipnote/group/service/GroupServiceTest.java (1)
src/test/java/project/flipnote/groupjoin/service/GroupJoinServiceTest.java (1)
ExtendWith(35-283)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (3)
src/main/java/project/flipnote/group/controller/GroupController.java (1)
41-49: PUT 그룹 수정 엔드포인트 구조 적절함 (검증 및 위임 흐름 OK)src/main/java/project/flipnote/group/model/GroupPutResponse.java (1)
27-39: DTO 매핑 구현은 명확하고 안전합니다엔터티의 게터를 그대로 위임 받아 불변 DTO로 구성하는 방식이 일관되고 명확합니다. NPE 가능성도 없으며, 직렬화 호환성도 좋습니다.
src/test/java/project/flipnote/group/service/GroupServiceTest.java (1)
36-38: 신규 DTO import 추가 적절수정 플로우 테스트에 필요한 GroupPutRequest/Response import가 정확합니다.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/main/java/project/flipnote/group/service/GroupService.java (1)
178-207: 그룹 수정 시 동시성 경쟁(TOCTOU)으로 무결성 훼손 가능 — 가입/초대와 동일 락/행락 패턴 적용 필요현재는 검증 이후 저장까지 락이 없어, 검증-저장 사이에 멤버 수가 변해
memberCount > maxMember상태가 발생할 수 있습니다. 초대/가입 경로는 Redisson 분산락 + DB 행락을 사용하므로 동일 정책을 본 메서드에도 적용해야 합니다. (이 이슈는 이전 코멘트에서도 이미 지적되었습니다.)아래와 같이 분산락(
group_member_lock:{groupId}) 확보 →findByIdForUpdate로 재조회 → 현재 멤버 수 재검증 → 업데이트 순으로 변경하세요.@@ - //그룹 수정 - @Transactional - public GroupPutResponse changeGroup(AuthPrinciple authPrinciple, @Valid GroupPutRequest req, Long groupId) { - - //1. 유저 조회 - UserProfile user = validateUser(authPrinciple); - - //2. 인원수 검증 - validateMaxMember(req.maxMember()); - - //3. 그룹 가져오기 - Group group = validateGroup(groupId); - - //4. 그룹 내 유저 조회 - GroupMember groupMember = validateGroupInUser(user, groupId); - - //5. 유저 권환 조회 - if (!groupMember.getRole().equals(GroupMemberRole.OWNER)) { - throw new BizException(GroupErrorCode.USER_NOT_PERMISSION); - } - - //4. 유저 수 보다 적게 할 경우 오류 - validateUserCount(group, req.maxMember()); - - group.changeGroup(req); - - groupRepository.save(group); - - return GroupPutResponse.from(group); - } + //그룹 수정 + @Transactional + public GroupPutResponse changeGroup(AuthPrinciple authPrinciple, @Valid GroupPutRequest req, Long groupId) { + // 1) 사전 검증(값 범위) + UserProfile user = validateUser(authPrinciple); + validateMaxMember(req.maxMember()); + + // 2) 동시성 제어 (초대/가입과 동일한 락 키 사용) + String lockKey = "group_member_lock:" + groupId; + RLock lock = redissonClient.getLock(lockKey); + boolean isLocked = false; + try { + isLocked = lock.tryLock(2, 3, TimeUnit.SECONDS); + if (!isLocked) { + throw new BizException(project.flipnote.common.exception.CommonErrorCode.SERVICE_TEMPORARILY_UNAVAILABLE); + } + // 3) 행락으로 최신 상태 재조회 + Group lockedGroup = groupRepository.findByIdForUpdate(groupId) + .orElseThrow(() -> new BizException(GroupErrorCode.GROUP_NOT_FOUND)); + // 4) 그룹 내 유저/권한 재검증 (역할 변경 레이스 대비) + GroupMember lockedMember = validateGroupInUser(user, groupId); + if (!lockedMember.getRole().equals(GroupMemberRole.OWNER)) { + throw new BizException(GroupErrorCode.USER_NOT_PERMISSION); + } + // 5) 현재 인원 수 대비 요청 maxMember 재검증 + validateMaxMemberNotLowerThanCurrentCount(lockedGroup, req.maxMember()); + // 6) 업데이트 + lockedGroup.changeGroup(req); + // JPA 영속 상태이므로 save 호출 불필요 + return GroupPutResponse.from(lockedGroup); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + throw new BizException(project.flipnote.common.exception.CommonErrorCode.SERVICE_TEMPORARILY_UNAVAILABLE); + } finally { + if (isLocked && lock.isHeldByCurrentThread()) { + lock.unlock(); + } + } + }클래스 외 범위에 필요한 추가 코드:
// imports import org.redisson.api.RLock; import org.redisson.api.RedissonClient; import java.util.concurrent.TimeUnit; // 필드 주입 private final RedissonClient redissonClient;보완 제안:
- 조인 경로와 동일하게 “현재 인원 수”의 소스 오브 트루스를 통일하세요(엔티티의 memberCount vs. repository.count). 최소한 같은 트랜잭션/행락 하에서 하나로 일관되게 사용해야 합니다.
🧹 Nitpick comments (5)
src/main/java/project/flipnote/group/service/GroupService.java (5)
171-176: 검증 의도 명확화(메서드명) 및 주석 정합성
validateUserCount는 “현재 인원 수보다 작은 maxMember 설정 불가”를 검사합니다. 의도를 드러내도록 메서드명을 바꾸고, 호출부 주석의 단계 번호도 정리해주세요.@@ - //유저수 검증 - private void validateUserCount(Group group, int maxMember) { + //현재 인원 수보다 작은 maxMember 설정 금지 + private void validateMaxMemberNotLowerThanCurrentCount(Group group, int maxMember) { if (group.getMemberCount() > maxMember) { throw new BizException(GroupErrorCode.INVALID_MEMBER_COUNT); } } @@ - //4. 유저 수 보다 적게 할 경우 오류 - validateUserCount(group, req.maxMember()); + // 유저 수보다 작은 maxMember는 허용하지 않음 + validateMaxMemberNotLowerThanCurrentCount(group, req.maxMember());Also applies to: 199-201
194-197: 오탈자: ‘권환’ → ‘권한’코드 가독성을 위해 주석 오탈자를 수정하세요.
- //5. 유저 권환 조회 + //5. 유저 권한 조회
204-204: 영속 엔티티는 변경 감지로 충분 — save 생략 가능
group/lockedGroup은 영속 상태이므로groupRepository.save(...)는 불필요합니다. 제거하여 트랜잭션 경계를 단순화하세요.- groupRepository.save(group);
185-187: PUT 의미상 문제 없으나, maxMember가 nullable이면 NPE 가능성 — DTO 제약 확인 필요
validateMaxMember(req.maxMember())는req.maxMember()가 null이면 NPE가 납니다. PUT이라면 필수 필드가 맞지만, DTO에서@NotNull이 보장되는지 확인해주세요. PATCH 전환 시엔 필드 존재 여부에 따라 부분 업데이트로 분기해야 합니다.원한다면
GroupPutRequest에 명시적으로@NotNull @Min(1) @Max(100)를 부여하거나, 서비스에서Objects.requireNonNull(req.maxMember(), "...")로 방어 코드를 추가할 수 있습니다.
57-61: 주석 위치/내용 불일치
//그룹 생성주석이 ‘그룹 내 유저 조회’ 메서드 위에 붙어 있어 오독의 여지가 있습니다. 주석을 정정해주세요.- //그룹 생성 + //그룹 내 유저 조회
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/main/java/project/flipnote/group/service/GroupService.java(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/main/java/project/flipnote/group/service/GroupService.java (2)
src/main/java/project/flipnote/group/entity/Group.java (1)
Getter(28-101)src/main/java/project/flipnote/group/service/GroupMemberPolicyService.java (1)
RequiredArgsConstructor(22-69)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (2)
src/main/java/project/flipnote/group/service/GroupService.java (2)
94-96: 주석 명확화 LGTM“그룹 내의 모든 권한 생성”으로 주석이 구체화되어 의미 전달이 더 잘 됩니다.
24-25: DTO import 추가 LGTM
GroupPutRequest/Response도입에 따른 import 추가는 적절합니다.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/main/java/project/flipnote/group/service/GroupService.java (1)
36-41: 서비스 계층 Bean Validation 활성화(@validated) 누락클래스에
@Validated가 없으면 메서드 파라미터에@Valid를 붙여도 서비스 레이어에서 검증이 수행되지 않습니다. 과거 코멘트와 동일 제안 반복입니다.적용 예시:
import org.springframework.transaction.annotation.Transactional; +import org.springframework.validation.annotation.Validated; @Slf4j @Service @RequiredArgsConstructor @Transactional(readOnly = true) -public class GroupService { +@Validated +public class GroupService {
🧹 Nitpick comments (15)
src/test/java/project/flipnote/group/service/GroupPolicyServiceTest.java (4)
58-61: Mockito 인자 매처 타입 구체화 및 락 키 검증 추가 제안
any()대신any(TimeUnit.class)로 명시해 오버로드 모호성을 줄이는 편이 안전합니다.- 분산락 키 계약도 함께 검증하면 회귀를 예방할 수 있습니다(현재 anyString 사용).
적용 예시:
- given(redissonClient.getLock(anyString())).willReturn(rLock); - given(rLock.tryLock(anyLong(), anyLong(), any())).willReturn(true); + given(redissonClient.getLock(anyString())).willReturn(rLock); + given(rLock.tryLock(anyLong(), anyLong(), any(java.util.concurrent.TimeUnit.class))).willReturn(true);테스트 본문 하단에 다음 검증을 추가하세요(선호: ArgumentCaptor 사용 또는 정확한 키 상수 사용):
// 락 키 계약 검증 then(redissonClient).should().getLock("group_lock:" + groupId);
40-70: 실패 케이스에서 unlock 호출 검증은 좋습니다. 성공 케이스에도 동일 검증을 추가하세요.현재 테스트는 실패 케이스에서만
unlock()호출을 검증합니다. 성공 케이스에서도 락 해제가 이루어지는지 검증하면 누수 회귀를 막을 수 있습니다.추가 예시(성공 케이스 끝부분):
then(rLock).should().unlock();
72-102: 성공 케이스의 검증 범위 확장 제안
- 이름/카테고리만 검증하고 있습니다. 변경 가능한 필드(설명, 공개 여부, 신청 필요, maxMember, 이미지 URL)도 함께 검증하면 의미가 큽니다.
getLock키 검증과unlock()호출 검증을 추가하세요.예시:
- assertEquals(req.name(), changeGroup.getName()); - assertEquals(req.category(), changeGroup.getCategory()); + assertEquals(req.name(), changeGroup.getName()); + assertEquals(req.category(), changeGroup.getCategory()); + assertEquals(req.description(), changeGroup.getDescription()); + assertEquals(req.publicVisible(), changeGroup.isPublicVisible()); + assertEquals(req.applicationRequired(), changeGroup.isApplicationRequired()); + assertEquals(req.maxMember(), changeGroup.getMaxMember()); + assertEquals(req.image(), changeGroup.getImageUrl()); + then(redissonClient).should().getLock("group_lock:" + groupId); + then(rLock).should().unlock();
40-70: 락 획득 실패/인터럽트/그룹 없음 분기 테스트 추가 권장정책 서비스는 다음 분기를 갖습니다:
tryLock실패 →SERVICE_TEMPORARILY_UNAVAILABLEfindByIdForUpdate결과 없음 →GROUP_NOT_FOUNDInterruptedException발생 → 인터럽트 복원 후SERVICE_TEMPORARILY_UNAVAILABLE각각을 별도 테스트로 추가하면 회귀 방지가 수월합니다.
테스트 스켈레톤 제공 가능: 원하시면 추가 케이스용 메서드 템플릿을 드리겠습니다.
src/main/java/project/flipnote/group/service/GroupService.java (3)
178-186: DTO 파라미터에 @Valid 추가 또는 import 제거 정리상단에
jakarta.validation.Valid를 import했지만 본 메서드 파라미터에는 적용되지 않았습니다. 컨트롤러에서만 검증한다면 import 제거, 서비스에서 함께 검증한다면@Valid추가 중 하나로 정리하세요.- public GroupPutResponse changeGroup(AuthPrinciple authPrinciple, GroupPutRequest req, Long groupId) { + public GroupPutResponse changeGroup(AuthPrinciple authPrinciple, @Valid GroupPutRequest req, Long groupId) {
171-176: 미사용 유틸 메서드 제거 또는 실제 사용 지점 연결
validateUserCount는 어디에서도 호출되지 않습니다. 정책 서비스 내validateMaxMemberUpdatable로 대체됐다면 제거가 깔끔합니다.- //유저수 검증 - private void validateUserCount(Group group, int maxMember) { - if (group.getMemberCount() > maxMember) { - throw new BizException(GroupErrorCode.INVALID_MEMBER_COUNT); - } - }
194-197: 주석 오타 수정: ‘권환’ → ‘권한’가독성 개선을 위해 주석 오타를 정정하세요.
- //5. 유저 권환 조회 + //5. 유저 권한 조회src/main/java/project/flipnote/group/service/GroupPolicyService.java (3)
26-28: 락 키 네이밍 일관성 유지:GroupMemberPolicyService와 통일 필요현재 키:
"group_lock:" + groupId
다른 경로(멤버 추가):"group_member_lock:" + groupId리소스가 동일(그룹)하므로 같은 키 스페이스를 사용하면 분산락 정책이 일관됩니다. 최소한 상수로 추출하여 중복 타이핑을 제거하세요.
- String lockKey = "group_lock:" + groupId; + final String lockKey = "group_member_lock:" + groupId; // GroupMemberPolicyService와 통일또는:
public class GroupPolicyService { + private static final String GROUP_LOCK_PREFIX = "group_member_lock:"; @@ - String lockKey = "group_lock:" + groupId; + String lockKey = GROUP_LOCK_PREFIX + groupId;
24-53: 락/DB 락의 결합 사용 패턴 적절 — 단, 공통 유틸리티 추출 고려분산락 + 행락 결합은 좋지만, 락 키 생성/타임아웃/예외 변환 로직을 공통 유틸(AOP 혹은 Helper)로 추출하면 중복을 줄일 수 있습니다. 기존
GroupMemberPolicyService에도 동일 적용 가능.
24-53: 분산락 키 일관성 확인 완료 및 상수화 권장검증 결과, 레포지토리 내 Redisson 분산락은
GroupPolicyService및GroupMemberPolicyService(및 관련 테스트)에서만 사용되고 있으며, 모두 동일한 키 접두사"group_lock:"를 사용하고 있음을 확인했습니다.
일관성에는 문제가 없으나, 아래 두 가지를 권장드립니다.
- 핵심 키 접두사
"group_lock:"를 상수(예:GROUP_LOCK_KEY_PREFIX)로 추출하여 오타 방지 및 재사용성 확보- 제공된 스크립트(rg 기반)를 CI 파이프라인에 통합하여, 향후에도 분산락 사용처 전수 점검 자동화
src/test/java/project/flipnote/group/service/GroupServiceTest.java (5)
17-19: 불필요한 import 제거
RLock,RedissonClient는 본 테스트에서 사용되지 않습니다. 정리하세요.-import org.redisson.api.RLock; -import org.redisson.api.RedissonClient;
352-358: 검증 대상을 엔드포인트 반환 DTO로 변경 + 위임 호출 검증 추가서비스의 반환값 매핑을 검증하려면 엔티티가 아닌
GroupPutResponse를 직접 검증하는 것이 더 적합합니다. 또한 정책 서비스 위임이 1회 호출되었는지도 검증하세요.- //when - GroupPutResponse res = groupService.changeGroup(authPrinciple, req, group.getId()); - - //then - assertEquals(req.name(), group.getName()); - assertEquals(req.category(), group.getCategory()); + //when + GroupPutResponse res = groupService.changeGroup(authPrinciple, req, group.getId()); + + //then + assertEquals(req.name(), res.name()); + assertEquals(req.category(), res.category()); + then(groupPolicyService).should().changeGroup(group.getId(), req);
383-391: 권한 실패 시 정책 서비스 미호출 보장 검증 추가권한 체크 이전에 위임이 발생하지 않음을 검증하면 리그레션을 막을 수 있습니다.
BizException exception = assertThrows(BizException.class, () -> groupService.changeGroup(authPrinciple, req, group.getId())); assertEquals(GroupErrorCode.USER_NOT_PERMISSION, exception.getErrorCode()); + then(groupPolicyService).should(never()).changeGroup(anyLong(), any());
421-429: 인원수 제약 실패 케이스에서 위임 호출/예외 흐름 검증 보강예외가 정책 서비스에서 전파되었는지 호출 검증을 추가하세요.
BizException exception = assertThrows(BizException.class, () -> groupService.changeGroup(authPrinciple, req, group.getId())); assertEquals(GroupErrorCode.INVALID_MEMBER_COUNT, exception.getErrorCode()); + then(groupPolicyService).should().changeGroup(group.getId(), req);
317-359: 추가 케이스 제안: 변경 요청의 maxMember 범위 검증
GroupService.changeGroup는 범위를validateMaxMember로 선제 검증합니다. 음수/상한 초과(>100) 요청에 대해INVALID_MAX_MEMBER가 발생하는지 별도 테스트를 추가하면 좋습니다.원하시면 테스트 메서드 템플릿을 제공하겠습니다.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
src/main/java/project/flipnote/group/entity/Group.java(3 hunks)src/main/java/project/flipnote/group/service/GroupPolicyService.java(1 hunks)src/main/java/project/flipnote/group/service/GroupService.java(5 hunks)src/test/java/project/flipnote/group/service/GroupPolicyServiceTest.java(1 hunks)src/test/java/project/flipnote/group/service/GroupServiceTest.java(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/project/flipnote/group/entity/Group.java
🧰 Additional context used
🧬 Code graph analysis (3)
src/main/java/project/flipnote/group/service/GroupPolicyService.java (2)
src/main/java/project/flipnote/group/service/GroupMemberPolicyService.java (2)
RequiredArgsConstructor(22-69)Transactional(31-68)src/main/java/project/flipnote/groupjoin/service/GroupJoinService.java (1)
Transactional(113-152)
src/test/java/project/flipnote/group/service/GroupPolicyServiceTest.java (2)
src/test/java/project/flipnote/group/service/GroupServiceTest.java (1)
ExtendWith(49-430)src/main/java/project/flipnote/group/service/GroupMemberPolicyService.java (1)
RequiredArgsConstructor(22-69)
src/test/java/project/flipnote/group/service/GroupServiceTest.java (1)
src/main/java/project/flipnote/group/service/GroupMemberPolicyService.java (1)
RequiredArgsConstructor(22-69)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (4)
src/main/java/project/flipnote/group/service/GroupService.java (1)
199-203: 정책 서비스로의 위임 구조 적절동시성/검증 책임을
GroupPolicyService로 위임하는 구조는 응집도를 높이고 트랜잭션 경계를 명확히 합니다. 유지하시길 권장합니다.src/main/java/project/flipnote/group/service/GroupPolicyService.java (2)
39-43: 영속성 컨텍스트 변경감지로 충분 — save 호출 불필요트랜잭션 내
findByIdForUpdate로 조회된 엔티티에 대한 변경은 커밋 시 플러시됩니다. 현재 구현은 적절합니다.
49-51: unlock 가드 조건 적절
isLocked && isHeldByCurrentThread조합으로 불필요한IllegalMonitorStateException을 방지합니다. 유지 권장.src/test/java/project/flipnote/group/service/GroupServiceTest.java (1)
164-165: 리포지토리 스텁의 명확성 개선 — LGTM
Optional.ofNullable→Optional.of로 명확히 바뀐 점 좋습니다(테스트 가독성 향상).
📝 변경 내용
✅ 체크리스트
💬 기타 참고 사항
Summary by CodeRabbit