feat: 어드민에서 지원 대학을 추가할 수 있도록#755
Merged
Merged
Conversation
* chore: application_choice 테이블 추가 및 지망 수 컬럼 추가하는 스크립트 작성 * feat: ApplicationChoice 클래스 추가, 최대 지망 수 필드 추가 * chore: n지망 컬럼 제약 조건 변경 - 컬럼 복수 유지에 대한 임시 조치 * feat: DTO 및 검증 로직 변경 * feat: JPQL 쿼리 변경 * feat: 서비스 메서드 변경 * feat: 어드민 국내 대학 삽입 시 최대 지망 수 관련 DTO에 제약조건 추가 * feat: 대학 검색 응답 필드 변경 * feat: 최대 지망 수 포함하도록 서비스 메서드 변경 * feat: QueryDSL 조인 -> 동적 선택으로 변경 * test: 테스트 픽스처 및 테스트 변경 * refactor: 컨벤션에 맞게 로직 변경 - private 메서드 위치 - Wrapper 타입 - 메서드 레퍼런스 방식 * refactor: 지망 리스트에서 null인 경우 검증 추가 * refactor: 초기 상태에서도 maxChoiceCount만큼의 리스트 크기가 생성되도록 * refactor: 불필요한 검증 제거 * refactor: BatchSize로 N+1 문제 해결 * test: 불필요한 Nested 제거 * chore: mock데이터 수정 * refactor: 유효한 id 검증을 서비스에 추가 - 저장 전에 검증하여 잘못된 id가 DB에 삽입되는 경우 방지 - 중복 조회 제거 * chore: CHECK 제약조건 추가 - 최대 1지망 지원 가능 * chore: FK, 인덱스 추가 * test: containsExactlyInAnyOrder로 변경 - 응답의 개수를 정확히 판별하기 위해 * refactor: 상수 도메인에서 중앙 관리하도록 * test: 최대 지망 대학임을 바로 알 수 있도록 메서드명 변경
whqtker
requested review from
Gyuhyeok99,
Hexeong,
JAEHEE25,
lsy1307,
sukangpunch and
wibaek
as code owners
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
Walkthrough이번 PR에서는 여러 관리자 기능이 함께 추가되고 기존 코드가 개선되었습니다.
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
🚥 Pre-merge checks | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✏️ 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: 0468dcfb56
ℹ️ 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: 8
🧹 Nitpick comments (7)
src/main/java/com/example/solidconnection/application/domain/ApplicationChoice.java (1)
7-24: ⚡ Quick win
equals()와hashCode()메서드 추가를 권장합니다.
- 컬렉션 동작:
@Embeddable클래스가@ElementCollection에서 사용될 때,equals()/hashCode()가 없으면 컬렉션 연산(remove, contains 등)이 예상대로 동작하지 않을 수 있습니다.- 데이터 일관성: 두
ApplicationChoice가 동일한지 비교할 때 객체 동등성 대신 값 동등성이 필요합니다.♻️ equals/hashCode 추가 제안
`@Embeddable` `@Getter` +@EqualsAndHashCode public class ApplicationChoice {또는 수동 구현:
+ `@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); + }🤖 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, The ApplicationChoice class is missing equals() and hashCode() methods, which are essential for an `@Embeddable` class used in collections. Add both methods to the ApplicationChoice class that compare instances based on their field values (choiceOrder and univApplyInfoId). This ensures that collection operations like remove and contains work correctly when ApplicationChoice instances are stored in `@ElementCollection` fields, and enables proper value equality comparisons between ApplicationChoice objects instead of relying on object reference equality.src/main/java/com/example/solidconnection/application/domain/Application.java (1)
78-86:choices필드의 즉시 로딩 전략 재검토 필요현재 EAGER 로딩과
@BatchSize(100)는 코드의 대부분의 경로에서choices를 즉시 접근하는 패턴에 최적화되어 있습니다. 예를 들어ApplicationQueryService의getApplicationsByChoiceOrder()메서드에서 반복 루프 내 모든 Application의 choices를 스트림으로 처리하며,SiteUserFilterRepositoryImpl과UnivApplyInfoResponse에서도 엔티티 로드 직후 choices를 접근합니다.다만 일부 쿼리 경로(예:
ApplicationSubmissionService라인 70,ApplicationQueryService라인 150)에서는 검증 목적으로만 Application을 조회하고 choices는 접근하지 않습니다. 이 점을 고려하면:
LAZY 전환 시 필수 조건: 현재 ApplicationRepository에 EntityGraph가 정의되지 않아, LAZY로 변경하면
buildChoiceMapForOrder()반복 구문에서 N+1 쿼리 문제가 발생합니다.현재 설계의 합리성:
@BatchSize(100)를 통해 컬렉션 일괄 로딩을 효율적으로 처리하므로, EAGER + 배치 전략이 이 코드의 지배적 접근 패턴에는 적합합니다.개선 방안: choices 접근 여부에 따라 쿼리 경로를 분리하거나, 성능 개선이 필요한 경우
@NamedEntityGraph를 정의하여 필요한 경로에서만 명시적으로 fetch join하는 방식을 고려하세요.🤖 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/Application.java` around lines 78 - 86, The choices field in the Application entity uses EAGER loading which loads choices for all queries regardless of whether they are accessed, leading to unnecessary overhead in validation-only queries like those in ApplicationSubmissionService line 70 and ApplicationQueryService line 150. To optimize, consider either splitting query paths based on whether choices are accessed, or defining a `@NamedEntityGraph` in ApplicationRepository to enable selective fetch join only where choices are needed, then change the choices field to LAZY loading. If using the EntityGraph approach, ensure to apply the graph explicitly in query methods that need choices (like buildChoiceMapForOrder() in ApplicationQueryService) to prevent N+1 query problems.src/test/java/com/example/solidconnection/admin/university/service/AdminUnivApplyInfoServiceTest.java (1)
253-291: ⚡ Quick win3) 예외 테스트는
CustomException타입뿐 아니라ErrorCode까지 함께 검증해 주세요.- 현재 Line 254~255, 272~273, 290~291은 타입만 확인해서, 잘못된 코드로 바뀌어도 테스트가 통과할 수 있습니다. - API 계약 회귀를 막으려면 케이스별 코드 단언이 필요합니다.제안 수정안
+import static org.assertj.core.api.Assertions.assertThatThrownBy; +import static com.example.solidconnection.common.exception.ErrorCode.INVALID_MARKDOWN_FORMAT; +import static com.example.solidconnection.common.exception.ErrorCode.TERM_NOT_FOUND; +import static com.example.solidconnection.common.exception.ErrorCode.HOME_UNIVERSITY_NOT_FOUND; - assertThatCode(() -> adminUnivApplyInfoService.importUnivApplyInfos(request)) - .isInstanceOf(CustomException.class); + assertThatThrownBy(() -> adminUnivApplyInfoService.importUnivApplyInfos(request)) + .isInstanceOfSatisfying(CustomException.class, + e -> assertThat(e.getErrorCode()).isEqualTo(INVALID_MARKDOWN_FORMAT)); - assertThatCode(() -> adminUnivApplyInfoService.importUnivApplyInfos(request)) - .isInstanceOf(CustomException.class); + assertThatThrownBy(() -> adminUnivApplyInfoService.importUnivApplyInfos(request)) + .isInstanceOfSatisfying(CustomException.class, + e -> assertThat(e.getErrorCode()).isEqualTo(TERM_NOT_FOUND)); - assertThatCode(() -> adminUnivApplyInfoService.importUnivApplyInfos(request)) - .isInstanceOf(CustomException.class); + assertThatThrownBy(() -> adminUnivApplyInfoService.importUnivApplyInfos(request)) + .isInstanceOfSatisfying(CustomException.class, + e -> assertThat(e.getErrorCode()).isEqualTo(HOME_UNIVERSITY_NOT_FOUND));🤖 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/AdminUnivApplyInfoServiceTest.java` around lines 253 - 291, The three test methods that check for invalid input (the unnamed test around line 254, 존재하지_않는_termId이면_예외_응답을_반환한다, and 존재하지_않는_homeUniversityId이면_예외_응답을_반환한다) currently only verify that a CustomException is thrown using assertThatCode().isInstanceOf(), but do not validate the specific ErrorCode. Update each of these test methods to also assert the expected ErrorCode value in addition to checking the exception type, so that API contract regressions are prevented if someone changes which error code is being thrown.src/main/java/com/example/solidconnection/university/domain/UnivApplyInfoColumn.java (1)
31-34: ⚡ Quick win1) 헤더 별칭 매칭은 입력 정규화(공백 제거)까지 같이 처리해 주세요
- Line 31~34에서 헤더를 원문 그대로 비교하고 있어, 앞뒤 공백이 포함된 입력이면 정상 별칭도 매칭 실패할 수 있습니다.
제안 diff
public static Optional<UnivApplyInfoColumn> findByAlias(String header) { + if (header == null) { + return Optional.empty(); + } + String normalizedHeader = header.trim(); return Arrays.stream(values()) - .filter(col -> col.aliases.contains(header)) + .filter(col -> col.aliases.contains(normalizedHeader)) .findFirst(); }🤖 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/university/domain/UnivApplyInfoColumn.java` around lines 31 - 34, The findByAlias method in UnivApplyInfoColumn compares the input header directly against aliases without normalizing whitespace, which causes valid aliases to fail matching when the input contains leading or trailing spaces. Modify the filter condition in the stream to normalize the input header by trimming whitespace before comparing it against the col.aliases to ensure consistent matching regardless of input formatting.src/test/java/com/example/solidconnection/application/fixture/ApplicationFixtureBuilder.java (1)
42-44: ⚡ Quick win1)
univApplyInfoIds에 null-safe + 방어적 복사를 적용해 주세요.현재는 입력 리스트를 그대로 보관해서
create()시점 NPE(입력이 null인 경우)나 외부 변경 사이드이펙트가 날 수 있습니다. 테스트 픽스처 안정성을 위해 setter에서 정규화해 두는 편이 안전합니다.제안 코드
public ApplicationFixtureBuilder univApplyInfoIds(List<Long> univApplyInfoIds) { - this.univApplyInfoIds = univApplyInfoIds; + this.univApplyInfoIds = (univApplyInfoIds == null) ? List.of() : List.copyOf(univApplyInfoIds); return this; }🤖 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/application/fixture/ApplicationFixtureBuilder.java` around lines 42 - 44, The univApplyInfoIds method in ApplicationFixtureBuilder needs to add null-safety and defensive copying. When the input univApplyInfoIds parameter is null, handle it safely by either initializing to an empty list or null value. When a non-null list is provided, create a defensive copy of it before assigning to the instance variable to prevent external modifications from affecting the fixture state and avoid NPE errors during create() execution.src/test/java/com/example/solidconnection/admin/university/service/AdminHomeUniversityServiceTest.java (1)
182-192: ⚡ Quick win1) 같은 이름 수정 케이스에서
maxChoiceCount변경 반영도 함께 검증해 주세요.현재 시나리오는 이름/지망수 모두 기존값이라, “이름은 같아도 지망수는 갱신되어야 함” 회귀를 잡지 못합니다. 이름은 그대로 두고
maxChoiceCount만 바꿔 검증하면 커버리지가 더 정확해집니다.제안 코드
`@Test` void 같은_이름으로_수정하면_성공한다() { // given HomeUniversity homeUniversity = homeUniversityFixture.인하대학교(); - AdminHomeUniversityUpdateRequest request = new AdminHomeUniversityUpdateRequest("인하대학교", 3); + AdminHomeUniversityUpdateRequest request = new AdminHomeUniversityUpdateRequest("인하대학교", 5); // when AdminHomeUniversityResponse response = adminHomeUniversityService.updateHomeUniversity(homeUniversity.getId(), request); // then - assertThat(response.name()).isEqualTo("인하대학교"); + HomeUniversity updated = homeUniversityRepository.findById(homeUniversity.getId()).orElseThrow(); + assertAll( + () -> assertThat(response.name()).isEqualTo("인하대학교"), + () -> assertThat(response.maxChoiceCount()).isEqualTo(5), + () -> assertThat(updated.getMaxChoiceCount()).isEqualTo(5) + ); }🤖 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 - 192, The test method `같은_이름으로_수정하면_성공한다()` currently verifies only the name field in the response assertion, but it does not validate that the `maxChoiceCount` is properly updated. Even though the request includes `maxChoiceCount` value of 3, the test does not confirm this value was actually persisted. To improve test coverage and catch potential regressions where `maxChoiceCount` updates fail silently, modify the test to set the `maxChoiceCount` in the request to a different value than what the fixture provides, and then add an additional assertion to verify that the response's `maxChoiceCount` matches the updated value from the request, not the original fixture value.src/test/java/com/example/solidconnection/common/util/MarkdownTableParserTest.java (1)
85-87: ⚡ Quick win1) 예외 테스트에서 에러 코드(메시지)까지 함께 검증해 주세요.
현재는
CustomException타입만 확인해서, 포맷 검증과 무관한 예외도 통과할 수 있습니다.INVALID_MARKDOWN_FORMAT메시지까지 고정하면 테스트 신뢰도가 올라갑니다.제안 코드
+import static com.example.solidconnection.common.exception.ErrorCode.INVALID_MARKDOWN_FORMAT; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatThrownBy; @@ assertThatThrownBy(() -> parser.parse(markdown)) - .isInstanceOf(CustomException.class); + .isInstanceOf(CustomException.class) + .hasMessage(INVALID_MARKDOWN_FORMAT.getMessage()); @@ assertThatThrownBy(() -> parser.parse(markdown)) - .isInstanceOf(CustomException.class); + .isInstanceOf(CustomException.class) + .hasMessage(INVALID_MARKDOWN_FORMAT.getMessage()); @@ assertThatThrownBy(() -> parser.parse(markdown)) - .isInstanceOf(CustomException.class); + .isInstanceOf(CustomException.class) + .hasMessage(INVALID_MARKDOWN_FORMAT.getMessage());Also applies to: 96-98, 104-106
🤖 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/common/util/MarkdownTableParserTest.java` around lines 85 - 87, The exception assertions in the MarkdownTableParserTest class currently only verify the exception type (CustomException) but do not validate the specific error code or message. This reduces test reliability since other exceptions could pass the check. Update all three exception test blocks (at lines 85-87, 96-98, and 104-106) to verify both the exception type AND that the exception message or error code equals INVALID_MARKDOWN_FORMAT. Use the hasMessage() or similar assertion method to chain the validation of the actual error message/code to the existing isInstanceOf(CustomException.class) check.
🤖 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/admin/university/dto/UnivApplyInfoImportRequest.java`:
- Around line 17-18: The columnMappings field in UnivApplyInfoImportRequest
currently only validates that the Map is not null using `@NotNull`, but does not
prevent empty maps or entries with blank/empty keys and values. Add additional
validation to ensure the columnMappings map is not empty and that neither its
keys nor values contain blank or empty strings. This can be accomplished by
either adding a custom validator annotation or implementing a class-level
validation method that checks these conditions, ensuring the import column
mapping contract is properly enforced before rows are processed.
In
`@src/main/java/com/example/solidconnection/admin/university/service/AdminUnivApplyInfoService.java`:
- Around line 45-51: The catch block in the row processing loop (around the
rowSaver.save call) is catching all Exception types, which masks infrastructure
and database failures as row-level failures and delays operational issue
detection. Additionally, directly exposing e.getMessage() risks leaking
sensitive internal error details. Refactor the exception handling to catch only
specific expected exceptions (such as business validation or domain-specific
exceptions that represent data quality issues), allowing unexpected exceptions
(database failures, infrastructure issues) to propagate. For the captured
expected exceptions, sanitize the error message before adding it to failedRows
instead of directly exposing getMessage(), either by using a generic message,
extracting a user-safe portion, or mapping the exception type to a predefined
error description.
In
`@src/main/java/com/example/solidconnection/application/domain/Application.java`:
- Around line 107-115: The Application constructor accepts a choices parameter
that is passed directly to new ArrayList<>(choices) without null validation,
which causes a NullPointerException with no meaningful error message if null is
provided. Add explicit null validation for the choices parameter at the
beginning of the constructor before using it, and throw an
IllegalArgumentException with a clear error message if choices is null, ensuring
that the failure reason is immediately apparent to callers.
In
`@src/main/java/com/example/solidconnection/application/dto/UnivApplyInfoResponse.java`:
- Around line 17-20: The `nameById` map lookup in both files fails to handle the
case where a key is not found, allowing null values to be included in results
when deleted `UnivApplyInfo` records are referenced. In
`src/main/java/com/example/solidconnection/application/dto/UnivApplyInfoResponse.java`
at lines 17-20, update the map lookup for `choice.getUnivApplyInfoId()` to use
`getOrDefault(choice.getUnivApplyInfoId(), "알 수 없음")` instead of `get()`. In
`src/main/java/com/example/solidconnection/siteuser/repository/custom/SiteUserFilterRepositoryImpl.java`
at lines 350-358, apply the same defensive logic to the `nameById::get` method
reference call by using `getOrDefault()` with a default fallback value. Ensure
both locations use the same null-handling approach for consistency.
In
`@src/main/java/com/example/solidconnection/application/service/ApplicationSubmissionService.java`:
- Around line 149-153: The getRandomNickname method has a TOCTOU race condition
where the existsByNicknameForApply check and actual database save are separated,
allowing duplicate nicknames in concurrent scenarios. Remove the pre-save
existence check in lines 151-153 and instead implement a bounded retry mechanism
that attempts to save with the generated nickname and catches unique constraint
violations, retrying with a new random nickname up to a maximum number of
attempts. Additionally, ensure the database has a unique constraint on the
nickname column to enforce data integrity at the database level.
In
`@src/main/java/com/example/solidconnection/common/util/MarkdownTableParser.java`:
- Around line 20-23: In the MarkdownTableParser.java file at lines 20-23, empty
or blank lines in the markdown table are being processed as valid rows, which
creates empty Map objects that cause failures during the save step. Add a filter
in the stream chain after the skip operation and before the map operation that
calls parseRow and buildRowMap to exclude lines that are empty or contain only
whitespace. This will prevent blank lines from being converted into empty Maps
and eliminate the "fake failure rows" that appear during the save phase.
In
`@src/main/java/com/example/solidconnection/university/domain/HomeUniversity.java`:
- Around line 33-35: The update method in the HomeUniversity class lacks domain
invariant validation for the maxChoiceCount parameter. Add validation logic in
the update method to ensure that maxChoiceCount is a positive value (greater
than 0) before assigning it to the instance variable. This will prevent invalid
values from being stored at the entity level and block validation bypass paths
such as direct entity creation or batch/migration inputs.
In
`@src/test/java/com/example/solidconnection/university/fixture/HomeUniversityFixtureBuilder.java`:
- Around line 31-33: The create() method in HomeUniversityFixtureBuilder
silently reuses an existing HomeUniversity entity when found by name, even if
the requested maxChoiceCount differs from the stored value. Modify the
findByName().orElseGet() chain to validate that when an existing entity is
retrieved, its maxChoiceCount matches the field value in the builder. If there's
a mismatch, throw an exception immediately to fail fast rather than silently
returning an incorrectly configured entity.
---
Nitpick comments:
In
`@src/main/java/com/example/solidconnection/application/domain/Application.java`:
- Around line 78-86: The choices field in the Application entity uses EAGER
loading which loads choices for all queries regardless of whether they are
accessed, leading to unnecessary overhead in validation-only queries like those
in ApplicationSubmissionService line 70 and ApplicationQueryService line 150. To
optimize, consider either splitting query paths based on whether choices are
accessed, or defining a `@NamedEntityGraph` in ApplicationRepository to enable
selective fetch join only where choices are needed, then change the choices
field to LAZY loading. If using the EntityGraph approach, ensure to apply the
graph explicitly in query methods that need choices (like
buildChoiceMapForOrder() in ApplicationQueryService) to prevent N+1 query
problems.
In
`@src/main/java/com/example/solidconnection/application/domain/ApplicationChoice.java`:
- Around line 7-24: The ApplicationChoice class is missing equals() and
hashCode() methods, which are essential for an `@Embeddable` class used in
collections. Add both methods to the ApplicationChoice class that compare
instances based on their field values (choiceOrder and univApplyInfoId). This
ensures that collection operations like remove and contains work correctly when
ApplicationChoice instances are stored in `@ElementCollection` fields, and enables
proper value equality comparisons between ApplicationChoice objects instead of
relying on object reference equality.
In
`@src/main/java/com/example/solidconnection/university/domain/UnivApplyInfoColumn.java`:
- Around line 31-34: The findByAlias method in UnivApplyInfoColumn compares the
input header directly against aliases without normalizing whitespace, which
causes valid aliases to fail matching when the input contains leading or
trailing spaces. Modify the filter condition in the stream to normalize the
input header by trimming whitespace before comparing it against the col.aliases
to ensure consistent matching regardless of input formatting.
In
`@src/test/java/com/example/solidconnection/admin/university/service/AdminHomeUniversityServiceTest.java`:
- Around line 182-192: The test method `같은_이름으로_수정하면_성공한다()` currently verifies
only the name field in the response assertion, but it does not validate that the
`maxChoiceCount` is properly updated. Even though the request includes
`maxChoiceCount` value of 3, the test does not confirm this value was actually
persisted. To improve test coverage and catch potential regressions where
`maxChoiceCount` updates fail silently, modify the test to set the
`maxChoiceCount` in the request to a different value than what the fixture
provides, and then add an additional assertion to verify that the response's
`maxChoiceCount` matches the updated value from the request, not the original
fixture value.
In
`@src/test/java/com/example/solidconnection/admin/university/service/AdminUnivApplyInfoServiceTest.java`:
- Around line 253-291: The three test methods that check for invalid input (the
unnamed test around line 254, 존재하지_않는_termId이면_예외_응답을_반환한다, and
존재하지_않는_homeUniversityId이면_예외_응답을_반환한다) currently only verify that a
CustomException is thrown using assertThatCode().isInstanceOf(), but do not
validate the specific ErrorCode. Update each of these test methods to also
assert the expected ErrorCode value in addition to checking the exception type,
so that API contract regressions are prevented if someone changes which error
code is being thrown.
In
`@src/test/java/com/example/solidconnection/application/fixture/ApplicationFixtureBuilder.java`:
- Around line 42-44: The univApplyInfoIds method in ApplicationFixtureBuilder
needs to add null-safety and defensive copying. When the input univApplyInfoIds
parameter is null, handle it safely by either initializing to an empty list or
null value. When a non-null list is provided, create a defensive copy of it
before assigning to the instance variable to prevent external modifications from
affecting the fixture state and avoid NPE errors during create() execution.
In
`@src/test/java/com/example/solidconnection/common/util/MarkdownTableParserTest.java`:
- Around line 85-87: The exception assertions in the MarkdownTableParserTest
class currently only verify the exception type (CustomException) but do not
validate the specific error code or message. This reduces test reliability since
other exceptions could pass the check. Update all three exception test blocks
(at lines 85-87, 96-98, and 104-106) to verify both the exception type AND that
the exception message or error code equals INVALID_MARKDOWN_FORMAT. Use the
hasMessage() or similar assertion method to chain the validation of the actual
error message/code to the existing isInstanceOf(CustomException.class) check.
🪄 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: c7896721-0cdd-4982-8f2d-fda9d6e2ec2a
📒 Files selected for processing (38)
src/main/java/com/example/solidconnection/admin/dto/UnivApplyInfoResponse.javasrc/main/java/com/example/solidconnection/admin/university/controller/AdminUnivApplyInfoController.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/dto/UnivApplyInfoFieldResponse.javasrc/main/java/com/example/solidconnection/admin/university/dto/UnivApplyInfoImportRequest.javasrc/main/java/com/example/solidconnection/admin/university/dto/UnivApplyInfoImportResponse.javasrc/main/java/com/example/solidconnection/admin/university/service/AdminHomeUniversityService.javasrc/main/java/com/example/solidconnection/admin/university/service/AdminUnivApplyInfoRowSaver.javasrc/main/java/com/example/solidconnection/admin/university/service/AdminUnivApplyInfoService.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/common/util/MarkdownTableParser.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/domain/UnivApplyInfoColumn.javasrc/main/java/com/example/solidconnection/university/dto/validation/ValidUnivApplyInfoChoiceValidator.javasrc/main/resources/data.sqlsrc/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/admin/university/service/AdminUnivApplyInfoServiceTest.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/common/util/MarkdownTableParserTest.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
- admin@test.email - Admin@1234 - 로컬 어드민 웹에 대한 접근 허용
- 추후 제거 예정
- host_university가 없는 경우 추가해줘야 한다.
whqtker
force-pushed
the
feat/745-univ-apply-info-import
branch
from
0468dcf to
6046b3f
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
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/admin/university/service/AdminUnivApplyInfoRowSaver.java (1)
122-124:⚠️ Potential issue | 🟠 Major | ⚡ Quick win2) 잘못된
columnMappings값이 조용히extraInfo로 떨어져 데이터가 누락됩니다헤더를 명시 매핑했는데 값이 오타인 경우에도 실패하지 않고
extraInfo로 저장됩니다. 이러면 구조화 필드가 누락된 채successCount가 올라가 데이터 정합성이 깨집니다.🔧 제안 수정안
private void applyField(ImportData data, String header, String value, Map<String, String> columnMappings) { - String targetField = columnMappings.getOrDefault(header, "extraInfo"); + boolean explicitlyMapped = columnMappings.containsKey(header); + String targetField = columnMappings.getOrDefault(header, "extraInfo"); @@ - applyStructuredField(data, header, targetField, value); + applyStructuredField(data, header, targetField, value, explicitlyMapped); } - private void applyStructuredField(ImportData data, String header, String fieldName, String value) { + private void applyStructuredField( + ImportData data, String header, String fieldName, String value, boolean explicitlyMapped + ) { switch (fieldName) { @@ - default -> data.extraInfo.put(header, value); + default -> { + if (explicitlyMapped) { + throw new CustomException(INVALID_INPUT, "유효하지 않은 컬럼 매핑 값입니다: " + fieldName); + } + data.extraInfo.put(header, value); + } } }Also applies to: 142-185
🤖 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/admin/university/service/AdminUnivApplyInfoRowSaver.java` around lines 122 - 124, The applyField method is silently falling back to "extraInfo" when a header is not found in the columnMappings using getOrDefault, which causes structured field data to be lost without detection. Instead of using getOrDefault with "extraInfo" as the default, validate that the header exists in the columnMappings and throw an exception or error when it is not found. This will prevent typos and missing mappings from silently corrupting data while still allowing successCount to increment, which breaks data consistency and makes debugging difficult.
🧹 Nitpick comments (1)
src/test/java/com/example/solidconnection/common/util/MarkdownTableParserTest.java (1)
100-121: ⚡ Quick win1) 예외 “타입”만 검증하면 에러 코드 회귀를 놓칠 수 있습니다.
현재는
CustomException여부만 확인해서, 다른 에러 코드로 잘못 던져도 테스트가 통과할 수 있습니다. 구조 검증 계약을 고정하려면INVALID_MARKDOWN_FORMAT까지 함께 검증하는 편이 안전합니다.변경 예시
+import static com.example.solidconnection.common.exception.ErrorCode.INVALID_MARKDOWN_FORMAT; @@ assertThatThrownBy(() -> parser.parse(markdown)) - .isInstanceOf(CustomException.class); + .isInstanceOf(CustomException.class) + .extracting("errorCode") + .isEqualTo(INVALID_MARKDOWN_FORMAT); @@ assertThatThrownBy(() -> parser.parse(markdown)) - .isInstanceOf(CustomException.class); + .isInstanceOf(CustomException.class) + .extracting("errorCode") + .isEqualTo(INVALID_MARKDOWN_FORMAT); @@ assertThatThrownBy(() -> parser.parse(markdown)) - .isInstanceOf(CustomException.class); + .isInstanceOf(CustomException.class) + .extracting("errorCode") + .isEqualTo(INVALID_MARKDOWN_FORMAT);🤖 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/common/util/MarkdownTableParserTest.java` around lines 100 - 121, The test methods `데이터_행이_없으면_예외를_던진다`, `헤더와_구분자만_있으면_예외를_던진다`, and the preceding test are only verifying that a CustomException is thrown but not checking the specific error code, which allows regressions to slip through. Enhance each assertThatThrownBy call to also verify that the exception has the error code `INVALID_MARKDOWN_FORMAT` by chaining an additional assertion that checks the error code value after verifying the exception instance type.
🤖 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/admin/term/service/AdminTermService.java`:
- Around line 31-37: The createTerm method in AdminTermService has a race
condition vulnerability where concurrent requests can bypass the findByName
validation and trigger a database unique constraint violation that is not
properly handled. To fix this, wrap the termRepository.save operation in a
try-catch block to catch the database unique constraint exception (typically
DataIntegrityViolationException or similar depending on your framework), and
re-throw it as a CustomException with TERM_ALREADY_EXISTS. This ensures that
duplicate term creation attempts are consistently mapped to the appropriate
exception regardless of timing, preventing 500 errors from being exposed to
clients.
- Around line 41-46: In the activateTerm method, the current implementation
first fetches the target term into the persistence context, then performs a bulk
deactivate operation on the database, which can cause the persistence context
state to diverge from the database state if the target term was already current.
To fix this, reorder the operations by calling
termRepository.deactivateCurrentTerm() before fetching the target term using
termRepository.findById(id), or alternatively refresh the target entity from the
database after the deactivate operation to ensure persistence context
consistency before calling target.activate().
In
`@src/main/java/com/example/solidconnection/admin/university/service/AdminUnivApplyInfoRowSaver.java`:
- Around line 47-52: The `universityKoreanName` field extracted from
`buildImportData` is used in `hostUniversityRepository.findByKoreanName()` and
`findOrCreateHostUniversity()` without validation, allowing empty or null values
to be processed. Add explicit null and blank validation for
`data.universityKoreanName` immediately after the `buildImportData` call and
before the `findByKoreanName()` lookup, throwing an appropriate exception if the
validation fails since this field serves as an identifier.
In `@src/main/resources/data.sql`:
- Around line 47-53: Remove the plain text password hints from the comments on
lines 50 and 53 (the `-- 12341234` and `-- Admin@1234` comments) in the INSERT
statement for site_user. Additionally, separate the admin account insertion (the
second VALUES row with 'admin@test.email' and 'ADMIN' role) from the general
seed data into a separate SQL file or configuration that is only used for
development/testing environments, not for production deployments. This prevents
hardcoded admin credentials from being exposed in the main data seed file.
In
`@src/test/java/com/example/solidconnection/admin/university/service/AdminUnivApplyInfoServiceTest.java`:
- Around line 248-265: The test method 대학명이_비어있으면_행이_실패한다 is intended to
validate empty university name handling, but the markdown table input at line
253 contains "Belgium" which causes the test to fail on country code validation
instead of university name validation. This means the test could pass even
without proper empty university name validation. To fix this, replace "Belgium"
with a valid country code value so that the test failure is specifically
triggered by the empty university name field rather than the country code field,
ensuring the test actually validates the intended behavior.
---
Outside diff comments:
In
`@src/main/java/com/example/solidconnection/admin/university/service/AdminUnivApplyInfoRowSaver.java`:
- Around line 122-124: The applyField method is silently falling back to
"extraInfo" when a header is not found in the columnMappings using getOrDefault,
which causes structured field data to be lost without detection. Instead of
using getOrDefault with "extraInfo" as the default, validate that the header
exists in the columnMappings and throw an exception or error when it is not
found. This will prevent typos and missing mappings from silently corrupting
data while still allowing successCount to increment, which breaks data
consistency and makes debugging difficult.
---
Nitpick comments:
In
`@src/test/java/com/example/solidconnection/common/util/MarkdownTableParserTest.java`:
- Around line 100-121: The test methods `데이터_행이_없으면_예외를_던진다`,
`헤더와_구분자만_있으면_예외를_던진다`, and the preceding test are only verifying that a
CustomException is thrown but not checking the specific error code, which allows
regressions to slip through. Enhance each assertThatThrownBy call to also verify
that the exception has the error code `INVALID_MARKDOWN_FORMAT` by chaining an
additional assertion that checks the error code value after verifying the
exception instance type.
🪄 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: 56d011dc-8e6d-49d0-b227-e97bf1d75b78
📒 Files selected for processing (17)
src/main/java/com/example/solidconnection/admin/term/controller/AdminTermController.javasrc/main/java/com/example/solidconnection/admin/term/dto/AdminTermCreateRequest.javasrc/main/java/com/example/solidconnection/admin/term/dto/AdminTermResponse.javasrc/main/java/com/example/solidconnection/admin/term/service/AdminTermService.javasrc/main/java/com/example/solidconnection/admin/university/dto/UnivApplyInfoFieldResponse.javasrc/main/java/com/example/solidconnection/admin/university/dto/UnivApplyInfoImportResponse.javasrc/main/java/com/example/solidconnection/admin/university/service/AdminUnivApplyInfoRowSaver.javasrc/main/java/com/example/solidconnection/admin/university/service/AdminUnivApplyInfoService.javasrc/main/java/com/example/solidconnection/common/exception/ErrorCode.javasrc/main/java/com/example/solidconnection/common/util/MarkdownTableParser.javasrc/main/java/com/example/solidconnection/term/domain/Term.javasrc/main/java/com/example/solidconnection/term/repository/TermRepository.javasrc/main/java/com/example/solidconnection/university/domain/UnivApplyInfoColumn.javasrc/main/resources/config/application-variable.ymlsrc/main/resources/data.sqlsrc/test/java/com/example/solidconnection/admin/university/service/AdminUnivApplyInfoServiceTest.javasrc/test/java/com/example/solidconnection/common/util/MarkdownTableParserTest.java
✅ Files skipped from review due to trivial changes (1)
- src/main/java/com/example/solidconnection/admin/term/dto/AdminTermResponse.java
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/java/com/example/solidconnection/common/exception/ErrorCode.java
- src/main/java/com/example/solidconnection/common/util/MarkdownTableParser.java
- label -> name
- 활성화할 학기에 대한 변수 이름을 termToActivate로 수정한다.
- 영문 대학명: 200자 - 기숙사, 어학 세부 조건: 2000자
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/test/java/com/example/solidconnection/admin/university/service/AdminUnivApplyInfoServiceTest.java (1)
239-245:⚠️ Potential issue | 🟠 Major | ⚡ Quick win1) 테스트 실패 원인을 대학명 공백으로 고정해주세요.
Line 244의
"Belgium"값 때문에 국가코드 검증이 먼저 실패하면, 이 테스트는대학명 공백검증 없이도 통과할 수 있습니다.변경 제안
void 대학명이_비어있으면_전체가_실패한다() { // given String markdown = """ | 대학명 | 국가코드 | |--------|----------| - | | Belgium | + | | US | """;🤖 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/AdminUnivApplyInfoServiceTest.java` around lines 239 - 245, The test method `대학명이_비어있으면_전체가_실패한다` is intended to verify that validation fails due to an empty university name, but the current markdown data in the test has `"Belgium"` as the country code value which may cause country code validation to fail first. Replace the `"Belgium"` value with a valid country code to ensure that the test failure is specifically caused by the empty university name field and not by invalid country code validation occurring first. This ensures the test accurately validates the intended requirement.
🤖 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.
Duplicate comments:
In
`@src/test/java/com/example/solidconnection/admin/university/service/AdminUnivApplyInfoServiceTest.java`:
- Around line 239-245: The test method `대학명이_비어있으면_전체가_실패한다` is intended to
verify that validation fails due to an empty university name, but the current
markdown data in the test has `"Belgium"` as the country code value which may
cause country code validation to fail first. Replace the `"Belgium"` value with
a valid country code to ensure that the test failure is specifically caused by
the empty university name field and not by invalid country code validation
occurring first. This ensures the test accurately validates the intended
requirement.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 31bf5dfb-9cfa-4b1b-a603-5b9950b882b1
📒 Files selected for processing (13)
src/main/java/com/example/solidconnection/admin/term/dto/AdminTermResponse.javasrc/main/java/com/example/solidconnection/admin/term/service/AdminTermService.javasrc/main/java/com/example/solidconnection/admin/university/dto/AdminHostUniversityCreateRequest.javasrc/main/java/com/example/solidconnection/admin/university/dto/AdminHostUniversityUpdateRequest.javasrc/main/java/com/example/solidconnection/admin/university/dto/UnivApplyInfoFieldResponse.javasrc/main/java/com/example/solidconnection/admin/university/dto/UnivApplyInfoImportRequest.javasrc/main/java/com/example/solidconnection/admin/university/service/AdminUnivApplyInfoRowSaver.javasrc/main/java/com/example/solidconnection/admin/university/service/AdminUnivApplyInfoService.javasrc/main/java/com/example/solidconnection/university/domain/HostUniversity.javasrc/main/java/com/example/solidconnection/university/domain/UnivApplyInfo.javasrc/main/resources/data.sqlsrc/main/resources/db/migration/V51__expand_column_length.sqlsrc/test/java/com/example/solidconnection/admin/university/service/AdminUnivApplyInfoServiceTest.java
✅ Files skipped from review due to trivial changes (2)
- src/main/resources/db/migration/V51__expand_column_length.sql
- src/main/java/com/example/solidconnection/university/domain/UnivApplyInfo.java
🚧 Files skipped from review as they are similar to previous changes (5)
- src/main/java/com/example/solidconnection/admin/term/dto/AdminTermResponse.java
- src/main/java/com/example/solidconnection/admin/university/dto/UnivApplyInfoImportRequest.java
- src/main/java/com/example/solidconnection/admin/term/service/AdminTermService.java
- src/main/java/com/example/solidconnection/admin/university/dto/UnivApplyInfoFieldResponse.java
- src/main/java/com/example/solidconnection/admin/university/service/AdminUnivApplyInfoRowSaver.java
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/resources/db/migration/V50__dynamic_choice_count.sql`:
- Around line 32-34: Before adding the UNIQUE constraint
uk_application_nickname_for_apply on the nickname_for_apply column in the
application table, you must first handle any existing duplicate values in the
production database. Add a query at the beginning of the migration script to
identify duplicates by grouping on nickname_for_apply and filtering for counts
greater than 1, then implement logic to either remove the duplicates or
consolidate them into a single record. Only after ensuring no duplicate values
exist should the ALTER TABLE statement with the UNIQUE constraint be executed to
prevent migration failures and unexpected rollbacks in production.
🪄 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: 0c796ce0-199d-4716-a9a3-cd8d929ce521
📒 Files selected for processing (11)
src/main/java/com/example/solidconnection/admin/term/controller/AdminTermController.javasrc/main/java/com/example/solidconnection/admin/term/service/AdminTermService.javasrc/main/java/com/example/solidconnection/admin/university/service/AdminUnivApplyInfoRowSaver.javasrc/main/java/com/example/solidconnection/admin/university/service/AdminUnivApplyInfoService.javasrc/main/java/com/example/solidconnection/application/service/ApplicationSubmissionService.javasrc/main/java/com/example/solidconnection/common/exception/ErrorCode.javasrc/main/java/com/example/solidconnection/common/util/MarkdownTableParser.javasrc/main/java/com/example/solidconnection/siteuser/repository/custom/SiteUserFilterRepositoryImpl.javasrc/main/resources/db/migration/V50__dynamic_choice_count.sqlsrc/test/java/com/example/solidconnection/admin/university/service/AdminUnivApplyInfoServiceTest.javasrc/test/java/com/example/solidconnection/common/util/MarkdownTableParserTest.java
✅ Files skipped from review due to trivial changes (1)
- src/main/java/com/example/solidconnection/common/exception/ErrorCode.java
🚧 Files skipped from review as they are similar to previous changes (7)
- src/main/java/com/example/solidconnection/admin/university/service/AdminUnivApplyInfoService.java
- src/main/java/com/example/solidconnection/admin/term/controller/AdminTermController.java
- src/main/java/com/example/solidconnection/siteuser/repository/custom/SiteUserFilterRepositoryImpl.java
- src/test/java/com/example/solidconnection/common/util/MarkdownTableParserTest.java
- src/main/java/com/example/solidconnection/application/service/ApplicationSubmissionService.java
- src/test/java/com/example/solidconnection/admin/university/service/AdminUnivApplyInfoServiceTest.java
- src/main/java/com/example/solidconnection/admin/university/service/AdminUnivApplyInfoRowSaver.java
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
1 participant
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.
관련 이슈
작업 내용
특이 사항
사용 방법은 추후 노션 페이지에 정리하겠습니다.
리뷰 요구사항 (선택)