Skip to content

Client 모듈 구현 #25

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

Merged
merged 11 commits into from
Jul 12, 2025
Merged

Client 모듈 구현 #25

merged 11 commits into from
Jul 12, 2025

Conversation

hjk0761
Copy link
Contributor

@hjk0761 hjk0761 commented Jul 7, 2025

🦡️ 관련 이슈

close #22

📍주요 변경 사항

Client 모듈을 구현했습니다.
외부 API 구현 기능을 담당하는 모듈이고, 아직 MCP 를 적용하지 않았기에 Github 관련 API 를 사용할 수 있도록 RestClient 를 설정해 주었습니다.

🎸기타

DTO 필드를 최소한으로 만들었습니다.
배치 작업이나 도메인에 저장할 때 등등 실제로 이 Github API 를 사용할 때 어느 필드까지 필요한지 가늠하기 어려워 해당 기능을 구현할 때에(가령 지표를 저장하거나, 배치로 오픈 소스 레포지토리를 긁어올 때 등) 추가적으로 수정할 수 있도록 했습니다.

Copy link

github-actions bot commented Jul 7, 2025

Test Results

0 tests   0 ✅  0s ⏱️
0 suites  0 💤
0 files    0 ❌

Results for commit ab59b4a.

♻️ This comment has been updated with latest results.

@hjk0761 hjk0761 requested review from slimsha2dy and jminkkk July 7, 2025 08:58
@hjk0761 hjk0761 self-assigned this Jul 7, 2025
@hjk0761 hjk0761 added this to OSSori Jul 7, 2025
@hjk0761 hjk0761 added this to the sprint 2 milestone Jul 7, 2025
Copy link
Contributor Author

@hjk0761 hjk0761 left a comment

Choose a reason for hiding this comment

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

구현하면서 들었던 생각 간단하게 써봤어요!

val githubClient: GithubClient
) {

@Disabled
Copy link
Contributor Author

Choose a reason for hiding this comment

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

테스트를 어떻게 만들까 고민하다 GithubClient 가 잘 돌아가는걸 확인하는게 의미있는 테스트라고 생각해 간단하게 구현했어요!
물론 실제로 Github API 를 호출하다보니 코스트가 커서 Disabled 해뒀습니다.

Copy link
Contributor

Choose a reason for hiding this comment

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

넵 저도 실제 호출을 테스트해볼 필요는 굳이 없다고 생각합니다~ 코루틴 환경이라면 저번에 얘기 나눴던 것처럼 타임아웃 테스트도 가능은 할 것 같은데 이것도 굳이 필요 없어 보이긴 하네요 ㅎㅎ


val description: String?,

@JsonProperty("html_url")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

본문에서 이야기했듯, 최소한의 지표들을 담았습니다.
이후에 지표를 구하는 등의 기능 구현 때 조금 수정이 필요할 거에요!

Comment on lines 33 to 35
private fun getGithubToken(): String {
return "$AUTHORIZATION_METHOD ${githubClientProperties.token}"
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

해당 메서드는 사용되지 않고 있어요.
그 이유는 이 메서드의 목적이 발급받은 PAT 를 Github API 에 같이 보내 제한을 늘리는 역할인데, 아직 PAT 이 필요한지 확실치 않아서 일단 주석처리 해뒀어요.
필요해지면 발급받은 PAT를 설정파일에 주입하면 됩니다!

return "$AUTHORIZATION_METHOD ${githubClientProperties.token}"
}

private inline fun <reified T> defaultGithubGetClient(uri: String): T {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

사실 이렇게 GithubClient 를 추상화 시킨건 조금 불필요한 처사일수도 있는데요,
중복되는 에러 검증 부분을 분리할 수 없다보니 불가피하게 restClient 자체를 한 단계 추상화 시키게 되었어요.
혹시 불필요하다고 생각되면 편하게 말해주세요!

+) 코틀린에서 제네릭쓰는 것도 신기하고, _로 사용되지 않는 인자를 치환하는것도 신기해서 써본것도 있어요..ㅎㅎ

Copy link
Contributor

Choose a reason for hiding this comment

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

inline fun.. 도 생소하네여 코틀린 열심히 해야겠다...^_^

Copy link
Contributor

Choose a reason for hiding this comment

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

저는 나름 중복되는 로직이라 좋은 것 같아요.
근데 저도 예전에 짜다가 inline + reified 찾아본 적 있긴 한데 inline은 성능이나 에러 추적을 조심해야 한다고 본 것 같긴 해요. 아마 이 부분에서는 괜찮을 것 같긴 하네요!

Copy link
Contributor

@jminkkk jminkkk left a comment

Choose a reason for hiding this comment

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

무빈~~~~
수고했어요!!! 이제 제 배치만 올라가면 되는 것?ㅋㅋㅋ
리뷰 조금 남겼습니다. 이번에도 같이 재밌게 이야기해봐용~~!

Comment on lines +1 to +2
security:
github:
Copy link
Contributor

Choose a reason for hiding this comment

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

이후 mcp도 security 하위에 위치하는 걸까요?
mcp에 대한 설정들이 추가되면 github 이랑 같은 depth일 것 같은데, security보다 client 로 가면 어떨까요..??

받아들여지지 않아도 되는 소소한 제안입니다...ㅎㅎ

Copy link
Contributor Author

Choose a reason for hiding this comment

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

depth 는 같겠지만, 아무래도 깃허브 토큰 혹은 MCP 토큰 도 있고, 로그인이 들어가면 Github 에 좀 더 종속적인 설정값들이 생기게 될 것 같아서 일단은 이렇게 둘께요!
MCP 가 들어오고나서 확인해봐도 좋을 것 같아요!

Copy link
Contributor

Choose a reason for hiding this comment

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

아하 넵넵
url이 security 하위에 있길래, security라는 명명 이유가 궁금했었어용 ㅎㅎ

mcp에 어떤 값들이 필요하거나 들어올지 모르니 일단 그대로 가시죠!

@JsonProperty("html_url")
val githubLink: String,

@JsonProperty("stargazers_count")
Copy link
Contributor

Choose a reason for hiding this comment

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

오 깃헙에서는 stargazers_count 이렇코롬 주는군여
신기방기

Copy link
Contributor

Choose a reason for hiding this comment

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

난 그닥

@@ -0,0 +1,23 @@
package dto
Copy link
Contributor

Choose a reason for hiding this comment

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

혹쉬 저희 패키지 컨벤션이 dto 아래 response가 하나 더 있었던 것 같아서...
확인 부탁드려용!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

헉 컨벤션을 놓쳤네요.. 바로 수정할께요!

Comment on lines 15 to 16
requestFactory.setConnectTimeout(Duration.ofSeconds(5))
requestFactory.setReadTimeout(Duration.ofSeconds(10))
Copy link
Contributor

Choose a reason for hiding this comment

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

오호 이 타임아웃 값들은 혹시 어떻게 설정된 걸까요?
connection timeout은 5초보다 짧게 설정되어도 괜찮을 것 같은데 어떨까요?

read timeout은 찾아보니 깃헙 측에서 요청 처리 시간이 10초 이상이 되면 server error를 날린다고 하니 적절한 것 같아요! 넘좋습니다 ㅎㅎ

github docs - troubleshooting-the-rest-api

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ReadTimeout 은 문서대로 10초로 설정한게 맞아요!
하지만 ConnectionTimeout 은 1초는 여러 네트워크 환경에 따라 충분히 오차가 발생할 수 있다고 생각헀고, 연결 자체를 수립하는 데에 ReadTimeout 과 같은 값인 10초는 너무 길다고 판단해 그 중간값으로 잡았습니다!

5초도 확실히 길다고 생각할 수 있을 것 같은데, 예기치 못한 상황을 고려해서 넉넉하게 잡은 이유도 있습니다 ㅎㅎ
2~3초 정도도 일상적인 상황에서 발생하지 않을 시간이라고 생각하긴 하는데, 몇 초 정도가 적당하다고 생각하나요?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

아무래도 Connection 까지 2초 이상 걸리는 상황이 일반적인 상황이 아닐 것이라고 생각해 2초로 제한을 낮췄습니다!

Copy link
Contributor

Choose a reason for hiding this comment

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

오홍 좋아요!
저도 찾아보다가 발견한 여러가지 재미있는 내용 공유해요!

TCP retransmission과 튜닝 포인트 글에서 Connection Timeout 관련 요약

Linux에서 InitRTO의 기본값은 1초이다. (* InitRTO는 클라이언트가 SYN 요청 후 SYN-ACK 응답이 올 때까지 기다리는 시간, 초과될 경우 재전송 )
따라서 TCP 커넥션을 최초 수립할 때, 즉 TCP handshake를 진행하는 과정에서 재전송이 1초보다 더 걸릴 수 있기 때문에 기본 애플리케이션 타임아웃은 1초보다 커야한다. (RFC 6298 - Computing TCP's Retransmission Timer에서도 1초 권장)
image

다만, 커넥션 풀 방식으로 네트워크 세션을 미리 만들어줄 때는 InitRTO가 발생할 일이 없어서 더 작은 값도 설정해도 된다.


현재는 타임아웃 설정이 SimpleClientHttpRequestFactory을 통해 지정되어 있는데요! 이 경우 커넥션 풀이 아닌 하나의 커넥션만 사용한다고 합니다.

따라서 요청할 때 커넥션이 수립되니 2초로 해도 좋을 것 같아요.

나중에 요청이 많아지거나 할 때 최적화를 위해 커넥션 풀을 생성하여 관리하는 PoolingHttpClientConnectionManager로의 전환을 고려해보는 것도 재밌을 것 같아요!

Copy link
Contributor

Choose a reason for hiding this comment

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

오 좋은 내용 찾아오셨네요 몰리~ 몰랐던 내용인데 좋네요

Comment on lines 43 to 48
.onStatus({ it.is4xxClientError }) { _, response ->
throw RuntimeException("Status code: ${response.statusCode}, Description: ${response.body}")
}
.onStatus({ it.is5xxServerError }) { _, _ ->
throw RuntimeException("Github Server Unavailable")
}
Copy link
Contributor

Choose a reason for hiding this comment

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

오호 요렇게 처리되었군여!
우테코 미션을 할 때 ResponseErrorHandler를 통해서 외부 요청에 대해 에러 핸들링을 했었던 기억이 나네요...ㅎㅎ

ResponseErrorHandler가 RestTemplate랑 RestClient 사용할 때 받은 응답에 에러 핸들링하는 얘인데 이 친구를 활용하면 github에서 받을 다양한 응답 케이스를 조금 더 수월하게 핸들링 할 수 있을 것 같기도 하네요!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

아 ResponseErrorHandler 구현체를 보다가 deprecated 라는 문구를 봐서 간단하게 구현했는데, 다시 보니까 한 메서드만 deprecated 된거 였네요,,,

혼자 생각을 많이 하다가 ResponseErrorHandler 를 사용해 보셨다니까 질문이 있어요! 물론 저도 ResponseErrorHandler 를 만드는게 좀 더 직관적이고 쉬운 접근일 것 같은데, 함수형 인터페이스를 잘 사용하면 같은 기능을 onStatus 에도 넣을 수 있을 것 같긴해요. 확실히 ResponseErrorHandler 가 편하고 직관적이었나요?

  • ) 추가적으로 고민이 있는데, 보시다시피 RuntimeException 을 사용하고 있어요 ㅠㅠ 적절한 커스텀 예외를 만들어야 할 것 같은데 네이밍 추천 받습니다..ㅎ.ㅎ

Copy link
Contributor Author

Choose a reason for hiding this comment

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

에러 헨들링을 한 곳에서 원하는만큼 커스텀 할수도 있고, 패키지 구분도 명확해진 부분이 좋은 것 같아서 ResponseErrorHandler 를 사용하도록 수정해봤어요!
지금 패키지 구분이 어떤지는 의견이 듣고싶네요 ㅎㅎ

+) 에러 또한 GithubResponseException 으로 만들어뒀어요! 더 좋은 이름있으면 편하게 제안주세요!

Comment on lines 28 to 32
class GithubClientTest(

@Autowired
val githubClient: GithubClient
) {
Copy link
Contributor

Choose a reason for hiding this comment

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

요고 보니까 급 생각난 PR
최근에 스프링 레포를 많이 구경했는데, 7.x에선 RestTestClient가 생길 것 같더라구요
ㄷㄱㄷㄱ

Spring-Framework - Add RestTestClient

Copy link
Contributor Author

Choose a reason for hiding this comment

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

와 이런게 생긴다니..
ㄷㄱㄷㄱ

Copy link
Contributor

Choose a reason for hiding this comment

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

스프링 레포를 챙겨보다니 대단하시네요

Copy link
Contributor

@jminkkk jminkkk left a comment

Choose a reason for hiding this comment

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

무빈 수고했어요!!! 👍

Comment on lines 16 to 19
throw GithubResponseException("Status code: ${response.statusCode}, Description: ${response.body}")
}
if (response.statusCode.is5xxServerError) {
throw GithubResponseException("Github Server Unavailable")
Copy link
Contributor

Choose a reason for hiding this comment

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

400대, 500대 응답 형식의 일관성을 위해 statusCode를 따로 인자로 전달해도 좋을 것 같다는 생각이 드네용..

사실 client 요청이 단순 조회 API라 에러 핸들링을 과하는 필요없을 것 같아서 이정도도 괜춘한 것 같습니다!

Copy link
Contributor Author

@hjk0761 hjk0761 Jul 11, 2025

Choose a reason for hiding this comment

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

혹여나 400대 에러 반환에 대해서 별다른 조치를 취할 수도 있겠다는 생각에 일단 메시지를 분리했었는데요, 응답 형식이 일관될 때 프론트에서든 처리가 더 용이하겠다는 생각이 들어 메시지를 통일하도록 수정할께요! 좋은 의견 고마워요 ㅎㅎ

추가적으로 GithubResponseException 도 기존 메시지 구조를 가져가도록 수정했어요. 예외를 던지는 입장에선 상태코드와 메시지만을 전달하도록 해봤어요.

Copy link
Contributor

@slimsha2dy slimsha2dy left a comment

Choose a reason for hiding this comment

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

무빈 수고 많으셨습니다!
냉철하고 까다롭게 리뷰 달려고 했는데 이미 너무 잘 돼있고 몰리도 리뷰를 꼼꼼하게 잘 해주셔서 이미 논의해볼 만한 내용은 많이 나온 것 같아요. 그냥 의견 관련만 조금 남겼고 바로 APPROVE 하겠습니다 ㅋ

Comment on lines +5 to +12
@ConfigurationProperties(prefix = "security.github")
data class GithubClientProperties(

val repositoryBaseUrl: String,

val searchRepositoryBaseUrl: String,

val token: String
Copy link
Contributor

Choose a reason for hiding this comment

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

이거는 GithubClient에서 분리한 이유가 따로 있나용? 다른 클래스에서도 사용될 것이라고 생각해서 분리한건가요?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

이렇게 구현한 의도는 앞에서 몰리가 질문해준 것과 비슷해요 참고. security 라는 분류 아래에 여러 외부 API 이용을 위한 값들을 위치시키려는 의도였는데, 지금은 구현된 부분인 Github Rest API 를 위한 값들만 들어가 있어요.
하지만 나중엔 예를들어 Github OAuth 와 관련된 값 등 Github 와는 연관있지만 Rest API 와는 다른 부분의 값들이 들어올 수도 있겠다고 생각했어요.

이런 생각에서 일단 GithubClientProperties 를 분리했는데, 아무래도 나중에 기능이 확장된다면 그에 맞게 설정 파일의 구조가 바뀌거나, 지금 GithubClientProperties 가 변하거나 그런 조치가 취해질 것 같아요!
알파카가 말해준 것처럼 다시 GithubClient 로 흡수될 수도 있을 것 같기도 해요 ㅎㅎ

val githubClient: GithubClient
) {

@Disabled
Copy link
Contributor

Choose a reason for hiding this comment

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

넵 저도 실제 호출을 테스트해볼 필요는 굳이 없다고 생각합니다~ 코루틴 환경이라면 저번에 얘기 나눴던 것처럼 타임아웃 테스트도 가능은 할 것 같은데 이것도 굳이 필요 없어 보이긴 하네요 ㅎㅎ

return "$AUTHORIZATION_METHOD ${githubClientProperties.token}"
}

private inline fun <reified T> defaultGithubGetClient(uri: String): T {
Copy link
Contributor

Choose a reason for hiding this comment

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

저는 나름 중복되는 로직이라 좋은 것 같아요.
근데 저도 예전에 짜다가 inline + reified 찾아본 적 있긴 한데 inline은 성능이나 에러 추적을 조심해야 한다고 본 것 같긴 해요. 아마 이 부분에서는 괜찮을 것 같긴 하네요!

@JsonProperty("html_url")
val githubLink: String,

@JsonProperty("stargazers_count")
Copy link
Contributor

Choose a reason for hiding this comment

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

난 그닥

Comment on lines 15 to 16
requestFactory.setConnectTimeout(Duration.ofSeconds(5))
requestFactory.setReadTimeout(Duration.ofSeconds(10))
Copy link
Contributor

Choose a reason for hiding this comment

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

오 좋은 내용 찾아오셨네요 몰리~ 몰랐던 내용인데 좋네요

Comment on lines 28 to 32
class GithubClientTest(

@Autowired
val githubClient: GithubClient
) {
Copy link
Contributor

Choose a reason for hiding this comment

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

스프링 레포를 챙겨보다니 대단하시네요

@hjk0761 hjk0761 merged commit eddc612 into develop Jul 12, 2025
3 checks passed
@github-project-automation github-project-automation bot moved this to Done in OSSori Jul 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

외부 API 호출 기능 구현
3 participants