ev-service 마이그레이션 by asp345 · Pull Request #308 · wafflestudio/snutt

asp345 · 2024-12-20T12:27:16Z

ev-service 프록시하는 부분을 기존에 쓰던 SnuttEvWebClient 사용해서 마이그레이션 진행했습니다

하면서 생긴 몇가지 문제가 있어서 공유드려요

req.method() 사용 불가: handler의 req에서 method()를 쓰면 에러가 나서 어쩔수 없이 http method마다 따로 함수를 만들어놨어요
SnuttEvWebClient read timeout 문제: 로컬에서 테스트할 때 SnuttEvWebClient의 read timeout을 초과할 때가 있고, 1.5초 정도로 늘리면 잘 되긴 하는데 올리는게 좋을까요?
/v1/users/me/lectures/latest 의 경우 query parameter에 json을 넣게 되어 있는데 이게 기본적으로는 스프링이 { 같은 특수문자를 변수 바꿔넣는 자리로 인식해서 이건 UriComponentsBuilder로 우회를 했어요

테스트 안돌아가서 다시 볼게요

Hank-Choi

제대론 안봤는데요
일단 테스트 깨지는 것 같고

또 응답에 유저 아이디 반환하면 유저 객체 반환하는 부분이 안보이네요

asp345 · 2024-12-21T01:08:52Z

테스트는 고쳤고 유저 정보 넣는거 추가할게요
에러 핸들링도 좀 더 잘해줘야 할거 같네요

SeonghaeJo · 2024-12-28T15:02:38Z

core/src/main/kotlin/evaluation/service/EvService.kt

+            }
+
+        val encodedJson =
+            withContext(Dispatchers.IO) {


이 부분에 왜 withContext를 썼는지, 그리고 왜 Dispatchers.IO를 썼는지 궁금하네

비슷하게 해보니까
URLEncoder.encode 에서 블라킹이 발생할 수 있다고 하는데
그렇다고 IO Dispatcher에서 돌리는 것도 좀 이상하다고 생각합니다.

SeonghaeJo

아직은 코드만 봤는데 테스트를 조금 더 해볼게

SeonghaeJo · 2024-12-28T15:08:46Z

core/src/main/kotlin/evaluation/service/EvService.kt

+                URLEncoder.encode(
+                    ObjectMapper().setPropertyNamingStrategy(PropertyNamingStrategies.SNAKE_CASE).writeValueAsString(recentLectures),
+                    "UTF-8",
+                )


objectMapper는 dependency injection해서 써도 될 듯

Hank-Choi

주요 코드 리뷰드립니다.

Hank-Choi · 2025-01-07T09:22:33Z

core/src/main/kotlin/common/exception/ProxyException.kt

+class ProxyException(
+    val statusCode: HttpStatusCode,
+    val errorBody: Map<String, Any?>,
+) : RuntimeException(ObjectMapper().writeValueAsString(errorBody))


Exception 파일인데 ObjectMapper를 가져오는게 조금 어색해요

그냥 message 빼도 될 것 같은데요
어차피 Throwable.message 쓰는 경우 없을테니..

그리고 이거 ev 서비스란 것도 명시해주세요
아마 이런식으로 프록시하는건 이게 마지막일거라서..

Hank-Choi · 2025-01-07T09:34:14Z

api/src/main/kotlin/filter/ErrorWebFilter.kt

+                if (throwable is ProxyException) {
+                    exchange.response.statusCode = throwable.statusCode
+                    exchange.response.headers.contentType = MediaType.APPLICATION_JSON
+                    exchange.response.writeWith(
+                        Mono.just(
+                            exchange.response
+                                .bufferFactory()
+                                .wrap(objectMapper.writeValueAsBytes(throwable.errorBody)),
+                        ),
+                    )
+                } else {
+                    val errorBody: ErrorBody
+                    val httpStatusCode: HttpStatusCode
+                    when (throwable) {
+                        is Snu4tException -> {
+                            httpStatusCode = throwable.error.httpStatus
+                            errorBody = makeErrorBody(throwable)
+                        }
+
+                        is ResponseStatusException -> {
+                            httpStatusCode = throwable.statusCode
+                            errorBody =
+                                makeErrorBody(
+                                    Snu4tException(
+                                        errorMessage = throwable.body.title ?: ErrorType.DEFAULT_ERROR.errorMessage,
+                                    ),
+                                )
+                        }
+
+                        else -> {
+                            log.error(throwable.message, throwable)
+                            httpStatusCode = HttpStatus.INTERNAL_SERVER_ERROR
+                            errorBody = makeErrorBody(Snu4tException())
+                        }
                    }


기존 when 이 있는데 depth가 더 들어가서 한눈에 안들어오네요

Suggested change

if (throwable is ProxyException) {

exchange.response.statusCode = throwable.statusCode

exchange.response.headers.contentType = MediaType.APPLICATION_JSON

exchange.response.writeWith(

Mono.just(

exchange.response

.bufferFactory()

.wrap(objectMapper.writeValueAsBytes(throwable.errorBody)),

),

)

} else {

val errorBody: ErrorBody

val httpStatusCode: HttpStatusCode

when (throwable) {

is Snu4tException -> {

httpStatusCode = throwable.error.httpStatus

errorBody = makeErrorBody(throwable)

}

is ResponseStatusException -> {

httpStatusCode = throwable.statusCode

errorBody =

makeErrorBody(

Snu4tException(

errorMessage = throwable.body.title ?: ErrorType.DEFAULT_ERROR.errorMessage,

),

)

}

else -> {

log.error(throwable.message, throwable)

httpStatusCode = HttpStatus.INTERNAL_SERVER_ERROR

errorBody = makeErrorBody(Snu4tException())

}

}

val errorBody: Any

val httpStatusCode: HttpStatusCode

when (throwable) {

is ProxyException -> {

httpStatusCode = throwable.statusCode

errorBody = throwable.errorBody

}

is Snu4tException -> {

httpStatusCode = throwable.error.httpStatus

errorBody = makeErrorBody(throwable)

}

is ResponseStatusException -> {

httpStatusCode = throwable.statusCode

errorBody =

makeErrorBody(

Snu4tException(errorMessage = throwable.body.title ?: ErrorType.DEFAULT_ERROR.errorMessage),

)

}

else -> {

log.error(throwable.message, throwable)

httpStatusCode = HttpStatus.INTERNAL_SERVER_ERROR

errorBody = makeErrorBody(Snu4tException())

}

}

그냥 이렇게 하면 안되나요

errorBody가 Any 타입으로만 바뀝니다.

Hank-Choi · 2025-01-07T10:33:41Z

core/src/main/kotlin/evaluation/service/EvService.kt

+
+        return snuttEvWebClient.get()
+            .uri { builder ->
+                UriComponentsBuilder.fromUri(builder.build())


이 라인은 왜 있는 거에요?

Hank-Choi · 2025-01-07T10:43:00Z

core/src/main/kotlin/evaluation/service/EvService.kt

+            }
+
+        val encodedJson =
+            withContext(Dispatchers.IO) {


비슷하게 해보니까
URLEncoder.encode 에서 블라킹이 발생할 수 있다고 하는데
그렇다고 IO Dispatcher에서 돌리는 것도 좀 이상하다고 생각합니다.

Hank-Choi · 2025-01-07T10:45:51Z

core/src/main/kotlin/evaluation/service/EvService.kt

+        val encodedJson =
+            withContext(Dispatchers.IO) {
+                URLEncoder.encode(
+                    ObjectMapper().setPropertyNamingStrategy(PropertyNamingStrategies.SNAKE_CASE).writeValueAsString(recentLectures),
+                    "UTF-8",
+                )
+            }
+
+        return snuttEvWebClient.get()
+            .uri { builder ->
+                UriComponentsBuilder.fromUri(builder.build())
+                    .path("/v1/users/me/lectures/latest")
+                    .queryParam("snutt_lecture_info", encodedJson)
+                    .queryParams(requestQueryParams)
+                    .build(true).toUri()
+            }


webclient uri 빌더 문법 저도 싫어하는데
Encoder 가 dispatcher에 쌓여있는게 좀 더 싫어서 아래 코드가 나아보입니다.

Suggested change

val encodedJson =

withContext(Dispatchers.IO) {

URLEncoder.encode(

ObjectMapper().setPropertyNamingStrategy(PropertyNamingStrategies.SNAKE_CASE).writeValueAsString(recentLectures),

"UTF-8",

)

}

return snuttEvWebClient.get()

.uri { builder ->

UriComponentsBuilder.fromUri(builder.build())

.path("/v1/users/me/lectures/latest")

.queryParam("snutt_lecture_info", encodedJson)

.queryParams(requestQueryParams)

.build(true).toUri()

}

val lectureInfoParam = ObjectMapper().setPropertyNamingStrategy(PropertyNamingStrategies.SNAKE_CASE).writeValueAsString(recentLectures)

return snuttEvWebClient.get()

.uri { builder ->

builder

.path("/v1/users/me/lectures/latest")

.queryParam("snutt_lecture_info", "{lectureInfoParam}")

.queryParams(requestQueryParams)

.build(lectureInfoParam)

}

Hank-Choi

LGTM

나중엔 여기서 모든 path를 관리하는 것도 고려해보죠

Hank-Choi · 2025-01-08T16:25:22Z

core/src/main/kotlin/evaluation/dto/EvUserDto.kt

+data class EvUserDto(
+    val id: String?,
+    val email: String?,
+    val local_id: String?,


네이밍 통일하시죠 JsonProperty로 json에만 스네이크케이스면 좋겠습니다.

ev-service 마이그레이션

82b9922

asp345 requested review from a team and PFCJeong as code owners December 20, 2024 12:27

asp345 requested review from Hank-Choi, SeonghaeJo and davin111 and removed request for a team December 20, 2024 12:27

/v1/users/me/lectures/latest 마이그레이션

dfbd1c8

asp345 force-pushed the feature/ev-service branch from bb60182 to dfbd1c8 Compare December 20, 2024 12:31

dto 분리

1e6d62d

Hank-Choi reviewed Dec 20, 2024

View reviewed changes

service 분리 및 최근 과목 수정

7301714

asp345 force-pushed the feature/ev-service branch from d93748d to 7301714 Compare December 21, 2024 01:02

asp345 added 3 commits December 21, 2024 10:29

handle empty body

f4465d7

fix endpoint

0f00915

응답의 user_id 사용자 정보로 교체

774bfa4

asp345 force-pushed the feature/ev-service branch from 70bcb71 to 774bfa4 Compare December 21, 2024 02:30

asp345 added 5 commits December 21, 2024 20:37

throw errors from ev

9a392cd

add headers

e14d853

suppress unchecked cast warning

4013766

webclient 오류 그대로 넘겨주기

563f222

serialize error body

04a15fd

SeonghaeJo reviewed Dec 28, 2024

View reviewed changes

Hank-Choi reviewed Jan 7, 2025

View reviewed changes

asp345 added 3 commits January 8, 2025 14:33

Merge remote-tracking branch 'origin/develop' into feature/ev-service

c1e07a3

에러 핸들링 수정

94709ce

url 변환 수정

b0faccf

use DI for objectMapper

077fd3b

Hank-Choi approved these changes Jan 8, 2025

View reviewed changes

remove snake case

ca95dd6

asp345 merged commit 5b6db29 into develop Jan 9, 2025
2 checks passed

asp345 deleted the feature/ev-service branch January 9, 2025 04:07

Conversation

asp345 commented Dec 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Hank-Choi left a comment

Choose a reason for hiding this comment

Uh oh!

asp345 commented Dec 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SeonghaeJo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Hank-Choi left a comment

Choose a reason for hiding this comment

Uh oh!

Hank-Choi Jan 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Hank-Choi Jan 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Hank-Choi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

asp345 commented Dec 20, 2024 •

edited

Loading

asp345 commented Dec 21, 2024 •

edited

Loading

Hank-Choi Jan 7, 2025 •

edited

Loading

Hank-Choi Jan 7, 2025 •

edited

Loading