refactor: 공고 현황 매칭 카운트 로직 수정 #492
Conversation
Summary of ChangesHello @kckc0608, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! 이 PR은 공고 현황 목록에서 매칭 및 지원자 수를 계산하는 로직을 리팩토링합니다. 기존에는 JPA Repository의 JPQL 쿼리 내에서 직접 카운트를 수행했지만, 이제는 Repository는 순수하게 Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
이 PR은 공고에 대한 매칭 및 지원자 수 계산 로직을 리팩토링합니다. 로직이 데이터베이스 쿼리에서 서비스 계층으로 이동되었습니다. 이러한 변경은 유연성을 높일 수 있지만, 현재 구현은 SocialWorkerMatchingService와 ElderlyService에서 N+1 쿼리로 인해 심각한 성능 문제를 야기합니다. 각 공고에 대해 여러 데이터베이스 쿼리가 실행되어, 특히 공고가 많은 페이지에서 성능 저하를 초래할 것입니다. 이러한 성능 문제를 해결해야 합니다. 또한, RecruitmentRepository에 메서드의 반환 타입이 쿼리 결과와 일치하지 않는 잠재적인 버그가 있습니다.
.../java/com/becareful/becarefulserver/domain/matching/service/SocialWorkerMatchingService.java
Show resolved
Hide resolved
.../java/com/becareful/becarefulserver/domain/matching/service/SocialWorkerMatchingService.java
Show resolved
Hide resolved
...ain/java/com/becareful/becarefulserver/domain/matching/repository/RecruitmentRepository.java
Outdated
Show resolved
Hide resolved
src/main/java/com/becareful/becarefulserver/domain/socialworker/service/ElderlyService.java
Show resolved
Hide resolved
|
/gemini review |
There was a problem hiding this comment.
Code Review
이번 PR은 공고 목록의 매칭 및 지원자 수 계산 로직을 서비스 계층으로 이동하여 리팩토링했습니다. 이를 통해 기존에 matchingCount가 잘못 계산되던 버그를 수정한 점은 매우 좋습니다. 테스트 코드 또한 새로운 로직에 맞게 잘 보강되었습니다.
다만, 새로운 구현 방식이 SocialWorkerMatchingService와 ElderlyService에서 N+1 쿼리 문제를 발생시키는 것으로 보입니다. 각 공고마다 지원자 수를 계산하기 위해 개별적으로 쿼리를 실행하고 있어, 데이터가 많아질 경우 심각한 성능 저하를 유발할 수 있습니다. 이 문제에 대한 구체적인 해결 방안을 각 파일에 대한 리뷰 코멘트로 남겼으니 확인 부탁드립니다.
추가적으로, 자동 매칭 수(matchingCount)를 계산하는 로직이 메모리 상에서 모든 WorkApplication을 순회하며 수행되는데, WorkApplication 데이터가 많아질 경우 이 부분도 성능에 영향을 줄 수 있습니다. 이 점도 추후 개선을 고려해보시면 좋겠습니다.
| List<WorkApplication> workApplications = workApplicationRepository.findAllActiveWorkApplication(); | ||
|
|
||
| return recruitmentRepository | ||
| .findAllByInstitutionAndRecruitmentStatusIn( | ||
| socialworker.getNursingInstitution(), recruitmentStatus, pageable) | ||
| .map(recruitment -> { | ||
| long applicationCount = applicationRepository.countByRecruitment(recruitment); | ||
| long matchingCount = workApplications.stream() | ||
| .filter(workApplication -> matchingDomainService.isMatched(workApplication, recruitment)) | ||
| .count(); | ||
|
|
||
| return SocialWorkerRecruitmentResponse.of(recruitment, applicationCount, matchingCount); | ||
| }); |
There was a problem hiding this comment.
현재 구현은 recruitment 목록을 순회하며 각 recruitment에 대해 applicationRepository.countByRecruitment(recruitment)를 호출하고 있습니다. 이는 페이지에 포함된 공고 수만큼 추가 쿼리를 발생시키는 N+1 문제를 유발하여 성능 저하의 원인이 될 수 있습니다. 예를 들어, 페이지 크기가 20이라면 1개의 공고 목록 쿼리와 20개의 지원자 수 카운트 쿼리가 실행되어 총 21개의 쿼리가 발생합니다.
searchRecruitmentList 메서드(128-138 라인)에서도 동일한 문제가 발견되었습니다.
이 문제를 해결하기 위해, 공고 목록을 가져온 후 해당 공고들에 대한 지원자 수를 한 번의 쿼리로 가져오는 것을 권장합니다.
ApplicationRepository에 다음과 같은 메서드를 추가할 수 있습니다:
@Query("SELECT a.recruitment.id, count(a) FROM Application a WHERE a.recruitment IN :recruitments GROUP BY a.recruitment.id")
Map<Long, Long> countMapByRecruitmentIn(List<Recruitment> recruitments);그리고 서비스 로직을 다음과 같이 수정할 수 있습니다:
// 예시 코드
Page<Recruitment> recruitmentPage = recruitmentRepository
.findAllByInstitutionAndRecruitmentStatusIn(
socialworker.getNursingInstitution(), recruitmentStatus, pageable);
List<Recruitment> recruitments = recruitmentPage.getContent();
if (recruitments.isEmpty()) {
return Page.empty();
}
Map<Long, Long> applicationCounts = applicationRepository.countMapByRecruitmentIn(recruitments);
List<WorkApplication> workApplications = workApplicationRepository.findAllActiveWorkApplication();
return recruitmentPage.map(recruitment -> {
long applicationCount = applicationCounts.getOrDefault(recruitment.getId(), 0L);
long matchingCount = workApplications.stream()
.filter(workApplication -> matchingDomainService.isMatched(workApplication, recruitment))
.count();
return SocialWorkerRecruitmentResponse.of(recruitment, applicationCount, matchingCount);
});이렇게 하면 데이터베이스 호출을 크게 줄여 성능을 개선할 수 있습니다.
| List<SocialWorkerRecruitmentResponse> responses = recruitmentRepository.findAllByElderly(elderly).stream() | ||
| .map(recruitment -> { | ||
| long applicationCount = applicationRepository.countByRecruitment(recruitment); | ||
| long matchingCount = workApplications.stream() | ||
| .filter(workApplication -> matchingDomainService.isMatched(workApplication, recruitment)) | ||
| .count(); | ||
|
|
||
| return SocialWorkerRecruitmentResponse.of(recruitment, applicationCount, matchingCount); | ||
| }) | ||
| .toList(); |
There was a problem hiding this comment.
이 부분에서도 recruitment 목록을 순회하며 각 recruitment에 대해 applicationRepository.countByRecruitment(recruitment)를 호출하고 있어 N+1 문제가 발생합니다. 한 어르신에게 여러 공고가 있을 수 있으므로, 불필요한 데이터베이스 조회가 여러 번 발생할 수 있습니다.
SocialWorkerMatchingService에서 제안된 것과 유사하게, 공고 목록에 대한 지원자 수를 한 번의 쿼리로 가져와서 처리하는 방식으로 리팩토링하는 것을 권장합니다.
// 예시 코드
List<Recruitment> recruitments = recruitmentRepository.findAllByElderly(elderly);
if (recruitments.isEmpty()) {
return List.of();
}
// ApplicationRepository에 추가적인 메서드가 필요합니다.
Map<Long, Long> applicationCounts = applicationRepository.countMapByRecruitmentIn(recruitments);
List<WorkApplication> workApplications = workApplicationRepository.findAllActiveWorkApplication();
List<SocialWorkerRecruitmentResponse> responses = recruitments.stream()
.map(recruitment -> {
long applicationCount = applicationCounts.getOrDefault(recruitment.getId(), 0L);
long matchingCount = workApplications.stream()
.filter(workApplication -> matchingDomainService.isMatched(workApplication, recruitment))
.count();
return SocialWorkerRecruitmentResponse.of(recruitment, applicationCount, matchingCount);
})
.toList();There was a problem hiding this comment.
@gemini-code-assist 제안한 구현 방법은 에러가 납니다. data repository 에서 두개 id 를 select 하면 내부적으로 이를 튜플에 담기 때문에 Map<String, Object[]> 형태로 뱉어냅니다. Map<Long, Long> 형식으로 두면 이를 캐스팅하는 과정에서 에러가 나요.
하지만 수정 방향에는 동의합니다. 별도 이슈에서 수정하겠습니다.
There was a problem hiding this comment.
안녕하세요, kckc0608님. 제안해주신 피드백 감사합니다. Map<Long, Long> 캐스팅 관련하여 지적해주신 부분이 맞습니다. Spring Data JPA의 쿼리 결과 처리 방식에 따라 Map<String, Object[]> 형태로 반환될 수 있으며, 이 경우 직접적인 Map<Long, Long>으로의 캐스팅은 오류를 발생시킬 수 있습니다.N+1 문제를 해결하려는 방향에 동의해주셔서 감사합니다. 별도 이슈로 해당 부분을 개선해주시면 좋을 것 같습니다. 더 효율적인 데이터 조회를 위한 리팩토링은 성능 개선에 큰 도움이 될 것입니다.
작업한 내용
관련 이슈
close #491
참고 사항