ISSUE-3 API 응답 Spec & 예외 Setting #9
Conversation
Walkthrough이번 변경 사항에서는 Changes
Assessment against linked issues
Possibly related PRs
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🔇 Additional comments (3)domain/src/main/kotlin/org/doorip/domain/DooripException.kt (3)
안녕하세요! 다음과 같이 수정할 수 있습니다: data object InvalidRequestValueException : ClientException("e4000", "잘못된 요청입니다.") {
- private fun readResolve(): Any = InvalidRequestValueException
}
data object MethodNotAllowedException : ClientException("e4050", "잘못된 HTTP method 요청입니다.") {
- private fun readResolve(): Any = MethodNotAllowedException
}
data object ConflictException : ClientException("e4090", "이미 존재하는 리소스입니다.") {
- private fun readResolve(): Any = ConflictException
}
다음과 같이 수정할 수 있습니다: data object NotFoundException : ServerException("e4040", "대상을 찾을 수 없습니다.") {
- private fun readResolve(): Any = NotFoundException
}
data object InternalServerException : ServerException("e5000", "서버 내부 오류입니다.") {
- private fun readResolve(): Any = InternalServerException
}Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
📒 Files selected for processing (8)
core/src/main/kotlin/org/doorip/core/TestService.kt(1 hunks)domain/src/main/kotlin/org/doorip/domain/DooripException.kt(1 hunks)presentation/api/src/main/kotlin/org/doorip/TestController.kt(0 hunks)presentation/api/src/main/kotlin/org/doorip/api/TestController.kt(1 hunks)presentation/api/src/main/kotlin/org/doorip/api/dto/ApiResponse.kt(1 hunks)presentation/api/src/main/kotlin/org/doorip/api/dto/ExceptionResponse.kt(1 hunks)presentation/api/src/main/kotlin/org/doorip/api/exception/ApiExceptionHandler.kt(1 hunks)presentation/api/src/main/kotlin/org/doorip/api/exception/ExceptionResponseFactory.kt(1 hunks)
💤 Files with no reviewable changes (1)
- presentation/api/src/main/kotlin/org/doorip/TestController.kt
| class UnknownException( | ||
| cause: Throwable? = null, | ||
| ) : CriticalException("e6000", "정의되지 않은 예외입니다. (로그 확인이 필요합니다.)") |
There was a problem hiding this comment.
UnknownException 클래스의 메시지 인자가 누락되었습니다
UnknownException 생성자에서 message 인자를 전달하지 않고 있습니다. CriticalException의 생성자는 code, message, cause를 요구하므로 message를 전달해야 합니다.
다음과 같이 수정하여 message 인자를 전달해주세요:
class UnknownException(
cause: Throwable? = null,
) : CriticalException("e6000", "정의되지 않은 예외입니다. (로그 확인이 필요합니다.)", cause)| data object NotFoundException : ServerException("e4040", "대상을 찾을 수 없습니다.") { private fun readResolve(): Any = NotFoundException } | ||
| data object InternalServerException : ServerException("e5000", "서버 내부 오류입니다.") { private fun readResolve(): Any = InternalServerException } | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
예외 클래스의 readResolve 메서드 사용에 대한 점검이 필요합니다
NotFoundException, InternalServerException에서도 위와 동일한 이유로 readResolve 메서드가 불필요할 수 있습니다.
위에서 제안한 것처럼 readResolve 메서드를 제거하여 코드를 간결하게 유지해주세요.
data object NotFoundException : ServerException("e4040", "대상을 찾을 수 없습니다.") {
- private fun readResolve(): Any = NotFoundException
}Committable suggestion skipped: line range outside the PR's diff.
| data object InvalidRequestValueException : ClientException("e4000", "잘못된 요청입니다.") { private fun readResolve(): Any = InvalidRequestValueException } | ||
| data object MethodNotAllowedException : ClientException("e4050", "잘못된 HTTP method 요청입니다.") { private fun readResolve(): Any = MethodNotAllowedException } | ||
| data object ConflictException : ClientException("e4090", "이미 존재하는 리소스입니다.") { private fun readResolve(): Any = ConflictException } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
예외 클래스의 readResolve 메서드 사용에 대한 점검이 필요합니다
InvalidRequestValueException, MethodNotAllowedException, ConflictException 등에서 readResolve 메서드를 사용하고 계신데, 이는 역직렬화 과정에서 싱글턴 패턴을 유지하기 위한 것으로 이해됩니다. 그러나 Kotlin data object는 이미 싱글턴을 보장하므로 readResolve 메서드가 불필요할 수 있습니다.
불필요한 코드를 제거하여 코드를 간결하게 유지하는 것을 권장합니다.
data object InvalidRequestValueException : ClientException("e4000", "잘못된 요청입니다.") {
- private fun readResolve(): Any = InvalidRequestValueException
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| data object InvalidRequestValueException : ClientException("e4000", "잘못된 요청입니다.") { private fun readResolve(): Any = InvalidRequestValueException } | |
| data object MethodNotAllowedException : ClientException("e4050", "잘못된 HTTP method 요청입니다.") { private fun readResolve(): Any = MethodNotAllowedException } | |
| data object ConflictException : ClientException("e4090", "이미 존재하는 리소스입니다.") { private fun readResolve(): Any = ConflictException } | |
| data object InvalidRequestValueException : ClientException("e4000", "잘못된 요청입니다.") | |
| data object MethodNotAllowedException : ClientException("e4050", "잘못된 HTTP method 요청입니다.") | |
| data object ConflictException : ClientException("e4090", "이미 존재하는 리소스입니다.") |
| protected fun handleException(ex: Exception): ExceptionResponseEntity { | ||
| return exceptionResponseFactory.create(UnknownException(ex)) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
예상치 못한 예외 처리에서 로그 추가를 권장합니다
Exception을 처리하는 블록에서 예외 스택트레이스를 로깅하면 디버깅에 도움이 됩니다.
예외를 로깅하여 추후 문제 발생 시 원인을 파악하기 쉽게 만들어주세요.
@ExceptionHandler(Exception::class)
protected fun handleException(ex: Exception): ExceptionResponseEntity {
logger.error("Unhandled exception caught", ex)
return exceptionResponseFactory.create(UnknownException(ex))
}로그 관리를 위한 로깅 프레임워크의 활용을 고려해주세요.
| @ControllerAdvice | ||
| internal class ApiExceptionHandler( | ||
| private val exceptionResponseFactory: ExceptionResponseFactory, | ||
| ) { |
There was a problem hiding this comment.
ApiExceptionHandler 클래스의 접근 제어자를 public으로 변경 고려
현재 ApiExceptionHandler 클래스가 internal로 선언되어 있습니다. Spring의 @ControllerAdvice를 사용하는 경우, Spring 프레임워크에서 해당 클래스를 빈으로 관리하기 때문에 최소한 public 또는 package-private이어야 합니다.
internal 접근 제어자를 제거하여 클래스가 public으로 선언되도록 수정해주세요:
-internal class ApiExceptionHandler(
+class ApiExceptionHandler(
private val exceptionResponseFactory: ExceptionResponseFactory,
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @ControllerAdvice | |
| internal class ApiExceptionHandler( | |
| private val exceptionResponseFactory: ExceptionResponseFactory, | |
| ) { | |
| @ControllerAdvice | |
| class ApiExceptionHandler( | |
| private val exceptionResponseFactory: ExceptionResponseFactory, | |
| ) { |
| data class ApiResponse<T>( | ||
| val status: Int, | ||
| val code: String, | ||
| val message: String, | ||
| val data: T?, | ||
| ) { |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
API 응답 클래스에 문서화를 추가해보세요
API 응답의 표준화를 위한 좋은 구조네요! 다만, 각 필드의 의미와 사용 방법에 대한 KDoc 문서화를 추가하면 더 좋을 것 같아요. 😊
아래와 같이 문서화를 추가해보는건 어떨까요?
+/**
+ * API 응답을 표준화하기 위한 데이터 클래스
+ *
+ * @param status HTTP 상태 코드
+ * @param code 비즈니스 응답 코드
+ * @param message 응답 메시지
+ * @param data 응답 데이터 (nullable)
+ */
data class ApiResponse<T>(
val status: Int,
val code: String,
val message: String,
val data: T?,
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| data class ApiResponse<T>( | |
| val status: Int, | |
| val code: String, | |
| val message: String, | |
| val data: T?, | |
| ) { | |
| /** | |
| * API 응답을 표준화하기 위한 데이터 클래스 | |
| * | |
| * @param status HTTP 상태 코드 | |
| * @param code 비즈니스 응답 코드 | |
| * @param message 응답 메시지 | |
| * @param data 응답 데이터 (nullable) | |
| */ | |
| data class ApiResponse<T>( | |
| val status: Int, | |
| val code: String, | |
| val message: String, | |
| val data: T?, | |
| ) { |
| fun <T> ok( | ||
| data: T? = null, | ||
| ): ResponseEntity<ApiResponse<T>> = ResponseEntity.ok( | ||
| ApiResponse( | ||
| status = 200, | ||
| code = "s2000", | ||
| message = "요청이 성공했습니다.", | ||
| data = data, | ||
| ), | ||
| ) | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
성공 코드를 상수로 분리하면 어떨까요?
성공 응답을 생성하는 메소드가 잘 구현되어 있네요! 코드의 재사용성과 유지보수성을 높이기 위해 상태 코드와 메시지를 상수로 분리하는 것은 어떨까요? 🤔
다음과 같이 변경해보는 건 어떨까요?
+ companion object {
+ private const val SUCCESS_STATUS = 200
+ private const val SUCCESS_CODE = "s2000"
+ private const val SUCCESS_MESSAGE = "요청이 성공했습니다."
+
fun <T> ok(
data: T? = null,
): ResponseEntity<ApiResponse<T>> = ResponseEntity.ok(
ApiResponse(
- status = 200,
- code = "s2000",
- message = "요청이 성공했습니다.",
+ status = SUCCESS_STATUS,
+ code = SUCCESS_CODE,
+ message = SUCCESS_MESSAGE,
data = data,
),
)Committable suggestion skipped: line range outside the PR's diff.
| fun <T> created( | ||
| data: T? = null, | ||
| ): ResponseEntity<ApiResponse<T>> = ResponseEntity.status( | ||
| HttpStatus.CREATED, | ||
| ).body( | ||
| ApiResponse( | ||
| status = 201, | ||
| code = "s2010", | ||
| message = "요청이 성공했습니다.", | ||
| data = data, | ||
| ), | ||
| ) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
created() 메소드도 동일한 패턴으로 개선하면 좋겠어요
created() 메소드도 잘 구현되어 있네요! ok() 메소드와 같은 방식으로 상수를 분리하면 일관성 있는 코드가 될 것 같아요. 👍
+ private const val CREATED_STATUS = 201
+ private const val CREATED_CODE = "s2010"
+ private const val CREATED_MESSAGE = "요청이 성공했습니다."
+
fun <T> created(
data: T? = null,
): ResponseEntity<ApiResponse<T>> = ResponseEntity.status(
HttpStatus.CREATED,
).body(
ApiResponse(
- status = 201,
- code = "s2010",
- message = "요청이 성공했습니다.",
+ status = CREATED_STATUS,
+ code = CREATED_CODE,
+ message = CREATED_MESSAGE,
data = data,
),
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fun <T> created( | |
| data: T? = null, | |
| ): ResponseEntity<ApiResponse<T>> = ResponseEntity.status( | |
| HttpStatus.CREATED, | |
| ).body( | |
| ApiResponse( | |
| status = 201, | |
| code = "s2010", | |
| message = "요청이 성공했습니다.", | |
| data = data, | |
| ), | |
| ) | |
| } | |
| private const val CREATED_STATUS = 201 | |
| private const val CREATED_CODE = "s2010" | |
| private const val CREATED_MESSAGE = "요청이 성공했습니다." | |
| fun <T> created( | |
| data: T? = null, | |
| ): ResponseEntity<ApiResponse<T>> = ResponseEntity.status( | |
| HttpStatus.CREATED, | |
| ).body( | |
| ApiResponse( | |
| status = CREATED_STATUS, | |
| code = CREATED_CODE, | |
| message = CREATED_MESSAGE, | |
| data = data, | |
| ), | |
| ) | |
| } |
| @Component | ||
| internal class ExceptionResponseFactory { | ||
|
|
||
| fun create(exception: DooripException): ResponseEntity<ExceptionResponse> { | ||
| val httpStatus = exception.getHttpStatus() | ||
|
|
||
| val exceptionResponse = ExceptionResponse( | ||
| status = httpStatus.value(), | ||
| code = exception.code, | ||
| message = exception.message, | ||
| ) | ||
|
|
||
| return ResponseEntity.status(httpStatus) | ||
| .body(exceptionResponse) | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
예외 처리에 로깅을 추가하면 좋을 것 같아요
예외 응답을 생성하는 팩토리가 깔끔하게 구현되어 있네요! 하지만 예외가 발생했을 때 디버깅을 위해 로깅을 추가하면 좋을 것 같아요. 😊
로깅을 추가하는 예시입니다:
+import org.slf4j.LoggerFactory
+
@Component
internal class ExceptionResponseFactory {
+ private val logger = LoggerFactory.getLogger(javaClass)
+
fun create(exception: DooripException): ResponseEntity<ExceptionResponse> {
+ logger.error("예외 발생: ${exception.message}", exception)
+
val httpStatus = exception.getHttpStatus()
val exceptionResponse = ExceptionResponse(
status = httpStatus.value(),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @Component | |
| internal class ExceptionResponseFactory { | |
| fun create(exception: DooripException): ResponseEntity<ExceptionResponse> { | |
| val httpStatus = exception.getHttpStatus() | |
| val exceptionResponse = ExceptionResponse( | |
| status = httpStatus.value(), | |
| code = exception.code, | |
| message = exception.message, | |
| ) | |
| return ResponseEntity.status(httpStatus) | |
| .body(exceptionResponse) | |
| } | |
| } | |
| import org.slf4j.LoggerFactory | |
| @Component | |
| internal class ExceptionResponseFactory { | |
| private val logger = LoggerFactory.getLogger(javaClass) | |
| fun create(exception: DooripException): ResponseEntity<ExceptionResponse> { | |
| logger.error("예외 발생: ${exception.message}", exception) | |
| val httpStatus = exception.getHttpStatus() | |
| val exceptionResponse = ExceptionResponse( | |
| status = httpStatus.value(), | |
| code = exception.code, | |
| message = exception.message, | |
| ) | |
| return ResponseEntity.status(httpStatus) | |
| .body(exceptionResponse) | |
| } | |
| } |
| internal fun DooripException.getHttpStatus(): HttpStatus = | ||
| when (this) { | ||
| is UnauthorizedException -> HttpStatus.FORBIDDEN | ||
| is UnauthenticatedException -> HttpStatus.UNAUTHORIZED | ||
|
|
||
| MethodNotAllowedException -> HttpStatus.METHOD_NOT_ALLOWED | ||
| ConflictException -> HttpStatus.CONFLICT | ||
|
|
||
| NotFoundException -> HttpStatus.NOT_FOUND | ||
|
|
||
| is ClientException -> HttpStatus.BAD_REQUEST | ||
| is ServerException, is CriticalException -> HttpStatus.INTERNAL_SERVER_ERROR | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
예외 타입을 그룹화하여 가독성을 높여보면 어떨까요?
HTTP 상태 코드 매핑이 잘 되어 있네요! 비슷한 성격의 예외들을 그룹화하면 코드의 의도를 더 잘 파악할 수 있을 것 같아요. 🤓
다음과 같이 구조화해보는 건 어떨까요?
internal fun DooripException.getHttpStatus(): HttpStatus =
when (this) {
+ // 인증/인가 관련 예외
is UnauthorizedException -> HttpStatus.FORBIDDEN
is UnauthenticatedException -> HttpStatus.UNAUTHORIZED
+ // 요청 관련 예외
MethodNotAllowedException -> HttpStatus.METHOD_NOT_ALLOWED
ConflictException -> HttpStatus.CONFLICT
-
NotFoundException -> HttpStatus.NOT_FOUND
-
is ClientException -> HttpStatus.BAD_REQUEST
+
+ // 서버 관련 예외
is ServerException, is CriticalException -> HttpStatus.INTERNAL_SERVER_ERROR
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| internal fun DooripException.getHttpStatus(): HttpStatus = | |
| when (this) { | |
| is UnauthorizedException -> HttpStatus.FORBIDDEN | |
| is UnauthenticatedException -> HttpStatus.UNAUTHORIZED | |
| MethodNotAllowedException -> HttpStatus.METHOD_NOT_ALLOWED | |
| ConflictException -> HttpStatus.CONFLICT | |
| NotFoundException -> HttpStatus.NOT_FOUND | |
| is ClientException -> HttpStatus.BAD_REQUEST | |
| is ServerException, is CriticalException -> HttpStatus.INTERNAL_SERVER_ERROR | |
| } | |
| internal fun DooripException.getHttpStatus(): HttpStatus = | |
| when (this) { | |
| // 인증/인가 관련 예외 | |
| is UnauthorizedException -> HttpStatus.FORBIDDEN | |
| is UnauthenticatedException -> HttpStatus.UNAUTHORIZED | |
| // 요청 관련 예외 | |
| MethodNotAllowedException -> HttpStatus.METHOD_NOT_ALLOWED | |
| ConflictException -> HttpStatus.CONFLICT | |
| NotFoundException -> HttpStatus.NOT_FOUND | |
| is ClientException -> HttpStatus.BAD_REQUEST | |
| // 서버 관련 예외 | |
| is ServerException, is CriticalException -> HttpStatus.INTERNAL_SERVER_ERROR | |
| } |
Related Issue ✔
close #3
Description ✔