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

Conversation

zinzoddari
Copy link
Member

RootExceptionHandler를 도입하였습니다.
실패에 대한 RFC-7807 규약에 맞춰 응답을 제공하기 위해 Problem을 도입해 보았습니다.
기본적인 500, 400 내용만 추가 하였고, 필요하실 때 추가적으로 덧붙여 사용하면 될 것 같습니다.

@zinzoddari zinzoddari added the 💻 setting 환경 설정 label Jan 2, 2024
@jucheolkang
Copy link

평소에 에러를 보여주는 것을 생각도 안 하고 기능을 구현하는 것만 생각했었는데 덕분에 하나 더 배웠습니다!
코드 짜시느라 고생하셨습니다.


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 작성 했었는데, 이렇게도 가능했군요...! 수정 했씁니다

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 더라구요... 정작 저도 잘 모르는 상태에서 남발한 것 같네요. 다시 꺼내왔습니다.

Copy link
Member

@beng9re beng9re left a comment

Choose a reason for hiding this comment

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

Problem에 대해서는 잘 모르고 있는 부분인데 적용을 하는거에 대해서 의미 깊게 생각합니다
한번 찾아 봐야겠어요

사소로운 코멘트 남겨 두었으니 참조 부탁드릴께요

@JHwan96
Copy link

JHwan96 commented Jan 3, 2024

ExceptionHandler 짜는 법을 잘 몰랐는데 잘 배우고 갑니다 :)

build.gradle.kts Outdated
@@ -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.

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

@zinzoddari zinzoddari requested a review from beng9re January 6, 2024 06:50
@beng9re beng9re self-assigned this Jan 6, 2024
Copy link
Member

@beng9re beng9re left a comment

Choose a reason for hiding this comment

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

👍

@zinzoddari zinzoddari merged commit d383ecf into develop Jan 6, 2024
1 check passed
@zinzoddari zinzoddari deleted the YSL-13 branch January 6, 2024 07:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
💻 setting 환경 설정
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants