[volume-6] 외부 PG 사 연동 및 장애복구 - 김윤선#247
Merged
kimyam1008 merged 6 commits intoLoopers-dev-lab:kimyam1008from Mar 23, 2026
Merged
[volume-6] 외부 PG 사 연동 및 장애복구 - 김윤선#247kimyam1008 merged 6 commits intoLoopers-dev-lab:kimyam1008from
kimyam1008 merged 6 commits intoLoopers-dev-lab:kimyam1008from
Conversation
📝 WalkthroughWalkthrough결제 도메인을 새로 구현하고 주문 취소 시 환급 처리를 추가한다. Payment 엔티티, PaymentFacade, PaymentTxService, PaymentSyncScheduler를 도입하고 PG 클라이언트와 회로차단기를 설정하여 결제 생명주기를 관리한다. Changes
Sequence DiagramsequenceDiagram
actor User
participant API as PaymentV1Controller
participant Facade as PaymentFacade
participant TxService as PaymentTxService
participant PgClient as PgClient
participant Repo as PaymentRepository
participant OrderRepo as OrderRepository
participant DB as Database
User->>API: POST /api/v1/payments<br/>(orderId, cardType, cardNo)
activate API
API->>Facade: pay(loginId, password, command)
activate Facade
Facade->>Facade: authenticate(loginId, password)
Facade->>OrderRepo: findById(orderId)
Facade->>Repo: findByOrderId(orderId)
alt Payment already exists
Facade->>Repo: return existing PaymentInfo
else Create new payment
Facade->>TxService: createPayment(orderId, userId, ...)
activate TxService
TxService->>Repo: save(Payment with PENDING)
TxService->>DB: INSERT Payment
deactivate TxService
Facade->>PgClient: requestPayment(userId, orderId, ...)
activate PgClient
PgClient->>PgClient: request with circuit breaker
PgClient->>DB: (external PG system)
PgClient->>Facade: return transactionKey or empty
deactivate PgClient
alt Transaction key received
Facade->>TxService: assignTransactionId(paymentId, txId)
activate TxService
TxService->>Repo: save(Payment with pgTransactionId)
TxService->>DB: UPDATE Payment
deactivate TxService
else No transaction key (PG failure)
Facade->>PgClient: getPaymentByOrderId(userId, orderId)
alt PG state available
Facade->>TxService: processPaymentResult(...)
activate TxService
TxService->>OrderRepo: findById(orderId)
TxService->>DB: UPDATE Order, Payment
deactivate TxService
end
end
Facade->>Repo: findByOrderId(orderId)
Facade->>Facade: return PaymentInfo
end
Facade->>API: return PaymentInfo
API->>User: HTTP 200 + PaymentResponse
deactivate Facade
deactivate API
sequenceDiagram
participant PG as PG System
participant API as PaymentV1Controller
participant Facade as PaymentFacade
participant TxService as PaymentTxService
participant Repo as PaymentRepository
participant OrderRepo as OrderRepository
participant DB as Database
PG->>API: POST /api/v1/payments/callback<br/>(transactionId, orderId, status)
activate API
API->>Facade: handleCallback(command)
activate Facade
Facade->>Repo: findByPgTransactionId(txId) or findByOrderId(orderId)
alt Payment not found
Facade->>API: throw CoreException (PAYMENT_NOT_FOUND)
else Payment found and still PENDING
Facade->>TxService: processPaymentResult(paymentId, orderId, txId, status)
activate TxService
alt Status = SUCCESS
TxService->>OrderRepo: findById(orderId)
alt Order status = CANCELLED
TxService->>Repo: save(Payment with REFUNDED)
else Normal order
TxService->>OrderRepo: update Order to CONFIRMED
TxService->>Repo: save(Payment with SUCCESS)
TxService->>DB: UPDATE Order, Payment
end
else Status = FAILED
TxService->>OrderRepo: findById(orderId)
TxService->>OrderRepo: update Order to CANCELLED
TxService->>DB: restore inventory, revoke coupon
TxService->>Repo: save(Payment with FAILED)
TxService->>DB: UPDATE Order, Payment
end
deactivate TxService
Facade->>Repo: findByOrderId(orderId)
Facade->>API: return updated PaymentInfo
else Payment already processed
Facade->>API: return current PaymentInfo (no action)
end
API->>PG: HTTP 200 + PaymentResponse
deactivate Facade
deactivate API
sequenceDiagram
participant Scheduler as PaymentSyncScheduler
participant Repo as PaymentRepository
participant PgClient as PgClient
participant TxService as PaymentTxService
participant DB as Database
Scheduler->>Scheduler: `@Scheduled`(fixedDelay=300000ms)
activate Scheduler
Scheduler->>Repo: findAllPendingBefore(threshold: 10 minutes ago)
alt No PENDING payments
Scheduler->>Scheduler: return (exit early)
else Found pending payments
Scheduler->>Scheduler: limit to batchSize (default 50)
loop for each Payment
Scheduler->>Repo: check if still PENDING
alt Status still PENDING
Scheduler->>PgClient: getPaymentByOrderId(userId, orderId)
activate PgClient
PgClient->>PgClient: retry with fixed backoff
PgClient->>DB: (external PG query)
PgClient->>Scheduler: return PgPaymentInfo or empty
deactivate PgClient
alt PG data exists
Scheduler->>TxService: processPaymentResult(...)
activate TxService
TxService->>Repo: save(Payment with new status)
TxService->>DB: UPDATE Payment/Order
deactivate TxService
Scheduler->>Scheduler: increment successCount
else No PG data
Scheduler->>Scheduler: log debug, keep PENDING
end
else Status no longer PENDING
Scheduler->>Scheduler: skip processing
end
end
Scheduler->>Scheduler: log summary (success/failure counts)
end
deactivate Scheduler
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 근거: 신규 파일 30+개 이상으로 Payment 도메인 전체 계층(도메인, 서비스, API, 인프라, PG 클라이언트) 구현. PaymentFacade의 인증/멱등성/상태 관리, PaymentTxService의 트랜잭션 경계 관리, PaymentSyncScheduler의 정기 동기화 로직 등 복잡한 상태 전이와 보상이 포함된다. OrderFacade 통합, 회로차단기/재시도 정책, 멀티 테스트 계층 등으로 인해 리뷰 범위가 넓다. Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip CodeRabbit can use Trivy to scan for security misconfigurations and secrets in Infrastructure as Code files.Add a .trivyignore file to your project to customize which findings Trivy reports. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
🧹 Nitpick comments (13)
apps/commerce-api/src/main/java/com/loopers/infrastructure/payment/PaymentJpaRepository.java (1)
11-17:status+created_at복합 인덱스 추가를 검토한다.
findAllByStatusAndCreatedAtBefore쿼리는 status와 created_at 조건을 모두 사용한다. 현재 인덱스 정의에는 이 복합 조건에 최적화된 인덱스가 없으므로, 배치 복구 쿼리 성능 저하가 우려된다.수정안: Payment 엔티티에 복합 인덱스 추가를 검토한다.
`@Index`(name = "idx_payment_status_created_at", columnList = "status, created_at")Based on learnings: DB 인덱스는 별도 마이그레이션 파이프라인으로 관리되므로, 해당 파이프라인에 인덱스 추가 요청이 필요하다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/commerce-api/src/main/java/com/loopers/infrastructure/payment/PaymentJpaRepository.java` around lines 11 - 17, The repository method findAllByStatusAndCreatedAtBefore uses both status and created_at so add a composite DB index for (status, created_at) on the Payment entity (e.g., add an index named idx_payment_status_created_at referencing columns status, created_at) to optimize that query; update the Payment entity's `@Table/`@Index metadata accordingly and then raise a change request to the DB migration pipeline to create the physical index (do not rely solely on JPA schema generation).apps/commerce-api/src/main/java/com/loopers/domain/payment/Payment.java (1)
64-66:assignTransactionId()는 상태 검증 없이 덮어쓰기가 가능하다.SUCCESS나 REFUNDED 상태에서도 pgTransactionId를 변경할 수 있어 감사 추적(audit trail)에 문제가 될 수 있다. 의도적 설계라면 주석으로 명시하고, 아니라면 PENDING 상태에서만 허용하도록 제한을 검토한다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/commerce-api/src/main/java/com/loopers/domain/payment/Payment.java` around lines 64 - 66, The assignTransactionId method currently overwrites pgTransactionId regardless of payment state; restrict changes to only when the payment is in PENDING (or whichever transient state you use) by checking the payment status field inside Payment.assignTransactionId and rejecting/throwing an IllegalStateException (or returning false) if status is not PENDING (allowing idempotent set if pgTransactionId is already equal). If this behavior is intentional, add a clarifying JavaDoc on Payment.assignTransactionId; also add unit tests around state transitions (PENDING -> SUCCESS/REFUNDED) to verify pgTransactionId cannot be changed after final states.apps/commerce-api/src/test/java/com/loopers/domain/payment/PaymentTest.java (2)
44-52:markSuccess중복 호출 시 기존pgTransactionId보존 여부 검증 필요현재 테스트는 예외 발생만 확인한다. 예외 발생 후 기존
pgTransactionId가 변경되지 않고 유지되는지도 검증하면 도메인 불변성 보장을 더 명확히 할 수 있다.검증 추가 제안
`@Test` void markSuccess_fail_alreadyProcessed() { Payment payment = Payment.create(1L, 2L, "SAMSUNG", "1234-5678", BigDecimal.valueOf(5000)); payment.markSuccess("txId1"); assertThatThrownBy(() -> payment.markSuccess("txId2")) .isInstanceOf(IllegalStateException.class); + + // 기존 txId가 유지되는지 확인 + assertThat(payment.getPgTransactionId()).isEqualTo("txId1"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/commerce-api/src/test/java/com/loopers/domain/payment/PaymentTest.java` around lines 44 - 52, Test currently only asserts that a second call to Payment.markSuccess throws; add an assertion that the existing pgTransactionId is preserved after the exception to ensure immutability. After calling payment.markSuccess("txId1") and asserting that calling payment.markSuccess("txId2") throws, verify payment.getPgTransactionId() (or the accessor used in the test) still equals "txId1" and optionally that payment.getStatus() remains SUCCESS; update the test method markSuccess_fail_alreadyProcessed to include these assertions referencing Payment.create and payment.markSuccess.
25-31: 음수 금액에 대한 테스트 케이스 추가 권장현재
BigDecimal.ZERO에 대한 검증만 존재한다. 음수 금액(BigDecimal.valueOf(-1000))에 대한 테스트도 추가하여 도메인 규칙이 음수를 정확히 거부하는지 확인하는 것이 좋다. 운영 중 잘못된 입력값이 유입될 경우 방어적 검증이 동작하는지 테스트로 보장해야 한다.추가 테스트 제안
`@DisplayName`("create: 주문 금액이 음수이면 예외가 발생한다") `@Test` void create_fail_negativeAmount() { assertThatThrownBy(() -> Payment.create(1L, 2L, "SAMSUNG", "1234-5678", BigDecimal.valueOf(-1000))) .isInstanceOf(IllegalArgumentException.class) .hasMessageContaining("결제 금액"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/commerce-api/src/test/java/com/loopers/domain/payment/PaymentTest.java` around lines 25 - 31, Add a test that asserts Payment.create rejects negative amounts: create a new test method (e.g., create_fail_negativeAmount) in PaymentTest that calls Payment.create(1L, 2L, "SAMSUNG", "1234-5678", BigDecimal.valueOf(-1000)) and verifies it throws IllegalArgumentException with a message containing "결제 금액", mirroring the existing zero-amount test to ensure negative-value validation in Payment.create is covered.apps/commerce-api/src/main/java/com/loopers/CommerceApiApplication.java (1)
14-14: 스케줄러 스레드 풀 및 에러 핸들링 설정 권장
@EnableScheduling활성화로PaymentSyncScheduler가 동작한다. 기본 스케줄러는 단일 스레드로 동작하여 작업 지연 시 다음 실행이 밀릴 수 있다. 운영 환경에서는TaskScheduler빈을 커스터마이징하여 스레드 풀 크기를 설정하고, 스케줄 작업 실패 시 에러 핸들링 전략을 정의하는 것이 바람직하다.스케줄러 설정 예시
`@Configuration` public class SchedulerConfig { `@Bean` public TaskScheduler taskScheduler() { ThreadPoolTaskScheduler scheduler = new ThreadPoolTaskScheduler(); scheduler.setPoolSize(2); scheduler.setThreadNamePrefix("payment-sync-"); scheduler.setErrorHandler(t -> log.error("Scheduled task failed", t)); scheduler.initialize(); return scheduler; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/commerce-api/src/main/java/com/loopers/CommerceApiApplication.java` at line 14, Add a custom TaskScheduler bean to replace the single-thread default used by `@EnableScheduling` so PaymentSyncScheduler runs on a pool and failures are handled: create a SchedulerConfig class that defines a TaskScheduler bean returning a ThreadPoolTaskScheduler with an increased pool size, a descriptive threadNamePrefix, and an error handler via setErrorHandler(...) that logs exceptions; ensure the bean is picked up by Spring (e.g., `@Configuration`) so scheduled tasks use this scheduler.apps/commerce-api/src/test/java/com/loopers/application/payment/PaymentTxServiceTest.java (2)
25-43:createPayment및assignTransactionId메서드 테스트 누락
PaymentTxService의 핵심 메서드인createPayment와assignTransactionId에 대한 테스트가 없다. 특히assignTransactionId는 현재 결제 미존재 시 무시하는 동작을 하므로, 이에 대한 테스트가 필요하다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/commerce-api/src/test/java/com/loopers/application/payment/PaymentTxServiceTest.java` around lines 25 - 43, Add unit tests in PaymentTxServiceTest to cover PaymentTxService.createPayment and PaymentTxService.assignTransactionId: write a test that verifies createPayment persists a Payment via mocked PaymentRepository and returns expected result when OrderRepository and ProductRepository provide valid order/product, and write two assignTransactionId tests—one where a Payment exists (mock paymentRepository.findById to return a Payment) and you assert paymentRepository.save is called with the transactionId set, and one where paymentRepository.findById returns empty and you assert no save/exception occurs. Use the existing mocks (paymentRepository, orderRepository, productRepository, couponIssueRepository) and the paymentTxService instance to locate where to add tests.
129-146: FAILED 콜백 시 쿠폰 복구 경로 테스트 누락
processPaymentResult_failed_orderCancelledWithRecovery테스트는 재고 복구(product.increaseStock)만 검증하고 쿠폰 복구 경로는 테스트하지 않는다.order에couponId가 설정된 경우couponIssueRepository.findByIdAndUserId→issue.revoke()흐름도 테스트해야 전체 복구 로직의 신뢰성을 보장할 수 있다.쿠폰 복구 테스트 추가 제안
`@DisplayName`("processPaymentResult: FAILED 콜백이면 쿠폰도 복구한다") `@Test` void processPaymentResult_failed_couponRevoked() { Payment payment = Payment.create(orderId, userId, "SAMSUNG", "1234-5678", BigDecimal.valueOf(5000)); when(paymentRepository.findById(1L)).thenReturn(Optional.of(payment)); // 쿠폰이 포함된 주문 생성 Order order = createPendingOrderWithCoupon(userId, 99L); when(orderRepository.findById(orderId)).thenReturn(Optional.of(order)); CouponIssue couponIssue = mock(CouponIssue.class); when(couponIssueRepository.findByIdAndUserId(99L, userId)).thenReturn(Optional.of(couponIssue)); paymentTxService.processPaymentResult(1L, orderId, "txId1", "FAILED"); verify(couponIssue).revoke(); verify(couponIssueRepository).save(couponIssue); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/commerce-api/src/test/java/com/loopers/application/payment/PaymentTxServiceTest.java` around lines 129 - 146, The test processPaymentResult_failed_orderCancelledWithRecovery only verifies stock recovery but misses the coupon recovery path; add a new or extend the existing test to set an order with a coupon (use createPendingOrderWithCoupon or set couponId on the Order), mock couponIssueRepository.findByIdAndUserId to return a mocked CouponIssue, call paymentTxService.processPaymentResult(1L, orderId, "txId1", "FAILED"), and then verify couponIssue.revoke() and couponIssueRepository.save(couponIssue) were invoked to assert the coupon recovery flow.apps/commerce-api/src/main/java/com/loopers/application/payment/PaymentTxService.java (2)
100-105: N+1 쿼리 패턴: 루프 내productRepository.findById호출
order.getOrderItems()순회 시마다productRepository.findById를 호출하여 N+1 쿼리가 발생한다. 주문 아이템이 많을 경우 DB 부하가 증가한다.productRepository.findAllById(productIds)로 일괄 조회 후 Map으로 변환하여 사용하는 것이 효율적이다. As per coding guidelines: "N+1 가능성, fetch join/EntityGraph 사용 여부, 대량 데이터에서의 병목을 점검한다."일괄 조회 방식으로 수정 제안
private void cancelOrderWithRecovery(Order order) { if (!order.cancel()) { return; } + List<Long> productIds = order.getOrderItems().stream() + .map(OrderItem::getProductId) + .toList(); + Map<Long, Product> productMap = productRepository.findAllById(productIds).stream() + .collect(Collectors.toMap(Product::getId, Function.identity())); + for (OrderItem item : order.getOrderItems()) { - productRepository.findById(item.getProductId()).ifPresent(product -> { + Product product = productMap.get(item.getProductId()); + if (product != null) { product.increaseStock(item.getQuantity()); productRepository.save(product); - }); + } } // ... coupon recovery }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/commerce-api/src/main/java/com/loopers/application/payment/PaymentTxService.java` around lines 100 - 105, The current loop in PaymentTxService calls productRepository.findById for each OrderItem, causing an N+1 query; instead collect productIds from order.getOrderItems(), call productRepository.findAllById(productIds) once, convert the returned Iterable to a Map keyed by productId, then iterate order.getOrderItems() and call product.increaseStock(item.getQuantity()) using the map lookup, and finally persist changes with productRepository.saveAll(updatedProducts) (or save per-product if needed); update references: order.getOrderItems(), productRepository.findAllById(...), product.increaseStock(...), and productRepository.saveAll(...).
95-98:order.cancel()실패 시 로깅 부재
order.cancel()이false를 반환하면 복구 로직을 건너뛰지만, 왜 취소가 실패했는지(이미 취소됨, 상태 전이 불가 등) 로그가 남지 않는다. 운영 중 문제 추적을 위해 실패 사유를 로깅하는 것이 좋다.로깅 추가 제안
+import lombok.extern.slf4j.Slf4j; +@Slf4j `@Service` `@RequiredArgsConstructor` public class PaymentTxService { // ... private void cancelOrderWithRecovery(Order order) { if (!order.cancel()) { + log.info("Order already cancelled or cannot be cancelled. orderId={}, status={}", + order.getId(), order.getStatus()); return; } // ... } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/commerce-api/src/main/java/com/loopers/application/payment/PaymentTxService.java` around lines 95 - 98, In cancelOrderWithRecovery, when order.cancel() returns false add a log statement that records why the cancel failed; use the service logger (e.g., PaymentTxService logger) to emit a WARN/ERROR including order.identifiers (order.getId()), current state (order.getStatus() or order.getState()/toString()) and any available failure reason (order.getCancelFailureReason() or similar), falling back to the order's toString() if no specific reason accessor exists; keep the current control flow (still return) but ensure the log provides enough context to trace the failure.apps/commerce-api/src/main/java/com/loopers/infrastructure/pg/PgOrderResponse.java (1)
9-12:transactions필드는 외부 API 응답에서 직접 할당되므로 방어적 복사로 불변성을 보장해야 한다Jackson 역직렬화 과정에서 외부 PG API의 리스트가 그대로 참조되어 이후 의도하지 않은 수정에 노출될 수 있다. 특히 결제 상태 복구 로직에서 트랜잭션 목록이 변경되면 비결정적 동작으로 이어질 수 있다. compact constructor에서
null을 빈 리스트로 치환하고List.copyOf()로 고정하여 외부 참조 변경을 차단해야 한다.♻️ 제안 수정안
public record PgOrderResponse( String orderId, List<PgPaymentInfo> transactions ) { + public PgOrderResponse { + transactions = (transactions == null) ? List.of() : List.copyOf(transactions); + } }추가 테스트로는 원본 리스트 변경이 record 상태에 반영되지 않음을 검증하고,
null입력 시 빈 리스트로 처리되는 케이스를 포함해야 한다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/commerce-api/src/main/java/com/loopers/infrastructure/pg/PgOrderResponse.java` around lines 9 - 12, PgOrderResponse의 transactions 필드는 Jackson 역직렬화로 외부 리스트를 그대로 참조하므로 compact constructor를 추가해 null이면 빈 리스트로 치환하고 List.copyOf(transactions)로 방어적 복사를 하여 불변성을 보장하세요; 즉 PgOrderResponse의 compact constructor 내에서 this.transactions를 List.copyOf(Optional.ofNullable(transactions).orElseGet(List::of)) 형태로 설정하도록 수정하고 관련 테스트로 원본 리스트 변경이 레코드에 반영되지 않음과 null 입력 시 빈 리스트 처리 케이스를 추가하세요.apps/commerce-api/src/test/java/com/loopers/interfaces/api/PaymentE2ETest.java (1)
187-193: E2E가 서명 없는 콜백과 무권한 admin sync를 정상 계약으로 고정한다Line 187-193과 Line 224-228은 각각 서명 없는 콜백과 인증 없는 관리자 sync를 그대로 200 성공 시나리오로 검증한다. 보안 수정이 들어가면 이 테스트가 올바른 변경을 실패로 만들거나, 반대로 필요한 실패 케이스를 놓치게 된다. 성공 케이스에는 의도한 서명/관리자 자격을 명시하고, 별도로 invalid signature와 non-admin 401/403 케이스를 추가하는 편이 안전하다. 추가로 replay callback까지 포함해 회귀 테스트를 넣는 것이 좋다.
As per coding guidelines:**/*Test*.java: 단위 테스트는 경계값/실패 케이스/예외 흐름을 포함하는지 점검한다.Also applies to: 224-228
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/commerce-api/src/test/java/com/loopers/interfaces/api/PaymentE2ETest.java` around lines 187 - 193, Update the existing success E2E assertions that call mockMvc.perform(post("/api/v1/payments/callback")...) and the admin sync test to include the required valid signature header/body and valid admin credentials (so the "SUCCESS" assertions reflect the intended authenticated flow), and add separate tests that post the same endpoints with an invalid signature (expect 401/403 or specific error) and with non-admin credentials (expect 401/403) to cover failure paths; also add a replay callback test that resubmits an already-processed callback and asserts rejection; locate and change the tests around the mockMvc.perform(...) calls in PaymentE2ETest.java to implement these cases.apps/commerce-api/src/main/java/com/loopers/infrastructure/pg/PgClientConfig.java (1)
49-55: PG 전용 회로차단기 설정이 전역 기본값으로 퍼진다Line 49의
configureDefault는 같은Resilience4JCircuitBreakerFactory에서 생성되는 다른 breaker에도 이 6초 타임리미터와 임계값을 그대로 적용한다. 운영 중 다른 외부 연동이 추가되면 PG 기준으로 불필요하게 OPEN 되거나 timeout 되어 장애 범위가 넓어진다.pg-payment,pg-query에만 명시적으로 설정을 묶고, 비PG breaker는 별도 기본값을 유지하도록 분리하는 편이 안전하다. 추가로 비PG breaker 하나를 생성했을 때 PG 설정이 섞이지 않는 통합 테스트를 넣는 것이 좋다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/commerce-api/src/main/java/com/loopers/infrastructure/pg/PgClientConfig.java` around lines 49 - 55, The current use of configureDefault on the Resilience4JCircuitBreakerFactory spreads the PG-specific 6s TimeLimiter and thresholds to all breakers; change the factory configuration so PG-specific settings are applied only for breakers with IDs "pg-payment" and "pg-query" (e.g. replace configureDefault(...) with factory.configure(id -> { if (id.equals("pg-payment") || id.equals("pg-query")) return new Resilience4JConfigBuilder(id).circuitBreakerConfig(cbConfig).timeLimiterConfig(tlConfig).build(); else return defaultPgSafeConfig; }) where defaultPgSafeConfig is a separate default config without the PG time limiter/thresholds), and add an integration test that creates a non-PG breaker from Resilience4JCircuitBreakerFactory and asserts it does not inherit the PG TimeLimiter/tresholds to prevent configuration bleed.apps/commerce-api/src/test/java/com/loopers/application/payment/PaymentFacadeIntegrationTest.java (1)
230-235: 사용자 미존재 예외 테스트에 PG 미호출 검증을 추가해야 한다운영 관점에서 인증/사용자 검증 실패 시 외부 PG 호출이 발생하면 불필요한 외부 트래픽과 장애 전파 위험이 생긴다. 수정안으로 예외 검증 뒤
pgClient무호출을 함께 단언해 실패 경로의 부작용을 차단해야 한다. 추가 테스트로 비밀번호 불일치 등 다른 인증 실패 케이스에도 동일한 무호출 보장을 검증하는 것이 좋다.수정 예시
void pay_fail_userNotFound() { assertThatThrownBy(() -> paymentFacade.pay("unknown", "pw", new PaymentDto.PayCommand(orderId, "SAMSUNG", "1234-5678"))) .isInstanceOf(CoreException.class) .satisfies(e -> assertThat(((CoreException) e).getErrorType()) - .isEqualTo(ErrorType.USER_NOT_FOUND)); + .isEqualTo(ErrorType.USER_NOT_FOUND)); + org.mockito.Mockito.verifyNoInteractions(pgClient); }As per coding guidelines
**/*Test*.java: "단위 테스트는 경계값/실패 케이스/예외 흐름을 포함하는지 점검한다."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/commerce-api/src/test/java/com/loopers/application/payment/PaymentFacadeIntegrationTest.java` around lines 230 - 235, The test PaymentFacadeIntegrationTest should not only assert that PaymentFacade.pay throws a CoreException with ErrorType.USER_NOT_FOUND but also verify that the external pgClient is never invoked; update the test to, after the assertThatThrownBy block, add a mockito assertion such as verifyNoInteractions(pgClient) or verify(pgClient, never()).<methodName/operation>() (use the actual pgClient method used to call the PG) to ensure no PG call occurs on user-not-found, and apply the same pgClient non-invocation assertion pattern to other authentication-failure tests (e.g., wrong password) to guarantee failure paths produce no external side effects.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@apps/commerce-api/src/main/java/com/loopers/application/payment/PaymentDto.java`:
- Around line 18-23: CallbackCommand currently exposes orderId as a String which
can cause NumberFormatException in PaymentFacade.handleCallback when calling
Long.parseLong; add defensive validation and explicit error handling either at
DTO construction or immediately in PaymentFacade.handleCallback: attempt to
parse command.orderId() inside a try/catch, map NumberFormatException (or
null/empty checks) to a controlled CoreException with ErrorType.BAD_REQUEST and
a clear message, and proceed only with a valid Long orderId; also add
unit/integration tests that invoke the callback with null, empty, and
non-numeric orderId values to assert the BAD_REQUEST error is returned.
- Around line 30-31: PaymentDto currently exposes raw cardNo (and cardType) in
API responses; update PaymentInfo.from() to mask the card number before setting
it on the DTO (or call a shared masking util) so that only non-sensitive parts
(e.g., last 4 digits) remain while all other digits are replaced (e.g., ****
**** **** 1234); ensure the PaymentDto field that holds cardNo is populated with
the masked value and not the raw number, and add/ reuse a small helper like
maskCardNumber(String) for consistency across codepaths.
In
`@apps/commerce-api/src/main/java/com/loopers/application/payment/PaymentFacade.java`:
- Around line 100-102: The current fallback in PaymentFacade that uses
paymentRepository.findByOrderId(orderId) when
findByPgTransactionId(command.transactionId()) is empty can attach a delayed
callback to the wrong (newer) payment; change the lookup logic so that after
failing findByPgTransactionId you only accept a fallback payment if it is a
single payment for the order with status PENDING and pgTransactionId == null (or
similarly unassigned), otherwise throw
CoreException(ErrorType.PAYMENT_NOT_FOUND); update the code paths around Payment
payment assignment and paymentRepository queries (findByPgTransactionId,
findByOrderId) to enforce this guard and add/adjust a regression test ensuring a
delayed callback from an old transaction does not mutate a newer payment.
- Around line 80-83: PaymentFacade 및 PaymentSyncScheduler에서 PG가 반환한 원본 상태 문자열을
그대로 processPaymentResult()에 전달하는 대신, PgPaymentInfo.status를 enum으로 정의하고 PG 응답값을
정규화(trim + uppercase)한 뒤 PG→내부 상태(SUCCESS/FAILED/PENDING)로 매핑하는 전용 매퍼를 만들어 4개
call site(PaymentFacade의 pgClient.getPaymentByOrderId(...) 호출들 및
PaymentSyncScheduler 호출들)를 모두 이 매퍼를 통해 전달하도록 수정하세요; 매핑 규칙은 "SUCCESS"만 성공으로,
"FAILED","FAIL","ERROR" 계열은 실패로, "PENDING","PROCESSING" 및 알 수 없는 값은
PENDING(미확정)으로 처리해 주문 취소/재고 복구가 조기에 발생하지 않게 하고, 대소문자/공백 변형에 대한 단위/회귀 테스트를 추가해
보장하세요.
In
`@apps/commerce-api/src/main/java/com/loopers/application/payment/PaymentSyncScheduler.java`:
- Around line 47-57: PaymentSyncScheduler currently calls
paymentRepository.findAllPendingBefore(threshold) which returns the full PENDING
backlog into pendingPayments and only then applies batchSize in-memory; change
the repository/query behind findAllPendingBefore(...) to apply the desired
ordering and a SQL LIMIT (batchSize) so the DB returns at most batchSize sorted
records and update PaymentSyncScheduler to use that limited result directly
(remove or rely on the existing subList logic). Reference findAllPendingBefore,
pendingPayments, targets, and batchSize when making the change; also add an
adapter/integration test asserting that the repository/query layer returns
exactly batchSize rows when backlog is large to prevent regressions.
In
`@apps/commerce-api/src/main/java/com/loopers/application/payment/PaymentTxService.java`:
- Around line 35-41: The assignTransactionId method currently swallows the case
where paymentRepository.findById(paymentId) is empty; change it to fail loudly
by replacing the ifPresent pattern with an explicit existence check that either
logs and throws a clear exception (e.g., EntityNotFoundException or custom
PaymentNotFoundException) or uses
paymentRepository.findById(paymentId).orElseThrow(...) so that
PaymentTxService.assignTransactionId(paymentId, pgTransactionId) will not
silently no-op when the payment is missing; keep the original behavior of
calling payment.assignTransactionId(pgTransactionId) and
paymentRepository.save(payment) when found.
In `@apps/commerce-api/src/main/java/com/loopers/domain/payment/Payment.java`:
- Around line 68-74: In Payment.markSuccess, prevent null pgTransactionId to
preserve data integrity by adding an explicit null-check at the start of the
method (e.g., if (pgTransactionId == null) throw new
IllegalArgumentException("pgTransactionId must not be null")); keep the existing
state check against PaymentStatus.PENDING and then assign this.pgTransactionId
and set this.status = PaymentStatus.SUCCESS as before; also add a unit test for
Payment.markSuccess(null) asserting that the method throws the expected
exception to cover this validation.
- Around line 33-34: Payment.cardNo currently persists the full PAN in plain
text; change the persistence to store only a PG-issued token or a masked PAN
(e.g., ****-****-****-1234) instead of the raw card number: update the PG
integration code that populates the Payment entity so it receives and sets
either paymentToken or maskedCardNo (create a new field such as paymentToken or
maskedCardNo on the Payment class and deprecate/remove cardNo), adjust the
`@Column` metadata (length/nullable) accordingly, and add validation/tests to
ensure only token/masked values are stored and raw PANs are never persisted.
In
`@apps/commerce-api/src/main/java/com/loopers/infrastructure/payment/PaymentJpaRepository.java`:
- Line 17: findAllByStatusAndCreatedAtBefore currently returns all matching
PENDING rows causing memory/DB pressure; change the repository method signature
to accept a Pageable (e.g., List<Payment>
findAllByStatusAndCreatedAtBefore(PaymentStatus status, ZonedDateTime createdAt,
Pageable pageable)) or add an explicit `@Query` with a LIMIT, and update the
caller PaymentSyncScheduler to pass PageRequest.of(0, 50) (or appropriate page
size) when invoking the repository; ensure any tests cover large-PENDING
scenarios.
In
`@apps/commerce-api/src/main/java/com/loopers/infrastructure/payment/PaymentRepositoryAdapter.java`:
- Around line 39-42: The current findAllPendingBefore in
PaymentRepositoryAdapter loads all matching rows into memory; change it to
accept a batchSize parameter and delegate to a JPA query that applies a Pageable
LIMIT instead of fetching all rows—add or use a repository method like
paymentJpaRepository.findAllByStatusAndCreatedAtBefore(PaymentStatus status,
ZonedDateTime createdAt, Pageable pageable) and call it from
PaymentRepositoryAdapter.findAllPendingBefore(createdAt, batchSize) with
PageRequest.of(0, batchSize) (optionally include a deterministic Sort),
returning the Page content so only up to batchSize rows are loaded from the DB.
In `@apps/commerce-api/src/main/java/com/loopers/infrastructure/pg/PgClient.java`:
- Line 45: PgPaymentRequest.of(...) is currently called with an amount converted
via longValue which silently truncates decimals; update the conversion in
PgClient (where PgPaymentRequest.of is invoked) to avoid loss by either using
BigDecimal.longValueExact() or converting to the PG-required minor units
explicitly (e.g., amount.multiply(new BigDecimal(100)).longValueExact()) so
1000.50 becomes 100050 when PG expects cents, and make PgPaymentRequest.of
accept the correctly scaled long; also add unit tests that pass a BigDecimal
with scale=2 (e.g., 1000.50) to verify the payload preserves exactness or is
rejected on non-exact conversion, referencing Order.totalAmount and
Payment.amount as the source fields.
In
`@apps/commerce-api/src/main/java/com/loopers/infrastructure/pg/PgPaymentRequest.java`:
- Around line 12-13: Replace the lossy conversion in PgPaymentRequest.of
(currently using amount.longValue()) with amount.longValueExact() to force
lossless conversion; catch ArithmeticException from longValueExact() and rethrow
as an IllegalArgumentException (preserving the caught exception as the cause) so
callers can handle validation consistently; update any constructor/creator calls
(PgPaymentRequest(...) usage) accordingly. Additionally, add unit tests for
PgPaymentRequest.of (or the factory) that assert: 1) passing 1000.5 throws
IllegalArgumentException, 2) passing a BigDecimal > Long.MAX_VALUE throws
IllegalArgumentException, and 3) passing 1000 converts successfully to the
expected PgPaymentRequest instance.
In
`@apps/commerce-api/src/main/java/com/loopers/interfaces/api/payment/PaymentV1Controller.java`:
- Around line 42-47: The syncPayment endpoint in PaymentV1Controller (method
syncPayment) is exposed without authentication/authorization; restrict access to
admins by adding security annotations (e.g., `@PreAuthorize`("hasRole('ADMIN')")
or `@Secured`("ROLE_ADMIN")) or enforce an internal-only filter for the
"/api-admin/v1/payments/{orderId}/sync" route, add audit logging inside
syncPayment (record caller identity, orderId, timestamp, and result), and update
tests to assert non-admin calls return 401/403 and admin calls succeed and
produce an audit entry.
- Around line 33-38: The callback endpoint (PaymentV1Controller.callback)
currently trusts external payloads and calls paymentFacade.handleCallback
directly; change it to first verify the PG sender signature (e.g. HMAC/shared
secret or mTLS) using fields from PaymentV1Dto.CallbackRequest and reject
requests with invalid signatures, then perform an authoritative cross-check by
querying the payment gateway for transactionId status and confirm
orderId/transactionId mapping before calling paymentFacade.handleCallback; also
enforce idempotency/replay protection (reject or ignore duplicate transactionId
callbacks) and add API tests covering invalid signature, replayed callback, and
forged orderId/transactionId combinations.
In
`@apps/commerce-api/src/main/java/com/loopers/interfaces/api/payment/PaymentV1Dto.java`:
- Around line 13-26: Remove bean validation annotations from the CreateRequest
record (remove `@NotNull/`@NotBlank on orderId, cardType, cardNo) and ensure
validation is performed in the domain layer instead: when converting to a
command or when constructing/processing PaymentDto.PayCommand validate fields
and throw the domain CoreException with appropriate error codes/messages for
missing/invalid orderId, cardType, or cardNo; update the
CreateRequest.toCommand() flow to return a plain PayCommand (no annotations) and
rely on the domain-level validation in PaymentDto.PayCommand (or in the
service/validator invoked after toCommand()) to enforce rules and produce
consistent error responses.
- Around line 38-60: PaymentResponse.from currently returns the raw cardNo from
PaymentDto.PaymentInfo; update PaymentResponse.from (and any helper used) to
mask cardNo before constructing PaymentResponse (e.g., keep first 4 and last 4
digits and replace middle digits with asterisks or hyphen-separated blocks like
1234-****-****-1451). Locate the PaymentResponse record and its static
from(PaymentDto.PaymentInfo info) method and transform info.cardNo() into a
masked value (ensure null/short checks) so the PaymentResponse.cardNo field and
any logs using it never contain the full PAN.
- Around line 28-36: Add defensive validation and explicit parse error handling:
annotate CallbackRequest's fields (at least orderId) with `@NotBlank/`@NotNull
like CreateRequest so incoming PG callbacks are validated before mapping to
PaymentDto.CallbackCommand (CallbackRequest -> toCommand). In
PaymentFacade.handleCallback(), wrap the Long.parseLong(command.orderId()) call
in a try-catch for NumberFormatException and convert it to a domain error (e.g.,
throw an application-specific exception mapped to ErrorType.INVALID_ORDER_ID)
instead of allowing it to bubble to the global handler. Also add integration
tests covering non-numeric orderId and out-of-range values to assert the
INVALID_ORDER_ID error path.
In `@apps/commerce-api/src/main/resources/application.yml`:
- Around line 32-34: Remove hardcoded localhost defaults for pg.base-url and
pg.callback-url in application.yml and replace them with
environment-variable-based placeholders (e.g., ${PG_BASE_URL:} and
${PG_CALLBACK_URL:}) or leave them unset so profile-specific files provide real
endpoints; add a production profile file (application-prd.yml) that supplies the
real PG endpoints for deployment; and add a loading test annotated with
`@ActiveProfiles`("prd") that asserts pg.base-url and pg.callback-url are not
"http://localhost" (or empty) to fail CI if production values are still pointing
to localhost.
In
`@apps/commerce-api/src/test/java/com/loopers/application/payment/PaymentFacadeIntegrationTest.java`:
- Line 37: PaymentFacadeIntegrationTest boots the full context via
`@SpringBootTest` which allows scheduled tasks to run and causes flaky async state
changes; change the test annotation to disable scheduling by setting the Spring
property (for example update the `@SpringBootTest` on class
PaymentFacadeIntegrationTest to `@SpringBootTest`(properties =
"spring.scheduling.enabled=false") so schedulers are disabled for this
integration test), or alternatively add a test-only configuration that disables
scheduling; keep the change local to PaymentFacadeIntegrationTest to preserve
scheduler tests in a separate explicitly-enabled integration test class.
In
`@apps/commerce-api/src/test/java/com/loopers/application/payment/PaymentTxServiceTest.java`:
- Around line 72-83: The test PaymentTxServiceTest.refundPayment_failed_noChange
and its assertions conflict with the intended behavior: it currently expects
paymentRepository.save(payment) to be called for a FAILED Payment but the test
name says "no change"; update either the test or the implementation. Option A:
rename the test to reflect that a FAILED payment is kept and saved (e.g.,
refundPayment_failed_saved) and keep the current verification of
paymentRepository.save(payment). Option B: change the implementation in
PaymentTxService.refundPayment to skip calling paymentRepository.save(payment)
when payment.getStatus() == PaymentStatus.FAILED so the test can assert no save;
ensure you modify only the branch handling FAILED and update the test
accordingly (reference: PaymentTxService.refundPayment,
PaymentTxServiceTest.refundPayment_failed_noChange, and paymentRepository.save).
---
Nitpick comments:
In
`@apps/commerce-api/src/main/java/com/loopers/application/payment/PaymentTxService.java`:
- Around line 100-105: The current loop in PaymentTxService calls
productRepository.findById for each OrderItem, causing an N+1 query; instead
collect productIds from order.getOrderItems(), call
productRepository.findAllById(productIds) once, convert the returned Iterable to
a Map keyed by productId, then iterate order.getOrderItems() and call
product.increaseStock(item.getQuantity()) using the map lookup, and finally
persist changes with productRepository.saveAll(updatedProducts) (or save
per-product if needed); update references: order.getOrderItems(),
productRepository.findAllById(...), product.increaseStock(...), and
productRepository.saveAll(...).
- Around line 95-98: In cancelOrderWithRecovery, when order.cancel() returns
false add a log statement that records why the cancel failed; use the service
logger (e.g., PaymentTxService logger) to emit a WARN/ERROR including
order.identifiers (order.getId()), current state (order.getStatus() or
order.getState()/toString()) and any available failure reason
(order.getCancelFailureReason() or similar), falling back to the order's
toString() if no specific reason accessor exists; keep the current control flow
(still return) but ensure the log provides enough context to trace the failure.
In `@apps/commerce-api/src/main/java/com/loopers/CommerceApiApplication.java`:
- Line 14: Add a custom TaskScheduler bean to replace the single-thread default
used by `@EnableScheduling` so PaymentSyncScheduler runs on a pool and failures
are handled: create a SchedulerConfig class that defines a TaskScheduler bean
returning a ThreadPoolTaskScheduler with an increased pool size, a descriptive
threadNamePrefix, and an error handler via setErrorHandler(...) that logs
exceptions; ensure the bean is picked up by Spring (e.g., `@Configuration`) so
scheduled tasks use this scheduler.
In `@apps/commerce-api/src/main/java/com/loopers/domain/payment/Payment.java`:
- Around line 64-66: The assignTransactionId method currently overwrites
pgTransactionId regardless of payment state; restrict changes to only when the
payment is in PENDING (or whichever transient state you use) by checking the
payment status field inside Payment.assignTransactionId and rejecting/throwing
an IllegalStateException (or returning false) if status is not PENDING (allowing
idempotent set if pgTransactionId is already equal). If this behavior is
intentional, add a clarifying JavaDoc on Payment.assignTransactionId; also add
unit tests around state transitions (PENDING -> SUCCESS/REFUNDED) to verify
pgTransactionId cannot be changed after final states.
In
`@apps/commerce-api/src/main/java/com/loopers/infrastructure/payment/PaymentJpaRepository.java`:
- Around line 11-17: The repository method findAllByStatusAndCreatedAtBefore
uses both status and created_at so add a composite DB index for (status,
created_at) on the Payment entity (e.g., add an index named
idx_payment_status_created_at referencing columns status, created_at) to
optimize that query; update the Payment entity's `@Table/`@Index metadata
accordingly and then raise a change request to the DB migration pipeline to
create the physical index (do not rely solely on JPA schema generation).
In
`@apps/commerce-api/src/main/java/com/loopers/infrastructure/pg/PgClientConfig.java`:
- Around line 49-55: The current use of configureDefault on the
Resilience4JCircuitBreakerFactory spreads the PG-specific 6s TimeLimiter and
thresholds to all breakers; change the factory configuration so PG-specific
settings are applied only for breakers with IDs "pg-payment" and "pg-query"
(e.g. replace configureDefault(...) with factory.configure(id -> { if
(id.equals("pg-payment") || id.equals("pg-query")) return new
Resilience4JConfigBuilder(id).circuitBreakerConfig(cbConfig).timeLimiterConfig(tlConfig).build();
else return defaultPgSafeConfig; }) where defaultPgSafeConfig is a separate
default config without the PG time limiter/thresholds), and add an integration
test that creates a non-PG breaker from Resilience4JCircuitBreakerFactory and
asserts it does not inherit the PG TimeLimiter/tresholds to prevent
configuration bleed.
In
`@apps/commerce-api/src/main/java/com/loopers/infrastructure/pg/PgOrderResponse.java`:
- Around line 9-12: PgOrderResponse의 transactions 필드는 Jackson 역직렬화로 외부 리스트를 그대로
참조하므로 compact constructor를 추가해 null이면 빈 리스트로 치환하고 List.copyOf(transactions)로 방어적
복사를 하여 불변성을 보장하세요; 즉 PgOrderResponse의 compact constructor 내에서 this.transactions를
List.copyOf(Optional.ofNullable(transactions).orElseGet(List::of)) 형태로 설정하도록
수정하고 관련 테스트로 원본 리스트 변경이 레코드에 반영되지 않음과 null 입력 시 빈 리스트 처리 케이스를 추가하세요.
In
`@apps/commerce-api/src/test/java/com/loopers/application/payment/PaymentFacadeIntegrationTest.java`:
- Around line 230-235: The test PaymentFacadeIntegrationTest should not only
assert that PaymentFacade.pay throws a CoreException with
ErrorType.USER_NOT_FOUND but also verify that the external pgClient is never
invoked; update the test to, after the assertThatThrownBy block, add a mockito
assertion such as verifyNoInteractions(pgClient) or verify(pgClient,
never()).<methodName/operation>() (use the actual pgClient method used to call
the PG) to ensure no PG call occurs on user-not-found, and apply the same
pgClient non-invocation assertion pattern to other authentication-failure tests
(e.g., wrong password) to guarantee failure paths produce no external side
effects.
In
`@apps/commerce-api/src/test/java/com/loopers/application/payment/PaymentTxServiceTest.java`:
- Around line 25-43: Add unit tests in PaymentTxServiceTest to cover
PaymentTxService.createPayment and PaymentTxService.assignTransactionId: write a
test that verifies createPayment persists a Payment via mocked PaymentRepository
and returns expected result when OrderRepository and ProductRepository provide
valid order/product, and write two assignTransactionId tests—one where a Payment
exists (mock paymentRepository.findById to return a Payment) and you assert
paymentRepository.save is called with the transactionId set, and one where
paymentRepository.findById returns empty and you assert no save/exception
occurs. Use the existing mocks (paymentRepository, orderRepository,
productRepository, couponIssueRepository) and the paymentTxService instance to
locate where to add tests.
- Around line 129-146: The test
processPaymentResult_failed_orderCancelledWithRecovery only verifies stock
recovery but misses the coupon recovery path; add a new or extend the existing
test to set an order with a coupon (use createPendingOrderWithCoupon or set
couponId on the Order), mock couponIssueRepository.findByIdAndUserId to return a
mocked CouponIssue, call paymentTxService.processPaymentResult(1L, orderId,
"txId1", "FAILED"), and then verify couponIssue.revoke() and
couponIssueRepository.save(couponIssue) were invoked to assert the coupon
recovery flow.
In `@apps/commerce-api/src/test/java/com/loopers/domain/payment/PaymentTest.java`:
- Around line 44-52: Test currently only asserts that a second call to
Payment.markSuccess throws; add an assertion that the existing pgTransactionId
is preserved after the exception to ensure immutability. After calling
payment.markSuccess("txId1") and asserting that calling
payment.markSuccess("txId2") throws, verify payment.getPgTransactionId() (or the
accessor used in the test) still equals "txId1" and optionally that
payment.getStatus() remains SUCCESS; update the test method
markSuccess_fail_alreadyProcessed to include these assertions referencing
Payment.create and payment.markSuccess.
- Around line 25-31: Add a test that asserts Payment.create rejects negative
amounts: create a new test method (e.g., create_fail_negativeAmount) in
PaymentTest that calls Payment.create(1L, 2L, "SAMSUNG", "1234-5678",
BigDecimal.valueOf(-1000)) and verifies it throws IllegalArgumentException with
a message containing "결제 금액", mirroring the existing zero-amount test to ensure
negative-value validation in Payment.create is covered.
In
`@apps/commerce-api/src/test/java/com/loopers/interfaces/api/PaymentE2ETest.java`:
- Around line 187-193: Update the existing success E2E assertions that call
mockMvc.perform(post("/api/v1/payments/callback")...) and the admin sync test to
include the required valid signature header/body and valid admin credentials (so
the "SUCCESS" assertions reflect the intended authenticated flow), and add
separate tests that post the same endpoints with an invalid signature (expect
401/403 or specific error) and with non-admin credentials (expect 401/403) to
cover failure paths; also add a replay callback test that resubmits an
already-processed callback and asserts rejection; locate and change the tests
around the mockMvc.perform(...) calls in PaymentE2ETest.java to implement these
cases.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: dae1b0ba-98b9-45bb-a9c4-fd3149492969
📒 Files selected for processing (34)
.gitignoreapps/commerce-api/build.gradle.ktsapps/commerce-api/src/main/java/com/loopers/CommerceApiApplication.javaapps/commerce-api/src/main/java/com/loopers/application/order/OrderFacade.javaapps/commerce-api/src/main/java/com/loopers/application/payment/PaymentDto.javaapps/commerce-api/src/main/java/com/loopers/application/payment/PaymentFacade.javaapps/commerce-api/src/main/java/com/loopers/application/payment/PaymentSyncScheduler.javaapps/commerce-api/src/main/java/com/loopers/application/payment/PaymentTxService.javaapps/commerce-api/src/main/java/com/loopers/domain/payment/Payment.javaapps/commerce-api/src/main/java/com/loopers/domain/payment/PaymentRepository.javaapps/commerce-api/src/main/java/com/loopers/domain/payment/PaymentStatus.javaapps/commerce-api/src/main/java/com/loopers/infrastructure/payment/PaymentJpaRepository.javaapps/commerce-api/src/main/java/com/loopers/infrastructure/payment/PaymentRepositoryAdapter.javaapps/commerce-api/src/main/java/com/loopers/infrastructure/pg/PgApiResponse.javaapps/commerce-api/src/main/java/com/loopers/infrastructure/pg/PgClient.javaapps/commerce-api/src/main/java/com/loopers/infrastructure/pg/PgClientConfig.javaapps/commerce-api/src/main/java/com/loopers/infrastructure/pg/PgOrderResponse.javaapps/commerce-api/src/main/java/com/loopers/infrastructure/pg/PgPaymentInfo.javaapps/commerce-api/src/main/java/com/loopers/infrastructure/pg/PgPaymentRequest.javaapps/commerce-api/src/main/java/com/loopers/infrastructure/pg/PgPaymentResponse.javaapps/commerce-api/src/main/java/com/loopers/interfaces/api/payment/PaymentV1Controller.javaapps/commerce-api/src/main/java/com/loopers/interfaces/api/payment/PaymentV1Dto.javaapps/commerce-api/src/main/java/com/loopers/support/error/ErrorType.javaapps/commerce-api/src/main/resources/application.ymlapps/commerce-api/src/test/java/com/loopers/application/order/OrderFacadeRetryTest.javaapps/commerce-api/src/test/java/com/loopers/application/order/OrderFacadeTest.javaapps/commerce-api/src/test/java/com/loopers/application/payment/PaymentFacadeIntegrationTest.javaapps/commerce-api/src/test/java/com/loopers/application/payment/PaymentFacadeTest.javaapps/commerce-api/src/test/java/com/loopers/application/payment/PaymentSyncSchedulerTest.javaapps/commerce-api/src/test/java/com/loopers/application/payment/PaymentTxServiceTest.javaapps/commerce-api/src/test/java/com/loopers/domain/payment/PaymentTest.javaapps/commerce-api/src/test/java/com/loopers/interfaces/api/PaymentE2ETest.javamodules/jpa/src/main/resources/jpa.ymlsupports/monitoring/src/main/resources/monitoring.yml
Comment on lines
+18
to
+23
| public record CallbackCommand( | ||
| String transactionId, | ||
| String orderId, | ||
| String status | ||
| ) { | ||
| } |
There was a problem hiding this comment.
CallbackCommand.orderId가 String 타입이면 파싱 오류 위험이 있다.
PaymentFacade.handleCallback()에서 Long.parseLong(command.orderId())를 호출하는데, orderId가 null, 빈 문자열, 또는 숫자가 아닌 경우 NumberFormatException이 발생한다. 외부 PG 콜백 데이터를 신뢰하면 안 된다.
수정안: DTO 생성 시점 또는 Facade에서 유효성 검증 및 예외 처리를 추가한다.
🛡️ 수정안: PaymentFacade에서 방어 코드 추가
public PaymentDto.PaymentInfo handleCallback(PaymentDto.CallbackCommand command) {
Long orderId;
try {
orderId = Long.parseLong(command.orderId());
} catch (NumberFormatException e) {
throw new CoreException(ErrorType.BAD_REQUEST, "Invalid orderId format", e);
}
// ...
}추가 테스트: 잘못된 orderId 형식(null, "", "abc")으로 콜백 호출 시 적절한 에러 응답 테스트를 추가한다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/commerce-api/src/main/java/com/loopers/application/payment/PaymentDto.java`
around lines 18 - 23, CallbackCommand currently exposes orderId as a String
which can cause NumberFormatException in PaymentFacade.handleCallback when
calling Long.parseLong; add defensive validation and explicit error handling
either at DTO construction or immediately in PaymentFacade.handleCallback:
attempt to parse command.orderId() inside a try/catch, map NumberFormatException
(or null/empty checks) to a controlled CoreException with ErrorType.BAD_REQUEST
and a clear message, and proceed only with a valid Long orderId; also add
unit/integration tests that invoke the callback with null, empty, and
non-numeric orderId values to assert the BAD_REQUEST error is returned.
Comment on lines
+30
to
+31
| String cardType, | ||
| String cardNo, |
There was a problem hiding this comment.
PaymentInfo에 cardNo가 포함되어 API 응답에 카드번호가 노출된다.
응답 DTO에 카드번호를 그대로 포함하면 클라이언트 로그, 네트워크 트래픽 등에서 민감정보가 노출될 수 있다. 마스킹 처리가 필요하다.
수정안: PaymentInfo.from()에서 마스킹 적용 또는 별도 마스킹 유틸 사용.
🛡️ 수정안: 마스킹 적용
public static PaymentInfo from(Payment payment) {
return new PaymentInfo(
payment.getId(),
payment.getOrderId(),
payment.getUserId(),
payment.getPgTransactionId(),
payment.getCardType(),
- payment.getCardNo(),
+ maskCardNo(payment.getCardNo()),
payment.getAmount(),
payment.getStatus(),
payment.getCreatedAt()
);
}
+private static String maskCardNo(String cardNo) {
+ if (cardNo == null || cardNo.length() < 4) return "****";
+ return "****-****-****-" + cardNo.substring(cardNo.length() - 4);
+}As per coding guidelines: 로깅 시 민감정보 노출 가능성을 점검해야 하며, API 응답도 동일한 기준이 적용된다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/commerce-api/src/main/java/com/loopers/application/payment/PaymentDto.java`
around lines 30 - 31, PaymentDto currently exposes raw cardNo (and cardType) in
API responses; update PaymentInfo.from() to mask the card number before setting
it on the DTO (or call a shared masking util) so that only non-sensitive parts
(e.g., last 4 digits) remain while all other digits are replaced (e.g., ****
**** **** 1234); ensure the PaymentDto field that holds cardNo is populated with
the masked value and not the raw number, and add/ reuse a small helper like
maskCardNumber(String) for consistency across codepaths.
Comment on lines
+80
to
+83
| pgClient.getPaymentByOrderId(user.getId(), command.orderId()) | ||
| .ifPresent(info -> paymentTxService.processPaymentResult( | ||
| payment.getId(), command.orderId(), info.transactionKey(), info.status() | ||
| )); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "PaymentFacade.java" | head -5Repository: Loopers-dev-lab/loop-pack-be-l2-vol3-java
Length of output: 169
🏁 Script executed:
cat -n apps/commerce-api/src/main/java/com/loopers/application/payment/PaymentFacade.java | sed -n '70,150p'Repository: Loopers-dev-lab/loop-pack-be-l2-vol3-java
Length of output: 3876
🏁 Script executed:
# Find processPaymentResult method definition
rg -A 20 "def processPaymentResult|processPaymentResult\s*\(" apps/commerce-api/src/main/java/com/loopers/application/payment/ -t javaRepository: Loopers-dev-lab/loop-pack-be-l2-vol3-java
Length of output: 11576
🏁 Script executed:
# Search for payment status enum/mapper
fd -e java -e kt -x rg -l "PaymentStatus|PAYMENT_STATUS|status.*enum" {} \; apps/commerce-api/src/main/java/com/loopers/application/payment/Repository: Loopers-dev-lab/loop-pack-be-l2-vol3-java
Length of output: 646
🏁 Script executed:
cat -n apps/commerce-api/src/main/java/com/loopers/application/payment/PaymentTxService.java | sed -n '1,100p'Repository: Loopers-dev-lab/loop-pack-be-l2-vol3-java
Length of output: 4392
🏁 Script executed:
# Get full processPaymentResult method
rg -A 40 "public void processPaymentResult" apps/commerce-api/src/main/java/com/loopers/application/payment/PaymentTxService.javaRepository: Loopers-dev-lab/loop-pack-be-l2-vol3-java
Length of output: 1636
🏁 Script executed:
# Check PaymentSyncScheduler full context
cat -n apps/commerce-api/src/main/java/com/loopers/application/payment/PaymentSyncScheduler.java | head -80Repository: Loopers-dev-lab/loop-pack-be-l2-vol3-java
Length of output: 3304
🏁 Script executed:
# Search for payment status enum definition
fd -type f -name "*.java" | xargs rg "enum PaymentStatus|class PaymentStatus" -A 10Repository: Loopers-dev-lab/loop-pack-be-l2-vol3-java
Length of output: 899
🏁 Script executed:
# Check PgClient implementation
find . -type f -name "*.java" | xargs rg "class PgClient|interface PgClient" -A 15Repository: Loopers-dev-lab/loop-pack-be-l2-vol3-java
Length of output: 3749
🏁 Script executed:
# Check PgPaymentInfo structure
rg "class PgPaymentInfo|record PgPaymentInfo|class.*PaymentInfo" -A 10 --type java | head -50Repository: Loopers-dev-lab/loop-pack-be-l2-vol3-java
Length of output: 806
🏁 Script executed:
# Search for status mapping/normalization
rg "toUpperCase|toLowerCase|trim|PgStatus|PG_STATUS|statusMapping|statusMapper" --type java | head -30Repository: Loopers-dev-lab/loop-pack-be-l2-vol3-java
Length of output: 1243
🏁 Script executed:
# Check if there are tests for processPaymentResult with non-SUCCESS status
rg "processPaymentResult.*PENDING|processPaymentResult.*PROCESSING|FAILED.*processPaymentResult" --type javaRepository: Loopers-dev-lab/loop-pack-be-l2-vol3-java
Length of output: 245
🏁 Script executed:
# Check all test cases for PaymentTxService
cat -n apps/commerce-api/src/test/java/com/loopers/application/payment/PaymentTxServiceTest.java | head -150Repository: Loopers-dev-lab/loop-pack-be-l2-vol3-java
Length of output: 7294
🏁 Script executed:
# Search for test cases with FAILED or other status values
rg "FAILED|PENDING|PROCESSING" apps/commerce-api/src/test/java/com/loopers/application/payment/PaymentTxServiceTest.java -B 2 -A 5Repository: Loopers-dev-lab/loop-pack-be-l2-vol3-java
Length of output: 2555
PG 상태 문자열을 검증/매핑 없이 그대로 처리하면 안 된다
PaymentFacade 80-83, 109-110, 137-140과 PaymentSyncScheduler의 4개 call site에서 PG의 원본 상태 문자열을 검증 없이 processPaymentResult()에 전달한다. processPaymentResult()는 "SUCCESS" 정확 문자열만 성공으로 보고 나머지는 모두 실패로 처리하므로, PG가 PENDING, PROCESSING, 신규 상태값, 대소문자 변형을 보내는 순간 주문 취소와 재고/쿠폰 복구가 조기에 발생한다.
PG 스펙 문서를 기준으로 PgPaymentInfo.status를 enum으로 정의하고, PG 응답값 → 내부 상태(SUCCESS/FAILED/PENDING)로 변환하는 매퍼를 만들어 4개 call site에서 재사용해야 한다. 대소문자/공백이 섞인 상태값도 정규화하여 처리하고, 미확정 상태(PENDING, PROCESSING, 알 수 없는 값)는 조회 결과 반영 또는 PENDING 유지로 남겨 안전성을 확보한다. 추가로 PENDING, PROCESSING, 알 수 없는 상태값, 대소문자 및 공백 변형에 대한 회귀 테스트를 필수로 추가한다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/commerce-api/src/main/java/com/loopers/application/payment/PaymentFacade.java`
around lines 80 - 83, PaymentFacade 및 PaymentSyncScheduler에서 PG가 반환한 원본 상태 문자열을
그대로 processPaymentResult()에 전달하는 대신, PgPaymentInfo.status를 enum으로 정의하고 PG 응답값을
정규화(trim + uppercase)한 뒤 PG→내부 상태(SUCCESS/FAILED/PENDING)로 매핑하는 전용 매퍼를 만들어 4개
call site(PaymentFacade의 pgClient.getPaymentByOrderId(...) 호출들 및
PaymentSyncScheduler 호출들)를 모두 이 매퍼를 통해 전달하도록 수정하세요; 매핑 규칙은 "SUCCESS"만 성공으로,
"FAILED","FAIL","ERROR" 계열은 실패로, "PENDING","PROCESSING" 및 알 수 없는 값은
PENDING(미확정)으로 처리해 주문 취소/재고 복구가 조기에 발생하지 않게 하고, 대소문자/공백 변형에 대한 단위/회귀 테스트를 추가해
보장하세요.
Comment on lines
+100
to
+102
| Payment payment = paymentRepository.findByPgTransactionId(command.transactionId()) | ||
| .or(() -> paymentRepository.findByOrderId(orderId)) | ||
| .orElseThrow(() -> new CoreException(ErrorType.PAYMENT_NOT_FOUND)); |
There was a problem hiding this comment.
orderId fallback이 지연 콜백을 다른 결제에 잘못 붙일 수 있다
Line 100-102에서 transactionId 조회가 실패하면 최신 결제를 돌려주는 findByOrderId(orderId)로 내려간다. 재시도 결제가 생긴 뒤 예전 트랜잭션의 지연 콜백이 도착하면, 이전 시도의 결과가 최신 결제에 반영되어 정합성이 깨질 수 있다. fallback은 pgTransactionId가 아직 비어 있는 단일 PENDING 결제인 경우로 엄격히 제한하거나, 모호하면 거부하고 별도 동기화 경로로 복구하는 편이 안전하다. 추가로 결제 A 실패 후 결제 B가 생성된 상태에서 A의 지연 콜백이 와도 B가 바뀌지 않는 회귀 테스트를 넣는 것이 좋다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/commerce-api/src/main/java/com/loopers/application/payment/PaymentFacade.java`
around lines 100 - 102, The current fallback in PaymentFacade that uses
paymentRepository.findByOrderId(orderId) when
findByPgTransactionId(command.transactionId()) is empty can attach a delayed
callback to the wrong (newer) payment; change the lookup logic so that after
failing findByPgTransactionId you only accept a fallback payment if it is a
single payment for the order with status PENDING and pgTransactionId == null (or
similarly unassigned), otherwise throw
CoreException(ErrorType.PAYMENT_NOT_FOUND); update the code paths around Payment
payment assignment and paymentRepository queries (findByPgTransactionId,
findByOrderId) to enforce this guard and add/adjust a regression test ensuring a
delayed callback from an old transaction does not mutate a newer payment.
Comment on lines
+47
to
+57
| List<Payment> pendingPayments = paymentRepository.findAllPendingBefore(threshold); | ||
|
|
||
| if (pendingPayments.isEmpty()) { | ||
| return; | ||
| } | ||
|
|
||
| log.info("[PaymentSyncScheduler] PENDING 복구 대상: {}건 ({}분 초과)", pendingPayments.size(), pendingThresholdMinutes); | ||
|
|
||
| List<Payment> targets = pendingPayments.size() > batchSize | ||
| ? pendingPayments.subList(0, batchSize) | ||
| : pendingPayments; |
There was a problem hiding this comment.
배치 크기 제한이 DB 조회에는 적용되지 않는다
Line 47에서 전체 PENDING backlog를 읽고 Line 55-57에서 메모리에서만 batchSize로 자른다. PG 장애로 미복구 건이 급증하면 매 실행마다 전체 목록을 메모리에 올리게 되어 DB/heap 압박이 커지고, 정작 처리하는 것은 일부 건뿐이라 복구 속도도 떨어진다. findAllPendingBefore()에 정렬과 LIMIT batchSize를 내려서 처음부터 필요한 수만 조회하도록 바꾸는 편이 안전하다. 추가로 backlog가 큰 상황에서 repository가 정확히 batchSize건만 읽는 adapter/integration 테스트를 넣는 것이 좋다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/commerce-api/src/main/java/com/loopers/application/payment/PaymentSyncScheduler.java`
around lines 47 - 57, PaymentSyncScheduler currently calls
paymentRepository.findAllPendingBefore(threshold) which returns the full PENDING
backlog into pendingPayments and only then applies batchSize in-memory; change
the repository/query behind findAllPendingBefore(...) to apply the desired
ordering and a SQL LIMIT (batchSize) so the DB returns at most batchSize sorted
records and update PaymentSyncScheduler to use that limited result directly
(remove or rely on the existing subList logic). Reference findAllPendingBefore,
pendingPayments, targets, and batchSize when making the change; also add an
adapter/integration test asserting that the repository/query layer returns
exactly batchSize rows when backlog is large to prevent regressions.
Comment on lines
+28
to
+36
| public record CallbackRequest( | ||
| String transactionKey, | ||
| String orderId, | ||
| String status | ||
| ) { | ||
| public PaymentDto.CallbackCommand toCommand() { | ||
| return new PaymentDto.CallbackCommand(transactionKey, orderId, status); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: CallbackCommand의 orderId 타입 및 사용처 확인
ast-grep --pattern $'record CallbackCommand($$$)'
rg -n -C3 'CallbackCommand' --type=javaRepository: Loopers-dev-lab/loop-pack-be-l2-vol3-java
Length of output: 8592
🏁 Script executed:
# Check PaymentFacade.handleCallback implementation for exception handling
rg -n -A 10 'public PaymentDto.PaymentInfo handleCallback' apps/commerce-api/src/main/java/com/loopers/application/payment/PaymentFacade.java
# Check if there's any validation on CallbackRequest
rg -n 'CallbackRequest' apps/commerce-api/src/main/java -A 5Repository: Loopers-dev-lab/loop-pack-be-l2-vol3-java
Length of output: 2176
🏁 Script executed:
# Check for validators on CallbackRequest
cat -n apps/commerce-api/src/main/java/com/loopers/interfaces/api/payment/PaymentV1Dto.java
# Check for global exception handlers
rg -n '@ControllerAdvice|@ExceptionHandler' apps/commerce-api/src/main/java --type=java -A 3
# Check if there are any validation annotations on orderId field
rg -n '@Valid|@NotNull|@NotBlank' apps/commerce-api/src/main/java/com/loopers/interfaces/api/payment/ --type=javaRepository: Loopers-dev-lab/loop-pack-be-l2-vol3-java
Length of output: 8008
CallbackRequest 입력 검증 부재 및 orderId 파싱 오류 처리 필요
CallbackRequest의 모든 필드가 검증 없이 String 타입으로 수신된다. 반면 CreateRequest는 @NotNull, @NotBlank 검증을 적용하고 있어 일관성이 떨어진다. 외부 PG 콜백 데이터이므로 더욱 방어적인 검증이 필요하다.
더 심각한 문제는 PaymentFacade.handleCallback() 98번 줄의 Long.parseLong(command.orderId())에서 NumberFormatException이 발생할 수 있지만 명시적인 예외 처리가 없다는 점이다. 현재 전역 예외 핸들러가 이를 감싸서 INTERNAL_ERROR로 반환하므로 클라이언트에게 명확한 유효성 검증 오류 메시지를 전달할 수 없다.
수정안:
CallbackRequest에@NotBlank검증 추가PaymentFacade.handleCallback()에서Long.parseLong()호출 시try-catch로NumberFormatException을 명시적으로 처리하여ErrorType.INVALID_ORDER_ID같은 도메인 에러로 변환- 파싱 실패 케이스에 대한 통합 테스트 추가 필요 (예: 숫자가 아닌 orderId, 초과된 Long 범위 값)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/commerce-api/src/main/java/com/loopers/interfaces/api/payment/PaymentV1Dto.java`
around lines 28 - 36, Add defensive validation and explicit parse error
handling: annotate CallbackRequest's fields (at least orderId) with
`@NotBlank/`@NotNull like CreateRequest so incoming PG callbacks are validated
before mapping to PaymentDto.CallbackCommand (CallbackRequest -> toCommand). In
PaymentFacade.handleCallback(), wrap the Long.parseLong(command.orderId()) call
in a try-catch for NumberFormatException and convert it to a domain error (e.g.,
throw an application-specific exception mapped to ErrorType.INVALID_ORDER_ID)
instead of allowing it to bubble to the global handler. Also add integration
tests covering non-numeric orderId and out-of-range values to assert the
INVALID_ORDER_ID error path.
Comment on lines
+38
to
+60
| public record PaymentResponse( | ||
| Long id, | ||
| Long orderId, | ||
| String pgTransactionId, | ||
| String cardType, | ||
| String cardNo, | ||
| BigDecimal amount, | ||
| PaymentStatus status, | ||
| ZonedDateTime createdAt | ||
| ) { | ||
| public static PaymentResponse from(PaymentDto.PaymentInfo info) { | ||
| return new PaymentResponse( | ||
| info.id(), | ||
| info.orderId(), | ||
| info.pgTransactionId(), | ||
| info.cardType(), | ||
| info.cardNo(), | ||
| info.amount(), | ||
| info.status(), | ||
| info.createdAt() | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
cardNo 필드 마스킹 처리 필요
PaymentResponse에서 카드번호(cardNo)를 그대로 반환하고 있다. 카드번호는 민감 정보(PII)이므로 API 응답 시 마스킹 처리가 필요하다 (예: 1234-****-****-1451). 로그 출력 시에도 마스킹되어야 하며, PCI-DSS 컴플라이언스 위반 가능성이 있다. As per coding guidelines: "로깅 시 민감정보 노출 가능성을 점검한다."
마스킹 처리 제안
public record PaymentResponse(
Long id,
Long orderId,
String pgTransactionId,
String cardType,
- String cardNo,
+ String maskedCardNo,
BigDecimal amount,
PaymentStatus status,
ZonedDateTime createdAt
) {
public static PaymentResponse from(PaymentDto.PaymentInfo info) {
return new PaymentResponse(
info.id(),
info.orderId(),
info.pgTransactionId(),
info.cardType(),
- info.cardNo(),
+ maskCardNo(info.cardNo()),
info.amount(),
info.status(),
info.createdAt()
);
}
+
+ private static String maskCardNo(String cardNo) {
+ if (cardNo == null || cardNo.length() < 8) return "****";
+ return cardNo.substring(0, 4) + "-****-****-" + cardNo.substring(cardNo.length() - 4);
+ }
}📝 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 record PaymentResponse( | |
| Long id, | |
| Long orderId, | |
| String pgTransactionId, | |
| String cardType, | |
| String cardNo, | |
| BigDecimal amount, | |
| PaymentStatus status, | |
| ZonedDateTime createdAt | |
| ) { | |
| public static PaymentResponse from(PaymentDto.PaymentInfo info) { | |
| return new PaymentResponse( | |
| info.id(), | |
| info.orderId(), | |
| info.pgTransactionId(), | |
| info.cardType(), | |
| info.cardNo(), | |
| info.amount(), | |
| info.status(), | |
| info.createdAt() | |
| ); | |
| } | |
| } | |
| public record PaymentResponse( | |
| Long id, | |
| Long orderId, | |
| String pgTransactionId, | |
| String cardType, | |
| String maskedCardNo, | |
| BigDecimal amount, | |
| PaymentStatus status, | |
| ZonedDateTime createdAt | |
| ) { | |
| public static PaymentResponse from(PaymentDto.PaymentInfo info) { | |
| return new PaymentResponse( | |
| info.id(), | |
| info.orderId(), | |
| info.pgTransactionId(), | |
| info.cardType(), | |
| maskCardNo(info.cardNo()), | |
| info.amount(), | |
| info.status(), | |
| info.createdAt() | |
| ); | |
| } | |
| private static String maskCardNo(String cardNo) { | |
| if (cardNo == null || cardNo.length() < 8) return "****"; | |
| return cardNo.substring(0, 4) + "-****-****-" + cardNo.substring(cardNo.length() - 4); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/commerce-api/src/main/java/com/loopers/interfaces/api/payment/PaymentV1Dto.java`
around lines 38 - 60, PaymentResponse.from currently returns the raw cardNo from
PaymentDto.PaymentInfo; update PaymentResponse.from (and any helper used) to
mask cardNo before constructing PaymentResponse (e.g., keep first 4 and last 4
digits and replace middle digits with asterisks or hyphen-separated blocks like
1234-****-****-1451). Locate the PaymentResponse record and its static
from(PaymentDto.PaymentInfo info) method and transform info.cardNo() into a
masked value (ensure null/short checks) so the PaymentResponse.cardNo field and
any logs using it never contain the full PAN.
Comment on lines
+32
to
+34
| pg: | ||
| base-url: http://localhost:8082 | ||
| callback-url: http://localhost:8080/api/v1/payments/callback |
There was a problem hiding this comment.
PG URL을 공통 문서에 localhost로 고정하면 운영 결제 장애가 발생한다
Line 33-34 값이 prd까지 그대로 적용되면 PG 호출과 콜백이 로컬 주소로 향해 결제 연동이 실패한다. 공통 문서에는 환경변수 기반 기본값만 두고, 실제 엔드포인트는 프로파일/배포 변수에서 주입해야 한다.
추가 테스트로 @ActiveProfiles("prd") 설정 로딩 테스트를 두고 pg.base-url, pg.callback-url이 localhost가 아닌 값을 강제 검증해야 한다.
🛠️ 제안 수정안
pg:
- base-url: http://localhost:8082
- callback-url: http://localhost:8080/api/v1/payments/callback
+ base-url: ${PG_BASE_URL:http://localhost:8082}
+ callback-url: ${PG_CALLBACK_URL:http://localhost:8080/api/v1/payments/callback}As per coding guidelines, "**/application*.yml: 환경별 분리(프로파일)와 기본값 적절성을 점검하고 ... 운영에 영향을 주는 설정 변경은 근거와 영향 범위를 요구한다."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/commerce-api/src/main/resources/application.yml` around lines 32 - 34,
Remove hardcoded localhost defaults for pg.base-url and pg.callback-url in
application.yml and replace them with environment-variable-based placeholders
(e.g., ${PG_BASE_URL:} and ${PG_CALLBACK_URL:}) or leave them unset so
profile-specific files provide real endpoints; add a production profile file
(application-prd.yml) that supplies the real PG endpoints for deployment; and
add a loading test annotated with `@ActiveProfiles`("prd") that asserts
pg.base-url and pg.callback-url are not "http://localhost" (or empty) to fail CI
if production values are still pointing to localhost.
| import static org.mockito.ArgumentMatchers.*; | ||
| import static org.mockito.Mockito.when; | ||
|
|
||
| @SpringBootTest |
There was a problem hiding this comment.
통합 테스트에서 스케줄러를 비활성화해 플래키를 제거해야 한다
운영 관점에서 @SpringBootTest로 스케줄러까지 함께 기동되면 결제 상태가 비동기로 변해 간헐 실패가 발생할 수 있다. 수정안으로 테스트 컨텍스트에서 스케줄링을 꺼서 격리를 보장하는 것이 안전하다. 추가 테스트는 스케줄러 동작을 별도 통합 테스트 클래스로 분리해 명시적으로 활성화된 환경에서만 검증하는 방식이 적절하다.
수정 예시
-@SpringBootTest
+@SpringBootTest(properties = {
+ "spring.task.scheduling.enabled=false"
+})
class PaymentFacadeIntegrationTest {As per coding guidelines **/*Test*.java: "통합 테스트는 격리 수준, 플래키 가능성, 테스트 데이터 준비/정리 전략을 점검한다."
📝 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
| @SpringBootTest | |
| `@SpringBootTest`(properties = { | |
| "spring.task.scheduling.enabled=false" | |
| }) | |
| class PaymentFacadeIntegrationTest { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/commerce-api/src/test/java/com/loopers/application/payment/PaymentFacadeIntegrationTest.java`
at line 37, PaymentFacadeIntegrationTest boots the full context via
`@SpringBootTest` which allows scheduled tasks to run and causes flaky async state
changes; change the test annotation to disable scheduling by setting the Spring
property (for example update the `@SpringBootTest` on class
PaymentFacadeIntegrationTest to `@SpringBootTest`(properties =
"spring.scheduling.enabled=false") so schedulers are disabled for this
integration test), or alternatively add a test-only configuration that disables
scheduling; keep the change local to PaymentFacadeIntegrationTest to preserve
scheduler tests in a separate explicitly-enabled integration test class.
Comment on lines
+72
to
+83
| @DisplayName("refundPayment: FAILED 결제가 있으면 아무것도 하지 않는다") | ||
| @Test | ||
| void refundPayment_failed_noChange() { | ||
| Payment payment = Payment.create(orderId, userId, "SAMSUNG", "1234-5678", BigDecimal.valueOf(5000)); | ||
| payment.markFailed(null); | ||
| when(paymentRepository.findByOrderId(orderId)).thenReturn(Optional.of(payment)); | ||
|
|
||
| paymentTxService.refundPayment(orderId); | ||
|
|
||
| assertThat(payment.getStatus()).isEqualTo(PaymentStatus.FAILED); | ||
| verify(paymentRepository).save(payment); | ||
| } |
There was a problem hiding this comment.
테스트명과 실제 동작 불일치: refundPayment_failed_noChange
테스트 설명이 "아무것도 하지 않는다"이지만 실제로는 paymentRepository.save(payment)가 호출된다. 이는 테스트명과 검증 내용이 불일치하여 코드 이해를 어렵게 한다. 테스트명을 "FAILED 상태 유지 후 저장한다" 등으로 변경하거나, 실제 구현이 불필요한 save를 하고 있다면 구현을 수정해야 한다.
테스트명 수정 제안
- `@DisplayName`("refundPayment: FAILED 결제가 있으면 아무것도 하지 않는다")
+ `@DisplayName`("refundPayment: FAILED 결제는 상태 유지 후 저장한다")
`@Test`
- void refundPayment_failed_noChange() {
+ void refundPayment_failed_statusUnchanged() {또는 구현에서 불필요한 save를 제거:
// PaymentTxService.refundPayment()
if (payment.getStatus() == PaymentStatus.SUCCESS) {
payment.markRefunded();
paymentRepository.save(payment);
} else if (payment.getStatus() == PaymentStatus.PENDING) {
payment.markFailed(null);
paymentRepository.save(payment);
}
// FAILED인 경우 save 호출하지 않음📝 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
| @DisplayName("refundPayment: FAILED 결제가 있으면 아무것도 하지 않는다") | |
| @Test | |
| void refundPayment_failed_noChange() { | |
| Payment payment = Payment.create(orderId, userId, "SAMSUNG", "1234-5678", BigDecimal.valueOf(5000)); | |
| payment.markFailed(null); | |
| when(paymentRepository.findByOrderId(orderId)).thenReturn(Optional.of(payment)); | |
| paymentTxService.refundPayment(orderId); | |
| assertThat(payment.getStatus()).isEqualTo(PaymentStatus.FAILED); | |
| verify(paymentRepository).save(payment); | |
| } | |
| `@DisplayName`("refundPayment: FAILED 결제는 상태 유지 후 저장한다") | |
| `@Test` | |
| void refundPayment_failed_statusUnchanged() { | |
| Payment payment = Payment.create(orderId, userId, "SAMSUNG", "1234-5678", BigDecimal.valueOf(5000)); | |
| payment.markFailed(null); | |
| when(paymentRepository.findByOrderId(orderId)).thenReturn(Optional.of(payment)); | |
| paymentTxService.refundPayment(orderId); | |
| assertThat(payment.getStatus()).isEqualTo(PaymentStatus.FAILED); | |
| verify(paymentRepository).save(payment); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/commerce-api/src/test/java/com/loopers/application/payment/PaymentTxServiceTest.java`
around lines 72 - 83, The test
PaymentTxServiceTest.refundPayment_failed_noChange and its assertions conflict
with the intended behavior: it currently expects paymentRepository.save(payment)
to be called for a FAILED Payment but the test name says "no change"; update
either the test or the implementation. Option A: rename the test to reflect that
a FAILED payment is kept and saved (e.g., refundPayment_failed_saved) and keep
the current verification of paymentRepository.save(payment). Option B: change
the implementation in PaymentTxService.refundPayment to skip calling
paymentRepository.save(payment) when payment.getStatus() == PaymentStatus.FAILED
so the test can assert no save; ensure you modify only the branch handling
FAILED and update the test accordingly (reference:
PaymentTxService.refundPayment,
PaymentTxServiceTest.refundPayment_failed_noChange, and paymentRepository.save).
Contributor
|
@coderabbitai 아래 체크리스트에 맞게 구현되어 있는지 확인해 ⚡ PG 연동 대응
🛡 Resilience 설계
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 26818
⚡ PG 연동 대응✅ RestTemplate으로 외부 시스템 호출
✅ 타임아웃 설정 및 실패 예외 처리
서킷브레이커 fallback에서 경고 로그 출력 후
✅ 결제 실패 응답에 대한 시스템 연동
✅ 콜백 방식 + 결제 상태 확인 API 연동
🛡 Resilience 설계✅ 서킷 브레이커 + 재시도 정책
✅ 외부 장애 시 내부 시스템 정상 응답 보호서킷 OPEN 또는 타임아웃 시 ✅ 콜백 미수신 시 상태 복구
스케줄러는 건별 예외를 격리(
✅ 타임아웃 실패 후 결제 정보 확인 및 시스템 반영
종합 결과
체크리스트 8개 항목 모두 구현되어 있다. 다만 아래 3가지 사항은 운영/보안 관점에서 추가 조치가 필요하다.
|
This was referenced Mar 24, 2026
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
2 participants
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.
📌 Summary
🧭 Context & Decision
문제 정의
선택지와 결정
1. PG 호출 위치 — 트랜잭션 내부 vs 외부
PaymentTxService분리)2. 결제 요청 재시도 — Retry 적용 vs 미적용
3. PENDING 복구 방식 — 수동 API만 vs 자동 스케줄러 추가
🏗️ Design Overview
변경 범위
commerce-api(payment, order, infrastructure/pg),supports/monitoringdomain/payment— Payment 엔티티, PaymentStatus(PENDING/SUCCESS/FAILED/REFUNDED), PaymentRepositoryapplication/payment— PaymentFacade, PaymentTxService, PaymentSyncScheduler, PaymentDtoinfrastructure/pg— PgClient, PgClientConfig(CircuitBreaker+TimeLimiter+Retry), PG 요청/응답 DTOinfrastructure/payment— PaymentJpaRepository, PaymentRepositoryAdapterinterfaces/api/payment— PaymentV1Controller, PaymentV1Dto주요 컴포넌트 책임
PaymentFacade: 결제 흐름 오케스트레이션 (인증 → 주문 검증 → 중복 확인 → 결제 생성 → PG 호출 → 복구 시도)PaymentTxService: 트랜잭션 단위 결제 상태 변경 (생성, 성공/실패 반영, 환불, 주문 취소 연동)PgClient: PG 시뮬레이터 HTTP 통신 (서킷브레이커 + 조회 Retry + fallback)PgClientConfig: 서킷브레이커(실패율+Slow Call), TimeLimiter, RetryTemplate, 상태 전이 로깅PaymentSyncScheduler: 10분 이상 PENDING 건 자동 복구 (5분 주기, 배치 50건, 개별 실패 격리)PaymentV1Controller: 결제 요청 / 콜백 수신 / 수동 sync API 엔드포인트🔁 Flow Diagram
Main Flow — 결제 요청 ~ 콜백 처리
sequenceDiagram autonumber participant Client participant API as PaymentV1Controller participant Facade as PaymentFacade participant TxService as PaymentTxService participant DB participant PG as PgClient (CB) participant PGServer as PG Simulator Client->>API: POST /api/v1/payments API->>Facade: pay(loginId, password, command) Facade->>Facade: authenticate(loginId, password) Facade->>DB: findByIdAndUserId(orderId, userId) Facade->>DB: findByOrderId(orderId) — 중복 확인 Facade->>TxService: createPayment() [TX1] TxService->>DB: save(Payment PENDING) TxService-->>Facade: Payment Facade->>PG: requestPayment() [TX 외부] PG->>PGServer: POST /api/v1/payments alt PG 요청 성공 PGServer-->>PG: transactionKey PG-->>Facade: Optional(transactionKey) Facade->>TxService: assignTransactionId() [TX2] else PG 요청 실패 (타임아웃/서킷 OPEN) PG-->>Facade: Optional.empty() Facade->>PG: getPaymentByOrderId() [복구 시도, Retry] alt 복구 성공 PG-->>Facade: PgPaymentInfo Facade->>TxService: processPaymentResult() [TX3] else 복구 실패 Facade-->>Client: Payment(PENDING) end end Note over PGServer: 1~5초 비동기 처리 후 콜백 PGServer->>API: POST /api/v1/payments/callback API->>Facade: handleCallback(command) Facade->>TxService: processPaymentResult() [TX] alt SUCCESS TxService->>DB: Payment SUCCESS, Order CONFIRMED else FAILED TxService->>DB: Payment FAILED, Order CANCELLED TxService->>DB: 재고 복구, 쿠폰 복구 endException Flow — 주문 취소 시 결제 처리
sequenceDiagram autonumber participant Client participant OrderFacade participant TxService as PaymentTxService participant DB Client->>OrderFacade: cancelOrder(userId, orderId) OrderFacade->>DB: order.cancel() OrderFacade->>DB: product.increaseStock() — 재고 복구 OrderFacade->>DB: couponIssue.revoke() — 쿠폰 복구 OrderFacade->>TxService: refundPayment(orderId) alt Payment SUCCESS TxService->>DB: Payment → REFUNDED else Payment PENDING TxService->>DB: Payment → FAILED (이후 콜백 무시됨) else Payment 없음 / FAILED TxService-->>OrderFacade: no-op endException Flow — PENDING 자동 복구 스케줄러
sequenceDiagram autonumber participant Scheduler as PaymentSyncScheduler participant DB participant PG as PgClient (CB + Retry) participant TxService as PaymentTxService loop 5분마다 Scheduler->>DB: findAllPendingBefore(now - 10min) alt PENDING 건 존재 (최대 50건) loop 건별 처리 Scheduler->>PG: getPaymentByOrderId() alt PG 조회 성공 PG-->>Scheduler: PgPaymentInfo Scheduler->>TxService: processPaymentResult() else PG 조회 실패 Scheduler-->>Scheduler: PENDING 유지 (개별 실패 격리) end end end end변경 목적
외부 PG 결제 연동 및 장애복구 기능 구현. 결제 요청 시 타임아웃·서킷브레이커·재시도를 통한 복원력 강화, PG 콜백 미수신 대비 자동복구 스케줄러(5분 주기) 및 수동 동기화 API 추가.
핵심 변경점
pg-payment,pg-query적용, GET 조회만 Retry), PgClientConfig(Resilience4j 설정: failureRate 50%, open timeout 30s, TimeLimiter 6s)./api/v1/payments(결제), POST/api/v1/payments/callback(콜백), POST/api-admin/v1/payments/{orderId}/sync(수동동기화).리스크/주의사항
테스트/검증 방법