Conversation
WalkthroughupdateGuardian 요청 DTO를 CreateGuardianRequest에서 UpdateGuardianRequest로 교체하고, 프로필 이미지 업데이트를 트랜잭션 동기화(afterCommit/afterCompletion) 기반으로 재구성해 업로드/삭제 시점을 제어하며, 기본 이미지 설정과 신규 업로드의 충돌을 감지해 UPDATE_PROFILE_CONFLICT를 도입했습니다. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant GuardianApi
participant GuardianService
participant Tx as TransactionManager
participant Store as ImageStorage
Client->>GuardianApi: PUT /guardians/{id} (UpdateGuardianRequest)
GuardianApi->>GuardianService: updateGuardian(user, id, request)
GuardianService->>Tx: begin transaction
alt request.isDefaultImage == true and hasNewFile == true
GuardianService-->>GuardianService: throw UPDATE_PROFILE_CONFLICT
else request.isDefaultImage == true
GuardianService-->>GuardianService: nextKey = null
GuardianService->>Tx: register afterCommit: delete(oldKey)
else hasNewFile == true
GuardianService->>Store: upload(newFile)
Store-->>GuardianService: newKey
GuardianService->>Tx: register afterCommit: delete(oldKey)
GuardianService->>Tx: register afterCompletion(rollback -> delete newKey)
GuardianService-->>GuardianService: nextKey = newKey
else
GuardianService-->>GuardianService: nextKey = currentKey
end
GuardianService->>Tx: commit
Tx-->>GuardianService: afterCommit callbacks
GuardianService->>Store: delete(oldKey) [if registered]
GuardianService-->>GuardianApi: return GuardianResponse
GuardianApi-->>Client: 200 GUARDIAN_UPDATED
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Possibly related PRs
Poem
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
src/main/java/com/blockguard/server/domain/guardian/api/GuardianApi.java (1)
51-53: UpdateGuardianRequest로의 전환 적절 + 문서/바인딩 명시 제안
- DTO 교체와 서비스 호출 변경 모두 일관성 있게 반영되어 있습니다.
- Swagger 사용자를 위해 isDefaultImage 동작(파일 미제공 시 기본이미지 전환/유지 로직)을 메서드 설명이나 파라미터 설명에 추가하면 좋습니다.
- Multipart 바인딩을 명확히 하려면 @ModelAttribute를 붙이는 것도 고려해주세요.
- @Valid UpdateGuardianRequest updateGuardianRequest) { + @Valid @ModelAttribute UpdateGuardianRequest updateGuardianRequest) {(필요 시 import org.springframework.web.bind.annotation.ModelAttribute 추가)
src/main/java/com/blockguard/server/domain/guardian/application/GuardianService.java (2)
85-91: S3 업로드 후 정리 로직 훌륭함 — but 삭제 실패 예외 처리 추가 권장트랜잭션 롤백 시 신규 업로드 파일 제거, 커밋 후 구키 삭제 설계 좋습니다. 다만 afterCommit/afterCompletion에서 s3Service.delete 예외가 발생하면 로깅 없이 전파되어 예기치 않은 에러를 유발할 수 있습니다. try-catch로 감싸고 로깅 처리하세요.
- @Override public void afterCommit() { - if (oldKey != null && !oldKey.equals(uploadedKey)) { - s3Service.delete(oldKey); - } - } + @Override public void afterCommit() { + if (oldKey != null && !oldKey.equals(uploadedKey)) { + try { + s3Service.delete(oldKey); + } catch (Exception e) { + // TODO: 로깅 (예: log.warn("S3 delete after commit failed. key={}", oldKey, e)) + } + } + } ... - @Override public void afterCompletion(int status) { - if (status == STATUS_ROLLED_BACK && uploadedKey != null) { - s3Service.delete(uploadedKey); - } - } + @Override public void afterCompletion(int status) { + if (status == STATUS_ROLLED_BACK && uploadedKey != null) { + try { + s3Service.delete(uploadedKey); + } catch (Exception e) { + // TODO: 로깅 (예: log.warn("S3 delete after rollback failed. key={}", uploadedKey, e)) + } + } + }참고: 로깅을 위해 클래스에 @slf4j 추가가 필요합니다.
추가 코드(파일 상단, 클래스 어노테이션):import lombok.extern.slf4j.Slf4j; @Slf4j @Service @RequiredArgsConstructor public class GuardianService { ... }
98-114: 동기화 등록 전 가드 추가(미세 개선) 및 상수화 제안
- 드물지만 트랜잭션 동기화가 비활성인 환경에서 호출될 수 있으므로 방어적으로 활성 여부 체크를 권장합니다.
- 디렉터리 문자열 "guardians"를 상수로 관리하면 오타/변경에 안전합니다.
- private void registerAfterCommitDelete(String oldKey, String uploadedKey) { - org.springframework.transaction.support.TransactionSynchronizationManager - .registerSynchronization(new org.springframework.transaction.support.TransactionSynchronization() { + private void registerAfterCommitDelete(String oldKey, String uploadedKey) { + if (!org.springframework.transaction.support.TransactionSynchronizationManager.isSynchronizationActive()) { + // 동기화 미활성 환경에서는 베스트 에포트(예: 롤백 시 업로드 파일 제거) 처리만 수행하거나 로그 후 반환 + return; + } + org.springframework.transaction.support.TransactionSynchronizationManager + .registerSynchronization(new org.springframework.transaction.support.TransactionSynchronization() { @Override public void afterCommit() { if (oldKey != null && !oldKey.equals(uploadedKey)) { s3Service.delete(oldKey); } } @Override public void afterCompletion(int status) { if (status == STATUS_ROLLED_BACK && uploadedKey != null) { s3Service.delete(uploadedKey); } } }); }또한 문자열 상수화(참고, 변경은 다른 라인에도 적용 필요):
private static final String S3_DIR_GUARDIANS = "guardians";업로드 호출부 변경:
-String uploadedKey = s3Service.upload(request.getProfileImage(), "guardians"); +String uploadedKey = s3Service.upload(request.getProfileImage(), S3_DIR_GUARDIANS);그리고 createGuardian(...)의 업로드 코드에도 동일 상수 적용을 권장합니다.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/main/java/com/blockguard/server/domain/guardian/api/GuardianApi.java(2 hunks)src/main/java/com/blockguard/server/domain/guardian/application/GuardianService.java(2 hunks)src/main/java/com/blockguard/server/domain/guardian/dto/request/UpdateGuardianRequest.java(1 hunks)src/main/java/com/blockguard/server/global/config/swagger/SwaggerResponseDescription.java(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: yeonju73
PR: Block-Guard/BlockGuard-Server#32
File: src/main/java/com/blockguard/server/domain/fraud/dto/request/FraudUrlRequest.java:6-10
Timestamp: 2025-07-29T05:20:31.567Z
Learning: yeonju73은 DTO 수준에서의 입력 필드 검증과 해당하는 에러 코드 추가를 선호한다.
🧬 Code Graph Analysis (4)
src/main/java/com/blockguard/server/domain/guardian/api/GuardianApi.java (2)
src/main/java/com/blockguard/server/domain/guardian/domain/Guardian.java (1)
updateGuardianInfo(48-52)src/main/java/com/blockguard/server/domain/guardian/dto/request/CreateGuardianRequest.java (1)
CreateGuardianRequest(11-24)
src/main/java/com/blockguard/server/domain/guardian/dto/request/UpdateGuardianRequest.java (5)
src/main/java/com/blockguard/server/domain/guardian/dto/request/CreateGuardianRequest.java (1)
CreateGuardianRequest(11-24)src/main/java/com/blockguard/server/domain/user/dto/request/UpdateUserInfo.java (1)
Getter(12-33)src/main/java/com/blockguard/server/domain/guardian/dto/request/UpdateGuardianPrimaryRequest.java (1)
UpdateGuardianPrimaryRequest(10-18)src/main/java/com/blockguard/server/domain/guardian/domain/Guardian.java (2)
updateGuardianInfo(48-52)Guardian(11-57)src/main/java/com/blockguard/server/domain/guardian/dto/response/GuardianResponse.java (1)
GuardianResponse(12-42)
src/main/java/com/blockguard/server/global/config/swagger/SwaggerResponseDescription.java (1)
src/main/java/com/blockguard/server/global/config/swagger/SwaggerConfig.java (3)
addGlobalErrorResponses(124-138)generateErrorCodeResponseExample(80-98)SwaggerConfig(28-139)
src/main/java/com/blockguard/server/domain/guardian/application/GuardianService.java (2)
src/main/java/com/blockguard/server/domain/guardian/domain/Guardian.java (2)
updateGuardianInfo(48-52)Guardian(11-57)src/main/java/com/blockguard/server/domain/guardian/dto/response/GuardianResponse.java (1)
GuardianResponse(12-42)
🔇 Additional comments (3)
src/main/java/com/blockguard/server/global/config/swagger/SwaggerResponseDescription.java (1)
73-75: UPDATE_GUARDIAN_FAIL에 UPDATE_PROFILE_CONFLICT 추가 적절서비스에서 충돌 상황을 명확히 에러 코드로 노출하도록 정리된 점 좋습니다. Swagger 예시에도 반영되어 문서 일관성이 좋아집니다.
src/main/java/com/blockguard/server/domain/guardian/application/GuardianService.java (2)
58-58: 시그니처 변경(새 DTO) 반영 OK컨트롤러 변경과 일치하며 트랜잭션 경계 내에서 처리되는 흐름이 명확합니다.
66-69: 입력 파생 상태 계산(wantDefault/hasNewFile) 명료null-safe(Boolean.TRUE.equals) 처리 및 MultipartFile empty 체크 적절합니다.
Comment on lines
+71
to
73
| if (wantDefault && hasNewFile) { | ||
| throw new BusinessExceptionHandler(ErrorCode.UPDATE_PROFILE_CONFLICT); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
충돌 검증을 DTO 레벨로 이동 권장
현재 서비스에서 UPDATE_PROFILE_CONFLICT를 던지지만, DTO 클래스 레벨 제약으로 조기 검증하면 컨트롤러 진입 시점에서 실패시켜 응답 일관성과 문서화가 좋아집니다. 기존 선호도(입력 단계 검증)와도 부합합니다. 위 DTO 코멘트의 커스텀 제약을 적용해보세요.
또한 해당 케이스에 대한 단위/통합 테스트 추가를 제안드립니다:
- isDefaultImage=true + 파일 제공 → 409 계열 오류(UPDATE_PROFILE_CONFLICT)
- isDefaultImage=true + 파일 미제공 → 기존 이미지 삭제
- isDefaultImage=null/false + 파일 미제공 → 기존 이미지 유지
- 파일 제공만 → 기존 이미지 교체 및 롤백 시 새 파일 삭제
원하시면 테스트 스켈레톤을 생성해드리겠습니다.
🤖 Prompt for AI Agents
In
src/main/java/com/blockguard/server/domain/guardian/application/GuardianService.java
around lines 71-73, the conflict check that throws UPDATE_PROFILE_CONFLICT
should be moved to input-level validation: add a custom Bean Validation
constraint on the DTO (e.g., @NoFileWhenDefaultImage or @ValidProfileImageState)
that enforces isDefaultImage==true cannot be submitted together with a non-empty
MultipartFile; implement the ConstraintValidator to inspect the DTO fields
(treat MultipartFile null/empty as no file), annotate the DTO and ensure @Valid
is used on the controller method so validation happens before reaching the
service, then remove the duplicate service-level check. Also add
unit/integration tests for the four cases the reviewer listed
(isDefaultImage=true + file -> expect 409/constraint violation,
isDefaultImage=true + no file -> existing image deleted behavior,
isDefaultImage=null/false + no file -> existing image retained, file-only upload
-> image replaced and on rollback new file deleted).
Comment on lines
+11
to
+14
| @Getter | ||
| @AllArgsConstructor | ||
| @Builder | ||
| public class UpdateGuardianRequest { |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
폼데이터(@ModelAttribute) 바인딩 안정성 확보: @NoArgsConstructor, @Setter 권장
현재 DTO는 @Getter, @AllArgsConstructor, @builder만 있어 기본 생성자/세터 없이 Multipart/form-data 바인딩 시 실패할 가능성이 있습니다(프로젝트에 커스텀 바인더가 없다면). 안전하게 다음을 권장합니다.
package com.blockguard.server.domain.guardian.dto.request;
import io.swagger.v3.oas.annotations.media.Schema;
import jakarta.validation.constraints.NotBlank;
import jakarta.validation.constraints.Pattern;
+import jakarta.validation.constraints.Size;
import lombok.AllArgsConstructor;
import lombok.Builder;
import lombok.Getter;
+import lombok.NoArgsConstructor;
+import lombok.Setter;
import org.springframework.web.multipart.MultipartFile;
-@Getter
-@AllArgsConstructor
-@Builder
+@Getter
+@Setter
+@NoArgsConstructor
+@AllArgsConstructor
+@Builder
public class UpdateGuardianRequest {검증 스크립트(레포에서 DTO 바인딩 관례 확인):
🏁 Script executed:
#!/bin/bash
# DTO들에 @NoArgsConstructor/@Setter 사용 사례 확인
rg -n "@NoArgsConstructor|@Setter" src/main/java | head -n 50
# 기존 CreateGuardianRequest도 동일 패턴인지 확인
rg -n "class CreateGuardianRequest" -A 20 src/main/javaLength of output: 3844
🏁 Script executed:
#!/bin/bash
# Locate and display UpdateGuardianRequest annotations and fields
file_update=$(rg -l "class UpdateGuardianRequest" -g "*.java")
echo "== File: $file_update =="
sed -n '1,50p' "$file_update"
# Locate and display CreateGuardianRequest annotations and fields for comparison
file_create=$(rg -l "class CreateGuardianRequest" -g "*.java")
echo "== File: $file_create =="
sed -n '1,50p' "$file_create"Length of output: 2412
UpdateGuardianRequest에 @NoArgsConstructor 및 @Setter 추가로 폼데이터(@ModelAttribute) 바인딩 안정화
현재 DTO는 @Getter, @AllArgsConstructor, @Builder만 선언되어 있어 Spring MVC(@ModelAttribute) 형태의 Multipart/form-data 바인딩 시 기본 생성자 또는 세터 메서드 부재로 인스턴스 생성·속성 주입이 실패할 가능성이 있습니다. 다음과 같이 @NoArgsConstructor와 @Setter를 추가해주세요.
- 대상 파일:
- src/main/java/com/blockguard/server/domain/guardian/dto/request/UpdateGuardianRequest.java
@@
-import lombok.AllArgsConstructor;
-import lombok.Builder;
-import lombok.Getter;
+import lombok.AllArgsConstructor;
+import lombok.Builder;
+import lombok.Getter;
+import lombok.NoArgsConstructor;
+import lombok.Setter;
-@Getter
-@AllArgsConstructor
-@Builder
+@Getter
+@Setter
+@NoArgsConstructor
+@AllArgsConstructor
+@Builder
public class UpdateGuardianRequest {
@NotBlank
private String name;
…참고: CreateGuardianRequest도 동일 패턴이므로, 함께 적용/검토하시면 좋습니다.
📝 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.
Suggested change
| @Getter | |
| @AllArgsConstructor | |
| @Builder | |
| public class UpdateGuardianRequest { | |
| // --- imports at the top of the file --- | |
| import lombok.AllArgsConstructor; | |
| import lombok.Builder; | |
| import lombok.Getter; | |
| import lombok.NoArgsConstructor; | |
| import lombok.Setter; | |
| @Getter | |
| @Setter | |
| @NoArgsConstructor | |
| @AllArgsConstructor | |
| @Builder | |
| public class UpdateGuardianRequest { | |
| @NotBlank | |
| private String name; | |
| … | |
| } |
🤖 Prompt for AI Agents
In
src/main/java/com/blockguard/server/domain/guardian/dto/request/UpdateGuardianRequest.java
around lines 11 to 14, the DTO only has @Getter, @AllArgsConstructor, and
@Builder which prevents Spring MVC @ModelAttribute multipart/form-data binding
from instantiating and populating the object; add Lombok's @NoArgsConstructor
and @Setter annotations to the class so a no-arg constructor and setters are
generated, enabling proper form-data binding (also review and apply the same
change to CreateGuardianRequest).
Comment on lines
+22
to
+26
| @Schema(requiredMode = Schema.RequiredMode.NOT_REQUIRED) | ||
| private MultipartFile profileImage; | ||
|
|
||
| private Boolean isDefaultImage; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
상호 배타 입력을 DTO 레벨 검증으로 끌어올리기
서비스에서 처리 중인 “isDefaultImage=true와 profileImage 동시 제공” 충돌을 DTO 클래스 레벨 검증으로 옮기면 요청 조기 실패와 문서화 측면에서 이점이 큽니다. 과거 선호도(입력 필드 검증 선호)에 부합합니다.
- 제안: 클래스 레벨 커스텀 제약 추가
- 어노테이션: @ExclusiveProfileImageUpdate
- 규칙: isDefaultImage == true 이면 profileImage는 비워야 함
예시 구현(새 파일):
Annotation:
@Target(ElementType.TYPE)
@Retention(RetentionPolicy.RUNTIME)
@Constraint(validatedBy = ExclusiveProfileImageUpdateValidator.class)
public @interface ExclusiveProfileImageUpdate {
String message() default "isDefaultImage and profileImage cannot be used together";
Class<?>[] groups() default {};
Class<? extends Payload>[] payload() default {};
}Validator:
public class ExclusiveProfileImageUpdateValidator
implements ConstraintValidator<ExclusiveProfileImageUpdate, UpdateGuardianRequest> {
@Override
public boolean isValid(UpdateGuardianRequest value, ConstraintValidatorContext ctx) {
boolean wantDefault = Boolean.TRUE.equals(value.getIsDefaultImage());
boolean hasNewFile = value.getProfileImage() != null && !value.getProfileImage().isEmpty();
return !(wantDefault && hasNewFile);
}
}사용:
@ExclusiveProfileImageUpdate
public class UpdateGuardianRequest { ... }또한 Swagger 설명 보강을 권장합니다.
- private Boolean isDefaultImage;
+ @Schema(description = "프로필 기본이미지로 되돌릴지 여부(파일 미제공 시 동작 제어). true면 기존 이미지 제거, false면 기존 이미지 유지", requiredMode = Schema.RequiredMode.NOT_REQUIRED, example = "false")
+ private Boolean isDefaultImage;필요하시면 어노테이션/밸리데이터 전체 PR 패치까지 만들어드리겠습니다.
🤖 Prompt for AI Agents
In
src/main/java/com/blockguard/server/domain/guardian/dto/request/UpdateGuardianRequest.java
around lines 22-26, add a class-level custom constraint to prevent simultaneous
use of isDefaultImage=true and a provided profileImage: create an annotation
@ExclusiveProfileImageUpdate and a
ConstraintValidator<ExclusiveProfileImageUpdate, UpdateGuardianRequest> that
returns false when Boolean.TRUE.equals(getIsDefaultImage()) and
getProfileImage() is non-null and not empty; annotate UpdateGuardianRequest with
@ExclusiveProfileImageUpdate and update the Swagger/Schema docs for profileImage
and isDefaultImage to clarify mutual exclusivity.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/main/java/com/blockguard/server/domain/guardian/api/GuardianApi.java(2 hunks)src/main/java/com/blockguard/server/domain/guardian/application/GuardianService.java(2 hunks)src/main/java/com/blockguard/server/domain/guardian/dto/request/UpdateGuardianRequest.java(1 hunks)src/main/java/com/blockguard/server/global/config/swagger/SwaggerResponseDescription.java(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: yeonju73
PR: Block-Guard/BlockGuard-Server#32
File: src/main/java/com/blockguard/server/domain/fraud/dto/request/FraudUrlRequest.java:6-10
Timestamp: 2025-07-29T05:20:31.567Z
Learning: yeonju73은 DTO 수준에서의 입력 필드 검증과 해당하는 에러 코드 추가를 선호한다.
🧬 Code Graph Analysis (4)
src/main/java/com/blockguard/server/global/config/swagger/SwaggerResponseDescription.java (1)
src/main/java/com/blockguard/server/domain/guardian/domain/Guardian.java (1)
updateGuardianInfo(48-52)
src/main/java/com/blockguard/server/domain/guardian/api/GuardianApi.java (2)
src/main/java/com/blockguard/server/domain/guardian/dto/request/UpdateGuardianPrimaryRequest.java (1)
UpdateGuardianPrimaryRequest(10-18)src/main/java/com/blockguard/server/domain/guardian/dto/request/CreateGuardianRequest.java (1)
CreateGuardianRequest(11-24)
src/main/java/com/blockguard/server/domain/guardian/dto/request/UpdateGuardianRequest.java (5)
src/main/java/com/blockguard/server/domain/guardian/dto/request/CreateGuardianRequest.java (1)
CreateGuardianRequest(11-24)src/main/java/com/blockguard/server/domain/guardian/dto/request/UpdateGuardianPrimaryRequest.java (1)
UpdateGuardianPrimaryRequest(10-18)src/main/java/com/blockguard/server/domain/user/dto/request/UpdateUserInfo.java (1)
Getter(12-33)src/main/java/com/blockguard/server/domain/guardian/domain/Guardian.java (2)
updateGuardianInfo(48-52)Guardian(11-57)src/main/java/com/blockguard/server/domain/guardian/dto/response/GuardianResponse.java (1)
GuardianResponse(12-42)
src/main/java/com/blockguard/server/domain/guardian/application/GuardianService.java (2)
src/main/java/com/blockguard/server/domain/guardian/domain/Guardian.java (2)
updateGuardianInfo(48-52)Guardian(11-57)src/main/java/com/blockguard/server/domain/guardian/dto/response/GuardianResponse.java (1)
GuardianResponse(12-42)
🔇 Additional comments (4)
src/main/java/com/blockguard/server/global/config/swagger/SwaggerResponseDescription.java (1)
73-74: 적절한 에러 코드 추가
UPDATE_PROFILE_CONFLICT에러 코드가 UPDATE_GUARDIAN_FAIL 시나리오에 올바르게 추가되었습니다. 이는 GuardianService에서 기본 이미지 설정과 새 파일 업로드를 동시에 요청할 때 발생하는 충돌을 처리하는 로직과 일치합니다.src/main/java/com/blockguard/server/domain/guardian/api/GuardianApi.java (1)
5-5: API 엔드포인트가 올바르게 업데이트됨
updateGuardian엔드포인트가UpdateGuardianRequest를 사용하도록 적절히 변경되었습니다. 이는 생성과 수정 요청을 분리하여 더 명확한 API 설계를 제공합니다.Also applies to: 51-52
src/main/java/com/blockguard/server/domain/guardian/application/GuardianService.java (2)
67-73: 프로필 이미지 업데이트 로직이 잘 구현됨기본 이미지 설정과 새 파일 업로드 간의 충돌을 적절히 처리하고 있습니다.
Boolean.TRUE.equals()를 사용하여 null-safe하게 처리한 점이 좋습니다.
79-91: 이미지 처리 로직의 명확한 구현세 가지 케이스(기본 이미지, 새 이미지, 기존 유지)를 명확하게 구분하여 처리하고 있으며, 각 경우에 대한 S3 파일 정리가 적절히 구현되었습니다.
Comment on lines
+99
to
+113
| private void registerAfterCommitDelete(String oldKey, String uploadedKey) { | ||
| org.springframework.transaction.support.TransactionSynchronizationManager | ||
| .registerSynchronization(new org.springframework.transaction.support.TransactionSynchronization() { | ||
| @Override public void afterCommit() { | ||
| if (oldKey != null && !oldKey.equals(uploadedKey)) { | ||
| s3Service.delete(oldKey); | ||
| } | ||
| } | ||
| @Override public void afterCompletion(int status) { | ||
| if (status == STATUS_ROLLED_BACK && uploadedKey != null) { | ||
| s3Service.delete(uploadedKey); | ||
| } | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
트랜잭션 동기화 로직 개선 제안
S3 삭제 작업이 트랜잭션 경계에서 적절히 처리되도록 구현되었습니다. 다만 몇 가지 개선사항을 제안합니다:
- S3 삭제 실패 시 로깅 추가
- 트랜잭션 활성 상태 확인
private void registerAfterCommitDelete(String oldKey, String uploadedKey) {
+ if (!TransactionSynchronizationManager.isActualTransactionActive()) {
+ // 트랜잭션이 없을 때 즉시 처리
+ if (oldKey != null) {
+ s3Service.delete(oldKey);
+ }
+ return;
+ }
+
org.springframework.transaction.support.TransactionSynchronizationManager
.registerSynchronization(new org.springframework.transaction.support.TransactionSynchronization() {
@Override public void afterCommit() {
if (oldKey != null && !oldKey.equals(uploadedKey)) {
- s3Service.delete(oldKey);
+ try {
+ s3Service.delete(oldKey);
+ } catch (Exception e) {
+ log.error("Failed to delete old S3 file: {}", oldKey, e);
+ }
}
}
@Override public void afterCompletion(int status) {
if (status == STATUS_ROLLED_BACK && uploadedKey != null) {
- s3Service.delete(uploadedKey);
+ try {
+ s3Service.delete(uploadedKey);
+ } catch (Exception e) {
+ log.error("Failed to delete uploaded S3 file after rollback: {}", uploadedKey, e);
+ }
}
}
});
}또한 클래스 상단에 로거 추가가 필요합니다:
import lombok.extern.slf4j.Slf4j;
@Slf4j
@Service
@RequiredArgsConstructor
public class GuardianService {🤖 Prompt for AI Agents
In
src/main/java/com/blockguard/server/domain/guardian/application/GuardianService.java
around lines 99 to 113, improve the transaction synchronization by first
ensuring TransactionSynchronizationManager.isSynchronizationActive() (or
isActualTransactionActive()) before calling registerSynchronization, add a
class-level Lombok logger (@Slf4j import/annotation) if missing, and wrap each
s3Service.delete(...) call inside a try-catch that logs failures with log.error
including the key and exception; keep the original commit/rollback key checks
but ensure deleted keys are logged on error.
Comment on lines
+25
to
+26
| private Boolean isDefaultImage; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
isDefaultImage 필드에 대한 검증 추가 필요
isDefaultImage 필드가 Boolean 타입으로 선언되어 있어 null 값을 가질 수 있습니다. 서비스 레이어에서 Boolean.TRUE.equals()로 처리하고 있지만, DTO 수준에서 명확한 검증이 필요합니다.
다음과 같이 수정하는 것을 제안합니다:
+ @NotNull(message = "isDefaultImage must not be null")
+ @Schema(description = "기본 이미지 사용 여부", example = "false", requiredMode = Schema.RequiredMode.REQUIRED)
- private Boolean isDefaultImage;
+ private Boolean isDefaultImage = false;또는 primitive boolean 타입 사용:
+ @Schema(description = "기본 이미지 사용 여부", example = "false", defaultValue = "false")
- private Boolean isDefaultImage;
+ private boolean isDefaultImage;🤖 Prompt for AI Agents
In
src/main/java/com/blockguard/server/domain/guardian/dto/request/UpdateGuardianRequest.java
around lines 25 to 26, the isDefaultImage field is declared as a nullable
Boolean which allows nulls at the DTO boundary; change it to a primitive boolean
or add DTO-level validation (e.g. annotate with @NotNull) so the field cannot be
null, then update any callers/tests accordingly to pass true/false (or handle
defaulting) and ensure imports for the validation annotation are added if used.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
src/main/java/com/blockguard/server/domain/guardian/dto/request/UpdateGuardianRequest.java (1)
22-26: 스키마 가시성 개선(선택 사항): isDefaultImage 의미/예시 명시클라이언트 혼선을 줄이기 위해 isDefaultImage에 대한 설명/예시를 추가해 주세요. 예) true: 프로필 초기화, false 또는 null: 기존 유지.
- private Boolean isDefaultImage; + @Schema(description = "기본 이미지로 재설정 여부 (true면 기존 이미지 제거, false 또는 미전송 시 기존 유지)", example = "false") + private Boolean isDefaultImage;src/main/java/com/blockguard/server/domain/guardian/api/GuardianApi.java (1)
51-53: 멀티파트 DTO 명시적 바인딩 권장: @ModelAttribute 추가consumes = MULTIPART_FORM_DATA에서 복합 객체는 기본적으로 @ModelAttribute로 바인딩되지만, 명시하면 가독성과 오해 방지에 도움이 됩니다. 생성 API와의 일관성도 확보됩니다.
- @Valid UpdateGuardianRequest updateGuardianRequest) { + @Valid @ModelAttribute UpdateGuardianRequest updateGuardianRequest) {src/main/java/com/blockguard/server/domain/guardian/application/GuardianService.java (3)
75-91: 트랜잭션-연동 S3 정리 플로우 적절 (사소 개선 제안 포함)커밋 후 기존 키 삭제, 롤백 시 신규 업로드 삭제로 정합성을 잘 보장했습니다. 사소한 개선 포인트:
- S3 삭제 실패 시 로그 남기기(운영 추적성)
- 디렉토리 상수("guardians") 중복 사용 → 상수화로 관리 일원화
예시:
- String uploadedKey = s3Service.upload(request.getProfileImage(), "guardians"); + String uploadedKey = s3Service.upload(request.getProfileImage(), S3_DIR_GUARDIANS);그리고 클래스 상단에 상수 추가:
private static final String S3_DIR_GUARDIANS = "guardians";
98-113: registerAfterCommitDelete 보강 제안: 트랜잭션 유무 체크 및 예외 안전성현재 호출 경로는 @transactional 내라 문제 없지만, 재사용성/안전성을 위해 아래 보강을 고려해 주세요.
- 트랜잭션 동기화 활성화 여부 검사
- 삭제 시 예외 처리 및 로그
private void registerAfterCommitDelete(String oldKey, String uploadedKey) { - org.springframework.transaction.support.TransactionSynchronizationManager - .registerSynchronization(new org.springframework.transaction.support.TransactionSynchronization() { + if (!org.springframework.transaction.support.TransactionSynchronizationManager.isSynchronizationActive()) { + // 비정상 경로 안전장치: 즉시 정리 + if (oldKey != null && !oldKey.equals(uploadedKey)) { + safeDelete(oldKey); + } + if (uploadedKey != null) { + // 롤백 개념 없음: 즉시 정리할지 정책 판단 필요 + } + return; + } + org.springframework.transaction.support.TransactionSynchronizationManager.registerSynchronization( + new org.springframework.transaction.support.TransactionSynchronization() { @Override public void afterCommit() { if (oldKey != null && !oldKey.equals(uploadedKey)) { - s3Service.delete(oldKey); + safeDelete(oldKey); } } @Override public void afterCompletion(int status) { if (status == STATUS_ROLLED_BACK && uploadedKey != null) { - s3Service.delete(uploadedKey); + safeDelete(uploadedKey); } } }); } + + private void safeDelete(String key) { + try { + s3Service.delete(key); + } catch (Exception e) { + // TODO: 로깅 프레임워크로 경고 로그 남기기 + } + }
36-55: 동일 트랜잭션-연동 정리 패턴의 create에도 적용 고려createGuardian에서는 업로드 직후 DB 저장 실패(롤백) 시 신규 파일이 고아가 될 여지가 있습니다. update에서 도입한 registerAfterCommitDelete 패턴을 create에도 일관 적용하면 S3 정합성이 더 강화됩니다.
Also applies to: 58-96
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/main/java/com/blockguard/server/domain/guardian/api/GuardianApi.java(2 hunks)src/main/java/com/blockguard/server/domain/guardian/application/GuardianService.java(2 hunks)src/main/java/com/blockguard/server/domain/guardian/dto/request/UpdateGuardianRequest.java(1 hunks)src/main/java/com/blockguard/server/global/config/swagger/SwaggerResponseDescription.java(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: yeonju73
PR: Block-Guard/BlockGuard-Server#32
File: src/main/java/com/blockguard/server/domain/fraud/dto/request/FraudUrlRequest.java:6-10
Timestamp: 2025-07-29T05:20:31.567Z
Learning: yeonju73은 DTO 수준에서의 입력 필드 검증과 해당하는 에러 코드 추가를 선호한다.
🧬 Code Graph Analysis (4)
src/main/java/com/blockguard/server/domain/guardian/api/GuardianApi.java (2)
src/main/java/com/blockguard/server/domain/guardian/domain/Guardian.java (2)
updateGuardianInfo(48-52)Guardian(11-57)src/main/java/com/blockguard/server/domain/guardian/dto/request/CreateGuardianRequest.java (1)
CreateGuardianRequest(11-24)
src/main/java/com/blockguard/server/domain/guardian/dto/request/UpdateGuardianRequest.java (3)
src/main/java/com/blockguard/server/domain/guardian/dto/request/CreateGuardianRequest.java (1)
CreateGuardianRequest(11-24)src/main/java/com/blockguard/server/domain/guardian/domain/Guardian.java (2)
Guardian(11-57)updateGuardianInfo(48-52)src/main/java/com/blockguard/server/domain/guardian/dto/response/GuardianResponse.java (1)
GuardianResponse(12-42)
src/main/java/com/blockguard/server/domain/guardian/application/GuardianService.java (2)
src/main/java/com/blockguard/server/domain/guardian/domain/Guardian.java (2)
updateGuardianInfo(48-52)Guardian(11-57)src/main/java/com/blockguard/server/domain/guardian/dto/request/CreateGuardianRequest.java (1)
CreateGuardianRequest(11-24)
src/main/java/com/blockguard/server/global/config/swagger/SwaggerResponseDescription.java (3)
src/main/java/com/blockguard/server/global/common/codes/ErrorCode.java (1)
Getter(7-51)src/main/java/com/blockguard/server/global/config/swagger/SwaggerConfig.java (1)
generateErrorCodeResponseExample(80-98)src/main/java/com/blockguard/server/global/config/swagger/CustomExceptionDescription.java (1)
CustomExceptionDescription(8-12)
🔇 Additional comments (3)
src/main/java/com/blockguard/server/global/config/swagger/SwaggerResponseDescription.java (1)
73-75: UPDATE_GUARDIAN 실패 응답에 신규 에러 코드 반영 적절프로필 이미지 업데이트 충돌 시나리오를 정확히 문서화했습니다. 기존 중복 이름 에러와 함께 케이스 커버가 좋아졌습니다.
src/main/java/com/blockguard/server/domain/guardian/dto/request/UpdateGuardianRequest.java (1)
15-21: PUT의 전체 갱신 의도 확인 필요 (이름/전화번호 필수 여부)현재 @notblank로 name/phoneNumber 모두 필수입니다. 이미지 변경만 원할 때도 두 값 전송이 필요합니다. API가 PUT(전체 갱신)로 설계되어 의도한 제약이라면 OK입니다. 부분 갱신(PATCH) 의도라면 해당 필드의 필수 제약을 완화하고 서비스에서 null 미전달 시 기존 값 유지 처리가 필요합니다.
src/main/java/com/blockguard/server/domain/guardian/application/GuardianService.java (1)
66-73: 동시 요청 상충 방지 로직 적절기본 이미지 전환과 새 파일 업로드의 동시 요청을 명확히 차단했습니다. 비즈니스 규칙에 부합하며 에러 코드 매핑도 일관적입니다.
Comment on lines
+11
to
+14
| @Getter | ||
| @AllArgsConstructor | ||
| @Builder | ||
| public class UpdateGuardianRequest { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
요청 DTO 바인딩 안정성 향상: 기본 생성자 추가 권장
멀티파트 폼(@ModelAttribute 기본 바인딩)에서 Jackson/Spring 바인더가 DTO 인스턴스화를 위해 기본 생성자를 필요로 합니다. 런타임 바인딩 실패를 예방하려면 @NoArgsConstructor 추가를 권장합니다.
import lombok.AllArgsConstructor;
import lombok.Builder;
import lombok.Getter;
+import lombok.NoArgsConstructor;
import org.springframework.web.multipart.MultipartFile;
@Getter
@AllArgsConstructor
+@NoArgsConstructor
@Builder
public class UpdateGuardianRequest {📝 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.
Suggested change
| @Getter | |
| @AllArgsConstructor | |
| @Builder | |
| public class UpdateGuardianRequest { | |
| import lombok.AllArgsConstructor; | |
| import lombok.Builder; | |
| import lombok.Getter; | |
| import lombok.NoArgsConstructor; | |
| import org.springframework.web.multipart.MultipartFile; | |
| @Getter | |
| @AllArgsConstructor | |
| @NoArgsConstructor | |
| @Builder | |
| public class UpdateGuardianRequest { | |
| // existing fields and methods… | |
| } |
🤖 Prompt for AI Agents
In
src/main/java/com/blockguard/server/domain/guardian/dto/request/UpdateGuardianRequest.java
around lines 11 to 14, the DTO lacks a no-args constructor which can cause
runtime binding failures for multipart/@ModelAttribute binding; add a
no-argument constructor by annotating the class with Lombok's @NoArgsConstructor
(or explicitly implement a public no-arg constructor) so Jackson/Spring can
instantiate the DTO during form binding, keeping the existing
@AllArgsConstructor and @Builder.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/main/java/com/blockguard/server/domain/guardian/dto/request/CreateGuardianRequest.java (2)
10-10: @NoArgsConstructor 주석 누락 가능성 — 바인딩 안정성 위해 클래스에 추가 권장multipart/form-data 바인딩(@ModelAttribute)에서 기본 생성자가 없으면 필드 바인딩 이슈가 생길 수 있습니다. 현재 NoArgsConstructor가 import 되어 있으므로 클래스에 실제로 붙이는 것을 권장합니다.
적용 예:
-@Getter -@AllArgsConstructor -@Builder +@Getter +@AllArgsConstructor +@NoArgsConstructor +@Builder public class CreateGuardianRequest {
18-18: 이름 길이 검증 추가 적합 — 메시지 i18n 키로 추출 검토검증 자체는 매우 적절합니다. 운영 일관성과 현지화를 위해 message 문자열을 messages.properties의 키로 추출하는 것도 고려해 주세요. 예: "{validation.guardian.name.size}".
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/java/com/blockguard/server/domain/guardian/dto/request/CreateGuardianRequest.java(1 hunks)src/main/java/com/blockguard/server/domain/guardian/dto/request/UpdateGuardianRequest.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/com/blockguard/server/domain/guardian/dto/request/UpdateGuardianRequest.java
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: yeonju73
PR: Block-Guard/BlockGuard-Server#32
File: src/main/java/com/blockguard/server/domain/fraud/dto/request/FraudUrlRequest.java:6-10
Timestamp: 2025-07-29T05:20:31.567Z
Learning: yeonju73은 DTO 수준에서의 입력 필드 검증과 해당하는 에러 코드 추가를 선호한다.
🧬 Code Graph Analysis (1)
src/main/java/com/blockguard/server/domain/guardian/dto/request/CreateGuardianRequest.java (3)
src/main/java/com/blockguard/server/domain/guardian/dto/request/UpdateGuardianRequest.java (1)
Getter(13-29)src/main/java/com/blockguard/server/domain/guardian/api/GuardianApi.java (2)
GuardianApi(22-75)createGuardian(36-43)src/main/java/com/blockguard/server/domain/guardian/domain/Guardian.java (1)
Guardian(11-57)
🔇 Additional comments (1)
src/main/java/com/blockguard/server/domain/guardian/dto/request/CreateGuardianRequest.java (1)
6-6: @SiZe import 추가 적절엔티티 컬럼 길이(Guardian.name length = 50)와 DTO 검증이 일치하도록 정렬된 점 좋습니다.
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
1 participant
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.
💻 Related Issue
closed #57
🚀 Work Description
🙇🏻♀️ To Reviewer
➕ Next
사기 분석 api 수정 마무리
Summary by CodeRabbit
신기능
개선
문서