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

[#10] 나의 여행 등록 설정 #12

Merged
merged 35 commits into from
Feb 1, 2025
Merged

[#10] 나의 여행 등록 설정 #12

merged 35 commits into from
Feb 1, 2025

Conversation

hynxp
Copy link
Collaborator

@hynxp hynxp commented Jan 24, 2025

#️⃣ Issue

🧑🏻‍🏫 메인 리뷰어 지정

📝 요약

나의 여행 일정을 등록하고 조회하는 기능을 구현했습니다.

🛠 작업 내용

  • 각 나라와 도시는 DB에 미리 기본 세팅 후 개발하였습니다.
  • 한 사람당 여행 일정은 1개만 등록가능합니다.

🤔 리뷰 시 참고 사항

  • 현재 연관관계에 따른 ERD는 아래와 같습니다.
스크린샷 2025-01-24 오전 8 54 56
  • MemberMemberTravelSchedule은 일대일매핑이지만 Member가 자신의 여행 일정을 자주 참조할 경우가 잦을 것 같아 단방향으로 매핑했습니다. 추후에 특정 도시를 여행 중인 사용자 목록과 같은 기능이 필요하다면 그때 양방향 참조를 설정해도 된다고 판단했습니다.
  • MemberTravel -> TravleSchedule 네이밍 변경하는 게 적절할까요?
  • member 패키지가 아닌 travelschedule 패키지에 관련 로직을 작성했는데 적절할까요?
  • TravelScheduleServiceTest.findTravelSchedule_ok에서 계속 org.hibernate.LazyInitializationException가 발생해 @Transactional을 추가했는데요. 다른 방법으로는 FetchType=EAGER로 변경하는 법이 있으나 Member조회 시 무조건 여행 일정을 select해야 하냐는 의문점이 있습니다.
  • MemberTravelSchedule을 조회할 때 이 여행 일정의 도시(travelScheduleCities)도 같이 조회돼야 한다고 생각해 FetchType.EAGER로 설정했는데 적절할까요?

✅ 체크리스트

  • PR 제목을 명령형으로 작성했습니다.
  • PR을 연관되는 github issue에 연결했습니다.
  • 리뷰 리퀘스트 전에 셀프 리뷰를 진행했습니다
  • 변경사항에 대한 테스트코드를 추가했습니다. 또는, 테스트 코드가 필요없는 이유가 있습니다.

@hynxp hynxp added the feat New feature or request label Jan 24, 2025
@hynxp hynxp requested a review from blue000927 January 24, 2025 00:22
@hynxp hynxp self-assigned this Jan 24, 2025
Copy link

@pjy1368 pjy1368 left a comment

Choose a reason for hiding this comment

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

[PR 질의응답]
Q. MemberTravel -> TravleSchedule 네이밍 변경하는 게 적절할까요?
A. 넵 저는 적절해 보입니다. Schedule이라는 표현이 더 직관적이라고 느껴져요!

Q. member 패키지가 아닌 travelschedule 패키지에 관련 로직을 작성했는데 적절할까요?
A. 이거는 좀 고민되는데 travelschedule가 member에 아예 종속적이지 않고 독립적으로 쓰일 여지가 있어서 괜찮다고 생각해요.

Q. TravelScheduleServiceTest.findTravelSchedule_ok에서 계속 org.hibernate.LazyInitializationException가 발생해 @transactional을 추가했는데요. 다른 방법으로는 FetchType=EAGER로 변경하는 법이 있으나 Member조회 시 무조건 여행 일정을 select해야 하냐는 의문점이 있습니다.
A. 네 저도 이거때문에 즉시 로딩을 권장해드리지는 않고, 가장 쉽게 하려면 테스트 메서드 위에 @transactional을 적용하고 가는 것이긴합니다. OSIV 설정을 테스트 단에서만 하는 방법도 있는데, 저는 JPA에 많은 시간을 지금 투자하는 것을 권장하지는 않아서 추후 학습해 보시면 좋을 듯합니다.

Q. MemberTravelSchedule을 조회할 때 이 여행 일정의 도시(travelScheduleCities)도 같이 조회돼야 한다고 생각해 FetchType.EAGER로 설정했는데 적절할까요?
A. 제가 코멘트로 남겨두긴했는데, 비즈니스 로직 특성상 "항상" 조회해야 된다면 그렇게 하셔도 됩니다.

@@ -80,8 +80,14 @@ private void validateIsTokenOwner(String refreshToken, Long memberId) {

private Member updateRefreshToken(Long memberId, String refreshToken) {
Member member = memberRepository.findById(memberId)
.orElseThrow(() -> MemberNotFoundException.EXCEPTION);
.orElseThrow(() -> new NotFoundException("해당하는 회원이 존재하지 않습니다."));

Choose a reason for hiding this comment

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

이거는 적용하라는 의미는 아니고 아이디어를 하나 드립니다.
매번 해당하는 ~가 존재하지 않습니다 메시지 적기 귀찮을 수 있으니, NotFoundException 단에서 "해당하는 %이 존재하지 않습니다." 상수를 만들어 놓고 생성자를 통해 문자열만 받거나 클래스 자체를 넘기는 방법도 있어요! 후자를 선택한다면 약간의 리플렉션 작업(클래스 이름 추출)을 하긴 해야하지만요 ㅎㅎ

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

오오.. 다른 프로젝트에서 본 것 같아요!
에러 핸들링 이슈(#9)에서 적용해보겠습니다~

src/test/java/com/hyun/udong/auth/oauth/TestOauth.java Outdated Show resolved Hide resolved
Comment on lines 32 to 36
@BeforeEach
void setUp() {
Member member = new Member(1L, SocialType.KAKAO, "hyun", "profile_image");
memberRepository.save(member);
}

Choose a reason for hiding this comment

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

truncate 등의 방식으로 DB를 완전히 비우는 작업이 있으면 좋겠습니다.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

테이블을 조회해 truncate 하는 클래스를 추가해보았습니다.
ObjectOptimisticLockingFailureException 관련해서 슬랙 메시지 보냈습니당 😢

src/test/resources/member.sql Outdated Show resolved Hide resolved
@hynxp hynxp mentioned this pull request Jan 30, 2025
1 task
@hynxp hynxp merged commit 150b90d into main Feb 1, 2025
1 check passed
@hynxp hynxp deleted the feat/register-travel branch February 1, 2025 11:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants