Skip to content

Conversation

@nayonsoso
Copy link
Collaborator

@nayonsoso nayonsoso commented Jul 3, 2025

관련 이슈

작업 내용

커밋 따라가며 확인하시면 쉽습니다 😊

가장 고민이 되었던 부분 :
"멘토 페이지"를 조회할 때, Mentor 엔티티와 연관되지 않은 SiteUser, Mentoring 을 함께 조회해야 했습니다.

  • 멘토의 site_user 의 nickname, profile_image 를 가져와야함
  • 멘토링 지원 여부를 파악하기 위해 metoring.mentee_id 를 확인해야 함

그런데 페이지네이션으로 N개의 멘토를 찾고, 각각에 대해 위 작업을 수행하면 (N+1) 문제가 발생합니다.
이를 해결하기 위해, List<Mentor>와 연관된 SiteUser, Mentoring 을 한번에 조회해오고, 분류하는 방법을 사용했습니다.
그리고 이 내용은 "엔티티를 조회"에 대한 내용이므로, Repository의 성격을 따른다고 생각했습니다.
따라서 MentorBatchQueryRepository 라는 레포지토리를 만들고, 거기에 매핑 로직을 만들었습니다.

특이 사항

  • 멘토 목록 조회에서, "page"를 넘겨줘야 무한 스크롤이 가능한데, bruno 회의할 때는 안넣었더라고요😅
    page 받도록 컨트롤러 구현했고, bruno 수정 PR도 만들어두었습니다.
    (https://github.com/solid-connection/api-docs/pull/23)

  • 멘토의 대학교와 국가 이름은 "교환학생 인증 기능"이 만들어질 때 추가되어야 합니다.
    todo로 남겨두고 넘어갔습니다

  • 테스트 픽스쳐 만들 때, 성혁님의 77896f1 커밋을 cherry-pick 했습니다 🙇🏻‍♀️🙇🏻‍♀️

리뷰 요구사항 (선택)

제가 임의로 해석한 부분이 있는데요😅
멘토 목록을 조회할 때, "멘티 수가 동일하다면 id 큰 순 (=최신순)으로 정렬"하도록 구현했습니다.
괜찮은지는 기획 채널에 물어보고 오겠습니다.

@coderabbitai

This comment was marked as outdated.

coderabbitai[bot]

This comment was marked as outdated.

coderabbitai[bot]

This comment was marked as outdated.

nayonsoso added 5 commits July 3, 2025 16:55
- Mentor와 SiteUser는 id 만 참조하는 관계이다.
- Mentor와 Mentoring도 id 만 참조하는 관계이다.
- "멘토 목록"에 대해서 매번 siteUser, mentoring과 join 하면 N+1 이 발생한다.
- 이를 해결하기 위해, 한번에 조회하고 매핑하여 1번의 쿼리로 해결한다.
Copy link
Member

@whqtker whqtker left a comment

Choose a reason for hiding this comment

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

고생하셨습니다 ~ 문제가 되는 부분은 없어 보입니다 !

Gyuhyeok99

This comment was marked as duplicate.

@Gyuhyeok99 Gyuhyeok99 self-requested a review July 4, 2025 06:08
Copy link
Contributor

@Gyuhyeok99 Gyuhyeok99 left a comment

Choose a reason for hiding this comment

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

고생하셨습니다!! 추가 의견은 리뷰에 남겨놓았습니다!
어쩔 수 없이 Repository 로직 먼저 작성하긴 했지만(이건 레이어드 아키텍처의 한계니까요 ㅎㅎ..)
비즈니스 로직을 먼저 작성해주시니 훨씬 이해가 잘되는 거 같습니다! 저도 다음부턴 이렇게 작업해야겠어요!

mentor.getMenteeCount(),
mentor.isHasBadge(),
mentor.getIntroduction(),
mentor.getChannels().stream().map(ChannelResponse::from).toList(),
Copy link
Contributor

Choose a reason for hiding this comment

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

이거 채널이 sequence 순서로 정렬하는 로직이 어디있나요?

Copy link
Collaborator Author

@nayonsoso nayonsoso Jul 7, 2025

Choose a reason for hiding this comment

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

그런 로직도 필요하겠네요..!

반영했습니다 🫡 91cef41


@Repository
@RequiredArgsConstructor
public class MentorBatchQueryRepository { // 연관관계가 설정되지 않은 엔티티들을 N+1 없이 하나의 쿼리로 조회
Copy link
Contributor

Choose a reason for hiding this comment

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

오 이렇게 N+1 해결하는 것도 정말 좋은 거 같습니다! 함수명을 잘지어주시니 아주 잘 이해가 되네요! stream은 제가 공부를 조금 더 해보겠습니다..

추가로 채널같은 경우는 N+1이 그대로 발생하는데 최대 2개인가 그러니 별 상관 없겠죠?

Copy link
Collaborator Author

@nayonsoso nayonsoso Jul 7, 2025

Choose a reason for hiding this comment

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

추가로 채널같은 경우는 N+1이 그대로 발생하는데 최대 2개인가 그러니 별 상관 없겠죠?

그래도 N+1 방지하는게 좋을 것 같습니다. 제가 놓친 부분이네요😅

짚어주셔서 감사합니다~! 반영했습니다. fcc9244

SiteUser notAppliedUser = siteUserFixture.사용자(2, "멘토링 지원 안한 사용자");
SiteUser appliedUser = siteUserFixture.사용자(3, "멘토링 지원한 사용자");
mentoringFixture.대기중_멘토링(mentor.getId(), appliedUser.getId());

Copy link
Contributor

@Gyuhyeok99 Gyuhyeok99 Jul 4, 2025

Choose a reason for hiding this comment

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

87번라인 개행 제거!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

😇 4487fbf

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (1)
src/main/java/com/example/solidconnection/mentor/repository/MentorRepository.java (1)

8-11: 멘토 조회 기능의 기본 구조가 잘 구성되어 있습니다!

  1. JpaRepository 상속: 표준 CRUD 기능을 제공하는 올바른 접근 방식입니다.
  2. Slice 반환 타입: 무한 스크롤 기능에 적합한 선택입니다. Page 대신 Slice를 사용하여 전체 카운트 쿼리를 생략할 수 있어 성능상 이점이 있습니다.
  3. 메서드명 개선 제안: findBy는 다소 일반적인 이름입니다. 의도를 더 명확히 하기 위해 findAllBy 또는 findMentorsBy로 변경하는 것을 고려해보세요.
- Slice<Mentor> findBy(Pageable pageable);
+ Slice<Mentor> findAllBy(Pageable pageable);
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between dec6e03 and 5541188.

📒 Files selected for processing (6)
  • src/main/java/com/example/solidconnection/common/dto/SliceResponse.java (1 hunks)
  • src/main/java/com/example/solidconnection/mentor/controller/MentorController.java (1 hunks)
  • src/main/java/com/example/solidconnection/mentor/domain/Mentor.java (2 hunks)
  • src/main/java/com/example/solidconnection/mentor/repository/MentorRepository.java (1 hunks)
  • src/main/java/com/example/solidconnection/mentor/service/MentorQueryService.java (1 hunks)
  • src/test/java/com/example/solidconnection/mentor/service/MentorQueryServiceTest.java (1 hunks)
✅ Files skipped from review due to trivial changes (2)
  • src/main/java/com/example/solidconnection/mentor/domain/Mentor.java
  • src/main/java/com/example/solidconnection/common/dto/SliceResponse.java
🚧 Files skipped from review as they are similar to previous changes (3)
  • src/test/java/com/example/solidconnection/mentor/service/MentorQueryServiceTest.java
  • src/main/java/com/example/solidconnection/mentor/service/MentorQueryService.java
  • src/main/java/com/example/solidconnection/mentor/controller/MentorController.java
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: build

Copy link
Contributor

@Gyuhyeok99 Gyuhyeok99 left a comment

Choose a reason for hiding this comment

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

고생하셨습니다! 궁금한 사항 리뷰로 남겼습니다!


public static <T, R> SliceResponse<R> of(Slice<T> slice, List<R> content) {
int nextPageNumber = slice.hasNext()
? slice.getNumber() + BASE_NUMBER + 1
Copy link
Contributor

Choose a reason for hiding this comment

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

이렇게 표기하니 더 가독성이 좋은 거 같네요!

Comment on lines 53 to 55
int pageNumber = getPageNumber(mentorPage);

return new SliceResponse<>(content, pageNumber);
Copy link
Contributor

Choose a reason for hiding this comment

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

여전히 getPageNumber를 사용하고 있는데

SliceResponse.of(mentorPage, content); 반영안하신건지 확인 부탁드립니다!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

앗차차 만들기만하고 써먹진 않았네요 😓
반영했습니다! 8095d36


@Transactional(readOnly = true)
public SliceResponse<MentorPreviewResponse> getMentorPreviews(String region, SiteUser siteUser, Pageable pageable) { // todo: 멘토의 '인증' 작업 후 region 필터링 추가
Slice<Mentor> mentorPage = mentorRepository.findBy(pageable);
Copy link
Contributor

Choose a reason for hiding this comment

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

findBy가 메서드명이 조금 어색하네요 😅 혹시 All을 빼신 이유가 있으실까요?

Copy link
Collaborator Author

@nayonsoso nayonsoso Jul 8, 2025

Choose a reason for hiding this comment

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

실수로 누락한 것 같습니다. 💀
지금은 수정했습니다! d906c7d

Comment on lines +45 to +48
@PageableDefault(size = 3, sort = "menteeCount", direction = DESC)
@SortDefaults({
@SortDefault(sort = "menteeCount", direction = Sort.Direction.DESC),
@SortDefault(sort = "id", direction = Sort.Direction.ASC)
Copy link
Contributor

Choose a reason for hiding this comment

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

Repository 쪽에서 말고 여기서 정렬조건을 구현하신 이유가 혹시 있나요? 성능상 차이는 없지만 궁금해서 여쭤봅니다 ㅎㅎ

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

이렇게 하면 고정된 정렬 기준이 아니라, 프론트가 요청하는 정렬도 적용할 수 있어서요!
더 유연한 api를 제공한다고 생각했습니다~

Comment on lines 41 to 46
private static List<ChannelResponse> toChannelResponses(List<Channel> channels) {
return channels.stream()
.sorted(Comparator.comparingInt(Channel::getSequence))
.map(ChannelResponse::from)
.toList();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

채널을 응답해주는 곳마다 중복 코드가 발생하게 될 거 같은데 이대로 DTO에서 정렬을 할까요 아니면

@orderby("sequence ASC")
private List channels = new ArrayList<>();

나 혹은 도메인에 이러한 정렬 함수를 만드는 게 좋을까요??

Copy link
Collaborator Author

@nayonsoso nayonsoso Jul 8, 2025

Choose a reason for hiding this comment

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

@OrderBy라는 어노테이션이 있군요! 덕분에 알게되었습니다~

sequnce 자체가 "멘토가 설정한 채널 순서"라는 의미를 가지고 있으니
도메인 단에서부터 정렬하여 가지고 있는게 좋다고 생각합니다.

고민되는 것은,

  • 엔티티 조회 시 SQL ORDER BY 절을 자동으로 추가하여 정렬된 상태로 컬렉션을 로드하는 @OrderBy를 사용할지?
  • 정렬된 상태로 조회해오는 것 뿐 아니라, 변경되는 컬렉션의 순서도 그대로 영속화하는 @OrderColumn을 사용할지?

였습니다.

그런데 지금의 로직 상에서는, 어떤 채널이 "순서만 변경"되는 기능은 없고, 전체 삭제 후 다시 삽입하기 때문에
@OrderColumn은 과하다는 생각을 했습니다.
그리고 순서가 변경되는 것이 영속화된다는 것은, "예측하기 어려운 코드"이기도 하고요.!
그래서 @OrderBy를 사용하는 쪽으로 반영했습니다 😊

58fd475

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (2)
src/test/java/com/example/solidconnection/mentor/service/MentorQueryServiceTest.java (2)

32-53: 테스트 클래스 설정이 체계적입니다.

  1. @DisplayName을 통한 한국어 설명이 명확합니다.
  2. 필요한 모든 픽스처가 @Autowired로 주입되어 있습니다.
  3. 테스트 데이터 설정이 적절합니다.

다만, 51번 라인의 TODO 주석이 눈에 띕니다.


51-51: TODO 주석에 대한 추적이 필요합니다.

멘토 인증 기능 추가와 관련된 TODO가 있습니다. 이 부분은 향후 개발 계획에 따라 업데이트가 필요할 것 같습니다.

이 TODO와 관련된 이슈를 별도로 생성하여 추적하시겠습니까?

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 91cef41 and 58fd475.

📒 Files selected for processing (6)
  • src/main/java/com/example/solidconnection/common/dto/SliceResponse.java (1 hunks)
  • src/main/java/com/example/solidconnection/mentor/domain/Mentor.java (2 hunks)
  • src/main/java/com/example/solidconnection/mentor/dto/MentorPreviewResponse.java (1 hunks)
  • src/main/java/com/example/solidconnection/mentor/repository/MentorRepository.java (1 hunks)
  • src/main/java/com/example/solidconnection/mentor/service/MentorQueryService.java (1 hunks)
  • src/test/java/com/example/solidconnection/mentor/service/MentorQueryServiceTest.java (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
  • src/main/java/com/example/solidconnection/mentor/domain/Mentor.java
  • src/main/java/com/example/solidconnection/mentor/repository/MentorRepository.java
  • src/main/java/com/example/solidconnection/mentor/dto/MentorPreviewResponse.java
  • src/main/java/com/example/solidconnection/mentor/service/MentorQueryService.java
  • src/main/java/com/example/solidconnection/common/dto/SliceResponse.java
🧰 Additional context used
🧠 Learnings (1)
src/test/java/com/example/solidconnection/mentor/service/MentorQueryServiceTest.java (1)
Learnt from: nayonsoso
PR: solid-connection/solid-connect-server#375
File: src/main/java/com/example/solidconnection/mentor/service/MentorMyPageService.java:47-53
Timestamp: 2025-07-05T17:54:42.475Z
Learning: MentorMyPageService에서 PUT 메서드 구현 시 전체 채널을 새로 생성하여 교체하는 방식을 사용하는 것이 PUT의 의미론적 특성과 일치하며, 트랜잭션 로킹 관점에서도 합리적인 접근이다.
🔇 Additional comments (4)
src/test/java/com/example/solidconnection/mentor/service/MentorQueryServiceTest.java (4)

1-31: 깔끔한 테스트 클래스 구조입니다! 👍

import문과 패키지 구조가 명확하고 필요한 의존성들이 모두 포함되어 있습니다. 특히 @TestContainerSpringBootTest를 사용한 통합 테스트 환경 구성이 적절합니다.


54-98: 멘토 단일 조회 성공 테스트가 포괄적입니다.

  1. 멘토 정보 조회 테스트 (58-76라인)

    • 멘토 기본 정보와 채널 정보를 모두 검증
    • assertAll을 사용한 그룹 검증이 효과적
  2. 멘토링 신청 여부 조회 테스트 (78-97라인)

    • 신청한 사용자와 신청하지 않은 사용자 시나리오 모두 검증
    • 실제 비즈니스 로직에 맞는 테스트 케이스

테스트 케이스가 실제 사용자 시나리오를 잘 반영하고 있습니다.


100-113: 예외 처리 테스트가 정확합니다.

존재하지 않는 멘토 ID에 대한 예외 처리를 적절히 검증하고 있습니다:

  • CustomException 타입 검증
  • MENTOR_NOT_FOUND 에러 코드 메시지 검증

예외 처리 로직이 올바르게 동작하는지 확인할 수 있는 좋은 테스트입니다.


115-192: 멘토 미리보기 목록 조회 테스트가 매우 상세합니다.

  1. 테스트 데이터 설정 (118-130라인)

    • 상수 정의와 @BeforeEach를 통한 효율적인 테스트 준비
  2. 멘토 미리보기 정보 조회 테스트 (132-155라인)

    • Map을 활용한 효율적인 응답 데이터 검증
    • 각 멘토의 닉네임과 채널 정보를 정확히 검증
  3. 멘토링 지원 여부 조회 테스트 (157-172라인)

    • 지원한 멘토와 지원하지 않은 멘토 구분 검증
  4. 페이지네이션 테스트 (174-190라인)

    • 다음 페이지 번호와 마지막 페이지 처리 모두 검증
    • 184라인의 NO_NEXT_PAGE_NUMBER 상수 사용이 의미가 명확합니다

특히 Map을 활용한 응답 데이터 검증 방식이 효과적이고, 페이지네이션 로직까지 꼼꼼히 테스트한 점이 인상적입니다.

Copy link
Contributor

@Gyuhyeok99 Gyuhyeok99 left a comment

Choose a reason for hiding this comment

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

고생하셨습니다! 승인했습니다!

Copy link
Contributor

@lsy1307 lsy1307 left a comment

Choose a reason for hiding this comment

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

수고하셨습니다! 규혁님이 수정할만한 부분 잘 짚어주셔서 저는 코멘트는 없네용 : )

@nayonsoso nayonsoso merged commit b640883 into solid-connection:develop Jul 9, 2025
2 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.

feat: 멘토 단일/목록 조회

4 participants