[Refactor] 결제 승인 트랜잭션 분리 및 멱등성 보강 (#62)#103
Conversation
|
Warning Review limit reached
More reviews will be available in 45 minutes and 23 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughIntroduces a ChangesIdempotent Payment Confirmation Flow
Sequence Diagram(s)sequenceDiagram
participant Client
participant PaymentService
participant PaymentTransactionService
participant TossPaymentClient
participant CreditService
Client->>PaymentService: confirm(user, request)
PaymentService->>PaymentTransactionService: startConfirmation(userId, request)
PaymentTransactionService-->>PaymentService: PaymentConfirmationStart(alreadyCompleted)
alt alreadyCompleted
PaymentService-->>Client: existing PaymentConfirmResponse
else
PaymentService->>TossPaymentClient: confirm(orderId, paymentKey, amount)
alt RuntimeException
TossPaymentClient-->>PaymentService: throws GeneralException
PaymentService->>PaymentTransactionService: failConfirmation(userId, orderId, paymentKey)
PaymentTransactionService-->>PaymentService: payment set to FAILED
PaymentService-->>Client: rethrows exception
else success
TossPaymentClient-->>PaymentService: TossPaymentResponse
PaymentService->>PaymentTransactionService: completeConfirmation(userId, request)
PaymentTransactionService->>CreditService: charge(userId, amount, orderId)
PaymentTransactionService-->>PaymentService: PaymentConfirmResponse
PaymentService-->>Client: PaymentConfirmResponse
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
src/test/java/com/jobdri/jobdri_api/domain/payment/service/PaymentServiceTest.java (1)
115-143: ⚡ Quick winConsider verifying mock invocation count for clarity.
The test correctly validates idempotency, but explicitly verifying that
tossPaymentClient.confirmis called exactly once (not twice) would make the early-return behavior more obvious to readers.✨ Proposed enhancement
Add after line 142:
assertThat(creditTransactionRepository.findAllByUserIdAndTypeOrderByCreatedAtDescIdDesc( user.getId(), CreditTransactionType.CHARGE )).hasSize(1); + verify(tossPaymentClient, times(1)).confirm(paymentKey, prepared.orderId(), 2500); }You'll need to add this import:
import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/java/com/jobdri/jobdri_api/domain/payment/service/PaymentServiceTest.java` around lines 115 - 143, The test confirmReturnsExistingResultWhenAlreadyCompleted validates idempotency through assertions but lacks an explicit verification that tossPaymentClient.confirm is called exactly once, making the early-return behavior less obvious. Add a verify statement after the existing assertions to explicitly check that tossPaymentClient.confirm is invoked exactly once using verify(tossPaymentClient, times(1)).confirm(paymentKey, prepared.orderId(), 2500). Additionally, add the required Mockito imports for verify and times at the top of the test file.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/main/java/com/jobdri/jobdri_api/domain/payment/entity/CreditTransaction.java`:
- Around line 13-21: The unique constraint defined in the CreditTransaction
entity's `@UniqueConstraint` annotation
(uk_credit_transactions_user_type_reference on user_id, type, and reference_id)
exists only in JPA metadata and is not enforced in the actual database schema.
Create a Flyway or Liquibase migration file in ops/db/migrations/ that first
identifies and removes any existing duplicate rows in the credit_transactions
table that violate the (user_id, type, reference_id) uniqueness requirement,
then explicitly adds the unique constraint using ALTER TABLE to enforce it at
the database level, ensuring the constraint definition matches what is declared
in the CreditTransaction entity's `@UniqueConstraint` annotation, and includes
pre-migration validation to confirm no duplicates remain before applying the
constraint.
In
`@src/main/java/com/jobdri/jobdri_api/domain/payment/service/CreditService.java`:
- Around line 41-50: The code currently treats a transaction as idempotent based
solely on matching (user, type, referenceId), but does not verify that the
amount in a retry request matches the original transaction amount. To fix this,
after retrieving the existingTransaction, calculate the transactionAmount for
the current request using resolveTransactionAmount, then compare this amount
with the existing transaction's amount before returning the old balance. If the
amounts do not match, reject the request by throwing an appropriate exception to
prevent amount mismatches from being silently ignored.
In
`@src/main/java/com/jobdri/jobdri_api/domain/payment/service/PaymentService.java`:
- Around line 80-86: The TossPaymentClient.confirm() method currently wraps both
HTTP errors and transient failures (timeouts, connection issues) into a single
GeneralException, causing PaymentService.confirm() to treat all exceptions
identically and mark payments as FAILED. To fix this, modify
TossPaymentClient.confirm() to distinguish between definitive rejections (4xx
HTTP responses) and transient failures (connection timeouts, network errors) by
throwing different exception types for each case. Then in
PaymentService.confirm(), update the catch block to only call
paymentTransactionService.failConfirmation() when catching the exception type
that represents definitive rejections, allowing transient failures to remain
unhandled so the payment stays in PROCESSING state for later reconciliation.
In
`@src/main/java/com/jobdri/jobdri_api/domain/payment/service/PaymentTransactionService.java`:
- Around line 35-37: The validation in PaymentTransactionService.confirm method
currently rejects all retries for payments in PROCESSING status by throwing an
exception on line 35, which prevents recovery if the confirmation process fails
after the status is set to PROCESSING. Modify the validation logic to allow
retries for the same paymentKey to be resumable; instead of unconditionally
throwing an exception for PROCESSING status, check if the incoming paymentKey
matches an existing PROCESSING payment and allow it to proceed as a resumable
operation rather than rejecting it as an error condition.
---
Nitpick comments:
In
`@src/test/java/com/jobdri/jobdri_api/domain/payment/service/PaymentServiceTest.java`:
- Around line 115-143: The test confirmReturnsExistingResultWhenAlreadyCompleted
validates idempotency through assertions but lacks an explicit verification that
tossPaymentClient.confirm is called exactly once, making the early-return
behavior less obvious. Add a verify statement after the existing assertions to
explicitly check that tossPaymentClient.confirm is invoked exactly once using
verify(tossPaymentClient, times(1)).confirm(paymentKey, prepared.orderId(),
2500). Additionally, add the required Mockito imports for verify and times at
the top of the test file.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8a9ceb38-23ce-4d3e-9109-54184c58f907
📒 Files selected for processing (9)
src/main/java/com/jobdri/jobdri_api/domain/payment/entity/CreditTransaction.javasrc/main/java/com/jobdri/jobdri_api/domain/payment/entity/Payment.javasrc/main/java/com/jobdri/jobdri_api/domain/payment/entity/PaymentStatus.javasrc/main/java/com/jobdri/jobdri_api/domain/payment/repository/CreditTransactionRepository.javasrc/main/java/com/jobdri/jobdri_api/domain/payment/service/CreditService.javasrc/main/java/com/jobdri/jobdri_api/domain/payment/service/PaymentService.javasrc/main/java/com/jobdri/jobdri_api/domain/payment/service/PaymentTransactionService.javasrc/test/java/com/jobdri/jobdri_api/domain/payment/service/CreditServiceTest.javasrc/test/java/com/jobdri/jobdri_api/domain/payment/service/PaymentServiceTest.java
| @Table( | ||
| name = "credit_transactions", | ||
| uniqueConstraints = { | ||
| @UniqueConstraint( | ||
| name = "uk_credit_transactions_user_type_reference", | ||
| columnNames = {"user_id", "type", "reference_id"} | ||
| ) | ||
| } | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Look for schema migrations or ddl-auto config that creates/enforces
# the credit_transactions reference_id mapping and composite uniqueness.
echo "Migration/config files:"
fd -i '(^V[0-9].*|.*changelog.*|.*schema.*|.*migration.*|application.*\.(yml|yaml|properties))$'
echo
echo "Relevant schema/config references:"
rg -n -C3 'uk_credit_transactions_user_type_reference|credit_transactions|reference_id|ddl-auto|hibernate\.hbm2ddl'Repository: JobDri-Developer/BackEnd
Length of output: 3867
Create an explicit migration to enforce the unique constraint and backfill existing duplicates.
The idempotency constraint (user_id, type, reference_id) is defined only in JPA metadata at lines 13-21. No explicit Flyway/Liquibase migration exists in ops/db/migrations/ and the constraint is absent from schema.sql. When deployed to production, ddl-auto: update cannot reliably create or enforce complex constraints, and existing duplicate rows will cause the constraint application to fail.
Create a migration file to:
- Identify and deduplicate existing
credit_transactionsrows by(user_id, type, reference_id) - Add the unique constraint explicitly via
ALTER TABLE - Ensure pre-deployment validation that no duplicates remain
Also applies to: 45-46
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@src/main/java/com/jobdri/jobdri_api/domain/payment/entity/CreditTransaction.java`
around lines 13 - 21, The unique constraint defined in the CreditTransaction
entity's `@UniqueConstraint` annotation
(uk_credit_transactions_user_type_reference on user_id, type, and reference_id)
exists only in JPA metadata and is not enforced in the actual database schema.
Create a Flyway or Liquibase migration file in ops/db/migrations/ that first
identifies and removes any existing duplicate rows in the credit_transactions
table that violate the (user_id, type, reference_id) uniqueness requirement,
then explicitly adds the unique constraint using ALTER TABLE to enforce it at
the database level, ensuring the constraint definition matches what is declared
in the CreditTransaction entity's `@UniqueConstraint` annotation, and includes
pre-migration validation to confirm no duplicates remain before applying the
constraint.
| CreditTransaction existingTransaction = creditTransactionRepository | ||
| .findByUserIdAndTypeAndReferenceId(managedUser.getId(), type, referenceId) | ||
| .orElse(null); | ||
| if (existingTransaction != null) { | ||
| return existingTransaction.getBalanceAfter(); | ||
| } | ||
|
|
||
| int transactionAmount = resolveTransactionAmount(type, amount); | ||
| applyCreditChange(managedUser, type, amount); | ||
| saveTransaction(managedUser, type, transactionAmount, description, referenceId); |
There was a problem hiding this comment.
Reject same referenceId replays with a different amount.
Line 44 returns the old balance for any existing (user, type, referenceId), even when the retry carries a different amount. Compare the signed transaction amount before treating the call as idempotent.
Suggested fix
+ int transactionAmount = resolveTransactionAmount(type, amount);
CreditTransaction existingTransaction = creditTransactionRepository
.findByUserIdAndTypeAndReferenceId(managedUser.getId(), type, referenceId)
.orElse(null);
if (existingTransaction != null) {
+ if (existingTransaction.getAmount() != transactionAmount) {
+ throw new GeneralException(
+ GeneralErrorCode.INVALID_PARAMETER,
+ "referenceId가 기존 거래 금액과 일치하지 않습니다."
+ );
+ }
return existingTransaction.getBalanceAfter();
}
- int transactionAmount = resolveTransactionAmount(type, amount);
applyCreditChange(managedUser, type, amount);
saveTransaction(managedUser, type, transactionAmount, description, referenceId);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@src/main/java/com/jobdri/jobdri_api/domain/payment/service/CreditService.java`
around lines 41 - 50, The code currently treats a transaction as idempotent
based solely on matching (user, type, referenceId), but does not verify that the
amount in a retry request matches the original transaction amount. To fix this,
after retrieving the existingTransaction, calculate the transactionAmount for
the current request using resolveTransactionAmount, then compare this amount
with the existing transaction's amount before returning the old balance. If the
amounts do not match, reject the request by throwing an appropriate exception to
prevent amount mismatches from being silently ignored.
| try { | ||
| TossPaymentConfirmResponse tossResponse = | ||
| tossPaymentClient.confirm(request.paymentKey(), request.orderId(), request.amount()); | ||
| validateTossResponse(request, tossResponse); | ||
| } catch (RuntimeException e) { | ||
| paymentTransactionService.failConfirmation(validatedUser.getId(), request.orderId(), request.paymentKey()); | ||
| throw e; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect Toss client failure taxonomy and confirm whether
# PaymentService can distinguish definitive payment rejection from unknown outcomes.
echo "Toss payment client definitions/usages:"
rg -n -C4 'class TossPaymentClient|interface TossPaymentClient|confirm\s*\('
echo
echo "Payment confirmation exception/status handling:"
rg -n -C4 'PAYMENT_CONFIRM_FAILED|PAYMENT_ALREADY_PROCESSED|RestClientException|WebClientResponseException|HttpStatus|RuntimeException'Repository: JobDri-Developer/BackEnd
Length of output: 50380
Do not mark payments FAILED for unknown gateway outcomes.
TossPaymentClient.confirm() catches both HttpStatusCodeException (HTTP error responses) and RestClientException (timeouts, connection failures) and wraps both into GeneralException(GeneralErrorCode.PAYMENT_CONFIRM_FAILED) with no distinction. PaymentService.confirm() then treats all RuntimeExceptions identically, calling failConfirmation() to mark the payment FAILED before rethrowing.
This conflates definitive rejections with transient failures. If Toss approves but the response times out or the client fails to receive it, the payment becomes FAILED locally and unrecoverable.
Distinguish transient/unknown outcomes from definitive rejections in TossPaymentClient (e.g., map 4xx Toss responses to a specific exception type for true declines, leave transport failures as unknown). Only call failConfirmation() for definitive rejections; unknown outcomes should remain in PROCESSING for later reconciliation.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@src/main/java/com/jobdri/jobdri_api/domain/payment/service/PaymentService.java`
around lines 80 - 86, The TossPaymentClient.confirm() method currently wraps
both HTTP errors and transient failures (timeouts, connection issues) into a
single GeneralException, causing PaymentService.confirm() to treat all
exceptions identically and mark payments as FAILED. To fix this, modify
TossPaymentClient.confirm() to distinguish between definitive rejections (4xx
HTTP responses) and transient failures (connection timeouts, network errors) by
throwing different exception types for each case. Then in
PaymentService.confirm(), update the catch block to only call
paymentTransactionService.failConfirmation() when catching the exception type
that represents definitive rejections, allowing transient failures to remain
unhandled so the payment stays in PROCESSING state for later reconciliation.
| if (payment.getStatus() == PaymentStatus.PROCESSING || payment.getStatus() == PaymentStatus.FAILED) { | ||
| throw new GeneralException(GeneralErrorCode.PAYMENT_ALREADY_PROCESSED, "이미 처리된 결제입니다."); | ||
| } |
There was a problem hiding this comment.
Make same-key PROCESSING confirmations recoverable.
PaymentService.confirm commits PROCESSING before the Toss call; if the process dies or completion fails afterward, Line 35 rejects every retry and leaves the payment stuck without credit. Treat the same paymentKey as resumable, or add a lease/reconciliation path.
One possible local recovery path
- if (payment.getStatus() == PaymentStatus.PROCESSING || payment.getStatus() == PaymentStatus.FAILED) {
+ if (payment.getStatus() == PaymentStatus.PROCESSING) {
+ if (!payment.hasPaymentKey(request.paymentKey())) {
+ throw new GeneralException(GeneralErrorCode.PAYMENT_ALREADY_PROCESSED, "이미 처리된 결제입니다.");
+ }
+ return new PaymentConfirmationStart(payment, false);
+ }
+ if (payment.getStatus() == PaymentStatus.FAILED) {
throw new GeneralException(GeneralErrorCode.PAYMENT_ALREADY_PROCESSED, "이미 처리된 결제입니다.");
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@src/main/java/com/jobdri/jobdri_api/domain/payment/service/PaymentTransactionService.java`
around lines 35 - 37, The validation in PaymentTransactionService.confirm method
currently rejects all retries for payments in PROCESSING status by throwing an
exception on line 35, which prevents recovery if the confirmation process fails
after the status is set to PROCESSING. Modify the validation logic to allow
retries for the same paymentKey to be resumable; instead of unconditionally
throwing an exception for PROCESSING status, check if the incoming paymentKey
matches an existing PROCESSING payment and allow it to proceed as a resumable
operation rather than rejecting it as an error condition.
1 participant
✨ 어떤 이유로 PR를 하셨나요?
📋 세부 내용 - 왜 해당 PR이 필요한지 작업 내용을 자세하게 설명해주세요
결제 승인 흐름을 외부 PG 호출과 내부 DB 트랜잭션으로 분리했습니다.
결제 상태 전이를
PENDING -> PROCESSING -> COMPLETED/FAILED로 정리했습니다.크레딧 거래에
referenceId기반 멱등성을 추가해 중복 충전/차감/환불을 방지했습니다.결제 재시도, 승인 실패, 크레딧 멱등성 관련 테스트를 보강했습니다.
기존에는 외부 결제 승인 호출이 DB 트랜잭션 안에서 수행되어 락 점유 시간이 길고 장애 전파 위험이 있었습니다.
동일 요청 재시도나 복구 시 크레딧 원장 중복 반영 가능성을 줄일 필요가 있었습니다.
결제 상태를 더 명확하게 표현해 운영 중 추적성과 복구 가능성을 높이기 위함입니다.
📸 작업 화면 스크린샷
🚨 관련 이슈 번호 [#62]
Summary by CodeRabbit
New Features
Bug Fixes
Tests