refactor: 대학 검색 응답 수정 #624
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Walkthrough
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 분 Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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. Comment |
whqtker
requested review from
Gyuhyeok99,
Hexeong,
JAEHEE25,
lsy1307,
sukangpunch and
wibaek
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
`@src/main/java/com/example/solidconnection/university/service/LikedUnivApplyInfoService.java`:
- Around line 61-69: In getHomeUniversityMap, avoid calling
homeUniversityRepository.findAllByIdIn when homeUniversityIds is empty: after
computing homeUniversityIds (from UnivApplyInfo::getHomeUniversityId), if the
list is empty return an empty immutable map (Map.of()) immediately; otherwise
proceed to call homeUniversityRepository.findAllByIdIn(...) and collect to a map
with HomeUniversity::getId and Function.identity().
In
`@src/main/java/com/example/solidconnection/university/service/UnivApplyInfoRecommendService.java`:
- Around line 100-108: getHomeUniversityMap may call the repository with an
empty homeUniversityIds list causing an unnecessary query or an empty IN clause
error; add a guard: after computing homeUniversityIds (variable name) check if
it's empty and if so return Map.of() immediately, otherwise proceed to call
homeUniversityRepository.findAllByIdIn(...) and collect to the map as before.
src/main/java/com/example/solidconnection/university/service/LikedUnivApplyInfoService.java
Outdated
Show resolved
Hide resolved
src/main/java/com/example/solidconnection/university/service/UnivApplyInfoRecommendService.java
Outdated
Show resolved
Hide resolved
- N+1 방지를 위해 fetch join도 추가
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/com/example/solidconnection/university/service/LikedUnivApplyInfoService.java (1)
36-50:homeUniversityfetch join 누락으로 인한 N+1 문제 발생.현재 코드에서 다음과 같은 성능 문제가 확인됩니다:
findUnivApplyInfosBySiteUserId쿼리가 fetch join 없이 일반 JOIN만 수행하고 있습니다.UnivApplyInfoPreviewResponse.of()메서드가univApplyInfo.getHomeUniversity().getName()을 호출하여 homeUniversity 관계를 접근합니다.UnivApplyInfo.homeUniversity는 LAZY 로딩으로 설정되어 있어, 반복된 쿼리가 발생합니다.동일한 엔티티를 다루는
UnivApplyInfoFilterRepositoryImpl의 다른 메서드들은 이미.leftJoin(univApplyInfo.homeUniversity, homeUniversity).fetchJoin()으로 올바르게 구현되어 있습니다. 동일한 패턴을 적용해 주세요.
🧹 Nitpick comments (1)
src/main/java/com/example/solidconnection/university/service/UnivApplyInfoQueryService.java (1)
50-54: 검색 기능 구현이 올바르게 진행되고 있습니다.확인 결과 요청하신 두 가지 사항이 모두 적절하게 구현되어 있습니다:
빈 검색어 처리: 이미 구현됨
null또는isBlank()상태이면 전체 대학 지원 정보를 ID 오름차순으로 반환하도록 처리되어 있습니다.검색 대상 필드 확인: 의도된 대로 동작 중
findAllByText는UnivApplyInfo.koreanName을 기준으로 검색하고 있으며, 추가로 국가명과 지역명도 함께 검색하는 구조입니다(우선순위 랭킹 적용).선택 사항: 입력 정규화 제안
현재 코드에서
text.trim()을 한 번 더 추가하면, 사용자 입력의 앞뒤 공백을 제거하여 더 깔끔한 검색 경험을 제공할 수 있습니다. 다만 이는 필수사항이 아니며, 선호에 따라 적용하셔도 됩니다.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@src/test/java/com/example/solidconnection/university/fixture/HomeUniversityFixtureBuilder.java`:
- Around line 14-27: The create() method in HomeUniversityFixtureBuilder can
call findByName(null) or attempt to save a null/empty name; add a precondition
at the start of create() that validates the instance field name is non-null and
not blank (e.g., trim and check length) and throw an IllegalStateException (or
similar) with a clear message like "Home university name must be set" to fail
fast and avoid ambiguous errors; update any tests if they relied on null
behavior.
Comment on lines
+14
to
+27
| private String name; | ||
|
|
||
| public HomeUniversityFixtureBuilder homeUniversity() { | ||
| return new HomeUniversityFixtureBuilder(homeUniversityRepository); | ||
| } | ||
|
|
||
| public HomeUniversityFixtureBuilder name(String name) { | ||
| this.name = name; | ||
| return this; | ||
| } | ||
|
|
||
| public HomeUniversity create() { | ||
| return homeUniversityRepository.findByName(name) | ||
| .orElseGet(() -> homeUniversityRepository.save(new HomeUniversity(null, name))); |
There was a problem hiding this comment.
1) 이름 미설정 시 예외 원인이 흐려질 수 있습니다.
- name이 없으면 findByName(null) 또는 null 저장 시도가 발생할 수 있어요.
- create()에서 선제 검증을 해두면 테스트 디버깅이 훨씬 쉬워집니다.
🔧 제안: create()에서 명시 검증 추가
+import java.util.Objects;
@@
public HomeUniversity create() {
+ Objects.requireNonNull(name, "homeUniversity name must be set");
return homeUniversityRepository.findByName(name)
.orElseGet(() -> homeUniversityRepository.save(new HomeUniversity(null, name)));
}🤖 Prompt for AI Agents
In
`@src/test/java/com/example/solidconnection/university/fixture/HomeUniversityFixtureBuilder.java`
around lines 14 - 27, The create() method in HomeUniversityFixtureBuilder can
call findByName(null) or attempt to save a null/empty name; add a precondition
at the start of create() that validates the instance field name is non-null and
not blank (e.g., trim and check length) and throw an IllegalStateException (or
similar) with a clear message like "Home university name must be set" to fail
fast and avoid ambiguous errors; update any tests if they relied on null
behavior.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
관련 이슈
작업 내용
{{URL}}/univ-apply-infos/search/filter를 무효화하고,{{URL}}/univ-apply-infos/search/text?value=로 통합합니다. 지원 대학 검색은 오직{{URL}}/univ-apply-infos/search/text?value=를 통해서 진행됩니다.기존 필터 검색의 조건은
languageTestType,testScore,countryCode인데, 프론트 측에서 필터링을 진행해주신다고 합니다.추가로 해당 지원 대학의 협정 대학 또한 응답에 포함시켰습니다(지원 대학 검색, 좋아요한 지원 대학 등등).
변경에 따른 테스트 코드 또한 수정했습니다.
간접 참조 방식에서 발생하는 N+1 문제를 막고자
Map방식을 사용했는데, 코드가 너무 중복되는 것 같아@ManyToOne+ fetch join을 사용하도록 변경했습니다. 지원 학교를 조회하는 경우 협정 대학 또한 같이 따라오는 관계라고 생각했습니다(== 어차피 같이 조회됨).특이 사항
리뷰 요구사항 (선택)