Skip to content

Conversation

@chemistryx
Copy link

안녕하세요 김준수 리뷰어님! 이번 Spring Core 8, 9, 10단계 리뷰이로 참여하게된 하수한이라고 합니다!
이번 미션을 통해 처음 리뷰를 부탁드리는 것 같은데, 잘 부탁드립니다!

이번 단계에서는 동적으로 시간을 관리할 수 있는 기능 추가를 중심으로 진행했습니다.

다음은 현재 프로젝트 구조입니다:

controller/
- HomeController.java : 메인 화면 관련 endpoint를 처리하는 컨트롤러
- ReservationController.java : 예약 관련 endpoint를 처리하는 컨트롤러
- TimeController.java : 시간 관련 endpoint를 처리하는 컨트롤러
dto/
- ErrorResponse.java : 오류 응답 정보를 담을 객체
- ReservationCreateRequest.java : 예약 생성 요청 Payload를 담을 객체
- ReservationCreateResponse.java : 예약 생성 요청 응답 정보를 담을 객체
- TimeCreateRequest.java : 시간 생성 요청 Payload를 담을 객체
- TimeCreateResponse.java : 시간 생성 요청 응답 정보를 담을 객체
exception/
- GlobalExceptionHandler.java : 공통 예외 핸들러
- TimeNotFoundException.java : 시간을 찾을 수 없을 때 발생하는 Custom Exception
model/
- Reservation.java : 예약 데이터의 구조를 정의한 Record
- Time.java : 시간 데이터의 구조를 정의한 Record
repository/
- ReservationRepository.java : 예약 관련 데이터를 DB에서 읽고 쓰는 기능을 캡슐화한 객체
- TimeRepository.java : 시간 관련 데이터를 DB에서 읽고 쓰는 기능을 캡슐화한 객체
service/
- ReservationService.java : 예약 관련 비즈니스 로직을 담은 객체
- TimeService.java : 시간 관련 비즈니스 로직을 담은 객체
validation/
- DateVaildator.java : ValidDate 어노테이션의 검증 로직
- TimeIdValidator.java : ValidTimeId 어노테이션의 검증 로직
- TimeValidator.java: ValidTime 어노테이션의 검증 로직
- ValidDate.java : 날짜를 검증하기 위한 Custom bean validator
- ValidTime.java : 시간을 검증하기 위한 Custom bean validator
- ValidTimeId.java : 시간 ID를 검증하기 위한 Custom bean validator
RoomescapeApplication.java : Main entrypoint

다음은 구현 과정에서 발생한 질문 사항입니다:

  1. 현재 Reservation Response를 보면 다음과 같은 형태를 가지고 있습니다.
[
    {
        "id": 1,
        "name": "123",
        "date": "2025-11-01",
        "time": {
            "time": "10:00:00",
            "id": 1
        }
    }
]    

기존 LocalTime 타입의 time 필드를 Time 객체로 변경한 뒤로 다음과 같이 JSON 필드가 자동으로 변경되었는데, 어떠한 사유로 인해 저렇게 변경된 것인지 궁금합니다! Time 객체로 변경하면서 저 부분도 Spring에서 자동으로 파싱? 해준걸까요..?

  1. 현재 ReservationCreateRequest에서 @ValidTimeId를 통해 입력 값에 대한 검증을 수행하고 있는데, 이 custom bean validator의 경우 내부적으로 TimeService.getById()를 호출하여 검증을 진행하고 있습니다. 검증 이후 ReservationService::createReservation() 에서도 위 메소드를 한번 더 호출하여 Time 객체를 받아오고 있는데, 이렇게 검증단에서 한번, 실제 비즈니스 로직에서 한번 더 호출함으로써 DB에 요청을 두번 날리게 되는 것이라고 보여지는데, 이게 과연 좋은 접근 방식인지 궁금합니다.

긴 글 읽어주셔서 감사합니다!! 🙂

Copy link

@boyekim boyekim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

안녕하세요 수한님! 김준수 리뷰어님이 몸이 안좋으셔서 제가 대신 왔습니다(반가워해주세요)

질문에 대한 답변입니다.

1. Time 파싱

질문 이해를 잘 못했습니다.. value가 왜 time으로 나오는지 여쭤보시는걸까요?? 만약 이거라면 @JsonProperty가 이유입니다. 직렬화할 때 이 친구덕에 원하는 이름을 넣어줄 수 있어요. @JsonProperty를 뗀다면 필드값이 그대로 value인 것을 확인하실 수 있습니다.
만약 이것에 대한 질문이 아니었다면 말씀해주세요~

2. 검증

Service 레이어를 호출하는 것이 괜찮은지에 대해서 먼저 생각해보시면 좋을 것 같습니다.
Validator와 Service가 각각 어떤 역할이라고 생각하시나요?
저는 Validator가 입력 값 형식 정도를 확인하면 된다고 생각합니다. repository까지 호출해야 하는 비즈니스 로직은 Service 레이어에 위치한다면 역할 분리가 잘 될 것 같아요.(Validator가 Service에 의존하게 되었기 때문입니다.)
결론은 custom bean validator를 사용해서 도메인의 정책에 맞게 검증한 것은 좋은 시도이나, 자신도 모르게 과한 책임을 부여한 것은 아닌지 생각해보셔도 좋을 것 같습니다!
수한님의 생각을 자유롭게 말씀해주세요

return false;
}
}
} No newline at end of file
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

개행 넣어주세요!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

앗 개행을 빼먹었네요 ㅎㅎ..

cefce3c 에 반영했습니다!

Comment on lines 35 to 37
public void deleteTime(int id) {
timeRepository.deleteById(id);
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

현재는 동일한 id에 대해 여러번 요청을 보내도 정상적으로 삭제가 되네요!
Time에 대한 삭제를 어떻게 정의하셨는지 의견을 듣고싶어요.(진짜 순수 물어보는거임)
Time 도메인에 있어서 없는 아이를 삭제할 경우에도 최초에 삭제가 호출되었을때와 상태가 같으니 성공시킨다(멱등성)와, 없는 친구를 삭제하는 경우는 예외상황으로 판별하여 에러를 뱉는다중에 전자를 선택한 이유가 있을까요??

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이 부분에 대해서는 지난 리뷰어님과도 논의해보았던 내용인데요. 당시에 논의해보았던 내용은 다음과 같습니다:

RFC 표준에 의하면 DELETE 메소드의 경우 멱등성이 보장되어야 한다고 나와 있습니다. 하지만 현재 제 구현같은 경우에는 만약 삭제할 리소스가 존재한다면 204를, 삭제될 리소스가 존재하지 않는다면 400을 던지도록 구현되어있는데, 이는 멱등성을 만족하지 않는다고 볼 수 있습니다.

이와 관련해서 생각해보았는데, 만약 제거된 리소스에 대해 다시 요청을 했을때 예외를 던진다면 사용자 입장에서는 삭제 요청 자체가 실패했다고 생각할 수도 있을 것 같아 멱등성을 유지하는 측면으로 수정하는 것이 더 적절하다고 판단했습니다.

하지만 이러한 방식으로 처리하는 경우 모든 상태에 대해 같은 응답 코드를 반환하기 때문에 사용자나 FE 단에서 예외 상황을 검출하기 쉽지 않겠다는 생각이 여전히 들었습니다..

이와 관련해서 다시 내용을 찾아보다 (cf. 멱등성 - MDN Web Docs 용어 사전) 아래와 같은 내용을 발견했습니다.

DELETE /idX/delete HTTP/1.1의 상태 코드는 응답마다 달라질 수 있지만, 그럼에도 멱등성을 가집니다.
상태 코드가 다름에도 서버 차원에서만 멱등성을 유지할 수 있다면 이것도 멱등성을 만족한다고 볼 수 있다는 것으로 이해했는데, 만약 그렇다면 오류 코드를 반환하는 방식이 더 직관적이라고 생각합니다. 이와 관련해서 수민님은 어떤 생각을 가지고 계신지 궁금합니다!

Copy link

@boyekim boyekim Dec 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

서버 최종 상태를 바라보는것에 동의하고, 상태코드는 사용자가 누구인지 보는 것 같아요.

"멱등성을 지켜야하므로 항상 클라이언트에게는 200을 준다" vs "멱등성을 지켜야하므로 서버에게 40x를 준다"
이렇게 판단하기보단
"해당 삭제는 유저가 리소스가 없는지 알 필요가 없기 때문에 항상 200을 준다" vs "해당 삭제는 서버(or 관리자)가 관리해야하므로 리소스가 없는 경우에는 40x를 준다/예외를 던진다" 이렇게 도메인 관점으로 보는 게 더 자연스럽다고 생각합니다.
그래서 결국 도메인의 정책을 더 신경써보시면 좋을 것 같아요.
Time의 관리는 관리자가 하게 될 것 같은데, 관리자는 리소스가 없는걸 알아야할 수도 있으니까요~!

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Time 삭제에서 추가 코멘트 하나 남기겠습니다!
만약 해당 시간의 Reservation이 존재하는 경우에 Time이 삭제될 경우를 생각해봐도 좋을 것 같습니다.
외래키 제약으로 Reservation이 있는 경우 삭제가 안되고 있긴 하나, 명확하게 예외를 던져주는것이 유지 보수 면에서도 더 좋아요
예외를 더 이해하기도 쉽고, DB가 막아주는 것 보단 정책으로 명시해주는게 올바른 설계일 것 같습니다.
수한님의 프로젝트라고 생각하구 정책을 주체적으로 생각해보셔도 좋을 것 같네요🐥👍

Copy link
Author

@chemistryx chemistryx Dec 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"해당 삭제는 유저가 리소스가 없는지 알 필요가 없기 때문에 항상 200을 준다" vs "해당 삭제는 서버(or 관리자)가 관리해야하므로 리소스가 없는 경우에는 40x를 준다/예외를 던진다" 이렇게 도메인 관점으로 보는 게 더 자연스럽다고 생각합니다.

확실히 도메인 관점에서 생각하게 된다면 제가 앞서 말한 고민도 해결할 수 있을 것 같네요! 좋은 인사이트 감사합니다. 👍 👍

1e2b24b 커밋에 반영했습니다!

Copy link
Author

@chemistryx chemistryx Dec 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

만약 해당 시간의 Reservation이 존재하는 경우에 Time이 삭제될 경우를 생각해봐도 좋을 것 같습니다.

이 부분의 경우에도 저도 처음 테스트를 해봤을 때 서버에서 단지 500만 던져줘서 원인을 파악하기 어려웠던 것 같습니다..!
말씀해주신대로 해당 FK 예외에 대해서도 좀 더 명확하게 처리할 수 있도록 수정해보겠습니다!

598a97e 커밋에 반영했습니다~

Comment on lines 12 to 16
private final TimeService timeService;

public TimeIdValidator(TimeService timeService) {
this.timeService = timeService;
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TimeIdValidator가 Service를 의존하고 있네요.
Validator가 비즈니스 로직을 침범하는 것에 대해 어떻게 생각하시나요?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

안그래도 이 부분의 경우 코멘트에서 말씀하신 것처럼 비즈니스 로직이 Bean Validator까지 넘어온다는 점에서 너무 과한 책임을 부담하는게 아닌가 라는 생각이 들었습니다 ㅠ

따라서 Bean Validator에서는 입력 형식만 검증하고, 시간 ID에 대한 검증 로직은 Service로 이관하는 형식으로 수정했습니다!

반영 커밋: 2a74a00

Comment on lines 12 to 15
@ExceptionHandler(TimeNotFoundException.class)
public ResponseEntity<ErrorResponse> handleReservationNotFoundException(TimeNotFoundException e) {
return new ResponseEntity<>(new ErrorResponse(e.getMessage()), HttpStatus.BAD_REQUEST);
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

수한님 코드에서는 현재 Reservation을 생성할 때 TimeId를 검증해주고, 이 Id가 잘못되었을때 TimeNotFoundException을 던져줍니다.
하지만 이 handleReservationNotFoundException에서 잡아주고있지 않아요. 왜일까요?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

추가로, 개발자가 예외가 어디서 발생했는지 파악할 수 있으려면 어떻게 할 수 있을까요?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

수한님 코드에서는 현재 Reservation을 생성할 때 TimeId를 검증해주고, 이 Id가 잘못되었을때 TimeNotFoundException을 던져줍니다.
하지만 이 handleReservationNotFoundException에서 잡아주고있지 않아요. 왜일까요?

이 부분의 경우 확인해보니 ValidateTimeId 내부에 있는 try-catch 문 안에서 TimeNotFoundException을 던질 때 return false로 bean validator 내부에서 모든 것을 다 처리해버려서 GlobalExceptionHandler까지 도달하지 않았던 것 같아요.
Validator 책임 분리 관련 코멘트(#532 (comment)) 에서 수정한 내용을 통해 해당 예외가 GlobalExceptionHandler까지 도달할 수 있도록 처리했습니다..!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

추가로, 개발자가 예외가 어디서 발생했는지 파악할 수 있으려면 어떻게 할 수 있을까요?

해당 부분은 로깅을 통해 처리할 수 있을 것 같은데, Java 내장 sout 대신 Spring에 내장된 로거를 통해 로그를 출력하도록 수정했습니다!

반영 커밋: 391eb2a

Comment on lines 27 to 33
public Time createTime(TimeCreateRequest request) {
LocalTime parsedTime = LocalTime.parse(request.time());

Time time = Time.create(parsedTime);

return timeRepository.save(time);
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

생성 시에 추가적으로 고려할 점은 없을까요?
같은 시간이 중복 저장 되는 경우에 대해서도 생각해보면 좋을 것 같습니다~

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

시간 입력값 파싱의 경우 DTO단에서 처리되니 문제가 없을 것 같습니다.

같은 시간 중복 저장의 경우 따로 명시가 되어있지 않아 처리를 해두지는 않았는데, 4594369 커밋에 반영해두었습니다!

@chemistryx
Copy link
Author

안녕하세요 수민님~! 상세하게 리뷰 달아주셔서 정말 감사합니다!
질문 1에 대해서는 제 설명이 좀 미흡했던 것 같아 부연 설명 드리자면, 일단 valuetime으로 나오는 것의 경우 구현 명세에 그렇게 나와있어 제가 임의로 추가한 부분이 맞습니다..!

제가 궁금했던 것은 예를 들면 Integer id의 경우 아래와 같은 형식으로 자동으로 직렬화? 되는 것 같은데

{
  "id": 1
}

제가 임의로 생성한 Time record에 대해서도 아래와 같이 자동으로 time필드 아래에 새로운 무언가가 자동으로 붙는 형식으로 구성이 되는 것을 확인했습니다.

{
    "id": 1,
    "name": "123",
    "date": "2025-11-01",
    "time": {
        "time": "10:00:00",
         "id": 1
     }
}

이 부분에 대해선 제가 따로 설정한 부분이 없는데, Spring에서 자체적으로 구성을 해주는 것인지 궁금합니다!

질문 2의 경우 말씀해주신 것처럼 DTO에서 모든 입력 값에 대한 검증을 완료하려고 하다보니 책임이 무거워진 측면이 없지않아 있다고 생각합니다.
따라서 #532 (comment) 코멘트에도 남겼지만, 책임 분리를 통해 DTO에서는 기본적인 입력 값에 대한 1차적인 검증만 수행하도록 수정했습니다..!

Copy link

@boyekim boyekim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

질문에 대한 답변 남깁니다!

직렬화에 대해

결론은 Spring이 자체적으로 구성을 해 주는 것이 맞습니다.
인텔리제이에서 External Libraries를 확인할 수 있을텐데요, 이 친구들이 의존성이에요.

image

"jackson"이 되게 많자나요??(jackson annotation, jackson databind 등등..) Json 관련해서는 이 친구들이 맡아주고 있습니다.
클래스 이름(time)을 필드값, 내부에서도 필드명과 값을 key와 value의 형태로 엮어줍니다.
그래서 별도의 설정 없이도 직렬화가 예쁘게 돼서 제공되는거에요.

추가적인 이야기를 해드리자면, 마샬링 이라는 용어가 다른 생태계에서 많이 쓰입니다. 더 큰 범위의 용어이니 마샬링을 키워드로 학습해보셔도 도움이 될 것 같아요.


@ExceptionHandler(TimeNotFoundException.class)
public ResponseEntity<ErrorResponse> handleTimeNotFoundException(TimeNotFoundException e) {
log.error(e.getMessage());
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

로그를 도입해서 예외 메시지를 볼 수 있는 점 좋네요!

하지만
image
이렇게 에러 메시지를 본다고 해서 어디에서 예외가 발생했는지 알기는 어려워 보입니다.
한번 예외 객체 e 자체를 함께 넘겨볼까요?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

확실히 로그에 stacktrace까지 출력하는 것이 원인을 파악하기 더 좋아보이네요!

9c74bed 커밋에 반영했습니다.

Comment on lines 35 to 37
public void deleteTime(int id) {
timeRepository.deleteById(id);
}
Copy link

@boyekim boyekim Dec 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

서버 최종 상태를 바라보는것에 동의하고, 상태코드는 사용자가 누구인지 보는 것 같아요.

"멱등성을 지켜야하므로 항상 클라이언트에게는 200을 준다" vs "멱등성을 지켜야하므로 서버에게 40x를 준다"
이렇게 판단하기보단
"해당 삭제는 유저가 리소스가 없는지 알 필요가 없기 때문에 항상 200을 준다" vs "해당 삭제는 서버(or 관리자)가 관리해야하므로 리소스가 없는 경우에는 40x를 준다/예외를 던진다" 이렇게 도메인 관점으로 보는 게 더 자연스럽다고 생각합니다.
그래서 결국 도메인의 정책을 더 신경써보시면 좋을 것 같아요.
Time의 관리는 관리자가 하게 될 것 같은데, 관리자는 리소스가 없는걸 알아야할 수도 있으니까요~!

Comment on lines 35 to 37
public void deleteTime(int id) {
timeRepository.deleteById(id);
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Time 삭제에서 추가 코멘트 하나 남기겠습니다!
만약 해당 시간의 Reservation이 존재하는 경우에 Time이 삭제될 경우를 생각해봐도 좋을 것 같습니다.
외래키 제약으로 Reservation이 있는 경우 삭제가 안되고 있긴 하나, 명확하게 예외를 던져주는것이 유지 보수 면에서도 더 좋아요
예외를 더 이해하기도 쉽고, DB가 막아주는 것 보단 정책으로 명시해주는게 올바른 설계일 것 같습니다.
수한님의 프로젝트라고 생각하구 정책을 주체적으로 생각해보셔도 좋을 것 같네요🐥👍

@chemistryx
Copy link
Author

아 직렬화의 경우 jackson이 자체적으로 처리해주는군요!
Spring의 경우 자체적으로 처리해주는 친구들이 많다보니 동작을 한번에 이해하기가 좀 어려운 부분이 있더라구요 ㅎㅎ
마샬링이라는 용어는 처음 들어보는데, 한번 학습해보겠습니다!

추가로 리뷰 코멘트 달아주신 부분은 모두 반영 완료했으니 한번 확인 부탁드립니다!

Copy link

@boyekim boyekim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

늦게 리뷰 추가했음에도 불구하고... 하나만 더 코멘트 남기겠습니다...!ㅎㅎ
비즈니스 로직이 DB의 예외에 의존하는 것에 대해서 쪼끔 더 생각해보시면 좋을 것 같아요.
시험기간이라고 들었는데 천천히 반영하셔두 됩니다.
화이팅!!

Comment on lines +43 to +52
public void deleteTime(int id) {
try {
int result = timeRepository.deleteById(id);

if (result == 0) throw new TimeNotFoundException("시간이 존재하지 않습니다.");
} catch (DataIntegrityViolationException e) {
throw new TimeInUseException("예약에 사용중인 시간은 삭제할 수 없습니다.");
}
}
}
Copy link

@boyekim boyekim Dec 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이 부분을 가독성 좋게 개선해보는걸 한번 시도해볼까요?

  1. try-catch로 포괄적인 DataIntegrityViolationException 예외를 그냥 터뜨리고 잡아버리는 방식일 경우, 팀원이 비즈니스 정책을 명확히 알 수 있을까요? DataIntegrityViolationException는 다양한 케이스를 포함하고 있습니다. 여기서 일부 케이스를 비즈니스 정책을 표현하는 데에 사용하는 것은 가독성을 저하시킨다고 생각합니다.
  2. 실패할 delete를 시도해보는 것에 수한님의 의도가 있을까요? 실패할 쿼리를 실행하고, 롤백시키는 과정은 비효율적일 수 있을 것 같습니다

추상적인 DB 예외에 의존하지 않고, 비즈니스 코드만 읽고 정책의 의도를 표현할 수 있다면 가독성이 굉장히 향상되어서 같이 일하는 사람도 즐거운 코딩생활을 할 수 있을 것 같아요. 이걸 달성하기 위해서는 비즈니스 정책을 DB 예외 발생 전에 검사하는 것도 방법일 것 같습니다.
ex)
"time id에 맞는 값이 존재하는지 확인하는 쿼리 실행" -> "없는것이 예외상황이라면 예외 발생" / "있다면 예약에 사용되는 time인지 확인하는 쿼리 실행" -> "예약에 이미 걸려있다면 예외 발생" -> "걸린 예약 없다면 삭제 진행"

저의 위 주장들에 대해 한번 생각해보시고, 수한님의 의견을 말씀해주세요!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants