[♻️ refactor/#101] 소셜 로그인 > 회원가입 로직 개선

[jsoonworld]

📄 Work Description

• SignUpService 클래스를 제거하고 해당 로직을 AuthService에 통합하였습니다.
• 기존에 분리되어 있던 회원가입 로직을 하나의 서비스로 일원화하여, 코드의 중복을 줄이고 서비스 구조를 단순화했습니다.
• AuthController에서 SignUpService를 호출하던 부분을 AuthService 호출로 수정하였습니다.
• 코드 내에서 불필요한 서비스 의존성을 제거하고, 코드의 가독성과 유지보수성을 향상시켰습니다.
• SignUpService의 삭제와 함께 불필요한 import 구문들도 정리하였습니다.

⚙️ ISSUE

• closed [♻️ refactor] 소셜 로그인 > 회원가입 로직 개선 #101

📷 Screenshot

Swagger 200

• 소셜 로그인 > 회원가입
  

💬 To Reviewers

• 기존의 결을 유지하기 위해서 AuthService에 통합을 하였는데 궁극적으로 분리하는 게 나을까요? auth 관련은 이렇게 통합해서 관리하는 게 좋을까요?
• 리뷰어분들께서 구조적인 변경으로 인해 놓칠 수 있는 부분이 있을지 검토해 주시면 감사하겠습니다.

[junggyo1020]

전체적으로 코드 내 서비스 의존성이나 가독성을 고려한 부분들이 보이네요!! 정말 고생많으셨습니다 :)

[junggyo1020]

signUp 메서드에서 user가 2번 저장되고 있어 두번째 저장 로직은 제거하는 것이 효율적일 것 같다는 생각이 드네요..!

[jsoonworld]

signUp 메서드에서 userRepository.save()가 두 번 호출되는 이유는 다음과 같습니다:

1. 첫 번째 save 호출: User 객체를 데이터베이스에 저장하여 userId를 생성합니다. 이 userId는 이후 토큰 생성에 필요하기 때문에, 반드시 먼저 저장이 이루어져야 합니다.

2. getToken(user) 호출: 생성된 userId를 사용해 토큰을 생성합니다. 이 과정에서 user 객체의 리프레시 토큰이 업데이트됩니다. 이 업데이트된 리프레시 토큰은 데이터베이스에 반영되어야 합니다.

3. 두 번째 save 호출: getToken(user) 호출 후 user 객체의 상태가 변경되었으므로, 변경된 상태를 다시 데이터베이스에 반영하기 위해 두 번째 save 호출이 필요합니다.

따라서, signUp 메서드에서 save를 두 번 호출하는 것은 필수적인 동작입니다. 첫 번째 save는 userId 생성에 필요하고, 두 번째 save는 리프레시 토큰의 업데이트된 상태를 반영하기 위함입니다.

만약에 signUp 메서드를 분리를 한다면 saveInitialUser() , updateUserWithToken()으로 나눠서 진행할 수 있을 것 같습니다.
제 생각에 지금 상태의 로직은 분리할 정도의 복잡도를 갖고 있지 않다고 생각이 드는데, 하나의 트랜잭션 안에서 save 가 2번 호출되는 것이 추후 테스트를 진행할 때 어려움을 겪을 것 같다면 분리하는 것도 좋다는 생각이 드네요!

signUp의 로직은 온보딩 정보가 저장되는 순간 토큰을 발급하는 것이 한 번에 일어나야 하므로 하나의 트랜잭션으로 묶는 것이 맞지 않을까 싶은데 어떻게 생각하시나요!?

[JungYoonShin]

getToken() 메서드 호출시에 하나의 트랜잭션 안에서 더티체킹을 통해서 rtk를 업데이트 해주고 있고, 만약에 getToken()에서 exception발생하여 롤백이 된다고 한다면 rtk가 변경되지 않은 상태에서 다시 유저를 저장하는게 무의미하지 않은가라는 생각이 드는데 어떻게 생각하시나요?!🤔

[junggyo1020]

SignUpService의 기능을 AuthService로 통합한 것은 코드 가독성과 유지보수성 측면에서 장점을 가지고 있다고 생각합니다.
다만, 서비스의 책임이 증가하면서 클래스의 역할이 복잡해질 가능성이 있어. 추후에 서비스의 확장을 가져가야 한다면, 다시 기능을 분리하는 것을 고려해봐도 좋을 것 같습니다!
추가적으로 나중에 토큰 관련 로직만 따로 분리하는 방법도 좋을 것 같다는 생각이 드네요.

[jsoonworld]

인정입니다. 추후 복잡해져서 관리가 어렵다고 느껴지는 순간이 온다면 그때 구조에 대해 다시 생각해 봐야겠어요!

[junggyo1020]

현재 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());

[jsoonworld]

인정입니다! 기존의 SignUpResponseDto와 SignUpRequestDto와 네이밍이 헷갈리지 않도록 DTO를 새로 만들어서 파라미터를 더 깔끔하게 정리해봐야겠네요. 덕분에 방향이 더 명확해졌습니다. 감사합니다!

[junggyo1020]

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();
}

[jsoonworld]

좋네요! 기존 방법을 그대로 간다면 access, refresh를 한 번에 생성했기에 한 곳에서 생성이 안 됐을 때 전체가 문제가 되고, 문제를 확인하기 어려울 것 같은데 생성 로직을 분리하면 중복, 테스트, 확장에도 용이할 것 같다는 생각이 들었습니다. 감사합니다.

생각나는 로직은 일단 다음과 같은데, 다음 리팩토링에서 적용해야겠네요!

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());
}

[JungYoonShin]

수고하셨습니다! 몇 가지 궁금한 점과 코멘트 남깁니다!!

[JungYoonShin]

Token 앞에서 final 붙여서 불변성 보장해주어도 좋을 것 같습니다!

[JungYoonShin]

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(); 
 }