Skip to content
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

Merged
merged 5 commits into from
Jul 28, 2024
Merged

♻️ change user api #110

merged 5 commits into from
Jul 28, 2024

Conversation

HaiSeong
Copy link

@HaiSeong HaiSeong commented Jul 26, 2024

잘못 설계했던 api를 변경했습니다.

기존 : api/user/{{id}}
변경 : api/user/me

@HaiSeong HaiSeong requested review from geoje and tackyu July 26, 2024 02:32
@HaiSeong HaiSeong self-assigned this Jul 26, 2024
Copy link
Contributor

Overall Project 91.66% 🍏
Files changed 100% 🍏

File Coverage
LoginUserArgumentResolverConfig.java 100% 🍏
UserController.java 100% 🍏
LoginService.java 95.2% 🍏

Copy link
Contributor

Overall Project 91.66% 🍏
Files changed 100% 🍏

File Coverage
LoginUserArgumentResolverConfig.java 100% 🍏
UserController.java 100% 🍏
LoginService.java 95.2% 🍏

Copy link
Contributor

@geoje geoje left a 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();
Copy link
Contributor

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번 발생할 필요가 있을지 의문이에요!
최초의 로그인에만 필요한 부분이니 큰 리소스 낭비가 없다고 판단 되면 굳이 합칠 필요는 없어 보여요.

Copy link
Author

@HaiSeong HaiSeong Jul 28, 2024

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")
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

@tackyu tackyu left a comment

Choose a reason for hiding this comment

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

👍👍

Copy link
Contributor

Overall Project 91.65% 🍏
Files changed 100% 🍏

File Coverage
LoginUserArgumentResolverConfig.java 100% 🍏
UserController.java 100% 🍏
LoginService.java 95.12% 🍏

@HaiSeong HaiSeong merged commit 05b8b5c into be/dev Jul 28, 2024
1 check passed
@HaiSeong HaiSeong deleted the be/feat/94 branch July 28, 2024 12:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BE Backend
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants