[3 - 4단계 방탈출 예약 대기] 구름(김민수) 미션 제출합니다. #93
Conversation
어떻게 활용하느냐에 어떤 종류의 요청을 처리하느냐에 따라 성능이 달라질 수 있을것 같은데요. 저도 status로 관리하지 않았을까 싶네요.
|
테스트 코드를 보았는데 해당 부분에 리뷰를 남겨두겠습니다. |
좋은 고민이네요. 👍 저도 도메인에서 발생할 수 있는 로직을 별도로 구분하기 위해 추상적인 domain 전용 Exception을 하나 만들어두고 활용합니다. 위와 같이 하면 말씀하신 것 처럼 프레임워크, 언어 레벨의 exception과 구분하여 보다 정확한 에러 핸들링이 가능해지는 장점이 있습니다. |
| @Test | ||
| @Sql("/waitings.sql") | ||
| @DisplayName("회원 아이디로 예약 대기 순번을 포함한 예약 대기들을 조회한다.") | ||
| void findReservationWithRanksByMemberId() { | ||
| List<ReservationWithRankDto> reservationWithRanks = reservationRepository | ||
| .findReservationWithRanksByMemberId(4L); | ||
|
|
||
| SoftAssertions.assertSoftly(softly -> { | ||
| softly.assertThat(reservationWithRanks).hasSize(3); | ||
|
|
||
| softly.assertThat(reservationWithRanks.get(0).reservation().getId()).isEqualTo(4L); | ||
| softly.assertThat(reservationWithRanks.get(0).rank()).isEqualTo(3); | ||
|
|
||
| softly.assertThat(reservationWithRanks.get(1).reservation().getId()).isEqualTo(6L); | ||
| softly.assertThat(reservationWithRanks.get(1).rank()).isEqualTo(0); | ||
|
|
||
| softly.assertThat(reservationWithRanks.get(2).reservation().getId()).isEqualTo(9L); | ||
| softly.assertThat(reservationWithRanks.get(2).rank()).isEqualTo(1); | ||
| }); | ||
| } |
There was a problem hiding this comment.
테스트 코드는 가능한 테스트 코드만 보고 무엇을 테스트하고자 하는지 파악할 수 있도록 작성하는 것이 좋습니다.
그래서 저는 Sql을 통해 setup data를 추가하는 행위를 지양하는 편입니다.
그 이유는 다음과 같습니다.
- 해당 insert 쿼리는 애플리케이션을 통해 insert 될 수 없는 데이터가 될 수 있습니다. 이는 곧 올바른 데이터 모델의 조회가 아닐 수 있다는 것과 일맥상통해집니다.
- 테스트 코드에서 검증하는 값들의 정확한 이유를 파악하기 위해 sql script를 확인해야합니다. 화면의 이동을 뜻하고 개발자의 인지과부하 요소로 작용할 수 있습니다.
- sql script에 종속적인 테스트가 되어 여러곳에서 sql script를 활용하거나 sql script가 변경되면 테스트가 같이 변경되어야 합니다. 변경을 해야하는 상황이 발생했을때 sql script를 어떻게 바꾸어야 하는지 작성한 사람이 아니면 알기 힘든 경우가 많습니다.
이러한 불편함이 있지만 경험하신 것처럼 장점도 분명히 있습니다. 테스트 코드 본문 자체는 간결해집니다.
저는 이러한 불편함을 해소하기 위해 test fixture를 사용합니다. 해당 테스트 클래스 혹은 testFixture 패키지에 데이터를 생성할 수 있는 private 메서드를 미리 준비해두고 연속적으로 호출하여 필요한 데이터를 구성하는 방법입니다.
There was a problem hiding this comment.
테스트 코드가 정말 도저히 모르겠네요.
궁금한게 있습니다.
- 엄청 크게 초기 데이터를 만들어놓고 다른 여러 곳에서 사용하지는 않나요?
- sql insert가 아닌 코드로 데이터를 넣어준다면, 어느 부분까지 검증된 코드를 넣나요? 예를 들어 도메인에 있는 검증만 거쳐서 된 값을 넣나요? 아니면 서비스에도 검증 로직이 있는데 그것까지 거친 값을 넣어주나요?
- 인증과 관련된 것을 처리하려면 거의 모든 곳에서 member가 필요합니다. 이런 경우는 어떻게 하나요?
- Fixture를 다른 폴더에 두고 사용할거면 sql이나 별반 다를 거 없지 않나요??
참고할만한 오픈된 프로젝트가 있을까요??
There was a problem hiding this comment.
저의 경험을 토대로 말씀드리는 것이니 참고만해주세요!
엄청 크게 초기 데이터를 만들어놓고 다른 여러 곳에서 사용하지는 않나요?
제가 속한 곳에서는 지양하는 방법입니다. 초기 데이터에 영향을 너무 많이 받고 해당 테스트가 어떤 초기 데이터에 영향을 받는지 명시적으로 알기가 너무 힘든 경우가 많아지기 때문입니다.
따라서 테스트 데이터는 테스트 메서드 내에서 그때 그때 생성해주는 편입니다.
그리고 테스트를 위해서는 최소한의 데이터만 준비하는 것이 좋습니다. 큰 데이터는 큰 비용을 초래하기 때문에 테스트의 비용또한 올라갈 것 같습니다.
sql insert가 아닌 코드로 데이터를 넣어준다면, 어느 부분까지 검증된 코드를 넣나요? 예를 들어 도메인에 있는 검증만 거쳐서 된 값을 넣나요? 아니면 서비스에도 검증 로직이 있는데 그것까지 거친 값을 넣어주나요?
보통 최소한의 데이터 정합성을 위해서 도메인에 객체를 생성후 insert합니다. service를 통한 데이터 생성은 가능한 지양하는 편입니다.
해당 service가 안정적인 데이터 셋을 제공하는지 명시적으로 보장되지 않기 때문입니다.
Fixture를 다른 폴더에 두고 사용할거면 sql이나 별반 다를 거 없지 않나요??
fixture를 이용하면 테스트 메서드에서 생성되는 데이터를 명시적으로 표현할 수 있지만 sql을 이용하면 해당 스크립트로 이동해서 생성되는 데이터를 확인해보아야 합니다.
지금의 경우 waiting.sql에서 무엇을 생성하는지 확인해야하는것이 fixture를 이용하면 테스트 메서드 내에 몇줄로 명시적으로 드러낼 수 있게 되는 것이 큰 장점으로 보입니다.
| @Query(""" | ||
| SELECT | ||
| new roomescape.domain.reservation.dto.ReservationWithRankDto( | ||
| r, | ||
| CAST (( | ||
| SELECT COUNT(r2) | ||
| FROM Reservation r2 | ||
| WHERE r2.status = 'WAITING' | ||
| AND r2.theme = r.theme | ||
| AND r2.date = r.date | ||
| AND r2.time = r.time | ||
| AND r2.id <= r.id | ||
| ) AS Long) | ||
| ) | ||
| FROM Reservation r | ||
| JOIN FETCH r.member | ||
| JOIN FETCH r.time | ||
| JOIN FETCH r.theme | ||
| WHERE r.member.id = :memberId | ||
| """) | ||
| List<ReservationWithRankDto> findReservationWithRanksByMemberId(Long memberId); |
There was a problem hiding this comment.
서브쿼리를 활용하는 방식은 성능이 그리 좋지 못한 경우가 많기 때문에
가능한 서브쿼리를 활용하지 않고 해결해보는 연습을 해보시면 좋을것 같습니다.
|
|
||
| default Reservation getById(Long id) { | ||
| return findById(id) | ||
| .orElseThrow(() -> new DomainNotFoundException("해당 id의 예약이 존재하지 않습니다.")); |
There was a problem hiding this comment.
가능한 exception에는 exception의 원인이 되는 값을 추가해주면 좋습니다.
애플리케이션 운영과 디버그에 큰 도움이 됩니다.
There was a problem hiding this comment.
다음과 같이 예외 메세지들을 구체적으로 수정했습니다.
default Reservation getByIdAndStatus(long id, ReservationStatus status) {
String message = String.format("해당 id와 예약 상태의 예약이 존재하지 않습니다. (id: %d, status: %s)", id, status);
return findByIdAndStatus(id, status)
.orElseThrow(() -> new DomainNotFoundException(message));
}| validateCurrentMemberAlreadyReserved(reservation); | ||
| validateReservationWaitingExists(reservation); |
There was a problem hiding this comment.
해당 회원의 정보로 예약을 찾고 도메인에게 메세지를 던지는건 어떨까요?
There was a problem hiding this comment.
Member에 OneToMany List 필드를 두고 검증하라는 의미인가요?
잘 이해가 가지 않아요,,,
There was a problem hiding this comment.
제가 리뷰를 드렸던 코드를 보면 아래와 같은데요.
private void validateCurrentMemberAlreadyReserved(Reservation reservation) {
boolean currentMemberAlreadyReserved = reservationRepository.existsByReservationWithMemberId(
reservation.getDate(),
reservation.getTime().getId(),
reservation.getTheme().getId(),
reservation.getMember().getId(),
ReservationStatus.RESERVED
);
if (currentMemberAlreadyReserved) {
throw new BadRequestException("해당 회원은 이미 예약을 하였습니다.");
}
}
private void validateReservationWaitingExists(Reservation reservation) {
boolean reservationWaitingExists = reservationRepository.existsByReservationWithMemberId(
reservation.getDate(),
reservation.getTime().getId(),
reservation.getTheme().getId(),
reservation.getMember().getId(),
ReservationStatus.WAITING
);
if (reservationWaitingExists) {
throw new BadRequestException("해당 회원은 이미 예약 대기를 하였습니다.");
}
}reservationRepository를 통해서 동일한 reservation의 상태를 묻는 행위를 두번하는 것으로 보였습니다.
그래서 reservationRepository를 통해 조건절에 status만 제외하여 조회한 Reseravtion에게
"너 예약 가능해?"라는 메세지를 보내고 해당 메세지를 받은 Reservation이 자신의 status를 확인하는 방법을 이야기 한 것이었습니다.
| if (reservationDateTime.isBefore(currentDateTime)) { | ||
| throw new DomainValidationException("지나간 날짜/시간에 대한 예약은 불가능합니다."); | ||
| } |
There was a problem hiding this comment.
여기도 조금 더 메세지에 정보를 담아주면 좋을 것 같습니다.
이 부분을 비롯에 다른 부분도 전체적으로 검토해주세요.
There was a problem hiding this comment.
다음과 같이 예외 메세지들을 구체적으로 수정했습니다.
public static Reservation create(
LocalDateTime currentDateTime,
LocalDate date,
Member member,
ReservationTime time,
Theme theme,
ReservationStatus status
) {
LocalDateTime reservationDateTime = LocalDateTime.of(date, time.getStartAt());
if (reservationDateTime.isBefore(currentDateTime)) {
String message = String.format("지나간 날짜/시간에 대한 예약은 불가능합니다. (예약 날짜: %s, 예약 시간: %s)", date, time.getStartAt());
throw new DomainValidationException(message);
}
return new Reservation(date, member, time, theme, status);
}| Reservation reservation = reservationRepository.getById(id); | ||
|
|
||
| validateReservationNotWaiting(reservation); | ||
| validateReservationAlreadyExists(reservation); | ||
|
|
||
| reservation.updateToReserved(); | ||
|
|
||
| return ReservationResponse.from(reservation); |
There was a problem hiding this comment.
조금 절차적으로 표현된 코드가 있어보입니다. 가능한 객체에게 메세지를 던질 수 있는 모양새로 변경해보시면 좋을것 같습니다.
There was a problem hiding this comment.
잘 이해가 가지 않아요,,,
검증이 문제인가요? updateToReserved가 문제인가요?
레벨 1 자바에서 레벨 2 스프링으로 넘어오면서 객체 지향적으로 생각하기가 힘드네요,,,
There was a problem hiding this comment.
private void validateReservationAlreadyExists(Reservation reservation) {
boolean reservationExists = reservationRepository.existsByReservation(
reservation.getDate(),
reservation.getTime().getId(),
reservation.getTheme().getId(),
ReservationStatus.RESERVED
);
if (reservationExists) {
throw new BadRequestException("이미 예약이 존재합니다.");
}
}
private void validateReservationNotWaiting(Reservation reservation) {
if (reservation.getStatus() != ReservationStatus.WAITING) {
throw new BadRequestException("예약 대기 상태가 아닙니다.");
}
}위 코드는 reservation 객체에게 메세지를 던질 수 있는 부분으로 보였기 때문입니다.
There was a problem hiding this comment.
reservation.getStatus() != ReservationStatus.WAITING이 부분을 말하셨던 거군요,,,
| @Transactional | ||
| public void rejectReservationWaiting(Long id) { | ||
| Reservation reservation = reservationRepository.getById(id); | ||
|
|
||
| validateReservationNotWaiting(reservation); | ||
|
|
||
| reservationRepository.deleteById(id); | ||
| } |
There was a problem hiding this comment.
특정 개선을 바라는것은 아닙니다.
논리적 삭제와 물리적 삭제에 대해 알아보시면 좋을것 같습니다.
There was a problem hiding this comment.
물리 삭제는 진짜 DELETE를 하는 것이고, 논리 삭제는 삭제 여부 필드를 두는 것이겠죠.
deletedAt이나 isDeleted 같은 필드를 둘 수 있겠군요.
데이터 복구 및 관리에 용이하겠네요.
또한 실제 삭제가 아닌 플래그만 변경해줘서 성능적으로도 좋을 것 같아요.
| LocalDate date, | ||
| Long timeId, | ||
| Long themeId, | ||
| Long memberId, | ||
| ReservationStatus status |
There was a problem hiding this comment.
해당 값들이 nullable한다는 등의 validate 가 필요없는 요청이라면 문제없겠지만
그렇지 않은 경우라면 하나의 dto로 관리하는것이 유리할 것 같습니다.
There was a problem hiding this comment.
private으로 내부에서만 사용하는데도 dto가 필요할까요?
There was a problem hiding this comment.
아 이 부분은 제가 private 메서드라는 것을 잘못 보았네요! 무시하셔도 됩니다 😄
|
|
||
| @Service | ||
| @Transactional(readOnly = true) | ||
| public class ReservationService { |
There was a problem hiding this comment.
사용자 기능과 관리자 기능이 하나의 service를 같이 사용하는것 같은데 이는 의도한 부분일까요?
분리하여 관리하는 것에 대한 고민을 해보셨는지 궁금합니다.
There was a problem hiding this comment.
controller뿐만 아니라 service도 분리를 해야하나요?
접근 제어 관점에서는 좋을 것 같지만 예약 생성과 같은 경우는 완전 동일하게 생길 것 같은데 재사용의 관점에서는 조금 별로 아닌가요?
There was a problem hiding this comment.
분리를 바라고 드린 리뷰가 아니라 말 그대로 분리에 대해 고민을 해보셨는지 궁금해서 질문드린 부분입니다.
제가 드리는 리뷰는 모두 반영해야하는 사항이 아닙니다. 😓
There was a problem hiding this comment.
첨언을 드리자면
완벽하게 공통된 기능을 사용하지 않는 이상 저는 분리하는 편입니다.
단일책임 원칙을 준수하고 싶은 욕심이 있어서요.😄
| @Transactional | ||
| public ReservationResponse addReservationWaiting(ReservationWaitingRequest request) { | ||
| Reservation reservation = createReservation( |
There was a problem hiding this comment.
동시에 두명의 사용자가 이 API에 요청을 한 경우 올바른 데이터 상태가 될 수 있도록 보장할 수 있나요?
There was a problem hiding this comment.
Lock을 걸거나 unique를 하기에는 비용이 큰 것 같아서 서버가 하나라면 syncronized를 사용할 것 같습니다.
서버가 여러 개라면 잘은 모르지만 global lock이라는 것을 할 것 같습니다.
h2라서 격리 수준 변경은 잘 안되는 것 같더라구요. 사실 잘 모르겠습니다,,,
There was a problem hiding this comment.
오 글로벌 락에 대해 알아보셨군요 👍
위 메서드에 내재되어있는 문제에 대해 인지하고 계신지
인지하고 있다면 어떠한 방식으로 해결할 수 있을지 알고계실까 하여 드린 리뷰였습니다 😄
변경 사항Reservation 테이블에 status로 예약과 예약 대기를 표현할려고 했으나 점점 불편함이 느껴지더라구요. 모르겠는 점객체지향적으로 생각하는 것을 많이 피드백 주신 것 같아요. 테스트 코드에서 초기 데이터를 sql로 하지 않고, 코드로 해볼려고 했는데 너무 복잡해지는 것 같아요. |
|
안타깝게도 제가 오픈소스로 공개된 프로젝트중에 참고로 알려드릴만한 프로젝트는 잘 모르겠네요. 😓 |
|
테스트와 관련하여 꼭 한번 읽어보시면 좋은 글을 첨부드립니다. |
| Reservation reservation = createReservation( | ||
| request.date(), | ||
| request.timeId(), | ||
| request.themeId(), | ||
| request.memberId() | ||
| ); |
There was a problem hiding this comment.
시간값에 대해 제어를 하기가 힘드니 Clock을 이용해주신것 같네요.
Clock이 모든 공개 메서드에서 사용된다면 모르겠지만 그렇지 않다면
저는 보통 시간값에 대한 제어를 하기 위해 외부(requestDto)에서 시간값을 입력받는 편입니다.
There was a problem hiding this comment.
그러니까 즉, Service에서 Clock을 주입받지 않고, Controller에서 Clock을 주입받고 dto로 넘기는게 좋다는 말씀이시죠?
Clock 의존성을 Controller로 다 옮기고 시간을 파라미터나 dto로 받도록 변경했습니다.
There was a problem hiding this comment.
오 아니요 dto에 requestDateTime과 같은 LocalDateTime 필드를 추가로 전달하도록 하는것 입니다.
현재 시간 값에 대한 제어를 dto에 전달되는 값을 통해서 하는 것입니다.
There was a problem hiding this comment.
Service에서 Clock을 사용하지 말고 예약 생성 dto(ReservationRequest)에서 currentDateTime을 받으라는 의미가 맞죠?,,,
public record ReservationRequest(
LocalDateTime currentDateTime, // 이 부분을 말하시는거 맞죠?,,,
LocalDate date,
Long themeId,
Long timeId,
Long memberId
) {
}
|
테스트 코드에서 sql파일을 다 지우고, fixture를 통해서 테스트해봤습니다. 이번 미션에서 개인적으로 schema를 여러 방식으로 바꿔봤는데 완벽하게 좋은 방식을 모르겠네요,,, |
사실 완벽하게 좋은 설계라는 것은 없다고 생각합니다. 동일한 요구사항에서도 애플리케이션을 사용하는 고객의 규모와 트래픽이 늘어남에 따라서 보통 데이터의 설계가 우선이 되지 않고 시스템의 설계가 우선이 되는편입니다. 그래서 저는 데이터 설계에 대한 책보다 시스템 설계에 대한 책이 더 도움이 될 것 같습니다. 이와 관련하여 유명한 책 두권이 있는데요. 가상면접 시리즈 두권을 추천드립니다. 지금은 조금 어려울 수 있는 책이라고 생각이 되긴하지만 지금한번 읽어보시고 나중에 다시한번 읽어보시면 보이지 않았던 부분이 새롭게 보이지 않을까 싶습니다. |
|
|
||
| @Service | ||
| @Transactional(readOnly = true) | ||
| public class ReservationService { |
There was a problem hiding this comment.
첨언을 드리자면
완벽하게 공통된 기능을 사용하지 않는 이상 저는 분리하는 편입니다.
단일책임 원칙을 준수하고 싶은 욕심이 있어서요.😄
| Reservation reservation = createReservation( | ||
| request.date(), | ||
| request.timeId(), | ||
| request.themeId(), | ||
| request.memberId() | ||
| ); |
안녕하세요 비밥! 구름 ⛅️입니다!
1단계 이후 오랜만에 뵙네요.
패키지 구조도 변경하고, 예외 처리 방법도 변경했습니다. 최대한 의존 방향을 지킬려고 노력했어요.
변경 사항이 많은 점 미리 죄송합니다,,,
미션 정보
3, 4단계에서는 예약 대기와 예약 대기 승인, 거절이 있었어요.
자동 승인과 수동 승인 중에 저는 수동 승인으로 개발했어요.
웹에서 쉽게 테스트하는 방법이 있어요.
피드백 받고 싶은 부분
미션이 점점 쌓이면서 참 많은 부분이 있는데요.
Reservation과 Waiting으로 Entity를 분리할 수도 있었겠지만 저는 크게 불편함을 못 느꼈어요.
오히려 하나로 관리하고 status로 처리해서 편하다고 생각했어요. 그러나 이게 성능적으로 안좋을까요?
점점 코드가 많아지면서 테스트가 힘들더라구요. 또한 어드민과 유저 인증이 생기면서 테스트가 더더욱 복잡해졌어요.
초기 데이터를 넣어주기도 했는데 이 방법이 적절한지 모르겠어요.
이전 미션들에서는 도메인에 표준 예외를 사용해서 처리할려고 했어요. 하지만 표준 에러를 GlobalExceptionHandler에서 잡을 경우 제가 원하지 않았던 자바나 스프링에서의 에러가 메세지로 응답될 수 있다고 생각했어요. 그래서 domain과 application exception을 만들고 이런 예외들만 GlobalExceptionHandler에서 잡을려고 했어요. 이게 적절했을까요?
초기 데이터 정보
어드민
유저