refactor: Siteuser 도메인 리팩터링 #336
Conversation
- SiteUser 엔티티의 nickname 컬럼에 unique 제약 조건 추가
- MyPageService에서 중복 닉네임 검증 로직 제거 - 컨트롤러에서 try-catch 문을 통해 예외 처리
- 변경된 컬럼만 업데이트하는 쿼리 사용
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 (
|
- nickname 제약 조건의 이름 명시적으로 지정
whqtker
requested review from
Gyuhyeok99,
lsy1307,
nayonsoso 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: 6
🧹 Nitpick comments (2)
src/main/java/com/example/solidconnection/auth/service/SignUpService.java (1)
70-74: 3) 더 이상 호출되지 않는 메서드 정리 필요
validateNicknameDuplicated는 현재 서비스 로직에서 호출되지 않습니다.
Dead code 는 유지보수를 방해하므로 삭제를 권장합니다.src/main/java/com/example/solidconnection/siteuser/service/MyPageService.java (1)
51-61: 2) JPQL bulk update 후 영속성 컨텍스트 불일치
updateNickname/updateProfileImage는 JPQLUPDATE이므로 1차 캐시에 남아있는siteUser엔티티의 필드가 갱신되지 않습니다.
메서드 종료 이후 동일 세션에서 엔티티를 다시 사용할 경우 데이터가 오래된 상태로 남아있어 의도치 않은 버그가 발생할 수 있습니다.
EntityManager.clear()혹은refresh(siteUser)호출- or 메서드 매개변수로 엔티티 대신 ID 만 받도록 변경
중 하나로 일관성을 확보해 주세요.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/main/java/com/example/solidconnection/auth/service/SignUpService.java(4 hunks)src/main/java/com/example/solidconnection/common/exception/ErrorCode.java(1 hunks)src/main/java/com/example/solidconnection/siteuser/controller/MyPageController.java(3 hunks)src/main/java/com/example/solidconnection/siteuser/domain/SiteUser.java(1 hunks)src/main/java/com/example/solidconnection/siteuser/repository/SiteUserRepository.java(2 hunks)src/main/java/com/example/solidconnection/siteuser/service/MyPageService.java(2 hunks)src/main/resources/db/migration/V13__set_unique_constraint_to_nickname.sql(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (3)
src/main/java/com/example/solidconnection/common/exception/ErrorCode.java (1)
99-101: 🎉 신규 에러코드 추가, 문제 없습니다
DATA_INTEGRITY_VIOLATION상수의 코드·메시지·HTTP 상태값이 기존 규칙과 일관성을 유지합니다. 다른 계층에서 재활용하기에 충분히 명확해 보여요. 수고하셨습니다!src/main/resources/db/migration/V13__set_unique_constraint_to_nickname.sql (1)
1-3: 1️⃣ 운영 DB에 적용 전, 중복 데이터 존재 여부 꼭 확인하세요이미 중복 닉네임이 저장돼 있다면 마이그레이션이 실패해 배포가 중단됩니다. 배포 직전에 다음 쿼리를 한 번 실행해 안전을 확보해 주세요.
SELECT nickname, COUNT(*) FROM site_user GROUP BY nickname HAVING COUNT(*) > 1;중복 레코드가 있다면 미리 정리하거나 임시 처리 정책을 마련하는 것을 권장드립니다.
src/main/java/com/example/solidconnection/siteuser/domain/SiteUser.java (1)
38-42: 2️⃣ 닉네임 유니크 제약 추가 확인엔티티에 제약 명시까지 반영된 점 아주 좋습니다. DB-스키마와 JPA 설정이 싱크를 맞추도록 명시적 이름을 사용한 것도 👍🏻 입니다.
src/main/java/com/example/solidconnection/siteuser/controller/MyPageController.java
Outdated
Show resolved
Hide resolved
src/main/java/com/example/solidconnection/siteuser/controller/MyPageController.java
Outdated
Show resolved
Hide resolved
src/main/java/com/example/solidconnection/siteuser/repository/SiteUserRepository.java
Outdated
Show resolved
Hide resolved
src/main/java/com/example/solidconnection/auth/service/SignUpService.java
Outdated
Show resolved
Hide resolved
src/main/java/com/example/solidconnection/auth/service/SignUpService.java
Outdated
Show resolved
Hide resolved
src/main/java/com/example/solidconnection/siteuser/service/MyPageService.java
Outdated
Show resolved
Hide resolved
- 게시글_좋아요_동시성_문제를_해결한다: 동일한 닉네임을 사용하지 않도록 변경 - 이메일이_같더라도_인증_유형이_다른_사용자는_정상_저장한다: 동일한 닉네임을 사용하지 않도록 변경 - 닉네임 무결성 테스트는 repository test로 분리 - 새로운_이미지로_성공적으로_업데이트한다: DB에서 사용자 조회 후 URL 비교
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/test/java/com/example/solidconnection/concurrency/PostLikeCountConcurrencyTest.java (1)
24-24: 대량 사용자 생성 루프의 트랜잭션 비용 검토 제안
- 현재
save호출을 1,000 번 반복하면서 각 건마다 즉시 flush 하지 않더라도 커넥션 풀 소진·성능 저하 가능성이 있습니다.saveAll+ 배치 flush 또는EntityManagerlevel batch size 설정으로 테스트 시간을 줄일 수 있습니다.- 테스트 목적에 영향 없으므로 성능을 조금 아껴보시는 건 어떨까요?
Also applies to: 95-97
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/test/java/com/example/solidconnection/concurrency/PostLikeCountConcurrencyTest.java(2 hunks)src/test/java/com/example/solidconnection/e2e/DynamicFixture.java(1 hunks)src/test/java/com/example/solidconnection/siteuser/repository/SiteUserRepositoryTest.java(3 hunks)src/test/java/com/example/solidconnection/siteuser/service/MyPageServiceTest.java(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/test/java/com/example/solidconnection/concurrency/PostLikeCountConcurrencyTest.java (1)
src/test/java/com/example/solidconnection/e2e/DynamicFixture.java (1)
DynamicFixture(7-18)
🔇 Additional comments (3)
src/test/java/com/example/solidconnection/siteuser/service/MyPageServiceTest.java (1)
121-122: 영속 상태로 재확인하도록 수정된 검증 👍
저장소에서 다시 조회해 비교하도록 바꾼 덕분에 실제 DB 반영 여부까지 확인할 수 있습니다. 테스트 신뢰도가 한 단계 올라갔네요.src/test/java/com/example/solidconnection/e2e/DynamicFixture.java (1)
9-16: 닉네임 파라미터 추가로 유틸 완성도 향상 🎉
메서드 명세와 내부 구현이 일관성을 되찾았습니다. 고정 문자열 대신 전달받은nickname을 써서 중복 제약 테스트에 잘 맞춰졌네요.src/test/java/com/example/solidconnection/siteuser/repository/SiteUserRepositoryTest.java (1)
73-78: 중복 닉네임 변경 검증 👍
엔티티 업데이트 후saveAndFlush로 즉시 검증한 부분이 모범적입니다. 제약 위반을 확실히 끌어내는 좋은 예시네요.
src/test/java/com/example/solidconnection/siteuser/repository/SiteUserRepositoryTest.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
src/test/java/com/example/solidconnection/siteuser/service/MyPageServiceTest.java (1)
121-121: Optional:get()대신orElseThrow()사용 제안
findById(...).get()은 값이 없을 때NoSuchElementException을 던지지만, 메시지가 불명확합니다.orElseThrow()를 쓰면 예외 유형과 메시지를 선택적으로 제어할 수 있어 테스트 의도를 더 분명히 드러낼 수 있습니다.- SiteUser updatedUser = siteUserRepository.findById(user.getId()).get(); + SiteUser updatedUser = siteUserRepository.findById(user.getId()).orElseThrow();src/test/java/com/example/solidconnection/e2e/DynamicFixture.java (1)
9-13: Fixture 시그니처 확장 👍, 하지만 AuthType 주입 고려
- 닉네임 인자를 받아 유니크 제약을 쉽게 우회할 수 있게 됐습니다.
- 다만 실제 도메인에서
AuthType이 필수라면, 동일한 메서드 오버로드로 AuthType까지 받도록 확장하면 테스트 유연성이 더 좋아집니다.src/test/java/com/example/solidconnection/siteuser/repository/SiteUserRepositoryTest.java (3)
27-33:saveAndFlush로 즉시 예외 발생 보장
save()만 호출하면 플러시 시점이 트랜잭션 종료까지 지연될 수 있어, 예상한DataIntegrityViolationException이 바로 발생하지 않을 위험이 있습니다. 테스트 안정성을 위해 아래와 같이 변경을 권장합니다.- siteUserRepository.save(user1); + siteUserRepository.saveAndFlush(user1); ... - assertThatCode(() -> siteUserRepository.save(user2)) + assertThatCode(() -> siteUserRepository.saveAndFlush(user2))
39-45: 중복 이메일-다른 AuthType 검증도 동일 패턴으로 통일위와 같은 이유로 이곳도
saveAndFlush를 사용하면 일관성과 즉시성 두 마리 토끼를 잡을 수 있습니다.
50-79: 닉네임 중복 테스트: 단계별 flush 권장
- user1 저장 후 곧바로 flush → 제약이 실제 DB에 반영
- user2 저장 시 중복 여부가 즉각 확인
- siteUserRepository.save(user1); + siteUserRepository.saveAndFlush(user1);같은 이유로 저장-후-변경 케이스도 첫 저장 단계에서 flush 해두면 테스트 의도가 더 명확해집니다.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/test/java/com/example/solidconnection/concurrency/PostLikeCountConcurrencyTest.java(2 hunks)src/test/java/com/example/solidconnection/e2e/DynamicFixture.java(1 hunks)src/test/java/com/example/solidconnection/siteuser/repository/SiteUserRepositoryTest.java(3 hunks)src/test/java/com/example/solidconnection/siteuser/service/MyPageServiceTest.java(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/test/java/com/example/solidconnection/concurrency/PostLikeCountConcurrencyTest.java (1)
src/test/java/com/example/solidconnection/e2e/DynamicFixture.java (1)
DynamicFixture(7-18)
🔇 Additional comments (2)
src/test/java/com/example/solidconnection/siteuser/service/MyPageServiceTest.java (1)
121-122: 엔티티 재조회로 검증 정확도 향상
- 서비스 호출 후 영속성 컨텍스트에 남아 있는 객체 대신 DB에서 새로 조회하여 상태 동기화 문제를 제거했습니다.
- 변경이 의도대로 저장됐는지 확실히 검증하므로 테스트 신뢰도가 상승했습니다.
src/test/java/com/example/solidconnection/concurrency/PostLikeCountConcurrencyTest.java (1)
95-97: 닉네임 파라미터 추가로 유니크 제약 대응 완료이제 이메일과 닉네임을 모두 고유하게 생성하므로 DB 제약 위반 가능성이 제거되었습니다. 테스트가 안정적으로 수행됩니다.
src/main/java/com/example/solidconnection/siteuser/controller/MyPageController.java
Outdated
Show resolved
Hide resolved
src/main/java/com/example/solidconnection/siteuser/service/MyPageService.java
Show resolved
Hide resolved
src/main/java/com/example/solidconnection/siteuser/repository/SiteUserRepository.java
Outdated
Show resolved
Hide resolved
src/main/java/com/example/solidconnection/auth/service/SignUpService.java
Show resolved
Hide resolved
- 중복 닉네임 검증보다 닉네임 수정 시간 검증이 선행되도록 변경 - 준영속 엔티티를 save()하는 과정에서 발생하는 N+1 문제 해결 - DB 레벨, 애플리케이션 레벨 검증 모두 사용하도록 - 위 변경사항에 따른 테스트 코드 수정
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/main/java/com/example/solidconnection/common/exception/CustomExceptionHandler.java(3 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)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/test/java/com/example/solidconnection/siteuser/service/MyPageServiceTest.java
- src/main/java/com/example/solidconnection/siteuser/service/MyPageService.java
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (2)
src/main/java/com/example/solidconnection/common/exception/CustomExceptionHandler.java (2)
7-7: 👍 신규 예외 타입 추가 확인 완료
DataIntegrityViolationException임포트가 잘 반영되어 있습니다. 다른 클래스에서 동일 예외를 직접 핸들링하지 않고 전역 핸들러로 일원화된 점도 좋습니다.
17-17: 코드 일관성 유지에 도움되는 static import
DATA_INTEGRITY_VIOLATION상수에 대한 static import는 가독성을 높여줍니다. 별다른 이슈 없습니다.
src/main/java/com/example/solidconnection/common/exception/CustomExceptionHandler.java
Show resolved
Hide resolved
|
074ca56 에 대한 추가 설명입니다.
기존 코드와의 변경 사항을 정리하면 다음과 같습니다.
|
- CustomExceptionHandler에 DB 레벨에서 중복 닉네임 검증 추가에 따라 try-catch 문 삭제
- save -> saveAndFlush
src/main/java/com/example/solidconnection/auth/service/SignUpService.java
Outdated
Show resolved
Hide resolved
src/main/java/com/example/solidconnection/siteuser/repository/SiteUserRepository.java
Outdated
Show resolved
Hide resolved
src/main/java/com/example/solidconnection/siteuser/controller/MyPageController.java
Outdated
Show resolved
Hide resolved
src/main/java/com/example/solidconnection/common/exception/CustomExceptionHandler.java
Show resolved
Hide resolved
src/main/java/com/example/solidconnection/siteuser/service/MyPageService.java
Show resolved
Hide resolved
src/main/java/com/example/solidconnection/siteuser/service/MyPageService.java
Show resolved
Hide resolved
src/test/java/com/example/solidconnection/siteuser/repository/SiteUserRepositoryTest.java
Outdated
Show resolved
Hide resolved
| assertThatCode(() -> siteUserRepository.saveAndFlush(user2)) | ||
| .isInstanceOf(DataIntegrityViolationException.class); |
There was a problem hiding this comment.
save에서 saveAnsFlush()로 바꾸신 이유가 궁금해요!
There was a problem hiding this comment.
DB 레벨 테스트는 save 대신 saveAndFlush를 사용하였습니다. 아무래도 DB에 반영시킨 후의 결과를 봐야 하니까요. 물론 중복된_닉네임으로_사용자를_저장하면_예외_응답을_반환한다 테스트는 save 를 사용해도 통과가 되지만, DB 레벨 테스트라 saveAndFlush를 사용하였습니다. 아무래도 성능 측면에서는 save가 더 좋긴 할텐데, 어떤 기준으로 save, saveAndFlush를 선택해야 할지 고민이 되네요 ...
There was a problem hiding this comment.
아무래도 DB에 반영시킨 후의 결과를 봐야 하니까요.
아무래도 성능 측면에서는 save가 더 좋긴 할텐데,
이 부분에 대해서 제가 아는 내용으로는, (아니라면 지적해주세요!)
우리 테스트 코드에서는 Transactional 어노테이션을 사용하지 않고 있기 때문에,
assert문 내부에 save와 saveAndFlush 의 동작은 실질적으로 차이가 없다고 알고 있습니다!
물론 Transactional 어노테이션이 있는 테스트 코드라면 반드시 saveAndFlush를 써야 assert문에 걸리겠지만요 ㅎㅎ
어떤 기준으로 save, saveAndFlush를 선택해야 할지 고민이 되네요 ...
저도 고민이 되네요.. 🤔 우리 컨벤션대로라면 save를 써도 될텐데,.
미래에 Transactional 어노테이션을 쓸 상황을 대비해서 saveAndFlush로 방어적인 코드를 작성해야할지...?
개인적으로 saveAndFlush를 쓰는게 더 좋겠다는 생각은 듭니다!
There was a problem hiding this comment.
그렇군요 .. @Transaction 이 붙은 테스트 코드 작성 시에는 주의해야겠네요.
저는 방어적으로 작성한다고 손해보지는 않는다고 생각합니다 ㅎㅎ
src/main/java/com/example/solidconnection/siteuser/service/MyPageService.java
Show resolved
Hide resolved
src/main/java/com/example/solidconnection/siteuser/service/MyPageService.java
Show resolved
Hide resolved
좋은 리뷰 감사합니다~ 피드백 주신 내용들 꼼꼼히 읽고 답변드리겠습니다! |
nayonsoso
left a comment
nayonsoso
left a comment
There was a problem hiding this comment.
코멘트 달아주신 부분 다 확인했습니다 😄
저는 이번에 resolve 하지 않은 부분에 추가 코멘트만 달았습니다~
수고하셨습니다!!
| * */ | ||
| @Transactional | ||
| public void updateMyPageInfo(SiteUser siteUser, MultipartFile imageFile, String nickname) { | ||
| SiteUser user = siteUserRepository.findById(siteUser.getId()) |
3 participants
관련 이슈
작업 내용
SiteUser엔티티의nickname컬럼에unique제약 조건을 추가하였고, 이에 따라 데이터베이스 컬럼 제약 조건을 수정하는 sql문을 작성하였습니다. 명시적으로 제약 조건 이름을 설정하였습니다.또한 닉네임 중복 시의 예외를 추적할 수 있도록 하였습니다.unique제약 조건 위반으로 인해 발생할 수 있는DataIntegrityViolationException예외를 처리하는 로직을try-catch문으로 작성하였습니다.CustomExceptionHandler에 작성하였습니다.엔티티를 저장하는 것이 아닌 특정 컬럼의 값을 업데이트하는 쿼리를 작성하여 수정하였습니다.준영속 엔티티에save메서드를 수행하여 N+1 문제가 발생하는 문제를 해결하였습니다.특이 사항
취소선 다음 내용이 리뷰 피드백을 반영하여 새롭게 작업한 내용입니다!
리뷰 요구사항 (선택)
DataIntegrityViolationException에서 구체적인 예외 내용을 얻기 위해determineErrorCode메서드를 사용하였습니다. 중복 코드가 될 거 같은데, 더 좋은 방법이 있을까요?