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

feat(member): 세션, 최고점수, 전적, 경험치를 포함해 온전한 계정 통합을 할 수 있다 #159

Merged
merged 4 commits into from
Jul 29, 2024

Conversation

0chil
Copy link
Collaborator

@0chil 0chil commented Jul 28, 2024

스압 PR 죄송합니다 ㅎㅎ
읽으시면서 궁금하신 점은 편하게 질문 주세요!

변경 사항

AS-IS

어지러운 계정 통합... 테스트조차 없었습니다. 거의 유령 코드...🥲
특히 전적이나 경험치가 이전되지 않는 문제가 있었습니다.

이번에 앱에서 통합을 구현하면서 생각나서! 서버에서도 손보게 되었습니다.

TO-BE

계정 통합을 member.service.AccountIntegration 인터페이스로 통합하였습니다.
해당 인터페이스의 구현체는 기존처럼 각 패키지 내에 구현되어 있고, MemberAccountService에서 DI를 통해 일괄 실행됩니다.

@Transactional
public Member integrate(long victimId, long currentMemberId) {
    for (AccountIntegration integration : accountIntegrations) {
        integration.execute(victimId, currentMemberId);
    }
    members.deleteById(victimId);
    return members.getById(currentMemberId);
}

특이사항

  • @Component vs @Service,, 스프링 빈 등록되는 점은 같은데, 어떻게 구분해서 사용하면 좋을까요?
    이번 PR을 작성하면서 스낵게임에서는 둘을 구분하는 기준이 없다는 것을 깨달아버렸습니다...
    하지만 개인적으로 컴포넌트가 더 작은 단위라는 생각도 들었는데요,

    컴포넌트는 '구성 요소'라는 뜻으로 조금 더 큰 요소(ex, 서비스)의 일부가 되어야 한다고 생각했습니다.
    그래서 AccountIntegration 구현체들을 @Component로 표기하였습니다.
    이런 의도를 좀 더 잘 표현하고자 아래 제약도 추가하게 되었습니다 👇

  • @Component의 메서드를 독립적으로 사용할 수 없게!
    Component가 독립적으로 사용되지 않도록 제약을 줘봤습니다.
    어떤 의미냐면, 'AccountIntegration 구현체들의 메서드를 실행하려면 기존 트랜잭션이 필수로 있어야 한다'는 제약을 추가했다는 의미입니다.

    말이 좀 어려운데요 😥
    계정 통합이 일부만 일어난다면 어떨까요?
    비즈니스 로직의 무결성이 깨질 수 있습니다. 예를 들면, 게임을 한 판도 안했지만 1등일 수 있게 되는거죠.
    이런 일을 막고자 좀 더 강력한 제약을 두었다고 이해해주시면 감사드리겠습니다.

    실제 코드로 보면 더욱 좋을 것 같은데요.

    @Component
    class SnackgameSessionTransfer(
        private val snackgameRepository: SnackgameRepository
    ) : AccountIntegration {
    
        @Transactional(propagation = Propagation.MANDATORY)
        override fun execute(victimMemberId: Long, currentMemberId: Long) {
            val transferredCount = snackgameRepository.transferSessions(victimMemberId, currentMemberId)
    
            log.debug("memberId={}의 스낵게임 세션 {}개를 memberId={}에게 이전했습니다", victimMemberId, transferredCount, currentMemberId)
        }
    }

    스프링 프레임워크 사용 환경에서, 이 메서드는 기존 트랜잭션이 없으면 오류가 발생합니다.
    전파 속성이 Propagation.MANDATORY로 표현되어 있기 때문인데요, (기본은 Propagation.REQUIRED)

    그래서 기존 트랜잭션이 있는 메서드 (예를 들면, MemberAccountService.integrate)에서만 호출할 수 있게 됩니다.
    확실히 있는 편이 안전하겠죠?

이상입니다!

@0chil 0chil requested a review from Hwanvely July 28, 2024 14:05
@0chil 0chil self-assigned this Jul 28, 2024
@0chil 0chil added the feat label Jul 28, 2024
@0chil 0chil changed the title Feat/complete account integration feat(member): 세션, 최고점수, 전적, 경험치를 포함해 온전한 계정 통합을 할 수 있다 Jul 28, 2024
@Hwanvely
Copy link
Collaborator

Hwanvely commented Jul 28, 2024

@Component vs @Service,, 스프링 빈 등록되는 점은 같은데, 어떻게 구분해서 사용하면 좋을까요?
👉 먼저 새로운 시각을 제시해주셔서 감사합니다! 저는 당연히 더 세분화된 역할을 갖고 있으면 작은 단위라고 생각했는데 땡칠 말처럼 '역할'에 초점을 맞춰서 @Service의 일부가 될 수도 있겠네요!

전파 속성이 Propagation.MANDATORY로 표현되어 있기 때문인데요, (기본은 Propagation.REQUIRED)

그래서 기존 트랜잭션이 있는 메서드 (예를 들면, MemberAccountService.integrate)에서만 호출할 수 있게 됩니다.
확실히 있는 편이 안전하겠죠?
👉 기가 막힌 장치인것 같습니다 역시 땡칠...

번외 질문이지만 위의 경우에서 ~transfer들에 모두 @Service를 붙이는 것은 이상할까요??

자세한 코드는 바로 더 뜯어보겠습니다. 더운데 고생하셨습니다!

Comment on lines +25 to +26
log.debug(
"memberId={}의 사과게임 세션 {}개를 memberId={}에게 이전했습니다",
Copy link
Collaborator

Choose a reason for hiding this comment

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

로깅 남겨둔 이유가 있을까요??

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

혹시나 통합이 잘 안되는 경우 로그를 쉽게 살펴보기 위해 로깅을 추가해두었습니다.
서버를 통째로 디버깅하기는 꽤 어려우니까요!

이 경우 패키지에 대한 로깅 레벨을 바꾸는 것 만으로 로그를 볼 수 있어 편리합니다 :)

그리고 이건 비밀인데,,
장기적으로 이벤트 기반으로 알림을 전송하는 현재 구조에서 벗어나,
로그 중 일부를 전송하도록 하면 어떨지를 고민하고 있습니다.

이유는 매번 이벤트를 만들기가 꽤 귀찮기 때문입니다 ㅎㅎ
그래서 미리미리 로그를 심어두는 측면도 있습니다

Copy link
Collaborator

Choose a reason for hiding this comment

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

오호 로그를 통한 전송이 어떻게 이루어지는지는 모르겠지만 이유는 알겠습니다!

Comment on lines -63 to -64
victim.invalidate();
return socialMember;
Copy link
Collaborator

Choose a reason for hiding this comment

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

둘다 Soft Delete라고 보이는데 수정한 이유가있나요??

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

멤버 도메인 객체에 invalidate를 호출하는 의미가 거의 없어서,
서비스 차원에서의 soft delete로 변경하였습니다.
물론 스키마가 바뀐건 아니라서, 추후 로직 확장에 따라 회귀할 가능성도 있습니다

@0chil 0chil merged commit 07933fb into dev Jul 29, 2024
13 checks 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