[방탈출 결제 / 배포 2 - 4단계] 조이썬(이영수) 미션 제출합니다. by youngsu5582 · Pull Request #143 · woowacourse/spring-roomescape-payment

youngsu5582 · 2024-06-10T07:32:56Z

안녕하세요! 범블비!! 되게 오랜만에 보는거 같네요,,

미션 및 구현은 그렇게 어렵지 않았으나 나름대로의 삽질(?) 을 한다고 오래 걸렸습니다.

문서중 특별한 내용으로는
에러 반환이 string 으로만 나타나거나 직접 """ ... ''' 와 같은 형태로 작성해주는게 이상하다고 생각해
커스텀 어노테이션 을 만들고 이를 기반으로 문서에 추가작성해주는 Optimizer 를 만들었습니다.
참고 링크

@ApiResponse(description = "삭제를 성공했습니다.", responseCode = "204")
@ApiErrorResponses(value = ErrorType.RESERVATION_NOT_DELETED, groups = ErrorTypeGroup.ADMIN)
void delete(@Min(1) long reservationId);

데이터는 기존과 같으며
application-prod 로 분리를 했으나, 확인할때 편의 때문에 dataLoader 는 true로 했습니다.
( 그렇기에 대기 취소는 가능하나 결제 취소는 불가능 - 실제 결제 정보를 넣은게 아니므로 )

ID: admin@roomescape.com
PWD: admin

사용자 계정1
ID: dev.chocochip@gmail.com
PWD: 1234

사용자 계정2
ID: dev.eden@gmail.com
PWD: 567

ERD는 기존 Entity 에서 머메이드 형식으로 변환 했습니다.

classDiagram
direction BT
class Member {
    Long  id
    String  password
    String email
    String name
    Role  role
}
class MemberReservation {
    Long  id
    ReservationStatus  reservationStatus
}
class Payment {
    Long  id
    String  paymentKey
    PaymentType  paymentType
    BigDecimal  amount
}
class Reservation {
    Long  id
    LocalDate  date
}
class ReservationTime {
    Long  id
    LocalTime  startAt
}
class Theme {
    Long  id
    String  description
    String  name
    String  thumbnail
    BigDecimal amount
}

MemberReservation "0..*" --> "1" Member 
MemberReservation "0..*" --> "1" Reservation 
Payment "0..*" --> "0..1" MemberReservation 
Reservation "0..*" --> "1" ReservationTime 
Reservation "0..*" --> "1" Theme

벌써 레벨2도 마지막 이네요...
해주신 피드백 들 최대한 반영하고 방학을 맞이하려 노력하겠습니다!!
( 저번에 Component - Configuration,Bean 아티클을 되게 흥미로웠습니다! 감사합니다! ☺️ )
p.s 궁금한 부분은 코드에 추가로 달겠습니다!

youngsu5582

코드적인 부분에서 궁금함은 여기까지 입니다!
마지막 리뷰 까지 잘 부탁드립니다! 🫡🫡

youngsu5582 · 2024-06-10T07:45:20Z

src/main/java/roomescape/member/domain/specification/AdminControllerSpecification.java

+
+import java.util.List;
+
+public interface AdminControllerSpecification {


저는 문서화 방법을 springdoc 을 통한 swagger 로 했습니다!

RestDocs 를 통해 하는게

Controller 단위 테스트를 통해 변경

그 상황에 맞는 케이스 전부 작성 해야만 한다

의 느낌이 들어서 거부감이 들어서였는데요!

반대편의 입장은 SpringDocs 가
실제 코드에 어노테이션 을 붙이는게 마음에 안든다고 생각 하더라구요.

제 의견으로는 어노테이션은 메타데이터이므로 몇개 정도는 추가되어도 상관없지 않나..? + 어노테이션을 통해 테스트 코드를 작성하는 시간을 줄일수 있다. ( 추가로, 컨트롤러 단위 테스트 자체가 불필요하다 생각합니다 )

굳이 어노테이션 추가를 경계해야할까요...?

개인적으로는 어노테이션 기반의 프로그래밍을 별로 안 좋아합니다.
말씀하신대로 단순히 메타데이터 역할만을 해야하는데, 그 사용방법을 잘 알고 유의하며 사용해야하는 어노테이션도 있기도 하죠.
또, 그런 유의사항을 지키지 않고 사용하거나 버그가 있을 때 어디서 버그가 났는지 추적하기도 굉장히 까다롭습니다.
사용하기도 편하고, 구현하기도 편한 건 맞긴 하지만 장기적으로 봤을 때 유지보수가 용이한가는 잘 모르겠네요.

저도 컨트롤러 단위 테스트는 필요없다고 생각하지만, docs를 위해서 만드는거라면 그 자체만으로도 충분히 이유가 될 수 있다고 생각해요.
처음 설정하는 게 조금 까다롭긴 하지만 일단 설정만 해두고 나면 그 이후로 테스트 하나 만드는 건 어렵지 않고, 프로덕션 코드를 건드리는 것보다는 테스트 코드에 뭔가 추가되는 게 맞지 않나 싶네요.

저도 컨트롤러 단위 테스트 단순만 놓고 보면 별로인거 같으나
docs 를 위한 단위 테스트라면 납득은 할 수 있는거 같습니다.

문서화를 위한 방법은 좀더 생각해봐야겠네요,,
사실 요구사항&기능이 변화함에 따라 테스트 코드가 너무 발목을 잡는다고 생각되어 불필요한 컨트롤러 단위 테스트가 더 필요없다고 생각도 했는데 그 발목을 잡는게 중요한 걸 수도 있겠네요 🥲

컨트롤러 테스트는 결국 클라-서버 간의 명세에 대한 테스트이기 때문에 그 명세의 변화에 대응해야하는 게 맞겠죠. 자연스럽게 docs 업데이트도 될 거구요. (실제로는 그냥 위키 같은데 문서를 작성하는 팀도 꽤나 많습니다)
정답이 있는 문제는 아니니 이것저것 사용해보시면 될 것 같습니다!

youngsu5582 · 2024-06-10T07:54:39Z

src/main/java/roomescape/member/domain/specification/AdminControllerSpecification.java

+
+import java.util.List;
+
+public interface AdminControllerSpecification {


해당 인터페이스는
관심사의 분리의 관점으로 접근했습니다.

어노테이션이 너무 많은데??
명세&검증 - 구현에 필요한 어노테이션들을 분리하자!

Specification 만 봐도 어떤 값들이 필요하고 어떤 검증이 필요하고 어떤 값을 응답할 것이다 가 유추가능할 것으로 기대되며
Controller 를 보면 온전히 Spring 구현 기능만 있어서 코드적 이해 가능

이라고 생각하는데
이 접근이 관심사의 분리적 접근이 맞는지 + 인터페이스로 분리한게 타당한지 궁금합니다!
( 결국은, 구현하는 Controller는 어노테이션들을 가지고 있을것 이므로 )

우선, @Override는 꼭 붙여주세요.
조금 변경 포인트가 늘어나긴 해도 Swagger를 사용할거라면 이렇게 사용해도 괜찮겠네요 👍

저는 어차피 public class AdminController implements AdminControllerSpecification 해당 구문에서
must either be declared abstract or implement abstract method 'create(MemberReservationRequest)' in 'AdminControllerSpecification'
를 띄울테니까 Override 를 생략해도 되겠지 라고 생각했는데

Override 를 명시적으로 붙이고 메소드를 작성함으로
추가적인 메소드를 만들때 제한 + 컴파일러 체크의 효과를 기대할 수도 있겠네요! 감사합니다 🫡🫡

youngsu5582 · 2024-06-10T08:01:18Z

src/main/java/roomescape/exception/RoomescapeException.java

+import org.springframework.http.HttpStatus;
+import org.springframework.http.HttpStatusCode;
+
+public class RoomescapeException extends RuntimeException {


저는 이전 미션들 까지는 ErrorType 과 같은 Enum이 필요 없다고 생각했습니다.

CustomException 을 필요마다 만들어서 사용하면 되는거 아니야?
라고 생각했습니다.

하지만, 늘어나는 Class + Exception Handler 관리하기 힘듬을 느꼈습니다.

이럴바엔 CustomException 하나에 + Enum 으로 관리하자를 느꼈는데

Enum 으로만 에러 케이스를 관리하는 것이 타당한지와
가장 큰 틀의 CustomException 을 벗어나는 Exception 의 필요성이 있는지 궁금합니다!
( 에러만의 추가적인 로직, 기능 제공이 있는게 아니라면 최상위 하나만 필요하다고 생각 )

자바가 갖고 있는 언어적 한계이기도 해서 enum으로 구분하는 것도 나쁘지 않아보이긴 하네요.
다만 런타임에 메세지를 넣어준다던지 하는 건 조금 어색하게 구현될 수도 있겠네요.

범블비...!

다만 런타임에 메세지를 넣어준다던지 하는 건 조금 어색하게 구현될 수도 있겠네요.

해당 내용이 조금 이해가 안되는데 설명을 더 해주실수 있을까요..? 😭😭

throw new RoomescapeException("AUTH-005","사용자는 어드민이 될 수 없습니다.");

이렇게 Enum 이 아닌 문자열 통한 생성이 어색하다는 걸까요?!

지금은 enum에서 에러 메세지를 갖고 있고, enum은 컴파일 타임에 모두 생성되니 런타임에 메세지를 넣어주는 게 어색할 것 같다는 얘기였습니다.
예를 들어 클라이언트에서 올린 값이 잘못돼서 그 값을 에러메세지에 첨부하고 싶으면
throw new Exception(clientValue + "값이 잘못되었습니다")와 같이 작성하면 되지만 enum에서 메세지를 관리하면 메세지를 넣어줄 때 어색할테니깐요.
formatting이나 Exception에 메세지 필드를 하나 더 둘 수는 있겠네요.

youngsu5582 · 2024-06-10T08:09:33Z

src/main/java/roomescape/reservation/domain/repository/MemberReservationRepository.java

    List<MemberReservation> findBy(Long memberId, Long themeId, ReservationStatus status, LocalDate startDate,
                                   LocalDate endDate);

+    @EntityGraph(attributePaths = {"reservation.theme","reservation.time","member"})


해당 부분에서 느낀점으로는 테이블 설계 + FetchType 을 어떻게 설정해야할까...? 였습니다.

EntityGraph 를 지정하지 않았을 때

내 예약 5개를 조회할 때 Lazy 로딩을 통해
23 번의 쿼리문이 나가는걸 확인했습니다.

5 : select r1_0.id,r1_0.date,r1_0.theme_id,r1_0.time_id from reservation r1_0 where r1_0.id=? 5 : select p1_0.id,p1_0.amount,p1_0.created_at,p1_0.member_reservation_id,p1_0.payment_key,p1_0.payment_type,p1_0.updated_at from payment p1_0 left join member_reservation mr1_0 on mr1_0.id=p1_0.member_reservation_id where mr1_0.id=? 5 : select mr1_0.id,mr1_0.created_at,mr1_0.member_id,mr1_0.reservation_id,mr1_0.reservation_status,mr1_0.updated_at from member_reservation mr1_0 join reservation r1_0 on r1_0.id=mr1_0.reservation_id where r1_0.id=? 4 : select rt1_0.id,rt1_0.start_at from reservation_time rt1_0 where rt1_0.id=? 3 : select t1_0.id,t1_0.description,t1_0.name,t1_0.amount,t1_0.thumbnail from theme t1_0 where t1_0.id=? 1 : select mr1_0.id,mr1_0.created_at,mr1_0.member_id,mr1_0.reservation_id,mr1_0.reservation_status,mr1_0.updated_at from member_reservation mr1_0 join member m1_0 on m1_0.id=mr1_0.member_id where m1_0.id=?

문제점을 느끼고 EntityGraph 를 통해 지정을 한후 11번의 쿼리로 줄였으나

mr1_0.id,mr1_0.created_at,mr1_0.member_id,m1_0.id,m1_0.created_at,m1_0.email,m1_0.name,m1_0.password,m1_0.role,m1_0.updated_at,mr1_0.reservation_id,r1_0.id,r1_0.date,r1_0.theme_id,t1_0.id,t1_0.description,t1_0.name,t1_0.amount,t1_0.thumbnail,r1_0.time_id,t2_0.id,t2_0.start_at,mr1_0.reservation_status,mr1_0.updated_at from member_reservation mr1_0 join member m1_0 on m1_0.id=mr1_0.member_id join reservation r1_0 on r1_0.id=mr1_0.reservation_id left join theme t1_0 on t1_0.id=r1_0.theme_id left join reservation_time t2_0 on t2_0.id=r1_0.time_id where m1_0.id=?

이렇게 join 문이 4번을 걸린걸 발견했습니다...

과연 정규화나 테이블을 최대한 분리한 설계가 더 좋을까?
라는 생각도 들었는데

이는 JPA 및 DB에 대한 지식이 부족해서 드는 것일까요?
( 그렇다고, 저 모든 값들을 받아오지 않게 하기 위해 Id 로 하거나 특정 칼럼만 받게 한다면 JPA 의 사용 이유가 해쳐지는거 같기도 합니다.. )

궁금한 점인데 Reservation이 member랑 status를 갖게 설계하지 않은 이유가 있을까요? 쿼리 11번도 많아보이네요.

JPA 의 사용 이유가 해쳐지는거 같기도 합니다..

해결해야할 문제가 먼저 있고, 그걸 해결하기 위해 기술을 사용하는 게 맞지 않을까 싶어요. 기술을 사용하면서 코드가 그 기술에 맞춰야한다면 별로 좋은 기술이 아닐까 싶은 생각이 듭니다.

지금 코드에서는 막상 객체를 불러와놓고 딱히 사용하는 구석이 없는 부분이 많아보이는데요, 이런 경우 적당히 id만 참조하게 해도 충분하지 않나 싶어요. 사용하게 되면 직접 id로 해당 객체를 불러오는 코드를 짜도 될 것 같네요.

해결해야할 문제가 먼저 있고, 그걸 해결하기 위해 기술을 사용하는 게 맞지 않을까 싶어요. 기술을 사용하면서 코드가 그 기술에 맞춰야한다면 별로 좋은 기술이 아닐까 싶은 생각이 듭니다.

저도 해당 부분은 격렬히 동의하는거 같습니다..!
코드가 먼저지, 기술(라이브러리) 가 먼저인 주객전도가 되면 안되는거 같습니다!

ddaaac

안녕하세요 조이썬 👋

구현 잘 해주셨네요.

몇 가지 코멘트 남겼으니 확인해주세요.

ddaaac · 2024-06-10T12:07:01Z

src/main/java/roomescape/member/domain/specification/AdminControllerSpecification.java

+
+import java.util.List;
+
+public interface AdminControllerSpecification {


개인적으로는 어노테이션 기반의 프로그래밍을 별로 안 좋아합니다.
말씀하신대로 단순히 메타데이터 역할만을 해야하는데, 그 사용방법을 잘 알고 유의하며 사용해야하는 어노테이션도 있기도 하죠.
또, 그런 유의사항을 지키지 않고 사용하거나 버그가 있을 때 어디서 버그가 났는지 추적하기도 굉장히 까다롭습니다.
사용하기도 편하고, 구현하기도 편한 건 맞긴 하지만 장기적으로 봤을 때 유지보수가 용이한가는 잘 모르겠네요.

저도 컨트롤러 단위 테스트는 필요없다고 생각하지만, docs를 위해서 만드는거라면 그 자체만으로도 충분히 이유가 될 수 있다고 생각해요.
처음 설정하는 게 조금 까다롭긴 하지만 일단 설정만 해두고 나면 그 이후로 테스트 하나 만드는 건 어렵지 않고, 프로덕션 코드를 건드리는 것보다는 테스트 코드에 뭔가 추가되는 게 맞지 않나 싶네요.

ddaaac · 2024-06-10T12:12:44Z

src/main/java/roomescape/member/domain/specification/AdminControllerSpecification.java

+
+import java.util.List;
+
+public interface AdminControllerSpecification {


우선, @Override는 꼭 붙여주세요.
조금 변경 포인트가 늘어나긴 해도 Swagger를 사용할거라면 이렇게 사용해도 괜찮겠네요 👍

ddaaac · 2024-06-10T12:16:22Z

src/main/java/roomescape/exception/RoomescapeException.java

+import org.springframework.http.HttpStatus;
+import org.springframework.http.HttpStatusCode;
+
+public class RoomescapeException extends RuntimeException {


자바가 갖고 있는 언어적 한계이기도 해서 enum으로 구분하는 것도 나쁘지 않아보이긴 하네요.
다만 런타임에 메세지를 넣어준다던지 하는 건 조금 어색하게 구현될 수도 있겠네요.

ddaaac · 2024-06-10T12:20:44Z

src/main/java/roomescape/global/config/SwaggerConfig.java

+@Configuration
+public class SwaggerConfig {
+


컨트롤러 코드 뿐만 아니라 프로덕션 환경에서는 필요없는 코드가 프로덕션 패키지 안에 꽤나 많이 들어간다는 점이 swagger의 단점이기도 하죠.

저도 사실
Swagger 가 문서 생성하는 기능을 컴파일단에서 완료하면 Swagger 무조건 사용해야 한다고 주장했을꺼 같은데
런타임 에도 Configuration 으로 Bean 관리 + 어노테이션이 Runtime 까지 유지
되는 면에서 아쉬움을 느끼는거 같습니다..
( 다른 언어에는 관련 링크 있는거 같은데 다소 아쉽네요 )

ddaaac · 2024-06-10T12:23:37Z

src/main/java/roomescape/global/util/OperationCustomizerWithEnums.java

+    @Override
+    public Operation customize(Operation operation, HandlerMethod handlerMethod) {
+        ApiResponses apiResponses = operation.getResponses();
+        ApiErrorResponses apiResponseCodes = handlerMethod.getMethodAnnotation(ApiErrorResponses.class);
+        ApiErrorResponse apiErrorResponse = handlerMethod.getMethodAnnotation(ApiErrorResponse.class);


어노테이션 기반 프로그래밍이 별로긴 해도 필요하다면 리플렉션을 박아가면서 기능 구현도 할 줄 알아야겠죠. 잘 구현해주셨어요 👍

이번에 어노테이션 기반으로 작성은 처음 해봤는데 생각보다 어렵네요...! ㅋㄱㅋㄱㅋㄱ
메타데이터 적인 접근(ErrorType을 저장) + 추가적인 기능을 위한 이정표(어노테이션이 있는지 확인 -> 이에따른 기능 수행)
으로 어노테이션&리플렉션을 접근했는데 맞을까요??

네네. 잘 구현해주셨습니다 👍

ddaaac · 2024-06-10T12:31:44Z

src/main/java/roomescape/reservation/domain/repository/MemberReservationRepository.java

    List<MemberReservation> findBy(Long memberId, Long themeId, ReservationStatus status, LocalDate startDate,
                                   LocalDate endDate);

+    @EntityGraph(attributePaths = {"reservation.theme","reservation.time","member"})


궁금한 점인데 Reservation이 member랑 status를 갖게 설계하지 않은 이유가 있을까요? 쿼리 11번도 많아보이네요.

JPA 의 사용 이유가 해쳐지는거 같기도 합니다..

해결해야할 문제가 먼저 있고, 그걸 해결하기 위해 기술을 사용하는 게 맞지 않을까 싶어요. 기술을 사용하면서 코드가 그 기술에 맞춰야한다면 별로 좋은 기술이 아닐까 싶은 생각이 듭니다.

지금 코드에서는 막상 객체를 불러와놓고 딱히 사용하는 구석이 없는 부분이 많아보이는데요, 이런 경우 적당히 id만 참조하게 해도 충분하지 않나 싶어요. 사용하게 되면 직접 id로 해당 객체를 불러오는 코드를 짜도 될 것 같네요.

ddaaac · 2024-06-10T12:33:55Z

src/main/java/roomescape/payment/domain/Payment.java

+import java.math.BigDecimal;
+


이 아래에 궁금한건데 하나의 member reservation에 payment가 여러개 생길 수 있나요?

이때 페어와 미래 확장성 & 요구사항 변경에 대해서 얘기해나갔을 때
분할결제가 생길수도 있지 않을까? 라는 얘기를 하며 차후 변경을 하는 것 보단
현재도 1:1 로 사용하나 명시적으로 @ManyToOne 을 붙혀 미래를 대비하자 라고 결정했으나
이또한 YANGI 원칙에 위배한거 같긴 하네요...! 🥲

youngsu5582 · 2024-06-11T12:50:06Z

범블비! 값진 리뷰들을 주셔서 감사합니다!
이번 반영은 테이블 비정규화 와 객체-> id 로 변경 부분을 시도해봤습니다!

우선

mr1_0.id,mr1_0.created_at,mr1_0.member_id,mr1_0.date,mr1_0.theme_id,t2_0.id,t2_0.description,t2_0.name,t2_0.amount,t2_0.thumbnail,mr1_0.time_id,t1_0.id,t1_0.start_at,mr1_0.reservation_status,mr1_0.updated_at from member_reservation mr1_0 join reservation_time t1_0 on t1_0.id=mr1_0.time_id join theme t2_0 on t2_0.id=mr1_0.theme_id where mr1_0.member_id=?

Reservation 을 별도 테이블에서 Embedded 로 변경하여
쿼리문을 6번으로 하는 것에는 성공하였습니다. ( 꽤나 대공사 였네요,,, 😭)

이번에 피드백을 반영하며 느낀점으로는

중복된 데이터들일지라도 조회시마다 JOIN문을 걸어야 한다면, 합치자!

이때, 중복된 데이터를 합치는 것도 데이터의 사용 빈도에 따라 결정하면 되는 걸까요?
( 조회시마다 JOIN문을 거는게 아니라면, JOIN 해도 괜찮을 거 같다고 생각도 듭니다. 결국은 정규화를 지키는게 더 클린해지므로 )

반드시 필요한 객체(객체 내부 비즈니스 로직을 수행하는데 필요하다 등등)가 아니라면 id 로만 선언해도 괜찮겠다...?

하지만, 기존에 설계되어 있는 부분을 수정하는게 상당히 큰 어려움을 요구하는걸 느꼈습니다...
( reservation 을 embedded 로 변경하는데 27개의 파일이 수정 된다든지... )
이 느낌때문에 테이블을 설계 하는 부분에서 꽤나 조심스럽게 할꺼같은데요,,

그런 맥락에서라면 더욱이 2번을 수행하기 어렵겠다는 느낌이 들었습니다.

기존에는 필요를 못느껴 id로 선언했으나, 객체로 변경해야 함을 느낄때 대규모 변경이 예상

추가적인 내용으로 DB를 잘못 설계하거나 변동할때 상당히 큰 변화를 들고오는걸 느꼈습니다..
( JPA이므로 도메인 변화 -> Repository 전파 -> Service 전파 ... )
이렇게 DB 의 변화에 깨지는건 설계의 문제인지 어쩔수 없는 부분일까요...?
( 칼럼을 추가하는건 그렇다고 치나, 정규화/비정규화나 데이터 분리시 에 대규모 변화 요망 - 이는 DB단에서 변화지 비즈니스 로직상 변화는 아니므로 )

비즈니스 로직에서 logging은 필요한 부분마다 설정하면 되는걸까요?
( 에러는 확인용으로 필요하다 생각했는데 비즈니스 로직 중 로깅은 필요한 이유나 필요한 부분을 잘 못 느끼겠습니다! )

ddaaac

안녕하세요 조이썬 👋

리뷰 반영 잘 해주셨습니다.

이번 미션은 여기서 마무리할게요. 고생 많으셨어요!

ddaaac · 2024-06-13T10:56:56Z

src/main/java/roomescape/reservation/domain/MemberReservation.java

-    @ManyToOne(fetch = FetchType.LAZY, optional = false)
-    @JoinColumn(name = "RESERVATION_ID")
-    private Reservation reservation;
+    @Embedded
+    private ReservationInfo reservation;


중복된 데이터들일지라도 조회시마다 JOIN문을 걸어야 한다면, 합치자!

조금 다른 얘기기는 하지만 개인적으로 JPA를 그리 좋아하지 않는 이유이기도 한데요, 도메인 영역의 코드와 DB 관련 내용들이 섞여있어서 도메인을 바라볼 때 DB 관점의 사고가 많이 들어가게 됩니다. DB가 없는 1단계라면 이 객체에 어떤 네이밍을 했고, 어떤 설계를 했을지 한 번 생각해보면 좋을 것 같아요(물론 DB를 빼놓고 생각할 수는 없지만요).
여튼 진짜로 바꾸실 줄은 몰랐는데 바꿔주셨네요 👍 개인적으로는 이런 설계를 할 때 도메인 관점에서 먼저 생각해보시면 좋을 것 같습니다.

ddaaac · 2024-06-13T10:58:29Z

src/main/java/roomescape/reservation/domain/MemberReservation.java

-    @ManyToOne(fetch = FetchType.LAZY, optional = false)
-    @JoinColumn(name = "RESERVATION_ID")
-    private Reservation reservation;
+    @Embedded
+    private ReservationInfo reservation;


반드시 필요한 객체(객체 내부 비즈니스 로직을 수행하는데 필요하다 등등)가 아니라면 id 로만 선언해도 괜찮겠다...?

실제 어플리케이션들은 도메인 간의 관계가 훨씬 복잡한 경우가 많습니다. 그런 경우 객체끼리 모두 참조하고 있으면 쿼리 수가 많아지거나 엄청난 JOIN 문이 발생하겠죠. 적절하게 id만 참조하게 해도 좋을 것 같아요. id만 갖고 있어도 로직에서 필요하면 쿼리해서 사용하면 될테니깐요.

ddaaac · 2024-06-13T11:02:10Z

src/main/java/roomescape/reservation/domain/MemberReservation.java

-    @ManyToOne(fetch = FetchType.LAZY, optional = false)
-    @JoinColumn(name = "RESERVATION_ID")
-    private Reservation reservation;
+    @Embedded
+    private ReservationInfo reservation;


이렇게 DB 의 변화에 깨지는건 설계의 문제인지 어쩔수 없는 부분일까요...?

아무래도 그렇죠. 그래서 저는 DB에 대응하는 객체와 도메인 객체를 아예 따로 두는 걸 선호하긴합니다. 이렇게 설계해두면 DB 스키마 변경이 있어도 DB <-> 도메인 매핑해주는 부분만 손보면 되긴 하거든요.
그리고, 어플리케이션 코드 변경 때문에 DB 변화를 두려워하는 것도 있지만 데이터가 많은 경우 마이그레이션도 문제가 됩니다. 실시간으로 데이터가 들어오고 수정되는 환경에서 마이그레이션을 하는 건 아무래도 부담이 꽤나 있죠.
첫 삽을 잘 뜨는 게 중요하긴 하지만 필요하다면 언제든지 변경할 수 있는 능력을 갖추는 것도 중요하기도 하구요.

ddaaac · 2024-06-13T11:04:47Z

src/main/java/roomescape/auth/service/jwt/JwtTokenProvider.java

-            log.warn(e.getMessage());
-            throw new AuthenticationException(ErrorType.SECURITY_EXCEPTION);
+            throw new RoomescapeException(ErrorType.SECURITY_EXCEPTION);


비즈니스 로직에서 logging은 필요한 부분마다 설정하면 되는걸까요?

특별한 사항이 없다면 에러 로그 정도만 받아보긴 할거에요.
하지만 원인을 찾기 힘든 버그를 찾을 때나, 예외를 던져서 로직을 중단하면 안 되지만 에러 상황을 로깅해야할 경우 등등에 사용하게 됩니다.
지금은 특별히 요구사항이 없어서 느끼지 못할 수 있지만 필요하면 자연스럽게 로깅을 하게 될 것 같습니다.

ddaaac · 2024-06-13T11:10:07Z

src/main/java/roomescape/member/domain/specification/AdminControllerSpecification.java

+
+import java.util.List;
+
+public interface AdminControllerSpecification {


컨트롤러 테스트는 결국 클라-서버 간의 명세에 대한 테스트이기 때문에 그 명세의 변화에 대응해야하는 게 맞겠죠. 자연스럽게 docs 업데이트도 될 거구요. (실제로는 그냥 위키 같은데 문서를 작성하는 팀도 꽤나 많습니다)
정답이 있는 문제는 아니니 이것저것 사용해보시면 될 것 같습니다!

ddaaac · 2024-06-13T11:13:24Z

src/main/java/roomescape/exception/RoomescapeException.java

+import org.springframework.http.HttpStatus;
+import org.springframework.http.HttpStatusCode;
+
+public class RoomescapeException extends RuntimeException {


지금은 enum에서 에러 메세지를 갖고 있고, enum은 컴파일 타임에 모두 생성되니 런타임에 메세지를 넣어주는 게 어색할 것 같다는 얘기였습니다.
예를 들어 클라이언트에서 올린 값이 잘못돼서 그 값을 에러메세지에 첨부하고 싶으면
throw new Exception(clientValue + "값이 잘못되었습니다")와 같이 작성하면 되지만 enum에서 메세지를 관리하면 메세지를 넣어줄 때 어색할테니깐요.
formatting이나 Exception에 메세지 필드를 하나 더 둘 수는 있겠네요.

ddaaac · 2024-06-13T11:15:57Z

src/main/java/roomescape/global/util/OperationCustomizerWithEnums.java

+    @Override
+    public Operation customize(Operation operation, HandlerMethod handlerMethod) {
+        ApiResponses apiResponses = operation.getResponses();
+        ApiErrorResponses apiResponseCodes = handlerMethod.getMethodAnnotation(ApiErrorResponses.class);
+        ApiErrorResponse apiErrorResponse = handlerMethod.getMethodAnnotation(ApiErrorResponse.class);


네네. 잘 구현해주셨습니다 👍

youngsu5582 added 15 commits June 5, 2024 21:31

feat: 테마,결제내 결제금액 도메인 통일

b9e7884

refactor: 예약 정보만 담는 DTO 분리

175b862

feat: 내 예약 조회시 결제 내역 포함 추가

6aec973

feat: 프론트,데이터 로더에 변경 반영

567240f

feat: 커스텀 예외 하나로 통일

8e03225

feat: SpringDocs 설치, 에러용 어노테이션 추가

10fe093

feat: 에러그룹용 어노테이션 추가, 에러 어노테이션을 통해 응답 만드는 optimizer 추가

c007788

feat: 로그인사용자 어노테이션에 @hidden 추가

31fd64f

feat: API 명세-구현 관심사 분리

3c4320a

feat: 예외 생성자 변경, 필요한 부분만 에러 로깅하게 변경

54b548c

feat: Configuration 에 operationCustomizer 추가

54117ee

feat: 쿼리 체크용 property 추가, 이에 따른 EntityGraph 로 성능 개선

44dc406

feat: 배포용 설정,deploy script 추가

d2aa6b4

feat: EC2 빌드 실패로 배포용 파일 추가

90384d1

fix: 잘못 들어간 환불 내용 이동

c3787aa

youngsu5582 commented Jun 10, 2024

View reviewed changes

ddaaac requested changes Jun 10, 2024

View reviewed changes

youngsu5582 added 2 commits June 11, 2024 20:00

feat: Payment 내 MemberReservation id로 변경

ed55d9e

feat: Reservation 테이블 삭제, Embedded 로 변경

24492b7

ddaaac approved these changes Jun 13, 2024

View reviewed changes

ddaaac merged commit 6891a21 into woowacourse:youngsu5582 Jun 13, 2024


		import java.util.List;

		public interface AdminControllerSpecification {

Comments

Conversation

youngsu5582 commented Jun 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

youngsu5582 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

youngsu5582 Jun 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

youngsu5582 Jun 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

youngsu5582 Jun 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ddaaac left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

youngsu5582 Jun 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

youngsu5582 commented Jun 11, 2024

Uh oh!

ddaaac left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

youngsu5582 commented Jun 10, 2024 •

edited

Loading

youngsu5582 Jun 11, 2024 •

edited

Loading

youngsu5582 Jun 10, 2024 •

edited

Loading

youngsu5582 Jun 10, 2024 •

edited

Loading

youngsu5582 Jun 11, 2024 •

edited

Loading