Conversation
f-lab-seb
left a comment
f-lab-seb
left a comment
There was a problem hiding this comment.
수고 많으셨습니다!! 코멘트 몇가지 남겼어요 🙏
|
|
||
| import org.springframework.boot.logging.LogLevel | ||
|
|
||
| data class CoreException( |
There was a problem hiding this comment.
data class 여야 하는 이유가 있을지 궁금합니다!
There was a problem hiding this comment.
잘못된 사용이었던 것 같습니다.
data class가 제공하는 기능들이 필요한 상황도 아니고, 불필요하게 닫아 놓은 것 같습니다.
예외를 계층적으로 구성하기 위해 상속 가능하도록 만들 필요가 있어 보입니다.
| throw CoreException( | ||
| type = OrderError.INVALID_STATUS_TRANSITION, | ||
| detail = "결제를 진행할 수 없는 상태입니다.", | ||
| ) |
There was a problem hiding this comment.
CoreException 를 open class 로 만들고, 도메인/상황별로 의미 있는 하위 예외 클래스를 만들어서 던지면 의도도 더 명확해지고, 보일러플레이트도 줄어들어 더 좋지 않을까 생각합니다.
e.g.)
class InvalidStatusTransitionException(
type: ErrorType = OrderError.INVALID_STATUS_TRANSITION,
message: String? = type.message,
) : CoreException(type = type, detail = message)
사용시
thorw InvalidStatusTransitionException(message = "결제를 진행할 수 없는 상태입니다.")
요런식으로 하면
throw IllegalStateException("결제를 진행할 수 없는 상태입니다.")
이렇게 사용하셨을 때 처럼 한줄로 끝날 것 같아요.
There was a problem hiding this comment.
말씀 주신 대로 의미 있는 하위 예외 클래스가 필요하다는 생각이 들어 CoreException을 open class로 만들었습니다.
그리고 InvalidStatusTransitionException을 만들어 보았는데 상황에 따라 다르겠지만 이미 클래스 이름이 예외를 잘 설명하기에 예외 메시지를 내부에 감추는 것도 좋을 것 같다는 생각이 들었습니다. throw InvalidStatusTransitionException(currentStatus, targetStatus)와 같이 원인이 된 컨텍스트를 전달받도록 하여 예외 클래스 내부에서 메시지로 조립하고, 디버깅에도 활용될 수 있도록 프로퍼티로 두는 방식을 사용해 보았습니다.
확실히 예외 하위 예외 클래스 이름만으로도 예외 상황이 바로 이해가 되고, 이전 방식에 비해 확실히 가독성이 좋아진 것 같습니다.
|
|
||
| import org.springframework.http.HttpStatus | ||
|
|
||
| interface ErrorType { |
There was a problem hiding this comment.
정말 좋네요. 👍👍👍
CoreError 외에도 모듈이나 도메인별로 Error Code 를 쉽게 추가 가능할 것 같고, 특정 enum 이 불필요하게 커지지도 않을 것 같아요.
| @ExceptionHandler(Exception::class) | ||
| fun handleException(e: Exception): ResponseEntity<ErrorResponse> = | ||
| handleCoreException(CoreException(type = CoreError.UNKNOWN_ERROR, cause = e)) |
There was a problem hiding this comment.
전체적으로 정말 좋은 구조를 잡아주셨다고 생각하는데, 이부분은 살짝 우려되네요.
모든 Exception 을 다 잡아서 공통 처리하는게 맞으려나 싶어서요.
예를 들어 프론트개발자가 GET 만 뚫어둔 API 에 POST 요청 했을 때,
HttpRequestMethodNotSupportedException 405 status 였다면 별도의 소통 없이 해결될 수도 있는 것을 500 Error 로 전환하면서 오히려 불필요한 소통비용이 발생할 수도 있을 것 같고,
그 외에 제가 지금 생각하지 못하는 경우를 놓치게 될 수도 있을까봐서요!
이런 식으로 http status 를 살려서 가는건 어떨까 싶었습니다.
fun resolveHttpStatus(e: Throwable): HttpStatus {
e::class.java.getAnnotation(ResponseStatus::class.java)?.let {
return it.code
}
return if (e is ResponseStatusException) {
e.statusCode as? HttpStatus ?: HttpStatus.INTERNAL_SERVER_ERROR
} else {
HttpStatus.INTERNAL_SERVER_ERROR
}
}
There was a problem hiding this comment.
이미 상태 코드가 적절히 정해진 예외를 500으로 override하는 것은 말씀 주신 소통 비용 관점에서 굉장히 비효율적인 것 같습니다. 말씀 주신 status를 살리는 방식이 필요하다 생각됩니다.
HttpRequestMethodNotSupportedException와 ResponseStatusException 모두 org.springframework.web.ErrorResponse 를 구현하여 이를 통해 httpStatus를 제공하고있습니다.
org.springframework.web.ErrorResponse 구현체로 범위를 넓혀서 status를 살릴 수 있도록 해보았습니다.
그런데 CoreException은 ErrorType를 베이스로 하고, ErrorType은 호출하는 클라이언트와의 규약이라고 생각합니다. CoreException을 사용한다는 것은 이 규약을 지키겠다는 것과도 같다고 생각했고, ErrorType과 매칭되지 않은 상태 코드로 CoreException을 응답하는 것은 피하고자 했습니다.
규약 내 존재하지 않는, 클라이언트가 예상하지 못한 에러는 500으로 재정의하는 것이 맞다고 생각되었기에 핸들링하지 않는 예외들에 대해서는 CoreError에 매칭되는 상태 코드가 없는 경우 CoreError.UNKNOWN_ERROR를 사용하도록 구성하였습니다.
| assertThatThrownBy { order.pay(paymentId = 456L) } | ||
| .isInstanceOf(IllegalStateException::class.java) | ||
| .isInstanceOf(CoreException::class.java) | ||
| .extracting { it as CoreException } | ||
| .extracting(CoreException::type) | ||
| .isEqualTo(OrderError.INVALID_STATUS_TRANSITION) |
There was a problem hiding this comment.
중요한 것은 아니고, extracting / 캐스팅 과정을 줄이고 싶다면 이런식으로 해도 좋을 것 같아요
assertThatThrownBy { order.pay(paymentId = 456L) }
.isInstanceOfSatisfying(CoreException::class.java) { e ->
assertThat(e.type).isEqualTo(OrderError.INVALID_STATUS_TRANSITION)
}
|
|
||
| @Test | ||
| fun `CREATED 상태가 아니면 IllegalStateException이 발생한다`() { | ||
| fun `CREATED 상태가 아니면 INVALID_STATUS_TRANSITION 에러가 발생한다`() { |
| override val code: String, | ||
| override val message: String, | ||
| ) : ErrorType { | ||
| INVALID_STATUS_TRANSITION(HttpStatus.CONFLICT, "O001", "주문 상태를 변경할 수 없습니다."), |
There was a problem hiding this comment.
크게 중요하진 않은데 여기 적힌 message 는 쓰임이 없는 것 같긴 하네욥 ㅋ_ㅋ
There was a problem hiding this comment.
이때는 쓰임이 없었던 것 같습니다... 그런데 이제는 쓰임이 생겼습니다!ㅎ
예외 응답 시 직접 작성한 메시지를 사용하지 않고 규약으로 정의된 메시지를 사용하고자 했습니다.
지금은 저 값이 그렇게 사용되도록 해 두었습니다.
INVALID_STATUS_TRANSITION 에러로 예외를 발생시키면 response에 O001 코드와 "주문 상태를 변경할 수 없습니다."라는 메시지가 응답에 포함되고, 부가 정보를 담고자 한다면 errorDetail을 사용하여 제공할 수 있게 구성했습니다.
|
|
||
| @RestControllerAdvice | ||
| class GlobalExceptionHandler { | ||
| private val log: Logger = LoggerFactory.getLogger(javaClass) |
There was a problem hiding this comment.
이것도 크게 중요하지는 않은데, 코틀린 쪽 logger wrapper 로 kotlin-logging 을 주로 사용하는 것 같아요.
선언이 조금 더 간편하기도 하고
private val log = KotlinLogging.logger {}
문자열 템플릿($var) 이런거 사용하기도 좀 편해요.
그리고 현재 구조로 하셨을 때는 실행되지 않을 로깅도,
문자열 연산이 먼저 실행되는데에 반해 (concrete text 비용)
log.debug { "비싼 텍스트 만드는 연산: ${expensiveOperation()}" }
kotlin logging 을 이렇게 사용하면, lambda 기반이기에 로깅이 정말 찍힐 때만 연산이 수행되어 (lazy logging) 성능상 이점도 있습니다.
There was a problem hiding this comment.
클로저라는 개념에 대해 몰랐는데 함수 형태로 이동하며 관련된 컨텍스트들을 들고 다닌다는 게 신기하네요.
함수형 프로그래밍의 기반이 되는 것 같은데 이것도 나중에 한번 파봐야겠습니다!
kotlin-logging 라이브러리 사용하도록 수정하였고, Lazy Execution 이점을 보기 위해 로그 메시지 남기는 시점부터 람다로 넘기도록 했습니다.
처음에는 일반 문자열만 받는 상황에서도 람다로 받으면 약간의 리소스라도 더 들까 싶어서 일반 문자열로 받는 것도 열어둘까 했습니다. 그런데 예외를 생성하는 것 자체가 stacktrace 캡처 등 비용이 많이 크고, 그것에 비하면 티도 안 날 정도로 작은 차이이기에
일관성을 제공하기 위해 일반 문자열은 받지 않고 람다만 받도록 구성하였습니다.
|
2 participants
Closes #21