[Feat] 토스페이먼츠 기반 크레딧 결제 구현#70
Merged
Merged
Conversation
- 크레딧 가격 플랜 조회 API 추가 - 토스 결제 준비 및 승인 API 추가 - 결제 완료 시 크레딧 충전 처리 - 크레딧 잔액 및 거래 내역 조회 API 추가 - 자소서 분석 시작 시 크레딧 선차감 적용 - 분석 실패 시 크레딧 자동 환불 처리 - 결제/크레딧 관련 엔티티 및 테스트 추가
📝 WalkthroughWalkthroughThis PR introduces a complete payment and credit system for JobDri, integrating TOSS Payments gateway for credit purchase and implementing credit-based deduction for analysis features. The change includes domain models, transactional credit services, payment orchestration, REST APIs, configuration, and test coverage, with credit management wired into the existing analysis service flow. ChangesPayment & Credit System
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
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 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: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/test/java/com/jobdri/jobdri_api/domain/analysis/service/AnalysisServiceTest.java (1)
79-118: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winThe new credit side effects added to
analyze(...)are still untested.This suite now seeds users with credits, but none of the tests verify that a successful analysis decrements the balance or that an LLM/persistence failure restores the deducted credit. Those are the core behavior changes in this PR, so I'd add one success-path balance assertion and one failure-path refund test here.
🤖 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 79 - 118, Add tests that assert credit side effects: after calling analysisService.analyze(user, mockApply.getId()) in the success test (using the existing saveUser seed), assert the user's credit balance decreased by the expected amount by reloading User via userRepository.findById(...) or the existing saveUser return value; and add a new failure-path test that mocks analysisAiClient.analyze(...) (or make analysisRepository.save(...) throw) to simulate an LLM/persistence error, then assert the originally deducted credit was refunded (balance unchanged) and that no Analysis persisted. Use the existing symbols analyze (AnalysisService.analyze), analysisAiClient.analyze, analysisRepository.save and userRepository.findById / saveUser to locate and implement the assertions and failure test.
🧹 Nitpick comments (3)
src/test/java/com/jobdri/jobdri_api/domain/payment/service/PaymentServiceTest.java (1)
62-74: ⚡ Quick winActually assert the persisted payment is
PENDING.Right now this only proves that a row was created. If
prepare(...)saved the wrong initial status, the test would still pass.✅ Small assertion addition
- assertThat(paymentRepository.findByOrderId(response.orderId())).isPresent(); + var payment = paymentRepository.findByOrderId(response.orderId()).orElseThrow(); + assertThat(payment.getStatus()).isEqualTo(PaymentStatus.PENDING); + assertThat(payment.getAmount()).isEqualTo(11500);🤖 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 62 - 74, Test currently only asserts a row was created; fetch the persisted Payment via paymentRepository.findByOrderId(response.orderId()) in PaymentServiceTest.prepare() and assert its status is PaymentStatus.PENDING (or the enum/constant used by your domain) so the test verifies the initial saved status from paymentService.prepare(user, new PaymentPrepareRequest("FIVE_TIMES")) is PENDING.src/main/java/com/jobdri/jobdri_api/domain/payment/service/PaymentService.java (1)
102-113: 🏗️ Heavy liftTransaction history retrieval should be paginated.
Current implementation always loads all rows for a user, which will degrade latency and memory as history grows.
🤖 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 102 - 113, getTransactions in PaymentService currently fetches all rows for a user; change it to return a paginated result by accepting pagination inputs (e.g., Pageable or page/size params) and using the repository pageable variants (replace findAllByUserIdOrderByCreatedAtDescIdDesc and findAllByUserIdAndTypeOrderByCreatedAtDescIdDesc with repository methods that accept a Pageable). Update PaymentService.getTransactions(User user, ...) to validate the user, build a PageRequest (or use the passed Pageable), call creditTransactionRepository.findAllByUserId(..., pageable) or findAllByUserIdAndType(..., pageable), and map the Page content to CreditTransactionResponse (returning a Page<CreditTransactionResponse> or a DTO containing content + paging metadata). Ensure repository signatures and the controller/service callers are updated accordingly.src/main/java/com/jobdri/jobdri_api/domain/payment/dto/toss/TossPaymentConfirmResponse.java (1)
8-8: 💤 Low valueUse primitive
intforTossPaymentConfirmResponse.totalAmount
TOSS Payments’ payment confirmation response payload includestotalAmount, and your code already treats it as required (PaymentService.validateTossResponsethrows whenresponse.totalAmount() == null). SwitchingtotalAmounttoint(and optionally simplifying the null check) improves consistency withTossPaymentConfirmRequest.amountand reduces nullable handling.🤖 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/dto/toss/TossPaymentConfirmResponse.java` at line 8, The DTO field TossPaymentConfirmResponse.totalAmount is declared as Integer but is treated as required; change its type to primitive int in TossPaymentConfirmResponse and update any constructors/builders to accept int so nullability is removed, then simplify the null-check in PaymentService.validateTossResponse (or remove response.totalAmount() == null checks) to rely on the primitive, keeping consistency with TossPaymentConfirmRequest.amount and avoiding unnecessary nullable handling.
🤖 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/analysis/service/AnalysisService.java`:
- Around line 61-65: The code currently calls creditService.use(...) before the
external LLM call (analysisAiClient.analyze), which can permanently consume user
credits if the process dies; change this to a durable reserve/finalize flow:
call a reservation method (e.g., creditService.reserve(user, 1, referenceId) or
create a CreditReservation) before calling
AnalysisService.analyze/analysisAiClient.analyze, then after a successful
llmResponse call finalize/commit the reservation (e.g.,
creditService.finalizeReservation(reservation) or
creditService.useReserved(...)); on any failure or exception cancel the
reservation (e.g., creditService.cancelReservation(reservation)). Also apply the
same reservation/finalize change to the other similar deduction sites mentioned
(lines around the second occurrence of creditService.use). Ensure method names
(reserve, finalizeReservation/cancelReservation) exist or add them to
CreditService to implement durable reconciliation.
In
`@src/main/java/com/jobdri/jobdri_api/domain/payment/dto/response/PaymentPrepareResponse.java`:
- Line 22: The expression payment.getUser().getEmail() in PaymentPrepareResponse
may NPE if Payment.user is null; update the constructor/factory in
PaymentPrepareResponse (where payment is accessed) to defensively handle a null
user by either using a null-safe access (e.g., userEmail = payment.getUser() !=
null ? payment.getUser().getEmail() : null) or by calling
Objects.requireNonNull(payment.getUser(), "Payment.user must not be null") if
the domain invariant requires a non-null user; alternatively, enforce the
invariant at the entity level by adding `@NotNull` to the Payment.user association
and documenting that Payment always contains a user.
In
`@src/main/java/com/jobdri/jobdri_api/domain/payment/dto/toss/TossPaymentConfirmRequest.java`:
- Around line 3-7: TossPaymentConfirmRequest lacks Bean Validation on its
components; annotate the record components paymentKey and orderId with `@NotBlank`
and annotate amount with `@Positive` (importing the appropriate
javax/jakarta.validation annotations consistent with the project) so validation
occurs before sending to the external TOSS API; update TossPaymentConfirmRequest
to declare these annotations on the record components and ensure any DTO
mapping/validation pipeline picks them up.
In `@src/main/java/com/jobdri/jobdri_api/domain/payment/entity/Payment.java`:
- Around line 28-35: Payment entity allows nulls for lifecycle-required fields;
mark orderId and planCode as non-nullable in the JPA mapping. Update the Payment
class by changing the `@Column` annotations for orderId and planCode to include
nullable = false (and optionally add javax.validation.constraints.NotNull on
those fields) so the database schema and JPA enforce non-null constraints for
orderId and planCode while leaving paymentKey as-is.
In
`@src/main/java/com/jobdri/jobdri_api/domain/payment/service/CreditService.java`:
- Around line 22-43: The current `@Transactional`(propagation =
Propagation.REQUIRES_NEW) on CreditService methods (charge, use, refund) causes
credit changes to commit even if an outer payment transaction rolls back; change
these methods to use default/REQUIRED propagation so credit updates participate
in the caller transaction (remove Propagation.REQUIRES_NEW from charge, use,
refund and rely on getManagedUser/saveTransaction within the same tx). If truly
isolated commits are needed for specific call paths, move those isolated
operations into separate methods in a different bean and annotate only those
methods with `@Transactional`(propagation = REQUIRES_NEW) so Spring proxy
semantics apply (do not keep REQUIRES_NEW on the existing charge/use/refund
methods).
- Around line 23-47: All three public methods charge, use, and refund must
validate that amount is a positive integer at the service boundary and reject
zero or negative values before performing any user retrieval/mutation; add an
initial guard in CreditService. For invalid amounts throw the appropriate
GeneralException (use an existing error code or add one like INVALID_AMOUNT)
with a clear message, and only call getManagedUser(), increaseCredit(),
decreaseCredit(), and saveTransaction after the check passes; ensure use still
handles insufficient credit via its existing try/catch.
In
`@src/main/java/com/jobdri/jobdri_api/domain/payment/service/PaymentService.java`:
- Around line 37-38: The PaymentService currently injects tossClientKey with a
default empty string which can hide missing configuration; update PaymentService
to fail fast by removing the empty default and/or adding a validation that
throws an exception when tossClientKey is null or blank (e.g. in the constructor
or a `@PostConstruct` method) so startup fails with a clear message; reference
tossClientKey and the PaymentService class to locate where to validate and throw
an IllegalStateException if the key is missing.
- Around line 64-87: The confirm method is vulnerable to double-processing under
concurrency; make confirm execute within a transaction and fetch the Payment
with a row-level lock before checking status and completing it (e.g., replace
paymentRepository.findByOrderId(...) in PaymentService.confirm with a locked
read such as paymentRepository.findByOrderIdForUpdate(...) or a repository
method annotated with `@Lock`(PESSIMISTIC_WRITE)/SELECT ... FOR UPDATE), then
perform the status check, toss validation, payment.complete(...) and
creditService.charge(...) while the transaction/lock is held to prevent
concurrent requests from both passing the PENDING check.
In
`@src/main/java/com/jobdri/jobdri_api/domain/payment/service/TossPaymentClient.java`:
- Around line 24-25: The `@Value` injection in TossPaymentClient currently uses a
permissive empty default for payment.toss.secret-key which lets the app start
without a key; remove the empty default from the `@Value` annotation on the
secretKey field so Spring fails to start when the property is missing (i.e.,
change the `@Value`("${payment.toss.secret-key:}") usage to require the property),
and add a clear check in the TossPaymentClient constructor or an `@PostConstruct`
that throws an explicit IllegalStateException if secretKey is null/blank to
ensure fail-fast behavior with a clear error message.
- Around line 30-49: The confirm method in TossPaymentClient currently builds a
REST client without any timeouts; update the restClientBuilder usage in confirm
to configure sensible connect and read (and optionally write) timeouts on the
client before calling .build() so the external TOSS call cannot block
indefinitely (e.g., set connectTimeout and response/read timeout on the
underlying HTTP client used by restClientBuilder). Ensure the configured
timeouts apply to the client built in TossPaymentClient.confirm and that timeout
exceptions still propagate as RestClientException (so the existing catch and
GeneralException mapping remains valid).
- Around line 43-48: The catch block in TossPaymentClient currently swallows
RestClientException details; change error handling to first catch
HttpStatusCodeException (to access e.getStatusCode() and
e.getResponseBodyAsString()) and include those details in the thrown
GeneralException (or its message/metadata) so TOSS HTTP status and response body
are preserved for debugging, then keep a fallback catch for generic
RestClientException that still preserves e.getMessage() or sets e as the cause;
also optionally log the extracted status and response body before rethrowing.
- Around line 32-34: The confirm() method in TossPaymentClient currently builds
a new RestClient via restClientBuilder.baseUrl(baseUrl).build() on every call;
instead, initialize and reuse a single RestClient instance (e.g., a private
final or volatile RestClient field) by building it once in the constructor or a
`@PostConstruct` method and then have confirm() use that cached RestClient; update
TossPaymentClient to add the RestClient field, move the
restClientBuilder.baseUrl(baseUrl).build() call into initialization, and remove
per-call builds to avoid repeated allocation.
---
Outside diff comments:
In
`@src/test/java/com/jobdri/jobdri_api/domain/analysis/service/AnalysisServiceTest.java`:
- Around line 79-118: Add tests that assert credit side effects: after calling
analysisService.analyze(user, mockApply.getId()) in the success test (using the
existing saveUser seed), assert the user's credit balance decreased by the
expected amount by reloading User via userRepository.findById(...) or the
existing saveUser return value; and add a new failure-path test that mocks
analysisAiClient.analyze(...) (or make analysisRepository.save(...) throw) to
simulate an LLM/persistence error, then assert the originally deducted credit
was refunded (balance unchanged) and that no Analysis persisted. Use the
existing symbols analyze (AnalysisService.analyze), analysisAiClient.analyze,
analysisRepository.save and userRepository.findById / saveUser to locate and
implement the assertions and failure test.
---
Nitpick comments:
In
`@src/main/java/com/jobdri/jobdri_api/domain/payment/dto/toss/TossPaymentConfirmResponse.java`:
- Line 8: The DTO field TossPaymentConfirmResponse.totalAmount is declared as
Integer but is treated as required; change its type to primitive int in
TossPaymentConfirmResponse and update any constructors/builders to accept int so
nullability is removed, then simplify the null-check in
PaymentService.validateTossResponse (or remove response.totalAmount() == null
checks) to rely on the primitive, keeping consistency with
TossPaymentConfirmRequest.amount and avoiding unnecessary nullable handling.
In
`@src/main/java/com/jobdri/jobdri_api/domain/payment/service/PaymentService.java`:
- Around line 102-113: getTransactions in PaymentService currently fetches all
rows for a user; change it to return a paginated result by accepting pagination
inputs (e.g., Pageable or page/size params) and using the repository pageable
variants (replace findAllByUserIdOrderByCreatedAtDescIdDesc and
findAllByUserIdAndTypeOrderByCreatedAtDescIdDesc with repository methods that
accept a Pageable). Update PaymentService.getTransactions(User user, ...) to
validate the user, build a PageRequest (or use the passed Pageable), call
creditTransactionRepository.findAllByUserId(..., pageable) or
findAllByUserIdAndType(..., pageable), and map the Page content to
CreditTransactionResponse (returning a Page<CreditTransactionResponse> or a DTO
containing content + paging metadata). Ensure repository signatures and the
controller/service callers are updated accordingly.
In
`@src/test/java/com/jobdri/jobdri_api/domain/payment/service/PaymentServiceTest.java`:
- Around line 62-74: Test currently only asserts a row was created; fetch the
persisted Payment via paymentRepository.findByOrderId(response.orderId()) in
PaymentServiceTest.prepare() and assert its status is PaymentStatus.PENDING (or
the enum/constant used by your domain) so the test verifies the initial saved
status from paymentService.prepare(user, new
PaymentPrepareRequest("FIVE_TIMES")) is PENDING.
🪄 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: 92fede2c-ede1-409e-9565-d06ec79211f4
📒 Files selected for processing (27)
.env.example.env.production.examplesrc/main/java/com/jobdri/jobdri_api/domain/analysis/service/AnalysisService.javasrc/main/java/com/jobdri/jobdri_api/domain/payment/controller/PaymentController.javasrc/main/java/com/jobdri/jobdri_api/domain/payment/dto/request/PaymentConfirmRequest.javasrc/main/java/com/jobdri/jobdri_api/domain/payment/dto/request/PaymentPrepareRequest.javasrc/main/java/com/jobdri/jobdri_api/domain/payment/dto/response/CreditBalanceResponse.javasrc/main/java/com/jobdri/jobdri_api/domain/payment/dto/response/CreditPlanResponse.javasrc/main/java/com/jobdri/jobdri_api/domain/payment/dto/response/CreditTransactionResponse.javasrc/main/java/com/jobdri/jobdri_api/domain/payment/dto/response/PaymentConfirmResponse.javasrc/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/CreditPlan.javasrc/main/java/com/jobdri/jobdri_api/domain/payment/entity/CreditTransaction.javasrc/main/java/com/jobdri/jobdri_api/domain/payment/entity/CreditTransactionType.javasrc/main/java/com/jobdri/jobdri_api/domain/payment/entity/Payment.javasrc/main/java/com/jobdri/jobdri_api/domain/payment/repository/CreditTransactionRepository.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/global/apiPayload/code/GeneralErrorCode.javasrc/main/resources/application-dev.yamlsrc/main/resources/application-prod.yamlsrc/test/java/com/jobdri/jobdri_api/domain/analysis/service/AnalysisServiceTest.javasrc/test/java/com/jobdri/jobdri_api/domain/payment/service/PaymentServiceTest.java
Comment on lines
+61
to
+65
| String referenceId = "mockApplyId=" + mockApply.getId(); | ||
| creditService.use(user, 1, "자소서 분석 크레딧 차감", referenceId); | ||
|
|
||
| Analysis analysis = analysisRepository.save(Analysis.create( | ||
| mockApply, | ||
| clampScore(llmResponse.score()), | ||
| clampScore(llmResponse.jobFit()), | ||
| clampScore(llmResponse.impact()), | ||
| clampScore(llmResponse.completeness()), | ||
| normalizeFeedback(llmResponse.feedback()) | ||
| )); | ||
| try { | ||
| AnalysisLlmResponse llmResponse = analysisAiClient.analyze(mockApply.getJobPosting(), answeredQuestions); |
There was a problem hiding this comment.
Pre-committing the credit deduction here can permanently strand user credits.
creditService.use(...) is committed before the external LLM call, and the refund is only best-effort inside this thread's catch. If the process dies, the request times out, or the node is terminated between those two steps, the user keeps the deducted credit with no analysis result and no refund. For this flow, the charge needs to stay in the outer transaction or be modeled as a reservation/finalize flow with durable reconciliation.
Also applies to: 82-84
🤖 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/analysis/service/AnalysisService.java`
around lines 61 - 65, The code currently calls creditService.use(...) before the
external LLM call (analysisAiClient.analyze), which can permanently consume user
credits if the process dies; change this to a durable reserve/finalize flow:
call a reservation method (e.g., creditService.reserve(user, 1, referenceId) or
create a CreditReservation) before calling
AnalysisService.analyze/analysisAiClient.analyze, then after a successful
llmResponse call finalize/commit the reservation (e.g.,
creditService.finalizeReservation(reservation) or
creditService.useReserved(...)); on any failure or exception cancel the
reservation (e.g., creditService.cancelReservation(reservation)). Also apply the
same reservation/finalize change to the other similar deduction sites mentioned
(lines around the second occurrence of creditService.use). Ensure method names
(reserve, finalizeReservation/cancelReservation) exist or add them to
CreditService to implement durable reconciliation.
| payment.getPrice(), | ||
| payment.getCreditAmount(), | ||
| clientKey, | ||
| payment.getUser().getEmail() |
There was a problem hiding this comment.
Potential NPE on user access.
payment.getUser().getEmail() will throw a NullPointerException if the user association is null. Verify that the Payment entity guarantees a non-null user, or add defensive null handling.
🛡️ Proposed defensive fix
- payment.getUser().getEmail()
+ payment.getUser() != null ? payment.getUser().getEmail() : nullAlternatively, if user is always required, ensure the Payment entity enforces this with @NotNull on the association and document this invariant.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| payment.getUser().getEmail() | |
| payment.getUser() != null ? payment.getUser().getEmail() : null |
🤖 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/dto/response/PaymentPrepareResponse.java`
at line 22, The expression payment.getUser().getEmail() in
PaymentPrepareResponse may NPE if Payment.user is null; update the
constructor/factory in PaymentPrepareResponse (where payment is accessed) to
defensively handle a null user by either using a null-safe access (e.g.,
userEmail = payment.getUser() != null ? payment.getUser().getEmail() : null) or
by calling Objects.requireNonNull(payment.getUser(), "Payment.user must not be
null") if the domain invariant requires a non-null user; alternatively, enforce
the invariant at the entity level by adding `@NotNull` to the Payment.user
association and documenting that Payment always contains a user.
Comment on lines
+3
to
+7
| public record TossPaymentConfirmRequest( | ||
| String paymentKey, | ||
| String orderId, | ||
| int amount | ||
| ) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add validation annotations for external API request.
This DTO is sent to the TOSS Payments API but lacks validation. Consider adding Bean Validation annotations to prevent invalid data:
@NotBlankonpaymentKeyandorderId@Positiveonamountto reject negative values
🔒 Proposed validation
+import jakarta.validation.constraints.NotBlank;
+import jakarta.validation.constraints.Positive;
+
public record TossPaymentConfirmRequest(
+ `@NotBlank`
String paymentKey,
+ `@NotBlank`
String orderId,
+ `@Positive`
int amount
) {
}🤖 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/dto/toss/TossPaymentConfirmRequest.java`
around lines 3 - 7, TossPaymentConfirmRequest lacks Bean Validation on its
components; annotate the record components paymentKey and orderId with `@NotBlank`
and annotate amount with `@Positive` (importing the appropriate
javax/jakarta.validation annotations consistent with the project) so validation
occurs before sending to the external TOSS API; update TossPaymentConfirmRequest
to declare these annotations on the record components and ensure any DTO
mapping/validation pipeline picks them up.
Comment on lines
+28
to
+35
| @Column(unique = true) | ||
| private String orderId; | ||
|
|
||
| @Column(unique = true) | ||
| private String paymentKey; | ||
|
|
||
| @Column | ||
| private String planCode; |
There was a problem hiding this comment.
Enforce non-null constraints for mandatory payment fields.
orderId and planCode are lifecycle-required, but DB columns currently allow nulls. That weakens persistence invariants and can create unresolvable payment rows.
Suggested fix
- `@Column`(unique = true)
+ `@Column`(nullable = false, unique = true)
private String orderId;
- `@Column`
+ `@Column`(nullable = false)
private String planCode;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| @Column(unique = true) | |
| private String orderId; | |
| @Column(unique = true) | |
| private String paymentKey; | |
| @Column | |
| private String planCode; | |
| `@Column`(nullable = false, unique = true) | |
| private String orderId; | |
| `@Column`(unique = true) | |
| private String paymentKey; | |
| `@Column`(nullable = false) | |
| private String planCode; |
🤖 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/Payment.java`
around lines 28 - 35, Payment entity allows nulls for lifecycle-required fields;
mark orderId and planCode as non-nullable in the JPA mapping. Update the Payment
class by changing the `@Column` annotations for orderId and planCode to include
nullable = false (and optionally add javax.validation.constraints.NotNull on
those fields) so the database schema and JPA enforce non-null constraints for
orderId and planCode while leaving paymentKey as-is.
Comment on lines
+22
to
+43
| @Transactional(propagation = Propagation.REQUIRES_NEW) | ||
| public int charge(User user, int amount, String description, String referenceId) { | ||
| User managedUser = getManagedUser(user); | ||
| managedUser.increaseCredit(amount); | ||
| saveTransaction(managedUser, CreditTransactionType.CHARGE, amount, description, referenceId); | ||
| return managedUser.getCredit(); | ||
| } | ||
|
|
||
| @Transactional(propagation = Propagation.REQUIRES_NEW) | ||
| public int use(User user, int amount, String description, String referenceId) { | ||
| User managedUser = getManagedUser(user); | ||
| try { | ||
| managedUser.decreaseCredit(amount); | ||
| } catch (IllegalArgumentException e) { | ||
| throw new GeneralException(GeneralErrorCode.INSUFFICIENT_CREDIT, "크레딧이 부족합니다."); | ||
| } | ||
| saveTransaction(managedUser, CreditTransactionType.USE, -amount, description, referenceId); | ||
| return managedUser.getCredit(); | ||
| } | ||
|
|
||
| @Transactional(propagation = Propagation.REQUIRES_NEW) | ||
| public int refund(User user, int amount, String description, String referenceId) { |
There was a problem hiding this comment.
REQUIRES_NEW here can commit credits even when outer payment transaction rolls back.
Because credit updates run in independent transactions, caller rollback can leave partial state (e.g., credit charged but payment status not completed).
Suggested direction
- `@Transactional`(propagation = Propagation.REQUIRES_NEW)
+ `@Transactional`
public int charge(User user, int amount, String description, String referenceId) { ... }
- `@Transactional`(propagation = Propagation.REQUIRES_NEW)
+ `@Transactional`
public int use(User user, int amount, String description, String referenceId) { ... }
- `@Transactional`(propagation = Propagation.REQUIRES_NEW)
+ `@Transactional`
public int refund(User user, int amount, String description, String referenceId) { ... }If independent transaction semantics are required for specific call paths, split those into dedicated methods instead of making all credit mutations REQUIRES_NEW.
🤖 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 22 - 43, The current `@Transactional`(propagation =
Propagation.REQUIRES_NEW) on CreditService methods (charge, use, refund) causes
credit changes to commit even if an outer payment transaction rolls back; change
these methods to use default/REQUIRED propagation so credit updates participate
in the caller transaction (remove Propagation.REQUIRES_NEW from charge, use,
refund and rely on getManagedUser/saveTransaction within the same tx). If truly
isolated commits are needed for specific call paths, move those isolated
operations into separate methods in a different bean and annotate only those
methods with `@Transactional`(propagation = REQUIRES_NEW) so Spring proxy
semantics apply (do not keep REQUIRES_NEW on the existing charge/use/refund
methods).
Comment on lines
+64
to
+87
| public PaymentConfirmResponse confirm(User user, PaymentConfirmRequest request) { | ||
| User validatedUser = userService.validateUser(user); | ||
| Payment payment = paymentRepository.findByOrderId(request.orderId()) | ||
| .orElseThrow(() -> new GeneralException( | ||
| GeneralErrorCode.PAYMENT_NOT_FOUND, | ||
| "결제 정보를 찾을 수 없습니다. orderId=" + request.orderId() | ||
| )); | ||
|
|
||
| if (!payment.getUser().getId().equals(validatedUser.getId())) { | ||
| throw new GeneralException(GeneralErrorCode.FORBIDDEN, "해당 결제에 접근할 수 없습니다."); | ||
| } | ||
| if (payment.getStatus() != PaymentStatus.PENDING) { | ||
| throw new GeneralException(GeneralErrorCode.PAYMENT_ALREADY_PROCESSED, "이미 처리된 결제입니다."); | ||
| } | ||
| if (payment.getPrice() != request.amount()) { | ||
| throw new GeneralException(GeneralErrorCode.PAYMENT_AMOUNT_MISMATCH, "결제 금액이 일치하지 않습니다."); | ||
| } | ||
|
|
||
| TossPaymentConfirmResponse tossResponse = | ||
| tossPaymentClient.confirm(request.paymentKey(), request.orderId(), request.amount()); | ||
| validateTossResponse(request, tossResponse); | ||
|
|
||
| payment.complete(request.paymentKey()); | ||
| int creditBalance = creditService.charge( |
There was a problem hiding this comment.
confirm is vulnerable to double-processing under concurrent requests.
Two requests can both pass the PENDING check before either commits, leading to duplicate credit charge for one order.
Suggested fix (row lock during confirm)
// PaymentRepository.java
+import org.springframework.data.jpa.repository.Lock;
+import jakarta.persistence.LockModeType;
@@
+ `@Lock`(LockModeType.PESSIMISTIC_WRITE)
+ Optional<Payment> findByOrderId(String orderId);// PaymentService.java
- Payment payment = paymentRepository.findByOrderId(request.orderId())
+ Payment payment = paymentRepository.findByOrderId(request.orderId())
.orElseThrow(() -> new GeneralException(
GeneralErrorCode.PAYMENT_NOT_FOUND,
"결제 정보를 찾을 수 없습니다. orderId=" + request.orderId()
));(If this repository method is reused elsewhere, consider adding a dedicated findByOrderIdForUpdate method instead.)
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| public PaymentConfirmResponse confirm(User user, PaymentConfirmRequest request) { | |
| User validatedUser = userService.validateUser(user); | |
| Payment payment = paymentRepository.findByOrderId(request.orderId()) | |
| .orElseThrow(() -> new GeneralException( | |
| GeneralErrorCode.PAYMENT_NOT_FOUND, | |
| "결제 정보를 찾을 수 없습니다. orderId=" + request.orderId() | |
| )); | |
| if (!payment.getUser().getId().equals(validatedUser.getId())) { | |
| throw new GeneralException(GeneralErrorCode.FORBIDDEN, "해당 결제에 접근할 수 없습니다."); | |
| } | |
| if (payment.getStatus() != PaymentStatus.PENDING) { | |
| throw new GeneralException(GeneralErrorCode.PAYMENT_ALREADY_PROCESSED, "이미 처리된 결제입니다."); | |
| } | |
| if (payment.getPrice() != request.amount()) { | |
| throw new GeneralException(GeneralErrorCode.PAYMENT_AMOUNT_MISMATCH, "결제 금액이 일치하지 않습니다."); | |
| } | |
| TossPaymentConfirmResponse tossResponse = | |
| tossPaymentClient.confirm(request.paymentKey(), request.orderId(), request.amount()); | |
| validateTossResponse(request, tossResponse); | |
| payment.complete(request.paymentKey()); | |
| int creditBalance = creditService.charge( | |
| public PaymentConfirmResponse confirm(User user, PaymentConfirmRequest request) { | |
| User validatedUser = userService.validateUser(user); | |
| Payment payment = paymentRepository.findByOrderId(request.orderId()) | |
| .orElseThrow(() -> new GeneralException( | |
| GeneralErrorCode.PAYMENT_NOT_FOUND, | |
| "결제 정보를 찾을 수 없습니다. orderId=" + request.orderId() | |
| )); | |
| if (!payment.getUser().getId().equals(validatedUser.getId())) { | |
| throw new GeneralException(GeneralErrorCode.FORBIDDEN, "해당 결제에 접근할 수 없습니다."); | |
| } | |
| if (payment.getStatus() != PaymentStatus.PENDING) { | |
| throw new GeneralException(GeneralErrorCode.PAYMENT_ALREADY_PROCESSED, "이미 처리된 결제입니다."); | |
| } | |
| if (payment.getPrice() != request.amount()) { | |
| throw new GeneralException(GeneralErrorCode.PAYMENT_AMOUNT_MISMATCH, "결제 금액이 일치하지 않습니다."); | |
| } | |
| TossPaymentConfirmResponse tossResponse = | |
| tossPaymentClient.confirm(request.paymentKey(), request.orderId(), request.amount()); | |
| validateTossResponse(request, tossResponse); | |
| payment.complete(request.paymentKey()); | |
| int creditBalance = creditService.charge( |
🤖 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 64 - 87, The confirm method is vulnerable to double-processing
under concurrency; make confirm execute within a transaction and fetch the
Payment with a row-level lock before checking status and completing it (e.g.,
replace paymentRepository.findByOrderId(...) in PaymentService.confirm with a
locked read such as paymentRepository.findByOrderIdForUpdate(...) or a
repository method annotated with `@Lock`(PESSIMISTIC_WRITE)/SELECT ... FOR
UPDATE), then perform the status check, toss validation, payment.complete(...)
and creditService.charge(...) while the transaction/lock is held to prevent
concurrent requests from both passing the PENDING check.
Comment on lines
+24
to
+25
| @Value("${payment.toss.secret-key:}") | ||
| private String secretKey; |
There was a problem hiding this comment.
Remove empty default for secret-key to fail fast.
An empty string default for payment.toss.secret-key allows the application to start with invalid configuration and fail at runtime during payment attempts. It's better to fail fast at startup if the secret key is missing.
🛡️ Proposed fix to remove unsafe default
- `@Value`("${payment.toss.secret-key:}")
+ `@Value`("${payment.toss.secret-key}")
private String secretKey;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| @Value("${payment.toss.secret-key:}") | |
| private String secretKey; | |
| `@Value`("${payment.toss.secret-key}") | |
| private String secretKey; |
🤖 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/TossPaymentClient.java`
around lines 24 - 25, The `@Value` injection in TossPaymentClient currently uses a
permissive empty default for payment.toss.secret-key which lets the app start
without a key; remove the empty default from the `@Value` annotation on the
secretKey field so Spring fails to start when the property is missing (i.e.,
change the `@Value`("${payment.toss.secret-key:}") usage to require the property),
and add a clear check in the TossPaymentClient constructor or an `@PostConstruct`
that throws an explicit IllegalStateException if secretKey is null/blank to
ensure fail-fast behavior with a clear error message.
Comment on lines
+30
to
+49
| public TossPaymentConfirmResponse confirm(String paymentKey, String orderId, int amount) { | ||
| try { | ||
| return restClientBuilder | ||
| .baseUrl(baseUrl) | ||
| .build() | ||
| .post() | ||
| .uri("/v1/payments/confirm") | ||
| .header(HttpHeaders.AUTHORIZATION, authorizationHeader()) | ||
| .header("Idempotency-Key", orderId) | ||
| .contentType(MediaType.APPLICATION_JSON) | ||
| .body(new TossPaymentConfirmRequest(paymentKey, orderId, amount)) | ||
| .retrieve() | ||
| .body(TossPaymentConfirmResponse.class); | ||
| } catch (RestClientException e) { | ||
| throw new GeneralException( | ||
| GeneralErrorCode.PAYMENT_CONFIRM_FAILED, | ||
| "토스페이먼츠 결제 승인에 실패했습니다." | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
Add timeout configuration to prevent indefinite blocking.
The external call to TOSS Payments API has no timeout configuration, which can block request threads indefinitely if the TOSS service is slow or unresponsive. This is a reliability risk that can cause cascading failures.
⏱️ Proposed fix to add timeout configuration
+import java.time.Duration;
+
`@PostConstruct`
public void init() {
this.restClient = restClientBuilder
.baseUrl(baseUrl)
+ .defaultRequest(request -> {
+ request.httpRequest(req -> {
+ req.setConnectTimeout(Duration.ofSeconds(5));
+ req.setReadTimeout(Duration.ofSeconds(10));
+ });
+ })
.build();
}🤖 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/TossPaymentClient.java`
around lines 30 - 49, The confirm method in TossPaymentClient currently builds a
REST client without any timeouts; update the restClientBuilder usage in confirm
to configure sensible connect and read (and optionally write) timeouts on the
client before calling .build() so the external TOSS call cannot block
indefinitely (e.g., set connectTimeout and response/read timeout on the
underlying HTTP client used by restClientBuilder). Ensure the configured
timeouts apply to the client built in TossPaymentClient.confirm and that timeout
exceptions still propagate as RestClientException (so the existing catch and
GeneralException mapping remains valid).
Comment on lines
+32
to
+34
| return restClientBuilder | ||
| .baseUrl(baseUrl) | ||
| .build() |
There was a problem hiding this comment.
Reuse RestClient instance instead of building per request.
Building a new RestClient on every confirm() call is inefficient and creates unnecessary object allocation overhead. The client should be built once (e.g., in a @PostConstruct method or constructor) and reused across all requests.
♻️ Proposed fix to reuse RestClient
`@Component`
`@RequiredArgsConstructor`
public class TossPaymentClient {
- private final RestClient.Builder restClientBuilder;
+ private RestClient restClient;
+ private final RestClient.Builder restClientBuilder;
`@Value`("${payment.toss.secret-key:}")
private String secretKey;
`@Value`("${payment.toss.base-url:https://api.tosspayments.com}")
private String baseUrl;
+
+ `@PostConstruct`
+ public void init() {
+ this.restClient = restClientBuilder
+ .baseUrl(baseUrl)
+ .build();
+ }
public TossPaymentConfirmResponse confirm(String paymentKey, String orderId, int amount) {
try {
- return restClientBuilder
- .baseUrl(baseUrl)
- .build()
+ return restClient
.post()🤖 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/TossPaymentClient.java`
around lines 32 - 34, The confirm() method in TossPaymentClient currently builds
a new RestClient via restClientBuilder.baseUrl(baseUrl).build() on every call;
instead, initialize and reuse a single RestClient instance (e.g., a private
final or volatile RestClient field) by building it once in the constructor or a
`@PostConstruct` method and then have confirm() use that cached RestClient; update
TossPaymentClient to add the RestClient field, move the
restClientBuilder.baseUrl(baseUrl).build() call into initialization, and remove
per-call builds to avoid repeated allocation.
Comment on lines
+43
to
+48
| } catch (RestClientException e) { | ||
| throw new GeneralException( | ||
| GeneralErrorCode.PAYMENT_CONFIRM_FAILED, | ||
| "토스페이먼츠 결제 승인에 실패했습니다." | ||
| ); | ||
| } |
There was a problem hiding this comment.
Preserve TOSS API error details for debugging.
The current error handling catches all RestClientException instances and throws a generic error, losing critical details from the TOSS API response (HTTP status codes, error codes, error messages). Payment failures are difficult to debug without this context.
🔍 Proposed fix to capture TOSS error details
+import org.springframework.web.client.HttpStatusCodeException;
+
public TossPaymentConfirmResponse confirm(String paymentKey, String orderId, int amount) {
try {
return restClient
.post()
.uri("/v1/payments/confirm")
.header(HttpHeaders.AUTHORIZATION, authorizationHeader())
.header("Idempotency-Key", orderId)
.contentType(MediaType.APPLICATION_JSON)
.body(new TossPaymentConfirmRequest(paymentKey, orderId, amount))
.retrieve()
.body(TossPaymentConfirmResponse.class);
+ } catch (HttpStatusCodeException e) {
+ throw new GeneralException(
+ GeneralErrorCode.PAYMENT_CONFIRM_FAILED,
+ "토스페이먼츠 결제 승인 실패: " + e.getStatusCode() + " - " + e.getResponseBodyAsString()
+ );
} catch (RestClientException e) {
throw new GeneralException(
GeneralErrorCode.PAYMENT_CONFIRM_FAILED,
- "토스페이먼츠 결제 승인에 실패했습니다."
+ "토스페이먼츠 결제 승인 중 오류 발생: " + e.getMessage()
);
}
}🤖 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/TossPaymentClient.java`
around lines 43 - 48, The catch block in TossPaymentClient currently swallows
RestClientException details; change error handling to first catch
HttpStatusCodeException (to access e.getStatusCode() and
e.getResponseBodyAsString()) and include those details in the thrown
GeneralException (or its message/metadata) so TOSS HTTP status and response body
are preserved for debugging, then keep a fallback catch for generic
RestClientException that still preserves e.getMessage() or sets e as the cause;
also optionally log the extracted status and response body before rethrowing.
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]
Summary by CodeRabbit
Release Notes