-
Notifications
You must be signed in to change notification settings - Fork 1
[LNK-67] global common 코틀린 마이그레이션 #86
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
The head ref may contain hidden characters: "refactor/LNK-67-Leenk-global-common-\uCF54\uD2C0\uB9B0-\uB9C8\uC774\uADF8\uB808\uC774\uC158"
Conversation
작업 개요Java 기반 예외 처리, 응답 래퍼, 페이지네이션 유틸리티, 상태 확인 컨트롤러를 Kotlin으로 마이그레이션하고, 새로운 에러 코드 설명 주석과 API 예제 시스템을 도입합니다. 추가로 Kotlin 마이그레이션 가이드 및 코드 리뷰 명령 문서를 작성합니다. 변경 사항
예상 코드 리뷰 노력🎯 4 (Complex) | ⏱️ ~45분 관련된 가능성이 있는 PR
제안된 레이블
제안된 리뷰어
시
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In @.claude/commands/code-review.md:
- Around line 30-34: The folder tree indentation in code-review.md is
misleading: make the "code-review-agent.md" entry appear nested under the
"agents/" directory by adjusting the ASCII tree spacing so
"code-review-agent.md" is indented beneath "agents/" (e.g., add the same
indentation level as "code-review.md" has under "commands/"); update the entries
for agents/ and code-review-agent.md so the tree clearly shows
code-review-agent.md as a child of agents/.
In @.claude/commands/kotlin-mirgrate.md:
- Around line 29-35: The example code block in the kotlin-mirgrate.md file lacks
a language tag (the block starting with "Task tool:" and containing
"subagent_type: kotlin-migration-agent"), which may trigger markdownlint MD040;
update the opening fence from ``` to ```text (or another appropriate language
like ```yaml) for that example code block so the fenced code block declares its
language and satisfies MD040.
In `@build.gradle.kts`:
- Around line 62-64: Replace the incompatible springmockk dependency version: in
build.gradle.kts update the testImplementation entry that currently reads
"com.ninja-squad:springmockk:5.0.1" to a Spring Boot 3.x–compatible 4.x release
(e.g., "com.ninja-squad:springmockk:4.x"), leaving the MockK dependency
(testImplementation("io.mockk:mockk:1.14.7")) as-is so MockK remains at 1.14.7
which is compatible with springmockk 4.x.
In
`@src/main/kotlin/leets/leenk/global/common/exception/GlobalExceptionHandler.kt`:
- Around line 57-81: The handlers handleMessageNotReadable and handleAll in
GlobalExceptionHandler currently expose exception messages to clients; change
responses to always use the public error message from ErrorCode (e.g.,
errorCode.message) and, when the cause is a BaseException, return cause.message
(or fallback to cause.errorCode.message) instead of ex.message; also record the
full exception details (stacktrace and original message) to a server-side logger
(e.g., logger.error) before building the ResponseEntity so diagnostics are
preserved while preventing internal info leakage in CommonResponse.error.
🧹 Nitpick comments (4)
src/main/kotlin/leets/leenk/global/common/exception/response/ValidErrorResponse.kt (1)
8-15: 팩토리 메서드가 불필요할 수 있습니다.Kotlin data class는 이미 public 생성자를 제공하므로
of팩토리 메서드는 선택적입니다. Java 코드와의 호환성이 필요하지 않다면 제거를 고려해볼 수 있습니다.단, 기존 Java 코드에서
ValidErrorResponse.of(...)를 호출하는 곳이 있다면 현재 구현이 적절합니다.src/test/kotlin/leets/leenk/global/common/exception/response/ValidErrorResponseTest.kt (1)
6-6: TODO 주석이 불필요해 보입니다.이 테스트는 이미 Kotest의 DescribeSpec을 사용한 Kotlin 스타일로 작성되어 있습니다. TODO 주석을 제거하거나 업데이트하는 것이 좋겠습니다.
src/test/kotlin/leets/leenk/global/common/exception/BaseExceptionTest.kt (1)
7-7: TODO 주석이 불필요해 보입니다.ValidErrorResponseTest.kt와 마찬가지로, 이 테스트도 이미 Kotest DescribeSpec을 사용한 Kotlin 스타일로 작성되어 있습니다.
src/test/kotlin/leets/leenk/global/common/exception/ResourceLockedExceptionTest.kt (1)
3-5: Kotest matcher로 교체하면 실패 메시지가 더 명확합니다.
assert대신 Kotest의shouldBeInstanceOf를 쓰면 실패 시 진단이 쉬워집니다.♻️ 제안 변경
-import io.kotest.matchers.shouldBe +import io.kotest.matchers.shouldBe +import io.kotest.matchers.types.shouldBeInstanceOf @@ - assert(exception is BaseException) + exception.shouldBeInstanceOf<BaseException>()Also applies to: 21-25
| @ExceptionHandler(HttpMessageNotReadableException::class) | ||
| fun handleMessageNotReadable(ex: HttpMessageNotReadableException): ResponseEntity<CommonResponse<Void?>> = | ||
| when (val cause = ex.mostSpecificCause) { | ||
| is BaseException -> { | ||
| ResponseEntity | ||
| .status(cause.errorCode.status) | ||
| .body(CommonResponse.error(cause.errorCode, ex.message ?: cause.errorCode.message)) | ||
| } | ||
|
|
||
| else -> { | ||
| val errorCode = ErrorCode.JSON_PARSE_ERROR | ||
| ResponseEntity | ||
| .status(errorCode.status) | ||
| .body(CommonResponse.error(errorCode, ex.message ?: errorCode.message)) | ||
| } | ||
| } | ||
|
|
||
| @ExceptionHandler(Exception::class) | ||
| fun handleAll(e: Exception): ResponseEntity<CommonResponse<Void?>> { | ||
| val errorCode = ErrorCode.INTERNAL_SERVER_ERROR | ||
| val body = CommonResponse.error(errorCode, e.message ?: errorCode.message) | ||
|
|
||
| return ResponseEntity | ||
| .status(errorCode.status) | ||
| .body(body) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
예외 메시지를 그대로 반환하면 내부 정보 노출 위험이 있습니다.
HttpMessageNotReadableException/Exception에서 ex.message를 그대로 내려주면 파싱 상세나 내부 정보가 노출될 수 있습니다. 사용자 응답은 errorCode.message로 제한하고, 상세는 로그로 남기는 쪽이 안전합니다. 또한 BaseException cause일 때는 wrapper 메시지 대신 cause 메시지를 사용하는 편이 의도에 맞습니다.
🔧 제안 변경
`@ExceptionHandler`(HttpMessageNotReadableException::class)
fun handleMessageNotReadable(ex: HttpMessageNotReadableException): ResponseEntity<CommonResponse<Void?>> =
when (val cause = ex.mostSpecificCause) {
is BaseException -> {
- ResponseEntity
- .status(cause.errorCode.status)
- .body(CommonResponse.error(cause.errorCode, ex.message ?: cause.errorCode.message))
+ val message = cause.message ?: cause.errorCode.message
+ ResponseEntity
+ .status(cause.errorCode.status)
+ .body(CommonResponse.error(cause.errorCode, message))
}
else -> {
val errorCode = ErrorCode.JSON_PARSE_ERROR
ResponseEntity
.status(errorCode.status)
- .body(CommonResponse.error(errorCode, ex.message ?: errorCode.message))
+ .body(CommonResponse.error(errorCode))
}
}
@@
fun handleAll(e: Exception): ResponseEntity<CommonResponse<Void?>> {
val errorCode = ErrorCode.INTERNAL_SERVER_ERROR
- val body = CommonResponse.error(errorCode, e.message ?: errorCode.message)
+ val body = CommonResponse.error(errorCode)
return ResponseEntity
.status(errorCode.status)
.body(body)
}🤖 Prompt for AI Agents
In
`@src/main/kotlin/leets/leenk/global/common/exception/GlobalExceptionHandler.kt`
around lines 57 - 81, The handlers handleMessageNotReadable and handleAll in
GlobalExceptionHandler currently expose exception messages to clients; change
responses to always use the public error message from ErrorCode (e.g.,
errorCode.message) and, when the cause is a BaseException, return cause.message
(or fallback to cause.errorCode.message) instead of ex.message; also record the
full exception details (stacktrace and original message) to a server-side logger
(e.g., logger.error) before building the ResponseEntity so diagnostics are
preserved while preventing internal info leakage in CommonResponse.error.
soo0711
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
수고하셨습니다! 대부분 마이그레이션이 잘 된 것 같아요 👍
diff가 거의 없어서 살짝 보기 어려운 점이 있네요 허허
저도 마이그레이션할 때는 수정으로 올려보겠습니당
| class StatusCheckControllerTest( | ||
| private val mockMvc: MockMvc, | ||
| @MockkBean private val jwtDecoder: JwtDecoder, | ||
| @MockkBean private val oauthProperty: OauthProperty, | ||
| @MockkBean private val customAuthenticationEntryPoint: CustomAuthenticationEntryPoint, | ||
| @MockkBean private val customAccessDeniedHandler: CustomAccessDeniedHandler, | ||
| ) : DescribeSpec({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
엔드포인트 헬스 체크 테스트는 단순 시나리오라 팀 가이드상 StringSpec으로도 충분할 것 같은데 DescribeSpec을 사용하신 특별한 이유가 있을까용???
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
다른 테스트들도 다 DescribeSpec을 사용하셨는데 이유가 궁금합니닷
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
제 파트너인 마이그레이션 에이전트가 그렇게 써버렸네요.. 간단한 테스트들은 스타일 바꿔두겠습니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
아 저희 컨벤션이 BehaviorSpec 이었군요.. 수정해두겠습니다
| exception.errorCode shouldBe ErrorCode.RESOURCE_LOCKED | ||
| exception.errorCode.code shouldBe 3003 | ||
| exception.errorCode.status shouldBe HttpStatus.CONFLICT | ||
| exception.message shouldBe "다른 사용자가 처리 중입니다. 잠시 후 다시 시도해주세요." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
메시지 검증 시 하드코딩하기보다 ErrorCode.RESOURCE_LOCKED.message를 사용하는 게 어떨까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
해당 테스트가 ErrorCode.RESOURES_LOCKED의 내용을 검증하는 거다 보니 검증의 대상(예외 코드)을 사용하지 않고 검증하는 목적입니다!
예외 코드를 검증해야하는데 예외코드를 사용할 수 없어서 문자열 자체가 들어갈 수 밖에 없었습니당.. 사실 메시지보다는 에러 코드가 더 중요한 부분이딘긴 해서 제외해도 될 것 같아요 어떤 방법이 나을까요??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
에러 코드, code, status 검증만으로도 충분한 것 같아서 메시지 검증은 제외하는게 좋을 것 같아요!
jj0526
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
수고하셨어요! 예외에 대해서도 테스트 처리를 잘해주셨네요
| @ExceptionHandler(Exception::class) | ||
| fun handleAll(e: Exception): ResponseEntity<CommonResponse<Void?>> { | ||
| val errorCode = ErrorCode.INTERNAL_SERVER_ERROR | ||
| val body = CommonResponse.error(errorCode, e.message ?: errorCode.message) | ||
|
|
||
| return ResponseEntity | ||
| .status(errorCode.status) | ||
| .body(body) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
서버 내부 오류로만 반환해주고 실제 오류 메세지는 log로 남기는게 좋을거같아요
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
예에에전에 논의로 링크 백엔드에서는 예외 로그는 남기지 않기로 해서 이전 코드에서도 로그가 없긴 했습니당.
사실상 프론트 측에서 다 확인이 가능하고, 예외 로그가 다 남아버리면 로그 확인이 어려워져서 그렇게 했던 것으로 기억해용
1winhyun
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
수고하셨습니다!! 코드래빗이 준 리뷰 하나 확인한번 부탁드려요!!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @.claude/commands/kotlin-mirgrate.md:
- Around line 1-42: Rename the command file whose current name string is
"kotlin-mirgrate.md" to the correct spelling "kotlin-migrate.md" and update any
internal references (e.g., command registration or README entries that mention
".claude/commands/kotlin-mirgrate.md" or the `/kotlin-migration` command) so
they point to the new filename; ensure the file header/description remains
unchanged and verify the command lookup uses "kotlin-migrate.md" to keep name
consistency with the `/kotlin-migration` command.
🧹 Nitpick comments (1)
.claude/commands/code-review.md (1)
9-13: 문장을 완성하여 명확성을 개선하세요.Line 12에서 "check current branch commit history with
git logand review"라는 문장이 불완전합니다. "review" 뒤에 무엇을 리뷰하는지 명시하면 더 명확해집니다.💡 제안 수정
1. Check staged changes with `git diff --staged` -2. If nothing staged, check current branch commit history with `git log` and review +2. If nothing staged, check current branch commit history with `git log` and review recent changes또는:
1. Check staged changes with `git diff --staged` -2. If nothing staged, check current branch commit history with `git log` and review +2. If nothing staged, review the current branch commit history using `git log`
| --- | ||
| description: "kotlin-migration-agent를 사용해 Java 파일을 코틀린으로 마이그레이션하는 명령어입니다." | ||
| --- | ||
|
|
||
| # Instructions | ||
|
|
||
| You MUST use the Task tool to invoke the kotlin-migration-agent immediately. | ||
|
|
||
| ## Input Processing | ||
|
|
||
| 1. If user provides a file path: | ||
| - Use Read tool to verify the file exists and is a Java file | ||
| - Pass the absolute file path to the agent | ||
|
|
||
| 2. If user provides a directory path: | ||
| - Use Glob to find all `.java` files in that directory | ||
| - Pass the directory path to the agent | ||
|
|
||
| 3. If no path provided: | ||
| - Ask user to specify the Java file or directory to migrate | ||
|
|
||
| ## Agent Invocation | ||
|
|
||
| Call the Task tool with: | ||
| - subagent_type: "kotlin-migration-agent" | ||
| - prompt: "Migrate [FILE_PATH or DIRECTORY_PATH] from Java to Kotlin following the Test-First methodology" | ||
| - description: "Migrate Java to Kotlin" | ||
|
|
||
| Example: | ||
| ```text | ||
| Task tool: | ||
| subagent_type: kotlin-migration-agent | ||
| prompt: Migrate src/main/java/leets/leenk/domain/feed/service/FeedGetService.java from Java to Kotlin following the Test-First methodology | ||
| description: Migrate Java to Kotlin | ||
| ``` | ||
|
|
||
| ## Important Notes | ||
|
|
||
| - NEVER perform migration yourself - ALWAYS delegate to kotlin-migration-agent | ||
| - Agent will handle: test writing, migration, refactoring, and ktlint verification | ||
| - Agent follows strict order: Test → Migrate → Refactor → Verify | ||
| - All agent output will be in Korean as per agent rules |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
파일명 오타 수정 필요
파일명이 kotlin-mirgrate.md로 되어 있는데, "migrate"의 철자가 틀렸습니다. kotlin-migrate.md로 변경해야 합니다. PR 설명에서 언급한 /kotlin-migration 명령어와의 일관성을 위해서도 정확한 철자 사용이 필요합니다.
📝 파일명 변경 제안
파일명을 다음과 같이 변경하세요:
- 현재:
.claude/commands/kotlin-mirgrate.md - 변경:
.claude/commands/kotlin-migrate.md
🤖 Prompt for AI Agents
In @.claude/commands/kotlin-mirgrate.md around lines 1 - 42, Rename the command
file whose current name string is "kotlin-mirgrate.md" to the correct spelling
"kotlin-migrate.md" and update any internal references (e.g., command
registration or README entries that mention
".claude/commands/kotlin-mirgrate.md" or the `/kotlin-migration` command) so
they point to the new filename; ensure the file header/description remains
unchanged and verify the command lookup uses "kotlin-migrate.md" to keep name
consistency with the `/kotlin-migration` command.
# Conflicts: # src/main/java/leets/leenk/global/common/exception/ErrorCode.java # src/main/java/leets/leenk/global/common/exception/ErrorCodeInterface.java # src/main/java/leets/leenk/global/common/exception/GlobalExceptionHandler.java # src/main/java/leets/leenk/global/common/exception/ResourceLockedException.java # src/main/kotlin/leets/leenk/global/common/exception/ApiErrorCodeExample.java # src/main/kotlin/leets/leenk/global/common/exception/CommonErrorCode.java # src/main/kotlin/leets/leenk/global/common/exception/ErrorCode.kt # src/main/kotlin/leets/leenk/global/common/exception/ExampleHolder.java # src/main/kotlin/leets/leenk/global/common/exception/ExplainError.java
작업 내용 💻
스크린샷 📷
같이 얘기해보고 싶은 내용이 있다면 작성 📢
Summary by CodeRabbit
릴리스 노트
버그 수정
개선사항
테스트
✏️ Tip: You can customize this high-level summary in your review settings.