refactor: Recruitment 생성 시 기간 중복 검증 추가#1355
Merged
Conversation
github-actions
bot
changed the title
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthrough리크루팅 생성 검증을 분리해 학기 중복(isRecruitmentSemesterOverlap)과 기간 중복(isRecruitmentPeriodOverlap)을 독립적으로 검사하도록 변경했습니다. Repository에 기간 중복 조회 메서드( Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Service as AdminRecruitmentService
participant Repo as RecruitmentRepository
participant Validator as RecruitmentValidator
participant DB as Database
Client->>Service: createRecruitment(request)
Service->>Repo: existsBySemester(academicYear, semester)
Repo->>DB: SELECT ... WHERE academicYear, semester
DB-->>Repo: result (true/false)
Service->>Repo: existsPeriodOverlapping(startDateTime, endDateTime)
Repo->>DB: SELECT ... WHERE period overlaps
DB-->>Repo: result (true/false)
Service->>Validator: validateRecruitmentCreate(isRecruitmentSemesterOverlap, isRecruitmentPeriodOverlap)
alt semester overlap
Validator-->>Service: throw RECRUITMENT_SEMESTER_OVERLAP
else period overlap
Validator-->>Service: throw RECRUITMENT_PERIOD_OVERLAP
else no overlap
Service->>Repo: save(new Recruitment)
Repo->>DB: INSERT ...
DB-->>Repo: saved
Repo-->>Service: persisted entity
Service-->>Client: created response
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
Job Summary for GradleCheck Style and Test to Develop :: build-test
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/test/java/com/gdschongik/gdsc/domain/recruitment/domain/RecruitmentValidatorTest.java (1)
15-38: 두 플래그가 동시에 true일 때 우선순위 테스트도 있으면 좋겠습니다.현재 단일 조건별 실패는 잘 검증됩니다.
isRecruitmentSemesterOverlap=true,isRecruitmentPeriodOverlap=true동시 입력 시 어떤 에러를 우선 반환하는지 테스트를 고정해두면 회귀 방지에 유리합니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/com/gdschongik/gdsc/domain/recruitment/domain/RecruitmentValidatorTest.java` around lines 15 - 38, Add a new test in RecruitmentValidatorTest that calls recruitmentValidator.validateRecruitmentCreate with both isRecruitmentSemesterOverlap=true and isRecruitmentPeriodOverlap=true and assert which CustomException message is returned (decide and assert either RECRUITMENT_SEMESTER_OVERLAP.getMessage() or RECRUITMENT_PERIOD_OVERLAP.getMessage()); this pins the priority behavior when both flags are true and prevents regressions of validateRecruitmentCreate's error precedence.src/test/java/com/gdschongik/gdsc/domain/recruitment/application/AdminRecruitmentServiceTest.java (1)
80-99: 기간 중복 경계값 테스트를 추가해 주세요.현재는 “완전 중복”만 검증합니다.
기존 종료일 == 신규 시작일경계 케이스도 포함해야 중복 판정 회귀를 안정적으로 막을 수 있습니다.✅ 테스트 케이스 추가 예시
+ `@Test` + void 기존_종료일과_신규_시작일이_같다면_실패한다() { + // given + Recruitment previousRecruitment = + Recruitment.create(FEE_NAME, FEE, Period.of(SEMESTER_START_DATE, SEMESTER_END_DATE), SEMESTER); + recruitmentRepository.save(previousRecruitment); + + RecruitmentCreateRequest request = new RecruitmentCreateRequest( + SEMESTER_END_DATE, // 기존 종료일과 동일한 시작일 + SEMESTER_END_DATE.plusDays(10), + ACADEMIC_YEAR + 1, + SEMESTER_TYPE, + FEE_AMOUNT, + FEE_NAME); + + // when & then + assertThatThrownBy(() -> adminRecruitmentService.createRecruitment(request)) + .isInstanceOf(CustomException.class) + .hasMessage(RECRUITMENT_PERIOD_OVERLAP.getMessage()); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/com/gdschongik/gdsc/domain/recruitment/application/AdminRecruitmentServiceTest.java` around lines 80 - 99, Add a boundary test to AdminRecruitmentServiceTest that verifies the overlap rule treats previous end == new start as overlapping: create and save a previous Recruitment via Recruitment.create(..., Period.of(SEMESTER_START_DATE, SEMESTER_END_DATE), ...), then build a RecruitmentCreateRequest whose start date equals that previous recruitment's end date (SEMESTER_END_DATE) and call adminRecruitmentService.createRecruitment(request); assert it throws CustomException with RECRUITMENT_PERIOD_OVERLAP.getMessage(). Place this in a new test method (e.g., 학기_기간_경계가_겹치면_실패한다) to explicitly cover the boundary case.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/main/java/com/gdschongik/gdsc/domain/recruitment/application/AdminRecruitmentService.java`:
- Around line 41-45: The existence checks
(recruitmentRepository.existsBySemester and
recruitmentRepository.existsByPeriodOverlapping) and subsequent save must be
made atomic to avoid race conditions; wrap the create flow in a transaction with
an elevated isolation level or use a pessimistic lock on the recruitment table
(e.g., `@Transactional`(isolation = Isolation.SERIALIZABLE) on the service method
that calls recruitmentValidator.validateRecruitmentCreate and
recruitmentRepository.save) and also add a DB-level unique constraint for
academicYear+semesterType and handle constraint violations by catching
DataIntegrityViolationException (or the specific persistence exception) to
translate into the same validation error path used by
recruitmentValidator.validateRecruitmentCreate.
In
`@src/main/java/com/gdschongik/gdsc/domain/recruitment/dao/RecruitmentCustomRepositoryImpl.java`:
- Around line 40-41: The overlap check in isOverlappingPeriod currently uses
recruitment.semesterPeriod.endDate.gt(startDate), which treats an existing
endDate equal to the new startDate as non-overlapping; change that comparison to
recruitment.semesterPeriod.endDate.goe(startDate) so the end boundary is
inclusive and such edge cases are considered overlapping; update the
BooleanExpression in isOverlappingPeriod accordingly to use goe for the endDate
comparison.
---
Nitpick comments:
In
`@src/test/java/com/gdschongik/gdsc/domain/recruitment/application/AdminRecruitmentServiceTest.java`:
- Around line 80-99: Add a boundary test to AdminRecruitmentServiceTest that
verifies the overlap rule treats previous end == new start as overlapping:
create and save a previous Recruitment via Recruitment.create(...,
Period.of(SEMESTER_START_DATE, SEMESTER_END_DATE), ...), then build a
RecruitmentCreateRequest whose start date equals that previous recruitment's end
date (SEMESTER_END_DATE) and call
adminRecruitmentService.createRecruitment(request); assert it throws
CustomException with RECRUITMENT_PERIOD_OVERLAP.getMessage(). Place this in a
new test method (e.g., 학기_기간_경계가_겹치면_실패한다) to explicitly cover the boundary
case.
In
`@src/test/java/com/gdschongik/gdsc/domain/recruitment/domain/RecruitmentValidatorTest.java`:
- Around line 15-38: Add a new test in RecruitmentValidatorTest that calls
recruitmentValidator.validateRecruitmentCreate with both
isRecruitmentSemesterOverlap=true and isRecruitmentPeriodOverlap=true and assert
which CustomException message is returned (decide and assert either
RECRUITMENT_SEMESTER_OVERLAP.getMessage() or
RECRUITMENT_PERIOD_OVERLAP.getMessage()); this pins the priority behavior when
both flags are true and prevents regressions of validateRecruitmentCreate's
error precedence.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 627a2834-cf5f-444b-a159-61a62b6235fb
📒 Files selected for processing (7)
src/main/java/com/gdschongik/gdsc/domain/recruitment/application/AdminRecruitmentService.javasrc/main/java/com/gdschongik/gdsc/domain/recruitment/dao/RecruitmentCustomRepository.javasrc/main/java/com/gdschongik/gdsc/domain/recruitment/dao/RecruitmentCustomRepositoryImpl.javasrc/main/java/com/gdschongik/gdsc/domain/recruitment/domain/service/RecruitmentValidator.javasrc/main/java/com/gdschongik/gdsc/global/exception/ErrorCode.javasrc/test/java/com/gdschongik/gdsc/domain/recruitment/application/AdminRecruitmentServiceTest.javasrc/test/java/com/gdschongik/gdsc/domain/recruitment/domain/RecruitmentValidatorTest.java
Comment on lines
+41
to
+45
| boolean isRecruitmentSemesterOverlap = recruitmentRepository.existsBySemester(semester); | ||
| boolean isRecruitmentPeriodOverlap = | ||
| recruitmentRepository.existsByPeriodOverlapping(request.semesterStartDate(), request.semesterEndDate()); | ||
|
|
||
| recruitmentValidator.validateRecruitmentCreate(isRecruitmentOverlap); | ||
| recruitmentValidator.validateRecruitmentCreate(isRecruitmentSemesterOverlap, isRecruitmentPeriodOverlap); |
There was a problem hiding this comment.
중복 체크와 저장이 원자적이지 않아 동시성에서 중복 생성이 가능합니다.
현재는 existsBy... 검사 후 save까지 경쟁 구간이 열려 있습니다. 동시 요청 2건이 모두 검증을 통과하면 중복 리크루팅이 저장될 수 있습니다.
🔒 최소 완화안 (트랜잭션 격리 상향)
+import org.springframework.transaction.annotation.Isolation;
import org.springframework.transaction.annotation.Transactional;
@@
- `@Transactional`
+ `@Transactional`(isolation = Isolation.SERIALIZABLE)
public void createRecruitment(RecruitmentCreateRequest request) {추가로, 학기 중복은 DB 유니크 제약(academicYear + semesterType)으로 반드시 이중 방어하는 것을 권장합니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/main/java/com/gdschongik/gdsc/domain/recruitment/application/AdminRecruitmentService.java`
around lines 41 - 45, The existence checks
(recruitmentRepository.existsBySemester and
recruitmentRepository.existsByPeriodOverlapping) and subsequent save must be
made atomic to avoid race conditions; wrap the create flow in a transaction with
an elevated isolation level or use a pessimistic lock on the recruitment table
(e.g., `@Transactional`(isolation = Isolation.SERIALIZABLE) on the service method
that calls recruitmentValidator.validateRecruitmentCreate and
recruitmentRepository.save) and also add a DB-level unique constraint for
academicYear+semesterType and handle constraint violations by catching
DataIntegrityViolationException (or the specific persistence exception) to
translate into the same validation error path used by
recruitmentValidator.validateRecruitmentCreate.
src/main/java/com/gdschongik/gdsc/domain/recruitment/dao/RecruitmentCustomRepositoryImpl.java
Outdated
Show resolved
Hide resolved
Sangwook02
requested changes
Mar 5, 2026
| public interface RecruitmentCustomRepository { | ||
| Optional<Recruitment> findCurrentRecruitment(LocalDateTime now); | ||
|
|
||
| boolean existsByPeriodOverlapping(LocalDateTime startDate, LocalDateTime endDate); |
Member
There was a problem hiding this comment.
Suggested change
| boolean existsByPeriodOverlapping(LocalDateTime startDate, LocalDateTime endDate); | |
| boolean existsPeriodOverlapping(LocalDateTime startDate, LocalDateTime endDate); |
Period를 통째로 넘기는 상황도 아니고, querydsl이라 By 안 써도 될 것 같습니다.
| } | ||
|
|
||
| private BooleanExpression isOverlappingPeriod(LocalDateTime startDate, LocalDateTime endDate) { | ||
| return recruitment.semesterPeriod.startDate.loe(endDate).and(recruitment.semesterPeriod.endDate.gt(startDate)); |
Member
There was a problem hiding this comment.
가독성 측면에서 수식 쓰는 것과 같은 순서로 하면 더 좋을 것 같아요
Suggested change
| return recruitment.semesterPeriod.startDate.loe(endDate).and(recruitment.semesterPeriod.endDate.gt(startDate)); | |
| return recruitment.semesterPeriod.endDate.gt(startDate).and(recruitment.semesterPeriod.startDate.loe(endDate)); |
Comment on lines
+29
to
+38
| @Override | ||
| public boolean existsByPeriodOverlapping(LocalDateTime startDate, LocalDateTime endDate) { | ||
| Integer fetchOne = queryFactory | ||
| .selectOne() | ||
| .from(recruitment) | ||
| .where(isOverlappingPeriod(startDate, endDate)) | ||
| .fetchFirst(); | ||
|
|
||
| return fetchOne != null; | ||
| } |
Member
There was a problem hiding this comment.
public 메서드들을 private 메서드들 위에 모아두지 않았었나요?
이건 기억이 안 나는데 다른 구현체 보고 컨벤션에 맞게 수정 부탁드립니다!
Job Summary for GradleCheck Style and Test to Develop :: build-test
|
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/main/java/com/gdschongik/gdsc/domain/recruitment/dao/RecruitmentCustomRepositoryImpl.java (1)
40-42:⚠️ Potential issue | 🔴 Critical기간 겹침 경계값이 배타(
gt)라 동일 경계 케이스를 허용합니다.Line [41]의
endDate.gt(startDate)는 기존 종료시각과 신규 시작시각이 같은 경우를 비중복으로 처리합니다. 현재 기간 판정이 포함 경계(goe) 기준이라면 일관성 깨집니다.🐛 제안 수정
private BooleanExpression isOverlappingPeriod(LocalDateTime startDate, LocalDateTime endDate) { - return recruitment.semesterPeriod.endDate.gt(startDate).and(recruitment.semesterPeriod.startDate.loe(endDate)); + return recruitment.semesterPeriod.endDate.goe(startDate).and(recruitment.semesterPeriod.startDate.loe(endDate)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/gdschongik/gdsc/domain/recruitment/dao/RecruitmentCustomRepositoryImpl.java` around lines 40 - 42, The overlap check in isOverlappingPeriod uses recruitment.semesterPeriod.endDate.gt(startDate), which treats identical boundary times as non-overlapping; change this to an inclusive comparison (use goe) so the condition matches the function's intended inclusive semantics—update recruitment.semesterPeriod.endDate.gt(startDate) to recruitment.semesterPeriod.endDate.goe(startDate) (and keep the existing recruitment.semesterPeriod.startDate.loe(endDate)) so identical boundaries are considered overlapping.src/main/java/com/gdschongik/gdsc/domain/recruitment/application/AdminRecruitmentService.java (1)
41-52:⚠️ Potential issue | 🔴 Critical중복 검사와 저장이 원자적이지 않아 동시성에서 중복 생성이 가능합니다.
Line [41]-Line [45]의 조회 기반 검증 뒤 Line [52]에서 저장하는 구조는 경쟁 구간이 열려 있습니다. 동시 요청 2건이 모두 검증을 통과하면 중복 리크루팅이 저장될 수 있습니다.
🔒 최소 완화안
+import org.springframework.transaction.annotation.Isolation; import org.springframework.transaction.annotation.Transactional; `@Slf4j` `@Service` `@RequiredArgsConstructor` `@Transactional`(readOnly = true) public class AdminRecruitmentService { - `@Transactional` + `@Transactional`(isolation = Isolation.SERIALIZABLE) public void createRecruitment(RecruitmentCreateRequest request) {추가로 DB 유니크 제약(예: 학기 키)과 제약 위반 예외 변환까지 같이 두는 이중 방어를 권장합니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/gdschongik/gdsc/domain/recruitment/application/AdminRecruitmentService.java` around lines 41 - 52, The concurrent duplicate-creation risk arises because AdminRecruitmentService performs read checks (recruitmentRepository.existsBySemester and recruitmentRepository.existsPeriodOverlapping) and then saves (recruitmentRepository.save) outside an atomic operation; fix by making the create flow transactional and relying on a DB-unique constraint on the semester (or period) as the primary guard, i.e., wrap the creation in a transaction in AdminRecruitmentService and perform recruitmentRepository.save(Recruitment.create(...)) within that transaction, and additionally catch and translate the persistence-level constraint violation (e.g., DataIntegrityViolationException / ConstraintViolationException) into the domain validation exception that recruitmentValidator.validateRecruitmentCreate would throw so concurrent requests result in a single persistent record and a clear domain error.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@src/main/java/com/gdschongik/gdsc/domain/recruitment/application/AdminRecruitmentService.java`:
- Around line 41-52: The concurrent duplicate-creation risk arises because
AdminRecruitmentService performs read checks
(recruitmentRepository.existsBySemester and
recruitmentRepository.existsPeriodOverlapping) and then saves
(recruitmentRepository.save) outside an atomic operation; fix by making the
create flow transactional and relying on a DB-unique constraint on the semester
(or period) as the primary guard, i.e., wrap the creation in a transaction in
AdminRecruitmentService and perform
recruitmentRepository.save(Recruitment.create(...)) within that transaction, and
additionally catch and translate the persistence-level constraint violation
(e.g., DataIntegrityViolationException / ConstraintViolationException) into the
domain validation exception that recruitmentValidator.validateRecruitmentCreate
would throw so concurrent requests result in a single persistent record and a
clear domain error.
In
`@src/main/java/com/gdschongik/gdsc/domain/recruitment/dao/RecruitmentCustomRepositoryImpl.java`:
- Around line 40-42: The overlap check in isOverlappingPeriod uses
recruitment.semesterPeriod.endDate.gt(startDate), which treats identical
boundary times as non-overlapping; change this to an inclusive comparison (use
goe) so the condition matches the function's intended inclusive semantics—update
recruitment.semesterPeriod.endDate.gt(startDate) to
recruitment.semesterPeriod.endDate.goe(startDate) (and keep the existing
recruitment.semesterPeriod.startDate.loe(endDate)) so identical boundaries are
considered overlapping.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: d3fa0fbc-e481-4c9c-8908-e515682fae6e
📒 Files selected for processing (3)
src/main/java/com/gdschongik/gdsc/domain/recruitment/application/AdminRecruitmentService.javasrc/main/java/com/gdschongik/gdsc/domain/recruitment/dao/RecruitmentCustomRepository.javasrc/main/java/com/gdschongik/gdsc/domain/recruitment/dao/RecruitmentCustomRepositoryImpl.java
Sangwook02
reviewed
Mar 6, 2026
| } | ||
|
|
||
| private BooleanExpression isOverlappingPeriod(LocalDateTime startDate, LocalDateTime endDate) { | ||
| return recruitment.semesterPeriod.endDate.gt(startDate).and(recruitment.semesterPeriod.startDate.loe(endDate)); |
Member
There was a problem hiding this comment.
근데 하나는 equal까지 들어가고 하나는 안 들어가는 이유가 있나요?
Member
Author
There was a problem hiding this comment.
실수입니다
goe, loe 사용하도록 변경했습니다.
Sangwook02
approved these changes
Mar 6, 2026
Member
Sangwook02
left a comment
Sangwook02
left a comment
There was a problem hiding this comment.
코멘트 하나 달아뒀는데 확인해보시고 문제 없으면 바로 머지하시면 될 것 같습니다!
Job Summary for GradleCheck Style and Test to Develop :: build-test
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🌱 관련 이슈
📌 작업 내용 및 특이사항
[문제 상황]
[해결 방안]
📝 참고사항
📚 기타
Summary by CodeRabbit
개선 사항
테스트