[♻️ refactor/#101] 소셜 로그인 > 회원가입 로직 개선 #102
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,19 @@ | ||
| package org.terning.terningserver.dto.auth.response; | ||
|
|
||
| import lombok.Builder; | ||
| import lombok.NonNull; | ||
| import org.terning.terningserver.domain.Token; | ||
|
|
||
| import static lombok.AccessLevel.*; | ||
|
|
||
| @Builder(access = PRIVATE) | ||
| public record AccessTokenGetResponseDto( | ||
| @NonNull String accessToken | ||
| ) { | ||
|
|
||
| public static AccessTokenGetResponseDto of(Token accessToken) { | ||
| return AccessTokenGetResponseDto.builder() | ||
| .accessToken(accessToken.getAccessToken()) | ||
| .build(); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,19 +1,20 @@ | ||
| package org.terning.terningserver.service; | ||
|
|
||
| import org.terning.terningserver.domain.enums.AuthType; | ||
| import org.terning.terningserver.dto.auth.request.SignInRequestDto; | ||
| import org.terning.terningserver.dto.auth.response.AccessTokenGetResponseDto; | ||
| import org.terning.terningserver.dto.auth.response.SignInResponseDto; | ||
| import org.terning.terningserver.dto.auth.response.SignUpResponseDto; | ||
|
|
||
| public interface AuthService { | ||
|
|
||
| SignInResponseDto signIn(String authAccessToken, SignInRequestDto request); | ||
|
|
||
| SignUpResponseDto signUp(String authId, String name, Integer profileImage, AuthType authType); | ||
|
|
||
| void signOut(long userId); | ||
|
|
||
| void withdraw(long userId); | ||
|
|
||
| AccessTokenGetResponseDto reissueToken(String refreshToken); | ||
|
Comment on lines
+13
to
+19
There was a problem hiding this comment. SignUpService의 기능을 AuthService로 통합한 것은 There was a problem hiding this comment. 인정입니다. 추후 복잡해져서 관리가 어렵다고 느껴지는 순간이 온다면 그때 구조에 대해 다시 생각해 봐야겠어요! |
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,9 +10,10 @@ | |
| import org.terning.terningserver.domain.Token; | ||
| import org.terning.terningserver.domain.User; | ||
| import org.terning.terningserver.dto.auth.request.SignInRequestDto; | ||
| import org.terning.terningserver.dto.auth.response.AccessTokenGetResponseDto; | ||
| import org.terning.terningserver.dto.auth.response.SignInResponseDto; | ||
| import org.terning.terningserver.domain.enums.AuthType; | ||
| import org.terning.terningserver.dto.auth.response.SignUpResponseDto; | ||
| import org.terning.terningserver.exception.CustomException; | ||
| import org.terning.terningserver.jwt.JwtTokenProvider; | ||
| import org.terning.terningserver.jwt.UserAuthentication; | ||
|
|
@@ -57,11 +58,19 @@ public SignInResponseDto signIn(String authAccessToken, SignInRequestDto request | |
| } | ||
|
|
||
| @Transactional | ||
|
Comment on lines
-61
to
-64
There was a problem hiding this comment. User 클래스를 보니까 클래스에 @builder를 붙여서 사용하고 계시더라구요! @builder를 엔티티 클래스에서 사용하기 위해서는 @NoArgsConstructor, @AllArgsConstructor 3개가 꼭 함께 사용되어야 하는데, 엔티티에 @NoArgsConstructor를 붙이는 건 지양하는게 좋다고 해요! (자세한 건 링크 첨부해두겠습니다!) 그래서 엔티티에 @builder를 붙이는 대신, 생성자에 @builder를 붙이고 객체 생성시에 static 메서드를 통해 객체를 생성할 수 있도록 하는 건 어떨까요?! 장순님 의견이 궁금합니다! 예시) @Builder
private Conversation(Simulation simulation, Patient patient, String content, LocalDateTime send_time) {
this.simulation = simulation;
this.patient = patient;
this.content = content;
this.send_time = send_time;
}
public static Conversation create(Simulation simulation, Patient patient, String content, LocalDateTime send_time) {
return Conversation.builder()
.simulation(simulation)
.patient(patient)
.content(content)
.send_time(send_time)
.build();
} |
||
| public SignUpResponseDto signUp(String authId, String name, Integer profileImage, AuthType authType) { | ||
|
There was a problem hiding this comment. 현재 signUp 메서드의 파라미터들이 모두 SignUpRequestDto에 포함되어 있으므로, 파라미터를 Dto로 통합하는 것이 메서드를 단순화 할 수 있어 더 좋은 방향이라는 생각이 들었습니다! @Transactional
public SignUpResponseDto signUp(SignUpRequestDto request) {
User user = userRepository.save(User.builder()
.authId(request.authId())
.name(request.name())
.authType(request.authType())
.profileImage(request.profileImage())
.build());There was a problem hiding this comment. 인정입니다! 기존의 |
||
|
|
||
| User user = userRepository.save(User.builder() | ||
| .authId(authId) | ||
| .name(name) | ||
| .authType(authType) | ||
| .profileImage(profileImage) | ||
| .build()); | ||
|
|
||
| Token token = getToken(user); | ||
| userRepository.save(user); | ||
|
Comment on lines
+63
to
+71
There was a problem hiding this comment. signUp 메서드에서 user가 2번 저장되고 있어 두번째 저장 로직은 제거하는 것이 효율적일 것 같다는 생각이 드네요..! There was a problem hiding this comment.
따라서, 만약에 signUp 메서드를 분리를 한다면 signUp의 로직은 온보딩 정보가 저장되는 순간 토큰을 발급하는 것이 한 번에 일어나야 하므로 하나의 트랜잭션으로 묶는 것이 맞지 않을까 싶은데 어떻게 생각하시나요!? There was a problem hiding this comment. getToken() 메서드 호출시에 하나의 트랜잭션 안에서 더티체킹을 통해서 rtk를 업데이트 해주고 있고, 만약에 getToken()에서 exception발생하여 롤백이 된다고 한다면 rtk가 변경되지 않은 상태에서 다시 유저를 저장하는게 무의미하지 않은가라는 생각이 드는데 어떻게 생각하시나요?!🤔 |
||
|
|
||
| return SignUpResponseDto.of(token.getAccessToken(), token.getRefreshToken(), user.getId(), authType); | ||
| } | ||
|
|
||
| @Override | ||
|
|
@@ -79,11 +88,10 @@ public void withdraw(long userId) { | |
| } | ||
|
|
||
| @Override | ||
| public AccessTokenGetResponseDto reissueToken(String refreshToken) { | ||
| val user = findUser(refreshToken); | ||
| Token accessToken = getAccessToken(user); | ||
| return AccessTokenGetResponseDto.of(accessToken); | ||
| } | ||
|
|
||
| private String getAuthId(AuthType authType, String authAccessToken) { | ||
|
|
@@ -99,13 +107,24 @@ public Token getToken(User user) { | |
| return token; | ||
| } | ||
|
|
||
| public Token getAccessToken(User user) { | ||
| val accessToken = generateAccessToken(new UserAuthentication(user.getId(), null, null)); | ||
| return accessToken; | ||
| } | ||
|
|
||
| private Token generateToken(Authentication authentication) { | ||
| return Token.builder() | ||
| .accessToken(jwtTokenProvider.generateToken(authentication, valueConfig.getAccessTokenExpired())) | ||
| .refreshToken(jwtTokenProvider.generateToken(authentication, valueConfig.getRefreshTokenExpired())) | ||
| .build(); | ||
| } | ||
|
|
||
| private Token generateAccessToken(Authentication authentication) { | ||
| return Token.builder() | ||
| .accessToken(jwtTokenProvider.generateToken(authentication, valueConfig.getAccessTokenExpired())) | ||
| .build(); | ||
| } | ||
|
|
||
|
Comment on lines
115
to
+127
There was a problem hiding this comment. accessToken을 생성하는 로직이 동일하기 때문에, 안쓰는 로직을 삭제하거나 하나의 메서드로 추출하는 방법도 고려해보면 좋을 것 같습니다! private String generateAccessTokenString(Authentication authentication) {
return jwtTokenProvider.generateToken(authentication, valueConfig.getAccessTokenExpired());
}
private Token generateToken(Authentication authentication) {
return Token.builder()
.accessToken(generateAccessTokenString(authentication))
.refreshToken(jwtTokenProvider.generateToken(authentication, valueConfig.getRefreshTokenExpired()))
.build();
}
private Token generateAccessToken(Authentication authentication) {
return Token.builder()
.accessToken(generateAccessTokenString(authentication))
.build();
}There was a problem hiding this comment. 좋네요! 기존 방법을 그대로 간다면 생각나는 로직은 일단 다음과 같은데, 다음 리팩토링에서 적용해야겠네요! public Token getToken(User user) {
String accessToken = createAccessToken(new UserAuthentication(user.getId(), null, null));
String refreshToken = createRefreshToken(new UserAuthentication(user.getId(), null, null));
user.updateRefreshToken(refreshToken);
return Token.builder()
.accessToken(accessToken)
.refreshToken(refreshToken)
.build();
}
public Token getAccessToken(User user) {
String accessToken = createAccessToken(new UserAuthentication(user.getId(), null, null));
return Token.builder()
.accessToken(accessToken)
.build();
}
private String createAccessToken(Authentication authentication) {
return jwtTokenProvider.generateToken(authentication, valueConfig.getAccessTokenExpired());
}
private String createRefreshToken(Authentication authentication) {
return jwtTokenProvider.generateToken(authentication, valueConfig.getRefreshTokenExpired());
} |
||
| private User findUser(long id) { | ||
| return userRepository.findById(id).orElseThrow(() -> new CustomException(INVALID_USER)); | ||
| } | ||
|
|
||
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.
Token 앞에서 final 붙여서 불변성 보장해주어도 좋을 것 같습니다!