Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (27)
Summary by CodeRabbit
Walkthrough결제 확인의 멱등성 처리 로직을 추가하고, LIVE_CODING 인터뷰 타입 및 관련 Bedrock/GPT 프롬프트·팩토리·서비스·테스트를 도입합니다. Changes결제 멱등성 구현
라이브 코딩 인터뷰 기능 추가
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Summary of ChangesHello, 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! 본 PR은 결제 승인 과정에서 발생할 수 있는 중복 요청 문제를 해결하기 위해 멱등성을 보장하는 로직을 도입했습니다. 데이터 무결성 예외를 활용하여 이미 처리된 결제에 대한 재요청을 감지하고, 기존 결제 상태에 따라 적절한 응답을 반환하거나 예외를 던짐으로써 시스템의 안정성을 높였습니다. Highlights
New Features🧠 You can now enable Memory (public preview) to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe 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 by creating a comment using either
Customization To customize the 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 Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. 결제 요청 중복되어, 멱등성으로 막아내니, 데이터는 안전하고, 사용자도 안심하네. Footnotes
|
Test Results 51 files 51 suites 1m 36s ⏱️ Results for commit 2014c72. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/samhap/kokomen/payment/service/PaymentFacadeService.java`:
- Around line 48-50: The catch for DataIntegrityViolationException in
PaymentFacadeService indiscriminately treats all constraint violations as
idempotent "same paymentKey" retries; update the handler to inspect the
exception cause (e.g., exception.getMostSpecificCause() / message or
SQLState/code) to detect whether the unique constraint violated is the
payment_key constraint and only then call
resolveExistingPayment(tosspaymentsPaymentService.readByPaymentKey(request.paymentKey()));
for other constraint violations (like order_id) rethrow or handle separately
(log full exception details and propagate or map to a proper error), and ensure
the log includes the exception info to aid debugging.
In
`@src/test/java/com/samhap/kokomen/payment/service/PaymentFacadeServiceTest.java`:
- Around line 325-336: The test
CLIENT_BAD_REQUEST_상태에_failure_정보가_없으면_기본_메시지로_BadRequestException을_던진다()
currently only asserts the thrown BadRequestException message but misses
verifying idempotency by ensuring external client is not invoked; update the
test to also verify that tosspaymentsClient.confirmPayment(...) was never called
(e.g., verify(tosspaymentsClient, times(0)).confirmPayment(...)) after calling
paymentFacadeService.confirmPayment(request), keeping existing setup that sets
payment state to PaymentState.CLIENT_BAD_REQUEST and saves via
tosspaymentsPaymentRepository.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 91d0a883-1724-48e8-a701-7c1a923cd84c
📒 Files selected for processing (7)
src/main/java/com/samhap/kokomen/payment/domain/PaymentState.javasrc/main/java/com/samhap/kokomen/payment/domain/TosspaymentsPayment.javasrc/main/java/com/samhap/kokomen/payment/service/PaymentFacadeService.javasrc/main/java/com/samhap/kokomen/payment/service/TosspaymentsPaymentResultService.javasrc/main/java/com/samhap/kokomen/payment/service/dto/PaymentResponse.javasrc/test/java/com/samhap/kokomen/payment/service/PaymentFacadeServiceTest.javasrc/test/java/com/samhap/kokomen/token/service/TokenFacadeServiceTest.java
Comment on lines
+48
to
+50
| } catch (DataIntegrityViolationException e) { | ||
| log.info("동일 paymentKey 결제 재요청 - 멱등 분기 처리, paymentKey: {}", request.paymentKey()); | ||
| return resolveExistingPayment(tosspaymentsPaymentService.readByPaymentKey(request.paymentKey())); |
There was a problem hiding this comment.
DataIntegrityViolationException 전체를 멱등 재요청으로 처리하면 오분류됩니다.
현재 분기는 모든 DataIntegrityViolationException을 동일 paymentKey 재요청으로 가정합니다. 하지만 TosspaymentsPayment는 order_id도 unique라서 다른 제약 위반까지 멱등 분기로 흡수될 수 있습니다(잘못된 상태 조회/응답 위험).
🔧 제안 수정안
try {
tosspaymentsPayment = tosspaymentsPaymentService.saveTosspaymentsPayment(request);
} catch (DataIntegrityViolationException e) {
- log.info("동일 paymentKey 결제 재요청 - 멱등 분기 처리, paymentKey: {}", request.paymentKey());
- return resolveExistingPayment(tosspaymentsPaymentService.readByPaymentKey(request.paymentKey()));
+ TosspaymentsPayment existing = tosspaymentsPaymentService.findByPaymentKey(request.paymentKey())
+ .orElseThrow(() -> {
+ log.error("결제 저장 무결성 위반 - paymentKey로 기존 결제를 찾지 못했습니다. paymentKey: {}", request.paymentKey(), e);
+ return e;
+ });
+ log.info("동일 paymentKey 결제 재요청 - 멱등 분기 처리, paymentKey: {}", request.paymentKey());
+ return resolveExistingPayment(existing);
}🤖 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/samhap/kokomen/payment/service/PaymentFacadeService.java`
around lines 48 - 50, The catch for DataIntegrityViolationException in
PaymentFacadeService indiscriminately treats all constraint violations as
idempotent "same paymentKey" retries; update the handler to inspect the
exception cause (e.g., exception.getMostSpecificCause() / message or
SQLState/code) to detect whether the unique constraint violated is the
payment_key constraint and only then call
resolveExistingPayment(tosspaymentsPaymentService.readByPaymentKey(request.paymentKey()));
for other constraint violations (like order_id) rethrow or handle separately
(log full exception details and propagate or map to a proper error), and ensure
the log includes the exception info to aid debugging.
Comment on lines
+325
to
+336
| void CLIENT_BAD_REQUEST_상태에_failure_정보가_없으면_기본_메시지로_BadRequestException을_던진다() { | ||
| TosspaymentsPayment payment = TosspaymentsPaymentFixtureBuilder.builder() | ||
| .paymentKey("payment_key") | ||
| .build(); | ||
| payment.updateState(PaymentState.CLIENT_BAD_REQUEST); | ||
| tosspaymentsPaymentRepository.save(payment); | ||
| ConfirmRequest request = createConfirmRequest(); | ||
|
|
||
| assertThatThrownBy(() -> paymentFacadeService.confirmPayment(request)) | ||
| .isInstanceOf(BadRequestException.class) | ||
| .hasMessage(PaymentServiceErrorMessage.INVALID_REQUEST.getMessage()); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
멱등성 핵심 검증(외부 호출 억제) 조건이 빠져 있습니다.
Line 325-336 테스트는 예외 메시지만 확인하고 tosspaymentsClient.confirmPayment 미호출을 확인하지 않아, 멱등성 회귀를 놓칠 수 있습니다. 외부 호출 0회를 함께 검증해 주세요.
제안 수정
assertThatThrownBy(() -> paymentFacadeService.confirmPayment(request))
.isInstanceOf(BadRequestException.class)
.hasMessage(PaymentServiceErrorMessage.INVALID_REQUEST.getMessage());
+ verify(tosspaymentsClient, times(0)).confirmPayment(any(), any());🤖 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/samhap/kokomen/payment/service/PaymentFacadeServiceTest.java`
around lines 325 - 336, The test
CLIENT_BAD_REQUEST_상태에_failure_정보가_없으면_기본_메시지로_BadRequestException을_던진다()
currently only asserts the thrown BadRequestException message but misses
verifying idempotency by ensuring external client is not invoked; update the
test to also verify that tosspaymentsClient.confirmPayment(...) was never called
(e.g., verify(tosspaymentsClient, times(0)).confirmPayment(...)) after calling
paymentFacadeService.confirmPayment(request), keeping existing setup that sets
payment state to PaymentState.CLIENT_BAD_REQUEST and saves via
tosspaymentsPaymentRepository.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements idempotency for the payment confirmation process by handling DataIntegrityViolationException during payment record creation. It introduces logic to resolve existing payments based on their current state, returning cached responses for successful payments or throwing specific exceptions for client-side failures. Feedback from the reviewer suggests strengthening this implementation by validating that the incoming request details (orderId and amount) match the existing record to prevent incorrect idempotent responses. Additionally, the reviewer recommends allowing the confirmation process to proceed if the payment is in a transient state such as NEED_APPROVE or CONNECTION_TIMEOUT.
Comment on lines
+46
to
+51
| try { | ||
| tosspaymentsPayment = tosspaymentsPaymentService.saveTosspaymentsPayment(request); | ||
| } catch (DataIntegrityViolationException e) { | ||
| log.info("동일 paymentKey 결제 재요청 - 멱등 분기 처리, paymentKey: {}", request.paymentKey()); | ||
| return resolveExistingPayment(tosspaymentsPaymentService.readByPaymentKey(request.paymentKey())); | ||
| } |
There was a problem hiding this comment.
결제 승인 요청 중 DataIntegrityViolationException이 발생했을 때, 단순히 기존 결제 정보를 조회하여 반환하는 것만으로는 멱등성을 완벽하게 보장하기 어렵습니다.
- 재시도 허용: 기존 결제 상태가
NEED_APPROVE나CONNECTION_TIMEOUT인 경우, 이는 아직 토스페이먼츠 API 호출이 성공하지 않았거나 네트워크 오류로 중단된 상태임을 의미합니다. 이 경우 예외를 던지지 않고 승인 프로세스를 계속 진행할 수 있도록 처리해야 합니다. - 요청 정보 검증: 멱등성 처리를 위해 기존 결제를 조회했을 때, 요청된
orderId와amount가 DB에 저장된 정보와 일치하는지 반드시 확인해야 합니다. 정보가 다를 경우 잘못된 멱등 요청으로 간주하고 예외를 발생시켜야 합니다.
try {
tosspaymentsPayment = tosspaymentsPaymentService.saveTosspaymentsPayment(request);
} catch (DataIntegrityViolationException e) {
log.info("동일 paymentKey 결제 재요청 - 멱등 분기 처리, paymentKey: {}", request.paymentKey());
tosspaymentsPayment = tosspaymentsPaymentService.readByPaymentKey(request.paymentKey());
PaymentResponse existingResponse = resolveExistingPayment(request, tosspaymentsPayment);
if (existingResponse != null) {
return existingResponse;
}
}
Comment on lines
+66
to
+79
| private PaymentResponse resolveExistingPayment(TosspaymentsPayment payment) { | ||
| if (payment.isApprovedOrCompleted()) { | ||
| log.info("이미 승인된 결제 재요청 - 멱등 응답 반환, paymentKey: {}, state: {}", | ||
| payment.getPaymentKey(), payment.getState()); | ||
| TosspaymentsPaymentResult result = tosspaymentsPaymentResultService.readByTosspaymentsPaymentId(payment.getId()); | ||
| return PaymentResponse.fromExisting(payment, result); | ||
| } | ||
| if (payment.isClientBadRequest()) { | ||
| log.info("클라이언트 원인 실패 결제 재요청 - paymentKey: {}", payment.getPaymentKey()); | ||
| throw new BadRequestException(readClientFailureMessage(payment.getId())); | ||
| } | ||
| log.error("비정상 상태 결제 재요청 - paymentKey: {}, state: {}", payment.getPaymentKey(), payment.getState()); | ||
| throw new InternalServerErrorException(PaymentServiceErrorMessage.CONFIRM_SERVER_ERROR.getMessage()); | ||
| } |
There was a problem hiding this comment.
resolveExistingPayment 메서드에서 NEED_APPROVE 및 CONNECTION_TIMEOUT 상태를 처리하여 승인 프로세스를 재시작할 수 있도록 개선이 필요합니다. 또한, 요청 정보(orderId, amount)에 대한 검증 로직을 추가하여 보안 및 정합성을 강화해야 합니다.
private PaymentResponse resolveExistingPayment(ConfirmRequest request, TosspaymentsPayment payment) {
if (!payment.getOrderId().equals(request.orderId()) || !payment.getTotalAmount().equals(request.amount())) {
log.warn("멱등 요청 정보 불일치 - paymentKey: {}, requestOrderId: {}, dbOrderId: {}",
payment.getPaymentKey(), request.orderId(), payment.getOrderId());
throw new BadRequestException("이미 존재하는 결제 키이나, 주문 정보가 일치하지 않습니다.");
}
if (payment.isApprovedOrCompleted()) {
log.info("이미 승인된 결제 재요청 - 멱등 응답 반환, paymentKey: {}, state: {}",
payment.getPaymentKey(), payment.getState());
TosspaymentsPaymentResult result = tosspaymentsPaymentResultService.readByTosspaymentsPaymentId(payment.getId());
return PaymentResponse.fromExisting(payment, result);
}
if (payment.isClientBadRequest()) {
log.info("클라이언트 원인 실패 결제 재요청 - paymentKey: {}", payment.getPaymentKey());
throw new BadRequestException(readClientFailureMessage(payment.getId()));
}
if (payment.getState() == PaymentState.NEED_APPROVE || payment.getState() == PaymentState.CONNECTION_TIMEOUT) {
log.info("결제 승인 전 단계에서 재요청 - 승인 프로세스 진행, paymentKey: {}, state: {}",
payment.getPaymentKey(), payment.getState());
return null;
}
log.error("비정상 상태 결제 재요청 - paymentKey: {}, state: {}", payment.getPaymentKey(), payment.getState());
throw new InternalServerErrorException(PaymentServiceErrorMessage.CONFIRM_SERVER_ERROR.getMessage());
}
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.
closed #
작업 내용
스크린샷
참고 사항