-
Notifications
You must be signed in to change notification settings - Fork 1
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
[DDING-55] Feed 삭제 API 구현 #200
Conversation
워크스루이 풀 리퀘스트는 클럽 피드 관련 API와 서비스에 대한 포괄적인 기능 확장을 다룹니다. 주요 변경 사항은 피드 삭제 기능의 추가로, API 엔드포인트 경로 구조 조정과 함께 피드 삭제를 위한 새로운 메서드들이 여러 계층에 걸쳐 구현되었습니다. 이 변경은 피드 관리 기능을 완성하고 API의 일관성을 개선합니다. 변경 사항
가능한 관련 PR
제안된 레이블
제안된 리뷰어
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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
Documentation and Community
|
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: 1
🧹 Nitpick comments (4)
src/main/java/ddingdong/ddingdongBE/domain/feed/service/GeneralFeedService.java (1)
41-45
: 로깅 추가를 제안드립니다.피드 삭제 작업에 대한 추적을 위해 로깅을 추가하면 좋을 것 같습니다.
다음과 같이 수정을 제안합니다:
@Transactional public void delete(Feed feed) { + log.info("피드 삭제 시작 - Feed ID: {}", feed.getId()); feedRepository.delete(feed); + log.info("피드 삭제 완료 - Feed ID: {}", feed.getId()); }src/main/java/ddingdong/ddingdongBE/domain/feed/service/FacadeClubFeedServiceImpl.java (1)
48-61
: 피드 삭제 로직이 잘 구현되었습니다.트랜잭션 처리와 파일 메타데이터 상태 업데이트가 적절히 구현되었습니다. 다만 다음 사항을 고려해보시기 바랍니다:
- 삭제 권한 검증 로직이 필요할 수 있습니다.
- 삭제된 피드에 대한 감사(audit) 로깅을 추가하면 좋을 것 같습니다.
src/test/java/ddingdong/ddingdongBE/domain/feed/service/FacadeClubFeedServiceImplTest.java (2)
51-52
: 사용되지 않는 의존성이 있습니다.
fileMetaDataServiceImpl
이 테스트 메서드에서 사용되지 않는 것으로 보입니다. 필요하지 않다면 제거하는 것이 좋겠습니다.
118-148
: 테스트 케이스가 잘 구현되었습니다.삭제 기능에 대한 테스트가 다음 사항들을 잘 검증하고 있습니다:
- 피드 엔티티 삭제 확인
- 파일 메타데이터 상태 변경 확인
추가로 고려해볼 사항들:
- 비디오 타입 피드 삭제 케이스도 테스트하면 좋을 것 같습니다.
- 존재하지 않는 피드 ID에 대한 예외 케이스도 테스트하면 좋을 것 같습니다.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/main/java/ddingdong/ddingdongBE/domain/feed/api/ClubFeedApi.java
(3 hunks)src/main/java/ddingdong/ddingdongBE/domain/feed/controller/ClubFeedController.java
(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/feed/service/FacadeClubFeedService.java
(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/feed/service/FacadeClubFeedServiceImpl.java
(2 hunks)src/main/java/ddingdong/ddingdongBE/domain/feed/service/FeedService.java
(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/feed/service/GeneralFeedService.java
(1 hunks)src/test/java/ddingdong/ddingdongBE/domain/feed/service/FacadeClubFeedServiceImplTest.java
(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build and analyze
🔇 Additional comments (4)
src/main/java/ddingdong/ddingdongBE/domain/feed/service/FeedService.java (1)
15-16
: 인터페이스에 삭제 기능이 적절히 추가되었습니다.Feed 엔티티를 매개변수로 받는 delete 메서드가 명확하게 정의되었습니다.
src/main/java/ddingdong/ddingdongBE/domain/feed/service/FacadeClubFeedService.java (1)
12-12
: Facade 계층에 적합한 삭제 메서드가 추가되었습니다.ID를 매개변수로 받는 것이 Facade 패턴의 관점에서 적절합니다.
src/main/java/ddingdong/ddingdongBE/domain/feed/service/FacadeClubFeedServiceImpl.java (1)
32-32
: 코드 최적화가 잘 되었습니다!이미지 처리 후 불필요한 비디오 체크를 방지하기 위한 early return 추가가 적절합니다.
src/main/java/ddingdong/ddingdongBE/domain/feed/api/ClubFeedApi.java (1)
45-52
: API 엔드포인트가 일관성 있게 구현되었습니다.삭제 API가 다음과 같이 잘 정의되었습니다:
- 적절한 HTTP 메소드(@DeleteMapping) 사용
- 보안 요구사항 명시
- 명확한 응답 코드(204) 정의
- Swagger 문서화 완료
@Override | ||
public void deleteFeed(Long feedId) { | ||
facadeClubFeedService.delete(feedId); | ||
} |
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.
💡 Codebase verification
보안 검증이 누락된 것이 확인되었습니다.
ClubFeedApi 인터페이스에 정의된 보안 요구사항(@securityrequirement)에 맞게 구현되어야 하나, updateFeed와 deleteFeed 메서드 모두에서 인증 파라미터(PrincipalDetails)가 누락되어 있습니다. 다음과 같이 수정이 필요합니다:
- public void updateFeed(Long feedId, UpdateFeedRequest updateFeedRequest) {
+ public void updateFeed(Long feedId, UpdateFeedRequest updateFeedRequest, PrincipalDetails principalDetails) {
+ User user = principalDetails.getUser();
facadeClubFeedService.update(updateFeedRequest.toCommand(feedId));
}
- public void deleteFeed(Long feedId) {
+ public void deleteFeed(Long feedId, PrincipalDetails principalDetails) {
+ User user = principalDetails.getUser();
facadeClubFeedService.delete(feedId);
}
🔗 Analysis chain
보안 검증이 필요합니다.
다른 엔드포인트들과 달리 deleteFeed 메서드에는 인증 검사(PrincipalDetails)가 누락되어 있습니다. 피드 삭제 권한이 있는 사용자만 접근할 수 있도록 보안 검증이 필요합니다.
다음과 같이 수정을 제안합니다:
- public void deleteFeed(Long feedId) {
+ public void deleteFeed(Long feedId, PrincipalDetails principalDetails) {
+ User user = principalDetails.getUser();
facadeClubFeedService.delete(feedId);
}
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Get the complete controller file
cat src/main/java/ddingdong/ddingdongBE/domain/feed/controller/ClubFeedController.java
# Search for security related configurations
rg "@Security|@PreAuthorize|@Secured" -A 2
# Find service implementation
fd "ClubFeedService" --type f
Length of output: 20533
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.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
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.
webhook test입니다
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: 1
🧹 Nitpick comments (2)
src/test/java/ddingdong/ddingdongBE/domain/feed/service/FacadeClubFeedServiceImplTest.java (2)
51-52
: 사용되지 않는 필드가 발견되었습니다.
fileMetaDataServiceImpl
필드가 테스트 메서드에서 사용되지 않고 있습니다. 불필요한 의존성은 제거하는 것이 좋습니다.- @Autowired - private FileMetaDataServiceImpl fileMetaDataServiceImpl;
118-150
: 이미지 Feed 삭제 테스트 코드의 개선이 필요합니다.테스트 코드가 기본적인 기능을 검증하고 있으나, 다음과 같은 개선사항이 있습니다:
- 불필요한 빈 줄이 있습니다 (123-124줄).
- 실패 케이스에 대한 테스트가 누락되어 있습니다 (예: 존재하지 않는 feedId).
- 테스트 데이터 설정 부분을 별도의 메서드로 추출하면 코드 재사용성이 향상될 것 같습니다.
다음과 같이 개선해 보시는 것은 어떨까요?:
@Test void deleteImage_WithNonExistentFeed_ShouldThrowException() { // given Long nonExistentFeedId = 999L; // when & then assertThatThrownBy(() -> facadeClubFeedService.delete(nonExistentFeedId)) .isInstanceOf(EntityNotFoundException.class); } private Feed createImageFeed() { return feedRepository.save( fixtureMonkey.giveMeBuilder(Feed.class) .set("feedType", FeedType.IMAGE) .set("activityContent", "활동내용") .set("club", null) .sample() ); } private FileMetaData createFileMetaData(UUID uuid, Feed feed, DomainType domainType) { return fileMetaDataRepository.save( fixtureMonkey.giveMeBuilder(FileMetaData.class) .set("id", uuid) .set("entityId", feed.getId()) .set("domainType", domainType) .set("fileStatus", FileStatus.COUPLED) .sample() ); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/test/java/ddingdong/ddingdongBE/domain/feed/service/FacadeClubFeedServiceImplTest.java
(3 hunks)
@DisplayName("주어진 feedId를 가진 Feed 엔터티를 삭제 및 fileMetaData 상태를 DELETED로 변경 - VIDEO") | ||
@Test | ||
void deleteVideo() { | ||
// given | ||
UUID uuid = UuidCreator.getTimeOrderedEpoch(); | ||
|
||
|
||
Feed savedFeed = feedRepository.save( | ||
fixtureMonkey.giveMeBuilder(Feed.class) | ||
.set("feedType", FeedType.VIDEO) | ||
.set("activityContent", "활동내용") | ||
.set("club", null) | ||
.sample() | ||
); | ||
fileMetaDataRepository.save( | ||
fixtureMonkey.giveMeBuilder(FileMetaData.class) | ||
.set("id", uuid) | ||
.set("entityId", savedFeed.getId()) | ||
.set("domainType", DomainType.FEED_VIDEO) | ||
.set("fileStatus", FileStatus.COUPLED) | ||
.sample() | ||
); | ||
entityManager.flush(); | ||
// when | ||
facadeClubFeedService.delete(savedFeed.getId()); | ||
entityManager.flush(); | ||
// then | ||
Feed feed = feedRepository.findById(savedFeed.getId()).orElse(null); | ||
FileMetaData fileMetaData = fileMetaDataRepository.findById(uuid).orElse(null); | ||
assertThat(feed).isNull(); | ||
assertThat(fileMetaData).isNotNull(); | ||
assertThat(fileMetaData.getFileStatus()).isEqualTo(FileStatus.DELETED); | ||
} |
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.
🛠️ Refactor suggestion
비디오 Feed 삭제 테스트의 중복 코드를 제거해야 합니다.
deleteVideo()
테스트 메서드가 deleteImage()
메서드와 거의 동일한 구조를 가지고 있습니다. 테스트 코드의 중복을 제거하고 가독성을 높이기 위해 파라미터화된 테스트로 리팩토링하는 것이 좋을 것 같습니다.
다음과 같이 개선해 보시는 것은 어떨까요?:
@ParameterizedTest
@EnumSource(FeedType.class)
void deleteFeed_ShouldDeleteFeedAndUpdateFileMetaData(FeedType feedType) {
// given
UUID uuid = UuidCreator.getTimeOrderedEpoch();
Feed savedFeed = createFeed(feedType);
createFileMetaData(uuid, savedFeed,
feedType == FeedType.IMAGE ? DomainType.FEED_IMAGE : DomainType.FEED_VIDEO);
entityManager.flush();
// when
facadeClubFeedService.delete(savedFeed.getId());
entityManager.flush();
// then
assertThat(feedRepository.findById(savedFeed.getId())).isEmpty();
FileMetaData fileMetaData = fileMetaDataRepository.findById(uuid).orElse(null);
assertThat(fileMetaData)
.isNotNull()
.extracting(FileMetaData::getFileStatus)
.isEqualTo(FileStatus.DELETED);
}
private Feed createFeed(FeedType feedType) {
return feedRepository.save(
fixtureMonkey.giveMeBuilder(Feed.class)
.set("feedType", feedType)
.set("activityContent", "활동내용")
.set("club", null)
.sample()
);
}
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.
코멘트 확인 부탁드립니다~
if (feed.isImage()) { | ||
fileMetaDataService.updateStatusToDelete(DomainType.FEED_IMAGE, feedId); | ||
return; | ||
} | ||
|
||
if (feed.isVideo()) { | ||
fileMetaDataService.updateStatusToDelete(DomainType.FEED_VIDEO, feedId); | ||
} |
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.
p3)
두 조건 모두 수행하는 코드가 상태를 DELETED로 바꾸는 것인데 이미지인지 비디오인지 체크하는 이유가 있을까요?
updateStatusToDelete
메서드가 해당 파리미터를 필요로 하는것이라면 상태면 수정하는 메서드를 새로 만드는게 나을거 같습니다!
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: 1
🧹 Nitpick comments (1)
src/main/java/ddingdong/ddingdongBE/domain/filemetadata/entity/DomainType.java (1)
32-36
: 클래스 타입 비교 시equals()
메서드 사용 권장
findAllByClassType
메서드에서 클래스 타입 비교를 위해==
연산자를 사용하고 있습니다. 클래스 로더에 따라 동일한 클래스를 가리키더라도 다른Class
객체가 될 수 있으므로, 정확한 비교를 위해equals()
메서드 사용을 권장합니다.다음과 같이 수정하는 것을 제안합니다:
- .filter(domainType -> domainType.getClassType() == classType) + .filter(domainType -> domainType.getClassType().equals(classType))
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/main/java/ddingdong/ddingdongBE/domain/feed/service/FacadeClubFeedServiceImpl.java
(2 hunks)src/main/java/ddingdong/ddingdongBE/domain/filemetadata/entity/DomainType.java
(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/filemetadata/service/FileMetaDataService.java
(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/filemetadata/service/FileMetaDataServiceImpl.java
(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build and analyze
🔇 Additional comments (5)
src/main/java/ddingdong/ddingdongBE/domain/filemetadata/entity/DomainType.java (2)
15-16
: 잘 구현된 Lombok 어노테이션 사용
@RequiredArgsConstructor
와@Getter
어노테이션을 사용하여 필요한 생성자와 게터 메서드를 자동으로 생성하였습니다. 이는 코드의 간결성과 유지보수성 향상에 도움이 됩니다.
18-28
: Enum 상수에 클래스 타입 추가로 확장성 향상각
DomainType
enum 상수에 대응하는Class<?>
타입을 추가하여 도메인 타입과 클래스 타입의 매핑을 명확하게 하였습니다. 이는 이후에 클래스 타입 기반의 처리가 필요할 때 유용하게 활용될 수 있습니다.src/main/java/ddingdong/ddingdongBE/domain/filemetadata/service/FileMetaDataService.java (1)
38-38
: 인터페이스에 새로운 메서드 추가로 기능 확장
updateStatusToDeleteByEntityId(Class<?> classType, Long entityId)
메서드를 추가하여 특정 클래스 타입과 엔티티 ID에 기반한 파일 메타데이터의 상태 업데이트 기능을 인터페이스에 포함시켰습니다. 이는 서비스의 활용성을 높이는 적절한 확장으로 보입니다.src/main/java/ddingdong/ddingdongBE/domain/feed/service/FacadeClubFeedServiceImpl.java (2)
32-32
: 불필요한 미디어 타입 체크 로직이 있습니다.이전 리뷰에서 지적된 것처럼, 상태 업데이트만 수행하는데 이미지/비디오 타입을 구분하는 것이 불필요해 보입니다.
다음과 같이 리팩토링하는 것을 제안합니다:
- if (feed.isImage()) { - fileMetaDataService.updateStatusToCoupled(command.mediaId(), DomainType.FEED_IMAGE, createdId); - return; - } - - if (feed.isVideo()) { - fileMetaDataService.updateStatusToCoupled(command.mediaId(), DomainType.FEED_VIDEO, createdId); - } + DomainType domainType = feed.isImage() ? DomainType.FEED_IMAGE : DomainType.FEED_VIDEO; + fileMetaDataService.updateStatusToCoupled(command.mediaId(), domainType, createdId);
48-54
: 피드 삭제 구현이 잘 되었습니다.트랜잭션 처리와 관심사 분리가 잘 되어있습니다. 다만 몇 가지 제안사항이 있습니다:
- 삭제 권한 검증 로직이 필요해 보입니다.
- 삭제된 피드에 대한 재삭제 시도 시 예외 처리가 필요할 수 있습니다.
권한 검증 관련 코드를 확인하기 위해 다음 스크립트를 실행해보겠습니다:
@Override | ||
public void updateStatusToDeleteByEntityId(Class<?> classType, Long entityId) { | ||
List<DomainType> domainTypes = DomainType.findAllByClassType(classType); | ||
domainTypes.forEach(domainType -> { | ||
updateStatusToDelete(domainType, entityId); | ||
}); | ||
} |
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.
데이터 변경 메서드에 @transactional 어노테이션 추가 필요
updateStatusToDeleteByEntityId
메서드는 데이터베이스에서 파일 메타데이터의 상태를 업데이트하는 로직을 포함하고 있지만, 현재 @Transactional
어노테이션이 적용되어 있지 않습니다. 클래스 레벨에서 @Transactional(readOnly = true)
로 설정되어 있기 때문에, 이 메서드에 별도로 @Transactional
을 추가해야 데이터 변경이 제대로 이루어집니다.
다음과 같이 어노테이션을 추가할 것을 제안합니다:
+ @Transactional
@Override
public void updateStatusToDeleteByEntityId(Class<?> classType, Long entityId) {
List<DomainType> domainTypes = DomainType.findAllByClassType(classType);
domainTypes.forEach(domainType -> {
updateStatusToDelete(domainType, entityId);
});
}
📝 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.
@Override | |
public void updateStatusToDeleteByEntityId(Class<?> classType, Long entityId) { | |
List<DomainType> domainTypes = DomainType.findAllByClassType(classType); | |
domainTypes.forEach(domainType -> { | |
updateStatusToDelete(domainType, entityId); | |
}); | |
} | |
@Transactional | |
@Override | |
public void updateStatusToDeleteByEntityId(Class<?> classType, Long entityId) { | |
List<DomainType> domainTypes = DomainType.findAllByClassType(classType); | |
domainTypes.forEach(domainType -> { | |
updateStatusToDelete(domainType, entityId); | |
}); | |
} |
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: 1
🧹 Nitpick comments (2)
src/main/java/ddingdong/ddingdongBE/domain/feed/entity/FeedType.java (2)
11-12
: FeedType과 DomainType의 연관성이 명확해졌습니다.각
FeedType
에 대응하는DomainType
을 명시적으로 연결하여 타입 간의 관계가 명확해졌습니다. 이는 파일 메타데이터 처리 시 도메인 타입을 쉽게 식별할 수 있게 해줍니다.하지만 한 가지 제안사항이 있습니다:
public enum FeedType { - IMAGE(DomainType.FEED_IMAGE), - VIDEO(DomainType.FEED_VIDEO); + IMAGE(DomainType.FEED_IMAGE, "image"), + VIDEO(DomainType.FEED_VIDEO, "video"); private final DomainType domainType; + private final String contentType;
contentType
필드를 추가하면findByContentType
메서드에서 대소문자 구분 없이 문자열 비교를 할 때 더 안전하게 처리할 수 있습니다.Also applies to: 14-14
Line range hint
16-22
: findByContentType 메서드의 예외 처리가 적절합니다.사용자가 이해하기 쉬운 예외 메시지를 제공하고 있습니다. 하지만 몇 가지 개선사항을 제안드립니다:
- 상수로 에러 메시지를 분리하는 것이 좋습니다:
private static final String INVALID_CONTENT_TYPE_MESSAGE = "Feed 내 해당 컨텐츠 종류(%s)는 지원하지 않습니다.";
- String.format을 사용하여 가독성을 높이는 것이 좋습니다:
throw new IllegalArgumentException(String.format(INVALID_CONTENT_TYPE_MESSAGE, contentType));
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/main/java/ddingdong/ddingdongBE/domain/feed/entity/FeedType.java
(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/feed/service/FacadeClubFeedServiceImpl.java
(2 hunks)src/main/java/ddingdong/ddingdongBE/domain/filemetadata/entity/DomainType.java
(2 hunks)src/main/java/ddingdong/ddingdongBE/domain/filemetadata/service/FileMetaDataServiceImpl.java
(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/java/ddingdong/ddingdongBE/domain/filemetadata/entity/DomainType.java
- src/main/java/ddingdong/ddingdongBE/domain/filemetadata/service/FileMetaDataServiceImpl.java
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build and analyze
🔇 Additional comments (5)
src/main/java/ddingdong/ddingdongBE/domain/feed/entity/FeedType.java (2)
3-3
: Lombok 어노테이션 사용이 적절합니다.
@Getter
와@RequiredArgsConstructor
어노테이션을 사용하여 코드를 간결하게 유지하였습니다.Also applies to: 5-6, 8-9
3-3
: DomainType 사용의 일관성 검증이 필요합니다.
DomainType.FEED_IMAGE
와DomainType.FEED_VIDEO
가 파일 메타데이터 처리 로직에서 일관되게 사용되는지 확인이 필요합니다.✅ Verification successful
DomainType이 일관되게 사용되고 있음을 확인했습니다.
FeedType
enum에서FEED_IMAGE
와FEED_VIDEO
타입이 적절하게 매핑되어 있습니다.- 서비스 계층과 테스트 코드에서도 동일한 패턴으로 사용되고 있습니다.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # DomainType 사용 검증 rg "DomainType\.FEED_" -t javaLength of output: 1248
src/main/java/ddingdong/ddingdongBE/domain/feed/service/FacadeClubFeedServiceImpl.java (3)
32-32
: 적절한 early return 추가로 코드 최적화이미지 피드 처리 후 early return을 추가하여 불필요한 비디오 타입 체크를 방지하였습니다. 코드가 더 효율적이고 명확해졌습니다.
53-53
: 상태 업데이트 로직 개선 제안
updateStatusToDelete
메서드에DomainType
을 전달하는 현재 방식은 이전 리뷰에서도 지적된 부분입니다. 상태 업데이트만을 위한 별도의 메서드를 만드는 것이 더 명확할 것 같습니다.제안:
-fileMetaDataService.updateStatusToDelete(feed.getFeedType().getDomainType(), feed.getId()); +fileMetaDataService.markAsDeleted(feed.getId());
50-54
: 예외 처리 보완 필요피드나 관련 파일 메타데이터가 존재하지 않는 경우에 대한 명시적인 예외 처리가 필요합니다.
다음 시나리오들에 대한 테스트 케이스 추가를 검토해주세요:
- 존재하지 않는 피드 ID
- 이미 삭제된 피드
- 파일 메타데이터 업데이트 실패
public void delete(Long feedId) { | ||
Feed feed = feedService.getById(feedId); | ||
feedService.delete(feed); | ||
fileMetaDataService.updateStatusToDelete(feed.getFeedType().getDomainType(), feed.getId()); | ||
} |
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.
권한 검증 로직 추가 필요
현재 구현에서는 피드 삭제 전에 사용자의 권한이나 소유권을 확인하는 로직이 없습니다. 보안을 위해 삭제 권한 검증을 추가하는 것이 좋겠습니다.
예시 구현:
@Transactional
public void delete(Long feedId) {
Feed feed = feedService.getById(feedId);
+ Club club = clubService.getByUserId(SecurityUtil.getCurrentUserId());
+ if (!feed.getClub().equals(club)) {
+ throw new UnauthorizedException("피드 삭제 권한이 없습니다.");
+ }
feedService.delete(feed);
fileMetaDataService.updateStatusToDelete(feed.getFeedType().getDomainType(), feed.getId());
}
Committable suggestion skipped: line range outside the PR's diff.
Quality Gate passedIssues Measures |
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.
고생하셨습니다!
🚀 작업 내용
Summary by CodeRabbit
릴리즈 노트
새로운 기능
버그 수정
개선 사항
이번 업데이트는 클럽 피드 관리 기능을 확장하고 시스템의 안정성을 높였습니다.