[1단계 방탈출 결제 / 배포] 알파카(최휘용) 미션 제출합니다. #36
Conversation
| @Value("${payment.approve.key}") | ||
| private String approveSecretKey; |
There was a problem hiding this comment.
지금은 코드에 없긴한데, 커밋 기록에는 secret key 가 남아있어요.
물론 지금은 미션이라 properties 파일도 다 git 에 올리지만, 이런 민간한 정보는 올리지 않는 경우가 많아요!
repository 가 private 처리 되어있다면 비교적 안전하겠지만, 그럼에도 올리지 않기도 해요 :)
There was a problem hiding this comment.
properties 파일을 숨기는 것은 인지하고 있던 부분이지만, 리뷰어님이 코드를 실행하는 것과 테스트용 secret key를 사용하는 미션이라는 사실 때문에 굳이 신경쓰지 않았습니다..ㅎㅎ
다만 커밋 기록에서도 조심해야 하는 건 미처 고려하지 못했네요 감사합니다 🙇
| @Component | ||
| public class PaymentClient { | ||
| private final RestClient restClient = RestClient.builder() | ||
| .baseUrl("https://api.tosspayments.com") |
There was a problem hiding this comment.
외부 api 경로와 url 을 코드가 아닌 곳에서 관리해보는건 어떨까요?
왜 그러면 좋을지 생각해보면 좋을 것 같아요!
There was a problem hiding this comment.
처음에는 API 호출을 여러 군데에서 할 경우 분리에서 얻는 장점을 생각했습니다.
근데 좀 더 고민해보니 그걸 고려할 정도로 토스페이먼츠에 대한 API 요청이 많아질까? + 굳이 지금 분리하지 않아도 나중에 분리가 필요할 때 그렇게 불편하지 않을 것 같아 지금 상황에서 고려할 부분은 아니다 라는 생각을 하게 되었습니다.
그래서 제 생각에는 아래의 리뷰에서 언급된, test에서 실제 api key를 사용하지 않아야 하는 이유와 비슷할 것 같습니다.
테스트에서 실제 api를 요청할 필요도 없고 요청해서도 안되므로 코드가 아닌 properties 등의 파일에서 관리하면서 테스트에서는 다른 url을 사용하도록 하면 편할 것 같습니다.
근데 코드 외에서 관리하는 건 외부로부터 숨길 수 있다는 것도 큰 장점 같은데, 이 부분에서의 필요성은 딱히 없어 보입니다.
혹시 제가 놓친 부분이 있을까요? 피케이의 의견이 궁금합니다.
There was a problem hiding this comment.
알파카 생각에는 따로 추출하는게 좋아보인다는 의미일까요? 아니면 그대로 유지하는게 좋아보인다는 의미일까요?
There was a problem hiding this comment.
충분히 분리할 만한 메리트가 있다고 생각했지만, 혹시 따로 추출함으로써 얻는 장점 중 혹시 제가 놓친 부분이 있을까 해서 여쭤봤습니다..ㅎㅎ
There was a problem hiding this comment.
ㅋㅋㅋㅋ 잘 적어주셨어요. 👍
어떤 경우엔 코드를 수정하거나 추가로 배포할 필요 없이 외부 파일만 수정하여 경로를 바꿔주거나 설정을 변경하기도 해요.
훨씬 편리하게 '자주 바뀔 수 있는 설정'을 관리하는거죠!
| public Payment approve(PaymentApproveRequest paymentApproveRequest, Member member) { | ||
| String encryptedKey = Base64.getEncoder().encodeToString(approveSecretKey.getBytes()); | ||
| ApproveApiResponse response = Optional.ofNullable(restClient.post() | ||
| .uri("/v1/payments/confirm") |
| .retrieve() | ||
| .onStatus(errorHandler) | ||
| .body(ApproveApiResponse.class)) | ||
| .orElseThrow(() -> new ApiCallException("알 수 없는 오류가 발생했습니다.")); |
There was a problem hiding this comment.
지금 미션에서 도입하지 않아도 괜찮지만,
외부 api 를 사용하면서 발생하는 문제를 미리 대응해볼만한 요소가 어떤게 있는지 생각해보는 것도 재밌을 것 같아요.
예를 들어 결제 승인 api 가 여러번 호출되거나 그 api 서버가 다운되었을 때가 있겠네요 🤔
There was a problem hiding this comment.
만약 API 서버가 다운된다면 요청을 보내고 응답이 오지 않아서 계속 기다리기만 하겠네요.
이번에 새로 read timeout, connection timeout이라는 키워드를 알게 됐는데 이를 적용하면 해결할 수 있을 것 같아 코드에 적용해봤습니다.
그런데 이 부분에 대해 궁금한 점이 생겼습니다.
만약 이런 타임아웃을 테스트하는 코드를 짜게 되면 실제로 3초를 기다려야 하나? 만약 3초가 아니라 30초 60초가 된다면 전부 기다리는 건 힘들 것 같은데.. 그렇다면 타임아웃에 대한 테스트는 포기해야 하나? 하는 고민이 들었습니다.
실제로 현업에서는 이를 어떻게 처리하는지, 피케이의 생각은 어떤지 궁금합니다.
There was a problem hiding this comment.
결제 승인 api가 여러번 호출되는 경우를 고민해봤는데 잘 모르겠네요..ㅎㅎ
제가 생각할 수 있는 경우는 결제 qr에 여러명이 동시에 접근해서 결제해 같은 승인 api가 여러번 호출되는 경우일 것 같습니다.
결제 승인 api를 호출할 때 결제 키인 paymentKey와 주문 키인 orderId를 넣어서 호출하는데 토스페이먼츠 API에서 중복된 결제에 대한 처리를 따로 어떻게 하는지 좀 더 알아봐야 할 것 같습니다.
만약 백엔드 코드에서 처리해야 한다면, 결제가 들어왔을 때 결제 정보인 paymentKey 등을 통해 이미 들어온 요청인지 확인하고 이전에 성공했는지 실패했는지에 따라 다시 요청을 보내거나 예외로 처리하는 식으로 해결할 수 있을 것 같습니다.
근데 만약 같은 요청이 동시에 들어와 db에 저장하기 전에 처리해야 한다면 위의 방식으로는 해결할 수 없을 것 같네요.
피케이께서 이에 대한 힌트를 조금만 더 주시면 제가 좀 더 알아보고 공부해봐도 괜찮을까요...? 🙇
-> 추가적으로 토스 API 문서를 확인해보니 POST 요청에는 멱등키 헤더를 추가해 중복 요청에 대한 처리를 할 수 있네요. 그런데 결제 취소나 지급 대행 요청 API에만 멱등키 헤더에 대한 내용이 나와 있어서 결제에서는 따로 처리가 필요하지 않은 것으로 느껴집니다. 이에 대해서는 조금만 더 알아보고 추가적으로 답변을 드리겠습니다.. 😢
There was a problem hiding this comment.
우선 타임아웃에 관련된 테스트는 저는 하지 않아요. JPA 의 기능에 대한 테스트를 작성하지 않는 것처럼, 외부 api 호출시 사용하는 기능을 검증하지 않는거죠.
그리고 “멱등키” 라는 키워드를 한 번 생각해보면 좋을 것 같아서 남긴 리뷰가 맞아요! 결제가 아닌 다른 도메인에서도 api 가 여러번 호출 되는 것을 방지하기 위해 사용하는 방식이거든요. 충분히 잘 알아보셨다고 생각합니다 👍
| @@ -0,0 +1,4 @@ | |||
| package roomescape.dto; | |||
|
|
|||
| public record PaymentApproveRequest(String paymentKey, String orderId, long amount) { | |||
There was a problem hiding this comment.
amount 를 long 으로 둔 이유가 궁금해요.
(틀렸다는건 아니고 그냥 궁금해서 남기는 리뷰입니다.)
There was a problem hiding this comment.
해당 값이 null인 경우를 고려할 필요가 없다고 생각해서 원시값을 사용하게 되었습니다!
| private final RestClient restClient = RestClient.builder() | ||
| .baseUrl("https://api.tosspayments.com") |
There was a problem hiding this comment.
빈으로 등록하지 않고 사용하시는 이유가 궁금해요.
그리고 RestClient 말고 다른 선택지도 있었을 것 같은데, 이걸 선택한 이유도 알려주세요!
There was a problem hiding this comment.
이미 빈으로 등록되어 있는 PaymentClient에서만 사용 중인 클래스이므로 지금 당장 빈으로 등록할 필요성을 느끼지 못했습니다.
API 요청이 늘어나게 될 경우 RestClient를 빈으로 등록하는 것의 이점을 의도하신 것이 맞으실까요?
RestClient는 함수형 프로그래밍으로 쓰기 편하고 가독성도 좋다고 생각해 선택하게 되었습니다.(그리고 좀 더 최신 기술이라 끌린 점도 있습니다..)
사실 여러 선택지를 상세하게 알아보고 비교해서 선택한 것은 아니라 좀 더 찾아보니, 비동기를 지원한다는 장점도 있었습니다.
There was a problem hiding this comment.
빈으로 등록한다면 어떻게 사용하는가에 따라 다른 api 요청에서도 사용할 수도 있지만, 저는 정말로 그냥 알파카가 어떤 생각으로 이렇게 만들었는지 궁금해서 리뷰를 남기는 경우가 대부분이에요. 딱히 틀리지도 않았구요!
| spring.jpa.properties.hibernate.format_sql=true | ||
| spring.jpa.hibernate.ddl-auto=create-drop | ||
| spring.jpa.defer-datasource-initialization=true | ||
| payment.approve.key=test_gsk_docs_OaPz8L5KdmQXkzRz3y47BMw6: |
There was a problem hiding this comment.
test 와 production profile 에서 같은 key 를 사용해도 괜찮을까요?
There was a problem hiding this comment.
테스트에서는 실제 key 값을 사용하지는 않으니까 같은 key를 사용하지 않아도 괜찮겠네요..
이 부분도 중요한 key 값이 아니라 신경 쓰지는 않았지만 test에서는 api 요청을 실제로 보내지도 않고 보내면 안 되므로 다른 값을 사용하도록 수정했습니다.
| @@ -0,0 +1,443 @@ | |||
| # 요구사항 문서 | |||
There was a problem hiding this comment.
지금은 결제가 예약에만 적용되지만, 예약 대기가 자동 승인되기 때문에, 추후 예약 대기와도 결제가 연동되어야 합니다. 따라서 결제와 예약을 최대한 분리하여 별도의 도메인으로 만들었습니다.
틀린 건 아니지만, 예약 대기가 자동승인됨. 대기할 때도 결제가 연동되어야함 이라는 부분 때문에 예약과 결제를 분리함 이라는 선택을 한 흐름을 제가 잘 이해하지 못했어요. 조금만 더 설명해주시겠어요?
There was a problem hiding this comment.
만약 예약과 결제를 분리하지 않고 하나의 도메인으로 처리하기 위해 예약 도메인 안에 결제 기능을 추가한다면, 예약 대기에서 결제 기능을 추가하려고 할 때 똑같이 기능 추가를 반복해야 하지만, 예약과 결제를 분리함으로써 결제 도메인을 예약 대기 로직에서 호출하는 것만으로 해결할 수 있을 것이라 생각했습니다!
| @@ -0,0 +1,443 @@ | |||
| # 요구사항 문서 | |||
|
|
|||
There was a problem hiding this comment.
제 마음에 들지 않는 코드가 있더라도 그것이 이미 이전 미션을 통해 충분히 학습한 내용이면 수정하지 않았습니다.
좋은 선택이에요.
분명 앞으로도 다른 사람의 코드를 보며 썩 마음에 들지 않더라도 그대로 두고 유지보수하는 경우가 훨씬 많을겁니다.
그런 것도 이 참에 한 번 경험해보는거죠! (반대로 그 사람도 내 코드를 안 좋아할 수도 있구요)
| @ExceptionHandler(ApiCallException.class) | ||
| public ResponseEntity<ErrorResponse> handle(ApiCallException e) { | ||
| logger.error(e.getMessage(), e); | ||
| return ResponseEntity | ||
| .status(HttpStatus.FORBIDDEN) | ||
| .body(new ErrorResponse(e.getMessage())); | ||
| } | ||
|
|
There was a problem hiding this comment.
ApiCallException 은 외부 api 로부터 받은 예외 메시지를 담고 있을거에요.
이를 client 로 바로 내려주기보단, 우리가 원하는 메시지로 바꿔주는건 어떨까요?
There was a problem hiding this comment.
토스페이먼츠 API에서 내려주는 예외 메시지의 종류가 너무 많은데 그 중에 사용자에게 전달해야 하는 메시지도 많아서 그냥 내려주기로 선택했었습니다.
그런데 리뷰를 받고 다시 확인해보니 사용자에게 전달될 필요가 없는 예외도 많아서 따로 처리를 해주는 편이 낫겠네요.
그래서 사용자가 알아야 할 예외 코드를 갖고 있는 enum을 만든 후 해당 enum에 없는 코드의 예외일 경우 따로 메시지를 처리하도록 수정했습니다.
그런데 이렇게 할 경우, 매번 외부 API를 호출할 때마다 이런 코드를 새로 추가해야 하나? + 예외의 종류가 더 많아진다면 관리 비용이 만만치 않을 것 같기도 하고, 토스페이먼츠의 예외 정책이 바뀌게 될 경우 덩달아 이 부분을 수정해야 한다는 단점도 클 것 같아 좋은 방법이 아니라는 생각이 듭니다.
피케이는 이에 대해 어떻게 생각하시나요?
There was a problem hiding this comment.
꼭 바꿔주진 않아도 괜찮습니다.
그리고 알파카가 말해주신 것처럼 너무 세부적으로 관리하려고 하면 관리포인트가 많아지는 것도 맞구요. 그래도 해야한다면 하긴 하죠!
이건 정답이 있는 주제라기보단, 외부 서비스를 활용한 기능을 어떻게 사용자한테 제공할 것인지에 따라 구현이 바뀌는 부분 같아요.
안녕하세요 피케이, 이번 방탈출 결제 / 배포 미션의 리뷰를 부탁드리게 된 알파카입니다.
평소에 몰래 흠모하던 피케이에게 리뷰를 받을 수 있게 되어서 영광입니다. 잘 부탁드립니다!!
이번 미션에서의 코드 변경사항입니다.
결제와 예약의 분리
지금은 결제가 예약에만 적용되지만, 예약 대기가 자동 승인되기 때문에, 추후 예약 대기와도 결제가 연동되어야 합니다. 따라서 결제와 예약을 최대한 분리하여 별도의 도메인으로 만들었습니다.
웹 개발 경험
스프링, JPA 및 웹 개발은 우테코 미션을 통해 처음 접해봤습니다.
0단계 기본 코드 준비
페어와 상의하여 누구의 기존 단계 코드를 가져올지 결정해야 했는데, 타인의 코드 기반에서 요구사항을 구현하며 확장해 나가는 경험을 하고자 해 제 페어인 로빈의 코드를 사용하게 되었습니다.
서로 이전 미션(미션 1, 2)의 요구사항을 본인의 철학에 맞게 구현했다보니, 여러 군데에서 코드의 차이점이 발견되었습니다. 하지만, 이번 미션에서 배워가야 할 것들에만 집중하기 위해, 제 마음에 들지 않는 코드가 있더라도 그것이 이미 이전 미션을 통해 충분히 학습한 내용이면 수정하지 않았습니다.
테스트용 계정