Replies: 7 comments
-
reviewer:
Request 에 요청에 필요한 값들이 어떤 형태로 들어와야하는지 구체적으로 가이드 해줄 수 있습니다. (참고사항) controller 에 적용할 수도 있는데 controller 에 사용하게되면 가독성이 떨어질 수 있어 |
Beta Was this translation helpful? Give feedback.
-
woowacourse/spring-roomescape-payment#84 (comment)
woowacourse/spring-roomescape-payment#39 (comment)
|
Beta Was this translation helpful? Give feedback.
-
🏃♂️🦆🔸
|
Beta Was this translation helpful? Give feedback.
-
리뷰어: 외부 연동 클라이언트와 관련된 부분은 docs를 주석으로 첨부하면 유용하다. woowacourse/spring-roomescape-payment#62 (comment) 리뷰어: 외부 클라이언트 테스트는 수행해본 후 disabled 처리한다. |
Beta Was this translation helpful? Give feedback.
-
리뷰어 : Exception이 어떤 http status를 반환해야할 지 web 계층의 정보를 판단해도 되나요? 예외는 controller layer인가요? 리뷰이 : Service에서 발생시킨 예외가 web 계층의 정보를 판단해 자신이 스스로 반환할 HTTP 상태코드를 정하고 있네요. 확실히 잘못된 부분이라 생각됩니다. 상태코드를 정하는 것은 GlobalExceptionHandler 같은 controller layer에서 처리하도록 해보겠습니다. 링크 : woowacourse/spring-roomescape-payment#72 (comment) 리뷰어 : 하나의 @transactional로 묶여있을 때 paymentClient.requestPaymentApproval이 지연된다면 어떤 문제가 발생할까요? 해결을 바라고 드렸던 코멘트는 아니었고 @Transcational을 외부호출과 함께 두었을때의 문제에 대해서 짚고 넘어가셨으면 해서 남긴 이야기여서 이정도로 넘어가도 좋을 것 같아요. 링크 : woowacourse/spring-roomescape-payment#72 (comment) 상황 : spring data jpa repository를 한번 래핑함 (repository -> repositoryImpl) 리뷰어 : (개인적으로는) jpa를 사용했을 때 현재와 같이 의존성을 빼는 행위가 불필요하다고 생각해요. 확장가능한 구조를 바라보았을 때 이미 jpaRepository를 상속받은 Repository 인터페이스이기 때문에 확장이 불가능한건 아니거든요. 물론 이상적인 구조에서는 jpa에 종속적인 entity와 순수 도메인 클래스를 분리한다라는 이야기도 있는데 저는 이또한 불필요하다고 생각합니다. "이상"과 "현실"은 다르기 때문에 공수가 많이 드는 작업은 굳이하지 않는것이 맞다고 생각해요. 특수한 케이스가 아닌 이상 분리한다고 하더라도 jpa entity와 도메인 클래스의 정보들이 완전히 동일할텐데 그럼 하는 이유가 없는 것 아닐까싶어요. 링크 : woowacourse/spring-roomescape-payment#78 (comment) 리뷰어 : 로그는 굳이 줄바꿈을 하지는 않아요. 콘솔에 출력을 보기 위함이 아니기 때문에 줄바꿈을 통해서 얻는 이득은 거의 없습니다. 수많은 로그들이 줄바꿈이 되어 정렬되어있으면 오히려 보기 힘들어요. 링크 : woowacourse/spring-roomescape-payment#78 (comment) 리뷰어 : openapi란 무엇인가요? 리뷰이 : openapi는 restful api를 정의하기 위한 표준이에요. openapi로 API의 엔드포인트, 메서드, 매개변수, 응답 형식 등을 명확하고 일관되게 기술할 수 있어요. 과거에 swagger로 불렸지만 팀이 smartbear에 합류하면서 v3부터 openapi로 이름을 변경했다는 히스토리가 있어요. 링크 : woowacourse/spring-roomescape-payment#120 (comment) 상황 : Member의 equals hashcode @Override
public boolean equals(Object o) {
if (this == o) return true;
if (o == null || getClass() != o.getClass()) return false;
Member member = (Member) o;
if (id == null || member.id == null) {
return Objects.equals(email, member.email);
}
return Objects.equals(id, member.id);
}
@Override
public int hashCode() {
if (id == null) {
return Objects.hash(email);
}
return Objects.hash(id);
} 리뷰어 :
equals의 목적은 무엇이라고 생각하시나요? 두 객체의 동등성을 비교하는 것이 그 목적이라면 unique한 email로만 equals를 사용해도 도메인 코드를 어플리케이션에서 사용할 때 문제가 없는 것 아닌가요?
이미 영속성 컨텍스트에 들어간 상태라는 것을 생각한다면 jpa에 종속된 현재 어플리케이션에서는 equals 동등성이 아니더라도 jpa persistence context에 들어갔다 나온 이후이기 때문에 동일성비교가 가능합니다. id로만 비교하는 것이 목적이라면 오히려 equals 동등성을 재정의하지 않아도 jpa가 알아서 동일성을 보장해주는 것 아닐까요?
객체의 상태를 보장하는 일반적인 Object메소드들은 로직을 태우게 된다면 다른 사람들이 봤을때 이해의 영역으로 넘어가기 때문에 가능하면 인텔리제이에서 자동으로 생성해주는 것 이상의 추가 분기 로직은 태우지 않는 편이에요. 링크 : woowacourse/spring-roomescape-payment#134 (comment) 리뷰어 : 팀프로젝트로 넘어가시다보면 지금과 같이 특정 요청 혹은 로직에 대해서 시작과 끝을 로깅하고 둘을 함께 트래킹해야할 때가 생길 수 있어요. 만들어주신 형태로 메소드단에서는 scope이 다른 곳으로 넘어가지 않기 때문에 이정도로 충분할 수 있는데 하나의 api요청에 대해서 모든 요청을 비슷하게 트래킹해야하는 상황이 발생할 수 있는거죠. 보통 이에 해결하는 방법으로는 MDC(Mapped Diagnostic Context)을 사용하기도 해요. 키워드만 기억해두시고 나중에 비슷한 상황이 발생했을 때 한번 고민해보시면 좋을 것 같아요. |
Beta Was this translation helpful? Give feedback.
-
DB 트랜잭션과 외부 통신은 분리하여 수행하는 것이 좋다 Reviewer : DB transaction 내에서 외부 통신은 지양하시는 것이 좋은데요, Reviewee :
추가로, 지금은 결제 로직을 예약 서비스에서 분리해서 결제 부분은 트랜잭션에서 제외되도록 했습니다. 그런데 상황에 따라 Reviewer : 찾아주신 이유가 맞습니다. 👍 woowacourse/spring-roomescape-payment#31 (comment) write-ahead Reviewee : payment의 경우 승인된 결제에 대해서만 저장하고 있어 실패 데이터를 모두 저장해야할지 의문이 생겼습니다. 그래서 우선 실패 결과를 DB에 저장은 하지 않고 예외 처리를 하고 로그를 남기고 있는데요, 혹시 의도하지 않은 결과가 왔을 때 일반적으로 어떤 처리를 하시는지 궁금합니다. Reviewer : 보통의 경우 write-ahead 테이블을 이용한 정보를 먼저 남깁니다. 보통 xxxRequest 라는 이름의 테이블을 많이 사용한 것 같네요. 가령 PaymentRequest 테이블을 만들어 pay 요청을 결제사에 전송하기 전에 먼저 생성하고 이후 상태를 변경하도록 전략을 취합니다. 위 테이블에 요청상태인지, 성공인지, 실패인지를 남기고 각 사유를 남겨두어 pay의 상태를 추적합니다. pay호출 전에 남기는 이유는 실제 호출까지 이루어졌는지 확인을 가장 확실하게 할 수 있는 방법이 db에 기록하는 것이기 때문입니다. 보통의 경우 결제를 한 이후 대사(reconciliation, 데이터 정합성을 맞추는 등의 행위)를 하는데 이를 위해서도 중요하게 사용되는 정보이기도 합니다. 또한 고객의 클레임이 들어왔을때 로그를 확인하는 것보다 admin등에 거래 요청을 확인할 수 있는 기능을 추가하여 대응할 수 있는 장점도 생기곤 합니다. woowacourse/spring-roomescape-payment#99 (comment) Reviewer : 보통 api를 제공하는 측에서 공유한 readtimeout보다는, 정확히 요청에 대한 처리가 완료되는 경우를 감안해서 1초의 여유를 두시는 것이 안정적입니다. api를 제공하는 측에서 10초의 readtimeout 시간을 가지고 있다면 11초로 fit하게 설정하는 것이 좋습니다. woowacourse/spring-roomescape-payment#31 (comment) Reviewer : 소소한 팁인데요. property로 이러한 시간과 관련된 값은 int로 주입받지 않고 처음부터 Duration으로 주입받을 수 있습니다. properties에 int가 아닌 Duration으로 변수형을 선언하시고 application.yml에 시간 단위를 적어주면 가능합니다. ex. |
Beta Was this translation helpful? Give feedback.
-
리뷰어 : 22억 이상을 결제하면 어떻게 되나요? (결제 금액을 나타내는 필드의 타입이 Integer 임) 리뷰이 : 리뷰어 : 만약 bad request로 22억이 들어온다면, 오버플로우 에러를 내는게 맞을지? validation error를 내는게 맞을지? 로 생각해볼수도 있겠네요. ref : woowacourse/spring-roomescape-payment#48 (comment) 리뷰어 : 만약 토스페이먼츠가 에러메시지를 변경했다면 방탈출 시스템도 같이 영향을 받겠네요. 토스페이먼츠의 변경이 방탈출 시스템에 영향을 주는건, 객체로 생각해보면 어떤가요? 리뷰이 : 특정 객체의 변경이 외부에 영향을 주는 것을 경계하라는 말씀으로 받아들였습니다. 객체지향에서의 OCP 가 떠오르는데, 잘 접근을 한 것인지 모르겠네요 ㅎㅎ 그런데, 여기서 어떤 메시지를 클라이언트에게 내려줄까? 에 대한 고민이 생겼습니다.
리뷰어 : 만약 저라면, 현재 프로젝트 수준에서는 "잔액부족 / 카드정보가 잘못되었어요" 빼고는 다 "결제실패했어요" 정도로 처리할것 같아요 ref : woowacourse/spring-roomescape-payment#22 (comment) (외부 API를 사용할 때 에러 핸들링 관련해서 중요한 고민 점이라 생각해서 추가해봤어요) 리뷰어 : 타임아웃을 잘 설정해 주셨네요. 왜 5 / 30 이어야 하는지 이유를 생각해볼까요? 리뷰이 : "connect / read 에서 이루어 지는 작업의 부하 정도가 다르기 때문" 일 것 같습니다. 반면 read 라는 것은 우리가 원하는 결과값을 받아오는 과정이고, 연결에 비해 더 무거운 작업에 해당할 것 같습니다. 키워드 : 타임아웃, tcp 3 way handshake ref : woowacourse/spring-roomescape-payment#22 (comment) 리뷰어 : 해당 테스트는 기본 ObjectMapper를 스프링 ObjectMapper라고 생각하고 사용하고 있는것 같은데, 그렇다면 스프링 ObjectMapper를 사용하든 스프링이 쓰는것과 똑같은 설정을 사용하자.
(출처 : https://docs.tosspayments.com/reference/using-api/req-res#유의사항 ref : woowacourse/spring-roomescape-payment#12 (comment) 리뷰어 : 사실, 현업에서도 이것 때문에 많은 문제가 발생하긴 해요. 다른 회사의 api를 사용하기도 하지만, 요즘 대부분의 회사들은 MSA 구조로 되어 있는 경우가 많아요. 멤버를 조회하려면 멤버 팀이 제공하는 api를 사용해야 한다든지... 외부 api는 네트워크를 이용해야 합니다. 네트워크와 local 코드 수행의 시간차이가 얼마나 나는지 아시나요?? 트랜잭션이, 매~~~~~~우 느린 task와 묶인다면 어떤 문제가 있을까요?? 알아볼 것 : 리뷰어 : 결제 도메인에서 String paymentKey를 Id로 가질 경우, 이런 문제가 발생할 수 있어요. INDEX 테코톡 15분이라는 시간에 비해 배우는게 많아서 보는게 좋긴한데.. 리뷰이 : 위처럼 구성하는 경우, 새로운 paymentKey가 추가되었을 때 많은 엔티티들이 이동하게 되어 추가 비용이 커질 수 있겠네요. ref : woowacourse/spring-roomescape-payment#2 (comment) 리뷰어 : @Component
public class RoomeescapeExceptionOperationCustomizer implements OperationCustomizer {...} spring docs 의 open api spec generator에서 필요한 부분을 잘 찾아 원하는 기능을 구현한것 같아요. Spring Doc 에도 개발자가 문서화를 위해 생성되는 여러 객체를 생성하는 방법을 커스텀할 수 있다. -> OperationCustomizer |
Beta Was this translation helpful? Give feedback.
-
.
Beta Was this translation helpful? Give feedback.
All reactions