[22기_이준영] 동시성 & 결제 연동 미션 제출합니다. #28
Conversation
| log.debug("[SVC] addMovieFavorite() start - user={}, movieId={}", username, movieId); | ||
| try { | ||
| User user = userRepository.findByUsername(username) | ||
| .orElseThrow(() -> new IllegalArgumentException("유저 없음")); |
There was a problem hiding this comment.
IllegalArgumentException보다 Usernamenotfoundexception이 더 적절해보이고, 그게 아니라면 다른 Exception들은 GeneralException으로 관리하는 게 더 보기 편할 것 같아요!
There was a problem hiding this comment.
CustomException을 쓰고싶었는데 시간이 없다는 핑계로 도입을 못했습니다 ㅠㅠ
수정해보겠습니다
There was a problem hiding this comment.
try-catch문에서 처리하는 것 보다는, @ControllerAdvice + @ExceptionHandler로 controller단과 service단의 책임분리를 하는 것이 좋을 것 같습니다 :)
There was a problem hiding this comment.
ApiResponse 구조 활용해서 응답통일하는 게 좋을 것 같아요!
| .map(MovieResponseDto::fromEntity) | ||
| .toList(); | ||
|
|
||
| if (movies.isEmpty()) { |
There was a problem hiding this comment.
log도 좋지만, 응답내려주는게 프론트입장에서 더 편할 것 같아요!
| private final ShowtimeRepository showtimeRepository; | ||
|
|
||
| //전체 영화 조회 | ||
| public List<MovieResponseDto> getAllMovies() { |
There was a problem hiding this comment.
@transactional(readOnly = true) 붙여줘도 좋을 것 같아요!
| rs.setTicket(savedTicket); | ||
| } | ||
|
|
||
| String paymentId = "junilyy-tkx-" + savedTicket.getId(); // 네가 정한 규칙 그대로 |
There was a problem hiding this comment.
junilyy-tkt이랑 junilyy-tkx가 혼용되는데, 따로 관리하는게 좋을 것 같아요!
There was a problem hiding this comment.
헉 이거 제가 tkt로 통일을 못시켰네요.. 수정하겠습니다.
| User user = userRepository.findByUsername(username) | ||
| .orElseThrow(() -> new IllegalArgumentException("유저 없음")); | ||
|
|
||
| // 최종 결제 금액 계산(!!!!!!계산 로직 분리하기!!!!!!!) |
There was a problem hiding this comment.
별개로, TODO 주석으로 달아놓으면 나중에 볼때 더 편할 것 같아요!
| } | ||
|
|
||
| @DeleteMapping("/orders/cancel/{orderId}") | ||
| public String cancelOrder(@AuthenticationPrincipal UserDetails userDetails, @PathVariable Long orderId) { |
There was a problem hiding this comment.
메서드 결과를 왜 Dto가 아닌 String으로 하셨는지 궁금합니다!
There was a problem hiding this comment.
1차원적인 생각으로, 다른 코드 구현없이 결과 확인을 쉽고 빠르게 할 수 있어서 저렇게 했었습니다..
이전 리뷰에서도 언급해주셨던 내용인데 제가 게을러서 못바꿨습니다 😭
프론트와의 통신을 고려해서 JsonObject 포멧으로 변환하거나 dto를 이용하는 방식으로 수정하겠습니다.
| @Transactional | ||
| public void payForOrder(String username, Long orderId) { | ||
| Order order = ordersRepository.findById(orderId) | ||
| .orElseThrow(() -> new IllegalArgumentException("주문 없음")); | ||
|
|
||
| if (!order.getUser().getUsername().equals(username)) { | ||
| throw new SecurityException("본인 주문만 결제할 수 있습니다."); | ||
| } | ||
|
|
||
| String paymentId = paymentIdForOrder(orderId); | ||
| String orderName = "CGV-Order-" + orderId; | ||
| String custom = "{\"orderId\":\"" + orderId + "\"}"; | ||
| int amount = order.getTotalPrice(); | ||
|
|
||
| // 결제 호출 | ||
| try { | ||
| paymentService.pay(paymentId, orderName, amount, custom); | ||
| paymentRecordRepository.findByPaymentId(paymentId) | ||
| .ifPresent(r -> r.setStatus(PaymentStatus.PAID)); | ||
| } catch (Exception e) { | ||
| log.warn("[SVC] 주문 결제 실패 - orderId={}, msg={}", orderId, e.getMessage()); | ||
| throw e; | ||
| } |
There was a problem hiding this comment.
저도 하나의 트랜잭션 내에서 클라이언트가 결제 서버에 요청을 보내도록 코드를 짰었는데, 지금 생각해보니 성능에 문제가 있을 것 같습니다. DB가 락을 잡은 상태로 외부 API를 호출하면 실행 시간이 외부 API에 종속되는 문제가 생길 것 같아요. 그래서 1. (트랜잭션 O) 재고 확인 및 차감 2. (트랜잭션 x) 결제 요청 3. (트랜잭션 O) 결제 결과에 따른 DB 조작 (결제 상태 변경 또는 롤백) -> 이런 식으로 트랜잭션을 분리해야 병목이 안 생길 것 같습니다. 저도 바꿔보겠습니다..!
| // 차감 | ||
| for (var oi : items) { | ||
| Stock stock = stockRepository | ||
| .findByProduct_IdAndTheater_Id(oi.getProduct().getId(), theaterId) | ||
| .orElseThrow(); | ||
| stock.setStock(stock.getStock() - oi.getQuantity()); | ||
| } | ||
| return null; | ||
| }); |
There was a problem hiding this comment.
결제 요청 이전에 락을 잡은 상태에서 재고 차감이 이루어지면 결제가 성공했지만 재고가 없는 상황을 막을 수 있을 것 같습니다!
| .currency("KRW") | ||
| .customData(customJson).build(); | ||
|
|
||
| return client().post() |
3 participants
월요일에 시험이 끝나서... 많이 미흡합니다 ㅠㅠㅠ
코드 리뷰 해주시면 이전 리뷰 내용이랑 함께 차근차근 고쳐가겠습니다!