-
Notifications
You must be signed in to change notification settings - Fork 16
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] N + 1 문제를 개선한다. #726
Conversation
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.
N + 1 문제가 발생하는 지점에서 @EntitiyGraph
어노테이션을 사용해서 Fetch Join 하였군요. 오늘 수달의 테코톡을 들으면서 생긴 의문점인데, 저희 같은 경우 pagination 을 사용할 때 fetch join 으로부터 발생하는 문제는 없을까요?
저희는 페이지네이션을 할 때 OneToMany를 사용하지 않아서 전혀 지장이 없습니다. - 매트 (1997. 09. 29 ~ ) |
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.
안녕하세요 매트 작성해주신 N+1 개선 코드와 쿼리 메서드를 옮겨주신 부분 모두 확인하였어요!
이제 API의 성능이 더욱 향상되었네요!!
한가지 의견만 남겨보았는데요~
수정할 부분이 보이지 않아 바로 approve 하였습니다!
고생하셨습니다~~🚀🚀
@EntityGraph(attributePaths = {"category", "category.member"}) | ||
List<CategoryRole> findByMemberId(final Long memberId); | ||
|
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.
한번에 fetch join 해오는군요!!
@@ -26,14 +23,11 @@ public ExternalCalendarService(final OAuthClient oAuthClient, final ExternalCale | |||
} | |||
|
|||
public ExternalCalendarsResponse findByMemberId(final Long memberId) { | |||
OAuthToken oAuthToken = oAuthTokenRepository.findByMemberId(memberId) | |||
.orElseThrow(NoSuchOAuthTokenException::new); | |||
OAuthToken oAuthToken = oAuthTokenRepository.getByMemberId(memberId); |
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.
레거시 했던 코드가 삭제됐군요!👍👍
@@ -12,10 +13,10 @@ public interface SubscriptionRepository extends JpaRepository<Subscription, Long | |||
|
|||
boolean existsByIdAndMemberId(final Long id, final Long memberId); | |||
|
|||
// TODO: N + 1 개선 예정 | |||
// TODO: @EntityGraph(attributePaths = {"category", "category.member"}) | |||
@EntityGraph(attributePaths = {"category", "category.member"}) |
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.
N+1이 개선 되었군요!!👍👍
@DisplayName("멤버와 카테고리 역할 목록으로 카테고리를 조회한다.") | ||
@Test | ||
void 멤버와_카테고리_역할_목록으로_카테고리를_조회한다() { | ||
// given | ||
Member 관리자 = memberRepository.save(관리자()); | ||
Member 후디 = memberRepository.save(후디()); | ||
|
||
Category 카테고리1 = categoryRepository.save(공통_일정(관리자)); | ||
Category 카테고리2 = categoryRepository.save(BE_일정(관리자)); | ||
Category 카테고리3 = categoryRepository.save(FE_일정(관리자)); | ||
Category 카테고리4 = categoryRepository.save(매트_아고라(관리자)); | ||
Category 카테고리5 = categoryRepository.save(후디_JPA_스터디(관리자)); |
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.
불필요한 테스트 코드가 삭제되었군요🚀🚀
@@ -113,14 +114,21 @@ public CategoriesWithPageResponse findMyCategories(final Long memberId, final St | |||
|
|||
// 멤버가 ADMIN이 아니어도 일정 추가/제거/수정이 가능하므로, findAdminCategories와 별도의 메소드로 분리해야함 | |||
public CategoriesResponse findScheduleEditableCategories(final Long memberId) { | |||
List<CategoryRole> categoryRoles = categoryRoleRepository.findByMemberId(memberId); |
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.
CategoryRepository
에 있던 CategoryRole
을 통한 데이터 조회 로직이 삭제되었네요!!
|
||
private List<Category> toCategories(final List<CategoryRole> categoryRoles, final Set<CategoryRoleType> roleTypes) { | ||
return categoryRoles.stream() | ||
.filter(categoryRole -> roleTypes.contains(categoryRole.getCategoryRoleType())) | ||
.map(CategoryRole::getCategory) | ||
.collect(Collectors.toList()); |
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.
a: 저는 변환 로직을 DTO로 옮기는 편이여서 CategoriesResponse
에서 해당 변환로직을 가지는건 어떨까해요!
한번 의견만 제시해보았어요😊😊
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.
이 부분은 항상 고민이네요... 저는 dto가 가벼운 걸 선호해서 일단 유지하겠습니다!
dce7415
to
6cddc52
Compare
[feat] PR을 등록한다.
작업 내용
N + 1 문제를 개선한다.
스크린샷
주의사항
Closes #718