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

Add Default Exception Handling Template #111

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

arsgsg1
Copy link
Member

@arsgsg1 arsgsg1 commented Jan 20, 2022

closes #110

  • 에러, 예외 상황에 공통으로 처리할 템플릿 클래스를 정의합니다.
  • Swagger 화면에서 Output payload에 대한 설명을 추가합니다.

@arsgsg1
Copy link
Member Author

arsgsg1 commented Jan 20, 2022

@seongbin9786
Copy link
Member

좌홍님 https://reflectoring.io/spring-boot-exception-handling/ 이 글에서 가져오신 구현체일까요?

@arsgsg1
Copy link
Member Author

arsgsg1 commented Jan 21, 2022

좌홍님 https://reflectoring.io/spring-boot-exception-handling/ 이 글에서 가져오신 구현체일까요?

내용이 비슷하긴 한데 제가 참고한 사이트는 https://medium.com/@georgeberar.contact/springboot-standardized-api-exception-handling-f31510861350 입니다.

@seongbin9786
Copy link
Member

다 좋긴 하네요. 감사합니다~


@Getter
@Setter
public class ApplicationException extends RuntimeException implements ApplicationExceptionPolicy {
Copy link
Member

Choose a reason for hiding this comment

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

ApplicationException과 ~Policy의 관계를 잘 이해를 못 하겠습니다 :( ExceptionReason을 생성자에서 받는 건 이해가 되는데요, implements까지 해야 되는 이유가 무엇인가요?

@seongbin9786
Copy link
Member

seongbin9786 commented Jan 22, 2022

제가 이 코드를 읽어보면서 리뷰해보니까 리뷰어 입장에서 조금 어려운 PR 같다는 생각이 들었습니다.

그 이유를 생각해보니

  1. 구현체여서 한 줄 한 줄 이해해야 한다.
  2. 나름의 설계가 들어간 상황이다. 즉 객체간의 관계에 대해 why를 직접 생각해봐야 한다.
  3. 코드의 양이 많아서 (2)가 어렵다.
  4. (일단 저는) 예외 처리에 대해 생각해본 적이 없어 리뷰가 더 어렵다.

에서 전체 구조와 그렇게 구성한 이유, 각 구현체 코드에 대한 why와 how에 대한 설명을 간략하더라도 명확하게 정리해주시면 리뷰가 원활할 것 같습니다.

@arsgsg1
Copy link
Member Author

arsgsg1 commented Jan 22, 2022

넵 우선은 노션에 정리해보고 슬랙에 올려보겠습니다.

@seongbin9786
Copy link
Member

감사합니다 =)

}

@Override
protected ResponseEntity<Object> handleMissingPathVariable(final MissingPathVariableException ex,
Copy link
Member

Choose a reason for hiding this comment

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

얘는 참조가 없는 걸로 나옵니다.

Copy link
Member Author

Choose a reason for hiding this comment

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

엇 어떻게 확인하신건가요? 포스트맨으로 path variable 일부러 빼거나 문자열로 넣어서 테스트해보니 handleMissingServletRequestParameter로 예외가 발생하긴 하네요

Copy link
Member

Choose a reason for hiding this comment

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

깃헙 file change 화면에서 검색만 해봤습니다 =)

@seongbin9786
Copy link
Member

제가 오늘 예외 처리 글들을 좀 읽어보고 결론낸 내용인데요,

  1. Domain 예외는 Domain Layer에서 선언되어야 할 것 같습니다.
  2. Policy 인터페이스 제거: data object인 Exception 객체가 어떤 기능을 구현할 이유가 없는 것 같습니다. 또한 지금 인터페이스는 내용까지 없으므로 불량합니다. data object 입장에서 구현해야 할 기능에 대해 policy라는 네이밍은 모호합니다.
  3. Business는 의미가 너무 넓기 때문에 Domain Layer를 지칭하는 DomainLayerException으로 이름을 바꾸는 게 좋을 것 같습니다(이 예외는 base class로 직접 사용되지 않습니다.)
  4. Http status는 controller layer에서 결정하는 게 맞으므로 status는 예외 객체에서 제거돼야하며, 이 status 정보는 (exception, status)로 관리하면 될 것 같습니다.

@arsgsg1
Copy link
Member Author

arsgsg1 commented Jan 28, 2022

1, 2, 4: 코드로 구현함에 감이 잘 안잡혀서 2, 4번 부분을 오늘 미팅에서 간단하게 들을 수 있을까요? + 현재 구현된 예외 클래스들을 도메인 레이어로 옮기면 될까요?
3: DomainLayerException 클래스가 base라면, 해당 클래스를 상속받아 새로운 예외 클래스들을 생성하는 방식인가요?

@seongbin9786
Copy link
Member

124/네
3/네

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Exception, Error Template and Output payload description
2 participants