💳 결제 서비스 #10

AnneMayor · 2022-01-01T17:48:29Z

-- 📝 서비스 목록 --
경매 결제정보 확인
결제하기

-- 💻 사용 기술 --
TossPay API
Redis Cache

f-lab-hubert

수고 많으셨습니다. 좀 더 유연한 코드를 어떻게 작성할 수 있을까를 생각하면서 리뷰를 드려봤어요 ㅎㅎ 제가 무조건 옳은 건 아니니 동의하시는 부분만 수정해보시면 좋을 것 같아요 :)

f-lab-hubert · 2022-01-08T04:05:35Z

build.gradle

@@ -47,7 +48,7 @@ dependencies {
    runtimeOnly 'io.jsonwebtoken:jjwt-impl:0.11.2', 'io.jsonwebtoken:jjwt-jackson:0.11.2'
    compileOnly 'org.projectlombok:lombok'
    developmentOnly 'org.springframework.boot:spring-boot-devtools'
-    implementation 'org.springframework.boot:spring-boot-starter-log4j2:2.6.0'
+    implementation 'org.springframework.boot:spring-boot-starter-log4j2:2.6.2'


logback도 있는데 log4j2를 사용 하신 이유가 있으실까요?? 최근 log4j2 보안 취약점이 있어서 해당 내용도 한번 살펴보시면 좋을 것 같아요 :)

ref:

https://logging.apache.org/log4j/2.x/

https://asec.ahnlab.com/ko/29479/

아 그 부분은 제가 사실 크게 고민하고 쓴건 아니라서 이번에 고민해볼게요 코멘트 감솨합니다 😄

근데 이거 저도 버전 업그레이드 시켜줘서 보안 취약점 대응한다고 했는데 그럼에도 취약하다고 Maven Repository에는 뜨네요... 🙄 아예 바꿔야겠어요 이참에

f-lab-hubert · 2022-01-08T04:37:53Z

src/main/java/com/dhmall/auction/dto/ProductDto.java

+
+@Getter
+@Setter
+@RequiredArgsConstructor


final인 필드가 없어서 원하시는대로 코드가 generate되지 않고 기본생성자로 생성될 것 같아요. dto는 보통 불변객체로 사용하기위해 final 변수를 선언하여 사용합니다. 하지만 framework에서 setter를 사용하는 경우 때문에 어쩔수 없이 setter를 정의 하는 경우가 있는데요. 해당 경우만 제외하고 final 변수로 선언하여 불변객체로 이용하시는게 좋습니다.

immutable object ref:

https://en.proft.me/2013/11/24/advantages-and-disadvantages-immutable-objects-jav/

spring framework setter ref:

https://jojoldu.tistory.com/407

아 넵넵 해당 부분 참고해서 리팩토링 진행하겠습니다!

f-lab-hubert · 2022-01-08T04:40:13Z

src/main/java/com/dhmall/auction/service/AuctionService.java

@@ -65,4 +68,12 @@ public void enterChatRoom(String roomId) {
    public ChannelTopic getTopic(String roomId) {
        return topics.get(roomId);
    }
+
+    public UserDto infoAuctionWinner(BigInteger id) {


method naming에 일관성이 있으면 좋을 것 같아요. 어떤 데이터를 가져올 때는 getXX이고 어떤 데이터를 가져올때는 infoXX인 것 처럼 보여서요. 코드의 일관성도 코드 가독성에 중요한 부분이라서요 ㅎㅎ

아 넵넵 일관성 있는 네이밍 가져가보겠습니다...ㅎㅎㅎ ☺️

f-lab-hubert · 2022-01-08T04:42:28Z

src/main/java/com/dhmall/config/SpringWebEngineConfig.java

+
+    @Bean
+    public RestTemplate restTemplate(RestTemplateBuilder restTemplateBuilder) {
+        return restTemplateBuilder.build();


지금은 restTemplate 생성시 default값으로 설정을 초기화 하지만 실제 운영에 사용될때는 중요 설정값을 잘 살펴보고 설정해주어야합니다. 예를들어 connectionPoolSize, connectionTimeout, readTimeout 같은 값들이 그 중 하나 일 것 같아요 :)

ref: https://e2e2e2.tistory.com/15

아 넵넵 알겠습니다 일단 이 부분은 제가 이슈로 등록해놓고 운영에서 돌릴 적에 수정하는 방향으로 가져가볼게요

f-lab-hubert · 2022-01-08T04:44:19Z

src/main/java/com/dhmall/payment/controller/PaymentController.java

+public class PaymentController {
+
+    private final PaymentService paymentService;
+    @Value("${redirect.url}")


lombok config 파일을 이용하면 final 변수로 선언후에도 사용이 가능합니다. lombok이 해당 애너테이션을 생성자에도 적용해주기 때문인데요. lombok.config 파일을 한번 살펴보시면 좋을 것 같아요.

ref: https://stackoverflow.com/questions/52321988/best-practice-for-value-fields-lombok-and-constructor-injection

아 넵넵 해당 링크 참고해서 다른 부분들도 수정하는 방향으로 가겠습니다!

f-lab-hubert · 2022-01-08T04:44:50Z

src/main/java/com/dhmall/payment/controller/PaymentController.java

+    @Value("${redirect.url}")
+    private String serverUrl;
+
+    @GetMapping("")


empty string을 꼭 설정해주어야하나요~?

이걸 왜 넣어줬을까요...ㅇㅅㅇ제가 봤을 땐 걍 다른 호출 메서드는 다 있는데 저거만 없어서 아쉬운(?) 마음에 추가했던거 같은데...걍 지우는 방향으로 가져가겠습니다;;

@GetMapping("request") 이렇게 가져갈게요!

f-lab-hubert · 2022-01-08T04:46:26Z

src/main/java/com/dhmall/payment/controller/PaymentController.java

+        return new ResponseEntity<>(payResult, httpHeaders, HttpStatus.OK);
+    }
+
+    @GetMapping("fail")


fail시에는 별도의 로직 처리가 필요없는걸까요?? 결제상태를 실패로 변경한다던지 등의 로직이요 아무처리도 안하고 응답만 내려주는 api가 필요한건가 궁금해서요 :)

아 제가 로직 수정하면서 저 fail 처리났을 적에 해줘야 하는 작업이 새로 생기긴 했습니다 해당 부분 수정해서 가져가보도록 할게요!

f-lab-hubert · 2022-01-08T04:49:48Z

src/main/java/com/dhmall/payment/service/PaymentService.java

+
+        HttpEntity<String> request = new HttpEntity<>(objectMapper.writeValueAsString(payloadMap), headers);
+
+        ResponseEntity<JsonNode> responseEntity = restTemplate.postForEntity(


전에 말씀드렸듯이 외부 연동은 별도의 adapter나 클라이언트를 만들어서 처리하는게 좋을 것 같아요. 직접 처리하다보니 PaymentService는 Toss 결제만을 위한 객체가 되어버렸는데요. PaymentService 요구조건에 카카오페이 결제, 카드 결제등의 다양한 수단이 추가되는 요구 사항이 추가되면 코드를 다 뜯어고쳐야할 것 같아서요. TossPaymentClient, KakaoPaymentClient 같은 클래스를 만들어서 외부연동을 하면 좋을 것 같아요. 결제라는 개념을 추상화하여 코드에 녹여내면 더 좋은 코드가 될 것 같아요

아 넵넵 해당 부분 안그래도 작업했는데 여기 부분에다 같이 머지해서 올리겠습니다~

f-lab-hubert · 2022-01-08T04:56:00Z

src/main/java/com/dhmall/payment/service/PaymentService.java

+
+            JsonNode body = responseEntity.getBody();
+
+            TossPayDto payResult = TossPayDto.builder()


하나의 메서드가 너무 많은 책임을 가지고 있는 것 같아요.

header setting

payload를 json serialize

toss 결제 연동

response를 dto로 parsing

해당 메서드를 테스트하려면 위의 내용들이 다 테스트해야하는데 테스트하기가 쉬워보이진 않아서요. 2번은 JsonUtils 같은 객체를 만들어서 json handling의 책임을 옮겨줄수 있을 것 같아요. json handling을 jackson의 ObjectMapper가 아니라 Gson으로도 처리할 수 있는데 json handling이 하나의 구현체와 너무 강결합 되어있는 것 같아요. json도 구현체가 아니라 데이터 포맷의 개념입니다. 구현체는 여러가지를 사용할 수 있어요 jackson에 문제가 발생하면 ObjectMapper를 사용하는 모든 코드를 다 바꿔줘야하는데 가능할까요?? JsonUtils 에서 json을 handling해준다면 JsonUtils만 코드를 수정해 주면 되어서 코드가 좀 더 유연해질 수 있어요. 이와같이 Toss 연동도 PaymentClient를 interface로 만들어두고 결제 개념을 추상화하고 구현체는 여러개를 유연하게 사용할 수 있도록 하면 더 좋을 것 같아요

아 넵넵!안그래도 테스트 코드 작성하는데 리팩토링 해야할 것으로 판단이 되더라구요 참고해서 작업하겠습니다~!

f-lab-hubert · 2022-01-08T05:09:48Z

src/main/java/com/dhmall/config/SecurityConfig.java

@@ -63,9 +63,11 @@ protected void configure(HttpSecurity http) throws Exception {
                .antMatchers("/users/isAlreadyUsed").permitAll()
                .antMatchers("/css/**", "/js/**", "/webjars/**").permitAll()
                // TODO: Spring Security + JWT 인증 로직 추가
+                .antMatchers("/**").permitAll()


이 설정은 전체 api를 permit시켜주게 되는 것 아닌가요??

어머 저거 잘못 올라갔어요;; 👀 이거 안그래도 수정해야합니다;;

… logback)

…lombok.config 파일 추가

…ion-payment

AnneMayor added 2 commits January 1, 2022 23:29

fix: log4j2 버전 업데이트

773d105

feature: 결제 API를 활용한 결제 기능 구현

a7ad360

AnneMayor added the feature Develop new feature label Jan 1, 2022

AnneMayor requested a review from f-lab-hubert January 1, 2022 17:48

AnneMayor self-assigned this Jan 1, 2022

f-lab-hubert reviewed Jan 8, 2022

View reviewed changes

AnneMayor added 7 commits February 6, 2022 10:26

chore: 버전 업그레이드 이후에도존재하는 log4j2 보안 취약점 이슈로 인한 logging 모듈 교체(log4j2 ->…

dd5722d

… logback)

chore: 코드 가독성을 위한 method naming 리팩토링 및 @value 어노테이션 final 생성자 주입을 위한 …

3e03967

…lombok.config 파일 추가

Merge branch 'develop' of github.com:f-lab-edu/sago into feature-auct…

9daedfc

…ion-payment

refactor: 불필요한 @Setter 어노테이션 삭제 및 final 불변 객체 필드 선언

6a3a9ba

chore: 잘못 commit된 코드 삭제(테스트 전용 소스코드 삭제)

5f95e59

Merge branch 'develop' of github.com:f-lab-edu/sago into feature-auct…

bd37cc1

…ion-payment

Merge branch 'develop' of github.com:f-lab-edu/sago into feature-auct…

8bb8286

…ion-payment

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

💳 결제 서비스 #10

💳 결제 서비스 #10

AnneMayor commented Jan 1, 2022 •

edited

f-lab-hubert left a comment

f-lab-hubert Jan 8, 2022

AnneMayor Feb 5, 2022

AnneMayor Feb 5, 2022

f-lab-hubert Jan 8, 2022 •

edited

AnneMayor Feb 5, 2022

f-lab-hubert Jan 8, 2022

AnneMayor Feb 5, 2022

f-lab-hubert Jan 8, 2022

AnneMayor Feb 5, 2022

f-lab-hubert Jan 8, 2022

AnneMayor Feb 5, 2022

f-lab-hubert Jan 8, 2022

AnneMayor Feb 5, 2022

AnneMayor Feb 5, 2022 •

edited

f-lab-hubert Jan 8, 2022

AnneMayor Feb 5, 2022

f-lab-hubert Jan 8, 2022

AnneMayor Feb 6, 2022

f-lab-hubert Jan 8, 2022

AnneMayor Mar 6, 2022

f-lab-hubert Jan 8, 2022

AnneMayor Feb 5, 2022


		HttpEntity<String> request = new HttpEntity<>(objectMapper.writeValueAsString(payloadMap), headers);

		ResponseEntity<JsonNode> responseEntity = restTemplate.postForEntity(


		JsonNode body = responseEntity.getBody();

		TossPayDto payResult = TossPayDto.builder()

💳 결제 서비스 #10

Are you sure you want to change the base?

💳 결제 서비스 #10

Conversation

AnneMayor commented Jan 1, 2022 • edited

f-lab-hubert 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

f-lab-hubert Jan 8, 2022 • edited

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

AnneMayor Feb 5, 2022 • edited

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

AnneMayor commented Jan 1, 2022 •

edited

f-lab-hubert Jan 8, 2022 •

edited

AnneMayor Feb 5, 2022 •

edited