refactor: Location 도메인 리팩터링 #340
Conversation
Walkthrough
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
src/main/java/com/example/solidconnection/auth/service/SignUpService.java (1)
80-85: 🛠️ Refactor suggestion중복 관심 국가 입력 시
DataIntegrityViolation가능성
signUpRequest.interestedCountries()에 중복 값이 들어오면, 유니크 제약 때문에 테스트 단계에서 바로 예외가 터질 수 있습니다. 사전에 중복 제거 → 불필요한 DB 트래픽·롤백을 줄이는 편이 좋습니다.List<String> interestedCountryNames = signUpRequest.interestedCountries(); -List<InterestedCountry> interestedCountries = countryRepository.findByKoreanNames(interestedCountryNames).stream() - .map(country -> new InterestedCountry(savedSiteUser, country)) - .toList(); +List<InterestedCountry> interestedCountries = countryRepository + .findByKoreanNames(interestedCountryNames.stream().distinct().toList()) + .stream() + .map(country -> new InterestedCountry(savedSiteUser, country)) + .toList();
🧹 Nitpick comments (5)
src/main/java/com/example/solidconnection/location/country/repository/InterestedCountryRepository.java (1)
11-13: 1) 인터페이스 명칭 교정은 적절하지만, 읽기 전용 힌트 추가 고려
findAllBySiteUser는 순수 조회 메서드입니다.@Transactional(readOnly = true)를 리포지토리 레벨(혹은 서비스 레벨)에 명시하면 플러시 우회·성능 이점이 있습니다.src/main/resources/db/migration/V15__add_unique_constraint_to_intersted.sql (1)
1-7: 4) 마이그레이션 스크립트 두 가지 주의점
1. 파일명 오타:intersted→interested로 교정하면 가독성이 좋아집니다.
2. 제약 추가 전 중복 데이터가 존재하면 실패합니다. 사전 정리 쿼리나ON CONFLICT DO NOTHING처리 여부를 검토해 주세요.src/main/java/com/example/solidconnection/auth/service/EmailSignUpService.java (1)
23-26: 6) 생성자 파라미터 순서 유지 권장
DI 프레임워크는 타입으로 주입하지만, 가독성을 위해 지역·국가 리포지토리 쌍의 순서를 맞추면 읽기 쉽습니다. 작은 리팩터지만 고려해 보세요.src/test/java/com/example/solidconnection/location/region/repository/RegionRepositoryTest.java (1)
44-46: flush 호출로 예외 시점을 명확히
현재save()만 호출하면 JPA Flush 타이밍에 따라 예외가 테스트 메서드 밖에서 발생할 수도 있습니다.save()후flush()를 명시 호출하면 람다 내부에서 예외가 확실히 터져 테스트 신뢰도가 올라갑니다.-assertThatCode(() -> interestedRegionRepository.save(secondInterest)) +assertThatCode(() -> { + interestedRegionRepository.save(secondInterest); + interestedRegionRepository.flush(); +}) .isInstanceOf(DataIntegrityViolationException.class);세 테스트 모두 동일 패턴이므로 한 번만 수정해도 무방합니다.
Also applies to: 60-65, 80-84
src/test/java/com/example/solidconnection/location/country/repository/CountryRepositoryTest.java (1)
44-46: Region 테스트와 동일, flush 권장
유니크 제약 예외가 메서드 안에서 확실히 발생하도록flush()추가를 권장드립니다.-assertThatCode(() -> interestedCountryRepository.save(secondInterest)) +assertThatCode(() -> { + interestedCountryRepository.save(secondInterest); + interestedCountryRepository.flush(); +}) .isInstanceOf(DataIntegrityViolationException.class);Also applies to: 60-65, 80-84
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
src/main/java/com/example/solidconnection/auth/service/EmailSignUpService.java(2 hunks)src/main/java/com/example/solidconnection/auth/service/SignUpService.java(3 hunks)src/main/java/com/example/solidconnection/auth/service/oauth/OAuthSignUpService.java(2 hunks)src/main/java/com/example/solidconnection/location/country/domain/InterestedCountry.java(1 hunks)src/main/java/com/example/solidconnection/location/country/repository/InterestedCountryRepository.java(1 hunks)src/main/java/com/example/solidconnection/location/region/domain/InterestedRegion.java(1 hunks)src/main/resources/db/migration/V15__add_unique_constraint_to_intersted.sql(1 hunks)src/test/java/com/example/solidconnection/location/country/repository/CountryRepositoryTest.java(1 hunks)src/test/java/com/example/solidconnection/location/region/repository/RegionRepositoryTest.java(1 hunks)src/test/java/com/example/solidconnection/university/service/UniversityRecommendServiceTest.java(4 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
src/main/java/com/example/solidconnection/location/region/domain/InterestedRegion.java (1)
src/main/java/com/example/solidconnection/location/country/domain/InterestedCountry.java (1)
Getter(15-40)
src/main/java/com/example/solidconnection/location/country/domain/InterestedCountry.java (1)
src/main/java/com/example/solidconnection/location/region/domain/InterestedRegion.java (1)
Getter(15-40)
src/test/java/com/example/solidconnection/location/country/repository/CountryRepositoryTest.java (1)
src/test/java/com/example/solidconnection/location/region/repository/RegionRepositoryTest.java (2)
TestContainerSpringBootTest(17-86)Nested(29-85)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (7)
src/main/java/com/example/solidconnection/location/region/domain/InterestedRegion.java (1)
18-23: 2) 유니크 제약 컬럼명 검증 필요
region필드에@JoinColumn(name = "region_code")가 없을 경우, 기본 컬럼명은region_id로 생성됩니다. 현재 제약은region_code를 지정하고 있어 스키마와 불일치 위험이 있습니다. 매핑 컬럼명을 다시 한 번 확인해 주세요.src/main/java/com/example/solidconnection/location/country/domain/InterestedCountry.java (1)
18-23: 3) Country 도 동일 이슈 가능성
country필드의 실제 FK 컬럼명이country_code가 맞는지 확인이 필요합니다. 매핑이 다르면 마이그레이션 시점에 제약 생성이 실패합니다.src/main/java/com/example/solidconnection/auth/service/EmailSignUpService.java (1)
6-7: 5) 리포지토리 임포트 교정 완료 확인
InterestedCountryRepository임포트 정상 반영되었습니다. 추가 이슈 없습니다.src/main/java/com/example/solidconnection/auth/service/oauth/OAuthSignUpService.java (1)
23-28: 이름 정정 반영 확인 완료
InterestedCountryRepository로의 리네이밍이 컨스트럭터·슈퍼 호출 모두에 정확히 반영되었습니다. 추가 이슈 없습니다.src/main/java/com/example/solidconnection/auth/service/SignUpService.java (1)
34-45: Repository 명칭 정정 OK
필드·생성자 모두에서interestedCountryRepository로 일관성 있게 교체되었습니다. 컴파일 오류 우려 없음.src/test/java/com/example/solidconnection/university/service/UniversityRecommendServiceTest.java (2)
37-38: 리포지토리 타입 교체 정상 반영
InterestedCountryRepository로의 교체로 테스트 컴파일 오류가 사라집니다. 추가 수정 사항 없습니다.
100-111: 테스트 동작 검증만 확인
저장·조회 로직 모두 새 Repository를 사용하므로 동작 자체는 그대로 유지됩니다. 별도 이슈 없습니다.Also applies to: 118-132
Gyuhyeok99
left a comment
Gyuhyeok99
left a comment
There was a problem hiding this comment.
확인했습니다! 저런 오타가 있었군요 😅
| assertThatCode(() -> { | ||
| InterestedRegion saved = interestedRegionRepository.save(secondInterest); | ||
| assertThat(saved.getId()).isNotNull(); | ||
| }).doesNotThrowAnyException(); | ||
| } |
There was a problem hiding this comment.
성공케이스에 doesNotThrowAnyException로 검증하신 이유가 있나요? 이건 순수 궁금증입니다!
보통 성공 케이스에 저는 저장하고 검증만 assertThat(saved.getId()).isNotNull(); 이런식으로 했었거든요!
There was a problem hiding this comment.
제가 작성한 테스트 코드가 예외 발생에 주안점을 두고 작성한 코드라 doesNotThrowAnyException 을 명시적으로 작성하였습니다!
src/main/java/com/example/solidconnection/location/country/domain/InterestedCountry.java
Outdated
Show resolved
Hide resolved
src/main/java/com/example/solidconnection/location/region/domain/InterestedRegion.java
Outdated
Show resolved
Hide resolved
src/main/resources/db/migration/V15__add_unique_constraint_to_intersted.sql
Outdated
Show resolved
Hide resolved
...test/java/com/example/solidconnection/location/country/repository/CountryRepositoryTest.java
Outdated
Show resolved
Hide resolved
src/test/java/com/example/solidconnection/location/region/repository/RegionRepositoryTest.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
src/test/java/com/example/solidconnection/location/region/repository/InterestedRegionRepositoryTest.java (2)
61-64: 테스트 로직을 더 명확하게 개선할 수 있습니다.현재
assertThatCode()내부에서 검증을 수행하고 있는데, 이는 예외가 발생하지 않았을 때만 실행되므로 테스트 의도가 명확하지 않을 수 있습니다.다음과 같이 개선해보세요:
// when & then -assertThatCode(() -> { - InterestedRegion saved = interestedRegionRepository.save(secondInterest); - assertThat(saved.getId()).isNotNull(); -}).doesNotThrowAnyException(); +assertThatCode(() -> interestedRegionRepository.save(secondInterest)) + .doesNotThrowAnyException(); + +InterestedRegion saved = interestedRegionRepository.save(secondInterest); +assertThat(saved.getId()).isNotNull();
80-83: 테스트 로직을 더 명확하게 개선할 수 있습니다.위의 테스트와 동일한 구조적 개선이 필요합니다.
다음과 같이 개선해보세요:
// when & then -assertThatCode(() -> { - InterestedRegion saved = interestedRegionRepository.save(secondInterest); - assertThat(saved.getId()).isNotNull(); -}).doesNotThrowAnyException(); +assertThatCode(() -> interestedRegionRepository.save(secondInterest)) + .doesNotThrowAnyException(); + +InterestedRegion saved = interestedRegionRepository.save(secondInterest); +assertThat(saved.getId()).isNotNull();src/test/java/com/example/solidconnection/location/country/repository/InterestedCountryRepositoryTest.java (3)
61-64: 테스트 로직 개선이 필요합니다.InterestedRegionRepositoryTest와 동일한 개선점이 적용됩니다.
다음과 같이 개선해보세요:
// when & then -assertThatCode(() -> { - InterestedCountry saved = interestedCountryRepository.save(secondInterest); - assertThat(saved.getId()).isNotNull(); -}).doesNotThrowAnyException(); +assertThatCode(() -> interestedCountryRepository.save(secondInterest)) + .doesNotThrowAnyException(); + +InterestedCountry saved = interestedCountryRepository.save(secondInterest); +assertThat(saved.getId()).isNotNull();
80-83: 테스트 로직 개선이 필요합니다.위의 테스트와 동일한 구조적 개선이 필요합니다.
다음과 같이 개선해보세요:
// when & then -assertThatCode(() -> { - InterestedCountry saved = interestedCountryRepository.save(secondInterest); - assertThat(saved.getId()).isNotNull(); -}).doesNotThrowAnyException(); +assertThatCode(() -> interestedCountryRepository.save(secondInterest)) + .doesNotThrowAnyException(); + +InterestedCountry saved = interestedCountryRepository.save(secondInterest); +assertThat(saved.getId()).isNotNull();
1-87: 테스트 코드 중복 제거를 고려해보세요.두 테스트 클래스가 거의 동일한 구조를 가지고 있어 향후 유지보수 시 중복 수정이 발생할 수 있습니다.
다음과 같은 방법으로 중복을 줄일 수 있습니다:
1. 공통 테스트 베이스 클래스 생성 2. 제네릭을 활용한 추상 테스트 클래스 구현 3. 테스트 유틸리티 메소드 분리향후 비슷한 엔티티가 추가될 때 재사용 가능한 구조를 고려해보세요.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/test/java/com/example/solidconnection/location/country/repository/InterestedCountryRepositoryTest.java(1 hunks)src/test/java/com/example/solidconnection/location/region/repository/InterestedRegionRepositoryTest.java(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (2)
src/test/java/com/example/solidconnection/location/region/repository/InterestedRegionRepositoryTest.java (1)
1-87: 잘 구성된 테스트 클래스입니다.복합 유니크 제약 조건을 체계적으로 테스트하는 구조가 잘 설계되어 있습니다. 네스티드 클래스를 사용한 테스트 그룹핑과 한국어 메소드명으로 가독성을 높인 점이 좋습니다.
1. 테스트 시나리오가 완전함: 동일 사용자+동일 지역, 다른 사용자+동일 지역, 동일 사용자+다른 지역 2. Spring Boot 테스트 설정이 적절함 3. 픽스쳐 사용으로 테스트 데이터 생성이 체계적임src/test/java/com/example/solidconnection/location/country/repository/InterestedCountryRepositoryTest.java (1)
1-87: 일관성 있는 테스트 구현이 잘 되어 있습니다.InterestedRegionRepositoryTest와 동일한 패턴으로 구현되어 프로젝트 전반에 걸친 일관성이 유지되고 있습니다. 복합 유니크 제약 조건 테스트가 체계적으로 구성되어 있습니다.
1. 지역 테스트와 동일한 구조로 일관성 확보 2. 국가 도메인에 특화된 픽스쳐 사용 3. 모든 제약 조건 시나리오 커버
|
제가 놓친 부분들 잘 확인해주셔서 감사합니다 영서님 ㅎㅎ |
nayonsoso
left a comment
nayonsoso
left a comment
There was a problem hiding this comment.
approve 합니다 🤗
InterestedCountryRepositoryTest 클래스와 InterestedRegionRepositoryTest클래스의 assert문에 대해서 이 코멘트#338 (comment)
에 대한 내용 적용해주시면 좋을 것 같아요!
그 후에 머지하시면 됩니다~
3 participants
관련 이슈
작업 내용
InterestedCountyRepository를InterestedCountryRepository로 수정하였습니다.특이 사항
리뷰 요구사항 (선택)