-
Notifications
You must be signed in to change notification settings - Fork 0
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
♻️ change user api #110
♻️ change user api #110
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.
많은 변경 사항이 없어 리뷰하기에 정말 좋았어요~
로그인 마스터 로키 파이팅입니다~
@@ -31,7 +31,7 @@ public GoogleLoginResponse loginWithGoogle(GoogleLoginRequest googleLoginRequest | |||
if (!userRepository.existsByEmail(email)) { | |||
return new GoogleLoginResponse(false, null); | |||
} | |||
User user = userRepository.findByEmail(email); | |||
User user = userRepository.findByEmail(email).orElseThrow(); |
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.
Optional
사용은 탁월한 선택이에요!
위쪽에서 existsByEmail
을 함으로써 아래쪽의 Optional`은 Not Null 이라는게 자명하긴 합니다.
하지만 굳이 비슷한 쿼리가 2번 발생할 필요가 있을지 의문이에요!
최초의 로그인에만 필요한 부분이니 큰 리소스 낭비가 없다고 판단 되면 굳이 합칠 필요는 없어 보여요.
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.
좋은 지적입니다!
public GoogleLoginResponse loginWithGoogle(GoogleLoginRequest googleLoginRequest) {
FirebaseToken decodedToken = decodeIdToken(googleLoginRequest.idToken());
String email = decodedToken.getEmail();
User user = userRepository.findByEmail(email).orElse(null);
if (user == null) {
return new GoogleLoginResponse(false, null);
}
String accessToken = jwtTokenManager.createToken(new TokenPayload(user.getId(), user.getEmail()));
return new GoogleLoginResponse(true, accessToken);
이 방법을 고민하고 있었어요.
말씀해주신대로 쿼리가 한번만 나가게 수정후 배포하겠습니다!
@@ -42,7 +48,8 @@ void getUserById() { | |||
|
|||
UserResponse actual = RestAssured.given().log().all() | |||
.contentType(ContentType.JSON) | |||
.when().get("/api/user/" + id) | |||
.header("Authorization", "Bearer " + accessToken) | |||
.when().get("/api/user/me") |
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.
👍👍
|
잘못 설계했던 api를 변경했습니다.
기존 :
api/user/{{id}}
변경 :
api/user/me