[FEATURE] 디자이너 1차 시안 및 수정본 제출 시 워터마크 로직 기능 추가#114
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthrough디자이너가 초안 또는 수정본을 제출하면 이벤트가 발행되고, 커밋 후 비동기로 원본 파일을 다운로드해 워터마크 이미지를 생성·업로드한 뒤 파일 상태를 완료 또는 실패로 전환하는 파이프라인이 추가되었습니다. Changes워터마크 처리 파이프라인
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant DesignerDraftFacade
participant EventPublisher
participant DraftWatermarkListener
participant DraftWatermarkService
participant S3FileManager
participant WatermarkImageProcessor
participant DraftWatermarkTransitionService
DesignerDraftFacade->>EventPublisher: DraftFilesSubmittedEvent(draftId)
EventPublisher->>DraftWatermarkListener: AFTER_COMMIT event
DraftWatermarkListener->>DraftWatermarkService: watermarkDraftFiles(draftId)
DraftWatermarkService->>S3FileManager: download(originalKey)
DraftWatermarkService->>WatermarkImageProcessor: createWatermarkedPreview(stream)
WatermarkImageProcessor-->>DraftWatermarkService: PNG bytes
DraftWatermarkService->>S3FileManager: upload(wmKey, bytes, PNG)
DraftWatermarkService->>DraftWatermarkTransitionService: complete(fileId, wmKey)
DraftWatermarkService->>DraftWatermarkTransitionService: fail(fileId)
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
src/test/java/ditda/backend/global/image/WatermarkImageProcessorTest.java (1)
72-108: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value하드코딩된 개인 경로를 설정 가능한 값으로 변경하는 것을 권장합니다.
@Disabled테스트이지만/Users/jong/Desktop/watermark-test경로가 소스에 남아있는 것은 바람직하지 않습니다. 시스템 프로퍼티나 환경 변수를 통해 경로를 주입받도록 변경하면, 팀 멤버가各自 로컬 환경에서 쉽게 실행할 수 있습니다.♻️ 제안: 시스템 프로퍼티 기반 경로 주입
`@Disabled`("로컬 확인 및 처리 시간 측정용 - 시스템 프로퍼티 'watermark.test.inputDir'를 설정하고 `@Disabled를` 지운 뒤 실행") `@Test` void manualPreviewForEyeCheck() throws IOException { // 입력: 시스템 프로퍼티로 폴더 경로 지정 - Path inputDir = Path.of("/Users/jong/Desktop/watermark-test"); + String inputDirPath = System.getProperty("watermark.test.inputDir"); + if (inputDirPath == null) { + throw new IllegalStateException("시스템 프로퍼티 'watermark.test.inputDir'를 설정하세요."); + } + Path inputDir = Path.of(inputDirPath); Path outputDir = inputDir.resolve("out"); Files.createDirectories(outputDir);🤖 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/ditda/backend/global/image/WatermarkImageProcessorTest.java` around lines 72 - 108, The manual preview test currently hardcodes a personal local path in manualPreviewForEyeCheck, which makes it non-portable. Update this test to read the input directory from a configurable source such as a system property or environment variable, with a sensible fallback or clear failure if unset. Keep the rest of the flow in WatermarkImageProcessorTest the same, but replace the fixed Path.of(...) usage so each developer can run it in their own environment.src/main/java/ditda/backend/global/image/WatermarkImageProcessor.java (1)
51-62: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
ImageIO.read반환값 null 체크가 필요합니다.
ImageIO.read는 손상된 이미지에 대해 예외 대신null을 반환할 수 있습니다. 이 경우logo필드가null이 되어drawWatermark에서 NPE가 발생하며, 모든 워터마크 처리가FAILED로 떨어집니다.🛡️ 제안 수정
- return ImageIO.read(logoStream); + BufferedImage image = ImageIO.read(logoStream); + if (image == null) { + throw new IllegalStateException("워터마크 로고 이미지를 읽을 수 없습니다: " + LOGO_PATH); + } + return image;🤖 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/ditda/backend/global/image/WatermarkImageProcessor.java` around lines 51 - 62, `WatermarkImageProcessor.loadLogo()` only handles IOException, but `ImageIO.read(logoStream)` can return null for an unreadable or corrupted logo and later cause `drawWatermark` to fail with an NPE. Update `loadLogo()` to explicitly check the return value from `ImageIO.read` and throw an `IllegalStateException` with the existing LOGO_PATH context when it is null, so the `logo` field is never initialized with a null image.
🤖 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/ditda/backend/domain/commission/draft/service/DraftWatermarkService.java`:
- Around line 59-64: In DraftWatermarkService, the watermark S3 key should be
normalized to a .png filename instead of preserving the original extension,
since WatermarkImageProcessor always produces PNG and the upload uses
S3ContentType.PNG. Update the key-building logic around originalKey, lastSlash,
and watermarkedKey so the result is commission/draft/wm/{uuid}.png even when the
source is .jpg. Also handle the no-slash case safely before substring to avoid
StringIndexOutOfBoundsException.
In `@src/main/java/ditda/backend/global/config/WatermarkAsyncConfig.java`:
- Around line 12-20: The watermarkExecutor bean in WatermarkAsyncConfig needs
graceful shutdown and bounded queue settings. Update the ThreadPoolTaskExecutor
setup in watermarkExecutor() to enable wait-for-completion on shutdown and set
an await termination timeout so queued watermark tasks can finish before exit,
and also configure a finite queue capacity instead of relying on the default
unbounded queue. Keep the changes localized to watermarkExecutor() and the
ThreadPoolTaskExecutor configuration.
In `@src/main/java/ditda/backend/global/image/WatermarkImageProcessor.java`:
- Around line 43-47: The watermark image output in WatermarkImageProcessor
currently ignores the ImageIO.write result, so a missing PNG writer can still
return an empty byte array that gets uploaded and marked complete. Update the
image serialization flow in the method that builds the preview bytes to check
the boolean return from ImageIO.write, and if it is false, fail the operation
instead of returning bytes; keep the existing ByteArrayOutputStream path only
when writing succeeds.
- Around line 104-127: `WatermarkImageProcessor.processWatermarkImage` can enter
an infinite loop when the source image is narrower than `LOGO_WIDTH_RATIO`,
because `logoWidth` (and therefore `stepX`/`stepY`) becomes zero. Add a minimum
size guard or clamp the computed logo dimensions and tile steps to at least 1
before entering the nested watermark-drawing loops, and short-circuit or skip
watermarking for images too small to safely tile.
---
Nitpick comments:
In `@src/main/java/ditda/backend/global/image/WatermarkImageProcessor.java`:
- Around line 51-62: `WatermarkImageProcessor.loadLogo()` only handles
IOException, but `ImageIO.read(logoStream)` can return null for an unreadable or
corrupted logo and later cause `drawWatermark` to fail with an NPE. Update
`loadLogo()` to explicitly check the return value from `ImageIO.read` and throw
an `IllegalStateException` with the existing LOGO_PATH context when it is null,
so the `logo` field is never initialized with a null image.
In `@src/test/java/ditda/backend/global/image/WatermarkImageProcessorTest.java`:
- Around line 72-108: The manual preview test currently hardcodes a personal
local path in manualPreviewForEyeCheck, which makes it non-portable. Update this
test to read the input directory from a configurable source such as a system
property or environment variable, with a sensible fallback or clear failure if
unset. Keep the rest of the flow in WatermarkImageProcessorTest the same, but
replace the fixed Path.of(...) usage so each developer can run it in their own
environment.
🪄 Autofix (Beta)
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.yml
Review profile: CHILL
Plan: Pro Plus
Run ID: 0342381c-8721-4f3a-85fb-e1c50823fabd
⛔ Files ignored due to path filters (1)
src/main/resources/images/watermark-logo.pngis excluded by!**/*.png
📒 Files selected for processing (14)
src/main/java/ditda/backend/domain/commission/draft/entity/CommissionDraftFile.javasrc/main/java/ditda/backend/domain/commission/draft/event/DraftFilesSubmittedEvent.javasrc/main/java/ditda/backend/domain/commission/draft/facade/DesignerDraftFacade.javasrc/main/java/ditda/backend/domain/commission/draft/listener/DraftWatermarkListener.javasrc/main/java/ditda/backend/domain/commission/draft/repository/CommissionDraftFileRepository.javasrc/main/java/ditda/backend/domain/commission/draft/service/DraftWatermarkService.javasrc/main/java/ditda/backend/domain/commission/draft/service/DraftWatermarkTransitionService.javasrc/main/java/ditda/backend/domain/commission/revision/facade/DesignerRevisionFacade.javasrc/main/java/ditda/backend/global/config/WatermarkAsyncConfig.javasrc/main/java/ditda/backend/global/image/WatermarkImageProcessor.javasrc/main/java/ditda/backend/global/image/exception/ImageErrorCode.javasrc/main/java/ditda/backend/global/s3/manager/S3FileManager.javasrc/test/java/ditda/backend/domain/commission/draft/service/DraftWatermarkServiceTest.javasrc/test/java/ditda/backend/global/image/WatermarkImageProcessorTest.java
fervovita
left a comment
There was a problem hiding this comment.
수고하셨어요!!
추가로, 현재로써는 ec2의 메모리가 여유로운 편은 아니라서 ec2 내부에서 워터마크 처리를 한다면 OOM이 날 수도 있을 것 같습니다.
추후에 AWS lambda를 이용하는 방식 등을 고려해보시면 좋을 것 같아요!
| public Executor watermarkExecutor() { | ||
| ThreadPoolTaskExecutor executor = new ThreadPoolTaskExecutor(); | ||
| executor.setCorePoolSize(1); | ||
| executor.setMaxPoolSize(1); | ||
| executor.setThreadNamePrefix("watermark-"); | ||
| executor.setWaitForTasksToCompleteOnShutdown(true); | ||
| executor.setAwaitTerminationSeconds(30); | ||
| executor.initialize(); | ||
| return executor; |
There was a problem hiding this comment.
corePoolSize/maxPoolSize를 모두 1로 두신 건 이미지 처리 메모리 때문에 OOM을 막으려는 의도로 보입니다 👍
관련해서 한 가지만 언급하지만, ThreadPoolTaskExecutor의 queueCapacity 기본값이 Integer.MAX_VALUE라 사실상 무제한 큐입니다. 이 상태에서 재배포/종료 시 waitForTasksToCompleteOnShutdown(true) + awaitTerminationSeconds(30)으로 인해, 큐에 남은 작업을 30초 안에 처리하지 못하면 그대로 유실됩니다. poolSize=1이라 유실이 더 잦을 것 같습니다.
유실된 파일은 PROCESSING으로 남는데, 이 복구는 #116 에서 스케줄러로 트래킹 중인 걸로 보여 별도 조치는 필요 없어 보입니다.
혹시 몰라 기록 차원에서 남깁니다!
| for (CommissionDraftFile file : files) { | ||
| try { | ||
| String watermarkedKey = createWatermarked(file.getFileUrl()); | ||
| draftWatermarkTransitionService.complete(file.getId(), watermarkedKey); | ||
| } catch (Exception exception) { | ||
| log.error("워터마크 처리 실패. draftFileId={}, fileUrl={}", file.getId(), file.getFileUrl(), exception); | ||
| draftWatermarkTransitionService.fail(file.getId()); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
여기서 현재는 워터마크 처리에 얼마나 걸리는지에 따라 추후 pool / queue 등을 변경해야 하니, 아래와 같이 간단하게 로그 정도를 찍는게 어떨까요..?
for (CommissionDraftFile file : files) {
long start = System.nanoTime();
try {
String watermarkedKey = createWatermarked(file.getFileUrl());
draftWatermarkTransitionService.complete(file.getId(), watermarkedKey);
log.info("워터마크 완료. draftFileId={}, elapsedMs={}",
file.getId(), (System.nanoTime() - start) / 1_000_000);
} catch (Exception exception) {
log.error("워터마크 실패. draftFileId={}, fileUrl={}, elapsedMs={}",
file.getId(), file.getFileUrl(), (System.nanoTime() - start) / 1_000_000, exception);
draftWatermarkTransitionService.fail(file.getId());
}
}이거는 간단하게 찍는거라서, 추후에 Micrometer 등을 이용해서 트래킹하는 것도 좋을 것 같습니다!
There was a problem hiding this comment.
추가로, 현재 로컬에서 20MB 크기의 파일을 업로드하니 파일 하나당 8초 정도 나오는 것 같습니다...
물론 어차피 해당 워터마크 파일을 사용자가 바로 볼 일은 거의 없겠지만, pool에 많은 수의 파일이 몰리면 병목이 발생할 것 같습니다.
이 부분은 추후에 최적화가 필요할 것 같아요...
There was a problem hiding this comment.
확인했습니다... ㅠㅠ 파일 9장이면 거의 1분 30초정도 걸리는 건데, 다른 외주까지 고려하면 생각보다 더 걸릴 수 있겠군요.... 해당 부분은 좀 더 찾아보고 말씀드리겠습니다 😢
-> 우선 해상도를 1600에서 1200으로 낮췄는데 결과 차이가 별로 없는 거같아서 해당 부분으로 한 번 테스트해보고 결과 말씀드리겠습니다 😄
| private String createWatermarked(String originalKey) throws IOException { | ||
|
|
||
| byte[] watermarked; | ||
| // 원본 s3 다운로드 후 워터마크 진행 | ||
| try (InputStream original = s3FileManager.download(BUCKET, originalKey)) { | ||
| watermarked = watermarkImageProcessor.createWatermarkedPreview(original); | ||
| } | ||
|
|
||
| // 워터마크 진행된 파일 s3 업로드 (commission/draft/{uuid}.png -> commission/draft/wm/{uuid}.png) | ||
| int lastSlash = originalKey.lastIndexOf('/'); | ||
| if (lastSlash < 0) { | ||
| throw new IllegalArgumentException("유효하지 않은 S3 키: " + originalKey); | ||
| } | ||
| String dir = originalKey.substring(0, lastSlash); | ||
| String filename = originalKey.substring(lastSlash + 1); | ||
| int lastDot = filename.lastIndexOf('.'); | ||
| String baseName = lastDot > 0 ? filename.substring(0, lastDot) : filename; | ||
| String watermarkedKey = dir + "/" + WATERMARK_DIR + "/" + baseName + ".png"; | ||
|
|
||
| s3FileManager.upload(BUCKET, watermarkedKey, watermarked, S3ContentType.PNG.getContentType()); | ||
|
|
||
| return watermarkedKey; | ||
| } |
There was a problem hiding this comment.
현재로써는 출력이 항상 PNG로 내보내고 있는데, 이 결정이 WatermarkImageProcessor/DraftWatermarkService에 나눠져 있어서 추후 변경된다면 변경하기 어려울 듯 싶습니다.
// global/image
public record WatermarkedImage(byte[] bytes, S3ContentType contentType) {}
// WatermarkImageProcessor
public WatermarkedImage createWatermarkedPreview(InputStream source) throws IOException {
...
if (!ImageIO.write(preview, "png", out)) { ... }
return new WatermarkedImage(out.toByteArray(), S3ContentType.PNG); // 포맷 결정
}
// DraftWatermarkService.createWatermarked
WatermarkedImage result = watermarkImageProcessor.createWatermarkedPreview(original);
...
String watermarkedKey = dir + "/" + WATERMARK_DIR + "/" + baseName + result.contentType().getExtension();
s3FileManager.upload(BUCKET, watermarkedKey, result.bytes(), result.contentType().getContentType());이런식으로 파일 확장자를 WarkmarkImageProcessor에서만 결정하게 하는게 어떨까요..?
🚀 Related issue
Closes #106
#️⃣ Summary
🔧 Changes
WatermarkImageProcessor구현resources/images/watermark-logo.png)로 적용watermarkExecutor) 구성S3FileManager에download/upload추가📸 Test Evidence
💬 Reviewer Notes
wm/경로에 별도 저장됩니다. 강사 조회는 기존 mapper가COMPLETED일 때만 워터마크 URL을 발급하는 구조를 사용합니다.Summary by CodeRabbit
New Features
Bug Fixes