[2 - 4단계 방탈출 결제 / 배포] 이든(최승준) 미션 제출합니다. #154
Conversation
- PaymentFixture 구현 - Reservation 생성 관련 테스트에 Payment 객체 생성 로직도 추가
| @@ -19,6 +19,8 @@ dependencies { | |||
| implementation 'org.springframework.boot:spring-boot-starter-data-jpa' | |||
| implementation 'org.springframework.boot:spring-boot-starter-validation' | |||
|
|
|||
| implementation 'org.springdoc:springdoc-openapi-starter-webmvc-ui:2.5.0' | |||
There was a problem hiding this comment.
질문) springdoc과 spring rest docs 중에 기술을 선택한 근거가 궁금해요.
There was a problem hiding this comment.
RestDocs는 사용해본 경험이 없기 때문에 학습에 대한 시간적 리소스가 필요한 반면,
Swagger는 사용해본 경험이 있어서 빠르게 적용할 수 있다는 생각에 선택하였습니다.
물론 미션이기 때문에 안 써본 기술을 학습해서 적용해보는 것도 중요하지만 문서화 기술은 조금 다른 범주라고 생각되었습니다.
문서화 도구는 미션 이후 따로 공부하거나, 레벨3 프로젝트 때 학습하여 적용해보는 등 아무때나 충분히 학습 및 사용가능한 하나의 기술이라고 생각합니다.
반면 결제 관련 부분에 대한 저의 고민과 설계를 현업자인 호돌에게 리뷰받을 수 있는 기회는 이번 리뷰활동이 마지막이라고 생각합니다.
때문에 결제 관련 설계에 더 시간적 리소스를 투자하고자 하였고,
이 과정에서 빠르게 적용하여 미션 요구사항을 충족시킬 수 있는 Swagger를 선택하게 되었습니다.
물론 Swagger와 RestDocs는 각 기술의 장단점이 있으며 그것또한 비교하며 선택해야 하지만,
이번엔 그 부분을 충분히 학습하고 고려해보지 못한 점이 아쉬움으로 남긴합니다.
RestDocs 에 대해서는 따로 학습하여 적용해보고 Swagger와의 장단점을 비교해보겠습니다!
| @@ -39,20 +43,23 @@ public ResponseEntity<Void> login(@RequestBody @Valid LoginRequest request, Http | |||
|
|
|||
| @RoleAllowed | |||
| @GetMapping("/login/check") | |||
| @Operation(summary = "[회원] 로그인 검증", description = "JWT 토큰을 통해 로그인 여부를 검사한다.") | |||
There was a problem hiding this comment.
의견) 문서에서 로그인이 필요한 엔드포인트에서 LoginMember이 인자로 노출되고 있는데요, springdocs의 auth 기능을 통해 사용할 수 있게 하면 좋을 것 같아요.
There was a problem hiding this comment.
그 부분에 대한 처리를 놓쳤네요..!
중요한 부분 짚어주셔서 감사합니다:)
처리해보겠습니다!
| public ResponseEntity<Void> deleteReservation( | ||
| @PathVariable @NotNull(message = "reservationId 값이 null일 수 없습니다.") Long reservationId, | ||
| @LoginMember Member member) { | ||
| reservationWaitingService.deleteReservationWaiting(reservationId, member); | ||
| return ResponseEntity.noContent().build(); | ||
| } | ||
|
|
||
| // TODO: deleteReservation 과 하나로 통합 |
There was a problem hiding this comment.
수정 전에 리뷰 요청시간이 다 되어서 미처 제거하지 못하고 요청을 드렸네요 🥲
제거하였습니다:)
| public Payment() { | ||
| } |
There was a problem hiding this comment.
맞습니다! protected여도 괜찮을 것 같네요!
감사합니다:)
| public void cancel(ZonedDateTime requestedAt, ZonedDateTime approvedAt, PaymentStatus status) { | ||
| this.requestedAt = requestedAt; | ||
| this.approvedAt = approvedAt; | ||
| this.status = status; | ||
| } |
There was a problem hiding this comment.
의견) cancel이라는 메소드명을 봤을 때 PaymentStatus를 인자로 받을 필요가 있을까요?
There was a problem hiding this comment.
주문 취소 시
토스에서 Response해주는 paymentStatus를 사용하고 싶어서 이렇게 작성했습니다만, 굉장히 어색해보이는 코드이긴 하네요..
차라리 changeStatus 라는 메서드로 사용하는 것이 나을까요??
조금 더 고민하고 괜찮은 메서드로 변경해보겠습니다!
| private String thumbnail; | ||
| private int price; |
There was a problem hiding this comment.
책임분리와 코드의 일관성 관점에서 좋은 의견인 것 같습니다:)
반영하겠습니다~!
| public void setBaseUrl(String baseUrl) { | ||
| this.baseUrl = baseUrl; | ||
| } | ||
|
|
||
| public void setConfirmEndpoint(String confirmEndpoint) { | ||
| this.confirmEndpoint = confirmEndpoint; | ||
| } | ||
|
|
||
| public void setCancelEndpoint(String cancelEndpoint) { | ||
| this.cancelEndpoint = cancelEndpoint; |
There was a problem hiding this comment.
의견) setter 없이 application properties를 만드는 방법이 있을거예요. record를 활용해봐도 좋을 것 같네요.
There was a problem hiding this comment.
setter 없이 properties 객체를 만드는 방법이 있는 지는 몰랐네요..!
한 번 찾아보겠습니다! 좋은 의견 감사합니다!
| public void deleteReservationPayment(Reservation reservation) { | ||
| Payment payment = paymentRepository.findByReservation(reservation) | ||
| .orElseThrow(NotFoundPaymentException::new); | ||
|
|
||
| paymentRepository.delete(payment); |
There was a problem hiding this comment.
질문) deleteReservationPayment에도 Transcational이 필요하지 않나요?
There was a problem hiding this comment.
클래스에 Transactional 애노테이션을 붙여둔 다른 Service 들과 다르게
PaymentService는 실수로 Transactional(readOnly=true) 붙여놓아서 제가 헷갈리고 빠뜨린 것 같습니다..
일관성을 고려해서 PaymentService도 클래스에 Transactional 애노테이션을 붙여 문제를 해결했습니다.
감사합니다!
| public static ReservationMineResponse ofReservationPayment(Reservation reservation, Optional<Payment> reservationPayment) { | ||
| if (reservationPayment.isPresent()) { | ||
| String message = BOOKED_MESSAGE; | ||
| if (reservation.isCancelStatus()) { | ||
| message = CANCELED_MESSAGE; | ||
| } | ||
| Payment payment = reservationPayment.get(); | ||
| return new ReservationMineResponse(reservation.getId(), | ||
| reservation.getTheme().getName().getName(), | ||
| reservation.getDate(), | ||
| reservation.getTime().getStartAt(), | ||
| message, | ||
| payment.getPaymentKey(), | ||
| payment.getAmount() | ||
| ); | ||
| } else { | ||
| return new ReservationMineResponse(reservation.getId(), |
src/main/resources/application.yml
Outdated
| jpa: | ||
| defer-datasource-initialization: true | ||
| properties: | ||
| hibernate: | ||
| show_sql: true | ||
| format_sql: true | ||
| hibernate: | ||
| ddl-auto: update |
There was a problem hiding this comment.
질문) 운영환경에서는 sql 로깅등이 다소 부담될 수 있어서, 환경별로 설정을 다르게 해주면 좋을 것 같아요.
There was a problem hiding this comment.
운영환경에 대해서는 고려해보지 못했네요..!
Profile에 따라 다르게 처리하도록 분리해보겠습니다.
운영환경을 생각했을 때, 이 부분 말고 더 고려할 점이 어떤 것들이 있을까요??
There was a problem hiding this comment.
운영환경과 개발환경에서 다르게 가져갈만한 것은 logging level이라던지, ddl-auto 관련 값들(현업에서는 create, update같은 것들을 쓰기에는 운영환경은 너무 위험하긴하죠 ㅋㅋ) API 문서 노출여부 등 다양하게 있을 것 같네요!
There was a problem hiding this comment.
local, prod로 application 환경을 분리했습니다!
그리고 prod에서는 DB에 변경을 발생시키지 않는 ddl-auto: validate로 처리했고 sql 로깅도 꺼두었습니다:)
확실히 운영에 jpa를 사용할 것인데 sql을 전부 로깅하게 되면 자동으로 생성되는 쿼리문들을 로깅하는 비용이 굉장히 부담되긴 할 것 같아요..
심지어 지금 배포환경은 aws에서도 굉장히 저렴한 편인 t4g.micro 라서 더 처리해야할 부분이기도 하네요.
좋은 의견 감사합니다 호돌!
| public class ThemePrice { | ||
| private int price; | ||
|
|
||
| protected ThemePrice() { | ||
| } | ||
|
|
||
| public ThemePrice(int price) { | ||
| this.price = price; | ||
| } | ||
|
|
||
| public int getPrice() { | ||
| return price; | ||
| } | ||
| } |
There was a problem hiding this comment.
의견) record로 써도 괜찮겠네요! 가격이다 보니 초기화 시에 가격정보에 대한 규칙(음수가 될 수 없다던지) 도 스며들어있으면 좋을 것 같아요.
There was a problem hiding this comment.
가격은 '음수가 될 수 없다'에 대한 검증 추가 너무 좋은 의견이신 것 같습니다! 처리해보겠습니다:)
그리고 VO를 record로 구현하는 것에 대한 의견도 정말 동의합니다.
record를 사용하면 equals&hashCode, toString 처리 뿐만 아니라 getter, final 처리 등등 다양한 보일러 플레이트 코드를 제거해주기 때문에 이점이 있다고 생각합니다!
저 또한 이런 이점이 있다고 생각하기 때문에 전 미션에서 VO를 record로 사용하였습니다!
하지만 현재 페어의 코드는 모든 VO가 record가 아닌 class로 사용되고 있어서
이 부분만 record로 처리하게 된다면 코드의 일관성이 깨지기 때문에 이전 코드와 마찬가지로 class로 작성했습니다.
하지만 저 또한 공감하고 고려하고 있었던 부분었기 때문에 호돌이 리뷰 주신 김에 한번 처리해보겠습니다:)
| public static ReservationMineResponse fromReservationWaitingInfo(ReservationWaitingWithRank waitingWithRank) { | ||
| return new ReservationMineResponse( | ||
| waitingWithRank.getWaiting().getReservation().getId(), | ||
| waitingWithRank.getWaiting().getReservation().getTheme().getName().getName(), | ||
| waitingWithRank.getWaiting().getReservation().getDate(), |
There was a problem hiding this comment.
의견) ""와 같이 의미가 있는 문자열은 상수화해주면 더 좋을것 같아요.
There was a problem hiding this comment.
저 또한 문자열 자체로 의미를 이해하기 어려운 부분은 이름을 붙여서 사용하는 것이 좋다고 생각합니다!
그런데 이 부분은 신경쓰지 못해 적용하지 못했네요 🥲
private static final String EMPTY_PAYMENT_KEY = ""; 로 처리해보았습니다:)
좋은 의견 감사합니다 호돌!
| @@ -73,6 +47,44 @@ public ReservationMineResponse(ReservationWaitingWithRank waitingWithRank) { | |||
| ); | |||
| } | |||
|
|
|||
| public static ReservationMineResponse ofReservationPayment(Reservation reservation, Optional<Payment> reservationPayment) { | |||
There was a problem hiding this comment.
질문) Payment가 empty인 경우는 어떤 경우인가요? 예외를 던져야하는 상황은 아닌가요?
There was a problem hiding this comment.
내 예약 조회 기능을 담당하는 ReservationService의 findMyReservation(Member member) 메서드에서는
내 모든 예약(reservation)정보를 조회하고 관련 결제(payment) 정보를 조회하는데요!
예약 정보들 중에, '결제 대기' 상태인 예약은 결제 정보가 존재하지 않습니다.
때문에 Optional로 받아서 payment정보가 isEmpty()이면
클라이언트에게 해당 예약의 상태를 "결제 대기" 로 전달하는 로직을 작성하기 위해 Optional을 사용하였습니다.
이 외에도 Optional가 isEmpty()이면 결제금액을 0으로 표시하거나, paymentKey를 공백("")으로 클라이언트에게 전달하는 등의 역할도 합니다.
혹시 호돌이 보시기에 처리 방법이 좋아 보이지 않으신다면 어떤 방향으로 개선했으면 좋았을까요??
There was a problem hiding this comment.
Why should Java 8's Optional not be used in arguments 을 읽어보면 좋겠네요.
의견) 차라리 서비스에서 Payment의 nullable 유무를 판단하고 ReservationMineResponse의 각각 다른 생성자를 사용하면 어떨까요?
There was a problem hiding this comment.
공유해주신 좋은 글 잘 읽었습니다!
생성자의 매개변수로 Optional을 사용하는 것은 그럴싸해 보이지만
생성자 호출부에서 Optional을 사용하면 로직이 지저분해질 뿐만 아니라
- 매개변수를 다룰 때, null, EmptyOptional, hasValueOptional 3개를 처리해야함
- 일반 객체에 비해 다루는 비용이 비쌈
등의 문제가 있다는 점을 알게 되었습니다.
때문에 차라리 생성자를 분리해야 사용해야할 것 같다는 생각이 들었네요:)
공유 감사합니다 호돌! 호돌의 말씀대로 차라리 오버로드된 각각의 생성자를 두어 관리하는 방식으로 리팩토링 해보겠습니다.
| if (reservationPayment.isPresent()) { | ||
| message = BOOKED_MESSAGE; | ||
| if (reservation.isCancelStatus()) { | ||
| message = CANCELED_MESSAGE; | ||
| } | ||
| } |
There was a problem hiding this comment.
중간중간 인덴트를 신경써주지 못한 로직이 있네요🥲
여기 말고도 전체 코드를 흝어보고 처리하겠습니다..
|
안녕하세요 호돌!
💭 변동사항우테코에서 제공하는 AWS EC2 사용불가호돌이 api 문서가 접속되지 않는다고 말씀해주셔서 확인해보려했는데 🤔 질문운영환경에서 테이블 스키마 sql 관리를 어떻게 할 수 있을까요?운영환경에서는 jpa를 믿기보단 sql을 직접 작성해서 관리할 수 있도록 하기 위해 
문제는 애플리케이션이 실행될 때마다 db가 초기화되지 않도록 하기 위해 |
There was a problem hiding this comment.
안녕하세요 이든 :)
이제 거의 다 왔네요.
몇 가지 코멘트 남겼으니 확인 부탁드릴게요!
혹시 호돌은 이 부분에 대해서 아시나요 🥲
스프링 문서에 보시면
In a JPA-based app, you can choose to let Hibernate create the schema or use schema.sql, but you cannot do both. Make sure to disable spring.jpa.hibernate.ddl-auto if you use schema.sql.
라고 되어있으니, schema.sql를 사용할 때는 hibernate ddl auto를 none으로 해줘야 정상적으로 작동할 것 같아요. 불필요한 설정은 다시 이전으로 되돌려도 될 것 같습니다!
호돌만 괜찮으시다면 로컬에서 확인 부탁드려도 괜찮을까요..??
확인했습니다!
운영환경에서 테이블 스키마 sql 관리를 어떻게 할 수 있을까요?
배포 전에 직접 데이터베이스에 SQL 쿼리를 날리고, hibernate ddl-auto를 validate해서 스키마가 맞지 않아 어플리케이션이 정상적으로 실행되지 않으면 그 때 수정하는 SQL 쿼리를 날리기도 하구요, 지금처럼 ddl 버전 관리를 하기 위해서는 flyway나 liquibase 같은 DB 형상 관리 툴을 사용하기도 합니다!
| @@ -44,7 +45,7 @@ public ResponseEntity<Void> login(@RequestBody @Valid LoginRequest request, Http | |||
| @RoleAllowed | |||
| @GetMapping("/login/check") | |||
| @Operation(summary = "[회원] 로그인 검증", description = "JWT 토큰을 통해 로그인 여부를 검사한다.") | |||
| public ResponseEntity<LoginCheckResponse> loginCheck(@LoginMember Member member) { | |||
| public ResponseEntity<LoginCheckResponse> loginCheck(@Parameter(hidden = true) @LoginMember Member member) { | |||
There was a problem hiding this comment.
사소한 의견) 모든 LoginMember에 해당 어노테이션을 붙여주는 것이 번거로울 수 있어서, OpenAPI 빈 생성 시에 SpringDocUtils의 기능을 사용하면 전역에서 LoginMember 타입을 문서에서 ignore할 수 있어요.
There was a problem hiding this comment.
우왓 이런 기능이 있었네요..!
바로 처리해보았습니다! SwaggerDocUtils의 Config에 무시할 애노테이션을 설정하는 기능이 있네요..!
SpringDocUtils.getConfig().addAnnotationsToIgnore(LoginMember.class); 이렇게 처리해보았는데 의도하신 바와 비슷할까요?? :)
| this.address = address; | ||
| validate(address); |
There was a problem hiding this comment.
의견) record의 Custom Constructor을 사용해도 괜찮겠네요.
public MemberEmail {
validate(address);
}There was a problem hiding this comment.
Custom Constructor 너무 좋네요:)
좋은 의견 감사합니다!
간단한 내용인데 record에 대해서 조금 더 찾아보고 사용할 걸 그랬나하는 반성도 되네요
| @@ -73,6 +47,44 @@ public ReservationMineResponse(ReservationWaitingWithRank waitingWithRank) { | |||
| ); | |||
| } | |||
|
|
|||
| public static ReservationMineResponse ofReservationPayment(Reservation reservation, Optional<Payment> reservationPayment) { | |||
There was a problem hiding this comment.
Why should Java 8's Optional not be used in arguments 을 읽어보면 좋겠네요.
의견) 차라리 서비스에서 Payment의 nullable 유무를 판단하고 ReservationMineResponse의 각각 다른 생성자를 사용하면 어떨까요?
| show_sql: false | ||
| format_sql: false | ||
| naming: | ||
| physical-strategy: org.hibernate.boot.model.naming.PhysicalNamingStrategyStandardImpl |
There was a problem hiding this comment.
질문) org.hibernate.boot.model.naming.PhysicalNamingStrategyStandardImpl를 사용하는 이유는 무엇인가요?
There was a problem hiding this comment.
이건 제 실수입니다..
처음에는 위에서 말씀드린 Schema-validation: missing table [name] 에러가 발생했을 때,
테이블명 컬럼명이 매칭되지 않아서 발생한 문제라고 생각되어서
그 해결과정에서 네이밍 규칙을 추가해보게 된 것인데요.
문제 해결 이후에 지웠어야했는데 지우지 못한 제 실수입니다..ㅎㅎ
…리해주는 방식에서 SpringDocUtils 일괄처리 방식으로 변경
생성자의 매개변수로 Optional을 사용하는 것은 그럴싸해 보이지만, 생성자 호출부에서 Optional을 사용하면 로직이 지저분해질 뿐만 아니라 매개변수를 다룰 때, null, EmptyOptional, hasValueOptional 3개를 처리해야함 일반 객체에 비해 다루는 비용이 비쌈 등의 문제가 있다. https://stackoverflow.com/questions/31922866/why-should-java-8s-optional-not-be-used-in-arguments
안녕하세요 호돌! |
There was a problem hiding this comment.
안녕하세요 이든 :)
이번 단계는 여기서 마무리해도 될 것 같네요.
레벨 2 마지막 미션까지 정말 고생 많으셨습니다.
우테코에서 남은 시간동안도 열심히 성장하시길 바라요.
수고하셨습니다!
호돌의 의견이 궁금합니다!
JPA의 validate를 쓰는 것이 좋죠. 그런데 문서에 나와있듯 아마 둘 중 하나를 비활성화를 하지 않으면 어플리케이션이 정상적으로 작동하지 않을거예요. 그렇다면 이렇게 해보는 것은 어떨까요?
개발 환경: sql init mode를 h2에서만 사용하도록 embedded나 always로 두기, hibernate ddl auto는 none으로 두기
운영 환경: sql은 연동된 db에서 직접 schema.sql에 있는 내용을 수동으로 실행하고, sql init mode는 never, hibernate ddl auto는 validate로 두기
| datasource: | ||
| url: jdbc:h2:mem:database | ||
| jpa: | ||
| hibernate.hbm2ddl.auto: create-drop |
There was a problem hiding this comment.
질문) 이 프로퍼티는 정상 작동하는건가요? 제가 알기로는 jpa.hibernate.ddl-auto를 사용하는 것이 맞다고 알고 있어서요.
안녕하세요 호돌!
이번 미션도 열심히 구현해보았습니다:)
리뷰 잘부탁드리겠습니다!
주요 구현 사항
✅ 결제(Payment) 엔티티 추가 및 기능 세분화
요구사항에 따라 결제 엔티티가 추가되었습니다.
그리고 결제 엔티티가 추가됨에 따라 예약관련 기능이 세분화 되었습니다.
[기능]
예약 취소: "예약을 취소하고 결제 금액을 환불한다. 이때 취소된 예약의 상태는 CANCELED로 변경되며, 예약 대기가 있다면 결제대기(PAYMENT_WAITING) 상태의 예약으로 전환된다."예약 삭제: "취소(CANCELED) 또는 결제 대기 상태(PAYMENT_WAITING) 의 예약 정보를 DB에서 삭제한다."예약 결제: "결제 대기 상태(PAYMENT_WAITING) 의 예약에 대해 결제를 수행한다."예약 추가: "결제 및 예약을 수행한다."관리자 예약 추가: "결제 없이 예약을 수행한다."[상태]
BOOKED: "예약 완료 상태"WAITING_PAYMENT: "앞선 예약이 취소되면1순위의 예약 대기가WAITING_PAYMENT상태의예약으로 등록됩니다."CANCELED: "예약 취소 상태"✅ 예약 정보 Soft Delete
예약 정보는 Soft Delete로 삭제하도록 구현하였습니다.
기본적으로 예약 정보 삭제는
예약 취소기능을 통해 수행하며예약 취소는
예약의 ReservationStatus를 CANCELED 상태로 만들고 결제 금액을 환불합니다.예약 정보 삭제는 ReservationStatus가
CANCELED또는WAITING_PAYMENT상태일 때내 예약정보 페이지에서 가능합니다.
ERD
✅ API 문서화
Swagger를 사용하여 API를 문서화하였습니다.
리뷰요청을 드리는 시점에 Request,Response 데이터에 대한 설명은 시간부족으로 추가하지 못했는데요!
리뷰 요청 이후 변경하면 호돌의 리뷰를 방해하게 될 것 같아서, 1차 리뷰 이후에 수정하겠습니다..!
주소: http://54.180.202.197:8080/swagger-ui.html
배포 서버 주소
http://54.180.202.197:8080/
테스트 계정
관리자
email : [email protected]
password : 1234567890
일반 사용자
email : [email protected]
password : 1234567890