fix: 멘토 지원서 승인 시 유저 Role 을 Mentor로 승격#639
Conversation
sukangpunch
requested review from
Gyuhyeok99,
Hexeong,
lsy1307,
whqtker and
wibaek
as code owners
Walkthrough
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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
left a comment
whqtker
left a comment
There was a problem hiding this comment.
고생하셨습니다 ~!
다만 현재는 siteUser의 role 만 MENTOR 로 변경하는 거 같은데, mentor 엔티티 또한 생성해야 합니다 ! 이후 관련하여 테스트(승인하면 멘토 생성)도 추가해주시면 좋겠습니다 !!
알겠습니다! |
- not null 인 필드에 빈문자열로 값을 채우는 것 보다, null 허용이 더 의미 있다 판단하여 null 을 허용하도록 하였습니다.
- 멘토 생성의 주체가 어드민으로 변경되어 Mentor 도메인의 Mentor 생성 api 를 제거
- 중복 멘토 생성 예외 처리 및 테스트 추가
![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
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/mentor/service/MentorMyPageService.java (1)
40-40:⚠️ Potential issue | 🟡 Minor사용하지 않는
mentorApplicationRepository의존성을 제거하세요.
createMentorMyPage메서드가 제거되면서 이 클래스 내에서mentorApplicationRepository를 참조하는 코드가 더 이상 없습니다. 불필요한 의존성은 정리하는 것이 좋습니다.
- import 제거 (15번 줄)
- 필드 주입 제거 (40번 줄)
정리할 코드
-import com.example.solidconnection.mentor.repository.MentorApplicationRepository;private final MentorRepository mentorRepository; private final SiteUserRepository siteUserRepository; private final HostUniversityRepository hostUniversityRepository; private final TermRepository termRepository; - private final MentorApplicationRepository mentorApplicationRepository;
🧹 Nitpick comments (2)
src/main/java/com/example/solidconnection/mentor/domain/Mentor.java (1)
56-68: 생성자의universityId파라미터 타입 불일치에 주의하세요.
- 생성자 파라미터는
Long universityId(박싱 타입, line 60)인데, 필드는long universityId(원시 타입, line 46)입니다.- 만약
null이 전달되면 auto-unboxing 시NullPointerException이 발생합니다.학습된 비즈니스 규칙에 따르면 APPROVED 상태의
MentorApplication은 항상 non-nulluniversityId를 가지므로 현재 흐름에서는 문제가 없지만, 방어적 코딩 관점에서 파라미터 타입을long으로 통일하거나, null 체크를 추가하는 것을 고려해 주세요. Based on learnings, APPROVED 상태의 MentorApplication은 항상 non-null universityId를 가진다는 비즈니스 규칙이 있습니다.💡 파라미터 타입 통일 제안
public Mentor( String introduction, String passTip, long siteUserId, - Long universityId, + long universityId, long termId ) {src/main/java/com/example/solidconnection/admin/service/AdminMentorApplicationService.java (1)
48-67: 멘토 생성 로직이 잘 구현되어 있습니다. 다만 검증 순서를 개선하면 더 안전합니다.현재 흐름:
mentorApplication.approve()(상태 변경) → line 51validateUserCanCreateMentor()(중복 검증) → line 55트랜잭션 롤백으로 인해 실제 데이터 무결성 문제는 없지만,
approve()이전에 모든 사전 검증을 완료하는 것이 방어적 프로그래밍 관점에서 더 안전합니다. 만약 향후approve()내부에 이벤트 발행 등의 부수 효과가 추가된다면, 롤백만으로는 충분하지 않을 수 있습니다.🔄 검증 순서 변경 제안
`@Transactional` public void approveMentorApplication(Long mentorApplicationId) { MentorApplication mentorApplication = mentorApplicationRepository.findById(mentorApplicationId) .orElseThrow(() -> new CustomException(MENTOR_APPLICATION_NOT_FOUND)); - mentorApplication.approve(); SiteUser siteUser = siteUserRepository.findById(mentorApplication.getSiteUserId()) .orElseThrow(() -> new CustomException(USER_NOT_FOUND)); validateUserCanCreateMentor(siteUser.getId()); + mentorApplication.approve(); siteUser.becomeMentor(); Mentor mentor = new Mentor( null, null, siteUser.getId(), mentorApplication.getUniversityId(), mentorApplication.getTermId() ); mentorRepository.save(mentor); }
| MentorApplication mentorApplication = mentorApplicationRepository.findById(mentorApplicationId) | ||
| .orElseThrow(() -> new CustomException(MENTOR_APPLICATION_NOT_FOUND)); | ||
|
|
||
| mentorApplication.approve(); | ||
|
|
||
| SiteUser siteUser = siteUserRepository.findById(mentorApplication.getSiteUserId()) | ||
| .orElseThrow(() -> new CustomException(USER_NOT_FOUND)); | ||
| validateUserCanCreateMentor(siteUser.getId()); | ||
|
|
||
| siteUser.becomeMentor(); |
There was a problem hiding this comment.
어차피 한 트랜잭션 내에서 작동하니 크게 상관은 없을 거 같긴한데 검증이 먼저되는 게 좋을 거 같아서
siteUser = siteUserRepository.findById(...) // 1. 유저 조회
validateUserCanCreateMentor(siteUser.getId()); // 2. 검증
mentorApplication.approve(); // 3. 상태 변경
siteUser.becomeMentor(); // 4. Role 승격
mentorRepository.save(mentor); // 5. 멘토 생성
이런식으로 순서를 바꾸는 건 어떤가요?
사소하긴 합니다
There was a problem hiding this comment.
해당 방향으로 수정 하겠습니다!
| Mentor mentor = new Mentor( | ||
| null, | ||
| null, | ||
| siteUser.getId(), | ||
| mentorApplication.getUniversityId(), | ||
| mentorApplication.getTermId() | ||
| ); |
There was a problem hiding this comment.
이거 null넘기는 것보단 밑에 3개만 받는 생성자나 정적팩토리 메서드 만드는 건 어떤가요?
예전에 형준님이 언급했었던 컨벤션인 거 같긴한데 저희가 잘 지켜지진 않고 있는 거로 압니다..
근데 지금 보니 좀 신경쓰이는 거 같아서 이번거부터 조금씩 고쳐보는 거 어떤가요?
There was a problem hiding this comment.
좋습니다! 3개만 받는 생성자를 활용 하는 방향으로 수정 해 보겠습니다.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@src/main/java/com/example/solidconnection/admin/service/AdminMentorApplicationService.java`:
- Around line 68-72: Add a DB-level UNIQUE constraint on the mentor.site_user_id
to prevent TOCTOU race conditions in addition to the existing application check
in validateUserCanCreateMentor (which uses mentorRepository.existsBySiteUserId).
Fix this by adding a migration that runs ALTER TABLE mentor ADD CONSTRAINT
uk_mentor_site_user_id UNIQUE (site_user_id) and/or annotate the Mentor entity
with `@Table`(uniqueConstraints = `@UniqueConstraint`(columnNames = "siteUserId"))
(ensure the column name matches the mapped column, e.g., site_user_id), and keep
the existing existsBySiteUserId check to preserve the friendly
CustomException(MENTOR_ALREADY_EXISTS) for pre-checks.
🧹 Nitpick comments (1)
src/main/java/com/example/solidconnection/mentor/domain/Mentor.java (1)
70-78: 새 생성자는 깔끔합니다. 다만Long universityId→long universityId자동 언박싱 주의가 필요합니다.
- 파라미터 타입이
Long(boxed)인데, 필드 타입은long(primitive)입니다 (Line 46).- 비즈니스 규칙상 승인 시점에
universityId는 항상 non-null이지만, 만약 null이 전달되면NullPointerException이 발생합니다.- 기존 생성자(Line 56-68)도 동일한 패턴이라 이 PR의 범위는 아니지만, 방어적으로 파라미터를
long으로 통일하거나 null 체크를 추가하면 더 안전합니다.💡 선택적 리팩터링 제안
public Mentor( - long siteUserId, - Long universityId, - long termId + long siteUserId, + long universityId, + long termId ) {Based on learnings, APPROVED 상태의 MentorApplication은 항상 non-null universityId를 가진다는 비즈니스 규칙이 있으므로,
long타입이 더 적절합니다.
| private void validateUserCanCreateMentor(long siteUserId) { | ||
| if (mentorRepository.existsBySiteUserId(siteUserId)) { | ||
| throw new CustomException(MENTOR_ALREADY_EXISTS); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Mentor 관련 마이그레이션 파일 찾기
find . -type f -name "*.sql" -o -name "*Mentor*Migration*.java" | head -20Repository: solid-connection/solid-connect-server
Length of output: 1469
🏁 Script executed:
# site_user_id 유니크 제약 관련 SQL 패턴 검색
rg -i 'unique.*site_user_id|site_user_id.*unique' --type sql -A 2 -B 2Repository: solid-connection/solid-connect-server
Length of output: 2555
🏁 Script executed:
# Mentor 엔티티 클래스 조회
find . -type f -name "Mentor.java" | grep -E 'entity|domain|model'Repository: solid-connection/solid-connect-server
Length of output: 150
🏁 Script executed:
# 전체 Mentor 엔티티 파일 위치 찾기
fd -t f "Mentor.java"Repository: solid-connection/solid-connect-server
Length of output: 148
🏁 Script executed:
# Mentor 엔티티 파일 확인
cat -n ./src/main/java/com/example/solidconnection/mentor/domain/Mentor.javaRepository: solid-connection/solid-connect-server
Length of output: 3974
🏁 Script executed:
# V19 마이그레이션 파일 (Mentor 테이블 생성) 확인
cat -n ./src/main/resources/db/migration/V19__create_mentor_related_tables.sqlRepository: solid-connection/solid-connect-server
Length of output: 2317
🏁 Script executed:
# V26 마이그레이션 파일 (term column 추가) 확인
cat -n ./src/main/resources/db/migration/V26__add_term_column_to_mentor.sqlRepository: solid-connection/solid-connect-server
Length of output: 155
멘토 중복 생성을 방지하기 위해 데이터베이스 레벨 유니크 제약을 추가하세요.
검증 결과, 현재 코드는 다음과 같은 상황입니다:
-
애플리케이션 레벨 검증
existsBySiteUserId()체크로 비즈니스 규칙을 지키고 있습니다.
-
데이터베이스 레벨 보호 부재 (권장 개선)
- mentor 테이블에 site_user_id 컬럼에 대한 UNIQUE 제약이 없습니다.
- Mentor 엔티티에도
@Table(uniqueConstraints) 정의가 없습니다. - TOCTOU(Time-of-Check-to-Time-of-Use) 경쟁 조건이 발생할 수 있습니다.
-
권장사항
- 마이그레이션 파일에서
ALTER TABLE mentor ADD CONSTRAINT uk_mentor_site_user_id UNIQUE (site_user_id);추가 - 또는 Mentor 엔티티에
@Table(uniqueConstraints =@UniqueConstraint(columnNames = "siteUserId"))추가
- 마이그레이션 파일에서
어드민 전용 기능이라 동시 호출 가능성은 낮지만, 데이터 무결성을 위해 DB 레벨 제약을 추가하면 더욱 안전합니다.
🤖 Prompt for AI Agents
In
`@src/main/java/com/example/solidconnection/admin/service/AdminMentorApplicationService.java`
around lines 68 - 72, Add a DB-level UNIQUE constraint on the
mentor.site_user_id to prevent TOCTOU race conditions in addition to the
existing application check in validateUserCanCreateMentor (which uses
mentorRepository.existsBySiteUserId). Fix this by adding a migration that runs
ALTER TABLE mentor ADD CONSTRAINT uk_mentor_site_user_id UNIQUE (site_user_id)
and/or annotate the Mentor entity with `@Table`(uniqueConstraints =
`@UniqueConstraint`(columnNames = "siteUserId")) (ensure the column name matches
the mapped column, e.g., site_user_id), and keep the existing existsBySiteUserId
check to preserve the friendly CustomException(MENTOR_ALREADY_EXISTS) for
pre-checks.
3 participants
관련 이슈
작업 내용
어드민에서 멘토 지원서를 승인 할 때, 유저의 Role 을 Mentor 로 승격하도록 기능을 수정하였습니다.
특이 사항
리뷰 요구사항 (선택)