MOSU-103 refactor: ApplicationService 리팩토링 #117
Conversation
WalkthroughThis change refactors the application domain from a school-centric model to an exam-centric one. It removes all school-related services, repositories, entities, and DTOs, introducing new exam, exam application, and lunch domain structures. Controllers, services, and error handling were updated to support the new exam-based logic, with batch operations and detailed error codes added. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ApplicationController
participant ApplicationService
participant ExamApplicationService
participant ExamJpaRepository
participant ExamApplicationJpaRepository
participant ExamSubjectJpaRepository
participant LunchJpaRepository
User->>ApplicationController: Submit ApplicationRequest
ApplicationController->>ApplicationService: apply(userId, request)
ApplicationService->>ExamJpaRepository: Validate exam IDs (duplicates)
ApplicationService->>ExamApplicationService: Register exam applications
ExamApplicationService->>ExamApplicationJpaRepository: Save exam applications
ExamApplicationService->>ExamSubjectJpaRepository: Save exam subjects
ApplicationService->>LunchJpaRepository: (if needed) Save lunch info
ApplicationService-->>ApplicationController: Return ApplicationResponse
ApplicationController-->>User: ApplicationResponse
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
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. 🪧 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 significantly refactors the ApplicationService to improve the efficiency and robustness of the application submission process. The core changes involve introducing bulk data insertion capabilities for application schools and their subjects, enhancing validation logic to prevent duplicate applications, and streamlining the process of verifying school existence based on application criteria.
Highlights
- Bulk Data Insertion: Implemented a new
BulkRepositoryto perform efficient multi-inserts forApplicationSchoolJpaEntityand associatedExamSubjectentities, replacing individual save operations. This significantly improves performance for applications involving multiple schools. - Enhanced Duplicate Application Prevention: Added comprehensive validation checks within the
applymethod to prevent users from submitting duplicate applications for the same school on the same exam date and to catch identical school requests within a single application payload. - Streamlined School Validation: Refactored the school lookup process to fetch all requested school entities in a single query using
SchoolQueryRepositoryImpl, improving performance and ensuring all specified schools exist before proceeding with the application. ApplicationServiceRefactoring: Theapplymethod inApplicationServicehas been largely rewritten to leverage the new bulk operations and validation logic, resulting in a more concise, efficient, and robust application submission flow.
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. ↩
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/main/java/life/mosu/mosuserver/infra/respository/BulkRepository.java (1)
58-59: Ensure proper enum handling for database storageUsing
String.valueOf()for enum conversion might not be the most robust approach. Consider using the enum's name() method or a dedicated converter.- ps.setString(12, String.valueOf(e.getArea())); - ps.setString(13, String.valueOf(e.getLunch())); + ps.setString(12, e.getArea().name()); + ps.setString(13, e.getLunch().name());
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/main/java/life/mosu/mosuserver/application/application/ApplicationService.java(2 hunks)src/main/java/life/mosu/mosuserver/domain/applicationschool/ApplicationSchoolJpaRepository.java(1 hunks)src/main/java/life/mosu/mosuserver/domain/school/SchoolQueryRepository.java(1 hunks)src/main/java/life/mosu/mosuserver/domain/school/SchoolQueryRepositoryImpl.java(2 hunks)src/main/java/life/mosu/mosuserver/global/exception/ErrorCode.java(2 hunks)src/main/java/life/mosu/mosuserver/infra/respository/BulkRepository.java(2 hunks)src/main/java/life/mosu/mosuserver/presentation/application/dto/ApplicationRequest.java(2 hunks)
🔇 Additional comments (11)
src/main/java/life/mosu/mosuserver/domain/school/SchoolQueryRepository.java (1)
9-11: LGTM! Returning full entities supports bulk processingThe change to return
SchoolJpaEntityobjects instead of just IDs is appropriate for the bulk processing approach, allowing the service layer to access full school data without additional queries.src/main/java/life/mosu/mosuserver/domain/school/SchoolQueryRepositoryImpl.java (1)
18-34: LGTM! Implementation correctly updated to return full entitiesThe implementation properly aligns with the interface change. The use of
selectFromto fetch complete entities is appropriate for the bulk processing requirements.src/main/java/life/mosu/mosuserver/domain/applicationschool/ApplicationSchoolJpaRepository.java (1)
41-49: LGTM! Well-designed bulk query methodsThe new methods effectively support bulk operations:
existsByUserIdAndSchoolIdInenables efficient duplicate checking across multiple schoolsfindAllWithSubjectsByIdInproperly uses LEFT JOIN FETCH to avoid N+1 queries when loading subjectssrc/main/java/life/mosu/mosuserver/global/exception/ErrorCode.java (2)
46-46: LGTM! Error code properly supports duplicate validation.The new
APPLICATION_SCHOOL_DUPLICATEDerror code appropriately usesBAD_REQUESTstatus for client-side validation failures when duplicate schools are detected within a single request.
94-95: LGTM! Multi-insert error codes properly categorized.The new error codes for bulk insert operations are correctly categorized as
INTERNAL_SERVER_ERRORsince they represent server-side failures during database operations.src/main/java/life/mosu/mosuserver/application/application/ApplicationService.java (6)
3-9: LGTM! Import additions support the bulk operations refactoring.The new imports are appropriately added to support the bulk processing logic:
Triplefor efficient condition groupingMapandFunctionfor school entity mappingHashSetfor duplicate detectionLocalDatefor date handlingBulkRepositoryfor batch operationsAlso applies to: 16-16, 22-22, 28-28, 42-42
51-60: LGTM! Admission ticket logic simplified and more robust.The refactored admission ticket logic is cleaner and more defensive by checking both filename and S3 key existence before saving. The direct conversion from
FileRequestto entity is more straightforward than the previous helper method approach.
62-74: LGTM! Effective duplicate detection within request.The duplicate detection logic is well-implemented:
- Uses
Tripleto create unique condition keys combining school name, area, and exam date- Converts to
HashSetto detect duplicates by size comparison- Throws appropriate error code for duplicate validation
76-85: LGTM! Bulk school existence validation is efficient.The school existence validation logic is well-designed:
- Uses bulk query to fetch all schools at once instead of individual queries
- Validates that all requested schools exist by count comparison
- Efficiently checks for prior applications using bulk existence check
87-104: LGTM! Efficient mapping strategy for bulk operations.The school mapping logic is well-implemented:
- Creates a map using the same Triple key structure for O(1) lookups
- Efficiently converts requests to entities using the pre-fetched school data
- Maintains consistency between condition checking and entity creation
106-112: Transactional handling in bulk operations has been validated
BulkRepositoryis annotated with@Transactional(propagation=REQUIRED by default), so it joins the service’s transactionApplicationService.applyis itself@Transactional, ensuring the entire save-and-fetch sequence is atomic- On any exception, the Spring transaction manager will roll back both the JDBC batch and subsequent JPA fetch
No changes are required here.
| import io.swagger.v3.oas.annotations.media.Schema; | ||
| import jakarta.validation.constraints.NotNull; | ||
| import java.util.Set; | ||
| import java.util.List; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify duplicate school handling after changing from Set to List
The change from Set to List allows duplicate school entries in the request. Ensure that duplicate school validation is properly handled in the service layer to prevent users from applying to the same school multiple times within a single request.
Also applies to: 22-22
🏁 Script executed:
#!/bin/bash
# Description: Check if duplicate school validation is implemented in the service layer
# Search for duplicate validation logic in ApplicationService
rg -A 10 "ApplicationService" | rg -A 10 "duplicate|distinct|unique"
# Look for the ApplicationService implementation
ast-grep --pattern 'class ApplicationService {
$$$
}'Length of output: 126
Here’s a deeper inspection to locate the service class and any duplicate-school handling logic:
This will confirm whether there’s an ApplicationService (or similarly named) class and if any de-duplication checks exist.
🏁 Script executed:
#!/bin/bash
# Attempt 2: locate and inspect ApplicationService and duplicate logic
echo "1) Finding ApplicationService.java file…"
fd ApplicationService.java
echo "2) Looking for 'class ApplicationService' declarations…"
rg -n "class .*ApplicationService" -C2 src/main/java
echo "3) Listing all *Service.java files…"
fd -e java | grep -i "Service.java"
echo "4) Searching for 'duplicate' and 'distinct' keywords in service layer…"
rg -n "duplicate" -C5 src/main/java
rg -n "distinct" -C5 src/main/java
echo "5) Searching for 'schools' references in service layer…"
rg -n "schools" -C5 src/main/javaLength of output: 23682
Add explicit duplicate‐school validation in ApplicationService.apply
The service currently collects request.schools() into a map—relying on Collectors.toMap to throw on duplicate keys—but offers no clear error or documentation. Introduce a preflight check to reject duplicate entries with a proper BadRequestException.
Locations to update:
- src/main/java/life/mosu/mosuserver/application/application/ApplicationService.java, inside the
apply(Long userId, ApplicationRequest request)method (around line 46–50)
Suggested snippet:
List<ApplicationSchoolRequest> schoolRequests = request.schools();
// reject duplicates early with a clear exception
if (schoolRequests.stream().map(ApplicationSchoolRequest::schoolName).distinct().count()
< schoolRequests.size()) {
throw new BadRequestException("Duplicate school entries are not allowed in a single application.");
}
// existing save and mapping logic…🤖 Prompt for AI Agents
In
src/main/java/life/mosu/mosuserver/application/application/ApplicationService.java
around lines 46 to 50, add an explicit preflight check in the apply(Long userId,
ApplicationRequest request) method to detect duplicate school entries before
processing. Retrieve the list of schools from the request, then compare the
count of distinct school names to the total list size. If duplicates exist,
throw a BadRequestException with a clear message indicating that duplicate
school entries are not allowed. This replaces relying on Collectors.toMap to
throw and provides clearer error handling.
src/main/java/life/mosu/mosuserver/infra/respository/BulkRepository.java
Outdated
Show resolved
Hide resolved
| public List<Long> saveAllApplicationSchoolsWithSubjects( | ||
| List<ApplicationSchoolJpaEntity> entities) { | ||
|
|
||
| List<Long> generatedIds = jdbcTemplate.execute((ConnectionCallback<List<Long>>) con -> { | ||
| try (PreparedStatement ps = con.prepareStatement( | ||
| SQL_INSERT_APPLICATION_SCHOOL, Statement.RETURN_GENERATED_KEYS)) { | ||
| for (ApplicationSchoolJpaEntity e : entities) { | ||
| ps.setDate(1, Date.valueOf(e.getExamDate())); | ||
| ps.setLong(2, e.getApplicationId()); | ||
| ps.setTimestamp(3, Timestamp.valueOf(LocalDateTime.now())); | ||
| ps.setLong(4, e.getSchoolId()); | ||
| ps.setTimestamp(5, Timestamp.valueOf(LocalDateTime.now())); | ||
| ps.setLong(6, e.getUserId()); | ||
| ps.setString(7, e.getSchoolName()); | ||
| ps.setString(8, e.getAddress().getDetail()); | ||
| ps.setString(9, e.getExaminationNumber()); | ||
| ps.setString(10, e.getAddress().getStreet()); | ||
| ps.setString(11, e.getAddress().getZipcode()); | ||
| ps.setString(12, String.valueOf(e.getArea())); | ||
| ps.setString(13, String.valueOf(e.getLunch())); | ||
| ps.addBatch(); | ||
| } | ||
|
|
||
| ps.executeBatch(); | ||
|
|
||
| List<Long> ids = new ArrayList<>(); | ||
| try (ResultSet rs = ps.getGeneratedKeys()) { | ||
| while (rs.next()) { | ||
| ids.add(rs.getLong(1)); | ||
| } | ||
| } | ||
| return ids; | ||
|
|
||
| } catch (RuntimeException e) { | ||
| throw new CustomRuntimeException(ErrorCode.APPLICATION_SCHOOL_MULTI_INSERT_ERROR); | ||
| } | ||
| }); | ||
|
|
||
| List<Object[]> subjectParams = new ArrayList<>(); | ||
|
|
||
| for (int i = 0; i < entities.size(); i++) { | ||
| Long applicationSchoolId = generatedIds.get(i); | ||
|
|
||
| for (Subject subj : entities.get(i).getSubjects()) { | ||
| subjectParams.add(new Object[]{applicationSchoolId, String.valueOf(subj)}); | ||
| } | ||
| } | ||
|
|
||
| try { | ||
| if (!subjectParams.isEmpty()) { | ||
| jdbcTemplate.batchUpdate(SQL_INSERT_SUBJECT, subjectParams); | ||
| } | ||
| } catch (RuntimeException e) { | ||
| throw new CustomRuntimeException(ErrorCode.EXAM_SUBJECT_MULTI_INSERT_ERROR); | ||
| } | ||
|
|
||
| return generatedIds; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding batch size limits for large datasets
The current implementation processes all entities in a single batch without size limits. For large datasets, this could cause memory issues or exceed database limits.
Consider processing in chunks:
private static final int BATCH_SIZE = 1000;
public List<Long> saveAllApplicationSchoolsWithSubjects(List<ApplicationSchoolJpaEntity> entities) {
List<Long> allGeneratedIds = new ArrayList<>();
for (int i = 0; i < entities.size(); i += BATCH_SIZE) {
List<ApplicationSchoolJpaEntity> batch = entities.subList(i,
Math.min(i + BATCH_SIZE, entities.size()));
List<Long> batchIds = processBatch(batch);
allGeneratedIds.addAll(batchIds);
}
return allGeneratedIds;
}🤖 Prompt for AI Agents
In src/main/java/life/mosu/mosuserver/infra/respository/BulkRepository.java
around lines 40 to 97, the current batch insert processes all entities at once,
which can cause memory or database issues with large datasets. Refactor the
method to split the input list into smaller batches (e.g., 1000 items each) and
process each batch separately, collecting and returning all generated IDs after
all batches are processed. Implement a helper method to handle each batch's
insert logic and call it iteratively for each chunk.
There was a problem hiding this comment.
Code Review
The code changes refactor the application submission logic for better performance using bulk database operations. The changes replace N+1 query patterns with batch processing. The review includes suggestions for the new BulkRepository to improve exception handling and data consistency.
| List<Long> generatedIds = jdbcTemplate.execute((ConnectionCallback<List<Long>>) con -> { | ||
| try (PreparedStatement ps = con.prepareStatement( | ||
| SQL_INSERT_APPLICATION_SCHOOL, Statement.RETURN_GENERATED_KEYS)) { | ||
| for (ApplicationSchoolJpaEntity e : entities) { | ||
| ps.setDate(1, Date.valueOf(e.getExamDate())); | ||
| ps.setLong(2, e.getApplicationId()); | ||
| ps.setTimestamp(3, Timestamp.valueOf(LocalDateTime.now())); | ||
| ps.setLong(4, e.getSchoolId()); | ||
| ps.setTimestamp(5, Timestamp.valueOf(LocalDateTime.now())); | ||
| ps.setLong(6, e.getUserId()); | ||
| ps.setString(7, e.getSchoolName()); | ||
| ps.setString(8, e.getAddress().getDetail()); | ||
| ps.setString(9, e.getExaminationNumber()); | ||
| ps.setString(10, e.getAddress().getStreet()); | ||
| ps.setString(11, e.getAddress().getZipcode()); | ||
| ps.setString(12, String.valueOf(e.getArea())); | ||
| ps.setString(13, String.valueOf(e.getLunch())); | ||
| ps.addBatch(); | ||
| } | ||
|
|
||
| ps.executeBatch(); | ||
|
|
||
| List<Long> ids = new ArrayList<>(); | ||
| try (ResultSet rs = ps.getGeneratedKeys()) { | ||
| while (rs.next()) { | ||
| ids.add(rs.getLong(1)); | ||
| } | ||
| } | ||
| return ids; | ||
|
|
||
| } catch (RuntimeException e) { | ||
| throw new CustomRuntimeException(ErrorCode.APPLICATION_SCHOOL_MULTI_INSERT_ERROR); | ||
| } | ||
| }); |
There was a problem hiding this comment.
The try-catch block catches RuntimeException, but methods on PreparedStatement (like executeBatch()) throw SQLException. Catch SQLException to handle database-related errors during the batch insert correctly. Also, use a single timestamp for all records created in the same batch for data consistency.
List<Long> generatedIds = jdbcTemplate.execute((ConnectionCallback<List<Long>>) con -> {
final Timestamp now = Timestamp.valueOf(LocalDateTime.now());
try (PreparedStatement ps = con.prepareStatement(
SQL_INSERT_APPLICATION_SCHOOL, Statement.RETURN_GENERATED_KEYS)) {
for (ApplicationSchoolJpaEntity e : entities) {
ps.setDate(1, Date.valueOf(e.getExamDate()));
ps.setLong(2, e.getApplicationId());
ps.setTimestamp(3, now);
ps.setLong(4, e.getSchoolId());
ps.setTimestamp(5, now);
ps.setLong(6, e.getUserId());
ps.setString(7, e.getSchoolName());
ps.setString(8, e.getAddress().getDetail());
ps.setString(9, e.getExaminationNumber());
ps.setString(10, e.getAddress().getStreet());
ps.setString(11, e.getAddress().getZipcode());
ps.setString(12, String.valueOf(e.getArea()));
ps.setString(13, String.valueOf(e.getLunch()));
ps.addBatch();
}
ps.executeBatch();
List<Long> ids = new ArrayList<>();
try (ResultSet rs = ps.getGeneratedKeys()) {
while (rs.next()) {
ids.add(rs.getLong(1));
}
}
return ids;
} catch (java.sql.SQLException e) {
throw new CustomRuntimeException(ErrorCode.APPLICATION_SCHOOL_MULTI_INSERT_ERROR);
}
});| } catch (RuntimeException e) { | ||
| throw new CustomRuntimeException(ErrorCode.EXAM_SUBJECT_MULTI_INSERT_ERROR); | ||
| } |
There was a problem hiding this comment.
Catch the more specific DataAccessException instead of RuntimeException for clarity.
| } catch (RuntimeException e) { | |
| throw new CustomRuntimeException(ErrorCode.EXAM_SUBJECT_MULTI_INSERT_ERROR); | |
| } | |
| } catch (org.springframework.dao.DataAccessException e) { | |
| throw new CustomRuntimeException(ErrorCode.EXAM_SUBJECT_MULTI_INSERT_ERROR); | |
| } |
chominju02
changed the title
…dmission ticket images and application schools - Deleted `AdmissionTicketImageJpaEntity` and its repository as they are no longer needed. - Removed `ApplicationSchoolJpaEntity`, `ApplicationSchoolJpaRepository`, and related projections and DTOs. - Eliminated `AddressJpaVO`, `Area`, `LunchMenu`, and `SchoolJpaEntity` along with their repositories and controllers. - Cleaned up associated request and response DTOs for application school and school management. - This cleanup reduces code complexity and improves maintainability by removing obsolete components.
…reation in FaqAttachmentService
…reation in InquiryAnswerAttachmentService
…reation in InquiryAttachmentService
…reation in NoticeAttachmentService
…nd improving readability
…commented-out from method in ApplicationNotifyRequest
…ify application existence check in PaymentService
…ositoryImpl to improve code clarity
…ryImpl to enhance code clarity
…icketResponse, ExamApplicationRequest, and ExamResponse
…ionJpaEntity and update related DTOs and methods
…DTOs for lunch management
…main and remove unused AreaDetail and AvailableSchoolResponse classes
… management, including ExamTicketImage, Exam, ExamApplication, and ExamSubject
…ent, including CRUD operations and exam association
… to clean up the codebase
There was a problem hiding this comment.
Actionable comments posted: 19
🔭 Outside diff range comments (7)
src/main/java/life/mosu/mosuserver/application/payment/PaymentService.java (3)
86-94: Critical: Application existence validation is completely disabledThe hardcoded
truevalue bypasses all application existence validation, creating a significant data integrity and security risk. Payments could be processed for non-existent applications.Either implement proper validation using the new exam application repositories or remove this validation entirely if it's no longer needed in the new domain model.
private void checkApplicationsExist(List<Long> applicationIds) { -// boolean existsAll = applicationSchoolJpaRepository.existsAllByIds(applicationIds, -// applicationIds.size()); - boolean existsAll = true; + // TODO: Implement validation using ExamApplicationJpaRepository + // boolean existsAll = examApplicationRepository.existsAllByIds(applicationIds, applicationIds.size()); + boolean existsAll = true; // TEMPORARY - REMOVE BEFORE PRODUCTION if (!existsAll) { log.warn("Application IDs not found: {}", applicationIds); throw new RuntimeException("존재하지 않는 신청입니다."); } }
53-53: Update variable naming to reflect new domain modelThe variable
applicationSchoolIdsshould be renamed to reflect the new exam-based domain model, such asexamApplicationIdsorapplicationIds.
86-94: Critical: Application existence validation is completely bypassed.The hardcoded
truevalue disables all application existence validation, allowing payments to be processed for non-existent applications. This creates a serious data integrity and security vulnerability.The validation should be properly refactored to use the new exam-based repositories instead of being disabled:
private void checkApplicationsExist(List<Long> applicationIds) { -// boolean existsAll = applicationSchoolJpaRepository.existsAllByIds(applicationIds, -// applicationIds.size()); - boolean existsAll = true; + // TODO: Implement with new exam application repository + boolean existsAll = examApplicationRepository.existsAllByIds(applicationIds, applicationIds.size()); if (!existsAll) { log.warn("Application IDs not found: {}", applicationIds); throw new RuntimeException("존재하지 않는 신청입니다."); } }src/main/java/life/mosu/mosuserver/global/initializer/DatabaseInitializer.java (1)
1-381: Remove the entire commented file instead of keeping it.The entire
DatabaseInitializerclass is commented out and relates to the deprecated school-centric domain model. Keeping large blocks of commented code creates maintenance burden and confusion.Consider either:
- Delete this file entirely if database initialization is no longer needed
- Create a new initializer for the exam-centric domain model if seeding is still required
Leaving 380+ lines of commented code is not a good practice and should be avoided.
src/main/java/life/mosu/mosuserver/infra/storage/application/S3Service.java (1)
42-43: Remove unusedpreSignedUrlExpirationMinutesfrom S3ServiceThe
@Value("${aws.s3.presigned-url-expiration-minutes}") private int preSignedUrlExpirationMinutes;field inS3Serviceis never referenced—expiration is already provided bys3Properties.getPresignedUrlExpirationMinutes(). To eliminate duplication, remove these two lines:File: src/main/java/life/mosu/mosuserver/infra/storage/application/S3Service.java
- @Value("${aws.s3.presigned-url-expiration-minutes}") - private int preSignedUrlExpirationMinutes;src/main/java/life/mosu/mosuserver/application/notify/NotifyVariableFactory.java (1)
22-34: Critical: Potential NullPointerException from null returns.The
createmethod's switch statement still routes to methods that now returnnull. This will cause NPE when the returnedNotificationVariableis used.Consider either:
- Throwing an exception to indicate these notification types are temporarily unsupported
- Implementing the new exam-based notification logic
private NotificationVariable createRefundVariable(Long targetId) { - return null; + throw new UnsupportedOperationException("Refund notifications are temporarily disabled during migration"); }src/main/java/life/mosu/mosuserver/presentation/application/dto/ApplicationSchoolRequest.java (1)
1-48: Remove this commented-out file.Since the entire file is commented out as part of the school-to-exam model migration, it should be deleted rather than kept as dead code. This improves codebase cleanliness and reduces confusion.
#!/bin/bash # Description: Check if this file is still referenced anywhere in the codebase # Search for any references to ApplicationSchoolRequest rg "ApplicationSchoolRequest" --type java
🧹 Nitpick comments (17)
src/main/java/life/mosu/mosuserver/application/lunch/LunchEventService.java (1)
18-22: Consider adding validation for the lunch creation event.While the current implementation is clean, consider adding validation to ensure the
CreateLunchEventcontains valid data before converting to entities.@Transactional public void create(CreateLunchEvent request) { + if (request == null || request.toEntities().isEmpty()) { + throw new IllegalArgumentException("CreateLunchEvent cannot be null or empty"); + } List<LunchJpaEntity> lunch = request.toEntities(); lunchJpaRepository.saveAll(lunch); }src/main/java/life/mosu/mosuserver/domain/application/ApplicationJpaRepository.java (1)
12-18: JPQL query implementation looks correct for duplicate checking.The query properly joins the entities and uses appropriate parameter binding. The CASE WHEN pattern for boolean return is standard JPQL practice.
Consider adding an index on the join columns if this query will be frequently executed with large exam ID lists for better performance.
src/main/java/life/mosu/mosuserver/domain/examapplication/ExamNumberGenerationService.java (1)
7-7: Consider using @service instead of @component.Since this class provides business logic for exam number generation,
@Servicewould be more semantically appropriate than@Component.-@Component +@Servicesrc/main/java/life/mosu/mosuserver/application/lunch/LunchService.java (2)
16-19: Consider adding @transactional(readOnly = true) for read operations.Adding read-only transaction annotation can provide performance benefits and clearer intent.
+@Transactional(readOnly = true) public List<LunchResponse> getByExamId(Long examId) { List<LunchJpaEntity> foundLunch = lunchJpaRepository.findByExamId(examId); return LunchResponse.fromList(foundLunch); }
17-17: Variable naming could be more precise.The variable name
foundLunchsuggests a single entity but contains a list. ConsiderfoundLunchesorlunchEntitiesfor clarity.-List<LunchJpaEntity> foundLunch = lunchJpaRepository.findByExamId(examId); -return LunchResponse.fromList(foundLunch); +List<LunchJpaEntity> lunchEntities = lunchJpaRepository.findByExamId(examId); +return LunchResponse.fromList(lunchEntities);src/main/java/life/mosu/mosuserver/application/examapplication/ExamApplicationService.java (1)
20-23: Consider adding error handling for batch operations.While the current implementation is correct, consider adding validation or error handling for edge cases:
- Empty exam application lists
- Failures during exam number generation
- Database constraint violations
@Transactional public List<ExamApplicationJpaEntity> register(RegisterExamApplicationEvent event) { + if (event.targetExams().isEmpty()) { + return List.of(); + } + List<ExamApplicationJpaEntity> examApplicationEntities = event.toEntity(); examNumberGenerationService.grantTo(examApplicationEntities); return examApplicationJpaRepository.saveAll(examApplicationEntities); }src/main/java/life/mosu/mosuserver/domain/examapplication/ExamApplicationJpaEntity.java (1)
18-54: Well-structured entity with some considerations for improvement.The entity follows good JPA practices with proper annotations and builder pattern. However, consider these improvements:
- Input validation: Add validation for non-null required fields in the builder constructor
- Exam number assignment: Consider adding validation in
grantExamNumberto prevent overwriting existing exam numbers- Static factory method: The
createmethod duplicates the builder pattern functionality - consider removing it for simplicity@Builder public ExamApplicationJpaEntity(Long applicationId, Long examId, Long lunchId) { + if (applicationId == null || examId == null) { + throw new IllegalArgumentException("ApplicationId and ExamId cannot be null"); + } this.applicationId = applicationId; this.examId = examId; this.lunchId = lunchId; } public void grantExamNumber(String examNumber) { + if (this.examNumber != null) { + throw new IllegalStateException("Exam number already assigned"); + } this.examNumber = examNumber; }src/main/java/life/mosu/mosuserver/presentation/lunch/LunchController.java (1)
29-31: TODO comment indicates incomplete implementation.The TODO comment suggests that CRUD operations for lunch management are planned but not yet implemented.
Would you like me to help generate the implementation for lunch CRUD operations or create an issue to track this task?
src/main/java/life/mosu/mosuserver/presentation/exam/dto/ExamRequest.java (1)
12-20: Consider adding validation annotations.The record fields lack validation annotations which could help catch invalid data early in the request processing pipeline.
public record ExamRequest( + @NotBlank String schoolName, + @NotBlank String areaName, + @NotNull @Valid AddressRequest address, + @NotNull @Future LocalDate examDate, + @NotNull @Min(1) Integer capacity, + @NotNull @Future LocalDateTime deadlineTime, + @NotNull @Valid List<ExamLunchRequest> lunch ) {src/main/java/life/mosu/mosuserver/presentation/lunch/dto/CreateLunchEvent.java (1)
15-19: Consider adding null safety checks.While the current implementation is clean, adding null checks could improve robustness, especially for the stream operations.
public List<LunchJpaEntity> toEntities() { + if (lunchRequests == null) { + return List.of(); + } return lunchRequests.stream() .map(request -> request.toEntity(examId)) .toList(); }src/main/java/life/mosu/mosuserver/application/notify/dto/ApplicationNotifyRequest.java (1)
19-26: Remove commented code instead of leaving it in the codebase.The commented factory method creates code clutter and should be completely removed since the school-centric model is being eliminated. Leaving commented code can cause confusion for future developers.
-// public static ApplicationNotifyRequest from( -// ApplicationSchoolNotifyProjection applicationSchool) { - - /// / return new ApplicationNotifyRequest( / - /// applicationSchool.paymentKey(), / applicationSchool.examDate(), / - /// applicationSchool.schoolName(), / applicationSchool.lunch() != null ? - /// applicationSchool.lunch().getLunchName() : "" / ); -// }src/main/java/life/mosu/mosuserver/application/notify/NotifyVariableFactory.java (1)
47-89: Track the migration of notification logic.These methods are commented out as part of the school-to-exam model migration. The implementations need to be updated to use the new exam-based entities.
Would you like me to create an issue to track the implementation of exam-based notification logic for these notification types?
src/main/java/life/mosu/mosuserver/application/exam/ExamService.java (1)
16-41: Consider adding service-level documentation.As this is a new core service in the exam-centric model, adding Javadoc would help other developers understand its purpose and usage.
+/** + * Service for managing exam-related operations. + * Handles exam registration, retrieval, and triggers lunch creation events. + */ @Service @RequiredArgsConstructor public class ExamService {src/main/java/life/mosu/mosuserver/application/application/ApplicationService.java (2)
48-56: Optimize duplicate check for better performance.The current implementation fetches all exam IDs upfront and then checks for duplicates. Consider using a more efficient approach that stops at the first duplicate found.
-// 중복 신청 검증 -List<Long> examIds = request.examApplication().stream() - .map(examApp -> examApp.examId()) - .toList(); - -boolean isDuplicate = applicationJpaRepository.existsByUserIdAndExamIds(userId, examIds); -if (isDuplicate) { - throw new CustomRuntimeException(ErrorCode.APPLICATION_SCHOOL_DUPLICATED); -} +// 중복 신청 검증 +Set<Long> examIds = request.examApplication().stream() + .map(examApp -> examApp.examId()) + .collect(Collectors.toSet()); + +boolean isDuplicate = applicationJpaRepository.existsByUserIdAndExamIds(userId, examIds); +if (isDuplicate) { + throw new CustomRuntimeException(ErrorCode.APPLICATION_SCHOOL_DUPLICATED); +}Also consider if the repository method can be optimized to use
EXISTSSQL clause for better performance.
58-86: Consider transaction boundary optimization.The current implementation performs multiple repository saves sequentially, which could benefit from better transaction management or batch operations for improved performance.
Consider grouping related operations or using the commented-out
ExamApplicationBulkRepositoryfor batch inserts:// Alternative approach using batch operations @Transactional public ApplicationResponse apply(Long userId, ApplicationRequest request) { // ... duplicate check logic ... ApplicationJpaEntity application = request.toApplicationJpaEntity(userId); ApplicationJpaEntity savedApplication = applicationJpaRepository.save(application); Long applicationId = savedApplication.getId(); // Use batch repository if available List<ExamApplicationJpaEntity> examApplicationEntities = examApplicationService.register( RegisterExamApplicationEvent.of(request.examApplication(), applicationId) ); // Consider batching these operations List<ExamApplicationJpaEntity> savedExamApplications = examApplicationJpaRepository.saveAll(examApplicationEntities); List<ExamSubjectJpaEntity> allExamSubjects = createExamSubjects(savedExamApplications, request.validatedSubjects()); examSubjectJpaRepository.saveAll(allExamSubjects); // ... rest of the method }src/main/java/life/mosu/mosuserver/global/exception/ErrorCode.java (1)
46-46: Update error message to reflect exam-centric model.The error message mentions "같은 학교" (same school) but this error code is now used for exam duplicate validation. Consider updating the message to be more accurate.
-APPLICATION_SCHOOL_DUPLICATED(HttpStatus.BAD_REQUEST, "동일 일자의 같은 학교를 신청할 수 없습니다."), +APPLICATION_SCHOOL_DUPLICATED(HttpStatus.BAD_REQUEST, "동일한 시험을 중복으로 신청할 수 없습니다."),Or consider renaming the error code itself to better reflect the exam-centric model:
-APPLICATION_SCHOOL_DUPLICATED(HttpStatus.BAD_REQUEST, "동일 일자의 같은 학교를 신청할 수 없습니다."), +EXAM_APPLICATION_DUPLICATED(HttpStatus.BAD_REQUEST, "동일한 시험을 중복으로 신청할 수 없습니다."),src/main/java/life/mosu/mosuserver/application/payment/PaymentService.java (1)
32-32: Remove commented-out code.Commented-out code should be removed rather than left in the codebase. Use version control to track removed dependencies.
- // private final ApplicationSchoolJpaRepository applicationSchoolJpaRepository;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (90)
src/main/java/life/mosu/mosuserver/application/admin/AdminService.java(1 hunks)src/main/java/life/mosu/mosuserver/application/application/ApplicationService.java(2 hunks)src/main/java/life/mosu/mosuserver/application/application/SchoolQuotaCacheManager.java(1 hunks)src/main/java/life/mosu/mosuserver/application/applicationschool/ApplicationSchoolService.java(0 hunks)src/main/java/life/mosu/mosuserver/application/event/EventAttachmentService.java(1 hunks)src/main/java/life/mosu/mosuserver/application/exam/ExamService.java(1 hunks)src/main/java/life/mosu/mosuserver/application/examapplication/ExamApplicationService.java(1 hunks)src/main/java/life/mosu/mosuserver/application/examapplication/dto/RegisterExamApplicationEvent.java(1 hunks)src/main/java/life/mosu/mosuserver/application/faq/FaqAttachmentService.java(2 hunks)src/main/java/life/mosu/mosuserver/application/inquiry/InquiryAnswerAttachmentService.java(2 hunks)src/main/java/life/mosu/mosuserver/application/inquiry/InquiryAttachmentService.java(2 hunks)src/main/java/life/mosu/mosuserver/application/lunch/LunchEventService.java(1 hunks)src/main/java/life/mosu/mosuserver/application/lunch/LunchService.java(1 hunks)src/main/java/life/mosu/mosuserver/application/notice/NoticeAttachmentService.java(2 hunks)src/main/java/life/mosu/mosuserver/application/notify/NotifyVariableFactory.java(1 hunks)src/main/java/life/mosu/mosuserver/application/notify/dto/ApplicationNotifyRequest.java(1 hunks)src/main/java/life/mosu/mosuserver/application/payment/PaymentService.java(2 hunks)src/main/java/life/mosu/mosuserver/application/school/SchoolQuotaCacheManager.java(0 hunks)src/main/java/life/mosu/mosuserver/application/school/SchoolService.java(0 hunks)src/main/java/life/mosu/mosuserver/domain/admin/ApplicationQueryRepositoryImpl.java(1 hunks)src/main/java/life/mosu/mosuserver/domain/admin/RefundQueryRepositoryImpl.java(1 hunks)src/main/java/life/mosu/mosuserver/domain/application/AdmissionTicketImageJpaRepository.java(0 hunks)src/main/java/life/mosu/mosuserver/domain/application/ApplicationJpaEntity.java(2 hunks)src/main/java/life/mosu/mosuserver/domain/application/ApplicationJpaRepository.java(1 hunks)src/main/java/life/mosu/mosuserver/domain/application/ExamTicketImageJpaEntity.java(2 hunks)src/main/java/life/mosu/mosuserver/domain/application/ExamTicketImageJpaRepository.java(1 hunks)src/main/java/life/mosu/mosuserver/domain/applicationschool/ApplicationSchoolJpaEntity.java(0 hunks)src/main/java/life/mosu/mosuserver/domain/applicationschool/ApplicationSchoolJpaRepository.java(0 hunks)src/main/java/life/mosu/mosuserver/domain/applicationschool/ApplicationSchoolNotifyProjection.java(0 hunks)src/main/java/life/mosu/mosuserver/domain/applicationschool/OneWeekNotifyProjection.java(0 hunks)src/main/java/life/mosu/mosuserver/domain/exam/AddressJpaVO.java(2 hunks)src/main/java/life/mosu/mosuserver/domain/exam/Area.java(1 hunks)src/main/java/life/mosu/mosuserver/domain/exam/ExamJpaEntity.java(1 hunks)src/main/java/life/mosu/mosuserver/domain/exam/ExamJpaRepository.java(1 hunks)src/main/java/life/mosu/mosuserver/domain/examapplication/ExamApplicationJpaEntity.java(1 hunks)src/main/java/life/mosu/mosuserver/domain/examapplication/ExamApplicationJpaRepository.java(1 hunks)src/main/java/life/mosu/mosuserver/domain/examapplication/ExamNumberGenerationService.java(1 hunks)src/main/java/life/mosu/mosuserver/domain/examapplication/ExamSubjectJpaEntity.java(1 hunks)src/main/java/life/mosu/mosuserver/domain/examapplication/ExamSubjectJpaRepository.java(1 hunks)src/main/java/life/mosu/mosuserver/domain/lunch/LunchJpaEntity.java(1 hunks)src/main/java/life/mosu/mosuserver/domain/lunch/LunchJpaRepository.java(1 hunks)src/main/java/life/mosu/mosuserver/domain/refund/RefundJpaRepository.java(1 hunks)src/main/java/life/mosu/mosuserver/domain/school/LunchMenu.java(0 hunks)src/main/java/life/mosu/mosuserver/domain/school/SchoolApplicationProjection.java(0 hunks)src/main/java/life/mosu/mosuserver/domain/school/SchoolJpaEntity.java(0 hunks)src/main/java/life/mosu/mosuserver/domain/school/SchoolJpaRepository.java(0 hunks)src/main/java/life/mosu/mosuserver/domain/school/SchoolQueryRepository.java(0 hunks)src/main/java/life/mosu/mosuserver/domain/school/SchoolQueryRepositoryImpl.java(0 hunks)src/main/java/life/mosu/mosuserver/global/exception/ErrorCode.java(2 hunks)src/main/java/life/mosu/mosuserver/global/initializer/DatabaseInitializer.java(1 hunks)src/main/java/life/mosu/mosuserver/global/runner/ApplicationSchoolPreWarmRunner.java(1 hunks)src/main/java/life/mosu/mosuserver/global/util/FileRequest.java(4 hunks)src/main/java/life/mosu/mosuserver/infra/respository/BulkRepository.java(1 hunks)src/main/java/life/mosu/mosuserver/infra/storage/application/S3Service.java(2 hunks)src/main/java/life/mosu/mosuserver/presentation/admin/AdminController.java(1 hunks)src/main/java/life/mosu/mosuserver/presentation/admin/dto/ApplicationListResponse.java(1 hunks)src/main/java/life/mosu/mosuserver/presentation/admin/dto/ExamTicketResponse.java(3 hunks)src/main/java/life/mosu/mosuserver/presentation/application/ApplicationController.java(1 hunks)src/main/java/life/mosu/mosuserver/presentation/application/dto/ApplicationRequest.java(2 hunks)src/main/java/life/mosu/mosuserver/presentation/application/dto/ApplicationResponse.java(1 hunks)src/main/java/life/mosu/mosuserver/presentation/application/dto/ApplicationSchoolRequest.java(1 hunks)src/main/java/life/mosu/mosuserver/presentation/application/dto/ApplicationSchoolResponse.java(0 hunks)src/main/java/life/mosu/mosuserver/presentation/application/dto/ExamApplicationRequest.java(1 hunks)src/main/java/life/mosu/mosuserver/presentation/application/dto/ExamApplicationResponse.java(1 hunks)src/main/java/life/mosu/mosuserver/presentation/application/dto/ExamWithSubjects.java(1 hunks)src/main/java/life/mosu/mosuserver/presentation/applicationschool/ApplicationSchoolController.java(0 hunks)src/main/java/life/mosu/mosuserver/presentation/applicationschool/ApplicationSchoolControllerDocs.java(0 hunks)src/main/java/life/mosu/mosuserver/presentation/applicationschool/dto/RefundRequest.java(0 hunks)src/main/java/life/mosu/mosuserver/presentation/applicationschool/dto/SubjectUpdateRequest.java(0 hunks)src/main/java/life/mosu/mosuserver/presentation/common/AddressResponse.java(1 hunks)src/main/java/life/mosu/mosuserver/presentation/exam/ExamController.java(1 hunks)src/main/java/life/mosu/mosuserver/presentation/exam/dto/AddressRequest.java(1 hunks)src/main/java/life/mosu/mosuserver/presentation/exam/dto/ExamLunchRequest.java(1 hunks)src/main/java/life/mosu/mosuserver/presentation/exam/dto/ExamRequest.java(1 hunks)src/main/java/life/mosu/mosuserver/presentation/exam/dto/ExamResponse.java(1 hunks)src/main/java/life/mosu/mosuserver/presentation/lunch/LunchController.java(1 hunks)src/main/java/life/mosu/mosuserver/presentation/lunch/LunchEventListener.java(1 hunks)src/main/java/life/mosu/mosuserver/presentation/lunch/dto/CreateLunchEvent.java(1 hunks)src/main/java/life/mosu/mosuserver/presentation/lunch/dto/LunchResponse.java(1 hunks)src/main/java/life/mosu/mosuserver/presentation/school/SchoolController.java(0 hunks)src/main/java/life/mosu/mosuserver/presentation/school/SchoolControllerDocs.java(0 hunks)src/main/java/life/mosu/mosuserver/presentation/school/dto/AreaDetail.java(0 hunks)src/main/java/life/mosu/mosuserver/presentation/school/dto/AreaMeta.java(0 hunks)src/main/java/life/mosu/mosuserver/presentation/school/dto/AvailableSchoolResponse.java(0 hunks)src/main/java/life/mosu/mosuserver/presentation/school/dto/ExamInfo.java(0 hunks)src/main/java/life/mosu/mosuserver/presentation/school/dto/LunchInfo.java(0 hunks)src/main/java/life/mosu/mosuserver/presentation/school/dto/LunchMenuRequest.java(0 hunks)src/main/java/life/mosu/mosuserver/presentation/school/dto/SchoolEditRequest.java(0 hunks)src/main/java/life/mosu/mosuserver/presentation/school/dto/SchoolRegistrationRequest.java(0 hunks)src/main/java/life/mosu/mosuserver/presentation/school/dto/SchoolResponse.java(0 hunks)
💤 Files with no reviewable changes (30)
- src/main/java/life/mosu/mosuserver/presentation/school/dto/LunchInfo.java
- src/main/java/life/mosu/mosuserver/domain/school/SchoolApplicationProjection.java
- src/main/java/life/mosu/mosuserver/domain/applicationschool/ApplicationSchoolNotifyProjection.java
- src/main/java/life/mosu/mosuserver/domain/school/SchoolQueryRepository.java
- src/main/java/life/mosu/mosuserver/domain/school/LunchMenu.java
- src/main/java/life/mosu/mosuserver/presentation/applicationschool/dto/SubjectUpdateRequest.java
- src/main/java/life/mosu/mosuserver/presentation/school/dto/ExamInfo.java
- src/main/java/life/mosu/mosuserver/presentation/school/dto/AvailableSchoolResponse.java
- src/main/java/life/mosu/mosuserver/presentation/school/SchoolControllerDocs.java
- src/main/java/life/mosu/mosuserver/presentation/school/dto/AreaMeta.java
- src/main/java/life/mosu/mosuserver/domain/applicationschool/ApplicationSchoolJpaEntity.java
- src/main/java/life/mosu/mosuserver/domain/application/AdmissionTicketImageJpaRepository.java
- src/main/java/life/mosu/mosuserver/domain/school/SchoolQueryRepositoryImpl.java
- src/main/java/life/mosu/mosuserver/presentation/school/dto/AreaDetail.java
- src/main/java/life/mosu/mosuserver/domain/school/SchoolJpaRepository.java
- src/main/java/life/mosu/mosuserver/domain/school/SchoolJpaEntity.java
- src/main/java/life/mosu/mosuserver/presentation/applicationschool/ApplicationSchoolController.java
- src/main/java/life/mosu/mosuserver/domain/applicationschool/OneWeekNotifyProjection.java
- src/main/java/life/mosu/mosuserver/application/school/SchoolQuotaCacheManager.java
- src/main/java/life/mosu/mosuserver/presentation/applicationschool/ApplicationSchoolControllerDocs.java
- src/main/java/life/mosu/mosuserver/presentation/school/dto/SchoolRegistrationRequest.java
- src/main/java/life/mosu/mosuserver/presentation/school/SchoolController.java
- src/main/java/life/mosu/mosuserver/presentation/school/dto/LunchMenuRequest.java
- src/main/java/life/mosu/mosuserver/application/applicationschool/ApplicationSchoolService.java
- src/main/java/life/mosu/mosuserver/presentation/school/dto/SchoolEditRequest.java
- src/main/java/life/mosu/mosuserver/presentation/application/dto/ApplicationSchoolResponse.java
- src/main/java/life/mosu/mosuserver/presentation/school/dto/SchoolResponse.java
- src/main/java/life/mosu/mosuserver/presentation/applicationschool/dto/RefundRequest.java
- src/main/java/life/mosu/mosuserver/domain/applicationschool/ApplicationSchoolJpaRepository.java
- src/main/java/life/mosu/mosuserver/application/school/SchoolService.java
✅ Files skipped from review due to trivial changes (22)
- src/main/java/life/mosu/mosuserver/domain/exam/Area.java
- src/main/java/life/mosu/mosuserver/presentation/common/AddressResponse.java
- src/main/java/life/mosu/mosuserver/presentation/application/ApplicationController.java
- src/main/java/life/mosu/mosuserver/application/admin/AdminService.java
- src/main/java/life/mosu/mosuserver/presentation/exam/dto/AddressRequest.java
- src/main/java/life/mosu/mosuserver/domain/application/ApplicationJpaEntity.java
- src/main/java/life/mosu/mosuserver/domain/exam/AddressJpaVO.java
- src/main/java/life/mosu/mosuserver/application/inquiry/InquiryAnswerAttachmentService.java
- src/main/java/life/mosu/mosuserver/presentation/admin/AdminController.java
- src/main/java/life/mosu/mosuserver/global/runner/ApplicationSchoolPreWarmRunner.java
- src/main/java/life/mosu/mosuserver/presentation/application/dto/ExamApplicationRequest.java
- src/main/java/life/mosu/mosuserver/presentation/exam/dto/ExamLunchRequest.java
- src/main/java/life/mosu/mosuserver/presentation/lunch/LunchEventListener.java
- src/main/java/life/mosu/mosuserver/domain/examapplication/ExamSubjectJpaEntity.java
- src/main/java/life/mosu/mosuserver/presentation/lunch/dto/LunchResponse.java
- src/main/java/life/mosu/mosuserver/presentation/admin/dto/ExamTicketResponse.java
- src/main/java/life/mosu/mosuserver/presentation/application/dto/ExamApplicationResponse.java
- src/main/java/life/mosu/mosuserver/domain/exam/ExamJpaRepository.java
- src/main/java/life/mosu/mosuserver/presentation/application/dto/ExamWithSubjects.java
- src/main/java/life/mosu/mosuserver/domain/exam/ExamJpaEntity.java
- src/main/java/life/mosu/mosuserver/domain/lunch/LunchJpaEntity.java
- src/main/java/life/mosu/mosuserver/presentation/exam/dto/ExamResponse.java
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/life/mosu/mosuserver/presentation/application/dto/ApplicationRequest.java
🔇 Additional comments (30)
src/main/java/life/mosu/mosuserver/application/application/SchoolQuotaCacheManager.java (1)
1-70: Remove commented-out code entirely.The entire
SchoolQuotaCacheManagerclass is commented out, indicating it's no longer needed in the new exam-centric architecture. Commented-out code should be removed rather than left in the codebase as it creates maintenance overhead and confusion.The AI summary describes this as "newly introduced" functionality, but the actual code shows the entire class is commented out, suggesting it's being deprecated as part of the architectural refactoring.
-//package life.mosu.mosuserver.application.school; -// -//import java.util.List; -//import lombok.RequiredArgsConstructor; -//import org.springframework.data.redis.core.RedisTemplate; -//import org.springframework.stereotype.Service; -// -//@Service -//@RequiredArgsConstructor -//public class SchoolQuotaCacheManager { -// -// private static final String REDIS_KEY_SCHOOL_MAX_CAPACITY = "school:max_capacity:"; -// private static final String REDIS_KEY_SCHOOL_CURRENT_APPLICATIONS = "school:current_applications:"; -// -// private final RedisTemplate<String, Long> redisTemplate; -// private final SchoolJpaRepository schoolRepository; -// -// public void cacheSchoolMaxCapacities() { -// List<SchoolJpaEntity> schools = schoolRepository.findAll(); -// for (SchoolJpaEntity school : schools) { -// addSchoolMaxCapacity(school.getSchoolName(), school.getCapacity()); -// } -// } -// -// public void cacheSchoolCurrentApplicationCounts() { -// List<SchoolApplicationProjection> schoolApplications = schoolRepository.countBySchoolNameGroupBy(); -// for (SchoolApplicationProjection projection : schoolApplications) { -// addSchoolCurrentApplicationCount(projection.schoolName(), projection.count()); -// } -// } -// -// public void addSchoolMaxCapacity(String schoolName, Long capacity) { -// String key = REDIS_KEY_SCHOOL_MAX_CAPACITY + schoolName; -// redisTemplate.opsForValue().set(key, capacity); -// } -// -// public void addSchoolCurrentApplicationCount(String schoolName, Long currentCount) { -// String key = REDIS_KEY_SCHOOL_CURRENT_APPLICATIONS + schoolName; -// redisTemplate.opsForValue().set(key, currentCount); -// } -// -// -// public Long getSchoolApplicationCounts(String schoolName) { -// return redisTemplate.opsForValue() -// .get(REDIS_KEY_SCHOOL_CURRENT_APPLICATIONS + schoolName); -// } -// -// public Long getSchoolCapacities(String schoolName) { -// return redisTemplate.opsForValue() -// .get(REDIS_KEY_SCHOOL_MAX_CAPACITY + schoolName); -// } -// -// public void increaseApplicationCount(String schoolName) { -// String key = REDIS_KEY_SCHOOL_CURRENT_APPLICATIONS + schoolName; -// redisTemplate.opsForValue().increment(key); -// } -// -// public void decreaseApplicationCount(String schoolName) { -// String key = REDIS_KEY_SCHOOL_CURRENT_APPLICATIONS + schoolName; -// Long currentValue = redisTemplate.opsForValue().get(key); -// if (currentValue != null && currentValue > 0) { -// redisTemplate.opsForValue().decrement(key); -// } -// } -// -// public void preloadSchoolData() { -// cacheSchoolMaxCapacities(); -// cacheSchoolCurrentApplicationCounts(); -// } -//}This aligns with the PR objective of refactoring from a school-centric to exam-centric application model.
Likely an incorrect or invalid review comment.
src/main/java/life/mosu/mosuserver/domain/lunch/LunchJpaRepository.java (1)
1-10: LGTM! Clean repository interface following Spring Data JPA conventions.The implementation correctly follows Spring Data JPA patterns with proper method naming that will automatically generate the appropriate query for finding lunch entities by exam ID.
src/main/java/life/mosu/mosuserver/domain/examapplication/ExamApplicationJpaRepository.java (1)
1-11: LGTM! Properly structured repository interface.The repository correctly implements Spring Data JPA patterns. The
findByApplicationIdmethod appropriately returns a List, indicating the one-to-many relationship between applications and exam applications in the new domain model.src/main/java/life/mosu/mosuserver/application/lunch/LunchEventService.java (1)
12-23: LGTM! Well-structured event-driven service.The implementation correctly follows Spring patterns with proper transaction management and efficient batch saving using
saveAll().src/main/java/life/mosu/mosuserver/domain/examapplication/ExamSubjectJpaRepository.java (1)
1-10: LGTM! Properly designed repository interface.The repository follows Spring Data JPA conventions correctly. The
findByExamApplicationIdmethod appropriately returns a List, supporting the one-to-many relationship between exam applications and their subjects in the new domain model.src/main/java/life/mosu/mosuserver/domain/examapplication/ExamNumberGenerationService.java (1)
17-19: Verify if UUID format meets exam number requirements.UUIDs are globally unique but may not be user-friendly for exam numbers. Consider if exam numbers need to be:
- Sequential or human-readable
- Shorter format
- Following specific patterns
If current format is acceptable, the implementation is correct.
src/main/java/life/mosu/mosuserver/application/examapplication/ExamApplicationService.java (1)
19-24: Transaction handling and batch operations look correct.The @transactional annotation ensures atomicity for the batch registration process. The combination of exam number generation and saveAll operation is efficient for bulk processing.
src/main/java/life/mosu/mosuserver/application/examapplication/dto/RegisterExamApplicationEvent.java (2)
12-20: Factory method implementation is clean and functional.The stream-based transformation from ExamApplicationRequest to TargetExam is efficient and follows good functional programming practices.
22-30: Entity conversion method is well-structured.The toEntity method properly maps the event data to JPA entities using the stream API. The entity creation pattern follows good domain modeling practices.
src/main/java/life/mosu/mosuserver/presentation/application/dto/ApplicationResponse.java (1)
7-23: Clean refactoring from school-centric to exam-centric model.The transition from
schoolstoexamsfield aligns well with the architectural shift described in the PR objectives. The dual factory methods provide good flexibility for different use cases.src/main/java/life/mosu/mosuserver/presentation/lunch/dto/CreateLunchEvent.java (1)
6-34: Well-structured event DTO with good separation of concerns.The nested record structure and conversion methods are clean and follow good practices. The examId parameter passing to the nested entity creation is handled correctly.
src/main/java/life/mosu/mosuserver/domain/application/ExamTicketImageJpaEntity.java (5)
3-8: LGTM: Clean import refactoringGood practice replacing wildcard imports with explicit Jakarta Persistence imports for better clarity and maintainability.
19-26: LGTM: Consistent entity and table namingThe table name and primary key column have been correctly updated to align with the exam-centric domain model. The naming convention is consistent throughout.
3-8: Good practice: Explicit imports over wildcard imports.The change from wildcard imports to explicit Jakarta Persistence imports improves code clarity and reduces potential naming conflicts.
19-26: LGTM: Consistent entity refactoring.The renaming from
admission_ticket_imagetoexam_ticket_imagefor both table and column names is consistent with the domain model shift from school-centric to exam-centric applications.
33-37: Constructor maintains proper encapsulation.The constructor properly delegates to the parent
Fileclass and maintains the same parameter structure, ensuring backward compatibility within the new domain model.src/main/java/life/mosu/mosuserver/presentation/admin/dto/ApplicationListResponse.java (2)
67-68: LGTM: Consistent field renameThe field rename from
admissionTickettoexamTicketwith the corresponding type change toExamTicketResponsealigns perfectly with the domain refactoring from admission-based to exam-based terminology.
68-68: Field rename aligns with domain model refactoring.The change from
admissionTickettoexamTicketwith typeExamTicketResponseis consistent with the broader refactoring from school-centric to exam-centric model.src/main/java/life/mosu/mosuserver/application/inquiry/InquiryAttachmentService.java (1)
54-54: LGTM! S3Service method call simplification.The removal of the explicit duration parameter from
getPreSignedUrlaligns well with centralizing expiration configuration within the S3Service. This makes the code cleaner and more maintainable.src/main/java/life/mosu/mosuserver/infra/storage/application/S3Service.java (1)
102-116: LGTM! Excellent API simplification by centralizing expiration configuration.The removal of the explicit
Durationparameter and internal retrieval fromS3Propertiesmakes the API cleaner while centralizing configuration. This is a good design improvement that reduces parameter passing across the codebase.src/main/java/life/mosu/mosuserver/application/faq/FaqAttachmentService.java (2)
53-53: LGTM!The removal of the expiration parameter from
getPreSignedUrlaligns with the centralized expiration management in S3Service.
32-32: Method signature confirmed
ThetoFaqAttachmentEntity(Long faqId)method exists as expected.•
src/main/java/life/mosu/mosuserver/global/util/FileRequest.java:24
public FaqAttachmentJpaEntity toFaqAttachmentEntity(Long faqId) { … }No further changes needed.
src/main/java/life/mosu/mosuserver/application/notice/NoticeAttachmentService.java (2)
32-32: LGTM! Good simplification of the lambda expression.The removal of explicit parameters in the lambda expression improves readability while maintaining functionality. This aligns well with the centralized approach to handling file metadata.
74-74: LGTM! Simplified presigned URL generation.Moving the expiration duration management to the S3Service centralizes configuration and reduces parameter passing. This is a good architectural improvement.
src/main/java/life/mosu/mosuserver/global/exception/ErrorCode.java (1)
94-104: LGTM! Well-structured error codes for the new domain model.The new error codes provide comprehensive coverage for the exam-centric refactoring:
- Multi-insert operation failures are properly handled
- Exam and exam application not found scenarios are covered
- Lunch-related errors are included
- HTTP status codes are appropriate
- Error messages are clear and user-friendly
These additions align well with the architectural shift from school-centric to exam-centric model.
src/main/java/life/mosu/mosuserver/global/util/FileRequest.java (5)
4-4: Import change aligns with domain refactoring.The import change from
AdmissionTicketImageJpaEntitytoExamTicketImageJpaEntitycorrectly reflects the domain model shift from school-based to exam-centric applications.
52-59: Method rename correctly reflects domain changes.The method rename from
toAdmissionTicketImageEntitytotoExamTicketImageEntityproperly aligns with the shift from admission-based to exam-based domain model. The entity creation logic remains consistent.
24-31: Entity conversion methods follow consistent patterns.All entity conversion methods follow a consistent builder pattern using the record's
fileNameands3Keyfields directly. The visibility settings are appropriately configured for each entity type:
- FAQ attachments: PUBLIC (appropriate for public documentation)
- Inquiry answer attachments: PRIVATE (appropriate for customer service responses)
- Event attachments: PUBLIC (appropriate for public event materials)
Also applies to: 61-68, 70-77
33-40: Private visibility verified via pre-signed URLsInquiry attachments are served with private S3 ACLs but exposed through pre-signed links:
- In
InquiryAttachmentService#createAttachDetailResponse,s3Service.getPreSignedUrl(attachment.getS3Key())generates access URLs forPRIVATEobjects.No further changes needed.
43-50: NoticeAttachment visibility is consistent with other public attachmentsThe change to
Visibility.PUBLICfor notice attachments matches the pattern used for FAQ and event attachments (also set to PUBLIC), while user-specific attachments (inquiry, exam ticket, inquiry answer) remain PRIVATE. No further action is needed.• File: src/main/java/life/mosu/mosuserver/global/util/FileRequest.java
–toFaqAttachmentEntity(line 28): PUBLIC
–toInquiryAttachmentEntity(line 37): PRIVATE
–toNoticeAttachmentEntity(line 43): PUBLIC
–toExamTicketImageEntity(line 56): PRIVATE
–toInquiryAnswerAttachmentEntity(line 65): PRIVATE
–toEventAttachmentEntity(line 74): PUBLIC
| // @Query(""" | ||
| // SELECT new life.mosu.mosuserver.domain.refund.RefundNotifyProjection( | ||
| // p.paymentKey, | ||
| // a.examDate, | ||
| // a.schoolName, | ||
| // p.paymentMethod, | ||
| // r.reason | ||
| // ) | ||
| // FROM RefundJpaEntity r | ||
| // LEFT JOIN PaymentJpaEntity p ON r.applicationSchoolId = p.applicationSchoolId | ||
| // LEFT JOIN ApplicationSchoolJpaEntity a ON r.applicationSchoolId = a.id | ||
| // WHERE r.applicationSchoolId = :applicationSchoolId | ||
| // """) | ||
| // Optional<RefundNotifyProjection> findRefundByApplicationSchoolId(Long applicationSchoolId); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify refund functionality replacement in the new exam-centric model.
The commented-out method findRefundByApplicationSchoolId suggests refund queries need to be adapted for the new exam-based model. Ensure that equivalent functionality exists for refund processing with exam applications.
Consider either:
- Implementing a replacement method for exam-based refunds
- Removing the commented code if no longer needed
- Adding a TODO comment explaining the migration plan
🏁 Script executed:
#!/bin/bash
# Search for any refund-related functionality that might need the commented method
rg -A 5 -B 5 "findRefundByApplicationSchoolId|RefundJpaRepository" --type javaLength of output: 6622
Implement exam-centric refund query and update notification logic
The commented-out findRefundByApplicationSchoolId has not been replaced by an exam-centric alternative. You’ll need to either provide a new repository method based on your exam model or remove the dead code and clearly document the migration plan.
• RefundJpaRepository.java
- Remove or replace the commented JPQL.
- Add a new method (e.g.
findRefundByExamApplicationId(Long examApplicationId)) that joinsRefundJpaEntity,PaymentJpaEntityand your exam-application entity to populateRefundNotifyProjection.
• NotifyVariableFactory.java
- Un-comment and update
createRefundVariableto call the new repository method. - Throw
APPLICATION_NOT_FOUNDor an appropriate exception if the projection isn’t present, then map it intoRefundNotifyRequest.
If you intend to postpone this work, replace the commented block with a clear // TODO: … pointing to the migration ticket.
🤖 Prompt for AI Agents
In src/main/java/life/mosu/mosuserver/domain/refund/RefundJpaRepository.java
around lines 7 to 20, the commented-out method findRefundByApplicationSchoolId
is obsolete and needs to be replaced with an exam-centric query. Remove the
commented JPQL and implement a new repository method named
findRefundByExamApplicationId that joins RefundJpaEntity, PaymentJpaEntity, and
the exam-application entity to return RefundNotifyProjection. Also, update
NotifyVariableFactory.java to un-comment and modify createRefundVariable to call
this new method, throwing an APPLICATION_NOT_FOUND exception if no projection is
found, and map the result into RefundNotifyRequest. If this work is deferred,
replace the commented code with a clear TODO referencing the migration ticket.
| List<TargetExam> targetExams = examApplicationRequests.stream() | ||
| .map(request -> new TargetExam(request.examId(), request.lunchId())) | ||
| .toList(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding null safety validation.
The factory method should validate input parameters to prevent null pointer exceptions during stream operations.
public static RegisterExamApplicationEvent of(
List<ExamApplicationRequest> examApplicationRequests,
Long applicationId
) {
+ if (examApplicationRequests == null || applicationId == null) {
+ throw new IllegalArgumentException("ExamApplicationRequests and applicationId cannot be null");
+ }
+
List<TargetExam> targetExams = examApplicationRequests.stream()
.map(request -> new TargetExam(request.examId(), request.lunchId()))
.toList();
return new RegisterExamApplicationEvent(targetExams, applicationId);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| List<TargetExam> targetExams = examApplicationRequests.stream() | |
| .map(request -> new TargetExam(request.examId(), request.lunchId())) | |
| .toList(); | |
| public static RegisterExamApplicationEvent of( | |
| List<ExamApplicationRequest> examApplicationRequests, | |
| Long applicationId | |
| ) { | |
| if (examApplicationRequests == null || applicationId == null) { | |
| throw new IllegalArgumentException("ExamApplicationRequests and applicationId cannot be null"); | |
| } | |
| List<TargetExam> targetExams = examApplicationRequests.stream() | |
| .map(request -> new TargetExam(request.examId(), request.lunchId())) | |
| .toList(); | |
| return new RegisterExamApplicationEvent(targetExams, applicationId); | |
| } |
🤖 Prompt for AI Agents
In
src/main/java/life/mosu/mosuserver/application/examapplication/dto/RegisterExamApplicationEvent.java
around lines 16 to 18, the stream operation on examApplicationRequests lacks
null safety checks. Add validation to ensure examApplicationRequests is not null
before streaming, and consider validating individual elements or their fields to
prevent null pointer exceptions during the map operation.
| @GetMapping() | ||
| public ResponseEntity<ApiResponseWrapper<List<LunchResponse>>> getByExamId( | ||
| @RequestParam Long examId) { | ||
| List<LunchResponse> lunchResponses = lunchService.getByExamId(examId); | ||
| return ResponseEntity.ok( | ||
| ApiResponseWrapper.success(HttpStatus.OK, "시험에 호환되는 도시락 전체 조회 성공", lunchResponses)); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation for examId parameter.
The controller endpoint is well-structured, but consider adding validation for the examId parameter to handle invalid inputs gracefully.
@GetMapping()
public ResponseEntity<ApiResponseWrapper<List<LunchResponse>>> getByExamId(
- @RequestParam Long examId) {
+ @RequestParam @Min(1) Long examId) {
List<LunchResponse> lunchResponses = lunchService.getByExamId(examId);
return ResponseEntity.ok(
ApiResponseWrapper.success(HttpStatus.OK, "시험에 호환되는 도시락 전체 조회 성공", lunchResponses));
}Also consider adding @Valid annotation and proper exception handling for when the examId doesn't exist.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/main/java/life/mosu/mosuserver/presentation/lunch/LunchController.java
around lines 22 to 28, add input validation for the examId parameter by checking
if it is null or less than or equal to zero before calling the service. Use
annotations like @RequestParam(required = true) and consider adding @Valid if
applicable. Implement proper exception handling to return meaningful error
responses when examId is invalid or does not exist, ensuring the controller
gracefully handles such cases.
| private Area parseArea(String areaName) { | ||
| try { | ||
| return Area.from(areaName); | ||
| } catch (Exception e) { | ||
| throw new RuntimeException(e); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve exception handling in parseArea method.
The current exception handling is too broad and loses important error information. Consider handling specific exceptions and providing more meaningful error messages.
private Area parseArea(String areaName) {
try {
return Area.from(areaName);
- } catch (Exception e) {
- throw new RuntimeException(e);
+ } catch (IllegalArgumentException e) {
+ throw new IllegalArgumentException("Invalid area name: " + areaName, e);
+ } catch (Exception e) {
+ throw new IllegalStateException("Failed to parse area name: " + areaName, e);
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private Area parseArea(String areaName) { | |
| try { | |
| return Area.from(areaName); | |
| } catch (Exception e) { | |
| throw new RuntimeException(e); | |
| } | |
| } | |
| private Area parseArea(String areaName) { | |
| try { | |
| return Area.from(areaName); | |
| } catch (IllegalArgumentException e) { | |
| throw new IllegalArgumentException("Invalid area name: " + areaName, e); | |
| } catch (Exception e) { | |
| throw new IllegalStateException("Failed to parse area name: " + areaName, e); | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/main/java/life/mosu/mosuserver/presentation/exam/dto/ExamRequest.java
between lines 44 and 50, the parseArea method catches a broad Exception and
rethrows it as a generic RuntimeException, which obscures the original error
details. Refine the exception handling by catching specific exceptions that
Area.from might throw, and throw a new exception with a clear, meaningful
message that includes the original exception as the cause to preserve error
context.
| public interface ExamTicketImageJpaRepository extends | ||
| JpaRepository<ExamTicketImageJpaEntity, Long> { | ||
|
|
||
| ExamTicketImageJpaEntity findByApplicationId(Long applicationId); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using Optional for null safety
The method findByApplicationId returns ExamTicketImageJpaEntity directly, which will return null if no record is found. Consider using Optional<ExamTicketImageJpaEntity> for better null safety and explicit handling of missing records.
- ExamTicketImageJpaEntity findByApplicationId(Long applicationId);
+ Optional<ExamTicketImageJpaEntity> findByApplicationId(Long applicationId);Don't forget to add the import:
+import java.util.Optional;🤖 Prompt for AI Agents
In
src/main/java/life/mosu/mosuserver/domain/application/ExamTicketImageJpaRepository.java
at line 8, the method findByApplicationId currently returns
ExamTicketImageJpaEntity directly, which can be null if no record is found.
Change the return type to Optional<ExamTicketImageJpaEntity> to improve null
safety and make the absence of a record explicit. Also, add the necessary import
for java.util.Optional at the top of the file.
🛠️ Refactor suggestion
Consider using Optional return type for better null safety.
The findByApplicationId method should return Optional<ExamTicketImageJpaEntity> to handle cases where no exam ticket image exists for the given application ID, following Spring Data JPA best practices.
- ExamTicketImageJpaEntity findByApplicationId(Long applicationId);
+ Optional<ExamTicketImageJpaEntity> findByApplicationId(Long applicationId);Don't forget to add the import:
import java.util.Optional;🤖 Prompt for AI Agents
In
src/main/java/life/mosu/mosuserver/domain/application/ExamTicketImageJpaRepository.java
at line 8, change the return type of the method findByApplicationId from
ExamTicketImageJpaEntity to Optional<ExamTicketImageJpaEntity> to improve null
safety and follow Spring Data JPA best practices. Also, add the import statement
for java.util.Optional at the top of the file.
| //package life.mosu.mosuserver.infra.respository; | ||
| // | ||
| //import java.sql.PreparedStatement; | ||
| //import java.sql.ResultSet; | ||
| //import java.sql.SQLException; | ||
| //import java.sql.Statement; | ||
| //import java.sql.Timestamp; | ||
| //import java.time.LocalDateTime; | ||
| //import java.util.ArrayList; | ||
| //import java.util.List; | ||
| //import life.mosu.mosuserver.domain.application.Subject; | ||
| //import life.mosu.mosuserver.domain.examapplication.ExamApplicationJpaEntity; | ||
| //import life.mosu.mosuserver.global.exception.CustomRuntimeException; | ||
| //import life.mosu.mosuserver.global.exception.ErrorCode; | ||
| //import lombok.RequiredArgsConstructor; | ||
| //import org.springframework.dao.DataAccessException; | ||
| //import org.springframework.jdbc.core.ConnectionCallback; | ||
| //import org.springframework.jdbc.core.JdbcTemplate; | ||
| //import org.springframework.stereotype.Repository; | ||
| //import org.springframework.transaction.annotation.Transactional; | ||
| // | ||
| //@Repository | ||
| //@RequiredArgsConstructor | ||
| //public class ExamApplicationBulkRepository { | ||
| // | ||
| // private static final String SQL_INSERT_EXAM_APPLICATION = """ | ||
| // INSERT INTO exam_application | ||
| // (created_at, updated_at, application_id, user_id, | ||
| // examId, examination_number) | ||
| // VALUES (?,?,?,?,?,?) | ||
| // """; | ||
| // private static final String SQL_INSERT_SUBJECT = """ | ||
| // INSERT INTO exam_subject (exam_application_id, subjects) VALUES (?, ?) | ||
| // """; | ||
| // private final JdbcTemplate jdbcTemplate; | ||
| // | ||
| // | ||
| // @Transactional | ||
| // public List<Long> saveAllExamApplicationsWithSubjects( | ||
| // List<ExamApplicationJpaEntity> entities) { | ||
| // | ||
| // List<Long> generatedIds = jdbcTemplate.execute((ConnectionCallback<List<Long>>) con -> { | ||
| // try (PreparedStatement ps = con.prepareStatement( | ||
| // SQL_INSERT_EXAM_APPLICATION, Statement.RETURN_GENERATED_KEYS)) { | ||
| // for (ExamApplicationJpaEntity e : entities) { | ||
| // ps.setTimestamp(1, Timestamp.valueOf(LocalDateTime.now())); | ||
| // ps.setTimestamp(2, Timestamp.valueOf(LocalDateTime.now())); | ||
| // ps.setLong(3, e.getApplicationId()); | ||
| // ps.setLong(4, e.getUserId()); | ||
| // ps.setLong(5, e.examId()); | ||
| // ps.setString(6, e.getExaminationNumber()); | ||
| // ps.addBatch(); | ||
| // } | ||
| // | ||
| // ps.executeBatch(); | ||
| // | ||
| // List<Long> ids = new ArrayList<>(); | ||
| // try (ResultSet rs = ps.getGeneratedKeys()) { | ||
| // while (rs.next()) { | ||
| // ids.add(rs.getLong(1)); | ||
| // } | ||
| // } | ||
| // return ids; | ||
| // | ||
| // } catch (SQLException e) { | ||
| // throw new CustomRuntimeException(ErrorCode.EXAM_APPLICATION_MULTI_INSERT_ERROR); | ||
| // } | ||
| // }); | ||
| // | ||
| // List<Object[]> subjectParams = new ArrayList<>(); | ||
| // | ||
| // for (int i = 0; i < entities.size(); i++) { | ||
| // Long applicationSchoolId = generatedIds.get(i); | ||
| // | ||
| // for (Subject subj : entities.get(i).getSubjects()) { | ||
| // subjectParams.add(new Object[]{applicationSchoolId, String.valueOf(subj)}); | ||
| // } | ||
| // } | ||
| // | ||
| // try { | ||
| // if (!subjectParams.isEmpty()) { | ||
| // jdbcTemplate.batchUpdate(SQL_INSERT_SUBJECT, subjectParams); | ||
| // } | ||
| // } catch (DataAccessException e) { | ||
| // throw new CustomRuntimeException(ErrorCode.EXAM_SUBJECT_MULTI_INSERT_ERROR); | ||
| // } | ||
| // | ||
| // return generatedIds; | ||
| // } | ||
| // | ||
| //} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove commented-out code or activate the implementation.
The entire ExamApplicationBulkRepository implementation is commented out, making it dead code. Either activate this implementation if it's needed for the exam-centric refactoring or remove it entirely to maintain code cleanliness.
If this implementation is intended for future use, consider:
-
Activating the code and addressing previous review feedback:
- The exception handling improvements suggested in past reviews
- Adding batch size limits for large datasets
- Using consistent timestamps across batch operations
-
Removing the commented code if it's no longer needed for the current exam-centric model
The current approach of keeping large blocks of commented code reduces maintainability and can confuse future developers about the intended architecture.
🤖 Prompt for AI Agents
In src/main/java/life/mosu/mosuserver/infra/respository/BulkRepository.java
lines 1 to 91, the entire ExamApplicationBulkRepository class is commented out,
resulting in dead code. To fix this, either fully uncomment and activate the
implementation if it is required, ensuring to address any prior review comments
such as improving exception handling, adding batch size limits, and using
consistent timestamps, or remove the entire commented block if it is no longer
needed to keep the codebase clean and maintainable.
| @PostMapping | ||
| public ResponseEntity<ApiResponseWrapper<Void>> register( | ||
| @RequestBody ExamRequest request | ||
| ) { | ||
| examService.register(request); | ||
| return ResponseEntity.ok( | ||
| ApiResponseWrapper.success(HttpStatus.CREATED, "시험 등록 성공")); | ||
|
|
||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation for the request body.
The register method lacks validation annotations for the ExamRequest parameter. Consider adding @Valid to ensure proper validation of incoming data.
@PostMapping
public ResponseEntity<ApiResponseWrapper<Void>> register(
- @RequestBody ExamRequest request
+ @RequestBody @Valid ExamRequest request
) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @PostMapping | |
| public ResponseEntity<ApiResponseWrapper<Void>> register( | |
| @RequestBody ExamRequest request | |
| ) { | |
| examService.register(request); | |
| return ResponseEntity.ok( | |
| ApiResponseWrapper.success(HttpStatus.CREATED, "시험 등록 성공")); | |
| } | |
| @PostMapping | |
| public ResponseEntity<ApiResponseWrapper<Void>> register( | |
| @RequestBody @Valid ExamRequest request | |
| ) { | |
| examService.register(request); | |
| return ResponseEntity.ok( | |
| ApiResponseWrapper.success(HttpStatus.CREATED, "시험 등록 성공")); | |
| } |
🤖 Prompt for AI Agents
In src/main/java/life/mosu/mosuserver/presentation/exam/ExamController.java
around lines 26 to 34, the register method's ExamRequest parameter lacks
validation. Add the @Valid annotation before the @RequestBody annotation on the
ExamRequest parameter to enable validation of the incoming request data
according to the constraints defined in the ExamRequest class.
| @GetMapping | ||
| public ResponseEntity<ApiResponseWrapper<List<ExamResponse>>> getByArea( | ||
| @RequestParam String areaName | ||
| ) { | ||
| List<ExamResponse> response = examService.getByArea(areaName); | ||
| return ResponseEntity.ok( | ||
| ApiResponseWrapper.success(HttpStatus.OK, "지역별 시험 조회 성공", response)); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve type safety for area parameter.
Using String areaName parameter reduces type safety. Consider using the Area enum directly or add validation to ensure the provided string is a valid area.
@GetMapping
public ResponseEntity<ApiResponseWrapper<List<ExamResponse>>> getByArea(
- @RequestParam String areaName
+ @RequestParam Area area
) {
- List<ExamResponse> response = examService.getByArea(areaName);
+ List<ExamResponse> response = examService.getByArea(area);Alternatively, if you need to keep the String parameter, add validation:
@GetMapping
public ResponseEntity<ApiResponseWrapper<List<ExamResponse>>> getByArea(
- @RequestParam String areaName
+ @RequestParam @Valid String areaName
) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @GetMapping | |
| public ResponseEntity<ApiResponseWrapper<List<ExamResponse>>> getByArea( | |
| @RequestParam String areaName | |
| ) { | |
| List<ExamResponse> response = examService.getByArea(areaName); | |
| return ResponseEntity.ok( | |
| ApiResponseWrapper.success(HttpStatus.OK, "지역별 시험 조회 성공", response)); | |
| } | |
| @GetMapping | |
| public ResponseEntity<ApiResponseWrapper<List<ExamResponse>>> getByArea( | |
| @RequestParam Area area | |
| ) { | |
| List<ExamResponse> response = examService.getByArea(area); | |
| return ResponseEntity.ok( | |
| ApiResponseWrapper.success(HttpStatus.OK, "지역별 시험 조회 성공", response)); | |
| } |
🤖 Prompt for AI Agents
In src/main/java/life/mosu/mosuserver/presentation/exam/ExamController.java
around lines 36 to 43, the getByArea method uses a String parameter for
areaName, which reduces type safety. To fix this, change the parameter type to
the Area enum if possible, so Spring automatically validates and converts the
input. If you must keep it as a String, add explicit validation logic to check
if the provided string matches a valid Area enum value before calling
examService.getByArea, and handle invalid inputs appropriately by returning a
bad request response.
| List<ExamWithSubjects> exams = examApplications.stream() | ||
| .map(examApplication -> { | ||
| // 시험 정보 조회 | ||
| ExamJpaEntity exam = examJpaRepository.findById(examApplication.getExamId()) | ||
| .orElseThrow(() -> new CustomRuntimeException(ErrorCode.EXAM_NOT_FOUND)); | ||
|
|
||
| // 점심 정보 조회 (점심은 null일 수 있음) | ||
| LunchJpaEntity lunch = null; | ||
| if (examApplication.getLunchId() != null) { | ||
| lunch = lunchJpaRepository.findById(examApplication.getLunchId()) | ||
| .orElseThrow(() -> new CustomRuntimeException(ErrorCode.LUNCH_NOT_FOUND)); | ||
| } | ||
|
|
||
| // 과목 정보 조회 | ||
| List<ExamSubjectJpaEntity> examSubjects = | ||
| examSubjectJpaRepository.findByExamApplicationId(examApplication.getId()); | ||
| Set<String> subjects = examSubjects.stream() | ||
| .map(examSubject -> examSubject.getSubject().getSubjectName()) | ||
| .collect(Collectors.toSet()); | ||
|
|
||
| // ExamWithSubjects 생성 | ||
| return new ExamWithSubjects( | ||
| examApplication.getId(), | ||
| exam.getArea().getAreaName(), | ||
| exam.getExamDate(), | ||
| exam.getSchoolName(), | ||
| lunch != null ? lunch.getName() : "신청 안 함", | ||
| examApplication.getExamNumber() != null ? examApplication.getExamNumber() : "", | ||
| subjects | ||
| ); | ||
| }) | ||
| .toList(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Extract complex nested logic for better maintainability.
The getApplications method contains deeply nested logic that could be extracted into separate methods for better readability and testability.
Consider extracting the exam processing logic:
private ExamWithSubjects createExamWithSubjects(ExamApplicationJpaEntity examApplication) {
ExamJpaEntity exam = examJpaRepository.findById(examApplication.getExamId())
.orElseThrow(() -> new CustomRuntimeException(ErrorCode.EXAM_NOT_FOUND));
LunchJpaEntity lunch = null;
if (examApplication.getLunchId() != null) {
lunch = lunchJpaRepository.findById(examApplication.getLunchId())
.orElseThrow(() -> new CustomRuntimeException(ErrorCode.LUNCH_NOT_FOUND));
}
List<ExamSubjectJpaEntity> examSubjects =
examSubjectJpaRepository.findByExamApplicationId(examApplication.getId());
Set<String> subjects = examSubjects.stream()
.map(examSubject -> examSubject.getSubject().getSubjectName())
.collect(Collectors.toSet());
return new ExamWithSubjects(
examApplication.getId(),
exam.getArea().getAreaName(),
exam.getExamDate(),
exam.getSchoolName(),
lunch != null ? lunch.getName() : "신청 안 함",
examApplication.getExamNumber() != null ? examApplication.getExamNumber() : "",
subjects
);
}Then simplify the main method:
List<ExamWithSubjects> exams = examApplications.stream()
.map(this::createExamWithSubjects)
.toList();🤖 Prompt for AI Agents
In
src/main/java/life/mosu/mosuserver/application/application/ApplicationService.java
around lines 110 to 141, the nested logic inside the stream mapping for
examApplications is complex and reduces readability. Extract this logic into a
new private method named createExamWithSubjects that takes an
ExamApplicationJpaEntity and returns an ExamWithSubjects by performing the exam,
lunch, and subject retrievals and constructing the object. Then, replace the
inline lambda in the stream with a method reference to this new method to
simplify the main method and improve maintainability and testability.
| List<ExamApplicationJpaEntity> examApplications = | ||
| examApplicationJpaRepository.findByApplicationId(application.getId()); | ||
|
|
||
| if (examApplications.isEmpty()) { | ||
| throw new CustomRuntimeException(ErrorCode.EXAM_APPLICATION_NOT_FOUND); | ||
| } | ||
|
|
||
| // ExamWithSubjects 리스트 생성 | ||
| List<ExamWithSubjects> exams = examApplications.stream() | ||
| .map(examApplication -> { | ||
| // 시험 정보 조회 | ||
| ExamJpaEntity exam = examJpaRepository.findById(examApplication.getExamId()) | ||
| .orElseThrow(() -> new CustomRuntimeException(ErrorCode.EXAM_NOT_FOUND)); | ||
|
|
||
| // 점심 정보 조회 (점심은 null일 수 있음) | ||
| LunchJpaEntity lunch = null; | ||
| if (examApplication.getLunchId() != null) { | ||
| lunch = lunchJpaRepository.findById(examApplication.getLunchId()) | ||
| .orElseThrow(() -> new CustomRuntimeException(ErrorCode.LUNCH_NOT_FOUND)); | ||
| } | ||
|
|
||
| // 과목 정보 조회 | ||
| List<ExamSubjectJpaEntity> examSubjects = | ||
| examSubjectJpaRepository.findByExamApplicationId(examApplication.getId()); | ||
| Set<String> subjects = examSubjects.stream() | ||
| .map(examSubject -> examSubject.getSubject().getSubjectName()) | ||
| .collect(Collectors.toSet()); | ||
|
|
||
| // ExamWithSubjects 생성 | ||
| return new ExamWithSubjects( | ||
| examApplication.getId(), | ||
| exam.getArea().getAreaName(), | ||
| exam.getExamDate(), | ||
| exam.getSchoolName(), | ||
| lunch != null ? lunch.getName() : "신청 안 함", | ||
| examApplication.getExamNumber() != null ? examApplication.getExamNumber() : "", | ||
| subjects | ||
| ); | ||
| }) | ||
| .toList(); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Address potential N+1 query performance issues.
The current implementation may cause N+1 query problems by fetching exam, lunch, and subject data individually for each exam application. Consider using batch fetching or JOIN queries.
Consider optimizing with batch queries:
// Fetch all required data in batches
Set<Long> examIds = examApplications.stream()
.map(ExamApplicationJpaEntity::getExamId)
.collect(Collectors.toSet());
Set<Long> lunchIds = examApplications.stream()
.map(ExamApplicationJpaEntity::getLunchId)
.filter(Objects::nonNull)
.collect(Collectors.toSet());
Map<Long, ExamJpaEntity> examMap = examJpaRepository.findAllById(examIds)
.stream().collect(Collectors.toMap(ExamJpaEntity::getId, Function.identity()));
Map<Long, LunchJpaEntity> lunchMap = lunchJpaRepository.findAllById(lunchIds)
.stream().collect(Collectors.toMap(LunchJpaEntity::getId, Function.identity()));
// Then use these maps in the stream processingVerify the current query patterns to confirm if N+1 issues exist:
🏁 Script executed:
#!/bin/bash
# Search for potential N+1 query patterns in repository methods
echo "Checking for repository query methods that might cause N+1 issues..."
# Look for findById calls in loops or streams
rg -A 3 -B 3 "\.findById\(" --type java
# Check for repository methods that should use batch fetching
rg -A 3 -B 3 "findByExamApplicationId|findAllById" --type javaLength of output: 16408
Optimize ApplicationService to Eliminate N+1 Queries
The current stream mapping issues separate lookups for each Exam, Lunch, and Subjects, resulting in N+1 queries. Batch‐fetch all related data up front and then map into your DTO.
• File: src/main/java/life/mosu/mosuserver/application/application/ApplicationService.java
Lines: 102–141
• Add a repository method to fetch subjects in bulk:
interface ExamSubjectJpaRepository extends JpaRepository<ExamSubjectJpaEntity, Long> {
List<ExamSubjectJpaEntity> findByExamApplicationIdIn(Collection<Long> ids);
}• Refactor mapping:
// 1. Collect IDs
Set<Long> applicationIds = examApplications.stream()
.map(ExamApplicationJpaEntity::getId)
.collect(Collectors.toSet());
Set<Long> examIds = examApplications.stream()
.map(ExamApplicationJpaEntity::getExamId)
.collect(Collectors.toSet());
Set<Long> lunchIds = examApplications.stream()
.map(ExamApplicationJpaEntity::getLunchId)
.filter(Objects::nonNull)
.collect(Collectors.toSet());
// 2. Batch‐fetch entities
Map<Long, ExamJpaEntity> examMap = examJpaRepository.findAllById(examIds)
.stream().collect(Collectors.toMap(ExamJpaEntity::getId, Function.identity()));
Map<Long, LunchJpaEntity> lunchMap = lunchJpaRepository.findAllById(lunchIds)
.stream().collect(Collectors.toMap(LunchJpaEntity::getId, Function.identity()));
List<ExamSubjectJpaEntity> allSubjects =
examSubjectJpaRepository.findByExamApplicationIdIn(applicationIds);
Map<Long, Set<String>> subjectsByApp = allSubjects.stream()
.collect(Collectors.groupingBy(
s -> s.getExamApplication().getId(),
Collectors.mapping(s -> s.getSubject().getSubjectName(), Collectors.toSet())
));
// 3. Build DTOs without further queries
List<ExamWithSubjects> exams = examApplications.stream()
.map(app -> new ExamWithSubjects(
app.getId(),
examMap.get(app.getExamId()).getArea().getAreaName(),
examMap.get(app.getExamId()).getExamDate(),
examMap.get(app.getExamId()).getSchoolName(),
Optional.ofNullable(app.getLunchId())
.map(lunchMap::get).map(LunchJpaEntity::getName).orElse("신청 안 함"),
Optional.ofNullable(app.getExamNumber()).orElse(""),
subjectsByApp.getOrDefault(app.getId(), Set.of())
))
.toList();This removes per‐entity findById and single‐ID subject queries, collapsing them into three database hits.
🤖 Prompt for AI Agents
In
src/main/java/life/mosu/mosuserver/application/application/ApplicationService.java
lines 102 to 141, the current code performs N+1 queries by fetching Exam, Lunch,
and Subject entities individually inside a stream map operation. To fix this,
add a repository method in ExamSubjectJpaRepository to fetch subjects by a
collection of application IDs. Then, batch-fetch all Exams, Lunches, and
Subjects upfront using findAllById and the new method, storing them in maps
keyed by their IDs. Finally, refactor the stream mapping to build
ExamWithSubjects DTOs by looking up these pre-fetched entities from the maps,
eliminating per-entity queries and reducing database hits to just three batch
queries.
…uotas and remove unused SchoolQuotaCacheManager
There was a problem hiding this comment.
Actionable comments posted: 9
♻️ Duplicate comments (2)
src/main/java/life/mosu/mosuserver/application/exam/ExamService.java (2)
22-35: Add @transactional to ensure data consistency.The register method saves an exam, updates cache, and publishes an event. Without transaction management, if any operation fails, the system could be left in an inconsistent state.
+import org.springframework.transaction.annotation.Transactional; +@Transactional public void register(ExamRequest request) { ExamJpaEntity exam = request.toEntity(); ExamJpaEntity savedExam = examJpaRepository.save(exam); examQuotaCacheManager.addSchoolCurrentApplicationCount( savedExam.getSchoolName(), 0L ); examQuotaCacheManager.addSchoolMaxCapacity( savedExam.getSchoolName(), savedExam.getCapacity() ); createLunch(request.toCreateLunchEvent(savedExam.getId())); }
37-41: Add error handling for invalid area names.The
Area.from()method might throw an exception for invalid area names. Consider adding proper error handling.public List<ExamResponse> getByArea(String areaName) { + if (areaName == null || areaName.isBlank()) { + throw new IllegalArgumentException("Area name cannot be null or empty"); + } Area area = Area.from(areaName); List<ExamJpaEntity> foundExams = examJpaRepository.findByArea(area); return ExamResponse.fromList(foundExams); }
🧹 Nitpick comments (2)
src/main/java/life/mosu/mosuserver/application/exam/ExamQuotaCacheManager.java (1)
16-17: Consider updating Redis key prefixes to reflect the exam-centric model.The Redis keys still use the "school:" prefix, which may be confusing now that the system has transitioned to an exam-centric model. Consider updating these to be more descriptive of the current domain.
- private static final String REDIS_KEY_SCHOOL_MAX_CAPACITY = "school:max_capacity:"; - private static final String REDIS_KEY_SCHOOL_CURRENT_APPLICATIONS = "school:current_applications:"; + private static final String REDIS_KEY_SCHOOL_MAX_CAPACITY = "exam:school:max_capacity:"; + private static final String REDIS_KEY_SCHOOL_CURRENT_APPLICATIONS = "exam:school:current_applications:";src/main/java/life/mosu/mosuserver/domain/exam/ExamJpaRepository.java (1)
36-45: Consider adding database indexes for performance.The query groups by school name and counts applications, which could be expensive without proper indexing.
Consider adding database indexes on:
ExamApplicationJpaEntity.examId(for the JOIN)ExamJpaEntity.schoolName(for the GROUP BY)This will significantly improve query performance as the number of applications grows.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
build.gradle(1 hunks)src/main/java/life/mosu/mosuserver/MosuServerApplication.java(0 hunks)src/main/java/life/mosu/mosuserver/application/application/ApplicationService.java(2 hunks)src/main/java/life/mosu/mosuserver/application/exam/ExamQuotaCacheManager.java(1 hunks)src/main/java/life/mosu/mosuserver/application/exam/ExamService.java(1 hunks)src/main/java/life/mosu/mosuserver/application/notify/NotifyVariableFactory.java(3 hunks)src/main/java/life/mosu/mosuserver/application/payment/PaymentService.java(3 hunks)src/main/java/life/mosu/mosuserver/domain/exam/ExamInfoProjection.java(1 hunks)src/main/java/life/mosu/mosuserver/domain/exam/ExamJpaRepository.java(1 hunks)src/main/java/life/mosu/mosuserver/domain/exam/SchoolExamCountProjection.java(1 hunks)src/main/java/life/mosu/mosuserver/global/runner/ApplicationSchoolPreWarmRunner.java(2 hunks)src/main/java/life/mosu/mosuserver/presentation/admin/AdminController.java(1 hunks)src/main/java/life/mosu/mosuserver/presentation/application/dto/ApplicationSchoolRequest.java(0 hunks)src/main/java/life/mosu/mosuserver/presentation/payment/dto/PreparePaymentRequest.java(1 hunks)
💤 Files with no reviewable changes (2)
- src/main/java/life/mosu/mosuserver/MosuServerApplication.java
- src/main/java/life/mosu/mosuserver/presentation/application/dto/ApplicationSchoolRequest.java
✅ Files skipped from review due to trivial changes (3)
- build.gradle
- src/main/java/life/mosu/mosuserver/domain/exam/SchoolExamCountProjection.java
- src/main/java/life/mosu/mosuserver/domain/exam/ExamInfoProjection.java
🚧 Files skipped from review as they are similar to previous changes (4)
- src/main/java/life/mosu/mosuserver/presentation/admin/AdminController.java
- src/main/java/life/mosu/mosuserver/application/payment/PaymentService.java
- src/main/java/life/mosu/mosuserver/global/runner/ApplicationSchoolPreWarmRunner.java
- src/main/java/life/mosu/mosuserver/application/application/ApplicationService.java
🔇 Additional comments (2)
src/main/java/life/mosu/mosuserver/presentation/payment/dto/PreparePaymentRequest.java (1)
3-7: LGTM on the architectural simplification.The simplified record structure aligns well with the exam-centric refactoring described in the PR objectives. Removing the complex
itemsstructure andgetSize()method reduces complexity and supports the new domain model effectively.src/main/java/life/mosu/mosuserver/application/exam/ExamQuotaCacheManager.java (1)
42-44: Capacity parameter type is consistent with the domain modelThe
ExamJpaEntity.capacityfield is defined as anInteger(seesrc/main/java/life/mosu/mosuserver/domain/exam/ExamJpaEntity.java), so changingaddSchoolMaxCapacityto accept anIntegermatches the domain. The internal conversion toLongis solely for Redis storage, which expects aLongvalue. No API inconsistency—this review comment can be ignored.Likely an incorrect or invalid review comment.
| public record PreparePaymentRequest( | ||
| @NotEmpty(message = "결제 항목은 비어 있을 수 없습니다.") | ||
| List<@Valid Item> items | ||
| Long applicationId | ||
| ) { |
There was a problem hiding this comment.
Add validation for the applicationId field.
The applicationId field lacks validation annotations, which could lead to runtime issues if null values are passed. Consider adding appropriate validation.
+import jakarta.validation.constraints.NotNull;
+
public record PreparePaymentRequest(
+ @NotNull(message = "Application ID cannot be null")
Long applicationId
) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public record PreparePaymentRequest( | |
| @NotEmpty(message = "결제 항목은 비어 있을 수 없습니다.") | |
| List<@Valid Item> items | |
| Long applicationId | |
| ) { | |
| import jakarta.validation.constraints.NotNull; | |
| public record PreparePaymentRequest( | |
| @NotNull(message = "Application ID cannot be null") | |
| Long applicationId | |
| ) { | |
| } |
🤖 Prompt for AI Agents
In
src/main/java/life/mosu/mosuserver/presentation/payment/dto/PreparePaymentRequest.java
around lines 3 to 5, the applicationId field lacks validation annotations. Add a
validation annotation such as @NotNull to the applicationId field to ensure it
is not null when the record is instantiated, preventing potential runtime
issues.
💡 Verification agent
🧩 Analysis chain
Verify that the simplified structure supports the new exam-centric model.
The significant simplification from a complex items structure to a single applicationId aligns with the architectural shift to an exam-centric model. However, ensure that this change doesn't inadvertently remove necessary functionality for handling multiple exam applications or payment scenarios.
🏁 Script executed:
#!/bin/bash
# Description: Verify how the simplified PreparePaymentRequest is used in the payment flow
# and ensure it adequately supports the exam-centric model
# Search for usage of PreparePaymentRequest in payment-related services
echo "=== Checking PreparePaymentRequest usage in payment services ==="
rg -A 10 -B 5 "PreparePaymentRequest" --type java
echo -e "\n=== Checking payment preparation logic ==="
rg -A 15 -B 5 "preparePayment\|PreparePayment" --type java
echo -e "\n=== Checking for any remaining school-based payment logic ==="
rg -A 5 -B 5 "school.*payment\|payment.*school" --type java -iLength of output: 9258
Implement exam-centric prepare logic in PaymentService
The DTO now only carries applicationId, but the service still ignores it and relies on legacy, commented-out size-based logic. To complete the exam-centric shift:
- In
src/main/java/life/mosu/mosuserver/application/payment/PaymentService.java
• Replace the commented lineswith code that:// int applicationCount = request.getSize(); // int totalAmount = DiscountPolicy.calculate(FIXED_QUANTITY, applicationCount);
1. Loads the application byrequest.applicationId(e.g., viaApplicationRepository).
2. Determines participant count or exam details from that entity.
3. Calculates the correcttotalAmount(using your discount policy). - Remove or resolve the
//TODO: 신청서 필요함.inPaymentWidgetController—ensure the controller and response include any new data needed for the frontend. - If the exam-centric model must support batch payments (multiple applications at once), decide whether to extend
PreparePaymentRequest(e.g., list of IDs) or handle that at a higher level.
| List<ExamJpaEntity> exams = examJpaRepository.findUpcomingExamInfo(LocalDate.now()); | ||
| for (ExamJpaEntity exam : exams) { | ||
| addSchoolMaxCapacity( | ||
| exam.getSchoolName(), | ||
| exam.getCapacity() | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify the new repository method implementations.
The caching logic now depends on new repository methods. Ensure these methods are properly implemented and tested.
🏁 Script executed:
#!/bin/bash
# Description: Verify the new ExamJpaRepository methods are properly defined
# Search for the findUpcomingExamInfo method implementation
ast-grep --pattern 'findUpcomingExamInfo($$$)'
# Search for SchoolExamCountProjection usage
rg -A 5 "SchoolExamCountProjection" --type javaLength of output: 3271
Missing findUpcomingExamInfo in ExamJpaRepository – implementation required
The cache manager now calls
examJpaRepository.findUpcomingExamInfo(LocalDate.now());but no such method is defined in
src/main/java/life/mosu/mosuserver/domain/exam/ExamJpaRepository.java. You’ll need to:
• Declare the repository method with the correct signature and query, for example:
@Query("SELECT e FROM ExamJpaEntity e WHERE e.scheduledDate >= :date")
List<ExamJpaEntity> findUpcomingExamInfo(LocalDate date);• Add corresponding unit or integration tests to cover this new method.
Once added and tested, the caching logic will function as expected.
🤖 Prompt for AI Agents
In src/main/java/life/mosu/mosuserver/domain/exam/ExamJpaRepository.java, you
need to declare the method findUpcomingExamInfo with the signature
List<ExamJpaEntity> findUpcomingExamInfo(LocalDate date) and annotate it with
@Query("SELECT e FROM ExamJpaEntity e WHERE e.scheduledDate >= :date") to fetch
exams scheduled from the given date onward. After adding this method, create
appropriate unit or integration tests to verify its behavior. This will ensure
the ExamQuotaCacheManager's call to findUpcomingExamInfo(LocalDate.now()) works
correctly.
| projection.schoolName()); | ||
| ExamInfoProjection examInfo = examJpaRepository.findExamInfo(targetId) | ||
| .orElseThrow( | ||
| () -> new CustomRuntimeException(ErrorCode.APPLICATION_SCHOOL_NOT_FOUND)); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update error codes to match the new domain model.
The error code APPLICATION_SCHOOL_NOT_FOUND is inconsistent with the exam-centric model and may be confusing.
- () -> new CustomRuntimeException(ErrorCode.APPLICATION_SCHOOL_NOT_FOUND));
+ () -> new CustomRuntimeException(ErrorCode.EXAM_NOT_FOUND));This change should be applied to all three exam notification methods (lines 74, 84, 94).
Also applies to: 84-84, 94-94
🤖 Prompt for AI Agents
In
src/main/java/life/mosu/mosuserver/application/notify/NotifyVariableFactory.java
at lines 74, 84, and 94, the error code APPLICATION_SCHOOL_NOT_FOUND is outdated
and inconsistent with the exam-centric domain model. Replace this error code in
all three exam notification methods with a new, appropriate error code that
reflects the exam context to maintain clarity and consistency.
| private NotificationVariable createApplicationVariable(Long targetId) { | ||
| ApplicationSchoolNotifyProjection projection = applicationSchoolJpaRepository.findPaymentByApplicationSchoolId( | ||
| targetId); | ||
| return ApplicationNotifyRequest.from(projection); | ||
| // ApplicationSchoolNotifyProjection projection = applicationSchoolJpaRepository.findPaymentByApplicationSchoolId( | ||
| // targetId); | ||
| // return ApplicationNotifyRequest.from(projection); | ||
| return null; | ||
| } |
There was a problem hiding this comment.
Critical: Application notification functionality is disabled.
Similar to the refund notification, this method returns null which will break the notification system.
private NotificationVariable createApplicationVariable(Long targetId) {
-// ApplicationSchoolNotifyProjection projection = applicationSchoolJpaRepository.findPaymentByApplicationSchoolId(
-// targetId);
-// return ApplicationNotifyRequest.from(projection);
- return null;
+ // TODO: Implement exam-based application notification logic
+ throw new UnsupportedOperationException("Application notifications not yet implemented for exam-centric model");
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private NotificationVariable createApplicationVariable(Long targetId) { | |
| ApplicationSchoolNotifyProjection projection = applicationSchoolJpaRepository.findPaymentByApplicationSchoolId( | |
| targetId); | |
| return ApplicationNotifyRequest.from(projection); | |
| // ApplicationSchoolNotifyProjection projection = applicationSchoolJpaRepository.findPaymentByApplicationSchoolId( | |
| // targetId); | |
| // return ApplicationNotifyRequest.from(projection); | |
| return null; | |
| } | |
| private NotificationVariable createApplicationVariable(Long targetId) { | |
| // TODO: Implement exam-based application notification logic | |
| throw new UnsupportedOperationException("Application notifications not yet implemented for exam-centric model"); | |
| } |
🤖 Prompt for AI Agents
In
src/main/java/life/mosu/mosuserver/application/notify/NotifyVariableFactory.java
between lines 64 and 69, the createApplicationVariable method currently returns
null, disabling application notification functionality and causing system
failures. To fix this, uncomment and properly implement the logic to fetch the
ApplicationSchoolNotifyProjection using
applicationSchoolJpaRepository.findPaymentByApplicationSchoolId with the
targetId, then convert it to a NotificationVariable by returning
ApplicationNotifyRequest.from(projection). Ensure the repository call and
conversion are correctly handled to restore notification functionality.
| private NotificationVariable createRefundVariable(Long targetId) { | ||
| RefundNotifyProjection projection = refundJpaRepository.findRefundByApplicationSchoolId( | ||
| targetId) | ||
| .orElseThrow( | ||
| () -> new CustomRuntimeException(ErrorCode.APPLICATION_SCHOOL_NOT_FOUND)); | ||
| return new RefundNotifyRequest( | ||
| projection.paymentKey(), projection.examDate(), projection.schoolName(), | ||
| projection.paymentMethod().getName(), projection.reason() | ||
| ); | ||
| // RefundNotifyProjection projection = refundJpaRepository.findRefundByApplicationSchoolId( | ||
| // targetId) | ||
| // .orElseThrow( | ||
| // () -> new CustomRuntimeException(ErrorCode.APPLICATION_SCHOOL_NOT_FOUND)); | ||
| // return new RefundNotifyRequest( | ||
| // projection.paymentKey(), projection.examDate(), projection.schoolName(), | ||
| // projection.paymentMethod().getName(), projection.reason() | ||
| // ); | ||
| return null; | ||
| } |
There was a problem hiding this comment.
Critical: Commented out notification functionality will break the system.
The refund notification method is commented out and returns null, which will likely cause null pointer exceptions in the notification system.
This needs immediate attention. Either:
- Implement the new exam-based refund notification logic
- Remove the REFUND_SUCCESS case from the switch statement in the
createmethod - Add proper null handling in the notification system
private NotificationVariable createRefundVariable(Long targetId) {
-// RefundNotifyProjection projection = refundJpaRepository.findRefundByApplicationSchoolId(
-// targetId)
-// .orElseThrow(
-// () -> new CustomRuntimeException(ErrorCode.APPLICATION_SCHOOL_NOT_FOUND));
-// return new RefundNotifyRequest(
-// projection.paymentKey(), projection.examDate(), projection.schoolName(),
-// projection.paymentMethod().getName(), projection.reason()
-// );
- return null;
+ // TODO: Implement exam-based refund notification logic
+ throw new UnsupportedOperationException("Refund notifications not yet implemented for exam-centric model");
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private NotificationVariable createRefundVariable(Long targetId) { | |
| RefundNotifyProjection projection = refundJpaRepository.findRefundByApplicationSchoolId( | |
| targetId) | |
| .orElseThrow( | |
| () -> new CustomRuntimeException(ErrorCode.APPLICATION_SCHOOL_NOT_FOUND)); | |
| return new RefundNotifyRequest( | |
| projection.paymentKey(), projection.examDate(), projection.schoolName(), | |
| projection.paymentMethod().getName(), projection.reason() | |
| ); | |
| // RefundNotifyProjection projection = refundJpaRepository.findRefundByApplicationSchoolId( | |
| // targetId) | |
| // .orElseThrow( | |
| // () -> new CustomRuntimeException(ErrorCode.APPLICATION_SCHOOL_NOT_FOUND)); | |
| // return new RefundNotifyRequest( | |
| // projection.paymentKey(), projection.examDate(), projection.schoolName(), | |
| // projection.paymentMethod().getName(), projection.reason() | |
| // ); | |
| return null; | |
| } | |
| private NotificationVariable createRefundVariable(Long targetId) { | |
| // TODO: Implement exam-based refund notification logic | |
| throw new UnsupportedOperationException("Refund notifications not yet implemented for exam-centric model"); | |
| } |
🤖 Prompt for AI Agents
In
src/main/java/life/mosu/mosuserver/application/notify/NotifyVariableFactory.java
around lines 52 to 62, the createRefundVariable method is commented out and
returns null, which will cause null pointer exceptions in the notification
system. To fix this, either fully implement the refund notification logic based
on the exam data as intended, or if that is not ready, remove the REFUND_SUCCESS
case from the switch statement in the create method to prevent calling this
method. Alternatively, ensure the notification system safely handles null values
returned from this method to avoid runtime errors.
| @@ -0,0 +1,47 @@ | |||
| package life.mosu.mosuserver.domain.exam; | |||
|
|
|||
| import io.lettuce.core.dynamic.annotation.Param; | |||
There was a problem hiding this comment.
Fix incorrect import for @param annotation.
You're importing @Param from io.lettuce.core.dynamic.annotation.Param instead of the Spring Data JPA annotation.
-import io.lettuce.core.dynamic.annotation.Param;
+import org.springframework.data.repository.query.Param;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import io.lettuce.core.dynamic.annotation.Param; | |
| -import io.lettuce.core.dynamic.annotation.Param; | |
| +import org.springframework.data.repository.query.Param; |
🤖 Prompt for AI Agents
In src/main/java/life/mosu/mosuserver/domain/exam/ExamJpaRepository.java at line
3, the import statement for the @Param annotation is incorrect; it imports from
io.lettuce.core.dynamic.annotation.Param instead of the correct Spring Data JPA
package. Replace this import with
org.springframework.data.repository.query.Param to fix the issue.
| @Query(""" | ||
| SELECT new life.mosu.mosuserver.domain.exam.ExamInfoProjection( | ||
| e.examDate, | ||
| ea.examNumber, | ||
| e.schoolName | ||
| ) | ||
| FROM ExamApplicationJpaEntity ea | ||
| JOIN ExamJpaEntity e ON ea.examId = e.id | ||
| WHERE ea.examId = :examId | ||
| """) | ||
| Optional<ExamInfoProjection> findExamInfo(@Param("examId") Long examId); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify the JPQL query structure and performance.
The query joins ExamApplicationJpaEntity with ExamJpaEntity but the relationship and performance implications should be verified.
🏁 Script executed:
#!/bin/bash
# Description: Verify the entity relationships and potential performance issues
# Check the ExamApplicationJpaEntity structure and relationships
ast-grep --pattern 'class ExamApplicationJpaEntity {
$$$
}'
# Check for any @JoinColumn or relationship annotations
rg -A 3 -B 3 "@JoinColumn|@ManyToOne|@OneToMany" --type java src/main/java/life/mosu/mosuserver/domain/exam/Length of output: 174
🏁 Script executed:
#!/bin/bash
# Locate the entity Java files in the repo
rg -l "class ExamApplicationJpaEntity" -g "*.java"
rg -l "class ExamJpaEntity" -g "*.java"
# Once we have the path to ExamApplicationJpaEntity, check for relationship mappings
rg -n "@ManyToOne|@JoinColumn|ExamJpaEntity" -g "*ExamApplicationJpaEntity.java"Length of output: 331
🏁 Script executed:
#!/bin/bash
# Display the top of the entity to inspect mappings and fields
sed -n '1,200p' src/main/java/life/mosu/mosuserver/domain/examapplication/ExamApplicationJpaEntity.java
# Specifically search for the examId field and any relationship annotation
rg -n "examId" -g "src/main/java/life/mosu/mosuserver/domain/examapplication/ExamApplicationJpaEntity.java"
rg -n "@ManyToOne|@JoinColumn" -g "src/main/java/life/mosu/mosuserver/domain/examapplication/ExamApplicationJpaEntity.java"Length of output: 2522
Fix JPQL join on non‐associated entities & optimize foreign‐key indexing
-
JPA JPQL doesn’t support
JOIN … ONbetween unmapped entities. You have two paths:
• Map the association inExamApplicationJpaEntityand join on it:
– Add a@ManyToOne @JoinColumn(name="exam_id") private ExamJpaEntity exam;
– Change the repository query to
java @Query(""" SELECT new life.mosu.mosuserver.domain.exam.ExamInfoProjection( e.examDate, ea.examNumber, e.schoolName ) FROM ExamApplicationJpaEntity ea JOIN ea.exam e WHERE e.id = :examId """) Optional<ExamInfoProjection> findExamInfo(@Param("examId") Long examId);
• Or use an implicit theta-join withoutON:@Query(""" SELECT new life.mosu.mosuserver.domain.exam.ExamInfoProjection( e.examDate, ea.examNumber, e.schoolName ) FROM ExamApplicationJpaEntity ea, ExamJpaEntity e WHERE ea.examId = e.id AND ea.examId = :examId """) Optional<ExamInfoProjection> findExamInfo(@Param("examId") Long examId);
-
Add a database index on the foreign-key column to speed up the join:
@Table( name = "exam_application", indexes = @Index(name = "idx_exam_application_exam_id", columnList = "exam_id") )
Affected files:
- src/main/java/life/mosu/mosuserver/domain/examapplication/ExamApplicationJpaEntity.java
- src/main/java/life/mosu/mosuserver/domain/exam/ExamJpaRepository.java
🤖 Prompt for AI Agents
In src/main/java/life/mosu/mosuserver/domain/exam/ExamJpaRepository.java lines
17 to 27, the JPQL query uses a JOIN ON between unmapped entities, which is
unsupported. Fix this by either mapping a @ManyToOne association in
ExamApplicationJpaEntity to ExamJpaEntity and updating the query to join via the
mapped association, or rewrite the query as a theta-join without ON using a
comma-separated FROM clause and a WHERE condition for the join. Additionally, in
src/main/java/life/mosu/mosuserver/domain/examapplication/ExamApplicationJpaEntity.java,
add a database index annotation on the foreign key column exam_id to optimize
join performance.
| examQuotaCacheManager.addSchoolCurrentApplicationCount( | ||
| savedExam.getSchoolName(), | ||
| 0L | ||
| ); | ||
| examQuotaCacheManager.addSchoolMaxCapacity( | ||
| savedExam.getSchoolName(), | ||
| savedExam.getCapacity() | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider moving cache updates to an event listener.
Manual cache updates in the service method create tight coupling and potential consistency issues. Consider handling cache updates through application events.
public void register(ExamRequest request) {
ExamJpaEntity exam = request.toEntity();
ExamJpaEntity savedExam = examJpaRepository.save(exam);
- examQuotaCacheManager.addSchoolCurrentApplicationCount(
- savedExam.getSchoolName(),
- 0L
- );
- examQuotaCacheManager.addSchoolMaxCapacity(
- savedExam.getSchoolName(),
- savedExam.getCapacity()
- );
+ publisher.publishEvent(new ExamRegisteredEvent(savedExam));
createLunch(request.toCreateLunchEvent(savedExam.getId()));
}This would require creating an ExamRegisteredEvent and corresponding event listener for cache management.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/main/java/life/mosu/mosuserver/application/exam/ExamService.java around
lines 26 to 33, the cache updates for school application count and max capacity
are done manually in the service method, causing tight coupling and potential
consistency issues. Refactor by creating an ExamRegisteredEvent that carries the
necessary exam data, then implement an event listener that listens for this
event and performs the cache updates. Remove the direct cache update calls from
the service method and instead publish the event after saving the exam.
…exam ticket retrieval; add ExamApplicationController and UpdateSubjectRequest for handling requests
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (3)
src/main/java/life/mosu/mosuserver/domain/examapplication/ExamSubjectJpaRepository.java (1)
6-11: Consider adding @Modifying annotation to the delete methodWhile Spring Data JPA will handle the transactional aspect, it's a best practice to add the
@Modifyingannotation to delete operations for clarity and to ensure proper flush mode handling.+import org.springframework.data.jpa.repository.Modifying; + public interface ExamSubjectJpaRepository extends JpaRepository<ExamSubjectJpaEntity, Long> { List<ExamSubjectJpaEntity> findByExamApplicationId(Long examApplicationId); + @Modifying void deleteByExamApplicationId(Long examApplicationId); }src/main/java/life/mosu/mosuserver/application/examapplication/ExamApplicationService.java (1)
63-65: Use method reference for cleaner codeThe lambda expression can be replaced with a method reference for better readability.
List<Subject> subjects = examSubjects.stream() - .map(es -> es.getSubject()) + .map(ExamSubjectJpaEntity::getSubject) .toList();src/main/java/life/mosu/mosuserver/presentation/examapplication/ExamApplicationController.java (1)
18-21: Add validation annotation to the controllerConsider adding
@Validatedannotation to enable validation of request bodies.+import org.springframework.validation.annotation.Validated; + @RestController @RequestMapping("/exam-application") @RequiredArgsConstructor +@Validated public class ExamApplicationController {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/main/java/life/mosu/mosuserver/application/examapplication/ExamApplicationService.java(1 hunks)src/main/java/life/mosu/mosuserver/domain/examapplication/ExamApplicationJpaRepository.java(1 hunks)src/main/java/life/mosu/mosuserver/domain/examapplication/ExamSubjectJpaRepository.java(1 hunks)src/main/java/life/mosu/mosuserver/global/exception/ErrorCode.java(2 hunks)src/main/java/life/mosu/mosuserver/presentation/admin/dto/ExamTicketResponse.java(2 hunks)src/main/java/life/mosu/mosuserver/presentation/examapplication/ExamApplicationController.java(1 hunks)src/main/java/life/mosu/mosuserver/presentation/examapplication/dto/UpdateSubjectRequest.java(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/main/java/life/mosu/mosuserver/domain/examapplication/ExamApplicationJpaRepository.java
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/java/life/mosu/mosuserver/presentation/admin/dto/ExamTicketResponse.java
- src/main/java/life/mosu/mosuserver/global/exception/ErrorCode.java
| @Transactional | ||
| public void updateSubjects(Long examApplicationId, UpdateSubjectRequest request) { | ||
| examSubjectJpaRepository.deleteByExamApplicationId(examApplicationId); | ||
| List<ExamSubjectJpaEntity> examSubjects = request.toEntityList(examApplicationId); | ||
| examSubjectJpaRepository.saveAll(examSubjects); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider race condition in subject updates
The current implementation deletes all subjects before saving new ones. This could lead to a race condition where subjects are temporarily missing. Consider using a single transaction with proper locking.
You might want to:
- Load existing subjects
- Compare and only update what's changed
- Or use
@Lockannotation for pessimistic locking if concurrent updates are expected
🤖 Prompt for AI Agents
In
src/main/java/life/mosu/mosuserver/application/examapplication/ExamApplicationService.java
around lines 46 to 51, the current updateSubjects method deletes all subjects
before saving new ones, risking a race condition with temporary data loss. To
fix this, modify the method to first load existing subjects for the given
examApplicationId, compare them with the incoming request data, and update only
the changed subjects instead of deleting all. Additionally, apply a pessimistic
lock using the @Lock annotation on the repository method that loads existing
subjects to prevent concurrent modifications during the update process.
| public ExamTicketResponse getExamTicket(Long userId, Long examApplicationId) { | ||
| ExamApplicationJpaEntity examApplication = examApplicationJpaRepository.findById( | ||
| examApplicationId) | ||
| .orElseThrow( | ||
| () -> new CustomRuntimeException(ErrorCode.EXAM_APPLICATION_NOT_FOUND)); | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Add validation to ensure exam application belongs to the user
The method accepts a userId parameter but doesn't validate that the exam application belongs to that user. This could be a security issue allowing users to access other users' exam tickets.
🏁 Script executed:
#!/bin/bash
# Description: Check if ExamApplicationJpaEntity has a userId field or relationship to verify ownership
# Search for ExamApplicationJpaEntity class definition to check for user relationship
ast-grep --pattern 'class ExamApplicationJpaEntity {
$$$
}'
# Also search for any userId or user-related fields
rg -A 10 "class ExamApplicationJpaEntity" | rg -i "user"Length of output: 1340
Ensure ExamApplication is Scoped to the Authenticated User
Currently, getExamTicket(Long userId, Long examApplicationId) fetches the application by ID only. Because ExamApplicationJpaEntity has no reference to its owner, you must prevent unauthorized access by:
• Adding a user_id column and mapping (e.g. @manytoone or simple @column) to ExamApplicationJpaEntity
• Updating the repository to expose a lookup by both user and application ID:
Optional<ExamApplicationJpaEntity>
findByUserIdAndApplicationId(Long userId, Long applicationId);• Changing the service call to:
ExamApplicationJpaEntity examApp =
examApplicationJpaRepository
.findByUserIdAndApplicationId(userId, examApplicationId)
.orElseThrow(() ->
new CustomRuntimeException(EXAM_APPLICATION_NOT_FOUND)
);These changes enforce that users can only retrieve their own exam tickets.
🤖 Prompt for AI Agents
In
src/main/java/life/mosu/mosuserver/application/examapplication/ExamApplicationService.java
around lines 54 to 59, the method getExamTicket fetches ExamApplicationJpaEntity
by examApplicationId only, allowing potential unauthorized access. To fix this,
add a user_id field to ExamApplicationJpaEntity with appropriate mapping, update
the repository to include a method findByUserIdAndApplicationId(Long userId,
Long applicationId), and modify the service method to use this new repository
method to ensure the exam application belongs to the authenticated user before
returning it.
| Long applicationId = examApplication.getApplicationId(); | ||
| ExamTicketImageJpaEntity examTicketImage = examTicketImageJpaRepository.findByApplicationId( | ||
| applicationId); | ||
| ; |
There was a problem hiding this comment.
Remove extra semicolon - syntax error
There's an unnecessary semicolon after the statement that will cause a compilation error.
ExamTicketImageJpaEntity examTicketImage = examTicketImageJpaRepository.findByApplicationId(
applicationId);
- ;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ; | |
| ExamTicketImageJpaEntity examTicketImage = examTicketImageJpaRepository.findByApplicationId( | |
| applicationId); |
🤖 Prompt for AI Agents
In
src/main/java/life/mosu/mosuserver/application/examapplication/ExamApplicationService.java
at line 70, remove the extra semicolon that is standing alone after a statement,
as it causes a syntax error and prevents compilation.
| public record UpdateSubjectRequest( | ||
| List<String> subjects | ||
| ) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding validation annotations
The record should validate that the subjects list is not null or empty to prevent potential NPEs or invalid states.
+import jakarta.validation.constraints.NotEmpty;
+import jakarta.validation.constraints.NotNull;
+
public record UpdateSubjectRequest(
+ @NotNull(message = "Subjects list cannot be null")
+ @NotEmpty(message = "Subjects list cannot be empty")
List<String> subjects
) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public record UpdateSubjectRequest( | |
| List<String> subjects | |
| ) { | |
| import jakarta.validation.constraints.NotEmpty; | |
| import jakarta.validation.constraints.NotNull; | |
| public record UpdateSubjectRequest( | |
| @NotNull(message = "Subjects list cannot be null") | |
| @NotEmpty(message = "Subjects list cannot be empty") | |
| List<String> subjects | |
| ) { |
🤖 Prompt for AI Agents
In
src/main/java/life/mosu/mosuserver/presentation/examapplication/dto/UpdateSubjectRequest.java
around lines 7 to 9, the subjects list in the record lacks validation, which can
lead to null or empty values causing errors. Add validation annotations such as
@NotNull and @NotEmpty to the subjects field to ensure it is neither null nor
empty, preventing potential null pointer exceptions or invalid states.
| private List<Subject> validatedSubject() { | ||
| return subjects.stream() | ||
| .map(Subject::valueOf) | ||
| .toList(); | ||
| } |
There was a problem hiding this comment.
Add validation for invalid subject names
The Subject::valueOf method will throw an IllegalArgumentException if the subject string doesn't match any enum value. This should be handled gracefully with proper error messages.
private List<Subject> validatedSubject() {
return subjects.stream()
- .map(Subject::valueOf)
+ .map(subjectName -> {
+ try {
+ return Subject.valueOf(subjectName);
+ } catch (IllegalArgumentException e) {
+ throw new CustomRuntimeException(
+ ErrorCode.INVALID_SUBJECT_NAME,
+ "Invalid subject name: " + subjectName
+ );
+ }
+ })
.toList();
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private List<Subject> validatedSubject() { | |
| return subjects.stream() | |
| .map(Subject::valueOf) | |
| .toList(); | |
| } | |
| private List<Subject> validatedSubject() { | |
| return subjects.stream() | |
| - .map(Subject::valueOf) | |
| + .map(subjectName -> { | |
| + try { | |
| + return Subject.valueOf(subjectName); | |
| + } catch (IllegalArgumentException e) { | |
| + throw new CustomRuntimeException( | |
| + ErrorCode.INVALID_SUBJECT_NAME, | |
| + "Invalid subject name: " + subjectName | |
| + ); | |
| + } | |
| + }) | |
| .toList(); | |
| } |
🤖 Prompt for AI Agents
In
src/main/java/life/mosu/mosuserver/presentation/examapplication/dto/UpdateSubjectRequest.java
between lines 11 and 15, the validatedSubject method currently calls
Subject::valueOf directly, which throws IllegalArgumentException for invalid
subject names. Modify the method to catch this exception for each subject
string, handle it gracefully by either collecting validation errors or throwing
a custom exception with a clear error message indicating which subject names are
invalid, instead of letting the exception propagate unhandled.
| @PutMapping("{examApplicationId}/subjects") | ||
| public ResponseEntity<ApiResponseWrapper<Void>> updateSubjects( | ||
| @PathVariable("examApplicationId") Long examApplicationId, | ||
| @RequestBody UpdateSubjectRequest request | ||
| ) { | ||
| examApplicationService.updateSubjects(examApplicationId, request); | ||
| return ResponseEntity.ok(ApiResponseWrapper.success(HttpStatus.OK, "과목 수정을 완료했습니다.")); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation annotation to request body
To ensure the validation annotations in UpdateSubjectRequest are triggered, add @Valid annotation.
+import jakarta.validation.Valid;
+
@PutMapping("{examApplicationId}/subjects")
public ResponseEntity<ApiResponseWrapper<Void>> updateSubjects(
@PathVariable("examApplicationId") Long examApplicationId,
- @RequestBody UpdateSubjectRequest request
+ @RequestBody @Valid UpdateSubjectRequest request
) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @PutMapping("{examApplicationId}/subjects") | |
| public ResponseEntity<ApiResponseWrapper<Void>> updateSubjects( | |
| @PathVariable("examApplicationId") Long examApplicationId, | |
| @RequestBody UpdateSubjectRequest request | |
| ) { | |
| examApplicationService.updateSubjects(examApplicationId, request); | |
| return ResponseEntity.ok(ApiResponseWrapper.success(HttpStatus.OK, "과목 수정을 완료했습니다.")); | |
| } | |
| import jakarta.validation.Valid; | |
| @PutMapping("{examApplicationId}/subjects") | |
| public ResponseEntity<ApiResponseWrapper<Void>> updateSubjects( | |
| @PathVariable("examApplicationId") Long examApplicationId, | |
| @RequestBody @Valid UpdateSubjectRequest request | |
| ) { | |
| examApplicationService.updateSubjects(examApplicationId, request); | |
| return ResponseEntity.ok(ApiResponseWrapper.success(HttpStatus.OK, "과목 수정을 완료했습니다.")); | |
| } |
🤖 Prompt for AI Agents
In
src/main/java/life/mosu/mosuserver/presentation/examapplication/ExamApplicationController.java
around lines 25 to 32, the updateSubjects method's request body parameter lacks
the @Valid annotation, so validation annotations in UpdateSubjectRequest are not
triggered. Add the @Valid annotation before the @RequestBody annotation on the
request parameter to enable validation processing.
| @GetMapping("{examApplicationId}/exam-ticket") | ||
| public ResponseEntity<ApiResponseWrapper<ExamTicketResponse>> getExamTicket( | ||
| @RequestParam Long userId, | ||
| @PathVariable("examApplicationId") Long examApplicationId | ||
| ) { | ||
| ExamTicketResponse response = examApplicationService.getExamTicket(userId, | ||
| examApplicationId); | ||
| return ResponseEntity.ok( | ||
| ApiResponseWrapper.success(HttpStatus.OK, "수험표 발급을 완료했습니다.", response)); | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Security concern: userId as query parameter
Passing userId as a query parameter is unusual and potentially insecure. User identification should typically come from the authenticated session/token rather than being client-provided.
Consider:
- Getting userId from the security context (e.g.,
@AuthenticationPrincipal) - If this is for admin purposes, ensure proper authorization checks are in place
🏁 Script executed:
#!/bin/bash
# Description: Check how authentication is handled in other controllers
# Search for authentication patterns in other controllers
rg -A 5 "@AuthenticationPrincipal|@PreAuthorize|SecurityContext" --glob "**/*Controller.java"
# Check if there's a security configuration
fd -e java SecurityConfig | xargs catLength of output: 18924
Enforce authentication and remove client-provided userId
The getExamTicket endpoint currently trusts a client-supplied userId query parameter, and your SecurityConfig permits all requests by default—allowing anyone to fetch any user’s exam ticket. To secure this:
• Remove the @RequestParam Long userId parameter.
• Retrieve the authenticated user’s ID from the security context (e.g. via @AuthenticationPrincipal or SecurityContextHolder).
• Restrict access to authenticated users by adding @PreAuthorize("isAuthenticated()") on the controller or changing authorizeHttpRequests to .anyRequest().authenticated().
• In examApplicationService.getExamTicket(...), verify that the requested examApplicationId belongs to the authenticated user.
• Audit other controllers that accept userId via @RequestParam (e.g. ProfileController, ApplicationController) and apply the same pattern.
Suggested diff for ExamApplicationController.java:
- @GetMapping("{examApplicationId}/exam-ticket")
- public ResponseEntity<ApiResponseWrapper<ExamTicketResponse>> getExamTicket(
- @RequestParam Long userId,
- @PathVariable("examApplicationId") Long examApplicationId
- ) {
- ExamTicketResponse response = examApplicationService.getExamTicket(userId,
- examApplicationId);
+ @GetMapping("{examApplicationId}/exam-ticket")
+ @PreAuthorize("isAuthenticated()")
+ public ResponseEntity<ApiResponseWrapper<ExamTicketResponse>> getExamTicket(
+ @AuthenticationPrincipal UserPrincipal principal,
+ @PathVariable Long examApplicationId
+ ) {
+ Long userId = principal.getId();
+ ExamTicketResponse response = examApplicationService.getExamTicket(userId, examApplicationId);
return ResponseEntity.ok(
ApiResponseWrapper.success(HttpStatus.OK, "수험표 발급을 완료했습니다.", response));
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @GetMapping("{examApplicationId}/exam-ticket") | |
| public ResponseEntity<ApiResponseWrapper<ExamTicketResponse>> getExamTicket( | |
| @RequestParam Long userId, | |
| @PathVariable("examApplicationId") Long examApplicationId | |
| ) { | |
| ExamTicketResponse response = examApplicationService.getExamTicket(userId, | |
| examApplicationId); | |
| return ResponseEntity.ok( | |
| ApiResponseWrapper.success(HttpStatus.OK, "수험표 발급을 완료했습니다.", response)); | |
| } | |
| @GetMapping("{examApplicationId}/exam-ticket") | |
| @PreAuthorize("isAuthenticated()") | |
| public ResponseEntity<ApiResponseWrapper<ExamTicketResponse>> getExamTicket( | |
| @AuthenticationPrincipal UserPrincipal principal, | |
| @PathVariable Long examApplicationId | |
| ) { | |
| Long userId = principal.getId(); | |
| ExamTicketResponse response = examApplicationService.getExamTicket(userId, examApplicationId); | |
| return ResponseEntity.ok( | |
| ApiResponseWrapper.success(HttpStatus.OK, "수험표 발급을 완료했습니다.", response)); | |
| } |
🤖 Prompt for AI Agents
In
src/main/java/life/mosu/mosuserver/presentation/examapplication/ExamApplicationController.java
around lines 34 to 43, remove the @RequestParam Long userId parameter from the
getExamTicket method and instead obtain the authenticated user's ID from the
security context using @AuthenticationPrincipal or SecurityContextHolder. Add
@PreAuthorize("isAuthenticated()") to the controller or configure SecurityConfig
to require authentication for all requests. Update
examApplicationService.getExamTicket to verify that the examApplicationId
belongs to the authenticated user. Also, review other controllers like
ProfileController and ApplicationController for similar userId parameters and
apply the same authentication enforcement pattern.
2 participants
✨ 구현한 기능
📢 논의하고 싶은 내용
🎸 기타
Summary by CodeRabbit
New Features
Improvements
Removals
Bug Fixes
Chores