-
Couldn't load subscription status.
- Fork 10
[22기_신혁] 동시성 & 결제 연동 미션 제출합니다. #25
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
base: gilbert09031
Are you sure you want to change the base?
Conversation
Updated README.md to provide detailed explanations on concurrency issues and solutions in multi-threaded environments, including synchronization, database locks, and distributed locks.
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.
수고하셨습니다!
저와 결제부분 플로우가 비슷한 거 같아요!
그리고 원시값과 문자열 포장, 로깅 부분을 꼼꼼하게 하신 거 같습니다! 제가 많이 어려워했던 부분인데 덕분에 배운 거 같습니다!!
cgv_clone/src/main/java/com/ceos22/cgv_clone/common/config/JpaConfig.java
Show resolved
Hide resolved
| AUTHENTICATION_FAILED(HttpStatus.UNAUTHORIZED, "A001", "인증에 실패했습니다."), | ||
| EXPIRED_TOKEN(HttpStatus.UNAUTHORIZED, "J003", "만료된 토큰입니다."), | ||
| MISSING_TOKEN(HttpStatus.UNAUTHORIZED, "J004", "토큰이 필요합니다."), | ||
| UNSUPPORTED_TOKEN(HttpStatus.UNAUTHORIZED, "J005", "지원하지 않는 토큰 형식입니다."), |
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.
오 에러코드 A, J C 등 다양하게 있는데 따로 정하는 기준이 있는건가요??
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.
도메인별 앞글자를 따오는 블로그를 참고했었습니다 !
아직 정리가 안되어있는데, 어느정도 에러코드들이 확정되면 정리해보겠습니다 !
|
|
||
| public void logout() { | ||
| public void logout(String accessToken) { | ||
| Authentication authentication = SecurityContextHolder.getContext().getAuthentication(); |
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.
authentication을 가져온 이유가 무엇때문인지 궁금합니다!
validateAuthentication 검증을 하는 거 같은데, 저는 이 검증 로직이 서비스 계층에 있는 것이 아키텍처적으로 조금 고민이 되서요!
인증 여부는 보안 필터/인터셉터 계층 또는 컨트롤러 계층에서 처리하고, 서비스 계층은 “인증이 이미 확보된 사용자”에 대해서 로그아웃 비즈니스 로직만 담당하는 구조로 생각해보았습니다!..
저는 그래서 주로 controller단에서 보안처리를 끝내고 비즈니스로직을 들어가는 방식인데, 혁님은 어떻게 생각하시나요?!
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.
그렇겠네요 ...! 필터나 컨트롤러에서 처리하는 것이 더 좋겠네요 ! 좋은 지적 감사합니다 ..!
| @RestController | ||
| @RequestMapping("api/wishes") | ||
| @RequiredArgsConstructor | ||
| public class WishController { |
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.
오 저는 이걸 movie, theater 관련 컨트롤러에 나눠서 구현했는데 이런 식도 좋은 거 같네용
| import java.util.List; | ||
|
|
||
| @RestController | ||
| @RequestMapping("api/wishes") |
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.
api/wishes 앞에 / 가 빠진 거 같습니다!!
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.
수정하겠습니다ㅜㅜ
| validateCartStatus(); | ||
| OrderDetail orderDetail = findOrderDetailsByProductId(productId); | ||
| if (orderDetail == null) { | ||
| throw new IllegalArgumentException("장바구니에 해당 상품 없음"); |
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.
이 부분 에러가 겹치는 거 같은데 에러코드에 추천하는 건 어떨까요??
|
|
||
| if (order.getOrderStatus() == OrderStatus.COMPLETED) { | ||
| throw new CustomException(ErrorCode.ORDER_ALREADY_COMPLETED); | ||
| if (order.getStatus() == OrderStatus.CONFIRMED) { |
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.
오잉 주문 취소가 안 된 상태인데 재고를 왜 올리는건가요?? 궁금합니다!
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.
맞네요.... 아직 취소가 가능한 상태에서 먼저 재고를 올리고 취소하는 흐름인 것 같은데 잘못된 것 같습니다 !
수정하겠습니다
|
|
||
| Product product; | ||
|
|
||
| if (Boolean.TRUE.equals(request.isCombo()) && request.comboItemIds() != null) { |
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.
우와 콤보까지 하셨군요 ! 👍
|
|
||
| @Transactional | ||
| public OrderResponse createOrder(Long memberId, OrderCreateRequest request) { | ||
| Member member = findMemberById(memberId); |
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.
OrderService 코드는 @transactional(readOnly = true) 조건보다는, 쓰기메서드가 더 많은 거 같아요!
@transactional(readOnly = true) 이 부분을 서비스 상단에 말고, 각 메서드에 붙이는 게 낫다고 보는데, 혁님은 어떻게 생각하시나요?
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.
서비스 상단에 readonly = true를 붙이고 트랜잭션을 하는 곳에서만 @transactional을 붙여서 트랜잭션이 일어나는 메서드를 구분하기 쉽게하고, 읽기만 하는 메서드에서 readOnly = true를 누락할 위험이 없어져서 이런 방식을 추천하는 글을 참고했었던 것 같습니다 !
대부분이 쓰기 메서드이긴 해서 메서드에 readOnly = true를 붙이는 방법도 좋을 것 같네요 !
| validateCartNotEmpty(cart); | ||
|
|
||
| cart.confirmOrder(); | ||
| cart.pendPayment(); |
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.
혹시 락 관련 로직은 없는걸까요?! 궁금한데 제가 못 찾고있어서 여쭤봅니다!..
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.
못했습니다... 죄송해요... 😢
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.
많은 인사이트 얻어갑니다. 고생 많으셨습니다!
| @Slf4j | ||
| public class PaymentClient { | ||
|
|
||
| private final WebClient paymentWebClient; |
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.
WebClient 사용법 알아갑니다!
| //================================================================================================================== | ||
| public Member findMemberById(Long id) { | ||
| return memberRepository.findById(id) | ||
| .orElseThrow(() -> new CustomException(ErrorCode.MEMBER_NOT_FOUND)); | ||
| } | ||
|
|
||
| public Schedule findScheduleById(Long id) { | ||
| return scheduleRepository.findById(id) | ||
| .orElseThrow(() -> new CustomException(ErrorCode.SCHEDULE_NOT_FOUND)); | ||
| } | ||
|
|
||
| public List<Seat> findSeatsByIds(List<Long> seatIds) { | ||
| return seatRepository.findAllById(seatIds); | ||
| } | ||
|
|
||
| public List<Reservation> findReservationByMember(Member member) { | ||
| return reservationRepository.findByMember(member) | ||
| .orElseThrow(() -> new CustomException(ErrorCode.RESOURCE_NOT_FOUND)); | ||
| } | ||
| //================================================================================================================== | ||
| private void validateNotReserved(Schedule schedule, List<Long> seatIds) { | ||
| if(reservationSeatRepository.existsByScheduleIdAndSeatIds(schedule.getScheduleId(), seatIds)) | ||
| throw new CustomException(ErrorCode.SEAT_ALREADY_RESERVED); | ||
| } | ||
| //================================================================================================================== | ||
| private void reserveSeats(Reservation reservation, List<Seat> seats) { | ||
| for(Seat seat : seats) { | ||
| ReservationSeat newSeat = ReservationSeat.create(seat); | ||
| reservation.addReservationSeat(newSeat); | ||
| } | ||
| } |
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.
책임 분리를 되게 상세하게 하신 것 같은데, 혹시 이렇게 하신 이유가 있을까요??
내일까지 시험 이슈로.... 과제가 많이 미흡합니다 😢
다음 제출 전까지 다 해올게요....