[feature] 동아리 활동사진 이미지 다중 업로드 지원하도록 수정한다 #808
Conversation
yw6938
requested review from
PororoAndFriends,
Zepelown,
alsdddk and
lepitaaar
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning
|
| Cohort / File(s) | 변경 요약 |
|---|---|
빌드 의존성 \backend/build.gradle`` |
software.amazon.awssdk:url-connection-client:2.26.0 의존성 추가 |
에러 코드 확장 \backend/src/main/java/moadong/global/exception/ErrorCode.java`` |
클럽/파일/사용자/토큰/비밀번호/지원서/암호화/FCM 등 다수(약 27개) ErrorCode 상수 추가 및 enum 종결 세미콜론 제거 |
컨트롤러: multipart → presigned URL \backend/src/main/java/moadong/media/controller/ClubImageController.java`` |
Multipart 업로드 엔드포인트 제거, generate* / complete* 2단계 엔드포인트로 교체; DTO(UploadUrlRequest, UploadCompleteRequest, PresignedUploadResponse) 사용; 생성자 주입(@Qualifier("cloudflare")) 추가; putFeeds에 @Valid 적용 |
서비스 인터페이스 변경 \backend/src/main/java/moadong/media/service/ClubImageService.java`` |
Multipart 기반 메서드 제거; generateLogoUploadUrl, generateFeedUploadUrls, generateCoverUploadUrl(PresignedUploadResponse 반환) 및 completeLogoUpload, completeCoverUpload 추가; 기존 delete/update 메서드 유지 |
클라우드 이미지 구현 변경 \backend/src/main/java/moadong/media/service/CloudflareImageService.java`` |
S3Presigner 통합으로 presigned URL 생성/완료 로직 추가; 업로드 파일명·URL·크기 검증; headObject 검사; 트랜잭션 경계 추가; 로깅·예외 처리 강화; 내부 헬퍼 메서드 다수 추가 |
업로드 DTO 추가 \backend/src/main/java/moadong/media/dto/PresignedUploadResponse.java`, `backend/src/main/java/moadong/media/dto/UploadCompleteRequest.java`, `backend/src/main/java/moadong/media/dto/UploadUrlRequest.java`` |
Presigned URL 응답 및 생성/완료 요청을 위한 record 타입 추가(유효성 애노테이션 포함) |
S3 설정 확장 \backend/src/main/java/moadong/media/util/S3Config.java`` |
S3Presigner 빈 추가(close destroyMethod), 자격증명 유효성 검증 로직 추가 및 s3Client/presigner 생성 시 검증 호출 |
테스트 제거 \backend/src/test/java/moadong/media/service/CloudflareClubImageServiceFeedTest.java`, `backend/src/test/java/moadong/media/service/CloudflareClubImageServiceLogoTest.java`` |
기존 multipart 기반 및 이전 흐름에 의존한 단위 테스트 클래스 삭제 |
DTO 유효성 강화 \backend/src/main/java/moadong/media/dto/FeedUpdateRequest.java`` |
feeds 필드에 @NotNull 추가 |
Sequence Diagram(s)
sequenceDiagram
participant Client
participant Controller as ClubImageController
participant Service as ClubImageService
participant Impl as CloudflareImageService
participant Presigner as S3Presigner
participant S3 as AWS S3
rect rgb(220,235,255)
Note over Client,Impl: 1) Presigned URL 생성
Client->>Controller: POST /generateXUploadUrl(clubId, fileName, contentType)
Controller->>Service: generateXUploadUrl(...)
Service->>Impl: generateXUploadUrl(...)
Impl->>Impl: validateFileName()/prepareFinalUrl()
Impl->>Presigner: presignPutObject(...)
Presigner-->>Impl: presignedUrl + requiredHeaders
Impl-->>Service: PresignedUploadResponse
Service-->>Controller: PresignedUploadResponse
Controller-->>Client: {presignedUrl, finalUrl, requiredHeaders}
end
rect rgb(220,255,220)
Note over Client,S3: 2) 클라이언트가 S3로 업로드
Client->>S3: PUT presignedUrl + headers + body
S3-->>Client: 200 OK
end
rect rgb(255,240,220)
Note over Client,Impl: 3) 업로드 완료 확인
Client->>Controller: POST /completeXUpload(clubId, fileUrl)
Controller->>Service: completeXUpload(...)
Service->>Impl: completeXUpload(...)
Impl->>S3: headObject(key)
S3-->>Impl: metadata(size)
alt size 및 유효성 통과
Impl->>Impl: update DB (transactional)
Impl-->>Service: void
Service-->>Controller: 200 OK
else 크기 초과/유효하지 않음
Impl->>S3: deleteObject(key)
Impl-->>Service: throw RestApiException
Service-->>Controller: error
end
end
Estimated code review effort
🎯 4 (Complex) | ⏱️ ~45분
- 추가로 집중 검토 권장 파일/영역:
- backend/src/main/java/moadong/media/service/CloudflareImageService.java : presigner 통합, headObject 검사, 검증 로직, 트랜잭션 경계
- backend/src/main/java/moadong/media/util/S3Config.java : presigner 빈 구성 및 자격증명 검증
- backend/src/main/java/moadong/global/exception/ErrorCode.java : 신규 코드 일관성 및 중복 여부
- 컨트롤러/서비스 시그니처 변경에 따른 API 호환성, 클라이언트 영향 및 삭제된 테스트 보강 필요성
Possibly related issues
- [refactor] MOA-284 동아리 활동사진 이미지 다중 업로드 지원하도록 수정한다 #779 — 멀티 활동사진 업로드(Generate/Complete presigned flow) 요구와 직접적으로 일치하므로 연결 가능.
Possibly related PRs
- 이미지 저장소 cloudflare로 고정 #500 — controller/service 주입 및 Cloudflare 관련 변경과 코드 레벨 교차점 존재.
- [release] 모아동 BE ver 1.0.2 #329 — ErrorCode enum에 유사 상수 추가로 충돌/중복 가능성.
- [feature] cover image 업로드 및 삭제 기능 구현 #537 — 커버/이미지 업로드 관련 컨트롤러·서비스 변경과 기능적 교차점 존재.
Suggested labels
✨ Feature, 📬 API
Suggested reviewers
- PororoAndFriends
- Zepelown
- lepitaaar
Pre-merge checks and finishing touches
❌ Failed checks (1 warning)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage | Docstring coverage is 9.09% which is insufficient. The required threshold is 80.00%. | You can run @coderabbitai generate docstrings to improve docstring coverage. |
✅ Passed checks (4 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | ✅ Passed | PR 제목은 Pre-Signed URL 기반 다중 업로드 리팩토링의 핵심 내용을 명확하게 반영하고 있습니다. |
| Linked Issues check | ✅ Passed | 코드 변경사항이 MOA-284의 핵심 목표인 Pre-Signed URL 기반 다중 이미지 업로드 구현 요구사항을 충족합니다. |
| Out of Scope Changes check | ✅ Passed | 모든 변경사항이 Pre-Signed URL 업로드 흐름 리팩토링과 관련된 범위 내 변경입니다. |
✨ Finishing touches
- 📝 Generate docstrings
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Post copyable unit tests in a comment
- Commit unit tests in branch
refactor/#779-club-photo-multi-upload-MOA-284
Tip
📝 Customizable high-level summaries are now available in beta!
You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
- Provide your own instructions using the
high_level_summary_instructionssetting. - Format the summary however you like (bullet lists, tables, multi-section layouts, contributor stats, etc.).
- Use
high_level_summary_in_walkthroughto move the summary from the description to the walkthrough section.
Example instruction:
"Divide the high-level summary into five sections:
- 📝 Description — Summarize the main change in 50–60 words, explaining what was done.
- 📓 References — List relevant issues, discussions, documentation, or related PRs.
- 📦 Dependencies & Requirements — Mention any new/updated dependencies, environment variable changes, or configuration updates.
- 📊 Contributor Summary — Include a Markdown table showing contributions:
| Contributor | Lines Added | Lines Removed | Files Changed |- ✔️ Additional Notes — Add any extra reviewer context.
Keep each section concise (under 200 words) and use bullet or numbered lists for clarity."
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later.
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 help to get the list of available commands and usage tips.
Test Results68 tests 65 ✅ 15s ⏱️ Results for commit 8759bf2. ♻️ This comment has been updated with latest results. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (9)
backend/build.gradle(1 hunks)backend/src/main/java/moadong/global/exception/ErrorCode.java(2 hunks)backend/src/main/java/moadong/media/controller/ClubImageController.java(2 hunks)backend/src/main/java/moadong/media/dto/PresignedUploadResponse.java(1 hunks)backend/src/main/java/moadong/media/dto/UploadCompleteRequest.java(1 hunks)backend/src/main/java/moadong/media/dto/UploadUrlRequest.java(1 hunks)backend/src/main/java/moadong/media/service/CloudflareImageService.java(4 hunks)backend/src/main/java/moadong/media/service/ClubImageService.java(1 hunks)backend/src/main/java/moadong/media/util/S3Config.java(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-05-19T05:45:52.957Z
Learnt from: lepitaaar
Repo: Moadong/moadong PR: 406
File: backend/src/main/java/moadong/club/service/ClubApplyService.java:34-38
Timestamp: 2025-05-19T05:45:52.957Z
Learning: The code duplication between createClubApplication and editClubApplication methods in ClubApplyService.java is acknowledged but will be addressed in a future refactoring, as per the developer's plan.
Applied to files:
backend/src/main/java/moadong/media/service/ClubImageService.java
🧬 Code graph analysis (1)
backend/src/main/java/moadong/media/service/CloudflareImageService.java (3)
backend/src/main/java/moadong/club/entity/Club.java (1)
Slf4j(28-153)backend/src/main/java/moadong/global/util/ObjectIdConverter.java (1)
ObjectIdConverter(7-17)backend/src/main/java/moadong/global/util/RandomStringUtil.java (1)
RandomStringUtil(5-22)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
🔇 Additional comments (3)
backend/src/main/java/moadong/global/exception/ErrorCode.java (3)
10-10: LGTM! 에러 코드 분류 주석이 가독성을 크게 개선합니다.각 에러 코드 범위별로 명확한 주석을 추가하여 코드의 유지보수성과 가독성이 향상되었습니다.
Also applies to: 22-22, 34-34, 41-41, 45-45, 49-49, 57-57, 61-61
30-30: Pre-Signed URL 워크플로우에 적합한 에러 코드 추가입니다.새로 추가된
INVALID_FILE_URL과FILE_TOO_LARGE는 Pre-Signed URL 업로드 방식에 필요한 검증 시나리오를 잘 다룹니다:
INVALID_FILE_URL: 업로드 완료 후 URL 검증 시 사용FILE_TOO_LARGE: 클라이언트 측 파일 크기 제한 검증 시 사용Also applies to: 32-32
26-26: IMAGE_DELETE_FAILED와 FILE_DELETE_FAILED의 구분 의도를 명확히 하세요.두 에러 코드가 의미상 매우 유사합니다:
- 601-4: "이미지 삭제에 실패하였습니다"
- 601-9: "파일 삭제에 실패하였습니다."
다만 명명상 구분(IMAGE vs FILE)이 있으므로, 각각이 어떤 컨텍스트에서 사용될 예정인지 명확히 하거나 통합을 고려하세요. 현재 두 에러 코드 모두 코드베이스에서 실제 사용되지 않으므로, 구현 시점에 적절한 에러 코드를 사용하도록 주의하세요.
backend/src/main/java/moadong/media/service/CloudflareImageService.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
backend/src/main/java/moadong/media/service/CloudflareImageService.java (2)
66-76: 트랜잭션 내부에서 S3 작업을 수행하는 설계를 재고하세요.
@Transactional내에서deleteFile(S3 네트워크 호출)을 수행하면 트랜잭션이 길어질 수 있습니다. S3 작업이 실패할 경우 DB 업데이트도 롤백되어 일관성은 유지되지만, 트랜잭션 타임아웃이나 성능 문제를 유발할 수 있습니다.대안:
- S3 삭제를 트랜잭션 외부로 이동 (orphaned files 허용)
- 비동기 삭제 큐 사용
- 현재 방식 유지하되 설계 의도를 주석으로 명시
현재 방식도 데이터 일관성 측면에서 유효하므로 팀의 요구사항에 따라 선택하시면 됩니다.
Also applies to: 154-164
333-340: 파일명이 null/empty일 때의 에러 코드를 재고하세요.
fileName이 null이거나 비어있을 때FILE_NOT_FOUND를 발생시키는 것은 의미상 약간 어색합니다.FILE_NOT_FOUND는 일반적으로 파일 경로에 파일이 존재하지 않을 때 사용하는데, 여기서는 파라미터가 누락되거나 유효하지 않은 상황입니다.
INVALID_FILE_NAME또는MISSING_FILE_NAME과 같은 에러 코드가 더 명확할 수 있습니다. 다만 기존ErrorCode정의에 제약이 있다면 현재 방식도 수용 가능합니다.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
backend/src/main/java/moadong/media/service/CloudflareImageService.java(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
backend/src/main/java/moadong/media/service/CloudflareImageService.java (3)
backend/src/main/java/moadong/club/entity/Club.java (1)
Slf4j(28-153)backend/src/main/java/moadong/global/util/ObjectIdConverter.java (1)
ObjectIdConverter(7-17)backend/src/main/java/moadong/global/util/RandomStringUtil.java (1)
RandomStringUtil(5-22)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
🔇 Additional comments (9)
backend/src/main/java/moadong/media/service/CloudflareImageService.java (9)
6-13: LGTM! 프리사인드 URL 흐름에 필요한 의존성이 적절히 추가되었습니다.로깅 지원(@slf4j), S3Presigner, 그리고 파일 검증을 위한 필요한 임포트들이 올바르게 추가되었습니다.
Also applies to: 36-36, 45-45
80-109: LGTM! 보안 개선이 반영되었습니다.기존 URL은 재검증하지 않고 새 URL만 검증하는 최적화가 적절하며, 이전 리뷰에서 지적된 클럽별 경로 검증(
clubId,FileType파라미터)이 추가되어 보안이 크게 개선되었습니다.
126-151: LGTM! 방어적 에러 처리가 적절합니다.null 체크, 로깅, 그리고 삭제 실패 시에도 계속 진행하는 방식은 멱등성을 고려한 적절한 에러 처리입니다.
167-190: LGTM! URL 생성 시점의 사전 검증이 적절합니다.
generateFeedUploadUrl에서MAX_FEED_COUNT체크는 사전 검증으로 의미가 있습니다. URL 발급과 실제 업로드 완료 사이에 race condition이 있을 수 있지만,completeFeedUpload에서 재검증하므로 최종적으로 보호됩니다.
193-205: LGTM! 업로드 완료 로직이 올바르게 구현되었습니다.기존 파일 삭제 → 새 파일 URL 설정 → 저장 순서가 적절하며, 보안 검증(
validateFileConstraints)도 올바르게 수행됩니다.Also applies to: 233-244
247-280: LGTM! 이전 리뷰의 보안 이슈가 해결되었습니다.
clubId와FileType를 파라미터로 받아 파일 경로가 요청한 클럽과 파일 타입에 해당하는지 검증하여, 다른 클럽의 파일을 참조하거나 임의 삭제하는 보안 취약점이 해결되었습니다.추가로 HeadObject를 통한 파일 크기 검증과 초과 시 즉시 삭제하는 로직도 적절합니다.
Based on learnings
286-292: LGTM! URL에서 key 추출 로직이 명확합니다.null 반환을 통해 호출자가 에러를 처리하도록 하는 방식이 적절합니다.
294-331: LGTM! Presigned URL 생성 로직이 올바르게 구현되었습니다.파일명에 유효하지 않은 문자가 포함된 경우 랜덤 문자열로 대체하는 처리가 적절하며, 10분 만료 시간과 필수 헤더 정보 제공이 클라이언트 직접 업로드에 적합합니다.
342-347: LGTM! Null 체크와 에러 처리가 적절합니다.
ClubRecruitmentInformation이 null인 경우를 방어하여 NPE를 방지하며, 로깅과 명확한 예외 처리가 이루어집니다.
backend/src/main/java/moadong/media/service/CloudflareImageService.java
Outdated
Show resolved
Hide resolved
lepitaaar
left a comment
lepitaaar
left a comment
There was a problem hiding this comment.
커밋단위가 조금 큰거같네요. 커밋하실때 줄이는게 좋을꺼같습니다!
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
backend/src/main/java/moadong/media/service/CloudflareImageService.java (1)
58-62: viewEndpoint가 null일 때 명시적으로 실패 처리하세요.현재
viewEndpoint가 null이면normalizedViewEndpoint가 빈 문자열이 되고,extractKeyOrNull에서 prefix가 "/"가 되어 의도하지 않은 URL과 매칭될 수 있습니다.다음과 같이 명시적 검증을 추가하세요:
@PostConstruct private void init() { + if (viewEndpoint == null || viewEndpoint.isBlank()) { + throw new IllegalStateException("cloud.aws.s3.view-endpoint must be configured"); + } - normalizedViewEndpoint = viewEndpoint != null ? viewEndpoint.replaceAll("/+$", "") : ""; + normalizedViewEndpoint = viewEndpoint.replaceAll("/+$", ""); }
🧹 Nitpick comments (3)
backend/src/main/java/moadong/media/service/ClubImageService.java (1)
17-26: 피드 업로드 흐름의 불일치를 명확히 문서화하세요.로고/커버는
generateXxxUploadUrl→completeXxxUpload2단계 흐름을 따르지만, 피드는generateFeedUploadUrl(여러 번 호출 가능) →updateFeeds(전체 URL 리스트 전달) 패턴을 사용합니다. 이 비대칭 설계는 피드의 다중 업로드를 지원하기 위한 것으로 보이지만, API 사용자에게 혼란을 줄 수 있습니다.인터페이스에 Javadoc을 추가하여 각 메서드의 사용 시나리오와 호출 순서를 명확히 문서화하는 것을 권장합니다.
예시:
+ /** + * 로고 이미지 업로드를 위한 Presigned URL을 생성합니다. + * 생성된 URL로 클라이언트가 직접 업로드 완료 후 completeLogoUpload를 호출해야 합니다. + */ PresignedUploadResponse generateLogoUploadUrl(String clubId, String fileName, String contentType); + + /** + * 피드 이미지 업로드를 위한 Presigned URL을 생성합니다. + * 여러 이미지를 업로드하려면 이 메서드를 여러 번 호출한 후, + * 모든 업로드가 완료되면 updateFeeds에 최종 URL 리스트를 전달해야 합니다. + */ PresignedUploadResponse generateFeedUploadUrl(String clubId, String fileName, String contentType);backend/src/main/java/moadong/media/service/CloudflareImageService.java (2)
168-180: 피드 개수 검증의 동시성 이슈를 인지하세요.
generateFeedUploadUrl의 피드 개수 검증(lines 174-178)은 트랜잭션 밖에서 수행되므로, 동시 호출 시 두 요청이 모두 제한 이하로 판단하여 URL을 생성할 수 있습니다.하지만 최종 검증은
updateFeeds(line 87-89)에서 트랜잭션 내에서 수행되고 Club 엔티티의@Version필드를 통한 낙관적 잠금으로 보호되므로, 실제 데이터 오염은 발생하지 않습니다. 현재 구현은 다층 방어(defense in depth) 패턴으로 볼 수 있습니다.클라이언트가
OptimisticLockingFailureException발생 시 업로드된 파일을 정리하거나 재시도하는 방법을 API 문서에 안내하는 것을 권장합니다.
219-252: 파일 크기 검증의 한계를 인지하고 있어야 합니다.Presigned URL 방식의 특성상 파일 크기 검증은 클라이언트가 업로드를 완료한 후에만 가능합니다(line 228-246). 이는 초과 크기 파일이 업로드된 후 삭제되므로 대역폭이 낭비되는 한계가 있습니다.
현재 구현은 초과 파일을 탐지하고 삭제한 후 적절한 예외를 던지므로 올바르게 처리되었습니다. 하지만 클라이언트 측에서 업로드 전에 파일 크기를 미리 검증하도록 API 문서에 권장 사항을 추가하는 것이 좋습니다.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
backend/src/main/java/moadong/media/controller/ClubImageController.java(2 hunks)backend/src/main/java/moadong/media/service/CloudflareImageService.java(4 hunks)backend/src/main/java/moadong/media/service/ClubImageService.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/src/main/java/moadong/media/controller/ClubImageController.java
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-05-19T05:45:52.957Z
Learnt from: lepitaaar
Repo: Moadong/moadong PR: 406
File: backend/src/main/java/moadong/club/service/ClubApplyService.java:34-38
Timestamp: 2025-05-19T05:45:52.957Z
Learning: The code duplication between createClubApplication and editClubApplication methods in ClubApplyService.java is acknowledged but will be addressed in a future refactoring, as per the developer's plan.
Applied to files:
backend/src/main/java/moadong/media/service/CloudflareImageService.javabackend/src/main/java/moadong/media/service/ClubImageService.java
🧬 Code graph analysis (1)
backend/src/main/java/moadong/media/service/CloudflareImageService.java (3)
backend/src/main/java/moadong/club/entity/Club.java (1)
Slf4j(28-153)backend/src/main/java/moadong/global/util/ObjectIdConverter.java (1)
ObjectIdConverter(7-17)backend/src/main/java/moadong/global/util/RandomStringUtil.java (1)
RandomStringUtil(5-22)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
🔇 Additional comments (6)
backend/src/main/java/moadong/media/service/CloudflareImageService.java (6)
77-105: 검증 로직과 트랜잭션 처리가 올바릅니다.새로운 URL 리스트에 대한 검증을 먼저 수행한 후 기존 파일을 삭제하는 순서가 적절합니다. 또한
validateFileConstraints에서 clubId와 fileType을 검증하여 다른 클럽의 파일 URL을 사용하는 것을 방지합니다.
122-147: 개선된 에러 핸들링이 적절합니다.정리(cleanup) 작업에서 예외를 던지는 대신 경고 로그를 남기고 계속 진행하는 것은 올바른 접근입니다. null/empty 체크와 S3 예외 처리가 적절하게 구현되었습니다.
188-216: 업로드 완료 메서드가 올바르게 구현되었습니다.
validateFileConstraints에서 clubId와 fileType 경로 검증을 수행하여(lines 191, 206), 다른 클럽에서 발급받은 URL을 사용하는 것을 방지합니다. 트랜잭션과 기존 파일 삭제 로직도 적절합니다.
266-303: Presigned URL 생성 로직이 올바릅니다.파일명의 잘못된 문자 처리, 10분 유효기간 설정, 필수 헤더 정보 제공이 적절하게 구현되었습니다.
requiredHeaders에 Content-Type만 포함된 것은 Presigned URL 서명에 Content-Type이 포함되어 클라이언트가 반드시 동일한 Content-Type으로 업로드해야 하기 때문에 올바른 구현입니다.
305-312: LGTM!파일명과 확장자 검증이 명확하고 올바르게 구현되었습니다.
314-319: LGTM!ClubRecruitmentInformation null 체크와 에러 로깅이 적절합니다.
lepitaaar
left a comment
lepitaaar
left a comment
There was a problem hiding this comment.
dto 검증 부분만 필수적으로 추가해주시면 좋겠습니다!
또한 complete 요청을 보내지않으면 고아파일들이 생길꺼같아 그부분도 나중에 같이 고민해보면 좋겠습니다!
| @PostConstruct | ||
| private void init() { | ||
| // viewEndpoint 정규화: 후행 슬래시 제거 | ||
| normalizedViewEndpoint = viewEndpoint != null ? viewEndpoint.replaceAll("/+$", "") : ""; |
There was a problem hiding this comment.
url의 //오류 또는 /의 누락 때문에 있는게 좋아보여서 썼습니당 굳이 필요없고 더 햇갈리기만 할까여?
| private void validateClubRecruitmentInformation(Club club) { | ||
| if (club.getClubRecruitmentInformation() == null) { | ||
| log.error("ClubRecruitmentInformation is null for club: {}", club.getId()); | ||
| throw new RestApiException(ErrorCode.CLUB_INFORMATION_NOT_FOUND); | ||
| } | ||
| } |
There was a problem hiding this comment.
검증하는 방어적 기법좋아보입니다! 근데 모집정보가 null일 경우가 존재하나요?
There was a problem hiding this comment.
실제 club엔티티가 처음 생성할때 clubRecruitmentInformation을 초기화하는걸 볼 수 있습니다!
public Club() {
this.name = "";
this.category = "";
this.division = "";
this.state = ClubState.UNAVAILABLE;
this.clubRecruitmentInformation = ClubRecruitmentInformation.builder().build();
}|
|
||
| // Presigned URL 생성 (10분 유효) | ||
| PutObjectPresignRequest presignRequest = PutObjectPresignRequest.builder() | ||
| .signatureDuration(Duration.ofMinutes(10)) |
There was a problem hiding this comment.
Duration 상수화나 프로퍼티로 빼는것도 좋아보이네요
| if (containsInvalidChars(fileName)) { | ||
| fileName = RandomStringUtil.generateRandomString(10); | ||
| } | ||
| if (file.getSize() > MAX_SIZE) { | ||
| try { | ||
| file = resizeImage(file, MAX_SIZE); | ||
| } catch (IOException e) { | ||
| throw new RestApiException(ErrorCode.FILE_TRANSFER_ERROR); | ||
| String extension = ""; | ||
| if (fileName.contains(".")) { | ||
| extension = fileName.substring(fileName.lastIndexOf(".")); | ||
| } | ||
| processedFileName = RandomStringUtil.generateRandomString(10) + extension; | ||
| } |
There was a problem hiding this comment.
왜 containsInvalidChars일때만 이름을 변경하는것일까요? 시큐어 코딩관점에선 파일을 저장할때 파일명을 난수로 저장하는것이 안전해서 무조건 파일이름을 변경하는게 좋을꺼같네요
| // Presigned URL 방식 엔드포인트 | ||
| @PostMapping("/{clubId}/logo/upload-url") | ||
| @Operation(summary = "로고 이미지 업로드 URL 생성", description = "로고 이미지 업로드를 위한 Presigned URL을 생성합니다.") | ||
| public ResponseEntity<?> generateLogoUploadUrl(@PathVariable String clubId, |
There was a problem hiding this comment.
현재 이미지 업로드 URL 생성이 단일 파일이름만 받아 업로드를 구현하는 것 같은데, 다중업로드를 위해 한개의 file이름을 받는것보다 리스트형태로 받을 수 있으면 좋지않을까요?
backend/src/main/java/moadong/media/service/CloudflareImageService.java
Outdated
Show resolved
Hide resolved
backend/src/main/java/moadong/media/controller/ClubImageController.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
추후에 다중업로드가 지원되는 피드만큼은 정상/예외 케이스를 검증하는 단위테스트 작성해보는 것도 좋을 거 같네용
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/src/main/java/moadong/media/controller/ClubImageController.java (1)
27-28: 클럽 이미지 작업에 대한 소유권/관리자 권한 검증이 완전히 누락되어 있습니다.검증 결과, 서비스 레이어(CloudflareImageService)의 모든 이미지 작업 메서드들이
clubId파라미터만 받고 현재 사용자 정보를 포함하지 않습니다. 결과적으로:
deleteLogo(),deleteCover(),updateFeeds(),completeLogoUpload(),completeCoverUpload()등 모든 메서드에서 소유권 검증이 없습니다- 인증된 모든 사용자가 다른 클럽의 이미지를 삭제/수정할 수 있습니다 (예: 사용자 A가
/api/club/{clubB_id}/logoDELETE 호출 시 실행됨)서비스 메서드는 현재 사용자 컨텍스트에 접근할 수 없으므로, 컨트롤러 레벨에서 명시적으로 현재 사용자를 받아 소유권을 검증한 후 서비스를 호출하거나,
@PreAuthorize에 커스텀 SpEL 표현식으로 역할/소유권 검증을 추가해야 합니다.
🧹 Nitpick comments (4)
backend/src/main/java/moadong/media/controller/ClubImageController.java (1)
83-83: 더 이상 사용하지 않는 API에 대한 주석을 정리하세요.주석에 "feed complete API는 더 이상 사용하지 않습니다"라고 명시되어 있습니다. 대신
updateFeeds에서 검증한다고 하는데, 이는 워크플로우를 이해하는 데 도움이 됩니다.더 명확하게 하려면:
- 왜 별도의 complete API가 필요 없는지 (피드는 여러 개를 한 번에 업데이트하므로)
updateFeeds엔드포인트와의 관계를 주석에 추가하는 것을 권장합니다.
backend/src/main/java/moadong/media/service/CloudflareImageService.java (3)
173-203: 배치 URL 생성 로직의 에러 처리 전략을 명확히 하세요.현재 구현에서 일부 요청이 실패해도 성공한 요청의 URL은 반환하고, 초과된 요청에 대해서는 에러 응답을 추가합니다. 이는 부분 성공(partial success) 패턴입니다.
고려 사항:
- 클라이언트가
success=false인 응답을 어떻게 처리해야 하는지 API 문서에 명시 필요- 일부 URL 생성 실패 시 클라이언트의 재시도 전략 고려
limit계산 후 나머지 요청에 대해 단일 에러 응답을 추가하는 방식이 적절한지 검토 (각 요청별로 개별 에러를 반환하는 것이 더 명확할 수 있음)
82-110: 동시성 제어에 대한 문서화를 개선하세요.과거 리뷰에서 "트랜잭션 내에서 최신 데이터를 다시 조회하여 Race Condition 방지" 주석에 대한 지적이 있었습니다. 실제 동시성 보호는
Club엔티티의@Version필드를 통한 낙관적 잠금으로 이루어집니다.현재 코드에는 관련 주석이 없는데, 다음을 고려하세요:
@Version낙관적 잠금이 동시 수정을 방어한다는 점을 메서드 주석에 명시OptimisticLockingFailureException발생 시 클라이언트 처리 방법 문서화- 클라이언트가 동시 수정 실패 시 업로드된 파일 정리 필요성 안내
Based on past review comments
336-341: Null 체크는 방어적 코딩이지만 실제로는 불필요합니다.과거 리뷰 코멘트에서 지적된 바와 같이,
Club엔티티의 생성자가 항상clubRecruitmentInformation을 초기화하므로 이 필드가 null일 수 없습니다.이 검증은 방어적 코딩으로 남겨두어도 무방하지만, 실제로 도달 불가능한 코드임을 인지하고 있어야 합니다. 로그만 남기고 예외를 던지지 않거나, 주석으로 "방어적 검증"임을 명시하는 것을 고려하세요.
Based on learnings
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
backend/src/main/java/moadong/media/controller/ClubImageController.java(2 hunks)backend/src/main/java/moadong/media/dto/FeedUpdateRequest.java(1 hunks)backend/src/main/java/moadong/media/dto/PresignedUploadResponse.java(1 hunks)backend/src/main/java/moadong/media/dto/UploadCompleteRequest.java(1 hunks)backend/src/main/java/moadong/media/dto/UploadUrlRequest.java(1 hunks)backend/src/main/java/moadong/media/service/CloudflareImageService.java(4 hunks)backend/src/main/java/moadong/media/service/ClubImageService.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/src/main/java/moadong/media/service/ClubImageService.java
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-05-19T05:45:52.957Z
Learnt from: lepitaaar
Repo: Moadong/moadong PR: 406
File: backend/src/main/java/moadong/club/service/ClubApplyService.java:34-38
Timestamp: 2025-05-19T05:45:52.957Z
Learning: The code duplication between createClubApplication and editClubApplication methods in ClubApplyService.java is acknowledged but will be addressed in a future refactoring, as per the developer's plan.
Applied to files:
backend/src/main/java/moadong/media/service/CloudflareImageService.java
📚 Learning: 2025-03-19T05:18:07.818Z
Learnt from: seongwon030
Repo: Moadong/moadong PR: 195
File: frontend/src/pages/AdminPage/AdminPage.tsx:7-7
Timestamp: 2025-03-19T05:18:07.818Z
Learning: AdminPage.tsx에서 현재 하드코딩된 클럽 ID('67d2e3b9b15c136c6acbf20b')는 로그인 기능 구현 후 동적으로 가져오는 방식으로 수정될 예정입니다.
Applied to files:
backend/src/main/java/moadong/media/service/CloudflareImageService.java
🧬 Code graph analysis (2)
backend/src/main/java/moadong/media/controller/ClubImageController.java (1)
backend/src/main/java/moadong/user/controller/UserController.java (1)
RestController(34-120)
backend/src/main/java/moadong/media/service/CloudflareImageService.java (4)
backend/src/main/java/moadong/club/entity/Club.java (1)
Slf4j(28-153)backend/src/main/java/moadong/global/util/ObjectIdConverter.java (1)
ObjectIdConverter(7-17)backend/src/main/java/moadong/global/util/RandomStringUtil.java (1)
RandomStringUtil(5-22)frontend/src/apis/updateFeedImages.ts (1)
updateFeedImages(4-23)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
🔇 Additional comments (12)
backend/src/main/java/moadong/media/dto/FeedUpdateRequest.java (1)
3-9: 유효성 검증 추가가 적절합니다.
@NotNull제약 조건 추가로 feeds 필드의 null 안전성이 보장되며, 컨트롤러의@Valid어노테이션과 잘 연동됩니다.backend/src/main/java/moadong/media/dto/UploadCompleteRequest.java (1)
1-9: 업로드 완료 DTO 구현이 적절합니다.
@NotBlank제약 조건으로 fileUrl의 유효성을 보장하며, presigned URL 워크플로우의 완료 단계에 적합한 간결한 구조입니다.backend/src/main/java/moadong/media/dto/PresignedUploadResponse.java (1)
5-11: 응답 구조가 성공/실패 케이스를 모두 처리합니다.성공 시
presignedUrl,finalUrl,requiredHeaders를, 실패 시success=false와failureReason을 반환하는 구조가 명확합니다. 클라이언트가success플래그로 분기 처리할 수 있어 사용성이 좋습니다.backend/src/main/java/moadong/media/controller/ClubImageController.java (1)
75-81: 다중 피드 업로드를 위한 배치 엔드포인트가 잘 구현되었습니다.
List<UploadUrlRequest>를 받아 여러 파일의 업로드 URL을 한 번에 생성하는 방식이 효율적입니다. 과거 리뷰 코멘트에서 제안된 다중 업로드 지원이 적절히 반영되었습니다.backend/src/main/java/moadong/media/service/CloudflareImageService.java (7)
60-67: viewEndpoint 검증이 적절히 구현되었습니다.과거 리뷰 코멘트에서 제기된 viewEndpoint null 처리 문제가 해결되었습니다.
@PostConstruct에서 필수 설정값을 명시적으로 검증하고, 누락 시IllegalStateException을 발생시켜 애플리케이션 시작 시점에 설정 오류를 조기 발견할 수 있습니다.Based on past review comments
289-294: 파일명 무작위화로 보안이 강화되었습니다.
RandomStringUtil.generateRandomString(10)을 사용해 파일명을 무작위로 생성하는 것은 시큐어 코딩 관점에서 적절합니다. 원본 파일명에 포함될 수 있는 특수문자나 경로 탐색 공격을 방지합니다.Based on past review comments
211-224: 트랜잭션 처리와 파일 정리가 적절합니다.업로드 완료 메서드가 다음을 올바르게 수행합니다:
validateFileConstraints로 파일 유효성 검증 (clubId 및 파일 타입 경로 포함)- 기존 파일 삭제로 스토리지 누수 방지
@Transactional보장으로 DB 일관성 유지과거 리뷰 코멘트에서 제기된 클럽별 파일 경로 검증 문제가
validateFileConstraints에서 해결되었습니다.Based on past review comments
242-275: 파일 제약 검증이 포괄적으로 구현되었습니다.검증 로직이 다음을 모두 확인합니다:
- URL 길이 제한
- clubId와 fileType 경로 검증 (무단 접근 방지)
- R2에서 실제 파일 존재 및 크기 확인
- 크기 초과 시 자동 삭제
과거 리뷰 코멘트에서 제기된 클럽별 경로 검증 이슈가 해결되었습니다 (Line 247).
성능 고려사항:
HeadObjectRequest는 업로드 완료 단계에서 추가 네트워크 호출을 발생시킵니다. 현재는 보안과 무결성을 우선하는 트레이드오프로 보이나, 대량 업로드 시 지연이 발생할 수 있습니다. 필요 시 비동기 처리나 배치 검증을 고려하세요.Based on past review comments
126-152: 파일 삭제 메서드의 에러 처리가 견고합니다.null/빈 경로, 잘못된 형식, S3 예외를 모두 graceful하게 처리하고 로그를 남깁니다. 삭제 실패가 전체 트랜잭션을 롤백하지 않도록 예외를 잡아서 처리하는 것이 적절합니다.
이는 파일이 이미 삭제되었거나 권한 문제가 있어도 DB 상태 업데이트는 계속 진행되도록 하여 시스템의 복원력을 높입니다.
53-58: 설정값이 적절히 외부화되었습니다.과거 리뷰 코멘트에서 제안된 Duration 상수화/프로퍼티 외부화가
@Value어노테이션을 통해 구현되었습니다. 기본값도 제공되어 있어 설정 누락 시 안전합니다.Based on past review comments
317-319: Review comment is incorrect — R2 presigned URLs only require headers that were signed.R2 presigned URLs require only the exact headers that were included when the URL was signed; if only Content-Type was specified during signing, no additional headers like x-amz-* are required.
The code shows
PutObjectRequest.builder().contentType(contentType)— meaning only Content-Type is signed. The currentrequiredHeadersimplementation correctly returns only Content-Type, which matches what was signed. No additional headers are needed.The review's assumption that x-amz-* headers are required appears to be based on AWS S3 conventions, which differ from Cloudflare R2's presigned URL behavior.
Likely an incorrect or invalid review comment.
backend/src/main/java/moadong/media/dto/UploadUrlRequest.java (1)
10-11: 정규식 패턴 수정이 필요합니다.image/jpg는 표준이 아닙니다.표준 MIME 타입은
image/jpeg이며image/jpg는 존재하지 않습니다. 코드에서 검증된contentType이 그대로 S3/Cloudflare API로 전달되므로, 일부 검증 프로세스는image/jpeg만 명시적으로 확인하여image/jpg를 거부할 수 있습니다.정규식에서
image/jpg를 제거하고image/jpeg만 허용하거나, 클라이언트가image/jpg를 보내는 경우 백엔드에서image/jpeg으로 정규화하는 방식을 고려하세요.
#️⃣연관된 이슈
📝작업 내용
트러블슈팅
Url에 사진을 올릴때 계속 SignatureDoesNotMatch가 떴습니다. 이유는 클라이언트가 서버에 보낸 요청(쿼리·헤더·바디)이 presigned URL을 만들 때 사용된 정확한 문자열과 달라서 발생합니다. 따라서 Bearer토큰을 빼고 요청을 보내서 성공했습니다.
또한 jpg타입같은 경우에는 따로 Content-type에 image/jpg라고 명시를 해줘야 오류가 안나타났습니다.
중점적으로 리뷰받고 싶은 부분(선택)
논의하고 싶은 부분(선택)
🫡 참고사항
Summary by CodeRabbit
새로운 기능
개선
버그 수정
테스트
기타(설정)