Conversation
# Conflicts: # src/main/java/konkuk/thip/room/adapter/out/persistence/RoomCommandPersistenceAdapter.java # src/main/java/konkuk/thip/room/application/port/out/RoomCommandPort.java
|
""" Walkthrough피드 수정 API 기능이 새롭게 도입되었습니다. 이를 위해 컨트롤러, 서비스, 포트, 도메인, JPA 어댑터, 매퍼 등 전반적인 구조가 확장·수정되었고, 피드 생성 시 카테고리 관련 로직이 제거되었습니다. 테스트 코드도 피드 수정 및 검증 시나리오를 포괄하도록 추가 및 수정되었습니다. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Controller as FeedCommandController
participant Service as FeedUpdateService
participant Port as FeedCommandPort
participant Adapter as FeedCommandPersistenceAdapter
participant DB
Client->>Controller: PATCH /feeds/{feedId} (FeedUpdateRequest)
Controller->>Service: updateFeed(FeedUpdateCommand)
Service->>Port: findById(feedId)
Port->>Adapter: findById(feedId)
Adapter->>DB: Query Feed, Tags, Content
DB-->>Adapter: Feed, Tags, Content
Adapter-->>Port: Feed 도메인 객체
Port-->>Service: Feed 도메인 객체
Service->>Service: 유효성 검증 및 변경 적용
Service->>Port: update(Feed)
Port->>Adapter: update(Feed)
Adapter->>DB: Feed, Content, Tag 갱신
DB-->>Adapter: OK
Adapter-->>Port: feedId
Port-->>Service: feedId
Service-->>Controller: feedId
Controller-->>Client: 200 OK (FeedIdResponse)
Estimated code review effort4 (약 1일) Suggested reviewers
Poem
✨ Finishing Touches
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. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
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: 5
🧹 Nitpick comments (4)
src/main/java/konkuk/thip/feed/adapter/in/web/request/FeedUpdateRequest.java (1)
15-15: 필드명 명확성 개선 권장
remainImageUrls라는 필드명이 의미를 명확하게 전달하지 못합니다. 이 필드가 "업데이트 후 유지할 이미지 URL 목록"을 나타낸다면, 더 명확한 이름을 사용하는 것이 좋습니다.- List<String> remainImageUrls + List<String> retainedImageUrls혹은 필드에 JavaDoc 주석을 추가하여 의미를 명확히 하세요.
src/main/java/konkuk/thip/feed/application/service/FeedUpdateService.java (1)
27-28: null 체크 로직 단순화 가능삼항 연산자 대신 더 간단한 방식으로 작성할 수 있습니다.
- Feed.validateImageCount(command.remainImageUrls() != null ? command.remainImageUrls().size() : 0); + int imageCount = command.remainImageUrls() != null ? command.remainImageUrls().size() : 0; + Feed.validateImageCount(imageCount);src/main/java/konkuk/thip/feed/application/service/FeedCreateService.java (1)
31-31: 이미지 삭제 전략 논의 필요TODO 주석에서 언급한 대로, 예외 발생 시 S3 이미지 삭제 방식에 대한 추가 논의가 필요합니다. 현재는 동기 방식으로 처리하고 있지만, 이벤트 기반이나 배치 방식을 고려해볼 수 있습니다.
고려사항:
- 트랜잭션 롤백과 S3 삭제의 일관성
- 삭제 실패 시 재시도 전략
- 고아 이미지 정리를 위한 배치 작업
src/main/java/konkuk/thip/feed/domain/Feed.java (1)
114-124: 이미지 소유권 검증 시 에러 메시지 개선 제안현재 구현은 올바르지만, 여러 이미지가 잘못된 경우 첫 번째 잘못된 이미지에서만 예외가 발생합니다. 모든 잘못된 이미지를 한 번에 보고하는 것이 사용자 경험에 더 좋을 수 있습니다.
public void validateOwnsImages(List<String> candidateImageUrls) { Set<String> myImageUrls = this.getContentList().stream() .map(Content::getContentUrl) .filter(url -> url != null && !url.isBlank()) .collect(Collectors.toSet()); + List<String> invalidUrls = new ArrayList<>(); for (String url : candidateImageUrls) { if (!myImageUrls.contains(url)) { - throw new InvalidStateException(INVALID_FEED_COMMAND, new IllegalArgumentException("해당 이미지는 이 피드에 존재하지 않습니다: " + url)); + invalidUrls.add(url); } } + if (!invalidUrls.isEmpty()) { + throw new InvalidStateException(INVALID_FEED_COMMAND, + new IllegalArgumentException("해당 이미지들은 이 피드에 존재하지 않습니다: " + String.join(", ", invalidUrls))); + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
src/main/java/konkuk/thip/common/exception/code/ErrorCode.java(1 hunks)src/main/java/konkuk/thip/feed/adapter/in/web/FeedCommandController.java(2 hunks)src/main/java/konkuk/thip/feed/adapter/in/web/request/FeedCreateRequest.java(0 hunks)src/main/java/konkuk/thip/feed/adapter/in/web/request/FeedUpdateRequest.java(1 hunks)src/main/java/konkuk/thip/feed/adapter/in/web/response/FeedCreateResponse.java(0 hunks)src/main/java/konkuk/thip/feed/adapter/in/web/response/FeedIdResponse.java(1 hunks)src/main/java/konkuk/thip/feed/adapter/out/jpa/FeedJpaEntity.java(2 hunks)src/main/java/konkuk/thip/feed/adapter/out/mapper/FeedMapper.java(2 hunks)src/main/java/konkuk/thip/feed/adapter/out/persistence/FeedCommandPersistenceAdapter.java(3 hunks)src/main/java/konkuk/thip/feed/adapter/out/persistence/repository/FeedTag/FeedTagJpaRepository.java(1 hunks)src/main/java/konkuk/thip/feed/adapter/out/persistence/repository/Tag/TagJpaRepository.java(1 hunks)src/main/java/konkuk/thip/feed/application/port/in/FeedUpdateUseCase.java(1 hunks)src/main/java/konkuk/thip/feed/application/port/in/dto/FeedCreateCommand.java(0 hunks)src/main/java/konkuk/thip/feed/application/port/in/dto/FeedUpdateCommand.java(1 hunks)src/main/java/konkuk/thip/feed/application/port/out/FeedCommandPort.java(1 hunks)src/main/java/konkuk/thip/feed/application/service/FeedCreateService.java(2 hunks)src/main/java/konkuk/thip/feed/application/service/FeedUpdateService.java(1 hunks)src/main/java/konkuk/thip/feed/domain/Feed.java(3 hunks)src/main/java/konkuk/thip/post/adapter/out/jpa/PostJpaEntity.java(1 hunks)src/main/java/konkuk/thip/room/adapter/out/persistence/RoomCommandPersistenceAdapter.java(0 hunks)src/main/java/konkuk/thip/room/application/port/out/RoomCommandPort.java(0 hunks)src/test/java/konkuk/thip/common/util/TestEntityFactory.java(3 hunks)src/test/java/konkuk/thip/feed/adapter/in/web/FeedCreateControllerTest.java(4 hunks)src/test/java/konkuk/thip/feed/adapter/in/web/FeedUpdateAPITest.java(1 hunks)src/test/java/konkuk/thip/feed/adapter/in/web/FeedUpdateControllerTest.java(1 hunks)
💤 Files with no reviewable changes (5)
- src/main/java/konkuk/thip/feed/application/port/in/dto/FeedCreateCommand.java
- src/main/java/konkuk/thip/feed/adapter/in/web/request/FeedCreateRequest.java
- src/main/java/konkuk/thip/room/application/port/out/RoomCommandPort.java
- src/main/java/konkuk/thip/room/adapter/out/persistence/RoomCommandPersistenceAdapter.java
- src/main/java/konkuk/thip/feed/adapter/in/web/response/FeedCreateResponse.java
🧰 Additional context used
🧠 Learnings (4)
src/main/java/konkuk/thip/feed/application/port/in/FeedUpdateUseCase.java (1)
Learnt from: seongjunnoh
PR: #43
File: src/main/java/konkuk/thip/book/application/port/out/BookCommandPort.java:0-0
Timestamp: 2025-07-03T03:05:05.031Z
Learning: THIP 프로젝트에서는 CQRS Port 분리 시 다음 컨벤션을 따름: CommandPort에는 findByXXX를 통해 도메인 엔티티를 찾아오는 메서드를 추가하고, QueryPort에는 조회 API의 response에 해당하는 데이터들을 DB로부터 조회하는 메서드를 추가함.
src/main/java/konkuk/thip/feed/application/port/out/FeedCommandPort.java (1)
Learnt from: seongjunnoh
PR: #43
File: src/main/java/konkuk/thip/book/application/port/out/BookCommandPort.java:0-0
Timestamp: 2025-07-03T03:05:05.031Z
Learning: THIP 프로젝트에서는 CQRS Port 분리 시 다음 컨벤션을 따름: CommandPort에는 findByXXX를 통해 도메인 엔티티를 찾아오는 메서드를 추가하고, QueryPort에는 조회 API의 response에 해당하는 데이터들을 DB로부터 조회하는 메서드를 추가함.
src/main/java/konkuk/thip/feed/adapter/out/persistence/repository/Tag/TagJpaRepository.java (1)
Learnt from: seongjunnoh
PR: #36
File: src/main/java/konkuk/thip/user/adapter/out/persistence/UserJpaRepository.java:7-7
Timestamp: 2025-06-29T09:47:31.299Z
Learning: Spring Data JPA에서 findBy{FieldName} 패턴의 메서드는 명시적 선언 없이 자동으로 생성되며, Optional 반환 타입을 사용하는 것이 null 안전성을 위해 권장됩니다.
src/main/java/konkuk/thip/feed/application/service/FeedCreateService.java (1)
Learnt from: seongjunnoh
PR: #43
File: src/main/java/konkuk/thip/book/application/port/out/BookCommandPort.java:0-0
Timestamp: 2025-07-03T03:05:05.031Z
Learning: THIP 프로젝트에서는 CQRS Port 분리 시 다음 컨벤션을 따름: CommandPort에는 findByXXX를 통해 도메인 엔티티를 찾아오는 메서드를 추가하고, QueryPort에는 조회 API의 response에 해당하는 데이터들을 DB로부터 조회하는 메서드를 추가함.
🧬 Code Graph Analysis (2)
src/test/java/konkuk/thip/feed/adapter/in/web/FeedUpdateControllerTest.java (1)
src/test/java/konkuk/thip/common/util/TestEntityFactory.java (1)
TestEntityFactory(26-225)
src/test/java/konkuk/thip/feed/adapter/in/web/FeedUpdateAPITest.java (1)
src/test/java/konkuk/thip/common/util/TestEntityFactory.java (1)
TestEntityFactory(26-225)
⏰ 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 (23)
src/main/java/konkuk/thip/feed/adapter/out/jpa/FeedJpaEntity.java (1)
45-51: 도메인 객체로부터 JPA 엔티티를 업데이트하는 메서드가 잘 구현되었습니다.
updateFrom메서드는 도메인 객체의 상태를 JPA 엔티티에 반영하는 깔끔한 방식을 제공합니다. 변경 가능한 모든 필드(content, isPublic, reportCount, likeCount, commentCount)가 적절히 업데이트되고 있습니다.src/main/java/konkuk/thip/feed/application/port/in/FeedUpdateUseCase.java (1)
5-7: 피드 업데이트 유스케이스 인터페이스가 깔끔하게 설계되었습니다.단일 책임 원칙을 따르며 CQRS 패턴에 잘 맞는 인터페이스입니다.
FeedUpdateCommand를 받아 업데이트된 피드 ID를 반환하는 명확한 계약을 제공합니다.src/main/java/konkuk/thip/feed/adapter/out/persistence/repository/Tag/TagJpaRepository.java (1)
14-15: 피드별 태그 조회를 위한 JPQL 쿼리가 효율적으로 구현되었습니다.중간 테이블
FeedTagJpaEntity를 통한 조인으로 특정 피드의 태그들을 단일 쿼리로 조회하는 효율적인 방식입니다. 메서드명도 Spring Data JPA 컨벤션을 잘 따르고 있습니다.src/main/java/konkuk/thip/feed/application/port/out/FeedCommandPort.java (1)
8-9: CQRS 패턴과 프로젝트 컨벤션에 부합하는 메서드들이 추가되었습니다.
update와findById메서드가 CommandPort에 적절히 추가되었습니다. 학습된 컨벤션에 따라 CommandPort에 도메인 엔티티를 찾아오는findById메서드가 포함된 것이 올바른 접근입니다.src/main/java/konkuk/thip/feed/adapter/in/web/response/FeedIdResponse.java (1)
3-6: 피드 생성과 업데이트 응답을 통합한 깔끔한 DTO입니다.Java record를 사용한 간결한 구현과 정적 팩토리 메서드가 잘 설계되었습니다. 기존의
FeedCreateResponse를 대체하여 생성과 업데이트 응답을 통합한 것은 코드 중복을 줄이는 좋은 리팩토링입니다.src/main/java/konkuk/thip/feed/adapter/out/persistence/repository/FeedTag/FeedTagJpaRepository.java (1)
12-14: 깔끔한 JPQL 삭제 쿼리 구현입니다.
@Modifying과 매개변수화된 쿼리를 올바르게 사용하여 피드-태그 관계를 효율적으로 삭제하는 구현이 잘 되어 있습니다. 피드 수정 시 기존 태그들을 일괄 삭제하는 용도로 적합합니다.src/main/java/konkuk/thip/common/exception/code/ErrorCode.java (1)
133-134: 피드 수정 기능을 위한 적절한 에러 코드 개선입니다.
INVALID_FEED_CREATE를INVALID_FEED_COMMAND로 변경하여 생성과 수정 모두에 재사용 가능하게 만들고,FEED_UPDATE_FORBIDDEN을 추가하여 권한 관련 에러를 명확히 구분한 것이 좋습니다. HTTP 상태 코드와 에러 코드 번호도 적절합니다.src/main/java/konkuk/thip/post/adapter/out/jpa/PostJpaEntity.java (1)
24-28: 피드 수정을 위한 적절한 접근 제어자 변경입니다.부모 클래스의 필드를
private에서protected로 변경하여 서브클래스에서 직접 접근할 수 있도록 한 것이 적절합니다. 이는FeedJpaEntity에서 피드 수정 시 해당 필드들을 업데이트할 수 있도록 지원하며, 상속 계층 내에서만 접근 가능하도록 적절히 제한되어 있습니다.src/main/java/konkuk/thip/feed/application/port/in/dto/FeedUpdateCommand.java (1)
5-20: 피드 수정을 위한 잘 설계된 커맨드 객체입니다.Java record를 사용하여 불변 데이터 전송 객체로 구현되었고, 피드 수정에 필요한 모든 데이터(콘텐츠, 공개 여부, 태그, 이미지, 식별자)를 포함하고 있습니다. 필드명도 명확하고 비즈니스 로직 없이 순수 데이터 캐리어 역할에 충실합니다.
src/main/java/konkuk/thip/feed/adapter/out/mapper/FeedMapper.java (2)
16-19: ContentMapper 의존성 주입이 적절히 추가되었습니다.
@RequiredArgsConstructor를 사용한 생성자 주입 방식으로ContentMapper의존성을 추가한 것이 스프링의 의존성 주입 베스트 프랙티스를 따르고 있습니다.
48-50: Feed-Content 양방향 매핑이 올바르게 구현되었습니다.
contentList매핑이 누락되어 있던 부분을contentMapper를 활용해 스트림으로 변환하도록 구현한 것이 좋습니다. 이로써 JPA 엔티티에서 도메인 객체로 변환 시 콘텐츠 정보가 올바르게 전달됩니다.src/test/java/konkuk/thip/feed/adapter/in/web/FeedCreateControllerTest.java (2)
21-21: 에러 코드 통합 변경 확인
INVALID_FEED_CREATE에서INVALID_FEED_COMMAND로의 변경이 일관되게 적용되었습니다. 이제 피드 생성과 수정 모두에서 동일한 에러 코드를 사용합니다.Also applies to: 58-58, 158-158
105-123: 카테고리 검증 제거 및 태그 검증 유지카테고리 관련 검증이 제거되고 태그 검증만 남은 것이 PR 목적에 부합합니다. 태그 개수 제한(최대 5개)과 중복 검증이 적절히 유지되고 있습니다.
src/main/java/konkuk/thip/feed/application/service/FeedUpdateService.java (1)
41-56: 부분 업데이트 로직 구현 확인각 필드의 null 체크를 통한 부분 업데이트 구현이 깔끔합니다. 도메인 메서드를 적절히 활용하고 있습니다.
src/main/java/konkuk/thip/feed/adapter/out/persistence/FeedCommandPersistenceAdapter.java (4)
39-47: 피드 조회 메서드 구현 확인CQRS 패턴에 따라 CommandPort에
findById메서드를 추가한 것이 적절합니다. 태그 정보도 함께 조회하여 도메인 엔티티로 변환하고 있습니다.
82-98: 중복 코드 제거를 위한 리팩토링
addAllContents와addAllTags메서드로 공통 로직을 추출한 것이 좋습니다. 저장과 업데이트 시 동일한 로직을 재사용할 수 있게 되었습니다.
94-97: 업데이트 시 기존 연관관계 처리업데이트 메서드에서
clear()와deleteAllByFeedJpaEntity()를 사용하여 기존 연관관계를 제거하는 방식이 적절합니다. 이는 고아 객체 문제를 방지합니다.Also applies to: 116-119
117-117: 이미 벌크 DELETE 방식으로 구현되어 있습니다
FeedTagJpaRepository의deleteAllByFeedJpaEntity는@Modifying @Query("DELETE FROM FeedTagJpaEntity ft WHERE ft.feedJpaEntity = :feedJpaEntity") void deleteAllByFeedJpaEntity(@Param("feedJpaEntity") FeedJpaEntity feedJpaEntity);로 한 번의 SQL DELETE 쿼리만 실행하므로 대량 삭제 시에도 벌크 연산이 적용되어 성능 우려가 낮습니다.
– src/main/java/konkuk/thip/feed/adapter/out/persistence/repository/FeedTag/FeedTagJpaRepository.java:
deleteAllByFeedJpaEntity메서드Likely an incorrect or invalid review comment.
src/main/java/konkuk/thip/feed/application/service/FeedCreateService.java (2)
35-35: 카테고리 검증 제거 확인
validateCategoryAndTags에서validateTags로 변경되어 카테고리 관련 검증이 제거되었습니다. PR 목적에 부합하는 변경입니다.
42-64: 이미지 업로드 예외 처리 개선
imageUrls를 null로 초기화하고 예외 발생 시에만 삭제하도록 개선되었습니다. 이는 불필요한 S3 삭제 요청을 방지합니다.src/test/java/konkuk/thip/common/util/TestEntityFactory.java (1)
178-225: 팩토리 메서드 구현이 적절합니다새로 추가된 피드 관련 팩토리 메서드들이 기존 패턴을 잘 따르고 있으며, 특히
createFeedWithContents에서 양방향 관계를 올바르게 설정하고 있습니다.src/test/java/konkuk/thip/feed/adapter/in/web/FeedUpdateAPITest.java (1)
1-216: 통합 테스트가 포괄적으로 잘 작성되었습니다피드 수정 API의 주요 시나리오들(태그 수정, 이미지 수정, 전체 필드 수정)을 모두 테스트하고 있으며, 데이터베이스 상태 검증까지 철저히 수행하고 있습니다.
src/main/java/konkuk/thip/feed/adapter/in/web/FeedCommandController.java (1)
34-42: 피드 수정 엔드포인트가 적절히 구현되었습니다PATCH 메서드를 사용한 부분 수정 구현이 RESTful 원칙에 부합하며, 인증된 사용자 검증과 요청 유효성 검사가 올바르게 적용되어 있습니다.
src/main/java/konkuk/thip/feed/adapter/in/web/request/FeedUpdateRequest.java
Show resolved
Hide resolved
src/main/java/konkuk/thip/feed/application/service/FeedUpdateService.java
Show resolved
Hide resolved
src/test/java/konkuk/thip/feed/adapter/in/web/FeedUpdateControllerTest.java
Outdated
Show resolved
Hide resolved
src/test/java/konkuk/thip/feed/adapter/in/web/FeedUpdateControllerTest.java
Show resolved
Hide resolved
src/test/java/konkuk/thip/feed/adapter/in/web/FeedUpdateControllerTest.java
Outdated
Show resolved
Hide resolved
buzz0331
left a comment
buzz0331
left a comment
There was a problem hiding this comment.
수고하셨습니다~ 코드가 깔끔하네욥
간단한 리뷰 하나 남겼는데 확인 부탁드릴게여
| @Override | ||
| @Transactional | ||
| public Long updateFeed(FeedUpdateCommand command) { | ||
|
|
||
| // 1. 유효성 검증 | ||
| Feed.validateTags(command.tagList()); | ||
| Feed.validateImageCount(command.remainImageUrls() != null ? command.remainImageUrls().size() : 0); | ||
|
|
||
| // 2. 피드 조회 및 유효성 검증 | ||
| Feed feed = feedCommandPort.findById(command.feedId()); | ||
| feed.validateCreator(command.userId()); | ||
|
|
||
| // 3. 도메인 내부 상태 변경 | ||
| applyPartialFeedUpdate(feed, command); | ||
|
|
||
| // 4. 업데이트 | ||
| return feedCommandPort.update(feed); | ||
| } |
| if (command.remainImageUrls() != null) { | ||
| feed.validateOwnsImages(command.remainImageUrls()); | ||
| feed.updateImages(command.remainImageUrls()); | ||
| } |
There was a problem hiding this comment.
처음에 코드를 읽었을 때, 도메인 규칙이 직관적으로 잘 와닿지 않아서 아래와 같은 방식으로 리팩토링해보면 어떨까 제안드립니다 🙂
Feed 도메인
public boolean isDeleteRequest(List<String> candidateImageUrls) {
validateOwnsImages(candidateImageUrls);
int currentImageCount = this.contentList.size();
int newImageCount = candidateImageUrls.size();
return newImageCount < currentImageCount;
}FeedUpdateService
private void applyPartialFeedUpdate(Feed feed, FeedUpdateCommand command) {
List<String> remainImageUrls = command.remainImageUrls();
if (remainImageUrls != null && feed.isDeleteRequest(remainImageUrls)) {
feed.updateImages(remainImageUrls);
}
if (command.contentBody() != null) {
feed.updateContent(command.contentBody());
}
if (command.isPublic() != null) {
feed.updateVisibility(command.isPublic());
}
if (command.tagList() != null) {
feed.updateTags(command.tagList());
}
}이렇게 수정하면 “요청 이미지 수가 줄어들면 삭제 요청이다”라는 도메인 규칙이 보다 명시적으로 표현되고, 도메인 객체 내부에 책임을 위임하여 캡슐화와 가독성 측면에서도 이점이 있을 것 같습니다!
혹시 의도와 다르다면 편하게 말씀해주세요 🙏
There was a problem hiding this comment.
리뷰 주신 내용 감사드립니다! 🙇♂️
말씀하신 "이미지 수가 줄어들면 삭제 요청이다" 라는 판단은 도메인 규칙처럼도 보일 수 있지만, 저는 이를 Patch 방식의 특성(Patch는 변경 필드만 보내는 구조)에 따라 어플리케이션 계층에서 해석되어야 할 책임이라고 해석했습니다.
현재 서비스 레이어에서는 remainImageUrls != null 등을 통해 수정 의도를 해석하고 있고, 도메인 객체(Feed)에서는 오직 현재 사용자에 의해 해당 이미지 수정 요청이 유효한가(“해당 유저가 자신의 피드 이미지에 대해서만 수정/삭제할 수 있다”)만 검증하고 있어 책임과 계층 분리가 좀 더 명시적으로 나타난다고 생각했습니다.
말씀하신 방식(isDeleteRequest)도 의미 표현이 명확하고 객체 내 응집도가 좋아질 수 있는 장점이 있어서 검토해볼만 하다고 생각합니다!
다만 지금 구조도 Patch 구조에 맞게 설계된 흐름이라 판단되어, 우선 유지하는 방향으로 가도 괜찮을 것 같습니다.
| } | ||
| } | ||
|
|
||
| //TODO 추후 이벤트 기반으로 트랜잭션 커밋후 S3 삭제하도록 리펙토링 or 사용하지 않는 이미지 배치 삭제방식 논의 |
seongjunnoh
left a comment
seongjunnoh
left a comment
There was a problem hiding this comment.
고생하셨습니다~~ 리뷰 확인부탁드립니다!
| } | ||
| } | ||
|
|
||
| //TODO 추후 이벤트 기반으로 트랜잭션 커밋후 S3 삭제하도록 리펙토링 or 사용하지 않는 이미지 배치 삭제방식 논의 |
| // 1. 유효성 검증 | ||
| Feed.validateTags(command.tagList()); | ||
| Feed.validateImageCount(command.remainImageUrls() != null ? command.remainImageUrls().size() : 0); |
There was a problem hiding this comment.
p2 : 현재 코드처럼 command의 tagList, remainImageUrl 의 유효성 검사를 Feed 도메인 내부에서 따로 수행한 후, tagList, imageList 를 update 하는 것 보다, command의 tagList, remainingImageUrl 들을 모두 Feed의 updateTag, updateImage 메서드의 인자로 던지고 Feed의 update 메서드 내부에서 인자로 받은 데이터에 대한 유효성 검사를 수행하는건 어떤가요??
'피드는 최대 5개의 태그, 최대 3개의 이미지를 가질 수 있다' 라는 도메인 규칙이 있고, 피드 수정시에 해당 도메인 규칙에 어긋나지 않는지에 대한 validation을 Feed 도메인에게 맡기는 것도 좋지만, update 메서드와 validation 메서드를 분리하는것보다는 update 메서드 내부에서 validation + update 을 수행하는 것이 더 좋지 않나 싶습니다
또한 현재 validation 메서드가 단순 개수 제한, 중복 여부 만을 검사하는데, update 메서드 내부로 옮기면 해당 태그가 enum에 정의한 유효한 태그인지, 해당 이미지 url이 content에 포함되는 유효한 url 인지 까지 검증할 수 있는 장점이 있을 것 같습니다!
There was a problem hiding this comment.
따라서 아래와 같은 전체 updateFeed 플로우를 제안드립니다
public Long updateFeed(Command command) {
// 1. 수정하고자 하는 Feed 조회
// 2. Feed의 이미지 update : feed.updateImages(userId, imageUrls)
// 3. Feed의 contentBody update : feed.updateContent(userId, contentBody)
// 4. Feed의 공개여부 update : feed.updateVisibility(userId, isPublic)
// 5. Feed의 태그 update : feed.updateTags(userId, tags)
// 6. 변경한 Feed DB에 반영
}
pubilc class Feed {
updateImages(userId, imageUrls) {
// 1. validateCreator(userId)
// 2. imageUrl이 Feed에 이미 존재하는 값인지 검증
// 3. imageUrl 만 Content 로 유지
}
updateContent(userId, contentBody) {
// 1. validateCreator(userId)
// 2. contentBody update
}
updateVisibility(userId, isPublic) {
// 1. validateCreator(userId)
// 2. contentBody update
}
updateTags(userId, tags) {
// 1. validateCreator(userId)
// 2. validate tags (태그 개수 등등)
// 3. tag update
}
validateCreator(userId) {
// Feed의 creator와 userId 가 일치하는지 확인
// 아니라면 exception을 던지기 or false를 리턴 & 이를 반환받는 곳에서 exception을 던지기 or ,,,,
}
}이러면 Feed의 수정은 Feed 도메인 내부에서 도메인 규칙을 적용하여 구현할 수 있고, 서비스는 update 하고자 하는 값을 feed에게 전달하여 업데이트를 요청만 하도록 리펙토링이 가능할 것 같습니다!
There was a problem hiding this comment.
훨씬 더 나은 것 같습니다 수정해보도록 하겠습니다
There was a problem hiding this comment.
음 근데 생각해봤는데 command의 tagList와 remainImageUrl의 유효성 검증은
파라미터 자체의 검증, 비즈니스 도메인 규칙 검증(태그 개수 제한, 이미지 개수 제한)
이 모두 관여하는 부분이라 서비스 계층에서 선행 검증하는 것은 어떤가요?
특히 서비스에서 먼저 유효성 검증을 하면, 불필요한 도메인 객체 조회/DB 쿼리 없이 파라미터 차원의 에러를 빠르게 감지할 수 있다는 장점이 있다고 봅니다. (피드가 없는 상황에서 굳이 쿼리를 날리지 않아도 되므로 효율적)
반면 도메인 내부로 검증 책임을 몰아줄 때는 "규칙 보장" 관점의 응집도가 높아지긴 하지만, 단순 파라미터 검증만으로 에러를 낼 수 있는 상황에서도 도메인 객체(Feed) 조회 등 불필요한 쿼리가 먼저 발생할 수 있어 성능 측면에선 아쉬울 것 같습니다.
There was a problem hiding this comment.
도메인 내부에서 update, create 할 시에 필요한 검증을 같이 하고, 서비스는 그저 도메인에게 update, create 를 요청함으로써 도메인 규칙 검증에서 자유로운 구조가 좋지않나 라는 생각에 리뷰를 달아본 것 이긴 한데, 희진님 의견처럼 개수 제한과 같이 파라미터 그 자체로 바로 검증할 수 있는 것은 service 에서 선행적으로 검사하는 것도 괜찮다고 생각합니다!
다만 service에서 개수 제한 검증을 하였더라도, 도메인 레벨에서도 하는것이 좋지않나 싶습니다
DB 쿼리와 관련된 성능을 고려하는 것도 매우 중요하지만, 코드의 가독성, 도메인 규칙의 일관성, 유지보수성 등등 여러 트레이드 오프를 고려해서 개발 및 리펙토링을 진행하는 것도 중요하다는 생각입니다!
There was a problem hiding this comment.
아하 그러면
서비스 계층에서 1차 선행 검사
도메인 내부에서 2차 보증적 검증을 함께 수행하는 방향으로 리팩토링하겠습니다!
| // 1. 피드 생성 비지니스 정책 검증 | ||
| Feed.validateCategoryAndTags(command.category(), command.tagList()); | ||
| Feed.validateTags(command.tagList()); | ||
| Feed.validateImageCount(images != null ? images.size() : 0); |
There was a problem hiding this comment.
p2 : 피드 생성 시에도 service에서 Feed의 static 메서드를 호출하여 태그, 이미지의 개수제한관 관련된 비즈니스 로직을 수행하는것 보다는, Feed.withoutId() 메서드를 통해 id값이 없는 최초 Feed 도메인을 생성하는 시점에 해당 validation을 내부적으로 수행하는건 어떤가요??
이렇게 되면 createFeed 메서드의 코드는
public Long createFeed(command, images) {
// 1. 책 id 값 로드
// 2. 이미지 s3에 업로드
// 3. Feed 도메인 엔티티 생성 (withoutId 메서드 활용)
// 4. 생성한 Feed 도메인 저장
}
class Feed {
withoutId(,,,) {
// 1. validate tags (다른 validation 필요하면 추가로 수행)
// 2. 매핑하여 Feed 도메인 생성
}위처럼 훨씬 간결해지고, Feed 도메인의 생성 시 필요한 도메인 규칙 검증을 빠짐없이 수행할 수 있을 것 같습니다!
| private void addAllContents(Feed feed, FeedJpaEntity feedJpaEntity) { | ||
| if (feed.getContentList().isEmpty()) return; | ||
| List<ContentJpaEntity> contents = feed.getContentList().stream() | ||
| .map(content -> contentMapper.toJpaEntity(content, feedJpaEntity)) | ||
| .toList(); | ||
| contents.forEach(feedJpaEntity.getContentList()::add); | ||
| } | ||
|
|
||
| contentJpaEntities.forEach(feedJpaEntity.getContentList()::add); | ||
| private void saveContents(Feed feed, FeedJpaEntity feedJpaEntity) { | ||
| addAllContents(feed, feedJpaEntity); | ||
| } |
There was a problem hiding this comment.
p3 : saveContents 메서드 없이 addAllContents 메서드만 있는건 어떤가요?
There was a problem hiding this comment.
처음엔 saveContents()를 update와의 공통 로직 분리를 위해 추출했는데, 현재 형태에선 단순 위임만 하는 구조라 오히려 의미가 모호해진 것 같으네요
의도가 더 잘 드러나도록 해당 메서드 이름을 수정해보도록 하겠습니다!
seongjunnoh
left a comment
seongjunnoh
left a comment
There was a problem hiding this comment.
@hd0rable 리뷰 요구사항에 남겨주신 글 확인했습니다!
-
Feed 를 애그리거트 루트, Content는 여기에 속하는 밸류 타입으로 보는게 맞는것 같은데, 저희가 처음에 DB 테이블과 매핑되는 jpa 객체를 모두 entity로 정의해서 @onetomany 어노테이션이 발생하는 것 같네요.
FeedMapper 가 ContentMapper를 의존하여 contentMapper에게 content의 매핑과정을 맡기니 책임이 흩어지지 않아 좋은 것 같습니다! -
Tag, FeedTag, Content, Alias 도 마찬가지로 entity 보다는 밸류 타입으로 생각하는게 맞는것 같은데, 추후에 리펙토링한다면 join, 지연 로딩과 관련된 고민에서 자유로울 수 있을 것 같습니다!
많이 고민하면서 개발하신 것 같네요!! 따봉 드립니다👍👍👍
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/main/java/konkuk/thip/feed/domain/Feed.java (1)
117-123: 이미지 업데이트 로직이 잘 구현되었습니다!작성자 검증, 이미지 개수 제한, 소유권 검증이 모두 적절히 처리되었습니다. 다만 PR 설명에서 언급된 대로, 삭제된 이미지의 S3 처리 전략이 필요합니다.
S3에서 삭제된 이미지를 처리하는 배치 작업이나 이벤트 기반 삭제 로직 구현이 필요하신가요? 새 이슈를 생성해드릴까요?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/main/java/konkuk/thip/feed/adapter/out/persistence/FeedCommandPersistenceAdapter.java(2 hunks)src/main/java/konkuk/thip/feed/application/service/FeedUpdateService.java(1 hunks)src/main/java/konkuk/thip/feed/domain/Feed.java(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/java/konkuk/thip/feed/application/service/FeedUpdateService.java
- src/main/java/konkuk/thip/feed/adapter/out/persistence/FeedCommandPersistenceAdapter.java
🔇 Additional comments (6)
src/main/java/konkuk/thip/feed/domain/Feed.java (6)
41-42: contentList 필드 추가가 적절합니다!빌더 기본값으로 빈 ArrayList를 설정하여 null 방지를 잘 처리했습니다.
47-48: 카테고리 검증 제거 및 null 체크 추가가 적절합니다!PR 목적에 맞게 카테고리 검증이 제거되었고, imageUrls의 null 체크가 추가되어 안전성이 향상되었습니다.
65-65: null 체크 추가로 안전성이 향상되었습니다!imageUrls가 null일 때 빈 리스트를 반환하여 NullPointerException을 방지합니다.
72-92: 태그 검증 로직과 에러 코드 통합이 적절합니다!카테고리 검증 제거와 INVALID_FEED_COMMAND로의 에러 코드 통합이 PR 목적에 부합합니다.
95-99: 작성자 검증 로직이 보안상 중요하고 잘 구현되었습니다!피드 수정 권한을 작성자로 제한하는 검증이 적절히 구현되었습니다.
125-135: 이미지 소유권 검증 로직이 효율적으로 구현되었습니다!Set을 사용한 O(1) 조회와 구체적인 에러 메시지 제공이 좋습니다.
3 participants
#️⃣ 연관된 이슈
📝 작업 내용
FeedIdResponse로 통일했습니다.INVALID_FEED_COMMAND로 통일하였습니다.📸 스크린샷
💬 리뷰 요구사항
해당FeedMappe에 ContentMapper를 의존관계를 갖도록 하여 구현했는데 피드 도메인(Feed)과 콘텐츠(Content)는 양방향 연관관계를 갖고 있으며, Feed가 애그리거트 루트로서 콘텐츠 리스트도 함께 관리하는 구조입니다.
따라서 도메인 객체 변환시점에서 Feed ↔ Content 간 매핑 정보도 함께 변환되어야 했습니다.
FeedJpaEntity → Feed(Domain) 변환 시 외부에서 Content 리스트를 따로 조회하거나 주입하는 것은 애그리거트 내부 연관 데이터를 외부에서 처리하는 책임 분산의 문제가 있어, FeedMapper 내부에서 ContentMapper를 의존성 주입받아 직접 처리하도록 구현했습니다.
도메인 규칙상, Feed는 자신이 가지는 콘텐츠의 생명주기를 함께 관리해야 하므로, 연관된 Content의 변환도 FeedMapper 내부에서 수행되는 것이 가장 자연스럽고 명확하다고 판단하여 해당방식과 같이 구현했는데 이 방식에대해 더 좋은 개선방안이 있다면 리뷰 부탁드립니다.
Feed와 Tag의 관계는 다대다(N:N)이며, 관계 테이블인 FeedTagJpaEntity를 통해 연결되어 있기 때문에, Feed와 Tag를 직접 조인하려면 중간 엔티티 조인(feed_tag)이 불가피한 구조입니다.
관계 테이블 조인은 Feed → FeedTag → Tag의 단순한 1회 Join만으로도 충분하기 때문에 JPQL 기반으로 깔끔하게 처리할 수 있다고 판단했습니다.
마찬가지로, Feed에 연결된 태그 정보를 제거하는 deleteAllByFeedJpaEntity 역시 복잡한 쿼리 없이 Feed 기준으로 중간 관계 테이블만 삭제하면 되므로, JPQL + @Modifying 조합으로 구현하는 것이 깔끔하다고 생각했습니다.
관련해서 더 좋은 개선방안이 있다면 리뷰 부탁드립니다.
📌 PR 진행 시 이러한 점들을 참고해 주세요
Summary by CodeRabbit
Summary by CodeRabbit
신규 기능
기능 개선
버그 수정
테스트
기타