[Spring MVC] 안금서 미션 제출합니다. #367
Conversation
There was a problem hiding this comment.
안녕하세요, 금서님
리뷰어 루카입니다.🐳
너무 늦어서 정말 죄송해요...ㅠㅠㅠㅠ🙇🙇🙇🙇🙇🙇🙇🙇🙇(코코닥 때문이라고 생각해주세요🤥)
🎯 리뷰 포인트
- Controller 분리하기
- 이해하기 좋은 HTTP API
- Reservation은 DTO인가?
🔮 질문에 대한 답변
Dto에 대해 공부해보고, 적용해보려고 합니다. 현재 컨트롤러에서 예외를 처리하고 있는데, 외부에서 데이터를 받을 때 유효성 검사는 Dto에서 해주는 게 훨씬 효율적인 방식인가요? 아니면 핵심 도메인에서 유효성 검사 코드나 annotation을 추가하는 게 더 나은 방법인가요?
검증 위치를 상상해볼까요?
- page view에서 입력을 받을 때 검증
- controller로 들어올 때 dto에서 검증
- 도메인 모델에서 검증
현재는 controller에서 검사하고 계신데요. 2번에 가깝겠네요. 왜 그렇게 하셨을까요?
일단 지금은 도메인 모델이 없어서 3번은 못하겠어요.
1번은 지금처럼 타임리프로하는 게아니라면 할 수 없을테니 백엔드에서 방어하는 방법은 아니겠네요.
2번이 나은지 3번이 나은지는 3번을 할 수 있는 코드가 된다면 추가로 논의해보죠.
제가 한 방식처럼, 예외 처리를 한 곳에 묶어서 관리하는게 더 효율적인 관리 방식인 것이죠?
예외 처리를 핸들링하는 ControllerAdvice를 말씀하시는것이겠죠? 해당 부분에도 리뷰를 달았는데, Global로 사용하는 것은 제가 봤을 땐 효율적으로보이진 않아요.
왜냐면 요청별로 다른 예외 응답을 줘야할 수도 있으니까요.
지금은 Controller 자체도 너무 많은 일을 하니 Controller를 쪼개보고 더 생각해봐야할 것 같아요
CRUD API에 대해 익숙하지 않아서 학습 테스트와 서치를 하며 구현해봤는데, 더 발전시킬 수 있는 방향이나 학습하면 좋을 것 같은 키워드 등등이 궁금합니다!
좋은 CRUD API는 정말 논쟁(?)이 많이 되는 이야기인데요.
이런 논의 거리에 무적 대답이 있자나요. "정답은 없다. 팀 컨벤션이 중요하다"
근데, API를 잘 작성하는 것은 저게 맞는것 같아요.
그 API를 사용하는 사람이 예상 가능하게 API를 만들어야해요.
말그대로 API는 유저에게 어플리케이션에 가장 가장자리를 제공하는 것이죠.
예상 가능한 API가 되도록 하는 많은 가이드들이 있습니다.
- HTTP에 대해 학습
- HTTP Method, Path, 요청 헤더, 요청 바디에 대한 고찰
- StatusCode, 응답 헤더, 응답 바디에 대한 고찰
- RESTful APi는 뭐지? (그런 REST API ... )
이런 단계적인 방법도 있겠죠?
근데 제가 추천하는 방법은 어떤 API를 작성하기 전에, 요구사항을 분석해서 README를 작성하고, API 명세서를 같이 작성해보는 거에요.
명세를 먼저 하는 것이 TDD를 하는 것 처럼 개발에 도움을 줄거에요.
리뷰 늦게 드려 다시한번 죄송합니다..ㅜ
궁금점 있으시면 연락 항상 주셔도되고, 리뷰 재밌게 읽어주세요.
다음 리뷰는 진짜 최선을 다해 일찍 드리겠습니다 ㅠㅠㅠㅠ
| private List<Reservation> reservations = new ArrayList<>(); | ||
| private AtomicLong index = new AtomicLong(1); |
There was a problem hiding this comment.
AtomicLong으로 ID를 부여하고,
Reservation을 List에 보관하는 프로그램이군요!! 정말 멋진 방법이네요.
이 부분이 제가 생각했을 때 꽤 중요한 부분이여서 몇가지 제안과 질문을 해보겠습니다.
There was a problem hiding this comment.
ReservationController의 역할
Reservation이라는 리소스에 접근은 이 Controller에서 밖에 못하게 해야할까요?
예를 들어, 만약 방탈출 관리자 기능 같은 새로운 API 가 만들어지면 어떻게 해야될까요?
1. MVC 패턴에서 Controller
- 이전 미션들에서 갖던 의미를 돌이켜보면 좋을 것 같아요.
2. 레이어드 아키텍처 적용?
- 레이어드 아키텍처라는 개념을 알고 계실까요?
- 간단하고 명확한 구조라서 프로젝트에 적용해보기 좋으실거에요.
- 꼭 그 구조를 따라야하는 것은 아니고 한번 고민해보죠.
3. @Controller의 의미
- Spring에서 Controller라는 Bean을 따로 명시한 이유가 뭘까요?
- 이것을 고민해보면서 저희가 Spring 같은 프레임워크를 사용했을 때 얻는 이점에 대해서 생각해보면 좋을 것 같아요.
- IoC, DI, AOP, ... 여러 이유가 있겠죠.
- 우리는
방탈출 예약 서비스를 만들기 위해 스프링을 사용한 것이죠. 스프링이 우선이 아리라는 말을 하고 싶은것인데요. - 우리는 Spring이 망해도
방탈출 예약 서비스를 서비스해야하죠! - 만약 Spring을 이 서비스에서 제거하고 다른 프레임워크로 바꾼다고 가정해보면 어떨까요?
- 현실에서 일어날 가능성이 낮은 일이지만, 이런 고민을 해보는게 @controller의 책임을 정의하는데에 도움이 될 것 같아요.
약간은 난해한 질문들을 드린 것 같아서 정리를 좀 해보자면,
이전 미션은 콘솔 어플리케이션이였는데, 웹 어플리케이션으로 변경한다고 생각하면 어떨까요?
일단 Controller를 제거하고 @controller가 붙은 이런 컨트롤러를 붙이겠죠? 이런 상상을 해보죠
There was a problem hiding this comment.
Reservation의 ID와 저장소
AtomicLong을 이용하여 ID를 부여한 것은 Reservation이 관리할 리소스(or 엔티티, or 레코드 ...)라는 의미겠죠?
지금은 메모리에 저장하고 있지만, 언젠가 RDB같은 곳에도 저장하게 될 수 있겠네요.
흠... 어제 스터디에서 JDBC 템플릿을 스터디한 것으로 알고 있는데요.
실제로 JDBC 템플릿을 적용해보거나,
그를 적용한다고 했을 때 어떤 것이 필요할까요?
그것이 현재 방식에서 비슷한 역할을 하고 있는 것은 누구일까요?
[무시해도 되는 리뷰]
Long을 사용하지 않고 AtomicLong을 사용한 것은 동시에 접근했을 때의 문제를 방지한 것으로 보이는데, 맞을까요?
그렇다면 AtomicLong만으로 충분할까요?🤔
ArrayList 정말 괜찮을까요?
정말 괜찮은지 테스트해볼까요?
| @GetMapping("/reservations") | ||
| @ResponseBody | ||
| public ResponseEntity<List<Reservation>> readReservation(){ | ||
| return ResponseEntity.ok(reservations); | ||
| } |
There was a problem hiding this comment.
[Comment]
여기에 @responsebody라는 어노테이션이 필요한가요?
전 지워봐도 잘 동작하는 것 같은데요...🤔
There was a problem hiding this comment.
그러게요,,, @responsebody는 객체의 데이터를 JSON으로 변환하여 응답 본문에 전달할 때 사용하는 것으로 알고 있는데요! 미션 초반에 어떤 방식으로 응답을 줘야할지 감이 안 잡혀서 이것저것 해보다가 붙이곤 지우지 않았네요 ㅎ.. ResponseEntity는 이미 응답 본문을 포함하고 있어 해당 어노테이션을 사용할 이유가 없군요! 새로 알아갑니다!
| @GetMapping("/reservation") | ||
| public String showReservationPage(){ | ||
| return "reservation"; | ||
| } | ||
|
|
||
| @GetMapping("/reservations") | ||
| @ResponseBody | ||
| public ResponseEntity<List<Reservation>> readReservation(){ | ||
| return ResponseEntity.ok(reservations); | ||
| } |
There was a problem hiding this comment.
[Comment]
Path를 정할 때 어떤 규칙 같은게 있을까요?
페이지를 보여주는 path는 단수고 조회하는 API path는 복수인 이유가 궁금해요
There was a problem hiding this comment.
요구사항
/reservation 요청 시 아래 화면과 같이 예약 관리 페이지가 응답할 수 있도록 구현하세요.
어드민 메인 페이지는 templates/reservation.html 파일을 이용하세요.
따로 규칙을 정해놓은 건 없습니다! 요구사항 내용입니다:)
There was a problem hiding this comment.
루카님이 말씀하셨던,
근데 제가 추천하는 방법은 어떤 API를 작성하기 전에, 요구사항을 분석해서 README를 작성하고, API 명세서를 같이 작성해보는 거에요.
명세를 먼저 하는 것이 TDD를 하는 것 처럼 개발에 도움을 줄거에요.
추천 방법을 빠른 시일 내에 적용해보아야겠네요.. 하하
| reservation.setId(index.getAndIncrement()); | ||
| reservations.add(reservation); | ||
|
|
||
| return ResponseEntity.created(URI.create("/reservations/"+reservation.getId())).body(reservation); |
There was a problem hiding this comment.
[Approve]
WOW!!! 👏👏👏👏👏👏👏👏👏👏👏👏👏👏👏👏
POST 요청을 날렸는데 ➡️ Created 201 응답이 오고 ➡️ Location 헤더에 해당 리소스의 아이디가 담겨오더라구요
정말 RESTful하다!라고 할 뻔 했으나...
[Request Change]
제가 이 API를 사용자라면 저 location 해더를 받아서 GET 요청을 날려서 생성된 정보를 받고 상세 페이지를 보여줄 것 같은데요.
@GetMapping("/reservations/{id}) 는 보이지 않네요...
혼란을 야기할 수 있는 것 같아요.
[Comment]
Location 헤더에 식별가능한 아이디를 담아서 보냈는데, 응답 바디에도 해당 정보를 담아서 보내주시는 이유가 있을까요?
There was a problem hiding this comment.
- @GetMapping("/reservations/{id}") 부분에 대해 : 제공된 테스트 케이스나 요구사항에서는 GET /reservations/{id}와 같은 상세 조회 API를 포함할 필요가 없었습니다. 하지만 루카님 말씀에 동의합니다! 만약 향후 상세 페이지를 조회하는 API가 필요할 경우, @GetMapping("/reservations/{id}") 로 해당 id를 가진 예약자의 상세 정보를 조회하는 기능을 구현해야겠네요!
| @DeleteMapping("/reservations/{id}") | ||
| @ResponseBody | ||
| public ResponseEntity<Void> deleteReservation(@PathVariable Long id){ | ||
| Reservation reservation = reservations.stream() | ||
| .filter(r -> Objects.equals(r.getId(), id)) | ||
| .findFirst() | ||
| .orElseThrow(()->new NotFoundReservationException("예악을 찾을 수 없습니다.")); |
There was a problem hiding this comment.
[Approve]
이렇게 지우려는 리소스가 없을 때, 예외 상황일까요?
없으면 오히려 좋아라고 생각할 수도 있지않을까용?
There was a problem hiding this comment.
오 그렇게도 생각할 수 있군요! 이 부분은 한 번 더 고민해보고 말씀드리겠습니다!
| @ExceptionHandler(NotFoundReservationException.class) | ||
| public ResponseEntity<String> handleNotFoundReservationException(NotFoundReservationException e){ | ||
| return ResponseEntity.badRequest().body(e.getMessage()); | ||
| } |
There was a problem hiding this comment.
[Comment]
이 에러는 id로 reservation 조회 실패한 상황에 쓰는 에러인 것으로 보이는데요.
400 배드 리퀘스트, 404 낫 파운드 둘 중 어떤 것이 적절할까요?
이건 정말 의견이 다를 수 있겠어요.
금서님은 왜 400으로 내려주셨을까요?
There was a problem hiding this comment.
저도 둘 중 고민을 하다가, 낫 파운드 상태 코드가 의미도 더 명확하고 적합하다고 생각했습니다! 근데 테스트에서는 400으로 내려야 통과되도록 설정되어있더라고요!
| package roomescape.exception; | ||
|
|
||
| public class MissingReservationDataException extends RuntimeException{ | ||
| public MissingReservationDataException(String message){ |
There was a problem hiding this comment.
[Approve]
꼭 메시지를 넣어주지 않아도 기본으로 생성되는 message가 있으면 예외 내용 파악하기도 사용성도 좋아질 것 같아용
There was a problem hiding this comment.
오 좋은 의견 감사합니다!! 해당 클래스는 삭제할 예정이라, 다른 사용자 정의 예외 클래스인 NotFoundReservationException에 적용해보았습니다!
|
|
||
| import lombok.Data; | ||
|
|
||
| @Data |
There was a problem hiding this comment.
하하... 단지 코드 몇 줄 줄일 수 있다고 아무 고려 사항 없이 사용해버렸네요. @DaTa 는 모든 필드에 대해 getter와 setter 사용이 가능한 걸로 알고 있는데, 모든 필드에 대해 setter가 열리게 되면,, 의도하지 않은 곳에서 수정이 이루어질 우려가 있을 것 같습니다.
루카님이 생각하는 가장 큰 우려 사항은 뭔지 여쭤봐도 될까요?
| public class Reservation { | ||
|
|
||
| private Long id; | ||
| private String name; | ||
| private String date; | ||
| private String time; | ||
|
|
||
| public Reservation() { | ||
| } | ||
|
|
||
| public Reservation(Long id, String name, String date, String time) { | ||
| this.id = id; | ||
| this.name = name; | ||
| this.date = date; | ||
| this.time = time; | ||
| } | ||
|
|
||
| } |
There was a problem hiding this comment.
[Request Change]
이 Reservation 객체는 DTO로서 사용하신건가요?
만약 DTO라면 저희가 이전 미션에서 만들었던 도메인 모델들이 공백이네요...
만약 현재 시간 이전 시간에 예약을 할 수 없다는 조건을 추가하면 그것은 어디로 가야할까요??
There was a problem hiding this comment.
앗 저의 의도는 dto가 아니고 도메인 모델로서 작동하도록 구현한 것인데,, 너무 정신없이(?) 미션을 하느라 코드가 엉망진창이네요 ㅎㅎㅎ... 다시 정신차리고 열심히 해보겠습니다!
pr 드릴 때 말씀드린 것처럼, dto를 적용해보려고 합니다! validation 관련 라이브러리를 추가하고 해당 dto에 클라이언트에 잘못된 데이터가 전달되는 것을 방지하는 유효성 검사를 하도록 기능을 추가해봤습니다! dto를 사용함으로써, 기존 Controller에서 하던 유효성 검사 로직을 제거할 수 있었습니다! (Law...of...킹미터 주의할게요)
그리고 알아보니, Controller에서 @Valid로 유효성 검사를 수행할 때, @notblank 조건을 위반하면 Spring은 MethodArgumentNotValidException 예외를 발생시킨다고 하네요! 기존 Controller에서 예외처리를 할 때는 MissingReservationDataException이라는 사용자 예외처리 방식을 사용했었는데, GlobalExceptionHandler에서 직접 예외를 처리하는 방식으로 수정해보았습니다!
만약 현재 시간 이전 시간에 예약을 할 수 없다는 조건을 추가하면 그것은 어디로 가야할까요??
루카님의 이 질문에는 도메인이라고 말씀드릴 수 있겠네요!
이제 슬슬 spring 미션에 대한 윤곽이 잡히는 것 같아요. 항상 감사합니다 루카님!
| if (reservation.getName().isEmpty() || reservation.getDate().isEmpty() ||reservation.getTime().isEmpty()){ | ||
| throw new MissingReservationDataException("예약 필수 정보가 입력되지 않았습니다."); | ||
| } |
There was a problem hiding this comment.
[Request Change]
Law... of... 킹...미...터
루카님 안녕하세요! 우선 미션 제출 조금 늦어진 점 죄송합니다!
벌써 세번째 리뷰라니...! 이번에도 날카롭고 아낌없는 리뷰 부탁드립니다 :)
우선 dm으로 말씀드렸던대로, 테스트는 다 통과되는 상황이지만
서버를 돌렸을 때 생기는 문제가 해결이 안 되어서 일단 미션 제출 먼저 한 후 계속 방법을 고안해보고 커밋하겠습니다!
제 상황에 대해 간단하게 말씀드리자면, 예약자와 예약 날짜까지는 잘 입력이 되는데 예약 시간 설정이 안 되는 문제가 있어서
예약 추가와 확인에 대한 기능을 구현한 것에 대해 확인을 하지 못하고 있습니다 ㅠㅠ
혹시 괜찮으시다면 같이 방법을 고안해보는 것도 좋을 것 같습니다 ㅎㅎ..
이 문제에 대해서는 최대한 빠르게 수정하는 방향으로 할게요!
그리고 추가적인 리팩토링도 차차 커밋하겠습니다!
Dto에 대해 공부해보고, 적용해보려고 합니다. 현재 컨트롤러에서 예외를 처리하고 있는데, 외부에서 데이터를 받을 때 유효성 검사는 Dto에서 해주는 게 훨씬 효율적인 방식인가요? 아니면 핵심 도메인에서 유효성 검사 코드나 annotation을 추가하는 게 더 나은 방법인가요?
제가 한 방식처럼, 예외 처리를 한 곳에 묶어서 관리하는게 더 효율적인 관리 방식인 것이죠?
CRUD API에 대해 익숙하지 않아서 학습 테스트와 서치를 하며 구현해봤는데, 더 발전시킬 수 있는 방향이나 학습하면 좋을 것 같은 키워드 등등이 궁금합니다!
이번에도 잘 부탁드립니다! 감사합니다 :)