fix: 프리사인드 url 조회 api eventId 검증 변경 - #185 #186
Conversation
📝 WalkthroughSummary by CodeRabbit릴리스 노트
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughS3Service.getS3PreSignedUrls 메서드에서 이벤트 존재 여부를 확인하는 validateExistEvent(eventId) 호출을 주석 처리하였으며, 나머지 미디어 정보 매핑 및 응답 생성 로직은 변경 없이 유지됩니다. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3분
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 2 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/main/java/com/permitseoul/permitserver/domain/admin/util/aws/S3Service.java (2)
47-48: 도달할 수 없는 예외 처리 블록입니다.
AdminEventNotFoundException은validateExistEvent메서드에서만 발생하는데, 해당 메서드 호출이 주석 처리되어 이 예외가 더 이상 발생하지 않습니다. 따라서 이 catch 블록은 실행될 수 없는 코드입니다.검증 로직을 복원하지 않는다면, 이 예외 처리 블록도 함께 제거해야 합니다.
54-56: Remove unusedvalidateExistEventmethod andAdminEventRetrieverdependency.The
validateExistEventmethod is dead code—its only call at line 36 is commented out and it's not invoked anywhere else in the codebase. Since this private method is the sole reason for injectingAdminEventRetrieverin S3Service, both the method and the dependency should be removed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/java/com/permitseoul/permitserver/domain/admin/util/aws/S3Service.java(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: sjk4618
Repo: PERMIT-SEOUL/permit-server PR: 48
File: src/main/java/com/permitseoul/permitserver/domain/guest/core/domain/entity/GuestEntity.java:31-37
Timestamp: 2025-07-15T09:37:32.765Z
Learning: sjk4618 prefers to implement factory methods or public constructors for entities when they are actually needed, rather than creating them proactively.
| final List<S3PreSignedUrlRequest.MediaInfoRequest> mediaInfoRequests) { | ||
| try { | ||
| validateExistEvent(eventId); | ||
| // validateExistEvent(eventId); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the file exists and get relevant sections
wc -l src/main/java/com/permitseoul/permitserver/domain/admin/util/aws/S3Service.javaRepository: PERMIT-SEOUL/permit-server
Length of output: 152
🏁 Script executed:
cat -n src/main/java/com/permitseoul/permitserver/domain/admin/util/aws/S3Service.javaRepository: PERMIT-SEOUL/permit-server
Length of output: 4642
🏁 Script executed:
# Check the controller or API layer that calls this service
echo "=== Admin API or Controller layer ==="
fd -e java -path "*/admin/*api*" -o -path "*/admin/*controller*"Repository: PERMIT-SEOUL/permit-server
Length of output: 293
🏁 Script executed:
# Search more broadly for where S3Service is used
rg -n 'S3Service|getS3PreSignedUrls' --type javaRepository: PERMIT-SEOUL/permit-server
Length of output: 874
🏁 Script executed:
# Search for eventId validation in the admin domain
rg -n 'validateEventExist|validateExist.*event|eventId.*valid' --type java -B 3 -A 3 | head -100Repository: PERMIT-SEOUL/permit-server
Length of output: 5017
🏁 Script executed:
# Also check if there's an aspect or interceptor that validates eventId
echo "=== Search for Aspect or Interceptor related to admin eventId ==="
rg -n '@Aspect|@Component|Interceptor' --type java src/main/java/com/permitseoul/permitserver/domain/admin/ -A 5 | head -80Repository: PERMIT-SEOUL/permit-server
Length of output: 11789
🏁 Script executed:
head -60 src/main/java/com/permitseoul/permitserver/domain/admin/base/api/controller/AdminController.javaRepository: PERMIT-SEOUL/permit-server
Length of output: 1904
Remove commented-out eventId validation or provide clear justification for its removal.
Commenting out validateExistEvent(eventId) on line 36 creates critical issues:
- Pre-signed URLs are generated for non-existent events, leading to orphaned S3 objects disconnected from the database
- Arbitrary eventId values can be used to upload files without validation
- Data integrity between the database and S3 is compromised
- The
validateExistEventmethod (lines 54-56) becomes unused dead code - The
AdminEventNotFoundExceptioncatch block (line 47) becomes unreachable
If validation has been moved elsewhere in the call chain (controller, aspect, or interceptor), explicitly document that. Otherwise, restore the validation call. The PR title indicates "검증 변경" (validation change) rather than removal—please clarify the intent.
🤖 Prompt for AI Agents
In
src/main/java/com/permitseoul/permitserver/domain/admin/util/aws/S3Service.java
around line 36, the call to validateExistEvent(eventId) has been commented out
causing missing event existence validation and dead code/unreachable exception
handling; restore the validateExistEvent(eventId) call at that location to
re-enable event existence checks (and remove or repurpose the now-unnecessary
AdminEventNotFoundException catch if it becomes redundant), or if validation was
intentionally moved elsewhere, add a clear comment and Javadoc noting where and
why validation occurs (including a link to the controller/aspect/interceptor)
and remove the unused validateExistEvent method and unreachable catch block to
keep code clean.
🔥Pull requests
⛳️ 작업한 브랜치
👷 작업한 내용
🚨 참고 사항