[Spring MVC (인증)] 주민기 미션 제출합니다. #123
Conversation
There was a problem hiding this comment.
민기님~ 코드 잘 작성해주셨네요.
의견이 궁금한 부분들에 리뷰 남겼습니다.
테스트는 어디까지?
다른 사람들 리뷰에서도 남겼지만
테스트를 어디까지 해야하는지는 팀 내 컨벤션과 목적에 따라 다를거 같아요.
즉, 자신이 왜 이 테스트를 해야하고, 테스트로 얻는 이점이 뭔지를 생각하는게 중요하다고 생각합니다.
예를 들어, Controller - Service - Repository 가 원스텝으로 다 되는데
이를 ControllerTest, ServiceTest, RepositoryTest 를 만드는건 비효율적일 겁니다.
그리고, 객체지향적으로 코드를 잘 나누면 하위 계층에서 명확하게 테스트 한 걸 상위 계층에서 또 해야하는가?
( 예를 들어, 로또 번호 생성해주는 Generator 에서 예외 테스트를 다 했는데 상위 Service 에서 또 다하는건 비효율적이겠죠. )
결국 자신만이 기준이 중요할 거 같아요.
( 저는, 테스트를 통해 내가 직접 서버를 구동해서 확인하지 않아도 되는가? 를 관점으로 테스트를 설계 해 나갑니다. )
디렉토리
디렉토리 부분은 저도 멀티 모듈이나, 멀티 패키지 기반으로 프로젝트를 해본적이 없어서 자세히 모르겠지만
단순한 도메인 관점에서 봤을 때 그게 common 이라는 생각은 들지 않는거 같아요.
제가 생각하는 common 은 정말 유틸성 static 메소드랑 같다고 생각해요.
( 어떤 도메인에서든 편하게 사용가능하며, 공통 관리를 위해 존재하는 무언가 )
Resolver나 Interceptor 가 어떤 도메인에서든 적용되기 보단 인증과 인가에 국한되는 도메인이라 생각이 되는데 민기님의 의견을 들려주셔도 좋아요!
TimeProvider 부분은 제가 의도한 대로 맞습니당 👍
| .orElse(null); | ||
|
|
||
| if (token == null) { | ||
| response.sendError(HttpServletResponse.SC_BAD_REQUEST); |
There was a problem hiding this comment.
sendError 를 사용했네요. setStatus 랑 차이점이 뭔가요?
|
|
||
| MemberTokenDto memberTokenDto = tokenService.extractMemberResponseFromToken(token); | ||
| Member member = memberService.findMemberByName(memberTokenDto.name()); | ||
| if (member == null || !member.getRole().equals("ADMIN")) { |
There was a problem hiding this comment.
ADMIN 인지 Member 가 스스로 알려줘도 괜찮을거 같아요.
| NativeWebRequest webRequest, WebDataBinderFactory binderFactory) { | ||
| HttpServletRequest httpServletRequest = (HttpServletRequest) webRequest.getNativeRequest(); | ||
| Cookie[] cookies = httpServletRequest.getCookies(); | ||
| String token = cookieProvider.extractTokenFromCookie(cookies) |
There was a problem hiding this comment.
위 Interceptor 에서는 Null 을 받아 처리했고, 여기선 에외를 던졌네요.
두개의 차이점이 있나요? ( 의견 묻는겁니당 )
|
|
||
| @PostMapping("/reservations") | ||
| public ResponseEntity create(@RequestBody ReservationRequest reservationRequest, LoginMember loginMember) { | ||
| if (reservationRequest.getDate() == null |
There was a problem hiding this comment.
Request 가 스스로 자신의 값이 비어있는지 검증해줘도 될 거 같네요.
| } | ||
|
|
||
| ReservationResponse reservationResponse = reservationService.save(reservationRequest, loginMember); | ||
| return ResponseEntity.created(URI.create("/reservations/" + reservationResponse.getId())).body(reservationResponse); |
| @@ -12,7 +18,10 @@ public ReservationService(ReservationDao reservationDao) { | |||
| this.reservationDao = reservationDao; | |||
| } | |||
|
|
|||
| public ReservationResponse save(ReservationRequest reservationRequest) { | |||
| public ReservationResponse save(ReservationRequest reservationRequest, LoginMember loginMember) { | |||
| if (reservationRequest.getName() == null) { | |||
There was a problem hiding this comment.
getName 과 setName 보다 더 그럴듯한 메소드명으로 해도 될 거 같아요.
굳이 getName 이 null 인지, set 을 한다도 Service 가 알 필요 없을거 같기도 하구요.
|
|
||
| @SpringBootTest | ||
| @TestPropertySource(properties = "spring.datasource.url=jdbc:h2:mem:testdb") | ||
| @Transactional |
| } | ||
|
|
||
| @Override | ||
| public boolean preHandle(HttpServletRequest request, HttpServletResponse response, Object handler) throws |
There was a problem hiding this comment.
Interceptor 는 왜 boolean 을 return 하게 하는거 같나요?
| } | ||
|
|
||
| @GetMapping("/reservations") | ||
| public List<ReservationResponse> list() { |
There was a problem hiding this comment.
아래는 ResponseEntity 를 return 하는데, 여기는 그냥 List 를 return 해서 혼란을 줄 수도 있을거 같아요 🙂
과제 수행하면서 드는 생각들
테스트는 어디까지?
지난번 피드백을 통해 테스트코드 작성을 통해 기능적, 역할적 분리를 알 수 있을거라고 피드백 주셨고 테스트코드를 짜보니 정말로 SRP 원칙이 위배되는 코드들을 확인할 수 있어서 좋았습니다 감사합니다 :)
이번과제를 진행하면서 테스트코드를 열심히 짜봤는데 또 드는 생각이 정말 세세한 예외상황들까지 모든 걸 체크하고 가는 것인가 아니면 딱 그 기능의 테스트를 목적으로하는 정도가 맞는가요?
또한 과제 중 하나인 HandlerMethodArgumentResolver를 구현하고 이에 대한 테스트코드를 짜보려고 했는데 엄청 복잡하게 꼬여서 실패했습니다. 이러한 클래스도 모두 테스트코드를 작성하는게 맞을까요?
디렉토리 정리
이번 과제를 진행하면서 너무 디렉토리 구조가 보기 불편해서 기능별로 묶어서 정리했습니다. 클래스들을 이동하면서 제일 고민했던 부분이 intercepter, resolver 이 두 클래스입니다. 인증에 관련되었으니 auth 디렉토리에 넣는게 맞는건지, 추후에 전역적으로 사용될 가능성이 존재하니 common 디렉토리에 넣는게 맞는걸까요?
소감
이번 과제를 통해서 무심코 보고 지나갔던 부분들을 직접 구현해보고 테스트해보는 과정을 통해 인증,인가에 대한 더 깊은 이해를 할 수 있어서 좋았습니다. 또한 저번에 피드백해주셨던 TimeProvider도 적용했으니 한번 봐주시면 좋을거 같습니다!
끝으로 매서운 피드백은 언제나 환영입니다! 감사합니당