Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Spring JDBC] 안금서 미션 제출합니다. #373

Open
wants to merge 42 commits into
base: goldm0ng
Choose a base branch
from

Conversation

goldm0ng
Copy link

안녕하세요 코코닥!
리뷰어로 매칭된 건 처음이네요. 아주 기대가 되는데요?? 🤥
제가 매번 리뷰어님들께 드리는 말이지만, 사소한 거라도 날카로운 리뷰 부탁드립니다!

이번 미션은 Spring MVC에서 받은 리뷰를 기반으로, 레이어드 아키텍쳐를 잘 적용해보려고 노력하였습니다! 기존 Spring MVC에서는 메모리 기반 MemoryReservationRepository를 사용했었는데, 이번 미션에서 JdbcTemplate를 사용하는 것을 고려하여 예약 관련 ReservationRepository를 인터페이스로 정의해놓고 JdbcReservationRepository를 구현하였습니다!

기존 미션에서는 Controller에 데이터 저장부터 비즈니스 로직까지, 너무 많은 역할과 책임을 위임했었는데요. 이번 미션에서는 Repository를 이용해 데이터를 관리하는 역할과 책임을 따로 분리하고, 비즈니스 로직을 따로 관리하는 service를 구현함으로써 코드의 유지보수가 용이하도록 구현해보았습니다.

미션 하면서 고민했던 것은, Dto를 어디서 처리해야할지에 대한 부분이었습니다. 저는 reservationId를 생성해 최종 Reservation 객체를 만들어 저장하는 역할은 데이터 관리 책임이 있는 Repository라고 판단했습니다. 혹시 더 좋은 의견이 있으시다면 코코닥의 생각을 공유해주세요!!

잘 부탁드립니다 코코닥

Copy link

@kokodak kokodak left a comment

Choose a reason for hiding this comment

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

안녕하세요 금서님! 리뷰가 늦어 죄송해요 ㅠㅠ

PR 본문에 남겨주신 이모티콘이 수상하지만.. 열심히 리뷰 남겨보았어요.

적어주신 질문은 리뷰 코멘트에서 언급했으니, 하나씩 반영해보시면서 금서님의 생각을 들려주세요. 궁금하신 점 있다면 언제든지 연락주시구요!

파이팅~

import lombok.Setter;

@Getter
@Setter
Copy link

Choose a reason for hiding this comment

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

DTO는 Data Transfer Object 라는 뜻인데요, 뜻 그대로 데이터를 안정성있게 잘 전송하기 위해 존재하는 객체예요.

그렇다면 어떻게 안정성있게 잘 전송 한다는 것일까요?
그것은 아마, 불변 이라는 개념과 깊게 연관되어 있을 거예요. 즉, 한번 정의한 DTO 객체 내부의 데이터는 누군가에 의해 오염되지 않아야 한다는 뜻이기도 합니다.

@Setter는 어떤가요? 이 목적에 어울리는 행위일까요? 🤔

DTO 라는 객체를 관례적으로만 만들어 사용하진 않았는지 생각해보시고, 왜 DTO를 사용해야 하는지, 어떻게 사용해야 할지를 고민해보시면 좋겠습니다.

Copy link
Author

Choose a reason for hiding this comment

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

앗 제가 @RequestBody로부터 Dto 데이터를 객체로 변환하는 과정에서 @Getter와 @Setter 둘 다 필요하다고 착각을 했네요! Dto를 사용하는 목적은 데이터를 안정성 있게 전달하는 목적으로 사용하는 건데, @Setter로 인해 외부로부터 데이터가 오염될 수 있겠어요... 🥶 수정하겠습니다.

처음에 DTO를 사용할 때, 저는 꼬꼬닭님께서 짚어주신 "DTO 객체를 관례적으로만 만들어서 사용"헸던 것 같아요. 이 점은 스스로 반성해야할 부분이라고 생각합니다! 구조를 짜고 구현할 때 내가 왜 이걸 사용했는지 명확한 목적이 있어야 할 것 같아요.

지난 MVC 미션을 하면서 DTO를 왜 사용해야하는지에 대해 직접 와닿지 않아서 조금 답답했었는데요!
지난 미션에서 DTO를 도입할 때, 문득 Entity를 그냥 넘기면 더 코드도 간결해지고 편할텐데 왜 불편함을 감수하면서까지 DTO라는 객체를 만들어서 보내는 것인지에 대해 궁금했습니다.

우선 Entity를 직접 반환할 경우 생기는 문제점에 대해 정리해봤습니다!

  1. 엔티티 구조를 변경한다면?
    만약 요구사항 변경에 의해 엔티티의 필드 이름이 변경될 경우, API 스펙이 변경되어 효과적인 유지 보수가 어려워진다.
    ex) Reservation Entity에서 필드 이름 name이 userName으로 변경되었을 경우, name 필드를 사용하던 프론트엔드 코드는 모두 깨진다. 따라서 userName으로 하나하나 변경해줘야 한다.
  1. 필요한 데이터만 전송하기 어렵다.
    엔티티를 직접 반환하면 엔티티에 존재하는 모든 데이터가 반환된다. (모든 데이터를 반환하게 되면 트래픽이 증가할 수 있으며 성능 및 비용 면에서도 현저한 차이를 가져올 수 있다고 한다.) 즉, 사용자가 필요로 하는 데이터만 전송하기에 어려움이 있다. 이로 인해 불필요한 데이터가 전달될 수 있다.
  1. 보안 문제
    Entity를 반환하면 테이블을 공개하는 것이나 다름 없으므로 민감한 정보가 노출될 가능성이 있다.
    ex) Entity에 새로운 필드 password가 추가 된다고 가정해본다면?
    - 비즈니스 요구사항에 따라 추가된 password 필드가 클라이언트로 그대로 노출된다. 이는 보안 문제로 이어질 수 있다.
  1. 순환 참조 문제
    Entity 간 양방향 관계가 존재할 경우, Entity를 반환하는 순간 순환 참조로 인한 무한 JSON 직렬화 이슈가 발생할 수 있다.

그렇다면 DTO와 Entity를 분리하게 된다면 얻는 장점은 무엇일까요?

  1. view와 model의 분리
    DTO는 view와 controller간의 인터페이스 역할을 하며, Entity는 model의 역할을 한다. 이러한 분리를 통해 MVC 패턴을 적용해 코드의 가독성과 유지보수를 용이하게 할 수 있다.
  1. 필요한 데이터만 선별해 서버 사용량 최소화
    DTO는 서버에서 클라이언트에 데이터를 전송하는 데 사용된다. 따라서 Entity 범위에서 필요한 필드를 재정의하여 필요한 데이터만 전송될 수 있도록 할 수 있다. 이로 인해 전송하는 데이터양과 네트워크 대역폭 사용량이 최적화되어 더 빠른 응당시간과 전송시간을 얻을 수 있다.
  1. 엔티티 구조가 변경되어도 안전
    Entity 구조가 변경 되더라도 DTO를 사용하여 데이터 전송을 처리하면 이 변경 사항이 클라이언트에 직접적으로 영향을 미치지 않는다. DTO를 사용하면 Entity의 변경으로 인한 영향을 최소화할 수 있으며, 클라이언트에게 필요한 데이터만 전달할 수 있다. 서버 측에서는 Entity의 변경을 해결하고 필요한 필드를 추가하거나 수정한 DTO를 클라이언트에 노출시키면 된다. 이로써 클라이언트와 서버 간의 결합도가 낮아지고 유지보수가 용이해진다.
  1. 순환 참조 예방
  1. Validation 코드와 모델링 코드 분리

6.보안 강화
DTO와 Entity를 분리함으로써, 테이블 구조는 서버측에만 알 수 있으므로 API 보안성을 강화시킬 수 있다.

Comment on lines 11 to 21
@NotBlank(message = "예약자 이름을 입력하세요.")
private String name;

@NotBlank(message = "예약 날짜를 입력하세요.")
private String date;

@NotBlank(message = "예약 시간을 입력하세요.")
private String time;

public ReservationDto() {
}
Copy link

Choose a reason for hiding this comment

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

위 리뷰의 컨텍스트를 이어서 답해보자면, DTO는 불변으로 만드는 것이 좋다고 생각해요.

그렇다면 각 필드에 final 키워드를 붙여볼 수 있겠는데, 현재는 빈(empty) 생성자가 있어서 필드 값을 불변으로 만들 수는 없겠네요.

아마 빈 생성자를 두신 이유는, @RequestBody 를 통해 unmarshalling(데이터 → 객체)할 경우 에러가 터지지 않기 위함이라고 생각합니다.

여기서 한 가지 문제는, application build를 Gradle로 하는지, IntelliJ로 하는지에 따라 빈 생성자 유무에 따른 에러 발생 여부가 달라진다는 점이에요.
https://konghana01.tistory.com/594

이런 일관성 없는 빌드 결과와, 그로 인한 러닝 커브 증가와 같은 것들이 자바 진영의 최대 문제점이라고 생각하는데요. 이를 안정성 있게, 그리고 일관성 있게 처리하기 위해 나온 대안이 record 타입이에요.

record 타입이 무엇인지, 왜 DTO를 record로 만들면 좋은지에 대해 학습해보시고, 기존의 DTO 클래스를 record 타입으로 변경해보시면 좋을 것 같아요. (금서님이 학습한 record 타입에 대한 설명을 간략하게 소개해주시면 좋겠어요 ㅎㅎ)

Copy link
Author

Choose a reason for hiding this comment

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

아마 빈 생성자를 두신 이유는, @RequestBody 를 통해 unmarshalling(데이터 → 객체)할 경우 에러가 터지지 않기 위함이라고 생각합니다.

네 맞습니다!

record 타입이 무엇인지, 왜 DTO를 record로 만들면 좋은지에 대해 학습해보시고, 기존의 DTO 클래스를 record 타입으로 변경해보시면 좋을 것 같아요.

학습해본 후 적용해보겠습니다~!

Copy link
Author

Choose a reason for hiding this comment

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

학습한 내용을 간단히 적어보겠습니다!

record는 객체 간에 불변 데이터를 전달하는 작업을 간단히 해주는 클래스 타입입니다. 불변 객체를 간단히 정의하기 위한 목적으로 설계되었다고 합니다.

[특징]

  • 멤버변수는 private final로 선언됩니다.
  • 필드별 getter가 자동으로 생성됩니다. (주의: getX()가 아닌 x()로 네이밍 처리)
  • 모든 멤버변수를 인자로 하는 public 생성자를 자동으로 생성합니다.
  • equal, hashcod, toString과 같은 표준 메서드를 자동으로 생성합니다.
  • 기본생성자는 제공하지 않으므로 필요한 경우 직접 생성해야 합니다.
  • Bean Validation과 같은 annotation 사용 가능
  • 상속은 불가능합니다.

DTO를 record 타입으로 사용하는 이유

  1. 간결성 : DTO 같은 데이터 전용 클래스를 정의할 때 필요한 보일러 플레이트 코드를 줄일 수 있습니다.
    • 보일러 플레이트 코드란? 특정한 기능을 구현하기 위해 반복적으로 작성해야 하는 코드 블럭
  2. 명확한 의도 : record를 사용함으로써 해당 클래스가 데이터 전용임을 명확히 나타낼 수 있습니다.
  3. 불변성 : record는 데이터의 불변성을 보장해 버그 발생을 최소화하고 코드의 안정성을 향상시킵니다.

Copy link
Author

Choose a reason for hiding this comment

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

흠 여기서 의문점이 하나 있습니다! record 타입으로 변경 후, 기본 생성자를 따로 설정해주지 않아도 @RequestBody 를 통해 unmarshalling(데이터 → 객체)가 에러 없이 잘 동작하네요... 🤔 이 부분에 대해 고민을 좀 해봐야겠어요

@@ -0,0 +1,28 @@
package roomescape.dto;
Copy link

Choose a reason for hiding this comment

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

레이어드 아키텍처를 언급해주셔서, 금서님께 궁금한 점을 몇가지 여쭤볼게요.

  1. 레이어드 아키텍처는 왜 적용하셨나요?
  2. 레이어드 아키텍처에서, 각 계층은 보통 패키지로 구현하게 되는데요. 그런 의미에서 봤을 때, dto는 계층이 될 수 있나요? controller나 service 패키지와 같은 레벨에 속해있어서 여쭤봤어요.
  3. 만약 dto가 계층이 될 수 있다면, 어떤 계층 사이에 존재해야할까요?

Copy link
Author

Choose a reason for hiding this comment

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

  1. 전 미션인 Spring MVC 미션 초반에서 domain, repository, controller 기반으로 구현하였었는데 책임의 분리가 제대로 이루어지진 않았어서 고민중이던 찰나에 루카님께서 레이어드 아키텍처라는 것을 살포시 언급주셨습니다! 학습해본 결과, 레이어드 아키텍처의 핵심이 "관심사의 분리"이더라고요.

i) 하나의 계층에 관심사가 여러개 존재한다면 해당 계층의 응집도가 떨어지고 결합도가 낮아진다.
ii) 각 계층들을 관심사 기준으로 분리함으로써 계층의 응집도를 높이고 결합도를 낮출 수 있다.
iii) 따라서 관심사의 분리를 통해 재사용성과 유지보수성을 높일 수 있다.

제가 고민하던 내용에 대한 해결책이 레이어드 아키텍처라는 생각이 들어 바로 적용해보았습니다!

  1. 이 부분도 전 미션에서 고민을 해봤어요. DTO는 데이터를 안전하게 전송하는 역할을 하죠.. 따라서 컨트롤러와 서비스, 또는 서비스와 데이터 접근 계층 간에서 데이터를 전송하는 도구(?)일 뿐 계층이 될 순 없다고 생각합니다.

  2. presentation - business 계층 사이에 존재해야한다고 생각합니다. 제가 기존에는 presentation - business - persistence 영역까지 DTO를 전달하는 코드를 구현했었는데, 이렇게 DTO가 persistence까지 침투하게 된다면, Controller가 바뀌거나 입력 방법 혹은 요구사항이 변경되었을 때 그 변경의 영향이 persistence layer까지 전파가 되기 때문에 해당 DTO를 Entity로 변환하는 위치를 조정하는 방향으로 수정했습니다!

여기서 약간의 고민 포인트는... Controller에서 Entity로 변환해서 Service에 전달할 거냐, Controller에서 Service로 DTO를 전달하고 Service에서 변환 후 비즈니스 로직을 실행할 거냐에 대한 내용인데,,, 우선 후자로 수정하긴 했습니다만 한 번 더 고민해보는 시간을 가져보도록 할게요.

@@ -0,0 +1,10 @@
package roomescape.exception;

public class NotFoundReservationException extends RuntimeException{
Copy link

Choose a reason for hiding this comment

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

Suggested change
public class NotFoundReservationException extends RuntimeException{
public class NotFoundReservationException extends RuntimeException {

자동 정렬 한번씩 돌리면 좋을 것 같아요~

Copy link
Author

Choose a reason for hiding this comment

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

반영했습니다~

Comment on lines 21 to 24
for (FieldError error : e.getBindingResult().getFieldErrors()) {
errors.put(error.getField(), error.getDefaultMessage());
log.info("validation error on field {} : {}", error.getField(),error.getDefaultMessage());
}
Copy link

Choose a reason for hiding this comment

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

form data에 대한 예외 처리를 이런 식으로도 해볼 수 있군요! 처음 알았어요. 덕분에 배워갑니다 👍

Comment on lines 27 to 38
KeyHolder keyHolder = new GeneratedKeyHolder();

jdbcTemplate.update(connection -> {
PreparedStatement ps = connection.prepareStatement(sql, new String[]{"id"});
ps.setString(1, reservationDto.getName());
ps.setString(2,reservationDto.getDate());
ps.setString(3,reservationDto.getTime());
return ps;
}, keyHolder);

Long generatedAutoId = keyHolder.getKey().longValue();
return new Reservation(generatedAutoId, reservationDto.getName(), reservationDto.getDate(), reservationDto.getTime());
Copy link

Choose a reason for hiding this comment

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

굿~ KeyHolder 잘 써주신 것 같아요.


Reservation save(ReservationDto reservationDto);

Optional<Reservation> findById(Long reservationId);
Copy link

Choose a reason for hiding this comment

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

Optional이 아닌, 확정된 Reservation 객체를 반환하는 것은 어떨까요? 즉 예외 처리의 책임을 옮겨보는 게 어떨까 하는 제안입니다.

Optional을 반환하게 되면, Repository를 사용하는 개발자 입장에서는 이에 대한 예외 처리를 직접 해줘야하는데, 아래와 같은 단점이 있을 것 같아요.

  1. 예외 처리에 대한 중복 코드가 발생할 수 있다.
  2. Repository API 사용에 대한 개발자 경험(DX)이 안 좋을 수 있다.

Copy link
Author

@goldm0ng goldm0ng Nov 22, 2024

Choose a reason for hiding this comment

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

꼬꼬닭님의 의견에 동의합니다!

그럼 예외처리는 어디서 책임지는 것이 역할과 책임에 맞게 잘 짠 코드가 될까요?

  1. repository
  2. service

우선 repository에서 예외를 던진다면, 확정된 값을 반환함으로써 이를 사용하는 주체는 따로 예외에 대한 것을 생각하지 않아도 되니까 편할 것 같네요. 다른 방법으로는 repository는 여전히 Optional로 반환을 하되, service에서 조건을 검사해서 예외를 던지는 방법도 있을 것 같아요. 각각의 장단점을 살펴봐야 할 것 같은데, 고민해보고 다시 코멘트 남기도록 하겠습니다!

지금은 repository에서 예외를 던지도록 수정했어요!


List<Reservation> findAll();

public Optional<Reservation> delete(Long reservationId);
Copy link

Choose a reason for hiding this comment

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

A. interface 내부에 정의된 메서드는 public이 디폴트예요. 따라서 public 키워드는 제거해도 괜찮습니다.
B. delete 라는 행위에 반환 값이 필요할까요? 일단 현재 금서님의 코드에서는 이 반환 값이 의미있게 사용되는 곳은 없어보여요.

Copy link
Author

Choose a reason for hiding this comment

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

A. 네! 맞습니다! 왜 저기에만 붙였는지 의문이네요. 제거하도록 하겠습니다~
B. 그러게요,, 아마 Controller에서 "삭제하고자 하는 예약이 없을 때", Optional로 반환된 값이 빈 값이면 예외를 던져주려고 그렇게 구현했던 것 같아요. 위 리뷰에서 repository에서 예외를 던져줌으로써 확정된 객체 Reservation을 반환하도록 수정하였으니, delete 메서드도 이에 따라 수정하겠습니다.

<수정 후>
예외처리는 findById 메서드에서 해주고 있으니
delete 메서드의 첫 줄Reservation deletedReservation = this.findById(reservationId); 에서 예외처리가 자동으로 될 것이고, 이에 따라 객체를 반환해서 예외처리를 따로 해주지 않아도 될 것 같네요.
반환형은 Optional -> void로 수정하는 것이 더 나은 방향 같습니다!

Comment on lines 18 to 28
public List<Reservation> checkReservations(){
return repository.findAll();
}

public Reservation addReservation(ReservationDto reservationDto){
return repository.save(reservationDto);
}

public Optional<Reservation> deleteReservation(Long reservationId){
return repository.delete(reservationId);
}
Copy link

Choose a reason for hiding this comment

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

현재 Service 계층은 무엇을 위해 존재하는 계층일까요? repository의 메서드를 대신 호출해주고 있는 것 이외에는 큰 역할이 없는 것 같아요. 제가 느낀 ReservationService는 계층만을 위한 계층이라는 느낌이 강하네요.

레이어드 아키텍처에 너무 얽매이지 않아도 됩니다. 딱히 역할이 없는 것 같다면, 만들지 않아도 괜찮아요. 아래 글을 한번 읽어보면 좋을 것 같습니다.

싱크홀 안티 패턴


public interface ReservationRepository {

Reservation save(ReservationDto reservationDto);
Copy link

Choose a reason for hiding this comment

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

미션 하면서 고민했던 것은, Dto를 어디서 처리해야할지에 대한 부분이었습니다. 저는 reservationId를 생성해 최종 Reservation 객체를 만들어 저장하는 역할은 데이터 관리 책임이 있는 Repository라고 판단했습니다.

질문 해주셨던 내용이 아마 이 코드에 대한 내용인 것 같아요. 이에 대한 제 생각을 남겨볼게요.

현재는 운 좋게도 ReservationDto의 값으로 Reservation 도메인 객체의 내용을 채워넣을 수 있어요. 그러나 만약, 시간이 지나서 ReservationDto 값만으로 Reservation 객체를 설명할 수 없는 순간이 온다면 어떻게 해야 할까요? 이를테면, ReservationDto 필드 값 + 여러 비즈니스 로직을 통해 가공된 값들 = Reservation 객체 인 상황이 생기는 거죠.

이 상황에서 여전히 Repository에 Dto를 넘겨서 Reservation을 만들고 있다면, Reservation을 만들기 위해 필요한 비즈니스 로직이 Repository 구현체에 흘러들어갈 거예요. 이는 계층에 대한 책임 관점에서 보았을 때, 적절하지 않은 책임 분배라고 생각되어요.

그런 의미에서, 장기적으로 봤을 때는 save 메서드에 넘어갈 파라미터는 Dto 보다 Reservation 객체 그 자체가 되는 것이 좋겠다는 생각이 듭니다. Reservation 객체를 만들기 위해 필요한 비즈니스 로직을 정의하는 계층은, 아마도 Service 계층이 될 거구요.

다만, 현재는 ReservationDto 값만으로도 Reservation 객체를 충분히 만들어낼 수 있기 때문에, 지금의 상황에서 Dto를 어디서 처리할 것인가는 금서님의 판단에 맡길게요.

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