MOSU-62 feat: application request, reponse 타입 변경 #64
Conversation
WalkthroughThe changes refactor the application process for schools by shifting from using school IDs in requests to identifying schools by name, area, and exam date. The existence check for duplicate applications is now performed per school. The Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ApplicationService
participant SchoolJpaRepository
participant ApplicationSchoolJpaRepository
User->>ApplicationService: apply(userId, ApplicationRequest)
loop For each school in request
ApplicationService->>SchoolJpaRepository: findBySchoolNameAndAreaAndExamDate()
SchoolJpaRepository-->>ApplicationService: SchoolJpaEntity
ApplicationService->>ApplicationSchoolJpaRepository: existsByUserIdAndSchoolId()
ApplicationSchoolJpaRepository-->>ApplicationService: boolean
alt Not exists
ApplicationService->>ApplicationSchoolJpaRepository: save(ApplicationSchoolJpaEntity)
else Already exists
ApplicationService-->>User: throw exception
end
end
ApplicationService-->>User: ApplicationResponse
Suggested reviewers
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @chominju02, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request refactors the data types and associated logic for handling application requests and responses, particularly concerning school information. The primary change involves shifting from identifying schools by a direct ID in the request to using a combination of school name, area, and exam date. This allows the system to fetch comprehensive school details from the database and ensures that the application response provides more user-friendly string representations for enum values like area and lunch.
Highlights
- Application Request Type Change: The
ApplicationSchoolRequestno longer requires aschoolId. Instead, schools are now identified by theirschoolName,area, andexamDatein the request payload. - School Lookup and Validation Logic: The
ApplicationServicehas been updated to findSchoolJpaEntityinstances using the providedschoolName,area, andexamDatefrom the request. It also now checks for existing applications for each school individually rather than in bulk. - Application Response Type Change: The
ApplicationSchoolResponsenow returns theareaandlunchfields as user-friendlyStringnames (e.g., '대치', '점심 제공') instead of their raw enum objects, improving client-side readability. - Repository Method Updates: The
ApplicationSchoolJpaRepository'sexistsByUserIdAndSchoolIdInmethod has been replaced withexistsByUserIdAndSchoolIdto support the new individual school validation flow. Additionally,SchoolJpaRepositorynow includes a new query method,findBySchoolNameAndAreaAndExamDate, to facilitate the updated school lookup logic. - Enum Data Update: The
Areaenum has been modified, replacing theDAEGUentry withNOWON.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
The pull request modifies the application request and response types. The request type changes involve removing the school ID and using school name, area, and exam date to identify the school. The response type changes involve returning area and lunch as strings instead of enums. The code also includes updates to the repository query and service logic to accommodate these changes.
| Long schoolId = schoolJpaRepository.findBySchoolNameAndAreaAndExamDate( | ||
| schoolRequest.schoolName(), | ||
| schoolRequest.validatedArea(schoolRequest.area()), | ||
| schoolRequest.examDate()) | ||
| .orElseThrow(() -> new CustomRuntimeException(ErrorCode.SCHOOL_NOT_FOUND)) |
There was a problem hiding this comment.
It looks like you're querying the schoolJpaRepository to find a school by name, area, and exam date, and then immediately extracting the ID. Consider creating a method that directly retrieves the school ID based on these criteria to avoid fetching the entire entity when only the ID is needed. This could improve performance, especially if the SchoolJpaEntity contains a lot of data.
Long schoolId = schoolJpaRepository.findSchoolIdBySchoolNameAndAreaAndExamDate(
schoolRequest.schoolName(),
schoolRequest.validatedArea(schoolRequest.area()),
schoolRequest.examDate())
.orElseThrow(() -> new CustomRuntimeException(ErrorCode.SCHOOL_NOT_FOUND));| if (applicationSchoolJpaRepository.existsByUserIdAndSchoolId(userId, schoolId)) { | ||
| throw new CustomRuntimeException(ErrorCode.APPLICATION_SCHOOL_ALREADY_APPLIED); | ||
| } |
There was a problem hiding this comment.
The check for existing applications is performed after retrieving the schoolId. If findBySchoolNameAndAreaAndExamDate fails, the application will throw a SCHOOL_NOT_FOUND exception. It might be more efficient to check for existing applications before attempting to retrieve the schoolId. This could prevent unnecessary database queries when the school doesn't exist.
if (applicationSchoolJpaRepository.existsByUserIdAndSchoolId(
userId, schoolRequest.schoolName(), schoolRequest.area(), schoolRequest.examDate())) {
throw new CustomRuntimeException(ErrorCode.APPLICATION_SCHOOL_ALREADY_APPLIED);
}![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/main/java/life/mosu/mosuserver/application/application/ApplicationService.java(1 hunks)src/main/java/life/mosu/mosuserver/domain/application/ApplicationSchoolJpaRepository.java(1 hunks)src/main/java/life/mosu/mosuserver/domain/school/Area.java(1 hunks)src/main/java/life/mosu/mosuserver/domain/school/SchoolJpaRepository.java(1 hunks)src/main/java/life/mosu/mosuserver/presentation/application/dto/ApplicationSchoolRequest.java(3 hunks)src/main/java/life/mosu/mosuserver/presentation/application/dto/ApplicationSchoolResponse.java(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build_and_ci
🔇 Additional comments (7)
src/main/java/life/mosu/mosuserver/presentation/application/dto/ApplicationSchoolResponse.java (2)
11-11: LGTM: Clean conversion to string representation.The changes correctly convert domain types to string representations for the API response, which is appropriate for presentation layer DTOs.
Also applies to: 14-14
20-22: LGTM: Well-structured factory method implementation.The factory method correctly extracts string values from domain objects and uses clear variable names for readability.
Also applies to: 29-29, 32-32
src/main/java/life/mosu/mosuserver/presentation/application/dto/ApplicationSchoolRequest.java (2)
65-65: validatedArea visibility is intentional
ThevalidatedAreamethod is called fromApplicationService(schoolRequest.validatedArea(schoolRequest.area())insrc/main/java/life/mosu/mosuserver/application/application/ApplicationService.java), so it must bepublic. No changes required here.
32-45: toEntity method implementation verifiedAll checks confirmed:
- School-related fields (schoolId, schoolName, area, address) are populated from the passed-in SchoolJpaEntity.
- The SchoolJpaEntity is retrieved via
schoolJpaRepository.findById(…).orElseThrow(...), guaranteeing it’s non-null.- The updated method signature (
toEntity(Long, Long, SchoolJpaEntity)) is used inApplicationService’sschoolRequest.toEntity(userId, applicationId, school)call and no other obsolete overloads exist.No further changes needed.
src/main/java/life/mosu/mosuserver/application/application/ApplicationService.java (3)
39-40: LGTM: Clean variable initialization.The variable declarations are clear and appropriate for the refactored logic.
42-43: LGTM: Standard entity creation pattern.The application entity creation and ID extraction follows the expected pattern.
68-68: LGTM: Appropriate response creation.The response creation using the saved entities list is correct and follows the expected pattern.
| MOKDONG("목동"), | ||
| DAEGU("대구"); | ||
|
|
||
| NOWON("노원"); |
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
Verify data migration for the Area enum change.
The change from DAEGU to NOWON could break existing data if there are schools or applications that reference the DAEGU area. Ensure that:
- All existing data using
DAEGUhas been properly migrated - No foreign key constraints will be violated
- This change is coordinated with any database schema updates
🏁 Script executed:
#!/bin/bash
# Check for any references to DAEGU in the codebase
rg -i "DAEGU" --type java
# Check for any database migration files
fd -e sql -e xml migrationLength of output: 54
Add a database migration for the Area enum change
Your enum update replaces DAEGU with NOWON but there are no existing migration scripts to handle this. Without one, any stored records with DAEGU will fail to deserialize. Please:
- Add a migration (e.g. Flyway/Liquibase) that
- Updates all rows in the schools (or related) table:
UPDATE … SET area = 'NOWON' WHERE area = 'DAEGU'; - (Optional) Protect against future failures by handling unknown enum values in your deserializer.
- Updates all rows in the schools (or related) table:
- Verify there are no stray references to
DAEGUin any file type (YAML, SQL, XML, etc.) beyond Java sources. - Coordinate this change with your release process so production data remains consistent.
🤖 Prompt for AI Agents
In src/main/java/life/mosu/mosuserver/domain/school/Area.java at line 6, the
enum value DAEGU was replaced with NOWON without a corresponding database
migration. To fix this, create a migration script (using Flyway or Liquibase)
that updates all existing records in the relevant table by running an SQL update
statement to replace 'DAEGU' with 'NOWON'. Additionally, review all project
files for any remaining references to 'DAEGU' and remove or update them.
Optionally, enhance the enum deserialization logic to handle unknown values
gracefully to prevent future failures. Coordinate this migration with your
deployment to ensure data consistency in production.
| Optional<SchoolJpaEntity> findBySchoolNameAndAreaAndExamDate(String schoolName, Area area, | ||
| LocalDate examDate); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consider database indexing for the new query method.
The new findBySchoolNameAndAreaAndExamDate method performs a composite query that may have performance implications. Ensure that:
- A composite database index exists on
(school_name, area, exam_date)columns - The query performance is acceptable for the expected data volume
- Consider if this method should have caching if it's called frequently
🏁 Script executed:
#!/bin/bash
# Check for database migration files that might contain index creation
rg -A 10 -B 10 "school_name.*area.*exam_date|exam_date.*area.*school_name" --type sql
# Look for any existing index definitions
rg -i "index.*school" --type sqlLength of output: 420
#!/bin/bash
# Locate the SchoolJpaEntity to inspect JPA @Table index definitions
entity_file=$(find src -type f -name "SchoolJpaEntity.java" | head -n 1)
if [[ -n "$entity_file" ]]; then
echo "Inspecting entity file: $entity_file"
rg -A3 "@Table" -n "$entity_file"
rg -i "@Index" -n "$entity_file"
else
echo "SchoolJpaEntity.java not found in src/"
fi
# List database migration/changelog files
echo
echo "Migration files under src/main/resources/db/:"
find src/main/resources/db/ -type f | grep -E "\.(sql|xml|yml|yaml)$" || echo " (none found)"
# Search migrations for index creation statements
echo
echo "Searching for explicit CREATE INDEX statements in migrations:"
rg -i "create\s\+index" -n src/main/resources/db/ || echo " (no CREATE INDEX found)"🏁 Script executed:
#!/bin/bash
# Locate the JPA entity to check @Table/@Index annotations
entity_file=$(find src -type f -name "SchoolJpaEntity.java" | head -n 1)
if [[ -n "$entity_file" ]]; then
echo ">>> Inspecting $entity_file for table/index annotations"
rg -A3 "@Table" -n "$entity_file" || echo " (no @Table annotation found)"
rg -n "@Index" -n "$entity_file" || echo " (no @Index annotation found)"
else
echo "ERROR: SchoolJpaEntity.java not found under src/"
fi
echo
# List all migration/changelog files in resources
echo ">>> Listing migration files under src/main/resources"
find src/main/resources -type f | grep -E "\.(sql|xml|yml|yaml)$" || echo " (no migration files found)"
echo
# Search migrations for explicit CREATE INDEX statements
echo ">>> Searching migrations for CREATE INDEX"
rg -i "create\s+index" -n src/main/resources || echo " (no CREATE INDEX statements found)"Length of output: 1396
Add composite database index for the new query
The findBySchoolNameAndAreaAndExamDate lookup will trigger a full-table scan unless you introduce an index on (school_name, area, exam_date). I didn’t find any existing JPA @Index on SchoolJpaEntity or a migration that creates this composite index. Please:
- In
src/main/java/life/mosu/mosuserver/domain/school/SchoolJpaEntity.java, update the@Tableannotation:@Table( name = "school", indexes = @Index( name = "idx_school_name_area_exam_date", columnList = "school_name,area,exam_date" ) )
- Or add a database migration (Flyway/Liquibase/SQL) to create:
CREATE INDEX idx_school_name_area_exam_date ON school (school_name, area, exam_date);
- Optionally, if this method is called very frequently, consider adding a second-level cache (e.g., Spring Cache) to reduce database hits.
🤖 Prompt for AI Agents
In src/main/java/life/mosu/mosuserver/domain/school/SchoolJpaEntity.java around
lines 9-10, the new query method findBySchoolNameAndAreaAndExamDate lacks a
composite index on (school_name, area, exam_date), causing full-table scans. Fix
this by updating the @Table annotation on SchoolJpaEntity to include an @Index
with name "idx_school_name_area_exam_date" and columnList
"school_name,area,exam_date". Alternatively, add a database migration script to
create this index. Optionally, consider adding a second-level cache if this
query is very frequent.
|
|
||
|
|
||
| boolean existsByUserIdAndSchoolIdIn(Long userId, Collection<Long> schoolIds); | ||
| boolean existsByUserIdAndSchoolId(Long userId, Long schoolId); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consider performance implications of individual existence checks.
The change from existsByUserIdAndSchoolIdIn to existsByUserIdAndSchoolId means the service layer will now make multiple individual database queries instead of one bulk query. This could impact performance when processing applications with multiple schools.
Consider:
- The performance impact of N individual queries vs 1 bulk query
- Whether the service layer should batch these checks
- Potential race conditions between the existence check and application creation
🏁 Script executed:
#!/bin/bash
# Check how this method is being used in the service layer
rg -A 5 -B 5 "existsByUserIdAndSchoolId" --type javaLength of output: 2979
Batch existence checks and guard against race conditions
Switching from a single bulk query to per-school existsByUserIdAndSchoolId calls in ApplicationService will execute one SELECT per school in the request. This may degrade performance for large batches and still leaves a window for duplicate inserts under concurrency. Please address the following:
• In ApplicationService.java (around the loop invoking existsByUserIdAndSchoolId), replace N individual queries with a single existsByUserIdAndSchoolIdIn(List<Long> schoolIds) call or pre-fetch all existing (userId, schoolId) pairs and check in memory.
• Add a unique constraint on (user_id, school_id) at the database level and catch the resulting exception (e.g. DataIntegrityViolationException) to handle any race-condition insert attempts.
• Ensure the check-then-insert logic runs within a single transaction or adjust isolation levels so that concurrent threads cannot slip past the existence check.
🤖 Prompt for AI Agents
In
src/main/java/life/mosu/mosuserver/domain/application/ApplicationSchoolJpaRepository.java
at line 10, the method existsByUserIdAndSchoolId causes multiple queries for
batch existence checks, which harms performance and allows race conditions.
Replace this with a method like existsByUserIdAndSchoolIdIn(List<Long>
schoolIds) to perform a single bulk query or pre-fetch all existing pairs and
check in memory. Additionally, add a unique constraint on (user_id, school_id)
in the database schema and handle DataIntegrityViolationException during inserts
to manage race conditions. Finally, ensure the check-and-insert logic runs
within a single transaction or adjust isolation levels to prevent concurrent
insert conflicts.
| for (ApplicationSchoolRequest schoolRequest : schoolRequests) { | ||
| Long schoolId = schoolJpaRepository.findBySchoolNameAndAreaAndExamDate( | ||
| schoolRequest.schoolName(), | ||
| schoolRequest.validatedArea(schoolRequest.area()), | ||
| schoolRequest.examDate()) | ||
| .orElseThrow(() -> new CustomRuntimeException(ErrorCode.SCHOOL_NOT_FOUND)) | ||
| .getId(); | ||
|
|
||
| if (applicationSchoolJpaRepository.existsByUserIdAndSchoolId(userId, schoolId)) { | ||
| throw new CustomRuntimeException(ErrorCode.APPLICATION_SCHOOL_ALREADY_APPLIED); | ||
| } | ||
|
|
||
| SchoolJpaEntity school = schoolJpaRepository.findById(schoolId) | ||
| .orElseThrow(() -> new CustomRuntimeException(ErrorCode.SCHOOL_NOT_FOUND)); | ||
|
|
||
| AddressJpaVO address = school.getAddress(); | ||
| ApplicationSchoolJpaEntity applicationSchoolJpaEntity = applicationSchoolRequest.toEntity( | ||
| userId, applicationId, address); | ||
| ApplicationSchoolJpaEntity saved = applicationSchoolJpaRepository.save( | ||
| applicationSchoolJpaEntity); | ||
| schoolEntities.add(saved); | ||
| }); | ||
| ApplicationSchoolJpaEntity applicationSchool = schoolRequest.toEntity(userId, | ||
| applicationId, school); | ||
| savedEntities.add(applicationSchoolJpaRepository.save(applicationSchool)); | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Optimize database calls and improve error handling.
The refactored logic has several issues that impact performance and maintainability:
- Redundant database call: Lines 60-61 fetch the school entity again after already retrieving it in lines 49-54
- Performance concern: The individual processing approach makes multiple database calls per school instead of bulk operations
- Potential transaction rollback: If any school fails validation, the entire application (including previously processed schools) will be rolled back
Apply this optimization to eliminate the redundant database call:
for (ApplicationSchoolRequest schoolRequest : schoolRequests) {
- Long schoolId = schoolJpaRepository.findBySchoolNameAndAreaAndExamDate(
+ SchoolJpaEntity school = schoolJpaRepository.findBySchoolNameAndAreaAndExamDate(
schoolRequest.schoolName(),
schoolRequest.validatedArea(schoolRequest.area()),
schoolRequest.examDate())
.orElseThrow(() -> new CustomRuntimeException(ErrorCode.SCHOOL_NOT_FOUND))
- .getId();
+ ;
- if (applicationSchoolJpaRepository.existsByUserIdAndSchoolId(userId, schoolId)) {
+ if (applicationSchoolJpaRepository.existsByUserIdAndSchoolId(userId, school.getId())) {
throw new CustomRuntimeException(ErrorCode.APPLICATION_SCHOOL_ALREADY_APPLIED);
}
- SchoolJpaEntity school = schoolJpaRepository.findById(schoolId)
- .orElseThrow(() -> new CustomRuntimeException(ErrorCode.SCHOOL_NOT_FOUND));
ApplicationSchoolJpaEntity applicationSchool = schoolRequest.toEntity(userId,
applicationId, school);
savedEntities.add(applicationSchoolJpaRepository.save(applicationSchool));
}Run this script to verify the school lookup method exists and returns the expected type:
🏁 Script executed:
#!/bin/bash
# Description: Verify the school repository method signature and return type
# Search for the findBySchoolNameAndAreaAndExamDate method definition
ast-grep --pattern 'findBySchoolNameAndAreaAndExamDate($$$) {
$$$
}'
# Also search for the method signature in interface definitions
rg -A 3 "findBySchoolNameAndAreaAndExamDate"Length of output: 1077
Optimize ApplicationService school lookup and eliminate redundant call
Verified that findBySchoolNameAndAreaAndExamDate(...) already returns a SchoolJpaEntity, so you can reuse it instead of querying by ID twice.
• File: src/main/java/life/mosu/mosuserver/application/application/ApplicationService.java
- Remove the extra
findById(schoolId)lookup - Use the initially retrieved
SchoolJpaEntityfor both the existence check and entity creation
for (ApplicationSchoolRequest schoolRequest : schoolRequests) {
- Long schoolId = schoolJpaRepository.findBySchoolNameAndAreaAndExamDate(
+ SchoolJpaEntity school = schoolJpaRepository.findBySchoolNameAndAreaAndExamDate(
schoolRequest.schoolName(),
schoolRequest.validatedArea(schoolRequest.area()),
schoolRequest.examDate())
.orElseThrow(() -> new CustomRuntimeException(ErrorCode.SCHOOL_NOT_FOUND))
- .getId();
+
if (applicationSchoolJpaRepository.existsByUserIdAndSchoolId(userId, school.getId())) {
throw new CustomRuntimeException(ErrorCode.APPLICATION_SCHOOL_ALREADY_APPLIED);
}
- SchoolJpaEntity school = schoolJpaRepository.findById(schoolId)
- .orElseThrow(() -> new CustomRuntimeException(ErrorCode.SCHOOL_NOT_FOUND));
ApplicationSchoolJpaEntity applicationSchool = schoolRequest.toEntity(userId,
applicationId, school);
savedEntities.add(applicationSchoolJpaRepository.save(applicationSchool));
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
src/main/java/life/mosu/mosuserver/application/application/ApplicationService.java
around lines 48 to 66, the code redundantly queries the school entity twice:
once with findBySchoolNameAndAreaAndExamDate and again with findById. To fix
this, store the SchoolJpaEntity returned by findBySchoolNameAndAreaAndExamDate
and reuse it for the existence check and entity creation, removing the second
findById call entirely.
1 participant
✨ 구현한 기능
📢 논의하고 싶은 내용
🎸 기타
Summary by CodeRabbit
New Features
Bug Fixes
Refactor