refactor: 프로필/썸네일 이미지 업로드를 동기 방식으로 전환#230
Merged
Merged
Conversation
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (20)
💤 Files with no reviewable changes (6)
📝 WalkthroughWalkthrough이미지 업로드 처리를 비동기 이벤트 방식( Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/test/java/com/codeit/team5/mopl/content/controller/ContentControllerTest.java (1)
114-124: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win제거된 응답 필드는 부재까지 검증하세요.
현재는
thumbnailUploadStatus검증을 제거해서 생성자/응답 형태는 맞지만, 필드가 실수로 다시 직렬화되어도 테스트가 통과합니다. PR 목표가 응답 계약에서 제거하는 것이라면 주요 응답 경로에doesNotExist()를 추가해 회귀를 잡는 편이 안전합니다.🧪 예시 보강
- .andExpect(jsonPath("$.watcherCount").value(0)); + .andExpect(jsonPath("$.watcherCount").value(0)) + .andExpect(jsonPath("$.thumbnailUploadStatus").doesNotExist());목록 응답은
$.data[0].thumbnailUploadStatus로 같은 검증을 추가하면 됩니다.Also applies to: 320-332, 539-550, 612-618
🤖 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/controller/ContentControllerTest.java` around lines 114 - 124, In the ContentControllerTest response assertions, add negative coverage for the removed thumbnailUploadStatus field so the contract change is enforced even if serialization regresses. Update the main create/content response checks around the existing jsonPath assertions to verify that thumbnailUploadStatus does not exist, and apply the same doesNotExist() check in the other affected response tests that already reference the ContentControllerTest response payloads, especially the list and detail paths tied to the same DTO shape.
🤖 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/content/service/ContentService.java`:
- Around line 58-59: 썸네일 업로드가 attachTags()의 태그 검증보다 먼저 실행되어, 이후
normalizeTagNames()에서 예외가 나면 DB는 롤백돼도 스토리지에는 orphan 파일이 남습니다.
ContentService.create()에서 태그 정규화/검증을 먼저 끝내고, 그 다음에
binaryContentService.upload()와 content.attachThumbnail()를 수행하도록 순서를 바꾸세요. 핵심 변경
지점은 create(), attachTags(), normalizeTagNames() 흐름입니다.
In `@src/main/java/com/codeit/team5/mopl/user/entity/User.java`:
- Around line 71-75: 프로필 이미지 교체 시 기존 BinaryContent가 DELETED로 전환되는 분기가 테스트로 고정되지
않았습니다. User.updateProfileImage와 이를 호출하는 UserServiceTest 경로를 기준으로, 기존
profileImage가 있는 사용자에게 새 이미지를 적용할 때 이전 이미지의 upload status가
BinaryContentUploadStatus.DELETED로 변경되는 회귀 테스트를 추가하세요. 신규 업로드 성공/실패 테스트와 별개로,
ContentServiceTest.update_withNewThumbnail_oldThumbnailMarkedDeleted()처럼 기존 이미지
상태 변경까지 검증하는 케이스를 넣어 주세요.
In `@src/main/java/com/codeit/team5/mopl/user/service/UserService.java`:
- Line 78: The profile image upload in UserService.updateProfileImage currently
persists to external storage before the database transaction is safely
committed, which can leave orphaned files if later flush/commit fails. Fix this
by either registering a rollback compensation to delete the uploaded object when
the transaction fails, or by changing the flow to use a temporary upload key and
only finalize the object after commit. Keep the change localized around
UserService.updateProfileImage and the
binaryContentService.upload(StorageDirectory.PROFILE, user.getId(), image) call.
In
`@src/main/resources/db/migration/V12__reduce_binary_content_upload_status.sql`:
- Around line 1-10: The backfill in V12__reduce_binary_content_upload_status.sql
incorrectly upgrades all binary_contents rows with upload_status in PENDING or
FAILED to COMPLETED, which can preserve broken uploads as valid data. Update
this migration so the data cleanup in the binary_contents table either verifies
the stored object exists before marking it COMPLETED, or more conservatively
clears those references and marks them DELETED instead; then keep the
ck_binary_contents_upload_status constraint aligned with the final allowed
statuses. Use the existing binary_contents.upload_status update and
constraint-alter statements as the place to adjust this logic.
In
`@src/test/java/com/codeit/team5/mopl/binarycontent/service/BinaryContentServiceTest.java`:
- Around line 54-63: 성공 케이스 테스트에서 `BinaryContentService.upload(...)`가
`binaryContentRepository.save(...)`를 실제로 호출하는지 검증이 빠져 있습니다.
`BinaryContentServiceTest`의 성공 케이스에 `save()` 호출 검증을 추가하고, 가능하면 `ArgumentCaptor`
또는 `argThat`를 사용해 저장된 `BinaryContent`의 URL과 `COMPLETED` 상태까지 함께 확인하세요. 특히
`binaryContentService.upload`, `binaryContentRepository.save`,
`binaryContentStorage.store`를 기준으로 해당 계약이 지켜지는지 테스트를 보강하면 됩니다.
In `@src/test/java/com/codeit/team5/mopl/user/service/UserServiceTest.java`:
- Around line 226-242: The current UserServiceTest case only proves that
UserService.update throws and leaves the in-memory User unchanged, but it does
not verify transactional rollback because this is a Mockito unit test without
the `@Transactional` proxy. Update the test around UserService.update,
userRepository.findWithProfileImageById, and binaryContentService.upload so it
either becomes a Spring integration test that reloads the User after the failure
and asserts both the name and profileImage were rolled back, or else
rename/retarget the test to only assert exception propagation.
---
Outside diff comments:
In
`@src/test/java/com/codeit/team5/mopl/content/controller/ContentControllerTest.java`:
- Around line 114-124: In the ContentControllerTest response assertions, add
negative coverage for the removed thumbnailUploadStatus field so the contract
change is enforced even if serialization regresses. Update the main
create/content response checks around the existing jsonPath assertions to verify
that thumbnailUploadStatus does not exist, and apply the same doesNotExist()
check in the other affected response tests that already reference the
ContentControllerTest response payloads, especially the list and detail paths
tied to the same DTO shape.
🪄 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: e18fa6fc-e97b-4b90-9494-34569c47a010
📒 Files selected for processing (19)
src/main/java/com/codeit/team5/mopl/binarycontent/entity/BinaryContent.javasrc/main/java/com/codeit/team5/mopl/binarycontent/entity/BinaryContentUploadStatus.javasrc/main/java/com/codeit/team5/mopl/binarycontent/event/BinaryContentUploadEvent.javasrc/main/java/com/codeit/team5/mopl/binarycontent/listener/BinaryContentUploadListener.javasrc/main/java/com/codeit/team5/mopl/binarycontent/service/BinaryContentService.javasrc/main/java/com/codeit/team5/mopl/content/dto/response/ContentResponse.javasrc/main/java/com/codeit/team5/mopl/content/entity/Content.javasrc/main/java/com/codeit/team5/mopl/content/mapper/ContentMapper.javasrc/main/java/com/codeit/team5/mopl/content/service/ContentService.javasrc/main/java/com/codeit/team5/mopl/user/entity/User.javasrc/main/java/com/codeit/team5/mopl/user/repository/UserRepository.javasrc/main/java/com/codeit/team5/mopl/user/service/UserService.javasrc/main/resources/db/migration/V12__reduce_binary_content_upload_status.sqlsrc/test/java/com/codeit/team5/mopl/binarycontent/listener/BinaryContentUploadListenerTest.javasrc/test/java/com/codeit/team5/mopl/binarycontent/service/BinaryContentServiceTest.javasrc/test/java/com/codeit/team5/mopl/content/controller/ContentControllerTest.javasrc/test/java/com/codeit/team5/mopl/content/repository/ContentRepositoryTest.javasrc/test/java/com/codeit/team5/mopl/content/service/ContentServiceTest.javasrc/test/java/com/codeit/team5/mopl/user/service/UserServiceTest.java
💤 Files with no reviewable changes (6)
- src/main/java/com/codeit/team5/mopl/binarycontent/entity/BinaryContentUploadStatus.java
- src/test/java/com/codeit/team5/mopl/binarycontent/listener/BinaryContentUploadListenerTest.java
- src/main/java/com/codeit/team5/mopl/binarycontent/event/BinaryContentUploadEvent.java
- src/main/java/com/codeit/team5/mopl/content/mapper/ContentMapper.java
- src/main/java/com/codeit/team5/mopl/content/dto/response/ContentResponse.java
- src/main/java/com/codeit/team5/mopl/binarycontent/listener/BinaryContentUploadListener.java
There was a problem hiding this comment.
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/test/java/com/codeit/team5/mopl/user/service/UserServiceTest.java (1)
205-214: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win이미지 관련 테스트에 실제
userId를 주입해ownerId전달을 검증하세요.
User.create(...)는id를 채우지 않는데, 여기서는eq(user.getId())로 stubbing/verify를 걸어서 결국binaryContentService.upload(..., null, ...)를 기대하게 됩니다. 그래서 이 테스트들은 “업로드 키 생성에 대상 사용자 ID를 넘긴다”는 실제 계약을 충분히 못 잡습니다.ReflectionTestUtils.setField(user, "id", userId)로 ID를 넣고eq(userId)로 맞추는 쪽이 더 정확합니다.예시 수정
UUID userId = UUID.randomUUID(); User user = User.create("user@example.com", "encoded-password", "기존이름"); + ReflectionTestUtils.setField(user, "id", userId); FileRequest image = new FileRequest(new byte[]{1, 2, 3}, "profile.jpg"); ... - when(binaryContentService.upload(eq(StorageDirectory.PROFILE), eq(user.getId()), any())) + when(binaryContentService.upload(eq(StorageDirectory.PROFILE), eq(userId), any())) .thenReturn(BinaryContent.completed("http://localhost/profiles/key.jpg")); ... - verify(binaryContentService).upload(eq(StorageDirectory.PROFILE), eq(user.getId()), any()); + verify(binaryContentService).upload(eq(StorageDirectory.PROFILE), eq(userId), any());Also applies to: 224-224, 231-236, 260-261
🤖 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/user/service/UserServiceTest.java` around lines 205 - 214, The image-related tests are stubbing and verifying binaryContentService.upload with user.getId(), but User.create(...) leaves the id null so the test is not actually asserting the intended ownerId contract. In UserServiceTest, set the created User’s id to the generated userId before using it (for example via ReflectionTestUtils.setField on user) and update all upload stubs/verifications to use eq(userId) instead of eq(user.getId()). Apply the same change to the related image upload test cases so they consistently verify that the target user ID is passed into binaryContentService.upload from the service methods.
🤖 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/content/service/ContentService.java`:
- Around line 60-62: Move the thumbnail upload in ContentService so it happens
only after ContentStats has been successfully created and saved, using the
ContentService flow around attachThumbnail and contentStatsRepository.save. The
current order uploads via binaryContentService.upload before the DB write/flush,
which can leave an orphaned file if the transaction rolls back. Reorder the
logic so the ContentStats linkage is completed first, or alternatively add
rollback compensation to delete the uploaded thumbnail if the save fails.
In
`@src/test/java/com/codeit/team5/mopl/user/service/UserServiceImageRollbackIntegrationTest.java`:
- Around line 47-61: The integration test only covers a user with no existing
profile image, so it misses the rollback contract for image replacement
failures. Update UserServiceImageRollbackIntegrationTest to seed a user that
already has a BinaryContent.completed(...) profile image, then trigger the
update failure through UserService.update and verify both the name and the
existing profile image URL remain unchanged after the exception. Use the
existing helpers and assertions around userRepository, findWithProfileImageById,
and binaryContentStorage to keep the failure case fixed against regressions.
---
Outside diff comments:
In `@src/test/java/com/codeit/team5/mopl/user/service/UserServiceTest.java`:
- Around line 205-214: The image-related tests are stubbing and verifying
binaryContentService.upload with user.getId(), but User.create(...) leaves the
id null so the test is not actually asserting the intended ownerId contract. In
UserServiceTest, set the created User’s id to the generated userId before using
it (for example via ReflectionTestUtils.setField on user) and update all upload
stubs/verifications to use eq(userId) instead of eq(user.getId()). Apply the
same change to the related image upload test cases so they consistently verify
that the target user ID is passed into binaryContentService.upload from the
service methods.
🪄 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: d6055777-13d3-4aa2-b516-3e8ce3c58591
📒 Files selected for processing (4)
src/main/java/com/codeit/team5/mopl/content/service/ContentService.javasrc/test/java/com/codeit/team5/mopl/binarycontent/service/BinaryContentServiceTest.javasrc/test/java/com/codeit/team5/mopl/user/service/UserServiceImageRollbackIntegrationTest.javasrc/test/java/com/codeit/team5/mopl/user/service/UserServiceTest.java
# Conflicts: # src/main/java/com/codeit/team5/mopl/content/service/ContentService.java
dstle
force-pushed
the
feature/152-image-upload-sync
branch
from
40d93a8 to
dd11409
Compare
plzslp
previously approved these changes
Jun 30, 2026
plzslp
left a comment
plzslp
left a comment
Collaborator
There was a problem hiding this comment.
schema.sql 파일 V12를 제가 V12__mapsid_content_stats.sql로 사용해서 V13으로 바꿔야할 것 같습니다.
다른 변경사항은 BinaryConent 업로드 로직이 동기 방식으로 잘 수정되어 있고, 프로필, 썸네일 수정시에 기존 BInaryConent 상태 변경 로직도 개별 엔티티로 잘 옮겨진 걸 확인했습니다.
아마 이전 PR인 콘텐츠 수집부분에서 Conflict가 생길 것 같은데 혹시라도 문제있는 부분은 말해주세요. ContentService 쪽은 주석 외에는 거의 건들지 않았기에 이미지 업로드 동기 방식 전환 PR 기준으로 수정하시면 될 것 같습니다.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/service/BinaryContentService.java`:
- Around line 23-28: `BinaryContentService.upload()` is doing external storage
I/O inside a `@Transactional` method, which can leave the uploaded object
orphaned if `binaryContentRepository.save(...)` or the surrounding transaction
fails. Refactor the `upload(...)` flow so `binaryContentStorage.store(...)` is
not executed under the database transaction, or add a compensating cleanup step
for rollback/after-completion using the `BinaryContentStorage` and
`BinaryContentRepository` integration points. Keep the transaction limited to
persisting the metadata after the file upload succeeds.
In `@src/main/java/com/codeit/team5/mopl/content/service/ContentService.java`:
- Around line 36-41: Update the class Javadoc on ContentService to match the new
thumbnail upload behavior: it currently says thumbnails are uploaded to S3
asynchronously via event-based processing, but this PR changes that flow to
synchronous handling. Rewrite the affected description so it accurately reflects
the synchronous upload path and no longer mentions event-based async behavior.
In `@src/test/java/com/codeit/team5/mopl/content/service/ContentServiceTest.java`:
- Around line 71-72: `ContentServiceTest` still declares an `eventPublisher`
mock and verifies it with `verifyNoInteractions`, but `ContentService` no longer
injects `ApplicationEventPublisher`, so this is dead setup/verification. Remove
the `eventPublisher` mock field and drop it from the `verifyNoInteractions`
call, keeping only the dependencies that `ContentService` actually uses in the
test.
🪄 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: d2019b42-90ff-491e-87d1-4823d39b4da4
📒 Files selected for processing (20)
src/main/java/com/codeit/team5/mopl/binarycontent/entity/BinaryContent.javasrc/main/java/com/codeit/team5/mopl/binarycontent/entity/BinaryContentUploadStatus.javasrc/main/java/com/codeit/team5/mopl/binarycontent/event/BinaryContentUploadEvent.javasrc/main/java/com/codeit/team5/mopl/binarycontent/listener/BinaryContentUploadListener.javasrc/main/java/com/codeit/team5/mopl/binarycontent/service/BinaryContentService.javasrc/main/java/com/codeit/team5/mopl/content/dto/response/ContentResponse.javasrc/main/java/com/codeit/team5/mopl/content/entity/Content.javasrc/main/java/com/codeit/team5/mopl/content/mapper/ContentMapper.javasrc/main/java/com/codeit/team5/mopl/content/service/ContentService.javasrc/main/java/com/codeit/team5/mopl/user/entity/User.javasrc/main/java/com/codeit/team5/mopl/user/repository/UserRepository.javasrc/main/java/com/codeit/team5/mopl/user/service/UserService.javasrc/main/resources/db/migration/V13__reduce_binary_content_upload_status.sqlsrc/test/java/com/codeit/team5/mopl/binarycontent/listener/BinaryContentUploadListenerTest.javasrc/test/java/com/codeit/team5/mopl/binarycontent/service/BinaryContentServiceTest.javasrc/test/java/com/codeit/team5/mopl/content/controller/ContentControllerTest.javasrc/test/java/com/codeit/team5/mopl/content/repository/ContentRepositoryTest.javasrc/test/java/com/codeit/team5/mopl/content/service/ContentServiceTest.javasrc/test/java/com/codeit/team5/mopl/user/service/UserServiceImageRollbackIntegrationTest.javasrc/test/java/com/codeit/team5/mopl/user/service/UserServiceTest.java
💤 Files with no reviewable changes (6)
- 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/listener/BinaryContentUploadListener.java
- src/main/java/com/codeit/team5/mopl/content/mapper/ContentMapper.java
- src/test/java/com/codeit/team5/mopl/binarycontent/listener/BinaryContentUploadListenerTest.java
- src/main/java/com/codeit/team5/mopl/content/dto/response/ContentResponse.java
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 3
🤖 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/service/BinaryContentService.java`:
- Around line 23-28: `BinaryContentService.upload()` is doing external storage
I/O inside a `@Transactional` method, which can leave the uploaded object
orphaned if `binaryContentRepository.save(...)` or the surrounding transaction
fails. Refactor the `upload(...)` flow so `binaryContentStorage.store(...)` is
not executed under the database transaction, or add a compensating cleanup step
for rollback/after-completion using the `BinaryContentStorage` and
`BinaryContentRepository` integration points. Keep the transaction limited to
persisting the metadata after the file upload succeeds.
In `@src/main/java/com/codeit/team5/mopl/content/service/ContentService.java`:
- Around line 36-41: Update the class Javadoc on ContentService to match the new
thumbnail upload behavior: it currently says thumbnails are uploaded to S3
asynchronously via event-based processing, but this PR changes that flow to
synchronous handling. Rewrite the affected description so it accurately reflects
the synchronous upload path and no longer mentions event-based async behavior.
In `@src/test/java/com/codeit/team5/mopl/content/service/ContentServiceTest.java`:
- Around line 71-72: `ContentServiceTest` still declares an `eventPublisher`
mock and verifies it with `verifyNoInteractions`, but `ContentService` no longer
injects `ApplicationEventPublisher`, so this is dead setup/verification. Remove
the `eventPublisher` mock field and drop it from the `verifyNoInteractions`
call, keeping only the dependencies that `ContentService` actually uses in the
test.
🪄 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: d2019b42-90ff-491e-87d1-4823d39b4da4
📒 Files selected for processing (20)
src/main/java/com/codeit/team5/mopl/binarycontent/entity/BinaryContent.javasrc/main/java/com/codeit/team5/mopl/binarycontent/entity/BinaryContentUploadStatus.javasrc/main/java/com/codeit/team5/mopl/binarycontent/event/BinaryContentUploadEvent.javasrc/main/java/com/codeit/team5/mopl/binarycontent/listener/BinaryContentUploadListener.javasrc/main/java/com/codeit/team5/mopl/binarycontent/service/BinaryContentService.javasrc/main/java/com/codeit/team5/mopl/content/dto/response/ContentResponse.javasrc/main/java/com/codeit/team5/mopl/content/entity/Content.javasrc/main/java/com/codeit/team5/mopl/content/mapper/ContentMapper.javasrc/main/java/com/codeit/team5/mopl/content/service/ContentService.javasrc/main/java/com/codeit/team5/mopl/user/entity/User.javasrc/main/java/com/codeit/team5/mopl/user/repository/UserRepository.javasrc/main/java/com/codeit/team5/mopl/user/service/UserService.javasrc/main/resources/db/migration/V13__reduce_binary_content_upload_status.sqlsrc/test/java/com/codeit/team5/mopl/binarycontent/listener/BinaryContentUploadListenerTest.javasrc/test/java/com/codeit/team5/mopl/binarycontent/service/BinaryContentServiceTest.javasrc/test/java/com/codeit/team5/mopl/content/controller/ContentControllerTest.javasrc/test/java/com/codeit/team5/mopl/content/repository/ContentRepositoryTest.javasrc/test/java/com/codeit/team5/mopl/content/service/ContentServiceTest.javasrc/test/java/com/codeit/team5/mopl/user/service/UserServiceImageRollbackIntegrationTest.javasrc/test/java/com/codeit/team5/mopl/user/service/UserServiceTest.java
💤 Files with no reviewable changes (6)
- 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/listener/BinaryContentUploadListener.java
- src/main/java/com/codeit/team5/mopl/content/mapper/ContentMapper.java
- src/test/java/com/codeit/team5/mopl/binarycontent/listener/BinaryContentUploadListenerTest.java
- src/main/java/com/codeit/team5/mopl/content/dto/response/ContentResponse.java
🛑 Comments failed to post (3)
src/main/java/com/codeit/team5/mopl/binarycontent/service/BinaryContentService.java (1)
23-28: 🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # upload()를 호출하는 지점과 그 트랜잭션 경계 확인 rg -nP --type=java -C3 '\bbinaryContentService\.upload\s*\(' # 스토리지 store/delete 인터페이스에 보상 삭제 수단이 있는지 확인 ast-grep run --pattern 'interface BinaryContentStorage { $$$ }' --lang java $(fd -t f BinaryContentStorage.java)Repository: sb10-part4-team5/sb10-mopl-team5
Length of output: 170
🏁 Script executed:
#!/bin/bash set -euo pipefail # Locate the target file and related service/interface definitions. git ls-files | rg 'BinaryContentService\.java|BinaryContentStorage\.java|ContentService\.java|UserService\.java' # Show compact structure for the target service and storage interface. for f in $(git ls-files | rg 'BinaryContentService\.java|BinaryContentStorage\.java'); do echo "### $f" ast-grep outline "$f" --view expanded || true done # Find all call sites of upload(). rg -n --type=java 'binaryContentService\.upload\s*\(' src/main/java # Show transactional annotations around callers if present. rg -n --type=java '`@Transactional`|binaryContentService\.upload\s*\(' src/main/java/com/codeit/team5/mopl -C 3Repository: sb10-part4-team5/sb10-mopl-team5
Length of output: 33854
🏁 Script executed:
#!/bin/bash set -euo pipefail # Read the relevant files in bounded slices. for f in src/main/java/com/codeit/team5/mopl/binarycontent/service/BinaryContentService.java \ src/main/java/com/codeit/team5/mopl/binarycontent/storage/BinaryContentStorage.java \ src/main/java/com/codeit/team5/mopl/content/service/ContentService.java \ src/main/java/com/codeit/team5/mopl/user/service/UserService.java; do if [ -f "$f" ]; then echo "### $f ($(wc -l < "$f") lines)" cat -n "$f" | sed -n '1,220p' fi doneRepository: sb10-part4-team5/sb10-mopl-team5
Length of output: 18368
🏁 Script executed:
#!/bin/bash set -euo pipefail # Check whether the storage abstraction supports cleanup/delete and how the S3/local impls behave. rg -n --type=java 'void delete\(|delete\(' src/main/java/com/codeit/team5/mopl/binarycontent/storage src/main/java/com/codeit/team5/mopl/binarycontent/service src/main/java/com/codeit/team5/mopl/binarycontent/entity -C 2 # Inspect the concrete storage implementations around store/toUrl methods. for f in src/main/java/com/codeit/team5/mopl/binarycontent/storage/s3/S3BinaryContentStorage.java \ src/main/java/com/codeit/team5/mopl/binarycontent/storage/local/LocalBinaryContentStorage.java; do echo "### $f ($(wc -l < "$f") lines)" cat -n "$f" | sed -n '1,220p' doneRepository: sb10-part4-team5/sb10-mopl-team5
Length of output: 170
🏁 Script executed:
#!/bin/bash set -euo pipefail # Inspect whether any async / after-commit mechanism already exists for binary content. rg -n --type=java 'TransactionPhase\.AFTER_COMMIT|TransactionSynchronization|`@TransactionalEventListener`|binaryContentStorage|BinaryContentUploadStatus' src/main/java/com/codeit/team5/mopl -C 2Repository: sb10-part4-team5/sb10-mopl-team5
Length of output: 12605
🏁 Script executed:
#!/bin/bash set -euo pipefail for f in src/main/java/com/codeit/team5/mopl/binarycontent/storage/s3/S3BinaryContentStorage.java \ src/main/java/com/codeit/team5/mopl/binarycontent/storage/local/LocalBinaryContentStorage.java; do echo "### $f ($(wc -l < "$f") lines)" cat -n "$f" | sed -n '1,220p' doneRepository: sb10-part4-team5/sb10-mopl-team5
Length of output: 4766
🏁 Script executed:
#!/bin/bash set -euo pipefail # Read the concrete storage implementations and any cleanup/delete support. for f in src/main/java/com/codeit/team5/mopl/binarycontent/storage/s3/S3BinaryContentStorage.java \ src/main/java/com/codeit/team5/mopl/binarycontent/storage/local/LocalBinaryContentStorage.java; do echo "### $f ($(wc -l < "$f") lines)" cat -n "$f" | sed -n '1,220p' done rg -n --type=java 'delete\(|remove\(' src/main/java/com/codeit/team5/mopl/binarycontent -C 2Repository: sb10-part4-team5/sb10-mopl-team5
Length of output: 4766
외부 스토리지 업로드는 트랜잭션 밖으로 분리하거나 보상 삭제를 붙여야 합니다.
BinaryContentService.upload()가@Transactional안에서 먼저store()를 호출해 S3/로컬 I/O 동안 DB 커넥션을 붙잡고, 이후save()나 상위 트랜잭션이 실패하면 업로드된 객체만 남습니다.BinaryContentStorage에는 삭제/롤백 처리 수단도 없어서 상태 불일치를 정리할 방법이 없습니다. 업로드를 분리해 메타데이터만 짧게 저장하거나,afterCompletion기반 삭제를 넣는 쪽이 안전합니다.🤖 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/service/BinaryContentService.java` around lines 23 - 28, `BinaryContentService.upload()` is doing external storage I/O inside a `@Transactional` method, which can leave the uploaded object orphaned if `binaryContentRepository.save(...)` or the surrounding transaction fails. Refactor the `upload(...)` flow so `binaryContentStorage.store(...)` is not executed under the database transaction, or add a compensating cleanup step for rollback/after-completion using the `BinaryContentStorage` and `BinaryContentRepository` integration points. Keep the transaction limited to persisting the metadata after the file upload succeeds.src/main/java/com/codeit/team5/mopl/content/service/ContentService.java (1)
36-41: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
클래스 Javadoc이 이번 변경과 모순됩니다 — "비동기 업로드(이벤트 기반)" 문구를 동기 처리로 갱신해 주세요.
이 PR의 본질이 썸네일 업로드를 이벤트 기반 비동기에서 동기 처리로 전환하는 것인데, 정작 클래스 설명에는 여전히 썸네일 이미지는 S3에 비동기 업로드(이벤트 기반)되며라고 적혀 있습니다. 코드와 문서가 어긋나면, 나중에 이 클래스를 읽는 사람이 실제로는 존재하지 않는 이벤트 리스너를 찾으러 다니게 되어 유지보수 비용이 늘어납니다. 문서는 코드만큼 자주 검증되지 않으니, 이런 동작 전환 시점에 함께 고쳐두는 게 안전합니다.
📝 제안 수정
- * <p>외부 API 수집 콘텐츠({`@link` TmdbContentService}, {`@link` SportsDbContentService})와 달리 - * 썸네일 이미지는 S3에 비동기 업로드(이벤트 기반)되며, 태그는 최대 10개 제한이 적용된다.</p> + * <p>외부 API 수집 콘텐츠({`@link` TmdbContentService}, {`@link` SportsDbContentService})와 달리 + * 썸네일 이미지는 {`@link` BinaryContentService#upload}를 통해 동기적으로 S3에 업로드되며, + * 업로드 실패 시 트랜잭션 롤백으로 기존 상태가 보존된다. 태그는 최대 10개 제한이 적용된다.</p>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements./** * 관리자가 직접 등록하는 콘텐츠의 CRUD를 담당하는 서비스. * * <p>외부 API 수집 콘텐츠({`@link` TmdbContentService}, {`@link` SportsDbContentService})와 달리 * 썸네일 이미지는 {`@link` BinaryContentService#upload}를 통해 동기적으로 S3에 업로드되며, * 업로드 실패 시 트랜잭션 롤백으로 기존 상태가 보존된다. 태그는 최대 10개 제한이 적용된다.</p> */🤖 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 36 - 41, Update the class Javadoc on ContentService to match the new thumbnail upload behavior: it currently says thumbnails are uploaded to S3 asynchronously via event-based processing, but this PR changes that flow to synchronous handling. Rewrite the affected description so it accurately reflects the synchronous upload path and no longer mentions event-based async behavior.src/test/java/com/codeit/team5/mopl/content/service/ContentServiceTest.java (1)
71-72: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # ContentService가 ApplicationEventPublisher에 의존하는지 확인 rg -nP 'ApplicationEventPublisher|eventPublisher|publishEvent' src/main/java/com/codeit/team5/mopl/content/service/ContentService.javaRepository: sb10-part4-team5/sb10-mopl-team5
Length of output: 170
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "== ContentService ==" fd -a 'ContentService.java' src/main/java src/test/java echo echo "== ContentServiceTest outline ==" ast-grep outline src/test/java/com/codeit/team5/mopl/content/service/ContentServiceTest.java --view expanded echo echo "== Relevant mentions in test ==" rg -n -C 2 'eventPublisher|verifyNoInteractions|verify\(' src/test/java/com/codeit/team5/mopl/content/service/ContentServiceTest.java echo echo "== Relevant mentions in service ==" rg -n -C 2 'ApplicationEventPublisher|eventPublisher|publishEvent' src/main/java/com/codeit/team5/mopl/content/service/ContentService.java || trueRepository: sb10-part4-team5/sb10-mopl-team5
Length of output: 5714
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "== ContentServiceTest top ==" sed -n '1,110p' src/test/java/com/codeit/team5/mopl/content/service/ContentServiceTest.java | cat -n echo echo "== ContentService top ==" sed -n '1,140p' src/main/java/com/codeit/team5/mopl/content/service/ContentService.java | cat -nRepository: sb10-part4-team5/sb10-mopl-team5
Length of output: 12166
eventPublishermock과 관련 검증을 제거하세요.
ContentService가ApplicationEventPublisher를 더 이상 주입받지 않아서, 이 mock은@InjectMocks대상이 아닙니다. 지금의verifyNoInteractions(..., eventPublisher)는 항상 통과하는 죽은 검증이라 테스트 의도를 흐립니다.eventPublisher필드와 해당 인자를verifyNoInteractions에서 빼고, 실제로 사용하는 의존성만 검증하도록 정리하는 편이 좋습니다.🤖 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 71 - 72, `ContentServiceTest` still declares an `eventPublisher` mock and verifies it with `verifyNoInteractions`, but `ContentService` no longer injects `ApplicationEventPublisher`, so this is dead setup/verification. Remove the `eventPublisher` mock field and drop it from the `verifyNoInteractions` call, keeping only the dependencies that `ContentService` actually uses in the test.
This was referenced Jul 3, 2026
Merged
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
2 participants
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.
관련 이슈
##작업내용
동기 전환
@Transactional롤백 → 기존 이미지 유지 + 에러 응답(사용자 재시도)BinaryContentUploadListener,BinaryContentUploadEvent삭제구조 정리
BinaryContentService.upload(...)신설 — user/content에 중복되던 업로드 로직 일원화User.updateProfileImage/Content.attachThumbnail)로 위임 → 서비스 중첩 if 제거BinaryContentUploadStatus→COMPLETED/DELETED2개로 축소 (flyway V12)ContentResponse.thumbnailUploadStatus제거 → Swagger 명세와 일치@EntityGraph→left join fetch테스트
BinaryContentServiceTest: 업로드 성공 / 실패 시 예외 전파·저장 안 함UserServiceTest.update_imageUploadFails: 업로드 실패 → 예외 + 프로필 미변경테스트 내용
체크리스트
리뷰 포인트
스크린샷 / 참고 자료
🔗 관련 이슈
#152📝 작업 내용
PENDING/FAILED/COMPLETED상태 전이를 별도 리스너가 처리하면서 흐름이 분산되어 있었는데, 이제는 업로드 성공 시에만COMPLETED가 저장되도록 바꿔 실패 시 트랜잭션 롤백으로 기존 이미지를 유지합니다.주요 변경사항
COMPLETED와DELETED만 남기고 정리했으며, 응답/API에서도 더 이상 불필요한 업로드 상태 필드를 노출하지 않도록 맞췄습니다.📊 변경 효과 요약 (가능한 경우에만)
PENDING,FAILED제거