-
Notifications
You must be signed in to change notification settings - Fork 0
[✨feat] Auth 구현 #40
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
[✨feat] Auth 구현 #40
Conversation
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.
전체적으로 Auth 도메인 구조를 단정하게 잘 잡아주셔서 인상 깊었어요!
필드 하나하나에 대해서도 필요한 길이 제한을 명확히 주신 점이나, 예외 설계까지 세심하게 신경 써주신 게 느껴졌습니다 👏
작은 제안들이 몇 가지 있었지만, 그만큼 잘 정리된 코드여서 보면서 저도 많이 배워갔어요!
무엇보다 도메인 관점에서 Auth를 어떻게 바라보셨는지가 잘 드러난 구현이라 좋았습니다.
구현하시느라 고생 많으셨고, 공유해주셔서 감사합니다! 🙌
@Column(length = 12) | ||
private var authType: AuthType, | ||
@Column(length = 255) | ||
private var refreshToken: String?, |
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.
한 가지 제안 겸 궁금한 점이 있어요!
현재 refreshToken
이 nullable한 String
으로 관리되고 있는데요, 혹시 이 부분을 VO로 분리해보는 건 어떻게 생각하시나요?
예를 들어 RefreshToken
이라는 VO로 감싸게 되면
- null을 명시적으로 표현하는 대신 더 의도적인 API 설계가 가능하고,
- 향후 유효성 검증이나 포맷 검사를 VO 내부로 위임할 수 있다는 점에서 유연성이 생길 수 있을 것 같았어요.
물론 지금처럼 간단한 구조에선 지나친 추상화일 수도 있긴 하지만,
조금씩 도메인 로직이 붙기 시작하면 VO 분리가 오히려 테스트나 유지보수에도 도움이 되지 않을까 싶더라구요 🤔
혹시 이 부분에 대해서는 어떻게 생각하시나요? 저는 개인적으로 도메인에서 토큰을 문자열로 직접 다루는 게 약간 아쉬울 수 있겠다 생각이 들어서 한번 여쭤보고 싶었어요 😊
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.
좋은 것 같아요!! VO의 특징을 잘 살리는 방향인 것 같네요!
해당 부분은 수정해놓겠습니다~!
@Column(length = 255) | ||
private var authId: String, |
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.
authId
는 도메인에서 꽤 중요한 값처럼 보여서, 혹시 VO로 분리해보는 건 어떻게 생각하시나요?
예를 들어 사용자 인증과 관련된 고유 식별자라면,
VO로 감싸서 포맷 검증이나 유효성 체크 같은 책임을 명확히 나눌 수 있을 것 같았어요.
값 자체에 의미가 담긴 경우라면 특히 더 유용할 수 있을 것 같아서요!
혹시 현재는 간단하게 처리하는 쪽으로 결정하신 이유가 있을까요? 🤔
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.
아직 authId에 대한 별도의 로직이 없어서 VO로 감싸지 않고 있었어요..!
말씀해 주신 것처럼 향후 중요한 로직이 추가될 것으로 보여 VO로 분리하도록 하겠습니다!
@Column(length = 12) | ||
private var authType: AuthType, |
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.
authType
은 현재 enum으로 잘 정의되어 있는데요,
JPA에서는 enum을 컬럼에 매핑할 때 @Enumerated(EnumType.STRING)
을 함께 명시해주면
숫자 ordinal이 아닌 이름 그대로 저장돼서 안정성이 더 높아질 수 있어요!
혹시 아직 붙이지 않으신 이유가 있으실까요?
의도적으로 처리하신 거라면 어떤 판단이 있으셨는지도 궁금했어요 🙂
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.
해당 부분은 제가 놓친 것 같네요!! 추가했습니다~!
덕분에 @Enumerated(EnumType.STRING)
에 대해서 한 번 더 알아가요! 감사합니다~👍
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.
Pull Request Overview
This PR implements the core authentication domain by adding embeddable ID/token types, an Auth
entity with update/reset methods, and a new error code for refresh token reset failures.
- Added
AuthId
andRefreshToken
embeddables - Created
Auth
entity withupdateRefreshToken
andresetRefreshToken
logic - Introduced
FAILED_REFRESH_TOKEN_RESET
inAuthErrorCode
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
File | Description |
---|---|
src/main/kotlin/com/terning/server/kotlin/domain/auth/RefreshToken.kt | Add RefreshToken embeddable with nullable value |
src/main/kotlin/com/terning/server/kotlin/domain/auth/AuthId.kt | Add AuthId embeddable |
src/main/kotlin/com/terning/server/kotlin/domain/auth/AuthErrorCode.kt | Add FAILED_REFRESH_TOKEN_RESET enum constant |
src/main/kotlin/com/terning/server/kotlin/domain/auth/Auth.kt | Define Auth entity with token update/reset methods |
Comments suppressed due to low confidence (1)
src/main/kotlin/com/terning/server/kotlin/domain/auth/Auth.kt:42
- The new
resetRefreshToken
logic isn't covered by tests; consider adding unit tests to verify both successful resets and failure paths.
fun resetRefreshToken() {
@@ -7,6 +7,7 @@ enum class AuthErrorCode( | |||
val message: String, | |||
) { | |||
INVALID_TOKEN(status = HttpStatus.UNAUTHORIZED, message = "유효하지 않은 토큰입니다."), | |||
FAILED_REFRESH_TOKEN_RESET(status = HttpStatus.BAD_REQUEST, 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.
The message has a typo: replace "리프레쉬" with the correct spelling "리프레시".
FAILED_REFRESH_TOKEN_RESET(status = HttpStatus.BAD_REQUEST, message = "리프레쉬 토큰 초기화에 실패하였습니다"), | |
FAILED_REFRESH_TOKEN_RESET(status = HttpStatus.BAD_REQUEST, message = "리프레시 토큰 초기화에 실패하였습니다"), |
Copilot uses AI. Check for mistakes.
try { | ||
this.refreshToken = RefreshToken(null) | ||
} catch (e: Exception) { | ||
throw AuthException(AuthErrorCode.FAILED_REFRESH_TOKEN_RESET) | ||
} |
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.
Catching a generic Exception
around a simple property assignment is unnecessary; consider removing the try-catch
or catching only specific exceptions where JPA operations may fail.
try { | |
this.refreshToken = RefreshToken(null) | |
} catch (e: Exception) { | |
throw AuthException(AuthErrorCode.FAILED_REFRESH_TOKEN_RESET) | |
} | |
this.refreshToken = RefreshToken(null) |
Copilot uses AI. Check for mistakes.
📄 Work Description
💭 Thoughts
refreshToken
의 경우 Nullalbe하기 때문에 타입을String?
으로 나타내주었습니다!✅ Testing Result
🗂 Related Issue