feat: 모의지원 최대 지망 수를 동적으로 관리하도록#746
Open
whqtker wants to merge 22 commits into
Open
Conversation
- 컬럼 복수 유지에 대한 임시 조치
- private 메서드 위치 - Wrapper 타입 - 메서드 레퍼런스 방식
whqtker
requested review from
Hexeong,
JAEHEE25,
lsy1307 and
sukangpunch
as code owners
🎯 Walkthrough
🎯 Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~60 minutes 📋 Suggested Reviewers
🚥 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 docstrings
🧪 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 074b5408cb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/test/java/com/example/solidconnection/admin/university/service/AdminHomeUniversityServiceTest.java (1)
182-191:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win2) \t같은 이름 수정 성공 케이스에
maxChoiceCount검증이 빠져 있습니다요청에
maxChoiceCount를 넣었는데 성공 검증은 이름만 확인하고 있어, 수정 매핑 누락 회귀를 놓칠 수 있습니다. 응답(또는 DB 재조회)에서maxChoiceCount도 함께 단언해 주세요.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/java/com/example/solidconnection/admin/university/service/AdminHomeUniversityServiceTest.java` around lines 182 - 191, The test method 같은_이름으로_수정하면_성공한다 in AdminHomeUniversityServiceTest is only asserting the name was updated but ignores maxChoiceCount; update the assertion to also verify maxChoiceCount was changed (either assert response.maxChoiceCount() equals the request value 3 or reload the entity from the repository and assert homeUniversity.getMaxChoiceCount() == 3) so the AdminHomeUniversityUpdateRequest -> adminHomeUniversityService.updateHomeUniversity mapping for maxChoiceCount is covered.
🧹 Nitpick comments (1)
src/main/java/com/example/solidconnection/application/domain/ApplicationChoice.java (1)
7-24: ⚡ Quick win
equals()와hashCode()구현을 권장합니다.
현재 상태
@Embeddable클래스에equals()/hashCode()가 없습니다.잠재적 문제
- Hibernate는
@ElementCollection의 변경 감지 시equals()를 사용합니다.- 기본 Object identity 비교로 인해 의도치 않은 dirty checking 동작이 발생할 수 있습니다.
- 컬렉션 내 요소 비교 및 제거 시 예상과 다르게 동작할 수 있습니다.
♻️ equals/hashCode 구현 제안
`@Embeddable` `@Getter` public class ApplicationChoice { `@Column`(name = "choice_order", nullable = false) private int choiceOrder; `@Column`(name = "univ_apply_info_id", nullable = false) private long univApplyInfoId; protected ApplicationChoice() { } public ApplicationChoice(int choiceOrder, long univApplyInfoId) { this.choiceOrder = choiceOrder; this.univApplyInfoId = univApplyInfoId; } + + `@Override` + public boolean equals(Object o) { + if (this == o) return true; + if (o == null || getClass() != o.getClass()) return false; + ApplicationChoice that = (ApplicationChoice) o; + return choiceOrder == that.choiceOrder && univApplyInfoId == that.univApplyInfoId; + } + + `@Override` + public int hashCode() { + return Objects.hash(choiceOrder, univApplyInfoId); + } }
java.util.Objectsimport 추가가 필요합니다.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/example/solidconnection/application/domain/ApplicationChoice.java` around lines 7 - 24, Implement proper equality for the embeddable ApplicationChoice by adding equals(Object) and hashCode() that base equality on the fields choiceOrder and univApplyInfoId (use java.util.Objects for null-safe comparison / hashing); update ApplicationChoice to include these methods so Hibernate’s ElementCollection dirty-checking and collection operations work correctly—add the import java.util.Objects and implement equals to check instance/type then compare choiceOrder and univApplyInfoId, and implement hashCode to return Objects.hash(choiceOrder, univApplyInfoId).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/main/java/com/example/solidconnection/application/dto/UnivApplyInfoResponse.java`:
- Around line 17-20: The mapping for choiceNames in UnivApplyInfoResponse can
insert nulls because nameById.get(choice.getUnivApplyInfoId()) may return null;
update the logic that builds choiceNames (the stream using
ApplicationChoice::getChoiceOrder and ApplicationChoice::getUnivApplyInfoId) to
defensively handle missing keys—either filter out choices with no name, replace
nulls with a sensible default (e.g., empty string or "Unknown"), or throw a
clear exception when data invariants are violated; ensure the chosen behavior is
applied consistently and documented in UnivApplyInfoResponse construction.
In
`@src/main/java/com/example/solidconnection/siteuser/repository/custom/SiteUserFilterRepositoryImpl.java`:
- Around line 350-358: The current collect into Map<Long,String> using
tuples.stream().collect(Collectors.toMap(...)) will NPE if
t.get(univApplyInfo.koreanName) is null and may produce nulls in the resulting
choiceNames when nameById lacks an entry; update the collection logic to
defensively handle null values by supplying a non-null value or merge function
to Collectors.toMap (use t.get(univApplyInfo.koreanName) == null ? "" :
thatValue or similar) and then when building choiceNames from univApplyInfoIds
map missing IDs to a safe default (e.g., empty string) or filter them out so
choiceNames contains no nulls; reference the tuples stream, univApplyInfo.id,
univApplyInfo.koreanName, nameById map and univApplyInfoIds when applying the
fix.
In
`@src/main/java/com/example/solidconnection/university/domain/HomeUniversity.java`:
- Around line 28-34: HomeUniversity의 maxChoiceCount가 0 또는 음수가 들어갈 수 있어 도메인 검증을
추가해야 합니다; HomeUniversity 클래스의 생성자와 update(String name, int maxChoiceCount) 메서드에서
maxChoiceCount <= 0일 경우 IllegalArgumentException(또는 도메인 전용 예외)을 던지도록 검증을 추가하고,
메시지는 "maxChoiceCount must be >= 1"처럼 명확히 표현하세요; 또한 필드 선언부나 JPA 엔티티
제약(annotation)만으로는 부족하므로 비즈니스 요구(최소 1)를 반영하는 검증 로직을 생성자와 update 메서드에 중복 없이 호출되는
private validateMaxChoiceCount(int) 같은 헬퍼 메서드로 분리해 재사용하세요.
In `@src/main/resources/db/migration/V50__dynamic_choice_count.sql`:
- Around line 4-12: Add a foreign key constraint on
application_choice.univ_apply_info_id referencing univ_apply_info(id) to enforce
referential integrity (e.g., name it fk_app_choice_univ_apply_info) and also
create an index on application_choice.univ_apply_info_id to improve lookup
performance; update the CREATE TABLE or add ALTER TABLE / CREATE INDEX
statements that reference the application_choice table and the univ_apply_info
id column accordingly.
In
`@src/test/java/com/example/solidconnection/application/fixture/ApplicationFixtureBuilder.java`:
- Around line 66-73: The ApplicationFixtureBuilder currently constructs
Application with a hard-coded max choice count of 1, which prevents tests from
exercising multi-choice scenarios; update the builder to accept and store a
configurable maxChoiceCount parameter (exposed on the builder's
API/constructor), replace the literal 1 in the Application(...) call with that
stored maxChoiceCount, and update any builder creation sites/tests to pass the
desired maxChoiceCount so fixtures can create 2+ choice applications when needed
(reference ApplicationFixtureBuilder and the Application(...) constructor call
and the maxChoiceCount builder field).
In
`@src/test/java/com/example/solidconnection/application/service/ApplicationQueryServiceTest.java`:
- Around line 127-131: The test in ApplicationQueryServiceTest currently uses
assertThat(response.choices().get(0)).containsAll(...), which only checks for
presence and can miss extra/unexpected entries; update the assertion to assert
strict equality regardless of order by replacing containsAll with
containsExactlyInAnyOrder so that ApplicantsResponse.of(...) entries must match
exactly (use response.choices().get(0) and the same ApplicantsResponse.of(...)
expected values).
In
`@src/test/java/com/example/solidconnection/university/fixture/HomeUniversityFixtureBuilder.java`:
- Around line 31-33: The create() method in HomeUniversityFixtureBuilder
currently reuses an existing HomeUniversity found by name via
homeUniversityRepository.findByName(name) without verifying maxChoiceCount,
causing tests requesting a different maxChoiceCount to get the wrong fixture;
update create() so that after finding an existing HomeUniversity you compare its
getMaxChoiceCount() to the builder's maxChoiceCount and if they differ set the
new value on the entity and persist it (e.g., call
homeUniversityRepository.save(existing) or an update method), otherwise return
the existing entity; reference HomeUniversityFixtureBuilder.create,
homeUniversityRepository.findByName, HomeUniversity.getMaxChoiceCount(), and
homeUniversityRepository.save() when making the change.
---
Outside diff comments:
In
`@src/test/java/com/example/solidconnection/admin/university/service/AdminHomeUniversityServiceTest.java`:
- Around line 182-191: The test method 같은_이름으로_수정하면_성공한다 in
AdminHomeUniversityServiceTest is only asserting the name was updated but
ignores maxChoiceCount; update the assertion to also verify maxChoiceCount was
changed (either assert response.maxChoiceCount() equals the request value 3 or
reload the entity from the repository and assert
homeUniversity.getMaxChoiceCount() == 3) so the AdminHomeUniversityUpdateRequest
-> adminHomeUniversityService.updateHomeUniversity mapping for maxChoiceCount is
covered.
---
Nitpick comments:
In
`@src/main/java/com/example/solidconnection/application/domain/ApplicationChoice.java`:
- Around line 7-24: Implement proper equality for the embeddable
ApplicationChoice by adding equals(Object) and hashCode() that base equality on
the fields choiceOrder and univApplyInfoId (use java.util.Objects for null-safe
comparison / hashing); update ApplicationChoice to include these methods so
Hibernate’s ElementCollection dirty-checking and collection operations work
correctly—add the import java.util.Objects and implement equals to check
instance/type then compare choiceOrder and univApplyInfoId, and implement
hashCode to return Objects.hash(choiceOrder, univApplyInfoId).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 9303249c-c45a-4c36-8c1e-356aff3eeb78
📒 Files selected for processing (27)
src/main/java/com/example/solidconnection/admin/dto/UnivApplyInfoResponse.javasrc/main/java/com/example/solidconnection/admin/university/dto/AdminHomeUniversityCreateRequest.javasrc/main/java/com/example/solidconnection/admin/university/dto/AdminHomeUniversityResponse.javasrc/main/java/com/example/solidconnection/admin/university/dto/AdminHomeUniversityUpdateRequest.javasrc/main/java/com/example/solidconnection/admin/university/service/AdminHomeUniversityService.javasrc/main/java/com/example/solidconnection/application/domain/Application.javasrc/main/java/com/example/solidconnection/application/domain/ApplicationChoice.javasrc/main/java/com/example/solidconnection/application/dto/ApplicationsResponse.javasrc/main/java/com/example/solidconnection/application/dto/UnivApplyInfoChoiceRequest.javasrc/main/java/com/example/solidconnection/application/dto/UnivApplyInfoResponse.javasrc/main/java/com/example/solidconnection/application/repository/ApplicationRepository.javasrc/main/java/com/example/solidconnection/application/service/ApplicationQueryService.javasrc/main/java/com/example/solidconnection/application/service/ApplicationSubmissionService.javasrc/main/java/com/example/solidconnection/common/exception/ErrorCode.javasrc/main/java/com/example/solidconnection/siteuser/repository/custom/SiteUserFilterRepositoryImpl.javasrc/main/java/com/example/solidconnection/university/domain/HomeUniversity.javasrc/main/java/com/example/solidconnection/university/dto/validation/ValidUnivApplyInfoChoiceValidator.javasrc/main/resources/db/migration/V50__dynamic_choice_count.sqlsrc/test/java/com/example/solidconnection/admin/service/AdminUserServiceTest.javasrc/test/java/com/example/solidconnection/admin/university/service/AdminHomeUniversityServiceTest.javasrc/test/java/com/example/solidconnection/application/fixture/ApplicationFixture.javasrc/test/java/com/example/solidconnection/application/fixture/ApplicationFixtureBuilder.javasrc/test/java/com/example/solidconnection/application/service/ApplicationQueryServiceTest.javasrc/test/java/com/example/solidconnection/application/service/ApplicationSubmissionServiceTest.javasrc/test/java/com/example/solidconnection/university/dto/validation/ValidUnivApplyInfoChoiceValidatorTest.javasrc/test/java/com/example/solidconnection/university/fixture/HomeUniversityFixture.javasrc/test/java/com/example/solidconnection/university/fixture/HomeUniversityFixtureBuilder.java
- 저장 전에 검증하여 잘못된 id가 DB에 삽입되는 경우 방지 - 중복 조회 제거
- 최대 1지망 지원 가능
- 응답의 개수를 정확히 판별하기 위해
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/main/resources/db/migration/V50__dynamic_choice_count.sql (2)
5-17:⚠️ Potential issue | 🟠 Major | ⚡ Quick win2. 동일 지원서의 중복 지망을 DB가 막지 못합니다.
현재 PK가
(application_id, choice_order)라서 같은univ_apply_info_id를 다른 순번으로 여러 번 넣을 수 있습니다. 이번 변경에서 중복 지망을 invalid로 보는 계약이라면, 스키마에도(application_id, univ_apply_info_id)유니크 제약을 같이 두는 편이 안전합니다.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/resources/db/migration/V50__dynamic_choice_count.sql` around lines 5 - 17, The schema allows the same univ_apply_info_id to be added multiple times for one application because the PK is (application_id, choice_order); add a unique constraint on (application_id, univ_apply_info_id) to prevent duplicate choices. Modify the application_choice table definition to include a UNIQUE constraint (or create a unique index) named e.g. ux_app_choice_app_univ (referencing table application_choice and columns application_id and univ_apply_info_id) so the DB enforces uniqueness at insert/update.
5-20:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift1. 기존 지원서의 지망 데이터 이관 단계가 빠져 있습니다.
Application저장 모델을application_choice로 전환하는데, 이 마이그레이션에는 기존first/second/third지망 값을 새 테이블로 옮기는 단계가 없습니다. 지금 상태로 배포하면 기존 지원서는choices가 비어 있는 것으로 읽혀서 조회/분류 결과가 깨질 가능성이 큽니다. 최소한 백필 SQL을 같은 릴리스에 포함하거나, 엔티티 전환 전후를 분리한 2단계 배포로 가야 합니다.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/resources/db/migration/V50__dynamic_choice_count.sql` around lines 5 - 20, The migration creates a new application_choice table but omits backfilling existing first/second/third choice columns, so existing Application rows will lose choices; add steps to migrate data from application.first_choice_university_info_for_apply_id, application.second_choice_university_info_for_apply_id and application.third_choice_university_info_for_apply_id into application_choice (INSERT INTO application_choice (application_id, choice_order, univ_apply_info_id) SELECT id, 1/2/3, <column> FROM application WHERE <column> IS NOT NULL) before dropping or changing those columns, or split this into two migrations (1: add table and backfill, 2: remove old columns) to ensure no data loss and consistent reads; reference the application_choice table and the columns first_choice_university_info_for_apply_id/second_choice_university_info_for_apply_id/third_choice_university_info_for_apply_id and the application primary key to implement the backfill and ordering.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/main/resources/db/migration/V50__dynamic_choice_count.sql`:
- Around line 5-17: The schema allows the same univ_apply_info_id to be added
multiple times for one application because the PK is (application_id,
choice_order); add a unique constraint on (application_id, univ_apply_info_id)
to prevent duplicate choices. Modify the application_choice table definition to
include a UNIQUE constraint (or create a unique index) named e.g.
ux_app_choice_app_univ (referencing table application_choice and columns
application_id and univ_apply_info_id) so the DB enforces uniqueness at
insert/update.
- Around line 5-20: The migration creates a new application_choice table but
omits backfilling existing first/second/third choice columns, so existing
Application rows will lose choices; add steps to migrate data from
application.first_choice_university_info_for_apply_id,
application.second_choice_university_info_for_apply_id and
application.third_choice_university_info_for_apply_id into application_choice
(INSERT INTO application_choice (application_id, choice_order,
univ_apply_info_id) SELECT id, 1/2/3, <column> FROM application WHERE <column>
IS NOT NULL) before dropping or changing those columns, or split this into two
migrations (1: add table and backfill, 2: remove old columns) to ensure no data
loss and consistent reads; reference the application_choice table and the
columns
first_choice_university_info_for_apply_id/second_choice_university_info_for_apply_id/third_choice_university_info_for_apply_id
and the application primary key to implement the backfill and ordering.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 818d8abc-33fd-4589-8a06-f66ff8b8d3e8
📒 Files selected for processing (6)
src/main/java/com/example/solidconnection/application/domain/Application.javasrc/main/java/com/example/solidconnection/application/service/ApplicationSubmissionService.javasrc/main/resources/data.sqlsrc/main/resources/db/migration/V50__dynamic_choice_count.sqlsrc/test/java/com/example/solidconnection/application/service/ApplicationQueryServiceTest.javasrc/test/java/com/example/solidconnection/application/service/ApplicationSubmissionServiceTest.java
🚧 Files skipped from review as they are similar to previous changes (4)
- src/test/java/com/example/solidconnection/application/service/ApplicationSubmissionServiceTest.java
- src/test/java/com/example/solidconnection/application/service/ApplicationQueryServiceTest.java
- src/main/java/com/example/solidconnection/application/service/ApplicationSubmissionService.java
- src/main/java/com/example/solidconnection/application/domain/Application.java
sukangpunch
reviewed
Jun 10, 2026
sukangpunch
left a comment
sukangpunch
left a comment
Contributor
There was a problem hiding this comment.
전반적으로 고정 3지망 구조에서 동적 지망 수로 잘 전환되었습니다. 아래 몇 가지 개선 포인트를 남깁니다.
| @Service | ||
| public class ApplicationQueryService { | ||
|
|
||
| private static final int DEFAULT_MAX_CHOICE_COUNT = 3; |
Contributor
There was a problem hiding this comment.
ApplicationQueryService와 ApplicationSubmissionService 동일하게 선언되어 있습니다. 나중에 기본 지망 수 정책이 바뀔 때 한쪽만 수정되는 버그가 생길 수 있어요.
|
|
||
| public HomeUniversity 인하대학교(int maxChoiceCount) { | ||
| return homeUniversityFixtureBuilder.homeUniversity() | ||
| .name("인하대학교_" + maxChoiceCount + "지망") |
Contributor
There was a problem hiding this comment.
"인하대학교_2지망"처럼 숫자가 포함된 대학명이 생성되면 테스트 실패 메시지에서 맥락 파악이 어려울 수 있습니다. 이름 대신 별도 픽스처 메서드를 두거나, 아예 다른 가상의 대학명을 사용하는 편이 명확합니다.
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
2 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.
관련 이슈
작업 내용
1. 엔티티 구조 변경
Application엔티티에서firstChoice/secondChoice/thirdChoiceUnivApplyInfoId컬럼을 제거했습니다.@Embeddable클래스ApplicationChoice를 추가했습니다. (잘 몰랐는데@ElementCollection매핑 대상 클래스는@Embeddable이 있어야 한다고 하더라고요 ..)application_choice을@ElementCollection으로 매핑했습니다.Application을 조회하는 과정에서,ApplicationChoice컬렉션을 로드하기 위해 추가 쿼리가 발생할 수 있습니다(N+1 문제). 이를 방지하기 위해@BatchSize(size = 100)를 적용했습니다.HomeUniversity엔티티에maxChoiceCount컬럼을 추가했습니다.2. DTO 구조 변경
maxChoiceCount또한 받도록 수정했습니다.List<>타입으로 변경했습니다.3. 검증 로직 변경
4. 스키마 관련
5. 테스트 코드 변경
특이 사항
리뷰 요구사항 (선택)
작업 범위가 꽤 큽니다. 엣지 케이스 위주로 봐주시면 감사하겠습니다 !