refactor: 바이너리 업로드를 트랜잭션 밖으로 분리#255
Merged
Merged
Conversation
|
Warning Review limit reached
Next review available in: 35 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthrough바이너리 업로드 흐름이 저장, 완료 저장, 보상 삭제로 분리됐고, 스토리지 삭제 계약과 S3/로컬 구현이 추가됐다. 콘텐츠 생성·수정은 Estimated code review effort: 4 (Complex) | ~60 minutes Changes
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
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)
269-291: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value교체 테스트에 반환값(UserResponse) 검증 추가 권장
update_replaceImage_oldImageDeleted테스트에서userMapper.toDto(user)를 스텁하고expected까지 만들어두고도, 정작update()의 반환값에 대한 assertion이 빠져 있어 이 스텁이 실질적으로 검증에 기여하지 않습니다. 이미지 교체 후 반환되는UserResponse의profileImageUrl까지 함께 확인하면 매핑 계층까지 포함한 end-to-end 검증이 되어 더 견고합니다.♻️ 제안
- userService.update(userId, new UserUpdateRequest("새이름"), newImage); + UserResponse result = userService.update(userId, new UserUpdateRequest("새이름"), newImage); // Then + assertThat(result).isSameAs(expected); assertThat(oldImage.getUploadStatus()).isEqualTo(BinaryContentUploadStatus.DELETED); assertThat(user.getProfileImage().getUrl()).isEqualTo("http://localhost/profiles/new.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/user/service/UserServiceTest.java` around lines 269 - 291, In update_replaceImage_oldImageDeleted, the test stubs userMapper.toDto(user) and builds an expected UserResponse but never verifies the result from userService.update(...). Add an assertion on the returned UserResponse from update() and check its profileImageUrl (and related fields if needed) so the stubbed mapper call is actually validated; use the update(), userMapper.toDto(), and UserResponse symbols to locate the test.
🤖 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/StorageConfig.java`:
- Around line 42-49: `StorageConfig`의 S3Client 생성에서 `apiCallTimeout`과
`apiCallAttemptTimeout`이 10초/5초로 하드코딩되어 있어 환경별 튜닝이 어렵습니다. `S3StorageProperties`에
`Duration` 기반 설정값을 추가하고, `createS3Client`에서 해당 프로퍼티를 읽어
`ClientOverrideConfiguration`에 주입하도록 변경하세요. 이렇게 하면 region/accessKey처럼 타임아웃도 외부
설정으로 관리할 수 있습니다.
In `@src/main/java/com/codeit/team5/mopl/user/facade/UserProfileFacade.java`:
- Around line 32-36: `UserProfileFacade` currently uploads the profile image
with `binaryContentService.upload(...)` before calling
`userService.update(...)`, which can leave orphaned S3 objects/BinaryContent
records if the update fails. Fix this by either adding a pre-validation step
before `upload` to confirm the user/request is valid, or by adding compensation
in `UserProfileFacade.update` so that any failure from `userService.update`
triggers cleanup of the uploaded `BinaryContent` (for example via a
delete/deactivate API on `binaryContentService`). Ensure the chosen flow keeps
`update`, `upload`, and `BinaryContent` handling consistent.
In `@src/test/java/com/codeit/team5/mopl/user/facade/UserProfileFacadeTest.java`:
- Around line 65-85: `UserProfileFacadeTest.updateProfile_withImage_success`
only covers the happy path and misses the failure case where
`binaryContentService.upload` succeeds but `userService.update` throws, which
can leave orphan S3 objects. Add a test around `UserProfileFacade.updateProfile`
that stubs `binaryContentService.upload` to return a `BinaryContent`, stubs
`userService.update` to throw, and asserts the exception is propagated while
verifying the upload occurred; use the existing `userProfileFacade`,
`binaryContentService`, and `userService` symbols to locate the flow.
---
Outside diff comments:
In `@src/test/java/com/codeit/team5/mopl/user/service/UserServiceTest.java`:
- Around line 269-291: In update_replaceImage_oldImageDeleted, the test stubs
userMapper.toDto(user) and builds an expected UserResponse but never verifies
the result from userService.update(...). Add an assertion on the returned
UserResponse from update() and check its profileImageUrl (and related fields if
needed) so the stubbed mapper call is actually validated; use the update(),
userMapper.toDto(), and UserResponse symbols to locate 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: b1c07ba2-b5ab-4c70-b44b-9311970ae76e
📒 Files selected for processing (9)
src/main/java/com/codeit/team5/mopl/binarycontent/service/BinaryContentService.javasrc/main/java/com/codeit/team5/mopl/config/StorageConfig.javasrc/main/java/com/codeit/team5/mopl/user/controller/UserController.javasrc/main/java/com/codeit/team5/mopl/user/facade/UserProfileFacade.javasrc/main/java/com/codeit/team5/mopl/user/service/UserService.javasrc/test/java/com/codeit/team5/mopl/user/controller/UserControllerTest.javasrc/test/java/com/codeit/team5/mopl/user/facade/UserProfileFacadeTest.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 (2)
- src/test/java/com/codeit/team5/mopl/user/service/UserServiceImageRollbackIntegrationTest.java
- src/main/java/com/codeit/team5/mopl/binarycontent/service/BinaryContentService.java
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/test/java/com/codeit/team5/mopl/binarycontent/storage/s3/S3BinaryContentStorageTest.java`:
- Around line 32-35: 현재 테스트는 null 입력 시 S3StorageProperties의 기본값이 적용되는지만 간접적으로
확인하고 있어 apiCallTimeout/apiCallAttemptTimeout의 실제 기본값(10초/5초) 검증이 빠져 있습니다.
S3StorageProperties의 compact constructor 기본값 로직을 직접 검증하는 별도 단위 테스트를 추가해 null 전달
시 각 Duration 값이 기대값으로 세팅되는지 assert하고, 필요하면 S3BinaryContentStorageTest에서는 해당 값
확인을 제거해 책임을 분리하세요.
In `@src/test/java/com/codeit/team5/mopl/user/facade/UserProfileFacadeTest.java`:
- Around line 104-123: In
UserProfileFacadeTest.updateProfile_updateFailsAfterUpload_propagates, the
failure path is covered but the test only verifies binaryContentService.upload;
add a verify for userService.update to make it explicit that the update call
happened before the RuntimeException was propagated. Use the existing mocks in
this test method and keep the verification style consistent with the neighboring
updateProfile tests so the failure point is clearly documented.
🪄 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: ffe930c6-f756-4ec9-b297-0a716a60013b
📒 Files selected for processing (4)
src/main/java/com/codeit/team5/mopl/binarycontent/storage/s3/S3StorageProperties.javasrc/main/java/com/codeit/team5/mopl/config/StorageConfig.javasrc/test/java/com/codeit/team5/mopl/binarycontent/storage/s3/S3BinaryContentStorageTest.javasrc/test/java/com/codeit/team5/mopl/user/facade/UserProfileFacadeTest.java
dstle
force-pushed
the
fix/254-binary-upload-outside-transaction
branch
from
17573fe to
0a75bbb
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/test/java/com/codeit/team5/mopl/user/controller/UserControllerTest.java (1)
76-719: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win신규
PATCH /{userId}/password엔드포인트에 대한 컨트롤러 테스트가 없습니다.
UserController에 새로 추가된updatePassword핸들러(인증 주체 바인딩,@Valid검증, 204 응답, CSRF 등)를 검증하는@WebMvcTest케이스가 이 파일에 보이지 않습니다. 서비스 레이어(UserServiceTest)에는 해당 로직 테스트가 있지만, 라우팅/직렬화/보안 필터 체인 통합 검증은 컨트롤러 테스트에서만 확인 가능합니다.정상 케이스, 검증 실패(400), 타인 요청(403 또는 서비스 예외 매핑) 등을 다루는 테스트 추가를 권장합니다. 원하시면 초안을 작성해 드릴까요?
🤖 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/controller/UserControllerTest.java` around lines 76 - 719, Add missing WebMvc coverage for the new UserController updatePassword handler by creating tests around the existing UserControllerTest setup and helpers such as mockMvc, authOf, and userService. Verify the PATCH /{userId}/password flow with authenticated principal binding, CSRF, valid request body, and a 204 response while asserting userService.updatePassword is invoked with the expected userId and request. Also add negative cases for `@Valid` failures returning 400 and an unauthorized/forbidden case for acting on another user or missing authentication, so routing, serialization, and security behavior are covered in this controller test.src/main/java/com/codeit/team5/mopl/content/service/ContentService.java (2)
55-63: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winJavadoc의
@param image가 실제 파라미터명과 어긋나요.파라미터가
image에서thumbnail로 바뀌었는데, 바로 위 Javadoc은 여전히@param image로 남아있습니다. IDE에서 파라미터 힌트나 문서를 확인하는 개발자 입장에서는 실제 시그니처와 다른 설명을 보게 되어 혼선을 줄 수 있습니다.📝 제안
/** * 관리자가 콘텐츠를 직접 생성한다. * * `@param` request 제목·설명·타입·태그 정보 - * `@param` image 썸네일 이미지 (null 허용) + * `@param` thumbnail 업로드 완료된 썸네일 정보 (null 허용) * `@return` 생성된 콘텐츠 응답 DTO */🤖 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 55 - 63, The Javadoc in ContentService.create is out of sync with the method signature because it still documents `@param` image while the actual parameter is thumbnail. Update the parameter tag in the create method’s Javadoc to match thumbnail so the documentation aligns with the current signature and IDE hints.
82-92: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
update도 동일하게 Javadoc 파라미터명이 어긋나 있어요.위
create와 같은 이유로,image→thumbnail로 파라미터명이 바뀌었지만 Javadoc은 갱신되지 않았습니다.📝 제안
/** * 콘텐츠 정보를 수정한다. 이미지가 제공되면 기존 썸네일을 DELETED 상태로 변경 후 교체한다. * * `@param` contentId 수정할 콘텐츠 UUID * `@param` request 변경할 제목·설명·태그 정보 - * `@param` image 새 썸네일 이미지 (null이면 기존 이미지 유지) + * `@param` thumbnail 업로드 완료된 새 썸네일 정보 (null이면 기존 이미지 유지) * `@return` 수정된 콘텐츠 응답 DTO * `@throws` ContentNotFoundException 콘텐츠가 존재하지 않을 때 */🤖 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 82 - 92, The Javadoc for ContentService.update is out of sync with the method signature because the parameter was renamed from image to thumbnail. Update the `@param` entry in update to use thumbnail so it matches the actual argument name, and check the adjacent create method Javadoc for the same naming consistency.
🤖 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/storage/s3/S3StorageProperties.java`:
- Around line 3-23: Verify and tune the default S3 timeout values in
S3StorageProperties because the current apiCallAttemptTimeout and apiCallTimeout
may be too aggressive for large uploads and SDK retries. Review the defaults in
the record constructor, validate them against real image sizes and slow network
conditions, and adjust the fallback durations if needed so retries can complete
without prematurely exhausting the total budget while still keeping transaction
hold time short.
In
`@src/main/java/com/codeit/team5/mopl/binarycontent/storage/StorageKeyFactory.java`:
- Around line 17-24: The generated storage key currently omits the owner
identifier, which removes the ability to group objects by owner at the storage
layer. Update StorageKeyFactory.generate to preserve owner-based prefixing (for
example by including ownerId in the key structure alongside StorageDirectory),
and verify the resulting key format still works with GeneratedKey and
ImageExtension handling. If the key format is intentionally not changing, ensure
the persistence flow that uses generate retains a reliable key-to-owner mapping
in the DB so later cleanup can still target all of a user’s files.
In `@src/main/java/com/codeit/team5/mopl/user/service/UserService.java`:
- Around line 123-136: updatePassword() currently changes the password after
only validateOwner(), so the normal password-change flow must require
re-authentication by verifying the user’s current password before encoding and
saving the new one. Update ChangePasswordRequest to include currentPassword,
then in UserService.updatePassword() load the user, compare currentPassword with
the stored password using passwordEncoder.matches(), and reject the change if it
does not match. If temporaryPasswordService is meant for forced resets, keep
that separate from the regular password-change path so updatePassword() only
handles authenticated changes.
---
Outside diff comments:
In `@src/main/java/com/codeit/team5/mopl/content/service/ContentService.java`:
- Around line 55-63: The Javadoc in ContentService.create is out of sync with
the method signature because it still documents `@param` image while the actual
parameter is thumbnail. Update the parameter tag in the create method’s Javadoc
to match thumbnail so the documentation aligns with the current signature and
IDE hints.
- Around line 82-92: The Javadoc for ContentService.update is out of sync with
the method signature because the parameter was renamed from image to thumbnail.
Update the `@param` entry in update to use thumbnail so it matches the actual
argument name, and check the adjacent create method Javadoc for the same naming
consistency.
In `@src/test/java/com/codeit/team5/mopl/user/controller/UserControllerTest.java`:
- Around line 76-719: Add missing WebMvc coverage for the new UserController
updatePassword handler by creating tests around the existing UserControllerTest
setup and helpers such as mockMvc, authOf, and userService. Verify the PATCH
/{userId}/password flow with authenticated principal binding, CSRF, valid
request body, and a 204 response while asserting userService.updatePassword is
invoked with the expected userId and request. Also add negative cases for `@Valid`
failures returning 400 and an unauthorized/forbidden case for acting on another
user or missing authentication, so routing, serialization, and security behavior
are covered in this controller 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: 946aefb0-c4f0-4286-a4da-0d364ff8e629
📒 Files selected for processing (30)
src/main/java/com/codeit/team5/mopl/binarycontent/dto/UploadedBinaryContent.javasrc/main/java/com/codeit/team5/mopl/binarycontent/event/BinaryContentDeleteEvent.javasrc/main/java/com/codeit/team5/mopl/binarycontent/eventlistener/BinaryContentDeleteEventListener.javasrc/main/java/com/codeit/team5/mopl/binarycontent/service/BinaryContentService.javasrc/main/java/com/codeit/team5/mopl/binarycontent/service/UploadWithRollback.javasrc/main/java/com/codeit/team5/mopl/binarycontent/storage/BinaryContentStorage.javasrc/main/java/com/codeit/team5/mopl/binarycontent/storage/StorageKeyFactory.javasrc/main/java/com/codeit/team5/mopl/binarycontent/storage/local/LocalBinaryContentStorage.javasrc/main/java/com/codeit/team5/mopl/binarycontent/storage/s3/S3BinaryContentStorage.javasrc/main/java/com/codeit/team5/mopl/binarycontent/storage/s3/S3StorageProperties.javasrc/main/java/com/codeit/team5/mopl/config/StorageConfig.javasrc/main/java/com/codeit/team5/mopl/content/controller/ContentController.javasrc/main/java/com/codeit/team5/mopl/content/facade/ContentFacade.javasrc/main/java/com/codeit/team5/mopl/content/service/ContentService.javasrc/main/java/com/codeit/team5/mopl/user/controller/UserController.javasrc/main/java/com/codeit/team5/mopl/user/facade/UserProfileFacade.javasrc/main/java/com/codeit/team5/mopl/user/service/UserService.javasrc/test/java/com/codeit/team5/mopl/binarycontent/eventlistener/BinaryContentDeleteEventListenerTest.javasrc/test/java/com/codeit/team5/mopl/binarycontent/service/BinaryContentServiceTest.javasrc/test/java/com/codeit/team5/mopl/binarycontent/storage/StorageKeyFactoryTest.javasrc/test/java/com/codeit/team5/mopl/binarycontent/storage/local/LocalBinaryContentStorageTest.javasrc/test/java/com/codeit/team5/mopl/binarycontent/storage/s3/S3BinaryContentStorageTest.javasrc/test/java/com/codeit/team5/mopl/content/controller/ContentControllerTest.javasrc/test/java/com/codeit/team5/mopl/content/facade/ContentFacadeTest.javasrc/test/java/com/codeit/team5/mopl/content/service/ContentServiceTest.javasrc/test/java/com/codeit/team5/mopl/user/controller/UserControllerTest.javasrc/test/java/com/codeit/team5/mopl/user/facade/UserProfileFacadeIntegrationTest.javasrc/test/java/com/codeit/team5/mopl/user/facade/UserProfileFacadeTest.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 (1)
- src/test/java/com/codeit/team5/mopl/user/service/UserServiceImageRollbackIntegrationTest.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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/test/java/com/codeit/team5/mopl/user/controller/UserControllerTest.java (1)
76-719: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win신규
PATCH /{userId}/password엔드포인트에 대한 컨트롤러 테스트가 없습니다.
UserController에 새로 추가된updatePassword핸들러(인증 주체 바인딩,@Valid검증, 204 응답, CSRF 등)를 검증하는@WebMvcTest케이스가 이 파일에 보이지 않습니다. 서비스 레이어(UserServiceTest)에는 해당 로직 테스트가 있지만, 라우팅/직렬화/보안 필터 체인 통합 검증은 컨트롤러 테스트에서만 확인 가능합니다.정상 케이스, 검증 실패(400), 타인 요청(403 또는 서비스 예외 매핑) 등을 다루는 테스트 추가를 권장합니다. 원하시면 초안을 작성해 드릴까요?
🤖 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/controller/UserControllerTest.java` around lines 76 - 719, Add missing WebMvc coverage for the new UserController updatePassword handler by creating tests around the existing UserControllerTest setup and helpers such as mockMvc, authOf, and userService. Verify the PATCH /{userId}/password flow with authenticated principal binding, CSRF, valid request body, and a 204 response while asserting userService.updatePassword is invoked with the expected userId and request. Also add negative cases for `@Valid` failures returning 400 and an unauthorized/forbidden case for acting on another user or missing authentication, so routing, serialization, and security behavior are covered in this controller test.src/main/java/com/codeit/team5/mopl/content/service/ContentService.java (2)
55-63: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winJavadoc의
@param image가 실제 파라미터명과 어긋나요.파라미터가
image에서thumbnail로 바뀌었는데, 바로 위 Javadoc은 여전히@param image로 남아있습니다. IDE에서 파라미터 힌트나 문서를 확인하는 개발자 입장에서는 실제 시그니처와 다른 설명을 보게 되어 혼선을 줄 수 있습니다.📝 제안
/** * 관리자가 콘텐츠를 직접 생성한다. * * `@param` request 제목·설명·타입·태그 정보 - * `@param` image 썸네일 이미지 (null 허용) + * `@param` thumbnail 업로드 완료된 썸네일 정보 (null 허용) * `@return` 생성된 콘텐츠 응답 DTO */🤖 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 55 - 63, The Javadoc in ContentService.create is out of sync with the method signature because it still documents `@param` image while the actual parameter is thumbnail. Update the parameter tag in the create method’s Javadoc to match thumbnail so the documentation aligns with the current signature and IDE hints.
82-92: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
update도 동일하게 Javadoc 파라미터명이 어긋나 있어요.위
create와 같은 이유로,image→thumbnail로 파라미터명이 바뀌었지만 Javadoc은 갱신되지 않았습니다.📝 제안
/** * 콘텐츠 정보를 수정한다. 이미지가 제공되면 기존 썸네일을 DELETED 상태로 변경 후 교체한다. * * `@param` contentId 수정할 콘텐츠 UUID * `@param` request 변경할 제목·설명·태그 정보 - * `@param` image 새 썸네일 이미지 (null이면 기존 이미지 유지) + * `@param` thumbnail 업로드 완료된 새 썸네일 정보 (null이면 기존 이미지 유지) * `@return` 수정된 콘텐츠 응답 DTO * `@throws` ContentNotFoundException 콘텐츠가 존재하지 않을 때 */🤖 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 82 - 92, The Javadoc for ContentService.update is out of sync with the method signature because the parameter was renamed from image to thumbnail. Update the `@param` entry in update to use thumbnail so it matches the actual argument name, and check the adjacent create method Javadoc for the same naming consistency.
🤖 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/storage/s3/S3StorageProperties.java`:
- Around line 3-23: Verify and tune the default S3 timeout values in
S3StorageProperties because the current apiCallAttemptTimeout and apiCallTimeout
may be too aggressive for large uploads and SDK retries. Review the defaults in
the record constructor, validate them against real image sizes and slow network
conditions, and adjust the fallback durations if needed so retries can complete
without prematurely exhausting the total budget while still keeping transaction
hold time short.
In
`@src/main/java/com/codeit/team5/mopl/binarycontent/storage/StorageKeyFactory.java`:
- Around line 17-24: The generated storage key currently omits the owner
identifier, which removes the ability to group objects by owner at the storage
layer. Update StorageKeyFactory.generate to preserve owner-based prefixing (for
example by including ownerId in the key structure alongside StorageDirectory),
and verify the resulting key format still works with GeneratedKey and
ImageExtension handling. If the key format is intentionally not changing, ensure
the persistence flow that uses generate retains a reliable key-to-owner mapping
in the DB so later cleanup can still target all of a user’s files.
In `@src/main/java/com/codeit/team5/mopl/user/service/UserService.java`:
- Around line 123-136: updatePassword() currently changes the password after
only validateOwner(), so the normal password-change flow must require
re-authentication by verifying the user’s current password before encoding and
saving the new one. Update ChangePasswordRequest to include currentPassword,
then in UserService.updatePassword() load the user, compare currentPassword with
the stored password using passwordEncoder.matches(), and reject the change if it
does not match. If temporaryPasswordService is meant for forced resets, keep
that separate from the regular password-change path so updatePassword() only
handles authenticated changes.
---
Outside diff comments:
In `@src/main/java/com/codeit/team5/mopl/content/service/ContentService.java`:
- Around line 55-63: The Javadoc in ContentService.create is out of sync with
the method signature because it still documents `@param` image while the actual
parameter is thumbnail. Update the parameter tag in the create method’s Javadoc
to match thumbnail so the documentation aligns with the current signature and
IDE hints.
- Around line 82-92: The Javadoc for ContentService.update is out of sync with
the method signature because the parameter was renamed from image to thumbnail.
Update the `@param` entry in update to use thumbnail so it matches the actual
argument name, and check the adjacent create method Javadoc for the same naming
consistency.
In `@src/test/java/com/codeit/team5/mopl/user/controller/UserControllerTest.java`:
- Around line 76-719: Add missing WebMvc coverage for the new UserController
updatePassword handler by creating tests around the existing UserControllerTest
setup and helpers such as mockMvc, authOf, and userService. Verify the PATCH
/{userId}/password flow with authenticated principal binding, CSRF, valid
request body, and a 204 response while asserting userService.updatePassword is
invoked with the expected userId and request. Also add negative cases for `@Valid`
failures returning 400 and an unauthorized/forbidden case for acting on another
user or missing authentication, so routing, serialization, and security behavior
are covered in this controller 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: 946aefb0-c4f0-4286-a4da-0d364ff8e629
📒 Files selected for processing (30)
src/main/java/com/codeit/team5/mopl/binarycontent/dto/UploadedBinaryContent.javasrc/main/java/com/codeit/team5/mopl/binarycontent/event/BinaryContentDeleteEvent.javasrc/main/java/com/codeit/team5/mopl/binarycontent/eventlistener/BinaryContentDeleteEventListener.javasrc/main/java/com/codeit/team5/mopl/binarycontent/service/BinaryContentService.javasrc/main/java/com/codeit/team5/mopl/binarycontent/service/UploadWithRollback.javasrc/main/java/com/codeit/team5/mopl/binarycontent/storage/BinaryContentStorage.javasrc/main/java/com/codeit/team5/mopl/binarycontent/storage/StorageKeyFactory.javasrc/main/java/com/codeit/team5/mopl/binarycontent/storage/local/LocalBinaryContentStorage.javasrc/main/java/com/codeit/team5/mopl/binarycontent/storage/s3/S3BinaryContentStorage.javasrc/main/java/com/codeit/team5/mopl/binarycontent/storage/s3/S3StorageProperties.javasrc/main/java/com/codeit/team5/mopl/config/StorageConfig.javasrc/main/java/com/codeit/team5/mopl/content/controller/ContentController.javasrc/main/java/com/codeit/team5/mopl/content/facade/ContentFacade.javasrc/main/java/com/codeit/team5/mopl/content/service/ContentService.javasrc/main/java/com/codeit/team5/mopl/user/controller/UserController.javasrc/main/java/com/codeit/team5/mopl/user/facade/UserProfileFacade.javasrc/main/java/com/codeit/team5/mopl/user/service/UserService.javasrc/test/java/com/codeit/team5/mopl/binarycontent/eventlistener/BinaryContentDeleteEventListenerTest.javasrc/test/java/com/codeit/team5/mopl/binarycontent/service/BinaryContentServiceTest.javasrc/test/java/com/codeit/team5/mopl/binarycontent/storage/StorageKeyFactoryTest.javasrc/test/java/com/codeit/team5/mopl/binarycontent/storage/local/LocalBinaryContentStorageTest.javasrc/test/java/com/codeit/team5/mopl/binarycontent/storage/s3/S3BinaryContentStorageTest.javasrc/test/java/com/codeit/team5/mopl/content/controller/ContentControllerTest.javasrc/test/java/com/codeit/team5/mopl/content/facade/ContentFacadeTest.javasrc/test/java/com/codeit/team5/mopl/content/service/ContentServiceTest.javasrc/test/java/com/codeit/team5/mopl/user/controller/UserControllerTest.javasrc/test/java/com/codeit/team5/mopl/user/facade/UserProfileFacadeIntegrationTest.javasrc/test/java/com/codeit/team5/mopl/user/facade/UserProfileFacadeTest.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 (1)
- src/test/java/com/codeit/team5/mopl/user/service/UserServiceImageRollbackIntegrationTest.java
🛑 Comments failed to post (3)
src/main/java/com/codeit/team5/mopl/binarycontent/storage/s3/S3StorageProperties.java (1)
3-23: 🚀 Performance & Scalability | 🔵 Trivial
타임아웃 기본값이 대용량 이미지 업로드에 적합한지 확인이 필요해요.
apiCallAttemptTimeout을 5초,apiCallTimeout을 10초로 설정하셨는데, AWS SDK의 기본 재시도 정책(통상 최대 3회 시도)을 고려하면 재시도가 다 이루어지기 전에 전체 예산(10초)을 초과해 조기 종료될 여지가 있어요. 이 값이 실제 서비스에서 다루는 이미지 파일 크기와 네트워크 환경(특히 느린 모바일 환경)에서도 충분한지 부하 테스트나 프로덕션 로그 기반으로 한 번 검증해보시는 걸 권장드립니다. 너무 짧으면 정상 업로드도 실패로 처리될 수 있고, 반대로 너무 길면 이번 PR의 목적(트랜잭션 점유 시간 단축)이 희석될 수 있어요.🤖 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/storage/s3/S3StorageProperties.java` around lines 3 - 23, Verify and tune the default S3 timeout values in S3StorageProperties because the current apiCallAttemptTimeout and apiCallTimeout may be too aggressive for large uploads and SDK retries. Review the defaults in the record constructor, validate them against real image sizes and slow network conditions, and adjust the fallback durations if needed so retries can complete without prematurely exhausting the total budget while still keeping transaction hold time short.src/main/java/com/codeit/team5/mopl/binarycontent/storage/StorageKeyFactory.java (1)
17-24: 🔒 Security & Privacy | 🔵 Trivial
키에서
ownerId가 빠지면서 스토리지 레벨의 소유자별 그룹핑이 사라져요.기능적으로는 문제없지만, 이후 사용자 데이터 삭제 요청(개인정보 파기 등) 시 S3 prefix 기준 일괄 삭제가 불가능해지고 전적으로 DB 참조를 통한 개별 삭제에 의존해야 합니다. 파일 수가 많아지면 삭제 절차가 더 번거로워질 수 있으니, 필요하다면 DB에
key와 소유자 매핑이 확실히 남아있는지 확인해 두는 게 좋겠습니다.🤖 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/storage/StorageKeyFactory.java` around lines 17 - 24, The generated storage key currently omits the owner identifier, which removes the ability to group objects by owner at the storage layer. Update StorageKeyFactory.generate to preserve owner-based prefixing (for example by including ownerId in the key structure alongside StorageDirectory), and verify the resulting key format still works with GeneratedKey and ImageExtension handling. If the key format is intentionally not changing, ensure the persistence flow that uses generate retains a reliable key-to-owner mapping in the DB so later cleanup can still target all of a user’s files.src/main/java/com/codeit/team5/mopl/user/service/UserService.java (1)
123-136: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Inspect ChangePasswordRequest to check for a currentPassword field fd -type f 'ChangePasswordRequest.java' --exec cat -n {}Repository: sb10-part4-team5/sb10-mopl-team5
Length of output: 341
🏁 Script executed:
#!/bin/bash set -euo pipefail echo '--- ChangePasswordRequest.java ---' fd -a 'ChangePasswordRequest.java' . echo for f in $(fd -a 'ChangePasswordRequest.java' .); do echo "### $f" cat -n "$f" done echo echo '--- usages of ChangePasswordRequest ---' rg -n "new ChangePasswordRequest|record ChangePasswordRequest|class ChangePasswordRequest|currentPassword|password\(\)" src testRepository: sb10-part4-team5/sb10-mopl-team5
Length of output: 4425
비밀번호 변경에는 현재 비밀번호 재입력 또는 재인증을 추가하세요.
ChangePasswordRequest가 새 비밀번호만 받는 구조라서updatePassword()는 소유권 확인만 통과하면 바로 비밀번호를 바꿀 수 있습니다. 세션 탈취 상황에선 계정 탈취를 오래 유지하기 쉬우니,currentPassword를 받아matches()로 검증하는 방식이 더 안전합니다. 임시 비밀번호 기반 강제 재설정이 의도라면, 일반 변경 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/user/service/UserService.java` around lines 123 - 136, updatePassword() currently changes the password after only validateOwner(), so the normal password-change flow must require re-authentication by verifying the user’s current password before encoding and saving the new one. Update ChangePasswordRequest to include currentPassword, then in UserService.updatePassword() load the user, compare currentPassword with the stored password using passwordEncoder.matches(), and reject the change if it does not match. If temporaryPasswordService is meant for forced resets, keep that separate from the regular password-change path so updatePassword() only handles authenticated changes.
Contributor
Author
|
@coderabbitai 리뷰해줘 |
|
(੭˃ᴗ˂)੭ 리뷰 준비 중... ✅ Action performedReview finished.
|
Contributor
Author
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 3
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/service/ContentService.java (1)
55-63: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winJavadoc
@param이름이 실제 파라미터명과 어긋나 있어요.
create/update모두 파라미터가FileRequest image→UploadedBinaryContent thumbnail로 바뀌었는데, 메서드 위 Javadoc은 여전히@param image를 가리키고 있습니다. 기능상 문제는 없지만, 문서와 시그니처가 어긋나면 이후 이 API를 사용하는 사람이 파라미터 의미를 오해할 수 있고 javadoc 도구가 경고를 낼 수도 있습니다. 파라미터 이름을 바꾼 김에 Javadoc도 같이 갱신해주면 깔끔할 것 같아요.✏️ 제안하는 Javadoc 수정
/** * 관리자가 콘텐츠를 직접 생성한다. * * `@param` request 제목·설명·타입·태그 정보 - * `@param` image 썸네일 이미지 (null 허용) + * `@param` thumbnail 업로드가 완료된 썸네일 정보 (null 허용) * `@return` 생성된 콘텐츠 응답 DTO *//** * 콘텐츠 정보를 수정한다. 이미지가 제공되면 기존 썸네일을 DELETED 상태로 변경 후 교체한다. * * `@param` contentId 수정할 콘텐츠 UUID * `@param` request 변경할 제목·설명·태그 정보 - * `@param` image 새 썸네일 이미지 (null이면 기존 이미지 유지) + * `@param` thumbnail 업로드가 완료된 새 썸네일 정보 (null이면 기존 이미지 유지) * `@return` 수정된 콘텐츠 응답 DTO * `@throws` ContentNotFoundException 콘텐츠가 존재하지 않을 때 */Also applies to: 82-92
🤖 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 55 - 63, The Javadoc parameter tags in ContentService are out of sync with the renamed thumbnail argument in create and update, so update the `@param` entry from the old image name to `thumbnail` in the comments for both methods. Keep the descriptions aligned with the actual `UploadedBinaryContent thumbnail` parameter so the documentation matches the method signatures and avoids Javadoc warnings.
🤖 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/UploadWithRollback.java`:
- Around line 18-41: `UploadWithRollback` 자체를 검증하는 전용 단위 테스트가 없으므로, `execute`의
핵심 분기들을 커버하는 `UploadWithRollbackTest`를 추가하세요.
`binaryContentService.uploadToStorage`, `eventPublisher.publishEvent`, 그리고 전달된
`persist` 함수 호출을 목(mock)으로 검증해 `image == null`일 때 업로드 없이 `persist(null)`만 호출되는지,
업로드 성공 후 `persist` 성공 시 보상 이벤트가 발행되지 않는지, `persist`가 `RuntimeException`을 던질 때
`BinaryContentDeleteEvent`가 발행되고 예외가 그대로 전파되는지, `uploadToStorage` 실패 시에는 이벤트가
발행되지 않는지를 확인하면 됩니다.
In
`@src/main/java/com/codeit/team5/mopl/binarycontent/storage/StorageKeyFactory.java`:
- Around line 17-24: `StorageKeyFactory.generate` now creates keys without
`ownerId`, so make sure the owner-to-key relationship is still preserved
elsewhere for lifecycle operations like bulk delete. Confirm the
`BinaryContent`/upload persistence path records the uploader or owner alongside
the generated key, and if any deletion workflow currently depends on path
prefixes, update it to use DB lookup by owner instead.
In `@src/test/java/com/codeit/team5/mopl/content/facade/ContentFacadeTest.java`:
- Around line 112-142: The ContentFacadeTest.update() coverage is missing the
“no image” path, so add a test for ContentFacade.update that passes a null image
and verifies it delegates to contentService.update(contentId, updateRequest,
null) without calling binaryContentService.uploadToStorage. Also tighten
update_withImage_success by asserting
binaryContentService.uploadToStorage(StorageDirectory.THUMBNAIL, ...) is
invoked, matching the stricter coverage already used in the create() tests.
---
Outside diff comments:
In `@src/main/java/com/codeit/team5/mopl/content/service/ContentService.java`:
- Around line 55-63: The Javadoc parameter tags in ContentService are out of
sync with the renamed thumbnail argument in create and update, so update the
`@param` entry from the old image name to `thumbnail` in the comments for both
methods. Keep the descriptions aligned with the actual `UploadedBinaryContent
thumbnail` parameter so the documentation matches the method signatures and
avoids Javadoc warnings.
🪄 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: 842067ac-56f3-47f6-b282-1e7c00b09d48
📒 Files selected for processing (30)
src/main/java/com/codeit/team5/mopl/binarycontent/dto/UploadedBinaryContent.javasrc/main/java/com/codeit/team5/mopl/binarycontent/event/BinaryContentDeleteEvent.javasrc/main/java/com/codeit/team5/mopl/binarycontent/eventlistener/BinaryContentDeleteEventListener.javasrc/main/java/com/codeit/team5/mopl/binarycontent/service/BinaryContentService.javasrc/main/java/com/codeit/team5/mopl/binarycontent/service/UploadWithRollback.javasrc/main/java/com/codeit/team5/mopl/binarycontent/storage/BinaryContentStorage.javasrc/main/java/com/codeit/team5/mopl/binarycontent/storage/StorageKeyFactory.javasrc/main/java/com/codeit/team5/mopl/binarycontent/storage/local/LocalBinaryContentStorage.javasrc/main/java/com/codeit/team5/mopl/binarycontent/storage/s3/S3BinaryContentStorage.javasrc/main/java/com/codeit/team5/mopl/binarycontent/storage/s3/S3StorageProperties.javasrc/main/java/com/codeit/team5/mopl/config/StorageConfig.javasrc/main/java/com/codeit/team5/mopl/content/controller/ContentController.javasrc/main/java/com/codeit/team5/mopl/content/facade/ContentFacade.javasrc/main/java/com/codeit/team5/mopl/content/service/ContentService.javasrc/main/java/com/codeit/team5/mopl/user/controller/UserController.javasrc/main/java/com/codeit/team5/mopl/user/facade/UserProfileFacade.javasrc/main/java/com/codeit/team5/mopl/user/service/UserService.javasrc/test/java/com/codeit/team5/mopl/binarycontent/eventlistener/BinaryContentDeleteEventListenerTest.javasrc/test/java/com/codeit/team5/mopl/binarycontent/service/BinaryContentServiceTest.javasrc/test/java/com/codeit/team5/mopl/binarycontent/storage/StorageKeyFactoryTest.javasrc/test/java/com/codeit/team5/mopl/binarycontent/storage/local/LocalBinaryContentStorageTest.javasrc/test/java/com/codeit/team5/mopl/binarycontent/storage/s3/S3BinaryContentStorageTest.javasrc/test/java/com/codeit/team5/mopl/content/controller/ContentControllerTest.javasrc/test/java/com/codeit/team5/mopl/content/facade/ContentFacadeTest.javasrc/test/java/com/codeit/team5/mopl/content/service/ContentServiceTest.javasrc/test/java/com/codeit/team5/mopl/user/controller/UserControllerTest.javasrc/test/java/com/codeit/team5/mopl/user/facade/UserProfileFacadeIntegrationTest.javasrc/test/java/com/codeit/team5/mopl/user/facade/UserProfileFacadeTest.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 (1)
- src/test/java/com/codeit/team5/mopl/user/service/UserServiceImageRollbackIntegrationTest.java
- BinaryContentService를 uploadToStorage(S3)/saveCompleted(DB)/deleteQuietly로 분리 - S3 업로드는 트랜잭션 밖, DB 저장만 짧은 트랜잭션 (S3Client 타임아웃 설정) - 업로드 후 실패 시 롤백을 UploadWithRollback으로 공통화, 삭제는 비동기 이벤트로 처리 - UserProfileFacade/ContentFacade가 업로드 조율 (User는 소유자 검증 선행) - S3 저장 경로에서 소유자 세그먼트 제거
- ContentFacade update의 이미지 없음 케이스 테스트 추가 - UploadWithRollback 전용 단위테스트 추가
dstle
force-pushed
the
fix/254-binary-upload-outside-transaction
branch
from
b05289f to
d93b68b
Compare
plzslp
previously approved these changes
Jul 3, 2026
plzslp
left a comment
plzslp
left a comment
Collaborator
There was a problem hiding this comment.
문제되었던 트랜잭션 문제가 잘 해결된 것 같습니다.
BinaryContentUploadStatus가 DELETED인 객체들을 삭제하는 작업은 별도 이슈로 만들면 될 것 같습니다.
There was a problem hiding this comment.
Actionable comments posted: 3
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/user/service/UserService.java (1)
76-87: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low value
UserService.update(...)의 전제 조건을 명시해 두면 좋겠습니다.지금은
UserProfileFacade가 소유권을 먼저 검증해서 동작상 문제는 없지만,update가public이라 다른 호출 경로가 생기면 검증 없이 프로필이 갱신될 수 있습니다. Javadoc으로 “호출 전 소유권 검증 필요”를 남기거나 내부 전용 메서드로 분리해 의도를 고정하면, 나중에 계층을 우회하는 실수를 줄일 수 있습니다.🤖 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/user/service/UserService.java` around lines 76 - 87, `UserService.update(...)`의 호출 전 전제 조건이 코드에 드러나지 않아 다른 경로에서 소유권 검증 없이 호출될 수 있습니다. `update` 메서드에 Javadoc으로 “호출 전에 소유권 검증이 필요함”을 명시하거나, `UserProfileFacade`에서만 쓰는 내부 전용 메서드로 분리해 의도를 고정하세요. `getUserWithProfileImage(...)`와 `update(...)`의 역할 경계를 분명히 해서 우회 호출 가능성을 줄이세요.
🤖 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/storage/local/LocalBinaryContentStorage.java`:
- Around line 46-53: The delete logic in LocalBinaryContentStorage.delete
currently logs “삭제 완료” unconditionally even when Files.deleteIfExists returns
false because nothing was removed. Capture the boolean result from
Files.deleteIfExists(destination) and branch the log message accordingly so the
debug output clearly distinguishes between an actual deletion and a no-op; keep
the FileStorageException handling unchanged.
In
`@src/main/java/com/codeit/team5/mopl/binarycontent/storage/s3/S3StorageProperties.java`:
- Around line 16-23: S3StorageProperties의 compact constructor에서
apiCallAttemptTimeout이 apiCallTimeout보다 큰 설정을 추가로 검증하도록 수정하세요. 현재는 기본값만 채우고 있는데,
외부 설정으로 들어온 값까지 포함해 apiCallAttemptTimeout.compareTo(apiCallTimeout) > 0인 경우
IllegalArgumentException을 던지도록 하고, 위치는 S3StorageProperties 생성자 안에서
apiCallTimeout/apiCallAttemptTimeout 기본값 설정 직후에 두면 됩니다.
In `@src/main/java/com/codeit/team5/mopl/config/AsyncConfig.java`:
- Around line 39-50: `binaryContentTaskExecutor` adds a second
`ThreadPoolTaskExecutor`, so any `@Async` usage without an explicit executor
name may become ambiguous at runtime; verify the consumer side such as
`BinaryContentDeleteEventListener` and either annotate the async entrypoint with
`@Async("binaryContentTaskExecutor")` or set a default executor via
`AsyncConfigurer` so Spring always knows which executor to use. Also add a brief
Javadoc comment on `binaryContentTaskExecutor` explaining the `CallerRunsPolicy`
choice to match the existing documentation style used by `batchJobTaskExecutor`.
---
Outside diff comments:
In `@src/main/java/com/codeit/team5/mopl/user/service/UserService.java`:
- Around line 76-87: `UserService.update(...)`의 호출 전 전제 조건이 코드에 드러나지 않아 다른 경로에서
소유권 검증 없이 호출될 수 있습니다. `update` 메서드에 Javadoc으로 “호출 전에 소유권 검증이 필요함”을 명시하거나,
`UserProfileFacade`에서만 쓰는 내부 전용 메서드로 분리해 의도를 고정하세요.
`getUserWithProfileImage(...)`와 `update(...)`의 역할 경계를 분명히 해서 우회 호출 가능성을 줄이세요.
🪄 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: 28b8bbbb-79c1-49ec-886e-4beb8983128b
📒 Files selected for processing (32)
src/main/java/com/codeit/team5/mopl/binarycontent/dto/UploadedBinaryContent.javasrc/main/java/com/codeit/team5/mopl/binarycontent/event/BinaryContentDeleteEvent.javasrc/main/java/com/codeit/team5/mopl/binarycontent/eventlistener/BinaryContentDeleteEventListener.javasrc/main/java/com/codeit/team5/mopl/binarycontent/service/BinaryContentService.javasrc/main/java/com/codeit/team5/mopl/binarycontent/service/UploadWithRollback.javasrc/main/java/com/codeit/team5/mopl/binarycontent/storage/BinaryContentStorage.javasrc/main/java/com/codeit/team5/mopl/binarycontent/storage/StorageKeyFactory.javasrc/main/java/com/codeit/team5/mopl/binarycontent/storage/local/LocalBinaryContentStorage.javasrc/main/java/com/codeit/team5/mopl/binarycontent/storage/s3/S3BinaryContentStorage.javasrc/main/java/com/codeit/team5/mopl/binarycontent/storage/s3/S3StorageProperties.javasrc/main/java/com/codeit/team5/mopl/config/AsyncConfig.javasrc/main/java/com/codeit/team5/mopl/config/StorageConfig.javasrc/main/java/com/codeit/team5/mopl/content/controller/ContentController.javasrc/main/java/com/codeit/team5/mopl/content/facade/ContentFacade.javasrc/main/java/com/codeit/team5/mopl/content/service/ContentService.javasrc/main/java/com/codeit/team5/mopl/user/controller/UserController.javasrc/main/java/com/codeit/team5/mopl/user/facade/UserProfileFacade.javasrc/main/java/com/codeit/team5/mopl/user/service/UserService.javasrc/test/java/com/codeit/team5/mopl/binarycontent/eventlistener/BinaryContentDeleteEventListenerTest.javasrc/test/java/com/codeit/team5/mopl/binarycontent/service/BinaryContentServiceTest.javasrc/test/java/com/codeit/team5/mopl/binarycontent/service/UploadWithRollbackTest.javasrc/test/java/com/codeit/team5/mopl/binarycontent/storage/StorageKeyFactoryTest.javasrc/test/java/com/codeit/team5/mopl/binarycontent/storage/local/LocalBinaryContentStorageTest.javasrc/test/java/com/codeit/team5/mopl/binarycontent/storage/s3/S3BinaryContentStorageTest.javasrc/test/java/com/codeit/team5/mopl/content/controller/ContentControllerTest.javasrc/test/java/com/codeit/team5/mopl/content/facade/ContentFacadeTest.javasrc/test/java/com/codeit/team5/mopl/content/service/ContentServiceTest.javasrc/test/java/com/codeit/team5/mopl/user/controller/UserControllerTest.javasrc/test/java/com/codeit/team5/mopl/user/facade/UserProfileFacadeIntegrationTest.javasrc/test/java/com/codeit/team5/mopl/user/facade/UserProfileFacadeTest.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 (1)
- src/test/java/com/codeit/team5/mopl/user/service/UserServiceImageRollbackIntegrationTest.java
plzslp
approved these changes
Jul 3, 2026
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.
관련 이슈
작업 내용
변경 사항
테스트 내용
체크리스트
리뷰 포인트
스크린샷 / 참고 자료
🔗 관련 이슈
#254📝 작업 내용
이 PR은 프로필/콘텐츠 이미지 업로드가 DB 트랜잭션 내부에서 동기적으로 수행되면서 트랜잭션 유지 시간이 길어지고, 업로드 지연/실패가 사용자 정보(및 콘텐츠) 갱신 로직까지 함께 영향을 주던 문제를 해결합니다.
핵심은 **“스토리지 업로드는 트랜잭션 밖에서 먼저 실행 → DB 변경은 짧은 트랜잭션으로만 수행”**하도록 흐름을 재구성하는 것입니다. 또한 업로드 결과(key/url)를 먼저 확보해 이후 단계에서 즉시 반영될 수 있게 책임을 분리했습니다.
주요 변경사항
BinaryContentService에서 트랜잭션 경계 재설정
BinaryContentService의 업로드는uploadToStorage(...)로 옮겨 트랜잭션 없이 수행하고, **DB 저장은saveCompleted(UploadedBinaryContent)에만@Transactional**을 적용했습니다.deleteQuietly(UploadedBinaryContent)로 “최선의 노력” 방식으로 처리되도록 정리했습니다.업로드-저장-보상 흐름을 UploadWithRollback로 표준화
RuntimeException이 발생하면, 업로드된 객체를 보상 삭제하기 위해BinaryContentDeleteEvent를 발행하도록 구성했습니다.@Async)되어, DB 트랜잭션을 불필요하게 오래 잡고 기다리는 형태를 피합니다.소유자(권한) 검증 위치 이동 + 업로드 선행
UserService에서UserProfileFacade.updateProfile(...)로 이동했습니다.UserController는 이미지를 먼저 업로드한 뒤, 그 결과(UploadedBinaryContent)를 facade/service에 전달하여 이후 단계의 DB 갱신을 수행합니다.UserService.update(...)는 더 이상 업로드 책임/권한 검증을 직접 다루지 않고, 업로드 결과가 있을 때만binaryContentService.saveCompleted(...)로 DB를 반영하도록 계약이 바뀌었습니다.API 계약 변경: UploadedBinaryContent 중심으로 데이터 흐름 정리
UploadedBinaryContent(key, url)DTO를 도입했습니다.UserService.update및ContentService의 썸네일 처리 흐름이FileRequest중심에서 업로드 결과 전달 방식으로 변경되어, “업로드 결과 확보 → DB 반영” 순서가 코드 구조에 반영됩니다.저장 키 생성 규칙 정리
StorageKeyFactory.generate(...)에서ownerId파라미터를 제거해 키 생성 시 소유자 경로 세그먼트를 포함하지 않는 구조로 변경했습니다.동일 패턴을 콘텐츠 썸네일 업로드에도 적용
ContentController는ContentFacade를 사용하도록 전환했습니다.ContentFacade가 업로드 후ContentService.create/update에UploadedBinaryContent를 전달하는 방식으로, 사용자 프로필과 동일하게 트랜잭션 밖 업로드 + 짧은 DB 반영 + 보상 삭제 이벤트 패턴을 적용했습니다.S3 API 호출 타임아웃 설정 강화
apiCallTimeout,apiCallAttemptTimeout를ClientOverrideConfiguration으로 주입하도록 변경했습니다.S3StorageProperties에 두 타임아웃 필드를 추가하고, 값이 없으면 기본값(10초/5초)을 채우도록 했습니다.📌 리뷰 시 주의/트레이드오프
📊 변경 효과 요약 (가능한 경우에만)
BinaryContentService(+22/-6),UserController(+21/-7),UserProfileFacade(+34/0),StorageConfig(+5/0)UploadedBinaryContent,saveCompleted, facade 호출 경로)에 맞춰 갱신했습니다. 또한UserServiceImageRollbackIntegrationTest는 삭제되었습니다. (정확한 총 테스트 수 증감은 이번 PR에서 측정하지 않음)