refactor: 프로필 이미지 업로드 제한 10MB 변경 및 소셜 로그인 프로필 수정 오류 수정#138
Merged
Conversation
Contributor
WalkthroughS3 이미지 삭제 시 URL 접두어 검증을 추가하여 외부 URL 처리를 방지하고, 멀티파트 파일 업로드 크기 제한을 5MB에서 10MB로 상향 조정했습니다. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~5 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/main/java/com/ongil/backend/global/config/s3/S3ImageService.java (1)
121-128:extractKey의 prefix 검증이deleteguard와 중복됨Line 77에서 이미
startsWith(generatePrefix())검증을 수행한 뒤extractKey를 호출하므로, Line 123의 동일한 검증은 사실상 도달 불가능한 코드가 되었습니다.generatePrefix()가 매번 문자열을 생성하므로 불필요한 호출이기도 합니다. prefix를 지역 변수로 추출하거나,extractKey내부 검증을 제거하는 것을 고려해 보세요.♻️ 리팩토링 제안: prefix를 한 번만 생성
public void delete(String imageUrl) { - if (!imageUrl.startsWith(generatePrefix())) { + String prefix = generatePrefix(); + if (imageUrl == null || !imageUrl.startsWith(prefix)) { log.info("S3 URL이 아니므로 삭제 생략: {}", imageUrl); return; } - String key = extractKey(imageUrl); + String key = imageUrl.substring(prefix.length());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/ongil/backend/global/config/s3/S3ImageService.java` around lines 121 - 128, The extractKey method duplicates a prefix check already done by the delete guard and repeatedly calls generatePrefix(); fix by removing the redundant startsWith validation inside extractKey and rely on the caller's check, or better, change extractKey(String imageUrl, String prefix) (or have the caller compute and pass a cached prefix) so generatePrefix() is invoked only once and the method simply returns imageUrl.substring(prefix.length()); update callers (e.g., the delete flow that currently calls extractKey and performs the startsWith check) to pass the computed prefix and keep error handling where the initial guard detects unexpected URLs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/com/ongil/backend/global/config/s3/S3ImageService.java`:
- Around line 76-80: The delete(String imageUrl) method should defensively
handle null and avoid duplicating the S3-prefix check already present in
extractKey(String imageUrl): add a null guard at the top of delete (e.g.,
Objects.requireNonNull(imageUrl, "imageUrl must not be null") or an explicit
null-check that logs and returns) and remove the startsWith(generatePrefix())
check from delete so that validation of the URL format is centralized in
extractKey(String imageUrl) (ensure extractKey throws a clear
IllegalArgumentException or logs when the URL is not an S3 URL).
---
Nitpick comments:
In `@src/main/java/com/ongil/backend/global/config/s3/S3ImageService.java`:
- Around line 121-128: The extractKey method duplicates a prefix check already
done by the delete guard and repeatedly calls generatePrefix(); fix by removing
the redundant startsWith validation inside extractKey and rely on the caller's
check, or better, change extractKey(String imageUrl, String prefix) (or have the
caller compute and pass a cached prefix) so generatePrefix() is invoked only
once and the method simply returns imageUrl.substring(prefix.length()); update
callers (e.g., the delete flow that currently calls extractKey and performs the
startsWith check) to pass the computed prefix and keep error handling where the
initial guard detects unexpected URLs.
Comment on lines
76
to
+80
| public void delete(String imageUrl) { | ||
| if (!imageUrl.startsWith(generatePrefix())) { | ||
| log.info("S3 URL이 아니므로 삭제 생략: {}", imageUrl); | ||
| return; | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "S3ImageService.java" -type fRepository: IT-Cotato/12th-OnGil-BE
Length of output: 137
🏁 Script executed:
# S3ImageService.java 파일의 delete() 메서드와 관련 메서드 확인
cat -n src/main/java/com/ongil/backend/global/config/s3/S3ImageService.java | head -150Repository: IT-Cotato/12th-OnGil-BE
Length of output: 4969
🏁 Script executed:
# delete() 메서드 호출부 검색 (테스트 제외)
rg -n 'delete\(' src/main/java --type=java -A 2 | grep -v testRepository: IT-Cotato/12th-OnGil-BE
Length of output: 4447
🏁 Script executed:
sed -n '50,75p' src/main/java/com/ongil/backend/domain/user/service/UserService.java | cat -nRepository: IT-Cotato/12th-OnGil-BE
Length of output: 811
🏁 Script executed:
# delete 메서드를 호출하는 전체 메서드 확인 (각 호출 전후 문맥)
sed -n '50,80p' src/main/java/com/ongil/backend/domain/user/service/UserService.java | cat -nRepository: IT-Cotato/12th-OnGil-BE
Length of output: 1024
imageUrl 파라미터 방어 로직 추가 권장
현재 UserService의 호출 지점에서는 delete() 호출 전에 null 체크가 되어 있으나, 메서드가 public이고 파라미터에 null 안전성 선언이 없어 향후 호출 시 실수로 null이 전달될 가능성이 있습니다. 예외 처리 관점에서 메서드 입구에서 명시적으로 null을 검증하는 것이 좋은 방식입니다.
또한 Line 77의 startsWith() 검사는 Line 123 extractKey()에서도 동일하게 수행되므로, 중복을 제거하고 책임을 명확히 하기 위해 extractKey()만 검증하도록 정리할 수 있습니다.
수정 제안
public void delete(String imageUrl) {
+ if (imageUrl == null || !imageUrl.startsWith(generatePrefix())) {
- if (!imageUrl.startsWith(generatePrefix())) {
log.info("S3 URL이 아니므로 삭제 생략: {}", imageUrl);
return;
}또는 Objects.requireNonNull() 사용:
+ Objects.requireNonNull(imageUrl, "imageUrl must not be null");
if (!imageUrl.startsWith(generatePrefix())) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/java/com/ongil/backend/global/config/s3/S3ImageService.java` around
lines 76 - 80, The delete(String imageUrl) method should defensively handle null
and avoid duplicating the S3-prefix check already present in extractKey(String
imageUrl): add a null guard at the top of delete (e.g.,
Objects.requireNonNull(imageUrl, "imageUrl must not be null") or an explicit
null-check that logs and returns) and remove the startsWith(generatePrefix())
check from delete so that validation of the URL format is centralized in
extractKey(String imageUrl) (ensure extractKey throws a clear
IllegalArgumentException or logs when the URL is not an S3 URL).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🔍️ 작업 내용
refactor: 프로필 이미지 업로드 제한 10MB 변경 및 소셜 로그인 프로필 수정 오류 수정
✨ 상세 설명
🛠️ 추후 리팩토링 및 고도화 계획
📸 스크린샷 (선택)
💬 리뷰 요구사항
Summary by CodeRabbit
릴리스 노트
버그 수정
개선 사항