[DDING-51] VodProcessingJob 완료 시 notification 전송 구현 #205
Conversation
# Conflicts: # src/main/java/ddingdong/ddingdongBE/domain/vodprocessing/service/FacadeVodProcessingJobServiceImpl.java # src/main/java/ddingdong/ddingdongBE/domain/vodprocessing/service/dto/command/CreatePendingVodProcessingJobCommand.java # src/test/java/ddingdong/ddingdongBE/domain/vodprocessing/service/FacadeGeneralVodProcessingJobServiceTest.java
…gJobStatus에 따른 notifiaction 전송 로직 추가
|
""" 워크스루이 풀 리퀘스트는 비디오 온디맨드(VOD) 처리 및 피드 관리 시스템의 여러 구성 요소를 개선합니다. 주요 변경 사항은 VOD 처리 작업의 알림 상태 관리, 서버 전송 이벤트(SSE)를 통한 작업 상태 업데이트, 그리고 피드 및 VOD 처리 작업 간의 상호작용을 강화하는 것입니다. 새로운 엔티티, 서비스, 리포지토리가 추가되어 시스템의 기능성과 유연성을 향상시켰습니다. 변경 사항
시퀀스 다이어그램sequenceDiagram
participant Client
participant FacadeVodProcessingJobService
participant VodProcessingJobService
participant FeedService
participant SseConnectionService
Client->>FacadeVodProcessingJobService: Update VOD Processing Job Status
FacadeVodProcessingJobService->>VodProcessingJobService: Update Job Status
FacadeVodProcessingJobService->>FeedService: Find Feed by ID
alt Feed Exists
FacadeVodProcessingJobService->>SseConnectionService: Send SSE Event
end
관련 가능성 있는 PR
제안된 리뷰어
Finishing Touches
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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (14)
src/main/java/ddingdong/ddingdongBE/domain/feed/service/FacadeClubFeedServiceImpl.java (2)
46-46: 예외 처리 추가를 고려해 주세요.알림 전송 과정에서 발생할 수 있는 예외가 피드 생성 트랜잭션 전체를 롤백시킬 수 있습니다. 알림 전송 실패가 피드 생성에 영향을 주지 않도록 예외 처리를 고려해 주세요.
예시:
try { checkVodProcessingJobAndNotify(feed); } catch (Exception e) { log.error("Failed to send notification for feed: {}", feed.getId(), e); // 필요한 경우 모니터링을 위한 메트릭 추가 }
69-73: SseEvent 생성 로직 분리를 고려해 주세요.
SseEvent생성 로직을 별도의 팩토리 메서드로 분리하면 테스트와 유지보수가 더 용이할 것 같습니다.private SseEvent<ConvertJobStatus> createVodProcessingEvent(ConvertJobStatus status) { return SseEvent.of( VOD_PROCESSING_EVENT, status, LocalDateTime.now() ); }src/main/java/ddingdong/ddingdongBE/domain/feed/service/FeedService.java (1)
15-15: 메서드 추가가 적절해 보입니다만, 문서화가 필요합니다.기존의
getById와 새로운findById메서드의 차이점을 명확히 하기 위해 Javadoc 주석 추가를 권장드립니다.+ /** + * Feed를 ID로 조회합니다. + * getById와 달리 Feed가 존재하지 않을 경우 예외를 발생시키지 않고 Optional.empty()를 반환합니다. + * + * @param feedId 조회할 Feed의 ID + * @return Optional<Feed> 조회된 Feed를 Optional로 감싸서 반환 + */ Optional<Feed> findById(Long feedId);src/main/java/ddingdong/ddingdongBE/sse/service/SseConnectionService.java (1)
Line range hint
16-48: 로그 메시지의 일관성 개선이 필요합니다.새로 추가된
send메소드의 한글 로그 메시지와의 일관성을 위해,subscribe메소드의 로그 메시지도 한글로 변경하는 것이 좋겠습니다.- log.info("SSE Connection established for user: {}", id); + log.info("SSE 연결 설정 완료 - userId: {}", id); // 연결 에러 콜백 sseEmitter.onError((ex) -> { - log.warn("SSE Connection error: {}", ex.getMessage()); + log.warn("SSE 연결 오류 발생: {}", ex.getMessage()); sseConnectionRepository.deleteById(id); }); try { sseEmitter.send( SseEmitter.event() .id(id) .name("connect") - .data("Connected successfully!") + .data("연결이 성공적으로 설정되었습니다!") ); } catch (IOException e) { - log.error("Error sending initial SSE event to user: {}", id, e); + log.error("초기 SSE 이벤트 전송 실패 - userId: {}", id, e); sseConnectionRepository.deleteById(id); }src/main/java/ddingdong/ddingdongBE/sse/service/dto/SseEvent.java (1)
11-13: 정적 팩토리 메서드 패턴이 잘 적용되었습니다.
of메서드를 통해 인스턴스 생성을 캡슐화한 것이 좋습니다. 하지만 몇 가지 개선 사항을 제안드립니다:public static <T> SseEvent<T> of(String eventName, T data, LocalDateTime timestamp) { + if (eventName == null || eventName.isBlank()) { + throw new IllegalArgumentException("이벤트 이름은 필수입니다."); + } + if (data == null) { + throw new IllegalArgumentException("데이터는 null일 수 없습니다."); + } + if (timestamp == null) { + timestamp = LocalDateTime.now(); + } return new SseEvent<>(eventName, data, timestamp); }
- 매개변수 유효성 검사를 추가하면 좋을 것 같습니다.
- timestamp가 null일 경우 현재 시간을 사용하도록 하면 편의성이 향상될 것 같습니다.
src/main/java/ddingdong/ddingdongBE/domain/vodprocessing/service/dto/command/CreatePendingVodProcessingJobCommand.java (1)
14-22: 아키텍처 설계가 잘 되었습니다.다음과 같은 점들이 긍정적입니다:
- VodProcessingJob과 VodProcessingNotification의 관계가 명확합니다
- 생성 시점부터 알림 상태를 관리할 수 있도록 구현되었습니다
- Builder 패턴을 통해 객체 생성이 깔끔하게 처리되었습니다
하나 제안드리고 싶은 점이 있습니다:
notification 파라미터가 null이 되는 경우에 대한 방어 로직 추가를 고려해보시면 좋을 것 같습니다:
public VodProcessingJob toPendingVodProcessingJob(VodProcessingNotification notification, FileMetaData fileMetaData) { + if (notification == null) { + throw new IllegalArgumentException("VodProcessingNotification must not be null"); + } return VodProcessingJob.builder()src/main/java/ddingdong/ddingdongBE/domain/vodprocessing/entity/VodNotificationStatus.java (1)
4-8: 각 상태값에 대한 설명 문서화 필요각 상태값(
PENDING,SENT,COMPLETE,FAIL,EXPIRED)의 의미와 상태 전이 조건을 JavaDoc으로 문서화하면 좋을 것 같습니다.예시:
public enum VodNotificationStatus { + /** 알림 대기 상태 */ PENDING, + /** 알림 전송됨 */ SENT, + /** 알림 처리 완료 */ COMPLETE, + /** 알림 처리 실패 */ FAIL, + /** 알림 만료됨 */ EXPIRED }src/main/java/ddingdong/ddingdongBE/domain/vodprocessing/service/VodProcessingNotificationService.java (1)
5-9: 알림 서비스 인터페이스 기능 확장 제안현재는 저장 기능만 있지만, 다음과 같은 추가 기능이 필요할 것 같습니다:
- 알림 상태 조회
- 알림 상태 업데이트
- 만료된 알림 조회
- 실패한 알림 재시도
예시:
public interface VodProcessingNotificationService { VodProcessingNotification save(VodProcessingNotification vodProcessingNotification); VodProcessingNotification getById(Long id); List<VodProcessingNotification> findByStatus(VodNotificationStatus status); VodProcessingNotification updateStatus(Long id, VodNotificationStatus status); List<VodProcessingNotification> findExpiredNotifications(); void retryFailedNotification(Long id); }src/main/java/ddingdong/ddingdongBE/domain/vodprocessing/service/VodProcessingJobService.java (1)
13-14: 메서드 문서화 필요새로 추가된
getByVideoFeedId메서드에 대한 JavaDoc 문서화가 필요합니다:
- 메서드의 목적
- 파라미터 설명
- 반환값 설명
- 발생 가능한 예외 상황
+ /** + * 비디오 피드 ID로 VOD 처리 작업을 조회합니다. + * + * @param videoFeedId 조회할 비디오 피드의 ID + * @return 조회된 VOD 처리 작업 + * @throws EntityNotFoundException 해당 ID의 처리 작업이 존재하지 않는 경우 + */ VodProcessingJob getByVideoFeedId(Long videoFeedId);src/main/java/ddingdong/ddingdongBE/domain/vodprocessing/service/GeneralVodProcessingJobService.java (1)
39-46: 메서드 문서화가 필요합니다.다른 개발자들의 이해를 돕기 위해 메서드의 목적, 파라미터, 예외 상황 등을 JavaDoc으로 문서화하면 좋을 것 같습니다.
+ /** + * 비디오 피드 ID로 VOD 처리 작업을 조회합니다. + * + * @param videoFeedId 조회할 비디오 피드의 ID + * @return 조회된 VOD 처리 작업 + * @throws ResourceNotFound 해당 ID의 VOD 처리 작업이 존재하지 않는 경우 + */ @Override public VodProcessingJob getByVideoFeedId(Long videoFeedId) {src/main/java/ddingdong/ddingdongBE/domain/vodprocessing/service/FacadeVodProcessingJobServiceImpl.java (1)
34-37: 트랜잭션 범위 최적화가 필요합니다.알림 저장과 VOD 처리 작업 저장이 하나의 트랜잭션으로 묶여있습니다. 두 작업이 반드시 같은 트랜잭션에서 실행되어야 하는지 검토가 필요합니다.
src/test/java/ddingdong/ddingdongBE/domain/vodprocessing/service/FacadeGeneralVodProcessingJobServiceTest.java (1)
54-64: 테스트 검증 로직 개선사항테스트 검증이 구조화되어 있고, VodProcessingNotification의 상태 검증이 적절히 추가되었습니다. 다만, 다음 사항을 고려해보시기 바랍니다:
- 알림 상태가 PENDING인 이유에 대한 테스트 설명 추가
- 실패 케이스에 대한 테스트 추가 고려
src/test/java/ddingdong/ddingdongBE/domain/vodprocessing/service/FacadeGeneralVodProcessingJobServiceMockingTest.java (2)
45-86: SSE 이벤트 발송 테스트 개선 제안파라미터화된 테스트가 잘 구현되어 있으나, 다음 개선사항을 고려해보세요:
- SseEvent의 구체적인 내용 검증 추가
- 발송되는 이벤트의 타입과 페이로드 검증
다음과 같이 verify 부분을 개선할 수 있습니다:
- verify(sseConnectionService).send(eq(userId.toString()), any(SseEvent.class)); + verify(sseConnectionService).send(eq(userId.toString()), argThat(event -> { + return event.getType().equals("VOD_PROCESSING") && + event.getPayload().contains(convertJobStats); + }));
1-127: 아키텍처 관련 제안사항Facade 패턴을 통한 구현이 잘 되어 있습니다. 다만, 다음 사항들을 고려해보시면 좋겠습니다:
- SSE 연결 실패시 재시도 메커니즘 추가 검토
- 알림 발송 실패에 대한 로깅 추가
- 클래스 수준의 JavaDoc 추가하여 테스트의 목적과 시나리오 문서화
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
src/main/java/ddingdong/ddingdongBE/domain/feed/entity/Feed.java(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/feed/service/FacadeClubFeedServiceImpl.java(3 hunks)src/main/java/ddingdong/ddingdongBE/domain/feed/service/FeedService.java(2 hunks)src/main/java/ddingdong/ddingdongBE/domain/feed/service/GeneralFeedService.java(2 hunks)src/main/java/ddingdong/ddingdongBE/domain/vodprocessing/controller/dto/request/UpdateVodProcessingJobStatusRequest.java(0 hunks)src/main/java/ddingdong/ddingdongBE/domain/vodprocessing/entity/VodNotificationStatus.java(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/vodprocessing/entity/VodProcessingNotification.java(2 hunks)src/main/java/ddingdong/ddingdongBE/domain/vodprocessing/repository/VodProcessingJobRepository.java(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/vodprocessing/repository/VodProcessingNotificationRepository.java(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/vodprocessing/service/FacadeVodProcessingJobServiceImpl.java(2 hunks)src/main/java/ddingdong/ddingdongBE/domain/vodprocessing/service/GeneralVodProcessingJobService.java(2 hunks)src/main/java/ddingdong/ddingdongBE/domain/vodprocessing/service/GeneralVodProcessingNotificationService.java(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/vodprocessing/service/VodProcessingJobService.java(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/vodprocessing/service/VodProcessingNotificationService.java(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/vodprocessing/service/dto/command/CreatePendingVodProcessingJobCommand.java(1 hunks)src/main/java/ddingdong/ddingdongBE/sse/service/SseConnectionService.java(1 hunks)src/main/java/ddingdong/ddingdongBE/sse/service/dto/SseEvent.java(1 hunks)src/test/java/ddingdong/ddingdongBE/domain/vodprocessing/service/FacadeGeneralVodProcessingJobServiceMockingTest.java(1 hunks)src/test/java/ddingdong/ddingdongBE/domain/vodprocessing/service/FacadeGeneralVodProcessingJobServiceTest.java(2 hunks)
💤 Files with no reviewable changes (1)
- src/main/java/ddingdong/ddingdongBE/domain/vodprocessing/controller/dto/request/UpdateVodProcessingJobStatusRequest.java
✅ Files skipped from review due to trivial changes (1)
- src/main/java/ddingdong/ddingdongBE/domain/vodprocessing/repository/VodProcessingNotificationRepository.java
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build and analyze
🔇 Additional comments (10)
src/main/java/ddingdong/ddingdongBE/domain/feed/service/FacadeClubFeedServiceImpl.java (1)
10-16: 의존성 주입이 잘 구현되었습니다!
VodProcessingJobService와SseConnectionService의 의존성 주입이 Constructor Injection 패턴을 통해 적절하게 구현되었습니다.Also applies to: 29-30
src/main/java/ddingdong/ddingdongBE/domain/feed/service/GeneralFeedService.java (1)
35-38: 구현이 깔끔해 보입니다!repository의 findById를 직접 반환하는 방식으로 구현이 잘 되어있습니다.
src/main/java/ddingdong/ddingdongBE/sse/service/dto/SseEvent.java (3)
1-4: 패키지 구조와 임포트가 깔끔합니다! 👍DTO 클래스를 위한 적절한 패키지 위치에 잘 배치되었습니다.
5-9: 제네릭 타입을 활용한 레코드 설계가 훌륭합니다!
- 제네릭 타입
T를 사용하여 다양한 데이터 타입을 지원할 수 있도록 설계한 점이 좋습니다.- 이벤트 이름, 데이터, 타임스탬프를 포함하는 구조가 SSE 이벤트의 목적에 잘 부합합니다.
1-15: 보안 관련 검토 사항SSE 이벤트를 통해 전송되는 데이터에 민감한 정보가 포함될 수 있으므로, 다음 사항들을 확인해주시기 바랍니다:
✅ Verification successful
SSE 구현이 보안적으로 적절하게 되어 있습니다
- 인증된 사용자만 SSE 연결을 생성할 수 있습니다
- 각 연결은 사용자별로 독립적으로 관리됩니다
- 1분 타임아웃이 적용되어 있어 불필요한 연결이 지속되지 않습니다
- 현재 코드베이스에서 민감한 정보가 SSE를 통해 전송되는 증거가 없습니다
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for potential sensitive data in SSE events # Search for PII or sensitive data patterns in event usage rg -i "password|token|key|secret|credit|주민|전화|이메일" --type java # Look for SSE event creation patterns ast-grep --pattern 'SseEvent.of($eventName, $data, $timestamp)'Length of output: 48763
Script:
#!/bin/bash # Search for direct SseEvent constructor usage and event emitter implementations rg -t java "new SseEvent[<]" -B 2 -A 2 # Search for SSE related classes and implementations fd -e java -x cat {} \; | rg -A 10 -B 2 "class.*Sse|implements.*Sse|@Sse"Length of output: 2164
src/main/java/ddingdong/ddingdongBE/domain/vodprocessing/service/dto/command/CreatePendingVodProcessingJobCommand.java (2)
6-6: VodProcessingNotification 의존성 추가가 적절합니다.알림 기능 구현을 위한 필수적인 의존성이 잘 추가되었습니다.
14-22: 테스트 커버리지 확인이 필요합니다.PR 설명에 따르면 알림 전송 기능의 통합 테스트가 어려워 모킹을 사용했다고 언급되어 있습니다. 이 컨텍스트에서 현재 클래스의 단위 테스트도 중요할 것 같습니다.
다음 스크립트로 관련 테스트 파일의 존재 여부를 확인해보겠습니다:
src/main/java/ddingdong/ddingdongBE/domain/vodprocessing/repository/VodProcessingJobRepository.java (1)
16-25: JOIN FETCH를 활용한 효율적인 쿼리 구현이 잘 되었습니다!성능 최적화를 위해 JOIN FETCH를 사용한 점이 좋습니다. Optional 반환 타입과 명확한 파라미터 네이밍도 적절합니다.
src/test/java/ddingdong/ddingdongBE/domain/vodprocessing/service/FacadeGeneralVodProcessingJobServiceTest.java (1)
11-11: VodNotificationStatus 관련 변경사항 검토코드가 잘 구성되어 있으며, 명령 객체 생성이 가독성 있게 정리되었습니다.
Also applies to: 43-45
src/test/java/ddingdong/ddingdongBE/domain/vodprocessing/service/FacadeGeneralVodProcessingJobServiceMockingTest.java (1)
29-43: 테스트 클래스 구성이 잘 되어있습니다모든 필요한 의존성들이 적절히 Mock 처리되어 있습니다.
src/main/java/ddingdong/ddingdongBE/domain/feed/service/FacadeClubFeedServiceImpl.java
Outdated
Show resolved
Hide resolved
src/main/java/ddingdong/ddingdongBE/sse/service/SseConnectionService.java
Outdated
Show resolved
Hide resolved
...ngdong/ddingdongBE/domain/vodprocessing/service/GeneralVodProcessingNotificationService.java
Show resolved
Hide resolved
...ngdong/ddingdongBE/domain/vodprocessing/service/GeneralVodProcessingNotificationService.java
Outdated
Show resolved
Hide resolved
| public static VodProcessingNotification pending() { | ||
| return VodProcessingNotification.builder() | ||
| .retryCount(0) | ||
| .vodNotificationStatus(VodNotificationStatus.PENDING) | ||
| .build(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
pending() 팩토리 메서드에 만료 시간 설정이 필요합니다.
현재 구현에서는 expiredAt이 설정되지 않았습니다. 알림의 만료 시간은 중요한 비즈니스 로직일 수 있으므로, 기본값이나 파라미터로 설정하는 것이 좋습니다.
public static VodProcessingNotification pending() {
return VodProcessingNotification.builder()
.retryCount(0)
.vodNotificationStatus(VodNotificationStatus.PENDING)
+ .expiredAt(LocalDateTime.now().plusDays(7)) // 예시: 7일 후 만료
.build();
}Committable suggestion skipped: line range outside the PR's diff.
src/main/java/ddingdong/ddingdongBE/domain/vodprocessing/entity/VodProcessingNotification.java
Show resolved
Hide resolved
| private void checkVodProcessingJobStatus(VodProcessingJob vodProcessingJob) { | ||
| ConvertJobStatus convertJobStatus = vodProcessingJob.getConvertJobStatus(); | ||
| if (convertJobStatus != ConvertJobStatus.PENDING) { | ||
| checkExistingFeedAndNotify(convertJobStatus, vodProcessingJob); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
상태 체크 로직 개선이 필요합니다.
현재 PENDING 상태가 아닌 모든 경우에 알림을 보내고 있습니다. ERROR나 COMPLETE 상태일 때만 알림을 보내도록 명시적으로 체크하는 것이 좋습니다.
private void checkVodProcessingJobStatus(VodProcessingJob vodProcessingJob) {
ConvertJobStatus convertJobStatus = vodProcessingJob.getConvertJobStatus();
- if (convertJobStatus != ConvertJobStatus.PENDING) {
+ if (convertJobStatus == ConvertJobStatus.COMPLETE ||
+ convertJobStatus == ConvertJobStatus.ERROR) {
checkExistingFeedAndNotify(convertJobStatus, vodProcessingJob);
}
}📝 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.
| private void checkVodProcessingJobStatus(VodProcessingJob vodProcessingJob) { | |
| ConvertJobStatus convertJobStatus = vodProcessingJob.getConvertJobStatus(); | |
| if (convertJobStatus != ConvertJobStatus.PENDING) { | |
| checkExistingFeedAndNotify(convertJobStatus, vodProcessingJob); | |
| } | |
| } | |
| private void checkVodProcessingJobStatus(VodProcessingJob vodProcessingJob) { | |
| ConvertJobStatus convertJobStatus = vodProcessingJob.getConvertJobStatus(); | |
| if (convertJobStatus == ConvertJobStatus.COMPLETE || | |
| convertJobStatus == ConvertJobStatus.ERROR) { | |
| checkExistingFeedAndNotify(convertJobStatus, vodProcessingJob); | |
| } | |
| } |
...va/ddingdong/ddingdongBE/domain/vodprocessing/service/FacadeVodProcessingJobServiceImpl.java
Outdated
Show resolved
Hide resolved
...dingdongBE/domain/vodprocessing/service/FacadeGeneralVodProcessingJobServiceMockingTest.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/test/java/ddingdong/ddingdongBE/domain/feed/service/FacadeFeedServiceTest.java (1)
81-82: TODO 주석의 명확성 개선이 필요합니다.현재 TODO 주석이 너무 모호합니다. 재작성이 필요한 구체적인 이유와 예상 완료 시점을 명시하는 것이 좋습니다.
다음과 같이 더 자세한 설명을 추가하는 것을 제안합니다:
-//TODO: Feed 조회 API 개발 완료 후 재작성 +//TODO: Feed 조회 API (GET /api/v1/feeds) 개발 완료 후 피드 정렬 로직 변경사항을 반영하여 재작성 필요 +// 관련 이슈: [DDING-XX] Feed 조회 API 개발src/test/java/ddingdong/ddingdongBE/domain/vodprocessing/service/FacadeGeneralVodProcessingJobServiceTest.java (2)
43-45: Command 객체 생성 포맷팅 개선가독성이 향상된 좋은 변경입니다. 하지만 테스트 데이터의 의도가 더 명확해질 수 있습니다.
다음과 같이 테스트 데이터의 의도를 명확히 하는 것을 고려해보세요:
- String convertJobId = "testId"; - String userId = "testId"; + String convertJobId = "test-convert-job-123"; + String userId = "test-user-456";
Line range hint
28-63: 테스트 전략 개선 필요PR 설명에 따르면 알림 전송 기능의 통합 테스트에 어려움이 있어 모킹을 사용했다고 언급되었습니다. 하지만 현재 테스트에서는 모킹 설정이 보이지 않습니다.
다음 사항들을 고려해보세요:
- 알림 전송 로직을 별도의 서비스로 분리하여 테스트 용이성 향상
- 통합 테스트와 단위 테스트를 명확히 구분
- TestContainers를 활용한 실제 SSE 테스트 구현 검토
이러한 개선을 통해 테스트의 신뢰성과 유지보수성을 높일 수 있습니다.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/test/java/ddingdong/ddingdongBE/domain/clubmember/service/FacadeCentralClubMemberServiceTest.java(0 hunks)src/test/java/ddingdong/ddingdongBE/domain/feed/service/FacadeFeedServiceTest.java(1 hunks)src/test/java/ddingdong/ddingdongBE/domain/vodprocessing/service/FacadeGeneralVodProcessingJobServiceMockingTest.java(1 hunks)src/test/java/ddingdong/ddingdongBE/domain/vodprocessing/service/FacadeGeneralVodProcessingJobServiceTest.java(2 hunks)
💤 Files with no reviewable changes (1)
- src/test/java/ddingdong/ddingdongBE/domain/clubmember/service/FacadeCentralClubMemberServiceTest.java
🚧 Files skipped from review as they are similar to previous changes (1)
- src/test/java/ddingdong/ddingdongBE/domain/vodprocessing/service/FacadeGeneralVodProcessingJobServiceMockingTest.java
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build and analyze
🔇 Additional comments (3)
src/test/java/ddingdong/ddingdongBE/domain/feed/service/FacadeFeedServiceTest.java (1)
83-137: 테스트 커버리지 유지가 필요합니다.
getNewestAll()테스트를 주석 처리하면서 중요한 기능에 대한 테스트 커버리지가 감소했습니다. 이는 잠재적인 버그를 놓칠 수 있는 위험을 초래할 수 있습니다.다음 사항들을 확인해주세요:
- 임시로라도 기본적인 정렬 기능을 검증하는 간단한 테스트를 유지하는 것이 좋습니다.
- 현재 주석 처리된 테스트가 검증하던 시나리오들이 다른 테스트들에 의해 커버되고 있는지 확인이 필요합니다.
테스트 재작성 시 다음 사항들을 고려해주세요:
- 정렬 로직을 별도의 단위 테스트로 분리
- 페이지네이션 관련 테스트 추가
- 실패 케이스에 대한 테스트 추가
src/test/java/ddingdong/ddingdongBE/domain/vodprocessing/service/FacadeGeneralVodProcessingJobServiceTest.java (2)
11-11: VodNotificationStatus 임포트 추가 확인새로운 알림 상태 enum을 사용하기 위한 적절한 임포트가 추가되었습니다.
53-63: 🛠️ Refactor suggestion알림 상태 검증 로직 추가에 대한 피드백
알림 상태 검증이 추가된 것은 좋은 변경입니다. 하지만 몇 가지 개선할 점이 있습니다:
- VodProcessingNotification이 null이 아님을 먼저 검증해야 합니다.
- 실패 케이스에 대한 테스트가 누락되어 있습니다.
다음과 같이 개선을 제안합니다:
assertThat(result.get()) .satisfies(job -> { assertThat(job.getConvertJobId()).isEqualTo(convertJobId); assertThat(job.getUserId()).isEqualTo(userId); assertThat(job.getConvertJobStatus()).isEqualTo(PENDING); + assertThat(job.getVodProcessingNotification()).isNotNull(); assertThat(job.getVodProcessingNotification()) .satisfies(notification -> assertThat( notification.getVodNotificationStatus()).isEqualTo( VodNotificationStatus.PENDING) ); });또한 실패 케이스를 위한 새로운 테스트 메소드 추가를 권장합니다.
실패 케이스 테스트의 누락 여부를 확인하기 위해 다음 스크립트를 실행합니다:
✅ Verification successful
VOD 처리 작업 테스트 개선 필요성 확인
리뷰 의견이 타당합니다. 다음과 같은 이유로 제안된 개선사항을 반영하는 것이 좋습니다:
- 코드베이스의 다른 서비스들은 모두 철저한 null 검증과 오류 케이스 테스트를 포함하고 있습니다.
- VodProcessingNotification이 null이 아님을 검증하는 것은 객체의 무결성 보장을 위해 필수적입니다.
- 실패 케이스 테스트 누락은 다른 서비스들의 테스트 패턴과 일관성이 떨어집니다.
리뷰어가 제안한 두 가지 개선사항 모두 반영하시기를 권장드립니다.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for test coverage of error cases in VodProcessingJob tests # Search for test methods containing 'Error' or 'Fail' rg -l "test.*(?:Error|Fail)" --type javaLength of output: 42
Script:
#!/bin/bash # Find all test files related to VodProcessing echo "=== VodProcessing Test Files ===" fd ".*VodProcessing.*Test\.java$" # Search for exception handling patterns in test files echo -e "\n=== Exception Handling Tests ===" rg "throws|Exception|should.*fail|실패|예외" --type java # Search for assertThat patterns with isNull or notNull echo -e "\n=== Null Check Patterns ===" rg "assertThat.*is(Not)?Null" --type javaLength of output: 25525
src/main/java/ddingdong/ddingdongBE/domain/feed/service/FacadeClubFeedServiceImpl.java
Outdated
Show resolved
Hide resolved
src/main/java/ddingdong/ddingdongBE/domain/feed/service/GeneralFeedService.java
Show resolved
Hide resolved
src/main/java/ddingdong/ddingdongBE/domain/feed/service/FacadeClubFeedServiceImpl.java
Outdated
Show resolved
Hide resolved
src/main/java/ddingdong/ddingdongBE/domain/feed/service/FacadeClubFeedServiceImpl.java
Show resolved
Hide resolved
src/main/java/ddingdong/ddingdongBE/sse/service/SseConnectionService.java
Outdated
Show resolved
Hide resolved
| @Query(""" | ||
| SELECT vj FROM VodProcessingJob vj | ||
| JOIN FETCH vj.fileMetaData fm | ||
| WHERE fm.entityId = :entityId | ||
| AND fm.domainType = :domainType | ||
| """) | ||
| Optional<VodProcessingJob> findFirstByFileMetaDataEntityIdAndDomainType( | ||
| @Param("entityId") Long entityId, | ||
| @Param("domainType") DomainType domainType | ||
| ); |
There was a problem hiding this comment.
p2)
해당 쿼리가 의도대로 동작하는지, 테스트 작성해주면 좋을 것 같아요
...va/ddingdong/ddingdongBE/domain/vodprocessing/service/FacadeVodProcessingJobServiceImpl.java
Outdated
Show resolved
Hide resolved
| SseEvent<ConvertJobStatus> sseEvent = SseEvent.of("vod-processing", convertJobStatus, LocalDateTime.now()); | ||
| sseConnectionService.send(vodProcessingJob.getUserId(), sseEvent); | ||
| VodProcessingNotification vodProcessingNotification = vodProcessingJob.getVodProcessingNotification(); | ||
| vodProcessingNotification.updateVodNotificationStatusToSent(LocalDateTime.now()); |
There was a problem hiding this comment.
p3)
코드가 중복되는 것 같아서, 하나의 메소드로 생성해도 좋을 것 같아요! 항상 같이 실행되는 작업 같네요
음..아니면
| SseEvent<ConvertJobStatus> sseEvent = SseEvent.of("vod-processing", convertJobStatus, LocalDateTime.now()); | |
| sseConnectionService.send(vodProcessingJob.getUserId(), sseEvent); | |
| VodProcessingNotification vodProcessingNotification = vodProcessingJob.getVodProcessingNotification(); | |
| vodProcessingNotification.updateVodNotificationStatusToSent(LocalDateTime.now()); | |
| sseConnectionService.sendVodProcessingJob(vodProcessingJob); | |
| vodProcessingNotificationService.updateVodNotificationStatusToSent(LocalDateTime.now()); |
There was a problem hiding this comment.
저도 작성하면서 고민했는데 애매한거 같습니다.. send 메서드는 좀더 범용적인 메서드로써 작동하도록 sse 관련 정보 말고는 사용하지 않으려 했는데 결국 현재 시점에선 VodProcessingJob 관련 notifiy만 수행하니 해당 정보를 받는 send() 메서드를 구현하는게 더 나을거 같네요!
There was a problem hiding this comment.
범용적인 메소드로 send()는 구현하되, 추가로 sendVodProcessingJob()를 만드는 것이 좋아보인다는 것이었습니다!
src/main/java/ddingdong/ddingdongBE/sse/service/SseConnectionService.java
Outdated
Show resolved
Hide resolved
| try { | ||
| sseEmitter.send(SseEmitter.event() | ||
| .name("sse") | ||
| .data(data)); | ||
| log.debug("SSE Event sent to user {}: {}", id, "sse"); | ||
| } catch (IOException e) { | ||
| log.error("Error sending SSE event to user: {}", id, e); | ||
| sseEmitter.complete(); |
There was a problem hiding this comment.
p5)
동작방식을 잘 이해하지 못해서 그러는데,
sse가 에러가 나서, 연결 종료를 한다면 클라이언트에게 전달해줘야 할 필요가 없나요!?
There was a problem hiding this comment.
sse 에러로 인해 연결이 종료될 경우 연결이 이미 끊겼기 때문에 연결 종료 이벤트를 클라이언트에게 전달할 방법은 없습니다.
다만 클라이언트 측에서 sse 연결 종료를 감지할 수 있는거로 알고 있습니다.
따라서 연결에 문제가 생겼을 경우 서버측에선 sse 연결 리소스를 제대로 정리해주는 작업만 수행하면 됩니다!
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
src/test/java/ddingdong/ddingdongBE/sse/service/SseConnectionServiceTest.java (1)
VOD 처리 작업 알림 테스트 케이스 추가 필요
다음 테스트 케이스들이 누락되어 있습니다:
FacadeVodProcessingJobServiceTest에 추가 필요:
- VOD 처리 작업 상태가 COMPLETE로 변경될 때 SSE 알림 전송 검증
- VOD 처리 작업 상태가 ERROR로 변경될 때 SSE 알림 전송 검증
- Feed가 존재하지 않을 때 알림이 전송되지 않는 케이스 검증
- 알림 전송 후 VodProcessingNotification 상태 변경 검증
구현된
FacadeVodProcessingJobServiceImpl의checkVodProcessingJobStatus와checkExistingFeedAndNotify메서드의 동작을 검증하는 테스트가 필요합니다.🔗 Analysis chain
Line range hint
1-71: VodProcessingJob 알림 테스트 케이스 추가 필요PR의 주요 목적인 VodProcessingJob 완료 시 알림 전송 기능에 대한 테스트가 누락되어 있습니다.
다음과 같은 테스트 케이스 추가를 제안드립니다:
- VodProcessingJob 상태가 COMPLETE로 변경될 때 알림 전송
- VodProcessingJob 상태가 ERROR로 변경될 때 알림 전송
- VIDEO Feed 생성 시 알림 전송
관련 코드를 찾아보기 위해 다음 스크립트를 실행해보겠습니다:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # VodProcessingJob 관련 알림 처리 코드 확인 echo "Searching for VodProcessingJob notification related code..." rg -A 5 "VodProcessingJob.*Notification"Length of output: 3017
Script:
#!/bin/bash # Search for notification-related implementations and tests echo "Searching for notification implementations..." rg -l "Notification|notification" --type java echo -e "\nSearching for event handlers..." rg -l "EventHandler|@EventListener" --type java echo -e "\nSearching for notification tests..." fd "Test.java" | rg "Notification|notification"Length of output: 1619
Script:
#!/bin/bash # Check notification service implementation and tests echo "Checking notification service implementation..." cat src/main/java/ddingdong/ddingdongBE/domain/vodprocessing/service/GeneralVodProcessingNotificationService.java echo -e "\nChecking existing test coverage..." cat src/test/java/ddingdong/ddingdongBE/domain/vodprocessing/service/FacadeGeneralVodProcessingJobServiceTest.javaLength of output: 4530
Script:
#!/bin/bash # Check status update implementation and SSE service echo "Checking status update implementation..." cat src/main/java/ddingdong/ddingdongBE/domain/vodprocessing/service/FacadeVodProcessingJobServiceImpl.java echo -e "\nChecking SSE service implementation..." cat src/main/java/ddingdong/ddingdongBE/sse/service/SseConnectionService.javaLength of output: 6636
🧹 Nitpick comments (6)
src/main/java/ddingdong/ddingdongBE/sse/repository/SseConnectionRepository.java (1)
10-10: findById 메서드의 반환 타입 변경이 적절합니다.SseEmitter를 Optional로 감싸서 반환하도록 변경한 것은 다음과 같은 이점이 있습니다:
- null 안전성 향상
- API 계약의 명확성 증가
- 에러 처리의 용이성
- Java의 최신 관행 준수
이 변경으로 인해 이 메서드를 호출하는 서비스 계층에서는 다음과 같은 방식으로 처리해야 합니다:
Optional<SseEmitter> emitter = repository.findById(id); emitter.ifPresentOrElse( e -> { /* 정상 처리 로직 */ }, () -> { /* emitter가 없을 때의 처리 로직 */ } );src/main/java/ddingdong/ddingdongBE/sse/repository/SseConnectionMapRepository.java (1)
20-21: findById 메서드의 반환 타입 개선이 잘 되었습니다다음과 같은 이유로 SseEmitter를 Optional로 감싸서 반환하도록 변경한 것이 적절합니다:
- null 반환 대신 명시적인 부재 표현이 가능합니다
- 호출하는 쪽에서 반드시 존재 여부를 처리하도록 강제할 수 있습니다
- Java의 최신 관행을 따르고 있습니다
이러한 변경은 SSE 연결 관리의 안정성을 높이는데 도움이 될 것입니다.
src/main/java/ddingdong/ddingdongBE/common/exception/SseException.java (1)
7-8: 상수 네이밍 컨벤션 개선 필요에러 메시지 상수의 네이밍이 일반적인 Java 상수 네이밍 컨벤션을 따르고 있지 않습니다.
다음과 같이 수정하는 것을 제안합니다:
- public static final String SSE_NOT_FOUND_ERROR_MESSAGE = "SSE Not Found = "; + private static final String ERROR_MESSAGE_SSE_NOT_FOUND = "SSE Not Found = ";src/main/java/ddingdong/ddingdongBE/domain/vodprocessing/service/FacadeVodProcessingJobServiceImpl.java (1)
54-63: 이벤트 이름을 상수로 분리하는 것이 좋겠습니다.이벤트 이름이 문자열 리터럴로 하드코딩되어 있습니다. 상수로 분리하여 재사용성과 유지보수성을 높이는 것이 좋겠습니다.
+ private static final String VOD_PROCESSING_EVENT = "vod-processing"; private void checkExistingFeedAndNotify(VodProcessingJob vodProcessingJob) { Optional<Feed> optionalFeed = feedService.findById(vodProcessingJob.getFileMetaData().getEntityId()); if (optionalFeed.isPresent()) { SseEvent<ConvertJobStatus> sseEvent = SseEvent.of( - "vod-processing", + VOD_PROCESSING_EVENT, vodProcessingJob.getConvertJobStatus(), LocalDateTime.now() ); sseConnectionService.sendVodProcessingNotification(vodProcessingJob, sseEvent); } }src/main/java/ddingdong/ddingdongBE/domain/feed/service/FacadeClubFeedServiceImpl.java (1)
66-76: 이벤트 관련 상수를 공유하는 것이 좋겠습니다.
- 이벤트 이름이
FacadeVodProcessingJobServiceImpl와 중복되어 있습니다.- 이벤트 관련 상수를 공통 상수 클래스로 분리하여 재사용하는 것이 좋겠습니다.
다음과 같은 개선을 제안드립니다:
- 공통 상수 클래스 생성:
public class SseEventConstants { public static final String VOD_PROCESSING_EVENT = "vod-processing"; }
- 상수 사용:
private void checkVodProcessingJobAndNotify(Feed feed) { VodProcessingJob vodProcessingJob = vodProcessingJobService.getByVideoFeedId(feed.getId()); if (vodProcessingJob.isPossibleNotify()) { SseEvent<ConvertJobStatus> sseEvent = SseEvent.of( - "vod-processing", + SseEventConstants.VOD_PROCESSING_EVENT, vodProcessingJob.getConvertJobStatus(), LocalDateTime.now() ); sseConnectionService.sendVodProcessingNotification(vodProcessingJob, sseEvent); } }src/test/java/ddingdong/ddingdongBE/sse/service/SseConnectionServiceTest.java (1)
49-50: 단언문 개선이 잘 이루어졌습니다!기존 단언문을 두 개의 명확한 검증으로 분리한 것이 좋은 개선입니다. Optional 값의 존재 여부를 먼저 확인한 후 실제 값을 검증하는 방식이 더 명확합니다.
다만 다음과 같은 개선사항을 제안드립니다:
- 단언문에 실패 메시지를 추가하면 테스트 실패 시 더 명확한 피드백을 받을 수 있습니다.
- VodProcessingJob 알림 전송 기능에 대한 테스트 케이스도 추가하면 좋을 것 같습니다.
다음과 같이 수정해보시는 건 어떨까요?:
- assertThat(sseConnectionRepository.findById(TEST_ID)).isPresent(); - assertThat(sseConnectionRepository.findById(TEST_ID).get()).isEqualTo(secondEmitter); + assertThat(sseConnectionRepository.findById(TEST_ID)) + .as("새로운 구독 후 저장소에서 Emitter를 찾을 수 있어야 합니다") + .isPresent(); + assertThat(sseConnectionRepository.findById(TEST_ID).get()) + .as("저장소의 Emitter는 새로 생성된 Emitter와 일치해야 합니다") + .isEqualTo(secondEmitter);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
src/main/java/ddingdong/ddingdongBE/common/exception/SseException.java(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/feed/service/FacadeClubFeedServiceImpl.java(3 hunks)src/main/java/ddingdong/ddingdongBE/domain/vodprocessing/entity/VodNotificationStatus.java(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/vodprocessing/entity/VodProcessingJob.java(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/vodprocessing/entity/VodProcessingNotification.java(2 hunks)src/main/java/ddingdong/ddingdongBE/domain/vodprocessing/service/FacadeVodProcessingJobServiceImpl.java(2 hunks)src/main/java/ddingdong/ddingdongBE/sse/repository/SseConnectionMapRepository.java(2 hunks)src/main/java/ddingdong/ddingdongBE/sse/repository/SseConnectionRepository.java(1 hunks)src/main/java/ddingdong/ddingdongBE/sse/service/SseConnectionService.java(2 hunks)src/test/java/ddingdong/ddingdongBE/common/support/DataJpaTestSupport.java(0 hunks)src/test/java/ddingdong/ddingdongBE/domain/vodprocessing/service/FacadeGeneralVodProcessingJobServiceMockingTest.java(1 hunks)src/test/java/ddingdong/ddingdongBE/sse/service/SseConnectionServiceTest.java(1 hunks)
💤 Files with no reviewable changes (1)
- src/test/java/ddingdong/ddingdongBE/common/support/DataJpaTestSupport.java
🚧 Files skipped from review as they are similar to previous changes (3)
- src/main/java/ddingdong/ddingdongBE/domain/vodprocessing/entity/VodNotificationStatus.java
- src/test/java/ddingdong/ddingdongBE/domain/vodprocessing/service/FacadeGeneralVodProcessingJobServiceMockingTest.java
- src/main/java/ddingdong/ddingdongBE/domain/vodprocessing/entity/VodProcessingNotification.java
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build and analyze
🔇 Additional comments (7)
src/main/java/ddingdong/ddingdongBE/sse/repository/SseConnectionRepository.java (1)
3-3: Optional import 추가가 적절합니다.null 안전성을 위한 Optional 사용은 좋은 방향성입니다.
src/main/java/ddingdong/ddingdongBE/sse/repository/SseConnectionMapRepository.java (1)
4-4: Optional 사용으로 null 처리 개선null 처리를 위한 Java의 공식적인 방법인 Optional을 도입한 것은 좋은 개선입니다.
src/main/java/ddingdong/ddingdongBE/common/exception/SseException.java (2)
9-11: 생성자 구현이 적절합니다.상위 클래스의 생성자를 올바르게 호출하고 있습니다.
1-5: sealed 클래스 사용이 적절한지 검토 필요sealed 클래스를 사용하면 하위 클래스를 제한할 수 있지만, 현재는 단일 하위 클래스만 존재합니다. 향후 추가될 하위 클래스가 없다면 final 클래스로 변경하는 것이 더 명확할 수 있습니다.
다음 스크립트로 다른 하위 클래스의 존재 여부를 확인해보세요:
✅ Verification successful
sealed 클래스 사용이 적절합니다
현재 구현에서 sealed 클래스 사용은 다음과 같은 이유로 적절합니다:
- SseEmitterNotFound라는 하나의 허용된 하위 클래스가 내부 클래스로 정의되어 있습니다
- 하위 클래스가 final로 선언되어 있어 상속 계층 구조가 명확하게 제한됩니다
- SSE 관련 예외를 특정 케이스로 제한하려는 설계 의도가 명확히 드러납니다
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # SseException을 상속받는 다른 클래스가 있는지 확인 ast-grep --pattern 'class $_ extends SseException'Length of output: 52
Script:
#!/bin/bash # View the complete content of SseException.java cat src/main/java/ddingdong/ddingdongBE/common/exception/SseException.javaLength of output: 668
src/main/java/ddingdong/ddingdongBE/domain/vodprocessing/entity/VodProcessingJob.java (1)
61-63: 메서드 구현이 깔끔합니다!상태 확인 로직이 명확하고 단일 책임 원칙을 잘 따르고 있습니다.
src/main/java/ddingdong/ddingdongBE/sse/service/SseConnectionService.java (1)
74-76: Optional을 활용한 구현이 개선되었습니다!이전 버전의 null 체크 대신 Optional을 사용하여 더 안전하고 명확한 코드가 되었습니다.
src/main/java/ddingdong/ddingdongBE/domain/vodprocessing/service/FacadeVodProcessingJobServiceImpl.java (1)
48-52: 상태 체크 로직이 개선되었습니다!
isPossibleNotify()메서드를 활용하여 명확하게 상태를 체크하도록 개선되었습니다.
src/main/java/ddingdong/ddingdongBE/sse/service/SseConnectionService.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/test/java/ddingdong/ddingdongBE/domain/vodprocessing/repository/VodProcessingJobRepositoryTest.java (3)
27-64: 테스트 메소드의 데이터 설정 개선이 필요합니다.테스트 구조는 잘 작성되어 있으나, 다음과 같은 개선사항을 제안드립니다:
- 테스트 데이터를 상수로 분리하여 재사용성을 높이는 것이 좋습니다.
- 파일 확장자나 상태값에 대한 다양한 케이스를 추가하면 좋을 것 같습니다.
다음과 같이 개선해 보시는 건 어떨까요?:
+ private static final Long TEST_ENTITY_ID = 1L; + private static final String TEST_FILE_KEY = "test-key"; + private static final String TEST_FILE_NAME = "test.mp4"; + private static final String TEST_USER_ID = "testUser"; + @Test void findFirstByFileMetaDataEntityIdAndDomainType() { // given - Long entityId = 1L; + Long entityId = TEST_ENTITY_ID; DomainType domainType = DomainType.FEED_VIDEO; FileMetaData fileMetaData = FileMetaData.builder() .id(UuidCreator.getTimeOrderedEpoch()) - .fileKey("test-key") - .fileName("test.mp4") + .fileKey(TEST_FILE_KEY) + .fileName(TEST_FILE_NAME)
66-79: 실패 케이스에 대한 테스트를 보완하면 좋겠습니다.현재는 존재하지 않는 entityId에 대한 테스트만 있습니다. 다음과 같은 케이스들도 고려해 보시면 좋을 것 같습니다:
- entityId가 null인 경우
- domainType이 다른 경우
- domainType이 null인 경우
예시:
@Test void findFirstByFileMetaDataEntityIdAndDomainType_ReturnEmptyWhenNullEntityId() { // given Long nullEntityId = null; DomainType domainType = DomainType.FEED_VIDEO; // when Optional<VodProcessingJob> result = vodProcessingJobRepository .findFirstByFileMetaDataEntityIdAndDomainType(nullEntityId, domainType); // then assertThat(result).isEmpty(); }
81-84: 유틸리티 메소드의 재사용성 개선이 필요합니다.
isPersistenceLoaded메소드는 다른 테스트에서도 유용하게 사용될 수 있습니다. 공통 테스트 유틸리티 클래스로 이동하는 것을 고려해 보시면 좋을 것 같습니다.예시:
// TestEntityUtils.java public class TestEntityUtils { public static boolean isPersistenceLoaded(Object entity) { return Persistence.getPersistenceUtil().isLoaded(entity); } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/test/java/ddingdong/ddingdongBE/domain/vodprocessing/repository/VodProcessingJobRepositoryTest.java(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build and analyze
🔇 Additional comments (2)
src/test/java/ddingdong/ddingdongBE/domain/vodprocessing/repository/VodProcessingJobRepositoryTest.java (2)
1-25: 테스트 클래스 구조가 잘 설계되어 있습니다!Spring Data JPA 테스트를 위한 기본 구조가 잘 갖춰져 있으며, 필요한 의존성들이 적절하게 주입되어 있습니다.
1-85: 테스트 커버리지가 적절히 구현되어 있습니다.VodProcessingJob 조회 기능에 대한 기본적인 테스트 케이스들이 잘 구현되어 있습니다. 제안드린 개선사항들은 테스트의 품질을 더욱 향상시키기 위한 것이며, 현재 구현도 충분히 견고합니다.
|
🚀 작업 내용
COMPLTE,ERRORCOMPLTE,ERROR🤔 고민했던 내용
각 작업 내용 모두 sse를 통한 notification 전송에 대한 동작을 통합 테스트로 작성할 수 없어 불가피하게 mocking을 사용하여 작성했습니다.
💬 리뷰 중점사항
Summary by CodeRabbit
릴리즈 노트
새로운 기능
SseEvent레코드 추가VodProcessingNotification을 위한 빌더 패턴 도입VodProcessingJob에서 알림 전송 가능 여부를 확인하는 메서드 추가VodProcessingNotificationService인터페이스 추가 및 저장 메서드 도입VodNotificationStatus열거형 값 추가개선 사항
FacadeVodProcessingJobServiceImpl의 SSE 이벤트 전송 로직 개선SseEmitter의 존재 여부를 확인하는 로직 개선버그 수정