[1단계 방탈출 결제 / 배포] 구름(김민수) 미션 제출합니다. #4

alstn113 · 2024-05-28T11:09:26Z

안녕하세요 찰리! 구름 ⛅️ 입니다.

1단계는 외부 결제 연동이었어요.
예약 금액이나 기타 부가적인 것들은 2-4단계에서 내용인 것 같아서 제외하고 개발했어요.

결제 승인에 대해서 테스트를 하고 싶었지만 paymentKey같은 것을 도저히 받을 수 없어서 테스트할 수 없었어요.
어떻게 처리하면 좋을까요?

read time out을 위해서 httpclient관련 의존성을 추가해줬습니다. 이게 적절한 방법일까요? 방법이 딱히 나오지를 않네요,,

이번 미션 변경 사항

미리 리뷰 감사합니다! 😊

초기 데이터 정보

어드민

아이디: [email protected]
패스워드: abc123

유저

아이디: [email protected]
패스워드: abc123

Gomding

모르고 Request Changes 로 안남겨서 코멘트가 중복으로 남았습니다 😢

안녕하세요 구름!
미션 빠르게 진행해주셨는데 리뷰가 늦어서 죄송합니다 🥲
몇가지 코멘트 남겼으니 확인해주세요!

궁금한 점 있으면 편하게 DM이나 코멘트 남겨주세요!

Gomding · 2024-05-29T11:49:54Z

README.md

@@ -0,0 +1,6 @@
+## 미션 4 요구사항


결제 승인에 대해서 테스트를 하고 싶었지만 paymentKey같은 것을 도저히 받을 수 없어서 테스트할 수 없었어요.
어떻게 처리하면 좋을까요?

어떤 상황인지 설명이 필요해요!

paymentKey를 어떤 상황에서 받을 수 없었는지 궁금해요

여기서 말하는 테스트가 테스트 코드인지 토스 UI에서 제공하는 API 테스트인지 궁금해요!

아래는 결제 요청 과정이에요.

클라이언트에서 토스 페이먼츠에 위젯을 통해 결제 요청을 하고 응답으로 paymentKey등을 받아요.

결제 요청 시 실제 휴대폰으로 QR을 찍고 결제를 해요.

그 값으로 방탈출 서버에 결제 정보 전달을 포함한 예약 요청을 해요.
서버에서는 예약 생성과 토스 페이먼츠에 결제 승인을 보내요.

이 과정에서 paymentKey를 받아서 처리하기가 힘들더라구요.
테스트를 할 필요가 있는 부분인지도 모르겠어요.

자세한 설명 감사해요! 😊
테스트 관련해서는 이후 코멘트에서 다뤄볼게요!

Gomding · 2024-05-29T11:50:29Z

src/main/java/roomescape/application/PaymentClient.java

+import roomescape.application.dto.request.PaymentRequest;
+import roomescape.application.dto.response.PaymentResponse;
+
+public interface PaymentClient {


PaymentClient 를 interface 로 구현해주신점 👍 👍 👍

어떤 의도를 가지고 interface 로 구현하셨을까요?

토스가 아닌 다른 결제사를 사용할 수도 있고, 요청 방법이 달라질 수도 있다고 생각했어요.
그래서 service에는 interface로 빼고, 구현은 infra에 뒀어요.

그리고 실제로 하지는 않았지만 Fake 객체를 만들어서 테스트할 수도 있겠다고 생각했어요.

토스가 아닌 다른 결제사를 사용할 수도 있고, 요청 방법이 달라질 수도 있다고 생각했어요.
그래서 service에는 interface로 빼고, 구현은 infra에 뒀어요.

DIP를 활용해서 멋지게 구현해주셨네요 :)
구름이 말하는 infra 계층은 어떤 계층일까요?

제가 생각하는 인프라 계층은 다른 계층을 지원하고, 외부 시스템과의 통합, 기술적 세부 사항을 관리하는 역할을 한다고 생각해요.

예를 들어 application에 있는 인터페이스 PaymentClient의 구현체인 TossPaymentClient와 같이 외부 요청 API가 있고, 구체적으로 어떻게 통신하는지를 관리한다고 생각해요.

훌륭하네요!
PaymentClient 로 추상화 해놨기때문에 앞으로 결제API를 변경하거나 추가될 수 있는데
PaymentClient 인터페이스에 맞춰서 구현만 하면 다른 결제 API 도 쉽게 사용할 수 있겠네요~ 👍

PaymentClient가 여럿 생긴다면 이렇게 주입받아 볼 수도 있겠네요 :)

@Service @Transactional(readOnly = true) public class ReservationService { private final ReservationRepository reservationRepository; private final ReservationTimeRepository reservationTimeRepository; private final ThemeRepository themeRepository; private final MemberRepository memberRepository; private final WaitingRepository waitingRepository; private final List<PaymentClient> paymentClient; } @Component public class APaymentClient { } @Component public class BPaymentClient { }

Gomding · 2024-05-29T11:53:20Z

README.md

@@ -0,0 +1,6 @@
+## 미션 4 요구사항
+
+- [x] 사용자가 날짜, 테마, 시간을 선택하고 결제를 해야 예약할 수 있다.


잊지않고 요구사항 정리 해주셨군요 👍 👍 👍

Gomding · 2024-05-29T12:03:33Z

src/main/java/roomescape/config/ClientConfig.java

+    public RestClient restClient(
+            @Value("${payment.base-url}") String paymentBaseUrl,
+            @Value("${payment.connection-timeout-ms}") int connectionTimeoutMs,
+            @Value("${payment.read-timeout-ms}") int readTimeoutMs


@ConfigurationProperties 를 사용해볼 수도 있겠네요 :)

개인적으로 전전 미션부터 ConfiguartionProperties를 사용해볼려고 했으나 이점을 아직 못 느꼈어요.
우선 properties에 대한 depth도 깊지 않고, IDE로 바로 값을 확인할 수 없을 뿐만 아니라 테스트할 때도 properties 객체를 따로 만들어줘야하는게 귀찮더라구요.

물론 ConfiguartionProperties로 관리하면 객체로 관리할 수 있고, 검증도 할 수 있다는 장점이 있다는 것을 알아요.

우선 properties에 대한 depth도 깊지 않고, IDE로 바로 값을 확인할 수 없을 뿐만 아니라 테스트할 때도 properties 객체를 따로 만들어줘야하는게 귀찮더라구요.

구름의 사용의도가 명확하네요 ㅎㅎ 👍
동의하는 부분이라 이대로 가시죠!

Gomding · 2024-05-29T12:04:46Z

src/main/java/roomescape/config/ClientConfig.java

+
+    @Bean
+    public RestClient restClient(
+            @Value("${payment.base-url}") String paymentBaseUrl,


클래스이름은 ClientConfig 라 범용적인 Client 설정이라 생각했는데
정작 baseUrl 은 결제 전용이군요 🤔
이 부분은 어떻게 생각하시나요?

baseUrl은 프로퍼티니까 변경가능하다고 생각했었습니다.
근데 결제뿐만 아니라 더 범용적으로 사용할 수 있겠네요.

baseUrl을 TossPaymentClient로 이동시키고, timeout부분도 payment가 아니라 client라는 이름으로 변경했습니다.

e682468

Gomding · 2024-05-29T12:17:58Z

src/main/java/roomescape/config/ClientConfig.java

+
+    private ClientHttpRequestFactory clientHttpRequestFactory(int connectionTimeoutMs, int readTimeoutMs) {
+        ConnectionConfig connectionConfig = ConnectionConfig.custom()
+                .setConnectTimeout(Timeout.ofMilliseconds(connectionTimeoutMs))


connectTimeout 시간을 5초로 지정하신 기준이 궁금해요 🙋‍♂️

Connection Timeout은 클라이언트에서 설정한 시간까지 서버에 연결되지 않으면 발생해요.

이 부분을 보는게 맞는지 모르겠지만 Connection Timeout이 3분으로 되어있더라구요.
일반적으로 어느 정도로 설정하는지 확인해보니 5~30초로 설정하더라구요.
그래서 5초면 적절할 것이라 판단했습니다.

5초는 어떤 기준에서 적절하다고 판단하셨을까요?
최대 30초로해서 더 관대하게 기다릴 수도 있었을것같아서요!
6초에 커넥션이 맺어졌다면 아쉽지 않을까요? 🤔

+) 하염없이 기다리면 서버에는 어떤 일이 생길까요?

Read Timeout의 경우에는 5초가 짧은 것 같은데 Connection Timeout의 경우 5초 정도면 문제가 있는 것이 아닌가 생각이 들었어요(개인적인 생각)

하염없이 기다리면 스레드 풀의 스레드를 계속 점유하는 문제가 생겨요. 이것은 스레드 풀의 고갈로 이어질 수 있고 서버가 응답하지 못하는 상황으로 연결될 수 있어요.

개인적인 생각도 좋습니다 ㅎㅎ

Read Timeout의 경우에는 5초가 짧은 것 같은데 Connection Timeout의 경우 5초 정도면 문제가 있는 것이 아닌가 생각이 들었어요(개인적인 생각)

여기도 명확한 의견 좋습니다!!

사실 어떤것이 적절한지는 상황마다 다를 수 있고
실제 문제가 발생하기 전에는 상상의 영역일 수 있어요
현재 상황에서 최선으로 보이는 것으로 설정하고 서비스를 운영 + 모니터링을 통해 적절하게 맞춰봐도 좋다고 생각해요 :)

Gomding · 2024-05-29T12:18:11Z

src/main/java/roomescape/config/ClientConfig.java

+                .build();
+
+        SocketConfig socketConfig = SocketConfig.custom()
+                .setSoTimeout(Timeout.ofMilliseconds(readTimeoutMs))


readTimeout 시간도 30초인 이유가 궁금해요 :)

Read Timeout은 클라이언트에서 설정한 시간까지 서버에서 응답이 오지 않으면 발생해요. 클라이언트와 서버가 연결은 됐지만, 서버가 클라이언트의 요청을 정상적으로 처리 못 했을 때 일어나요. Read Timeout은 클라이언트에서 너무 많은 데이터를 조회했거나, 서버가 요청을 처리하는데 시간이 오래 걸려서 발생하는 경우가 많아요.

Read Timeout은 연결은 됐지만 서버가 요청을 처리하지 못할 때 일어나요.
특히 서버가 요청을 처리하는데 시간이 오래 걸려서 주로 발생한다고 생각해요.

토스 페이먼츠 문서에 Read Timeout은 30-60초로 설정하면 된다고하더라구요.
제 일반적인 경험 상 크게 오래 걸린 적이 없는 것 같아서 최소 값인 30초로 정했어요.

토스 페이먼츠 타임아웃 문서

크... 문서까지 꼼꼼하게 확인하셨군요 👍 👍 👍 👍
훌륭합니다!!

Gomding · 2024-05-29T12:18:35Z

src/main/java/roomescape/infra/TossPaymentClient.java

+                .header("Authorization", encodedSecretKey)
+                .body(paymentRequest)
+                .retrieve()
+                .onStatus(HttpStatusCode::isError, (req, res) -> {


에러 잡아서 처리하신점 👍

Gomding · 2024-05-29T12:19:52Z

src/main/java/roomescape/infra/TossPaymentClient.java

+                    PaymentErrorResponse errorResponse = objectMapper
+                            .readValue(res.getBody(), PaymentErrorResponse.class);
+
+                    throw new PaymentException(errorResponse.message());


모든 에러 메세지를 클라이언트에게 떠넘겨도 괜찮을까요? 🤔

API 호출시 발생할 수 있는 에러마다 적절하게 처리하는 방법은 없었을까요!

흠,,, 정확히 의미를 모르겠어요.
의미를 모르겠다기 보다는 어느 정도까지 처리를 하는게 맞는지 궁금해요.

토스 페이먼츠 결제 승인에 예외 코드들이 많이 있어요.

클라이언트 에러 부분에 대해서는 클라이언트한테 그대로 넘기고,
서버 에러 부분에 대해서는 "결제 승인 에러입니다."와 같은 특정한 예외 메세지를 넘기는 것이 좋은 방법일까요?

다른 방법이 잘 떠오르지 않네요 ,,,

클라이언트와 서버 에러로 분리해서 처리해줬습니다.

9955535

앗 제가 애매하게 코멘트를 드렸군요... 😭
생각했던것은 토스 API 문서에 에러코드에 대한 상세 정보가 있을거에요

결제 승인 API 에러 코드

에러 객체

에러 코드를 가지고 클라이언트에게 적절하게 응답해줄 수 있을거에요 :)
만약 토스에서 내려주는 에러 응답에 메세지를 그대로 사용하면
토스에서 에러 메세지를 변경하는대로 클라이언트도 다른 에러 메세지를 받게 되겠죠..
(토스의 변화에 우리의 서비스에 변화가 생긴다..?)

외부 API 를 사용한다면 에러 코드를 가지고 적절하게 우리의 것으로 변경해볼 수 있어요!!

예시를 들어보면 에러 객체에 code가 INVALID_CARD_EXPIRATION로 왔을 때 아래와 같이 메세지를 변환해볼 수 있곘네요 :)

토스 메세지 : 카드 정보를 다시 확인해주세요. (유효기간)

처리한 메세지 : 유효하지 않은 카드입니다. 유효기간을 포함하여 카드 정보를 확인해주세요

물론 토스의 API 는 에러 메세지가 친절하고 보기 좋은 편이지만
그렇지 않은 경우도 많이봤습니다 😂

클라이언트에 보낼만한 것들은 예외 메세지를 보내도록하고 나머지는 "결제 승인 중 오류가 발생했습니다"로 응답했습니다.
추가로 빠른 대응을 위해서 로그를 사용했습니다.

19a731a

Gomding · 2024-05-29T12:22:08Z

src/test/java/roomescape/application/ReservationServiceTest.java

@@ -55,6 +59,9 @@ class ReservationServiceTest extends BaseServiceTest {
    @Autowired
    private ReservationTimeRepository reservationTimeRepository;

+    @SpyBean
+    private PaymentClient paymentClient;


결제 테스트를 작성하신것을 보니
코멘트에 써주신게 테스트 코드에서 결제 테스트가 불가능하다는 말씀이었던것 같네요 :)

만약 맞다면 실제 결제 테스트가 필요하다고 생각하신 이유가 궁금해요!
가짜 객체를 사용한 테스트를 신뢰할 수 없다면 그 이유도 공유해주시면 좋겠어요 🤗

외부 API 요청이 들어가면 컨트롤러나 서비스에서 적절하게 가짜 객체를 이용해서 단순하게 테스트를 하는게 맞다고 생각합니다.
그래도 한 번쯤은 실제 요청을 끼고 전체 동작을 테스트해야 신뢰성이 생긴다고 생각했어요.

저는 가짜 객체를 활용한 테스트로 현재 충분하다고 생각해요!
외부 API 가 정상 동작하는지는 테스트 코드에서 큰 관심사가 아니라고 생각해서요 :)
테스트 코드에서는 우리가 만든 로직에 대한 테스트에 집중하는것이 좋다고 생각해요 ㅎㅎ

Gomding

안녕하세요 구름!
미션 빠르게 진행해주셨는데 리뷰가 늦어서 죄송합니다 🥲
몇가지 코멘트 남겼으니 확인해주세요!

궁금한 점 있으면 편하게 DM이나 코멘트 남겨주세요!

alstn113 · 2024-05-29T16:00:46Z

추가 변경 사항입니다.

'AUTHORIZATION'을 HttpHeaders.AUTHORIZATION으로 변경했습니다. f1eb62d
결제 금액을 Long에서 BigDecimal로 변경했습니다. 11a91ac

궁금한 부분입니다.

현재 예약 생성과 결제 승인이 한 transaction에 물려있습니다.
그 이유는 예약 후 결제 실패하면 rollback하기 위함인데요.
하지만 외부 요청이 transaction에 있을 경우 요청 지연이 있으면 DB 커넥션도 계속 물고 있어서 문제가 될 것 같아요.
해결 방법으로 이벤트를 사용하는 방법이 있을 것 같아요. 혹시 다른 방법은 없을까요?

Gomding

안녕하세요 구름!
의견 남겨주셔서 감사해요 🙇
추가로 코멘트 남겼으니 확인해주세요 🙏

궁금한 점 있으면 편하게 DM이나 코멘트 남겨주세요~!!

Gomding · 2024-05-30T14:35:42Z

README.md

@@ -0,0 +1,6 @@
+## 미션 4 요구사항


자세한 설명 감사해요! 😊
테스트 관련해서는 이후 코멘트에서 다뤄볼게요!

Gomding · 2024-05-30T14:36:05Z

src/main/java/roomescape/application/PaymentClient.java

+import roomescape.application.dto.request.PaymentRequest;
+import roomescape.application.dto.response.PaymentResponse;
+
+public interface PaymentClient {


토스가 아닌 다른 결제사를 사용할 수도 있고, 요청 방법이 달라질 수도 있다고 생각했어요.
그래서 service에는 interface로 빼고, 구현은 infra에 뒀어요.

DIP를 활용해서 멋지게 구현해주셨네요 :)
구름이 말하는 infra 계층은 어떤 계층일까요?

Gomding · 2024-05-30T14:37:36Z

src/main/java/roomescape/application/ReservationService.java

@@ -58,6 +63,8 @@ public synchronized ReservationResponse addReservation(ReservationRequest reques

        Reservation savedReservation = reservationRepository.save(reservation);

+        paymentClient.confirmPayment(new PaymentRequest(request.paymentKey(), request.orderId(), request.amount()));
+


구름의 메서드내에서 개행 기준은 무엇인지 궁금해요!
글쓰기와 똑같이 코드에서도 개행에는 의미가 있어야한다고 생각해서요 :)
저와 스타일이 다르길래
인사이트를 얻고자 물어보는것이니 편하게 말씀해주시면 됩니다 🤗

어떤 부분을 말하시는걸까요?

save 다음 개행을 한 이유는 생성과 지불이라는 성질이 달라서 입니다.
paymentClient.confirmPayment 다음 개행을 한 이유는 return이 잘 보이라고 항상 return 위를 띄우는 편입니다.

규칙을 가지고 계시는군요 ㅎㅎ
저는 한 줄이라도 줄이고자 문맥이 확실하게 나뉘는 상황이 아니면 개행을 가능한 안하고 있어요!
(저는 생성 후 지불하는구나 라고 읽히기때문에 흐름상 개행이 없는게 더 좋다고 생각해서요. 특히 서비스 로직은 흐름이니.. 😄 )
return 전 개행은 구름과 동일하게 하고있습니다 :)

Gomding · 2024-05-30T14:42:53Z

src/main/java/roomescape/application/dto/request/ReservationRequest.java

+        String paymentKey,
+        String orderId,
+        BigDecimal amount,
+        String paymentType,


Request 에 결제에 대한 요청은 한 번 더 묶어볼 수도 있을것같아요 :)
개인 취향인 부분이니 당연히 필수 반영아닙니다!!
구름의 취향은 어떤지 궁금해요 🙋‍♂️

제가 말씀드린것을 표현해보자면 아래와 같아요

public record ReservationRequest( LocalDateTime currentDateTime, LocalDate date, Long themeId, Long timeId, Long memberId, PaymentRequest payment ) { } public record PaymentRequest( String paymentKey, String orderId, BigDecimal amount, String paymentType, )

조금 복잡해지긴 했지만 추후 확장을 생각하면 좋은 것 같아요.
참고로 AdminReservationWebRequest, ReservationWebRequest라는 Web Dto가 있고, 서비스로 줄 때 ReservationRequest로 변경해서 줍니다. 혹시 헷갈리실 것 같아서요.

외부 API 요청에 사용하는 것들을 PaymentApiResponse, PaymentApiRequest로 변경했습니다.

d04d158

변경사항까지 설명해주셔서 감사해요!
리뷰어를 배려하여 반영 커밋 링크까지 남겨주시는것이 너무 친절하네요 👍

Gomding · 2024-05-30T14:48:37Z

src/main/java/roomescape/infra/payment/TossPaymentClient.java

+                .body(PaymentResponse.class);
+    }
+
+    private String encodeSecretKey(String secretKey) {


encode 로직도 client 에 넣어주신점 👍

Gomding · 2024-05-30T15:06:38Z

src/main/java/roomescape/infra/TossPaymentClient.java

+                    PaymentErrorResponse errorResponse = objectMapper
+                            .readValue(res.getBody(), PaymentErrorResponse.class);
+
+                    throw new PaymentException(errorResponse.message());


앗 제가 애매하게 코멘트를 드렸군요... 😭
생각했던것은 토스 API 문서에 에러코드에 대한 상세 정보가 있을거에요

결제 승인 API 에러 코드

에러 객체

에러 코드를 가지고 클라이언트에게 적절하게 응답해줄 수 있을거에요 :)
만약 토스에서 내려주는 에러 응답에 메세지를 그대로 사용하면
토스에서 에러 메세지를 변경하는대로 클라이언트도 다른 에러 메세지를 받게 되겠죠..
(토스의 변화에 우리의 서비스에 변화가 생긴다..?)

외부 API 를 사용한다면 에러 코드를 가지고 적절하게 우리의 것으로 변경해볼 수 있어요!!

예시를 들어보면 에러 객체에 code가 INVALID_CARD_EXPIRATION로 왔을 때 아래와 같이 메세지를 변환해볼 수 있곘네요 :)

토스 메세지 : 카드 정보를 다시 확인해주세요. (유효기간)

처리한 메세지 : 유효하지 않은 카드입니다. 유효기간을 포함하여 카드 정보를 확인해주세요

물론 토스의 API 는 에러 메세지가 친절하고 보기 좋은 편이지만
그렇지 않은 경우도 많이봤습니다 😂

Gomding · 2024-05-30T15:15:40Z

src/test/java/roomescape/application/ReservationServiceTest.java

@@ -55,6 +59,9 @@ class ReservationServiceTest extends BaseServiceTest {
    @Autowired
    private ReservationTimeRepository reservationTimeRepository;

+    @SpyBean
+    private PaymentClient paymentClient;


저는 가짜 객체를 활용한 테스트로 현재 충분하다고 생각해요!
외부 API 가 정상 동작하는지는 테스트 코드에서 큰 관심사가 아니라고 생각해서요 :)
테스트 코드에서는 우리가 만든 로직에 대한 테스트에 집중하는것이 좋다고 생각해요 ㅎㅎ

Gomding · 2024-05-30T15:27:26Z

src/main/java/roomescape/infra/payment/TossPaymentClient.java

+    public TossPaymentClient(
+            @Value("${payment.base-url}") String paymentBaseUrl,
+            @Value("${payment.secret-key}") String secretKey,
+            RestClient.Builder restClient,


ClientConfig 에서 설정한 빈이 여기에 주입되고 있을까요? 🤔
구름이 애써 설정해준 timeout 설정들이 안들어오고 있을것같아서요 😳
디버깅을 해봐도 좋을것같아요 :)

음 ㅋㅋㅋ
RestClient.Builder를 해도 innerClass라서 된다는 크루의 말을 믿었는데
역시 모두 디버깅을 해봐야겠네요 ㅎㅎ

수정했습니다 :)

e92e909

Gomding · 2024-05-30T15:30:03Z

src/main/java/roomescape/application/ReservationService.java

@@ -58,6 +63,8 @@ public synchronized ReservationResponse addReservation(ReservationRequest reques

        Reservation savedReservation = reservationRepository.save(reservation);

+        paymentClient.confirmPayment(new PaymentRequest(request.paymentKey(), request.orderId(), request.amount()));


궁금한 부분입니다.
현재 예약 생성과 결제 승인이 한 transaction에 물려있습니다.
그 이유는 예약 후 결제 실패하면 rollback하기 위함인데요.
하지만 외부 요청이 transaction에 있을 경우 요청 지연이 있으면 DB 커넥션도 계속 물고 있어서 문제가 될 것 같아요.
해결 방법으로 이벤트를 사용하는 방법이 있을 것 같아요. 혹시 다른 방법은 없을까요?

이벤트를 사용한다면 어떤 방식으로 풀어볼 생각이실까요? 👍

하지만 외부 요청이 transaction에 있을 경우 요청 지연이 있으면 DB 커넥션도 계속 물고 있어서 문제가 될 것 같아요.

좋은 고민이네요!! 👍

다음 단계에서 공부하고 적용해본 후 알려드리겠습니다 ㅎㅎ,,,

Gomding · 2024-05-30T15:30:46Z

src/main/java/roomescape/application/dto/request/ReservationRequest.java

@@ -8,6 +9,10 @@ public record ReservationRequest(
        LocalDate date,
        Long themeId,
        Long timeId,
+        String paymentKey,
+        String orderId,
+        BigDecimal amount,


Long에서 BigDecimal 로 변경하신 이유도 궁금해요 🤗

어떤 장점을 느끼셨을까요?

다른 리뷰에서 금액으로 소수점이 나오면 어떻게 할거냐는 피드백을 봤어요.
그래서 저도 그것을 대응하고자 소수점 정밀도가 높은 BigDecimal을 사용했어요. 금액에서 자주 사용하기도 하구요. :)

Gomding

안녕하세요 구름!
코멘트 반영하느라 고생하셨어요 👍 👍 👍
요구사항은 다 만족하셔서 이번 단계는 이만 머지할게요 :)
다음단계도 화이팅입니다~

Gomding · 2024-06-01T01:33:06Z

src/main/java/roomescape/application/PaymentClient.java

+import roomescape.application.dto.request.PaymentRequest;
+import roomescape.application.dto.response.PaymentResponse;
+
+public interface PaymentClient {


훌륭하네요!
PaymentClient 로 추상화 해놨기때문에 앞으로 결제API를 변경하거나 추가될 수 있는데
PaymentClient 인터페이스에 맞춰서 구현만 하면 다른 결제 API 도 쉽게 사용할 수 있겠네요~ 👍

PaymentClient가 여럿 생긴다면 이렇게 주입받아 볼 수도 있겠네요 :)

@Service @Transactional(readOnly = true) public class ReservationService { private final ReservationRepository reservationRepository; private final ReservationTimeRepository reservationTimeRepository; private final ThemeRepository themeRepository; private final MemberRepository memberRepository; private final WaitingRepository waitingRepository; private final List<PaymentClient> paymentClient; } @Component public class APaymentClient { } @Component public class BPaymentClient { }

Gomding · 2024-06-01T01:35:04Z

src/main/java/roomescape/config/ClientConfig.java

+    public RestClient restClient(
+            @Value("${payment.base-url}") String paymentBaseUrl,
+            @Value("${payment.connection-timeout-ms}") int connectionTimeoutMs,
+            @Value("${payment.read-timeout-ms}") int readTimeoutMs


우선 properties에 대한 depth도 깊지 않고, IDE로 바로 값을 확인할 수 없을 뿐만 아니라 테스트할 때도 properties 객체를 따로 만들어줘야하는게 귀찮더라구요.

구름의 사용의도가 명확하네요 ㅎㅎ 👍
동의하는 부분이라 이대로 가시죠!

Gomding · 2024-06-01T01:39:07Z

src/main/java/roomescape/config/ClientConfig.java

+
+    private ClientHttpRequestFactory clientHttpRequestFactory(int connectionTimeoutMs, int readTimeoutMs) {
+        ConnectionConfig connectionConfig = ConnectionConfig.custom()
+                .setConnectTimeout(Timeout.ofMilliseconds(connectionTimeoutMs))


개인적인 생각도 좋습니다 ㅎㅎ

Read Timeout의 경우에는 5초가 짧은 것 같은데 Connection Timeout의 경우 5초 정도면 문제가 있는 것이 아닌가 생각이 들었어요(개인적인 생각)

여기도 명확한 의견 좋습니다!!

사실 어떤것이 적절한지는 상황마다 다를 수 있고
실제 문제가 발생하기 전에는 상상의 영역일 수 있어요
현재 상황에서 최선으로 보이는 것으로 설정하고 서비스를 운영 + 모니터링을 통해 적절하게 맞춰봐도 좋다고 생각해요 :)

Gomding · 2024-06-01T01:43:03Z

src/main/java/roomescape/infra/payment/PaymentConfirmErrorCode.java

+import java.util.Arrays;
+import org.springframework.http.HttpStatus;
+
+public enum PaymentConfirmErrorCode {


그저 감동이군요 😢
고생하셨습니다.....
외부 API 사용시에 외부 서비스의 에러를 핸들링 해야하는일이 많으니 경험만 해보자 라는 취지였는데
굉장히 잘 해주셨네요 😮

+) 토스만의 에러코드가 될테니 이름에 표현되면 좋겠어요!
다른 외부 API 가 생긴다면 enum을 분리하는것이 좋을것 같아서요
구름은 어떻게 생각하시나요?

아 그러네요!
Infra인데 구체적으로 적는 걸 깜빡했네요

Gomding · 2024-06-01T01:46:40Z

src/main/java/roomescape/application/ReservationService.java

@@ -58,6 +63,8 @@ public synchronized ReservationResponse addReservation(ReservationRequest reques

        Reservation savedReservation = reservationRepository.save(reservation);

+        paymentClient.confirmPayment(new PaymentRequest(request.paymentKey(), request.orderId(), request.amount()));
+


규칙을 가지고 계시는군요 ㅎㅎ
저는 한 줄이라도 줄이고자 문맥이 확실하게 나뉘는 상황이 아니면 개행을 가능한 안하고 있어요!
(저는 생성 후 지불하는구나 라고 읽히기때문에 흐름상 개행이 없는게 더 좋다고 생각해서요. 특히 서비스 로직은 흐름이니.. 😄 )
return 전 개행은 구름과 동일하게 하고있습니다 :)

Gomding · 2024-06-01T02:20:39Z

src/main/java/roomescape/application/dto/request/ReservationRequest.java

+        String paymentKey,
+        String orderId,
+        BigDecimal amount,
+        String paymentType,


변경사항까지 설명해주셔서 감사해요!
리뷰어를 배려하여 반영 커밋 링크까지 남겨주시는것이 너무 친절하네요 👍

Gomding · 2024-06-01T02:21:28Z

src/main/java/roomescape/infra/payment/TossPaymentClient.java

@@ -21,12 +21,12 @@ public class TossPaymentClient implements PaymentClient {
    public TossPaymentClient(
            @Value("${payment.base-url}") String paymentBaseUrl,
            @Value("${payment.secret-key}") String secretKey,
-            RestClient.Builder restClient,
+            RestClient restClient,


잘 수정해주셨네요!! 👍

alstn113 added 8 commits May 28, 2024 14:31

migrate third mission

531920a

docs: 미션 4 요구사향 작성

fb2af1d

refactor: 클라이언트 결제 페이지 추가

ada555f

refactor: 예약 결제 실패 예외 메세지 경고 알림 추가

fdc5122

feat: 결제 요청 url, secret-key 프로퍼티 추가

9994922

feat: 결제 요청을 위한 외부 API 연동

6947987

refactor: 예약 결제 기능 추가

5ab9b0e

test: 예약 생성 테스트에 외부 요청 mocking 추가

ac81767

alstn113 changed the base branch from main to alstn113 May 28, 2024 11:12

alstn113 added 4 commits May 29, 2024 14:18

refactor: connection timeout, connection read time out 처리

e10ea65

refactor: connection, read timeout 30초로 변경

06d69f1

refactor: connection timeout 5초로 변경

647a1cd

refactor: timeout properties로 이동

06f2c76

Gomding reviewed May 29, 2024

View reviewed changes

Gomding requested changes May 29, 2024

View reviewed changes

alstn113 added 3 commits May 29, 2024 23:54

refactor: ClientConfig의 payment 코드 분리

e682468

refactor: 'AUTHORIZATION'을 HttpHeaders에 있는 것으로 변경

f1eb62d

refactor: 결제 금액 Long -> BigDecimal로 변경

11a91ac

alstn113 added 7 commits May 30, 2024 15:47

refactor: rest template builder로 변경

bd3b43c

chore: 공백 줄 제거

1b1cff8

refactor: 메서드 개선

c8fd7dc

refactor: httpclient5 의존성 제거

f9d766c

fix: Read Timeout 안들어가는 문제 수정

9e8d607

refactor: 결제 승인 예외를 클라이언트와 서버 에러로 분리

9955535

fix: Toss Payment Client 에러 핸들러 사용

3a8b51a

Gomding requested changes May 30, 2024

View reviewed changes

alstn113 added 3 commits May 31, 2024 01:14

refactor: ReservationRequest에서 PaymentRequest 부분 분리

d04d158

fix: RestClient.Builder 의존성 주입 문제 수정

e92e909

refactor: rest template 코드 개선

06be8b4

refactor: 토스 페이먼츠 예외 구체적으로 변경

19a731a

Gomding approved these changes Jun 1, 2024

View reviewed changes

Gomding merged commit f226ef6 into woowacourse:alstn113 Jun 1, 2024

		@@ -0,0 +1,6 @@
		## 미션 4 요구사항

		- [x] 사용자가 날짜, 테마, 시간을 선택하고 결제를 해야 예약할 수 있다.

		@@ -58,6 +63,8 @@ public synchronized ReservationResponse addReservation(ReservationRequest reques

		Reservation savedReservation = reservationRepository.save(reservation);

		paymentClient.confirmPayment(new PaymentRequest(request.paymentKey(), request.orderId(), request.amount()));

[1단계 방탈출 결제 / 배포] 구름(김민수) 미션 제출합니다. #4

[1단계 방탈출 결제 / 배포] 구름(김민수) 미션 제출합니다. #4

Conversation

alstn113 commented May 28, 2024 • edited Loading

초기 데이터 정보

Gomding left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alstn113 May 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alstn113 May 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Gomding left a comment

Choose a reason for hiding this comment

alstn113 commented May 29, 2024 • edited Loading

추가 변경 사항입니다.

궁금한 부분입니다.

Gomding left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Gomding left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alstn113 commented May 28, 2024 •

edited

Loading

Gomding left a comment •

edited

Loading

alstn113 May 30, 2024 •

edited

Loading

alstn113 May 29, 2024 •

edited

Loading

alstn113 commented May 29, 2024 •

edited

Loading