-
Notifications
You must be signed in to change notification settings - Fork 1
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/#409] 멤버 가입시 이메일과 컬럼 수정/생성 트랜잭션 분리 #410
Conversation
val token = if (Objects.isNull(isSignUpBeforeMember)) { | ||
headComment = SIGNUP_HEAD_COMMENT | ||
subComment = SIGNUP_SUB_COMMENT | ||
email = useCaseIn.email | ||
memberDao.insertMember( | ||
InsertMemberCommand( | ||
email = useCaseIn.email, | ||
memberType = MemberType.PREAUTH | ||
/** 가입 이력 여부를 기준으로 가입 처리 */ | ||
saveMemberTxCase.execute( | ||
SaveMemberTxCaseIn( | ||
record = it, | ||
email = useCaseIn.email | ||
) | ||
) ?: throw InsertException("member.insertfail.record") | ||
} else { | ||
/** 삭제한 회원이라면 회원 타입을 PREAUTH로 변경 */ | ||
if (isSignUpBeforeMember!!.isDeleted) { | ||
val isUpdate = memberDao.updateMemberType( | ||
UpdateDeletedMemberTypeCommand( | ||
id = isSignUpBeforeMember.memberId, | ||
memberType = MemberType.PREAUTH | ||
) | ||
) | ||
if (isUpdate != 1L) { | ||
throw InsertException("member.updatefail.record") | ||
} | ||
|
||
memberDao.selectMemberByEmail( | ||
SelectMemberByEmailNotConsiderDeletedAtQuery( | ||
email = useCaseIn.email | ||
) | ||
)?.memberId ?: throw InsertException("member.selectfail.record") | ||
} else { | ||
/** 이미 가입한 회원이라면 회원 ID를 반환 */ | ||
isSignUpBeforeMember.memberId | ||
} | ||
}.let { | ||
/** 회원 ID를 암호화하여 토큰으로 사용 */ | ||
idEncryption.encrypt(it.toString()) | ||
) | ||
} |
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.
이메일을 보내는 것과 디비 상에 이메일을 저장하는 것을 분리했다는 말씀이신가요?
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.
[이전]
트랜잭션 시작 -> 멤버 수정/생성 -> 이메일 전송 -> 트랜잭션 종료
[수정]
트랜잭션 시작 -> 멤버 수정/생성 -> 트랜잭션 종료 -> 이메일 전송
이렇게 수정했어요.!
JPA라면 쓰지기연 때문에 수정/생성이 최대한 늦게 한번에 일어나는데 jOOQ는 쓰기 지연이 없는 것으로 알고 있어서 바로 바로 쿼리가 나갈꺼에요.
그렇게 되면 커밋하기 전까지 수정/생성 할때 락을 잡고 있다 생각해서 분리 하는게 맞지 않을까? 생각했어요
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.
아니면 먼저 메일을 보내놓고 콜백왔을 때 디비에서 처리
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.
아니면 먼저 메일을 보내놓고 콜백왔을 때 디비에서 처리
요것은 현재 이메일에 들어가는 내용중에 멤버의 ID 값이 있어서 불가능할 것 같아요!
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.
그럼 가입대기 멤버에서 다음 상태로 변경되는 시점은 언젠가요?
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.
import java.util.* | ||
|
||
@Component | ||
class SaveMemberTxCase( |
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.
우선 TxCase라고 네이밍하였는데 어떠신가요?
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.
얘 자체는 그냥 멤버 저장하는 행위인건데 Tx라는 용어를 굳이 넣어야 하나 싶습니다
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.
우리가 Service를 다른 도메인을 사용할 때 네이밍으로 쓰고 있어서 다른 적절한 네이밍이 떠오르지 않았어요.!
그래서 우선 트랜잭션을 분리한 것이니 UseCase와 유사한 느낌으로 TxCase로 해본거였슴다.!
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.
이번 PR말고도 다른 구현을 하며 트랜잭션을 분리해야하는 경우가 필요할 수도 있을 것 같은데 어떤 네이밍이 좋을까요??
return dto.record?.let { signUpBeforeMember -> | ||
signUpBeforeMember.takeIf { it.isDeleted }?.let { | ||
ifDeletedMember(it.memberId) | ||
} ?: ifMember(signUpBeforeMember) | ||
} ?: ifNewMember(dto.email) |
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.
takeIf 처음 보네요 재밌는거 배워갑니다~
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.
이메일 보내는 것과 디비에 저장하는걸 분리하셨다는거 같은데 제대로 이해를 못해서 간단하게 다시 설명 가능할까요
val token = if (Objects.isNull(isSignUpBeforeMember)) { | ||
headComment = SIGNUP_HEAD_COMMENT | ||
subComment = SIGNUP_SUB_COMMENT | ||
email = useCaseIn.email | ||
memberDao.insertMember( | ||
InsertMemberCommand( | ||
email = useCaseIn.email, | ||
memberType = MemberType.PREAUTH | ||
/** 가입 이력 여부를 기준으로 가입 처리 */ | ||
saveMemberTxCase.execute( | ||
SaveMemberTxCaseIn( | ||
record = it, | ||
email = useCaseIn.email | ||
) | ||
) ?: throw InsertException("member.insertfail.record") | ||
} else { | ||
/** 삭제한 회원이라면 회원 타입을 PREAUTH로 변경 */ | ||
if (isSignUpBeforeMember!!.isDeleted) { | ||
val isUpdate = memberDao.updateMemberType( | ||
UpdateDeletedMemberTypeCommand( | ||
id = isSignUpBeforeMember.memberId, | ||
memberType = MemberType.PREAUTH | ||
) | ||
) | ||
if (isUpdate != 1L) { | ||
throw InsertException("member.updatefail.record") | ||
} | ||
|
||
memberDao.selectMemberByEmail( | ||
SelectMemberByEmailNotConsiderDeletedAtQuery( | ||
email = useCaseIn.email | ||
) | ||
)?.memberId ?: throw InsertException("member.selectfail.record") | ||
} else { | ||
/** 이미 가입한 회원이라면 회원 ID를 반환 */ | ||
isSignUpBeforeMember.memberId | ||
} | ||
}.let { | ||
/** 회원 ID를 암호화하여 토큰으로 사용 */ | ||
idEncryption.encrypt(it.toString()) | ||
) | ||
} |
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.
이메일을 보내는 것과 디비 상에 이메일을 저장하는 것을 분리했다는 말씀이신가요?
import java.util.* | ||
|
||
@Component | ||
class SaveMemberTxCase( |
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.
얘 자체는 그냥 멤버 저장하는 행위인건데 Tx라는 용어를 굳이 넣어야 하나 싶습니다
return dto.record?.let { signUpBeforeMember -> | ||
signUpBeforeMember.takeIf { it.isDeleted }?.let { | ||
ifDeletedMember(it.memberId) | ||
} ?: ifMember(signUpBeforeMember) | ||
} ?: ifNewMember(dto.email) |
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.
takeIf 처음 보네요 재밌는거 배워갑니다~
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.
확인요망
private val saveMemberTxCase: SaveMemberTxCase, | ||
) { | ||
companion object { | ||
private const val AUTH_HEAD_COMMENT = "few 로그인 링크입니다." | ||
private const val AUTH_SUB_COMMENT = "로그인하시려면 아래 버튼을 눌러주세요!" | ||
private const val SIGNUP_HEAD_COMMENT = "few에 가입해주셔서 감사합니다." | ||
private const val SIGNUP_SUB_COMMENT = "가입하신 이메일 주소를 확인해주세요." | ||
} | ||
|
||
@Transactional | ||
fun execute(useCaseIn: SaveMemberUseCaseIn): SaveMemberUseCaseOut { | ||
/** email을 통해 가입 이력이 있는지 확인 */ |
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.
코드 다시보니까 이거 SaveMemberUseCase에도 @transactional가 있어서 SaveMemberTxCase로 로직 빼낼 필요 없을거 같아요 종준님, 어차피 메일 보내는건 @async 아님?
|
||
@Transactional | ||
fun execute(dto: SaveMemberTxCaseIn): SaveMemberTxCaseOut { |
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.
여기처럼 SaveMemberTxCase에도 @transactional이 걸려있어서 분리하든 안하든 똑같겠네요
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.
우선 메일 전송 여부도 응답으로 내려주고 있어서 async로 하고 있지 않고
기존 UseCase의 트랜잭션은 재가 분리하지 않고 커밋을 했네요..! 🙇🏻
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.
REQUIRES_NEW 트랜잭션 전파 레벨 함 보시면 될거 같습니다
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.
Tx로 분리한다고 하면 UC에는 DB 관련된 행동이 없어서 트랜잭션을 빼도 되겠다고 생각했는데 REQUIRES_NEW가 더 좋을까요?
저도 댓글 남겨주신거 보고 생각해보니 더 좋을 것 같긴합니다.!!!
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.
아 제가 잘못 말씀드렸네요 (REQUIRES_NEW 아님)
코드는 기존 상태로 되돌린 담에 어싱크로 이메일 전송해야하는게 맞지않나요?
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.
응답에 이메일 전송 성공 여부까지 내려주고 있어서 불가능함다.!
다른 이메일은 다 비동기 처리했는데 인증 관련된 부분이라
나는 분명 버튼 누루고 전송되었다고 하는데 왜 이메일이 없어???? 이런 경험보다
응답이 조금 느려도 이메일 전송 여부까지 내려줘서 이메일 전송이 안되었을 경우에는 재시도하게 하는게 좋지 않을까 했어요!
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.
회의로 말씀해보죠
🎫 연관 이슈
resolved: #409
💁♂️ PR 내용
🙏 작업
[As-Is]
기존 트랜잭션에는 칼럼 수정/생성 이후 메일을 보내는 것까지 포함되어있었습니다.
[To-Be]
칼럼 수정/생성을 따로 분리하여 메일 보내는 것과 상관없이 트랜잭션을 끝낼 수 있도록 하였습니다.
🙈 PR 참고 사항
📸 스크린샷
🤖 테스트 체크리스트