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

feat: YSL-13 RootExceptionHandler 및 Problem 적용 #10

Merged
merged 7 commits into from
Jan 6, 2024
Merged
Show file tree
Hide file tree
Changes from 4 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
3 changes: 3 additions & 0 deletions build.gradle.kts
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,9 @@ dependencies {

developmentOnly("org.springframework.boot:spring-boot-devtools")
testImplementation("org.springframework.boot:spring-boot-starter-test")

implementation("org.zalando:problem-spring-web-starter:0.27.0")
implementation("org.springframework.boot:spring-boot-starter-validation")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
implementation("org.springframework.boot:spring-boot-starter-validation")
implementation("org.springframework.boot:spring-boot-starter-actuator")
implementation("org.springframework.boot:spring-boot-starter-data-jpa")
implementation("org.springframework.boot:spring-boot-starter-web")
implementation("org.springframework.boot:spring-boot-starter-validation")

다음과 같이 spring-starter끼리 모아보는것은 어떨까요?

Copy link
Member Author

Choose a reason for hiding this comment

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

나름 생각해서 구분 해 보았어요

}

tasks.withType<Test> {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
package org.idiot.yesslave.global.exception;

import io.swagger.v3.oas.annotations.media.Schema;
import lombok.AllArgsConstructor;
import lombok.Getter;

@Getter
@AllArgsConstructor
@Schema(description = "입력된 값에 대한 유효성 검사에 실패했을 경우의 응답값")
public class InvalidResponse {
@Schema(description = "오류가 발생한 field명", example = "verificationCode")
private String field;

@Schema(description = "오류 메세지", example = "확인번호를 입력해주세요.")
private String message;

@Schema(description = "잘못 입력된 field 입력 값", example = "ABCDE")
private Object rejectValue;
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,96 @@
package org.idiot.yesslave.global.exception;

import io.swagger.v3.oas.annotations.media.Content;
import io.swagger.v3.oas.annotations.media.Schema;
import io.swagger.v3.oas.annotations.responses.ApiResponse;
import lombok.extern.slf4j.Slf4j;
import org.springframework.boot.autoconfigure.EnableAutoConfiguration;
import org.springframework.boot.autoconfigure.web.servlet.error.ErrorMvcAutoConfiguration;
import org.springframework.http.HttpStatus;
import org.springframework.http.ResponseEntity;
import org.springframework.validation.BindingResult;
import org.springframework.web.bind.MethodArgumentNotValidException;
import org.springframework.web.bind.annotation.ControllerAdvice;
import org.springframework.web.bind.annotation.ExceptionHandler;
import org.springframework.web.bind.annotation.ResponseStatus;
import org.zalando.problem.Problem;

import javax.validation.ConstraintViolationException;
import java.util.ArrayList;
import java.util.List;

import static org.zalando.problem.Status.BAD_REQUEST;
import static org.zalando.problem.Status.INTERNAL_SERVER_ERROR;

@Slf4j
@ControllerAdvice
@EnableAutoConfiguration(exclude = ErrorMvcAutoConfiguration.class)
public class RootExceptionHandler {
@ExceptionHandler(Exception.class)
@ResponseStatus(value = HttpStatus.INTERNAL_SERVER_ERROR)
@ApiResponse(responseCode = "500", description = "Internal Server Error", content = {
@Content(schema = @Schema(implementation = Problem.class))
})
public ResponseEntity<Problem> exceptionHandler(Exception e) {

log.error("[ 500 ERROR ] : ", e);

Problem problem = Problem.builder()
.withStatus(INTERNAL_SERVER_ERROR)
Copy link
Member

Choose a reason for hiding this comment

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

static import를 통해서 가독성을 올리고자 하셨군요 좋습니다.

개인적으로 한편으로는 HttpStatus 클래스의 상수와 헷갈릴수도 있다고 생각해서 클래스를 축약하지 않는거에 대해서는 어떻게 생각하시나요?
잘 못 되었다는건 아니고 의견을 듣고 싶어요

Copy link
Member Author

Choose a reason for hiding this comment

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

사실 HttpStatus라고 생각해서 static import 처리를 해주었었는데, 알고보니 problem의 Status 더라구요... 정작 저도 잘 모르는 상태에서 남발한 것 같네요. 다시 꺼내왔습니다.

.withTitle(INTERNAL_SERVER_ERROR.getReasonPhrase())
.withDetail(e.getMessage())
.build();

return ResponseEntity
.status(HttpStatus.INTERNAL_SERVER_ERROR)
.body(problem);
}

@ResponseStatus(value = HttpStatus.BAD_REQUEST)
@ApiResponse(responseCode = "400", description = "Bad Request", content = {
@Content(schema = @Schema(implementation = InvalidResponse.class))
})
@ExceptionHandler(MethodArgumentNotValidException.class)
public ResponseEntity<Problem> methodArgumentNotValidExceptionHandler(MethodArgumentNotValidException e) {
log.error("[ 400 ERROR ] : ", e);

BindingResult bindingResult = e.getBindingResult();

List<InvalidResponse> responses = new ArrayList<>();

bindingResult.getFieldErrors().forEach(fieldError -> {
responses.add(new InvalidResponse(fieldError.getField(), fieldError.getDefaultMessage(), fieldError.getRejectedValue()));
});
Copy link
Member

Choose a reason for hiding this comment

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

map연산으로 개선이 가능할꺼 같아요!

Copy link
Member Author

Choose a reason for hiding this comment

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

map 연산이라함은 무엇일까요? 정확히 감이 잘 안오네요 ㅠ.ㅠ

Copy link
Member

Choose a reason for hiding this comment

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

        List<InvalidResponse> responses = new ArrayList<>();

        bindingResult.getFieldErrors().forEach(fieldError -> {
            responses.add(new InvalidResponse(fieldError.getField(), fieldError.getDefaultMessage(), fieldError.getRejectedValue()));
        });

        final List<InvalidResponse> responses = bindingResult.getFieldErrors().stream().map().....

다음과 같이 바꾼다면 final 키워드를 살려서 작성하면 아래에서의 변경을 안한다는 암묵적인 의미를 줄수도 있을꺼란 생각이 들어서요

Copy link
Member Author

Choose a reason for hiding this comment

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

헉 당연 자연스럽게 foreach 작성 했었는데, 이렇게도 가능했군요...! 수정 했씁니다


Problem problem = Problem.builder()
.withStatus(BAD_REQUEST)
.withTitle(BAD_REQUEST.getReasonPhrase())
.with("parameters", responses)
.build();

return ResponseEntity
.status(HttpStatus.BAD_REQUEST)
.body(problem);
}

@ResponseStatus(value = HttpStatus.BAD_REQUEST)
@ApiResponse(responseCode = "400", description = "Bad Request", content = {
@Content(schema = @Schema(implementation = InvalidResponse.class))
})
@ExceptionHandler(ConstraintViolationException.class)
public Problem constraintViolationExceptionHandler(ConstraintViolationException e) {
log.error("[ 400 ERROR ] : ", e);

List<InvalidResponse> responses = new ArrayList<>();

e.getConstraintViolations().forEach(fieldError -> {
responses.add(new InvalidResponse(fieldError.getPropertyPath().toString(), fieldError.getMessage(), fieldError.getInvalidValue()));
});

return Problem.builder()
.withStatus(BAD_REQUEST)
.withTitle(BAD_REQUEST.getReasonPhrase())
.with("parameters", responses)
.build();
}
}
6 changes: 6 additions & 0 deletions src/main/resources/application.yml
Original file line number Diff line number Diff line change
Expand Up @@ -15,3 +15,9 @@ spring:
format_sql: true
show_sql: true
dialect: org.hibernate.dialect.MySQL8Dialect
web:
resources:
add-mappings: false
mvc:
throw-exception-if-no-handler-found: true