Feature/#2/login #7
Conversation
Feature/#2/login
| eternal="false" | ||
| diskSpoolBufferSizeMB="20" | ||
| timeToIdleSeconds="300" timeToLiveSeconds="600" | ||
| memoryStoreEvictionPolicy="LFU" |
There was a problem hiding this comment.
timeToIdleSeconds="300" timeToLiveSeconds="600"
도 통일성 있게 잘바꿈을 해주면 좋겠습니다.
한번 보셨는지 모르겠지만 EHCache의 설정들에 대해서 모르시다면 한번 어떤 값들에 대한 부분인지 봐주시는게 좋습니다.
이러한 설정값들에 대해 이해하면 동작방식에 대해 이해하기도 좋습니다.
|
|
||
| private final T body; | ||
|
|
||
| @Builder |
There was a problem hiding this comment.
Builder를 사용하신 이유는 무엇인가요?
코드가 돌아가는데 아무 문제도 없지만 한번 고민해보면 좋은 포인트입니다.
Builder는 빌더 패턴을 구현한 것인데요. 가급적 필수 매개변수가 4개 이상일 때 사용하면 좋은 패턴입니다.
현재는 기본 생성자로 해도 될만큼 3개의 매개변수를 갖고 있고, 향후 필드가 추가될 가능성이 적은 클래스이기 때문에 이런 부분에 대해 같이 고려해봐주시면 좋습니다.
|
|
||
| builder.append(fieldError.getField()); | ||
| builder.append(" : "); | ||
| builder.append(fieldError.getDefaultMessage()); |
There was a problem hiding this comment.
메시지에 대해 이런 형태로 에러메시지를 제공하실 수도 있지만
많이 사용하는 방법은 커스텀 메시지를 작성하고 해당 메시지로 응답처리하는 형태입니다.
@NotBlank("userId는 공백일 수 없습니다.")
| BindingResult bindingResult = e.getBindingResult(); | ||
| List<StringBuilder> message = new ArrayList<>(); | ||
|
|
||
| for(FieldError fieldError : bindingResult.getFieldErrors()) { |
There was a problem hiding this comment.
대부분 자바 코딩 컨벤션은 for if 뒤에 ( 전에 공백이 들어갑니다.
코딩 컨벤션을 적용해보세요.
|
|
||
| return userInfo; | ||
| } | ||
|
|
||
| @Override | ||
| @Cacheable(value="findUserCache", key="#id") |
There was a problem hiding this comment.
유저 아이디를 기반으로 캐싱 처리를 하고 있는데요.
해당 유저가 삭제되거나 해당 유저 정보가 변경되면 어떻게 될까요?
현재 캐싱 시간은 10분인데 그사이 유저한테 잘못된 정보가 갈수도 있을텐데요.
@CachePut @CacheEvict와 같은 설정들에 대해 알아보고 적용을 해보시길 바랍니다.
| } | ||
|
|
||
| @Override | ||
| public void validateUserId(String id) { |
There was a problem hiding this comment.
해당 메서드에 대한 테스트 케이스가 없는것 같습니다.
그리고 주요 로직에 대한 테스트 케이스 누락이 있는것 같은데요.
인텔리J에서 Project > test > java에서 우클릭 > Run 'Tests in '......' with Coverage
와 같이 해서 테스트 커버리지를 확인할 수 있습니다.
코드들이 테스트가 얼마나 커비되는지 확인도 해보세요.
| private PasswordEncoder passwordEncoder; | ||
|
|
||
| @Test | ||
| @DisplayName("패스워드 암호화 테스트") |
There was a problem hiding this comment.
해당 테스트는 단순한 로컬 확인용인가요?
PasswordEncoder와 내부 구현체에 대한 검증은 해당 구현체쪽에서 할 테스트에 속하고 이 프로젝트에서 테스트할 부분은 아닙니다. 단순한 로컬 확인용이면 @ignore 처리해주시는게 좋습니다. SpringBootTest이다보니 향후 젠킨스와 같은 툴에서 테스트를 돌리다보면 불필요하게 시간을 늘리기 때문입니다.
|
|
||
| private final LocalDateTime timestamp = LocalDateTime.now(); | ||
|
|
||
| private final int code; |
There was a problem hiding this comment.
이후의 코드들을 보면 여기서 code값에 대해서 단순히 Http 응답 상태값을 넣어주는데요.
굳이 중복으로 넣어줄 필요 없습니다. 클라이언트는 여기에 http 응답 상태를 안 넣어줘도 확인을 할 수가 있습니다.
응답값에 이러한 code값을 넣고 전달할 때는 이 애플리케이션에서 정의한 CODE값이 있을 때 넣어주는 것입니다. 별도 정의한 CODE값이 없다면 해당 code는 제거하는게 더 좋습니다.
| public class ExceptionController { | ||
|
|
||
| private static ExceptionType exceptionType(Exception e) { | ||
| return new ExceptionType(e.getClass().getSimpleName()); |
There was a problem hiding this comment.
응답의 Body값에는 유저 정보 조회시 원래는 유저에 대한 정보가 와야 하는 필드인데요.
에러가 났을 때에는 해당 에러에 대한 클래스 이름이 오는 것은 클라이언트 측에서는 불필요한 정보이고
응답 데이터의 일관성이 없어지게 됩니다.
서버쪽에서 에러나 검증 문제가 있을 때에는 응답 바디에는 null이 들어가는게 더 일관성이 있습니다.
merge를 잘못하였습니다.
#2 argumentResolver 추가 / response 리뷰반영 부터가 금주 commit 내용입니다.