MOSU-140 refactor: Application JPA 의존성 Processor로 위임#141
Conversation
WalkthroughThis update introduces a new Changes
Sequence Diagram(s)sequenceDiagram
participant Controller
participant ApplicationService
participant GetApplicationsStepProcessor
participant ApplicationRepo
participant ExamAppRepo
participant ExamRepo
participant SubjectRepo
participant PaymentRepo
Controller->>ApplicationService: getApplications(userId)
ApplicationService->>GetApplicationsStepProcessor: process(userId)
GetApplicationsStepProcessor->>ApplicationRepo: findAllByUserId(userId)
GetApplicationsStepProcessor->>ExamAppRepo: findByApplicationIds(appIds)
GetApplicationsStepProcessor->>ExamRepo: findByIds(examIds)
GetApplicationsStepProcessor->>SubjectRepo: findByExamIds(examIds)
GetApplicationsStepProcessor->>PaymentRepo: findByApplicationIds(appIds)
GetApplicationsStepProcessor-->>ApplicationService: List<ApplicationResponse>
ApplicationService-->>Controller: List<ApplicationResponse>
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~18 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches
🧪 Generate unit tests
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 @wlgns12370, 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 focuses on a significant refactoring of the application's core service layer, specifically within the application and exam management domains. The main objective is to reduce the ApplicationService's direct dependencies on JPA repositories by delegating data access and complex business logic to dedicated 'Processor' components. This change aims to improve modularity, maintainability, and testability of the codebase. Additionally, it includes important functional updates to ensure data consistency related to payment statuses and minor code quality improvements.
Highlights
- Core Refactoring: JPA Dependency Delegation: The primary goal of this PR is to refactor
ApplicationServiceby delegating its direct JPA repository dependencies and associated business logic to new or existingStepProcessorclasses. This promotes a cleaner architecture with improved separation of concerns. - New Processor for Application Retrieval: Introduced a new
GetApplicationsStepProcessorto encapsulate the logic for retrieving user applications, which was previously handled directly withinApplicationService. - Enhanced StepProcessor Interface: The generic
StepProcessorinterface has been updated fromStepProcessor<T>toStepProcessor<REQ, RES>, allowing for distinct request and response types, which improves type safety and flexibility for various processing steps. - Functional Enhancement: Payment Status Filtering: Modified several JPA queries across
ApplicationJpaRepositoryandExamApplicationJpaRepositoryto explicitly filter results based onpaymentStatus = 'DONE'. This ensures that only successfully paid applications and exam entries are considered in relevant operations, preventing issues with incomplete transactions. - Code Cleanup and Minor Refactorings: Performed general code cleanup, including the removal of commented-out code in
ApplicationRequest.java, renaming ofprovidesLunch()tohasNotLunch()inExamJpaEntity.javafor better semantic clarity, and the removal ofSaveApplicationStepProcessor.java. - Security Enhancement: Added
@PreAuthorizeannotation to thecreatemethod inInquiryController.javato enforce user authentication and role-based access control (hasRole('USER')).
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 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 or fill out our survey to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request refactors the ApplicationService by delegating JPA dependencies to processors, improving the service layer. It also generalizes the StepProcessor interface. Key changes include updated JPQL queries to check for DONE payment status and a new authorization rule in InquiryController. The review comments suggest improvements for method naming, SQL keyword capitalization, handling of empty lists, query readability, and adding explanatory comments for security annotations.
| public static ApplicationResponse empty() { | ||
| return new ApplicationResponse(null, null); | ||
| } |
There was a problem hiding this comment.
The empty() factory method returns null for the exams list. This is generally considered an anti-pattern as it can lead to NullPointerException in client code that attempts to iterate over the list. It is safer to always return an empty collection instead of null.
| public static ApplicationResponse empty() { | |
| return new ApplicationResponse(null, null); | |
| } | |
| public static ApplicationResponse empty() { | |
| return new ApplicationResponse(null, List.of()); | |
| } |
| public boolean hasNotLunch() { | ||
| return lunchName == null; | ||
| } |
There was a problem hiding this comment.
The method name hasNotLunch is a bit unconventional as it's phrased in the negative, which can sometimes be less clear to read than a positive statement. While it works with the method reference, a more idiomatic name would improve long-term maintainability. Consider renaming it to something like doesNotProvideLunch.
| public boolean hasNotLunch() { | |
| return lunchName == null; | |
| } | |
| public boolean doesNotProvideLunch() { | |
| return lunchName == null; | |
| } |
| JOIN ExamJpaEntity e on ea.examId = e.id | ||
| JOIN PaymentJpaEntity p on p.examApplicationId = ea.id |
There was a problem hiding this comment.
For consistency with other SQL keywords in this query (which are in uppercase), it's good practice to also capitalize the on keyword.
| JOIN ExamJpaEntity e on ea.examId = e.id | |
| JOIN PaymentJpaEntity p on p.examApplicationId = ea.id | |
| JOIN ExamJpaEntity e ON ea.examId = e.id | |
| JOIN PaymentJpaEntity p ON p.examApplicationId = ea.id |
|
|
||
| @PostMapping | ||
|
|
||
| @PreAuthorize("isAuthenticated() and hasRole('USER')") |
There was a problem hiding this comment.
Adding @PreAuthorize is a good security measure. Consider adding a comment explaining why this annotation is necessary and what security concern it addresses. This helps future developers understand the purpose of the annotation and avoid accidentally removing it.
| @PreAuthorize("isAuthenticated() and hasRole('USER')") | |
| @PreAuthorize("isAuthenticated() and hasRole('USER')") // Ensures only authenticated users with the 'USER' role can create inquiries |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/main/java/life/mosu/mosuserver/presentation/application/dto/ApplicationResponse.java (1)
20-22: Consider using empty collections instead of null values.The
empty()factory method follows good patterns, but returningnullvalues in DTOs can cause issues for API consumers. Consider using an empty list forexamsand potentiallyOptional<Long>forapplicationIdor a sentinel value like0L.public static ApplicationResponse empty() { - return new ApplicationResponse(null, null); + return new ApplicationResponse(null, List.of()); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
src/main/java/life/mosu/mosuserver/application/application/ApplicationService.java(3 hunks)src/main/java/life/mosu/mosuserver/application/application/processor/GetApplicationsStepProcessor.java(1 hunks)src/main/java/life/mosu/mosuserver/application/application/processor/RegisterApplicationStepProcessor.java(2 hunks)src/main/java/life/mosu/mosuserver/application/application/processor/SaveApplicationStepProcessor.java(0 hunks)src/main/java/life/mosu/mosuserver/application/application/processor/SaveExamTicketStepProcessor.java(1 hunks)src/main/java/life/mosu/mosuserver/application/application/vaildator/ApplicationValidator.java(1 hunks)src/main/java/life/mosu/mosuserver/application/auth/processor/SignUpAccountStepProcessor.java(1 hunks)src/main/java/life/mosu/mosuserver/application/auth/processor/SignUpProfileStepProcessor.java(1 hunks)src/main/java/life/mosu/mosuserver/application/inquiry/InquiryAnswerService.java(0 hunks)src/main/java/life/mosu/mosuserver/domain/application/ApplicationJpaRepository.java(1 hunks)src/main/java/life/mosu/mosuserver/domain/exam/ExamJpaEntity.java(1 hunks)src/main/java/life/mosu/mosuserver/domain/examapplication/ExamApplicationJpaRepository.java(5 hunks)src/main/java/life/mosu/mosuserver/global/processor/StepProcessor.java(1 hunks)src/main/java/life/mosu/mosuserver/presentation/application/dto/ApplicationRequest.java(0 hunks)src/main/java/life/mosu/mosuserver/presentation/application/dto/ApplicationResponse.java(1 hunks)src/main/java/life/mosu/mosuserver/presentation/inquiry/InquiryController.java(1 hunks)
💤 Files with no reviewable changes (3)
- src/main/java/life/mosu/mosuserver/presentation/application/dto/ApplicationRequest.java
- src/main/java/life/mosu/mosuserver/application/application/processor/SaveApplicationStepProcessor.java
- src/main/java/life/mosu/mosuserver/application/inquiry/InquiryAnswerService.java
🔇 Additional comments (28)
src/main/java/life/mosu/mosuserver/presentation/inquiry/InquiryController.java (2)
39-39: Good security enhancement!Adding authorization to the inquiry creation endpoint is a solid improvement that ensures only authenticated users with the USER role can create inquiries. This change maintains consistency with other user-level operations in the controller.
60-67: Verify missing authorization on getInquiryDetailThe
getInquiryDetailendpoint (GET /inquiry/{postId}) has no@PreAuthorizeand, given our SecurityConfig’s.anyRequest().permitAll(), is effectively open to anonymous users. All other inquiry endpoints enforce role checks via method-level@PreAuthorize.
Please confirm whether returning inquiry details publicly is intentional or if this method should be secured (e.g., only authenticated users or specific roles).src/main/java/life/mosu/mosuserver/global/processor/StepProcessor.java (1)
3-5: Excellent architectural improvement with dual generic types.The refactoring from
StepProcessor<T>toStepProcessor<REQ, RES>provides better type safety and flexibility, allowing processors to have distinct input and output types rather than being constrained to the same type.src/main/java/life/mosu/mosuserver/domain/exam/ExamJpaEntity.java (1)
75-77: Good method renaming for clarity.The rename from
providesLunch()tohasNotLunch()improves code readability by eliminating double negation in calling code and making the boolean condition more explicit.src/main/java/life/mosu/mosuserver/application/application/vaildator/ApplicationValidator.java (1)
51-51: Consistent update with domain entity changes.The change from lambda expression with negation to method reference
ExamJpaEntity::hasNotLunchimproves readability and maintains consistency with the domain entity method renaming.src/main/java/life/mosu/mosuserver/application/auth/processor/SignUpAccountStepProcessor.java (1)
14-14: Proper implementation of updated StepProcessor interface.The class correctly implements the new dual-generic
StepProcessor<UserJpaEntity, UserJpaEntity>interface while maintaining the same input/output type, which is appropriate for this account creation processor.src/main/java/life/mosu/mosuserver/application/auth/processor/SignUpProfileStepProcessor.java (2)
14-15: LGTM: Interface signature updated correctly.The change from
StepProcessor<ProfileJpaEntity>toStepProcessor<ProfileJpaEntity, ProfileJpaEntity>aligns with the refactored interface that now explicitly specifies both input and output types. Since this processor handles the same type for both input and output, the implementation is consistent.
14-15: LGTM: Interface signature updated correctly.The update to implement
StepProcessor<ProfileJpaEntity, ProfileJpaEntity>correctly aligns with the refactored interface design while maintaining the same input/output types and preserving existing functionality.src/main/java/life/mosu/mosuserver/application/application/processor/SaveExamTicketStepProcessor.java (2)
13-14: LGTM: Interface signature updated correctly.The change to
StepProcessor<ApplicationProcessingContext, ApplicationProcessingContext>is consistent with the interface refactoring and correctly specifies both input and output types as the same.
13-14: LGTM: Consistent interface signature update.The change to
StepProcessor<ApplicationProcessingContext, ApplicationProcessingContext>maintains consistency with the refactored interface design while preserving the existing processing logic.src/main/java/life/mosu/mosuserver/domain/examapplication/ExamApplicationJpaRepository.java (7)
35-35: Good: Payment status filtering added for data consistency.Adding the
AND p.paymentStatus = 'DONE'condition ensures only exam applications with completed payments are returned, which improves data consistency.
64-76: LGTM: Query formatting improved.The JPQL query formatting has been improved for better readability while maintaining the same logic.
75-75: Payment status filtering verifiedAll usages of the updated repository methods have been reviewed and handle “only DONE” applications as expected:
- ExamApplicationService uses findExamApplicationInfoById(...) and reacts with an EXAM_APPLICATION_NOT_FOUND exception when no result is returned.
- NotifyVariableFactory invokes findExamInfo(...) and findExamInfoWithExamNumber(...) (and throws EXAM_APPLICATION_NOT_FOUND) only for customers who have completed payment.
- findExamAndPaymentByExamApplicationId(...) is currently unused (its previous caller is commented out).
No callers expect non-DONE payment statuses, so the added
AND p.paymentStatus = 'DONE'filter will not break existing functionality.
35-35: Payment status filtering correctly implemented.The addition of
AND p.paymentStatus = 'DONE'ensures only completed payments are returned, which improves data consistency.
75-75: Consistent payment status filtering applied.The same payment completion filter is correctly applied to maintain consistency across query methods.
90-90: Payment filtering maintained in ExamInfo query.Consistent application of the payment status filter ensures only completed transactions are included.
106-106: Verify Payment Status Filter Across QueriesThe addition of
AND p.paymentStatus = 'DONE'changes the behavior of several repository methods and their callers:
ExamApplicationService.java
- Method:
findExamApplicationInfoById(examApplicationId)- Impact: Unpaid or pending applications will now result in
Optional.empty()→ throwsEXAM_APPLICATION_NOT_FOUND.NotifyVariableFactory.java
- Methods:
findExamInfo(targetId)findExamInfoWithExamNumber(targetId)- Impact: Pre-exam notification flows (1 week, 3 days, 1 day before) will skip any applications whose payment isn’t DONE.
ApplicationJpaRepository.java
- Method:
existsByUserIdAndExamIds(userId, examIds)- Impact: Existence checks now only count paid exam applications.
Please confirm that:
- Excluding non-paid applications in all of these scenarios aligns with the intended business rules.
- Exception handling and error messages distinguish between “not found” vs. “payment incomplete.”
- Relevant tests and documentation have been updated to cover the new filter behavior.
src/main/java/life/mosu/mosuserver/application/application/processor/RegisterApplicationStepProcessor.java (4)
1-1: Good: Package structure improved.Moving the processor to the dedicated
processorpackage improves code organization and separation of concerns.
18-19: LGTM: Interface signature updated correctly.The change to
StepProcessor<RegisterApplicationCommand, RegisterApplicationCommand>is consistent with the interface refactoring and correctly specifies both input and output types.
1-1: Good refactoring: Package organization improved.Moving the processor to the dedicated
processorpackage improves code organization and follows the established pattern for processor classes.
18-19: Interface signature updated consistently.The update to
StepProcessor<RegisterApplicationCommand, RegisterApplicationCommand>maintains consistency with the refactored interface design across all processor classes.src/main/java/life/mosu/mosuserver/application/application/processor/GetApplicationsStepProcessor.java (3)
17-26: LGTM: Clean dependency injection setupThe processor is properly configured as a Spring component with constructor injection for all required repositories. The separation of concerns is well-maintained by encapsulating JPA operations within this dedicated processor.
29-34: LGTM: Proper handling of empty resultsThe early return pattern for empty applications is efficient and prevents unnecessary processing. The empty list return is appropriate for the use case.
36-44: ApplicationContext chaining is safe as implementedWe’ve confirmed that
fetchExams,fetchSubjects, andfetchPaymentsall invoke Spring Data JPAfindBy…Inmethods, which by contract return non-null lists (empty when there’s no match). The collectors:
.toMap(...)infetchExamsandfetchPaymentsare fed distinct keys (no duplicates).Collectors.groupingBy(...)infetchSubjectshandles zero or many entries cleanly.- In
assemble(), missing subjects default toList.of(), and missing payments simply passnullinto the DTO (which should be handled at the presentation layer).Any exceptions from the repository (e.g. connectivity issues) will propagate as
DataAccessException, which you can catch or translate at a higher layer if desired. No additional null-checks or try/catch blocks are required withinApplicationContextitself.src/main/java/life/mosu/mosuserver/application/application/ApplicationService.java (3)
6-7: LGTM: Clean import organizationThe imports have been properly updated to reflect the new processor dependency and the relocated RegisterApplicationStepProcessor.
31-31: LGTM: Proper dependency injectionThe GetApplicationsStepProcessor is correctly injected as a final field, maintaining consistency with other processor dependencies.
64-66: Excellent refactoring: Simplified service methodThe getApplications method is now much cleaner and follows the single responsibility principle. The complex JPA operations and data assembly logic has been properly delegated to the dedicated processor, making the service more focused on orchestration rather than implementation details.
src/main/java/life/mosu/mosuserver/domain/application/ApplicationJpaRepository.java (1)
12-25: Verify duplicate application handling after payment filterThe updated
existsByUserIdAndExamIdsnow only detects applications with a'DONE'payment status. We found a single caller in the validator:
- File:
src/main/java/life/mosu/mosuserver/application/application/vaildator/ApplicationValidator.java
Method:NoDuplicateApplication(Long userId, List<Long> examIds)
Logic:Incomplete or pending payments will now returnboolean alreadyApplied = applicationJpaRepository.existsByUserIdAndExamIds(userId, examIds); if (alreadyApplied) { throw new CustomRuntimeException(ErrorCode.APPLICATION_SCHOOL_DUPLICATED); }false, allowing users to reapply before payment completion.Please verify that this new behavior aligns with your business requirements—i.e., should pending‐payment applications be excluded from duplicate checks, or do you need to adjust the query or validation logic?
1 participant
✨ 구현한 기능
📢 논의하고 싶은 내용
🎸 기타
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores