[1 - 3단계 방탈출 사용자 예약] 페드로(류형욱) 미션 제출합니다. #33

hw0603 · 2024-05-02T08:20:30Z

안녕하세요 범블비, 6기 BE 페드로입니다.
1-3단계 기능 요구사항 구현을 완료하여 리뷰 요청 드려요!

이번 미션을 진행하면서 생긴 궁금한 점들이 몇 가지 있는데, 이 부분은 별도의 코멘트로 달아 두도록 하겠습니다.
리뷰 잘 부탁드립니다😀

이전의 웹 개발 경험

스프링을 공부하며 사용해 본 경험은 직전 미션이 처음이라고 보셔도 될 것 같아요.
주로 Python 기반의 간단한 API 서버를 개발해 본 경험이 있습니다.(FastAPI, flask 사용)
클라우드 환경에서의 배포(자동화) 경험은 없고, 온프레미스 서버로의 간단한 배포는 경험해 본 적이 있습니다.

베이스 코드는 클로버 @eunjungL 의 이전 미션 코드를 기반으로 구현하였습니다.
시간이 조금 촉박해서 테스트 패키지의 코드들은 리팩토링에 신경을 많이 쓰지 못했던 것 같아요🥲
변경사항 바로가기

Co-authored-by: Hyunguk Ryu <[email protected]>

- createInitReservation() 오타 수정 Co-authored-by: Hyunguk Ryu <[email protected]>

Co-authored-by: Hyunguk Ryu <[email protected]>

- Table reset sql 통합 Co-authored-by: Hyunguk Ryu <[email protected]>

Co-authored-by: Hyunguk Ryu <[email protected]>

- RowMapper 메서드 static화 Co-authored-by: Hyunguk Ryu <[email protected]>

- 인기 테마 조회 시 조회 조건(일자, 개수) 상수화 Co-authored-by: Hyunguk Ryu <[email protected]>

Co-authored-by: Hyunguk Ryu <[email protected]>

hw0603 · 2024-05-02T08:55:18Z

DB의 기능을 어느 정도까지 활용해도 되는 것인가?

일반적인 관계형 DB를 기준으로, 쿼리를 통해 처리할 수 있는 것들이 상당히 많습니다.
예를 들어 이번 미션에서 상위 10개의 인기 테마를 정렬하여 보여주는 요구사항의 경우,

날짜 구간 검색
상위 N개 limit 처리
인기도에 따라 정렬
이 모두를 쿼리를 통해 처리할수도 있고, 애플리케이션 상에서 처리할수도 있습니다.

지금까지는 도메인 요구사항으로 판단될 경우 모두 애플리케이션 단에서 처리하는 것이 맞다고 판단했었고, DB가 관계형 DB가 아닌 NoSQL과 같은 구조로 변경될 경우에 변경이 서비스 로직까지 전파되면 안 된다고 생각하여 쿼리 의존도를 높게 가져가지 않는 방향으로 구현해 왔었습니다.

하지만 점점 요구사항이 많아지고, 모든 것을 애플리케이션에서 처리하려니 구현해야 할 코드가 급격히 많아지는 것을 느꼈습니다. '관계형 DB를 사용하는 장점을 제대로 누리지 못하고 있는 것인가?' 하는 생각도 들구요.

이번 미션에서는 1. 날짜 구간 검색 만 DB에서 처리하고 나머지는 애플리케이션이 처리하도록 해 두었는데요, 범블비는 특정 요구사항의 구현을 쿼리를 통해 처리할지, 애플리케이션의 로직으로 처리할지를 판단하는 기준이 무엇이라고 생각하시나요?

Exception Handler 위계

요청 JSON에 잘못된 형식의 날짜/시간이 들어올 경우 JSON 역직렬화 과정에서 DateTimeParseException이 발생하는데, 스프링 내부적으로 이를 잡고, HttpMessageNotReadableException으로 던지고 있습니다.

사용자에게 오류 응답을 전송하려면 제대로 된 오류의 원인을 명시하는 것이 좋을 것 같아서 원인이 DateTimeParseException 인 경우 응답 메시지를 별도로 지정해 주고 싶었어요.

하지만 실제로 던져지는 예외 자체는 HttpMessageNotReadableException 이기 때문에, HttpMessageNotReadableException에 대한 핸들러가 별도로 존재할 경우 DateTimeParseException에 대한 핸들러가 호출되지 않는 문제가 있었습니다.

아래와 같이 .getRootCause()로 가져온 후 instanceof로 검사하는 방법 외에는 방법을 찾지 못해 우선 이렇게 구현해 두었습니다. 다른 좋은 방법이 있을까요?

    @ExceptionHandler(HttpMessageNotReadableException.class)
    public ResponseEntity<ErrorResponse> handleHttpMessageNotReadableException(HttpMessageNotReadableException exception) {
        if (exception.getRootCause() instanceof DateTimeException) {
            return handleDateTimeParseException();
        }

        ErrorResponse data = new ErrorResponse(HttpStatus.BAD_REQUEST, "요청에 잘못된 형식의 값이 있습니다.");
        return ResponseEntity.badRequest().body(data);
    }

    private ResponseEntity<ErrorResponse> handleDateTimeParseException() {
        ErrorResponse data = new ErrorResponse(HttpStatus.BAD_REQUEST, "잘못된 형식의 날짜 혹은 시간입니다.");
        return ResponseEntity.status(HttpStatus.BAD_REQUEST).body(data);
    }

Domain/Entity 의 `equals()`

도메인 객체와 DB 엔티티를 아직까지는 분리할 필요성을 느끼지 못해 한 객체로 같이 사용하고 있습니다. 중복 예외 처리 중 객체 내부의 값이 모두 같은지를 비교해야 하는 소요가 있었는데요, equals()를 오버라이딩 하려니 엄연히 id라는 필드를 가지는 객체인데 id는 무시하고 값들의 집합으로 동등성을 판단하는 것이 어색하다고 생각했습니다.

우선은 아래와 같이 메서드를 별도로 선언하여 비교하도록 해 두었어요. 지금과 같은 구조에서 어떤 값을 기준으로 동등성 비교를 해야 할까요?

    public boolean isDuplicated(Reservation other) {
        return date.equals(other.date)
                && time.getId().equals(other.time.getId())
                && theme.getId().equals(other.theme.getId());
    }

ddaaac

안녕하세요 페드로 👋

구현 잘 해주셨습니다.

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

ddaaac · 2024-05-02T12:14:21Z

src/main/java/roomescape/controller/ReservationTimeController.java

@@ -23,6 +23,14 @@ public List<ReservationTimeResponse> readTimes() {
        return reservationTimeService.readReservationTimes();
    }

+    @GetMapping(params = {"date", "themeId"})


params는 생략 가능할 것 같아요.

현재 GET /times 에 대한 핸들러가 2개이고, 쿼리 파라미터의 여부에 따라서 다른 핸들러가 호출되도록 해 두었습니다.
이 경우에도 params = {} 부분의 생략이 가능한가요?

@RequestMapping("/times") public class ReservationTimeController { // ... @GetMapping // 쿼리스트링 없을 때 전체 조회 public List<ReservationTimeResponse> readTimes() { return reservationTimeService.readReservationTimes(); } @GetMapping(params = {"date", "themeId"}) // 쿼리스트링 있으면 필터링해서 조회 public List<ReservationTimeResponse> readTimes( @RequestParam(value = "date") LocalDate date, @RequestParam(value = "themeId") Long themeId ) { return reservationTimeService.readReservationTimes(date, themeId); }

생략할 경우 충돌로 인해 다음과 같은 오류가 발생합니다.

...: Ambiguous mapping. Cannot map 'reservationTimeController' method roomescape.controller.ReservationTimeController#readTimes(LocalDate, Long) to {GET [/times]}: There is already 'reservationTimeController' bean method

아이고 같은 경로의 API가 존재했군요. 죄송합니다 😅

ddaaac · 2024-05-02T12:16:39Z

src/main/java/roomescape/domain/Reservation.java

+
+    private void validateName(String name) {
+        if (name.isBlank()) {
+            throw new BadRequestException("이름은 공백일 수 없습니다.");


도메인에서 "Request"의 존재를 알아야할까요?

작성하면서도 고민을 했던 부분이였는데요, 처음에는 IllegalArgumentException을 던지도록 했었지만 Exception Handler 에서 내장 예외를 잡아버릴 경우 해당 예외의 상세 메시지가 클라이언트로 전달되게 됩니다.

개발자가 작성한 유효성 검증 로직이 실패하여 명시적으로 던져지는 내장 예외라면 큰 문제는 없지만, 내장 예외는 그 특성상 의도하지 않은 곳에서도 던져질 수 있기 때문에 예외에 포함되는 메시지를 클라이언트에게 그대로 노출시키는 것은 잠재적으로 위험하다고 생각했습니다.

물론 도메인에서는 IllegalArgumentException을 던지고, 사용하는 쪽에서 커스텀 예외로 컨버팅하여 던지는 것이 제일 좋겠지만 이 경우 해당 도메인을 생성할 때 마다 try~catch 문으로 감싸 줘야 하는 불편이 생깁니다.

위 사항들을 종합적으로 판단했을 때, 도메인 객체 역시 현재 프로젝트 내에 종속적인 객체라고 볼 수 있으므로 프로젝트에 전역적으로 사용할 커스텀 예외를 작성하고, 도메인에서 그 예외를 던지는 것이 크게 어색하지 않다고 판단하여 지금처럼 구현해 두었습니다.

혹시 더 좋은 방법이 있을까요?

전역적으로 사용할 거라면 request란 단어가 포함되는 것이 적절할까 생각해보면 좋을 것 같아요.

개인적으로는 객체의 네이밍을 할 때와 비슷하게 구체적인 예외 클래스를 사용하는 걸 선호하는 편입니다. 예외에도 경중이 있는데 모두 같은 예외를 사용하다보면 이를 나눠야 될 때 힘들더라구요.

클라이언트에서 웹서버로의 요청 뿐만 아니라, 객체 간의 메시지 전달 역시 request라고 볼 수 있다고 생각하여 request 라는 단어를 사용하는 데 큰 무리가 없다고 판단했었어요. 하지만 코드를 읽는 사람에 따라 어색하게 느껴질 수도 있겠네요🥲

확실히 예외가 발생한 이유에 따라 서로 다른 예외를 던지도록 해 두는 것이 유지보수 측면에서 좋을 것 같아요. BadRequestException 의 생성자는 protected로 변경하고, 이를 상속받은 구체 예외 클래스들을 던지도록 변경해 두었습니다!

ddaaac · 2024-05-02T12:21:28Z

src/main/java/roomescape/service/ThemeService.java

+        LocalDate end = LocalDate.now().minusDays(1L);
+        LocalDate start = end.minusDays(POPULAR_THEME_PERIOD);


요구사항의 인기 테마 예시를 보면 4월1일~4월7일이라고 되어있습니다.
지금은 어떤 결과가 나올까요?

날짜 계산을 잘못 생각 했네요😅 지금처럼 계산한다면 3월 31일 ~ 4월 6일의 인기 테마가 조회될 것 같아요.
수정 완료했습니다!

ddaaac · 2024-05-02T12:25:54Z

src/main/java/roomescape/service/ThemeService.java

+                .collect(Collectors.groupingBy(reservation -> reservation.getTheme().getId(), Collectors.counting()));
+    }
+
+    public ThemeResponse readTheme(Long id) {
+        Theme theme = themeRepository.findById(id)
+                .orElseThrow(() -> new BadRequestException("존재하지 않는 테마입니다."));
+        return ThemeResponse.from(theme);
+    }


reservation에서 Theme을 꺼내서 바로 사용하면 되지 않을까요? 다시 themeRepo를 통해 가져오는 이유가 있을까요?

놓쳤던 부분이네요..! 말씀하신 대로 Theme 테이블에 이중으로 접근할 필요가 없을 것 같아요.
Reservation 내에 있는 테마를 바로 꺼내서 사용하도록 수정하고, 테마를 기준으로 groupingBy를 사용하기 위해 Theme의 equals() 와 hashCode() 를 id값을 기준으로 오버라이드했습니다!

ddaaac · 2024-05-02T12:28:48Z

src/main/java/roomescape/controller/ThemeController.java

+    @GetMapping
+    public ResponseEntity<List<ThemeResponse>> readThemes() {
+        List<ThemeResponse> themes = themeService.readThemes();
+        return ResponseEntity.ok(themes);


개인적인 생각이지만 200을 줄거면 ResponseEntity를 주지 않아도 괜찮다고 생각해요.
조금이라도 간결하게 쓸 수 있으면 그 편이 낫지 않나 싶습니다.

저는 보통 통일성 측면에서 ResponseEntity로 감싸는 것을 선호하는 편이였습니다.

메서드의 시그니처에서 ResponseEntity 를 반환하고 있는 것을 보면 '아 이 메서드는 웹 요청에 응답하기 위한 메서드이구나'를 바로 파악할 수 있고, 한 RestController 내에서 반환하는 타입이 List, void, ResponseDto 등으로 모두 상이한 것은 어색하기도 하고 추상화 수준이 맞지 않는다고 생각했어요.
(심지어 헤더에 명시적으로 값을 넣어야 하거나, 200 이 아닌 응답을 해야 할 경우에는 한 컨트롤러 내에 ResponseEntity로 감싸진 반환값과 감싸지지 않은 반환값이 같이 존재하게 되니까요🥲)

직전 미션에서 작성했던 ReservationController의 경우, 요구사항에서 예약 추가 요청 시 200을 응답하게 되어 있기도 했었고 이번 미션 진행 시 페어와 자신의 코드 중 선택하여 진행하고, 지난 코드는 수정하지 않는다 라는 조건이 있어 ResponseEntity로 감싸지 않았습니다.

하지만 테마 추가 시에는 201 CREATED를 응답하도록 되어 있기도 하고, 반환값의 형태를 통일하는게 읽기 좋을 것 같다고 생각하여 모두 감싸 주었어요.

범블비는 한 컨트롤러 내의 메서드에서 ResponseEntity, List, DTO, void 등의 상이한 반환값을 가지고 있는 상태는 크게 신경쓰지 않으시는 것인지 궁금합니다!

#56 (comment)

좋은 의견 공유 감사합니다👍

ddaaac · 2024-05-02T12:31:29Z

src/main/java/roomescape/service/ThemeService.java

+    public void deleteTheme(Long id) {
+        if (reservationRepository.existsByThemeId(id)) {
+            throw new BadRequestException("해당 테마에 예약이 존재합니다.");
+        }
+        themeRepository.deleteById(id);
+    }


delete 동작의 초점을 어디에 맞출거냐에 따라서 예외처리를 하지 않는게 적당하고 생각할 수 있습니다.
delete를 호출한 이후 해당id에 해당하는 자원이 없어야한다고 하면 예외를 굳이 던질 필요는 없겠죠.

도메인 특성(예약) 상 삭제할 테마를 참조하는 예약이 존재할 경우, 마음대로 삭제돼서는 안 된다고 생각했습니다.

CASCADE와 같이 특정 테마에 대한 삭제 요청 시 모든 관련된 예약도 같이 삭제시킬수도 있지만, 그보다는 해당 테마가 운영되지 않을 예정임을 모든 고객들에게 알리고, 예약한 고객들의 예약을 먼저 취소하고 나서야 비로소 테마가 삭제되는 것이 자연스럽다고 판단했어요.

아이고... 죄송합니다.
페드로 리뷰 남길 때 개인적인 사정이 있어서 그런지 실수가 많네요... 😅

ddaaac · 2024-05-02T12:32:28Z

src/main/resources/static/js/time.js

@@ -129,6 +129,6 @@ function requestDelete(id) {

  return fetch(`${API_ENDPOINT}/${id}`, requestOptions)
      .then(response => {
-        if (response.status !== 204) throw new Error('Delete failed');
+        if (response.status !== 200) throw new Error('Delete failed');


여기 수정해주신 이유가 있나요? delete 응답이 204를 주는 것 같아서요.

@DeleteMapping("/{id}") public void deleteTime(@PathVariable Long id) { reservationTimeService.deleteTime(id); }

ReservationTimeController#deleteTime()은 현재 성공 시 200을 응답하는 것 같은데 혹시 이 부분을 말씀하시는게 맞을까요?

여기도 잘못 봤군요,,,
그런데 204 대신에 200으로 바꿔주신 이유가 있을까요?

ReservationTimeController는 지난 미션이였던 '방탈출 예약 관리'의 '7단계 - 시간 관리 기능' 에서 추가된 API에 대응하기 위해 만들어진 컨트롤러였는데요,

해당 API의 요구사항은 다음과 같았습니다.

request DELETE /times/1 HTTP/1.1 response HTTP/1.1 200

이번 미션에서는 시간 관리 API의 변경이 요구사항으로 명시되지 않은 상태라 이전의 요구사항을 그대로 가져가는 것이 맞다고 생각했는데, 정작 js 상에서는 204 를 검증하고 있어서 수정해 두었습니다!

ddaaac · 2024-05-02T12:37:18Z

src/test/java/roomescape/service/ReservationServiceTest.java

-        this.startAt = LocalTime.of(10, 10);
-        this.date = LocalDate.of(2024, 11, 16);
-        this.reservationFixture = new Reservation(id, name, date, new ReservationTime(id, startAt));
+        this.date = LocalDate.now().plusMonths(6);


여기 date는 fixture과 강하게 의존하고 있는 것 같은데요, 테스트에 필드로 가져가기 어색한 것 같네요.

Fixtures 클래스로의 픽스쳐 분리가 비교적 최근에 이루어져서, 이전에 작성했던 코드들 중 덜 이동된 필드들이 있었네요🥲 수정 완료해 두었습니다!

추가로 서비스 테스트에서 Service 객체를 초기화하고 픽스쳐 필드에 값을 설정하기 위한 @beforeeach 어노테이션을 삭제하고, @Injectmocks 를 사용하도록 변경하였습니다.

잘 구현해주셨네요.

참고로 그냥 @ 를 붙이면 멘션이 되기 때문에 @BeforeEach처럼 한 번 감싸주면 조금 더 편하게 코멘트 작성이 가능합니다.

매 번 멘션이 되길래 저 사람들은 계정을 노리고 만든 건가... 생각만 했었는데 멘션을 피하는 방법이 있었네요ㅋㅋㅋ 감사합니다!

ddaaac · 2024-05-02T12:44:02Z

src/main/java/roomescape/service/ReservationService.java

+    private void validateDuplicated(Reservation reservation) {
+        reservationRepository.findAll().stream()
+                .filter(reservation::isDuplicated)


두 가지를 얘기해볼 수 있을 것 같아요

단순히 findAll을 호출하는 건 위험합니다. 어플리케이션이 커질 때를 생각해보시면 좋을 것 같아요

다음은 조금 복잡한 얘기가 될 수 있을텐데요, id를 부여받지 않은 Reservation이 isDuplicate으로 비교되는 게 적당할까에 대해서도 고민해보시면 좋을 것 같습니다. 이건 넘기셔도 되구요.

애플리케이션이 커져서 저장된 예약이 많아진다면, 예약이 추가될 때 마다 전체 탐색을 하는 것은 확실히 위험한 선택이겠네요😅 SELECT EXISTS 쿼리를 활용하도록 변경하였습니다.

다음은 조금 복잡한 얘기가 될 수 있을텐데요, id를 부여받지 않은 Reservation이 isDuplicate으로 비교되는 게 적당할까에 대해서도 고민해보시면 좋을 것 같습니다. 이건 넘기셔도 되구요.

이 부분은 쿼리를 통한 검증으로 변경되면서 현재는 제거되기는 했지만, 처음 코드를 작성했을 때 부터 고민되던 부분이라 PR 작성 시 질문(3번째)으로 남겨두긴 했었습니다. 엔티티와 도메인을 같이 사용하면서 생긴 문제라고 생각하는데, 저런 고민이 된다면 둘을 분리해야 하는 시점으로 생각해야 하는 걸까요?

페드로가 생각하시는 엔티티와 도메인의 정의가 어떻게 되나요? 꼭 둘을 분리해야할까요?

원래의 엔티티는 식별자를 가지는 값 객체로서, 식별자를 통해 동등성을 비교할 수 있는 객체로 이해하고 있습니다.

이를 DB 엔티티로 범위를 좁히면 DB 테이블과 연관된 POJO로, 테이블에서 하나의 열을 표현할 수 있는 객체라고 할 수 있을 것 같아요.
도메인은 시스템의 핵심적인 비즈니스 로직과 데이터를 관리하는 객체입니다.

일반적으로 도메인에 포함된 데이터를 (형태 변환 없이) DB에 그대로 영속화하기 때문에, 엔티티와 도메인을 현재 단계에서 꼭 구분해야 할 필요는 없다고 생각합니다.

이번 미션에서 제 코드를 사용하지는 않았지만, 저는 실제로 직전 미션에서 모든 계층의 DTO와 엔티티, 도메인을 전부 분리시켜 초기 설계를 진행했었는데요. 분리에서 얻는 장점보다 유지보수가 어려워지고 요구사항 변경 시 수정해야 할 부분이 프로젝트 여기저기에 산재되게 된다는 단점이 더 크게 다가와 엔티티와 도메인을 통합하여 사용하는 것이 더 나을 것 같아는 결론을 내렸습니다.

다만 위에서 질문드린 것처럼 DB에서 생성된 id를 가지고 있는 엔티티와, id가 꼭 필요하지는 않은 도메인 객체를 통합하다 보니 동등성 비교를 어떤 값을 기준으로 해야 하는 것인지는 아직 명쾌한 답을 찾지 못했습니다. id 필드가 존재하는 것은 자명하므로, equals 오버라이딩은 이를 기준으로 해야 하는 것이 타당한데, 경우에 따라 객체의 값이 전부 같은지를 검사해야 하는 문맥이 전혀 없지만은 않았던 것 같아요.

~~아직 제대로 갈피를 잡지 못한 것 같아서, 조금만 더 힌트를 얻을 수 있다면 감사하겠습니다..ㅎㅎ~~

id가 부여된 객체의 동등성은 id로 해야겠죠. 코멘트를 남긴 의도는 exist 쿼리 등을 활용해서 존재하면 안 될 객체는 생성하지 않는 게 나을 것 같다는 의도였습니다. 잘 변경해주신 것 같네요.

앗 밑에 있는 요 답변을 이 코멘트를 단 이후에 봤네요..! 어느 정도 해결이 된 것 같아요 🙂

분리에서 얻는 장점보다 유지보수가 어려워지고 요구사항 변경 시 수정해야 할 부분이 프로젝트 여기저기에 산재되게 된다는 단점

잘 봐주셨네요. 프로젝트의 성격이나 크기에 따라서 얼마든지 생각이 바뀔 수 있겠죠. 유연하게 사고해주시면 될 것 같아요!

…on에서 테마 정보를 가져와서 사용하도록 변경

@Injectmocks

- 픽스쳐 값 설정과 Service 객체 초기화를 위해 사용하던 테스트 클래스의 @beforeeach 제거 - Service 객체 초기화를 위해 @Injectmocks 사용

…t()를 사용하도록 변경 - size 검증 시 hasSize()를 사용하도록 변경

hw0603 · 2024-05-07T08:49:45Z

안녕하세요 범블비! 말씀하신 부분들 반영하여 다시 리뷰 요청 드립니다🙂
범블비의 생각이 좀 더 듣고 싶은 부분들은 질문으로 남겨 두었어요
리뷰 잘 부탁 드립니다!

ddaaac

안녕하세요 페드로 👋

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

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

ddaaac · 2024-05-07T11:23:17Z

src/main/java/roomescape/controller/ReservationTimeController.java

@@ -23,6 +23,14 @@ public List<ReservationTimeResponse> readTimes() {
        return reservationTimeService.readReservationTimes();
    }

+    @GetMapping(params = {"date", "themeId"})


아이고 같은 경로의 API가 존재했군요. 죄송합니다 😅

ddaaac · 2024-05-07T11:28:01Z

src/main/java/roomescape/domain/Reservation.java

+
+    private void validateName(String name) {
+        if (name.isBlank()) {
+            throw new BadRequestException("이름은 공백일 수 없습니다.");


전역적으로 사용할 거라면 request란 단어가 포함되는 것이 적절할까 생각해보면 좋을 것 같아요.

개인적으로는 객체의 네이밍을 할 때와 비슷하게 구체적인 예외 클래스를 사용하는 걸 선호하는 편입니다. 예외에도 경중이 있는데 모두 같은 예외를 사용하다보면 이를 나눠야 될 때 힘들더라구요.

ddaaac · 2024-05-07T11:30:18Z

src/main/java/roomescape/controller/ThemeController.java

+    @GetMapping
+    public ResponseEntity<List<ThemeResponse>> readThemes() {
+        List<ThemeResponse> themes = themeService.readThemes();
+        return ResponseEntity.ok(themes);


#56 (comment)

ddaaac · 2024-05-07T11:32:13Z

src/main/java/roomescape/service/ThemeService.java

+    public void deleteTheme(Long id) {
+        if (reservationRepository.existsByThemeId(id)) {
+            throw new BadRequestException("해당 테마에 예약이 존재합니다.");
+        }
+        themeRepository.deleteById(id);
+    }


아이고... 죄송합니다.
페드로 리뷰 남길 때 개인적인 사정이 있어서 그런지 실수가 많네요... 😅

ddaaac · 2024-05-07T11:33:59Z

src/main/java/roomescape/service/ReservationTimeService.java

+    public ReservationTimeResponse createTime(ReservationTimeCreateRequest request) {
+        ReservationTime reservationTime = request.toReservationTime();
+
+        validateDuplicated(reservationTime);
+
+        ReservationTime savedReservationTime = reservationTimeRepository.save(reservationTime);


멀티 스레드 환경에서 같은 time을 add하는 요청이 여러번 빠르게 올 경우 이 검증 로직을 통과할 수 있습니다.
이런 경우는 db에 unique 인덱스를 넣어줄 수 있을 것 같네요.

이번 미션에서 고려할만한 내용은 아니지만 언젠가 한번쯤은 겪을 수 있는 에러라 서비스 단의 검증 로직은 항상 이런 부분을 생각하면 좋을 것 같아서 남긴 코멘트입니다.

동시성 문제가 생기는군요.. 큰 고민 없이 넘어갔을 수도 있는 부분인데 잘 짚어주셔서 감사합니다😊

제안해 주신 방법이 UNIQUE 제약 조건을 걸어주고, 서비스에서는 별도의 validation을 하지 않다가 DB단에서 UNIQUE constraint violation 이 발생했을 때 던져지는 예외를 잡아 사용자에게 실패를 알리는 방식이 맞을까요?

두 개 다 존재하면 좋을 것 같네요. 로직이 서비스 단에 있으면 해당 값의 unique 조건을 좀 더 파악하기 쉬울거구요. 로직이 더 진행되기 전에 미리 검증을 통해 뒤에 불필요한 로직이 실행될 필요도 없을 것 같아요.
서비스 로직이 간혹 제대로 동작하지 않는 경우도 있으니 DB에도 제약조건을 걸어주고요.

ddaaac · 2024-05-07T11:37:58Z

src/test/java/roomescape/service/ReservationServiceTest.java

-        this.startAt = LocalTime.of(10, 10);
-        this.date = LocalDate.of(2024, 11, 16);
-        this.reservationFixture = new Reservation(id, name, date, new ReservationTime(id, startAt));
+        this.date = LocalDate.now().plusMonths(6);


잘 구현해주셨네요.

참고로 그냥 @ 를 붙이면 멘션이 되기 때문에 @BeforeEach처럼 한 번 감싸주면 조금 더 편하게 코멘트 작성이 가능합니다.

ddaaac · 2024-05-07T11:46:09Z

src/main/java/roomescape/service/ThemeService.java

+    private Map<Long, Long> collectReservationByTheme(LocalDate start, LocalDate end) {
+        return reservationRepository.findByDateBetween(start, end).stream()
+                .collect(Collectors.groupingBy(reservation -> reservation.getTheme().getId(), Collectors.counting()));
+    }


이번 미션에서는 1. 날짜 구간 검색 만 DB에서 처리하고 나머지는 애플리케이션이 처리하도록 해 두었는데요, 범블비는 특정 요구사항의 구현을 쿼리를 통해 처리할지, 애플리케이션의 로직으로 처리할지를 판단하는 기준이 무엇이라고 생각하시나요?

혹시 이 질문 제가 놓쳤던 걸까요? 그렇다면 또 한 번 죄송합니다...

이건 그때그때 다르겠지만, 저도 쿼리는 최대한 간단하게 가져가려고 하는 편입니다. 아마도 between(date, date) 같은 쿼리는 인덱스를 탈 수 있을텐데요, 그 정도만 인덱스를 통해 빠르게 검색하고 또 이 정도면 충분히 데이터 사이즈도 줄어서 어플리케이션에 넘어올 거기 때문에 적당해보여요. 웬만하면 로직은 도메인에 두려고 하는 편입니다.

아닙니다ㅎㅎ 답변 감사드려요!

ddaaac · 2024-05-07T11:49:09Z

src/main/java/roomescape/exception/GlobalExceptionHandler.java

+    public ResponseEntity<ErrorResponse> handleHttpMessageNotReadableException(HttpMessageNotReadableException exception) {
+        if (exception.getRootCause() instanceof DateTimeException) {
+            return handleDateTimeParseException();
+        }


아래와 같이 .getRootCause()로 가져온 후 instanceof로 검사하는 방법 외에는 방법을 찾지 못해 우선 이렇게 구현해 두었습니다. 다른 좋은 방법이 있을까요?

넵 잘 구현해주셨어요. 도메인 로직을 구현할 때는 instanceof 가 발생하면 설계를 고민해볼 수 있겠지만, 스프링과 같은 프레임워크/라이브러리를 사용할 땐 instanceof를 사용해야할 때도 있습니다.

ddaaac · 2024-05-07T11:51:46Z

src/main/java/roomescape/domain/Reservation.java

+
+import java.time.LocalDate;
+
+public class Reservation {


public boolean isDuplicated(Reservation other) { return date.equals(other.date) && time.getId().equals(other.time.getId()) && theme.getId().equals(other.theme.getId()); }

Reservation에서는 이런 코드를 찾지 못했는데 혹시 어디를 말씀하시는걸까요?
아니면 exist 쿼리로 바꾸면서 제거된 부분일까요?

id가 부여된 객체의 동등성은 id로 해야겠죠. 코멘트를 남긴 의도는 exist 쿼리 등을 활용해서 존재하면 안 될 객체는 생성하지 않는 게 나을 것 같다는 의도였습니다. 잘 변경해주신 것 같네요.

아니면 exist 쿼리로 바꾸면서 제거된 부분일까요?

넵 맞습니다! EXIST 쿼리를 활용하는 형태로 변경하면서 사라진 부분이였어요.

처음에는 EXIST 와 같은 쿼리도 '기능'을 DB에 의존적인 쿼리로 해결하는 부분이라고 생각하여 최대한 지양하려고 했는데, 범블비의 코멘트를 보고 생각이 조금 바뀌었습니다.

어차피 도메인의 영속화를 위해 관계형 DB를 사용하기로 결정했다면, DB에서 제공하는 기능들을 잘 활용하는 것도 DB를 DB답게 사용하는 일인 것 같아요. 물론 말씀해 주신 대로 로직과 관련된 부분을 최대한 도메인에 둬야 한다는 것에는 백번 동의합니다👍

ddaaac · 2024-05-07T11:55:16Z

src/main/java/roomescape/service/ReservationService.java

+    private void validateDuplicated(Reservation reservation) {
+        reservationRepository.findAll().stream()
+                .filter(reservation::isDuplicated)


페드로가 생각하시는 엔티티와 도메인의 정의가 어떻게 되나요? 꼭 둘을 분리해야할까요?

ddaaac

안녕하세요 페드로 👋

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

다음 단계 진행해주세요!

ddaaac · 2024-05-08T12:47:45Z

src/main/java/roomescape/service/ReservationTimeService.java

+    public ReservationTimeResponse createTime(ReservationTimeCreateRequest request) {
+        ReservationTime reservationTime = request.toReservationTime();
+
+        validateDuplicated(reservationTime);
+
+        ReservationTime savedReservationTime = reservationTimeRepository.save(reservationTime);


두 개 다 존재하면 좋을 것 같네요. 로직이 서비스 단에 있으면 해당 값의 unique 조건을 좀 더 파악하기 쉬울거구요. 로직이 더 진행되기 전에 미리 검증을 통해 뒤에 불필요한 로직이 실행될 필요도 없을 것 같아요.
서비스 로직이 간혹 제대로 동작하지 않는 경우도 있으니 DB에도 제약조건을 걸어주고요.

ddaaac · 2024-05-08T12:49:40Z

src/main/java/roomescape/service/ReservationService.java

+    private void validateDuplicated(Reservation reservation) {
+        reservationRepository.findAll().stream()
+                .filter(reservation::isDuplicated)


분리에서 얻는 장점보다 유지보수가 어려워지고 요구사항 변경 시 수정해야 할 부분이 프로젝트 여기저기에 산재되게 된다는 단점

잘 봐주셨네요. 프로젝트의 성격이나 크기에 따라서 얼마든지 생각이 바뀔 수 있겠죠. 유연하게 사고해주시면 될 것 같아요!

ddaaac · 2024-05-08T12:51:33Z

src/main/java/roomescape/repository/reservation/ReservationDao.java

+    @Override
+    public List<Reservation> findAll() {
+        String sql = """
+                SELECT
+                    r.id AS reservation_id,


이런 쿼리는 장기적으로 봤을 때 위험할 수 있습니다. 예약은 줄어들지 않고 늘기만 하기 때문에 슬로우 쿼리가 될 가능성이 높습니다. 로직을 수정해보시면 좋을 것 같아요.

findAll()의 존재 자체에 초점을 두고 하시는 말씀이실까요? 아니면 테이블 3개를 JOIN 하는 부분을 말씀하시는걸까요?

고민을 조금 길게 해 봤는데 전체 예약 목록 리스트를 페이지 단위로 나누어 특정 부분만 조회하거나 무한 스크롤 형태로 구현하는 것 말고는 잘 생각이 나지 않아요🥲

혹시 후자를 말씀하시는 거라면 지금과 같은 테이블 구조에서 N개 테이블을 조인하지 않고 온전한 Reservation을 가져오는 것이 가능한지 잘 모르겠습니다. 어떤 형태의 로직 개선인지 힌트를 조금 받을 수 있을까요?

ddaaac · 2024-05-08T12:54:48Z

src/test/java/roomescape/service/ReservationServiceTest.java

+@ExtendWith(MockitoExtension.class)
+@DisplayName("예약 서비스")
+class ReservationServiceTest {
+    @InjectMocks
+    private ReservationService reservationService;


Mockito를 사용하지 않고 이 테스트를 구현하려면 어떻게 해야할까요?

이미 각 Repository들이 인터페이스 형태로 되어 있으므로, Fake 객체를 만드는 데 크게 어려움이 없을 것 같아요.

Mockito를 사용하지 않는다면 DB에서 제공하는 기능들을 간단하게 구현한 Fake 객체를 활용해 볼 수 있을 것 같습니다!

eunjungL and others added 30 commits April 30, 2024 15:59

feat: 어드민 코드 이전

27cbcb0

Co-authored-by: Hyunguk Ryu <[email protected]>

docs(README): 1단계 요구사항 추가

47665fb

Co-authored-by: Hyunguk Ryu <[email protected]>

feat(ReservationService): 지난 시점에 대한 예약 예외 처리 구현

319e6c8

Co-authored-by: Hyunguk Ryu <[email protected]>

remove(/test/repository): 사용하지 않는 FakeDao 제거

5f6f8ff

Co-authored-by: Hyunguk Ryu <[email protected]>

fix(ReservationControllerTest): 테스트 시 항상 미래의 날자를 사용하도록 수정

4d62cca

- createInitReservation() 오타 수정 Co-authored-by: Hyunguk Ryu <[email protected]>

feat(ReservationService): 중복된 예약 예외처리 구현

63bec82

Co-authored-by: Hyunguk Ryu <[email protected]>

feat(Reservation): 예약 생성 시 예약자명이 공백일 경우 예외 처리 구현

1a350aa

Co-authored-by: Hyunguk Ryu <[email protected]>

feat(GlobalExceptionHandler): BadRequest handler 추가

bbeb00a

Co-authored-by: Hyunguk Ryu <[email protected]>

feat(GlobalExceptionHandler): 유효하지 않은 날짜 또는 시간에 대한 예외 처리 구현

4ccbd4b

Co-authored-by: Hyunguk Ryu <[email protected]>

test(/test/controller): 잘못된 형식의 본문 요청 시 예외 처리 구현

01a4652

Co-authored-by: Hyunguk Ryu <[email protected]>

test(ReservationServiceTest): 등록되지 않은 시간에 대한 예약 생성 테스트 코드 추가

c677f67

Co-authored-by: Hyunguk Ryu <[email protected]>

feat(ReservationTimeService): 중복된 예약 시간 생성 예외 처리 구현

425c94f

Co-authored-by: Hyunguk Ryu <[email protected]>

feat(ReservationTimeService): 시간 삭제 시 참조된 예약이 있을 때 예외 처리 구현

4c3b7a1

Co-authored-by: Hyunguk Ryu <[email protected]>

docs(README): 2단계 요구사항 추가

b19bdd5

Co-authored-by: Hyunguk Ryu <[email protected]>

feat(AdminController): 테마 목록 페이지 응답 구현

fbd7f15

Co-authored-by: Hyunguk Ryu <[email protected]>

feat(AdminController): 예약 시 테마를 지정할 수 있는 예약 페이지 응답 구현

be0322f

Co-authored-by: Hyunguk Ryu <[email protected]>

feat(ThemeController): 테마 추가 기능 구현

a19edca

Co-authored-by: Hyunguk Ryu <[email protected]>

feat(ThemeController): 테마 조회 기능 구현

92bc053

Co-authored-by: Hyunguk Ryu <[email protected]>

feat(ThemeController): 테마 삭제 구현

d2af8ac

Co-authored-by: Hyunguk Ryu <[email protected]>

feat(ReservationController): 방탈출 예약 시 테마 정보 포함하는 예약 기능 구현

41f4b4f

Co-authored-by: Hyunguk Ryu <[email protected]>

feat(ReservationService): 동시간대 예약 시 예외 처리 구현

4e5a8ab

Co-authored-by: Hyunguk Ryu <[email protected]>

feat(ThemeService): 중복된 이름의 테마 추가 시 예외 처리 구현

7034dfc

Co-authored-by: Hyunguk Ryu <[email protected]>

feat(Theme): 테마 추가 시 이름, 설명, 썸네일 중 하나라도 빈 경우 예외 처리 구현

35294cb

Co-authored-by: Hyunguk Ryu <[email protected]>

feat(ThemeService): 테마 삭제 시 참조된 예약이 있을 때 예외 처리 구현

0423fa0

- Table reset sql 통합 Co-authored-by: Hyunguk Ryu <[email protected]>

fix(js): js 코드 API 명세에 맞게 수정

a4ef28d

Co-authored-by: Hyunguk Ryu <[email protected]>

docs(README): 3단계 기능 요구사항 추가

41587b5

Co-authored-by: Hyunguk Ryu <[email protected]>

feat(UserController): 사용자 예약 페이지 구현

5c7b024

Co-authored-by: Hyunguk Ryu <[email protected]>

feat(ThemeController): 지난 일주일 간 인기 테마 조회 구현

2cff101

Co-authored-by: Hyunguk Ryu <[email protected]>

test(ThemeServiceTest): 인기 테마 조회 테스트 구현

2936539

Co-authored-by: Hyunguk Ryu <[email protected]>

docs(README): 누락된 3단계 요구사항 추가

3d4ab2b

Co-authored-by: Hyunguk Ryu <[email protected]>

eunjungL and others added 6 commits May 2, 2024 11:36

feat(ReservationTimeController): 선택된 테마에 따라 예약 가능 여부가 포함된 시간 조회 기능 구현

7b2c065

Co-authored-by: Hyunguk Ryu <[email protected]>

refactor(ReservationTimeController): 쿼리 파라미터에 따른 핸들러 분리

2a6e14d

Co-authored-by: Hyunguk Ryu <[email protected]>

refactor: 패키지 구조 정리

f0fcb30

Co-authored-by: Hyunguk Ryu <[email protected]>

refactor: 메서드 정의 순서 변경

c90785b

- RowMapper 메서드 static화 Co-authored-by: Hyunguk Ryu <[email protected]>

refactor(ThemeService): 테마별 예약 횟수 분류 메서드 분리

c9923d0

- 인기 테마 조회 시 조회 조건(일자, 개수) 상수화 Co-authored-by: Hyunguk Ryu <[email protected]>

refactor(/test): 테스트 픽스쳐 분리

4a8bc9b

Co-authored-by: Hyunguk Ryu <[email protected]>

hw0603 marked this pull request as draft May 2, 2024 08:20

hw0603 marked this pull request as ready for review May 2, 2024 08:38

ddaaac requested changes May 2, 2024

View reviewed changes

hw0603 added 9 commits May 7, 2024 10:59

fix(ReservationTimeController): GetMapping에 누락된 / 문자 추가

6937af0

fix(ThemeService): 인기 테마 조회 시 날짜 범위 오류 수정

7a6a868

refactor(ThemeService): 인기 테마 조회 시 ThemeRepository에 접근하지 않고 Reservati…

aade4c3

…on에서 테마 정보를 가져와서 사용하도록 변경

refactor(ReservationService): 중복 예약 검증 시 전체탐색하지 않고 쿼리를 통해 검증하도록 변경

9f46f9c

refactor(Dao): RowMapper를 static 변수로 변경

ac1adb5

refactor(ReservationDao): 존재 여부 확인 메서드들에서 SELECT EXIST() 를 사용하도록 변경

630ff06

fix(ThemeControllerTest): 인기 테마 조회 요청 테스트 시 잘못 지정된 path 수정

2f628e4

refactor(/test): 테스트에서 사용되는 픽스쳐들을 모두 Fixtures 클래스로 이동

07f4c95

- 픽스쳐 값 설정과 Service 객체 초기화를 위해 사용하던 테스트 클래스의 @beforeeach 제거 - Service 객체 초기화를 위해 @Injectmocks 사용

refactor(/test): Optional 값 검증 시 .isPresent().isTrue() 가 아닌 .isPresen…

7834789

…t()를 사용하도록 변경 - size 검증 시 hasSize()를 사용하도록 변경

ddaaac requested changes May 7, 2024

View reviewed changes

refactor(/exception): 예외 클래스 세분화

0fc2cdd

ddaaac approved these changes May 8, 2024

View reviewed changes

ddaaac merged commit eea4a8f into woowacourse:hw0603 May 8, 2024

This was referenced May 8, 2024

[1 - 3단계 방탈출 사용자 예약] 카고(정민호) 미션 제출합니다. #38

Merged

[4 - 6단계 방탈출 사용자 예약] 미아(이종미) 미션 제출합니다. #92

Merged

		LocalDate end = LocalDate.now().minusDays(1L);
		LocalDate start = end.minusDays(POPULAR_THEME_PERIOD);

[1 - 3단계 방탈출 사용자 예약] 페드로(류형욱) 미션 제출합니다. #33

[1 - 3단계 방탈출 사용자 예약] 페드로(류형욱) 미션 제출합니다. #33

Conversation

hw0603 commented May 2, 2024 • edited Loading

이전의 웹 개발 경험

hw0603 commented May 2, 2024 • edited Loading

DB의 기능을 어느 정도까지 활용해도 되는 것인가?

Exception Handler 위계

Domain/Entity 의 equals()

ddaaac 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

Choose a reason for hiding this comment

hw0603 May 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hw0603 May 7, 2024 • edited Loading

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

hw0603 May 4, 2024 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hw0603 May 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hw0603 commented May 7, 2024

ddaaac 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

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

Choose a reason for hiding this comment

ddaaac 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hw0603 commented May 2, 2024 •

edited

Loading

hw0603 commented May 2, 2024 •

edited

Loading

Domain/Entity 의 `equals()`

hw0603 May 7, 2024 •

edited

Loading

hw0603 May 7, 2024 •

edited

Loading

hw0603 May 4, 2024 •

edited

Loading

hw0603 May 7, 2024 •

edited

Loading