[Fix] 결제 및 크레딧 동시성 처리 보강#71
Merged
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughHarden payment and credit flows: add DTO validation and non-nullability, introduce repository row-locking, adjust transactional behavior in CreditService, cache/configure Toss RestClient at startup, and add concurrency tests for payment confirmation and analysis credit usage. ChangesPayment & Credit Concurrency Safety
Sequence DiagramsequenceDiagram
participant Client
participant PaymentService
participant PaymentRepository
participant TossPaymentClient
participant CreditService
participant UserRepository
participant CreditTransactionRepo
Client->>PaymentService: confirm(orderId, paymentKey, amount)
PaymentService->>PaymentRepository: findByOrderIdForUpdate(orderId)
PaymentRepository-->>PaymentService: Payment (locked)
PaymentService->>TossPaymentClient: confirm(request)
TossPaymentClient-->>PaymentService: TossPaymentConfirmResponse
PaymentService->>CreditService: charge(user, amount, desc, refId)
CreditService->>UserRepository: findByIdForUpdate(userId)
UserRepository-->>CreditService: User (locked)
CreditService->>CreditService: validatePositiveAmount(amount)
CreditService->>CreditTransactionRepo: save(CHARGE)
CreditTransactionRepo-->>CreditService: persisted
PaymentService-->>Client: confirmationResult
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
🚥 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 docstrings
🧪 Generate unit tests (beta)
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: 1
🧹 Nitpick comments (3)
src/test/resources/application-test.yaml (1)
80-84: ⚡ Quick winAvoid using the live Toss base URL in the
testprofile
src/test/resources/application-test.yamlconfigurespayment.toss.base-url: https://api.tosspayments.com. WhilePaymentServiceTestmocksTossPaymentClient(@MockBean), other@SpringBootTestcoverage (e.g.,JobdriApiApplicationTests) still boots the realTossPaymentClientbean during context load—so keeping a live base URL risks unintended outbound calls and tiestestconfig to external environment.payment: toss: client-key: test-toss-client-key secret-key: test-toss-secret-key base-url: https://api.tosspayments.comPrefer a dummy/local stub base URL for
test(so the real client, if ever instantiated, never points to the production Toss endpoint).🤖 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/resources/application-test.yaml` around lines 80 - 84, The test profile should not point to the real Toss endpoint; update src/test/resources/application-test.yaml so payment.toss.base-url uses a local/dummy URL (e.g., http://localhost:PORT or http://127.0.0.1) instead of https://api.tosspayments.com so any real TossPaymentClient (or tests that boot the full context like JobdriApiApplicationTests) cannot make outbound calls; keep the test keys (payment.toss.client-key and payment.toss.secret-key) as-is and ensure tests that mock TossPaymentClient (e.g., PaymentServiceTest with `@MockBean`) still operate normally.src/main/java/com/jobdri/jobdri_api/domain/payment/service/CreditService.java (1)
43-50: 💤 Low valueRedundant refund when transaction rolls back.
Since
use()andrefund()now participate in the parent transaction (noREQUIRES_NEW), whenAnalysisService.analyze()catches aRuntimeException, callsrefund(), and rethrows, the entire transaction rolls back—including both theUSEandREFUNDoperations. The explicitrefund()call becomes a no-op.This isn't a bug (the final state is correct), but the compensation pattern is now redundant. Consider either:
- Removing the explicit
refund()call in callers and relying on rollback, or- Documenting that
refund()is for non-rollback scenarios (e.g., manual admin actions)🤖 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 43 - 50, The refund() method in CreditService now participates in the caller's transaction, so explicit compensation calls (e.g., calls to CreditService.refund() from AnalysisService.analyze() after catching a RuntimeException) are redundant because a rollback will undo the use() and refund() together; either remove those explicit refund() calls from callers like AnalysisService.analyze() and rely on transactional rollback, or update the CreditService.refund() javadoc and any developer docs to state that refund() is intended only for out-of-transaction/manual/admin compensations (non-rollback scenarios) so callers understand when to invoke it.src/test/java/com/jobdri/jobdri_api/domain/analysis/service/AnalysisServiceTest.java (1)
345-349: 💤 Low valueFragile assumption about initial credit value.
saveUserWithCreditassumesUser.signup()initializes credit to 1, then addscredit - 1. If the initial credit value inUser.signup()changes, this helper will silently produce incorrect credit values.Consider setting the credit directly or documenting the assumption:
private User saveUserWithCredit(String email, int credit) { User user = User.signup("테스트 사용자", email, "encoded-password"); - user.increaseCredit(credit - 1); + // User.signup() initializes credit to 1, so add (credit - 1) to reach target + int initialCredit = user.getCredit(); + user.increaseCredit(credit - initialCredit); return userRepository.save(user); }🤖 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/analysis/service/AnalysisServiceTest.java` around lines 345 - 349, The helper saveUserWithCredit assumes User.signup() starts with 1 credit; instead compute the needed delta from the actual initial value and adjust accordingly: after creating the user with User.signup(...) call user.getCredit() (or equivalent accessor) to get currentCredit, compute delta = credit - currentCredit and call user.increaseCredit(delta) (or set the credit explicitly via a setter/constructor if available) before saving; update references to saveUserWithCredit, User.signup, user.getCredit, and user.increaseCredit in the change.
🤖 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/service/TossPaymentClient.java`:
- Around line 62-71: In TossPaymentClient's exception handlers for
HttpStatusCodeException and RestClientException, stop including
e.getResponseBodyAsString() and e.getMessage() in the GeneralException thrown to
clients; instead throw a generic PAYMENT_CONFIRM_FAILED message, and log the
full exception details internally (e.g., using the class logger) after
sanitizing/truncating sensitive content for diagnostics. Update the catch blocks
around the HTTP call (the HttpStatusCodeException and RestClientException
handlers) to call logger.error(...) with the sanitized/truncated
response/message and stack trace, then throw a user-facing GeneralException with
a non-sensitive description.
---
Nitpick comments:
In
`@src/main/java/com/jobdri/jobdri_api/domain/payment/service/CreditService.java`:
- Around line 43-50: The refund() method in CreditService now participates in
the caller's transaction, so explicit compensation calls (e.g., calls to
CreditService.refund() from AnalysisService.analyze() after catching a
RuntimeException) are redundant because a rollback will undo the use() and
refund() together; either remove those explicit refund() calls from callers like
AnalysisService.analyze() and rely on transactional rollback, or update the
CreditService.refund() javadoc and any developer docs to state that refund() is
intended only for out-of-transaction/manual/admin compensations (non-rollback
scenarios) so callers understand when to invoke it.
In
`@src/test/java/com/jobdri/jobdri_api/domain/analysis/service/AnalysisServiceTest.java`:
- Around line 345-349: The helper saveUserWithCredit assumes User.signup()
starts with 1 credit; instead compute the needed delta from the actual initial
value and adjust accordingly: after creating the user with User.signup(...) call
user.getCredit() (or equivalent accessor) to get currentCredit, compute delta =
credit - currentCredit and call user.increaseCredit(delta) (or set the credit
explicitly via a setter/constructor if available) before saving; update
references to saveUserWithCredit, User.signup, user.getCredit, and
user.increaseCredit in the change.
In `@src/test/resources/application-test.yaml`:
- Around line 80-84: The test profile should not point to the real Toss
endpoint; update src/test/resources/application-test.yaml so
payment.toss.base-url uses a local/dummy URL (e.g., http://localhost:PORT or
http://127.0.0.1) instead of https://api.tosspayments.com so any real
TossPaymentClient (or tests that boot the full context like
JobdriApiApplicationTests) cannot make outbound calls; keep the test keys
(payment.toss.client-key and payment.toss.secret-key) as-is and ensure tests
that mock TossPaymentClient (e.g., PaymentServiceTest with `@MockBean`) still
operate normally.
🪄 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: 53e6a5d4-6aee-42ad-9482-d64e17c932b6
📒 Files selected for processing (12)
src/main/java/com/jobdri/jobdri_api/domain/payment/dto/response/PaymentPrepareResponse.javasrc/main/java/com/jobdri/jobdri_api/domain/payment/dto/toss/TossPaymentConfirmRequest.javasrc/main/java/com/jobdri/jobdri_api/domain/payment/dto/toss/TossPaymentConfirmResponse.javasrc/main/java/com/jobdri/jobdri_api/domain/payment/entity/Payment.javasrc/main/java/com/jobdri/jobdri_api/domain/payment/repository/PaymentRepository.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/TossPaymentClient.javasrc/main/java/com/jobdri/jobdri_api/domain/user/repository/UserRepository.javasrc/test/java/com/jobdri/jobdri_api/domain/analysis/service/AnalysisServiceTest.javasrc/test/java/com/jobdri/jobdri_api/domain/payment/service/PaymentServiceTest.javasrc/test/resources/application-test.yaml
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/service/TossPaymentClient.java`:
- Around line 67-72: In TossPaymentClient's catch blocks for
HttpStatusCodeException and RestClientException, fix the SLF4J
placeholder/argument mismatch by logging structured fields and the throwable
separately: adjust the existing log.warn calls (the one using e.getStatusCode()
and truncate(e.getResponseBodyAsString()) and the one using e.getMessage()) so
each log.warn only supplies arguments that match its {} placeholders (remove the
throwable from that varargs list), and then emit the stack trace with a separate
log.warn or log.warn("Toss payment confirm failed", e) call; reference the
existing catch blocks and the truncate(...) usage to locate where to change.
🪄 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: 345a13b1-7c47-4e19-82ae-b59f2f47e591
📒 Files selected for processing (1)
src/main/java/com/jobdri/jobdri_api/domain/payment/service/TossPaymentClient.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
✨ 어떤 이유로 PR를 하셨나요?
📋 세부 내용 - 왜 해당 PR이 필요한지 작업 내용을 자세하게 설명해주세요
📸 작업 화면 스크린샷
🚨 관련 이슈 번호 [#62 #70 ]
Summary by CodeRabbit
New Features
Bug Fixes
Improvements
Tests