[Spring 지하철 노선도 - 1,2단계] 렉스(오성원) 미션 제출합니다. #229
Conversation
| public class DuplicateNameException extends RuntimeException { | ||
|
|
||
| public DuplicateNameException(String message) { | ||
| super(message); | ||
| } | ||
| } |
There was a problem hiding this comment.
커스텀 예외를 만들 때, 상속 받는 예외 클래스의 선택
이번 미션을 진행하며 중복된 이름에 대한 예외처리와 DB로 부터 알맞는 값을 찾지 못하는 경우에는 커스텀 예외를 발생시키도록 코드를 작성했습니다. 그런데 여기서 커스텀 예외를 생성할 때 어떤 예외 클래스를 상속받는 것이 더 좋을지에 대하여 의문이 들더라고요.
상속 예외 클래스의 경우 Exception으로부터 RuntimeException, IllegallArgumetException까지 많은 클래스들이 존재하는데 보통 커스텀 예외 클래스의 경우, 어느정도 레벨의 예외를 상속하는게 좋을까요?
개인적인 생각으로는 중복된 이름의 오류의 경우, CheckedExeption인 Exception은 올바르지 못한 후보인 것 같고 UncheckedException인 RuntimeException, IllegallArgumetException 중에서는 어떤 클래스를 상속하던간에 @ControllerAdvice에서 예외를 잡아주기에 상관이 없다고 생각되면서도 그래도 가장 구체적인 하위의 예외를 상속하는게 더 좋을까?하는 생각도 드는 것 같습니다😭
| @ExceptionHandler(DuplicateNameException.class) | ||
| public ResponseEntity<ErrorMessageResponse> duplicateStationNameException(DuplicateNameException e) { | ||
| return ResponseEntity.status(HttpStatus.BAD_REQUEST) | ||
| .body(new ErrorMessageResponse(e.getMessage())); | ||
| } |
There was a problem hiding this comment.
올바른 상태코드
지난 체스미션 때부터 지금까지 각각 예외 상황에 따른 반환 상태코드를 어떤 것을 선택하는게 올바른지 잘 모르겠습니다..😭
상태 코드를 정할 때마다 Http Status 를 참조하는데도 실패에 따른 반환 상태코드 선택이 너무 어렵네요
현재 중복된 이름으로 데이터를 생성하는 Post Request를 보냈을 때, 발생하는 예외의 경우 400번대 코드를 반환해줬으나, 올바른 상태코드를 반환한 것인지 확신이 들지 않습니다.
- input값으로 서비스 로직 수행중 오류 발생하는 상황
- sql문 수행 중 예외 발생하는 상황
위의 상황의 경우 각각 어떤 상태코드를 반환해주는 것이 좋을까요😢
| public Line findById(Long id) { | ||
| String SQL = "select * from line where id = ?;"; | ||
| try { | ||
| return jdbcTemplate.queryForObject(SQL, rowMapper(), id); | ||
| } catch (DataAccessException e) { | ||
| throw new NotFoundException("id에 맞는 지하철 노선이 없습니다."); | ||
| } | ||
| } | ||
|
|
||
| public void update(Line line) { | ||
| findById(line.getId()); |
There was a problem hiding this comment.
존재하지 않는 데이터 Delete, Update의 예외처리
현재 update, delete의 경우, 요청이 왔을 때 해당 데이터들이 존재하는지 먼저 검증을 하기 위해 쿼리문 실행 전에 다음과 같이 findById() 메서드를 추가했습니다.
원래는 아래의 코드와 같이 예외처리를 하고 싶었으나, select문과는 다르게 delete의 경우 원하는 값이 존재하지 않을 경우 EmptyResultDataAccessException를 발생시키지 않아서 위와 같이 작성하였습니다.
public void delete(Long id) {
String SQL = "delete from line where id = ?";
try {
jdbcTemplate.update(SQL, id);
} catch (DataAccessException e) {
throw new NotFoundException("id에 맞는 지하철 노선이 없습니다.");
}
}코드들을 작성하며 존재하지 않는 값에 대한 delete, update 예외처리 방법들에 대해 생각해봤습니다.
- 작성 코드와 같이
findById()를 통해 검증을 한다. -> 삭제나 업데이트를 하기 위해서 테이블 접근을 2번이나 해야한다. - 아무런 검증을 하지 않는다? -> update와 delete의 메서드는 존재하지 않는 데이터에 대한 요청에도
DataAccessException을 발생시키지 않아, DB에 변경이 없어도 요청이 성공했다고 응답하게 된다.
개인적으로는 안정적인 서비스와 올바른 Response를 생각했을 때에는 테이블에 2번 접근하더라도 1번 방법이 더 좋을 것 같다고 생각합니다. 이러한 내용들을 과거 서비스에서는 어떤 방식으로 검증을 하는지 여쭙고 싶습니다.
| public Line findById(Long id) { | ||
| String SQL = "select * from line where id = ?;"; | ||
| try { | ||
| return jdbcTemplate.queryForObject(SQL, rowMapper(), id); | ||
| } catch (DataAccessException e) { | ||
| throw new NotFoundException("id에 맞는 지하철 노선이 없습니다."); | ||
| } |
There was a problem hiding this comment.
Query 실행할 때 발생하는 예외 처리의 위치 (DAO vs Service)
SQL문 실행에 대한 예외처리의 위치가 DAO에 있는 것이 좋을지, 아니면 Service에서 예외를 처리해주는 것이 좋을지에 대해서도 많은 고민을 했습니다.
현재는 DAO에서 예외처리를 추가해줬으나, 실제 서비스들에서는 jpa나 jdbc의 예외들을 어느 layer에서 처리를 하는지 궁금합니다🧐
| @GetMapping(value = "/stations", produces = MediaType.APPLICATION_JSON_VALUE) | ||
| public ResponseEntity<List<StationResponse>> showStations() { | ||
| List<Station> stations = StationDao.findAll(); | ||
| List<StationResponse> stationResponses = stations.stream() | ||
| .map(it -> new StationResponse(it.getId(), it.getName())) | ||
| .collect(Collectors.toList()); | ||
| List<StationResponse> stationResponses = stationService.findAll(); | ||
| return ResponseEntity.ok().body(stationResponses); | ||
| } |
There was a problem hiding this comment.
기본 제공된 코드이긴 하지만, 혹시 모르고 있었다면 이번 기회에 학습해 보아요. produces 설정은 어떤 역할을 할까요? 그리고 지금 이 설정을 지우면 어떻게 될까요?
There was a problem hiding this comment.
consumes와 produces는 이전 next-step spring학습 레포지토리 의 mvc branch에서 학습을 해본적이 있었습니다.
그때 학습을 하며 이해한 내용과 이번에 추가로 학습한 내용을 정리해보자면 두 설정은 request header의 값을 확인하며 controller로의 mapping을 제한해주는 것으로 이해했습니다. 이러한 속성 설정을 해둔다면 controller에서 보내는 데이터와 받는 데이터를 제한하여 오류 상황을 줄일 수 있는 것 같습니다.
@Consumes: 수신 하고자하는 데이터 포맷을 정의한다.@Produces: 출력하고자 하는 데이터 포맷을 정의한다.
해당 내용을 학습하며 미션 API 문서 의 request와 response에 맞게 모든 controller 메서드에 consumes과 produces 속성을 아래와 같이 설정하고 몇가지 테스트를 해봤습니다.
@GetMapping(value = "/stations",
consumes = MediaType.APPLICATION_JSON_VALUE, produces = MediaType.APPLICATION_JSON_VALUE)
public ResponseEntity<List<StationResponse>> showStations() {
List<StationResponse> stationResponses = stationService.findAll();
return ResponseEntity.ok().body(stationResponses);
}위와 같이 지정을 하고 몇가지 테스트를 해봤는데 몇가지 의문이 생겨 질문을 남깁니다😭
produces = MediaType.APPLICATION_JSON_VALUE의 설정을 하였을 때, header에Accept: application/json이 없어도 controller에서는 해당 request를 받아서 처리해주더군요..accept값이application/json타입이 아닌 다른 값으로 request를 보낼 경우 406과 같은 에러가 반환되지만 값이 없을 때는 왜 응답을 해주는지 궁급합니다. 제가 이해한 바로는 consumes, produces의 경우 header의 값을 확인하여 해당 값 설정이 존재하고 일치하여야지만 요청을 받아주는 것으로 이해를 하였는데 잘못 이해한 것일까요?? 아니면 Accept의 default값이application/json인 것일까요..?? (consumes는 produces와 동일한 테스트를 해본 결과 해당 header값이 존재하지 않으면 415 Unsupported Media Type에러를 던져주는 것을 확인했습니다.)- consume, produces는 request나 response에 body값이 없더라도 api문서에 accept와 contextType이 지정되어 있으면 정의를 해주는 것이 좋은지 여쭙고 싶습니다. 해당 속성 설정을 해두면 데이터를 제한함으로써 오류 상황을 줄일 수 있다지만, body가 없는 요청,응답에 대해서는 해당 설정을 해주는 것이 과연 좋은지? 궁금합니다.
src/test/java/wooteco/subway/controller/LineControllerTest.java
Outdated
Show resolved
Hide resolved
src/test/java/wooteco/subway/controller/LineControllerTest.java
Outdated
Show resolved
Hide resolved
ResponseEntity.ok의 shortcut기능을 적용해 body의 값을 바로 넣어주도록 수정
- 커스텀 Exception인 NotFoundException을 제거하고, ControllerAdvice에서 ExceptionHanlerController를 잡도록 설정
- 모든 데이터를 불러와 service에서 체크하는 기존 로직을 중복 데이터 저장시 발생하는 DuplicateKeyException을 처리하는 방법으로 수정
- 컨트롤러에서 서비스 계층으로 데이터를 전달할 때는 DTO를 통해 전달하도록 수정 - 서비스에서 컨트롤러로 전달하는 DTO를 정적 팩터리 메서드로 도메인 객체를 받아서 생성하도록 수정
- size만 체크하던 테스트에서 내부 값을 비교하도록 수정
|
안녕하세요 코니👋🏻 보내주신 피드백들을 반영해봤습니다!! 바쁘신 와중에도 시간내어 리뷰해주셔서 감사합니다🙇🏻♂️ |
| @ExceptionHandler(EmptyResultDataAccessException.class) | ||
| public ResponseEntity<ErrorMessageResponse> handleEmptyResultDataAccessException() { | ||
| return ResponseEntity.status(HttpStatus.NOT_FOUND) | ||
| .body(new ErrorMessageResponse("요청한 리소스를 DB에서 찾을 수 없습니다.")); |
There was a problem hiding this comment.
"DB에서" 찾을 수 없다는 것까지 알려줄 필요가 있을까요? 물론 정답이 없겠지만, 예외 메시지는 어느 정도까지 자세한 것이 좋을까요?
There was a problem hiding this comment.
"DB에서" 찾을 수 없다는 것까지 알려줄 필요가 있을까요?
굳이 DB에서 찾을 수 없다!라는 메시지를 보낼 필요는 없는 것 같습니다.
간단하게 "요청한 리소스를 찾을 수 없습니다."의 메시지만 출력해줘도 괜찮을 것 같습니다.
물론 정답이 없겠지만, 예외 메시지는 어느 정도까지 자세한 것이 좋을까요?
지금까지 미션을 진행하며 느낀바로는 예외 메시지를 상세하게 내보내주는 것과 코드를 간단하게 작성하는 것은 tradeoff한 관계라고 생각됩니다. 예외 메시지를 상세하게 담아서 exception을 발생시키기 위해서는 try/catch를 통해 예외를 잡고 각각 상황에 맞는 메시지를 담아서 보내야하는데, 이런 경우에는 코드의 길이가 길어지고 가독성이 떨어지는 문제도 있는 것 같다 생각됩니다.
그래서 현재 저는 서비스 로직과 관련있는 내용의 경우, 서비스 로직내에서 메시지를 담은 예외를 던져두고 조회 예외, 중복키 관련 예외와 같은 예외들은 exceptionHandler를 통한 통합 예외를 던져주도록 설정했습니다.
| @ExceptionHandler(DuplicateKeyException.class) | ||
| public ResponseEntity<ErrorMessageResponse> handleDuplicateKeyException() { | ||
| return ResponseEntity.status(HttpStatus.BAD_REQUEST) | ||
| .body(new ErrorMessageResponse("중복된 데이터가 존재합니다.")); | ||
| } | ||
|
|
||
| @ExceptionHandler(DataAccessException.class) | ||
| public ResponseEntity<ErrorMessageResponse> handleDataAccessException() { | ||
| return ResponseEntity.status(HttpStatus.BAD_REQUEST) | ||
| .body(new ErrorMessageResponse("DB관련 작업에서 오류가 발생했습니다.")); | ||
| } |
There was a problem hiding this comment.
지금 이 ExceptionControllerAdvice가 속한 패키지는 ui인데, 이 메서드들이 다루는 예외는 스프링의 dao 패키지에 속해 있네요. dao의 예외들은 어디서 처리하는 것이 좋을지 고민해 보아도 좋을 것 같아요.
| @DisplayName("노선의 이름과 색깔을 수정한다.") | ||
| @Test | ||
| void update() { | ||
| Line saveLine = lineDao.save(line); | ||
| Line expected = new Line(saveLine.getId(), "다른 분당선", "green"); | ||
|
|
||
| lineDao.update(expected); | ||
| Line actual = lineDao.findById(saveLine.getId()); | ||
|
|
||
| assertThat(actual).isEqualTo(expected); | ||
| } | ||
|
|
||
| @DisplayName("노선을 삭제한다.") | ||
| @Test | ||
| void delete() { | ||
| Line saveLine = lineDao.save(line); | ||
|
|
||
| lineDao.delete(saveLine.getId()); | ||
| List<Line> lines = lineDao.findAll(); | ||
|
|
||
| assertThat(lines.contains(saveLine)).isFalse(); | ||
| } |
There was a problem hiding this comment.
수정과 삭제 작업에서도, happy path만 테스트하지 말고 예외적인 상황에서 예상한 대로 동작하는지 검증하는 테스트를 작성해 두면 좋지 않을까요? 존재하지 않는 리소스를 대상으로 한 요청, 컬럼 크기를 초과하는 데이터로의 수정 요청, 중복 이름으로의 수정 요청 등 다양한 상황이 존재하겠네요.
| public class AccessNoneDataException extends RuntimeException { | ||
|
|
||
| public AccessNoneDataException() { | ||
| } | ||
| } |
There was a problem hiding this comment.
이 예외의 생성자는 DataLengthException의 생성자와 모양새가 다르군요. ExceptionHandlerController에서 항상 고정 메시지를 노출하기로 결정하셨기 때문일까요?
There was a problem hiding this comment.
처음에는 고정 메시지를 출력하기 위해 생성자가 메시지를 담지 않도록 하였는데, 위의 피드백을 반영하며 생각해보니 고정 예외 메시지를 내보낼 거면 해당 예외 클래스를 생성하지 않고 EmptyResultDataAccessException로 내보내면 되는데..왜 커스텀 예외를 만들었지?라는 생각이 들었습니다.
해당 AccessNoneDataException예외는 서비스 실행 도중 발생시킨 예외라 더욱 구체적인 상황을 담아 내보내면 예외 지점을 알아차리기 쉬울 것 같아 메시지를 담도록 수정했습니다.
|
|
||
| private void validateDataSize(String name) { | ||
| if (name.isEmpty() || name.length() > 255) { | ||
| throw new DataLengthException("역 이름이 빈 값이거나 최대 범위를 초과했습니다."); |
There was a problem hiding this comment.
- 사용자에게 직접 노출되는 메시지라면, 최대 범위가 얼마인지 명시해 준다면 더 좋겠죠?
- 이 역할을 서비스에서 해야 할까요? 더 좋은 방법은 없을까요?
There was a problem hiding this comment.
- 사용자에게 직접 노출되는 메시지라면, 최대 범위가 얼마인지 명시해 준다면 더 좋겠죠?
더욱 구체적인 내용을 담도록 수정했습니다!
- 이 역할을 서비스에서 해야 할까요? 더 좋은 방법은 없을까요?
해당 로직은 리펙터링을 하며 도메인으로 이동시켰습니다!!
| } | ||
|
|
||
| private void validateExistData(Long lineId) { | ||
| boolean isExist = stationDao.existStationById(lineId); |
There was a problem hiding this comment.
Bool 변수 이름 제대로 짓기 위한 최소한의 영어 문법을 한번 읽어 보면 좋을 것 같아요!
❗주의❗is로 시작하는 변수명을 짓다가 범하는 흔한 실수가 바로 is + 동사원형 을 쓰는 것이다.
isAuthorize, isHide, isFind 등등.
There was a problem hiding this comment.
is+형용사 문법을 이용하여 변수명을 수정하였습니다!!
좋은 자료 공유해주셔서 감사합니다🙇🏻♂️
| public class DataLengthException extends RuntimeException { | ||
|
|
||
| public DataLengthException(String message) { | ||
| super(message); | ||
| } | ||
| } |
There was a problem hiding this comment.
커스텀 예외 클래스가
RuntimeException나IllegallArgumetException를 상속받을 때 모두 커스텀 예외 클래스의 이름으로@ControllerAdvice에서 잡아줘서 발생할 수 있을 문제가 떠오르지 않네요..머리속으로는 더욱더 세분화된
IllegallArgumetException와 같은 자식클래스를 상속 받는 것이 더 좋을거라 생각되지만 이유를 설명하지 못하겠네요😅혹시 관련하여 학습할 수 있는 키워드가 있을까요..?
아시겠지만 RuntimeException은 굉장히 상위의 예외 클래스예요. 그래서 조금 더 구체적인 예외를 이용한다면 커스텀 예외를 어떤 상황을 위해 만든 것인지 조금 더 쉽게 파악할 수 있는 장점이 있을 것 같아요.
또한, 지금은 모든 커스텀 예외를 ExceptionHandlerController에서 일일이 처리하고 있지만, 만약 프로그램이 커지고 커스텀 예외가 100개쯤 된다면 이걸 일일이 다 처리할 수 있을까요? 적당한 계층 구조를 만들면 가장 아래에 있는 예외들은 굳이 @ExceptionHandler를 통해 처리하지 않고, 상위의 예외를 통해 일괄 처리할 수 있을 거예요.
혹시 잘 이해가 안 되었다면 @ExceptionHandler가 어떤 방식으로 예외를 처리하는지 학습해 보아요.
| @GetMapping(value = "/stations", produces = MediaType.APPLICATION_JSON_VALUE) | ||
| public ResponseEntity<List<StationResponse>> showStations() { | ||
| List<Station> stations = StationDao.findAll(); | ||
| List<StationResponse> stationResponses = stations.stream() | ||
| .map(it -> new StationResponse(it.getId(), it.getName())) | ||
| .collect(Collectors.toList()); | ||
| return ResponseEntity.ok().body(stationResponses); | ||
| List<StationResponse> stationResponses = stationService.findAll(); | ||
| return ResponseEntity.ok(stationResponses); | ||
| } |
There was a problem hiding this comment.
이 부분에서 남긴 코멘트에 대해, 정말 꼼꼼하게 학습하셨군요 👏
produces = MediaType.APPLICATION_JSON_VALUE의 설정을 하였을 때, header에Accept: application/json이 없어도 controller에서는 해당 request를 받아서 처리해주더군요..accept값이application/json타입이 아닌 다른 값으로 request를 보낼 경우 406과 같은 에러가 반환되지만 값이 없을 때는 왜 응답을 해주는지 궁급합니다. 제가 이해한 바로는 consumes, produces의 경우 header의 값을 확인하여 해당 값 설정이 존재하고 일치하여야지만 요청을 받아주는 것으로 이해를 하였는데 잘못 이해한 것일까요?? 아니면 Accept의 default값이application/json인 것일까요..?? (consumes는 produces와 동일한 테스트를 해본 결과 해당 header값이 존재하지 않으면 415 Unsupported Media Type에러를 던져주는 것을 확인했습니다.)
기회가 된다면, 우리가 HTTP 요청을 보냈을 때 스프링이 어떻게 해당 요청을 처리할 수 있는 컨트롤러의 메서드를 찾아내는지에 관해 학습해 보시면 좋을 것 같아요.
그리고, Accept 헤더의 기본값은 당연히 application/json이 아닙니다. (참고) 다만, 요청하는 입장에서 받을 수 있는 타입을 제한하지 않았다면 당연히 보낼 수 있는 값을 그냥 보내주는 것이 합리적인 선택 아닐까요?
- consume, produces는 request나 response에 body값이 없더라도 api문서에 accept와 contextType이 지정되어 있으면 정의를 해주는 것이 좋은지 여쭙고 싶습니다. 해당 속성 설정을 해두면 데이터를 제한함으로써 오류 상황을 줄일 수 있다지만, body가 없는 요청,응답에 대해서는 해당 설정을 해주는 것이 과연 좋은지? 궁금합니다.
이건 프로젝트 컨벤션에 따라 다를 것 같아요. 팀에서 명시하기로 정했다면 지정해 줘야 하겠죠?
다만, 요즘은 대부분 json만을 이용하는 추세라서 굳이 명시하지 않고, 특별히 다른 타입을 사용하는 경우가 있다면 그때만 지정해 주는 것 같습니다.
질문의 핵심을 잘못 이해했네요 😅
body가 없는 경우에 대해서는, 해당 헤더의 역할을 잘 이해하셨다면 설정할 필요가 있는지 스스로 판단이 가능할 것 같은데, 어떤가요?
안녕하세요 코니👋🏻
레벨1 로또 미션 이후로 다시 뵙게 되었네요!!
이번에도 코니의 리뷰를 통해 많은 성장을 해보겠습니다!!
미션을 진행하며 생긴 궁금증들을 코드 코멘트로 남겼습니다🥸
감사합니다🙇🏻♂️