[1 - 3단계 방탈출 사용자 예약] 이든(최승준) 미션 제출합니다. #57

PgmJun · 2024-05-02T08:40:58Z

안녕하세요 웨지! 이든입니다 🙌
첫 리뷰요청이네요! 리뷰 잘 부탁드리겠습니다:)
DM에서 말씀하신대로 변경에 관련된 내용 또는 질문은 코멘트로 남겨놓겠습니다 감사합니다~!

리뷰는 이전 미션 코드를 제외한 여기를 이용해주시면 될 것 같습니다:)

상황공유 💭

이번 미션 구현하는 과정에서 제 코드가 아닌 페어의 코드를 이어받아 진행했습니다.
때문에 제가 작성하지 않은 코드가 많이 존재해서(특히 테스트 코드) 코드에 대한 이해도나 근거가 부족할 수 있습니다.
이 부분에 대해서는 양해부탁드립니다 😭

Co-authored-by: PgmJun <chltmdwns96@gmail.com> Co-authored-by: J-I-H-O <jeongjiho0731@gmail.com>

그저 String을 return하는 방식은 이후 응답 데이터 확장 설계를 할 때 클라이언트의 응답값 흭득 방식에 변경이 발생해야하기 때문에 break change가 발생 때문에 요청 정보/에러 정보를 담은 응답 객체(ApiResponse)를 두어 사용

sihyung92

안녕하세요 이든~ 리뷰어 웨지입니다!
리뷰 반영 잘 해주셨어요 ㅎ
추가 리뷰 남겨두었으니 확인해주세요.
감사합니다!

sihyung92 · 2024-05-06T03:03:07Z

src/main/java/roomescape/global/dto/response/ApiResponse.java

+import roomescape.global.exception.error.ErrorType;
+
+public record ApiResponse<T>(
+        boolean isSuccess,


꼭 필요한가? 싶긴 한 필드입니다 ㅎ
success = true인 상황은 항상 2xx이고, 반대는 4xx나 5xx인 상황이라서요
오히려 2xx일 때 false거나 4xx일 때 true로 내려오면 더 헷갈릴거 같네요

우선 저는 웨지와 함께 코드를 만들어가는 중이고
웨지가 복잡성을 느끼셨기 때문에 isSuccess 필드를 제거하긴 하였습니다.

하지만
HTTP API 스팩은 지켜서 요청을 보냈지만, 내부 검증 과정에서 문제가 발생한 경우 400 에러를 보내게 되면
“너가 보낸 요청이 정해둔 Api Spec과 달라” 라고 전달하는 느낌이기 때문에
요청이 애플리케이션에 도달한 경우 200을 Response 해주되, 내부에서 isSuccess 는 false로 전달하여
요청 방식에는 문제가 없지만 내부적으로 조건이 충족되지 않음을 표현해준다는 김영한님의 글에도 공감이 갔었기에
이 부분에 관해서 웨지는 어떻게 생각하시는지 의견을 듣고싶었어요!

사실 클라이언트와 소통할 수 있었다면 함께 편한 방식을 정하면 끝이었겠지만
서버와 클라이언트를 다 다루고 있으니 처리방식이 고민되었던 것 같네요

내용을 길게 설명하느라 핵심적인 궁금 포인트가 빠졌습니다..!
결론적으로 궁금한 내용은
”평균적으로 어떤 방식이 더 클라이언트에 혼란을 주지 않는 방식일까?“ 였습니다.

sihyung92 · 2024-05-06T03:05:41Z

src/main/java/roomescape/global/exception/model/ConflictException.java

-    public ConflictException() {
-        super();
-    }
+public class ConflictException extends CustomException {


ValidateException과 ConflictException의 차이점을 잘 모르겠습니다.
다른 개발자가 다른 비즈니스를 개발하다가 둘 중 하나를 골라야한다면 언제 어떤 오류를 선택해야할까요?

그리고 이걸 매번 설명하는 것보다 좀더 분명한 의미의 오류명이면 더 좋겠죠.

값 검증 관련 예외는 ValidateException,
데이터 충돌(이미 존재하는 데이터를 다시 추가, 특정 데이터에 FK로 연관되있는 데이터를 삭제하려함) 관련 예외는 ConflictException으로 구분해보았는데
가독성이 많이 저하되어 보이는군요 🥲

우선 DataConflictException으로 변경해보았는데 의미가 조금은 분명해졌으려나요?

sihyung92 · 2024-05-06T03:07:59Z

src/main/java/roomescape/global/exception/error/ErrorType.java

+    // 400 Bad Request
+    INVALID_ERROR("유효하지 않은 값입니다."),
+
+    // 409 Conflict
+    TIME_IS_USED_CONFLICT("삭제할 수 없는 시간대입니다."),
+    TIME_DUPLICATION_CONFLICT("이미 해당 시간이 존재합니다."),
+    RESERVATION_DUPLICATION_CONFLICT("이미 예약이 존재합니다."),
+    RESERVATION_PERIOD_CONFLICT("이미 지난 시간대는 예약할 수 없습니다."),
+
+    // 400 Bad Request
+    BAD_REQUEST("요청 형식이 잘못되었습니다."),


같은 코드끼리 뭉쳐놓는 것 아니었나요? INVALID_ERROR 와 BAD_REQUEST는 내외하는 이유가 있나요? ㅎㅎ

앗 이 부분은 실수입니다..! 처리해두겠습니다 감사합니다!

sihyung92 · 2024-05-06T03:10:58Z

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

        this.id = id;
        this.name = name;
        this.date = date;
        this.time = time;
        this.theme = theme;
+
+        if (StringUtils.isBlank(name) || date == null || time == null || theme == null) {
+            throw new ValidateException(ErrorType.INVALID_ERROR, String.format("유효하지 않은 값입니다.%n%s", this.toString()));


너무 모호한 오류명입니다. 어떤 값이 왜 유효하지 않은지 설명해줘야하는데, 맥락없이 유효하지 않은 값이라고 하면 사용자에게 불친절한 API가 됩니다.
불친절한 API는 서버개발자에게 직통 전화로 연결되는 결과로 이어지니, 24시간 유선연락 대기조를 하고 싶은 게 아니시라면 가급적 친절한 API를 구성하시길 권합니다

보안적인 부분을 생각한다는 것이 불친절한 응답값으로 이어졌네요..

24시간 유선연락 대기조를 하고 싶은 게 아니시라면 가급적 친절한 API를 구성하시길 권합니다

이 내용을 기반으로 연락이 오지 않을 정도의 정보를 넘겨주도록 최대한 개선해보겠습니다!

sihyung92 · 2024-05-06T03:12:24Z

src/main/java/roomescape/global/dto/response/ApiResponse.java

+    }
+
+    public static <T> ApiResponse<T> fail(final ErrorType errorType) {
+        return new ApiResponse<>(false, errorType.getDescription(), null);


이렇게 처리되면 유저에겐 errorType의 description만 내려가게 되는데, errorType description이 범용적으로 구성되어있어 클라이언트에게 실질적인 내용을 전달하지 못합니다.
오류 로깅용 메시지와 클라이언트 오류 응답용 메시지를 구분하는 것은 좋지만 클라이언트가 너무 불편해지지 않도록 신경써주세요.

서버의 어떤 데이터가 존재하지 않고, 왜 실패했는 지 등의 정보를 노출하는 것이 보안에 좋지 않다는 이야기를 많이 들어서
이렇게 처리하게 되었네요..

요청 실패 시, 클라이언트 측에 어느정도까지 정보를 노출할 지에 대한 기준을 잘 정하는 것이 아직은 어려운 것 같습니다.
우선 클라이언트에게 조금 더 친절한 정보를 내려주도록 변경해보겠습니다!

그리고 클라이언트의 요청 실패 시, 클라이언트에게 어느정도까지 정보를 내려줘야하는지에 대한 웨지의 기준이 궁금합니다!

프론트 입장을 겪어보시면 좋은데요, 실제로 happy case에서 조금만 수틀리면 500 내려버리거나 400 이라면서 설명은 없는 API들을 많이 만나게 됩니다 ㅋㅋ
이러면 서버 개발자를 향한 적개심이 차오르게 되는데요, 엄청 까칠한 질문 DM("500 나는데 확인좀요..")을 받고 싶으신게 아니라면 상대가 validation 실패 시 뭐가 필요할지 생각해보세요
보내주지 않은 필드, 잘못된 필드, 정책을 지키지 못한 경우 등을 친절하게 응답해주면 될겁니다

이 부분을 다 수정해서 보내면 "정상적인 API 요청" 이 될텐데요, 이게 보안상 문제가 될까요?
보안의 문제가 되는 것은 내 서버가 어떤 스택으로 구성되어있는지 오류메시지로 힌트를 줘버리거나, 응답값으로 개인정보가 새나가는 구멍은 없는지 등입니다

보안의 문제가 되는 것은 내 서버가 어떤 스택으로 구성되어있는지 오류메시지로 힌트를 줘버리거나, 응답값으로 개인정보가 새나가는 구멍은 없는지 등입니다

제가 너무 가벼운 내용까지 감춰야한다고 생각했나봐요.
웨지 덕분에 인사이트를 많이 얻어가는 것 같습니다 :)

sihyung92 · 2024-05-06T03:22:15Z

src/main/java/roomescape/controller/ReservationController.java


-        return ResponseEntity.created(URI.create("/reservations/" + reservationResponse.id()))
-                .body(reservationResponse);
+        response.setHeader("Location", "/reservations/" + reservationResponse.id());


Location이라는 변수명이 실수할 여지가 있어서 수정 전 방식대로 활용하시는 것도 좋다고 생각합니다 ㅎ

이런 방법도 있습니다.

@PostMapping("/reservations") public ResponseEntity<ApiResponse<ReservationResponse>> saveReservation( @RequestBody final ReservationRequest reservationRequest ) { ReservationResponse reservationResponse = reservationService.addReservation(reservationRequest); URI location = ServletUriComponentsBuilder.fromCurrentRequest() .path("/{id}") .buildAndExpand(reservationResponse.id()) .toUri(); return ResponseEntity.created(location).body(ApiResponse.success(reservationResponse)); }

지금처럼 일반 문자열로 관리하면 충분히 실수할 여지가 있을 것 같습니다..!
예시로 첨부해주신 내용도 너무 좋은 것 같습니다!

그런데 로직이 너무 길어지는 느낌이 있어서
혹시 Location이라는 문자열을 실수할 여지가 있는 부분에 대해
미리 정의되어있는 HttpHeaders.LOCATION 을 활용해서 해결하는 방식은 어떤가요??

response.setHeader(HttpHeaders.LOCATION, "/reservations/" + reservationResponse.id());

이렇게 변경해볼 수 있을 것 같습니다!

네네 우려하는 부분은 매직넘버이니 좋은 처리입니다 ㅎ

sihyung92 · 2024-05-06T03:25:16Z

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

+        LocalDate today = LocalDate.now();
+        LocalDate startDate = today.minusDays(7);
+        LocalDate endDate = today.minusDays(1);
+


이후 코드리뷰 중 themes/top API를 봤는데요, 해당 API의 경우 말씀하신것처럼 특정 용도에만 사용하도록 생성한 API이므로 너무 다른 요구사항을 많이 받아들이는 건 안 좋습니다 (사용자가 헷갈립니다)

직전 코멘트는 themes/search 처럼 범용적인 API를 구축한 경우에 해당해 말씀드렸습니다

sihyung92 · 2024-05-06T03:31:54Z

src/main/java/roomescape/dao/TimeDao.java

+                SELECT
+                    rt.id, start_at, IFNULL(is_reserved, 'False') AS is_reserved
+                FROM reservation_time rt
+                LEFT JOIN (
+                    SELECT rt.id, 'True' AS is_reserved
+                    FROM reservation_time rt
+                    JOIN reservation r ON r.time_id = rt.id
+                    WHERE r.date = ? AND r.theme_id = ?
+                ) rvt ON rt.id = rvt.id


보고 15초 정도 생각하다 생각을 멈췄습니다 ㅋ_ㅋ
제 생각엔 reservation_time와 reservation의 조인 1번으로 충분한 요구사항입니다 (LEFT JOIN 내부에서의 join은 불필요)

아마 이 쿼리는 DB 내 쿼리 옵티마이저 (쿼리 최적화 도구)가 1회 조인만 하도록 변경할 가능성이 높습니다.
1회 조인만 하도록 변경해보셔요 (불가능하다고 생각되면 말씀주셔요. 저도 개선 쿼리 안 짜봐서요)

으악 죄송합니다 😅
의미없는 조인을 해버렸네요..
의도하신 쿼리가 이게 맞을까요?!

SELECT rt.id, start_at, IFNULL(is_reserved, 'False') AS is_reserved FROM reservation_time rt LEFT JOIN ( SELECT r.time_id, 'True' AS is_reserved FROM reservation r WHERE r.date = ? AND r.theme_id = ? ) r ON rt.id = r.time_id

조인절 내에서 서브쿼리를 활용하시는 이유가 있을까요? 아래처럼도 가능할거 같은데용

SELECT rt.id, start_at, IFNULL(r.time_id, 'False') AS is_reserved FROM reservation_time rt LEFT JOIN reservation r ON rt.id = r.time_id WHERE r.date = ? AND r.theme_id = ?

저도 처음에 웨지 말씀처럼 쿼리를 작성했다가 서브쿼리를 활용하게 된 이유는
위의 SQL 결과와 같습니다 🥲

구하고자 하는 내용은 등록된 모든 시간대의 start_at, 예약이 되었는지에 대한 여부 is_reserved 인데요
서브쿼리 없이 LEFT JOIN 처리 후 date나 theme_id 등 reservation 테이블에만 존재하는 컬럼을 통해 조건문을 사용하게 되면
예약이 안되어있는 시간대는 조회결과에서 사라지게 됩니다.

때문에 방법을 고민하다가 서브쿼리로 해결하게 되었는데요
혹시 서브쿼리를 사용하지 않고도 해결할 수 있는 더 좋은 방법이 있을까요??

is null 조건문과 or로 활용해보셔요 ㅎ
(r.date is null or r.date = '2024-05-05')

SELECT rt.id, rt.start_at, CASE WHEN r.time_id IS NOT NULL THEN 'True' ELSE 'False' END AS is_reserved FROM reservation_time rt LEFT JOIN reservation r ON rt.id = r.time_id AND r.date = ? AND r.theme_id = ?;

남겨주신 리뷰를 보고 해결해보려고 했는데 잘 안돼서 생각해보니
JOIN의 ON절에서 애초에 r.time_id와 rt.id 가 묶여서 애초에 예약에 예약시간이 존재하지 않으면
WHERE 절에 조건을 작성하니까 조회자체가 안되네요 🥲

그래서 ON절에 조건을 작성했더니 이런식으로도 해결 가능했습니다!
이렇게 처리해도 괜찮을까요?

sihyung92 · 2024-05-06T03:41:49Z

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

+import java.util.List;
+
+@Service
+public class ReservationService {


repository와 dao를 교환함으로써 어떤 아키텍처를 결정하시게 된 것인지 인지하고 계시면 좋습니다.

repository 패턴을 활용할 경우 도메인에서 데이터 전달체를 모르게 됩니다 (도메인에서 repository 인터페이스를 정의하고 구현체는 persistence layer에서 주입합니다)
이 경우 비즈니스 로직을 데이터 소스 종류와 상관없이 재활용 할 수 있게되지만 도메인 보호계층이 생겨서 복잡성이 증대됩니다.

dao를 도메인이 직접 의존하면 작성하신 코드 대로입니다. 비즈니스 로직(service 로직)이 특정 데이터소스 (DB)에 종속되어 있으므로 재활용이 어려우나 코드가 간결해지고 DB 요구사항에 직접적으로 맞출 수 있습니다.

특히 우테코는 어렵고 복잡하면 좋은 것이라고 생각하는 경향성이 있으나, 저는 반대입니다. 복잡성 자체가 악입니다. 복잡성을 도입하려면 타당한 이유가 있어야한다고 생각합니다.

좀 더 적다가 너무 개인 의견이 많이 들어가서 생략했습니다 ㅋㅋ
두 패턴의 장단점에 대해 생각해보셔요. 둘 중 정답은 없습니다.

자세한 설명 감사합니다.
저도 처음부터 각종 디자인 패턴 등등을 도입해서 복잡한 구현을 하려다 오히려 코드가 망가진 경험을 레벨1에서 겪어보았기에
타당한 이유없는 복잡한 코드를 선호하지 않아요:)

Repository 패턴의 탄생 배경은
이전에는 Service 계층의 비즈니스 로직에서 DAO를 통해 데이터를 관리하다가
Service 계층에서 다루는 도메인 엔티티 객체에 대한 DAO가 늘어남에 따라
Service 계층의 비즈니스 로직에 계속해서 수정이 발생하고 복잡도가 증가해서 유지보수성이 저하되었고
이에 대한 해결책으로 Repository 패턴이 탄생한 것으로 알고 있는데요!

때문에 저또한 현재는 지금처럼 Dao만 다루어 관리하다가
코드의 복잡성이 더욱 늘어나 가독성 및 유지보수성이 저하되어 분리가 필요한 시점이라고 느껴지면
그때 Repository 패턴을 적용해볼 예정입니다!

넵 좋은 접근법이라 생각합니다

sihyung92 · 2024-05-06T03:45:51Z

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

+        if (requestDate.isBefore(today) || (requestDate.isEqual(today) && time.getStartAt()
+                .isBefore(LocalTime.now()))) {


내부 조건식 가독성이 떨어져요.
깨진 유리창 효과 때문에 이런식으로 조건식을 만들어두면 다른 개발자들이 계속 || 와 괄호로 감싸진 조건식을 덧대게 됩니다

requestDate.isBefore(today)와 (requestDate.isEqual(today) && time.getStartAt() .isBefore(LocalTime.now())는 같은 추상화 수준이니 뒤 조건식을 메서드로 추출해서 더 쉽게 파악되도록 해주세요

Suggested change

if (requestDate.isBefore(today) || (requestDate.isEqual(today) && time.getStartAt()

.isBefore(LocalTime.now()))) {

if (requestDate.isBefore(today) ||

something(today, time)) {

깨진 유리창 효과 주의하며 개발하겠습니다..!
좋은 의견 감사합니다.

private boolean isReservationInPast(final LocalDate requestDate, final Time requestTime, final LocalDateTime now) { LocalDate today = now.toLocalDate(); LocalTime nowTime = now.toLocalTime(); // 날짜가 지난 경우 if (requestDate.isBefore(today)) { return true; } // 날짜가 오늘이지만, 지난 시간인 경우 if (requestDate.isEqual(today) && requestTime.getStartAt().isBefore(nowTime)) { return true; } return false; }

혹시 검증 메서드의 이름을 지난 날짜에 대한 예약인 지 검증한다는 뜻의 isReservationInPast 로 설정하여 조금 더 확실한 추상화 수준을 만들어주고
위와 같이 조건문을 2개로 분리후 주석으로 의미를 정확히 표기해주는 것은 어떤가요??
코드라인이 길어져 아쉽긴 하지만 이렇게 하면 메서드를 분리하지 않고 가독성도 챙길 수 있을 것 같아서요!

네네 이것도 좋은 방법이라고 생각합니다

대신 주석은 필요없다고 생각합니다 ㅎ
코드와 동어 반복인 주석은 공해입니다 (주석도 유지보수 대상인데 둘이 달라지면 더 헷갈리겠죠 보통 주석을 믿거든요)

주석에 대해선 제가 뭐라고 하는 것 보다 우테코에 많다고 알고있는 좋은코드 나쁜코드 5.2 주석문의 적절한 사용 (127p) 를 봐주세용

동어 반복 + 로직 변경 시 주석도 변경을 잊어버리게 되면 문제가 생긴다는 점에서 제거하는 편이 좋겠네요!
좋은 의견 감사합니다.
적절한 주석의 사용법도 너무 궁금한데 내일 캠퍼스에 가면 책도 바로 읽어보겠습니다

sihyung92 · 2024-05-07T10:56:15Z

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

+                reservationRequest.timeId(), reservationRequest.date(), theme.getId());
+
+        if (duplicateTimeReservations.size() > 0) {
+            throw new DataConflictException(ErrorType.RESERVATION_DUPLICATION_CONFLICT,


#57 (comment) 코멘트 관련

이 상황이 "PK가 충돌하거나 FK가 충돌하는 상황" 등과 관련이 있나요?
data conflict는 무언가 데이터 정합성이 깨진 상황이 연상되는데요, 이 경우는 그런 경우는 아닌걸로 보여서요

그리고 또 DataConflictException는 여전히 모호하다는 생각이 듭니다. DB에서 발생하면 모두 DataConflict인가요? 혹은 PK나 FK 제약조건에 걸렸을 때만 DataConflict 인가요? DB에서 발생하는 모든 예외를 catch하고자 하는 목적이면 DBException, SQLException 등이 직감적으로 다가오는거 같습니다

자세히 설명해보자면 다음과 같은 상황에 사용하고자 만든 예외였습니다.

[이미 존재하는 데이터를 다시 추가]

이미 예약이 존재하는 시간에 예약 요청을 보낸 경우

(ex. theme_id=1, time_id=1 에 이미 예약이 있음에도 불구하고 같은 theme_id, time_id 로 예약 추가 요청이 들어온 경우)

이미 특정 시간의 레코드가 존재하는데, 또다시 같은 시간 등록을 요청한 경우

(ex. reservation_time 테이블에 start_at이 17:00 인 레코드가 이미 존재하는데, start_at이 17:00인 reservation_time의 등록 요청이 들어온 경우)

[특정 데이터에 연관되어 있는 데이터를 삭제하려함]

예약이 존재하는 시간대를 삭제하려고 하는 경우

(ex. 17:00에 예약이 존재하는데 reservation_time에서 17:00 을 삭제하려고 하는 경우)

이 내용들은 모두 서버의 현재 상태와 충돌하는 요청에 관련된 것이기 때문에 Conflict라는 네이밍을 붙여주었는데요.
그저 Conflict라는 Exception 네이밍을 통해서는 웨지의 말씀대로 의도를 드러내기 어려울 수 있다고 생각이 듭니다.
(예시도 적다보니 2종류로 나뉘는데 말이죠)

적절한 네이밍으로 변경하여 2번째 미션 리뷰요청에 함께 보내겠습니다!
좋은 의견 감사드립니다!

sihyung92 · 2024-05-07T11:03:23Z

src/main/java/roomescape/global/dto/response/ApiResponse.java

+    }
+
+    public static <T> ApiResponse<T> fail(final ErrorType errorType) {
+        return new ApiResponse<>(false, errorType.getDescription(), null);


프론트 입장을 겪어보시면 좋은데요, 실제로 happy case에서 조금만 수틀리면 500 내려버리거나 400 이라면서 설명은 없는 API들을 많이 만나게 됩니다 ㅋㅋ
이러면 서버 개발자를 향한 적개심이 차오르게 되는데요, 엄청 까칠한 질문 DM("500 나는데 확인좀요..")을 받고 싶으신게 아니라면 상대가 validation 실패 시 뭐가 필요할지 생각해보세요
보내주지 않은 필드, 잘못된 필드, 정책을 지키지 못한 경우 등을 친절하게 응답해주면 될겁니다

이 부분을 다 수정해서 보내면 "정상적인 API 요청" 이 될텐데요, 이게 보안상 문제가 될까요?
보안의 문제가 되는 것은 내 서버가 어떤 스택으로 구성되어있는지 오류메시지로 힌트를 줘버리거나, 응답값으로 개인정보가 새나가는 구멍은 없는지 등입니다

sihyung92

안녕하세요 이든~~ 리뷰어 웨지입니다.
코멘트 답변을 포함해 리뷰 남겼으니 확인해보셔요!
다음 step 때 반영해주셔도 될거같아 머지 할테니 다음 step에서 반영해주세요.
감사합니다~~

sihyung92 · 2024-05-07T11:12:46Z

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

        if (StringUtils.isBlank(name) || date == null || time == null || theme == null) {
-            throw new ValidateException(ErrorType.INVALID_ERROR, String.format("유효하지 않은 값입니다.%n%s", this.toString()));
+            throw new ValidateException(ErrorType.RESERVATION_REQUEST_DATA_BLANK,


케이스 별로 자르는게 낫지 않냐는 리뷰를 남기려다가 뒤늦게 this를 봤습니다
영리한 해법이네요 ㅎ

sihyung92 · 2024-05-07T11:13:10Z

src/main/java/roomescape/dto/reservation/ReservationRequest.java

@@ -24,7 +22,8 @@ public record ReservationRequest(
    public ReservationRequest {
        if (StringUtils.isBlank(name) || StringUtils.isBlank(date.toString())
                || StringUtils.isBlank(timeId.toString()) || StringUtils.isBlank(themeId.toString())) {
-            throw new ValidateException(ErrorType.BAD_REQUEST, "공백 또는 null이 포함된 요청입니다.");
+            throw new ValidateException(ErrorType.REQUEST_DATA_BLANK,
+                    String.format("공백 또는 null이 포함된 요청입니다. [values: %s]", this));


"예약" 이라는 설명이 빠졌네요 ㅎ

놓치고 지나친 부분이네요..! 체크 감사합니다!
아래 리뷰들도 반영하였습니다!

sihyung92 · 2024-05-07T11:13:29Z

src/main/java/roomescape/dto/theme/ThemeRequest.java

@@ -13,7 +13,8 @@ public record ThemeRequest(
 ) {
    public ThemeRequest {
        if (StringUtils.isBlank(thumbnail) || StringUtils.isBlank(name) || StringUtils.isBlank(description)) {
-            throw new ValidateException(ErrorType.BAD_REQUEST, "공백 또는 null이 포함된 요청입니다.");
+            throw new ValidateException(ErrorType.REQUEST_DATA_BLANK,
+                    String.format("공백 또는 null이 포함된 요청입니다. [values: %s]", this));


여기도요!

sihyung92 · 2024-05-07T11:13:36Z

src/main/java/roomescape/dto/time/TimeRequest.java

@@ -15,7 +15,8 @@ public record TimeRequest(

    public TimeRequest {
        if (StringUtils.isBlank(startAt.toString())) {
-            throw new ValidateException(ErrorType.BAD_REQUEST, "공백 또는 null이 포함된 요청입니다.");
+            throw new ValidateException(ErrorType.REQUEST_DATA_BLANK,
+                    String.format("공백 또는 null이 포함된 요청입니다. [values: %s]", this));


여기도요2!

sihyung92 · 2024-05-07T11:15:22Z

src/main/java/roomescape/global/dto/response/ApiResponse.java

    }

    public static <T> ApiResponse<T> fail(final ErrorType errorType) {
-        return new ApiResponse<>(false, errorType.getDescription(), null);
+        return new ApiResponse<>(errorType.getDescription(), null);


이 부분을 그대로 두시고 오류 메시지를 상수화 하셨군요?
이번에 수정해주신 오류메시지가 내려가는 게 더 좋다고 생각합니다
상수에 정의해주신 메시지로는 (ex. 테마(Theme) 생성에 유효하지 않은 값(null OR 공백)이 입력되었습니다.) 난 분명히 다 잘 보냈다고 생각하는 프론트를 만족시킬 수 없습니다

프론트를 만족시킬 수 없다는 내용이 웨지가 직접 경험해보신 내용인 것 같아서 더욱 와닿네요 🤣
어느 부분이 잘못되었는 지 직접 확인할 수 있도록 구체적인 내용도 함께 보내는 방식으로 변경해보겠습니다

sihyung92 · 2024-05-07T11:15:54Z

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


-        if (duplicateTimeReservation.size() > 0) {
-            throw new ConflictException(ErrorType.RESERVATION_DUPLICATION_CONFLICT, "이미 해당 날짜/시간/테마에 예약이 존재합니다.");
+        // 날짜가 지난 경우


요 부분은 코멘트 답변 중 남겨두었습니당~

확인하겠습니다!

J-I-H-O and others added 30 commits April 30, 2024 14:12

init: 이전 미션 코드 반영

bd5a99e

style: 줄바꿈 수정

b0bd9bb

docs: 예외 상황 정의

ba6b536

Co-authored-by: PgmJun <chltmdwns96@gmail.com> Co-authored-by: J-I-H-O <jeongjiho0731@gmail.com>

feat: 유효하지 않은 시간 생성 검증 기능 구현

3592c1d

Co-authored-by: PgmJun <chltmdwns96@gmail.com> Co-authored-by: J-I-H-O <jeongjiho0731@gmail.com>

feat: 유효하지 않은 시간 생성 검증 기능 구현

75c7ee4

Co-authored-by: PgmJun <chltmdwns96@gmail.com> Co-authored-by: J-I-H-O <jeongjiho0731@gmail.com>

feat: 예약이 존재하는 시간 삭제를 방어하는 기능 구현

ec2e3b0

Co-authored-by: PgmJun <chltmdwns96@gmail.com> Co-authored-by: J-I-H-O <jeongjiho0731@gmail.com>

Merge remote-tracking branch 'origin/step1' into step1

e37964d

style: 줄바꿈 정리

a8a4a02

feat: 유효하지 않은 예약 등록 요청 검증 로직 구현

0fe2794

Co-authored-by: PgmJun <chltmdwns96@gmail.com> Co-authored-by: J-I-H-O <jeongjiho0731@gmail.com>

feat: 중복된 날짜 및 시간 예약 등록 검증 로직 구현

540fdee

Co-authored-by: PgmJun <chltmdwns96@gmail.com> Co-authored-by: J-I-H-O <jeongjiho0731@gmail.com>

chore: gradle 설정 변경

5d6008e

Co-authored-by: PgmJun <chltmdwns96@gmail.com> Co-authored-by: J-I-H-O <jeongjiho0731@gmail.com>

feat: 지나간 날짜와 시간에 대한 예약 생성 방지 기능 구현

7307152

Co-authored-by: PgmJun <chltmdwns96@gmail.com> Co-authored-by: J-I-H-O <jeongjiho0731@gmail.com>

docs: 테마 API 명세 추가

4f592bd

Co-authored-by: PgmJun <chltmdwns96@gmail.com> Co-authored-by: J-I-H-O <jeongjiho0731@gmail.com>

chore: 테마 관련 DDL 추가

7331554

Co-authored-by: PgmJun <chltmdwns96@gmail.com> Co-authored-by: J-I-H-O <jeongjiho0731@gmail.com>

feat: 테마 관리 페이지 요청 API 구현

17af3db

Co-authored-by: PgmJun <chltmdwns96@gmail.com> Co-authored-by: J-I-H-O <jeongjiho0731@gmail.com>

fix: 예약 관리 페이지 파일 변경

9e8790e

Co-authored-by: PgmJun <chltmdwns96@gmail.com> Co-authored-by: J-I-H-O <jeongjiho0731@gmail.com>

docs: 테마 관련 REST API 명세 추가

e951bbb

Co-authored-by: PgmJun <chltmdwns96@gmail.com> Co-authored-by: J-I-H-O <jeongjiho0731@gmail.com>

feat: 테마 조회 기능 구현

3bbd400

Co-authored-by: PgmJun <chltmdwns96@gmail.com> Co-authored-by: J-I-H-O <jeongjiho0731@gmail.com>

feat: 테마 추가 기능 구현

40d021b

feat: 테마 삭제 기능 구현

ff72e78

Co-authored-by: PgmJun <chltmdwns96@gmail.com> Co-authored-by: J-I-H-O <jeongjiho0731@gmail.com>

feat: 예약 도메인과 테마 도메인 연동

fe93600

Co-authored-by: PgmJun <chltmdwns96@gmail.com> Co-authored-by: J-I-H-O <jeongjiho0731@gmail.com>

feat: 사용자 예약 페이지 요청 API 구현

60d7ed3

Co-authored-by: PgmJun <chltmdwns96@gmail.com> Co-authored-by: J-I-H-O <jeongjiho0731@gmail.com>

chore: 더미 데이터 삽입 SQL 작성

478963c

Co-authored-by: PgmJun <chltmdwns96@gmail.com> Co-authored-by: J-I-H-O <jeongjiho0731@gmail.com>

feat: 특정 날짜의 특정 테마 예약 정보 조회 기능 구현

45fbf52

Co-authored-by: PgmJun <chltmdwns96@gmail.com> Co-authored-by: J-I-H-O <jeongjiho0731@gmail.com>

docs: 인기 테마 API 명세 추가

efb21f8

Co-authored-by: PgmJun <chltmdwns96@gmail.com> Co-authored-by: J-I-H-O <jeongjiho0731@gmail.com>

feat: 인기 테마 조회 기능 구현

f7177bd

Co-authored-by: PgmJun <chltmdwns96@gmail.com> Co-authored-by: J-I-H-O <jeongjiho0731@gmail.com>

refactor: 테스트 성능 개선

1f76b8f

Co-authored-by: PgmJun <chltmdwns96@gmail.com> Co-authored-by: J-I-H-O <jeongjiho0731@gmail.com>

refactor: 메서드 분리

772631f

Co-authored-by: PgmJun <chltmdwns96@gmail.com> Co-authored-by: J-I-H-O <jeongjiho0731@gmail.com>

style: 완료된 TODO 제거

ac95a28

Co-authored-by: PgmJun <chltmdwns96@gmail.com> Co-authored-by: J-I-H-O <jeongjiho0731@gmail.com>

style: 테스트 given, when, then 분리

830df0c

Co-authored-by: PgmJun <chltmdwns96@gmail.com> Co-authored-by: J-I-H-O <jeongjiho0731@gmail.com>

PgmJun added 16 commits May 4, 2024 16:24

refactor: HTTP Response 형식 수정(List -> Object)

230ce75

style: 컨트롤러 메서드 네이밍 변경

4cec0d8

style: DAO 메서드 네이밍 변경

04131d6

style: Service 메서드 네이밍 변경

a0899cf

refactor: 메서드 인자 final로 불변 처리

7b816fd

refactor: 예약 시간 정보 조회 API Endpoint 수정

4a9a980

refactor: 테스트에서 컴포넌트 객체 사용을 위해 Import 애노테이션 적용

0f8ef2c

test: MVC Controller 테스트 작성

c983dfd

feat: 입력부 검증 로직 구현

41fa7a2

feat: 도메인 검증 로직 구현

818b1eb

refactor: 예약 시간 정보 조회 로직 - 서브 쿼리 사용하여 하나의 SQL로 조회하도록 수정

a2905fc

refactor: 테스트 코드 @BeforeEach 제거하고 필요한 곳에서만 데이터 삽입해주도록 변경

79de6f5

fix: 00:30분 일때 1시간 전은 23:30분 이기 때문에 다른 날짜로 판별되어 테스트코드 실패하는 문제 해결

604a0c0

style: Repository 네이밍 용도에 맞도록 Dao로 변경

a0f45d0

refactor: 특정 기간 사이 인기테마 조회 API 쿼리스트링으로 날짜값 받도록 변경

15276bd

sihyung92 requested changes May 6, 2024

View reviewed changes

PgmJun added 10 commits May 7, 2024 00:25

refactor: 의미가 불분명한 ConflictException 네이밍 변경 (DataConflictException)

b40b582

refactor: 에러 메시지 클라이언트 친화적으로 개선

7620bc5

refactor: Location 헤더 상수 사용하도록 변경

f6b6e37

refactor: 로깅 예외 메시지 구체화

4014865

refactor: 인기테마 조회 API에 defaultValue 적용

80690a0

refactor: 복잡성 제거를 위해 ApiResponse isSuccess 필드 제거

272a414

refactor: 예약 시간 정보 조회 쿼리에서 의미없는 조인 쿼리 제거

3175c22

refactor: 조건식 가독성 향상을 위한 조건문 분리

81687e0

fix: 잘못된 예외 객체 사용 로직 수정 (DataConflict -> Validate)

bc3aaf6

fix: 불필요한 포맷팅 애노테이션 제거

a80fcbd

sihyung92 reviewed May 7, 2024

View reviewed changes

sihyung92 approved these changes May 7, 2024

View reviewed changes

sihyung92 merged commit bbaf858 into woowacourse:pgmjun May 7, 2024

		if (requestDate.isBefore(today) \|\| (requestDate.isEqual(today) && time.getStartAt()
		.isBefore(LocalTime.now()))) {

[1 - 3단계 방탈출 사용자 예약] 이든(최승준) 미션 제출합니다. #57

[1 - 3단계 방탈출 사용자 예약] 이든(최승준) 미션 제출합니다. #57

Conversation

PgmJun commented May 2, 2024 • edited Loading

상황공유 💭

sihyung92 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

PgmJun May 6, 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

PgmJun May 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PgmJun May 6, 2024 • edited Loading

Choose a reason for hiding this comment

sihyung92 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

PgmJun May 6, 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

PgmJun 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

PgmJun May 9, 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

PgmJun May 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PgmJun May 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PgmJun May 8, 2024 • edited Loading

Choose a reason for hiding this comment

sihyung92 May 7, 2024 • edited Loading

Choose a reason for hiding this comment

sihyung92 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

PgmJun commented May 2, 2024 •

edited

Loading

PgmJun May 6, 2024 •

edited

Loading

PgmJun May 6, 2024 •

edited

Loading

PgmJun May 6, 2024 •

edited

Loading

sihyung92 May 7, 2024 •

edited

Loading

PgmJun May 6, 2024 •

edited

Loading

PgmJun May 7, 2024 •

edited

Loading

PgmJun May 9, 2024 •

edited

Loading

PgmJun May 7, 2024 •

edited

Loading

PgmJun May 8, 2024 •

edited

Loading

PgmJun May 8, 2024 •

edited

Loading

sihyung92 May 7, 2024 •

edited

Loading