-
Notifications
You must be signed in to change notification settings - Fork 0
refactor: 매칭 엔티티를 지원 엔티티로 대체 #475
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
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! 이 Pull Request는 기존의 매칭 시스템을 리팩토링하여 'Matching' 엔티티를 'Application' 엔티티로 대체하는 것을 목표로 합니다. 이 변화는 요양보호사의 구직 지원 및 사회복지사의 근무 제안과 관련된 핵심 로직을 간소화하고, 상태 관리를 더욱 직관적으로 만듭니다. 엔티티 구조 변경에 맞춰 서비스 계층, 컨트롤러, 데이터 접근 계층의 코드도 전반적으로 업데이트되어 시스템의 일관성과 유지보수성을 향상시켰습니다. 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
|
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.
Code Review
매칭(Matching) 엔티티를 지원(Application) 엔티티로 대체하는 대규모 리팩토링을 진행하셨네요. 도메인 모델이 더 명확해진 점이 인상적입니다. 코드 변경 범위가 넓은 만큼, 몇 가지 추가적으로 개선할 수 있는 부분을 발견했습니다. 주로 데이터 무결성, 성능(N+1 쿼리, 비효율적인 데이터 조회), 그리고 새로운 로직의 잠재적인 엣지 케이스에 대한 내용입니다. 자세한 내용은 아래 개별 코멘트를 참고해주세요.
...java/com/becareful/becarefulserver/domain/matching/domain/service/MatchingDomainService.java
Show resolved
Hide resolved
...java/com/becareful/becarefulserver/domain/matching/domain/service/MatchingDomainService.java
Show resolved
Hide resolved
src/main/java/com/becareful/becarefulserver/domain/caregiver/service/CaregiverService.java
Show resolved
Hide resolved
src/main/java/com/becareful/becarefulserver/domain/chat/service/SocialWorkerChatService.java
Show resolved
Hide resolved
| .map(workApplication -> recruitmentRepository.findAll().stream() | ||
| .filter(recruitment -> !matchingDomainService | ||
| .calculateMatchingStatus(workApplication, recruitment) | ||
| .equals(MatchingResultStatus.제외)) | ||
| .map(recruitment -> CaregiverRecruitmentResponse.of( | ||
| recruitment, | ||
| matchingDomainService.calculateMatchingStatus(workApplication, recruitment))) | ||
| .toList()) | ||
| .orElse(null); |
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.
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.
@gemini-code-assist 이 로직도 '매칭 여부' 를 판단하는 로직은 정책과 관련이 있다보니 서비스 계층에서 처리하고 싶어 이렇게 구성하였습니다. 하지만 말씀하신 문제에도 동의하는데 어떻게 리팩토링하면 좋을까요?
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.
안녕하세요, kckc0608님.
recruitmentRepository.findAll()을 호출하여 모든 공고를 메모리로 가져온 후 필터링하는 방식은 공고 수가 많아질 경우 성능 저하와 메모리 문제를 유발할 수 있다는 점에 동의합니다. 매칭 로직(matchingDomainService.calculateMatchingStatus)을 서비스 계층에서 유지하고 싶다는 의견도 이해합니다.
이러한 제약 조건 내에서 성능 문제를 해결하기 위한 가장 효과적인 방법은 **페이지네이션(Pagination)**을 사용하여 데이터베이스에서 공고를 작은 단위로 나누어 가져온 후, 각 페이지에 대해 매칭 로직을 적용하는 것입니다.
제시된 코드 스니펫은 recruitmentRepository.findAll() 대신 recruitmentRepository.findAll(Pageable pageable)을 사용하여 공고를 페이지 단위로 조회하고, 각 페이지의 내용을 스트림으로 처리하여 매칭 로직을 적용합니다. 이렇게 하면 모든 공고를 한 번에 메모리에 로드하는 것을 방지하여 메모리 사용량을 줄이고, 대량의 데이터 처리 시 발생할 수 있는 성능 문제를 완화할 수 있습니다.
적용 시 필요한 import:
java.util.ArrayListorg.springframework.data.domain.Pageorg.springframework.data.domain.PageRequestorg.springframework.data.domain.Pageable
개선된 코드 제안:
| .map(workApplication -> recruitmentRepository.findAll().stream() | |
| .filter(recruitment -> !matchingDomainService | |
| .calculateMatchingStatus(workApplication, recruitment) | |
| .equals(MatchingResultStatus.제외)) | |
| .map(recruitment -> CaregiverRecruitmentResponse.of( | |
| recruitment, | |
| matchingDomainService.calculateMatchingStatus(workApplication, recruitment))) | |
| .toList()) | |
| .orElse(null); | |
| .map(workApplication -> { | |
| java.util.List<com.becareful.becarefulserver.domain.matching.dto.response.CaregiverRecruitmentResponse> matchedRecruitments = new java.util.ArrayList<>(); | |
| int pageNumber = 0; | |
| int pageSize = 100; // 페이지 크기 조정 필요 | |
| org.springframework.data.domain.Pageable pageable = org.springframework.data.domain.PageRequest.of(pageNumber, pageSize); | |
| org.springframework.data.domain.Page<com.becareful.becarefulserver.domain.matching.domain.Recruitment> recruitmentPage; | |
| do { | |
| recruitmentPage = recruitmentRepository.findAll(pageable); | |
| recruitmentPage.getContent().stream() | |
| .filter(recruitment -> matchingDomainService.isMatched(workApplication, recruitment)) | |
| .map(recruitment -> com.becareful.becarefulserver.domain.matching.dto.response.CaregiverRecruitmentResponse.of( | |
| recruitment, | |
| matchingDomainService.calculateMatchingStatus(workApplication, recruitment))) | |
| .forEach(matchedRecruitments::add); | |
| pageable = pageable.next(); | |
| } while (recruitmentPage.hasNext()); | |
| return matchedRecruitments; | |
| }) |
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.
@gemini-code-assist 이 경우, 프론트로부터 요청을 받을 때부터 페이지네이션이 필요하지 않나요?
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.
안녕하세요, kckc0608님. 좋은 질문입니다.
네, 맞습니다. 프론트엔드에서부터 페이지네이션을 적용하는 것이 가장 이상적이고 효율적인 방법입니다.
이전 제안은 recruitmentRepository.findAll()이 모든 공고를 한 번에 메모리에 로드하는 것을 방지하기 위한 서버 내부적인 최적화였습니다. 즉, 데이터베이스에서 공고를 가져올 때 페이지 단위로 가져와 메모리 사용량을 줄이는 데 목적이 있었습니다. 하지만 이 로직은 결국 matchedRecruitments 리스트에 모든 매칭된 공고를 담아 반환하기 때문에, 매칭되는 공고의 수가 매우 많아지면 여전히 메모리 문제나 응답 시간 지연이 발생할 수 있습니다.
따라서, 말씀하신 대로 프론트엔드에서 페이지 번호와 페이지 크기를 요청하고, 백엔드에서는 이 파라미터를 받아 Page<CaregiverRecruitmentResponse>와 같은 형태로 응답하는 것이 전체적인 성능과 사용자 경험을 위해 필요합니다.
이를 위해서는 getCaregiverMatchingRecruitmentList 메서드의 시그니처를 Pageable 객체를 받도록 변경하고, recruitmentRepository에서 Pageable을 사용하여 필터링된 결과를 직접 페이지 단위로 가져오도록 수정해야 합니다. matchingDomainService.isMatched와 같은 필터링 로직을 데이터베이스 쿼리 레벨로 푸시하는 것이 가장 효율적이지만, 이것이 어렵다면 recruitmentRepository.findAll(pageable)로 가져온 페이지의 내용에 대해서만 필터링을 적용하고, 그 결과를 PageImpl로 감싸서 반환하는 방식을 고려할 수 있습니다. 다만 후자의 경우, 한 페이지를 채울 만큼의 매칭된 공고를 찾기 위해 여러 페이지의 원본 공고를 조회해야 할 수도 있어 추가적인 최적화가 필요할 수 있습니다.
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.
@gemini-code-assist 현재 프론트에는 페이지네이션이 없습니다. 기획, 디자인이 변경되어야 하는 상황이니 그 부분을 제외하고 다른 방법이 있는지 알려주세요. 혹시 프론트에서 무한 스크롤을 구현한다면 어떤 식으로 구현하나요?
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.
안녕하세요, kckc0608님. 좋은 질문입니다. 이전 답변에서 제가 제안했던 방식은 서버 내부적으로 모든 공고를 한 번에 메모리에 로드하는 것을 방지하기 위한 최적화였습니다. 즉, 데이터베이스에서 공고를 페이지 단위로 가져와서 매칭 로직을 적용한 후, 그 결과들을 모아서 최종적으로 하나의 리스트로 반환하는 방식입니다. 이 방식은 프론트엔드에서 페이지네이션 요청을 보내지 않아도 서버의 메모리 사용량을 효율적으로 관리할 수 있다는 장점이 있습니다. 따라서 프론트엔드 기획/디자인 변경 없이 서버 내부의 성능 문제를 완화하는 한 가지 방법이 될 수 있습니다. 하지만 말씀하신 대로, 매칭되는 공고의 수가 매우 많다면 최종적으로 프론트엔드로 전달되는 리스트의 크기가 커져 네트워크 부하 및 프론트엔드 렌더링 성능 문제가 발생할 수 있습니다. 프론트엔드에서 무한 스크롤을 구현하는 경우, 백엔드에서는 다음과 같이 구현할 수 있습니다. 1. API 엔드포인트 변경: 현재 getCaregiverMatchingRecruitmentList 메서드가 List<CaregiverRecruitmentResponse>를 반환하지만, 이를 Page<CaregiverRecruitmentResponse>와 같이 페이지 정보를 포함하는 객체를 반환하도록 변경해야 합니다. 2. 요청 파라미터 추가: 프론트엔드에서는 API 요청 시 page (페이지 번호)와 size (페이지당 항목 수) 파라미터를 함께 전송합니다. 3. 쿼리 수정: 백엔드에서는 이 page와 size 파라미터를 받아 recruitmentRepository.findAll(Pageable pageable)과 같은 방식으로 데이터베이스에서 필요한 만큼의 공고만 조회합니다. 4. 매칭 로직 적용: 조회된 페이지 단위의 공고에 대해서만 매칭 로직을 적용하고, 그 결과를 Page 객체로 감싸서 반환합니다. 이 때, PageImpl을 사용하여 매칭된 결과들로 새로운 Page 객체를 생성할 수 있습니다. 무한 스크롤은 사용자 경험 측면에서 초기 로딩 속도를 개선하고, 서버의 부담을 분산시키는 효과적인 방법입니다. 프론트엔드에서 스크롤 이벤트를 감지하여 다음 페이지를 요청하는 방식으로 구현됩니다. 만약 프론트엔드 변경이 불가피하다면, 무한 스크롤은 사용자 경험과 시스템 성능을 동시에 개선할 수 있는 좋은 대안이 될 것입니다.
.../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
src/main/java/com/becareful/becarefulserver/domain/caregiver/service/CaregiverService.java
Outdated
Show resolved
Hide resolved
.../becareful/becarefulserver/domain/matching/dto/response/MatchingCaregiverDetailResponse.java
Show resolved
Hide resolved
| COUNT(a), | ||
| COUNT(a) |
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.
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.
@gemini-code-assist 매칭 카운트는 매칭 적합도 로직을 계산해서 구해야 합니다. 하지만 해당 로직이 비즈니스 계층에 있다보니 해당 로직을 불러오는 방법을 고민하느라 이렇게 임시로 작성했습니다. 좋은 방법이 있을까요?
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.
안녕하세요, matchingCount를 비즈니스 로직으로 계산해야 하는 어려움을 이해합니다. JPQL에서 복잡한 비즈니스 로직을 직접 구현하는 것은 유지보수와 가독성 측면에서 좋지 않습니다. 따라서, matchingCount는 서비스 계층에서 계산하는 것이 더 적절합니다.
현재 JPQL 쿼리에서 SocialWorkerRecruitmentResponse DTO를 직접 생성하고 있기 때문에, matchingCount 필드에 임시로 0L을 할당하고, 실제 값은 서비스 계층에서 조회 후 업데이트하는 방식으로 처리하는 것을 제안합니다.
서비스 계층에서의 처리 흐름 (예시):
RecruitmentRepository에서Recruitment와Elderly, 그리고applicantCount를 포함하는 중간 DTO (예:RecruitmentWithApplicantCountProjection)를 조회합니다. 이때matchingCount는0L로 설정합니다.SocialWorkerMatchingService에서 이 중간 DTO 리스트를 받습니다.- 각
RecruitmentWithApplicantCountProjection에 대해:- 해당
Recruitment에 연결된 모든Application(또는WorkApplication)을 조회합니다. MatchingDomainService의isMatched메서드를 사용하여 각Application이 매칭 적합도 로직에 부합하는지 확인하고, 그 수를matchingCount로 계산합니다.- 계산된
matchingCount와applicantCount를 포함하여 최종SocialWorkerRecruitmentResponseDTO를 생성하여 반환합니다.
- 해당
이렇게 하면 데이터베이스 계층과 비즈니스 로직 계층의 역할을 명확히 분리할 수 있습니다. 당장은 matchingCount에 0L을 할당하는 것으로 쿼리를 수정하고, 서비스 계층 로직을 구현하는 것을 고려해 보세요.
| COUNT(a), | |
| COUNT(a) | |
| 0L, | |
| COUNT(a) |
kckc0608
left a comment
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.
확인해주세요.
.../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
| List<MatchingCaregiverSimpleResponse> matchedCaregivers = | ||
| workApplicationRepository.findAllActiveWorkApplication().stream() | ||
| .filter(workApplication -> !matchingDomainService | ||
| .calculateMatchingStatus(workApplication, recruitment) | ||
| .equals(MatchingResultStatus.제외)) | ||
| .map(workApplication -> { | ||
| String careerTitle = careerRepository | ||
| .findByCaregiver(workApplication.getCaregiver()) | ||
| .map(Career::getTitle) | ||
| .orElse("경력서를 작성하지 않았습니다."); | ||
| MatchingResultStatus result = | ||
| matchingDomainService.calculateMatchingStatus(workApplication, recruitment); | ||
| return MatchingCaregiverSimpleResponse.of(workApplication, result, careerTitle); | ||
| }) | ||
| .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.
@gemini-code-assist 좋은 지적입니다. 그런데 '매칭이 되었는지' 를 판별하는 로직은 정책에 따라 수시로 바뀔 수 있어 서비스 계층에서 관리하고 싶은데, 좋은 리팩토링 방법이 있을까요?
...ain/java/com/becareful/becarefulserver/domain/matching/service/CaregiverMatchingService.java
Show resolved
Hide resolved
| .map(workApplication -> recruitmentRepository.findAll().stream() | ||
| .filter(recruitment -> !matchingDomainService | ||
| .calculateMatchingStatus(workApplication, recruitment) | ||
| .equals(MatchingResultStatus.제외)) | ||
| .map(recruitment -> CaregiverRecruitmentResponse.of( | ||
| recruitment, | ||
| matchingDomainService.calculateMatchingStatus(workApplication, recruitment))) | ||
| .toList()) | ||
| .orElse(null); |
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.
@gemini-code-assist 이 로직도 '매칭 여부' 를 판단하는 로직은 정책과 관련이 있다보니 서비스 계층에서 처리하고 싶어 이렇게 구성하였습니다. 하지만 말씀하신 문제에도 동의하는데 어떻게 리팩토링하면 좋을까요?
| COUNT(a), | ||
| COUNT(a) |
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.
@gemini-code-assist 매칭 카운트는 매칭 적합도 로직을 계산해서 구해야 합니다. 하지만 해당 로직이 비즈니스 계층에 있다보니 해당 로직을 불러오는 방법을 고민하느라 이렇게 임시로 작성했습니다. 좋은 방법이 있을까요?
...java/com/becareful/becarefulserver/domain/matching/domain/service/MatchingDomainService.java
Show resolved
Hide resolved
...java/com/becareful/becarefulserver/domain/matching/domain/service/MatchingDomainService.java
Show resolved
Hide resolved
src/main/java/com/becareful/becarefulserver/domain/chat/service/SocialWorkerChatService.java
Show resolved
Hide resolved
src/main/java/com/becareful/becarefulserver/domain/caregiver/service/CaregiverService.java
Show resolved
Hide resolved
|
/gemini review |
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.
Code Review
매칭 엔티티를 지원 엔티티로 대체하는 대규모 리팩토링을 진행하셨네요. 도메인 모델을 개선하고 기존의 자동 매칭 생성 로직을 제거하여 시스템을 더 명확하고 확장 가능하게 만든 점이 인상적입니다. 전반적으로 좋은 방향의 변경이지만, 몇 가지 중요한 수정이 필요해 보입니다. 특히 엔티티 삭제 시 연관된 데이터 처리가 누락된 부분과, 일부 비즈니스 로직의 잠재적인 버그를 발견했습니다. 아래 리뷰 코멘트에서 자세한 내용을 확인해주세요.
src/main/java/com/becareful/becarefulserver/domain/caregiver/service/CaregiverService.java
Show resolved
Hide resolved
...java/com/becareful/becarefulserver/domain/matching/domain/service/MatchingDomainService.java
Show resolved
Hide resolved
.../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
| long chatRoomId = | ||
| socialWorkerMatchingService.proposeMatching(recruitmentId, caregiver.getId(), LocalDate.now()); | ||
| long chatRoomId = socialWorkerMatchingService.proposeWork(recruitmentId, caregiver.getId(), LocalDate.now()); | ||
| Assertions.assertThat(application.getApplicationStatus()).isEqualTo(ApplicationStatus.근무제안); |
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.
테스트 코드에서 proposeWork 메소드 호출 후 application 객체의 상태를 검증하고 있습니다. 하지만 application 객체는 proposeWork 호출 이전에 조회되었기 때문에 상태 변경이 반영되지 않은 오래된(stale) 객체입니다. 이로 인해 assertThat(application.getApplicationStatus()).isEqualTo(ApplicationStatus.근무제안) 검증이 실패할 가능성이 높습니다. proposeWork 호출 후에 application 객체를 다시 조회하여 최신 상태로 검증해야 합니다.
| Assertions.assertThat(application.getApplicationStatus()).isEqualTo(ApplicationStatus.근무제안); | |
| Assertions.assertThat(applicationRepository.findById(application.getId()).orElseThrow().getApplicationStatus()).isEqualTo(ApplicationStatus.근무제안); |
작업한 내용
관련 이슈
close #456
참고 사항