[Refactor] 응원 등록 이미지 트랜잭션 분리 및 보상 트랜잭션 적용#216
Conversation
WalkthroughCheer 등록 흐름을 CheerRegisterFacade로 분리하고, FileClient의 비동기 파일 이동을 동기 단일 스레드로 전환했으며 이미지 이동 실패 시 파일 및 DB 롤백(보상 트랜잭션)을 추가했습니다. 서비스는 CheerCreationResult 반환으로 분리되고 테스트 인프라가 업데이트되었습니다. Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant CheerController
participant CheerRegisterFacade
participant CheerService
participant FileClient
participant Repository
Client->>CheerController: POST /cheers (request)
CheerController->>CheerRegisterFacade: registerCheer(request, result, memberId, domain)
CheerRegisterFacade->>CheerService: createCheer(request, result, memberId)
CheerService->>Repository: save(cheer)
CheerService-->>CheerRegisterFacade: CheerCreationResult
rect rgb(200,220,255)
Note over CheerRegisterFacade,FileClient: 동기 이미지 이동 처리
CheerRegisterFacade->>FileClient: moveTempFilesToPermanent(tempKeys)
FileClient-->>CheerRegisterFacade: movedKeys / exception
end
alt 이동 성공
CheerRegisterFacade->>CheerService: saveCheerImages(cheerId, images, movedKeys)
CheerService->>Repository: save(cheerImages)
CheerRegisterFacade-->>CheerController: CheerResponse
CheerController->>Client: 200 OK
else 이동 중 예외
rect rgb(255,200,200)
Note over CheerRegisterFacade,FileClient: 롤백: 삭제 및 DB 정리
CheerRegisterFacade->>FileClient: deleteFiles(movedKeys)
CheerRegisterFacade->>CheerService: deleteCheer(cheerId)
CheerService->>Repository: delete(cheer)
end
CheerRegisterFacade-->>CheerController: throw BusinessException(FAIL_TEMP_IMAGE_PROCESS)
CheerController->>Client: Error Response
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Poem
Pre-merge checks and finishing touches❌ 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 |
lvalentine6
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (4)
src/main/java/eatda/client/file/FileClient.java (1)
53-72: 멱등성 고려사항현재 구현은 동일한 파일을 두 번 이동하려고 하면 실패합니다(소스 키가 이미 삭제됨). 재시도 시나리오나 네트워크 실패 후 복구 시 이는 문제가 될 수 있습니다.
필요한 경우, 대상 키가 이미 존재하는지 확인하거나
copyObject전에 소스 키의 존재 여부를 확인하여 멱등성을 개선할 수 있습니다. 다만 현재 파사드 계층에서 재시도를 하지 않는다면 이는 추후 개선사항으로 미뤄도 됩니다.src/test/java/eatda/facade/cheer/CheerRegisterFacadeTest.java (2)
121-132: 예외 발생을 명시적으로 검증하는 것을 권장합니다.현재 try-catch로 예외를 무시하고 있어, 예외가 발생하지 않아도 테스트가 통과할 수 있습니다.
assertThrows를 사용하면 예외 발생과 롤백을 모두 검증할 수 있습니다.- try { - cheerRegisterFacade.registerCheer( - request, - storeResult, - member.getId(), - ImageDomain.CHEER - ); - } catch (Exception ignored) { - } + assertThrows(Exception.class, () -> + cheerRegisterFacade.registerCheer( + request, + storeResult, + member.getId(), + ImageDomain.CHEER + ) + ); assertThat(cheerRepository.count()).isZero();
257-295: 헬퍼 메서드 네이밍을 더 명확하게 하는 것을 권장합니다.
getRegisterRequest()와getCheerRegisterRequest()가 매우 유사한 이름이지만 다른 목적으로 사용됩니다. 의도를 명확히 하면 가독성이 향상됩니다.예시:
getRegisterRequest()→createTwoImageRequest()getCheerRegisterRequest()→createThreeImageRequest()src/main/java/eatda/service/cheer/CheerService.java (1)
71-91:sortedImages와permanentKeys크기 불일치 시 예외가 발생할 수 있습니다.
IntStream.range(0, sortedImages.size())로 순회하면서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("이미지 정보와 키 목록의 크기가 일치하지 않습니다."); + } + 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.java(2 hunks)src/main/java/eatda/controller/cheer/CheerController.java(2 hunks)src/main/java/eatda/controller/cheer/CheerResponse.java(1 hunks)src/main/java/eatda/exception/BusinessErrorCode.java(1 hunks)src/main/java/eatda/facade/CheerCreationResult.java(1 hunks)src/main/java/eatda/facade/CheerRegisterFacade.java(1 hunks)src/main/java/eatda/service/cheer/CheerService.java(8 hunks)src/test/java/eatda/document/BaseDocumentTest.java(2 hunks)src/test/java/eatda/document/cheer/CheerDocumentTest.java(2 hunks)src/test/java/eatda/facade/BaseFacadeTest.java(1 hunks)src/test/java/eatda/facade/cheer/CheerRegisterFacadeTest.java(1 hunks)src/test/java/eatda/service/cheer/CheerServiceTest.java(9 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/main/java/eatda/client/file/FileClient.java (1)
src/main/java/eatda/facade/CheerRegisterFacade.java (1)
Slf4j(17-76)
⏰ 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 (19)
src/main/java/eatda/exception/BusinessErrorCode.java (1)
26-32:CHEER_NOT_FOUND추가는 적절합니다.
CHE005코드는 현재 이 정의 한 곳에서만 사용되며,CheerService의 두 위치(77번, 137번 줄)에서 Cheer 엔티티 조회 실패 시에만 던져집니다.HttpStatus.NOT_FOUND로 설정되어 있어 404 응답이 명확하게 표현되므로, 에러 코드 중복도 없고 의미도 정확하게 일치합니다.src/test/java/eatda/document/cheer/CheerDocumentTest.java (1)
102-102: LGTM! 파사드 기반 아키텍처로의 전환이 올바르게 반영되었습니다.테스트가 새로운
CheerRegisterFacade를 올바르게 모킹하고 있으며, 성공 및 실패 시나리오 모두 적절하게 커버하고 있습니다.Also applies to: 140-140
src/test/java/eatda/document/BaseDocumentTest.java (1)
10-10: LGTM! 테스트 베이스 클래스에 파사드 모킹 지원이 추가되었습니다.새로운
CheerRegisterFacade의존성이 문서 테스트에서 모킹 가능하도록 올바르게 추가되었습니다.Also applies to: 70-71
src/main/java/eatda/controller/cheer/CheerController.java (1)
9-9: LGTM! 컨트롤러가 파사드 패턴으로 올바르게 위임하도록 개선되었습니다.컨트롤러가 복잡한 다단계 플로우를 직접 오케스트레이션하는 대신
CheerRegisterFacade에 위임하는 것은 관심사 분리 측면에서 좋은 접근입니다. 파사드 계층에서 트랜잭션 경계와 보상 트랜잭션을 처리할 수 있게 되었습니다.Also applies to: 33-33, 40-40
src/test/java/eatda/facade/BaseFacadeTest.java (1)
1-36: LGTM! 파사드 테스트를 위한 베이스 클래스가 잘 구성되었습니다.외부 의존성(OauthClient, MapClient, FileClient)을 적절히 모킹하고, 필요한 생성자와 리포지토리를 주입하며, DatabaseCleaner를 통해 테스트 격리를 보장합니다.
NONE웹 환경 설정도 파사드 레이어 테스트에 적합합니다.src/main/java/eatda/facade/CheerCreationResult.java (1)
1-7: LGTM! 결과 타입의 명확한 캡슐화입니다.
CheerCreationResult레코드는 서비스 계층이 컨트롤러 응답 타입에 결합되지 않고 도메인 엔티티를 반환할 수 있게 해줍니다. 깔끔한 관심사 분리입니다.src/test/java/eatda/service/cheer/CheerServiceTest.java (2)
19-19: LGTM! 테스트가 새로운 서비스 시그니처에 맞춰 올바르게 업데이트되었습니다.
CheerCreationResult기반의 새로운 플로우를 반영하여 테스트가 잘 수정되었습니다. 모든 테스트 시나리오가 여전히 적절하게 커버되고 있으며, 멀티라인 생성자 형식과var사용으로 가독성도 향상되었습니다.Also applies to: 47-62, 70-85, 88-119, 122-145, 148-165, 168-188
296-309: LGTM! 헬퍼 메서드로 테스트 코드 중복이 제거되었습니다.
storeSearchResult()헬퍼 메서드는 테스트 전반에서 반복되는StoreSearchResult생성 로직을 효과적으로 제거합니다.src/main/java/eatda/client/file/FileClient.java (1)
23-23: LGTM! 매직 스트링 제거로 코드 유지보수성이 향상되었습니다.
PATH_DELIMITER상수를 도입하여 경로 구분자를 일관되게 사용하고 있습니다.Also applies to: 59-59, 82-83
src/main/java/eatda/facade/CheerRegisterFacade.java (3)
57-62: LGTM!
sortImages헬퍼 메서드가 명확하고 간결하게 구현되어 있습니다.
64-75: LGTM!파일 이동 로직이
FileClient에 적절히 위임되어 있으며,FileClient내부에서 부분 실패 시 이미 이동된 파일을 정리하는 것을 확인했습니다.
25-55: 파일 I/O와 DB 트랜잭션의 분리가 잘 구현되었습니다.PR 목표대로
createCheer가 먼저 커밋되고, 이미지 이동은 트랜잭션 외부에서 수행되며, 실패 시 보상 트랜잭션으로 정리하는 설계가 적절합니다.src/test/java/eatda/facade/cheer/CheerRegisterFacadeTest.java (3)
36-81: LGTM!정상 경로 테스트가 잘 작성되어 있습니다. 응답 검증과 mock 상호작용 검증이 모두 포함되어 있습니다.
134-174: 테스트명과 실제 동작의 차이를 확인해 주세요.테스트명은 "부분적으로 성공한 후 실패"를 의미하지만,
FileClient.moveTempFilesToPermanent는 내부적으로 부분 실패 시 이미 이동된 파일을 정리합니다. Facade 입장에서는 부분 성공 여부를 알 수 없으므로,이미지_이동_중_실패하면_응원을_삭제한다테스트와 동일한 시나리오를 검증하고 있습니다.테스트 의도에 따라 중복 테스트를 제거하거나, 테스트명을 수정하는 것을 고려해 주세요.
218-255: LGTM!이미지가 없는 경우의 테스트가 잘 작성되어 있습니다.
Mockito.never()를 사용하여 파일 이동이 호출되지 않음을 검증하는 것이 적절합니다.src/main/java/eatda/service/cheer/CheerService.java (4)
47-60: LGTM!
createCheer가CheerCreationResult를 반환하도록 리팩토링되어 이미지 처리가 Facade로 분리되었습니다. 책임 분리가 명확합니다.
129-132: LGTM!보상 트랜잭션을 위한
deleteCheer메서드가 간결하게 구현되어 있습니다.
134-140: LGTM!단일 응원 조회 메서드가 명확하게 구현되어 있습니다.
107-115: LGTM!응답 매핑 로직이 간결하게 리팩토링되었습니다.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/test/java/eatda/facade/cheer/CheerRegisterFacadeTest.java (2)
202-217: 이전 리뷰에서 지적된 문제가 여전히 남아있습니다.이 코드 영역은 이전 리뷰 코멘트에서 이미 지적되었지만 수정되지 않았습니다. try-catch 블록 외부의 assertion과 verify로 인해 예외가 발생하지 않으면 테스트가 false positive로 통과할 수 있습니다.
다음 diff를 적용하여 예외 발생을 명시적으로 검증하세요:
- try { - cheerRegisterFacade.registerCheer( - request, - storeResult, - member.getId(), - ImageDomain.CHEER - ); - } catch (Exception ignored) { - } - - assertThat(cheerRepository.count()) - .as("DB 에러(컬럼 길이 초과) 발생 시 응원글은 삭제되어야 한다.") - .isZero(); - - verify(fileClient).deleteFiles(movedKeys); + assertThrows(Exception.class, () -> + cheerRegisterFacade.registerCheer( + request, + storeResult, + member.getId(), + ImageDomain.CHEER + ) + ); + + assertThat(cheerRepository.count()) + .as("DB 에러(컬럼 길이 초과) 발생 시 응원글은 삭제되어야 한다.") + .isZero(); + + verify(fileClient).deleteFiles(movedKeys);
161-174: try-catch 블록 외부의 assertion으로 인한 false positive 위험이 있습니다.예외가 발생하지 않아도 테스트가 통과하므로, 부분 성공 후 실패 시 보상 트랜잭션이 제대로 동작하지 않아도 테스트가 성공할 수 있습니다.
다음 diff를 적용하여 예외 발생을 명시적으로 검증하세요:
- try { - cheerRegisterFacade.registerCheer( - request, - storeResult, - member.getId(), - ImageDomain.CHEER - ); - } catch (Exception ignored) { - } - - assertThat(cheerRepository.count()) - .as("부분 성공 후 실패 시 Cheer는 삭제되어야 한다.") - .isZero(); + assertThrows(SdkException.class, () -> + cheerRegisterFacade.registerCheer( + request, + storeResult, + member.getId(), + ImageDomain.CHEER + ) + ); + + assertThat(cheerRepository.count()) + .as("부분 성공 후 실패 시 Cheer는 삭제되어야 한다.") + .isZero();
🧹 Nitpick comments (1)
src/test/java/eatda/facade/cheer/CheerRegisterFacadeTest.java (1)
258-296: 헬퍼 메서드 이름이 명확하게 구분되지 않습니다.
getRegisterRequest()와getCheerRegisterRequest()는 유사한 이름으로 각 메서드가 어떤 시나리오에 사용되는지 명확하지 않습니다. 특히getCheerRegisterRequest()는 부분 성공 테스트에만 사용되므로, 용도를 반영한 이름으로 변경하는 것을 고려해보세요.예시:
- private CheerRegisterRequest getRegisterRequest() { + private CheerRegisterRequest createRequestWithTwoImages() { // ... } - private CheerRegisterRequest getCheerRegisterRequest() { + private CheerRegisterRequest createRequestWithThreeImages() { // ... }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/test/java/eatda/facade/cheer/CheerRegisterFacadeTest.java(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/test/java/eatda/facade/cheer/CheerRegisterFacadeTest.java (1)
src/test/java/eatda/document/cheer/CheerDocumentTest.java (3)
Nested(42-154)Nested(156-241)Nested(243-312)
⏰ 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 (2)
src/test/java/eatda/facade/cheer/CheerRegisterFacadeTest.java (2)
36-81: 정상 시나리오 테스트가 잘 작성되었습니다.응원 등록 성공 케이스를 명확하게 검증하고 있으며, 이미지 이동과 응답 데이터 검증이 적절합니다.
219-256: 이미지 없는 등록 케이스가 적절하게 검증되었습니다.엣지 케이스를 잘 다루고 있으며, fileClient 호출이 없음을
Mockito.never()로 명확하게 검증하고 있습니다.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/test/java/eatda/facade/cheer/CheerRegisterFacadeTest.java (3)
134-173: 테스트 이름과 실제 동작이 일치하지 않습니다."부분적으로 성공한 후 실패"를 테스트한다고 명시되어 있지만, mock 설정(lines 153-159)이 즉시 예외를 던지므로 실제 부분 성공 시나리오를 검증하지 못합니다. 이전 테스트(
이미지_이동_중_실패하면_응원을_삭제한다)와 본질적으로 동일한 동작을 테스트합니다.부분 성공 시나리오를 실제로 테스트하려면:
fileClient.moveTempFilesToPermanent가 일부 키를 반환한 후 내부적으로 실패하는 상황을 시뮬레이션하거나- 파일 이동 후 후속 작업에서 실패하는 시나리오를 구성해야 합니다.
만약
FileClient내부에서 부분 성공/롤백을 처리한다면, 해당 로직은FileClient단위 테스트에서 검증하는 것이 더 적합합니다.
201-208: 예외 타입이 너무 광범위합니다.
Exception.class는 너무 포괄적이어서 의도하지 않은 예외도 테스트를 통과시킬 수 있습니다. DB 제약 조건 위반에 대한 구체적인 예외 타입(예:DataIntegrityViolationException또는ConstraintViolationException)을 사용하면 테스트의 정확성이 향상됩니다.- assertThrows(Exception.class, () -> + assertThrows(DataIntegrityViolationException.class, () -> cheerRegisterFacade.registerCheer( request, storeResult, member.getId(), ImageDomain.CHEER ) );
257-295: 헬퍼 메서드 통합을 고려해 보세요.
getRegisterRequest()와getCheerRegisterRequest()메서드가 유사한 구조를 가지고 있습니다. 이미지 개수와 설명을 파라미터로 받는 단일 헬퍼 메서드로 통합하면 중복을 줄이고 테스트 의도를 더 명확히 표현할 수 있습니다.@NonNull private CheerRegisterRequest createRegisterRequest(String description, 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", "농민백암순대", description, images, List.of(CheerTagName.GOOD_FOR_DATING) ); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/test/java/eatda/facade/cheer/CheerRegisterFacadeTest.java(1 hunks)
⏰ 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 (4)
src/test/java/eatda/facade/cheer/CheerRegisterFacadeTest.java (4)
1-33: 클래스 구조 및 임포트가 적절합니다.테스트 클래스 설정이 적절하며, BaseFacadeTest를 확장하여 필요한 의존성을 상속받고 있습니다.
37-82: 성공 시나리오 테스트가 잘 구성되어 있습니다.응답 내용 검증과 mock 상호작용 검증이 적절히 포함되어 있습니다.
84-132: 이전 리뷰 피드백이 반영되어 assertThrows를 사용하고 있습니다.예외 발생을 명시적으로 검증하고, 이후 cleanup 검증을 수행하는 구조로 개선되었습니다.
218-255: 이미지 없는 경우의 엣지 케이스가 잘 테스트되었습니다.이미지가 없을 때 파일 이동이 호출되지 않음을 검증하고, 응원이 정상적으로 등록됨을 확인하는 적절한 테스트입니다.
|
|
🎉 This PR is included in version 1.8.0-develop.20 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 1.9.4 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
2 participants
✨ 개요
🧾 관련 이슈
Close #213
🔍 참고 사항 (선택)
Summary by CodeRabbit
새로운 기능
개선 사항
버그 픽스
테스트
✏️ Tip: You can customize this high-level summary in your review settings.