Skip to content

feat: 로컬 파일 업로드 로직 구현#140

Merged
plzslp merged 41 commits into
devfrom
feature/106-local-file-upload
Jun 25, 2026
Merged

feat: 로컬 파일 업로드 로직 구현#140
plzslp merged 41 commits into
devfrom
feature/106-local-file-upload

Conversation

@plzslp

@plzslp plzslp commented Jun 24, 2026

Copy link
Copy Markdown
Collaborator

관련 이슈

작업 내용

  • 비동기로 실행되는 로컬 파일 업로드 로직을 구현하고 테스트 코드를 작성 했습니다.
  • 비동기 스레드간 MDC, SecurityContext 전달을 위해 TaskDecorator를 추가했습니다.
  • BinaryContentStorage 인터페이스를 통해 Local 환경과 S3 환경별 구현체를 선택할 수 있도록 추상화했습니다.
  • Content 필드에 파일 업로드 상태(BinaryContentUploadStatus)를 추가하여 추후 확장을 열어두었습니다.
  • Content-ContentStats의 OneToOne 연관관계 매핑을 추가하였습니다. (Repository 조회 메서드에 EntityGraph 추가)
  • Content 필드 중 contentTags의 타입을 List > Set으로 변경하였습니다.

변경 사항

  • binarycontent 디렉토리 추가 > 리스너, 예외, 이벤트 클래스 작성
  • V5__add_thumbnail_upload_status 추가
  • application-dev.yml에 storage 및 static-locations 설정 추가
  • application-test.yml에 storage 설정 추가

테스트 내용

  • 테스트 코드 작성
  • 로컬 테스트 완료
  • API 테스트 완료
  • 기타 테스트 완료

체크리스트

  • 코드 컨벤션을 지켰습니다.
  • 불필요한 코드를 제거했습니다.
  • 주석이 필요한 부분에 추가했습니다.
  • 관련 문서를 수정했습니다.
  • 리뷰어가 이해하기 쉽도록 작성했습니다.

리뷰 포인트

  • 중점적으로 봐줬으면 하는 부분을 작성해주세요.

스크린샷 / 참고 자료

  • 필요한 경우 첨부해주세요.

🔗 관련 이슈

  • close #106

📝 작업 내용

  • 프로필 사진과 콘텐츠 썸네일을 파일로 저장하고, 저장 결과 URL과 업로드 상태를 반환할 수 있도록 업로드 흐름을 추가했습니다.
  • 기존에는 이미지 경로/URL 중심으로 다루던 구조였는데, 이제는 바이너리 저장소 추상화를 두어 Local/S3 저장 방식을 설정으로 선택할 수 있게 바꿨습니다.
  • 비동기 업로드와 상태 갱신이 분리되도록 구성해, 본문 생성 흐름은 유지하면서 업로드는 커밋 이후 백그라운드에서 처리되게 했습니다.

주요 변경사항

  1. 저장 방식 추상화 및 비동기 업로드 흐름 도입

    • BinaryContentStorage 인터페이스를 추가해 Local/S3 구현체를 분리했고, 업로드 키 생성과 URL 변환 책임을 통일했습니다.
    • 썸네일/프로필 이미지 업로드는 이벤트 발행 후 비동기 리스너가 실제 저장을 수행하도록 변경했습니다.
  2. 콘텐츠/유저 도메인 모델 변경

    • Content의 썸네일과 User의 프로필 이미지를 문자열 URL이 아닌 BinaryContent 연관관계로 전환했습니다.
    • ContentStatsContent와 OneToOne으로 묶고, 조회 시 함께 읽히도록 리포지토리 EntityGraph를 반영했습니다.
    • contentTags는 중복 제어가 쉬운 Set 기반으로 바꿨습니다.
  3. 상태 및 예외 처리 보강

    • 업로드 상태(PENDING, COMPLETED, FAILED)를 추가해 저장 성공/실패를 도메인에 반영하도록 했습니다.
    • 로컬 저장 디렉터리 초기화, 파일 저장 실패, 업로드 대상 미존재 상황에 대한 예외를 세분화했습니다.
    • 비동기 실행 시 MDC, SecurityContext가 전달되도록 TaskDecorator 구성을 추가했습니다.

📊 변경 효과 요약

  • 정량 지표는 이번 PR에서 측정하지 않음
  • 테스트 추가: 업로드 리스너, 로컬 저장소, 서비스 계층, 콘텐츠 생성 흐름 관련 테스트 보강
  • 설정/마이그레이션 추가: 로컬/테스트용 스토리지 설정 및 DB 스키마 변경 포함

@plzslp plzslp self-assigned this Jun 24, 2026
@coderabbitai

coderabbitai Bot commented Jun 24, 2026

Copy link
Copy Markdown

Review Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

바이너리 저장 계약, 로컬/S3 구현, 업로드 상태 엔티티·예외·이벤트·서비스가 추가됐다. 콘텐츠와 사용자 엔티티는 썸네일과 프로필 이미지를 BinaryContent 연관으로 바꾸고, 콘텐츠 태그는 Set 기반으로 전환됐다. 콘텐츠 생성 시 썸네일을 저장해 업로드 이벤트를 발행하며, 커밋 이후 리스너가 저장 결과에 따라 상태를 갱신한다. 관련 마이그레이션, 설정, 테스트도 함께 수정됐다.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

  • sb10-part4-team5/sb10-mopl-team5#96: User의 프로필 이미지를 BinaryContent로 바꾸는 흐름이 이번 PR의 사용자 이미지 연관 변경과 같은 파일 경로를 다룹니다.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed 제목이 로컬 파일 업로드 로직 구현이라는 핵심 변경을 정확히 요약합니다.
Linked Issues check ✅ Passed #106의 프로필 사진·썸네일 업로드, Local/S3 추상화, 정적 경로 반환 요구를 코드가 대부분 충족합니다.
Out of Scope Changes check ✅ Passed 변경된 항목들은 업로드 흐름, 저장소 추상화, 연관관계/설정/테스트 보강으로 보이며 뚜렷한 이탈 범위는 보이지 않습니다.
Description check ✅ Passed 필수 섹션과 핵심 변경·테스트 내용이 대부분 채워져 있어 템플릿 요구사항을 충분히 충족합니다.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feature/106-local-file-upload

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 13

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/main/java/com/codeit/team5/mopl/content/mapper/ContentMapper.java (1)

23-27: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

tags 응답 순서가 비결정적으로 바뀝니다.

Content 쪽 컬렉션이 HashSet인데 여기서 정렬 없이 toList()로 내보내면, 같은 데이터여도 응답 순서가 실행/조회 경로마다 달라질 수 있습니다. API는 여전히 List<String>를 노출하므로 소비자 입장에서는 순서가 계약처럼 보이기 쉽습니다.

선택지는 다음 둘 중 하나입니다.

  • 입력 순서를 유지하고 싶다면 엔티티 컬렉션을 LinkedHashSet으로 바꾸기
  • 순서 자체가 중요하지 않다면 여기서 sorted()로 API 출력을 고정하기
🤖 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/com/codeit/team5/mopl/content/mapper/ContentMapper.java` around
lines 23 - 27, The tags response order is currently non-deterministic because
ContentMapper.toTagNames() streams from a HashSet-backed collection and returns
toList() without ordering. Fix this by making the output order stable: either
preserve insertion order at the Content entity collection level with
LinkedHashSet, or sort the names in toTagNames() before collecting so the
List<String> returned by ContentMapper is consistent across requests.
🤖 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/com/codeit/team5/mopl/binarycontent/BinaryContentStorage.java`:
- Around line 8-11: BinaryContentStorage.generateKey currently appends the
client-provided filename extension directly into the storage key, which can
break URL/download path resolution when the extension contains unsafe
characters. Update generateKey to normalize the extension before use, preferably
by allowing only a small validated whitelist of safe extensions and falling back
to no extension when invalid; keep the key format consistent with toUrl and
storage lookup behavior.

In
`@src/main/java/com/codeit/team5/mopl/binarycontent/event/BinaryContentUploadEvent.java`:
- Around line 5-9: BinaryContentUploadEvent currently exposes the mutable byte[]
directly, so updates after publication can leak into the async AFTER_COMMIT
handler; update the BinaryContentUploadEvent record to make a defensive copy of
bytes in the constructor and return a copy from the accessor so callers cannot
mutate the stored payload.

In
`@src/main/java/com/codeit/team5/mopl/binarycontent/storage/local/LocalBinaryContentStorage.java`:
- Around line 39-40: The exception wrapping in LocalBinaryContentStorage is
dropping the original IOException cause, which makes failures hard to diagnose.
Update the catch/throw paths in LocalBinaryContentStorage so that
FileStorageException, UploadDirectoryInitException, and
BinaryContentStorageException preserve the underlying throwable by adding
Throwable-cause constructor overloads and passing the caught IOException or init
failure through. Use the affected throw sites in LocalBinaryContentStorage to
attach the cause instead of creating cause-less exceptions.

In
`@src/main/java/com/codeit/team5/mopl/binarycontent/storage/s3/S3BinaryContentStorage.java`:
- Around line 23-24: The S3BinaryContentStorage.store method currently always
throws UnsupportedOperationException, causing every upload under storage.type=s3
to fail. Fix this by either implementing the actual S3 write logic in
store(String key, byte[] bytes) or, if S3 is intentionally unsupported, removing
this runtime failure path and making the application fail fast at startup when
S3 is selected so the misconfiguration is detected earlier.

In
`@src/main/java/com/codeit/team5/mopl/binarycontent/storage/s3/S3StorageProperties.java`:
- Around line 6-11: S3StorageProperties currently exposes secretKey through the
record’s default toString(), which can leak the raw secret in logs or
exceptions. Update S3StorageProperties to prevent sensitive data from being
printed by either overriding toString() to mask secretKey while keeping the
record, or converting it to a regular class if you need finer control over
string representation. Make sure the fix specifically covers any debug/log
output paths that may call S3StorageProperties.toString().

In `@src/main/java/com/codeit/team5/mopl/config/AsyncConfig.java`:
- Around line 6-8: AsyncConfig currently enables async support without defining
a dedicated executor for upload-related work. Update the AsyncConfig class to
expose a named ThreadPoolTaskExecutor bean and wire it through the async
configuration so upload tasks use controlled pool settings. Keep the executor
settings explicit by fixing queueCapacity and a RejectedExecutionHandler, and
reference the AsyncConfig class and its async setup so the upload path is
isolated from Spring Boot’s default executor.

In `@src/main/java/com/codeit/team5/mopl/content/entity/Content.java`:
- Around line 76-77: `Content.contentTags` was switched to `HashSet`, which
makes the tag order returned through `ContentMapper.toTagNames(...)`
nondeterministic. Change the collection in `Content` to an order-preserving set
such as `LinkedHashSet`, or update `ContentMapper.toTagNames(...)` to explicitly
sort before returning the `List<String>`, so the API’s tag order stays stable
and predictable.
- Around line 121-125: `Content.addTag` currently relies on
`ContentTag.create(this, tag)`, but `ContentTag` instances are being created
with an empty `ContentTagId`, so `equals/hashCode` treats multiple new entries
as identical and the `contentTags` Set can drop tags. Update `ContentTag.create`
(and any related constructor/factory logic) to populate the composite identifier
from `content.getId()` and `tag.getId()` before adding to the Set, or adjust
`ContentTag.equals/hashCode` to use the `(content, tag)` business key
consistently.

In `@src/main/java/com/codeit/team5/mopl/content/entity/ContentTag.java`:
- Around line 47-56: `ContentTag.equals()` and `hashCode()` currently compare
only `id`, so all newly created tags with an empty `ContentTagId` collapse into
one entry in `Content.addTag()`’s `HashSet`. Fix this by making
`ContentTag.create()` initialize the embedded id immediately with `new
ContentTagId(content.getId(), tag.getId())`, or otherwise update `ContentTag`
equality to fall back to `content` and `tag` while the id is transient. Keep the
behavior aligned with `ContentTag`, `ContentTagId`, and `Content.addTag()` so
distinct tags are not treated as duplicates before persistence.

In `@src/main/java/com/codeit/team5/mopl/content/service/ContentService.java`:
- Around line 49-55: The thumbnail upload flow in ContentService currently sets
the thumbnail URL before reading the bytes, which can leave a stale URL behind
when thumbnail.getBytes() throws IOException. Update the try block in the
thumbnail handling logic to read the byte[] first, then call
initThumbnail(binaryContentStorage.toUrl(key)) and publish the
BinaryContentUploadEvent only after the bytes are successfully available. Keep
the existing catch path and ensure failThumbnailUpload() is still used when the
read fails.

In `@src/main/resources/application-dev.yml`:
- Around line 43-51: The local storage setup is returning a file-system-backed
static URL instead of the required API endpoint path. Update the local storage
flow so `storage.local.base-url` points to a download API/handler rather than
the uploaded directory, and remove or avoid direct static exposure of
`upload-dir` through `web.resources.static-locations`. Make sure the `base-url +
"/" + key` behavior in the local storage implementation still resolves to the
API endpoint contract.

In
`@src/test/java/com/codeit/team5/mopl/binarycontent/storage/local/LocalBinaryContentStorageTest.java`:
- Around line 23-30: The current LocalBinaryContentStorageTest only covers
store() I/O failures, so add a dedicated test around LocalBinaryContentStorage
and its init() path to verify constructor-time initialization failure. Create a
case where the upload root in LocalStorageProperties points to an existing file
instead of a directory, then assert that new
LocalBinaryContentStorage(properties) throws UploadDirectoryInitException. Keep
the existing setUp-based store() failure coverage, but add this separate
constructor initialization regression test so the init() failure path is
explicitly protected.

In `@src/test/java/com/codeit/team5/mopl/content/service/ContentServiceTest.java`:
- Around line 53-58: ContentServiceTest does not currently lock in the thumbnail
upload branch of create(), so regressions in key generation, URL assignment, and
BinaryContentUploadEvent publication can slip through. Add a test for the
thumbnail-present path that stubs BinaryContentStorage.generateKey() and
toUrl(), verifies ApplicationEventPublisher.publishEvent() is called, and
asserts the saved Content has thumbnailUploadStatus set to PENDING; also add a
thumbnail-absent test that verifies no interactions with BinaryContentStorage or
eventPublisher. Use the existing ContentServiceTest setup and the create() flow
to locate the behavior under test.

---

Outside diff comments:
In `@src/main/java/com/codeit/team5/mopl/content/mapper/ContentMapper.java`:
- Around line 23-27: The tags response order is currently non-deterministic
because ContentMapper.toTagNames() streams from a HashSet-backed collection and
returns toList() without ordering. Fix this by making the output order stable:
either preserve insertion order at the Content entity collection level with
LinkedHashSet, or sort the names in toTagNames() before collecting so the
List<String> returned by ContentMapper is consistent across requests.
🪄 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: ASSERTIVE

Plan: Pro Plus

Run ID: 67080016-6c89-44a1-8444-2966ff9eed92

📥 Commits

Reviewing files that changed from the base of the PR and between f9cd2c0 and ea3c443.

📒 Files selected for processing (28)
  • .gitignore
  • src/main/java/com/codeit/team5/mopl/binarycontent/BinaryContentStorage.java
  • src/main/java/com/codeit/team5/mopl/binarycontent/entity/BinaryContentUploadStatus.java
  • src/main/java/com/codeit/team5/mopl/binarycontent/event/BinaryContentUploadEvent.java
  • src/main/java/com/codeit/team5/mopl/binarycontent/exception/BinaryContentStorageException.java
  • src/main/java/com/codeit/team5/mopl/binarycontent/exception/FileStorageException.java
  • src/main/java/com/codeit/team5/mopl/binarycontent/exception/UploadDirectoryInitException.java
  • src/main/java/com/codeit/team5/mopl/binarycontent/listener/BinaryContentUploadListener.java
  • src/main/java/com/codeit/team5/mopl/binarycontent/storage/local/LocalBinaryContentStorage.java
  • src/main/java/com/codeit/team5/mopl/binarycontent/storage/local/LocalStorageProperties.java
  • src/main/java/com/codeit/team5/mopl/binarycontent/storage/s3/S3BinaryContentStorage.java
  • src/main/java/com/codeit/team5/mopl/binarycontent/storage/s3/S3StorageProperties.java
  • src/main/java/com/codeit/team5/mopl/config/AsyncConfig.java
  • src/main/java/com/codeit/team5/mopl/config/StorageConfig.java
  • src/main/java/com/codeit/team5/mopl/content/dto/response/ContentResponse.java
  • src/main/java/com/codeit/team5/mopl/content/entity/Content.java
  • src/main/java/com/codeit/team5/mopl/content/entity/ContentTag.java
  • src/main/java/com/codeit/team5/mopl/content/mapper/ContentMapper.java
  • src/main/java/com/codeit/team5/mopl/content/repository/ContentRepository.java
  • src/main/java/com/codeit/team5/mopl/content/service/ContentService.java
  • src/main/java/com/codeit/team5/mopl/watcher/repository/WatchingSessionRepository.java
  • src/main/resources/application-dev.yml
  • src/main/resources/db/migration/V5__add_thumbnail_upload_status.sql
  • src/test/java/com/codeit/team5/mopl/binarycontent/listener/BinaryContentUploadListenerTest.java
  • src/test/java/com/codeit/team5/mopl/binarycontent/storage/local/LocalBinaryContentStorageTest.java
  • src/test/java/com/codeit/team5/mopl/content/controller/ContentControllerTest.java
  • src/test/java/com/codeit/team5/mopl/content/service/ContentServiceTest.java
  • src/test/resources/application-test.yml

Comment thread src/main/java/com/codeit/team5/mopl/binarycontent/BinaryContentStorage.java Outdated
Comment thread src/main/java/com/codeit/team5/mopl/content/entity/ContentTag.java
Comment thread src/main/java/com/codeit/team5/mopl/content/service/ContentService.java Outdated
Comment thread src/main/resources/application-dev.yml Outdated

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/main/java/com/codeit/team5/mopl/binarycontent/listener/BinaryContentUploadListener.java (1)

24-40: 🗄️ Data Integrity & Integration | 🟠 Major

비동기 리스너는 상태 전용 업데이트로 바꾸는 편이 안전합니다. BinaryContentUploadListener.handle()findById()로 읽은 Content를 비동기/별도 트랜잭션 흐름에서 다시 save()로 merge합니다. Content에는 @Version이 없어서, 이 사이에 다른 요청이 title, description 같은 필드를 바꾸면 오래된 스냅샷이 함께 덮어써질 수 있습니다.

  • 추천: thumbnail_upload_status만 갱신하는 update 쿼리로 변경
    • 장점: 덮어쓰기 범위가 가장 작고, 이 흐름의 의도와도 맞습니다.
  • 대안: @Version 추가 + 단일 트랜잭션
    • 장점: 충돌을 조용히 덮지 않고 드러냅니다.
    • 단점: 충돌 시 재시도/예외 처리까지 같이 설계해야 합니다.
🤖 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/com/codeit/team5/mopl/binarycontent/listener/BinaryContentUploadListener.java`
around lines 24 - 40, `BinaryContentUploadListener.handle()` currently loads a
full `Content` entity and saves it back in an async listener, which can
overwrite unrelated fields with a stale snapshot. Replace the read-modify-save
flow with a targeted status-only update for the thumbnail upload state (for
example via a repository update method) so only the upload status changes. If
you keep the entity merge approach, add optimistic locking to `Content` with
`@Version` and handle conflicts explicitly.
♻️ Duplicate comments (1)
src/test/java/com/codeit/team5/mopl/content/service/ContentServiceTest.java (1)

112-116: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

썸네일 URL 설정 계약은 아직 테스트에 완전히 잠기지 않았습니다.

지금은 thumbnailUploadStatus와 이벤트 key만 검증해서, create()toUrl() 결과를 Content.thumbnailUrl에 넣지 않아도 이 테스트는 통과합니다. 이번 PR 목표가 “저장 후 URL 반환”까지 포함되어 있으니, savedContent.getThumbnailUrl() 값이나 binaryContentStorage.toUrl(...) 호출까지 같이 고정해 두는 편이 안전합니다.

예시 보강
         assertThat(savedContent.getDescription()).isEqualTo("테스트 설명");
+        assertThat(savedContent.getThumbnailUrl())
+                .isEqualTo("http://localhost:8080/thumbnails/test-content/test.jpg");
         assertThat(savedContent.getThumbnailUploadStatus()).isEqualTo(BinaryContentUploadStatus.PENDING);

         ArgumentCaptor<BinaryContentUploadEvent> eventCaptor = ArgumentCaptor.forClass(BinaryContentUploadEvent.class);
         verify(eventPublisher).publishEvent(eventCaptor.capture());
         assertThat(eventCaptor.getValue().key()).isEqualTo("thumbnails/test-content/test.jpg");
+        verify(binaryContentStorage).toUrl("thumbnails/test-content/test.jpg");
🤖 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/com/codeit/team5/mopl/content/service/ContentServiceTest.java`
around lines 112 - 116, `ContentServiceTest`의 `create()` 계약이
`thumbnailUploadStatus`와 이벤트 `key`만 검증하고 있어 `Content.thumbnailUrl` 설정이 빠져도
통과합니다. `savedContent.getThumbnailUrl()`을 추가로 검증하거나
`binaryContentStorage.toUrl(...)` 호출을 `ContentService.create()`와 함께 고정해서, 썸네일
업로드 후 URL이 저장되는 동작을 테스트에 포함시키세요. `savedContent`, `eventCaptor`,
`binaryContentStorage`, `toUrl()`를 기준으로 테스트를 보강하면 위치를 쉽게 찾을 수 있습니다.
🤖 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/com/codeit/team5/mopl/config/AsyncConfig.java`:
- Around line 25-27: `AsyncConfig`의 executor가 `CallerRunsPolicy`를 사용해 포화 시
`BinaryContentUploadListener.handle(...)` 작업이 요청 스레드에서 직접 실행되는 문제가 있습니다.
`setRejectedExecutionHandler(...)`를 `AbortPolicy`로 바꿔 즉시 거절/재시도 흐름으로 넘기거나, 업로드
전용 별도 큐로 분리해 웹 요청 경로로 I/O가 되돌아오지 않도록 수정하세요. `BinaryContentUploadListener`와
`contentRepository.save(...)`가 요청 지연에 직접 영향을 주지 않게 만드는 방향으로 조정하면 됩니다.

In
`@src/main/java/com/codeit/team5/mopl/global/async/SecurityContextTaskDecorator.java`:
- Around line 10-18: SecurityContextTaskDecorator.decorate currently captures
and reuses the same SecurityContext instance, which can leak shared state and
clear the caller’s authentication. Update decorate to copy the current
authentication into a fresh SecurityContext before running the task, and in the
returned Runnable save the previous context, set the copied one, then restore
the original context in finally instead of always clearing it. Use
SecurityContextTaskDecorator, decorate, SecurityContextHolder, and
SecurityContext as the key symbols when making the fix.

---

Outside diff comments:
In
`@src/main/java/com/codeit/team5/mopl/binarycontent/listener/BinaryContentUploadListener.java`:
- Around line 24-40: `BinaryContentUploadListener.handle()` currently loads a
full `Content` entity and saves it back in an async listener, which can
overwrite unrelated fields with a stale snapshot. Replace the read-modify-save
flow with a targeted status-only update for the thumbnail upload state (for
example via a repository update method) so only the upload status changes. If
you keep the entity merge approach, add optimistic locking to `Content` with
`@Version` and handle conflicts explicitly.

---

Duplicate comments:
In `@src/test/java/com/codeit/team5/mopl/content/service/ContentServiceTest.java`:
- Around line 112-116: `ContentServiceTest`의 `create()` 계약이
`thumbnailUploadStatus`와 이벤트 `key`만 검증하고 있어 `Content.thumbnailUrl` 설정이 빠져도
통과합니다. `savedContent.getThumbnailUrl()`을 추가로 검증하거나
`binaryContentStorage.toUrl(...)` 호출을 `ContentService.create()`와 함께 고정해서, 썸네일
업로드 후 URL이 저장되는 동작을 테스트에 포함시키세요. `savedContent`, `eventCaptor`,
`binaryContentStorage`, `toUrl()`를 기준으로 테스트를 보강하면 위치를 쉽게 찾을 수 있습니다.
🪄 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: ASSERTIVE

Plan: Pro Plus

Run ID: 3f25c45a-34ca-4263-802a-af2b8ed07b8f

📥 Commits

Reviewing files that changed from the base of the PR and between ea3c443 and e955027.

📒 Files selected for processing (15)
  • src/main/java/com/codeit/team5/mopl/binarycontent/BinaryContentStorage.java
  • src/main/java/com/codeit/team5/mopl/binarycontent/event/BinaryContentUploadEvent.java
  • src/main/java/com/codeit/team5/mopl/binarycontent/exception/BinaryContentStorageException.java
  • src/main/java/com/codeit/team5/mopl/binarycontent/exception/FileStorageException.java
  • src/main/java/com/codeit/team5/mopl/binarycontent/exception/UploadDirectoryInitException.java
  • src/main/java/com/codeit/team5/mopl/binarycontent/listener/BinaryContentUploadListener.java
  • src/main/java/com/codeit/team5/mopl/binarycontent/storage/local/LocalBinaryContentStorage.java
  • src/main/java/com/codeit/team5/mopl/config/AsyncConfig.java
  • src/main/java/com/codeit/team5/mopl/content/entity/ContentTag.java
  • src/main/java/com/codeit/team5/mopl/content/mapper/ContentMapper.java
  • src/main/java/com/codeit/team5/mopl/global/async/CompositeTaskDecorator.java
  • src/main/java/com/codeit/team5/mopl/global/async/MdcTaskDecorator.java
  • src/main/java/com/codeit/team5/mopl/global/async/SecurityContextTaskDecorator.java
  • src/test/java/com/codeit/team5/mopl/binarycontent/storage/local/LocalBinaryContentStorageTest.java
  • src/test/java/com/codeit/team5/mopl/content/service/ContentServiceTest.java

Comment thread src/main/java/com/codeit/team5/mopl/config/AsyncConfig.java Outdated
@plzslp plzslp requested a review from metDaisy June 25, 2026 01:06

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

🤖 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/com/codeit/team5/mopl/binarycontent/listener/BinaryContentUploadListener.java`:
- Around line 28-33: Separate the storage step from the status update in
BinaryContentUploadListener so a successful store is not later overwritten as
FAILED. In the listener handling the upload event, keep
binaryContentStorage.store(...) in its own success/failure path, then update
BinaryContentUploadStatus.COMPLETED only after storage succeeds, and handle any
exception from contentRepository.updateThumbnailUploadStatus(...) separately
without marking the upload as FAILED. Use the BinaryContentUploadListener method
that processes the event and the contentRepository/binaryContentStorage calls to
locate the change.

In
`@src/main/java/com/codeit/team5/mopl/content/repository/ContentRepository.java`:
- Around line 18-20: The update method in ContentRepository currently discards
the affected-row count, so BinaryContentUploadListener can treat a 0-row JPQL
update as success and log completion incorrectly. Change
updateThumbnailUploadStatus to return the affected row count instead of void,
and update the call site in BinaryContentUploadListener to check that exactly
one row was updated before considering the asynchronous status transition
successful.

In
`@src/test/java/com/codeit/team5/mopl/binarycontent/listener/BinaryContentUploadListenerTest.java`:
- Around line 33-64: Directly calling BinaryContentUploadListener.handle
bypasses the `@Async`, `@TransactionalEventListener`(AFTER_COMMIT), and REQUIRES_NEW
behavior, so these tests don’t verify the real contract. Update the tests around
BinaryContentUploadListener.handle by exercising the listener through the Spring
context and publishing a BinaryContentUploadEvent instead of invoking
handle(event) directly, then assert the status changes after commit. Keep the
existing mocks for BinaryContentStorage and ContentRepository, but add at least
one integration-style test that confirms the listener is actually registered and
runs asynchronously after transaction commit.
- Line 45: The test in BinaryContentUploadListenerTest is comparing byte[] using
event.bytes(), but BinaryContentUploadEvent.bytes() returns a cloned array each
call, so the verification will fail even when contents match. Update the
assertion around binaryContentStorage.store to compare the stored bytes by
content instead of reference, using aryEq(bytes) or an ArgumentCaptor<byte[]>
while keeping the existing storage path check intact.
🪄 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: ASSERTIVE

Plan: Pro Plus

Run ID: e4d303ad-5b57-4cd6-959b-1bc3a9ef0fe9

📥 Commits

Reviewing files that changed from the base of the PR and between e955027 and 4db18f1.

📒 Files selected for processing (5)
  • src/main/java/com/codeit/team5/mopl/binarycontent/listener/BinaryContentUploadListener.java
  • src/main/java/com/codeit/team5/mopl/config/AsyncConfig.java
  • src/main/java/com/codeit/team5/mopl/content/repository/ContentRepository.java
  • src/main/java/com/codeit/team5/mopl/global/async/SecurityContextTaskDecorator.java
  • src/test/java/com/codeit/team5/mopl/binarycontent/listener/BinaryContentUploadListenerTest.java
💤 Files with no reviewable changes (1)
  • src/main/java/com/codeit/team5/mopl/config/AsyncConfig.java

Comment thread src/main/java/com/codeit/team5/mopl/content/repository/ContentRepository.java Outdated
Comment thread src/main/java/com/codeit/team5/mopl/content/entity/Content.java Outdated
Comment thread src/main/java/com/codeit/team5/mopl/content/entity/ContentTag.java Outdated
@metDaisy

Copy link
Copy Markdown
Collaborator

추가적으로 contentservice 에게 너무 많은 역할과 책임을 가지고 있는 건 아닌지 한번 고려해주셨으면 좋겠습니다.
content service 는 content 에 대한 비즈니스 로직을 담당합니다.
이 중 CRUD 가 포함되죠.
현재 service 에서 content 생성뿐만 아니라 contentstats, tag 객체도 생성하고 있습니다.
content service 의 역할이 이 객체들에 대한 비즈니스 로직을 담당해야 한다면 괜찮다고 생각합니다.

metDaisy
metDaisy previously approved these changes Jun 25, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 9

🤖 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/com/codeit/team5/mopl/binarycontent/listener/BinaryContentUploadListener.java`:
- Around line 24-38: BinaryContentUploadListener.handle currently wraps the
whole flow in `@Transactional`(REQUIRES_NEW), so binaryContentStorage.store runs
while holding a DB connection. Move the external upload in
handle(BinaryContentUploadEvent) outside any transaction, then call a separate
short transactional method to load the BinaryContent and invoke completeUpload
or failUpload. Keep the existing BinaryContentUploadListener, handle,
binaryContentRepository, and binaryContentStorage symbols, but split the I/O and
state update paths so only the status change is transactional.

In
`@src/main/java/com/codeit/team5/mopl/binarycontent/storage/local/LocalBinaryContentStorage.java`:
- Line 16: The activation condition for LocalBinaryContentStorage is
inconsistent with its implementation name: it currently uses
mopl.storage.type=memory while representing a local file-backed storage bean,
which can leave BinaryContentStorage unregistered when mopl.storage.type=local
is set. Update the `@ConditionalOnProperty` on LocalBinaryContentStorage to match
the intended contract (prefer local) and make the default configuration align
with that value so the LocalBinaryContentStorage and S3BinaryContentStorage
conditions remain clear and mutually understandable.

In `@src/main/java/com/codeit/team5/mopl/content/entity/Content.java`:
- Around line 52-54: The Content.thumbnail mapping is declared as `@OneToOne` but
the database only enforces a foreign key, so the 1:1 relationship is not
guaranteed. Update the Content.thumbnail join mapping to include a unique
constraint on the thumbnail_id join column, and add the matching UNIQUE
constraint in the Flyway migration that creates/updates the binary content
reference so the DB enforces the same cardinality.

In `@src/main/java/com/codeit/team5/mopl/user/entity/User.java`:
- Around line 38-40: `User.profileImage` is mapped as `@OneToOne`, but the
database column lacks a uniqueness constraint, so align the model and schema.
Either update the `User` entity’s `@JoinColumn` to enforce uniqueness for
`profile_image_id` and add a `UNIQUE` constraint in
`V8__add_profile_image_to_users.sql`, or if sharing the same `BinaryContent`
across users is intended, change `profileImage` from `@OneToOne` to `@ManyToOne`
so the mapping matches the actual relationship.

In `@src/main/resources/application-prod.yml`:
- Around line 44-47: The prod configuration is forcing mopl.storage.type to s3
even though S3BinaryContentStorage::store still throws
UnsupportedOperationException, so locate the mopl.storage setting in
application-prod.yml and either keep production on memory until S3 upload
support is implemented or update the storage implementation and prod switch
together in the same change.

In `@src/main/resources/db/migration/V6__add_binary_contents.sql`:
- Around line 15-17: The migration in V6__add_binary_contents.sql drops
contents.thumbnail_url and contents.thumbnail_upload_status too early, which
would lose existing thumbnail data. Update the migration sequence so existing
rows are first backfilled by creating BinaryContent records from thumbnail_url
with upload_status set to COMPLETED and populating contents.thumbnail_id, then
remove the old columns. Keep the changes localized around the
V6__add_binary_contents.sql migration and the ContentService/BinaryContent data
flow so the new schema supports both migrated and newly created content.

In `@src/main/resources/db/migration/V7__restructure_content_stats.sql`:
- Around line 1-18: The current V7 migration recreates content_stats and drops
existing review_count/rating_sum/watcher_count data, so update the migration to
preserve and migrate existing records instead of relying on a destructive
DROP/CREATE. Use the existing content_stats and contents.stats_id relationship
as the anchor: before removing the old structure, copy the current aggregated
values into the new schema and re-link rows as needed, then add the new foreign
key on contents.stats_id. Keep the logic in V7__restructure_content_stats.sql
aligned with the existing content_stats/content_id and contents/stats_id symbols
so the migration is safe for environments with live data.

In `@src/main/resources/db/migration/V8__add_profile_image_to_users.sql`:
- Around line 5-6: The migration currently drops profile_image_url in
V8__add_profile_image_to_users.sql without preserving existing user images, so
update this migration to first backfill data by creating binary_contents records
and populating users.profile_image_id from the existing URL via the users table
before removing the old column. Use the migration and the users/profile_image_id
flow as the key symbols to locate the change, and keep the DROP COLUMN only
after the data has been safely migrated.

In `@src/test/java/com/codeit/team5/mopl/content/service/ContentServiceTest.java`:
- Around line 106-108: The test around ContentServiceTest should verify the full
BinaryContentUploadEvent payload, not just key. Update the assertion block that
captures the event from eventPublisher.publishEvent so it also checks that
binaryContentId matches the saved BinaryContent’s id and that bytes contains the
expected {1, 2, 3} payload, using BinaryContentUploadEvent and the saved
BinaryContent reference to locate the right values.
🪄 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: ASSERTIVE

Plan: Pro Plus

Run ID: 3e940c0a-9041-4c50-abeb-8972aede24a0

📥 Commits

Reviewing files that changed from the base of the PR and between 4db18f1 and 82396ca.

📒 Files selected for processing (23)
  • src/main/java/com/codeit/team5/mopl/binarycontent/entity/BinaryContent.java
  • src/main/java/com/codeit/team5/mopl/binarycontent/event/BinaryContentUploadEvent.java
  • src/main/java/com/codeit/team5/mopl/binarycontent/listener/BinaryContentUploadListener.java
  • src/main/java/com/codeit/team5/mopl/binarycontent/repository/BinaryContentRepository.java
  • src/main/java/com/codeit/team5/mopl/binarycontent/storage/local/LocalBinaryContentStorage.java
  • src/main/java/com/codeit/team5/mopl/binarycontent/storage/local/LocalStorageProperties.java
  • src/main/java/com/codeit/team5/mopl/binarycontent/storage/s3/S3BinaryContentStorage.java
  • src/main/java/com/codeit/team5/mopl/binarycontent/storage/s3/S3StorageProperties.java
  • src/main/java/com/codeit/team5/mopl/content/entity/Content.java
  • src/main/java/com/codeit/team5/mopl/content/entity/ContentStats.java
  • src/main/java/com/codeit/team5/mopl/content/mapper/ContentMapper.java
  • src/main/java/com/codeit/team5/mopl/content/repository/ContentRepository.java
  • src/main/java/com/codeit/team5/mopl/content/service/ContentService.java
  • src/main/java/com/codeit/team5/mopl/user/entity/User.java
  • src/main/java/com/codeit/team5/mopl/user/mapper/UserMapper.java
  • src/main/resources/application-dev.yml
  • src/main/resources/application-prod.yml
  • src/main/resources/db/migration/V6__add_binary_contents.sql
  • src/main/resources/db/migration/V7__restructure_content_stats.sql
  • src/main/resources/db/migration/V8__add_profile_image_to_users.sql
  • src/test/java/com/codeit/team5/mopl/binarycontent/listener/BinaryContentUploadListenerTest.java
  • src/test/java/com/codeit/team5/mopl/content/service/ContentServiceTest.java
  • src/test/resources/application-test.yml

Comment thread src/main/java/com/codeit/team5/mopl/content/entity/Content.java
Comment thread src/main/java/com/codeit/team5/mopl/user/entity/User.java
Comment thread src/main/resources/application-prod.yml
Comment thread src/main/resources/db/migration/V6__add_binary_contents.sql
Comment thread src/main/resources/db/migration/V7__restructure_content_stats.sql
Comment thread src/main/resources/db/migration/V8__add_profile_image_to_users.sql
metDaisy
metDaisy previously approved these changes Jun 25, 2026
@plzslp plzslp force-pushed the feature/106-local-file-upload branch from 82396ca to 51b133d Compare June 25, 2026 04:34

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 5

♻️ Duplicate comments (3)
src/main/java/com/codeit/team5/mopl/binarycontent/listener/BinaryContentUploadListener.java (1)

25-32: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

저장 성공 후 상태 갱신이 실패하면 FAILED로 덮어써져 파일과 DB 상태가 어긋납니다.

store(...)updateUploadStatus(COMPLETED)가 같은 try에 묶여 있어서, 파일 저장은 이미 성공했는데 updateUploadStatus(COMPLETED)만 실패(예: 일시적 DB 오류)하면 catch가 FAILED를 기록합니다.

  • 왜 문제인가: 실제로는 파일이 존재하는데 상태는 FAILED로 남아, 이후 재시도/정리 로직이 "없는 파일"로 오판해 중복 업로드나 잘못된 삭제를 유발할 수 있습니다. 즉 두 단계(외부 저장 / 상태 전이)의 실패 의미가 뒤섞입니다.
  • 개선 방향: 저장 단계와 상태 갱신 단계를 분리하세요. 저장 실패만 FAILED로 처리하고 return한 뒤, 저장 성공 경로에서 COMPLETED를 갱신합니다. 이렇게 하면 catch가 잡는 실패는 "저장 실패"로 한정됩니다.
🔧 제안 수정
     public void handle(BinaryContentUploadEvent event) {
         try {
             binaryContentStorage.store(event.key(), event.bytes());
-            binaryContentService.updateUploadStatus(event.binaryContentId(), BinaryContentUploadStatus.COMPLETED);
-            log.debug("파일 업로드 완료 - binaryContentId: {}", event.binaryContentId());
         } catch (Exception e) {
             binaryContentService.updateUploadStatus(event.binaryContentId(), BinaryContentUploadStatus.FAILED);
             log.error("파일 업로드 실패 - binaryContentId: {}", event.binaryContentId(), e);
+            return;
         }
+
+        binaryContentService.updateUploadStatus(event.binaryContentId(), BinaryContentUploadStatus.COMPLETED);
+        log.debug("파일 업로드 완료 - binaryContentId: {}", event.binaryContentId());
     }
🤖 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/com/codeit/team5/mopl/binarycontent/listener/BinaryContentUploadListener.java`
around lines 25 - 32, Separate the file storage and upload-status update steps
in BinaryContentUploadListener so only a failure in
binaryContentStorage.store(...) sets BinaryContentUploadStatus.FAILED, while a
failure in binaryContentService.updateUploadStatus(..., COMPLETED) does not
overwrite an already successful upload. Keep the try/catch around the store
path, return immediately after marking FAILED on storage errors, and move the
COMPLETED status update into the success path after store(...) in
BinaryContentUploadListener.handleEvent (or the equivalent listener method) so
the catch block no longer conflates DB update failures with upload failures.
src/main/java/com/codeit/team5/mopl/content/service/ContentService.java (2)

51-61: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

바이트를 읽기 전에 BinaryContent를 저장·연결해서, 읽기 실패 시 영구 PENDING 레코드가 남습니다.

현재 순서는 generateKeyBinaryContent.pending 저장 → attachThumbnail → 그다음에 thumbnail.getBytes()입니다. 문제는 getBytes()IOException을 던질 때입니다.

  • IOException은 catch에서 경고 로그만 남기고 재던지지 않으므로 트랜잭션은 정상 커밋됩니다.
  • 그 결과 PENDING 상태의 BinaryContent가 저장되고 content에 연결되지만, 이벤트가 발행되지 않아 업로드도 COMPLETED/FAILED 전이도 일어나지 않습니다.
  • 즉 API 응답에는 실제 파일이 절대 존재하지 않을 URL이 PENDING으로 영구히 남고, 리스너가 정리할 기회조차 없습니다.

가장 단순하고 부작용이 적은 해법은 바이트를 먼저 읽고, 성공한 뒤에만 저장·연결·발행하는 것입니다. 그러면 읽기 실패 시 어떤 BinaryContent도 만들어지지 않아 썸네일 없는 상태로 깔끔하게 귀결됩니다. (대안으로 catch에서 BinaryContentFAILED로 돌리는 방법도 있으나, 준비 단계 실패와 업로드 단계 실패가 섞여 의미가 흐려지므로 순서 교정이 더 명확합니다.)

🔧 제안 수정
         if (thumbnail != null && !thumbnail.isEmpty()) {
             try {
+                byte[] bytes = thumbnail.getBytes();
                 String key = binaryContentStorage.generateKey(content.getId(), thumbnail.getOriginalFilename());
                 BinaryContent binaryContent = binaryContentRepository.save(
                         BinaryContent.pending(binaryContentStorage.toUrl(key)));
                 content.attachThumbnail(binaryContent);
-                eventPublisher.publishEvent(new BinaryContentUploadEvent(binaryContent.getId(), key, thumbnail.getBytes()));
+                eventPublisher.publishEvent(new BinaryContentUploadEvent(binaryContent.getId(), key, bytes));
             } catch (IOException e) {
                 log.warn("썸네일 바이트 읽기 실패 - contentId: {}", content.getId(), e);
             }
         }
🤖 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/com/codeit/team5/mopl/content/service/ContentService.java`
around lines 51 - 61, The thumbnail handling in ContentService#save (the block
using generateKey, BinaryContent.pending, attachThumbnail, and
eventPublisher.publishEvent) saves and links a PENDING BinaryContent before
thumbnail.getBytes() succeeds, so an IOException can leave a permanent dangling
record. Read the bytes first, and only after that succeeds create the key,
persist BinaryContent.pending, attach it to content, and publish
BinaryContentUploadEvent. Keep the existing IOException logging, but ensure no
BinaryContent is saved or attached when byte reading fails.

54-57: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoff

BinaryContent 생성·저장 책임이 ContentService로 흘러 들어오고 있습니다.

ContentServicebinaryContentRepository.save(BinaryContent.pending(...))BinaryContent의 라이프사이클을 직접 통제하고 있습니다. 이미 리뷰에서 지적된 것처럼, 엔티티 생성·CRUD는 해당 도메인의 서비스(BinaryContentService)가 단일 지점에서 책임지는 편이 계층 분리 원칙에 부합합니다.

  • 현 구조의 단점: BinaryContent 영속화 로직이 두 곳(여기 + BinaryContentService.updateUploadStatus)으로 분산돼, 상태 규칙이 바뀔 때 누락 위험이 커집니다.
  • 개선 방향: BinaryContentServicecreatePending(url) 같은 생성 메서드를 두고 ContentService는 이를 호출만 하도록 위임하면, ContentService는 "콘텐츠 + 썸네일 연결 + 이벤트 발행"이라는 오케스트레이션에만 집중할 수 있습니다.

핵심 흐름을 바꾸는 변경은 아니니 후속 정리로 가져가도 무방합니다.

🤖 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/com/codeit/team5/mopl/content/service/ContentService.java`
around lines 54 - 57, `ContentService` is directly creating and saving
`BinaryContent`, which should be owned by `BinaryContentService`. Move the
pending-entity creation and repository save into a dedicated
`BinaryContentService` method such as `createPending(url)`, and have
`ContentService` call that helper instead of
`binaryContentRepository.save(BinaryContent.pending(...))`. Keep
`ContentService` focused on attaching the thumbnail and publishing the
`BinaryContentUploadEvent`, and align the new flow with the existing
`BinaryContentService.updateUploadStatus` lifecycle handling.
🤖 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/com/codeit/team5/mopl/binarycontent/BinaryContentStorage.java`:
- Around line 13-15: In BinaryContentStorage, the extension normalization in the
logic that builds normalizedExt is using extension.toLowerCase() with the
default locale, which can break ALLOWED_EXTENSIONS matching in locale-sensitive
environments. Update the normalization to use Locale.ROOT and reuse a single
lowercased value (for example in a lowerExt variable) for both the whitelist
check and the "." + extension result so the comparison and key generation stay
consistent.

In `@src/main/java/com/codeit/team5/mopl/binarycontent/entity/BinaryContent.java`:
- Around line 26-35: The BinaryContent pending and updateUploadStatus methods
currently accept null values that only fail later against the nullable=false
constraint. Add immediate null validation in BinaryContent.pending(String url)
and BinaryContent.updateUploadStatus(BinaryContentUploadStatus status), so
invalid input is rejected at the entity boundary with clear failures before
persistence.

In `@src/main/java/com/codeit/team5/mopl/content/entity/Content.java`:
- Around line 70-72: `Content.stats` is modeled as a one-to-one relation but
`stats_id` is not uniquely enforced in either the `Content` entity or the
`V7__restructure_content_stats.sql` migration. Update the `Content` mapping on
the `stats` field to make the `@JoinColumn(name = "stats_id")` unique, and
adjust the `contents.stats_id` schema in the migration to add a UNIQUE
constraint so the JPA model and database stay aligned.

In `@src/main/java/com/codeit/team5/mopl/global/async/MdcTaskDecorator.java`:
- Around line 13-22: `MdcTaskDecorator.decorate` currently always calls
`MDC.clear()` in `finally`, which is only safe as long as the task always runs
on a pooled worker thread. Make the decorator defensive by capturing the current
MDC context before `runnable.run()` and restoring that original context in
`finally` instead of unconditionally clearing it, so `MdcTaskDecorator`
preserves the caller thread’s MDC if execution ever happens under a policy like
`CallerRunsPolicy`.

In `@src/main/resources/application.yml`:
- Line 27: The base application.yml lost the default mopl.storage.type value,
which can prevent the conditional BinaryContentStorage bean from being created
in deployments that do not override it. Restore the default storage type in the
base configuration and leave environment/profile-specific overrides in their
respective files so the storage-related startup path remains valid.

---

Duplicate comments:
In
`@src/main/java/com/codeit/team5/mopl/binarycontent/listener/BinaryContentUploadListener.java`:
- Around line 25-32: Separate the file storage and upload-status update steps in
BinaryContentUploadListener so only a failure in binaryContentStorage.store(...)
sets BinaryContentUploadStatus.FAILED, while a failure in
binaryContentService.updateUploadStatus(..., COMPLETED) does not overwrite an
already successful upload. Keep the try/catch around the store path, return
immediately after marking FAILED on storage errors, and move the COMPLETED
status update into the success path after store(...) in
BinaryContentUploadListener.handleEvent (or the equivalent listener method) so
the catch block no longer conflates DB update failures with upload failures.

In `@src/main/java/com/codeit/team5/mopl/content/service/ContentService.java`:
- Around line 51-61: The thumbnail handling in ContentService#save (the block
using generateKey, BinaryContent.pending, attachThumbnail, and
eventPublisher.publishEvent) saves and links a PENDING BinaryContent before
thumbnail.getBytes() succeeds, so an IOException can leave a permanent dangling
record. Read the bytes first, and only after that succeeds create the key,
persist BinaryContent.pending, attach it to content, and publish
BinaryContentUploadEvent. Keep the existing IOException logging, but ensure no
BinaryContent is saved or attached when byte reading fails.
- Around line 54-57: `ContentService` is directly creating and saving
`BinaryContent`, which should be owned by `BinaryContentService`. Move the
pending-entity creation and repository save into a dedicated
`BinaryContentService` method such as `createPending(url)`, and have
`ContentService` call that helper instead of
`binaryContentRepository.save(BinaryContent.pending(...))`. Keep
`ContentService` focused on attaching the thumbnail and publishing the
`BinaryContentUploadEvent`, and align the new flow with the existing
`BinaryContentService.updateUploadStatus` lifecycle handling.
🪄 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: ASSERTIVE

Plan: Pro Plus

Run ID: 6c9ed10d-2c0e-4e76-984e-c82f8e32244f

📥 Commits

Reviewing files that changed from the base of the PR and between 82396ca and 51b133d.

📒 Files selected for processing (44)
  • .gitignore
  • src/main/java/com/codeit/team5/mopl/binarycontent/BinaryContentStorage.java
  • src/main/java/com/codeit/team5/mopl/binarycontent/entity/BinaryContent.java
  • src/main/java/com/codeit/team5/mopl/binarycontent/entity/BinaryContentUploadStatus.java
  • src/main/java/com/codeit/team5/mopl/binarycontent/event/BinaryContentUploadEvent.java
  • src/main/java/com/codeit/team5/mopl/binarycontent/exception/BinaryContentNotFoundException.java
  • src/main/java/com/codeit/team5/mopl/binarycontent/exception/BinaryContentStorageException.java
  • src/main/java/com/codeit/team5/mopl/binarycontent/exception/FileStorageException.java
  • src/main/java/com/codeit/team5/mopl/binarycontent/exception/UploadDirectoryInitException.java
  • src/main/java/com/codeit/team5/mopl/binarycontent/listener/BinaryContentUploadListener.java
  • src/main/java/com/codeit/team5/mopl/binarycontent/repository/BinaryContentRepository.java
  • src/main/java/com/codeit/team5/mopl/binarycontent/service/BinaryContentService.java
  • src/main/java/com/codeit/team5/mopl/binarycontent/storage/local/LocalBinaryContentStorage.java
  • src/main/java/com/codeit/team5/mopl/binarycontent/storage/local/LocalStorageProperties.java
  • src/main/java/com/codeit/team5/mopl/binarycontent/storage/s3/S3BinaryContentStorage.java
  • src/main/java/com/codeit/team5/mopl/binarycontent/storage/s3/S3StorageProperties.java
  • src/main/java/com/codeit/team5/mopl/config/AsyncConfig.java
  • src/main/java/com/codeit/team5/mopl/config/StorageConfig.java
  • src/main/java/com/codeit/team5/mopl/content/dto/response/ContentResponse.java
  • src/main/java/com/codeit/team5/mopl/content/entity/Content.java
  • src/main/java/com/codeit/team5/mopl/content/entity/ContentStats.java
  • src/main/java/com/codeit/team5/mopl/content/entity/ContentTag.java
  • src/main/java/com/codeit/team5/mopl/content/mapper/ContentMapper.java
  • src/main/java/com/codeit/team5/mopl/content/repository/ContentRepository.java
  • src/main/java/com/codeit/team5/mopl/content/service/ContentService.java
  • src/main/java/com/codeit/team5/mopl/global/async/CompositeTaskDecorator.java
  • src/main/java/com/codeit/team5/mopl/global/async/MdcTaskDecorator.java
  • src/main/java/com/codeit/team5/mopl/global/async/SecurityContextTaskDecorator.java
  • src/main/java/com/codeit/team5/mopl/user/entity/User.java
  • src/main/java/com/codeit/team5/mopl/user/mapper/UserMapper.java
  • src/main/java/com/codeit/team5/mopl/watcher/repository/WatchingSessionRepository.java
  • src/main/resources/application-dev.yml
  • src/main/resources/application.yml
  • src/main/resources/db/migration/V5__add_thumbnail_upload_status.sql
  • src/main/resources/db/migration/V6__add_binary_contents.sql
  • src/main/resources/db/migration/V7__restructure_content_stats.sql
  • src/main/resources/db/migration/V8__add_profile_image_to_users.sql
  • src/main/resources/db/migration/V9__add_unique_constraints_to_binary_content_fks.sql
  • src/test/java/com/codeit/team5/mopl/binarycontent/listener/BinaryContentUploadListenerTest.java
  • src/test/java/com/codeit/team5/mopl/binarycontent/service/BinaryContentServiceTest.java
  • src/test/java/com/codeit/team5/mopl/binarycontent/storage/local/LocalBinaryContentStorageTest.java
  • src/test/java/com/codeit/team5/mopl/content/controller/ContentControllerTest.java
  • src/test/java/com/codeit/team5/mopl/content/service/ContentServiceTest.java
  • src/test/resources/application-test.yml

Comment thread src/main/java/com/codeit/team5/mopl/binarycontent/BinaryContentStorage.java Outdated
Comment thread src/main/java/com/codeit/team5/mopl/content/entity/Content.java
Comment thread src/main/resources/application.yml
- BinaryContentStorage: 확장자 정규화 고정
- BinaryContent: pending/updateUploadStatus null 검증 추가
- Content: stats_id UNIQUE 제약 추가
@plzslp plzslp requested a review from metDaisy June 25, 2026 05:06
Comment thread src/main/java/com/codeit/team5/mopl/binarycontent/entity/BinaryContent.java Outdated
Comment thread src/main/java/com/codeit/team5/mopl/content/entity/Content.java Outdated
Comment thread src/main/java/com/codeit/team5/mopl/content/mapper/ContentMapper.java Outdated
metDaisy
metDaisy previously approved these changes Jun 25, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Feat] BinaryContent 테이블 추가 [Feat] 로컬 파일 업로드 로직 작성

2 participants