Feat: [FN-301] 그룹 삭제 API 구현 #5
Open
Conversation
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
개요그룹 삭제 기능이 구현되었습니다. 클린 아키텍처의 각 계층을 따라 DeleteGroupUseCase 인터페이스, DeleteGroupCommand 레코드, DeleteGroupService 구현, 그리고 GroupController의 새로운 엔드포인트가 추가되었으며, 저장소 포트와 어댑터도 확장되었습니다. 변경 사항
예상 코드 검토 시간🎯 3 (Moderate) | ⏱️ ~20 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: 3
🤖 Fix all issues with AI agents
In `@src/main/java/flipnote/group/adapter/in/web/GroupController.java`:
- Around line 118-136: Fix the typo in the TODO comment inside GroupController
(in the deleteGroup method of class GroupController): change both occurrences of
"권환" to "권한" so the TODO reads correctly (e.g., "추후 권한 체크 후 권한 확인 후 삭제"),
preserving the comment structure and spacing.
In
`@src/main/java/flipnote/group/adapter/out/persistence/GroupRepositoryAdapter.java`:
- Around line 39-42: The delete method in GroupRepositoryAdapter currently calls
groupRepository.deleteById(groupId) without checking existence, which is
inconsistent with findById's behavior; update GroupRepositoryAdapter.delete(Long
groupId) to first verify the group exists (e.g., use
groupRepository.existsById(groupId) or reuse findById logic) and if not present
throw the same IllegalArgumentException used elsewhere, otherwise call
groupRepository.deleteById(groupId) to perform the deletion so behavior and
client responses remain consistent.
In `@src/main/java/flipnote/group/application/service/DeleteGroupService.java`:
- Around line 10-19: Add transactional and ownership checks to
DeleteGroupService: annotate the class or deleteGroup method with
`@Transactional`, load the group via GroupRepositoryPort (e.g., findById or
getById) inside deleteGroup(DeleteGroupCommand cmd), verify that the fetched
group's ownerId equals cmd.userId, and if not throw an appropriate
access/authorization exception; only call groupRepository.delete(cmd.groupId())
after the existence and ownership checks succeed (and handle group-not-found by
throwing a not-found exception).
🧹 Nitpick comments (2)
src/main/java/flipnote/group/adapter/in/web/MemberController.java (1)
1-18: 스캐폴딩 전용 컨트롤러가 메인 브랜치에 병합됩니다.이 파일은 구현이 전혀 없이 TODO 주석만 포함된 빈 클래스입니다. 그룹 삭제 API PR의 범위에 포함될 필요가 없어 보이며, 해당 기능을 실제 구현할 때 추가하는 것이 더 적절합니다.
만약 의도적으로 포함시킨 것이라면, 최소한
@RestController와@RequestMapping어노테이션을 미리 추가하는 것을 권장합니다.src/main/java/flipnote/group/application/service/DeleteGroupService.java (1)
17-18: 소프트 삭제(Soft Delete) 고려 필요현재 물리적 삭제(
deleteById)를 수행하고 있습니다. 그룹에 속한 멤버, 노트 등 연관 데이터가 있을 경우 데이터 정합성 문제가 발생할 수 있습니다. 소프트 삭제(deleted플래그 또는deletedAt타임스탬프) 도입을 검토해 보세요. 당장이 아니더라도 아키텍처 방향 결정이 필요한 사항입니다.
Comment on lines
+118
to
+136
| /** | ||
| * 그룹 삭제 | ||
| * todo 추후 권환 체크 후 권환 확인 후 삭제 | ||
| * @param userId | ||
| * @param groupId | ||
| * @return | ||
| */ | ||
| @DeleteMapping("/{groupId}") | ||
| public ResponseEntity<Void> deleteGroup( | ||
| @RequestHeader("X-USER-ID") Long userId, | ||
| @PathVariable("groupId") Long groupId | ||
| ) { | ||
|
|
||
| DeleteGroupCommand cmd = new DeleteGroupCommand(userId, groupId); | ||
|
|
||
| deleteGroupUseCase.deleteGroup(cmd); | ||
|
|
||
| return ResponseEntity.noContent().build(); | ||
| } |
There was a problem hiding this comment.
엔드포인트 구조는 적절합니다. TODO 코멘트에 오타가 있습니다.
Line 120: 권환 → 권한 (2회 반복). 사소하지만 나중에 TODO를 검색할 때 혼동을 줄 수 있습니다.
- * todo 추후 권환 체크 후 권환 확인 후 삭제
+ * todo 추후 권한 체크 후 권한 확인 후 삭제🤖 Prompt for AI Agents
In `@src/main/java/flipnote/group/adapter/in/web/GroupController.java` around
lines 118 - 136, Fix the typo in the TODO comment inside GroupController (in the
deleteGroup method of class GroupController): change both occurrences of "권환" to
"권한" so the TODO reads correctly (e.g., "추후 권한 체크 후 권한 확인 후 삭제"), preserving the
comment structure and spacing.
Comment on lines
+39
to
+42
| @Override | ||
| public void delete(Long groupId) { | ||
| groupRepository.deleteById(groupId); | ||
| } |
There was a problem hiding this comment.
존재하지 않는 그룹 삭제 시 예외 처리 부재
Spring Data JPA 3.x의 deleteById는 해당 ID의 엔티티가 존재하지 않아도 예외 없이 무시됩니다. 같은 클래스의 findById는 존재하지 않을 때 IllegalArgumentException을 던지므로 동작이 일관되지 않습니다. 존재하지 않는 그룹에 대해 204를 반환하면 클라이언트에게 잘못된 성공 신호를 줄 수 있습니다.
🔧 존재 확인 후 삭제하는 방식 제안
`@Override`
public void delete(Long groupId) {
- groupRepository.deleteById(groupId);
+ if (!groupRepository.existsById(groupId)) {
+ throw new IllegalArgumentException("Group not Exist");
+ }
+ groupRepository.deleteById(groupId);
}📝 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.
Suggested change
| @Override | |
| public void delete(Long groupId) { | |
| groupRepository.deleteById(groupId); | |
| } | |
| `@Override` | |
| public void delete(Long groupId) { | |
| if (!groupRepository.existsById(groupId)) { | |
| throw new IllegalArgumentException("Group not Exist"); | |
| } | |
| groupRepository.deleteById(groupId); | |
| } |
🤖 Prompt for AI Agents
In
`@src/main/java/flipnote/group/adapter/out/persistence/GroupRepositoryAdapter.java`
around lines 39 - 42, The delete method in GroupRepositoryAdapter currently
calls groupRepository.deleteById(groupId) without checking existence, which is
inconsistent with findById's behavior; update GroupRepositoryAdapter.delete(Long
groupId) to first verify the group exists (e.g., use
groupRepository.existsById(groupId) or reuse findById logic) and if not present
throw the same IllegalArgumentException used elsewhere, otherwise call
groupRepository.deleteById(groupId) to perform the deletion so behavior and
client responses remain consistent.
Comment on lines
+10
to
+19
| @Service | ||
| @RequiredArgsConstructor | ||
| public class DeleteGroupService implements DeleteGroupUseCase { | ||
|
|
||
| private final GroupRepositoryPort groupRepository; | ||
|
|
||
| @Override | ||
| public void deleteGroup(DeleteGroupCommand cmd) { | ||
| groupRepository.delete(cmd.groupId()); | ||
| } |
There was a problem hiding this comment.
@Transactional 누락 및 권한 검증 부재
두 가지 문제가 있습니다:
-
@Transactional누락: 삭제는 쓰기 작업이므로@Transactional어노테이션이 필요합니다. 향후 삭제 로직이 복잡해지면(연관 데이터 정리 등) 트랜잭션 보장이 필수적입니다. -
userId가 사용되지 않음:DeleteGroupCommand에userId가 전달되지만 서비스에서 전혀 참조하지 않습니다. 현재 상태에서는 어떤 사용자든 임의의 그룹을 삭제할 수 있어 보안 위험이 있습니다. TODO로 남겨두셨지만, 최소한 그룹 소유자인지 확인하는 로직은 이 PR에 포함하는 것을 권장합니다.
🔧 `@Transactional` 추가 및 소유자 검증 제안
package flipnote.group.application.service;
+import org.springframework.transaction.annotation.Transactional;
import org.springframework.stereotype.Service;
import flipnote.group.application.port.in.DeleteGroupUseCase;
import flipnote.group.application.port.in.command.DeleteGroupCommand;
import flipnote.group.application.port.out.GroupRepositoryPort;
import lombok.RequiredArgsConstructor;
`@Service`
`@RequiredArgsConstructor`
public class DeleteGroupService implements DeleteGroupUseCase {
private final GroupRepositoryPort groupRepository;
`@Override`
+ `@Transactional`
public void deleteGroup(DeleteGroupCommand cmd) {
+ var group = groupRepository.findById(cmd.groupId());
+ // TODO: group.getOwnerId()와 cmd.userId() 비교하여 권한 검증
groupRepository.delete(cmd.groupId());
}
}🤖 Prompt for AI Agents
In `@src/main/java/flipnote/group/application/service/DeleteGroupService.java`
around lines 10 - 19, Add transactional and ownership checks to
DeleteGroupService: annotate the class or deleteGroup method with
`@Transactional`, load the group via GroupRepositoryPort (e.g., findById or
getById) inside deleteGroup(DeleteGroupCommand cmd), verify that the fetched
group's ownerId equals cmd.userId, and if not throw an appropriate
access/authorization exception; only call groupRepository.delete(cmd.groupId())
after the existence and ownership checks succeed (and handle group-not-found by
throwing a not-found exception).
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
릴리스 노트