[DEV-56] Vod Processing Job 생성 API 구현 #195
Conversation
Walkthrough이 변경 사항은 여러 파일에서의 수정으로 구성되어 있으며, 주로 Java 클래스의 이름 변경, 새로운 클래스 및 인터페이스 추가, 메서드 시그니처 수정, 그리고 데이터베이스 테이블 생성과 관련된 내용을 포함합니다. JaCoCo 구성에서 특정 클래스의 제외 항목이 추가되었고, 보안 구성에서 내부 API 엔드포인트에 대한 접근이 허용되었습니다. 또한, 비디오 처리 작업과 관련된 새로운 서비스 및 엔티티가 도입되었습니다. Changes
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
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: 14
🧹 Outside diff range and nitpick comments (17)
src/main/java/ddingdong/ddingdongBE/domain/notice/service/GeneralNoticeService.java (1)
14-14: 클래스 이름 변경이 적절합니다.
NoticeServiceImpl에서GeneralNoticeService로의 이름 변경은 해당 서비스의 일반적인 공지사항 처리 특성을 더 잘 표현합니다. 구현 세부사항을 나타내는*Impl접미사를 제거한 것도 좋은 변경입니다.클래스 수준의 JavaDoc 주석을 추가하여 이 서비스의 일반적인 특성과 책임을 문서화하면 좋을 것 같습니다.
src/main/java/ddingdong/ddingdongBE/domain/notice/service/FacadeAdminNoticeServiceImpl.java (1)
17-17: 인터페이스 기반 의존성 주입으로의 개선이 좋습니다.
NoticeService인터페이스 타입으로의 변경은 구체적인 구현체와의 결합도를 낮추고 유연성을 향상시킵니다. 이는 단위 테스트 작성과 향후 유지보수를 더 용이하게 만듭니다.이러한 패턴을 다른 서비스들에도 일관되게 적용하는 것을 권장드립니다.
src/test/java/ddingdong/ddingdongBE/file/service/S3FileServiceTest.java (3)
39-42: 경계값 테스트 케이스 추가 필요
userId에 대한 다음과 같은 테스트 케이스들이 누락되어 있습니다:
- 음수 값
- 0값
- null 값
- 매우 큰 값 (Long.MAX_VALUE)
테스트 메소드 추가를 제안드립니다:
@Test @DisplayName("잘못된 userId로 요청시 예외가 발생한다") void generatePreSignedUrlRequest_WithInvalidUserId() { // given LocalDateTime now = LocalDateTime.now(); Long invalidUserId = -1L; String fileName = "test.jpg"; // when & then assertThatThrownBy(() -> new GeneratePreSignedUrlRequestCommand(now, invalidUserId, fileName)) .isInstanceOf(IllegalArgumentException.class) .hasMessageContaining("유효하지 않은 사용자 ID"); }
78-80: 비디오 테스트에도 동일한 경계값 테스트 필요비디오 관련 테스트에도
userId관련 경계값 테스트가 필요합니다.
generateVIDEOPreSignedUrlRequest메소드에 대해서도 앞서 제안된 경계값 테스트를 추가해주세요.
94-96: 테스트 가독성 개선 필요테스트의 가독성을 높이기 위해 문자열 포맷팅 로직을 별도의 헬퍼 메소드로 추출하는 것을 추천드립니다.
+ private String generateExpectedKeyFormat(String type, LocalDateTime date, Long userId) { + return String.format("%s/%d-%d-%d/%s/", + type, date.getYear(), date.getMonthValue(), date.getDayOfMonth(), userId); + } assertThat(request.getKey()) .as("Key should contain correct date, userId, and fileId") - .contains(String.format("%s/%d-%d-%d/%s/", - "VIDEO", now.getYear(), now.getMonthValue(), now.getDayOfMonth(), userId)) + .contains(generateExpectedKeyFormat("VIDEO", now, userId)) .contains(id.toString());src/main/java/ddingdong/ddingdongBE/file/service/S3FileService.java (1)
Line range hint
95-107: 비디오 URL 생성 로직 개선이 필요합니다.다음 사항들을 고려해 주시기 바랍니다:
- 해상도가 "_720.m3u8"로 하드코딩되어 있어 다른 해상도 지원이 어렵습니다.
key파라미터에 대한 null/empty 검증이 누락되었습니다.- TODO 주석의 구체적인 내용이 필요합니다.
다음과 같이 개선하는 것을 제안드립니다:
public UploadedVideoUrlQuery getUploadedVideoUrl(String key) { + if (key == null || key.trim().isEmpty()) { + throw new IllegalArgumentException("Key must not be null or empty"); + } String fileId = extractFileId(key); String region = amazonS3Client.getRegionName(); + // 해상도를 설정값으로 관리 + @Value("${video.resolution:720}") + private String defaultResolution; String thumbnailOriginUrl = generateS3Url(outputBucket, region, "thumbnail", fileId, ".jpg"); String thumbnailCdnUrl = generateCdnUrl("thumbnail", fileId, ".jpg"); - String videoOriginUrl = generateS3Url(outputBucket, region, "hls", fileId, "_720.m3u8"); - String videoCdnUrl = generateCdnUrl("hls", fileId, "_720.m3u8"); + String videoOriginUrl = generateS3Url(outputBucket, region, "hls", fileId, "_" + defaultResolution + ".m3u8"); + String videoCdnUrl = generateCdnUrl("hls", fileId, "_" + defaultResolution + ".m3u8"); return new UploadedVideoUrlQuery(thumbnailOriginUrl, thumbnailCdnUrl, videoOriginUrl, videoCdnUrl); }src/main/java/ddingdong/ddingdongBE/domain/vodprocessing/service/FacadeVodProcessingJobService.java (1)
5-8: Facade 패턴이 적절히 사용되었습니다.인터페이스가 명확하고 단일 책임 원칙을 잘 따르고 있습니다. 다만 다음 사항들을 고려해보시면 좋겠습니다:
- 메서드 명세에 대한 JavaDoc 문서화 추가
- 반환값이 job의 ID임을 명확히 하기 위해 메서드 이름을
createAndGetId로 변경 고려src/main/java/ddingdong/ddingdongBE/domain/vodprocessing/service/dto/command/CreatePendingVodProcessingJobCommand.java (1)
11-17: 상태 검증 로직 추가를 고려해보세요현재 구현은 기능적으로는 문제가 없으나, 다음 사항들을 고려해보시면 좋을 것 같습니다:
- convertJobId와 userId의 유효성 검증 로직 추가
- PENDING 상태 전환에 대한 명시적인 검증 로직 추가
예시 구현:
public VodProcessingJob toPendingVodProcessingJob() { + validateFields(); return VodProcessingJob.builder() .convertJobId(convertJobId) .userId(userId) .convertJobStatus(ConvertJobStatus.PENDING) .build(); } +private void validateFields() { + if (convertJobId == null || convertJobId.isBlank()) { + throw new IllegalArgumentException("convertJobId는 필수값입니다"); + } + if (userId == null || userId.isBlank()) { + throw new IllegalArgumentException("userId는 필수값입니다"); + } +}src/main/java/ddingdong/ddingdongBE/domain/vodprocessing/service/FacadeVodProcessingJobServiceImpl.java (1)
1-20: AWS Lambda 통합 관련 고려사항PR 설명에 따르면 이 구현은 AWS Lambda와 통합될 예정입니다. 다음 사항들을 고려해보시면 좋겠습니다:
- Lambda 호출 실패 시의 재시도 정책
- 작업 상태 변경에 대한 이벤트 처리
- AWS 리소스 접근을 위한 보안 설정
이러한 요구사항들을 구현하기 위해 별도의 설계 논의가 필요할 것 같습니다. 필요하시다면 상세한 아키텍처 제안을 도와드릴 수 있습니다.
src/main/java/ddingdong/ddingdongBE/domain/feed/service/GeneralFeedService.java (1)
14-14: 클래스 문서화 개선 필요클래스 이름이
FeedService에서GeneralFeedService로 변경되었습니다. 이러한 변경의 의도와General접두사의 의미를 명확히 하기 위해 클래스 수준의 JavaDoc 문서화가 필요합니다.다음과 같이 문서화를 추가하는 것을 추천드립니다:
+/** + * 피드 조회를 위한 일반적인 서비스 구현체입니다. + * 기본적인 CRUD 작업을 처리하며, 특수한 비즈니스 로직은 다른 특화된 서비스에서 처리합니다. + */ public class GeneralFeedService {src/test/java/ddingdong/ddingdongBE/domain/documents/service/GeneralDocumentServiceTest.java (1)
Line range hint
15-39: 테스트 케이스 보강이 필요합니다.현재는 문서 생성에 대한 테스트만 구현되어 있습니다. 다음과 같은 추가 테스트 케이스들이 필요합니다:
- 문서 조회
- 문서 수정
- 문서 삭제
- 예외 상황 처리
테스트 커버리지를 높이기 위한 추가 테스트 케이스 구현이 필요합니다. 도움이 필요하시다면 말씀해 주세요.
src/main/resources/db/migration/V29__create_tables_vod_processing.sql (1)
13-23: vod_processing_notification 테이블의 날짜 타입 일관성이 필요합니다.
created_at과updated_at은 timestamp 타입인데 반해,expired_at과sent_at은 datetime 타입입니다. 일관성을 위해 모든 날짜 컬럼을 timestamp 타입으로 통일하는 것이 좋습니다.CREATE TABLE vod_processing_notification ( id BIGINT AUTO_INCREMENT NOT NULL, created_at timestamp NULL, updated_at timestamp NULL, - expired_at datetime NULL, - sent_at datetime NULL, + expired_at timestamp NULL, + sent_at timestamp NULL, retry_count INT NOT NULL, notification_status VARCHAR(255) NOT NULL, CONSTRAINT PK_vod_processing_notification PRIMARY KEY (id) );src/main/java/ddingdong/ddingdongBE/domain/fixzone/service/GeneralFixZoneService.java (2)
Line range hint
26-28: 예외 메시지 개선이 필요합니다.현재 예외 메시지에 내부 ID 값이 그대로 노출되고 있습니다. 보안상의 이유로 내부 ID를 직접적으로 노출하는 것은 피하는 것이 좋습니다.
- return fixZoneRepository.findById(fixZoneId) - .orElseThrow(() -> new ResourceNotFound("FixZone(id=" + fixZoneId + ")를 찾을 수 없습니다.")); + return fixZoneRepository.findById(fixZoneId) + .orElseThrow(() -> new ResourceNotFound("요청하신 고정구역을 찾을 수 없습니다."));
14-14: 클래스 수준의 문서화가 필요합니다.서비스 클래스의 책임과 역할을 명확히 하기 위해 JavaDoc 문서화를 추가해 주세요.
+/** + * 고정구역(FixZone)에 대한 기본적인 CRUD 작업을 처리하는 서비스입니다. + * 모든 읽기 작업은 기본적으로 읽기 전용 트랜잭션으로 처리되며, + * 쓰기 작업은 개별적으로 트랜잭션이 적용됩니다. + */ public class GeneralFixZoneService implements FixZoneService {src/main/java/ddingdong/ddingdongBE/domain/documents/service/GeneralDocumentService.java (1)
Line range hint
20-24: 문서 생성 시 중복 검사 로직 추가 필요문서 생성 시 동일한 제목이나 내용을 가진 문서가 이미 존재하는지 확인하는 로직을 추가하면 좋겠습니다.
@Transactional @Override public Long create(Document document) { + if (documentRepository.existsByTitle(document.getTitle())) { + throw new IllegalArgumentException("이미 존재하는 문서 제목입니다."); + } Document createdDocument = documentRepository.save(document); return createdDocument.getId(); }src/test/java/ddingdong/ddingdongBE/domain/vodprocessing/service/FacadeGeneralVodProcessingJobServiceTest.java (1)
24-43: 테스트 케이스 보완 필요현재 테스트는 기본적인 성공 케이스만 다루고 있습니다. 다음과 같은 추가 테스트 케이스를 고려해 주세요:
- 중복된 convertJobId 처리
- 잘못된 userId 입력 시 처리
- null 값 입력 시 처리
예시 테스트 케이스를 생성해드릴까요?
build.gradle (1)
121-123: 코드 커버리지 제외 범위 검토 필요
Controller와GeneralService클래스를 커버리지 측정에서 제외하는 것이 적절한지 검토가 필요합니다:
Controller제외는 일반적이나, 중요한 비즈니스 로직이 포함되어 있다면 테스트가 필요할 수 있습니다.GeneralService클래스는 핵심 비즈니스 로직을 포함할 수 있으므로, 테스트 제외 시 품질 저하의 위험이 있습니다.테스트 제외 대상을 더 구체적으로 정의하고, 핵심 비즈니스 로직은 반드시 테스트 커버리지에 포함시키는 것을 권장드립니다.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (30)
build.gradle(1 hunks)src/main/java/ddingdong/ddingdongBE/common/config/SecurityConfig.java(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/documents/service/GeneralDocumentService.java(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/feed/service/FacadeFeedService.java(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/feed/service/GeneralFeedService.java(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/fixzone/service/GeneralFixZoneService.java(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/notice/service/FacadeAdminNoticeServiceImpl.java(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/notice/service/FacadeNoticeServiceImpl.java(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/notice/service/GeneralNoticeService.java(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/vodprocessing/controller/VodProcessingJobController.java(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/vodprocessing/controller/dto/request/CreatePendingVodProcessingJobRequest.java(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/vodprocessing/entity/ConvertJobStatus.java(1 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(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/vodprocessing/repository/VodProcessingJobRepository.java(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/vodprocessing/service/FacadeVodProcessingJobService.java(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/vodprocessing/service/FacadeVodProcessingJobServiceImpl.java(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/vodprocessing/service/GeneralVodProcessingJobService.java(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/vodprocessing/service/VodProcessingJobService.java(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/vodprocessing/service/dto/command/CreatePendingVodProcessingJobCommand.java(1 hunks)src/main/java/ddingdong/ddingdongBE/file/controller/S3FileController.java(1 hunks)src/main/java/ddingdong/ddingdongBE/file/service/S3FileService.java(2 hunks)src/main/java/ddingdong/ddingdongBE/file/service/dto/command/GeneratePreSignedUrlRequestCommand.java(1 hunks)src/main/resources/db/migration/V29__create_tables_vod_processing.sql(1 hunks)src/test/java/ddingdong/ddingdongBE/domain/documents/service/GeneralDocumentServiceTest.java(2 hunks)src/test/java/ddingdong/ddingdongBE/domain/fixzone/service/FacadeAdminGeneralFixZoneServiceTest.java(1 hunks)src/test/java/ddingdong/ddingdongBE/domain/fixzone/service/FacadeCentralGeneralFixZoneServiceTest.java(1 hunks)src/test/java/ddingdong/ddingdongBE/domain/vodprocessing/service/FacadeGeneralVodProcessingJobServiceTest.java(1 hunks)src/test/java/ddingdong/ddingdongBE/file/service/S3FileServiceTest.java(4 hunks)
✅ Files skipped from review due to trivial changes (4)
- src/main/java/ddingdong/ddingdongBE/domain/vodprocessing/entity/VodNotificationStatus.java
- src/main/java/ddingdong/ddingdongBE/domain/vodprocessing/entity/ConvertJobStatus.java
- src/test/java/ddingdong/ddingdongBE/domain/fixzone/service/FacadeAdminGeneralFixZoneServiceTest.java
- src/test/java/ddingdong/ddingdongBE/domain/fixzone/service/FacadeCentralGeneralFixZoneServiceTest.java
🔇 Additional comments (11)
src/main/java/ddingdong/ddingdongBE/domain/notice/service/FacadeNoticeServiceImpl.java (1)
23-23: 일관된 아키텍처 개선이 잘 이루어졌습니다.
FacadeAdminNoticeServiceImpl과 동일한 패턴으로 NoticeService 인터페이스를 사용하도록 변경된 것이 좋습니다. 이러한 일관된 변경은 코드베이스의 전반적인 품질을 향상시킵니다.
다른 Facade 서비스들도 동일한 패턴을 따르는지 확인이 필요합니다:
✅ Verification successful
아키텍처 패턴이 일관되게 적용되었음을 확인했습니다.
모든 Facade 서비스 구현체들이 구체 클래스(*Impl) 대신 인터페이스를 의존성으로 주입받고 있습니다. 예를 들어:
NoticeServiceQuestionServiceClubServiceBannerService
등이 인터페이스 형태로 주입되어 있습니다.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: 모든 Facade 서비스의 의존성 주입 패턴 확인
# Facade 서비스 구현체들의 의존성 주입 패턴 검사
rg -l "class.*Facade.*Impl.*implements" | xargs rg "private final.*Impl.*Service"
Length of output: 83
Script:
#!/bin/bash
# Let's try a different approach to check the Facade service implementations
# First, find all Facade implementation files
echo "=== Facade Implementation Files ==="
rg -l "class.*Facade.*Impl"
echo -e "\n=== Service Dependencies in Facade Implementations ==="
# Then check their service dependencies
rg -l "class.*Facade.*Impl" | xargs rg "private final.*Service"
Length of output: 11083
src/main/java/ddingdong/ddingdongBE/file/service/dto/command/GeneratePreSignedUrlRequestCommand.java (1)
7-7: 필드 타입 변경에 대한 마이그레이션 계획 필요
String authId에서 Long userId로의 타입 변경은 기존 데이터와 호환성 문제를 일으킬 수 있습니다. 다음 사항들을 고려해주세요:
- 기존 데이터 마이그레이션 계획
- 롤백 전략
- 단계적 배포 전략
src/main/java/ddingdong/ddingdongBE/file/service/S3FileService.java (1)
128-128: userId를 사용하는 키 생성 방식의 영향도 확인이 필요합니다.
authId에서 userId로의 변경이 기존 저장된 파일들의 접근에 영향을 미치지 않는지 확인이 필요합니다.
다음 스크립트를 실행하여 기존 키 패턴 사용 여부를 확인해주세요:
✅ Verification successful
키 생성 방식의 변경이 안전하게 이루어졌음을 확인했습니다.
테스트 코드와 구현을 검토한 결과:
- 파일 키는
{serverProfile}/{mediaType}/{date}/{userId}/{fileId}형식으로 일관되게 생성됩니다. - 기존
authId관련 참조가 전혀 없어 이전 키 패턴과의 충돌 위험이 없습니다. - 테스트에서 검증된 키 생성 패턴이 새로운 구조와 정확히 일치합니다.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the impact of key generation changes
# Check for existing key patterns in the codebase
rg -A 2 "authId.*getKeyMediaType|getKeyMediaType.*authId"
# Check for any remaining references to the old key format
ast-grep --pattern 'authId.*serverProfile.*keyMediaType'
Length of output: 118
Script:
#!/bin/bash
# Let's check for any references to key generation patterns and file storage logic
# Check for key generation patterns
rg -A 5 "generateKey|getKey"
# Check for S3 related file operations
rg -A 5 "S3.*upload|upload.*S3"
# Look for file service implementations
fd -e java "FileService"
# Check git history for the specific file to understand the change
git log -p -- src/main/java/ddingdong/ddingdongBE/file/service/S3FileService.java
Length of output: 7752
src/main/java/ddingdong/ddingdongBE/domain/vodprocessing/controller/dto/request/CreatePendingVodProcessingJobRequest.java (1)
10-12: toCommand 메서드가 적절히 구현되었습니다.
명확하고 간단한 매핑 구현이 잘 되어있습니다.
src/main/java/ddingdong/ddingdongBE/domain/vodprocessing/repository/VodProcessingJobRepository.java (1)
8-12: 구현이 깔끔하고 적절합니다!
Spring Data JPA의 네이밍 컨벤션을 잘 따르고 있으며, Optional을 사용한 null 처리가 안전하게 구현되어 있습니다.
src/main/java/ddingdong/ddingdongBE/domain/vodprocessing/service/GeneralVodProcessingJobService.java (2)
10-13: 클래스 구조가 잘 설계되어 있습니다.
Spring 서비스 계층에 적절한 어노테이션들이 사용되었으며, 읽기 전용 트랜잭션 설정이 기본값으로 잘 지정되어 있습니다.
24-36: 예외 처리가 잘 구현되어 있습니다.
getById와 getByConvertJobId 메서드에서 리소스를 찾지 못했을 때의 예외 처리가 명확하게 구현되어 있습니다. 메시지도 한글로 일관성 있게 작성되어 있습니다.
src/main/java/ddingdong/ddingdongBE/domain/vodprocessing/entity/VodProcessingJob.java (2)
41-49: 빌더 패턴이 적절히 구현되어 있습니다.
모든 필드를 포함하는 빌더가 잘 구현되어 있으며, private 생성자를 통해 객체 생성을 제어하고 있습니다.
28-30: 연관 관계 매핑 검토 필요
VodProcessingNotification과의 일대일 관계에서 cascade 설정이 누락되어 있습니다. 알림 엔티티의 생명주기를 관리하는 방식을 명확히 해야 합니다.
src/main/java/ddingdong/ddingdongBE/domain/documents/service/GeneralDocumentService.java (1)
16-16: 클래스 이름 변경이 적절합니다.
DocumentServiceImpl에서 GeneralDocumentService로의 이름 변경은 구현의 성격을 더 잘 표현하고 있습니다.
src/main/java/ddingdong/ddingdongBE/domain/feed/service/FacadeFeedService.java (1)
22-22: 서비스 이름 변경에 따른 영향도 확인 필요
feedService에서 generalFeedService로의 이름 변경이 다른 컴포넌트에 미치는 영향을 확인해주세요.
다음 스크립트로 연관된 참조를 확인할 수 있습니다:
Also applies to: 26-26, 33-33, 40-40
✅ Verification successful
Let me verify the git history to see if this is a recent rename and check for any other potential references.
변수명 변경이 안전하게 이루어졌음을 확인했습니다
코드베이스 분석 결과, feedService에서 generalFeedService로의 이름 변경이 FacadeFeedService 클래스 내부에서만 이루어졌으며, 다른 컴포넌트에는 영향을 미치지 않았습니다. 모든 참조가 올바르게 업데이트되어 있고 다른 곳에서 이전 이름을 사용하는 경우가 없습니다.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# feedService 참조 확인
rg "feedService" --type java
# GeneralFeedService 사용 확인
rg "GeneralFeedService" --type java
Length of output: 312
Script:
#!/bin/bash
# Check git history for feedService rename
git log -p | rg -A 5 -B 5 "feedService|GeneralFeedService"
# Check for constructor/autowired usage
ast-grep --pattern 'class $_ {
$$$
@Autowired
private $_FeedService $_;
$$$
}'
# Check for any Feed related service references
rg "Feed.*Service" --type java
Length of output: 2375
| @@ -24,7 +24,7 @@ public UploadUrlResponse getPreSignedUrl(PrincipalDetails principalDetails, Stri | |||
| LocalDateTime now = LocalDateTime.now(); | |||
| GeneratePreSignedUrlRequestQuery query = | |||
| s3FileService.generatePresignedUrlRequest( | |||
| new GeneratePreSignedUrlRequestCommand(now, user.getAuthId(), fileName)); | |||
| new GeneratePreSignedUrlRequestCommand(now, user.getId(), fileName)); | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
사용자 유효성 검사 추가 필요
user.getId()를 호출하기 전에 다음과 같은 유효성 검사가 필요합니다:
- 사용자 객체가 null이 아닌지 확인
- ID가 유효한지 확인
다음과 같은 방식으로 개선을 제안드립니다:
@Override
public UploadUrlResponse getPreSignedUrl(PrincipalDetails principalDetails, String fileName) {
User user = principalDetails.getUser();
+ if (user == null || user.getId() == null) {
+ throw new IllegalArgumentException("유효하지 않은 사용자입니다.");
+ }
LocalDateTime now = LocalDateTime.now();
GeneratePreSignedUrlRequestQuery query =
s3FileService.generatePresignedUrlRequest(
new GeneratePreSignedUrlRequestCommand(now, user.getId(), fileName));📝 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.
| new GeneratePreSignedUrlRequestCommand(now, user.getId(), fileName)); | |
| @Override | |
| public UploadUrlResponse getPreSignedUrl(PrincipalDetails principalDetails, String fileName) { | |
| User user = principalDetails.getUser(); | |
| if (user == null || user.getId() == null) { | |
| throw new IllegalArgumentException("유효하지 않은 사용자입니다."); | |
| } | |
| LocalDateTime now = LocalDateTime.now(); | |
| GeneratePreSignedUrlRequestQuery query = | |
| s3FileService.generatePresignedUrlRequest( | |
| new GeneratePreSignedUrlRequestCommand(now, user.getId(), fileName)); |
src/main/java/ddingdong/ddingdongBE/domain/vodprocessing/service/VodProcessingJobService.java
Show resolved
Hide resolved
| public record CreatePendingVodProcessingJobRequest( | ||
| String convertJobId, | ||
| String userId | ||
| ) { |
There was a problem hiding this comment.
요청 DTO에 유효성 검증이 필요합니다.
필드 유효성 검증을 위한 Jakarta Validation 어노테이션 추가가 필요합니다:
public record CreatePendingVodProcessingJobRequest(
@NotBlank(message = "변환 작업 ID는 필수입니다")
String convertJobId,
@NotBlank(message = "사용자 ID는 필수입니다")
String userId
) {
...va/ddingdong/ddingdongBE/domain/vodprocessing/service/FacadeVodProcessingJobServiceImpl.java
Show resolved
Hide resolved
...va/ddingdong/ddingdongBE/domain/vodprocessing/service/FacadeVodProcessingJobServiceImpl.java
Show resolved
Hide resolved
src/main/java/ddingdong/ddingdongBE/domain/vodprocessing/entity/VodProcessingNotification.java
Show resolved
Hide resolved
| CREATE TABLE vod_processing_job | ||
| ( | ||
| id BIGINT AUTO_INCREMENT NOT NULL, | ||
| created_at timestamp NULL, | ||
| updated_at timestamp NULL, | ||
| notification_id BIGINT NULL, | ||
| convert_job_id VARCHAR(255) NOT NULL, | ||
| user_id VARCHAR(255) NOT NULL, | ||
| convert_job_status VARCHAR(255) NULL, | ||
| CONSTRAINT PK_vod_processing_job PRIMARY KEY (id) | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
vod_processing_job 테이블 스키마 개선이 필요합니다.
다음 사항들을 고려해 주세요:
created_at과updated_at은 NOT NULL로 설정하는 것이 좋습니다convert_job_status는 중요한 상태 값이므로 NOT NULL로 설정하고 기본값을 지정하는 것이 좋습니다user_id와convert_job_id컬럼에 대한 인덱스 추가를 고려해 주세요
CREATE TABLE vod_processing_job
(
id BIGINT AUTO_INCREMENT NOT NULL,
- created_at timestamp NULL,
- updated_at timestamp NULL,
+ created_at timestamp NOT NULL,
+ updated_at timestamp NOT NULL,
notification_id BIGINT NULL,
convert_job_id VARCHAR(255) NOT NULL,
user_id VARCHAR(255) NOT NULL,
- convert_job_status VARCHAR(255) NULL,
+ convert_job_status VARCHAR(255) NOT NULL DEFAULT 'PENDING',
CONSTRAINT PK_vod_processing_job PRIMARY KEY (id)
);
+CREATE INDEX IX_vod_processing_job_user_id ON vod_processing_job(user_id);
+CREATE INDEX IX_vod_processing_job_convert_job_id ON vod_processing_job(convert_job_id);📝 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.
| CREATE TABLE vod_processing_job | |
| ( | |
| id BIGINT AUTO_INCREMENT NOT NULL, | |
| created_at timestamp NULL, | |
| updated_at timestamp NULL, | |
| notification_id BIGINT NULL, | |
| convert_job_id VARCHAR(255) NOT NULL, | |
| user_id VARCHAR(255) NOT NULL, | |
| convert_job_status VARCHAR(255) NULL, | |
| CONSTRAINT PK_vod_processing_job PRIMARY KEY (id) | |
| ); | |
| CREATE TABLE vod_processing_job | |
| ( | |
| id BIGINT AUTO_INCREMENT NOT NULL, | |
| created_at timestamp NOT NULL, | |
| updated_at timestamp NOT NULL, | |
| notification_id BIGINT NULL, | |
| convert_job_id VARCHAR(255) NOT NULL, | |
| user_id VARCHAR(255) NOT NULL, | |
| convert_job_status VARCHAR(255) NOT NULL DEFAULT 'PENDING', | |
| CONSTRAINT PK_vod_processing_job PRIMARY KEY (id) | |
| ); | |
| CREATE INDEX IX_vod_processing_job_user_id ON vod_processing_job(user_id); | |
| CREATE INDEX IX_vod_processing_job_convert_job_id ON vod_processing_job(convert_job_id); |
.../java/ddingdong/ddingdongBE/domain/vodprocessing/service/GeneralVodProcessingJobService.java
Show resolved
Hide resolved
| @Column(nullable = false) | ||
| private String convertJobId; | ||
|
|
||
| @Column(nullable = false) | ||
| private String userId; | ||
|
|
||
| @Enumerated(EnumType.STRING) | ||
| private ConvertJobStatus convertJobStatus; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
상태 값 제약조건 추가 필요
convertJobStatus에 대한 기본값 설정이 누락되어 있습니다. 또한 userId와 convertJobId의 길이 제한도 추가하면 좋겠습니다.
@Column(nullable = false)
+@Size(max = 255)
private String convertJobId;
@Column(nullable = false)
+@Size(max = 255)
private String userId;
@Enumerated(EnumType.STRING)
+@Column(nullable = false)
+@Builder.Default
private ConvertJobStatus convertJobStatus = ConvertJobStatus.PENDING;📝 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.
| @Column(nullable = false) | |
| private String convertJobId; | |
| @Column(nullable = false) | |
| private String userId; | |
| @Enumerated(EnumType.STRING) | |
| private ConvertJobStatus convertJobStatus; | |
| @Column(nullable = false) | |
| @Size(max = 255) | |
| private String convertJobId; | |
| @Column(nullable = false) | |
| @Size(max = 255) | |
| private String userId; | |
| @Enumerated(EnumType.STRING) | |
| @Column(nullable = false) | |
| @Builder.Default | |
| private ConvertJobStatus convertJobStatus = ConvertJobStatus.PENDING; |
| API_PREFIX + "/feeds/**", | ||
| API_PREFIX + "/internal/**") |
There was a problem hiding this comment.
내부 API 엔드포인트에 대한 보안 강화 필요
내부 API 엔드포인트(/internal/**)에 대한 무제한 접근 허용은 잠재적인 보안 위험을 초래할 수 있습니다. 내부 API는 일반적으로 더 엄격한 접근 제어가 필요합니다.
다음과 같은 보안 강화 방안을 고려해 주세요:
- IP 화이트리스트 적용
- API 키 인증 추가
- 내부 네트워크 접근 제한
예시 구현:
- API_PREFIX + "/internal/**")
+ API_PREFIX + "/internal/**").hasIpAddress("trusted-ip-range")또는 커스텀 필터를 통한 API 키 검증:
@Component
public class InternalApiAuthFilter extends OncePerRequestFilter {
@Override
protected void doFilterInternal(HttpServletRequest request,
HttpServletResponse response,
FilterChain filterChain) throws ServletException, IOException {
if (request.getRequestURI().contains("/internal/")) {
String apiKey = request.getHeader("X-API-Key");
if (!isValidApiKey(apiKey)) {
response.sendError(HttpStatus.UNAUTHORIZED.value());
return;
}
}
filterChain.doFilter(request, response);
}
}💡 Codebase verification
내부 API 엔드포인트 보안 검증 완료
내부 API 엔드포인트의 사용이 적절하게 제한되어 있습니다:
/server/internal/trigger/vod-processing-job엔드포인트만 내부 API로 사용되고 있으며,@Hidden어노테이션으로 문서화에서 제외되어 있습니다.- SecurityConfig에서
/internal/**경로에 대한 접근 제어가permitAll()로 설정되어 있어, 보안 강화가 필요할 수 있습니다.
🔗 Analysis chain
내부 API 엔드포인트 사용 현황 확인 필요
코드베이스 전체에서 내부 API 엔드포인트의 사용 패턴을 확인하여 일관된 보안 정책을 적용해야 합니다.
Let me gather more information about the internal endpoints and their security configuration.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: 내부 API 엔드포인트 사용 패턴 확인
# 컨트롤러에서 내부 API 엔드포인트 매핑 검색
echo "컨트롤러의 내부 API 엔드포인트 매핑:"
rg -t java "@RequestMapping.*internal|@GetMapping.*internal|@PostMapping.*internal|@PutMapping.*internal|@DeleteMapping.*internal"
# 보안 관련 설정 파일 검색
echo "\n보안 설정 파일:"
fd -e java -e yml -e properties | rg -i "security|auth"
Length of output: 1221
Script:
#!/bin/bash
# Check SecurityConfig implementation for internal endpoints
echo "SecurityConfig implementation:"
ast-grep --pattern 'class SecurityConfig {
$$$
}'
# Check for any internal API controllers
echo -e "\nInternal API Controllers:"
fd -e java | xargs rg "class.*Internal.*Controller"
# Check for any internal-related paths or constants
echo -e "\nInternal API path constants:"
rg -t java "internal" -C 2
Length of output: 1610
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
build.gradle (1)
122-129: JaCoCo 제외 패턴이 Sonar 설정과 일관성있게 구성되었습니다.JaCoCo의 제외 패턴이 sonar.coverage.exclusions와 동일하게 설정되어 있어 일관성이 유지되고 있습니다. 다만, 한 가지 제안드리고 싶은 점이 있습니다.
제외 패턴을 상수로 추출하여 재사용하면 더 유지보수하기 좋을 것 같습니다. 다음과 같이 수정을 고려해보세요:
+ def coverageExclusions = [ + "**/ddingdong/ddingdongBE/common/**", + "**/ddingdong/ddingdongBE/**/**Request**", + "**/ddingdong/ddingdongBE/**/**Response**", + "**/ddingdong/ddingdongBE/**/**Command**", + "**/ddingdong/ddingdongBE/**/**Query**", + "**/ddingdong/ddingdongBE/**/**Dto**", + "**/ddingdong/ddingdongBE/**/**Controller**", + "**/ddingdong/ddingdongBE/**/General*Service.class" + ] sonar { properties { // ... other properties ... property "sonar.coverage.exclusions", coverageExclusions.join(',') } } jacocoTestReport { // ... other config ... afterEvaluate { classDirectories.setFrom(files(classDirectories.files.collect { fileTree(dir: it, exclude: coverageExclusions) })) } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
build.gradle(1 hunks)
🔇 Additional comments (2)
build.gradle (2)
100-102: sonar.exclusions 설정이 간소화되었습니다.
common 디렉토리만 제외하도록 설정이 단순화되었네요. 이는 적절해 보입니다.
105-114: 코드 커버리지 제외 패턴이 체계적으로 정의되었습니다.
새로운 sonar.coverage.exclusions 설정이 다음과 같은 패턴들을 명확하게 정의하고 있습니다:
- DTO 관련 클래스 (Request, Response, Command, Query, Dto)
- 컨트롤러 클래스
- General로 시작하는 서비스 구현체
이는 테스트 커버리지 측정에서 불필요한 클래스들을 효과적으로 제외할 수 있게 해줍니다.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
build.gradle (1)
114-121: JaCoCo 제외 패턴이 적절히 구성되었습니다.테스트 커버리지 측정 시 제외할 클래스들이 명확하게 정의되어 있습니다. 특히 DTO, Controller, Command 등 테스트가 불필요한 클래스들이 잘 식별되어 있습니다.
한 가지 제안사항으로는,
General*Service패턴이 너무 광범위할 수 있으니 좀 더 구체적인 패턴을 고려해 보시면 좋을 것 같습니다.다음과 같이 더 구체적인 패턴을 고려해 보세요:
- "**/ddingdong/ddingdongBE/**/General*Service.class" + "**/ddingdong/ddingdongBE/**/facade/General*Service.class"
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
build.gradle(1 hunks)
🔇 Additional comments (1)
build.gradle (1)
100-104: SonarQube 설정이 개선되었습니다.
코드 커버리지 제외 패턴이 잘 정의되어 있으며, 멀티라인 문자열 형식으로의 변경으로 가독성이 향상되었습니다. 다만, 새로 추가된 VOD 처리 작업 관련 클래스들의 테스트 커버리지가 제외되지 않도록 주의해 주세요.
✅ Verification successful
SonarQube 설정이 적절하게 구성되어 있습니다.
VOD 처리 관련 클래스들의 테스트 커버리지 제외 패턴을 확인한 결과:
- 테스트가 필요한 핵심 비즈니스 로직 클래스들은 제외되지 않습니다:
FacadeVodProcessingJobServiceImplVodProcessingJobVodProcessingNotification
- 적절하게 제외된 클래스들:
GeneralVodProcessingJobService(General*Service 패턴)VodProcessingJobController(Controller 패턴)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# VOD 처리 관련 클래스들이 제외 패턴에 포함되지 않는지 확인
fd -e java -x grep -l "class.*Vod" {} \; | while read -r file; do
if echo "$file" | grep -qE "(Request|Response|Command|Query|Dto|Controller|General.*Service)\.java$"; then
echo "제외될 클래스: $file"
else
echo "테스트 대상 클래스: $file"
fi
done
Length of output: 2715
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
build.gradle (1)
114-121: JaCoCo 제외 패턴이 체계적으로 구성되었습니다.새로운
General*Service패턴을 포함하여 테스트 커버리지 제외 대상이 일관된 구조로 잘 정의되어 있습니다. 다만, 향후 새로운 패턴이 추가될 때마다 이 설정을 업데이트해야 할 수 있으므로, 문서화를 통해 팀원들과 이 규칙을 공유하는 것이 좋을 것 같습니다.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
build.gradle(1 hunks)
🔇 Additional comments (1)
build.gradle (1)
100-104: SonarQube 설정이 적절해 보입니다.
코드 커버리지 제외 패턴이 잘 구성되어 있으며, 멀티라인 문자열 형식으로의 변경으로 가독성이 향상되었습니다.
| import org.springframework.web.bind.annotation.RestController; | ||
|
|
||
| @Hidden | ||
| @RestController("/server/internal/trigger/vod-processing-job") |
There was a problem hiding this comment.
url 매핑은 @RequestMapping에 해야 할 것 같습니다.
그리고 혹시 Api 인터페이스를 안만든 이유가 따로 있나요?!
There was a problem hiding this comment.
해당 컨트롤러는 클라이언트에게 제공되는 API가 아니라 trigging을 위한 API 여서 따로 swagger 명세를 안하기 위해서 인터페이스를 명세하지 않았습니다.
|
🚀 작업 내용
POST: /server/internal/vod-processing-job+lamda를 통한 실제 트리깅은 테스트 서버 배포 후 따로 진행 예정입니다.
🤔 고민했던 내용
이에 기존 s3Key에서 userId를 사용하는 것으로 대체한다면 vodProcessingJob을 생성할 때 처음부터 매핑이 완료된 상태로 요청을 보낼 수 있어 해당 방식으로 수정하였습니다.
💬 리뷰 중점사항
lamda 트리깅을 API URL 설계가 괜찮은지 의견 부탁드립니다!
Summary by CodeRabbit
릴리스 노트
신규 기능
/server/internal/trigger/vod-processing-job)가 추가되었습니다.vod_processing_job,vod_processing_notification)이 생성되었습니다.버그 수정
문서화
리팩토링