Conversation
…트랜잭션 적용 및 단위 테스트 추가
HackSung
left a comment
HackSung
left a comment
There was a problem hiding this comment.
2단계 미션 잘 진행해 주셨습니다! 👍
피드백 코멘트와 질문들을 남겨드렸으니 확인 부탁드릴께요~ 🙏
|
|
||
| --- | ||
|
|
||
| ## 5. [구조 변경 + 동작 변경] AdminProductController Repository 의존 분리 |
There was a problem hiding this comment.
d27586c 커밋에 구조 변경과 작동 변경이 섞여 있는데 이 부분도 시간이 부족해서 한꺼번에 처리하신 거지요?
인지하고 계시다면 상관없습니다. 😄
|
|
||
| ### 4-1. 배경 | ||
|
|
||
| 예외 체계 전환, Service 분리, 트랜잭션 정책 적용 이후 도메인(비즈니스) 로직이 서비스 계층에 집중되었다. |
There was a problem hiding this comment.
예외 체계 전환 전후로 클라이언트가 받는 응답이 달라지지 않았다는 것을 어떻게 검증하셨나요? 인수 테스트로 커버된 부분인지 혹은 별도 검증 방법이 있었는지 궁금합니다.
|
|
||
| wishService.removeWishByMemberAndProduct(member.getId(), option.getProduct().getId()); | ||
|
|
||
| sendKakaoMessageIfPossible(member, saved, option); |
There was a problem hiding this comment.
sendKakaoMessageIfPossible()은 카카오 메시지 API를 호출하는데
@Transactional선언으로 이 호출은 트랜잭션 내부에서 실행됩니다.
외부 API 응답이 늦어지면 DB 커넥션을 불필요하게 점유하게 되고
현재는 catch (Exception ignored)로 예외를 무시하고 있어 롤백되지는 않지만
커넥션 풀 고갈 위험이 있습니다.
카카오 메시지 전송을 트랜잭션 밖으로 분리하는 방안(예: @TransactionalEventListener 혹은 메서드 분리)을 검토하셨나요?
현재 구조에서 트랜잭션 내 외부 호출을 유지하기로 한 이유가 있다면 궁금합니다.
| verify(optionService).save(option); | ||
| verify(memberService).save(member); | ||
| verify(orderRepository).save(any(Order.class)); | ||
| verify(wishService).removeWishByMemberAndProduct(1L, 10L); |
There was a problem hiding this comment.
요구사항에는 다음과 같이 명시되어 있는데요.
예외가 발생하는지만 확인하는 것으로 충분하지 않다. 상태를 재조회하거나 결과를 관찰 가능한 방식으로 검증해야 한다.
현재 OrderServiceTest의 위시 삭제 검증은 "호출 여부"만 확인하고 있습니다.
요구사항이 기대하는 "상태를 재조회하거나 결과를 관찰 가능한 방식"과는 거리가 있어보이는데
주문 완료 후 실제로 위시가 삭제되었는지를 확인하는 통합 테스트나 인수 테스트가 별도로 존재하나요?
단위 테스트에서 mock verify만으로는 실제 DB 연동에서의 정합성을 보장하기 어려울 것 같은데 이 부분에 대한 검증 전략이 궁금합니다.
| | 항목 | 내용 | | ||
| |---|---| | ||
| | 도메인 메서드 추가 | `Wish` 엔티티에 `assertOwner(Long memberId)` 추가 | | ||
| | 서비스 단순화 | `WishService#removeWish`의 직접 비교/예외 throw 제거 후 `wish.assertOwner(member.getId())` 호출로 변경 | |
There was a problem hiding this comment.
도메인 객체가 자신의 불변 규칙을 스스로 보호하도록 한 것은 매우 좋은 방향같습니다. 👍
| } catch (Exception ignored) { | ||
| } |
There was a problem hiding this comment.
모든 예외를 무시하고 있어 장애 상황에서 디버깅이 어려울 수 있지 않을까요?
| Category category = categoryService.findByIdOrThrow(id); | ||
| category.update(request.name(), request.color(), request.imageUrl(), request.description()); | ||
| categoryService.save(category); |
There was a problem hiding this comment.
조회 → 수정 → 저장이 Controller 레벨에서 이루어지고 있어
이 흐름 전체가 하나의 트랜잭션으로 묶이지 않겠네요.
findByIdOrThrow는 readOnly 트랜잭션에서 실행되고
save는 별도 쓰기 트랜잭션에서 실행되고 있어
동시성 상황에서 조회와 저장 사이에 다른 요청이 끼어들 수 있을 것 같습니다.
다른 도메인에서는 ProductService.updateProduct()처럼 Service 내에서 조회-수정-저장을
하나의 @Transactional로 묶었는데
CategoryController만 Controller에서 이 흐름을 조립하는 이유가 있을까요?
의도적인 구분인지 리팩터링 과정에서 누락된 것인지 궁금합니다.
| * @since 1.0 | ||
| */ | ||
| @Component |
There was a problem hiding this comment.
structural-change-plan.md에서 "Javadoc 삭제 — 나머지 엔티티에도 없으므로 통일한다"고 명시했는데 여기에는 Javadoc이 남아 있네요?
| .orElseThrow(() -> new NoSuchElementException("카테고리가 존재하지 않습니다. id=" + categoryId)); | ||
|
|
||
| product.update(name, price, imageUrl, category); | ||
| productRepository.save(product); |
There was a problem hiding this comment.
트랜잭션 안에서 조회된 엔티티는 Dirty Checking에 의해 트랜잭션 커밋 시점에 자동으로 UPDATE 쿼리가 나가게 되는데 의도적으로 save()호출을 명시하신 것인지 궁금합니다.
| private final AuthenticationResolver authenticationResolver; | ||
|
|
||
| public Page<WishResponse> getWishes(String authorization, Pageable pageable) { | ||
| Member member = extractMember(authorization); |
There was a problem hiding this comment.
현재 OrderService, WishService 등에서 메서드 인자로 String authorization을 받고
내부에서 AuthenticationResolver를 통해 Member를 추출하고 있는데요.
Service 레이어가 HTTP 헤더(Authorization token string)에 의존하게 된 특별한 이유가 있을까요?
현재 구조에서는 Service 테스트 시 Authorization String 포맷을 맞춰줘야 하는 번거로움이 있을 것 같습니다.
Controller 레벨에서 HandlerMethodArgumentResolver 활용 등을 고려해 보셨을까요?
2 participants
step2 리팩터링을 진행하면서, 서비스/도메인 책임 분리와 예외 처리 표준화, 트랜잭션 정책 정비를 중심으로 구조를 정리했습니다.
또한 step1에서 미처 반영하지 못한 일부 구조 변경도 함께 보완했습니다.
상세 작업 로그는 programming-log-step2.md에 정리되어 있습니다.