Skip to content

Conversation

@chemistryx
Copy link

@chemistryx chemistryx commented Nov 6, 2025

승현님 안녕하세요! 이번 Spring MVC 미션 리뷰이로 참여하게 된 하수한이라고 합니다!
이번 PR에서는 1단계 - 홈 화면, 2단계 - 예약 조회, 3단계 - 예약 추가 / 취소, 4단계 - 예외 처리 네가지 단계를 진행하였습니다.

현재 프로젝트 구조는 다음과 같습니다:

controller/
- HomeController.java : 메인 화면 관련 endpoint를 처리하는 컨트롤러
- ReservationController.java : 예약 관련 endpoint를 처리하는 컨트롤러
dto/
- ErrorResponse.java : 오류 응답 정보를 담을 객체
- ReservationCreateRequest.java : 예약 생성 요청 Payload를 담을 객체
- ReservationCreateResponse.java : 예약 생성 요청 응답 정보를 담을 객체
exception/
- GlobalExceptionHandler.java : 공통 예외 핸들러
- ReservationNotFoundException.java : 예약을 찾을 수 없을 때 발생하는 Custom Exception
- ReservationValidationException.java : 예약 값 검증에 실패했을 때 발생하는 Custom Exception
model/
- Reservation.java : 예약 데이터를 저장하기 위한 Record
service/
- ReservationService.java : 예약 관련 비즈니스 로직을 담은 객체
RoomescapeApplication.java : Main entrypoint

초기에는 ReservationController에 모든 비즈니스 로직을 작성해두었으나, 기능이 점점 많아짐에 따라 비즈니스 로직을 ReservationService로 나누어 보았습니다. 생성 시 id의 경우 내부 로직에 의해 자동으로 부여되는데, 이때 AtomicInteger를 사용하여 동시에 여러 요청이 들어와도 id값이 중복되지 않도록 처리하였습니다.

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

  1. 현재 ReservationCreateRequest 관련 예외 처리를 ReservationService에서 수행하고 있습니다. 지금 단계에서는 인자가 그렇게 많지 않아 값 검증 로직에 있어 부담이 없어 보이긴 하나, 만약 Payload에 많은 양의 인자가 들어오고, 이를 모두 검증해야 할 필요가 있다면 어떠한 방식으로 검증 로직을 설계하는 것이 가장 좋은 접근인지 궁금합니다.

  2. 현재 생성 요청, 생성 응답에 대해 DTO가 모두 작성되어 있습니다. 물론 DTO를 통해 응답 값을 필터링 한다거나 계층 간 분리를 명확하게 할 수 있다는 점에서 DTO는 필요하다고 생각합니다. 다만, 규모가 커짐에 따라 DTO 역시 많아질 것으로 예상이 되는데, 이때 만약 엔티티(e.g., Reservation)의 수정이 필요하여 수정하게 되면, 기존에 작성되었던 DTO 역시 이에 맞춰 수정이 이루어질 필요가 있다고 생각합니다.
    제 생각엔 이러한 과정이 굉장히 번거로울 것으로 보이는데, 이를 해결할 수 있는 방법이 있는지, 아니면 이를 감수하고서라도 DTO를 사용함에 있어서 얻는 장점이 더 명확하여 제가 가진 우려의 경우 무시해도 되는 수준인지 궁금합니다!

다음은 지난 1, 2단계 미션 관련 코멘트 / 질문 사항이었으나, 제 나름대로 이유를 찾아본 뒤 코멘트를 달아보았습니다. 승현님께서 한번 보시고 관련해서 더 해주실 말씀이 있다면 언제든지 달아주세요!

지난 1, 2단계 미션 관련 코멘트

기존 boilerplate 코드에서는 웹 관련 의존성이 추가되어있지 않아 build.gradlespring-boot-starter-web 의존성을 추가했습니다.
추가로 클라이언트 요청 시 HTML 파일을 응답하기 위해 thymeleaf 템플릿 엔진을 도입하였습니다.

예약 데이터의 경우 Reservation 모델을 통해 관리할 수 있도록 구현하였습니다!

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

  1. 초기에 예약 데이터를 저장하기 위한 객체를 생성할 때 아래와 같이 VO를 만들었는데, 이를 Controller에 적용한 후 테스트를 해보니 다음과 같은 오류가 발생했습니다. 일단 Record로 변경하여 해결하긴 했는데, 어떠한 조건에서 다음과 같은 오류가 발생하는 것인지 궁금합니다!
    ➡️ 관련해서 찾아보니 Spring의 경우 내부적으로 Jackson이라는 라이브러리를 활용하여 JSON 직렬화를 수행하는데, 이 과정에서 접근할 수 있는 필드(getter)가 없어 직렬화를 할 수 없어 발생한 오류로 이해했습니다.

기존 Reservation VO:

public class Reservation {
    private final int id;
    private final String name;
    private final String date;
    private final String time;

    public Reservation(int id, String name, String date, String time) {
        this.id = id;
        this.name = name;
        this.date = date;
        this.time = time;
    }
}

발생 오류:

com.fasterxml.jackson.databind.exc.InvalidDefinitionException: No serializer found for class roomescape.model.Reservation and no properties discovered to create BeanSerializer (to avoid exception, disable SerializationFeature.FAIL_ON_EMPTY_BEANS) (through reference chain: java.util.ImmutableCollections$ListN[0])

긴 글 읽어주셔서 감사드리며, 이번 리뷰도 잘 부탁드립니다!

Copy link

@BackFoxx BackFoxx left a comment

Choose a reason for hiding this comment

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

안녕하세요! 1-4단계 미션을 함께 구현하여 제출해주시느라 고생 많으셨습니다.
코드 리뷰와 함께 질문에 대한 제 의견을 남길게요.

1. ReservationCreateRequest 검증을 ReservationService에서 하는 것

ReservationCreateRequest를 대상으로 실행하는 검증 과정(이름이 있는가, 시간이 있는가)은 결국 Reservation이라는 도메인 객체를 만들기 위한 검증 과정입니다.
'도메인 객체와 관련된 작업은 도메인 객체 안으로 응집한다'라는 객체지향 개념을 중심에 두고자 한다면, 작성해주신 검증 로직은 Request도 Service도 아닌 Reservation 안에 있어야 자연스럽겠죠.
그런데 현재 단계에서의 검증 작업은 값 존재 유무만 검증하는 굉장히 단순한 작업입니다. 이 정도로 당연한 검증을 굳이 도메인 안에서 해야 하냐, 도메인에는 더 복잡하고 중요한 검증 작업만 넣고, 이렇게 단순한 일은 밖에서 미리 거르는 게 좋겠다 생각하실 수도 있습니다. 그 때는 spring boot에서 제공하는 bean validation 기능이 큰 도움이 됩니다. 이 키워드로 학습해 보시고 한번 적용해서 어떤 게 더 편해지나 실험해보시는 것을 추천드립니다.

2. 엔티티가 수정되면 DTO도 수정해야 하니 DTO가 많아지면 유지보수가 힘들지 않은가

엔티티가 수정됐을 때 같이 패치해야 하는 범위가 DTO에서 끝난다면 그건 아주 잘 설계된 것입니다.
보통 클래스가 늘어나는 게 번거로워서 DTO를 안 두거나 엔티티의 속성을 그대로 따라가는 유명무실 DTO를 만들어 사용하는 경우가 많은데,
이 때 엔티티에 변경이 발생하면 변경이 일어난 영역 너머의 수많은 레이어에 변경이 전파됩니다.
DTO 몇개 고치는 것보다 더 무서운 일이 일어나지요.
그런 점에서 DTO는 수도꼭지같은 존재입니다. 물이 집에 넘쳐흐르지 않고 수도꼭지 선에서 물이 멈추게 해주지만, 꼭지를 직접 돌리는 수고 정도는 감수해야 합니다.

import roomescape.model.Reservation;

public class ReservationService {
private final AtomicInteger id;

Choose a reason for hiding this comment

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

좋은 시도입니다.

그런데 ReservationController에서 ReservationService를 사용할 때,
new ReservationService()로 새 인스턴스를 만들어 사용하고 있습니다.
만약 ReservationService2Controller가 생겼을 때 거기서도 new ReservationService() 방식으로 이 서비스를 사용한다면, id값이 0부터 다시 카운팅되어 중복 id를 가진 Reservation이 생길 가능성이 있겠네요.
위 문제를 방지하려면 어떤 방법이 있을까요?

Copy link
Author

Choose a reason for hiding this comment

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

해당 문제의 경우 ReservationService에 싱글톤 패턴을 적용하여 모든 컨트롤러가 같은 인스턴스를 사용하게 하는 방식으로 해결할 수 있을 것 같습니다!

이와 관련해서 혹시 Spring 자체적으로 처리하는 방식이 있는지 궁금하여 찾아보니 @Service 어노테이션을 사용하면, 해당 어노테이션이 붙은 클래스를 Spring에서 Bean으로 등록하여 자체적으로 싱글톤 객체로 관리해준다는 것을 확인하여 적용해보았습니다!

171cc5c 커밋에 반영했습니다.

}

public Reservation createReservation(ReservationCreateRequest request) {
if (request.name() == null || request.name().isBlank()) {

Choose a reason for hiding this comment

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

Spring Framework에는 util이라는 패키지를 제공해주는데, 위 코드를 한 번에 검증해주는 메서드가 있을 겁니다. 찾아보세요!

Copy link
Author

Choose a reason for hiding this comment

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

찾아보니 org.springframework.util 아래에 StringUtils::hasText()라는 헬퍼 메소드가 있는 것을 확인했습니다!
따라서 해당 메소드를 사용하여 검증 로직을 구성하도록 수정했습니다..!

ea93181 커밋에 반영했습니다.

throw new ReservationValidationException("시간은 공백일 수 없습니다.");
}

Reservation reservation = new Reservation(id.incrementAndGet(), request.name(), request.date(), request.time());

Choose a reason for hiding this comment

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

date와 time을 String 그대로 영속화하는 것보다 더 자연스러운 형태로 영속화하는 것이 확장성 면에서 더 유리합니다.
어떻게 저장해야 더 자연스러울까요?

Copy link
Author

Choose a reason for hiding this comment

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

아무래도 date는 날짜, time은 시간을 의미하니 이와 관련된 타입(LocalDate, LocalTime)을 쓰는 방식이 값 검증이나 날짜/시간 연산 측면에서 더 바람직한 것 같습니다.

해당 타입에 맞게 model 및 DTO에 대한 수정을 진행했습니다!

3c2b610 커밋에 반영했습니다!

boolean removed = reservations.removeIf((reservation -> reservation.id() == id));

if (!removed) {
throw new ReservationNotFoundException("예약을 찾을 수 없습니다.");

Choose a reason for hiding this comment

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

'Http 메서드의 멱등성' 키워드로 레퍼런스를 찾아보세요,
찾아보신 후, 이미 제거된 요소를 제거하려 할 때 예외를 뱉는 것이 좋은지, 아니면 다른 의견을 갖게 되셨는지 수한님의 생각을 알려주세요!

Copy link
Author

Choose a reason for hiding this comment

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

HTTP 메소드의 멱등성에 대한 개념은 처음 접해보는지라 관련해서 한번 찾아보았습니다.

멱등성 (Idempotency): 같은 요청을 여러번 보내더라도 결과가 동일하게 보장되는 속성

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

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

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


public class ReservationService {
private final AtomicInteger id;
private final List<Reservation> reservations;

Choose a reason for hiding this comment

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

동시성을 우려해 AtomicInteger를 쓰셨는데, 그럼 이 List는 동시성에서 안전한 자료구조인가요?

Copy link
Author

@chemistryx chemistryx Nov 13, 2025

Choose a reason for hiding this comment

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

현재 생성 요청 시 증가되는 ID 값에 대한 동시성 문제만 고려하고 있는데, 말씀해주신대로 실제 Reservation이 배열에 삽입 / 삭제되는 과정에서도 동시성 문제가 발생할 수 있다는 사실을 간과한 것 같습니다. 이와 관련해서 ArrayList 기반 구조를 유지하면서 동시성이 보장되는 자료구조에 대해 찾아보니 크게 두가지가 있었습니다.

  1. Collections.synchronizedList()
  2. CopyOnWriteArrayList

두 자료구조 모두 thread-safe하여 동시성이 보장된다는 것은 확인하였지만, CopyOnWriteArrayList의 경우 쓰기 시 새로운 배열을 복사한 뒤 교체하는 방식을 통해 thread-safety를 구현하기 때문에 쓰기 연산 성능에서 단점이 존재한다고 파악하였습니다. ResevationService의 경우 예약 추가/삭제가 빈번할 것으로 예상되기에 쓰기 성능에서 상대적인 이점을 가진 Collections.synchronizedList()를 도입하여 수정을 진행했습니다!

147570b 커밋에 반영했습니다.


@ResponseBody
@GetMapping("/reservations")
public List<Reservation> getReservations() {

Choose a reason for hiding this comment

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

위처럼 사용하는 것과 @RestController를 사용하는 것 중 어느 쪽을 더 선호하시나요? 선호하시는 이유도 궁금합니다!

Choose a reason for hiding this comment

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

추가로, 이 메서드 그대로 @ResponseBody만 떼면 어떤 일이 일어나나요?

Copy link
Author

Choose a reason for hiding this comment

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

위처럼 사용하는 것과 @RestController를 사용하는 것 중 어느 쪽을 더 선호하시나요? 선호하시는 이유도 궁금합니다!

@RestController의 경우 @Controller@ResponseBody가 결합된 형태와 같다고 알고있습니다. 보통의 JSON 데이터만을 반환하는 컨트롤러의 경우 @RestController를 선호하긴 하나, 현재 구현의 경우 컨트롤러 내부에 view template을 반환하는 로직이 함께 있어 기본 형태는 @Controller로 정의하고, JSON 데이터로 반환이 필요한 엔드포인트의 경우 @ResponseBody 어노테이션을 추가하여 JSON 데이터를 반환할 수 있도록 설정해두었습니다!

Copy link
Author

Choose a reason for hiding this comment

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

추가로, 이 메서드 그대로 @ResponseBody만 떼면 어떤 일이 일어나나요?

말씀해주신 상황을 테스트해보니, 현재 ReservationController의 경우 @Controller 어노테이션이 달려 있어 @ResponseBody가 없는 경우 view template을 우선적으로 찾으려고 시도하는것을 확인할 수 있었습니다. 하지만 해당 메소드의 경우 view template를 리턴하지 않기 때문에 다음과 같은 오류가 출력되고 있음을 확인했습니다:

org.thymeleaf.exceptions.TemplateInputException: Error resolving template [reservations], template might not exist or might not be accessible by any of the configured Template Resolvers
	at org.thymeleaf.engine.TemplateManager.resolveTemplate(TemplateManager.java:869) ~[thymeleaf-3.1.1.RELEASE.jar:3.1.1.RELEASE]
	at org.thymeleaf.engine.TemplateManager.parseAndProcess(TemplateManager.java:607) ~[thymeleaf-3.1.1.RELEASE.jar:3.1.1.RELEASE]
	at org.thymeleaf.TemplateEngine.process(TemplateEngine.java:1103) ~[thymeleaf-3.1.1.RELEASE.jar:3.1.1.RELEASE]
	at org.thymeleaf.TemplateEngine.process(TemplateEngine.java:1077) ~[thymeleaf-3.1.1.RELEASE.jar:3.1.1.RELEASE]
	at org.thymeleaf.spring6.view.ThymeleafView.renderFragment(ThymeleafView.java:372) ~[thymeleaf-spring6-3.1.1.RELEASE.jar:3.1.1.RELEASE]
	at org.thymeleaf.spring6.view.ThymeleafView.render(ThymeleafView.java:192) ~[thymeleaf-spring6-3.1.1.RELEASE.jar:3.1.1.RELEASE]
	at org.springframework.web.servlet.DispatcherServlet.render(DispatcherServlet.java:1415) ~[spring-webmvc-6.0.9.jar:6.0.9]
	at org.springframework.web.servlet.DispatcherServlet.processDispatchResult(DispatcherServlet.java:1159) ~[spring-webmvc-6.0.9.jar:6.0.9]

@@ -0,0 +1,3 @@
package roomescape.model;

public record Reservation(int id, String name, String date, String time) { }

Choose a reason for hiding this comment

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

도메인 객체를 record로 설계하셨군요!
나중에 예약 시간이나 예약자 이름이 변경되는 경우처럼
특정 Reservation을 수정하는 경우엔 어떻게 대응하실 계획이신가요?

Copy link
Author

Choose a reason for hiding this comment

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

현재 record로 설계된 구조 특성상 immutable하기 때문에 만약 값에 대한 수정이 필요하다면 변경 값이 들어간 새로운 객체를 생성하여 교체하는 방식으로 진행해야할 것 같습니다!

public ResponseEntity<Void> deleteReservation(@PathVariable int id) {
reservationService.deleteReservation(id);

return ResponseEntity.noContent().build();

Choose a reason for hiding this comment

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

ResponseBody를 사용하지 않는 요청의 반환 타입으로 ResponseEntity를 쓸 수도 있지만
@ResponseStatus를 이용하여 응답 Status를 명시하는 방법도 있습니다. 어느 쪽을 선호하시나요?

Copy link
Author

Choose a reason for hiding this comment

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

구현 당시에는 ResponseEntity를 통한 방법밖에 몰라서 이러한 방식으로 구현을 진행했습니다. 승현님께서 말씀해주신 @ResponseStatus어노테이션을 사용하면 굳이 리턴타입을 명시하지 않아도 된다는 점에서 더 효율적인 것 같아 해당 방식으로 수정을 진행했습니다!

b28c395 커밋에 반영했습니다.

@chemistryx
Copy link
Author

chemistryx commented Nov 13, 2025

검증 로직의 scope와 DTO에 관한 승현님의 좋은 인사이트 감사합니다! 굉장히 궁금했던 내용이었는데, 덕분에 궁금증을 해소할 수 있었습니다 ㅎㅎ..

다음은 언급해주신 bean validation 적용 과정에서 발생한 질문 사항입니다..!

ReservationCreateRequest에 대한 검증을 말씀해주신 bean validation을 적용하여 한번 수정해보았는데, 개인적으로는 입력단에서 바로 검증을 수행하여 controller, service와 같은 다른 비즈니스 로직 입장에서 검증에 대한 부담을 덜 수 있다는 점에서 매력적이라고 생각했습니다!

다만 적용 후 테스트해보니 name의 경우에는 공백 체크가 정상적으로 이루어지지만, datetime의 경우 @NotNull을 통해 null 여부만 체크를 하는데, 이 과정에서 만약 값은 존재하나 이상한 포맷(e.g., 22:0)이 입력된 경우 GlobalExceptionHandler::handleValidationException단에서 걸리지 않고 최하단 handleFallbackException에서 걸려 400이 아닌 500이 응답되는 현상이 발생되고 있습니다. 이 경우 어떤 방식으로 검증 처리를 해야 이 부분도 핸들링할 수 있을까요..?

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