Skip to content

Conversation

@m-a-king
Copy link
Collaborator

@m-a-king m-a-king commented Jan 1, 2025

resolved :

📌 과제 설명

  • 유저 도메인을 추가하여 봉사자와 기관의 상위 도메인 역할을 하도록 구현했습니다.
  • 인증/인가 로직에 변화가 생겼으며, 카카오/구글 등의 소셜 로그인을 지원할 수 있는 구조로 개선했습니다.
    • 이전에는 사업자가 될 수 있다는 가정하에 실명 인증 대체제로 구성되어 있었습니다.
    • 현재는 OAuth ID만을 활용하여 클릭 한 번으로 로그인을 지원하는 형태로 변경했습니다.

👩‍💻 요구 사항과 구현 내용

  1. 유저 도메인 추가
    • 봉사자와 기관의 상위 도메인으로 역할 수행.
  2. 인증/인가 로직 수정
    • 소셜 로그인(Kakao, Google 등)을 지원할 수 있도록 구조 변경.

✅ PR 포인트 & 궁금한 점

  1. TODO 주석 관련
    • 개인적인 메모 및 향후 작업을 위한 주석이 포함되어 있습니다.
    • 리뷰어의 피드백을 받으면서 수정 및 보완할 예정입니다.
  2. 이벤트 기반 봉사자/기관 등록
    • 이 부분은 프론트엔드와 협의가 필요해 현재로서는 구현을 미뤄둔 상태입니다.
  3. 현재 작업 상태
    • 어플리케이션이 정상적으로 작동 가능한 구조로 변경 중이며, 완성까지 시간이 더 필요합니다.
  4. 유저, 유저 공통 속성, 유저 세팅을 한 개의 쿼리 서비스에서 처리하려고 했습니다. 이에 대한 피드백을 부탁드립니다.
image
  1. 이어질 진행 상황은 이슈에 등록하겠습니다!

@m-a-king m-a-king self-assigned this Jan 1, 2025
@m-a-king m-a-king linked an issue Jan 2, 2025 that may be closed by this pull request
2 tasks
private static final String PREFIX = "Bearer ";

public String getValueWithPrefix() {
return PREFIX + this.value;
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.

감사합니다 ㅎㅎ

@Service
@RequiredArgsConstructor
@Transactional(readOnly = true)
public class OAuthInfoCheckerImpl implements OAuthInfoChecker {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Cheker가 보편적인 이름인가요? 나중에 참고해보려고 여쭤봅니다!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

존재 여부를 확인하는 유즈케이스에서 클래스 이름에 exist라는 단어를 사용하는 대신, 더 추상화된 이름을 선택하려고 고민했습니다. 클래스는 추상적인 이름을, 메서드는 구체화된 이름을 갖도록 설계하는 것을 목표로 했습니다.

또한, validate는 검증의 뉘앙스가 강하고, inspector나 analyzer는 작업의 성격과 잘 맞지 않아 적합하지 않다고 판단했습니다. 만약 검증의 의미가 더 강했다면 validate를, 추가적인 처리가 필요했다면 processor를 사용했겠지만, 이번 작업은 단순히 존재 여부만을 확인하는 가벼운 작업이기 때문에, exist라는 단어를 피하면서도 적합한 이름으로 checker를 선택했습니다.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

덧붙여, 원래 기존 도메인에서는 이러한 로직이 쿼리 유즈케이스(서비스)에 포함되는 것이 일반적이지만, 인증/인가 로직에서는 별도로 구분하는 것이 훨씬 명확하고 적합하다고 판단해서 checker와 같은 클래스가 나타난 것 같습니다.

Copy link
Collaborator

Choose a reason for hiding this comment

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

답변 감사합니다!

return oAuthInfoQueryService.getUserIdByCommonOAuthInfo(oauthInfo);
}

private User registerOAuthUser(CommonOAuthInfo oauthInfo) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

@transactional(readOnly = true)라서 save 트랜잭션이 발생하지 않을거라고 생각되는데요
메서드에 @transactional를 붙여줘야하지 않을까 생각했습니다
또 private 메서드가 많아진것 같아 클래스를 나누는것도 고민해보면 좋을것 같은데 어떻게 생각하시나요

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@transactional(readOnly = true)라서 save 트랜잭션이 발생하지 않을거라고 생각되는데요
메서드에 @transactional를 붙여줘야하지 않을까 생각했습니다

완전히 실수했네요! 수정하겠습니다.

또 private 메서드가 많아진것 같아 클래스를 나누는것도 고민해보면 좋을것 같은데 어떻게 생각하시나요

제가 코드를 검토했을 때는 이미 클래스는 많이 나뉘어져 있다고 생각했습니다.
혹시 추상화 수준을 맞추기 위해 한 줄의 코드를 추가적인 private 메서드로 묶다 보니, private 메서드의 개수가 많아 보이는 것은 아닐까 싶습니다.
좋은 아이디어가 있으시거나 특별히 신경쓰이는 부분을 짚어주시면 다시 한 번 더 살펴보겠습니다~

Copy link
Collaborator

Choose a reason for hiding this comment

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

문제가 있다고는 생각하지 않습니다!
private 메서드가 많아진 것 같다면 클래스의 책임에 대해 고민해볼 차례라는 말을 들어본 적이 있어서
고민해볼만한 부분이지 않을까 싶어서 말씀드려봤습니다!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

동감합니다. 말씀해주신 부분 주시하겠습니다 ㅎㅎ

}

private OAuthInfo createOAuthInfo(User user, CommonOAuthInfo commonOAuthInfo) {
return OAuthInfo.builder()
Copy link
Collaborator

Choose a reason for hiding this comment

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

취향 차이일수도 있습니다만
저는 엔티티 클래스 내부에 create 메서드를 만들어서 내부 상태, 필드를 드러나지 않게 하는 코드로 바꿔가는 중인데
재중님께서는 이런방법 어떻게 생각하시나요 예시 코드는 아래와 같습니다

@Builder
private Order(List<Product> products, OrderStatus orderStatus, LocalDateTime registeredDateTime) {
        this.orderStatus = orderStatus;
        this.totalPrice = calculateTotalPrice(products);
        this.registeredDateTime = registeredDateTime;
        this.orderProducts = products.stream()
            .map(product -> new OrderProduct(this, product))
            .collect(Collectors.toList());
    }

public static Order create(List<Product> products, LocalDateTime registeredDateTime) {
        return Order.builder()
            .orderStatus(OrderStatus.INIT)
            .products(products)
            .registeredDateTime(registeredDateTime)
            .build();
    }

Copy link
Collaborator Author

@m-a-king m-a-king Jan 2, 2025

Choose a reason for hiding this comment

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

저도 좋은 의견이라고 생각합니다. 저도 자주 그런 방식으로 구현했지만, 정확한 기준 없이 그때마다 끌리는 대로 결정했던 것 같습니다. 서비스에서 직접 빌더 패턴을 사용하는 대신, 도메인 내부에 public으로 create 메서드를 만들어 private 빌더 패턴을 활용하는 방식을 제안하시는 것이 맞을까요?

Copy link
Collaborator

Choose a reason for hiding this comment

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

도메인 내부에 public으로 create 메서드를 만들어 private 빌더 패턴을 활용하는 방식
네 맞습니다 저도 프로젝트마다 다른 방법으로 구현했었는데 이번에는 엔티티 외부에서 내부 상태를 알 필요가 없다는 말에 공감이 되서
빌더를 숨기는 방법을 선택했습니다

Copy link
Collaborator

Choose a reason for hiding this comment

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

오 좋은 거 같아요
저도 적용하겠습니다

eqOAuthInfo(provider, oauthId),
isNotDeleted()
)
.fetchFirst() != null;
Copy link
Collaborator

Choose a reason for hiding this comment

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

최적화된 exists 좋은것 같아요 저도 참고하겠습니다


// 비용 절감: 특정 필드만 조회하여 데이터베이스 접근을 최소화
// 최신 데이터에 더 가까움
boolean isCustomized2 = userQueryUseCase.getIsCustomizedByUserId(userId);
Copy link
Collaborator

@7zrv 7zrv Jan 2, 2025

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.

저도 서진님이랑 같은 생각입니다.

assertThat(savedUserCommonAttribute.isCustomized()).isEqualTo(false);

}
} No newline at end of file
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.

감사합니다. 계속 하나씩 나타나네요...ㅎㅎ

// when
UserCommonAttribute savedUserCommonAttribute = userCommonAttributeRepository.save(userCommonAttribute);

// then
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.

감사합니다. 객체 간의 동등성을 검사하는 방식이 더 좋을 것 같아 해당 방향으로 수정하겠습니다!

void setup() {
UserAuthInfo userAuthInfo = UserAuthInfo.createForOAuth(OAuthProvider.NAVER);

user = userRepository.save(User.from(userAuthInfo, UserRole.VOLUNTEER));
Copy link
Collaborator

Choose a reason for hiding this comment

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

저는 BeforeEach에서 데이터 생성시에 데이터의 상태를 변환하는 테스트에 영향을 받는 케이스가 있었는데요
예를들어 쪽지의 경우 QueryService에서 읽음처리를 해주어야해서 데이터 상태가 변해 다른 테스트 케이스에 영향을 주는일이 있었습니다.
QueryService지만 추후에 문제가 되는 부분이 없을지 고민해보시면 좋을것 같아요!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

beforeEach라면 각 케이스마다 모든 값이 재설정되지 않는지 여쭤보고 싶습니다!

혹시 beforeEach 안에서 상태 변화를 할 이후에 대해서 조언해 주신 걸까요?

Copy link
Collaborator

Choose a reason for hiding this comment

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

저는 단위 테스트에서는 문제가 없었지만 통합테스트시에 문제가 생겼던 케이스인데요
지금 생각해보니 BeforeEach가 아닌 바깥의 클래스 변수로 생성했던 엔티티가 문제였던거 같기도 합니다
재중님의 경우에는
private User user;
이 부분이 될 거 같아요
전 생성을 분할하고 나서 해결이 됐던 케이스라 말씀드려봤습니다

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

오호 참고하겠습니다!


@BeforeEach
void setup() {
UserAuthInfo userAuthInfo = new UserAuthInfo("[email protected]", "Test User");
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.

감사합니다!

@7zrv
Copy link
Collaborator

7zrv commented Jan 2, 2025

고생하셨습니다

Copy link
Collaborator

@leebs0521 leebs0521 left a comment

Choose a reason for hiding this comment

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

고생하셨습니다.

여러번 살펴 봤는데 잘 작성하신 것 같습니다.
궁금 사항의 3번의 경우는 저는 개인적으로 지금은 메서드가 3가지 뿐이라 괜찮아 보입니다.


// 비용 절감: 특정 필드만 조회하여 데이터베이스 접근을 최소화
// 최신 데이터에 더 가까움
boolean isCustomized2 = userQueryUseCase.getIsCustomizedByUserId(userId);
Copy link
Collaborator

Choose a reason for hiding this comment

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

저도 서진님이랑 같은 생각입니다.

- 달라진 oauth 결정 사항에 따라서 기존 로직 삭제.
- JPA 메서드 네이밍으로 인한 오류 수정 (OAuth → oauth로 통일된 상태, Jpa는 oAuth를 원하는 상태)
- QueryDSL 공통 조건 메서드 추출
- Qclass import 문제 해결
- 메서드 순서 변경
- RequestDto로 두는 것보다 VO의 개념으로 두는 것이 더 올바르다고 판단.
- user가 특정 데이터베이스(H2 등)의 예약어로 사용됨.
- 구현은 프론트엔드와의 협의 후로 미룸.
- 유저 아이디 기준 조회.
- 도메인 내부에 public으로 create 메서드를 만들어 private 빌더 패턴을 활용
- 소나 큐브 이슈 해결
@m-a-king m-a-king force-pushed the refactor/239-user-domain branch from e39eca4 to f25b048 Compare January 3, 2025 08:34
@sonarqubecloud
Copy link

sonarqubecloud bot commented Jan 3, 2025

@m-a-king m-a-king merged commit d7ad70b into main Jan 3, 2025
3 checks passed
@m-a-king m-a-king deleted the refactor/239-user-domain branch January 3, 2025 09:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[REFACTOR] 유저 리팩토링

5 participants