-
Notifications
You must be signed in to change notification settings - Fork 1
MOSU-51 feat: 결제 고도화 및 보상 트랜잭션 구성 (PR 닫지말고, 댓글 달지마요 아직 덜했어요) #53
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…필드를 Long에서 Integer로 수정
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR enhances the payment flow by propagating rollbacks via events, restructuring confirm logic with retries, and composing compensation transactions on failure.
- Introduces event-driven confirm workflow in
PaymentService, with retryable logic and compensation viaPaymentEventListenerandPaymentFailureHandler. - Refactors DTOs and payload types to support multiple application IDs, adds size/count helpers, and adjusts amount types.
- Removes old persistence repos, reorganizes domain repositories, and extends JPA entities with failure constructors and static helpers.
Reviewed Changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/test/java/.../TossPaymentClientTest.java | Updated test payload type and removed duplicate dummy method |
| src/main/java/.../PreparePaymentRequest.java | Added getSize() helper |
| src/main/java/.../PaymentRequest.java | Changed to List<Long> applicationIds, added applicantSize() |
| src/main/java/.../persistence/profile/ProfileJpaRepository.java | Removed old profile repo |
| src/main/java/.../persistence/application/ApplicationJpaRepository.java | Removed old application repo |
| src/main/java/.../infra/payment/dto/TossPaymentPayload.java | Changed amount to Integer |
| src/main/java/.../infra/payment/dto/ConfirmTossPaymentResponse.java | Renamed toPaymentJpaEntity to toEntity and removed quantity |
| src/main/java/.../domain/school/SchoolRepository.java | Added new SchoolRepository |
| src/main/java/.../domain/payment/PaymentRepository.java | Added findByOrderId, noted index TODO |
| src/main/java/.../domain/payment/PaymentMethod.java | Improved from() filter, added logging |
| src/main/java/.../domain/payment/PaymentJpaEntity.java | Removed quantity, added failure constructors and status change |
| src/main/java/.../domain/payment/PaymentAmountVO.java | Added ofFailure() factory |
| src/main/java/.../domain/discount/DiscountPolicy.java | Added static calculate() helper |
| src/main/java/.../domain/applicationschool/ApplicationSchoolJpaRepository.java | Fixed package/imports |
| src/main/java/.../domain/application/ApplicationJpaRepository.java | New existsAllByIds query |
| src/main/java/.../application/payment/PaymentService.java | Refactored confirm flow, added helper methods and event publishing |
| src/main/java/.../application/payment/PaymentFailureHandler.java | New handler for failed payments |
| src/main/java/.../application/payment/PaymentEventListener.java | Added retryable compensation listener |
| src/main/java/.../application/payment/PaymentEvent.java | Extended event payload with IDs and totalAmount |
| src/main/java/.../application/applicationschool/ApplicationSchoolService.java | Cleaned imports/formatting |
| src/main/java/.../application/application/ApplicationService.java | Cleaned imports/formatting |
Comments suppressed due to low confidence (2)
src/main/java/life/mosu/mosuserver/presentation/payment/dto/PreparePaymentRequest.java:13
- [nitpick] The method name
getSize()is generic and inconsistent withapplicantSize()inPaymentRequest. Consider renaming both to something likegetItemCount()orgetApplicationCount()for clarity and consistency.
public int getSize() {
src/main/java/life/mosu/mosuserver/application/payment/PaymentEventListener.java:37
- The retryable compensation logic in
afterRollbackHandlerisn’t covered by existing tests. Consider adding unit or integration tests to validate retry behavior and failure handling.
@Retryable(
| public static PaymentMethod from(String raw) { | ||
| return Arrays.stream(values()) | ||
| .filter(v -> v.name().equalsIgnoreCase(raw)) | ||
| .filter(v -> v.name.equalsIgnoreCase(raw)) |
Copilot
AI
Jul 6, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using the field name to match the input may shadow the enum's name() method and lead to confusion. If you intend to match the constant name, use v.name().equalsIgnoreCase(raw); otherwise rename the field (e.g., label) and update references accordingly.
| } catch (Exception ex) { | ||
| log.error("error : {}", ex.getMessage()); | ||
| publisher.publishEvent(PaymentEvent.ofFailed(applicationIds, orderId, amount)); | ||
| throw ex; |
Copilot
AI
Jul 6, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Catching generic Exception will also catch business logic errors and trigger retries unintentionally under @Retryable. Consider catching more specific exceptions or rethrowing non-retryable exceptions to avoid infinite retry loops.
| } catch (Exception ex) { | |
| log.error("error : {}", ex.getMessage()); | |
| publisher.publishEvent(PaymentEvent.ofFailed(applicationIds, orderId, amount)); | |
| throw ex; | |
| } catch (HttpStatusCodeException ex) { | |
| log.error("HTTP error during payment confirmation: {}", ex.getMessage()); | |
| publisher.publishEvent(PaymentEvent.ofFailed(applicationIds, orderId, amount)); | |
| throw ex; // Allow retry for HTTP status-related exceptions | |
| } catch (RuntimeException ex) { | |
| log.error("Unexpected runtime error: {}", ex.getMessage()); | |
| publisher.publishEvent(PaymentEvent.ofFailed(applicationIds, orderId, amount)); | |
| throw ex; // Rethrow non-retryable runtime exceptions |
|
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request refactors the payment processing logic to support batch payments for multiple applications and introduces a robust failure handling mechanism using transactional events and retries. The changes significantly improve the structure and reliability of the payment confirmation flow.
My review focuses on further improving reliability and maintainability:
- A critical issue with an incorrect
@Recovermethod that could lead to data inconsistency has been identified. - I've suggested ensuring atomicity in the payment failure handler by adding transactional support.
- Other suggestions include improving logging, code cleanup by removing unused dependencies and commented-out code, and optimizing database calls.
|
|
||
| private final PaymentRepository paymentRepository; | ||
|
|
||
| public void handlePaymentFailure(PaymentEvent event) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The handlePaymentFailure method performs multiple database operations. Since it's called from an event listener with Propagation.NOT_SUPPORTED, these operations are not executed within a single transaction. This could lead to an inconsistent state if one of the subsequent operations fails after the first one has succeeded. To ensure atomicity, this method should be annotated with @Transactional.
| public void handlePaymentFailure(PaymentEvent event) { | |
| @Transactional | |
| public void handlePaymentFailure(PaymentEvent event) { |
| public class PaymentEventListener { | ||
|
|
||
| // 1. 트랜잭션 커밋 직전에 호출됨 (롤백 가능 상태) | ||
| private final PaymentRepository paymentRepository; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| paymentRepository.saveAll(existingPayments); | ||
| paymentRepository.saveAll(newPayments); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These two separate calls to paymentRepository.saveAll() can be combined into a single, more efficient database operation. This reduces network overhead and is a good practice for batch operations.
List<PaymentJpaEntity> allPaymentsToSave = new ArrayList<>(existingPayments);
allPaymentsToSave.addAll(newPayments);
paymentRepository.saveAll(allPaymentsToSave);| savePayments(paymentEntities); | ||
| publisher.publishEvent(PaymentEvent.ofSuccess(applicationIds, orderId, amount)); | ||
| } catch (Exception ex) { | ||
| log.error("error : {}", ex.getMessage()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current error logging only captures the exception message. This can make debugging difficult as the context (stack trace) is lost. It's a best practice to log the full exception object to get the stack trace.
| log.error("error : {}", ex.getMessage()); | |
| log.error("error : {}", ex.getMessage(), ex); // include ex to log the stack trace |
| // return new ConfirmTossPaymentResponse( | ||
| // "tviva20250702231345mODA4", // paymentKey | ||
| // "IDMAoki7azYp8SzQ06LMt12323235", // orderId | ||
| // "DONE", // status | ||
| // "2025-07-02T23:14:33+09:00", // approvedAt | ||
| // 1_000, // totalAmount | ||
| // 1_000, // balanceAmount | ||
| // 1_000, // suppliedAmount | ||
| // 1_000, // vat | ||
| // 1_000, // taxFreeAmount | ||
| // "간편결제" // method | ||
| // ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😎 연결된 이슈
✨ 구현한 기능
Retryable을 통해서 재시도 로직 구성📢 논의하고 싶은 내용
🎸 기타