-
Notifications
You must be signed in to change notification settings - Fork 2
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
[BE] refactor: 발생하는 모든 예외의 형식 통일 #69
Changes from 2 commits
2b5e95e
99d7010
cb12488
4cc7680
aafa347
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,24 +2,45 @@ | |
|
||
import java.util.List; | ||
import java.util.Map; | ||
import org.springframework.http.HttpHeaders; | ||
import org.springframework.http.HttpStatus; | ||
import org.springframework.http.HttpStatusCode; | ||
import org.springframework.http.ProblemDetail; | ||
import org.springframework.http.ResponseEntity; | ||
import org.springframework.lang.Nullable; | ||
import org.springframework.web.bind.MethodArgumentNotValidException; | ||
import org.springframework.web.bind.annotation.ExceptionHandler; | ||
import org.springframework.web.bind.annotation.RestControllerAdvice; | ||
import org.springframework.web.context.request.WebRequest; | ||
import org.springframework.web.servlet.mvc.method.annotation.ResponseEntityExceptionHandler; | ||
import reviewme.global.exception.BadRequestException; | ||
import reviewme.global.exception.FieldErrorResponse; | ||
import reviewme.global.exception.NotFoundException; | ||
|
||
@RestControllerAdvice | ||
public class GlobalExceptionHandler { | ||
public class GlobalExceptionHandler extends ResponseEntityExceptionHandler { | ||
|
||
@ExceptionHandler(NotFoundException.class) | ||
public ProblemDetail handleNotFoundException(NotFoundException ex) { | ||
return ProblemDetail.forStatusAndDetail(HttpStatus.NOT_FOUND, ex.getErrorMessage()); | ||
public ResponseEntity<ProblemDetail> handleNotFoundException(NotFoundException ex) { | ||
return ResponseEntity.status(HttpStatus.NOT_FOUND) | ||
.body(ProblemDetail.forStatusAndDetail(HttpStatus.NOT_FOUND, ex.getErrorMessage())); | ||
} | ||
|
||
@ExceptionHandler(MethodArgumentNotValidException.class) | ||
public ProblemDetail handleMethodArgumentNotValidException(MethodArgumentNotValidException ex) { | ||
@ExceptionHandler(BadRequestException.class) | ||
public ResponseEntity<ProblemDetail> handleBadRequestException(BadRequestException ex) { | ||
return ResponseEntity.status(HttpStatus.BAD_REQUEST) | ||
.body(ProblemDetail.forStatusAndDetail(HttpStatus.BAD_REQUEST, ex.getErrorMessage())); | ||
} | ||
|
||
@ExceptionHandler(Exception.class) | ||
public ResponseEntity<ProblemDetail> handleException(Exception ex) { | ||
return ResponseEntity.status(HttpStatus.INTERNAL_SERVER_ERROR) | ||
.body(ProblemDetail.forStatusAndDetail(HttpStatus.INTERNAL_SERVER_ERROR, "서버 에러가 발생했습니다.")); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 변수 추출하는 게 보기 편할 것 같습니다 🥹 |
||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 전체 Exception을 잡는 것을 가장 아래로 내리면 어떨까요? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 전체 Excetion을 처리하는 ExceptionHandler를 가장 아래에 두는 것은 ExceptionHandler가 선언한 순서대로 우선순위를 갖기 때문이라고 알고있어요. 그렇다면 ExceptionHandler는 ExceptionHandler끼리 모여있는게 더 보기 편할 것 같다는 생각이 들어요! |
||
|
||
@Override | ||
public ResponseEntity<Object> handleMethodArgumentNotValid( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 이건 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ResponseEntityExceptionHandler 의 메서드 중에서 handleMethodArgumentNotValid만 오버라이드 한 이유는 무엇인가요? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
MethodArgumentNotValid는 '컨트롤러에서 하는 request dto 검증'을 만족하지 않을 때 발생하는 에러입니다. 그리고 이 에러는 우리가 지정한 메세지를 보여주도록 커스텀할 필요가 있어요. e.g. 답변을 입력해주세요 커스텀이 필요하기 때문에 MethodArgumentNotValid는 오버라이드를 해준 것이고, 나머지 스프링 기본 예외들은 ResponseEntityExceptionHandler의 핸들링으로 처리되는 것만으로도 충분하다고 생각해서 요놈만 오버라이드 해주었습니다🤗 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. MethodArgumentNotValidException.class 를 잡도록 하고 상속받지 않는건 어떤가요? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
MethodArgumentNotValidException ex, HttpHeaders headers, HttpStatusCode status, WebRequest request) { | ||
List<FieldErrorResponse> fieldErrors = ex.getBindingResult() | ||
.getFieldErrors() | ||
.stream() | ||
|
@@ -34,6 +55,13 @@ public ProblemDetail handleMethodArgumentNotValidException(MethodArgumentNotVali | |
); | ||
Map<String, Object> properties = Map.of("fieldErrors", fieldErrors); | ||
problemDetail.setProperties(properties); | ||
return problemDetail; | ||
return ResponseEntity.status(HttpStatus.BAD_REQUEST).body(problemDetail); | ||
} | ||
|
||
@Override | ||
public ResponseEntity<Object> handleExceptionInternal( | ||
Exception ex, @Nullable Object body, HttpHeaders headers, HttpStatusCode statusCode, WebRequest request) { | ||
return ResponseEntity.status(statusCode) | ||
.body(ProblemDetail.forStatusAndDetail(statusCode, ex.getMessage())); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. exception의 메시지를 그대로 내려도 괜찮을까요? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 헉 NoResourceFoundException같은 것은 내부 구조를 노출할 수도 있겠네요..! |
||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,10 @@ | ||
package reviewme.review.exception; | ||
|
||
import reviewme.global.exception.NotFoundException; | ||
|
||
public class ReviewNotFoundException extends NotFoundException { | ||
|
||
public ReviewNotFoundException() { | ||
super("리뷰가 존재하지 않습니다."); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,18 +1,18 @@ | ||
package reviewme.review.repository; | ||
|
||
import jakarta.persistence.EntityNotFoundException; | ||
import org.springframework.data.jpa.repository.JpaRepository; | ||
import org.springframework.stereotype.Repository; | ||
import reviewme.member.domain.Member; | ||
import reviewme.member.domain.ReviewerGroup; | ||
import reviewme.review.domain.Review; | ||
import reviewme.review.exception.ReviewNotFoundException; | ||
|
||
@Repository | ||
public interface ReviewRepository extends JpaRepository<Review, Long> { | ||
|
||
boolean existsByReviewerAndReviewerGroup(Member reviewer, ReviewerGroup reviewerGroup); | ||
|
||
default Review getReviewById(Long id) { | ||
return findById(id).orElseThrow(EntityNotFoundException::new); | ||
return findById(id).orElseThrow(ReviewNotFoundException::new); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
기존 ProblemDetail만 반환하는 방식에서 ResponseEntity로 한 번 더 감싸서 보내야하는 이유가 궁금해요!
한 번 더 감싸서 보내면서 상태코드가 두 번 들어가게되는 점도 함께 눈에 띄여서요~
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이건 여기에 한해서 그냥
ProblemDetail
내려줘도 될 것 같아요There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ResponseEntity로 감싸면 헤더에도 상태코드를 지정할 수 있고, 감싸지 않으면 헤더에는 무조건 200ok로 나가게 되나요? 헤더에 상태코드를 지정하면 어떤 점이 좋은가요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"ResponseEntity로 감싸는 것"과 "ProblemDetail을 그냥 내려주는 것"은 동일한 응답을 합니다.
그런데 왜 굳이 ResponseEntity로 감쌌냐면요!
ResponseEntityExceptionHandler
의 함수들은ResponseEntity<Object>
를 응답하니깐요//그래서 클래스 전체적으로 통일을 하기 위해서 그랬습니다 ㅎㅎ
예를들어 아래와 같은 코드가 통일감이 없다 생각해서 그랬던 것입니다.
그런데 여러분이 보셨을 때 어색하다면.. ProblemDetail을 리턴하도록 바꾸겠습니다!