Skip to content
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

Merged
merged 5 commits into from
Jul 23, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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()));
}
Copy link
Contributor

@skylar1220 skylar1220 Jul 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

기존 ProblemDetail만 반환하는 방식에서 ResponseEntity로 한 번 더 감싸서 보내야하는 이유가 궁금해요!
한 번 더 감싸서 보내면서 상태코드가 두 번 들어가게되는 점도 함께 눈에 띄여서요~

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이건 여기에 한해서 그냥 ProblemDetail 내려줘도 될 것 같아요

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ResponseEntity로 감싸면 헤더에도 상태코드를 지정할 수 있고, 감싸지 않으면 헤더에는 무조건 200ok로 나가게 되나요? 헤더에 상태코드를 지정하면 어떤 점이 좋은가요?

Copy link
Contributor Author

@nayonsoso nayonsoso Jul 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

기존 ProblemDetail만 반환하는 방식에서 ResponseEntity로 한 번 더 감싸서 보내야하는 이유가 궁금해요!

헤더에 상태코드를 지정하면 어떤 점이 좋은가요?

"ResponseEntity로 감싸는 것"과 "ProblemDetail을 그냥 내려주는 것"은 동일한 응답을 합니다.


그런데 왜 굳이 ResponseEntity로 감쌌냐면요!
ResponseEntityExceptionHandler의 함수들은 ResponseEntity<Object> 를 응답하니깐요//
그래서 클래스 전체적으로 통일을 하기 위해서 그랬습니다 ㅎㅎ

예를들어 아래와 같은 코드가 통일감이 없다 생각해서 그랬던 것입니다.

@RestControllerAdvice
public class GlobalExceptionHandler extends ResponseEntityExceptionHandler {
     // 여기에선 ProblemDetail을 리턴
    @ExceptionHandler(NotFoundException.class)
    public ProblemDetail handleNotFoundException(NotFoundException ex) {
        return ProblemDetail.forStatusAndDetail(HttpStatus.NOT_FOUND, ex.getErrorMessage());
    }

    // 여기에선 ResponseEntity<Object>를 리턴, 그런데 이건 오버라이드 한 것이므로 리턴 타입을 바꿀 수 없음
    @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()));
    }
    // 🤔 이럴바에 그냥 ResponseEntity<XXX>로 통일하자!
}

그런데 여러분이 보셨을 때 어색하다면.. ProblemDetail을 리턴하도록 바꾸겠습니다!


@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, "서버 에러가 발생했습니다."));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

변수 추출하는 게 보기 편할 것 같습니다 🥹

}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

전체 Exception을 잡는 것을 가장 아래로 내리면 어떨까요?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

전체 Excetion을 처리하는 ExceptionHandler를 가장 아래에 두는 것은 ExceptionHandler가 선언한 순서대로 우선순위를 갖기 때문이라고 알고있어요. 그렇다면 ExceptionHandler는 ExceptionHandler끼리 모여있는게 더 보기 편할 것 같다는 생각이 들어요!


@Override
public ResponseEntity<Object> handleMethodArgumentNotValid(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Object를 사용하네요? 리턴값이 ProblemDetail인 듯하네요.. 🥹

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이건 Override 때문에 어쩔 수 없네요 🤔

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ResponseEntityExceptionHandler 의 메서드 중에서 handleMethodArgumentNotValid만 오버라이드 한 이유는 무엇인가요?

Copy link
Contributor Author

@nayonsoso nayonsoso Jul 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ResponseEntityExceptionHandler 의 메서드 중에서 handleMethodArgumentNotValid만 오버라이드 한 이유는 무엇인가요?

MethodArgumentNotValid는 '컨트롤러에서 하는 request dto 검증'을 만족하지 않을 때 발생하는 에러입니다. 그리고 이 에러는 우리가 지정한 메세지를 보여주도록 커스텀할 필요가 있어요. e.g. 답변을 입력해주세요

커스텀이 필요하기 때문에 MethodArgumentNotValid는 오버라이드를 해준 것이고, 나머지 스프링 기본 예외들은 ResponseEntityExceptionHandler의 핸들링으로 처리되는 것만으로도 충분하다고 생각해서 요놈만 오버라이드 해주었습니다🤗

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MethodArgumentNotValidException.class 를 잡도록 하고 상속받지 않는건 어떤가요?

Copy link
Contributor

Choose a reason for hiding this comment

The 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()
Expand All @@ -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()));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

exception의 메시지를 그대로 내려도 괜찮을까요? Internal 오류이면 내부에서 그냥 처리를 하는 게 나을 것 같아요.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

vvv 자원이 없다는 건 내부 구조를 노출할 수 있는 창구가 될 것 같았습니다.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

헉 NoResourceFoundException같은 것은 내부 구조를 노출할 수도 있겠네요..!
그럼 404 라는 의미는 드러내되, '정적 파일이 없다'와 같은 구체적인 예외 메세지는 수정하도록 바꿔보겠습니다

}
}
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);
}
}