-
Notifications
You must be signed in to change notification settings - Fork 0
[Refactor] 응원 등록 이미지 트랜잭션 분리 및 보상 트랜잭션 Prod 적용 #217
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
[Refactor] 응원 등록 이미지 트랜잭션 분리 및 보상 트랜잭션 적용
Walkthrough응원 등록 프로세스를 팩사드 패턴으로 리팩토링하여 서비스와 파일 처리 계층을 분리합니다. 파일 클라이언트의 이미지 이동 흐름을 동기화하고 에러 처리 및 롤백 메커니즘을 강화하며, 응원 생성과 이미지 저장을 독립적인 단계로 분리합니다. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant CheerRegisterFacade
participant CheerService
participant FileClient
participant Database as DB / Repository
participant S3
Client->>CheerRegisterFacade: registerCheer(request, storeResult, memberId, imageDomain)
CheerRegisterFacade->>CheerService: createCheer(request, storeResult, memberId)
CheerService->>Database: save(Cheer)
Database-->>CheerService: saved Cheer
CheerService-->>CheerRegisterFacade: CheerCreationResult(cheer, store)
alt Images Provided
CheerRegisterFacade->>CheerRegisterFacade: sortImages(uploadedImages)
CheerRegisterFacade->>FileClient: moveTempFilesToPermanent(imageDomain, cheerId, sortedImages)
alt Move Success
FileClient->>S3: copy temp → permanent
S3-->>FileClient: success
FileClient-->>CheerRegisterFacade: permanentKeys
CheerRegisterFacade->>CheerService: saveCheerImages(cheerId, sortedImages, permanentKeys)
CheerService->>Database: save(CheerImages)
Database-->>CheerService: success
CheerService-->>CheerRegisterFacade: success
CheerRegisterFacade->>CheerService: getCheerResponse(cheerId)
CheerService-->>CheerRegisterFacade: CheerResponse
CheerRegisterFacade-->>Client: CheerResponse
else Move/Save Failure
FileClient->>Client: throw SdkException
CheerRegisterFacade->>FileClient: deleteFiles(permanentKeys)
FileClient->>S3: delete
CheerRegisterFacade->>CheerService: deleteCheer(cheerId)
CheerService->>Database: delete(Cheer)
CheerRegisterFacade->>Client: throw exception
end
else No Images
CheerRegisterFacade->>CheerService: getCheerResponse(cheerId)
CheerService-->>CheerRegisterFacade: CheerResponse
CheerRegisterFacade-->>Client: CheerResponse
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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/eatda/service/cheer/CheerService.java (1)
55-56: 동시성 요청 시 상점 조회/저장에서 고유 제약 조건 위반 발생
kakaoId에 unique 제약 조건이 존재하지만,findByKakaoId후orElseGet으로 저장하는 패턴은 동시성을 처리하지 못합니다. 동일한kakaoId로 동시 요청이 들어올 때, 두 요청 모두 조회 결과가 없어 저장을 시도하면 두 번째 요청이DataIntegrityViolationException으로 실패합니다.해결 방안:
DataIntegrityViolationException캐치 후 재조회@Lock(LockModeType.PESSIMISTIC_WRITE)사용- 분산 락(Redis 등) 활용
🧹 Nitpick comments (5)
src/main/java/eatda/client/file/FileClient.java (1)
74-79: deleteFiles에서 개별 삭제 실패 시 나머지 파일 삭제가 중단됨
keys.forEach(this::deleteObject)는 첫 번째 삭제 실패 시 예외가 전파되어 나머지 파일 삭제가 실행되지 않습니다. 롤백 시나리오에서는 가능한 한 많은 파일을 삭제하는 것이 바람직합니다.🔎 Best-effort 삭제 구현
public void deleteFiles(List<String> keys) { if (keys.isEmpty()) { return; } - keys.forEach(this::deleteObject); + for (String key : keys) { + try { + deleteObject(key); + } catch (SdkException e) { + log.warn("파일 삭제 실패. key={}", key, e); + } + } }src/main/java/eatda/controller/cheer/CheerResponse.java (1)
17-28:cheer.getStore()lazy loading 접근 안전성 검토
Store필드는@ManyToOne(fetch = FetchType.LAZY)로 설정되어 있으며, 현재CheerResponse(Cheer, String)생성자는CheerService.getCheerResponse()에서만 호출되는데 이 메서드가@Transactional(readOnly = true)로 보호되어 있으므로 트랜잭션 컨텍스트 내에서 lazy loading이 안전하게 작동합니다. 다만, 향후 이 생성자가 다른 곳에서 트랜잭션 외부에서 호출될 경우LazyInitializationException이 발생할 수 있는 설계상 취약점이 있습니다. 생성자의 트랜잭션 의존성을 명시적으로 문서화하거나,Store의 fetch 타입을EAGER로 변경하는 것을 검토해보세요.src/test/java/eatda/facade/cheer/CheerRegisterFacadeTest.java (2)
84-132: 테스트명과 실제 동작이 일치하지 않습니다.
이미지_이동_중_실패하면_응원을_삭제한다테스트는 실제로 응원이 "삭제"되는 것이 아니라, 롤백으로 인해 응원이 "저장되지 않음"을 검증합니다. Facade 코드를 보면createCheer가 먼저 호출되고, 이미지 이동 실패 시deleteCheer가 호출되므로 테스트명은 정확합니다.다만, 테스트에서 응원이 실제로 생성된 후 삭제되었는지 더 명확하게 검증하려면
deleteCheer호출 여부도 확인하는 것이 좋습니다.🔎 deleteCheer 호출 검증 추가 제안
assertThat(cheerRepository.count()).isZero(); + + // 롤백 시 deleteCheer가 호출되었는지 검증 (선택적) + // verify(cheerService).deleteCheer(anyLong()); }
257-295: 헬퍼 메서드 중복을 줄일 수 있습니다.
getRegisterRequest()와getCheerRegisterRequest()는 이미지 개수만 다르고 거의 동일합니다. 파라미터화된 단일 헬퍼 메서드로 통합할 수 있습니다.🔎 통합된 헬퍼 메서드 제안
@NonNull private CheerRegisterRequest createRegisterRequest(int imageCount) { List<CheerRegisterRequest.UploadedImageDetail> images = IntStream.rangeClosed(1, imageCount) .mapToObj(i -> new CheerRegisterRequest.UploadedImageDetail( "temp/key" + i + ".jpg", (long) i, "image/jpeg", 1000L)) .toList(); return new CheerRegisterRequest( "kakao-1", "농민백암순대", "맛있어요", images, List.of(CheerTagName.GOOD_FOR_DATING) ); }src/main/java/eatda/service/cheer/CheerService.java (1)
71-91:sortedImages와permanentKeys크기 불일치 시 예외 발생 가능성이 있습니다.
permanentKeys.get(i)호출 시 두 리스트의 크기가 다르면IndexOutOfBoundsException이 발생합니다. Facade에서 올바르게 호출하면 문제없지만, 방어적으로 크기 검증을 추가하는 것이 안전합니다.🔎 크기 검증 추가 제안
@Transactional public void saveCheerImages(Long cheerId, List<CheerRegisterRequest.UploadedImageDetail> sortedImages, List<String> permanentKeys) { + if (sortedImages.size() != permanentKeys.size()) { + throw new IllegalArgumentException( + "sortedImages와 permanentKeys의 크기가 일치해야 합니다: " + + sortedImages.size() + " vs " + permanentKeys.size()); + } + Cheer cheer = cheerRepository.findById(cheerId) .orElseThrow(() -> new BusinessException(BusinessErrorCode.CHEER_NOT_FOUND));
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
src/main/java/eatda/client/file/FileClient.javasrc/main/java/eatda/controller/cheer/CheerController.javasrc/main/java/eatda/controller/cheer/CheerResponse.javasrc/main/java/eatda/exception/BusinessErrorCode.javasrc/main/java/eatda/facade/CheerCreationResult.javasrc/main/java/eatda/facade/CheerRegisterFacade.javasrc/main/java/eatda/service/cheer/CheerService.javasrc/test/java/eatda/document/BaseDocumentTest.javasrc/test/java/eatda/document/cheer/CheerDocumentTest.javasrc/test/java/eatda/facade/BaseFacadeTest.javasrc/test/java/eatda/facade/cheer/CheerRegisterFacadeTest.javasrc/test/java/eatda/service/cheer/CheerServiceTest.java
🧰 Additional context used
🧬 Code graph analysis (1)
src/test/java/eatda/facade/BaseFacadeTest.java (2)
src/test/java/eatda/DatabaseCleaner.java (1)
DatabaseCleaner(11-47)src/test/java/eatda/EatdaApplicationTests.java (1)
SpringBootTest(6-12)
🪛 GitHub Actions: Notify Discord on `/noti` Comment
src/test/java/eatda/document/BaseDocumentTest.java
[error] 93-93: No such file or directory
src/main/java/eatda/facade/CheerCreationResult.java
[error] 93-93: No such file or directory
src/test/java/eatda/document/cheer/CheerDocumentTest.java
[error] 93-93: No such file or directory
src/main/java/eatda/client/file/FileClient.java
[error] 93-93: No such file or directory
src/main/java/eatda/facade/CheerRegisterFacade.java
[error] 93-93: No such file or directory
src/test/java/eatda/facade/cheer/CheerRegisterFacadeTest.java
[error] 93-93: No such file or directory
src/main/java/eatda/controller/cheer/CheerResponse.java
[error] 93-93: No such file or directory
src/main/java/eatda/service/cheer/CheerService.java
[error] 93-93: No such file or directory
src/main/java/eatda/exception/BusinessErrorCode.java
[error] 93-93: No such file or directory
src/test/java/eatda/facade/BaseFacadeTest.java
[error] 93-93: No such file or directory
src/test/java/eatda/service/cheer/CheerServiceTest.java
[error] 93-93: No such file or directory
src/main/java/eatda/controller/cheer/CheerController.java
[error] 93-93: No such file or directory
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
🔇 Additional comments (17)
src/main/java/eatda/exception/BusinessErrorCode.java (1)
31-31: LGTM!새로운 에러 코드가 기존 패턴(CHE00X)과 일관되게 추가되었고,
HttpStatus.NOT_FOUND가 적절하게 적용되었습니다.src/test/java/eatda/document/cheer/CheerDocumentTest.java (2)
102-102: LGTM!
CheerRegisterFacade도입에 따라 mock 대상이 올바르게 변경되었습니다.
139-140: LGTM!실패 케이스도 facade를 통해 테스트되도록 일관성 있게 수정되었습니다.
src/main/java/eatda/facade/CheerCreationResult.java (1)
1-7: LGTM!응원 생성 결과를 캡슐화하기 위한 간결한 record 정의입니다. Cheer와 Store를 함께 반환해야 하는 요구사항에 적합합니다.
src/test/java/eatda/document/BaseDocumentTest.java (1)
70-72: LGTM!
CheerRegisterFacade에 대한 mock bean이 기존 패턴과 일관되게 추가되었습니다.src/main/java/eatda/controller/cheer/CheerController.java (2)
33-33: LGTM!Facade 패턴을 도입하여 응원 등록 workflow를 분리한 것이 좋습니다. Controller는 HTTP 처리에 집중하고, Facade가 비즈니스 orchestration을 담당합니다.
40-40: LGTM!응원 등록 로직이
CheerRegisterFacade로 위임되었습니다.ImageDomain.CHEER를 명시적으로 전달하여 도메인 구분이 명확합니다.src/test/java/eatda/facade/BaseFacadeTest.java (1)
15-36: LGTM!Facade 테스트를 위한 기반 클래스가 잘 구성되었습니다:
DatabaseCleaner로 테스트 격리 보장- 외부 클라이언트(OAuth, Map, File) mock 처리
- 테스트 fixture 및 repository autowiring
src/main/java/eatda/client/file/FileClient.java (2)
53-66: 이동 중간에 실패 시 원본 임시 파일 일부가 이미 삭제된 상태가 됨현재 로직은
copyObject→deleteObject순서로 각 파일을 처리합니다. N번째 파일에서copyObject성공 후deleteObject도 성공하고, N+1번째 파일의copyObject에서 실패하면:
- 1~N번 임시 파일은 이미 삭제됨
- 롤백으로 1~N번 permanent 파일만 삭제됨
원본 임시 파일 복구가 불가능한 상태가 됩니다. 이 동작이 의도된 것인지 확인이 필요합니다.
23-23: LGTM!
PATH_DELIMITER상수 도입으로 매직 스트링 사용을 제거하고 일관성을 확보했습니다.src/test/java/eatda/facade/cheer/CheerRegisterFacadeTest.java (1)
175-216: DB 저장 실패 시 롤백 검증이 잘 구현되었습니다.이미지 이동 성공 후 DB 저장 실패 시 파일 삭제(
deleteFiles) 호출을 검증하는 테스트입니다. 보상 트랜잭션 패턴이 잘 테스트되고 있습니다.src/main/java/eatda/facade/CheerRegisterFacade.java (2)
64-75:moveImages의 빈 리스트 검사는 방어적이지만 중복됩니다.Line 33에서 이미
request.images().isEmpty()검사 후 early return이 있으므로,sortedImages가 비어있을 수 없습니다. 하지만 방어적 프로그래밍 관점에서 유지해도 무방합니다.
25-55: 트랜잭션 분리 설계가 적절합니다.Facade 메서드에
@Transactional이 없고, 각 서비스 호출(createCheer,saveCheerImages,deleteCheer)이 개별 트랜잭션으로 실행됩니다. 이는 S3 작업과 DB 작업을 분리하여 부분 실패 시 보상 트랜잭션을 수행하려는 PR 목적에 부합합니다.src/test/java/eatda/service/cheer/CheerServiceTest.java (2)
101-118: 새로운CheerCreationResult반환 타입에 맞게 테스트가 잘 업데이트되었습니다.
createCheer호출 후CheerCreationResult에서cheer()와store()를 추출하여 검증하는 방식이 새로운 API 설계에 적합합니다.
296-309: 테스트 헬퍼 메서드 추가로 코드 중복이 줄었습니다.
storeSearchResult(String kakaoId)헬퍼 메서드가 테스트 전반에서 일관된StoreSearchResult생성을 담당하여 가독성과 유지보수성이 향상되었습니다.src/main/java/eatda/service/cheer/CheerService.java (2)
129-132:deleteCheer가 존재하지 않는 응원 삭제 시 예외를 던지지 않습니다.
deleteById는 ID가 존재하지 않아도 예외를 던지지 않습니다. Facade의 롤백 시나리오에서는 이것이 문제가 되지 않지만, 다른 컨텍스트에서 호출될 경우 의도치 않은 동작이 발생할 수 있습니다. 현재 사용 패턴에서는 문제없으나 명시적 검증을 고려해볼 수 있습니다.
134-140:getCheerResponse메서드가 잘 구현되었습니다.Facade에서 응원 등록 완료 후 응답을 생성하는 데 사용됩니다. 예외 처리가 명확하고 역할이 분명합니다.
|
🎉 This PR is included in version 1.9.4 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |



✨ 개요
🧾 관련 이슈
#213 #216
🔍 참고 사항 (선택)
Summary by CodeRabbit
릴리스 노트
새로운 기능
개선 사항
✏️ Tip: You can customize this high-level summary in your review settings.