-
Notifications
You must be signed in to change notification settings - Fork 0
Feat: [FN-298] 그룹 수정 API 구현 #3
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Caution Review failedThe pull request is closed. Walkthrough그룹 수정(Change Group) 기능이 추가되었습니다. 컨트롤러에 PUT 엔드포인트가 추가되고, ChangeGroupUseCase/ChangeGroupCommand/ChangeGroupService가 도입되며 영속성 계층에 조회(findById)·업데이트(update)와 매핑(toDomain)이 추가되었습니다. 도메인에는 createdAt/modifiedAt 필드가 추가되었습니다. 변경사항
시퀀스 다이어그램sequenceDiagram
participant Client
participant GroupController
participant ChangeGroupService
participant GroupRepositoryPort
participant GroupEntityDB as GroupEntity(DB)
participant GroupMapper
Client->>GroupController: PUT /v1/groups/{groupId} + ChangeGroupRequestDto, X-USER-ID
GroupController->>ChangeGroupService: change(ChangeGroupCommand)
ChangeGroupService->>GroupRepositoryPort: findById(groupId)
GroupRepositoryPort->>GroupEntityDB: SELECT GroupEntity by id
GroupEntityDB-->>GroupRepositoryPort: GroupEntity
GroupRepositoryPort-->>ChangeGroupService: GroupEntity
ChangeGroupService->>GroupEntityDB: entity.change(cmd)
GroupEntityDB-->>GroupRepositoryPort: 변경 반영 (영속성)
GroupRepositoryPort-->>ChangeGroupService: 업데이트된 Group (매핑 전)
ChangeGroupService->>GroupMapper: toDomain(updatedEntity)
GroupMapper-->>ChangeGroupService: Group (도메인)
ChangeGroupService-->>GroupController: ChangeGroupResult
GroupController-->>Client: 200 OK + ChangeGroupResponseDto
예상 코드 리뷰 노력🎯 3 (보통) | ⏱️ ~22분 관련 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/flipnote/group/domain/model/group/Group.java (1)
14-92:⚠️ Potential issue | 🔴 Critical그룹 수정을 위한 도메인 메서드가 없습니다.
Group도메인 모델에create()와restore()는 있지만, 변경 사항을 적용하는 메서드가 없습니다. 이로 인해ChangeGroupService에서 command의 값을 도메인 객체에 반영할 수 없는 치명적 버그가 발생합니다.ChangeGroupCommand(또는 개별 파라미터)를 받아 필드를 갱신하고 유효성 검증을 수행하는change()메서드를 추가해야 합니다.
🤖 Fix all issues with AI agents
In `@src/main/java/flipnote/group/adapter/out/entity/GroupEntity.java`:
- Around line 81-94: The change(...) method currently accepts and assigns
memberCount, allowing external callers to overwrite a derived value; remove
memberCount from the change method signature and stop assigning this.memberCount
inside GroupEntity.change(...) so group edits cannot arbitrarily mutate
memberCount, and instead provide/keep dedicated methods (e.g.,
incrementMemberCount(), decrementMemberCount(), or adjustMemberCountBy(...))
that are the only ways to mutate memberCount if needed.
In `@src/main/java/flipnote/group/application/port/out/GroupRepositoryPort.java`:
- Line 5: The GroupRepositoryPort currently imports and depends on adapter-layer
GroupEntity; change the port to use the domain model only by removing the import
of flipnote.group.adapter.out.entity.GroupEntity and updating the interface
signatures so findById returns Optional<Group> and update accepts a domain Group
(or other domain types) instead of GroupEntity; move all Entity↔Domain mapping
logic into the repository adapter implementation (the class that implements
GroupRepositoryPort) so adapters handle conversion between GroupEntity and the
domain Group, keeping the application port free of adapter types.
In `@src/main/java/flipnote/group/application/service/ChangeGroupService.java`:
- Around line 30-41: The change method currently maps the DB entity to a domain
Group using GroupMapper.toDomain but never applies values from
ChangeGroupCommand (cmd) or checks cmd.userId(), so update persists unchanged
data; add a mutator on the domain model (e.g., Group.change(ChangeGroupCommand)
that applies name, category, description, joinPolicy, visibility, maxMember,
imageRefId and performs authorization/permission checks using cmd.userId(), then
in ChangeGroupService.change call GroupMapper.toDomain(groupEntity), invoke
domain.change(cmd) before calling groupRepository.update(domainGroup,
groupEntity) so the repository persists the updated state.
🧹 Nitpick comments (4)
src/main/java/flipnote/group/application/port/in/command/ChangeGroupCommand.java (1)
7-17:CreateGroupCommand와의 중복 필드가 많습니다.
ChangeGroupCommand와CreateGroupCommand는name,category,description,joinPolicy,visibility,maxMember,imageRefId총 7개의 필드가 동일합니다. 현재 수준에서는 수용 가능하지만, 향후 필드가 추가되거나 변경될 때 두 커맨드를 동시에 수정해야 하는 부담이 생길 수 있으므로, 공통 필드를 별도의 record나 인터페이스로 추출하는 것을 고려해 볼 수 있습니다.src/main/java/flipnote/group/adapter/out/persistence/GroupRepositoryAdapter.java (1)
31-34:GroupRepositoryPort가GroupEntity를 반환하여 헥사고날 아키텍처 경계를 위반합니다.
findById가Optional<GroupEntity>를 반환하면 application 포트가 adapter 계층의 영속성 엔티티에 의존하게 됩니다. 도메인 객체(Optional<Group>)를 반환하도록 변경하면 계층 분리를 유지할 수 있습니다. 다만, 현재 JPA dirty checking을 활용하기 위해 entity 참조가 필요한 구조이므로 즉시 변경이 어려울 수 있습니다.src/main/java/flipnote/group/application/service/ChangeGroupService.java (1)
8-9: Application 서비스에서 adapter 계층을 직접 참조합니다.
GroupEntity와GroupMapper는 adapter 계층에 속하는데, application 서비스에서 직접 import하고 있어 헥사고날 아키텍처의 의존성 방향을 위반합니다. Port 인터페이스가 도메인 객체만 반환하도록 개선하면 이 의존성을 제거할 수 있습니다.src/main/java/flipnote/group/domain/model/group/Group.java (1)
10-10: 사용되지 않는lombok.Setterimport가 남아있습니다.
@Setter어노테이션이 클래스에서 제거되었으나 import는 남아있습니다.
Summary by CodeRabbit