feat: 기숙사 인증 사진 업로드 API#81
Conversation
📝 WalkthroughWalkthroughThe PR adds presigned profile and dormitory verification image uploads, restricts profile updates to deletion-only image changes, introduces semester-end calculation and expiry processing, exposes an internal calculation endpoint, and documents manual dormitory approval and rejection procedures. ChangesDormitory verification workflow
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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
🧹 Nitpick comments (5)
src/test/java/com/leets/tdd/user/controller/InternalToolControllerTest.java (1)
49-55: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the no-date calculation, not only field presence.
The test passes for any non-null timestamp, so regressions in default-date calculation or serialization would go unnoticed. Inject a fixed
Clock/business date and assert the expected semester end.🤖 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/leets/tdd/user/controller/InternalToolControllerTest.java` around lines 49 - 55, Strengthen calcDormSemesterEnd_withoutDateParam_returns200 by injecting a fixed Clock or business date into the controller’s date-calculation path, then assert that data.dorm_verified_until equals the expected semester-end value for that date instead of only checking field existence. Preserve the existing successful-response assertion.src/main/java/com/leets/tdd/global/config/SecurityConfig.java (1)
29-31: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winNarrow the anonymous matcher to the calculator route.
"/api/v1/internal/**"permits every current and future endpoint below this prefix, including any future data-changing tool. Permit onlyGET /api/v1/internal/dorm-semester-end, or verify that all endpoints under this prefix are intentionally public.Proposed narrowing
- "/api/v1/internal/**" + "/api/v1/internal/dorm-semester-end"🤖 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/leets/tdd/global/config/SecurityConfig.java` around lines 29 - 31, Update the anonymous authorization matcher in SecurityConfig to permit only GET requests for /api/v1/internal/dorm-semester-end instead of the entire /api/v1/internal/** prefix. Preserve the existing public WebSocket route and require authentication for other internal endpoints.src/main/java/com/leets/tdd/user/scheduler/DormVerificationExpiryScheduler.java (1)
35-46: 🚀 Performance & Scalability | 🔵 TrivialProcess overdue records in bounded batches.
The repository returns every overdue row as one
List, while the transaction remains open during network calls to S3. A large backlog can cause memory pressure, long locks, or transaction timeouts. Use pagination/chunking and independent cleanup processing.🤖 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/leets/tdd/user/scheduler/DormVerificationExpiryScheduler.java` around lines 35 - 46, Update the overdue-processing flow in DormVerificationExpiryScheduler to fetch and process records in bounded pages or chunks instead of loading the full result from findByDormStatusAndDormVerifiedUntilBefore. Ensure each batch is processed independently, including dormitory expiration and dormVerificationImageKey deletion, with transactions scoped per batch so S3 calls do not keep one transaction open across the entire backlog.src/test/java/com/leets/tdd/user/controller/UserControllerTest.java (1)
249-333: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover invalid request validation at the controller boundary.
Add MockMvc cases for an unsupported
contentTypeand blankkey, asserting400and noUserServiceinvocation. The current tests only exercise valid payloads and service-thrown errors.🤖 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/leets/tdd/user/controller/UserControllerTest.java` around lines 249 - 333, Extend the controller tests around presignDormVerificationUpload_success_returns200 and confirmDormVerificationUpload_success_returns200 with MockMvc cases for an unsupported contentType and a blank key. Assert each request returns 400 and verify the corresponding UserService method is never invoked, covering validation before service execution.src/main/resources/application.yaml (1)
27-30: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the unused multipart limits.
Uploads flow through presigned S3 PUTs, and no multipart upload handlers remain in the application; move size validation to the presigned-url/confirm flow if limits should still be enforced.
🤖 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/resources/application.yaml` around lines 27 - 30, Remove the servlet.multipart max-file-size and max-request-size properties from the application configuration. Do not add replacement multipart handling; retain any upload size validation only in the existing presigned URL or confirmation flow if enforcement is required.
🤖 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 `@docs/operations/dormitory-verification-admin-guide.md`:
- Around line 21-24: Add an appropriate fenced-code language identifier, such as
http or text, to the code block containing the dorm-semester-end API example,
while preserving its request and response content.
- Around line 29-35: Update the dormitory approval SQL so the named parameter in
dorm_verified_until is unquoted, using :dormVerifiedUntil directly as the bind
value; preserve the existing update behavior and other assignments.
In `@src/main/java/com/leets/tdd/user/controller/InternalToolController.java`:
- Around line 39-40: The business-time handling is inconsistent because semester
calculation and expiry checks use the JVM default timezone. In
InternalToolController, replace the omitted-date fallback with an explicit
Asia/Seoul clock/time source; in DormVerificationExpiryScheduler, use the same
Asia/Seoul time source for dormVerifiedUntil comparisons and configure the
scheduled job with zone = "Asia/Seoul" so its midnight execution uses Seoul
wall-clock time.
In `@src/main/java/com/leets/tdd/user/dto/ProfileUpdateRequest.java`:
- Around line 27-28: Update UserService.updateProfile and the
ProfileUpdateRequest flow so an omitted profileImageUrl (null) preserves the
existing image, while an explicit empty string clears it before calling
User.updateProfile. Add a regression test covering nickname/dormitory-only
updates with an existing profile image and asserting the image remains
unchanged.
In
`@src/main/java/com/leets/tdd/user/scheduler/DormVerificationExpiryScheduler.java`:
- Around line 33-46: Separate the database update flow in
expireOverdueVerifications from S3 cleanup: persist the EXPIRED state within the
transaction, then enqueue each non-null dorm verification image key for an
after-commit/outbox worker. Perform imageStorageService.delete through that
worker with idempotent retry handling, so rollbacks or later failures cannot
leave S3 deletion ahead of the database state.
In `@src/main/java/com/leets/tdd/user/service/UserService.java`:
- Around line 161-179: Update UserService.confirmProfileImageUpload at
src/main/java/com/leets/tdd/user/service/UserService.java:161-179 to capture
user.getProfileImageUrl() before user.updateProfileImageKey(key), then call
imageStorageService.delete(oldKey) after updating when the old key is non-null
and differs from key. Apply the same cleanup in the dormitory resubmit branch at
src/main/java/com/leets/tdd/user/service/UserService.java:208-240 by capturing
dormitory.getDormVerificationImageKey() before dormitory.resubmit(...) and
deleting it afterward under the same conditions.
---
Nitpick comments:
In `@src/main/java/com/leets/tdd/global/config/SecurityConfig.java`:
- Around line 29-31: Update the anonymous authorization matcher in
SecurityConfig to permit only GET requests for
/api/v1/internal/dorm-semester-end instead of the entire /api/v1/internal/**
prefix. Preserve the existing public WebSocket route and require authentication
for other internal endpoints.
In
`@src/main/java/com/leets/tdd/user/scheduler/DormVerificationExpiryScheduler.java`:
- Around line 35-46: Update the overdue-processing flow in
DormVerificationExpiryScheduler to fetch and process records in bounded pages or
chunks instead of loading the full result from
findByDormStatusAndDormVerifiedUntilBefore. Ensure each batch is processed
independently, including dormitory expiration and dormVerificationImageKey
deletion, with transactions scoped per batch so S3 calls do not keep one
transaction open across the entire backlog.
In `@src/main/resources/application.yaml`:
- Around line 27-30: Remove the servlet.multipart max-file-size and
max-request-size properties from the application configuration. Do not add
replacement multipart handling; retain any upload size validation only in the
existing presigned URL or confirmation flow if enforcement is required.
In `@src/test/java/com/leets/tdd/user/controller/InternalToolControllerTest.java`:
- Around line 49-55: Strengthen calcDormSemesterEnd_withoutDateParam_returns200
by injecting a fixed Clock or business date into the controller’s
date-calculation path, then assert that data.dorm_verified_until equals the
expected semester-end value for that date instead of only checking field
existence. Preserve the existing successful-response assertion.
In `@src/test/java/com/leets/tdd/user/controller/UserControllerTest.java`:
- Around line 249-333: Extend the controller tests around
presignDormVerificationUpload_success_returns200 and
confirmDormVerificationUpload_success_returns200 with MockMvc cases for an
unsupported contentType and a blank key. Assert each request returns 400 and
verify the corresponding UserService method is never invoked, covering
validation before service execution.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d8424c86-878c-400e-aaf0-41ddd2087834
📒 Files selected for processing (27)
docs/operations/dormitory-verification-admin-guide.mdsrc/main/java/com/leets/tdd/global/config/SecurityConfig.javasrc/main/java/com/leets/tdd/user/controller/InternalToolController.javasrc/main/java/com/leets/tdd/user/controller/UserController.javasrc/main/java/com/leets/tdd/user/domain/DormSemesterCalculator.javasrc/main/java/com/leets/tdd/user/domain/Dormitory.javasrc/main/java/com/leets/tdd/user/domain/User.javasrc/main/java/com/leets/tdd/user/dto/DormSemesterEndResponse.javasrc/main/java/com/leets/tdd/user/dto/DormVerificationConfirmRequest.javasrc/main/java/com/leets/tdd/user/dto/DormVerificationPresignRequest.javasrc/main/java/com/leets/tdd/user/dto/DormVerificationPresignResponse.javasrc/main/java/com/leets/tdd/user/dto/DormVerificationUploadResponse.javasrc/main/java/com/leets/tdd/user/dto/ProfileImageConfirmRequest.javasrc/main/java/com/leets/tdd/user/dto/ProfileImagePresignRequest.javasrc/main/java/com/leets/tdd/user/dto/ProfileImagePresignResponse.javasrc/main/java/com/leets/tdd/user/dto/ProfileImageUploadResponse.javasrc/main/java/com/leets/tdd/user/dto/ProfileUpdateRequest.javasrc/main/java/com/leets/tdd/user/exception/UserErrorCode.javasrc/main/java/com/leets/tdd/user/repository/DormitoryRepository.javasrc/main/java/com/leets/tdd/user/scheduler/DormVerificationExpiryScheduler.javasrc/main/java/com/leets/tdd/user/service/UserService.javasrc/main/resources/application.yamlsrc/test/java/com/leets/tdd/user/controller/InternalToolControllerTest.javasrc/test/java/com/leets/tdd/user/controller/UserControllerTest.javasrc/test/java/com/leets/tdd/user/domain/DormSemesterCalculatorTest.javasrc/test/java/com/leets/tdd/user/scheduler/DormVerificationExpirySchedulerTest.javasrc/test/java/com/leets/tdd/user/service/UserServiceTest.java
| UPDATE dormitory | ||
| SET dorm_status = 'APPROVED', | ||
| dorm_verified_at = NOW(), | ||
| dorm_verified_until = ':dormVerifiedUntil', -- 1번에서 받은 값 그대로 | ||
| reject_reason = NULL | ||
| WHERE user_id = :userId | ||
| AND dorm_status = 'PENDING'; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Remove the quotes around the named SQL parameter.
':dormVerifiedUntil' is interpreted as a literal string by named-parameter SQL clients, not as a bind parameter. Use dorm_verified_until = :dormVerifiedUntil; if literal substitution is intended, document replacing it with a properly quoted timestamp value.
Proposed correction
- dorm_verified_until = ':dormVerifiedUntil', -- 1번에서 받은 값 그대로
+ dorm_verified_until = :dormVerifiedUntil, -- 1번에서 받은 값 그대로📝 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.
| UPDATE dormitory | |
| SET dorm_status = 'APPROVED', | |
| dorm_verified_at = NOW(), | |
| dorm_verified_until = ':dormVerifiedUntil', -- 1번에서 받은 값 그대로 | |
| reject_reason = NULL | |
| WHERE user_id = :userId | |
| AND dorm_status = 'PENDING'; | |
| UPDATE dormitory | |
| SET dorm_status = 'APPROVED', | |
| dorm_verified_at = NOW(), | |
| dorm_verified_until = :dormVerifiedUntil, -- 1번에서 받은 값 그대로 | |
| reject_reason = NULL | |
| WHERE user_id = :userId | |
| AND dorm_status = 'PENDING'; |
🤖 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 `@docs/operations/dormitory-verification-admin-guide.md` around lines 29 - 35,
Update the dormitory approval SQL so the named parameter in dorm_verified_until
is unquoted, using :dormVerifiedUntil directly as the bind value; preserve the
existing update behavior and other assignments.
| LocalDateTime verifiedAt = (date != null ? date : LocalDate.now()).atStartOfDay(); | ||
| LocalDateTime semesterEnd = DormSemesterCalculator.calcSemesterEnd(verifiedAt); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
rg -n 'spring\.jpa.*timezone|user\.timezone|TimeZone|ZoneId|TZ|`@Scheduled`' \
src/main/resources/application.yaml src/main/javaRepository: Leets-Official/TDD-BE
Length of output: 392
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== relevant files =="
git ls-files 'src/main/java/com/leets/tdd/user/controller/InternalToolController.java' \
'src/main/java/com/leets/tdd/user/scheduler/DormVerificationExpiryScheduler.java' \
'src/main/java/com/leets/tdd/user/**' \
'src/main/java/com/leets/tdd/global/**' \
'src/main/resources/**' \
'build.gradle' 'build.gradle.kts' 'pom.xml'
echo
echo "== application configs =="
for f in $(git ls-files 'src/main/resources/**'); do
echo "--- $f"
sed -n '1,220p' "$f"
done
echo
echo "== controller excerpt =="
sed -n '1,90p' src/main/java/com/leets/tdd/user/controller/InternalToolController.java | cat -n
echo
echo "== scheduler excerpts =="
sed -n '1,90p' src/main/java/com/leets/tdd/user/scheduler/DormVerificationExpiryScheduler.java | cat -n
sed -n '1,90p' src/main/java/com/leets/tdd/global/jwt/RefreshTokenCleanupScheduler.java | cat -n
echo
echo "== dorm semester calculator excerpt =="
rg -n "class DormSemesterCalculator|calcSemesterEnd|verifiedAt" src/main/java
sed -n '1,160p' src/main/java/com/leets/tdd/user/calculator/DormSemesterCalculator.java | cat -n
echo
echo "== repository query =="
rg -n "findByDormStatusAndDormVerifiedUntilBefore|verifyUntil|DormStatus" src/main/java/com/leets/tdd/user src/main/java/com/leets/tdd/global -g '*.java'Repository: Leets-Official/TDD-BE
Length of output: 28830
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== DormSemesterCalculator =="
cat -n src/main/java/com/leets/tdd/user/domain/DormSemesterCalculator.java
echo
echo "== scheduler configuration search =="
rg -n "EnableScheduling|spring\.task\s|\btask\s" src/main/java src/test/java build.gradle pom.xml || true
echo
echo "== repository query implementation =="
sed -n '1,180p' src/main/java/com/leets/tdd/user/repository/DormitoryRepository.java | cat -n
echo
echo "== Dormitory entity date fields =="
rg -n "dormVerifiedUntil|verified_at|dorm_verified_until|LocalDateTime|LocalDate" src/main/java/com/leets/tdd/user/domain/Dormitory.java src/main/java/com/leets/tdd/user/repository/DormitoryRepository.java src/main/java/com/leets/tdd/user/scheduler/DormVerificationExpiryScheduler.java -C 2Repository: Leets-Official/TDD-BE
Length of output: 8885
Use a consistent business “now” for semester/expiry checks.
InternalToolController defaults omitted dates with LocalDate.now() (JVM default timezone) before calculating semester end, and DormVerificationExpiryScheduler compares dormVerifiedUntil with LocalDateTime.now(). Since the app sets jackson.time-zone: Asia/Seoul and the cron is wall-clock midnight, use an explicit Asia/Seoul clock/@Scheduled(zone = "Asia/Seoul") so semester calculation and daily expiry don’t drift with the JVM default timezone.
📍 Affects 2 files
src/main/java/com/leets/tdd/user/controller/InternalToolController.java#L39-L40(this comment)src/main/java/com/leets/tdd/user/scheduler/DormVerificationExpiryScheduler.java#L32-L36
🤖 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/leets/tdd/user/controller/InternalToolController.java`
around lines 39 - 40, The business-time handling is inconsistent because
semester calculation and expiry checks use the JVM default timezone. In
InternalToolController, replace the omitted-date fallback with an explicit
Asia/Seoul clock/time source; in DormVerificationExpiryScheduler, use the same
Asia/Seoul time source for dormVerifiedUntil comparisons and configure the
scheduled job with zone = "Asia/Seoul" so its midnight execution uses Seoul
wall-clock time.
| // null이거나 빈 문자열이어야만 통과한다(프로필 사진 해제 전용) - 값이 있으면 무조건 거부. | ||
| @Pattern(regexp = "^$", message = "프로필 사진은 업로드 API로만 설정할 수 있습니다. 여기서는 삭제(빈 값)만 가능합니다.") |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick win
Every profile edit now silently wipes the user's photo — no way to preserve it.
Since this constraint only ever lets profileImageUrl be null or "", and UserService.updateProfile maps both to null unconditionally before calling User.updateProfile(nickname, null), calling PATCH /me/profile to change just the nickname or dormitory will always clear the existing profile photo. Before this PR a client could resend its current URL to no-op preserve the image; that workaround is now blocked, so there is no remaining way to edit nickname/dormitory without losing the photo.
Consider distinguishing "field omitted" (null → leave unchanged) from an explicit clear signal ("" → delete), e.g. in UserService.updateProfile:
💡 Proposed fix
- String newProfileImageUrl = request.profileImageUrl();
- user.updateProfile(newNickname, newProfileImageUrl == null || newProfileImageUrl.isBlank()
- ? null : newProfileImageUrl);
+ String newProfileImageUrl = request.profileImageUrl();
+ if (newProfileImageUrl == null) {
+ user.updateProfile(newNickname, user.getProfileImageUrl());
+ } else {
+ user.updateProfile(newNickname, newProfileImageUrl.isBlank() ? null : newProfileImageUrl);
+ }Please also add a regression test asserting that an existing profileImageUrl survives a nickname/dormitory-only update.
🤖 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/leets/tdd/user/dto/ProfileUpdateRequest.java` around lines
27 - 28, Update UserService.updateProfile and the ProfileUpdateRequest flow so
an omitted profileImageUrl (null) preserves the existing image, while an
explicit empty string clears it before calling User.updateProfile. Add a
regression test covering nickname/dormitory-only updates with an existing
profile image and asserting the image remains unchanged.
| @Transactional | ||
| public void expireOverdueVerifications() { | ||
| List<Dormitory> overdue = dormitoryRepository | ||
| .findByDormStatusAndDormVerifiedUntilBefore(DormStatus.APPROVED, LocalDateTime.now()); | ||
| if (overdue.isEmpty()) { | ||
| return; | ||
| } | ||
|
|
||
| for (Dormitory dormitory : overdue) { | ||
| String imageKey = dormitory.getDormVerificationImageKey(); | ||
| dormitory.expire(); | ||
| if (imageKey != null) { | ||
| imageStorageService.delete(imageKey); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Do not perform S3 deletion inside the database transaction.
@Transactional can roll back the EXPIRED updates, but it cannot roll back imageStorageService.delete(...). If a later deletion or database commit fails, S3 and the database can disagree. Move cleanup to an outbox/after-commit worker with idempotent retries, or otherwise separate expiry persistence from storage deletion.
🤖 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/leets/tdd/user/scheduler/DormVerificationExpiryScheduler.java`
around lines 33 - 46, Separate the database update flow in
expireOverdueVerifications from S3 cleanup: persist the EXPIRED state within the
transaction, then enqueue each non-null dorm verification image key for an
after-commit/outbox worker. Perform imageStorageService.delete through that
worker with idempotent retry handling, so rollbacks or later failures cannot
leave S3 deletion ahead of the database state.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/leets/tdd/user/service/UserServiceTest.java`:
- Around line 478-491: Update confirmProfileImageUpload so deletion of the
previous profile image key occurs only after the transaction commits, using the
existing transactional event or synchronization mechanism rather than calling
imageStorageService.delete synchronously. Revise
confirmProfileImageUpload_replacesExistingPhoto_deletesOldKey to verify deletion
is scheduled/deferred and executed after commit.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 60c597fc-2155-44ac-b39b-ca5faa5aee53
📒 Files selected for processing (7)
docs/operations/dormitory-verification-admin-guide.mdsrc/main/java/com/leets/tdd/user/scheduler/DormVerificationExpiryScheduler.javasrc/main/java/com/leets/tdd/user/service/DormVerificationExpiryService.javasrc/main/java/com/leets/tdd/user/service/UserService.javasrc/test/java/com/leets/tdd/user/scheduler/DormVerificationExpirySchedulerTest.javasrc/test/java/com/leets/tdd/user/service/DormVerificationExpiryServiceTest.javasrc/test/java/com/leets/tdd/user/service/UserServiceTest.java
🚧 Files skipped from review as they are similar to previous changes (2)
- docs/operations/dormitory-verification-admin-guide.md
- src/main/java/com/leets/tdd/user/service/UserService.java
| @Test | ||
| @DisplayName("기존 프로필 사진이 있었으면 새 사진 반영 후 예전 사진을 S3에서 지운다") | ||
| void confirmProfileImageUpload_replacesExistingPhoto_deletesOldKey() { | ||
| User user = newUser(); | ||
| user.updateProfileImageKey("profiles/1/old.jpg"); | ||
| when(userRepository.findById(1L)).thenReturn(Optional.of(user)); | ||
| when(imageStorageService.resolveViewUrl(PROFILE_KEY)) | ||
| .thenReturn("https://public.example.com/" + PROFILE_KEY); | ||
|
|
||
| userService.confirmProfileImageUpload(1L, new ProfileImageConfirmRequest(PROFILE_KEY)); | ||
|
|
||
| assertThat(user.getProfileImageUrl()).isEqualTo(PROFILE_KEY); | ||
| verify(imageStorageService).delete("profiles/1/old.jpg"); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate test and service files =="
git ls-files | rg 'src/test/java/com/leets/tdd/user/service/UserServiceTest\.java|com/leets/tdd/user/service/UserService\.java|imageStorageService|Repository' | sed -n '1,200p'
echo
echo "== test lines =="
sed -n '440,510p' src/test/java/com/leets/tdd/user/service/UserServiceTest.java
echo
echo "== service references =="
rg -n "confirmProfileImageUpload|Transactional|`@Tx`|ProfileImage|userRepository|imageStorageService|save|delete" src/main src/test -g '*.java' | sed -n '1,260p'Repository: Leets-Official/TDD-BE
Length of output: 38531
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== UserService confirm method =="
sed -n '150,195p' src/main/java/com/leets/tdd/user/service/UserService.java
echo
echo "== User profile image methods =="
sed -n '220,245p' src/main/java/com/leets/tdd/user/domain/User.java
echo
echo "== Test setup and imports around UserServiceTest =="
sed -n '1,110p' src/test/java/com/leets/tdd/user/service/UserServiceTest.java
echo
echo "== imports for Spring Transaction in test/service =="
rg -n "import org\.springframework\.transaction|TestableTransactionManager|AbstractTransactionTest|TransactionalTestExecutionListener|Transactional|`@TestableTransaction`\b" src/test src/main -g '*.java' | sed -n '1,240p'Repository: Leets-Official/TDD-BE
Length of output: 13523
🌐 Web query:
Spring Framework TransactionAwareDataSourcePlatformTransactionManager after commit callback TransactionSynchronizationManager registerSynchronization
💡 Result:
In the Spring Framework, you can register an after-commit callback by implementing the TransactionSynchronization interface and registering it with the TransactionSynchronizationManager [1][2]. The TransactionSynchronization interface provides the afterCommit method, which is invoked immediately after a transaction has successfully committed [3][4]. This is the appropriate place to perform post-commit actions, such as sending confirmation emails or triggering follow-up tasks that should only occur if the main transaction is successful [2][4]. To register your synchronization, use the following approach: TransactionSynchronizationManager.registerSynchronization(new TransactionSynchronization { @Override public void afterCommit { // Perform post-commit actions here } }); Important Considerations: 1. Lifecycle: While the main transaction has already committed when afterCommit is called, the transactional resources (like database connections) are often still active and accessible [2][5]. 2. Participating in Transactions: Because resources may still be active, any data access code executed within afterCommit will "participate" in the original transaction, meaning no further commits will occur for it [3][2]. If you need to perform additional transactional work that requires its own transaction, you must explicitly use PROPAGATION_REQUIRES_NEW [2][4]. 3. Registration: You must ensure that transaction synchronization is active before calling registerSynchronization by checking TransactionSynchronizationManager.isSynchronizationActive [1][6]. If it is not active, the registration will throw an IllegalStateException [1][7]. 4. Callback Order: If you have multiple synchronizations registered, they are executed in the order of their assigned priority if they implement the Ordered interface [1][6]. The TransactionAwareDataSourcePlatformTransactionManager (and other Spring transaction managers) handles these callbacks automatically through their internal commit process, which triggers the registered synchronizations once the actual transaction commit has finished [8][9].
Citations:
- 1: https://docs.spring.io/spring-framework/docs/7.0.7/javadoc-api/org/springframework/transaction/support/TransactionSynchronizationManager.html
- 2: https://docs.spring.io/spring-framework/docs/current/javadoc-api/org/springframework/transaction/support/TransactionSynchronization.html
- 3: https://docs.spring.io/spring-framework/docs/6.0.14/javadoc-api/org/springframework/transaction/support/TransactionSynchronization.html
- 4: https://github.com/spring-projects/spring-framework/blob/master/spring-tx/src/main/java/org/springframework/transaction/support/TransactionSynchronization.java
- 5: https://docs.spring.io/spring-framework/docs/6.1.5/javadoc-api/org/springframework/transaction/support/TransactionSynchronization.html
- 6: https://docs.spring.io/spring-framework/docs/6.0.3/javadoc-api/org/springframework/transaction/support/TransactionSynchronizationManager.html
- 7: https://docs.spring.io/spring-framework/docs/5.3.13/javadoc-api/org/springframework/transaction/support/TransactionSynchronizationManager.html
- 8: https://github.com/spring-projects/spring-framework/blob/main/spring-tx/src/main/java/org/springframework/transaction/support/AbstractPlatformTransactionManager.java
- 9: https://docs.spring.io/spring-framework/docs/7.0.6/javadoc-api/org/springframework/transaction/support/AbstractPlatformTransactionManager.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Repository implementation/transaction config candidates =="
git ls-files | rg 'src/(main|test)/java|src/(main|test)/resources' | rg 'Transaction\.(java|properties$|xml$)|config|jpa|database|Testcontainers|Mockito|`@Enable`' | sed -n '1,200p'
echo
echo "== build dependency for testcontainers/spring test =="
if [ -f build.gradle ]; then cat build.gradle | sed -n '1,220p'; fi
if [ -f build.gradle.kts ]; then cat build.gradle.kts | sed -n '1,220p'; fi
echo
echo "== any existing after-commit / event / transaction sync usages =="
rg -n "TransactionSynchronization|afterCommit|ApplicationEventPublisher|EventTransaction|TransactionalEventListener|afterCommit|TestableTransactionManager|MockExternalResource|TransactionTemplate|MockitoExtension|TestTemplate" src/main src/test -g '*.java' | sed -n '1,200p'Repository: Leets-Official/TDD-BE
Length of output: 6084
Defer old profile-image deletion to an after-commit cleanup.
confirmProfileImageUpload is transactional, so userRepository.save(user) is only committed after the method returns. Calling imageStorageService.delete(previousKey) in the transaction puts the new profile at risk of pointing to a deleted S3 object if the transaction rolls back afterward. Move the delete out of the transaction, e.g. a transactional event/synchronization handler, and make this test assert that deletion is scheduled/deferred instead of emitted synchronously.
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 484-484: Avoid building a URL host from untrusted input
Context: "https://public.example.com/" + PROFILE_KEY
Note: [CWE-20] Improper Input Validation.
(tainted-url-host)
🤖 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/leets/tdd/user/service/UserServiceTest.java` around lines
478 - 491, Update confirmProfileImageUpload so deletion of the previous profile
image key occurs only after the transaction commits, using the existing
transactional event or synchronization mechanism rather than calling
imageStorageService.delete synchronously. Revise
confirmProfileImageUpload_replacesExistingPhoto_deletesOldKey to verify deletion
is scheduled/deferred and executed after commit.
| private boolean belongsToUser(ImageCategory category, Long userId, String key) { | ||
| if (key == null) { | ||
| return false; | ||
| } | ||
| String expectedPrefix = "%s/%d/".formatted(category.getPrefix(), userId); | ||
| if (!key.startsWith(expectedPrefix)) { | ||
| return false; | ||
| } | ||
| String remainder = key.substring(expectedPrefix.length()); | ||
| return !remainder.isEmpty() && !remainder.contains("/") && !remainder.contains(".."); | ||
| } | ||
|
|
There was a problem hiding this comment.
이 소유권 검증 방식 참고하겠습니다. 채팅 이미지는 여기가 "이 유저가 이 팟 참여자인지"(party_participants 조회) 검증으로 바뀔 것 같은데, 참여 API 나오면 비슷한 위치에 넣으려고 합니다. 수고하셨어요!
2 participants
작업 내용
변경 사항
리뷰 포인트
체크리스트
Summary by CodeRabbit