refactor: application domain refactoring #330
Conversation
- UniversityInfoForApply FK매핑 삭제 및 Long타입으로 변경, 네이밍 변경 - 사용하지 않는 updateUniversityChoice 메서드 제거
- AS-IS: 각 지망별로 Repository를 호출하여 N+1 문제 발생 단일키 임시 캐싱 - TO-BE: 매핑 제거 후 직접 데이터를 분류하여 N+1문제 제거 및 성능 향상 캐싱 제거
- AS-IS: 기존 Repository에서 University타입으로 반환 - TO-BE: Entity변경, 비즈니스 로직 변환에 맞춰 반환타입 Long으로 수정 findByIdsWithUniversityAndLocation 메서드 추가 사용하지 않게 된 getUniversityInfoForApplyByIdAndTerm 메서드 삭제
- 서비스 로직 변경을 위해 3개로 분리되어 있는 기존 메서드를 한 번의 쿼리로 조회하도록 변경
- Entity변경에 따른 수정. 기존 로직은 변화 없음
- Entity변경에 따른 수정 - 전원 통과 - ApplicationQueryService 마지막 메서드 테스트 의도에 맞게 수정
Walkthrough
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
⏰ Context from checks skipped due to timeout of 90000ms (1)
🔇 Additional comments (8)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
src/main/java/com/example/solidconnection/university/repository/UniversityInfoForApplyRepository.java (1)
61-68: 1. 성능 최적화를 위한 bulk fetching 메서드 추가새로운
findByIdsWithUniversityAndLocation메서드가 훌륭하게 구현되었습니다:• join fetch를 통한 연관 엔터티 즉시 로딩으로 N+1 쿼리 문제 해결 • 여러 ID를 한 번에 처리하여 데이터베이스 호출 횟수 최소화 • university, country, region을 모두 fetch join으로 필요한 데이터를 한 번에 로드이 변경으로 기존의 개별 조회 방식보다 훨씬 효율적인 데이터 접근이 가능해졌네요.
더 나은 성능을 위해 추가로 고려해볼 수 있는 개선사항:
@Query(""" select distinct uia from UniversityInfoForApply uia join fetch uia.university u join fetch u.country c join fetch u.region r where uia.id in :ids + order by uia.id """)ID 순서로 정렬을 추가하면 결과의 일관성을 보장할 수 있습니다.
src/main/java/com/example/solidconnection/application/domain/Application.java (2)
56-63: 대학 선택 ID 필드의 null 가능성 명시화 필요현재 대학 선택 필드들이 nullable임에도
@Column어노테이션에 명시되어 있지 않습니다. 명확성을 위해 nullable 속성을 추가하는 것을 권장합니다:-@Column +@Column(nullable = true) private Long firstChoiceUniversityApplyInfoId; -@Column +@Column(nullable = true) private Long secondChoiceUniversityApplyInfoId; -@Column +@Column(nullable = true) private Long thirdChoiceUniversityApplyInfoId;
81-101: 생성자 매개변수 순서 불일치두 번째 생성자에서
updateCount가 대학 선택 ID들보다 먼저 나오는데, 이는 다른 생성자들과 일관성이 없습니다.도메인 로직 순서를 고려하면 대학 선택이 먼저 오고, 그 다음 업데이트 횟수가 오는 것이 자연스럽습니다. 매개변수 순서를 통일하는 것을 고려해보세요.
src/main/java/com/example/solidconnection/application/service/ApplicationQueryService.java (2)
56-62: Stream 처리 시 null 체크 로직 개선 가능현재 null 필터링이 잘 되어 있지만, 더 명확한 코드를 위해 다음과 같이 개선할 수 있습니다:
List<Long> universityIds = Stream.of( userLatestApplication.getFirstChoiceUniversityApplyInfoId(), userLatestApplication.getSecondChoiceUniversityApplyInfoId(), userLatestApplication.getThirdChoiceUniversityApplyInfoId() ) .filter(Objects::nonNull) .distinct() // 중복 제거 추가 .collect(Collectors.toList());동일한 대학에 중복 지원하는 경우를 고려하여
distinct()를 추가하면 불필요한 쿼리를 더 줄일 수 있습니다.
74-119: 지원서 분류 로직의 효율적인 구현 ✨Map을 활용한 지원서 분류와 스트림을 이용한 DTO 변환이 효율적으로 구현되었습니다.
다만 가독성 향상을 위해 각 선택 순위별 처리를 별도 메서드로 분리하는 것을 고려해보세요:
private List<UniversityApplicantsResponse> createChoiceApplicants( List<UniversityInfoForApply> universityInfos, Map<Long, List<Application>> choiceMap, SiteUser siteUser) { return universityInfos.stream() .map(uia -> UniversityApplicantsResponse.of( uia, choiceMap.getOrDefault(uia.getId(), List.of()).stream() .map(ap -> ApplicantResponse.of(ap, Objects.equals(siteUser.getId(), ap.getSiteUser().getId()))) .toList())) .toList(); }이렇게 하면 중복 코드를 줄이고 유지보수성을 높일 수 있습니다.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
src/main/java/com/example/solidconnection/application/domain/Application.java(3 hunks)src/main/java/com/example/solidconnection/application/repository/ApplicationRepository.java(1 hunks)src/main/java/com/example/solidconnection/application/service/ApplicationQueryService.java(2 hunks)src/main/java/com/example/solidconnection/application/service/ApplicationSubmissionService.java(2 hunks)src/main/java/com/example/solidconnection/university/repository/UniversityInfoForApplyRepository.java(1 hunks)src/main/java/com/example/solidconnection/university/repository/custom/UniversityFilterRepository.java(1 hunks)src/main/java/com/example/solidconnection/university/repository/custom/UniversityFilterRepositoryImpl.java(1 hunks)src/test/java/com/example/solidconnection/application/fixture/ApplicationFixture.java(1 hunks)src/test/java/com/example/solidconnection/application/fixture/ApplicationFixtureBuilder.java(3 hunks)src/test/java/com/example/solidconnection/application/service/ApplicationQueryServiceTest.java(17 hunks)src/test/java/com/example/solidconnection/application/service/ApplicationSubmissionServiceTest.java(1 hunks)src/test/java/com/example/solidconnection/support/integration/BaseIntegrationTest.java(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (11)
src/test/java/com/example/solidconnection/application/service/ApplicationSubmissionServiceTest.java (1)
89-101: 테스트 어설션이 새로운 엔터티 구조에 맞게 올바르게 업데이트되었습니다.리팩토링 변경사항과 일치하는 테스트 수정사항들:
- 직접 ID 필드 접근으로 변경:
getFirstChoiceUniversityApplyInfoId(),getSecondChoiceUniversityApplyInfoId(),getThirdChoiceUniversityApplyInfoId()메서드를 사용하여 중첩된 엔터티 참조 대신 직접 ID 값을 확인- 가독성 향상: 어설션 포맷팅이 개선되어 각 검증 항목이 명확하게 구분됨
- 논리적 일관성 유지: 지원서 상태, 업데이트 횟수, 삭제 플래그 등 기존 검증 로직은 그대로 유지
src/main/java/com/example/solidconnection/university/repository/custom/UniversityFilterRepository.java (1)
11-11:✅ Verification successful
성능 최적화를 위한 반환 타입 변경이 적절합니다.
메서드 시그니처 변경사항:
- 반환 타입 최적화:
List<University>에서List<Long>으로 변경하여 메모리 사용량과 데이터 전송량 감소- 쿼리 성능 향상: 전체 엔터티 대신 ID만 반환하여 불필요한 데이터 로딩 방지
- 리팩토링 목표 달성: JPA 프록시 오버헤드 제거 및 N+1 쿼리 문제 해결에 기여
이 메서드를 호출하는 모든 곳에서
LongID를 올바르게 처리하는지 확인해보겠습니다:
🏁 Script executed:
#!/bin/bash # Description: 메서드 호출자들이 새로운 반환 타입을 올바르게 처리하는지 확인 # Test: findByRegionCodeAndKeywords 메서드 사용처 검색 rg -A 10 "findByRegionCodeAndKeywords" --type javaLength of output: 8346
메서드 호출 검증 완료
변경된
findByRegionCodeAndKeywords메서드 호출 처리 상태를 점검했습니다:
- ApplicationQueryService
List<Long> universityIds = universityFilterRepository.findByRegionCodeAndKeywords(...)호출부에서 반환된 Long ID를 정상 처리합니다.- ApplicationQueryService 하위 로직
- 조회된 ID로
applicationRepository.findApplicationsForChoices및universityInfoForApplyRepository.findByIdsWithUniversityAndLocation호출이 문제없이 동작합니다.- UniversityQueryServiceTest
- 해당 테스트는
findByRegionCodeAndKeywordsAndLanguageTestTypeAndTestScoreAndTerm메서드를 검증하며 변경 대상 메서드에는 영향이 없습니다.- 기타 호출부 없음
- 리포지토리 내
findByRegionCodeAndKeywords는 위 외 추가 호출 지점이 발견되지 않았습니다.모든 호출자가 Long ID를 올바르게 처리하고 있어 변경 사항을 승인합니다.
src/test/java/com/example/solidconnection/support/integration/BaseIntegrationTest.java (1)
427-445: 통합 테스트 설정이 새로운 Application 엔터티 구조에 맞게 일관되게 업데이트되었습니다.테스트 데이터 설정 변경사항들:
- 생성자 파라미터 변경:
UniversityInfoForApply객체 대신LongID 값들을 전달하도록 수정- 일관된 업데이트: 모든 Application 인스턴스가 동일한 패턴으로 업데이트됨
테스트유저_2_괌대학_B_괌대학_A_린츠_카톨릭대학_지원서: 2L, 1L, 15L테스트유저_3_괌대학_A_괌대학_B_그라츠공과대학_지원서: 1L, 2L, 14L테스트유저_4_메이지대학_그라츠대학_서던덴마크대학_지원서: 18L, 13L, 9L- 기타 테스트 케이스들도 동일한 패턴으로 적용
- 예측 가능한 테스트 데이터: 하드코딩된 ID 값들로 안정적인 통합 테스트 환경 구성
src/main/java/com/example/solidconnection/university/repository/custom/UniversityFilterRepositoryImpl.java (1)
31-46: 인터페이스 변경에 맞는 구현체 수정이 올바르게 완료되었습니다.쿼리 최적화 변경사항들:
- 선택 필드 최적화:
.select(university.id)로 변경하여 필요한 ID만 조회- 쿼리 구조 개선:
.selectFrom(university)에서.from(university)로 변경하여 명시적인 선택 필드 지정- 중복 제거 추가:
.distinct()호출로 조인으로 인한 중복 ID 방지- 필터링 로직 유지: 지역 코드와 키워드 기반 필터링 로직은 그대로 유지하여 기능적 일관성 보장
- 가독성 향상: where 절 포맷팅 개선으로 조건문 구조가 더 명확해짐
이러한 변경사항들은 전체 엔터티를 로딩하지 않고도 필요한 대학 ID만 효율적으로 조회할 수 있게 해줍니다.
src/main/java/com/example/solidconnection/application/service/ApplicationSubmissionService.java (2)
53-56: 1. 대학 선택 정보 처리 방식 개선 - ID 기반 접근으로 전환UniversityInfoForApply 엔터티를 직접 조회하는 대신 ID만을 사용하도록 변경된 점이 좋습니다. 이 변경으로 다음과 같은 이점을 얻을 수 있습니다:
• JPA 프록시 객체 생성 오버헤드 제거 • 불필요한 연관 엔터티 조회 방지 • 메모리 사용량 최적화
66-76:✅ Verification successful
2. Application 생성자 호출 방식 업데이트
새로운 Application 생성자가 university ID들을 직접 받도록 변경되어 리팩토링 목표와 일치합니다. 코드가 더 간결해지고 엔터티 의존성이 줄어들었네요.
Application 엔터티의 생성자가 올바르게 변경되었는지 확인해보겠습니다:
🏁 Script executed:
#!/bin/bash # Description: Application 엔터티의 생성자 시그니처 확인 # Application 엔터티의 생성자를 확인하여 Long 타입 파라미터를 받는지 검증 ast-grep --pattern $'class Application { $$$ Application($$$, Long $_, Long $_, Long $_, $$$) { $$$ } $$$ }'Length of output: 121
아래 스크립트를 실행해서 Application 엔티티의 생성자 시그니처를 직접 확인해 주세요:
🏁 Script executed:
#!/bin/bash # 1) Application.java 파일 경로 찾기 APP_FILE=$(find src/main/java -name Application.java) echo "Application 엔터티 파일: $APP_FILE" # 2) 생성자 정의 추출 grep -En 'public Application\(' "$APP_FILE"Length of output: 532
아래 스크립트를 실행해서
Application클래스의 각 생성자 시그니처를 한눈에 확인해 주세요:
🏁 Script executed:
#!/bin/bash # Application.java 경로 찾기 APP_FILE=$(find src/main/java -name Application.java) echo "📄 Application 엔티티 파일: $APP_FILE" # 60~110라인 출력하여 모든 생성자 시그니처 확인 echo "🔍 생성자 정의 전체 보기 (라인 60~110):" sed -n '60,110p' "$APP_FILE"Length of output: 2204
✅ Application 생성자 시그니처 정상 확인
교차 검증 결과, 새로운 생성자가 기대한 파라미터를 정확히 수용함을 확인했습니다.
- 생성자 파라미터 구성 검증
src/main/java/com/example/solidconnection/application/domain/Application.java81~89라인에서
SiteUser, Gpa, LanguageTest, String term, Integer updateCount, Long firstChoiceUniversityApplyInfoId, Long secondChoiceUniversityApplyInfoId, Long thirdChoiceUniversityApplyInfoId, String nicknameForApply순으로 정의된 생성자를 발견했습니다.- 리팩토링 목표 일치
- 세 개의 Long 타입 대학 선택 ID를 직접 받도록 변경되어 호출부가 깔끔해졌고, 엔터티 의존성이 줄었습니다.
src/test/java/com/example/solidconnection/application/fixture/ApplicationFixture.java (1)
17-37: 1. 테스트 픽스처의 일관된 리팩토링 적용테스트 픽스처가 프로덕션 코드의 변경사항과 완벽하게 일치하도록 업데이트되었습니다:
• 파라미터 타입을 UniversityInfoForApply → Long ID로 변경 • builder 메서드 호출도 ID 기반으로 수정 • 테스트와 실제 코드 간의 일관성 유지이런 식으로 테스트 코드를 함께 업데이트해주신 것이 매우 좋습니다. 리팩토링 시 테스트 코드도 함께 관리하는 것이 중요하죠.
src/main/java/com/example/solidconnection/application/repository/ApplicationRepository.java (1)
23-37:✅ Verification successful
1. 효율적인 통합 쿼리 메서드로 성능 대폭 개선
기존의 3개 개별 쿼리를 하나로 통합한
findApplicationsForChoices메서드가 매우 잘 설계되었습니다:• 첫 번째, 두 번째, 세 번째 선택을 OR 조건으로 한 번에 처리 • join fetch를 통한 siteUser 연관 엔터티 미리 로딩 • List<Long> 파라미터로 여러 대학을 효율적으로 처리 • 데이터베이스 호출 횟수를 3분의 1로 감소이 변경으로 N+1 쿼리 문제가 해결되고 전반적인 성능이 크게 향상될 것으로 예상됩니다.
새로운 쿼리 메서드가 기존 서비스 로직에서 올바르게 사용되고 있는지 확인해보겠습니다:
🏁 Script executed:
#!/bin/bash # Description: 새로운 findApplicationsForChoices 메서드 사용처 확인 # ApplicationQueryService에서 이 메서드를 사용하는지 확인 rg -A 10 "findApplicationsForChoices" --type java # 기존의 개별 쿼리 메서드 호출이 남아있지 않은지 확인 rg "findApplicationsByFirstChoice|findApplicationsBySecondChoice|findApplicationsByThirdChoice" --type javaLength of output: 4647
새 통합 쿼리 메서드 사용 및 잔여 메서드 제거 검증 완료
- 효율적인 쿼리 메서드 사용 위치 확인
ApplicationQueryService.java에서findApplicationsForChoices가 정상 호출됩니다.- 기존 개별 쿼리 메서드 제거 확인
findApplicationsByFirstChoice,findApplicationsBySecondChoice,findApplicationsByThirdChoice호출 흔적이 없습니다.- 최종 승인
- N+1 문제 해결 및 성능이 대폭 개선되었습니다.
src/test/java/com/example/solidconnection/application/service/ApplicationQueryServiceTest.java (1)
412-446: 개선된 경쟁자 목록 검증 로직 👍이전의 단순한
containsExactly검증에서 더 세밀한 검증으로 개선되었습니다:
- 각 지원 순위별로 분리하여 검증
- 빈 지원자 목록 필터링
isMine플래그 정확성 확인이러한 상세한 검증은 테스트의 신뢰성을 높여줍니다.
src/test/java/com/example/solidconnection/application/fixture/ApplicationFixtureBuilder.java (1)
21-23: 테스트 픽스처 빌더 리팩토링 완료 ✅도메인 모델의 변경사항에 따라 테스트 픽스처가 적절히 수정되었습니다:
- 필드 타입 변경: UniversityInfoForApply → Long ID
- 메서드 시그니처 업데이트: setter 메서드들이 ID를 받도록 변경
- 생성자 호출 수정: Application 생성 시 ID 사용
변경사항이 일관성 있게 적용되었습니다.
Also applies to: 42-55, 78-80
src/main/java/com/example/solidconnection/application/service/ApplicationQueryService.java (1)
38-50: 쿼리 최적화 및 벌크 조회 도입 👏기존의 여러 번의 쿼리 실행을 3번의 효율적인 쿼리로 줄인 훌륭한 리팩토링입니다:
- 대학 ID 필터링: 지역과 키워드로 한 번에 필터링
- 지원서 벌크 조회: 모든 관련 지원서를 한 번에 조회
- 대학 정보 벌크 조회: 필요한 대학 정보를 한 번에 조회
N+1 문제를 효과적으로 해결했습니다.
src/test/java/com/example/solidconnection/application/service/ApplicationQueryServiceTest.java
Outdated
<
8000
span class="btn-link color-fg-muted f6 Details-content--closed">Show resolved
Hide resolved
- UniversityInfoForApplyId 하드코딩 되어있는 부분 수정 - 테스트 통과 확인
There was a problem hiding this comment.
수연님 우선, 1차 리뷰 드립니다~
리뷰하다보니 너무 길어져서요😅 끊어서 리뷰해보도록 할게요!
그리고, 저런 코드들을 리팩터링하게 해서 정말 죄송하다는 말씀을 드립니다..
저 코드를 보아하니 그때 마침 제가 함수형 인터페이스를 새로 배워서 몹시 신이 난 상태였던 것 같습니다.
그래도 그렇지 저런 코드를 만들다니… 정말 죄송합니다 🫠
그럼에도 짧은 시간안에 도메인을 이해하시고, 우리 팀의 PR 형식에 맞게 수정해주셔서 감사합니다🥹
[컨벤션] 표시한 것은, 우리 팀에 원래 있던 컨벤션입니다.
저는 팀은 하나의 컨벤션을 따라야한다는 주의입니다.
제가 좋아하는 책의 구절을 인용합니다!
팀이나 조직 전체가 같은 코딩 스타일을 따르면, 그것은 마치 모두가 같은 언어를 유창하게 말하는 것과 비슷하다. 서로 오해할 위험이 크게 줄어들고, 이로 인해 버그가 줄어들고, 혼란스러운 코드를 이해하는 데 낭비하는 시간이 줄어든다.
‘왜 이런 컨벤션을 따라야하는지 이해가 되지 않는다’하는 부분이 있으시면 말씀해주세요~
src/main/java/com/example/solidconnection/application/domain/Application.java
Outdated
Show resolved
Hide resolved
src/main/java/com/example/solidconnection/application/domain/Application.java
Show resolved
Hide resolved
src/main/java/com/example/solidconnection/application/service/ApplicationQueryService.java
Outdated
Show resolved
Hide resolved
src/main/java/com/example/solidconnection/application/service/ApplicationQueryService.java
Outdated
Show resolved
Hide resolved
src/main/java/com/example/solidconnection/application/service/ApplicationQueryService.java
Outdated
Show resolved
Hide resolved
src/test/java/com/example/solidconnection/application/service/ApplicationQueryServiceTest.java
Outdated
Show resolved
Hide resolved
src/test/java/com/example/solidconnection/application/service/ApplicationQueryServiceTest.java
Outdated
Show resolved
Hide resolved
src/test/java/com/example/solidconnection/application/service/ApplicationQueryServiceTest.java
Outdated
Show resolved
Hide resolved
src/test/java/com/example/solidconnection/application/service/ApplicationQueryServiceTest.java
Outdated
Show resolved
Hide resolved
| assertThat(response.firstChoice()) | ||
| .filteredOn(uar -> !uar.applicants().isEmpty()) | ||
| .containsExactly( | ||
| UniversityApplicantsResponse.of( | ||
| universityInfoForApplyFixture.괌대학_A_지원_정보(), | ||
| List.of( | ||
| ApplicantResponse.of(application1, true), | ||
| ApplicantResponse.of(application2, false), | ||
| ApplicantResponse.of(application3, false) | ||
| ) | ||
| ) | ||
| ); | ||
|
|
||
| assertThat(response.secondChoice()) | ||
| .filteredOn(uar -> !uar.applicants().isEmpty()) | ||
| .containsExactly( | ||
| UniversityApplicantsResponse.of( | ||
| universityInfoForApplyFixture.괌대학_B_지원_정보(), | ||
| List.of( | ||
| ApplicantResponse.of(application1, true), | ||
| ApplicantResponse.of(application2, false) | ||
| ) | ||
| ) | ||
| ); | ||
|
|
||
| assertThat(response.thirdChoice()) | ||
| .filteredOn(uar -> !uar.applicants().isEmpty()) | ||
| .containsExactly( | ||
| UniversityApplicantsResponse.of( | ||
| universityInfoForApplyFixture.서던덴마크대학교_지원_정보(), | ||
| List.of( | ||
| ApplicantResponse.of(application1, true) | ||
| ) | ||
| ) | ||
| ); |
There was a problem hiding this comment.
이 테스트코드의 목적은, 이번_학기_지원한_대학_중_미선택이_있을_때_경쟁자_목록을_조회하는 것입니다.
즉,
- 선택하지 않은 대학교에 대한 정보는 보이지 않아야한다.
- 다른 사용자가 다른 대학에 지원한 상태에서도, 본인이 선택한 대학에 대한 것만 보인다.
는 것을 검증하기 위한 코드입니다.
(지금보니 테스트 이름이 목적을 부족하게 설명하긴 했네요 😓)
그런데 테스트 코드의 given 절과 then 절이 바뀜으로써, 테스트 코드의 목적이 달라졌습니다.
원래 user1은 괌A만 선택했기에, 괌A에 대한 검증만 했습니다.
하지만 지금은 세개 대학을 선택했기에, 모두 다 보이네요..!
그리고 세개의 지망을 다 선택해서 테스트 함수의 이름과도 호응이 되지 않게 되었습니다.
테스트의 목적에 맞게 코드를 수정하는게 좋아보입니다!
| for (Application a : applications) { | ||
| if (a.getFirstChoiceUniversityApplyInfoId() != null) { | ||
| firstChoiceMap.computeIfAbsent(a.getFirstChoiceUniversityApplyInfoId(), k -> new ArrayList<>()).add(a); | ||
| } | ||
| if (a.getSecondChoiceUniversityApplyInfoId() != null) { | ||
| secondChoiceMap.computeIfAbsent(a.getSecondChoiceUniversityApplyInfoId(), k -> new ArrayList<>()).add(a); | ||
| } | ||
| if (a.getThirdChoiceUniversityApplyInfoId() != null) { | ||
| thirdChoiceMap.computeIfAbsent(a.getThirdChoiceUniversityApplyInfoId(), k -> new ArrayList<>()).add(a); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
a 말고 application으로 그냥 명확히 하는 건 어떤가요?
if (a.getFirstChoiceUniversityApplyInfoId() != null) {
firstChoiceMap.computeIfAbsent(a.getFirstChoiceUniversityApplyInfoId(), k -> new ArrayList<>()).add(a);
}
똑같은 것을 first, second, third냐에 따라만 다르게 호출하니 함수로 빼도 좋을 거 같네요!
| List<UniversityApplicantsResponse> firstChoiceApplicants = universityInfos.stream() | ||
| .map(uia -> UniversityApplicantsResponse.of( | ||
| uia, | ||
| firstChoiceMap.getOrDefault(uia.getId(), List.of()).stream() | ||
| .map(ap -> ApplicantResponse.of(ap, Objects.equals(siteUser.getId(), ap.getSiteUser().getId()))) | ||
| .toList())) | ||
| .toList(); | ||
|
|
There was a problem hiding this comment.
여기도 함수로 빼도 좋을 거 같네요!
지망별로 application을 fetch join으로 가져오면 5번의 쿼리로 분류 필요 없이 응답을 할 수 있는데 지금은 3번의 쿼리로 메모리에서 분류를 하고 있으니 어느정도의 트레이드 오프가 있을지 궁금하네요 ㅎㅎ 성능은 지금 방식이 당연히 더 좋을 거 같습니다!
There was a problem hiding this comment.
이 부분도 반영했습니다!
추가로 성능에 대한 부분은 비교를 한 번 해봐야 할 것 같긴 하네요!
| where uia.id in :ids | ||
| """) | ||
| List<UniversityInfoForApply> findByIdsWithUniversityAndLocation(@Param("ids") List<Long> ids); | ||
|
|
| join fetch u.country c | ||
| join fetch u.region r | ||
| where uia.id in :ids | ||
| """) |
There was a problem hiding this comment.
[컨벤션] """)를 혹시 일부로 왼쪽에 붙여두신건가요? 쿼리 블록들이 조금 어색하게 느껴진 거 같아요
@nayonsoso 이것도 뭔가 정해야할까요?
There was a problem hiding this comment.
일부러는 아니고 실수 입니다! 비슷한 부분 컨벤션 통일하여 반영하였습니다.
- 코드 컨벤션 통일 - 변수명 이해하기 쉽도록 수정 - Application Repository universityId기반과 uiaId기반으로 분기
- 마지막 메서드 테스트 의도에 맞게 수정 - 컨벤션에 맞게 수정 - BaseIntegrationTest에서 firstUIAId에 null값을 입력하는 부분 수정
- 조회 성능 향상을 위해 Application Entity Index추가 - Application Repository 불필요한 공백 삭제
- Application FK제약조건 제거 - column명 변경 - Index 추가
| @Column(nullable = false) | ||
| private Long firstChoiceUniversityApplyInfoId; | ||
|
|
||
| @ManyToOne(fetch = FetchType.LAZY) | ||
| private UniversityInfoForApply secondChoiceUniversity; | ||
| @Column | ||
| private Long secondChoiceUniversityApplyInfoId; | ||
|
|
||
| @ManyToOne(fetch = FetchType.LAZY) | ||
| private UniversityInfoForApply thirdChoiceUniversity; | ||
| @Column | ||
| private Long thirdChoiceUniversityApplyInfoId; |
There was a problem hiding this comment.
두 가지 변경이 있으면 좋겠는데요!
1/ 컬럼명 변경 (flyway 스크립트도 추가)
first_choice_university_id가 실제로는 universityInfoForApply 엔티티를 참조하고 있지만, 이름만 보면 university를 참조하는 것처럼 오해할 수 있는 것 같아요.
그래서 컬럼명을 first_choice_university_info_for_apply_id로 변경하는건 어떤가요?
2/ 엔티티 필드명 변경 (@Column(name=사용)
오늘 회의에서 정한대로, universityInfoForApply를 UnivApplyInfo로 바꾸기로 했으니 이 필드명도 firstChoiceUnivApplyInfoId로 바꾸는건 어떤가요?
@Column(nullable = false , name = "first_choice_university_info_for_apply_id")
private long firstChoiceUnivApplyInfoId;
@Column(name = "second_choice_university_info_for_apply_id")
private Long secondChoiceUnivApplyInfoId;
@Column(name = "third_choice_university_info_for_apply_id")
private Long thirdChoiceUnivApplyInfoId;⬆️ 이렇게요!
| UniversityInfoForApply firstChoiceUniversity, | ||
| UniversityInfoForApply secondChoiceUniversity, | ||
| UniversityInfoForApply thirdChoiceUniversity, | ||
| Long firstChoiceUniversityApplyInfoId, |
| UniversityInfoForApply firstChoiceUniversity, | ||
| UniversityInfoForApply secondChoiceUniversity, | ||
| UniversityInfoForApply thirdChoiceUniversity, | ||
| Long firstChoiceUniversityApplyInfoId, |
| AND a.term = :term | ||
| AND a.isDelete = false | ||
| """) | ||
| SELECT a |
There was a problem hiding this comment.
| SELECT a | |
| SELECT a |
| @Query(""" | ||
| SELECT DISTINCT uifa | ||
| FROM UniversityInfoForApply uifa | ||
| JOIN FETCH uifa.university u | ||
| JOIN FETCH u.country c | ||
| JOIN FETCH u.region r | ||
| WHERE u.id IN :universityIds | ||
| """) | ||
| List<UniversityInfoForApply> findByUniversityIdsWithUniversityAndLocation(@Param("universityIds") List<Long> universityIds); |
There was a problem hiding this comment.
이부분은 'country와 region을 select해오는 쿼리가 추가로 나가는 것을 방지하기 위해'
fetch join 을 하신 것 같아요.
그런데 함수의 이름에서 이를 드러내지는 않아도 될 것 같아요.
findAllByUniversityIds로 이름을 변경해도 괜찮을 것 같은데, 수연님은 어떻게 생각하세요?
| // 1. 대학 ID 필터링 (regionCode, keyword) | ||
| List<Long> universityIds = universityFilterRepository.findByRegionCodeAndKeywords(regionCode, List.of(keyword)); | ||
| if (universityIds.isEmpty()) { | ||
| return new ApplicationsResponse(List.of(), List.of(), List.of()); | ||
| } | ||
|
|
||
| // 2. 조건에 맞는 모든 Application 한 번에 조회 | ||
| List<Application> applications = applicationRepository.findApplicationsByUniversityChoices(universityIds, VerifyStatus.APPROVED, term); | ||
|
|
||
| // 3. 대학정보 조회 | ||
| List<UniversityInfoForApply> universityInfosForApply = universityInfoForApplyRepository.findByUniversityIdsWithUniversityAndLocation(universityIds); | ||
|
|
||
| // 4. 지원서 분류 및 DTO 변환 | ||
| return classifyApplicationsByChoice(universityInfosForApply, applications, siteUser); |
There was a problem hiding this comment.
지금은 query dsl로 만든 레포지토리에서 university로 필티링하고,
그 필터링된 universityIds로 universityInfoForApply 를 조회하고 있는데요!
query dsl로 만든 레포지토리에서 universityInfoForApply 자체를 반환하게 하는건 어떠신가요?
그럼 쿼리가 하나 줄어듭니다!
// ApplicationQueryService.java
// 1. 대학 지원 정보 필터링 (regionCode, keyword)
List<UniversityInfoForApply> univApplyInfos = universityFilterRepository.findByRegionCodeAndKeywords(regionCode, List.of(keyword));
if (univApplyInfos.isEmpty()) {
return new ApplicationsResponse(List.of(), List.of(), List.of());
}
// 2. 조건에 맞는 모든 Application 한 번에 조회
List<Long> univApplyInfoIds = univApplyInfos.stream()
.map(UniversityInfoForApply::getId)
.toList();
List<Application> applications = applicationRepository.findApplicationsByUniversityChoices(univApplyInfoIds, VerifyStatus.APPROVED, term);
// 3. 지원서 분류 및 DTO 변환
return classifyApplicationsByChoice(univApplyInfos, applications, siteUser);// UniversityFilterRepositoryImpl.java
@Override
public List<UniversityInfoForApply> findByRegionCodeAndKeywords(String regionCode, List<String> keywords) {
QUniversityInfoForApply universityInfoForApply = QUniversityInfoForApply.universityInfoForApply;
QUniversity university = QUniversity.university;
QCountry country = QCountry.country;
QRegion region = QRegion.region;
return queryFactory
.selectFrom(universityInfoForApply)
.join(university.country, country).fetchJoin() // ❗️여기서 region, country fetch join
.join(country.region, region).fetchJoin()
.where(
regionCodeEq(region, regionCode)
.and(countryOrUniversityContainsKeyword(country, university, keywords))
)
.distinct()
.fetch();
}| private List<UniversityApplicantsResponse> createUniversityApplicantsResponses( | ||
| List<UniversityInfoForApply> universityInfosForApply, | ||
| Map<Long, List<Application>> choiceMap, | ||
| SiteUser siteUser) { | ||
| return universityInfosForApply.stream() | ||
| .map(uia -> UniversityApplicantsResponse.of( | ||
| uia, | ||
| choiceMap.getOrDefault(uia.getId(), List.of()).stream() | ||
| .map(ap -> ApplicantResponse.of(ap, Objects.equals(siteUser.getId(), ap.getSiteUser().getId()))) |
There was a problem hiding this comment.
이 서비스 코드에서는 엔티티를 DTO 형태로 매핑하는 작업을 DTO 쪽에서 이미 맡고 있습니다.
그렇다면 매핑 책임을 전적으로 DTO 쪽에 일관되게 위임해보는 건 어떨까요?
private List<UniversityApplicantsResponse> createUniversityApplicantsResponses(
List<UniversityInfoForApply> universityInfosForApply,
Map<Long, List<Application>> choiceMap,
SiteUser siteUser) {
return universityInfosForApply.stream()
.map(uia -> UniversityApplicantsResponse.of(uia, choiceMap.getOrDefault(uia.getId(), List.of()), siteUser))
.toList();
}// UniversityApplicantsResponse.java
public static UniversityApplicantsResponse of(UniversityInfoForApply universityInfoForApply, List<Application> applications, SiteUser siteUser) {
return new UniversityApplicantsResponse(
universityInfoForApply.getKoreanName(),
universityInfoForApply.getStudentCapacity(),
universityInfoForApply.getUniversity().getRegion().getKoreanName(),
universityInfoForApply.getUniversity().getCountry().getKoreanName(),
applications.stream()
.map(application -> ApplicantResponse.of(application, isUsers(application, siteUser)))
.toList());
}
private static boolean isUsers(Application application, SiteUser siteUser) {
return application.getSiteUser().getId().equals(siteUser.getId());
}이렇게 하면 서비스 코드가 훨씬 깔끔해지고, 책임 분리가 명확해져서 유지보수에도 도움이 될 것 같아요.
검토해보시면 좋을 것 같습니다! 😊
| UniversityInfoForApply thirdChoiceUniversity = Optional.ofNullable(universityChoiceRequest.thirdChoiceUniversityId()) | ||
| .map(id -> universityInfoForApplyRepository.getUniversityInfoForApplyByIdAndTerm(id, term)) | ||
| .orElse(null); | ||
| Long firstChoiceUniversityId = universityChoiceRequest.firstChoiceUniversityId(); |
| WHERE u.id IN :universityIds | ||
| """) | ||
| List<UniversityInfoForApply> findByUniversityIdsWithUniversityAndLocation(@Param("universityIds") List<Long> universityIds); | ||
|
|
| @@ -71,6 +71,7 @@ class ApplicationQueryServiceTest { | |||
| private LanguageTestScore languageTestScore2; | |||
| private LanguageTestScore languageTestScore3; | |||
|
|
|||
|
|
|||
| ALTER TABLE application DROP FOREIGN KEY FKeajojvwgn069mfxhbq5ja1sws; | ||
| ALTER TABLE application DROP FOREIGN KEY FKepp2by7frnkt1o1w3v4t4lgtu; | ||
| ALTER TABLE application DROP FOREIGN KEY FKi822ljuirbu9o0lnd9jt7l7qg; |
There was a problem hiding this comment.
이 FK는 왜 없애시는건가요? 논의된게 있었나요? (제가 기억하지 못하는걸수도 있습니다 😅)
관련 이슈
작업 내용
📌기존 코드의 문제점
기존 Entity는 UniversityInfoForApply를 @ ManyToOne 연관관계로 직접 참조하고 있었습니다.
1. 불필요한 연관관계 관리
2. N+1 위험
📌개선 방향
1. Entity수정
2. 비즈니스 로직 수정
📌기타 변경사항
특이 사항
DB스키마가 변경되었습니다. 확인해주시기 바랍니다.
리뷰 요구사항 (선택)
불필요한 변경이나 삭제가 없었는지 확인해주시면 감사하겠습니다.
또한 성능 개선의 여지가 더 있는지, 문제가 발생할 부분이 있는지 그리고 테스트 코드가 의도된 바와 같은지 확인해주시면 감사하겠습니다.