[LNK-33] 이미지 리사이징 로직 수정 #78
There are no files selected for viewing
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
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| package leets.leenk.domain.media.application.exception; | ||
|
|
||
| import leets.leenk.global.common.exception.BaseException; | ||
|
|
||
| public class S3CopyException extends BaseException { | ||
| public S3CopyException() { | ||
| super(ErrorCode.S3_COPY_FAILED); | ||
| } | ||
| } |
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,37 @@ | ||
| package leets.leenk.domain.media.domain.service; | ||
|
|
||
| import lombok.RequiredArgsConstructor; | ||
| import org.springframework.stereotype.Service; | ||
|
|
||
| import java.net.URI; | ||
| import java.util.List; | ||
|
|
||
| @Service | ||
| @RequiredArgsConstructor | ||
| public class MediaS3Service { | ||
|
|
||
| private final S3PresignedUrlService s3PresignedUrlService; | ||
|
|
||
| public String moveToOriginals(String tempUrl) { | ||
| if (tempUrl == null || tempUrl.isBlank()) { | ||
| return tempUrl; | ||
| } | ||
|
|
||
| if (tempUrl.contains("/originals/")) { | ||
| return tempUrl; | ||
| } | ||
|
|
||
| String tempKey = extractKeyFromUrl(tempUrl); | ||
| String originalsKey = tempKey.replace("temp/", "originals/"); | ||
|
|
||
| s3PresignedUrlService.copyObject(tempKey, originalsKey); | ||
| s3PresignedUrlService.deleteObject(tempKey); | ||
|
|
||
| return s3PresignedUrlService.getUrl(originalsKey); | ||
| } | ||
|
|
||
| private String extractKeyFromUrl(String url) { | ||
| URI uri = URI.create(url); | ||
| return uri.getPath().substring(1); | ||
| } | ||
|
Comment on lines
+15
to
+36
There was a problem hiding this comment. 🧩 Analysis chain
현재 구현은 아래처럼 경로가 - private String extractKeyFromUrl(String url) {
- URI uri = URI.create(url);
- return uri.getPath().substring(1);
- }
+ private String extractKeyFromUrl(String url) {
+ URI uri = URI.create(url);
+ String path = uri.getPath();
+
+ if (path == null || path.isBlank()) {
+ return path;
+ }
+
+ return path.startsWith("/") ? path.substring(1) : path;
+ }현재 호출부들은 모두 정규화된 URL을 넘길 것으로 보이지만, 향후 유지보수나 재사용성을 고려하면 이 정도 방어는 해 두는 편이 좋겠습니다. Length of output: 1312 🏁 Script executed: #!/bin/bash
# Find test files for MediaS3Service
fd -e java -p '**/test/**' -x rg -l 'MediaS3Service|moveToOriginals' {}Length of output: 289 🏁 Script executed: #!/bin/bash
# Check the S3PresignedUrlService to understand URL format it returns
rg -nP -A 10 'getUrl\s*\(' --type=java | head -50Length of output: 3664 🏁 Script executed: #!/bin/bash
# Look for any URL creation or S3 URL construction in the codebase
rg -nP 's3.*url|url.*s3' -i --type=java | head -30Length of output: 3222 Length of output: 1884 🏁 Script executed: #!/bin/bash
# Check UserUpdateRequest to see profileImage field definition
rg -nP 'profileImage' --type=java -B 2 -A 2 | head -40Length of output: 3227 🏁 Script executed: #!/bin/bash
# Find any test files for media or S3 related functionality
fd '\.java$' -path '*/test*' -type f | xargs rg -l 'moveToOriginals|extractKeyFromUrl' 2>/dev/nullLength of output: 550 🏁 Script executed: #!/bin/bash
# Check if there's any URL validation logic before calling moveToOriginals
rg -nP -B 5 'moveToOriginals\(' --type=java | head -60Length of output: 5897 Length of output: 424 🏁 Script executed: #!/bin/bash
# Check if URI.getPath() can return null and what substring(1) would do
cat > /tmp/test_uri.java << 'EOF'
import java.net.URI;
public class TestURI {
public static void main(String[] args) {
// Test case 1: absolute URL (S3)
String url1 = "https://bucket.s3.amazonaws.com/temp/file123";
URI uri1 = URI.create(url1);
String path1 = uri1.getPath();
System.out.println("URL: " + url1);
System.out.println("Path: " + path1);
System.out.println("After substring(1): " + (path1 != null ? path1.substring(1) : "NULL"));
System.out.println();
// Test case 2: relative path (potential bug)
String url2 = "temp/file123";
URI uri2 = URI.create(url2);
String path2 = uri2.getPath();
System.out.println("URL: " + url2);
System.out.println("Path: " + path2);
System.out.println("After substring(1): " + (path2 != null && path2.length() > 0 ? path2.substring(1) : "INVALID"));
System.out.println();
// Test case 3: just filename
String url3 = "file123";
URI uri3 = URI.create(url3);
String path3 = uri3.getPath();
System.out.println("URL: " + url3);
System.out.println("Path: " + path3);
System.out.println("After substring(1): " + (path3 != null && path3.length() > 0 ? path3.substring(1) : "INVALID"));
}
}
EOF
cd /tmp && javac test_uri.java && java TestURILength of output: 44
현재 구현은
특히 - private String extractKeyFromUrl(String url) {
- URI uri = URI.create(url);
- return uri.getPath().substring(1);
- }
+ private String extractKeyFromUrl(String url) {
+ URI uri = URI.create(url);
+ String path = uri.getPath();
+
+ if (path == null || path.isBlank()) {
+ return path;
+ }
+
+ return path.startsWith("/") ? path.substring(1) : path;
+ }현재 호출부에서 정규화된 S3 URL을 전달하지만, 향후 재사용성과 입력 안정성을 위해 방어 코드 추가를 권장합니다. 🤖 Prompt for AI Agents |
||
| } | ||
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
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
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
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
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
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.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
S3 작업의 에러 처리와 트랜잭션 일관성을 보장해야 합니다.
현재 구현에는 다음과 같은 문제가 있습니다:
moveToOriginals나updateMediaUrl이 실패해도 예외가 처리되지 않아 부분 실패 시 일관성이 깨질 수 있습니다.다음 개선 방안을 고려하세요:
medias.forEach(media -> { - String newMediaUrl = mediaS3Service.moveToOriginals(media.getMediaUrl()); - mediaUpdateService.updateMediaUrl(media, newMediaUrl); + try { + String newMediaUrl = mediaS3Service.moveToOriginals(media.getMediaUrl()); + mediaUpdateService.updateMediaUrl(media, newMediaUrl); + } catch (Exception e) { + // 실패 시 이미 처리된 미디어들도 롤백하거나, 전체 작업 실패 처리 + throw new MediaProcessingException("미디어 처리 중 오류 발생", e); + } });추가로, S3 작업 실패 시 이미 복사된 파일들을 정리하는 보상 트랜잭션(compensating transaction) 로직도 고려해보세요.
🤖 Prompt for AI Agents
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
copy 작업 실패 시 예외 처리가 되고 Transaction어노테이션으로 전체 트랜잭션이 롤백되어서 일관성이 보장 돼
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@soo0711, 말씀하신 부분이 부분적으로 맞지만, 중요한 분산 트랜잭션 문제가 여전히 존재합니다.
@transactional의 한계:
@Transactional어노테이션은 데이터베이스 트랜잭션만 제어합니다. S3와 같은 외부 시스템 작업은 Spring의 트랜잭션 관리 범위 밖에 있어서 롤백할 수 없습니다.문제 시나리오:
위 경우:
개선 방안:
현재 구현에서
S3CopyException으로 인한 즉각적인 실패는 잘 처리되지만, S3 성공 후 DB 실패 시나리오에 대한 정리 로직이 필요합니다. 이 부분을 어떻게 처리할 계획이신가요?🧠 Learnings used
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
originals에 고아 파일이 남는 건 맞지만 서비스 동작에 영향 없을 것 같고 DB 커밋 실패는 매우 드물게 발생하기 때문에 보상 트랜잭션 구현 복잡도가 얻는 실익보다 크다고 생각해
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@soo0711, 합리적인 판단이라고 생각합니다. 말씀하신 대로 DB 커밋 실패는 매우 드물고, 고아 파일이 서비스 동작에는 영향을 주지 않으니 보상 트랜잭션의 복잡도가 실익을 초과할 수 있겠네요.
다만 장기적으로 고아 파일이 누적될 경우를 대비해, 필요하다면 추후 S3 라이프사이클 정책이나 주기적인 정리 스크립트로 처리하는 것도 고려해볼 수 있을 것 같습니다. 현재 구현은 이대로 괜찮아 보입니다! 👍
🧠 Learnings used