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

[REFACTOR/#174] MemberService 로직 리팩 및 회원가입 테스트 코드 작성 #176

Merged
merged 5 commits into from
Feb 11, 2025

Conversation

JungYoonShin
Copy link
Member

@JungYoonShin JungYoonShin commented Jan 30, 2025

📄 Work Description

  • MemberService 로직 리팩 및 회원가입 테스트 코드를 작성했습니다.

⚙️ ISSUE

📷 Screenshot

스크린샷 2025-01-30 오후 5 58 36

💬 To Reviewers

✅ 1. MemberService 코드 리팩 진행했습니다.

  • 다시 코드를 보니까.. 중복된 로직이 여러 메서드에서 사용되고 있더라구요..! 그래서 최대한 중복되는 로직은 메서드로 따로 빼서 사용할 수 있도록 했습니다!
  • 회의때 facade 패턴 도입할 지 고민했었는데, 우선은 그대로 두었습니다! 통합테스트를 짜게 되는 바람에 해당 고민은 테스트코드 좀 더 짜보면서 필요성이 커지면 도입하는 걸로 해볼게요!!

✅ 2. 동일한 이메일로 사용자가 동시에 가입할 경우, 하나의 계정만 생성되는지에 대한 테스트코드를 작성했습니다.

AS-IS

  • 기존에는 사용자가 회원가입을 할 경우, 해당 이메일로 가입한 유저가 이미 있는지에 대한 여부를 비즈니스 로직에서 처리하고 있었습니다.
  • 비즈니스로직에서 처리하는 것보다 DB단에게 역할을 위임하는게 조금 더 안전하고 확실할 것이라 판단했습니다. (비즈니스 레이어에서 처리시, 동시성 이슈가 발생할 수 있으며 DB단에서 Exception이 발생했을 때 예외처리를 해주면 될 것이라고 생각하였습니다.)

TO-BE


추가적으로 알게 된 사실

  • unique 제약 조건을 추가하면, mysql 기준으로 Unique index을 자동으로 설정해준다고 해요! 그래서 select 쿼리가 나가면 full-scan대신 Unique Index를 통해 조회하게 되어 성능이 유의미하게 향상된다고 합니다!! 요건 성능테스트로 확실히 테스트 해봐도 좋을 것 같아욥 ㅎㅎ. -> 관련해서는 블로그 링크 첨부해둘게여ㅕ

🔗 Reference

https://kth990303.tistory.com/451

- 회원가입시 동일한 이메일로 사용자가 동시에 가입할 경우에 대한 테스트 진행했습니다.
Copy link
Contributor

@ppparkta ppparkta left a comment

Choose a reason for hiding this comment

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

동시성 문제에 대해서 검증한다는건 아예 생각도 못하고 있었는데 정윤님 코드 보고 다양한 상황에서의 문제점을 고민해야겠다고 느꼈습니다. 멀티쓰레드로 테스트 작성하신 것도 좋구요!

이번에 레포지토리 레이어의 통합테스트를 잘 작성해주셨는데 서비스 레이어를 테스트 할 때는 회의 때 얘기해주신 퍼싸드 패턴이 도입되면 좋을 것 같아요. 필요하지 않은 시점에 무리하게 코드를 수정하지 않으신 것도 좋네요.

고생 많으셨어요👍

@@ -23,7 +23,7 @@
@Entity
@Getter
@Builder
@NoArgsConstructor(access = PROTECTED)
@NoArgsConstructor
Copy link
Contributor

Choose a reason for hiding this comment

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

이부분 protected 접근제어는 왜 제거된건가요?

Copy link
Member Author

Choose a reason for hiding this comment

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

엇 이거 다시 추가해놀을게요!!

Comment on lines 93 to 98
// memberRepository.findByEmail(memberSignUpReqDto.getEmail())
// .ifPresent(
// existingMember -> {
// throw new RestApiException(USER_ALREADY_EXISTS);
// }
// );
Copy link
Contributor

Choose a reason for hiding this comment

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

사용하지 않는 코드라면 지워주세요!!

import static org.assertj.core.api.Assertions.assertThat;
import static org.junit.jupiter.api.Assertions.*;

@DataJpaTest
Copy link
Contributor

Choose a reason for hiding this comment

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

해당 테스트를 DataJpaTest로 실행하신 이유가 있나요?
SpringBootTest와의 차이점이 내부 트랜잭션 유무라고 알고있는데, 멀티쓰레드 코드가 의도대로 동작하는지 궁금해서요..!!

Copy link
Member Author

Choose a reason for hiding this comment

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

헙 해당 테스트가 데이터 엑세스 레이어(repository)에 대한 테스트여서, SpringBootTest에 비해 가벼운 DataJpaTest를 사용했는데
트랜잭션 관련 부분은 미쳐 생각을 못했네요...! 해당 부분 좀 더 테스트 해보고 제가 오늘 회의때 공유하겠습니다!!

완젼 놓친 부분 찦어주셔서 감사해요오😊

@JungYoonShin JungYoonShin merged commit c4f7357 into develop Feb 11, 2025
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

로그인/회원가입 로직 리팩
2 participants