Feat: [FN-297] 그룹 조회 API 구현 #2
Merged
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
stoneTiger0912
changed the title
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. Walkthrough그룹 관리 기능 확장: 그룹 변경 및 조회 엔드포인트 추가, 새로운 UseCase 및 Service 구현, CreateGroupCommand에 userId 포함, 도메인 모델에 타임스탬프 필드 및 메서드 추가. Changes
Sequence DiagramsequenceDiagram
participant Client
participant GroupController
participant FindGroupService
participant GroupRepositoryPort
participant GroupMapper
Client->>GroupController: GET /{groupId}
GroupController->>FindGroupService: findGroup(FindGroupCommand)
FindGroupService->>GroupRepositoryPort: findById(groupId)
GroupRepositoryPort-->>FindGroupService: Optional<GroupEntity>
FindGroupService->>GroupMapper: toDomain(GroupEntity)
GroupMapper-->>FindGroupService: Group
FindGroupService-->>GroupController: FindGroupResult
GroupController-->>Client: ResponseEntity<FindGroupResponseDto>
sequenceDiagram
participant Client
participant GroupController
participant ChangeGroupService
participant GroupRepositoryPort
participant GroupMapper
Client->>GroupController: PUT /{groupId}
GroupController->>ChangeGroupService: change(ChangeGroupCommand)
ChangeGroupService->>GroupRepositoryPort: findById(groupId)
GroupRepositoryPort-->>ChangeGroupService: Optional<GroupEntity>
ChangeGroupService->>GroupMapper: toDomain(GroupEntity)
GroupMapper-->>ChangeGroupService: Group
ChangeGroupService->>GroupRepositoryPort: update(Group, GroupEntity)
GroupRepositoryPort-->>ChangeGroupService: Group
ChangeGroupService-->>GroupController: ChangeGroupResult
GroupController-->>Client: ResponseEntity<ChangeGroupResponseDto>
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Possibly related PRs
Poem
🚥 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: 7
🤖 Fix all issues with AI agents
In `@src/main/java/flipnote/group/adapter/out/entity/GroupEntity.java`:
- Around line 81-94: The change() method on GroupEntity currently accepts and
assigns memberCount which can unintentionally overwrite the persisted count;
remove memberCount from the change(String name, Category category, String
description, JoinPolicy joinPolicy, Visibility visibility, Integer maxMember,
Long imageRefId) parameter list and stop assigning this.memberCount inside
GroupEntity.change so the existing memberCount remains unchanged; update all
callers (places invoking GroupEntity.change and any tests) to stop passing
memberCount (they should use ChangeGroupRequestDto/ChangeGroupCommand which do
not include memberCount) or adapt to the new signature.
In
`@src/main/java/flipnote/group/adapter/out/persistence/GroupRepositoryAdapter.java`:
- Around line 31-34: The adapter currently exposes persistence type by returning
Optional<GroupEntity> from GroupRepositoryAdapter.findById; change this method
to return Optional<Group> (the port's expected type) and map the result inside
the adapter by transforming GroupEntity to the domain Group (e.g.,
groupRepository.findById(id).map(entity -> entity.toDomain()) or via a
GroupMapper.toDomain(entity)); do not change the GroupRepositoryPort
signature—perform the mapping within GroupRepositoryAdapter and return
Optional.empty()/mapped Optional<Group> as appropriate.
In `@src/main/java/flipnote/group/api/dto/response/FindGroupResponseDto.java`:
- Around line 11-29: FindGroupResponseDto is missing the group's id and
memberCount fields; add Long id and Integer memberCount to the record signature
and update the static factory/mapper method from(...) to populate these new
fields from the Group entity (use Group.getId() and Group.getMemberCount() or
equivalent), and then update any constructor/usage sites to pass the new values;
ensure names are FindGroupResponseDto and from so callers compile.
In `@src/main/java/flipnote/group/application/port/out/GroupRepositoryPort.java`:
- Around line 5-13: The port interface GroupRepositoryPort currently depends on
adapter layer entity GroupEntity; change its API to use only the domain model by
making findById return Optional<Group> (not Optional<GroupEntity>) and remove
the GroupEntity parameter from update so update accepts only a Group; move any
entity-to-domain mapping and entity lookup/update responsibility into the
adapter (e.g., GroupRepositoryAdapter) where you should call
GroupMapper.toDomain() and perform entity retrieval/modification internally.
In `@src/main/java/flipnote/group/application/service/ChangeGroupService.java`:
- Around line 30-41: change(ChangeGroupCommand cmd) 현재 domain 객체를 그대로 update에 넘겨
cmd의 변경값이 반영되지 않으므로, GroupMapper.toDomain(groupEntity)로 만든 domainGroup에
ChangeGroupCommand의 필드(예: name, category, description 등)를 적용한 후
groupRepository.update를 호출하도록 수정하세요; 구체적으로는 ChangeGroupCommand의 각
nullable/optional 필드를 확인해 domainGroup의 변경 메서드(예: setName/changeName, setCategory
등)나 새로운 도메인 생성자를 사용해 값을 덮어쓰고 나서 groupRepository.update(domainGroup,
groupEntity)을 호출하도록 변경하여 변경사항이 DB에 반영되게 만드세요.
- Around line 8-9: ChangeGroupService is importing adapter types (GroupEntity,
GroupMapper) because GroupRepositoryPort exposes adapter types; refactor the
port to expose only domain types (replace Optional<GroupEntity> with
Optional<Group>, change update(Group, GroupEntity) to update(Group) returning
Group or id), then update the adapter implementation to perform mapping between
Group and GroupEntity internally (use GroupMapper inside the adapter, not the
service). After that remove GroupEntity and GroupMapper imports from
ChangeGroupService and adjust its calls to use the revised GroupRepositoryPort
methods (findById(Long) -> Optional<Group>, update(Group) -> Group or id).
In `@src/main/java/flipnote/group/application/service/FindGroupService.java`:
- Around line 7-8: FindGroupService currently depends on adapter types because
GroupRepositoryPort returns Optional<GroupEntity> and the service imports
GroupEntity and GroupMapper; change the port signature so GroupRepositoryPort
returns Optional<Group> (domain model) and move the mapping from
GroupEntity->Group into the adapter implementation (GroupRepositoryAdapter) so
FindGroupService no longer imports GroupEntity/GroupMapper; also mark the
service read-only method with `@Transactional`(readOnly = true) on the method (or
class) that performs the find operation. Ensure the unique symbols updated are
GroupRepositoryPort (return type), GroupRepositoryAdapter (implement mapping),
and FindGroupService (remove adapter imports and add `@Transactional`(readOnly =
true)).
🧹 Nitpick comments (9)
src/main/java/flipnote/group/application/port/in/command/ChangeGroupCommand.java (1)
7-17:maxMember타입이ChangeGroupRequestDto의Integer와 불일치합니다.
ChangeGroupRequestDto에서는Integer(wrapper)를 사용하지만, 이 커맨드에서는int(primitive)를 사용합니다. DTO에서@NotNull검증이 있어 실제 NPE 가능성은 낮지만, 일관성을 위해 타입을 맞추는 것을 권장합니다.CreateGroupCommand도 동일한 불일치가 있습니다.src/main/java/flipnote/group/adapter/out/persistence/mapper/GroupMapper.java (1)
10-12:@Component와static메서드가 혼용되어 있습니다.모든 메서드가
static이므로@Component어노테이션과 Spring 빈 등록이 불필요합니다. 유틸리티 클래스로 사용하려면@Component를 제거하고, Spring 빈으로 관리하려면static을 제거하고 인스턴스 메서드로 변경하세요.♻️ 유틸리티 클래스로 통일하는 경우
-@Component `@NoArgsConstructor`(access = AccessLevel.PRIVATE) public class GroupMapper {src/main/java/flipnote/group/adapter/out/persistence/GroupRepositoryAdapter.java (1)
40-54:update메서드: 명시적save()호출 없이 JPA dirty checking에 의존하고 있습니다.
groupEntity.change(...)호출 후groupRepository.save()를 명시적으로 호출하지 않고 JPA dirty checking에 의존하고 있습니다.@Transactional범위 내에서 동작하긴 하지만, 명시적인save()호출이 의도를 더 명확하게 전달합니다.또한,
update(Group group, GroupEntity groupEntity)시그니처에서GroupEntity를 파라미터로 받는 것은 port 계층의 추상화를 약화시킵니다. 내부적으로 ID 기반 조회 후 변경하는 방식으로 리팩토링을 고려해 주세요.src/main/java/flipnote/group/application/service/FindGroupService.java (2)
27-37: 읽기 전용 트랜잭션 어노테이션 누락 및userId미사용.
- 조회 서비스이므로
@Transactional(readOnly = true)를 추가하면 DB 커넥션 최적화(읽기 전용 라우팅, 플러시 모드 비활성화 등)에 도움이 됩니다.FindGroupCommand에 포함된userId가 메서드 내에서 전혀 사용되지 않습니다. 그룹 조회 시 가시성(Visibility) 검증이나 권한 확인이 필요하다면 반영이 필요하고, 불필요하다면 커맨드에서 제거하는 것이 좋습니다.제안: `@Transactional`(readOnly = true) 추가
+import org.springframework.transaction.annotation.Transactional; + `@Service` `@RequiredArgsConstructor` public class FindGroupService implements FindGroupUseCase { private final GroupRepositoryPort groupRepository; `@Override` + `@Transactional`(readOnly = true) public FindGroupResult findGroup(FindGroupCommand cmd) {
30-32:IllegalArgumentException대신 도메인 전용 예외 사용을 권장합니다.
IllegalArgumentException은 범용 예외로, 글로벌 예외 핸들러에서 HTTP 상태 코드 매핑 시 다른 유효성 검증 실패와 구분하기 어렵습니다.GroupNotFoundException같은 도메인 전용 예외를 정의하면 404 응답과 명확하게 매핑할 수 있습니다.src/main/java/flipnote/group/api/dto/response/ChangeGroupResponseDto.java (1)
11-45: 응답 DTO에groupId가 포함되어 있지 않습니다.그룹 수정 응답에
groupId가 빠져 있으면 클라이언트가 어떤 그룹이 수정되었는지 식별하기 어렵습니다.id필드를 추가하는 것을 고려해 주세요.제안: groupId 필드 추가
public record ChangeGroupResponseDto( + Long groupId, + String name, // ... 기존 필드 ) { public static ChangeGroupResponseDto from(ChangeGroupResult result) { Group group = result.group(); return new ChangeGroupResponseDto( + group.getId(), group.getName(), // ... 기존 매핑 ); } }src/main/java/flipnote/group/domain/model/group/Group.java (2)
66-92:restore()메서드의 파라미터가 11개로, 가독성과 유지보수성이 낮아질 수 있습니다.향후 필드가 추가되면 파라미터가 더 늘어날 가능성이 높습니다. 파라미터 객체 패턴(예:
GroupSnapshotrecord)이나 Builder 패턴 도입을 검토해 보시면 좋겠습니다.
28-29:create()시createdAt/modifiedAt가 설정되지 않습니다.JPA 엔티티의
@CreatedDate/@LastModifiedDate로 관리된다면 동작상 문제는 없지만,create()직후 도메인 객체의 타임스탬프가null인 점은 인지해 주세요. 필요 시create()내에서LocalDateTime.now()를 설정하면 도메인 모델의 자기 완결성이 높아집니다.Also applies to: 36-51
src/main/java/flipnote/group/adapter/in/web/GroupController.java (1)
42-61: 그룹 생성 API 응답 코드200 OK→201 Created변경을 권장합니다.기존 코드이긴 하나, 리소스 생성 시에는 REST 관례상
ResponseEntity.status(HttpStatus.CREATED).body(res)또는ResponseEntity.created(uri).body(res)가 더 적절합니다.
Comment on lines
31
to
34
| @Override | ||
| public Optional<GroupEntity> findById(Long id) { | ||
| return groupRepository.findById(id); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
findById가 Optional<GroupEntity>를 반환하여 port 인터페이스를 통해 adapter 타입이 노출됩니다.
이 메서드의 반환 타입이 GroupRepositoryPort를 통해 application 계층에 GroupEntity 의존성을 전파합니다. ChangeGroupService에서 언급한 대로 port 인터페이스는 Optional<Group>을 반환하고, 매핑은 adapter 내부에서 처리하는 것이 바람직합니다.
🤖 Prompt for AI Agents
In
`@src/main/java/flipnote/group/adapter/out/persistence/GroupRepositoryAdapter.java`
around lines 31 - 34, The adapter currently exposes persistence type by
returning Optional<GroupEntity> from GroupRepositoryAdapter.findById; change
this method to return Optional<Group> (the port's expected type) and map the
result inside the adapter by transforming GroupEntity to the domain Group (e.g.,
groupRepository.findById(id).map(entity -> entity.toDomain()) or via a
GroupMapper.toDomain(entity)); do not change the GroupRepositoryPort
signature—perform the mapping within GroupRepositoryAdapter and return
Optional.empty()/mapped Optional<Group> as appropriate.
src/main/java/flipnote/group/application/port/out/GroupRepositoryPort.java
Outdated
Show resolved
Hide resolved
src/main/java/flipnote/group/application/service/ChangeGroupService.java
Show resolved
Hide resolved
src/main/java/flipnote/group/application/service/ChangeGroupService.java
Show resolved
Hide resolved
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
릴리스 노트