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

YW2-237 feat: 공통 Response 및 ControllerAdvice 정의 #5

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

Conversation

ktj1997
Copy link
Collaborator

@ktj1997 ktj1997 commented Sep 30, 2023

🎯 작업 내역

  • 공통 Response를 정의했습니다.
  • 공통 Exception을 처리할 수 있는 RestControllerAdvice를 정의했습니다.
  • 임의로 구성한 것이니, 누락된 것이 있거나 과해보이는 것이 있으면 의견 부탁드립니다.

📜 Jira 티켓

https://yapp22-web2.atlassian.net/browse/YW2-237

📁 영향범위 (모듈)

  • dangle-api

📒 SQL (DDL 변경사항 있는 경우만)

---
Resolves: # See also: #

@ktj1997 ktj1997 added the ✨ Feature Introduce new features. label Sep 30, 2023
@ktj1997 ktj1997 requested a review from rilac1 September 30, 2023 13:57
@ktj1997 ktj1997 changed the title feat: 공통 Response 및 ControllerAdvice 정의 [YW2-237]-feat: 공통 Response 및 ControllerAdvice 정의 Sep 30, 2023
@ktj1997 ktj1997 changed the title [YW2-237]-feat: 공통 Response 및 ControllerAdvice 정의 YW2-237 feat: 공통 Response 및 ControllerAdvice 정의 Sep 30, 2023
Copy link
Member

@rilac1 rilac1 left a comment

Choose a reason for hiding this comment

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

LGTM 👍
코멘트 확인 부탁드려요~!

in 4000 until 4100 -> HttpStatus.BAD_REQUEST
in 4100 until 4200 -> HttpStatus.NOT_FOUND
in 5000 until 5100 -> HttpStatus.INTERNAL_SERVER_ERROR
else -> HttpStatus.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.

에러코드 범위 확인했습니다~!

if(activeProfilesResolver.isPrd()){
return null
}
return this.stackTraceToString()
Copy link
Member

Choose a reason for hiding this comment

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

오호 Prod 환경에서는 스택트레이스를 노출하지 않는군요,,

val data: T? = null,
val error: ErrorResponse? = null,
val debug: String? = null
){
Copy link
Member

Choose a reason for hiding this comment

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

이 부분 SealedInterface 적용해보는건 어떨까요?

sealed interface ApiResponse<T> {
    val result: ResponseType

    data class Success<T>(
        val data: T?
    ) : ApiResponse<T> {
        override val result = ResponseType.SUCCESS
    }

    data class Error<T>(
        val error: ErrorResponse? = null,
        val debug: String? = null
    ) : ApiResponse<T> {
        override val result = ResponseType.ERROR
    }
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

sealed interface를 잘 사용해볼 버릇을 안해봐서 정확한 사용처? 에 대해서 아직 와닿지않는 것 같습니다 ㅎㅎ..
사실 사용해본 것도 enum과 같이 when절 -> else 미정의시 컴파일 타임에 에러 잡기위한 용도 정도밖에 없었어서요

혹시 sealed 키워드 (클래스, 인터페이스 포함) 를 사용하시는 기준이 있을까요?
그와 더불어서, 제안해주신 코드 대로 진행한다면 성공때와 실패때의 응답이 달라질 것 같은데 혹시맞을까요?
클라이언트 측에서는 어차피 하나의 모델로 대응가능할 것 같긴합니다만

data class ErrorResponse(
val code: Int,
val message: String,
val data: String? = null
Copy link
Member

@rilac1 rilac1 Oct 2, 2023

Choose a reason for hiding this comment

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

ErrorResponse의 data 부는 어떤 값들이 들어가게 될까요?

특별한 값이 아니라면, DangleExeption만으로도 표현이 가능할 것 같아서요!
(message는 사용자에게 직접 노출되는 에러메시지)

data class Error<T>(
    val code: String,
    val message: String,
    val debug: String? = null,
) : ApiResponse<T> {
    override val result = ResponseType.ERROR
}

ApiResponse.Error(
    error = dangleException.errorCode.value,
    message = dangleException.errorCode.message,
    debug = dangleException.debug,
)

@rilac1
Copy link
Member

rilac1 commented Oct 2, 2023

추가질문) 저희 클라이언트에서 아래 ResponseType 값을 바라보고 있나요?
성공 여부를 HttpStatus 200으로 분기하는지, ResponseType으로 분기하는지 궁금해서요!

enum class ResponseType{
    SUCCESS, ERROR
}

@ktj1997
Copy link
Collaborator Author

ktj1997 commented Oct 3, 2023

지금은 그냥 200으로 분기하고 있는걸로 알고있습니다.
CustomResponse가 아닌 ReposeEntity로 넘기고 있어서요 ㅎㅎ..

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

Successfully merging this pull request may close these issues.

2 participants