feat : payment 실패시 status 변경 #74
Conversation
|
Warning Rate limit exceeded@polyglot-k has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 9 minutes and 29 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThis update introduces a Redis-backed caching mechanism for school quotas and application counts, refactors various payment and application classes to consistently use "applicationSchoolId" instead of "applicationId," and adds supporting infrastructure for cache preloading at application startup. Several new classes and methods are introduced, while some repository interfaces and fields are renamed or removed for clarity. Changes
Sequence Diagram(s)sequenceDiagram
participant SpringApp as Spring Application
participant PreWarmRunner as ApplicationSchoolPreWarmRunner
participant CacheManager as SchoolQuotaCacheManager
participant Redis as Redis
SpringApp->>PreWarmRunner: ApplicationReadyEvent
PreWarmRunner->>CacheManager: preloadSchoolData()
CacheManager->>CacheManager: cacheSchoolMaxCapacities()
CacheManager->>Redis: Store capacities per school
CacheManager->>CacheManager: cacheSchoolCurrentApplicationCounts()
CacheManager->>Redis: Store application counts per school
sequenceDiagram
participant User as User
participant PaymentService as PaymentService
participant SchoolRepo as SchoolRepository
participant CacheManager as SchoolQuotaCacheManager
participant Redis as Redis
User->>PaymentService: confirm(paymentRequest)
PaymentService->>SchoolRepo: checkApplicationsExist(applicationSchoolIds)
PaymentService->>CacheManager: increaseApplicationCount(schoolName)
CacheManager->>Redis: INCR application count
PaymentService->>PaymentService: Process payment
Suggested reviewers
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @polyglot-k, 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 primarily focuses on refining the domain model by transitioning from a generic 'application' identifier to a more specific 'application school' identifier across the payment processing components. Concurrently, it introduces a robust Redis-based caching mechanism to efficiently manage and track school quotas, including current application counts and maximum capacities. These changes collectively aim to improve the accuracy of the domain model, enhance performance for quota-related operations, and ensure proper handling of payment failures within the updated context.
Highlights
- Domain Model Refinement: The core domain concept of 'application' has been refined to 'application school' across various entities, DTOs, and services. This includes renaming fields like
applicationIdtoapplicationSchoolIdinPaymentEvent,PaymentJpaEntity,PaymentRequest, andPreparePaymentRequestto improve clarity and specificity within the payment and school application context. - Payment Failure Handling Enhancement: The
PaymentFailureHandlerhas been updated to align with the newapplicationSchoolIdnaming, ensuring that payment statuses are correctly changed and new failure entities are created with the refined identifier. A comment was added indicating that batch follow-up processing might be required for newly created failure entities. - Introduction of School Quota Caching with Redis: A significant new feature is the implementation of
SchoolQuotaCacheManager, which utilizes Redis to cache and manage school capacities and current application counts. This includes new files for Redis configuration (RedisConfig.java), a new JPA projection (SchoolApplicationProjection.java), and an application runner (ApplicationSchoolPreWarmRunner.java) to pre-load this data into Redis on application startup, enhancing performance and consistency for quota checks.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
The pull request introduces a caching layer for school application quotas and refactors payment-related code. A compilation error due to a missing repository method and a race condition in the cache logic were identified. A naming inconsistency was also addressed.
src/main/java/life/mosu/mosuserver/application/payment/PaymentService.java
Show resolved
Hide resolved
src/main/java/life/mosu/mosuserver/application/school/SchoolQuotaCacheManager.java
Show resolved
Hide resolved
src/main/java/life/mosu/mosuserver/application/payment/PaymentFailureHandler.java
Outdated
Show resolved
Hide resolved
src/main/java/life/mosu/mosuserver/presentation/payment/dto/PreparePaymentRequest.java
Outdated
Show resolved
Hide resolved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🔭 Outside diff range comments (2)
src/main/java/life/mosu/mosuserver/application/payment/PaymentEvent.java (1)
13-26: Update parameter names in static factory methods.The static factory methods still use
applicationIdsas parameter names, but the record component is nowapplicationSchoolIds. This inconsistency could cause confusion.Apply this diff to fix the parameter names:
- public static PaymentEvent ofSuccess(List<Long> applicationIds, String orderId, + public static PaymentEvent ofSuccess(List<Long> applicationSchoolIds, String orderId, Integer totalAmount) { - return new PaymentEvent(applicationIds, orderId, PaymentStatus.DONE, totalAmount); + return new PaymentEvent(applicationSchoolIds, orderId, PaymentStatus.DONE, totalAmount); } - public static PaymentEvent ofCancelled(List<Long> applicationIds, String orderId, + public static PaymentEvent ofCancelled(List<Long> applicationSchoolIds, String orderId, Integer totalAmount) { - return new PaymentEvent(applicationIds, orderId, PaymentStatus.CANCELLED_DONE, totalAmount); + return new PaymentEvent(applicationSchoolIds, orderId, PaymentStatus.CANCELLED_DONE, totalAmount); } - public static PaymentEvent ofFailed(List<Long> applicationIds, String orderId, + public static PaymentEvent ofFailed(List<Long> applicationSchoolIds, String orderId, Integer totalAmount) { - return new PaymentEvent(applicationIds, orderId, PaymentStatus.ABORTED, totalAmount); + return new PaymentEvent(applicationSchoolIds, orderId, PaymentStatus.ABORTED, totalAmount); }src/main/java/life/mosu/mosuserver/application/payment/PaymentService.java (1)
88-94: Missing repository methodexistsAllByIds
The call toapplicationSchoolJpaRepository.existsAllByIds(...)inPaymentServicewill fail because no such method is declared. Please add or implement it in:• src/main/java/life/mosu/mosuserver/domain/applicationschool/ApplicationSchoolJpaRepository.java
Suggested diff:
public interface ApplicationSchoolJpaRepository extends JpaRepository<ApplicationSchool, Long> { + /** + * Verify that all given application IDs exist. + * @param applicationIds the list of IDs to check + * @param count the expected number of matching records + * @return true if the repository contains all IDs + */ + boolean existsAllByIds(List<Long> applicationIds, int count); }Alternatively, implement a custom
@Queryor use a count comparison (e.g.countByIdIn(...) == size) to achieve the same check.
🧹 Nitpick comments (6)
src/main/java/life/mosu/mosuserver/domain/payment/PaymentJpaEntity.java (1)
84-97: PopulatepaymentMethodin the failure factory or default it explicitly
ofFailure()leavespaymentMethodasnull, but the column is present in the table.
If the downstream code ever assumes a non-null value (e.g., for analytics or enum mapping) this can blow up at runtime. Two options:
- Pass a sensible enum (
PaymentMethod.NONE,UNKNOWN, etc.) to the builder here.- Mark the column
nullable = trueand document that it is null for failures.Consider updating the factory:
- return PaymentJpaEntity.builder() - .applicationSchoolId(applicationSchoolId) - .orderId(orderId) - .paymentStatus(paymentStatus) - .paymentAmount(paymentAmount) - .build(); + return PaymentJpaEntity.builder() + .applicationSchoolId(applicationSchoolId) + .orderId(orderId) + .paymentStatus(paymentStatus) + .paymentAmount(paymentAmount) + .paymentMethod(PaymentMethod.UNKNOWN) // or NONE + .build();Please confirm that consumers of
paymentMethodcan handlenullor apply the suggested default.src/main/java/life/mosu/mosuserver/global/config/RedisConfig.java (1)
14-21: Consider enhancing RedisTemplate configuration for production readiness.The current configuration is functional but has some limitations:
- The
RedisTemplate<String, Long>type constraint limits this bean to only Long values. Consider if you need a more generic template for future extensibility.- Hash serializers are not configured, which may cause issues if hash operations are used later.
- Production-ready configurations like connection timeouts, retry policies, and connection pool settings are missing.
Consider this enhanced configuration:
@Bean public RedisTemplate<String, Long> redisTemplate( RedisConnectionFactory redisConnectionFactory) { RedisTemplate<String, Long> redisTemplate = new RedisTemplate<>(); redisTemplate.setConnectionFactory(redisConnectionFactory); redisTemplate.setKeySerializer(new StringRedisSerializer()); redisTemplate.setValueSerializer(new GenericToStringSerializer<>(Long.class)); + redisTemplate.setHashKeySerializer(new StringRedisSerializer()); + redisTemplate.setHashValueSerializer(new GenericToStringSerializer<>(Long.class)); + redisTemplate.afterPropertiesSet(); return redisTemplate; }src/main/java/life/mosu/mosuserver/domain/school/SchoolApplicationProjection.java (1)
3-3: LGTM! Clean and appropriate projection record.The record definition is well-structured for its intended use as a JPQL projection. Consider adding JavaDoc to document the purpose and usage of this projection.
+/** + * Projection record for aggregating school application data. + * Used to retrieve school information along with application counts. + */ public record SchoolApplicationProjection(Long schoolId, String schoolName, Long count) {src/main/java/life/mosu/mosuserver/domain/school/SchoolRepository.java (1)
9-15: LGTM! Well-structured aggregation query with performance considerations.The JPQL query correctly uses LEFT JOIN to include all schools and properly aggregates application counts. Consider adding an ORDER BY clause for consistent results and evaluate query performance with large datasets.
Consider adding ordering for consistent results:
@Query(""" SELECT new life.mosu.mosuserver.domain.school.SchoolApplicationProjection(s.id, s.schoolName, COUNT(a)) FROM SchoolJpaEntity s LEFT JOIN ApplicationSchoolJpaEntity a ON a.schoolId = s.id GROUP BY s.id, s.schoolName + ORDER BY s.schoolName """)src/main/java/life/mosu/mosuserver/application/school/SchoolQuotaCacheManager.java (2)
15-16: Consider adding cache expiration strategy and error handling.The current implementation has no cache expiration strategy, which could lead to stale data, and lacks error handling for Redis failures.
Consider these architectural improvements:
- Cache TTL: Add expiration times to prevent stale data
- Fallback strategy: Handle Redis unavailability by falling back to database
- Cache warming strategy: Consider incremental updates vs full refresh
- Monitoring: Add metrics for cache hit/miss ratios
Would you like me to help design a more robust caching strategy?
21-35: Consider performance optimization and error handling.The current implementation loads all data synchronously which could impact startup time with large datasets.
Consider these improvements:
- Add error handling for Redis operations:
public void cacheSchoolMaxCapacities() { try { List<SchoolJpaEntity> schools = schoolRepository.findAll(); for (SchoolJpaEntity school : schools) { String key = REDIS_KEY_SCHOOL_MAX_CAPACITY + school.getSchoolName(); redisTemplate.opsForValue().set(key, school.getCapacity()); } } catch (Exception e) { log.error("Failed to cache school max capacities", e); throw new CacheException("Failed to cache school data", e); } }
- Consider adding TTL for cache entries:
redisTemplate.opsForValue().set(key, school.getCapacity(), Duration.ofHours(24));
- Use batch operations for better performance:
Map<String, Long> capacities = schools.stream() .collect(Collectors.toMap( school -> REDIS_KEY_SCHOOL_MAX_CAPACITY + school.getSchoolName(), SchoolJpaEntity::getCapacity )); redisTemplate.opsForValue().multiSet(capacities);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
src/main/java/life/mosu/mosuserver/application/payment/PaymentEvent.java(1 hunks)src/main/java/life/mosu/mosuserver/application/payment/PaymentFailureHandler.java(1 hunks)src/main/java/life/mosu/mosuserver/application/payment/PaymentService.java(5 hunks)src/main/java/life/mosu/mosuserver/application/school/SchoolQuotaCacheManager.java(1 hunks)src/main/java/life/mosu/mosuserver/domain/application/ApplicationJpaRepository.java(0 hunks)src/main/java/life/mosu/mosuserver/domain/application/ApplicationSchoolJpaRepository.java(0 hunks)src/main/java/life/mosu/mosuserver/domain/payment/PaymentJpaEntity.java(4 hunks)src/main/java/life/mosu/mosuserver/domain/school/SchoolApplicationProjection.java(1 hunks)src/main/java/life/mosu/mosuserver/domain/school/SchoolJpaEntity.java(2 hunks)src/main/java/life/mosu/mosuserver/domain/school/SchoolRepository.java(1 hunks)src/main/java/life/mosu/mosuserver/global/config/RedisConfig.java(1 hunks)src/main/java/life/mosu/mosuserver/global/runner/ApplicationSchoolPreWarmRunner.java(1 hunks)src/main/java/life/mosu/mosuserver/presentation/payment/dto/PaymentRequest.java(2 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/domain/application/ApplicationJpaRepository.java
- src/main/java/life/mosu/mosuserver/domain/application/ApplicationSchoolJpaRepository.java
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/main/java/life/mosu/mosuserver/application/school/SchoolQuotaCacheManager.java (1)
src/main/java/life/mosu/mosuserver/global/runner/ApplicationSchoolPreWarmRunner.java (1)
RequiredArgsConstructor(9-19)
🔇 Additional comments (19)
src/main/java/life/mosu/mosuserver/domain/school/SchoolJpaEntity.java (3)
4-13: LGTM: Explicit imports improve code clarity.The change from wildcard imports to explicit imports is a good practice that improves code readability and avoids potential naming conflicts.
20-20: LGTM: Consistent annotation formatting.The formatting adjustments for
@Tableand@Columnannotations improve code consistency.Also applies to: 26-26
44-51: LGTM: Well-structured constructor follows JPA best practices.The new constructor properly initializes all fields except the auto-generated ID, which is a standard pattern for JPA entities. This supports the broader domain refactoring mentioned in the summary.
src/main/java/life/mosu/mosuserver/presentation/payment/dto/PaymentRequest.java (2)
8-8: LGTM - Consistent naming update.The field rename from
applicationIdstoapplicationSchoolIdsis consistent with the broader refactoring effort.
19-19: LGTM - Method correctly updated.The method properly uses the renamed field
applicationSchoolIds.src/main/java/life/mosu/mosuserver/application/payment/PaymentFailureHandler.java (3)
22-23: LGTM - Consistent method call update.The method call correctly uses
getApplicationSchoolId()in line with the refactoring.
25-27: LGTM - Consistent field access update.The code properly uses
event.applicationSchoolIds()matching the updated PaymentEvent structure.
33-39: LGTM - Consistent variable naming and method calls.The variable naming and method calls have been updated consistently with the refactoring from applicationId to applicationSchoolId.
src/main/java/life/mosu/mosuserver/application/payment/PaymentService.java (4)
6-6: LGTM - Repository import updated.The import has been correctly updated to use
ApplicationSchoolJpaRepository.
33-33: LGTM - Repository field updated.The field has been correctly updated to use
ApplicationSchoolJpaRepository.
54-54: LGTM - Variable naming updated.The variable name has been correctly updated to
applicationSchoolIds.
138-141: LGTM - Stream operation updated.The stream operation correctly uses
request.applicationSchoolIds()matching the updated PaymentRequest structure.src/main/java/life/mosu/mosuserver/application/school/SchoolQuotaCacheManager.java (3)
1-64: Flag inconsistency between PR objective and implementation.The PR title indicates this is about "payment 실패시 status 변경" (payment failure status changes), but the actual implementation is entirely about Redis caching for school quotas. This suggests either:
- The PR description is incorrect, or
- The files being reviewed don't match the intended changes
Please verify that these files correspond to the correct PR objective. If this is intended to be a separate feature, consider updating the PR title and description to accurately reflect the Redis caching implementation.
Likely an incorrect or invalid review comment.
21-27: Optimize database queries to avoid N+1 pattern.The current implementation fetches all schools and iterates through them individually, which is inefficient for large datasets.
Consider using batch operations:
public void cacheSchoolMaxCapacities() { List<SchoolJpaEntity> schools = schoolRepository.findAll(); - for (SchoolJpaEntity school : schools) { - String key = REDIS_KEY_SCHOOL_MAX_CAPACITY + school.getSchoolName(); - redisTemplate.opsForValue().set(key, school.getCapacity()); - } + Map<String, Long> capacityMap = schools.stream() + .collect(Collectors.toMap( + school -> REDIS_KEY_SCHOOL_MAX_CAPACITY + school.getSchoolName(), + SchoolJpaEntity::getCapacity + )); + redisTemplate.opsForValue().multiSet(capacityMap); }Likely an incorrect or invalid review comment.
1-64: Inconsistency with PR objectives detected.The PR title indicates "payment 실패시 status 변경" (payment failure status change), but the implemented code focuses entirely on Redis caching for school quotas and application counts with no payment-related functionality.
Please verify:
- Is this the correct code for the intended PR?
- Should there be additional payment failure handling code?
- Is the PR title incorrect for these changes?
Likely an incorrect or invalid review comment.
src/main/java/life/mosu/mosuserver/global/config/RedisConfig.java (1)
1-22: LGTM! Standard Redis configuration setup.The RedisTemplate configuration follows Spring Boot best practices with appropriate serializers for the specific use case (String keys, Long values).
src/main/java/life/mosu/mosuserver/domain/school/SchoolApplicationProjection.java (1)
3-5: LGTM! Appropriate use of Java records for projection data.The record design is clean and well-suited for carrying aggregated school application data from JPQL queries.
src/main/java/life/mosu/mosuserver/global/runner/ApplicationSchoolPreWarmRunner.java (1)
1-19: LGTM! Proper implementation of startup cache warming.The component correctly uses
ApplicationReadyEventto ensure cache preloading occurs after full application initialization.src/main/java/life/mosu/mosuserver/domain/school/SchoolRepository.java (1)
9-15: Index coverage on application_school.school_id neededI didn’t find any index on the
school_idcolumn in your JPA entity annotations or SQL migration files. Please verify that an index exists forapplication_school.school_id(either via JPA’s@Table(indexes=…)or in your database migration scripts) to ensure optimal performance for the aggregation query.• Check
ApplicationSchoolJpaEntity(and any other relevant entities) for a@Table(indexes = …)annotation that includesschool_id.
• Review your migration files (e.g., Flyway or Liquibase scripts undersrc/main/resources/db/migration) for aCREATE INDEXonapplication_school.school_id.
✨ 구현한 기능
📢 논의하고 싶은 내용
🎸 기타
Summary by CodeRabbit
New Features
Improvements
Bug Fixes