refactor: 기획 변경에 맞게 필터링 검색 기능 수정 #471
Conversation
- UnivApplyInfoFilterRepositoryImpl가 아니라 UnivApplyInfoRepository를 의존하게 한다. - 그렇게 해도 되는 이유는, UnivApplyInfoRepository가 UnivApplyInfoFilterRepository를 extend하고 있기 때문
nayonsoso
requested review from
Gyuhyeok99,
lsy1307,
whqtker and
wibaek
as code owners
Walkthrough
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
✨ Finishing Touches
🧪 Generate unit tests
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. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (12)
src/main/java/com/example/solidconnection/university/domain/LanguageTestType.java (2)
7-8: 1) CEFR/JLPT 비교는 문자열 비교 대신 ‘도메인 랭킹’으로 명시화하는 것이 안전합니다.
문자열 비교는 공백/소문자/포맷 변형에 취약하여 오동작 여지가 있습니다. 도메인 랭킹 매핑으로 비교를 명시화하면 유지보수성과 안정성이 올라갑니다.
- CEFR: A1 < A2 < B1 < B2 < C1 < C2 순서를 랭킹 테이블로 비교하세요.
- JLPT: N1 > N2 > N3 > N4 > N5 규칙을 랭킹 테이블로 비교하세요.
적용 예시(diff: 초기화 부분 교체):
- CEFR(String::compareTo), - JLPT(Comparator.reverseOrder()), + CEFR(LanguageTestType::compareCefrLevel), + JLPT(LanguageTestType::compareJlptLevel),추가 메서드(파일 하단에 배치):
private static int compareCefrLevel(String s1, String s2) { String a = s1 == null ? "" : s1.trim().toUpperCase(); String b = s2 == null ? "" : s2.trim().toUpperCase(); List<String> order = List.of("A1","A2","B1","B2","C1","C2"); int ia = order.indexOf(a), ib = order.indexOf(b); if (ia < 0 || ib < 0) return a.compareTo(b); // 미정의 값은 안전하게 문자열 비교로 폴백 return Integer.compare(ia, ib); } private static int compareJlptLevel(String s1, String s2) { String a = s1 == null ? "" : s1.trim().toUpperCase(); String b = s2 == null ? "" : s2.trim().toUpperCase(); // N1(최상) > N2 > N3 > N4 > N5(최하) Map<String,Integer> rank = Map.of("N1", 5, "N2", 4, "N3", 3, "N4", 2, "N5", 1); Integer ra = rank.get(a), rb = rank.get(b); if (ra == null || rb == null) return b.compareTo(a); // 미정의 값은 역순 문자열 비교로 폴백 return Integer.compare(ra, rb); }
19-19: 2) ETC가 항상 0을 반환하므로 점수 필터는 항상 ‘통과’합니다. 의도 확인이 필요합니다.
서비스 구현에서 compare(...) >= 0 로 판단하기 때문에, ETC 선택 시 점수 입력 유무와 관계없이 필터가 적용되지 않습니다. 의도된 정책인지 확인 부탁드립니다. 의도된 것이 아니라면 ETC는 null 비교자(비교 불가 시 필터 미적용)로 분기하거나, 서비스 레벨에서 ETC 선택 시 점수 필터를 스킵하는 것이 명확합니다.src/main/java/com/example/solidconnection/university/dto/UnivApplyInfoFilterSearchRequest.java (2)
11-11: 1) record에서 가변 인자(String... countryCode) 대신 컬렉션 사용을 권장합니다.
Spring @ModelAttribute 바인딩과 직렬화/문서화 측면에서 List 또는 String[]가 더 안전하고 직관적입니다. 또한 필드명이 단수형(countryCode)이라 오해 소지가 있습니다.
- 필드명을 countryCodes(복수)로 변경.
- 타입을 List으로 변경하여 바인딩 호환성 강화.
적용 예시(diff):
@@ -package com.example.solidconnection.university.dto; +package com.example.solidconnection.university.dto; import com.example.solidconnection.university.domain.LanguageTestType; import jakarta.validation.constraints.NotNull; +import java.util.List; public record UnivApplyInfoFilterSearchRequest( @NotNull(message = "어학 시험 종류를 선택해주세요.") LanguageTestType languageTestType, String testScore, - String... countryCode + List<String> countryCodes ) { }서비스/리포지토리 연계 변경 예시(다른 파일):
- service: request.countryCodes().toArray(new String[0]) 또는 저장소 시그니처를 컬렉션으로 변경.
- repository: findAllByFilter(...)의 varargs를 Collection로 교체하는 것도 고려.
8-10: 2) testScore 형식 검증이 없어 런타임 NumberFormatException 위험이 있습니다.
LanguageTestType.compare 내부에서 Integer/Double 파싱 실패 시 500이 발생할 수 있습니다. 입력 단계에서 포맷을 보장하는 것이 안전합니다.
- 간단 대안: @pattern을 부착하고, 타입별(예: IELTS=소수, TOEIC/TOEFL=정수) 정책은 서비스에서 분기 검증.
- 견고 대안: LanguageTestType + testScore의 교차 필드 검증용 커스텀 Constraint(Validator) 추가.
원하시면 타입별 스코어 검증용 커스텀 Constraint와 Validator 구현을 만들어 드리겠습니다.
src/main/java/com/example/solidconnection/university/repository/custom/UnivApplyInfoFilterRepository.java (1)
11-11: 1) 파라미터명(countryKoreanNames)과 구현/쿼리(country.code)가 불일치합니다.
구현(Impl)에서는 country.code 필드를 사용하고 있어 ‘KoreanNames’라는 이름은 혼란을 줍니다. 명확히 countryCodes로 맞춰주세요.적용 예시(diff):
- List<UnivApplyInfo> findAllByFilter(LanguageTestType testType, String testScore, String term, String ... countryKoreanNames); + List<UnivApplyInfo> findAllByFilter(LanguageTestType testType, String testScore, String term, String ... countryCodes);src/main/java/com/example/solidconnection/university/repository/custom/UnivApplyInfoFilterRepositoryImpl.java (2)
124-129: 2) countryCodesIn에서 공백/빈 문자열 정규화가 있으면 안전합니다.
외부 입력이므로 빈 값이 섞여 들어오면 조건이 희미해질 수 있습니다. 간단 필터만 추가해도 방어 효과가 있습니다.적용 예시(diff):
- return country.code.in(Arrays.asList(givenCountryCodes)); + return country.code.in( + Arrays.stream(givenCountryCodes) + .map(s -> s == null ? "" : s.trim()) + .filter(s -> !s.isEmpty()) + .toList() + );
131-139: 3) 같은 시험 유형의 요구사항이 여러 개면 findFirst()는 비결정적입니다. anyMatch()가 의도에 더 부합합니다.
여러 트랙/전형이 있는 경우 하나라도 기준을 충족하면 통과하는 것이 자연스럽습니다. anyMatch가 더 안전합니다.적용 예시(diff):
- return univApplyInfo.getLanguageRequirements().stream() - .filter(languageRequirement -> languageRequirement.getLanguageTestType().equals(givenTestType)) - .findFirst() - .map(requirement -> givenTestType.compare(givenTestScore, requirement.getMinScore())) - .orElse(-1) >= 0; + return univApplyInfo.getLanguageRequirements().stream() + .filter(req -> req.getLanguageTestType().equals(givenTestType)) + .anyMatch(req -> givenTestType.compare(givenTestScore, req.getMinScore()) >= 0);src/test/java/com/example/solidconnection/university/service/UnivApplyInfoQueryServiceTest.java (4)
58-70: 캐시 히트 검증은 좋습니다만, 테스트 간 캐시 간섭 가능성은 차단해 주세요.
- 동일 Fixture에서 동일 엔티티(또는 동일 ID)가 재사용되면, 이전 테스트에서 이미 캐시가 적중되어 repository 호출 횟수(times(1)) 검증이 흔들릴 수 있습니다.
- 해결책: 각 테스트마다 신규 엔티티(ID) 보장 또는 캐시 초기화가 필요합니다.
- 제안: CacheManager/전용 캐시 유틸을 주입해 @AfterEach에서 해당 캐시 키 패턴을 비우거나, 테스트용 캐시 이름/키 suffix를 분리하세요.
예시(참고용, 파일 외부 코드 스니펫):
// 예: 테스트 클래스 상단 @Autowired CacheManager cacheManager; // 예: 각 테스트 종료 후 캐시 초기화 @AfterEach void tearDown() { // 실제 캐시 이름/키 전략에 맞게 정리 (예: "customCacheManager" 내 캐시/키) // Objects.requireNonNull(cacheManager.getCache("univApplyInfo")).clear(); }
72-83: 예외 검증을 캐시 AOP에 비의존적으로 단순화하세요.
- 현재는 RuntimeException 래핑에 종속된 검증 체인입니다.
- 루트 원인만 검증하도록 바꾸면 캐시 적용 방식 변경에도 테스트가 견고해집니다.
다음과 같이 수정하는 것을 제안드립니다:
- assertThatExceptionOfType(RuntimeException.class) - .isThrownBy(() -> univApplyInfoQueryService.getUnivApplyInfoDetail(invalidUnivApplyInfoId)) - .havingRootCause() - .isInstanceOf(CustomException.class) - .withMessage(UNIV_APPLY_INFO_NOT_FOUND.getMessage()); + assertThatThrownBy(() -> univApplyInfoQueryService.getUnivApplyInfoDetail(invalidUnivApplyInfoId)) + .hasRootCauseInstanceOf(CustomException.class) + .hasRootCauseMessage(UNIV_APPLY_INFO_NOT_FOUND.getMessage());추가로 필요한 정적 임포트:
import static org.assertj.core.api.Assertions.assertThatThrownBy;
106-121: 테스트 명칭을 의도를 더 분명하게 다듬어 주세요.
- 현재 이름은 해석에 따라 "요구 점수 이상인 대학" 또는 "사용자 점수 이상만 허용"으로 혼동될 수 있습니다.
- 본문 로직은 “사용자 점수(800)로 지원 가능한 곳(요구 점수 ≤ 사용자 점수)”을 검증합니다.
아래처럼 메서드명을 조정하면 의도가 더 선명합니다:
- void 어학_시험_점수가_기준치_이상인_곳을_필터링한다() { + void 내_점수로_지원_가능한_대학을_필터링한다() {
123-147: 국가 코드 필터링 테스트 훌륭합니다. 소문자/혼합형 입력에 대한 보강을 권장합니다.
- 복수 국가 코드(varargs) 바인딩과 필터링 결과를 잘 검증하고 있습니다.
- 추가로 ‘us, ca’ 같은 소문자/혼합형 입력에 대한 정규화(대문자 변환) 동작도 테스트로 보강하면 안정성이 올라갑니다.
예: "us" 입력 시에도 동일 결과를 기대하는 케이스를 하나 더 추가해 주세요.
src/main/java/com/example/solidconnection/university/controller/UnivApplyInfoController.java (1)
46-53: TODO 처리: 좋아요 목록 응답도 래퍼(UnivApplyInfoPreviewResponses)로 통일하세요.
- 본 PR 목적(프리뷰 응답 래퍼 도입)에 맞게 응답 일관성을 확보하는 것이 바람직합니다.
- 컨트롤러 레벨에서 간단히 포장하여 리턴하면 하위 서비스 변경 없이도 정렬됩니다.
다음과 같이 수정을 제안드립니다:
- // todo: return 타입 UnivApplyInfoPreviewResponses 같이 객체로 묶어서 반환하는 것으로 변경 필요 @GetMapping("/like") - public ResponseEntity<List<UnivApplyInfoPreviewResponse>> getLikedUnivApplyInfos( + public ResponseEntity<UnivApplyInfoPreviewResponses> getLikedUnivApplyInfos( @AuthorizedUser long siteUserId ) { - List<UnivApplyInfoPreviewResponse> likedUnivApplyInfos = likedUnivApplyInfoService.getLikedUnivApplyInfos(siteUserId); - return ResponseEntity.ok(likedUnivApplyInfos); + List<UnivApplyInfoPreviewResponse> likedUnivApplyInfos = + likedUnivApplyInfoService.getLikedUnivApplyInfos(siteUserId); + return ResponseEntity.ok(new UnivApplyInfoPreviewResponses(likedUnivApplyInfos)); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (8)
src/main/java/com/example/solidconnection/university/controller/UnivApplyInfoController.java(2 hunks)src/main/java/com/example/solidconnection/university/domain/LanguageTestType.java(2 hunks)src/main/java/com/example/solidconnection/university/dto/UnivApplyInfoFilterSearchRequest.java(1 hunks)src/main/java/com/example/solidconnection/university/repository/UniversityRepository.java(0 hunks)src/main/java/com/example/solidconnection/university/repository/custom/UnivApplyInfoFilterRepository.java(1 hunks)src/main/java/com/example/solidconnection/university/repository/custom/UnivApplyInfoFilterRepositoryImpl.java(2 hunks)src/main/java/com/example/solidconnection/university/service/UnivApplyInfoQueryService.java(2 hunks)src/test/java/com/example/solidconnection/university/service/UnivApplyInfoQueryServiceTest.java(2 hunks)
💤 Files with no reviewable changes (1)
- src/main/java/com/example/solidconnection/university/repository/UniversityRepository.java
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/main/java/com/example/solidconnection/university/controller/UnivApplyInfoController.java (1)
src/main/java/com/example/solidconnection/university/service/UnivApplyInfoQueryService.java (1)
RequiredArgsConstructor(17-55)
⏰ 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
🔇 Additional comments (5)
src/main/java/com/example/solidconnection/university/service/UnivApplyInfoQueryService.java (1)
41-48: 1) 필터 기반 검색 파이프라인이 간결합니다. LGTM.
DTO → 저장소 → 응답 매핑 흐름이 직관적이며 부수효과가 없습니다. DTO 필드명이 countryCodes로 바뀌면 호출부도 함께 갱신만 해주세요.src/main/java/com/example/solidconnection/university/repository/custom/UnivApplyInfoFilterRepositoryImpl.java (2)
108-116: 4) null 반환으로 동적 조건을 조립하는 패턴이 깔끔합니다.
Querydsl where(varargs)와 null 무시 규칙을 잘 활용하셨습니다. 가독성과 확장성이 좋아졌습니다.Also applies to: 117-123
94-97: 5) 점수 미입력 시 DB 레벨까지만 필터링하고, 점수 비교는 자바에서 처리한 선택이 타당합니다.
쿼리 복잡도 대비 이득을 고려하면 현 접근이 실용적입니다. 성능 이슈가 생기면 이후에 서브쿼리/계산 컬럼 등으로 대체를 검토하면 됩니다.Also applies to: 98-103
src/test/java/com/example/solidconnection/university/service/UnivApplyInfoQueryServiceTest.java (1)
89-105: 필터: 시험 종류 기준 동작 검증이 명확하고 유효합니다.
- TOEIC 지정 시, 해당 요건을 가진 데이터만 반환되는지를 정확히 검증하고 있습니다.
- containsExactly 사용으로 결과 개수/순서까지 엄밀히 확인한 점이 좋습니다.
src/main/java/com/example/solidconnection/university/controller/UnivApplyInfoController.java (1)
90-97: 필터 검색 엔드포인트 설계 적합, 다중 국가 코드 바인딩 방식만 명시해 주세요.
- @ModelAttribute로 DTO 바인딩하는 선택은 GET 쿼리 파라미터 설계와 잘 맞습니다.
- countryCode 가변 인자 수용 시, ‘countryCode=US&countryCode=CA’ 형태를 공식 문서(api-docs)에 명시해 주면 혼선을 줄일 수 있습니다. ‘US,CA’ 콤마 구분은 바인딩되지 않습니다.
src/main/java/com/example/solidconnection/university/controller/UnivApplyInfoController.java
Show resolved
Hide resolved
.../example/solidconnection/university/repository/custom/UnivApplyInfoFilterRepositoryImpl.java
Show resolved
Hide resolved
src/main/java/com/example/solidconnection/university/service/UnivApplyInfoQueryService.java
Show resolved
Hide resolved
nayonsoso
force-pushed
the
469/refactor-univ-apply-info-filter-search
branch
from
ec57419 to
4e0af93
Compare
| return ResponseEntity.ok(univApplyInfoDetailResponse); | ||
| } | ||
|
|
||
| // todo: return타입 UniversityInfoForApplyPreviewResponses로 추후 수정 필요 |
There was a problem hiding this comment.
고민한 지점 : 페이지네이션으로 내려줘야 할지, 검색 결과를 한번에 내려줘야할지 고민이 되었습니다.
그런데 뒤에 언급하는 내용처럼
점수의 경우 DB단에서 비교하기가 매우 까다롭기 때문에 메모리에서 필터링 하는데
이때 페이지네이션을 적용하기 까다롭다는 점과,
한 학기의 대학 지원 정보가 약 400개정도이므로, 엄청난 성능 저하는 있지 않을것이라 판단하여
검색된 결과 전체를 반환하도록 구현했습니다!
|
|
||
| List<UnivApplyInfo> findAllByRegionCodeAndKeywordsAndLanguageTestTypeAndTestScoreAndTerm( | ||
| String regionCode, List<String> keywords, LanguageTestType testType, String testScore, String term); | ||
| List<UnivApplyInfo> findAllByFilter(LanguageTestType testType, String testScore, String term, String ... countryKoreanNames); |
There was a problem hiding this comment.
함수 이름을 변경했습니다.
findAllByRegionCodeAndKeywordsAndLanguageTestTypeAndTestScoreAndTerm → findAllByFilter
사용되는 모든 요소를 함수 이름에 나열하는게 더 가독성을 떨어뜨린다고 느껴졌습니다 😅
따라서 비지니스 로직을 표현하도록 함수명을 변경했습니다.
| /* | ||
| * 시험 유형에 따라 성적 비교 방식이 다르다. | ||
| * 입력된 점수가 대학에서 요구하는 최소 점수보다 높은지를 '쿼리로' 비교하기엔 쿼리가 지나치게 복잡해진다. | ||
| * 따라서 이 부분만 자바 코드로 필터링한다. | ||
| * */ | ||
| return filteredUnivApplyInfo.stream() | ||
| .filter(uai -> compareMyTestScoreToMinPassScore(uai, testType, testScore) >= 0) | ||
| .filter(uai -> isGivenScoreOverMinPassScore(uai, testType, testScore)) | ||
| .toList(); | ||
| } |
There was a problem hiding this comment.
LanguageTestType enum 을 확인하시면 아시겠지만,
어학 시험마다 "무엇이 더 높은 점수인지" 비교하는 로직이 각각 다릅니다.
이것을 조건으로 쿼리 문을 실행하려 하면, 엄청나게 긴 case 문이 발생할 것입니다 😱
따라서 이 부분은 자바의 메모리에서 처리하게 했습니다.
앞에도 언급했듯, 한 학기에 열리는 대학은 약 400여개입니다.
여기에서 어학 시험 종류와 국가 코드로 한번씩 필터링 된 것을 메모리에 불러오는 것이므로
메모리와 관련된 이슈는 없을 것이라 생각합니다!
nayonsoso
force-pushed
the
469/refactor-univ-apply-info-filter-search
branch
from
4e0af93 to
f3476fb
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/main/java/com/example/solidconnection/university/service/UnivApplyInfoQueryService.java (1)
50-53: null 반환으로 인한 NPE 가능성 제거: 빈 래퍼를 반환하세요
- 현재 null을 반환하면 컨트롤러/호출부에서 NPE가 날 수 있어요.
- 구현 전까지도 빈 래퍼를 반환하면 방어적으로 동작합니다.
- 과거 코멘트와 동일 제안이며, 바로 적용 가능한 1줄 변경입니다.
@Transactional(readOnly = true) public UnivApplyInfoPreviewResponses searchUnivApplyInfoByText(String text) { - // todo: 구현 - return null; + // todo: 구현 + return new UnivApplyInfoPreviewResponses(List.of()); }원하시면 컨트롤러 레벨 단위 테스트도 함께 추가해드릴게요.
src/main/java/com/example/solidconnection/university/repository/custom/UnivApplyInfoFilterRepositoryImpl.java (1)
80-85: N+1 완화: to-one 연관 fetchJoin 추가 권장 (중복 코멘트)
- Preview 매핑에서 university/country 접근 시 LAZY 로딩이면 N+1이 발생할 수 있어요.
- to-one 연관(university, country)은 fetchJoin으로 미리 당겨오는 것이 안전합니다.
- distinct는 이미 사용 중이므로 결과 중복도 제어 가능합니다.
- List<UnivApplyInfo> filteredUnivApplyInfo = queryFactory.selectFrom(univApplyInfo) - .join(univApplyInfo.university, university) - .join(university.country, country) - .join(univApplyInfo.languageRequirements, languageRequirement) - .fetchJoin() + List<UnivApplyInfo> filteredUnivApplyInfo = queryFactory.selectFrom(univApplyInfo) + .join(univApplyInfo.university, university).fetchJoin() + .join(university.country, country).fetchJoin() + .join(univApplyInfo.languageRequirements, languageRequirement).fetchJoin() .where( languageTestTypeEq(languageRequirement, testType), termEq(univApplyInfo, term), countryCodesIn(country, countryCodes) ) .distinct() .fetch();src/main/java/com/example/solidconnection/university/controller/UnivApplyInfoController.java (1)
98-104: 6) /search/text가 null 바디(200 OK)를 반환합니다. 빈 래퍼로 보장해 주세요.
한 문장으로 요약하면, 서비스가 null을 돌려 현재는 스키마 계약(UnivApplyInfoPreviewResponses) 위반 상태입니다.아래와 같이 컨트롤러에서 널-가드를 추가해 주세요.
@GetMapping("/search/text") public ResponseEntity<UnivApplyInfoPreviewResponses> searchUnivApplyInfoByText( @RequestParam(required = false) String text ) { - UnivApplyInfoPreviewResponses response = univApplyInfoQueryService.searchUnivApplyInfoByText(text); - return ResponseEntity.ok(response); + UnivApplyInfoPreviewResponses response = univApplyInfoQueryService.searchUnivApplyInfoByText(text); + if (response == null) { + response = new UnivApplyInfoPreviewResponses(java.util.Collections.emptyList()); + } + return ResponseEntity.ok(response); }참고로, 컨트롤러보다 서비스에서 non-null 계약을 보장하는 방식도 좋습니다(파일 외 참고용):
// UnivApplyInfoQueryService.java @Transactional(readOnly = true) public UnivApplyInfoPreviewResponses searchUnivApplyInfoByText(String text) { // todo: 실제 구현 return new UnivApplyInfoPreviewResponses(java.util.List.of()); }
🧹 Nitpick comments (5)
src/main/java/com/example/solidconnection/university/dto/UnivApplyInfoFilterSearchRequest.java (1)
9-12: countryCode → countryCodes 복수형 일관성 적용 안내
DTO 파라미터명 수정
- src/main/java/com/example/solidconnection/university/dto/UnivApplyInfoFilterSearchRequest.java에서
List<String> countryCode를List<String> countryCodes로 변경하세요.서비스 호출부 변경
- src/main/java/com/example/solidconnection/university/service/UnivApplyInfoQueryService.java에서
request.countryCode()를request.countryCodes()로 업데이트하세요.추가 사용처 점검
- 테스트나 다른 코드에서
countryCode()호출이 남아 있다면countryCodes()로 일괄 교체해주시기 바랍니다.- List<String> countryCode + List<String> countryCodes - .findAllByFilter(request.languageTestType(), request.testScore(), term, request.countryCode()) + .findAllByFilter(request.languageTestType(), request.testScore(), term, request.countryCodes())src/main/java/com/example/solidconnection/university/repository/custom/UnivApplyInfoFilterRepository.java (1)
11-11: 파라미터명 일관성 점검 및 정정 제안
- 파라미터명 일치화
- 인터페이스의
countryKoreanNames를countryCodes로 변경하여 구현체(UnivApplyInfoFilterRepositoryImpl)와 동일하게 맞춰요.- 서비스 호출부 메소드명 일관성 검토
UnivApplyInfoQueryService에서request.countryCode()대신 복수형countryCodes()로 메소드명 일치 여부를 검토해요.- 테스트 및 문서 통일
- 테스트 코드와 API 문서에서도
countryCodes로 통일하면 가독성과 유지보수가 더 편해져요.- List<UnivApplyInfo> findAllByFilter(LanguageTestType testType, String testScore, String term, List<String> countryKoreanNames); + List<UnivApplyInfo> findAllByFilter(LanguageTestType testType, String testScore, String term, List<String> countryCodes);src/test/java/com/example/solidconnection/university/service/UnivApplyInfoQueryServiceTest.java (1)
1-150: 텍스트 검색에 대한 최소 동작 테스트 추가 제안
- 서비스가 빈 래퍼를 반환하도록 바꾸면, 텍스트 검색의 기본 동작도 테스트해두면 좋습니다.
- NPE 방지와 API 계약(Non-Null 응답)을 함께 보장합니다.
- 예시 메서드를 하나 추가해두면 회귀 방지에 유용합니다.
예시:
@Test void 텍스트_검색은_구현_전까지_빈_리스트를_반환한다() { UnivApplyInfoPreviewResponses response = univApplyInfoQueryService.searchUnivApplyInfoByText("anything"); assertThat(response.univApplyInfoPreviews()).isEmpty(); }src/main/java/com/example/solidconnection/university/repository/custom/UnivApplyInfoFilterRepositoryImpl.java (1)
130-138: 여러 요구사항(동일 시험 유형) 존재 시 비교 기준 명시 필요
- 현재는 동일 시험 유형이 여러 개 등록된 경우 첫 번째 요소만 비교합니다(findFirst).
- 도메인 제약이 “시험 유형당 하나”라면 주석으로 인variants를 명시해두면 추후 오해를 줄일 수 있어요.
- 만약 다수 가능성까지 고려한다면, “가장 낮은 최소 요구점수” 기준으로 비교하는 로직이 더 보수적입니다.
인바리언트 확인 또는 개선안 중 택1:
- 레코드 주석 추가: “언어 시험 유형별 최소 요구사항은 1건으로 보장된다.”
- 혹은 아래처럼 최소 요구점수 기준 비교(참고용):
return univApplyInfo.getLanguageRequirements().stream() .filter(r -> r.getLanguageTestType().equals(givenTestType)) .map(r -> r.getMinScore()) .min((a, b) -> givenTestType.compare(a, b)) .map(minRequired -> givenTestType.compare(givenTestScore, minRequired) >= 0) .orElse(false);src/main/java/com/example/solidconnection/university/controller/UnivApplyInfoController.java (1)
90-97: 5) /search/filter 엔드포인트 설계는 타당합니다. 다만 ‘전체 반환’에는 안전장치 한 가지를 제안합니다.
한 문장으로 요약하면, 현재 규모(학기당 ~400건)에서는 무리가 없으나 데이터 증가·광범위 조회 시 대용량 응답 가능성이 있어 상한선이나 가드를 고려해 보세요.
- 제안 1. 최대 응답 건수 상한(예: 1000건)을 서버 측에서 방어하고 초과 시 경고 로그/메트릭을 남기기.
- 제안 2. 계약상 페이지네이션이 불가하다면, 최소한 size 파라미터에 대한 내부 상한을 두고 무제한 확장을 막기.
- 보너스 제안. 좋아요 목록(/like) 응답도 같은 래퍼 타입으로 통일해 클라이언트 처리 로직을 단순화하면 좋습니다(파일 내 TODO 참고).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (8)
src/main/java/com/example/solidconnection/university/controller/UnivApplyInfoController.java(2 hunks)src/main/java/com/example/solidconnection/university/domain/LanguageTestType.java(2 hunks)src/main/java/com/example/solidconnection/university/dto/UnivApplyInfoFilterSearchRequest.java(1 hunks)src/main/java/com/example/solidconnection/university/repository/UniversityRepository.java(0 hunks)src/main/java/com/example/solidconnection/university/repository/custom/UnivApplyInfoFilterRepository.java(1 hunks)src/main/java/com/example/solidconnection/university/repository/custom/UnivApplyInfoFilterRepositoryImpl.java(1 hunks)src/main/java/com/example/solidconnection/university/service/UnivApplyInfoQueryService.java(2 hunks)src/test/java/com/example/solidconnection/university/service/UnivApplyInfoQueryServiceTest.java(2 hunks)
💤 Files with no reviewable changes (1)
- src/main/java/com/example/solidconnection/university/repository/UniversityRepository.java
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/com/example/solidconnection/university/domain/LanguageTestType.java
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/main/java/com/example/solidconnection/university/controller/UnivApplyInfoController.java (1)
src/main/java/com/example/solidconnection/university/service/UnivApplyInfoQueryService.java (1)
RequiredArgsConstructor(17-55)
⏰ 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
🔇 Additional comments (11)
src/main/java/com/example/solidconnection/university/dto/UnivApplyInfoFilterSearchRequest.java (1)
7-13: DTO 구성 자체는 목적에 부합합니다
- languageTestType에 @NotNull이 있어 컨트롤러 바인딩 시 명확한 오류 메시지를 줄 수 있어 좋아요.
- testScore, countryCodes는 선택적 파라미터로 동작하고, Repository에서 null/blank/empty를 우아하게 처리하도록 되어 있어 일관적입니다.
src/main/java/com/example/solidconnection/university/service/UnivApplyInfoQueryService.java (2)
41-48: 필터 기반 검색 흐름은 간결하고 반환 래퍼도 일관적입니다
- Repository → DTO 매핑 → PreviewResponses 래퍼로의 변환이 읽기 좋고 테스트와도 일치합니다.
- toList() 사용으로 불변 리스트를 반환하는 것도 안전합니다.
23-24: 필드 가시성 축소:term을private으로 변경 제안
- public → private
- 구성 프로퍼티는 외부에서 변경될 필요가 없으므로 가시성을 줄여 안전성을 높입니다.- 캡슐화 및 동시성 강화
- 필드를 외부에 노출하지 않으면 변경 관리와 동시성 이슈 방지에 유리합니다.- 기존 주입 방식 그대로 유지
- Spring의 @value 리플렉션 주입은 private 필드에서도 정상 동작하므로 추가적인 접근자(getter) 구현이 필요 없습니다.- @Value("${university.term}") - public String term; + @Value("${university.term}") + private String term;src/test/java/com/example/solidconnection/university/service/UnivApplyInfoQueryServiceTest.java (3)
59-71: 캐시 검증 테스트 구성 좋습니다
- 동일 ID 두 번 호출 후 Repository 1회만 호출되는 것을 검증해 의도한 캐시 동작을 명확히 보장합니다.
- equals 비교로 동일 응답임을 확인하는 것도 깔끔합니다.
107-122: 점수 기준 필터 테스트 시나리오 타당합니다
- 800 요청 시 최소 요구 800 충족, 900 요구 대학은 제외되는 기대가 비즈니스와 일치합니다.
- containsExactlyInAnyOrder는 한 건일 때도 안정적으로 동작합니다.
124-148: 국가 코드 필터 테스트가 현실적인 분기(단일 vs 다중 코드)를 잘 커버합니다
- 단일 코드와 다중 코드를 각각 검증해 where in 동작을 확실히 보장합니다.
- assertAll로 복수 검증을 묶어 가독성이 좋아요.
src/main/java/com/example/solidconnection/university/repository/custom/UnivApplyInfoFilterRepositoryImpl.java (1)
71-91: where 절의 null-무시 패턴은 깔끔하고 일관적입니다
- languageTestTypeEq/termEq/countryCodesIn에서 null/blank/empty를 null 반환해 QueryDSL이 무시하도록 한 패턴은 유지보수에 좋습니다.
- testScore는 메모리 필터로 분리해 쿼리 복잡도를 낮춘 판단도 현실적입니다.
src/main/java/com/example/solidconnection/university/controller/UnivApplyInfoController.java (4)
6-6: 1) 필터 요청 DTO import가 변경된 계약과 정확히 부합합니다.
한 문장으로 요약하면, /search/filter에 @ModelAttribute 바인딩을 위한 준비가 깔끔합니다.
8-8: 2) 응답 래퍼(UnivApplyInfoPreviewResponses) import로 반환 스키마 일관성이 강화됩니다.
한 문장으로 요약하면, 컬렉션을 래핑해 계약을 고정하는 방향이 좋습니다.
13-13: 3) Jakarta @Valid import로 DTO 수준 유효성 검증이 활성화됩니다.
한 문장으로 요약하면, 누락/형식 오류를 컨트롤러 진입 시점에서 400으로 빠르게 차단할 수 있습니다.
19-19: 4) @ModelAttribute import가 필터 쿼리 바인딩에 적절합니다.
한 문장으로 요약하면, GET 쿼리 파라미터를 레코드 DTO로 안전하게 매핑할 수 있습니다.
src/test/java/com/example/solidconnection/university/service/UnivApplyInfoQueryServiceTest.java
Show resolved
Hide resolved
whqtker
left a comment
whqtker
left a comment
There was a problem hiding this comment.
정말 고생하셨습니다 ~ 열심히 작업해주셔서 감사합니다 👍
개인적인 의견인데, 어학 점수 자체를 비교자를 통해 비교하는 방법 대신, 어학 점수를 정규화(?)하여 점수 비교 방식을 단순히 정수 비교로 바꾼다면 QueryDSL에서 점수를 필터링할 수 있을거고, 페이징도 적용할 수 있을 것 같습니다. 예를 들어 CERF는 A1에 1, C2에 6을 부여하고, JLPT는 N5에 1, N1에 5를 부여하는 식으로 설정하면 어학 시험 종류에 구애받지 않고 동일한 검증 기준(정수 비교)를 사용할 수 있으니까요.
물론 그에 따른 트레이드오프는 있을거고, 추후 페이징을 적용하면 좋을 것 같아 간단히 생각해봤습니다. 이 방법에 대해 어떻게 생각하시나요 ?
|
|
||
| // then | ||
| assertThat(response.univApplyInfoPreviews()) | ||
| .containsExactlyInAnyOrder(UnivApplyInfoPreviewResponse.from(괌대학_A_지원_정보)); |
There was a problem hiding this comment.
900점인 대학은 포함되지 않는다는 검증이 추가되면 좋을 것 같습니다 !
There was a problem hiding this comment.
오 저는 생각치도 못했는데 새로운 방법 제시해주셔서 감사드립니다 🙇🏻♀️
사실 어떻게 구현할지 생각하니 조금 복잡해지기는 하는 것 같습니다😅
|
동의합니다 ! 일단은 뒤로 제쳐두고, 우선순위가 높아지면 고려해봐야겠네요 |
4 participants
관련 이슈
작업 내용
수경님과 회의한 내용에 따라, 대학지원정보 필터링 검색 기능을 수정합니다.
어떻게 API 기획이 바뀌었는지는 api-docs PR에 자세히 나와있습니다🤗
https://github.com/solid-connection/api-docs/pull/48
특이 사항
코멘트로 남겨두겠습니다!