[REFACTOR] S3 파일 업로드 모듈 범용화 및 미사용 코드 정리 - #157
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthrough녹음 업로드 URL 발급을 Playing 도메인 전용 DTO와 공통 ChangesS3 업로드 모듈 범용화
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/test/java/com/mr/global/file/s3/service/RecordingObjectKeyGeneratorTest.java (1)
21-21: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win테스트 클래스 이름을
S3ObjectKeyGeneratorTest로 변경하세요.프로덕션 클래스는
S3ObjectKeyGenerator로 변경되었습니다. 테스트 클래스는RecordingObjectKeyGeneratorTest이름을 유지합니다. 이 PR의 목표는 Recording 전용 이름을 범용 이름으로 바꾸는 것입니다. 테스트 이름이 남으면 검색과 추적이 어려워집니다.Java에서는 public 여부와 무관하게 클래스 이름과 파일 이름을 함께 변경해야 합니다. 파일을
src/test/java/com/mr/global/file/s3/service/S3ObjectKeyGeneratorTest.java로 이동하세요.메서드 이름
belongsToUser(Line 242)와doesNotBelongToUser(Line 263)도belongsToOwner계약에 맞춰belongsToOwner,doesNotBelongToOwner로 정리하면 좋습니다.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/java/com/mr/global/file/s3/service/RecordingObjectKeyGeneratorTest.java` at line 21, Rename the test class RecordingObjectKeyGeneratorTest to S3ObjectKeyGeneratorTest and move the file to S3ObjectKeyGeneratorTest.java to match the production class. Also rename belongsToUser and doesNotBelongToUser to belongsToOwner and doesNotBelongToOwner to align with the belongsToOwner contract.src/test/java/com/mr/global/file/s3/service/S3FileServiceTest.java (1)
696-700: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value불필요한
S3Exception캐스팅을 제거하세요.
S3Exception.builder().build()에서 생성된 객체는S3Exception타입이므로(S3Exception)캐스팅을 지우면 훨씬 clean합니다. 관련 문서는 AWS SDK for Java v2의S3Exception.Builder예시를 참고하세요.两处 캐스팅은 다음과 같습니다.
src/test/java/com/mr/global/file/s3/service/S3FileServiceTest.java:696-700src/test/java/com/mr/global/file/s3/service/S3FileServiceTest.java:725-729🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/java/com/mr/global/file/s3/service/S3FileServiceTest.java` around lines 696 - 700, Remove the redundant S3Exception casts from both S3Exception.builder() constructions in S3FileServiceTest, including the instances around the 404 exception setup and the second builder setup. Keep the existing statusCode, message, and exception behavior unchanged.src/test/java/com/mr/domain/playing/service/PlayingServiceTest.java (1)
1096-1097: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win기대값을 운영 변환 로직과 분리해 주세요.
현재 테스트는
request.toCommand()와RecordingUploadUrlResponse.from(presignedUpload)를 기대값 생성에 재사용합니다. 변환 로직이 잘못되어도 테스트가 같은 잘못된 값을 기대하면 통과할 수 있습니다.FileUploadCommand의 파일명, Content-Type, 파일 크기를 직접 구성하고, 응답도 공개 필드를 명시적으로 검증해PlayingService와 S3 계약을 독립적으로 확인해 주세요.Also applies to: 1107-1110
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/java/com/mr/domain/playing/service/PlayingServiceTest.java` around lines 1096 - 1097, 테스트의 기대값 생성을 운영 변환 로직과 분리하세요. `PlayingService` 테스트에서 `request.toCommand()` 대신 파일명, Content-Type, 파일 크기를 직접 지정해 `FileUploadCommand`를 구성하고, `RecordingUploadUrlResponse.from(presignedUpload)`도 사용하지 말고 응답의 공개 필드를 직접 검증하여 S3 계약을 독립적으로 확인하세요.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/java/com/mr/global/file/s3/service/S3FileService.java`:
- Around line 58-71: createPresignedUpload()는 command.fileSize()만 신뢰하므로, URL 발급
전후에 S3 최대 업로드 크기를 강제하도록 수정하세요. Presigned POST의 content-length-range 정책이나 적용 가능한
버킷 정책을 사용해 제한을 업로드 자체에 포함하고, 현재 PUT presigning 흐름과 호환되지 않으면 해당 방식으로 전환하세요.
validateUploadedFile()에 의존하지 않도록 하며, 필요하면 초과 또는 미완료 객체를 정리하는 Lifecycle 정책도
구성하세요.
In `@src/main/java/com/mr/global/file/s3/service/S3ObjectKeyGenerator.java`:
- Around line 84-91: S3ObjectKeyGenerator의 확장자 결정 흐름에서 파일명 확장자만
validateExtension으로 통과시키지 말고, resolveExtensionByContentType로 정규화한 contentType의
대표 확장자를 먼저 구한 뒤 originalFileName의 확장자와 일치하는지 검증하도록 update하세요. extractExtension,
validateExtension, resolveExtensionByContentType의 연결을 유지하되, recording.mp3와
audio/webm처럼 서로 다른 허용 조합이 통과하지 않도록 같은 형식일 때만 Object Key와 S3 메타데이터가 일치하게 만드세요.
---
Nitpick comments:
In `@src/test/java/com/mr/domain/playing/service/PlayingServiceTest.java`:
- Around line 1096-1097: 테스트의 기대값 생성을 운영 변환 로직과 분리하세요. `PlayingService` 테스트에서
`request.toCommand()` 대신 파일명, Content-Type, 파일 크기를 직접 지정해 `FileUploadCommand`를
구성하고, `RecordingUploadUrlResponse.from(presignedUpload)`도 사용하지 말고 응답의 공개 필드를 직접
검증하여 S3 계약을 독립적으로 확인하세요.
In
`@src/test/java/com/mr/global/file/s3/service/RecordingObjectKeyGeneratorTest.java`:
- Line 21: Rename the test class RecordingObjectKeyGeneratorTest to
S3ObjectKeyGeneratorTest and move the file to S3ObjectKeyGeneratorTest.java to
match the production class. Also rename belongsToUser and doesNotBelongToUser to
belongsToOwner and doesNotBelongToOwner to align with the belongsToOwner
contract.
In `@src/test/java/com/mr/global/file/s3/service/S3FileServiceTest.java`:
- Around line 696-700: Remove the redundant S3Exception casts from both
S3Exception.builder() constructions in S3FileServiceTest, including the
instances around the 404 exception setup and the second builder setup. Keep the
existing statusCode, message, and exception behavior unchanged.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: c926a200-6eda-436a-8e10-161ef33f4bdd
📒 Files selected for processing (17)
src/main/java/com/mr/domain/playing/controller/PlayingController.javasrc/main/java/com/mr/domain/playing/dto/req/RecordingUploadUrlRequest.javasrc/main/java/com/mr/domain/playing/dto/res/RecordingUploadUrlResponse.javasrc/main/java/com/mr/domain/playing/service/PlayingService.javasrc/main/java/com/mr/global/file/s3/controller/RecordingUploadController.javasrc/main/java/com/mr/global/file/s3/dto/FileUploadCommand.javasrc/main/java/com/mr/global/file/s3/dto/PresignedUrlUpload.javasrc/main/java/com/mr/global/file/s3/dto/ValidatedFile.javasrc/main/java/com/mr/global/file/s3/dto/req/RecordingUploadCompleteRequest.javasrc/main/java/com/mr/global/file/s3/dto/res/RecordingUploadCompleteResponse.javasrc/main/java/com/mr/global/file/s3/service/RecordingUploadService.javasrc/main/java/com/mr/global/file/s3/service/S3FileService.javasrc/main/java/com/mr/global/file/s3/service/S3ObjectKeyGenerator.javasrc/test/java/com/mr/domain/playing/service/PlayingServiceTest.javasrc/test/java/com/mr/global/file/s3/service/RecordingObjectKeyGeneratorTest.javasrc/test/java/com/mr/global/file/s3/service/RecordingUploadServiceTest.javasrc/test/java/com/mr/global/file/s3/service/S3FileServiceTest.java
💤 Files with no reviewable changes (5)
- src/main/java/com/mr/global/file/s3/dto/req/RecordingUploadCompleteRequest.java
- src/main/java/com/mr/global/file/s3/dto/res/RecordingUploadCompleteResponse.java
- src/main/java/com/mr/global/file/s3/controller/RecordingUploadController.java
- src/test/java/com/mr/global/file/s3/service/RecordingUploadServiceTest.java
- src/main/java/com/mr/global/file/s3/service/RecordingUploadService.java
| PutObjectRequest putObjectRequest = | ||
| PutObjectRequest.builder() | ||
| .bucket(s3Properties.bucket()) | ||
| .key(objectKey) | ||
| .contentType(normalizedContentType) | ||
| .build(); | ||
|
|
||
| PutObjectPresignRequest presignRequest = | ||
| PutObjectPresignRequest.builder() | ||
| .signatureDuration( | ||
| s3Properties.presignedUrlExpiration() | ||
| ) | ||
| .putObjectRequest(putObjectRequest) | ||
| .build(); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Presigned PUT에 업로드 크기 조건이 포함되는지와 미완료 객체 정리 정책을 확인합니다.
rg -n -C 4 \
'PutObjectPresignRequest|PostObject|content-length-range|contentLength|maxFileSize|LifecycleConfiguration|deleteObject' \
srcRepository: Musereview/BE
Length of output: 31061
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== S3FileService relevant sections =="
sed -n '1,260p' src/main/java/com/mr/global/file/s3/service/S3FileService.java
echo
echo "== S3Properties =="
sed -n '1,80p' src/main/java/com/mr/global/file/s3/config/S3Properties.java
echo
echo "== upload callers requesting presigned URL =="
rg -n -C 5 'generatePresignedUrlForUpload|presignUrlForUpload|presign|fileSize|objectKeyGenerator' src/main/javaRepository: Musereview/BE
Length of output: 26932
Denial of Service (CWE-400): Uncontrolled Resource Consumption
Reachability: External
Presigned URL 발급 전에 최대 업로드 크기를 강제하세요.
createPresignedUpload()는 command.fileSize()만 검증하고, 실제 Content-Length 검증은 validateUploadedFile() 뒤에서 실행됩니다. 사용자가 작은 fileSize로 URL을 받은 뒤 제한을 초과하는 객체를 업로드하는 경우, validateUploadedFile() 호출을 생략하면 초과 객체가 S3에 남습니다. Presigned POST의 content-length-range 정책, 버킷 정책, 또는 미완료 업로드 정리 Lifecycle 정책으로 URL 발급 전후 크기를 함께 제어하세요. AWS 업로드 정책 문서도 참고하면 좋습니다. 예: AWS S3 upload restrictions, presigned POST content-length range, Lifecycle rules.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/java/com/mr/global/file/s3/service/S3FileService.java` around lines
58 - 71, createPresignedUpload()는 command.fileSize()만 신뢰하므로, URL 발급 전후에 S3 최대
업로드 크기를 강제하도록 수정하세요. Presigned POST의 content-length-range 정책이나 적용 가능한 버킷 정책을 사용해
제한을 업로드 자체에 포함하고, 현재 PUT presigning 흐름과 호환되지 않으면 해당 방식으로 전환하세요.
validateUploadedFile()에 의존하지 않도록 하며, 필요하면 초과 또는 미완료 객체를 정리하는 Lifecycle 정책도
구성하세요.
There was a problem hiding this comment.
Lifecycle 정책은 추후 도입 예정입니다.
There was a problem hiding this comment.
@on1yoneprivate 확인했습니다. Lifecycle 정책을 별도 작업으로 도입하는 방향은 적절합니다.
다만 Lifecycle 정책은 남은 객체를 나중에 삭제합니다. Presigned PUT의 업로드 크기를 즉시 제한하지는 않습니다. 따라서 현재 지적 사항은 이 PR에서는 미해결 상태로 유지하겠습니다.
원하시면 S3 Lifecycle 정리 정책과 업로드 크기 제한 방안을 포함한 후속 GitHub 이슈를 생성하겠습니다. 🐇
You are interacting with an AI system.
There was a problem hiding this comment.
리팩토링 방향 좋습니다~ Playing 도메인이 S3 DTO에 직접 의존 안 하게 분리한 것도 깔끔하고, FileUploadCommand/ValidatedFile/PresignedUrlUpload로 범용 DTO 나눈 것도 프로필 이미지, 백킹트랙 업로드를 고려하는 좋은 구조라고 생각해요.
코멘트 확인 부탁드려요!
+코드래빗이 리뷰 달기 전에 작성해서 코드래빗과 중복되는 부분이 있을 수 있습니다... 중복된 부분이면 그냥 리졸브해서 넘겨주세요!
| normalizedContentType | ||
| ); | ||
|
|
||
| PutObjectRequest putObjectRequest = |
There was a problem hiding this comment.
🔴 P1 — S3FileService.java (58~71줄, createPresignedUpload())
PutObjectRequest putObjectRequest = PutObjectRequest.builder()
.bucket(s3Properties.bucket())
.key(objectKey)
.contentType(normalizedContentType)
.build();
PutObjectPresignRequest presignRequest = PutObjectPresignRequest.builder()
.signatureDuration(s3Properties.presignedUrlExpiration())
.putObjectRequest(putObjectRequest)
.build();문제 상황: presigned URL 발급 시점엔 command.fileSize()만 검증하고, 실제 업로드된 파일의 크기 검증은 이후 validateUploadedFile() 호출 시점에야 이뤄져요.
왜 문제가 될 수 있는가?: 클라이언트가 작은 fileSize로 URL을 발급받고 실제로는 훨씬 큰 파일을 업로드한 뒤, 업로드 완료 흐름(validateUploadedFile())을 아예 호출하지 않으면 초과 용량 객체가 검증 없이 S3에 그대로 남아요. presigned PUT 자체엔 크기 제한을 강제하는 옵션이 없다 보니 저장 비용이나 악의적인 대용량 업로드에 취약할 수 있어요.
수정 방향: presigned PUT 대신 content-length-range 조건을 넣을 수 있는 presigned POST로 전환하거나, 버킷 정책 + Lifecycle 룰(미완료/초과 객체 자동 정리)로 업로드 자체에 제한을 거는 방향은 어떨까요?
혹시 이 부분은 리뷰 요청사항으로 남기신 "예외 코드 처리 방식"이랑 연결돼서 다음 PR에서 다루실 예정이라 남겨두신걸까요?
There was a problem hiding this comment.
지적해 주신 부분 이해했습니다.
다만 현재 프론트 연동 일정상 업로드 방식을 PUT에서 POST로 변경하기에는 영향 범위가 있어, 논의가 필요할 것 같습니다. 따라서 PR에서는 리팩토링 범위에 집중하고 Presigned PUT 방식을 유지하려고 합니다.
대신 후속 이슈로 미검증 객체에 대한 Lifecycle 정책을 적용하여 운영상 위험을 최소화하고,
Presigned POST 전환 및 content-length-range를 통한 실제 업로드 크기 강제는 디벨롭 기간에 반영하려고 합니다!
3 participants
📍 개요
S3 파일 업로드 모듈을 Recording 도메인에서 분리하여 범용적으로 사용할 수 있도록 리팩토링했습니다.
또한 Playing 도메인의 S3 의존성을 줄이고, 미사용 API 및 코드를 제거하여 구조를 단순화했습니다.
⛓️💥 관련 이슈
🛠️ 작업 내용
S3 파일 업로드 모듈 리팩토링
RecordingUploadService→S3FileService로 리팩토링S3ObjectKeyGenerator로 변경FileUploadCommandValidatedFilePresignedUrlUploadvalidateUploadedFile()로 통합Playing 도메인 의존성 분리
RecordingUploadUrlRequestRecordingUploadUrlResponseglobal.file.s3DTO에 직접 의존하지 않도록 수정코드 정리
/api/s3/complete) 제거테스트
S3FileServiceTest리팩토링PlayingServiceTest수정🔥 리뷰 요청 사항
✅ 체크리스트
📎 참고 사항
Summary by CodeRabbit
변경 사항
개선 사항