feat: 관심 지역 및 국가 수정 API 추가#453
Conversation
|
Warning Rate limit exceeded@whqtker has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 1 minutes and 23 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
Walkthrough
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
✨ 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: 5
🧹 Nitpick comments (4)
src/main/java/com/example/solidconnection/location/region/repository/InterestedRegionRepository.java (1)
6-13: 국가 레포와 동일한 패턴 — 이름 명확화·인덱스·리턴값 개선 제안기능은 충분하지만, 아래와 같이 손보면 운영·유지보수성이 좋아집니다.
- 메서드명: deleteAllBySiteUserId가 “다건 삭제” 의도를 더 잘 전달합니다.
- 삭제 카운트: long 반환으로 실제 삭제 건수 로깅/테스트가 쉬워집니다.
- 인덱스: interested_region.site_user_id에 인덱스가 없다면 추가 고려가 필요합니다(대량 삭제 성능).
- @Modifying: 파생 deleteBy 메서드에는 필수는 아니나, flush/clear 자동화를 원하면 유지해도 됩니다. 컨벤션을 정해 일관성 확보를 권장합니다.
적용 예시는 다음과 같습니다.
- @Modifying(clearAutomatically = true, flushAutomatically = true) - void deleteBySiteUserId(long siteUserId); + @Modifying(clearAutomatically = true, flushAutomatically = true) + long deleteAllBySiteUserId(long siteUserId);src/main/java/com/example/solidconnection/siteuser/controller/MyPageController.java (1)
54-61: 입력 검증 활성화(@Valid)와 204 응답 제안(선택) — 빈 바디 PATCH에는 No Content가 더 명확합니다엔드포인트는 간결하고 좋습니다. 다만 DTO에 정규화/검증을 넣는다면, 컨트롤러에서도 @Valid를 붙여 엔드-투-엔드로 검증을 활성화하는 편이 안전합니다. 응답 바디가 없는 경우 204가 REST 표현에 더 잘 맞습니다.
- @Valid 추가: DTO의 검증 애너테이션/정규화 로직이 제대로 동작하도록 보장합니다.
- 204 No Content(선택): 바디가 없는 성공 응답에 더 적합한 상태 코드입니다.
- consumes 지정(선택): JSON만 받도록 명시하면 API 계약이 분명해집니다.
필수 변경(검증 활성화):
- public ResponseEntity<Void> updateLocation( + public ResponseEntity<Void> updateLocation( @AuthorizedUser long siteUserId, - @RequestBody LocationUpdateRequest request + @RequestBody @jakarta.validation.Valid LocationUpdateRequest request ) { myPageService.updateLocation(siteUserId, request); - return ResponseEntity.ok().build(); + return ResponseEntity.ok().build(); }선택 변경(상태 코드 204):
- return ResponseEntity.ok().build(); + return ResponseEntity.noContent().build();선택 변경(consumes 명시):
- @PatchMapping("/location") + @PatchMapping(value = "/location", consumes = "application/json")src/main/java/com/example/solidconnection/location/region/service/InterestedRegionService.java (1)
32-36: ② 중복된 매핑 로직(DRY) 제거 제안
- saveInterestedRegion와 updateInterestedRegion에 동일한 매핑 로직이 반복됩니다.
- 공통 헬퍼로 추출하면 가독성과 유지보수성이 좋아집니다.
다음과 같은 헬퍼 메서드 도입을 고려해 주세요(클래스 내부 private 메서드로 추가).
private List<InterestedRegion> buildInterestedRegions(SiteUser siteUser, List<String> koreanNames) { return regionRepository.findAllByKoreanNameIn(koreanNames) .stream() .map(region -> new InterestedRegion(siteUser, region)) .toList(); }사용 예:
- saveInterestedRegion: interestedRegionRepository.saveAll(buildInterestedRegions(siteUser, koreanNames));
- updateInterestedRegion: 위 정규화·검증 로직 이후 동일하게 호출
src/test/java/com/example/solidconnection/siteuser/service/MyPageServiceTest.java (1)
260-352: ② 부분 업데이트(null 의미)와 중복 입력 처리에 대한 추가 테스트 제안
- “null 필드는 변경 없음, 빈 리스트는 전체 삭제” 의미를 보장하는 테스트가 있으면 회귀 방지에 도움이 됩니다.
- 중복 국가/지역명이 전달될 때 중복 저장이 방지되는지(서비스에서 distinct 처리 시)를 검증해 주세요.
아래 테스트를 동일 Nested 클래스(관심_지역_및_국가_변경_테스트) 내에 추가하는 것을 제안합니다.
@@ class 관심_지역_및_국가_변경_테스트 { + @Test + void 국가만_null이_아닌_경우에는_국가만_변경하고_지역은_그대로_둔다() { + // given: 초기 상태(국가=캐나다, 지역=영미권) + interestedCountryRepository.save(new InterestedCountry(user, 캐나다)); + interestedRegionRepository.save(new InterestedRegion(user, 영미권)); + + // when: 국가만 미국으로 변경, 지역은 null로 전달(변경 없음 의미) + LocationUpdateRequest request = new LocationUpdateRequest(null, List.of(미국.getKoreanName())); + myPageService.updateLocation(user.getId(), request); + + // then + List<InterestedCountry> updatedCountries = interestedCountryRepository.findAllBySiteUserId(user.getId()); + List<InterestedRegion> updatedRegions = interestedRegionRepository.findAllBySiteUserId(user.getId()); + + assertAll( + () -> assertThat(updatedCountries).hasSize(1) + .extracting(InterestedCountry::getCountryCode) + .containsExactly(미국.getCode()), + () -> assertThat(updatedRegions).hasSize(1) + .extracting(InterestedRegion::getRegionCode) + .containsExactly(영미권.getCode()) + ); + } + + @Test + void 지역만_null이_아닌_경우에는_지역만_변경하고_국가는_그대로_둔다() { + // given: 초기 상태(국가=미국, 지역=영미권) + interestedCountryRepository.save(new InterestedCountry(user, 미국)); + interestedRegionRepository.save(new InterestedRegion(user, 영미권)); + + // when: 지역만 아시아로 변경, 국가는 null로 전달(변경 없음 의미) + LocationUpdateRequest request = new LocationUpdateRequest(List.of(아시아.getKoreanName()), null); + myPageService.updateLocation(user.getId(), request); + + // then + List<InterestedCountry> updatedCountries = interestedCountryRepository.findAllBySiteUserId(user.getId()); + List<InterestedRegion> updatedRegions = interestedRegionRepository.findAllBySiteUserId(user.getId()); + + assertAll( + () -> assertThat(updatedCountries).hasSize(1) + .extracting(InterestedCountry::getCountryCode) + .containsExactly(미국.getCode()), + () -> assertThat(updatedRegions).hasSize(1) + .extracting(InterestedRegion::getRegionCode) + .containsExactly(아시아.getCode()) + ); + } + + // 아래 테스트는 서비스에서 distinct 처리(또는 DB 유니크 제약)가 추가된 경우 활성화하세요. + @Test + void 중복된_국가와_지역명이_전달되어도_중복_저장되지_않는다() { + // given + List<String> dupCountries = List.of(미국.getKoreanName(), 미국.getKoreanName(), 미국.getKoreanName()); + List<String> dupRegions = List.of(영미권.getKoreanName(), 영미권.getKoreanName()); + LocationUpdateRequest request = new LocationUpdateRequest(dupRegions, dupCountries); + + // when + myPageService.updateLocation(user.getId(), request); + + // then + List<InterestedCountry> updatedCountries = interestedCountryRepository.findAllBySiteUserId(user.getId()); + List<InterestedRegion> updatedRegions = interestedRegionRepository.findAllBySiteUserId(user.getId()); + + assertAll( + () -> assertThat(updatedCountries).hasSize(1) + .extracting(InterestedCountry::getCountryCode) + .containsExactly(미국.getCode()), + () -> assertThat(updatedRegions).hasSize(1) + .extracting(InterestedRegion::getRegionCode) + .containsExactly(영미권.getCode()) + ); + } }
📜 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 settings in your CodeRabbit configuration.
📒 Files selected for processing (8)
src/main/java/com/example/solidconnection/location/country/repository/InterestedCountryRepository.java(1 hunks)src/main/java/com/example/solidconnection/location/country/service/InterestedCountryService.java(1 hunks)src/main/java/com/example/solidconnection/location/region/repository/InterestedRegionRepository.java(1 hunks)src/main/java/com/example/solidconnection/location/region/service/InterestedRegionService.java(1 hunks)src/main/java/com/example/solidconnection/siteuser/controller/MyPageController.java(2 hunks)src/main/java/com/example/solidconnection/siteuser/dto/LocationUpdateRequest.java(1 hunks)src/main/java/com/example/solidconnection/siteuser/service/MyPageService.java(3 hunks)src/test/java/com/example/solidconnection/siteuser/service/MyPageServiceTest.java(4 hunks)
⏰ 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 (3)
src/main/java/com/example/solidconnection/siteuser/controller/MyPageController.java (1)
5-5: 필요한 DTO 임포트 추가는 적절합니다LocationUpdateRequest 사용을 위한 임포트가 정확합니다. 별도 이슈 없습니다.
src/main/java/com/example/solidconnection/siteuser/service/MyPageService.java (1)
39-40: ① 서비스 의존성 주입 추가는 적절합니다
- 관심 국가/지역 서비스 주입으로 역할 분리가 명확합니다.
src/test/java/com/example/solidconnection/siteuser/service/MyPageServiceTest.java (1)
260-352: ① E2E 시나리오 3건(갱신/추가/비움) 커버 우수
- 기존 관심사 교체, 신규 추가, 빈 리스트 삭제까지 핵심 경로가 잘 검증되었습니다.
- 픽스처/레포지토리 연결도 일관적입니다.
...ava/com/example/solidconnection/location/country/repository/InterestedCountryRepository.java
Outdated
Show resolved
Hide resolved
...main/java/com/example/solidconnection/location/country/service/InterestedCountryService.java
Show resolved
Hide resolved
src/main/java/com/example/solidconnection/location/region/service/InterestedRegionService.java
Show resolved
Hide resolved
src/main/java/com/example/solidconnection/siteuser/dto/LocationUpdateRequest.java
Show resolved
Hide resolved
src/main/java/com/example/solidconnection/siteuser/service/MyPageService.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/main/java/com/example/solidconnection/location/country/repository/InterestedCountryRepository.java (1)
11-11: 지역 레포와 일관된 네이밍/반환형으로 정리하여 가독성과 관측성 개선.한 문장으로 요약하면, Line [11]의 deleteBySiteUserId는 동작은 안전하지만, deleteAllBySiteUserId로 이름을 바꾸고 long을 반환하면 더 좋습니다. 이전 코멘트와 같은 제안이며, 두 레포 간 일관성도 확보됩니다.
- 이름 정리: 여러 건 삭제 의도가 드러나도록 deleteAllBySiteUserId로 변경을 권장합니다.
- 반환값 추가: void → long으로 바꿔 삭제 건수를 로그/모니터링/테스트에서 활용하세요.
- @Modifying 옵션: 필요 시 clearAutomatically/flushAutomatically를 켜서 즉시 일관성을 확보할 수 있습니다. 팀 컨벤션에 맞춰 선택하세요.
- 호출부 영향: 서비스의 deleteBySiteUserId 호출부를 전부 deleteAllBySiteUserId로 바꿔주세요. 삭제 건수 반환을 사용해 로그를 남기면 운영에 도움이 됩니다.
- 성능 메모: interested_country.site_user_id 컬럼에 인덱스가 있는지 확인하면 배치 삭제 성능이 일정해집니다.
적용 diff(해당 줄 범위 내 변경):
- void deleteBySiteUserId(long siteUserId); + long deleteAllBySiteUserId(long siteUserId);선택 사항으로 @Modifying을 붙일 경우:
import org.springframework.data.jpa.repository.Modifying; @Modifying(clearAutomatically = true, flushAutomatically = true) long deleteAllBySiteUserId(long siteUserId);간단 검증 스크립트:
- 서비스 호출부에서 메서드명을 일괄 치환해야 하는지 점검합니다.
- InterestedCountry 엔티티가 siteUserId(또는 siteUser.id 경로)를 보유하는지 재확인합니다.
#!/bin/bash set -euo pipefail echo "1) 서비스/콜사이트에서 deleteBySiteUserId 호출 확인" rg -n -A 2 -B 2 $'deleteBySiteUserId\\(' src/main/java || true echo echo "2) InterestedCountry 엔티티에서 siteUserId/관계 필드 확인" fd InterestedCountry.java | while read -r f; do echo "---- $f" sed -n '1,200p' "$f" | rg -n -A 2 -B 2 $'class|siteUserId|siteUser' done
📜 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 settings in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/main/java/com/example/solidconnection/location/country/repository/InterestedCountryRepository.java(1 hunks)src/main/java/com/example/solidconnection/location/region/repository/InterestedRegionRepository.java(1 hunks)
|
|
||
| List<InterestedRegion> findAllBySiteUserId(long siteUserId); | ||
|
|
||
| void deleteBySiteUserId(long siteUserId); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
여러 건 삭제 의도 명확화와 관측성 향상 제안(이름/반환값/옵션 플러시).
한 문장으로 요약하면, Line [11]의 파생 삭제 메서드는 동작은 문제없지만, 이름과 반환형을 다듬으면 유지보수성과 관측성이 좋아집니다.
-
- 이름 정리: 여러 건을 지우는 메서드임이 드러나도록 메서드명을 deleteAllBySiteUserId로 바꾸면 가독성이 올라갑니다.
-
- 반환값 추가: void 대신 long을 반환해 삭제된 행 수를 알 수 있게 하면 운영 로그/테스트 단언에 유용합니다.
-
- 플러시/클리어 옵션: @Modifying(clearAutomatically = true, flushAutomatically = true)를 붙이면 영속성 컨텍스트 동기화가 명시적이라, 이후 즉시 조회 일관성이 필요할 때 도움이 됩니다. 선택 사항입니다.
-
- 트랜잭션 경계: 서비스 레이어에 @transactional이 이미 붙어 있다면 충분합니다. 그렇지 않다면 추가를 고려해주세요.
-
- 성능 메모: 대량 삭제 빈도가 있다면 interested_region.site_user_id 컬럼 인덱스를 점검해 주세요. 하나의 DELETE … WHERE site_user_id = ? 문으로 실행되어도 인덱스가 없으면 테이블 스캔이 발생합니다.
적용 diff(해당 줄 범위 내 변경):
- void deleteBySiteUserId(long siteUserId);
+ long deleteAllBySiteUserId(long siteUserId);추가로, 선택적 어노테이션 및 import는 아래처럼 반영 가능합니다(선택):
// 상단 import
import org.springframework.data.jpa.repository.Modifying;
// 메서드 선언부에 부착
@Modifying(clearAutomatically = true, flushAutomatically = true)
long deleteAllBySiteUserId(long siteUserId);검증 스크립트 제공:
-
- InterestedRegion 엔티티에 siteUserId(또는 siteUser.id 경로)가 실제로 존재하는지 확인합니다.
-
- InterestedRegionService.updateInterestedRegion에 @transactional이 있는지 확인합니다.
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "1) InterestedRegion 엔티티에서 siteUserId/관계 필드 확인"
fd InterestedRegion.java | while read -r f; do
echo "---- $f"
sed -n '1,200p' "$f" | rg -n -A 2 -B 2 $'class|siteUserId|siteUser'
done
echo
echo "2) InterestedRegionService의 @Transactional 확인 및 updateInterestedRegion 시그니처 확인"
fd InterestedRegionService.java | while read -r f; do
echo "---- $f"
rg -n -A 3 -B 3 $'@Transactional|class|updateInterestedRegion' "$f" || true
doneLength of output: 2358
여러 건 삭제 의도 명확화 및 반환값 권장 — 검증 완료(조치 권장)
검증: InterestedRegion 엔티티에 siteUserId 필드가 존재하며, InterestedRegionService.updateInterestedRegion은 @transactional로 보호되어 있습니다.
- 이름 정리: deleteBySiteUserId를 deleteAllBySiteUserId로 변경해 다건 삭제 의도를 명시하세요.
- 반환값 추가: void 대신 long을 반환해 삭제된 행 수를 운영 로그/테스트에서 확인 가능하게 하세요.
- @Modifying 옵션(선택): 필요 시 @Modifying(clearAutomatically = true, flushAutomatically = true)를 붙여 영속성 컨텍스트 동기화를 보장하세요.
- 트랜잭션 경계 확인: 현재 updateInterestedRegion에 @transactional이 있으므로 서비스 레벨에서 트랜잭션은 확보되어 있습니다.
- 성능 점검: 대량 삭제가 빈번하면 interested_region.site_user_id 컬럼에 인덱스가 있는지 확인하세요.
수정/확인 대상 파일:
- src/main/java/com/example/solidconnection/location/region/repository/InterestedRegionRepository.java — 메서드 선언 변경 필요 (라인 ~11).
- src/main/java/com/example/solidconnection/location/region/service/InterestedRegionService.java — 호출부 업데이트 필요: interestedRegionRepository.deleteBySiteUserId(...) → deleteAllBySiteUserId(...).
- src/main/java/com/example/solidconnection/location/region/domain/InterestedRegion.java — siteUserId 필드 존재 확인됨.
권장 diff(해당 줄 범위 내 변경):
- void deleteBySiteUserId(long siteUserId);
+ @Modifying(clearAutomatically = true, flushAutomatically = true)
+ long deleteAllBySiteUserId(long siteUserId);선택적 import:
import org.springframework.data.jpa.repository.Modifying;위 검증 결과를 바탕으로 위 변경을 권장합니다.
🤖 Prompt for AI Agents
In
src/main/java/com/example/solidconnection/location/region/repository/InterestedRegionRepository.java
around line 11, rename the repository method deleteBySiteUserId to
deleteAllBySiteUserId and change its return type from void to long so it returns
the number of deleted rows; optionally annotate the method with
@Modifying(clearAutomatically = true, flushAutomatically = true) (add the import
if used) to ensure persistence-context sync for bulk deletes, and then update
the service call in
src/main/java/com/example/solidconnection/location/region/service/InterestedRegionService.java
to invoke interestedRegionRepository.deleteAllBySiteUserId(...); also verify
interested_region.site_user_id is indexed if bulk deletes are frequent.
There was a problem hiding this comment.
deleteBySiteUserId를 deleteAllBySiteUserId로 변경해 다건 삭제 의도를 명시하세요.
이건 반영하는게 좋겠습니다!
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
nayonsoso
left a comment
nayonsoso
left a comment
There was a problem hiding this comment.
빠른 merge를 위해 approve 드립니다!
bruno 업데이트도 잊지 말아주세요 ㅎㅎ
이 PR을 읽으면서,..
성혁님이 코드를 잘 작성하셔서 제가 걱정하지 않고 떠나도 괜찮겠다는 생각을 했습니다🤗
|
|
||
| List<InterestedRegion> findAllBySiteUserId(long siteUserId); | ||
|
|
||
| void deleteBySiteUserId(long siteUserId); |
There was a problem hiding this comment.
deleteBySiteUserId를 deleteAllBySiteUserId로 변경해 다건 삭제 의도를 명시하세요.
이건 반영하는게 좋겠습니다!
| public ResponseEntity<Void> updateLocation( | ||
| @AuthorizedUser long siteUserId, | ||
| @RequestBody LocationUpdateRequest request | ||
| ) { |
There was a problem hiding this comment.
| ) { | |
| ) { |
포매팅 해주세요~
| @PatchMapping("/location") | ||
| public ResponseEntity<Void> updateLocation( | ||
| @AuthorizedUser long siteUserId, | ||
| @RequestBody LocationUpdateRequest request |
There was a problem hiding this comment.
| @RequestBody LocationUpdateRequest request | |
| @RequestBody @Valid LocationUpdateRequest request |
@Valid 어노테이션이 있어야 Dto 어노테이션 검증이 가능합니다!
There was a problem hiding this comment.
헛 ... 이걸 빼먹다니 ... 수정했습니다
| countryRepository.saveAll(List.of(미국, 캐나다, 일본)); | ||
| regionRepository.saveAll(List.of(영미권, 아시아)); |
There was a problem hiding this comment.
fixture의 함수를 호출하기만 하더라도 repository에 저장되므로
이 두 라인은 없어도 됩니다~
| assertAll( | ||
| () -> assertThat(updatedCountries).hasSize(2) | ||
| .extracting(InterestedCountry::getCountryCode) | ||
| .containsExactlyInAnyOrder(캐나다.getCode(), 일본.getCode()), | ||
| () -> assertThat(updatedRegions).hasSize(1) | ||
| .extracting(InterestedRegion::getRegionCode) | ||
| .containsExactly(아시아.getCode()) | ||
| ); |
There was a problem hiding this comment.
정말 사소한 부분이긴 한데요..! 😓
containsExactlyInAnyOrder나 containsExactly같은 함수는
요소 자체 뿐 아니라 사이즈에 대한 검증도 포함이 되어있기 때문에
hasSize(2)가 없어도 될 것 같습니다.
There was a problem hiding this comment.
'exactly' 가 크기 검증도 수행한다는 거였군요 ... 크기 관련 검증 제거하였습니다 !
src/main/java/com/example/solidconnection/siteuser/service/MyPageService.java
Show resolved
Hide resolved
src/main/java/com/example/solidconnection/location/region/service/InterestedRegionService.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/main/java/com/example/solidconnection/location/region/repository/InterestedRegionRepository.java (1)
11-11: 1) 삭제 건수 관측을 위해 반환형을 long으로 바꾸면 운영/테스트 가시성이 좋아집니다.한 줄만 바꿔도 로그/단언에서 즉시 활용 가능한 삭제 건수를 얻을 수 있어요. Line [11]의 시그니처만 다음처럼 손봐 주세요.
- void deleteAllBySiteUserId(long siteUserId); + long deleteAllBySiteUserId(long siteUserId);선택 사항(권장): 벌크 삭제 후 영속성 컨텍스트 동기화가 필요할 수 있으니, 아래처럼 @Modifying 옵션을 붙이는 것도 고려해 보세요.
+ @org.springframework.data.jpa.repository.Modifying(clearAutomatically = true, flushAutomatically = true) long deleteAllBySiteUserId(long siteUserId);필요 시 별도 import:
import org.springframework.data.jpa.repository.Modifying;
🧹 Nitpick comments (6)
src/main/java/com/example/solidconnection/location/region/repository/InterestedRegionRepository.java (2)
9-11: 2) 파라미터 타입을 Long으로 통일하면 레포지토리 간 일관성이 올라갑니다.다른 레포지토리(InterestedCountryRepository 등)와의 일관성 및 JPA 관례 측면에서 wrapper 타입 사용을 권장합니다. Line [9], Line [11]의 파라미터를 Long으로 맞춰 주세요.
- List<InterestedRegion> findAllBySiteUserId(long siteUserId); + List<InterestedRegion> findAllBySiteUserId(Long siteUserId); - long deleteAllBySiteUserId(long siteUserId); + long deleteAllBySiteUserId(Long siteUserId);
11-11: 3) 성능 메모: site_user_id 인덱스를 점검해 주세요.사용자 단위 벌크 삭제가 빈번하다면 interested_region.site_user_id에 인덱스가 없을 경우 테이블 스캔이 발생합니다. 운영 트래픽/데이터 규모를 고려해 인덱스 존재 여부를 확인해 두는 것을 권장드립니다.
src/test/java/com/example/solidconnection/siteuser/service/MyPageServiceTest.java (4)
252-341: 4) 입력 중복 처리에 대한 보강 테스트를 추가하면 품질이 올라갑니다.중복 한국어명이 요청으로 들어올 때(예: 일본, 일본), 중복 관심사 레코드가 생성되지 않아야 합니다. 서비스가 내부적으로 Set 처리 또는 DB 유니크 제약으로 방어하고 있는지를 테스트로 고정하면 안정성이 올라갑니다.
예시 테스트 스케치:
@Test void 중복된_국가_또는_지역_이름이_전달되어도_중복_저장은_되지_않는다() { List<String> newCountries = List.of(일본.getKoreanName(), 일본.getKoreanName()); List<String> newRegions = List.of(영미권.getKoreanName(), 영미권.getKoreanName()); LocationUpdateRequest request = new LocationUpdateRequest(newRegions, newCountries); myPageService.updateLocation(user.getId(), request); List<InterestedCountry> updatedCountries = interestedCountryRepository.findAllBySiteUserId(user.getId()); List<InterestedRegion> updatedRegions = interestedRegionRepository.findAllBySiteUserId(user.getId()); assertAll( () -> assertThat(updatedCountries).extracting(InterestedCountry::getCountryCode) .containsExactly(일본.getCode()), () -> assertThat(updatedRegions).extracting(InterestedRegion::getRegionCode) .containsExactly(영미권.getCode()) ); }원하시면 PR에 맞춰 실제 코드로 생성해 드리겠습니다.
276-279: 5) 존재하지 않는 한국어명이 전달될 때의 정책을 테스트로 고정해 주세요.미존재 이름이 포함되면 무시하고 나머지로 저장할지, 전체 요청을 실패시킬지 정책이 명확하면 좋습니다. 현재 로직상 findAllByKoreanNameIn 결과만 저장되므로 일부/전체 미존재 케이스별 기대 동작을 테스트에 추가해 주세요.
Also applies to: 300-303, 327-327
95-98: 6) 테스트 격리 보장 확인(선택).user는 @beforeeach에서 생성되지만, 트랜잭션 롤백/테이블 클린업 전략에 따라 데이터가 누적될 수 있습니다. 현재 테스트들은 user.getId()를 기준으로만 조회하므로 큰 문제는 없어 보이나, 테스트 격리 보장을 위해 각 테스트가 독립 사용자(예: FixtureBuilder로 매번 신규 생성)를 쓰거나 클래스/메서드 트랜잭션 롤백을 확실히 하는지 확인해 주세요.
Also applies to: 252-341
284-286: 7) 중복된 조회/추출 코드를 헬퍼로 추출하면 가독성이 더 좋아집니다.업데이트 후 결과 조회/추출이 3곳에서 반복됩니다. 간단한 private 헬퍼로 중복을 줄일 수 있습니다.
예시:
private List<String> getCountryCodesOf(Long userId) { return interestedCountryRepository.findAllBySiteUserId(userId) .stream().map(InterestedCountry::getCountryCode).toList(); } private List<String> getRegionCodesOf(Long userId) { return interestedRegionRepository.findAllBySiteUserId(userId) .stream().map(InterestedRegion::getRegionCode).toList(); }Also applies to: 308-310, 333-335
📜 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 (6)
src/main/java/com/example/solidconnection/location/country/repository/InterestedCountryRepository.java(1 hunks)src/main/java/com/example/solidconnection/location/country/service/InterestedCountryService.java(1 hunks)src/main/java/com/example/solidconnection/location/region/repository/InterestedRegionRepository.java(1 hunks)src/main/java/com/example/solidconnection/location/region/service/InterestedRegionService.java(1 hunks)src/main/java/com/example/solidconnection/siteuser/controller/MyPageController.java(2 hunks)src/test/java/com/example/solidconnection/siteuser/service/MyPageServiceTest.java(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/main/java/com/example/solidconnection/location/country/service/InterestedCountryService.java
- src/main/java/com/example/solidconnection/location/country/repository/InterestedCountryRepository.java
- src/main/java/com/example/solidconnection/location/region/service/InterestedRegionService.java
- src/main/java/com/example/solidconnection/siteuser/controller/MyPageController.java
🔇 Additional comments (3)
src/test/java/com/example/solidconnection/siteuser/service/MyPageServiceTest.java (3)
270-295: 1) 교체 시나리오 테스트가 명확하고 기대 결과가 잘 드러납니다.기존 관심사를 세팅한 뒤 교체되는지 확인하는 플로우가 깔끔합니다. 코드/한글명 매핑을 검증하는 단언도 적절합니다.
297-319: 2) 신규 추가 시나리오도 자연스럽습니다.초기 상태에서 추가가 정상 동작하는지 검증이 선명하고, 단언도 정확합니다.
321-341: 3) 빈 리스트로 삭제 시나리오가 잘 커버되었습니다.삭제 후 조회 결과가 비어 있음을 직접 확인하는 방식이 안전합니다.
Gyuhyeok99
left a comment
Gyuhyeok99
left a comment
There was a problem hiding this comment.
고생하셨습니다~ 아주 깔끔하게 잘 구현하신 거 같네요!
다른 분들의 의견이 궁금한 네이밍 하나만 의견 내보겠습니다..!
그냥 /location 으로 하면 my/location인데 이게 관심있는 지역이라는 걸 잘 명시해주지 못한다는 생각이 들었습니다..!
혹시 /my/interest-location 이렇게 확실히 명시하는 건 어떤가요?
그렇게한다면 함수명도 updateInterestedLocation로 바뀌어야할 거 같긴 합니다!
@nayonsoso @wibaek
더 나은 의견이 있다면 알려주세요!
제 의견도 살짝 덧붙여보자면, 규혁 님 의견처럼 의미적으로 분명하지 않다는 점에 동의합니다. |
|
영서님도 괜찮으면 그렇게 하시죠!! |
|
생각해보니 그렇네요😅 규혁님이 잘 짚어주셨습니다. |
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
src/test/java/com/example/solidconnection/siteuser/service/MyPageServiceTest.java (1)
3-13: 컴파일 에러: 누락된 import 추가가 필요합니다 (PasswordUpdateRequest, assertAll, assertThatThrownBy, PASSWORD_MISMATCH).아래 4가지가 없어 컴파일 에러가 발생합니다. 다음과 같이 import를 추가해 주세요.
- PasswordUpdateRequest 클래스 import.
- assertAll 정적 import.
- assertThatThrownBy 정적 import.
- PASSWORD_MISMATCH 정적 import.
적용 diff:
import static com.example.solidconnection.common.exception.ErrorCode.CAN_NOT_CHANGE_NICKNAME_YET; +import static com.example.solidconnection.common.exception.ErrorCode.PASSWORD_MISMATCH; import static com.example.solidconnection.siteuser.service.MyPageService.MIN_DAYS_BETWEEN_NICKNAME_CHANGES; import static com.example.solidconnection.siteuser.service.MyPageService.NICKNAME_LAST_CHANGE_DATE_FORMAT; import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; +import static org.junit.jupiter.api.Assertions.assertAll; import static org.mockito.BDDMockito.any; import static org.mockito.BDDMockito.eq; import static org.mockito.BDDMockito.given; import static org.mockito.BDDMockito.never; import static org.mockito.BDDMockito.then; import com.example.solidconnection.common.exception.CustomException; +import com.example.solidconnection.siteuser.dto.PasswordUpdateRequest;Also applies to: 14-33
📜 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 (2)
src/main/java/com/example/solidconnection/siteuser/service/MyPageService.java(4 hunks)src/test/java/com/example/solidconnection/siteuser/service/MyPageServiceTest.java(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/com/example/solidconnection/siteuser/service/MyPageService.java
🧰 Additional context used
🪛 GitHub Actions: CI with Gradle
src/test/java/com/example/solidconnection/siteuser/service/MyPageServiceTest.java
[error] 87-87: variable countryFixture is already defined in class MyPageServiceTest
[error] 300-300: PasswordUpdateRequest cannot be found
[error] 307-307: Cannot find symbol: method assertAll(...)
[error] 317-317: PasswordUpdateRequest cannot be found
[error] 320-320: Cannot find symbol: method assertAll(...)
[error] 322-322: PasswordUpdateRequest cannot be found
[error] 324-324: PasswordUpdateRequest cannot be found
[error] 361-361: Cannot find symbol: method assertAll(...)
[error] 383-383: Cannot find symbol: method assertAll(...)
[error] 385-385: Cannot find symbol: method assertAll(...)
[error] 410-410: Cannot find symbol: method assertAll(...)
🔇 Additional comments (2)
src/test/java/com/example/solidconnection/siteuser/service/MyPageServiceTest.java (2)
344-414: 관심 지역/국가 변경 테스트 커버리지 좋습니다.세 가지 핵심 시나리오를 자연스럽게 검증하고 있어 신뢰도가 높습니다.
- 교체: 기존 관심값을 새로운 목록으로 완전히 대체합니다.
- 신규: 기존이 비어있는 경우에도 정상 추가됩니다.
- 삭제: 빈 리스트로 전체 삭제 동작을 보장합니다.
71-73: 관심 지역 레포지토리 주입 추가 적절합니다.updateLocation 흐름에서 상태 검증을 위해 실제 저장소를 조회하는 테스트 구성과 잘 맞습니다.
src/test/java/com/example/solidconnection/siteuser/service/MyPageServiceTest.java
Outdated
Show resolved
Hide resolved
src/test/java/com/example/solidconnection/siteuser/service/MyPageServiceTest.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/test/java/com/example/solidconnection/siteuser/service/MyPageServiceTest.java (1)
414-451: (6) 중복 정의된 헬퍼 메서드 제거 필요 — 현재 컴파일 실패 원인파일 상단(이미 존재)과 동일 시그니처 메서드 4개가 하단에 중복되어 있습니다. 파이프라인 로그의 컴파일 에러는 이 중복 때문입니다. 하단 블록 전체를 삭제하세요.
적용 diff(하단 중복 블록 전체 제거):
- private int createLikedUnivApplyInfos(SiteUser testUser) { - LikedUnivApplyInfo likedUnivApplyInfo1 = new LikedUnivApplyInfo(null, univApplyInfoFixture.괌대학_A_지원_정보().getId(), testUser.getId()); - LikedUnivApplyInfo likedUnivApplyInfo2 = new LikedUnivApplyInfo(null, univApplyInfoFixture.메이지대학_지원_정보().getId(), testUser.getId()); - LikedUnivApplyInfo likedUnivApplyInfo3 = new LikedUnivApplyInfo(null, univApplyInfoFixture.코펜하겐IT대학_지원_정보().getId(), testUser.getId()); - - likedUnivApplyInfoRepository.save(likedUnivApplyInfo1); - likedUnivApplyInfoRepository.save(likedUnivApplyInfo2); - likedUnivApplyInfoRepository.save(likedUnivApplyInfo3); - return likedUnivApplyInfoRepository.countBySiteUserId(testUser.getId()); - } - - private MockMultipartFile createValidImageFile() { - return new MockMultipartFile( - "image", - "test.jpg", - "image/jpeg", - "test image content".getBytes() - ); - } - - private String createExpectedErrorMessage(LocalDateTime modifiedAt) { - String formatLastModifiedAt = String.format( - "(마지막 수정 시간 : %s)", - NICKNAME_LAST_CHANGE_DATE_FORMAT.format(modifiedAt) - ); - return CAN_NOT_CHANGE_NICKNAME_YET.getMessage() + " : " + formatLastModifiedAt; - } - - private SiteUser createSiteUserWithCustomProfile() { - return siteUserFixtureBuilder.siteUser() - .email("customProfile@example.com") - .authType(AuthType.EMAIL) - .nickname("커스텀프로필") - .profileImageUrl("profile/profileImageUrl") - .role(Role.MENTEE) - .password("customPassword123") - .create(); - }
🧹 Nitpick comments (1)
src/test/java/com/example/solidconnection/siteuser/service/MyPageServiceTest.java (1)
323-413: (7) 선택 제안: 예외 케이스와 멱등성 테스트를 2건 추가해보면 좋아요관심 지역/국가 갱신 API 특성상 다음 두 케이스를 보강하면 회귀 리스크를 더 낮출 수 있습니다.
- 7-1. 존재하지 않는 국가/지역명이 포함된 경우 예외 혹은 무시 동작 검증(현재 서비스 정책에 맞춰 기대값 정의)
- 7-2. 동일한 목록으로 연속 PATCH 시 DB 변화가 없는지(멱등성) 검증
원하시면 스켈레톤 테스트 코드를 이어서 제안드릴게요.
📜 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 (1)
src/test/java/com/example/solidconnection/siteuser/service/MyPageServiceTest.java(5 hunks)
🧰 Additional context used
🪛 GitHub Actions: CI with Gradle
src/test/java/com/example/solidconnection/siteuser/service/MyPageServiceTest.java
[error] 414-414: method createLikedUnivApplyInfos(SiteUser) is already defined in class MyPageServiceTest
[error] 425-425: method createValidImageFile() is already defined in class MyPageServiceTest
[error] 434-434: method createExpectedErrorMessage(LocalDateTime) is already defined in class MyPageServiceTest
[error] 442-442: method createSiteUserWithCustomProfile() is already defined in class MyPageServiceTest
[error] 297-297: Cannot find symbol: PasswordUpdateRequest
[error] 304-304: Cannot find symbol: assertAll
[error] 314-314: Cannot find symbol: PasswordUpdateRequest
[error] 317-317: Cannot find symbol: assertThatThrownBy
[error] 319-319: Cannot find symbol: hasMessage(PASSWORD_MISMATCH)
[error] 358-358: Cannot find symbol: assertAll
[error] 382-382: Cannot find symbol: assertAll
[error] 407-407: Cannot find symbol: assertAll
🔇 Additional comments (3)
src/test/java/com/example/solidconnection/siteuser/service/MyPageServiceTest.java (3)
19-22: (1) 지역 도메인/리포지토리 import 추가는 적절합니다관심 지역 기능 테스트를 위한 도메인/픽스처/리포지토리 의존성 주입 준비가 깔끔합니다.
71-73: (2) InterestedRegionRepository 주입 추가 OK테스트 내 검증을 위해 리포지토리를 직접 조회하는 패턴과 잘 맞습니다.
86-88: (3) RegionFixture 주입 추가 OK국가와 동일한 방식으로 지역 픽스처도 주입하여 일관성이 좋습니다.
src/test/java/com/example/solidconnection/siteuser/service/MyPageServiceTest.java
Show resolved
Hide resolved
src/test/java/com/example/solidconnection/siteuser/service/MyPageServiceTest.java
Outdated
Show resolved
Hide resolved
whqtker
force-pushed
the
feat/447-add-updating-interested-country-region-api
branch
from
cd327ef to
6abb108
Compare
- /location -> /interested-location
3 participants
관련 이슈
작업 내용
관심 국가 및 지역을 수정하는 API를 구현하였습니다.
UI 상에는 '관심 국가 변경' 탭만 있으나, 이 탭에서 지역도 변경할 수 있는 것으로 간주하고 구현하였습니다.사용자의
InterestedXXX삭제 후 요청의interestedXXX를 추가하는 방식으로 구현했습니다.특이 사항
리뷰 요구사항 (선택)